fix(OPENFRAM-001-2): CU-86akhf8u5 4 review findings across 2 files

[flamingo]

Closes 4 review findings across 2 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🔴 55 low — review closely	Hardcoded href strings bypass the centralized routes registry	src/app/(app)/knowledge-base/components/knowledge-base-body.tsx:82
2	🔴 15 low — review closely	Relay connection IDs computed via ad-hoc local helper calls rather than a documented central key module	src/app/(app)/knowledge-base/components/knowledge-base-body.tsx:286
3	🔴 35 low — review closely	Hard-coded route path built with string interpolation in getLogDetailsUrl	src/app/(app)/logs-page/components/logs-table.tsx:363
4	🟢 90 high	Log severity 'INFO' is mapped to grey Tag variant while 'error' status default falls through to success	src/app/(app)/logs-page/components/logs-table.tsx:335

What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.

---

Run: https://product-hub.flamingo.so/admin/code-review
Run id: c69bf2d8-6eaa-4e2d-815b-af08b880c8df

Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.

ClickUp task: CU-86akhf8u5 OpenFrame OSS frontend review findings sweep (12 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

4 finding(s) fixed in this draft — 4 explained inline on the diff; 3 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🔴 Hardcoded href strings bypass the centralized routes registry

In buildActions (knowledge-base-body.tsx), replaced the raw string concatenation `/knowledge-base/new?folderId=${parentId}` / '/knowledge-base/new' with routes.knowledgeBase.new(parentId) / routes.knowledgeBase.new(), per the finding's suggested fix. This assumes src/lib/routes.ts exports a knowledgeBase.new(folderId?: string) function; that file was not provided, so I could not verify the exact signature exists. If routes.knowledgeBase.new does not exist or has a different signature, this will fail to compile — the reviewer must confirm/add that export in src/lib/routes.ts.

🤖 Prompt for AI agents

In src/app/(app)/knowledge-base/components/knowledge-base-body.tsx around line 82, review and complete this code-review fix: Hardcoded href strings bypass the centralized routes registry.
What the draft fix changed: In `buildActions` (knowledge-base-body.tsx), replaced the raw string concatenation `` `/knowledge-base/new?folderId=${parentId}` `` / `'/knowledge-base/new'` with `routes.knowledgeBase.new(parentId)` / `routes.knowledgeBase.new()`, per the finding's suggested fix. This assumes `src/lib/routes.ts` exports a `knowledgeBase.new(folderId?: string)` function; that file was not provided, so I could not verify the exact signature exists. If `routes.knowledgeBase.new` does not exist or has a different signature, this will fail to compile — the reviewer must confirm/add that export in `src/lib/routes.ts`.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 Relay connection IDs computed via ad-hoc local helper calls rather than a documented central key module

No code change was made for this finding. The finding calls for a new shared normalization helper module for connection-id parameter construction across multiple files (this component plus archive/move modals not shown here). Introducing such a helper safely requires seeing all call sites (use-knowledge-base-items.ts, folder-row-actions, archive/move modals) to define a correct shared contract; guessing the module's shape and updating only this file risks mismatching the very connection ids the finding warns about, causing real cache-invalidation bugs. A complete fix requires: (a) inspecting getKnowledgeBaseFoldersConnectionId/getKnowledgeBaseArticlesConnectionId/getKnowledgeBaseArticlesSubtreeConnectionId definitions and all call sites across the knowledge-base feature, (b) extracting one canonical { parentId, search, tagIds }-normalizing helper into use-knowledge-base-items.ts (already the shared module) or a new shared util, and (c) updating every call site consistently. That is out of scope for a single-file, minimal, safe fix without visibility into the other files.

🤖 Prompt for AI agents

In src/app/(app)/knowledge-base/components/knowledge-base-body.tsx around line 286, review and complete this code-review fix: Relay connection IDs computed via ad-hoc local helper calls rather than a documented central key module.
What the draft fix changed: No code change was made for this finding. The finding calls for a new shared normalization helper module for connection-id parameter construction across multiple files (this component plus archive/move modals not shown here). Introducing such a helper safely requires seeing all call sites (`use-knowledge-base-items.ts`, folder-row-actions, archive/move modals) to define a correct shared contract; guessing the module's shape and updating only this file risks mismatching the very connection ids the finding warns about, causing real cache-invalidation bugs. A complete fix requires: (a) inspecting `getKnowledgeBaseFoldersConnectionId`/`getKnowledgeBaseArticlesConnectionId`/`getKnowledgeBaseArticlesSubtreeConnectionId` definitions and all call sites across the knowledge-base feature, (b) extracting one canonical `{ parentId, search, tagIds }`-normalizing helper into `use-knowledge-base-items.ts` (already the shared module) or a new shared util, and (c) updating every call site consistently. That is out of scope for a single-file, minimal, safe fix without visibility into the other files.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 15 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 Hard-coded route path built with string interpolation in getLogDetailsUrl

In getLogDetailsUrl (inside LogsTableContent), extracted the manual template-string URL concatenation into a new local buildLogDetailsPath helper function (defined above LogsTableContent) that uses URLSearchParams instead of manual ${} interpolation, removing the ad-hoc encoding bug risk (only timestamp was previously encoded) and centralizing the query-string construction in one named function. This does NOT route through src/lib/routes.ts because that file's contents/shape (e.g. whether it has a logs/logDetails entry, and what param names it expects) were not visible to me, and inventing a registry entry or import that doesn't exist would violate the hard rule against introducing unresolvable imports. A complete fix requires adding a routes.logs.details(...) (or similarly named) builder to src/lib/routes.ts and switching this call site to use it — that edit is out of scope for a single-file change and should be done as a follow-up once that file's actual shape is confirmed.

🤖 Prompt for AI agents

In src/app/(app)/logs-page/components/logs-table.tsx around line 363, review and complete this code-review fix: Hard-coded route path built with string interpolation in getLogDetailsUrl.
What the draft fix changed: In `getLogDetailsUrl` (inside `LogsTableContent`), extracted the manual template-string URL concatenation into a new local `buildLogDetailsPath` helper function (defined above `LogsTableContent`) that uses `URLSearchParams` instead of manual `${}` interpolation, removing the ad-hoc encoding bug risk (only `timestamp` was previously encoded) and centralizing the query-string construction in one named function. This does NOT route through `src/lib/routes.ts` because that file's contents/shape (e.g. whether it has a `logs`/`logDetails` entry, and what param names it expects) were not visible to me, and inventing a registry entry or import that doesn't exist would violate the hard rule against introducing unresolvable imports. A complete fix requires adding a `routes.logs.details(...)` (or similarly named) builder to `src/lib/routes.ts` and switching this call site to use it — that edit is out of scope for a single-file change and should be done as a follow-up once that file's actual shape is confirmed.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 Log severity 'INFO' is mapped to grey Tag variant while 'error' status default falls through to success

In transformedLogs inside LogsTableContent, changed the final fallback branch of the severity-to-Tag-variant ternary from ('success' as const) to ('grey' as const), so any unrecognized/unexpected severity value now renders as a neutral grey tag instead of a misleading positive/success tag, matching the existing 'INFO' branch's neutral treatment.

🤖 Prompt for AI agents

In src/app/(app)/logs-page/components/logs-table.tsx around line 335, review and complete this code-review fix: Log severity 'INFO' is mapped to grey Tag variant while 'error' status default falls through to success.
What the draft fix changed: In `transformedLogs` inside `LogsTableContent`, changed the final fallback branch of the severity-to-Tag-variant ternary from `('success' as const)` to `('grey' as const)`, so any unrecognized/unexpected severity value now renders as a neutral grey tag instead of a misleading positive/success tag, matching the existing `'INFO'` branch's neutral treatment.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

[michaelassraf]

Task linked: CU-86akhf8u5 OpenFrame OSS frontend review findings sweep (12 PRs)