Skip to content

Replace keyword relevance detection with LLM-based intent classification for chat context#22

Closed
etsraphael wants to merge 3 commits intomainfrom
feature/chat-optimised-0
Closed

Replace keyword relevance detection with LLM-based intent classification for chat context#22
etsraphael wants to merge 3 commits intomainfrom
feature/chat-optimised-0

Conversation

@etsraphael
Copy link
Copy Markdown
Owner

Implement LLM-based intent classification to enhance chat context understanding, replacing the previous keyword relevance detection method.

Copilot AI review requested due to automatic review settings March 19, 2026 07:06
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@etsraphael etsraphael closed this Mar 19, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Mar 24, 2026

test

@claude
Copy link
Copy Markdown

claude Bot commented Mar 24, 2026

Frontend Code Review — forge-fe/

React 19 + TypeScript + Vite + Tailwind CSS v4 + shadcn/ui


🔴 Bugs

1. Session prepend logic is inverted — new sessions never appear in the sidebar
src/pages/Chat.tsx:417–430

savedSessionId = event.session_id   // just assigned
if (!savedSessionId) {              // always false — dead code
  setSessions(...)                  // new session is NEVER prepended
}

The intent is "if this is a new session, prepend it". Fix: check activeSessionId (the state value) rather than savedSessionId (just set on the line above).

2. Save/Delete errors are silent with no user feedback
src/components/board/task-detail-modal.tsx:151–155, 163–168

Both catch blocks call console.error only. If the API request fails, the user sees no indication. Add an error state and surface it in the modal.


🟠 Security

3. Markdown links rendered without rel="noopener noreferrer"
src/components/chat/markdown-content.tsx:59

a tags are in allowedElements but there is no custom renderer applying safety attributes. AI-generated links can perform reverse tabnapping:

components={{
  a: ({ href, children }) => (
    <a href={href} target="_blank" rel="noopener noreferrer">{children}</a>
  ),
}}

4. External images can be loaded from arbitrary URLs
src/components/chat/markdown-content.tsx:60

img is in allowedElements. Any markdown content from the model can embed arbitrary external images, enabling tracking pixels and IP leakage. Remove img from the list or add a custom renderer that validates src.

5. OAuth popup not closed on component unmount
src/pages/Connectors.tsx:185–213

cleanupPopup clears the interval but does not close the popup window itself. Add popupRef.current?.close() inside cleanupPopup.


🟠 Accessibility

6. Delete button in session sidebar is a <span>, not a <button>
src/pages/Chat.tsx:133–141

<span onClick={(e) => { e.stopPropagation(); onDelete(s.id) }}>
  <Trash2 />
</span>

<span> is not keyboard-focusable and conveys no ARIA role. Nesting a clickable element inside a <button> is also invalid HTML. Extract as a standalone <button type="button" aria-label="Delete session"> outside the parent button.

7. Labels not associated with inputs (missing htmlFor/id pairing)
src/components/board/task-detail-modal.tsx:262–309

All <label> elements in the edit form lack htmlFor and inputs lack id. Screen readers cannot associate them. Same pattern in Connectors.tsx:

<label htmlFor="edit-type">Type</label>
<select id="edit-type" ...>

8. Mobile hamburger button lacks aria-label
src/components/app-shell.tsx:192–196

The icon-only <button> has no accessible name. Add aria-label="Open navigation".

9. Chat textarea has no accessible label
src/pages/Chat.tsx:607–626

Only a placeholder is provided, which disappears on input and is not recognised as a label by all assistive tech. Add aria-label="Chat message".


🟡 React / TypeScript

10. Unsafe type casts from API responses
src/components/board/kanban-board.tsx:33–41

type: raw.type as BoardTask['type'],
column: raw.column_id as BoardColumn,

These bypass TypeScript at runtime. An unexpected API value silently produces invalid state. Add runtime guards or use zod for parsing.

11. No AbortController for the streaming fetch
src/pages/Chat.tsx:389–499

If the user navigates away or starts a new chat mid-stream, the previous stream continues running and calls setMessages against stale state. Add an AbortController and abort on cleanup.

12. persistReorder called with stale tasks closure
src/components/board/kanban-board.tsx:126

if (isOverColumn) {
  persistReorder(tasks)  // may be stale — drag-over setTasks batches are pending
  return
}

Track the current task list with a ref to ensure persistence always uses the latest state.

13. Non-null assertion on a potentially-null value
src/components/board/task-detail-modal.tsx:371

The condition (commandOutput || runningCommand) means commandOutput can be null when runningCommand is truthy. The ! assertion is incorrect. Fix:

{commandOutput && <MarkdownContent content={commandOutput} />}

🟡 UX / Other

14. ProjectSwitcher click-outside uses a full-screen invisible <div>
src/components/project-switcher.tsx:44

The fixed inset-0 overlay blocks all keyboard navigation while the dropdown is open. Use a mousedown useEffect listener instead (the pattern used correctly in ModelSelector).

15. "System OK" status is hardcoded
src/components/app-shell.tsx:119–121

The green dot shows "System OK" regardless of actual backend state. Wire it to a health endpoint or remove it.

16. console.error left in production paths
Chat.tsx, Connectors.tsx, kanban-board.tsx, task-detail-modal.tsx

Gate with import.meta.env.DEV or use a centralized error handler.

17. No error boundary around MarkdownContent

If rehype-highlight or react-markdown throws on unexpected model output, the entire Chat page unmounts with a blank screen. Wrap MarkdownContent in an ErrorBoundary to contain failures to the individual message bubble.


✅ Positives

  • allowedElements whitelist prevents arbitrary HTML injection — solid baseline for markdown safety.
  • postMessage origin check (event.origin !== window.location.origin) is correctly implemented in the OAuth flow.
  • memo() on MarkdownContent is the right call to avoid re-renders on every streaming token.
  • KeyboardSensor in kanban-board.tsx provides proper drag-and-drop keyboard support.
  • TypeScript prop interfaces are consistently defined for all components — no untyped any.
  • Responsive mobile sidebar pattern is clean and well-implemented.

Priority summary:

Priority Item
🔴 Bug Session prepend logic skips new sessions
🔴 Bug Save/Delete errors give no user feedback
🟠 Security Markdown <a> missing rel=noopener noreferrer
🟠 Security Arbitrary image URLs allowed in markdown
🟠 A11y Delete <span> must be a <button>
🟠 A11y <label> not associated with inputs via htmlFor
🟡 React No AbortController for streaming fetch
🟡 React Non-null assertion commandOutput! is incorrect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants