fix(web-react): channels phone mockup, sidebar filters, settings tabs, copy icons, welcome page, theme colors - #124
Conversation
Two-package monorepo for the reusable AI chat UI: - jcode-ui-core: framework-agnostic types (Message/ToolCall/Approval/ThreadItem), ChatRuntime abstraction + ExternalStoreRuntime (wraps any Redux-shaped store), MockRuntime (for demos/tests), ToolRendererRegistry (plugin seam), and headless React primitives (Thread with virtualization + auto-follow, MessageView, Composer, ToolCallView, ApprovalBlock, AskUserBlock). - jcode-ui: styled components wrapping the primitives with token-driven Tailwind 4 styling, the marked+highlight.js+DOMPurify markdown pipeline, and 9 default tool renderers (terminal/file-viewer/diff/search/todo/skill/team/browser-shot/generic). Both packages typecheck and build clean. Core dist + CSS bundle verified. Root pnpm-workspace.yaml added; .gitignore updated for node_modules/ and dist/.
- ChatDemo: scripted mock-runtime playground component (the 'website footprint') that streams a full conversation through message/tool/approval item kinds. - /chat-ui page: hero, live demo, feature grid, quick-start code sample. - site/docs/chat-ui/: runtime, primitives, tool-renderers, theming pages. - SiteNav + routing wired. Site typechecks and builds clean (ChatUIPage bundles at 350KB gzip — highlight.js + marked dominate, expected for a live demo). - jcode-ui core dep switched to file: so the isolated site workspace resolves it.
…ct shell Replaces the Vue app's runtime layer + shell with a React equivalent that consumes the jcode-ui component library: - lib/ : framework-agnostic ports (api.ts 384-line client, apiBase.ts dual-host contract, authToken.ts, useDesktop.ts Tauri bridge, ws.ts singleton client, types.ts full backend contract). Nearly verbatim from web/src/composables. - app/store.ts : RTK store split across 4 slices (chat/session/model/ui) with async thunks (sendMessage/stopAgent/resolveApproval/submitAskUser/editMessage). - app/runtime.ts : createExternalStoreRuntime adapter — the single seam between RTK and jcode-ui's RuntimeState. - app/wsBridge.ts : WS events → Redux dispatches (replaces Vue App.vue coupling). - components/ : product shell (Sidebar, ChatView, ProjectHeader, GoalBanner, AutomationsView, ChannelsView, CommandPalette, AuthGate, SetupView). typechecks + builds clean (626 modules, 421KB gzip bundle — code-splitting is a follow-up). Splits the 1.2k-line Vue chat store into focused slices per the migration assessment. The dual-host (browser/Tauri) contract is preserved.
- Makefile: add build-web-react (builds core → jcode-ui → CSS → web-react →
dist-react), FRONTEND var to select web/web-react, lint-react target.
Tolerates pnpm ERR_PNPM_IGNORED_BUILDS (esbuild/@parcel/watcher) so the build
chain is non-fragile.
- pnpm-workspace.yaml: onlyBuiltDependencies for esbuild + @parcel/watcher.
- .npmrc: auto-install-peers for the monorepo.
- AGENTS.md: document the Vue→React migration, web-react/, packages/, and the
build-web-react target; mark web/ as production-during-migration.
- packages/jcode-ui{,-core}/: README.md, LICENSE, .npmignore, publishConfig
(access public), files array — npm pack dry-run verified (styles.css + dist
included, src excluded).
Verified: make build-web-react end-to-end green; all 4 TS projects typecheck
(jcode-ui-core, jcode-ui, web-react, site); Go backend builds (embed intact);
Tauri frontendDist still points at the Vue dist (React is parallel).
…replay + separated chat-ui docs Runtime fixes (found via headless-browser testing against the live Go backend): - externalStore.ts: cache the normalized RuntimeState keyed on the host state reference so getState() returns a stable identity between dispatches. Without this, useSyncExternalStore infinite-looped (Maximum update depth) and crashed every page that rendered <Thread>. This was the root cause of the blank app. - context.tsx: trust the runtime's snapshot stability (remove the redundant and buggy double-cache); useRuntimeState/useRuntimeSelector now pass runtime.getState directly. - Thread.tsx VirtualizedThread: the scroll container resolved to height 0 inside flex parents, so the virtualizer rendered 0 rows. Restructured to flex:1 + min-height:0 so the height resolves through the chain. Verified the demo now streams its scripted conversation. web-react (product app): - loadSession thunk: replay a session's JSONL history into the timeline (was a TODO — the app booted to an empty chat). Walks entries, rebuilds messages + tool calls, matches tool_call_id, falls back to most-recent session on 404. - App.tsx boot: load current session, fall back to most-recent if empty. - toolInfo.ts: ported extractToolDisplayInfo (mirrors backend, for replay). - Sidebar openSession now loads the session via the thunk. - vite.config.ts: pin port 5173 (matches Tauri devUrl). site (component library docs + showcase): - Separated chat-ui docs from jcode product docs. New /chat-ui/docs/* route with its own pipeline (chatUiDocs.ts), nav tree, ChatUiDocsLayout, index, and DocPage. The sidebar now lists ONLY jcode-ui docs — no mixing with the jcode product docs (Agent/Plan Mode/Browser). Back-link to the chat-ui landing. - components.md: new component reference page with an assistant-ui→jcode-ui mapping table + props tables for every component (closes the assistant-ui feature-parity gap). - ChatDemo: rewrote the layout to flex/flex-col + min-h-0 so the virtualized Thread gets a concrete height (was rendering empty). Verified end-to-end with headless Chrome: - web-react: boots, loads session timeline (real history), streams a live prompt (model replied), tool cards render (✓ shell/read/edit +N/-M), Stop button swaps correctly, theme tokens applied, no JS errors. - site /chat-ui: ChatDemo streams the full scripted conversation (message→tool →approval) with virtualization; docs index + sub-pages render standalone. Build artifacts (jcode-new binary, *.tsbuildinfo) gitignored.
…ui parity); wire ⌘K
Closes the assistant-ui component-catalog gaps found in the parity audit. Every
assistant-ui component now has a jcode-ui equivalent, documented in components.md
with a full mapping table (✅ library / 🟡 product-level / field-driven).
New components (jcode-ui):
- Reasoning: collapsible model thinking block ('Thought for Ns'), markdown,
driven by message.reasoning. Mirrors assistant-ui Reasoning.
- Sources: citation chip list with snippet popovers, driven by
message.sources (MessageSource[]). Mirrors assistant-ui Sources.
- Attachment + AttachmentList: standalone image-attachment thumbnails (also
embedded in ChatInput). Mirrors assistant-ui Attachment.
Message now renders Reasoning (before body) + Sources (after body) automatically
when those fields are present. Added reasoning/sources/MessageSource to the core
Message type.
components.md: rewrote the assistant-ui→jcode-ui mapping to cover ALL 16 catalog
entries (Thread, ThreadList, Composer, Attachment, Markdown, DiffViewer, Image,
Context Display, Message Timing, Reasoning, Sources, Tool Fallback, Tool Group,
Assistant Modal/Sidebar, Model Selector, makeAssistantToolUI) with status +
notes. Added reference sections for Reasoning/Sources/Attachment with props.
web-react:
- App.tsx: wired global keyboard shortcuts (⌘K command palette, ⌘N new chat,
Esc closes overlays) — was missing entirely.
- main.tsx: dev-only window.__jcodeStore exposure for testing (stripped in prod).
- vite-env.d.ts: added (vite/client types for import.meta.env).
Demo (site/playground): the scripted conversation now ends with a message that
has reasoning + sources, so the ChatUIPage live demo showcases those components.
Verified end-to-end (headless Chrome):
- web-react: all 6 views render (ChatView, Automations w/ real data, Channels,
SetupView, AuthGate, CommandPalette via ⌘K + store toggle). No JS errors.
- site: all 8 pages render without regression (Home/Desktop/CLI/Showcase/Docs/
Chat-UI/Chat-UI Docs/Privacy).
- Demo single-play: Reasoning ('Thought process') + Sources (2 chips) render on
the final assistant message; production build bundles them.
…cts it) Tauri 2 strictly validates the config schema and rejects unknown fields. The override config had a $comment field for documentation, which crashed desktop-react-dev on startup: Error: Additional properties are not allowed (`$comment` was unexpected) Removed the $comment (and the redundant $schema). The build block alone is the only override needed; everything else is inherited from tauri.conf.json via deep merge. Verified: make desktop-react-dev now boots the Tauri window + Go sidecar + Vite dev server end-to-end.
…le + Automations/Channels parity Closes the major feature gaps vs the Vue app. Each ported file was read in full from the Vue source and typechecks individually. ChatInput.tsx (product composer, ~1.2k lines): - Full port of the Vue 2.2k-line composer: autosizing textarea, send/queue/stop (IME-safe), slash-command menu, MODE picker (approval/plan/full_access), MODEL picker (current/favorites/recent/all-providers with capability dots + context limit + manage-models dialog), EFFORT picker, '+' menu (attach images, slash insert, Goal arming), image attachments (paste + file picker + thumbnails), type-ahead queue chips, ⌘L focus, click-outside. ChatView now uses this product ChatInput instead of the library's minimal one. SettingsDialog.tsx (~1.5k lines): - Full port of the Vue 2.7k-line settings: 8 tabs (Providers/Models/MCP/Skills/ Appearance/Browser/Remote/Usage). Providers tab fully ported (CRUD + catalog + advanced config + custom models). MCP has OAuth-login polling. Browser has site-permissions editor. Usage has totals + trend chart. Opened via ⌘, and the header gear button. useTheme.ts + ThemeToggle.tsx: - Theme system ported (system/light/dark + 7 named themes, useSyncExternalStore, localStorage persistence, applies data-theme + .dark class). Toggle button in the header (sun/moon flip + swatch dropdown). AutomationsView.tsx: full CRUD (create/edit form with schedule/mode/project, run history with filter, templates picker, enable/disable, run-now, delete). ChannelsView.tsx: WeChat QR-login flow (login → QR → 2s poll → online → logout) + enable/disable + BLE card. ProjectHeader: added settings gear + ThemeToggle. App.tsx: renders SettingsDialog, ⌘, shortcut. Verified via headless Chrome: model+mode pickers render, Settings opens with all 8 tabs + real provider data, theme toggle flips .dark class, Automations shows real automation + templates + create, Channels shows WeChat card. No JS errors. typecheck + production build pass. Also: untracked the accidentally-committed internal/web/dist-react build output and gitignored it.
…+ correct theme/settings placement Fixes the shell-layout regressions vs the Vue app. Each piece was compared against the Vue source before implementing. TopBar.tsx (NEW): the floating top-right panel menu (absolute top:6 right:14, matching Vue exactly). Button = RectangleStackIcon + caret + live status dot (running/connected/disconnected priority). Dropdown: Plan(⇧⌘P)/Files(⇧⌘E)/ Changes(⇧⌘G)/Terminal(⌘`) with inline diff stat on Changes. Was entirely missing. RightPanel.tsx (NEW): the right-side panel with Plan/Files/Changes tabs. Plan = TaskList (todos from store); Files = file tree (api.files); Changes = diff viewer (api.diff). Close button + tab switcher. Was missing. TerminalPanel.tsx (NEW): bottom-docked xterm.js terminal. Creates a PTY, opens the PTY WebSocket (jcode-auth subprotocol), attaches FitAddon + WebLinksAddon, reads theme colors from tokens, handles resize + theme changes. Was missing. App.tsx (rewritten Shell): panel system (rightPanelOpen/rightPanelTab/ bottomPanel/bottomPanelHeight state + togglePanel logic + resize handle). Renders TopBar (chat view only, like Vue), Sidebar, main (chat + bottom terminal panel), RightPanel. Panel keyboard shortcuts wired (⇧⌘P/E/G, ⌘`). Removed the incorrect ProjectHeader — Vue has no header bar (TopBar carries the chrome). ChatView.tsx: removed ProjectHeader (the chat canvas has no header in Vue). SettingsDialog.tsx: rewritten chrome from small centered modal → FULL-SCREEN overlay (fixed inset-0, opaque bg, left nav rail + right content panel — matches Vue's settings shell). All 8 tab implementations preserved. Sidebar.tsx: moved settings gear + theme toggle to the FOOTER (Vue's placement), not the header. Added compact mode to ThemeToggle. Verified via headless Chrome: - TopBar at top:6/right:14 with 'Panels menu' label + status dot ✓ - Panel menu opens, Files click shows RightPanel ✓ - ⌘` opens xterm terminal (canvas present) ✓ - Settings (⌘,) is full-screen 1280x840 with left rail ✓ - Sidebar footer has settings + theme; header has neither ✓ - Channels view renders ✓ - typecheck + production build pass
…, copy icons, welcome page, theme colors, titlebar gap Each fix was compared against the Vue source before implementing. Channels: removed BLE card (Vue doesn't have it here), added the phone mockup (WeChat conversation mock with approval card), promo card with brand glow + feature list. Two-column stage layout matching Vue exactly. Sidebar: added 'Chat' heading + filter menu (status/time/sort), session grouping by recency (Today/Yesterday/This Week/Older), right-click context menu (Pin/Archive/Mark read/Delete via api.updateTask), hover/transition animations. Running-task breathing ring. SettingsDialog: tabs now match Vue exactly — general, appearance, providers, mcp, skills, browser, ssh, channels, shortcuts, usage. Removed the standalone Models tab (models live inside Providers in Vue). Added GeneralTab (mode/auto- approve/max-iterations/language), ShortcutsTab (keyboard reference), ChannelsTab (link to full page). Renamed Remote→SSH. ChatMessage copy/edit: changed from text 'Copy'/'Edit' to icons (Square2StackIcon/ CheckIcon/PencilSquareIcon) matching Vue. Added renderActions slot to the MessageView primitive. Welcome page: ChatView now shows a centered hero ([ J CODE ]) + composer when there are no messages, matching Vue's welcome screen. Titlebar gap: added .titlebar-drag strip + app-shell class + is-tauri-macos padding (28px top inset for the Tauri overlay title bar). Theme colors: imported tokens.generated.css (Go-generated named themes: dracula, nord, midnight, solarized, etc.). Without it, selecting a named theme only toggled .dark without applying the theme's actual colors. Verified: dracula theme now renders bg=#282A36, primary=#FFB86C. Verified via headless Chrome: - Welcome page: hero + composer + titlebar-drag ✓ - Channels: phone mockup present, no BLE, promo features ✓ - Settings: all 10 tabs correct (no Models), no missing tabs ✓ - Theme: dracula colors applied correctly ✓ - typecheck + production build pass
📝 WalkthroughWalkthroughThis PR migrates the frontend from Vue to React while keeping Vue as production. It adds two new packages (jcode-ui-core headless primitives/runtime and jcode-ui styled components), a new web-react Vite/Redux app mirroring the existing web app's features, chat-ui documentation/demo pages on the docs site, and supporting build tooling (Makefile, Tauri config, workspace config). A separate, unrelated draft document proposes a tool-search architecture change. ChangesReact Frontend Migration
Tool Search Architecture Draft
Estimated code review effort: 5 (Critical) | ~180 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant WebReactApp as web-react App
participant ReduxStore as Redux Store
participant WSClient
participant Backend as Go Backend
Browser->>WebReactApp: load, initApiBase()
WebReactApp->>Backend: api.health()
Backend-->>WebReactApp: health status
WebReactApp->>ReduxStore: seed provider/model/mode
WebReactApp->>ReduxStore: loadSession()
ReduxStore->>Backend: fetch session entries
Backend-->>ReduxStore: session timeline
WebReactApp->>WSClient: bridgeWS(client, getState, dispatch)
WSClient->>Backend: connect /api/ws
Backend-->>WSClient: agent_text / tool_call / tool_result events
WSClient->>ReduxStore: dispatch chatActions.*
ReduxStore-->>WebReactApp: state update via useChatRuntime
WebReactApp->>Browser: render Thread/ChatInput via jcode-ui
sequenceDiagram
participant User
participant Composer as Composer (jcode-ui-core)
participant Runtime as ChatRuntime
participant Registry as ToolRendererRegistry
participant ToolCallView
User->>Composer: type message, press Enter
Composer->>Runtime: sendMessage / enqueueMessage
Runtime-->>ToolCallView: new ThreadItem (tool)
ToolCallView->>Registry: get(tool.name)
Registry-->>ToolCallView: matched ToolRenderer
ToolCallView->>User: render expandable tool output
User->>Runtime: resolveApproval(id, approved, allowAll)
Runtime-->>ToolCallView: updated approval state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
web-react/src/components/Sidebar.tsx-210-214 (1)
210-214: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWindow
blurlistener never closes the context menu.
onKeyonly reacts whene.key === 'Escape', but ablurevent carries nokey, so this handler does nothing. If the intent is to dismiss the menu when the window loses focus, wire a dedicated handler.🐛 Proposed fix
function onKey(e: KeyboardEvent) { if (e.key === 'Escape') setCtx(null) } + function onBlur() { + setCtx(null) + } document.addEventListener('mousedown', onDown) document.addEventListener('keydown', onKey) - window.addEventListener('blur', onKey as never) + window.addEventListener('blur', onBlur) return () => { document.removeEventListener('mousedown', onDown) document.removeEventListener('keydown', onKey) - window.removeEventListener('blur', onKey as never) + window.removeEventListener('blur', onBlur) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/components/Sidebar.tsx` around lines 210 - 214, The Sidebar context-menu blur handling is wired to onKey, but that handler only closes on Escape and won’t respond to a window blur event. Update the blur listener in Sidebar.tsx to use a dedicated dismiss handler (or equivalent logic) that closes the menu when the window loses focus, and keep the existing onKey behavior only for keyboard events.docs/tool-search-architecture-draft.md-375-395 (1)
375-395: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLabel the locale snippet fence.
This bare fenced block will trip markdownlint. Add a language tag (or convert it to a table) so the doc stays lint-clean.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/tool-search-architecture-draft.md` around lines 375 - 395, The fenced locale snippet in the tool search architecture draft is unlabeled and will fail markdownlint. Update the snippet by adding an appropriate language tag to the fenced block, or convert the localization keys/values section into a table; keep the content around the existing settings.mcp.toolSearch entries intact.Source: Linters/SAST tools
docs/tool-search-architecture-draft.md-437-438 (1)
437-438: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace host-specific absolute paths with repo-relative ones.
The
/Users/...paths leak local machine details and make the doc non-portable. Use repo-relative paths (or drop the list) so the draft can move between environments cleanly.♻️ Suggested cleanup
- `/Users/jack/workpath/jjj/jcode/internal/config/config.go` + `internal/config/config.go`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/tool-search-architecture-draft.md` around lines 437 - 438, The “Key files touched” list in the draft contains host-specific absolute paths, which makes the document non-portable. Update that section to use repo-relative paths or remove the path list entirely, keeping the references tied to the same symbols/files like config.go, toolsearch.go, toolsearch_resolve.go, toolsearch_cap.go, web.go, acp.go, interactive.go, server.go, SettingsDialog.vue, and the locale files.Makefile-70-76 (1)
70-76: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStderr suppression on install hides genuine failures.
Line 72 uses
2>/dev/null || true, which silences all stderr from the install step. While the comment explains this toleratesERR_PNPM_IGNORED_BUILDS, it also hides real errors (network failures, lockfile corruption, missing packages). The build then continues and fails downstream with confusing "module not found" errors instead of a clear install failure message.Consider filtering only the known benign warning instead of suppressing all stderr:
🛡️ Proposed fix to narrow error suppression
- -pnpm install --frozen-lockfile 2>/dev/null || true + -pnpm install --frozen-lockfile 2>&1 | grep -v "ERR_PNPM_IGNORED_BUILDS" || trueOr alternatively, capture the exit code and only continue if it matches the ignored-builds error:
- -pnpm install --frozen-lockfile 2>/dev/null || true + -pnpm install --frozen-lockfile 2>&1 | tee /dev/stderr | grep -q "ERR_PNPM_IGNORED_BUILDS" || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 70 - 76, The install step in build-web-react is suppressing all stderr with pnpm install --frozen-lockfile 2>/dev/null || true, which hides real failures. Update the build-web-react target to keep the known benign ERR_PNPM_IGNORED_BUILDS case tolerated, but only by filtering or conditionally ignoring that specific warning in the install step; use the existing build-web-react and pnpm install commands as the anchor, and avoid masking other errors so genuine install problems fail fast with a clear message.site/src/playground/mockScript.ts-41-61 (1)
41-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScope the sequence counter to each demo run.
msg(),tool(), andapproval()close over the module-globalseq, andbuildDemoScript()resets it on every call. If this is reused or a previous run is still draining timers, sequence numbers can collide and React keys will stop being stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/src/playground/mockScript.ts` around lines 41 - 61, The sequence counter used by msg(), tool(), and approval() is module-global and reset inside buildDemoScript(), which can cause collisions across demo runs and unstable React keys. Scope seq to each buildDemoScript() invocation by creating the counter locally and threading it through the item factory helpers, or otherwise ensuring each script build gets its own isolated monotonically increasing sequence. Keep the fix centered around buildDemoScript() and the helper functions msg, tool, and approval.packages/jcode-ui/src/toolRenderers/browserShot.tsx-18-21 (1)
18-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPotential double-slash in image URL if
apiBaseends with/.The regex captures a path starting with
/api/..., andapiBaseis concatenated directly. IfapiBasehas a trailing slash (e.g.,http://host:3000/), the result ishttp://host:3000//api/browser/shots/...which may 404 on stricter servers.🔧 Proposed fix
- return m ? `${apiBase}${m[1]}` : '' + return m ? `${apiBase.replace(/\/$/, '')}${m[1]}` : ''🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/toolRenderers/browserShot.tsx` around lines 18 - 21, The browser shot URL builder in browserShot.tsx can produce a double slash when concatenating apiBase with the captured /api/browser/shots path. Update the useMemo logic around src so it normalizes apiBase before concatenation or otherwise joins the base and path safely, ensuring the generated image URL is correct whether apiBase ends with a slash or not.packages/jcode-ui/src/components/ChatInput.tsx-85-105 (1)
85-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid using array index as key for the removable attachments list.
When an attachment is removed via
remove(i), the remaining items shift indices, causing React to reuse the wrong DOM nodes. This can lead to incorrect image previews after deletion. If image objects have a unique identifier (e.g.,id,data, ormedia_type + data), use that as the key instead.🔧 Proposed fix
- {imgs.map((img, i) => ( - <div key={i} className="relative"> + {imgs.map((img, i) => ( + <div key={`${img.media_type}-${img.data.slice(0, 32)}`} className="relative">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/components/ChatInput.tsx` around lines 85 - 105, The removable attachments list in ChatInput uses the array index as the React key, which can cause the wrong preview to be reused after remove(i) shifts items. Update the renderAttachments mapping to use a stable unique identifier from each img object in place of i, such as an id field or a composite derived from img.data and img.media_type, so the attachment thumbnails remain correctly matched after deletions.packages/jcode-ui/src/styles/components.css-48-52 (1)
48-52: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace deprecated
word-break: break-wordwithoverflow-wrap: break-word.Stylelint flags
word-break: break-wordas deprecated on lines 51 and 75. The modern equivalent isoverflow-wrap: break-word, which provides the same behavior (breaking long words only when they would overflow) without using a deprecated keyword.🔧 Proposed fix
.jcode-diff-table td { padding: 0 0.5rem; white-space: pre-wrap; - word-break: break-word; + overflow-wrap: break-word; }Apply the same change at line 75 for
.jcode-file-table td:.jcode-file-table td { padding: 0 0.5rem; white-space: pre-wrap; - word-break: break-word; + overflow-wrap: break-word; }Also applies to: 72-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/styles/components.css` around lines 48 - 52, Replace the deprecated word-break usage in the CSS tables with the modern overflow-wrap property. Update the jcode-diff-table td rule and the matching jcode-file-table td rule in components.css, swapping word-break: break-word for overflow-wrap: break-word while keeping the existing wrapping behavior. Use the table selector blocks in the stylesheet to locate both occurrences.Source: Linters/SAST tools
packages/jcode-ui-core/src/primitives/AskUserBlock.tsx-36-37 (1)
36-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify
submitbehavior: comment says "no-op" but implementation always dispatches.The interface comment on line 37 states
submitis a "no-op if nothing chosen per question," but the implementation on lines 87-99 always callsactions.submitAskUser(tool.askUserId, answers)— even when no selections or "Other" text exist for any question. This sends answers with empty strings, which is semantically distinct fromskip(line 102, which sends[]). A consumer reading the comment may expectsubmitto do nothing when no options are selected, leading to unexpected backend data.Either update the comment to match the actual behavior, or add a guard to make
submittruly no-op when nothing is chosen:📝 Suggested fix — add a guard to match the documented "no-op" behavior
const submit = useCallback(() => { + const hasAnyAnswer = questions.some((q) => { + const key = keyOf(q) + return (state.selected[key]?.length ?? 0) > 0 || (state.other[key] ?? '').length > 0 + }) + if (!hasAnyAnswer) return const answers: AskUserAnswer[] = questions.map((q) => { const key = keyOf(q) const sel = state.selected[key] ?? [] const other = state.other[key] ?? '' return { question_header: key, answer: sel.length > 0 ? sel.join(', ') : other, selected: sel.length > 0 ? sel : undefined, } }) if (tool.askUserId) actions.submitAskUser(tool.askUserId, answers) }, [actions, keyOf, questions, state, tool.askUserId])Alternatively, update the comment to reflect the current behavior:
- /** Submit the current selections (no-op if nothing chosen per question). */ + /** Submit the current selections. Questions with no selection yield an empty answer. Use `skip` to submit no answers. */Also applies to: 87-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui-core/src/primitives/AskUserBlock.tsx` around lines 36 - 37, The `submit` contract in `AskUserBlock` is inconsistent with its implementation: the comment says it is a no-op when nothing is chosen, but `submit` always builds answers and calls `actions.submitAskUser`. Update either the `submit` comment to match the current behavior or, preferably, add a guard in `submit` so it only dispatches when at least one question has a selected option or non-empty “Other” text, while keeping `skip` as the explicit empty submission path.web-react/src/app/wsBridge.ts-61-63 (1)
61-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
onTodoUpdateswallows API errors silently.
api.todos().then(...)has no.catch(). If the fetch fails, the rejection is unhandled and todos silently won't update — hard to debug in production.🛡️ Proposed fix: add error handling
onTodoUpdate: () => { - void api.todos().then((todos) => dispatch(chatActions.setTodos(todos))) + void api.todos() + .then((todos) => dispatch(chatActions.setTodos(todos))) + .catch(() => { /* todo refresh failed — non-fatal */ }) },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/app/wsBridge.ts` around lines 61 - 63, `onTodoUpdate` in wsBridge currently calls `api.todos()` without handling failures, so the rejection is effectively swallowed. Update this callback to add explicit error handling around the `api.todos().then(...)` flow, ideally by chaining a catch or wrapping the await in try/catch, and log or surface the failure before returning so `chatActions.setTodos` only runs on success.web-react/src/components/SetupView.tsx-26-35 (1)
26-35: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSilently swallowed API errors in
useEffectleave users stuck.If
setupProviders()orsetupProviderModels()fails, the user sees an empty select with no error message and no way to proceed. Surface the error so the user knows to check their connection or retry.💡 Proposed fix: surface load errors
useEffect(() => { - api.setupProviders().then(setProviders).catch(() => {}) + api.setupProviders().then(setProviders).catch(() => setError('Failed to load providers')) }, []) useEffect(() => { if (!selected) return setModels([]) setModel('') - api.setupProviderModels(selected.id).then(setModels).catch(() => {}) + api.setupProviderModels(selected.id).then(setModels).catch(() => setError('Failed to load models')) }, [selected])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/components/SetupView.tsx` around lines 26 - 35, The two `useEffect` fetch paths in `SetupView` are swallowing failures from `api.setupProviders()` and `api.setupProviderModels()`, which leaves the form empty with no guidance. Update the effects to capture and surface those errors through the component state/UI instead of using empty `catch` blocks, so the user sees a clear message and can retry or check connectivity. Use the existing `selected`, `setProviders`, `setModels`, and `setModel` flows in `SetupView` to wire the error state into the rendered selects.web-react/src/app/store.ts-471-476 (1)
471-476: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
loadSessionsilently swallows all fetch errors.The comment says "A 404 means the session has no JSONL yet," but the catch block catches every error including network failures and server errors. The user gets no feedback that the session failed to load.
💡 Proposed fix: distinguish 404 from other errors
let entries: SessionEntry[] try { entries = await api.session(uuid) - } catch { - return + } catch (err) { + // A 404 means the session has no JSONL yet (fresh, never-used session). + if (err instanceof Error && err.message.includes('404')) return + dispatch(uiActions.setConnectionError('Failed to load session')) + return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/app/store.ts` around lines 471 - 476, The loadSession flow in store.ts is swallowing every api.session(uuid) failure, so update the try/catch around loadSession to only ignore the expected 404 case and surface all other errors. Use the existing api.session call and the loadSession logic to inspect the thrown error’s status/code, return early only when it indicates “not found,” and otherwise rethrow or dispatch an error state so network and server failures are visible to the user.web-react/src/components/ChatView.tsx-44-47 (1)
44-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix closing bracket typo in hero text —
)should be].Line 47 renders
)as the closing bracket, producing[J CODE)instead of[J CODE].🐛 Proposed fix
- <span style={{ opacity: 0.4 }}>)</span> + <span style={{ opacity: 0.4 }}>]</span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/components/ChatView.tsx` around lines 44 - 47, The hero text in ChatView has a closing bracket typo, so the rendered branding reads “[J CODE)” instead of “[J CODE]”. Update the JSX in ChatView’s hero span block to replace the closing parenthesis with a closing square bracket, keeping the existing styling and structure intact.web-react/src/components/ChatInput.tsx-1056-1077 (1)
1056-1077: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winInvalid HTML: interactive
StarIconSolidwithonClicknested inside a<button>.The favorites row is a
<button>element containingStarIconSolid(an SVG) with its ownonClickhandler. Nesting interactive content inside a<button>is invalid HTML and can cause accessibility issues — the star toggle is not keyboard-accessible and screen readers may not announce it. Restructure to use a<div role="button">for the row (matching the all-providers section at line 1092) and a separate<button>for the star.♿ Proposed fix for valid HTML and accessibility
- <button + <div key={`fav-${r.provider}-${r.model}`} - type="button" + role="button" + tabIndex={0} onClick={() => selectModel(r.provider, r.model)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + void selectModel(r.provider, r.model) + } + }} className="group flex w-full items-center gap-2.5 rounded-[var(--radius-md)] border-none bg-transparent px-2 py-1.5 text-left transition-colors hover:bg-[var(--color-muted)]" > {/* ... ProviderMonogram, name, subline, capability icons ... */} - <StarIconSolid - className="h-3.5 w-3.5 shrink-0 text-[var(--color-primary)]" - onClick={(e: ReactMouseEvent) => { e.stopPropagation(); void toggleFavorite(r.provider, r.model) }} - /> + <button + type="button" + aria-label="Remove from favorites" + onClick={(e: ReactMouseEvent) => { e.stopPropagation(); void toggleFavorite(r.provider, r.model) }} + className="shrink-0 border-none bg-transparent p-0 text-[var(--color-primary)]" + > + <StarIconSolid className="h-3.5 w-3.5" /> + </button> - </button> + </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/components/ChatInput.tsx` around lines 1056 - 1077, The favorites row in ChatInput currently nests the clickable StarIconSolid inside the main row <button>, which is invalid HTML and hurts accessibility. Update this row to match the all-providers pattern by making the container a non-button element with button semantics for selection, and move the favorite toggle into its own dedicated <button> with its own click handler. Keep the existing selectModel and toggleFavorite behavior intact, and preserve the ProviderMonogram/getModelDisplayName/modelSubline layout.
🧹 Nitpick comments (16)
web-react/src/components/AutomationsView.tsx (1)
834-861: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRun rows show a clickable affordance but have no action.
These rows use
cursor-pointerand a hover background (and the header comment mentions "clickable rows"), yet there's noonClick. Either wire the intended navigation (e.g. open the run's session) or drop the pointer/hover styling to avoid a misleading affordance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/components/AutomationsView.tsx` around lines 834 - 861, The run rows in AutomationsView are styled as clickable but have no action attached. Update the row container in the filtered runs list to either add the intended navigation behavior (for example, opening the run/session when clicked) using the existing row data like r.session_id, or remove the cursor-pointer and hover styling from that block if no click handler is meant to exist. Keep the change localized to the row markup in AutomationsView..gitignore (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
*.tsbuildinfoignore rule.
*.tsbuildinfoappears at both line 14 (Node/pnpm block) and line 30. The line 30 entry is redundant since line 14 already covers it globally.Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore at line 14, The .gitignore has a duplicate `*.tsbuildinfo` ignore entry; remove the redundant second rule and keep only the existing one in the Node/pnpm block so the ignore list stays deduplicated.web-react/vite.config.ts (1)
6-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment references wrong dist directory.
The comment says "builds into ../internal/web/dist" but the actual
outDiris../internal/web/dist-react. Lines 16-18 clarify the migration rationale, but the top comment is misleading on first read.✏️ Suggested comment fix
-// mirroring web/vite.config.ts: builds into ../internal/web/dist (consumed by the -// Go embed.FS and the Tauri shell), and proxies /api → the Go server on :8080 in dev. +// mirroring web/vite.config.ts: builds into ../internal/web/dist-react (consumed by the +// Go embed.FS and the Tauri shell), and proxies /api → the Go server on :8080 in dev.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/vite.config.ts` around lines 6 - 7, The top-level comment in the Vite config is stale and points to the wrong build output path, which makes the config misleading at a glance. Update the explanatory comment near the vite config header to match the actual outDir used by the config in this file, and keep the migration note aligned with the symbols that define the build target (for example the Vite config export and its outDir setting).Makefile (1)
158-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoot workspace install prerequisite for desktop-react targets is not enforced.
The comment at lines 163-164 states that
pnpm installmust have run once at the repo root, butdesktop-react-devanddesktop-react-buildonly runpnpm installinside$(DESKTOP_DIR). If a developer runsmake desktop-react-buildwithout a prior root install, the React workspace packages won't have their dependencies, and the build will fail with unclear errors.Consider adding a root workspace install guard or documenting the prerequisite more prominently (e.g., in the
desktop-react-devecho message).♻️ Optional: add a prerequisite check
desktop-react-dev: desktop-sidecar `@echo` "Launching desktop (React frontend)…" + `@echo` " (requires 'pnpm install' at repo root at least once)" cd $(DESKTOP_DIR) && (pnpm install 2>/dev/null || npm install) && \ pnpm tauri dev --config src-tauri/tauri.react.conf.json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 158 - 174, The desktop-react-dev and desktop-react-build targets only install dependencies inside $(DESKTOP_DIR), but the React workspace depends on a prior repo-root install. Update these Makefile targets to enforce the root workspace prerequisite (for example by adding a guard/check before running pnpm tauri dev/build, or by making the prerequisite explicit in the desktop-react-dev/desktop-react-build flow) so the React frontend dependencies are available before invoking tauri.react.conf.json.packages/jcode-ui/package.json (1)
66-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
@tailwindcss/typographytodevDependencies.This is a Tailwind build plugin used only during
build:css(via the@tailwindcss/cliwhich is already indevDependencies). Consumers import the pre-builtdist/styles.cssand never need this package at runtime. Keeping it independenciesunnecessarily inflates install size for downstream consumers.♻️ Proposed fix
"dependencies": { "`@heroicons/react`": "^2.2.0", - "`@tailwindcss/typography`": "^0.5.16", "dompurify": "^3.2.4", "highlight.js": "^11.11.1", "jcode-ui-core": "file:../jcode-ui-core", "marked": "^18.0.0", "marked-highlight": "^2.2.2" }, "devDependencies": { "`@tailwindcss/cli`": "^4.1.0", + "`@tailwindcss/typography`": "^0.5.16", "`@types/react`": "^18.3.18",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/package.json` at line 66, Move `@tailwindcss/typography` out of dependencies and into devDependencies in package.json, since it is only used by the Tailwind build step and not needed at runtime. Update the package manifest so the build tooling remains available for build:css via `@tailwindcss/cli`, but downstream consumers of dist/styles.css do not install this plugin as a production dependency.packages/jcode-ui/src/lib/apiBaseContext.tsx (1)
7-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer importing
ReactNodedirectly over relying on the globalReactnamespace.The file uses
import { createContext } from 'react'(named import) but referencesReact.ReactNodeon line 14. This works when@types/reactprovides the globalReactnamespace, but it's inconsistent with the named-import style. Importingtype ReactNodedirectly is more robust and self-documenting.♻️ Proposed refactor
-import { createContext } from 'react' +import { createContext, type ReactNode } from 'react' export const ApiBaseContext = createContext<string>('') export interface ApiBaseProviderProps { /** API base URL with no trailing slash. */ apiBase: string - children: React.ReactNode + children: ReactNode }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/lib/apiBaseContext.tsx` around lines 7 - 15, The ApiBaseProviderProps type currently relies on the global React namespace for the children prop, which is inconsistent with the existing named import style. Update apiBaseContext by importing the ReactNode type directly alongside createContext, then use that imported type for children in ApiBaseProviderProps so the component context types are explicit and self-contained.packages/jcode-ui/src/toolRenderers/fileViewer.tsx (1)
16-24: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMemoize
pathparsing alongsidelinesto avoid re-parsing on every render.
pathis computed viaJSON.parse(args)on every render, whilelinesis memoized. Moving both into a singleuseMemokeeps them consistent and avoids redundant parsing.♻️ Proposed refactor
export const FileViewerRenderer = memo(function FileViewerRenderer({ args, output }: ToolRendererProps) { - let path = '' - try { - const parsed = JSON.parse(args) - path = parsed.path ?? parsed.file_path ?? '' - } catch { - // ignore - } - const lines = useMemo(() => parseLines(output), [output]) + const { path, lines } = useMemo(() => { + let p = '' + try { + const parsed = JSON.parse(args) + p = parsed.path ?? parsed.file_path ?? '' + } catch { /* ignore */ } + return { path: p, lines: parseLines(output) } + }, [args, output])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/toolRenderers/fileViewer.tsx` around lines 16 - 24, FileViewerRenderer is parsing args into path on every render while only lines is memoized. Move the JSON.parse(args) logic into the same useMemo used for parseLines(output), or a shared useMemo that returns both path and lines, so path is computed once per args change and stays consistent with output. Keep the fix centered in FileViewerRenderer and preserve the existing fallback behavior for invalid args.packages/jcode-ui/src/toolRenderers/diff.tsx (1)
63-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a real line-matching diff algorithm for readability.
buildDiffmarks every old line as deleted and every new line as added without matching unchanged lines. For small edits this is fine, but for larger changes the diff becomes very noisy — unchanged lines appear twice (once red, once green). A lightweight LCS-based line diff would produce context lines and only highlight actual changes.If this is intentionally simple for v1, feel free to skip. Otherwise, consider using a library like
diff(npm) or a minimal LCS implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/toolRenderers/diff.tsx` around lines 63 - 82, `buildDiff` in the diff renderer is currently treating every line as changed, which makes larger diffs noisy and hard to read. Update the line-diff logic in `buildDiff` to match unchanged lines instead of emitting all old lines as deletions and all new lines as additions. Use a lightweight LCS-based approach or a small diff library so the `DiffRow` output includes context/unchanged lines and only highlights real edits. Keep the existing `path` parsing and `DiffRow` structure intact while replacing the per-line emit loop.packages/jcode-ui/src/components/Reasoning.tsx (2)
27-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-expandedto the toggle button.Screen readers cannot convey whether the reasoning content is expanded or collapsed. Adding
aria-expanded={expanded}is a one-line accessibility improvement consistent with the disclosure pattern.♿ Proposed improvement
<button type="button" onClick={() => setExpanded((e) => !e)} + aria-expanded={expanded} className="flex items-center gap-1.5 rounded-[var(--radius-md)] px-2 py-1 text-[0.75rem] italic text-[var(--color-muted-foreground)] transition-colors hover:bg-[var(--neutral-wash-soft)]" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/components/Reasoning.tsx` around lines 27 - 34, Add the missing disclosure state to the toggle in Reasoning by updating the button in Reasoning.tsx to expose aria-expanded={expanded}. Keep the change localized to the existing toggle button that uses setExpanded and ChevronDownIcon so screen readers can announce whether the reasoning section is open or closed.
35-39: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the
renderMarkdownoutput.
renderMarkdown(reasoning)runs the full marked + highlight.js + DOMPurify pipeline on every render when expanded.MarkdownBodyinMessage.tsxcorrectly wraps this inuseMemo. Apply the same pattern here to avoid redundant re-computation for long chain-of-thought text.♻️ Proposed refactor
export function Reasoning({ reasoning, defaultExpanded = false, durationMs }: ReasoningProps) { const [expanded, setExpanded] = useState(defaultExpanded) + const html = useMemo(() => renderMarkdown(reasoning), [reasoning]) const label = durationMs != null ? `Thought for ${(durationMs / 1000).toFixed(1)}s` : 'Thought process' return ( <div className="jcode-reasoning my-1"> <button type="button" onClick={() => setExpanded((e) => !e)} className="flex items-center gap-1.5 rounded-[var(--radius-md)] px-2 py-1 text-[0.75rem] italic text-[var(--color-muted-foreground)] transition-colors hover:bg-[var(--neutral-wash-soft)]" > <ChevronDownIcon className={`h-3 w-3 transition-transform ${expanded ? 'rotate-180' : ''}`} /> {label} </button> {expanded && ( <div className="jcode-prose mt-1 border-l-2 border-[var(--color-border)] pl-3 text-[0.82rem] italic leading-relaxed text-[var(--color-muted-foreground)]" - dangerouslySetInnerHTML={{ __html: renderMarkdown(reasoning) }} + dangerouslySetInnerHTML={{ __html: html }} /> )} </div> ) }Don't forget to add
useMemoto the React import:-import { useState } from 'react' +import { useMemo, useState } from 'react'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/components/Reasoning.tsx` around lines 35 - 39, The expanded Reasoning view is recomputing the full `renderMarkdown(reasoning)` pipeline on every render, so memoize that output just like `MarkdownBody` in `Message.tsx`. Update `Reasoning` to import `useMemo`, compute the rendered HTML once from `reasoning`, and pass the memoized value into `dangerouslySetInnerHTML` so re-renders don’t repeat the marked/highlight.js/DOMPurify work.packages/jcode-ui/src/components/ToolCallCard.tsx (1)
66-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-expandedto the header toggle button.The header button controls content visibility but doesn't expose its expanded/collapsed state to assistive technologies. Adding
aria-expanded={expanded}is a one-line accessibility improvement. Note: if theToolCallViewprimitive fromjcode-ui-corealready injectsaria-expandedonto the rendered header, this would be redundant — verify the primitive's behavior.♿ Proposed improvement
<button type="button" onClick={onToggle} + aria-expanded={expanded} className={`group flex w-full items-center gap-2 rounded-[var(--radius-md)] px-2 py-1 text-left text-[0.82rem] transition-colors hover:bg-[var(--neutral-wash-soft)] ${🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jcode-ui/src/components/ToolCallCard.tsx` around lines 66 - 88, The toggle button in ToolCallCard needs to expose its open/closed state for accessibility. Update the header button that uses onToggle and expanded to include an aria-expanded attribute bound to expanded, and verify whether ToolCallView or any shared primitive already applies this so you don’t duplicate it. Keep the change localized to the button markup in ToolCallCard.web-react/src/lib/ws.ts (1)
59-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCached handler map ignores
setHandlersupdates.
handlerForbuildshandlerMaponce and caches it. The lambdas captureconst h = this.handlersat build time. IfsetHandlersis called later,this.handlersis replaced but the cached map still dispatches to the old object. In the currentbridgeWSusage this is latent (handlers are set once before connecting), but it is a correctness trap for any future caller.♻️ Proposed fix: invalidate cache on setHandlers
setHandlers(handlers: WSHandlers): void { this.handlers = handlers + this.handlerMap = null }Also applies to: 141-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/lib/ws.ts` around lines 59 - 61, The cached handler map in WS ignores later updates because handlerFor builds it from the current handlers only once; update setHandlers so it invalidates or rebuilds the cached map whenever this.handlers changes, ensuring future dispatch uses the new WSHandlers instead of the stale captured object. Make the fix in the WS class around setHandlers and handlerFor so the cache and the handler source stay in sync.web-react/src/lib/api.ts (1)
16-38: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftConsider adding a default timeout to
request()to prevent indefinite UI hangs.
initApiBase()ensures the backend is healthy before mount, but individual requests can still hang (e.g., slow query, stalled connection). Without a timeout, the calling component's promise never resolves or rejects, leaving the UI in a loading state indefinitely. AnAbortControllerwith a configurable default (e.g., 30s) would allow callers to surface a actionable error.💡 Example timeout wrapper
async function request<T>(path: string, options?: RequestOptions): Promise<T> { const token = getAuthToken() const headers = new Headers(options?.headers) if (!headers.has('Content-Type')) headers.set('Content-Type', 'application/json') if (token && !options?.skipAuth && !headers.has('Authorization')) { headers.set('Authorization', `Bearer ${token}`) } const controller = new AbortController() const timeoutId = setTimeout(() => controller.abort(), 30_000) try { const resp = await fetch(`${apiBase}${path}`, { ...options, headers, signal: controller.signal }) if (resp.status === 401 && !options?.skipAuth) { notifyAuthExpired() } if (!resp.ok) { const body = await resp.json().catch(() => ({ error: resp.statusText })) const err = new Error(body.error || `HTTP ${resp.status}`) as Error & { status?: number } err.status = resp.status throw err } return resp.json() } catch (e) { if (e instanceof DOMException && e.name === 'AbortError') { throw new Error('Request timed out') } throw e } finally { clearTimeout(timeoutId) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/lib/api.ts` around lines 16 - 38, The request helper in request() can hang indefinitely because fetch has no timeout, so add a default timeout using an AbortController and a clearable timer around the existing fetch call. Wire the controller.signal into the fetch options, keep the existing auth/header and 401 handling intact, and convert AbortError into a user-facing timeout error so callers can recover. Make the timeout configurable if possible, but keep a sensible default in request() for all calls.web-react/src/app/store.ts (1)
387-387: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer top-level type imports over inline
import()types.
ChatImageandAskUserAnswerfromjcode-ui-coreare used via inlineimport()syntax while other types from the same package are imported at the top of the file. Add them to the existing import for consistency.♻️ Proposed refactor
-import type { ThreadItem, Message, ToolCall, Approval, TokenSnapshot, Goal, TodoItem, QueuedMessage, AskUserQuestion } from 'jcode-ui-core' +import type { ThreadItem, Message, ToolCall, Approval, TokenSnapshot, Goal, TodoItem, QueuedMessage, AskUserQuestion, ChatImage, AskUserAnswer } from 'jcode-ui-core'Then update the thunk signatures:
- async (payload: { text: string; images?: import('jcode-ui-core').ChatImage[]; mode?: AgentMode }, { dispatch, getState }) => { + async (payload: { text: string; images?: ChatImage[]; mode?: AgentMode }, { dispatch, getState }) => {- async (payload: { id: string; answers: import('jcode-ui-core').AskUserAnswer[] }, { dispatch }) => { + async (payload: { id: string; answers: AskUserAnswer[] }, { dispatch }) => {Also applies to: 425-425
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/app/store.ts` at line 387, The thunk payload and related signatures are using inline import() types for ChatImage and AskUserAnswer while the rest of the file already imports jcode-ui-core types at the top. Update the existing top-level import in store.ts to include these types, then replace the inline import() annotations in the affected thunk signatures with the imported symbols for consistency.web-react/src/App.tsx (1)
167-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse lazy initialization for the tool registry.
useRef(createDefaultToolRegistry())callscreateDefaultToolRegistry()on every render, but only the first result is stored. UseuseStatewith a lazy initializer to avoid unnecessary calls.♻️ Proposed refactor
- const registry = useRef(createDefaultToolRegistry()).current + const [registry] = useState(() => createDefaultToolRegistry())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/App.tsx` around lines 167 - 169, The Shell component is eagerly creating the tool registry on every render via useRef(createDefaultToolRegistry()), even though only the first value is kept. Update Shell to use a lazy initializer with useState for the registry so createDefaultToolRegistry() runs only once, and keep the existing registry variable usage unchanged.web-react/src/components/ChatInput.tsx (1)
352-369: 📐 Maintainability & Code Quality | 🔵 Trivial
toggleFavoritefetches updated favorites but discards them.After calling
api.toggleFavorite, the code fetchesapi.modelState()and extractsnewFavs, then immediately discards both withvoid newFavsandvoid favoriteModels. The star tint won't flip in the UI until a fullapi.models()refresh re-seeds the slice. The TODO at line 357 documents this, but the fetched data is silently dropped.Would you like me to generate a
setFavoriteModelsreducer and wire it here so the toggle reflects immediately?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-react/src/components/ChatInput.tsx` around lines 352 - 369, The toggleFavorite flow fetches the updated favorites but drops them, so the UI state never updates immediately. In ChatInput.tsx, use the result from api.modelState() to update the favoriteModels-backed slice instead of discarding newFavs and favoriteModels; wire this through a proper setFavoriteModels reducer or equivalent state update so the star tint reflects the toggle right away. Keep the api.toggleFavorite and api.modelState calls, but replace the no-op voids with the actual state-setting path in toggleFavorite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6679309-4bf5-47c2-ac92-0ae266357d4f
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsite/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (121)
.gitignore.npmrcAGENTS.mdMakefiledesktop/src-tauri/tauri.react.conf.jsondocs/tool-search-architecture-draft.mdinternal/model/registry_generated.gojcode-newpackages/jcode-ui-core/.npmignorepackages/jcode-ui-core/LICENSEpackages/jcode-ui-core/README.mdpackages/jcode-ui-core/package.jsonpackages/jcode-ui-core/src/adapters/index.tspackages/jcode-ui-core/src/hooks/index.tspackages/jcode-ui-core/src/index.tspackages/jcode-ui-core/src/primitives/ApprovalBlock.tsxpackages/jcode-ui-core/src/primitives/AskUserBlock.tsxpackages/jcode-ui-core/src/primitives/Composer.tsxpackages/jcode-ui-core/src/primitives/MessageView.tsxpackages/jcode-ui-core/src/primitives/Thread.tsxpackages/jcode-ui-core/src/primitives/ToolCallView.tsxpackages/jcode-ui-core/src/primitives/index.tspackages/jcode-ui-core/src/runtime/context.tsxpackages/jcode-ui-core/src/runtime/externalStore.tspackages/jcode-ui-core/src/runtime/index.tspackages/jcode-ui-core/src/runtime/mockRuntime.tspackages/jcode-ui-core/src/types/index.tspackages/jcode-ui-core/tsconfig.build.jsonpackages/jcode-ui-core/tsconfig.jsonpackages/jcode-ui/.npmignorepackages/jcode-ui/LICENSEpackages/jcode-ui/README.mdpackages/jcode-ui/package.jsonpackages/jcode-ui/src/components/ApprovalBanner.tsxpackages/jcode-ui/src/components/AskUserCard.tsxpackages/jcode-ui/src/components/Attachment.tsxpackages/jcode-ui/src/components/ChatInput.tsxpackages/jcode-ui/src/components/ContextBar.tsxpackages/jcode-ui/src/components/Message.tsxpackages/jcode-ui/src/components/Reasoning.tsxpackages/jcode-ui/src/components/Sources.tsxpackages/jcode-ui/src/components/Thread.tsxpackages/jcode-ui/src/components/ToolCallCard.tsxpackages/jcode-ui/src/components/ToolRegistryContext.tsxpackages/jcode-ui/src/index.tspackages/jcode-ui/src/lib/apiBaseContext.tsxpackages/jcode-ui/src/lib/markdown.tspackages/jcode-ui/src/styles/animations.csspackages/jcode-ui/src/styles/components.csspackages/jcode-ui/src/styles/entry.csspackages/jcode-ui/src/styles/tokens.csspackages/jcode-ui/src/toolRenderers/browserShot.tsxpackages/jcode-ui/src/toolRenderers/diff.tsxpackages/jcode-ui/src/toolRenderers/fileViewer.tsxpackages/jcode-ui/src/toolRenderers/generic.tsxpackages/jcode-ui/src/toolRenderers/index.tspackages/jcode-ui/src/toolRenderers/search.tsxpackages/jcode-ui/src/toolRenderers/skill.tsxpackages/jcode-ui/src/toolRenderers/team.tsxpackages/jcode-ui/src/toolRenderers/terminal.tsxpackages/jcode-ui/src/toolRenderers/todo.tsxpackages/jcode-ui/tsconfig.build.jsonpackages/jcode-ui/tsconfig.jsonpnpm-workspace.yamlsite/docs/chat-ui/components.mdsite/docs/chat-ui/index.mdsite/docs/chat-ui/primitives.mdsite/docs/chat-ui/runtime.mdsite/docs/chat-ui/theming.mdsite/docs/chat-ui/tool-renderers.mdsite/package.jsonsite/src/App.tsxsite/src/components/SiteNav.tsxsite/src/lib/chatUiDocs.tssite/src/pages/ChatUIPage.tsxsite/src/pages/chatui.csssite/src/pages/chatui/ChatUiDocPage.tsxsite/src/pages/chatui/ChatUiDocsIndex.tsxsite/src/pages/chatui/ChatUiDocsLayout.tsxsite/src/playground/ChatDemo.tsxsite/src/playground/mockScript.tssite/tsconfig.app.tsbuildinfoweb-react/index.htmlweb-react/package.jsonweb-react/src/App.tsxweb-react/src/app/hooks.tsweb-react/src/app/runtime.tsweb-react/src/app/store.tsweb-react/src/app/wsBridge.tsweb-react/src/components/AuthGate.tsxweb-react/src/components/AutomationsView.tsxweb-react/src/components/ChannelsView.tsxweb-react/src/components/ChatInput.tsxweb-react/src/components/ChatView.tsxweb-react/src/components/CommandPalette.tsxweb-react/src/components/GoalBanner.tsxweb-react/src/components/ProjectHeader.tsxweb-react/src/components/RightPanel.tsxweb-react/src/components/SettingsDialog.tsxweb-react/src/components/SetupView.tsxweb-react/src/components/Sidebar.tsxweb-react/src/components/TerminalPanel.tsxweb-react/src/components/ThemeToggle.tsxweb-react/src/components/TopBar.tsxweb-react/src/lib/api.tsweb-react/src/lib/apiBase.tsweb-react/src/lib/authToken.tsweb-react/src/lib/automation.tsweb-react/src/lib/toolInfo.tsweb-react/src/lib/types.tsweb-react/src/lib/useDesktop.tsweb-react/src/lib/useTheme.tsweb-react/src/lib/ws.tsweb-react/src/main.tsxweb-react/src/styles.cssweb-react/src/styles/tokens.generated.cssweb-react/src/vite-env.d.tsweb-react/tsconfig.app.jsonweb-react/tsconfig.jsonweb-react/tsconfig.node.jsonweb-react/vite.config.ts
💤 Files with no reviewable changes (1)
- site/tsconfig.app.tsbuildinfo
| Defaulting accessor (mirrors `CompactionThreshold`; named `ToolSearchSettings()` to avoid the field/method name clash; returns a **value** so concurrent sessions never read a half-mutated struct): | ||
| ```go | ||
| func (c *Config) ToolSearchSettings() ToolSearchConfig { | ||
| out := ToolSearchConfig{Mode: "auto", Threshold: 20, UnattendedFallback: "native-or-off"} | ||
| if c == nil || c.ToolSearch == nil { | ||
| return out | ||
| } | ||
| switch c.ToolSearch.Mode { | ||
| case "off", "auto", "client", "model": | ||
| out.Mode = c.ToolSearch.Mode | ||
| case "": | ||
| // empty -> keep default "auto" (matches DefaultMode empty-string fallback prior art) | ||
| default: | ||
| // unknown persisted value -> default "auto" (defensive; the PUT handler rejects bad | ||
| // values with 400, so a bad value can only arrive via hand-edit) | ||
| } | ||
| if c.ToolSearch.Threshold > 0 { | ||
| out.Threshold = c.ToolSearch.Threshold | ||
| } | ||
| switch c.ToolSearch.UnattendedFallback { | ||
| case "off", "client", "native-or-off": | ||
| out.UnattendedFallback = c.ToolSearch.UnattendedFallback | ||
| } | ||
| out.AlwaysLoadServers = c.ToolSearch.AlwaysLoadServers | ||
| return out |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Deep-copy AlwaysLoadServers before returning the config value.
This accessor still aliases the slice, so callers can observe cross-session mutations even though the struct itself is copied. Deep-copy every nested mutable field here; otherwise the in-place reload plan later in the doc still shares state.
♻️ Suggested fix
- out.AlwaysLoadServers = c.ToolSearch.AlwaysLoadServers
+ out.AlwaysLoadServers = append([]string(nil), c.ToolSearch.AlwaysLoadServers...)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Defaulting accessor (mirrors `CompactionThreshold`; named `ToolSearchSettings()` to avoid the field/method name clash; returns a **value** so concurrent sessions never read a half-mutated struct): | |
| ```go | |
| func (c *Config) ToolSearchSettings() ToolSearchConfig { | |
| out := ToolSearchConfig{Mode: "auto", Threshold: 20, UnattendedFallback: "native-or-off"} | |
| if c == nil || c.ToolSearch == nil { | |
| return out | |
| } | |
| switch c.ToolSearch.Mode { | |
| case "off", "auto", "client", "model": | |
| out.Mode = c.ToolSearch.Mode | |
| case "": | |
| // empty -> keep default "auto" (matches DefaultMode empty-string fallback prior art) | |
| default: | |
| // unknown persisted value -> default "auto" (defensive; the PUT handler rejects bad | |
| // values with 400, so a bad value can only arrive via hand-edit) | |
| } | |
| if c.ToolSearch.Threshold > 0 { | |
| out.Threshold = c.ToolSearch.Threshold | |
| } | |
| switch c.ToolSearch.UnattendedFallback { | |
| case "off", "client", "native-or-off": | |
| out.UnattendedFallback = c.ToolSearch.UnattendedFallback | |
| } | |
| out.AlwaysLoadServers = c.ToolSearch.AlwaysLoadServers | |
| return out | |
| Defaulting accessor (mirrors `CompactionThreshold`; named `ToolSearchSettings()` to avoid the field/method name clash; returns a **value** so concurrent sessions never read a half-mutated struct): |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tool-search-architecture-draft.md` around lines 249 - 273, The
ToolSearchSettings accessor currently returns a copied ToolSearchConfig but
still aliases AlwaysLoadServers, so callers can share mutable slice state across
sessions. Update ToolSearchSettings on Config to deep-copy AlwaysLoadServers
before returning the value, and ensure any other nested mutable fields in
ToolSearchConfig are also cloned so the returned config is fully independent.
| export function useIsAtBottom<T extends HTMLElement>(threshold = 80) { | ||
| const { ref, onScroll, scrollToBottom } = useAutoScroll<T>(threshold) | ||
| return { ref, onScroll, scrollToBottom } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
useIsAtBottom does not implement its documented re-render behavior.
The JSDoc claims this hook "re-renders the component when the flag flips" and "intentionally tracks a coarse boolean," but the implementation simply wraps useAutoScroll and returns { ref, onScroll, scrollToBottom } — no useState, no isAtBottom value, and no re-render trigger. The hook is named useIsAtBottom yet doesn't return isAtBottom.
🐛 Proposed fix
-import { useCallback, useEffect, useRef } from 'react'
+import { useCallback, useEffect, useRef, useState } from 'react' export function useIsAtBottom<T extends HTMLElement>(threshold = 80) {
- const { ref, onScroll, scrollToBottom } = useAutoScroll<T>(threshold)
- return { ref, onScroll, scrollToBottom }
+ const { ref, onScroll, scrollToBottom, getIsAtBottom } = useAutoScroll<T>(threshold)
+ const [isAtBottom, setIsAtBottom] = useState(true)
+
+ const onScrollWithState = useCallback(() => {
+ onScroll()
+ const next = getIsAtBottom()
+ setIsAtBottom((prev) => (prev === next ? prev : next))
+ }, [onScroll, getIsAtBottom])
+
+ return { ref, onScroll: onScrollWithState, scrollToBottom, isAtBottom }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function useIsAtBottom<T extends HTMLElement>(threshold = 80) { | |
| const { ref, onScroll, scrollToBottom } = useAutoScroll<T>(threshold) | |
| return { ref, onScroll, scrollToBottom } | |
| } | |
| import { useCallback, useEffect, useRef, useState } from 'react' | |
| export function useIsAtBottom<T extends HTMLElement>(threshold = 80) { | |
| const { ref, onScroll, scrollToBottom, getIsAtBottom } = useAutoScroll<T>(threshold) | |
| const [isAtBottom, setIsAtBottom] = useState(true) | |
| const onScrollWithState = useCallback(() => { | |
| onScroll() | |
| const next = getIsAtBottom() | |
| setIsAtBottom((prev) => (prev === next ? prev : next)) | |
| }, [onScroll, getIsAtBottom]) | |
| return { ref, onScroll: onScrollWithState, scrollToBottom, isAtBottom } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jcode-ui-core/src/hooks/index.ts` around lines 56 - 59,
`useIsAtBottom` currently just forwards `useAutoScroll` and never exposes or
updates an `isAtBottom` flag, so it does not match its JSDoc or name. Update the
hook in `useIsAtBottom` to track bottom-state with React state (or equivalent),
derive a coarse boolean from scroll position, and trigger a re-render when that
boolean changes. Keep the existing `ref`, `onScroll`, and `scrollToBottom`
behavior from `useAutoScroll`, but return the new `isAtBottom` value as part of
the hook’s result.
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault() | ||
| saveEdit() | ||
| } else if (e.key === 'Escape') { | ||
| cancelEdit() | ||
| } | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add IME composition guard to the edit textarea key handler.
The edit textarea's onKeyDown lacks the IME composition check that Composer.tsx (line 144) correctly implements. CJK users pressing Enter to confirm an IME composition will unintentionally trigger saveEdit(), saving unconfirmed text and exiting edit mode.
🐛 Proposed fix — match the IME guard pattern from Composer.tsx
onKeyDown={(e) => {
+ if (e.nativeEvent.isComposing || e.keyCode === 229) return
if (e.key === 'Enter' && !e.shiftKey) {
e.preventDefault()
saveEdit()
} else if (e.key === 'Escape') {
cancelEdit()
}
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' && !e.shiftKey) { | |
| e.preventDefault() | |
| saveEdit() | |
| } else if (e.key === 'Escape') { | |
| cancelEdit() | |
| } | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.nativeEvent.isComposing || e.keyCode === 229) return | |
| if (e.key === 'Enter' && !e.shiftKey) { | |
| e.preventDefault() | |
| saveEdit() | |
| } else if (e.key === 'Escape') { | |
| cancelEdit() | |
| } | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jcode-ui-core/src/primitives/MessageView.tsx` around lines 106 -
113, The edit textarea key handler in MessageView is missing the IME composition
guard, so Enter can incorrectly trigger saveEdit during composition. Update the
onKeyDown logic in MessageView to match the guard pattern used in Composer by
checking the native event’s isComposing state before handling Enter, while
keeping Escape mapped to cancelEdit. This should be applied in the textarea
handler where saveEdit and cancelEdit are invoked.
| {questions.map((q, qi) => { | ||
| const key = q.header ?? q.question | ||
| const sel = controls.selected[key] ?? [] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Key collision risk when multiple questions share the same header or question text.
const key = q.header ?? q.question is used to index into controls.selected[key] and controls.other[key]. If two questions have the same header (or both lack headers and share identical question text), their selection state and "Other" input values will collide, causing incorrect behavior.
Consider using the question index qi as part of the key to guarantee uniqueness.
🔧 Proposed fix
- const key = q.header ?? q.question
+ const key = `${qi}:${q.header ?? q.question}`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {questions.map((q, qi) => { | |
| const key = q.header ?? q.question | |
| const sel = controls.selected[key] ?? [] | |
| {questions.map((q, qi) => { | |
| const key = `${qi}:${q.header ?? q.question}` | |
| const sel = controls.selected[key] ?? [] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jcode-ui/src/components/AskUserCard.tsx` around lines 32 - 34, The
selection state in AskUserCard can collide when multiple questions share the
same header or question text because the current key is derived from q.header ??
q.question. Update the keying logic inside AskUserCard’s questions.map callback
to guarantee uniqueness by including the question index qi (or another stable
unique identifier), and use that same unique key consistently for both
controls.selected and controls.other so each question maintains independent
state.
| {subtitle && ( | ||
| <span className="truncate text-[var(--color-muted-foreground)]" dangerouslySetInnerHTML={{ __html: subtitle }} /> | ||
| )} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unsanitized dangerouslySetInnerHTML with subtitle — XSS risk.
tool.displayInfo?.subtitle is injected via dangerouslySetInnerHTML without DOMPurify sanitization. Unlike MarkdownBody and Reasoning (which route through renderMarkdown → DOMPurify), the subtitle bypasses sanitization entirely. The title on line 76 is safely rendered as text content, suggesting the subtitle should follow the same pattern. If the subtitle is derived from tool arguments (file paths, commands) that can be influenced by user input or prompt injection, this is an exploitable XSS vector.
🔒 Proposed fix — render subtitle as text
{subtitle && (
- <span className="truncate text-[var(--color-muted-foreground)]" dangerouslySetInnerHTML={{ __html: subtitle }} />
+ <span className="truncate text-[var(--color-muted-foreground)]">{subtitle}</span>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {subtitle && ( | |
| <span className="truncate text-[var(--color-muted-foreground)]" dangerouslySetInnerHTML={{ __html: subtitle }} /> | |
| )} | |
| {subtitle && ( | |
| <span className="truncate text-[var(--color-muted-foreground)]">{subtitle}</span> | |
| )} |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 77-77: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(react-unsafe-html-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jcode-ui/src/components/ToolCallCard.tsx` around lines 77 - 79, The
subtitle in ToolCallCard is rendered with dangerouslySetInnerHTML without
sanitization, creating an XSS risk. Update the ToolCallCard subtitle rendering
path so tool.displayInfo?.subtitle is treated as plain text like the title, or
pass it through the same safe rendering/sanitization flow used by MarkdownBody
and Reasoning before display. Keep the fix localized to the subtitle branch in
ToolCallCard and remove the raw HTML injection.
| <Link className="chatui-docs-link" to="/docs/chat-ui"> | ||
| Read the docs → | ||
| </Link> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the docs link route — /docs/chat-ui should be /chat-ui/docs.
The "Read the docs" CTA points to /docs/chat-ui, but all internal links in ChatUiDocPage.tsx use /chat-ui/docs as the base route (lines 26, 44, 69, 77). This link will navigate to the wrong route — either a 404 or the jcode product docs route with slug chat-ui, showing incorrect content.
🔗 Proposed fix
- <Link className="chatui-docs-link" to="/docs/chat-ui">
+ <Link className="chatui-docs-link" to="/chat-ui/docs">
Read the docs →
</Link>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Link className="chatui-docs-link" to="/docs/chat-ui"> | |
| Read the docs → | |
| </Link> | |
| <Link className="chatui-docs-link" to="/chat-ui/docs"> | |
| Read the docs → | |
| </Link> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@site/src/pages/ChatUIPage.tsx` around lines 69 - 71, The “Read the docs” CTA
in ChatUIPage uses the wrong route and should match the ChatUiDocPage base path.
Update the Link target in ChatUIPage so it points to /chat-ui/docs instead of
/docs/chat-ui, keeping it consistent with the internal links used by
ChatUiDocPage and avoiding navigation to the wrong docs page.
| var dark = | ||
| stored === 'dark' || | ||
| stored === 'jcode-dark' || | ||
| (stored === 'system' && matchMedia('(prefers-color-scheme: dark)').matches) | ||
| var cls = document.documentElement.classList | ||
| cls.toggle('dark', dark) | ||
| if (stored && stored !== 'system' && stored !== 'light' && stored !== 'dark') { | ||
| document.documentElement.setAttribute('data-theme', stored) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pre-mount script misses dark named themes other than jcode-dark.
The inline dark-mode check only covers 'dark' and 'jcode-dark', but midnight, dracula, and nord-dark are also dark themes (per THEMES in useTheme.ts). For those themes, dark evaluates to false, so the .dark class is not added — but data-theme IS set (line 20-22). Since style.css has :root:not(.dark) / .dark forks (per the useTheme.ts comment at line 6), users with these themes will see a flash of light-mode styles until the JS bundle loads and applyTheme() corrects the .dark class.
🐛 Proposed fix: include all dark theme IDs in the pre-mount check
var stored = localStorage.getItem('jcode_theme') || 'system'
+ var darkIds = { 'jcode-dark': 1, midnight: 1, dracula: 1, 'nord-dark': 1 }
var dark =
stored === 'dark' ||
stored === 'jcode-dark' ||
+ darkIds[stored] === 1 ||
(stored === 'system' && matchMedia('(prefers-color-scheme: dark)').matches)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var dark = | |
| stored === 'dark' || | |
| stored === 'jcode-dark' || | |
| (stored === 'system' && matchMedia('(prefers-color-scheme: dark)').matches) | |
| var cls = document.documentElement.classList | |
| cls.toggle('dark', dark) | |
| if (stored && stored !== 'system' && stored !== 'light' && stored !== 'dark') { | |
| document.documentElement.setAttribute('data-theme', stored) | |
| } | |
| var stored = localStorage.getItem('jcode_theme') || 'system' | |
| var darkIds = { 'jcode-dark': 1, midnight: 1, dracula: 1, 'nord-dark': 1 } | |
| var dark = | |
| stored === 'dark' || | |
| stored === 'jcode-dark' || | |
| darkIds[stored] === 1 || | |
| (stored === 'system' && matchMedia('(prefers-color-scheme: dark)').matches) | |
| var cls = document.documentElement.classList | |
| cls.toggle('dark', dark) | |
| if (stored && stored !== 'system' && stored !== 'light' && stored !== 'dark') { | |
| document.documentElement.setAttribute('data-theme', stored) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-react/index.html` around lines 14 - 22, The pre-mount theme script in
index.html only treats 'dark' and 'jcode-dark' as dark, so named dark themes
like midnight, dracula, and nord-dark miss the initial .dark class and flash
light styles. Update the inline dark-mode detection to use the same dark-theme
set as useTheme.ts/THEMES (or a shared dark-theme lookup) so the variable that
drives cls.toggle('dark', dark) returns true for every dark theme before mount.
| dispatch(chatActions.addMessage({ role: 'user', content: payload.text, images: payload.images })) | ||
| dispatch(chatActions.setRunning(true)) | ||
| const resp = await api.chat(payload.text, payload.mode, sessionId, payload.images) | ||
| if (!state.session.currentSessionId) dispatch(sessionActions.setCurrentSession(resp.session_id)) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
sendMessage leaves isRunning stuck on api.chat() failure.
If the HTTP call to api.chat() throws (network error, 500, etc.), isRunning is already true and there is no catch to reset it. The UI locks into a perpetual loading state with no recovery short of a page refresh.
🔒 Proposed fix: wrap api.chat in try/catch
dispatch(chatActions.addMessage({ role: 'user', content: payload.text, images: payload.images }))
dispatch(chatActions.setRunning(true))
- const resp = await api.chat(payload.text, payload.mode, sessionId, payload.images)
- if (!state.session.currentSessionId) dispatch(sessionActions.setCurrentSession(resp.session_id))
+ try {
+ const resp = await api.chat(payload.text, payload.mode, sessionId, payload.images)
+ if (!state.session.currentSessionId) dispatch(sessionActions.setCurrentSession(resp.session_id))
+ } catch (err) {
+ dispatch(chatActions.setRunning(false))
+ dispatch(chatActions.addMessage({
+ role: 'system',
+ content: err instanceof Error ? err.message : 'Failed to send message',
+ level: 'error',
+ }))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dispatch(chatActions.addMessage({ role: 'user', content: payload.text, images: payload.images })) | |
| dispatch(chatActions.setRunning(true)) | |
| const resp = await api.chat(payload.text, payload.mode, sessionId, payload.images) | |
| if (!state.session.currentSessionId) dispatch(sessionActions.setCurrentSession(resp.session_id)) | |
| }, | |
| dispatch(chatActions.addMessage({ role: 'user', content: payload.text, images: payload.images })) | |
| dispatch(chatActions.setRunning(true)) | |
| try { | |
| const resp = await api.chat(payload.text, payload.mode, sessionId, payload.images) | |
| if (!state.session.currentSessionId) dispatch(sessionActions.setCurrentSession(resp.session_id)) | |
| } catch (err) { | |
| dispatch(chatActions.setRunning(false)) | |
| dispatch(chatActions.addMessage({ | |
| role: 'system', | |
| content: err instanceof Error ? err.message : 'Failed to send message', | |
| level: 'error', | |
| })) | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-react/src/app/store.ts` around lines 398 - 402, The sendMessage flow in
store.ts leaves chatActions.setRunning(true) stuck when api.chat throws. Wrap
the api.chat call in the sendMessage handler with try/catch and reset the
running state in the failure path (and ideally in a finally block) so the UI can
recover after network or server errors. Keep the existing
dispatch(chatActions.addMessage(...)) and sessionActions.setCurrentSession logic
in the success path, but ensure chatActions.setRunning(false) is always reached
even if api.chat fails.
| export const editMessage = createAsyncThunk( | ||
| 'chat/edit', | ||
| async (payload: { id: string; text: string }, { dispatch }) => { | ||
| // Trim the timeline up to (and including) the edited message, then resend. | ||
| dispatch(chatActions.clearChat()) | ||
| await dispatch(sendMessage({ text: payload.text })) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
editMessage clears the entire timeline instead of trimming.
The comment says "Trim the timeline up to (and including) the edited message, then resend," but clearChat() wipes all conversation history. The assistant loses all context from before the edit point, which is almost certainly not the intended behavior.
🐛 Proposed fix: add a trimTimeline reducer and use it in editMessage
Add a reducer that truncates the timeline at the target message:
setTimeline(s, a: { payload: ThreadItem[] }) {
s.timeline = a.payload
},
+ trimTimeline(s, a: { payload: string }) {
+ const idx = s.timeline.findIndex(
+ (i) => i.kind === 'message' && i.data.id === a.payload,
+ )
+ if (idx !== -1) {
+ s.timeline = s.timeline.slice(0, idx)
+ }
+ streamingText = ''
+ streamingMsgId = ''
+ },Then update editMessage:
export const editMessage = createAsyncThunk(
'chat/edit',
async (payload: { id: string; text: string }, { dispatch }) => {
- // Trim the timeline up to (and including) the edited message, then resend.
- dispatch(chatActions.clearChat())
+ dispatch(chatActions.trimTimeline(payload.id))
await dispatch(sendMessage({ text: payload.text }))
},
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-react/src/app/store.ts` around lines 435 - 442, The edit flow is wiping
all chat history via chatActions.clearChat instead of trimming the timeline to
the edited message. Add a reducer on the chat slice (for example, trimTimeline)
that truncates the timeline at the target message, then update editMessage to
dispatch that reducer before resend. Use the existing editMessage thunk and
chatActions location to wire this in so only messages after the edit point are
removed.
| this.ws.onclose = () => { | ||
| this.connected = false | ||
| if (this.pingTimer) { | ||
| clearInterval(this.pingTimer) | ||
| this.pingTimer = null | ||
| } | ||
| this.ws = null | ||
| this.retryTimer = setTimeout(() => this.connect(), 3000) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
disconnect() does not prevent reconnection — onclose unconditionally schedules a retry.
disconnect() clears retryTimer and pingTimer, then calls this.ws?.close(). However, close() triggers the onclose handler asynchronously, which sets a new retryTimer = setTimeout(() => this.connect(), 3000). The client will reconnect 3 seconds after every intentional disconnect. The same issue affects connect() when called while already connected: the old connection's onclose fires and nulls this.ws (clobbering the new connection) plus schedules an extra retry.
🔒 Proposed fix: null out onclose before closing in both methods
connect(): void {
if (this.ws) {
+ this.ws.onclose = null
this.ws.close()
this.ws = null
} disconnect(): void {
if (this.retryTimer) clearTimeout(this.retryTimer)
if (this.pingTimer) clearInterval(this.pingTimer)
- this.ws?.close()
+ if (this.ws) {
+ this.ws.onclose = null
+ this.ws.close()
+ }
this.ws = null
this.connected = false
}Also applies to: 132-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-react/src/lib/ws.ts` around lines 111 - 119, The WebSocket client in
ws.ts is always scheduling a reconnect from the onclose handler, which breaks
intentional disconnects and can clobber a newer connection during reconnects.
Update the WebSocket lifecycle logic around the connect() and disconnect()
methods so that before calling close() you detach or neutralize the current
socket’s onclose handler, then clear ws/pingTimer/retryTimer only for the socket
instance being shut down. Keep the retry scheduling in onclose only for
unexpected closes, and guard against stale close events from an old socket
overriding a newer one.
…, copy icons, welcome page, theme colors (#124)
Summary
Fixes the detailed UI-parity issues vs the Vue app. Each fix was compared against the Vue source line-by-line before implementing.
Issues fixed (from feedback)
Verification (headless Chrome)
🤖 Generated with ZCode
Summary by CodeRabbit
New Features
Bug Fixes