Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 2 commits into
base: main
Choose a base branch
from
+36 −3
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab9a8f8
fix(OPENFRAM-001-2): 4 review findings across 2 files
flamingo[bot] Sep 14, 2026
692f769
fix(OPENFRAM-001-2): 4 review findings across 2 files
flamingo[bot] Sep 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/app/(app)/knowledge-base/components/knowledge-base-body.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ function buildActions(
onNewFolder: () => void,
emphasizeAddArticle = false,
): PageActionButton[] {
const newArticleHref = parentId ? `/knowledge-base/new?folderId=${parentId}` : '/knowledge-base/new';

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

const newArticleHref = parentId ? routes.knowledgeBase.new(parentId) : routes.knowledgeBase.new();
const actions: PageActionButton[] = [
{
label: 'New Folder',
Comment on lines 79 to 85

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

Expand Down
37 changes: 35 additions & 2 deletions src/app/(app)/logs-page/components/logs-table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,33 @@ interface LogsTableContentProps {
onMobileFilterClose: () => void;
}

/**
* Builds the internal `/log-details` navigation URL for a log row.
*
* NOTE: kept as a local, explicit query-string builder rather than routed
* through a centralized registry entry, because no `logs`/`logDetails` entry
* currently exists in the routes registry to reuse without inventing its
* shape. Centralizing this path-building here (a single named helper) at
* least removes the inline duplication risk at the call site; migrating to
* a shared `routes.ts` entry should be a follow-up once that entry exists.
*/
function buildLogDetailsPath(params: {
id: string;
ingestDay: string;
toolType: string;
eventType: string;
timestamp?: string | null;
}): string {
const searchParams = new URLSearchParams({
id: params.id,
ingestDay: params.ingestDay,
toolType: params.toolType,
eventType: params.eventType,
timestamp: params.timestamp || '',
});
return `/log-details?${searchParams.toString()}`;
}

// ----------------------------------------------------------------
// Inner content — uses Relay hooks, must be inside Suspense
// ----------------------------------------------------------------
Expand Down Expand Up @@ -343,7 +370,7 @@ function LogsTableContent({
? ('grey' as const)
: log.severity === 'CRITICAL'
? ('critical' as const)
: ('success' as const),
: ('grey' as const),
},
source: {
name: toToolLabel(log.toolType),
Comment on lines 370 to 376

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand All @@ -363,7 +390,13 @@ function LogsTableContent({
const getLogDetailsUrl = useCallback((log: UiLogEntry): string => {

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

const original = log.originalLogEntry;
const id = log.id || log.logId;
return `/log-details?id=${id}&ingestDay=${original.ingestDay}&toolType=${original.toolType}&eventType=${original.eventType}&timestamp=${encodeURIComponent(original.timestamp || '')}`;
return buildLogDetailsPath({
id,
ingestDay: original.ingestDay,
toolType: original.toolType,
eventType: original.eventType,
timestamp: original.timestamp,
});
}, []);

const columns = useMemo<ColumnDef<UiLogEntry>[]>(
Expand Down
Loading