Wire Chat Workspace inspector and status rails#2600
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the runtime state for the Chat Workspace status and inspector rails, addressing GitHub issue #2033. It introduces a sendError state to handle failed sends, tracks workspace readiness, and replaces placeholder panels in the inspector rail with real runtime and recovery copy. Corresponding unit and E2E tests have been added or updated. However, a critical issue was found where selectedModelLabel is used in InspectorRail but is neither defined in its props nor passed to it from ChatWorkspaceConsole, which will prevent the model recovery copy from rendering correctly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| backendAvailable={backendAvailable} | ||
| workspaceReady={workspaceReady} | ||
| streaming={streaming} | ||
| sendError={sendError} |
There was a problem hiding this comment.
The selectedModelLabel prop is not being passed to InspectorRail. Inside InspectorRail, selectedModelLabel is used to determine modelRecoveryCopy (e.g., showing "Choose a model before sending." when no model is selected). Without passing this prop, selectedModelLabel will be undefined at runtime, and the recovery copy will never be displayed.
Please pass selectedModelLabel={selectedModelLabel} to <InspectorRail />.
sendError={sendError}
selectedModelLabel={selectedModelLabel}
PR Summary by QodoWire Chat Workspace inspector/status rails to real runtime state
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
Context used✅ Tickets:
🎫 /chat-workspace: live backend chat flow and scoped persistence 🎫 /chat-workspace: complete source rail and staged-context lifecycle 🎫 /chat-workspace: harden responsive layout and accessibility +2 more✅ Compliance rules (platform):
74 rules 1. Stop test lacks halt assertion
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| await expect( | ||
| page.getByLabel("Chat workspace status").getByText("Streaming") | ||
| ).toBeVisible() | ||
| await expect( | ||
| page | ||
| .getByRole("complementary", { name: "Chat workspace inspector" }) | ||
| .getByText("Streaming") | ||
| ).toBeVisible() |
There was a problem hiding this comment.
1. Stop test lacks halt assertion 📎 Requirement gap ☼ Reliability
The Playwright test for stop-generation only asserts the stop button becomes hidden, but it does not verify that streaming output/tokens stop and the UI stops updating. This does not meet the compliance requirement to prove generation halts during active streaming.
Agent Prompt
## Issue description
The `/chat-workspace` Playwright test `shows stop generation while the workspace stream is active` clicks `Stop generating`, but it only checks the button becomes hidden and does not prove that generation actually halts (no further streamed content/updates).
## Issue Context
Compliance requires an explicit verification that stopping generation halts streaming updates, not just that a control disappeared.
## Fix Focus Areas
- apps/tldw-frontend/e2e/smoke/chat-workspace-live-backend.spec.ts[660-695]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export const WorkspaceStatusStrip = ({ | ||
| backendAvailable, | ||
| workspaceReady = true, | ||
| streaming, | ||
| stagedSourceCount | ||
| sendError, | ||
| stagedSourceCount, | ||
| selectedModelLabel, | ||
| selectedPersonaLabel, | ||
| assistantSource | ||
| }: WorkspaceStatusStripProps) => { |
There was a problem hiding this comment.
2. Unsafe workspaceready default 🐞 Bug ≡ Correctness
InspectorRail and WorkspaceStatusStrip default workspaceReady to true while keeping it optional, so any caller omission will silently render “Ready” even if workspace identity is still hydrating. This is a correctness footgun that can mask missing wiring and make the rails disagree with the actual send guard logic.
Agent Prompt
### Issue description
`workspaceReady` is optional and defaults to `true` in both `WorkspaceStatusStrip` and `InspectorRail`. If a future refactor or alternate call site omits the prop, the UI will incorrectly show a hydrated/ready state.
### Issue Context
These components are currently only used from `ChatWorkspaceConsole`, which *does* pass `workspaceReady`, but the component APIs now allow silent misrendering.
### Fix Focus Areas
- apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceStatusStrip.tsx[4-13]
- apps/packages/ui/src/components/Option/ChatWorkspace/WorkspaceStatusStrip.tsx[37-46]
- apps/packages/ui/src/components/Option/ChatWorkspace/InspectorRail.tsx[10-22]
- apps/packages/ui/src/components/Option/ChatWorkspace/InspectorRail.tsx[92-104]
### Suggested change
- Make `workspaceReady` required (`workspaceReady: boolean`) in both prop types.
- Remove the default initializer (`workspaceReady = true`) so TS forces callers to pass the real readiness.
- (Alternative if you truly need optional) default to `false` rather than `true` so missing wiring degrades safely to “Loading workspace context.”
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Summary
Test Plan
bunx vitest run src/components/Option/ChatWorkspace/__tests__ --maxWorkers=1npx playwright test e2e/smoke/chat-workspace-live-backend.spec.ts --project=chromiumnpx playwright test e2e/smoke/stage5-release-gate.spec.ts --project=chromium --grep "Chat Workspace"git diff --checkTypecheck Note
NODE_OPTIONS=--max-old-space-size=8192 bunx tsc --noEmit -p tsconfig.jsonstill fails on unrelated existing test type errors outside this touched scope:ChatGreetingPickergreetingSelectionChecksum,background-session-storequickIngestBatches,TldwChat.abortspread args, andcharacter-exporttuple cast.Tracking
Closes #2033.
Refs #1239.
Merge Gate
AI-authored PR: maintainer needs to add the required human-written Change summary before merge.
Summary by cubic
Wired the Chat Workspace inspector and status rails to real runtime state (backend, workspace hydration, streaming, send failures) with clear recovery hints. Replaces placeholder panels and surfaces streaming/failure in the UI and tests.
sendErrortoChatWorkspaceRuntimeState;WorkspaceChatPanelreports it and it’s lifted viaChatWorkspacePage/ChatWorkspaceConsole.WorkspaceStatusStripshows precedence: server unavailable → loading workspace context → send failed → streaming → ready; adds pills for reconnect, wait for workspace identity, select a model, no persona, and context staged.InspectorRailnow shows runtime label and recovery copy; adds model/persona guidance; removed “Approvals” and “Task Progress” placeholders.Written for commit b29bb0a. Summary will update on new commits.