feat: scope workspace context by top-level agent#123
Merged
daggerhashimoto merged 26 commits intomasterfrom Mar 20, 2026
Merged
feat: scope workspace context by top-level agent#123daggerhashimoto merged 26 commits intomasterfrom
daggerhashimoto merged 26 commits intomasterfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds per-agent workspace scoping across server and frontend: agent ID normalization and workspace resolution, per-workspace file ops and watchers, agent-aware server routes, and frontend UI/state changes to persist and guard workspace-scoped data. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (UI)
participant Route as Server Route
participant WS as AgentWorkspace
participant FileOps as FileOps
participant FS as File System
Client->>Route: GET /api/files/tree?agentId=research
Route->>WS: resolveAgentWorkspace('research')
WS-->>Route: { agentId:'research', workspaceRoot, memoryPath, memoryDir }
Route->>FileOps: getTree(workspaceRoot)
FileOps->>FS: readdir(workspaceRoot)
FS-->>FileOps: entries
FileOps-->>Route: tree
Route-->>Client: { tree, workspaceInfo }
sequenceDiagram
participant App as App Component
participant OpenFiles as useOpenFiles
participant Dialog as WorkspaceSwitchDialog
participant FileTree as FileTreePanel
App->>OpenFiles: query hasDirtyFiles('main')
OpenFiles-->>App: true
App->>FileTree: requestWorkspaceTransition('research')
App->>App: shouldGuardWorkspaceSwitch() -> true
App->>Dialog: show
Dialog->>App: onSaveAndSwitch()
App->>OpenFiles: saveAllDirtyFiles('main')
OpenFiles-->>App: ok
App->>FileTree: switch workspaceAgentId -> 'research'
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
daggerhashimoto
force-pushed
the
feat/agent-scoped-workspace-context
branch
from
2ed9994 to
a147750
Compare
daggerhashimoto
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/features/memory/useMemories.ts (1)
61-79:⚠️ Potential issue | 🟠 MajorReset local memory state on agent switches.
This reconciliation path assumes every
initialMemorieschange is a same-agent refresh. WhenagentIdchanges, it can carry optimistic rows from the previous agent into the new list, and it can also skip the sync entirely if both agents reuse the sameinitialMemoriesreference. Key this merge byagentId, or hard-resetmemories/prevInitialRefwhen the active agent changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/memory/useMemories.ts` around lines 61 - 79, The reconciliation currently merges pending/optimistic rows into incoming initialMemories without accounting for agent switches; update the effect in useMemories to also be keyed by agentId so a change of agent either hard-resets prevInitialRef.current and memories (call setMemories(initialMemories)) or uses agentId as part of the equality check before merging. Concretely, reference prevInitialRef, initialMemories, setMemories and agentId: add agentId to the effect dependencies, detect if prevAgentIdRef/currentAgentId differs and if so clear prevInitialRef.current and reset memories to initialMemories; otherwise continue the existing merge logic but only merge pending items when agentId matches.src/features/sessions/SpawnAgentDialog.tsx (1)
23-28:⚠️ Potential issue | 🟡 MinorMove
FALLBACK_MODELSto a separate file to fix the ESLint error.Exporting the constant alongside the component breaks React Fast Refresh, which requires component files to only export components. This is causing the pipeline failure.
Proposed fix
Create a new file
src/features/sessions/fallbackModels.ts:import type { InlineSelectOption } from '@/components/ui/InlineSelect'; export const FALLBACK_MODELS: InlineSelectOption[] = [ { value: 'openai-codex/gpt-5.4', label: 'gpt-5.4' }, { value: 'anthropic/claude-haiku-4-5', label: 'claude-haiku-4-5' }, { value: 'anthropic/claude-sonnet-4-5', label: 'claude-sonnet-4-5' }, { value: 'anthropic/claude-opus-4-6', label: 'claude-opus-4-6' }, ];Then in
SpawnAgentDialog.tsx:-export const FALLBACK_MODELS: InlineSelectOption[] = [ - { value: 'openai-codex/gpt-5.4', label: 'gpt-5.4' }, - { value: 'anthropic/claude-haiku-4-5', label: 'claude-haiku-4-5' }, - { value: 'anthropic/claude-sonnet-4-5', label: 'claude-sonnet-4-5' }, - { value: 'anthropic/claude-opus-4-6', label: 'claude-opus-4-6' }, -]; +import { FALLBACK_MODELS } from './fallbackModels';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/sessions/SpawnAgentDialog.tsx` around lines 23 - 28, Move the FALLBACK_MODELS constant out of the component file into a new module and import it back: create a new file exporting FALLBACK_MODELS (typed as InlineSelectOption[]) — e.g., fallbackModels.ts exporting the constant — then remove the export of FALLBACK_MODELS from SpawnAgentDialog.tsx and replace the local constant with an import (import { FALLBACK_MODELS } from './fallbackModels'); ensure SpawnAgentDialog.tsx only exports React components to satisfy Fast Refresh/ESLint.
🧹 Nitpick comments (6)
server/routes/gateway.test.ts (1)
128-129: Consider asserting runtime wiring, not just exported constant.This can still pass if
MODEL_LIST_TIMEOUT_MSis exported correctly but not used inexecFileoptions. Consider hitting/api/gateway/modelsand asserting the capturedopts.timeout === mod.MODEL_LIST_TIMEOUT_MS.Suggested test hardening
- const mod = await import('./gateway.js'); - expect(mod.MODEL_LIST_TIMEOUT_MS).toBeGreaterThanOrEqual(15_000); + let capturedTimeout: number | undefined; + execFileImpl = (_bin: unknown, _args: unknown, opts: unknown, cb: unknown) => { + capturedTimeout = (opts as { timeout?: number }).timeout; + (cb as (err: null, stdout: string) => void)(null, GOOD_MODELS); + }; + + const mod = await import('./gateway.js'); + const app = new Hono(); + app.route('/', mod.default); + const res = await app.request('/api/gateway/models'); + expect(res.status).toBe(200); + expect(mod.MODEL_LIST_TIMEOUT_MS).toBeGreaterThanOrEqual(15_000); + expect(capturedTimeout).toBe(mod.MODEL_LIST_TIMEOUT_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/gateway.test.ts` around lines 128 - 129, The test currently only checks the exported constant MODEL_LIST_TIMEOUT_MS but not that it's actually passed into child_process.execFile; update the test to import the module (mod.MODEL_LIST_TIMEOUT_MS), stub/mock child_process.execFile (capture the opts argument) and then perform an HTTP request to the route '/api/gateway/models' (or invoke the gateway handler) so you can assert that the captured opts.timeout === mod.MODEL_LIST_TIMEOUT_MS; reference the execFile call and the MODEL_LIST_TIMEOUT_MS constant when adding the assertion.server/lib/file-ops.ts (1)
368-370: Clarify intent:toWorkspaceRelativereturn value discarded.Line 369 calls
toWorkspaceRelative(targetDirAbs, workspaceRoot)but discards the return value. This appears intentional for validation (throws if path escapes workspace), but a brief comment would clarify the side-effect-only usage.📝 Proposed comment for clarity
const targetDirAbs = params.targetDirAbs || workspaceRoot; + // Validate targetDirAbs is within workspace (throws if invalid) toWorkspaceRelative(targetDirAbs, workspaceRoot);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/file-ops.ts` around lines 368 - 370, Call out that toWorkspaceRelative(targetDirAbs, workspaceRoot) is intentionally invoked for its validation/side-effect (throws if path escapes workspace) and its return value is intentionally discarded; add a brief inline comment above that call in file-ops.ts explaining this intent and referencing the variables targetDirAbs and workspaceRoot so future readers know the call is for validation only, not for its return value.server/routes/workspace.test.ts (1)
7-99: Consider adding a negative test for invalidagentId.The suite covers happy paths well. Consider adding a test verifying that an invalid
agentId(e.g., containing path traversal characters) returns a 400 error, which would exercise theInvalidAgentIdErrorhandling added in the route.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/workspace.test.ts` around lines 7 - 99, Add a negative test in the same suite that uses buildApp() and app.request() to call a workspace route (e.g., GET /api/workspace/tools or GET /api/workspace) with an invalid agentId containing path-traversal characters (for example "../bad" or "research/..") and assert the response status is 400 and the JSON indicates failure; this will exercise InvalidAgentIdError handling in the workspace route handler and ensure malformed agentId inputs are rejected.src/features/memory/ConfirmDeleteDialog.tsx (1)
53-83: Consider aborting in-flight fetch whenagentIdchanges.If the user switches agents while a section fetch is in progress, the stale response could update state after the component has re-rendered for a different agent. Consider using an
AbortControlleror a request ID to ignore outdated responses.🛡️ Proposed fix with AbortController
useEffect(() => { if (!open || (!isSection && !isDaily) || !memoryText) { setMarkdownContent(null); return; } + const controller = new AbortController(); + const fetchContent = async () => { setContentLoading(true); try { const params = new URLSearchParams({ title: memoryText, agentId }); if (memoryDate) { params.set('date', memoryDate); } - const res = await fetch(`/api/memories/section?${params.toString()}`); + const res = await fetch(`/api/memories/section?${params.toString()}`, { + signal: controller.signal, + }); + if (controller.signal.aborted) return; const data = await res.json(); if (data.ok && data.content) { setMarkdownContent(data.content); } else { setMarkdownContent(null); } - } catch { - setMarkdownContent(null); + } catch (err) { + if (err instanceof Error && err.name !== 'AbortError') { + setMarkdownContent(null); + } } finally { - setContentLoading(false); + if (!controller.signal.aborted) { + setContentLoading(false); + } } }; fetchContent(); + + return () => controller.abort(); }, [agentId, open, isSection, isDaily, memoryText, memoryDate]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/memory/ConfirmDeleteDialog.tsx` around lines 53 - 83, The in-flight fetch in the useEffect (fetchContent) can update state after agentId changes; fix by creating an AbortController inside the effect, pass controller.signal to the fetch call within fetchContent, and on effect cleanup call controller.abort() so stale requests are cancelled; also handle AbortError in the catch path before calling setMarkdownContent/setContentLoading to avoid updating state for aborted requests (references: useEffect, fetchContent, setMarkdownContent, setContentLoading).src/hooks/useDashboardData.ts (1)
122-129: Type mismatch:eventAgentIdmay beundefinedbutFileChangedHandlerexpectsstring.On Line 127,
eventAgentIdis typed asstring | undefined, butFileChangedHandler(Line 23) expects(path: string, agentId: string) => void. WheneventAgentId === agentIdRef.currentis true andagentIdRef.currentis'main',eventAgentIdmust also be'main'(a string), so runtime behavior is correct. However, TypeScript won't infer this narrowing.🔧 Proposed fix to satisfy type checker
if (data?.path && eventAgentId === agentIdRef.current) { - onFileChangedRef.current?.(data.path, eventAgentId); + onFileChangedRef.current?.(data.path, agentIdRef.current); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useDashboardData.ts` around lines 122 - 129, The variable eventAgentId is typed as string | undefined so TypeScript complains when passing it to FileChangedHandler (which requires a string); update the call site inside the check that already ensures eventAgentId === agentIdRef.current to convince the compiler by narrowing or asserting non-null: after confirming data?.path && eventAgentId === agentIdRef.current, call onFileChangedRef.current with a non-null string (for example use a non-null assertion or cast: pass eventAgentId! or eventAgentId as string, or assign const confirmedAgentId = eventAgentId; and pass confirmedAgentId) so the types match; refer to eventAgentId, agentIdRef.current, and onFileChangedRef.current/FileChangedHandler to locate the code to change.server/routes/memories.ts (1)
141-147: Consider using a typed discriminated union or throwing for cleaner control flow.The
resolveWorkspaceOrResponsehelper returningResponse | AgentWorkspaceworks but requiresinstanceof Responsechecks at every call site. An alternative would be to throw and let a Hono error handler return the 400, or use a discriminated union.That said, the current approach is explicit and the pattern is used consistently throughout the file, so this is a minor readability preference rather than an issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/memories.ts` around lines 141 - 147, The helper resolveWorkspaceOrResponse currently returns Response | AgentWorkspace which forces instanceof Response checks; change it to throw a Hono error (e.g., throw new Error or HTTPError) on invalid agentId and let the global error handler return 400, or refactor to a discriminated union like { success: true, workspace: AgentWorkspace } | { success: false, response: Response } and update call sites (where resolveWorkspaceOrResponse is used) to handle the single typed branch (either catch the thrown error or check the success flag) instead of instanceof checks; modify resolveWorkspaceOrResponse and all callers (and any error middleware) accordingly to maintain behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/lib/file-watcher.ts`:
- Around line 44-49: The watcher only monitors the main workspace and tags every
event with agentId: 'main'; update startFileWatcher to watch all per-agent
workspace directories (e.g., iterate existing workspace-<agentId> folders and/or
watch the parent dir with recursive handling) and change the emitters
(broadcastMainWorkspaceFileChanged, broadcastMemoryChanged and any other
file/memory emitters referenced) to determine the correct agentId from the
changed filePath (parse the workspace-<agentId> segment or map file paths to
agent IDs) and include that agentId in the broadcast payloads instead of
hardcoding 'main'; also make the broadcast functions generic (or add a parameter
agentId) so events for non-main agents produce memory.changed/file.changed for
the right agent.
In `@src/App.tsx`:
- Around line 524-525: Remove the unused eslint-disable comment that precedes
the setLogGlow call: delete the line "// eslint-disable-next-line
react-hooks/set-state-in-effect -- transient log glow is intentional UI feedback
from external log updates" so only the statement setLogGlow(true); remains;
ensure no other unused eslint-disable directives related to
react-hooks/set-state-in-effect are left in the surrounding function (the
setLogGlow call) so CI lint warnings are resolved.
In `@src/features/file-browser/hooks/useOpenFiles.ts`:
- Around line 444-475: The async file-read staleness check currently only
compares agentIdRef.current to requestAgentId, which allows old responses to
overwrite newer state; introduce a monotonically increasing request epoch/token
(e.g., requestEpochRef or requestToken) generated when starting a read and pass
it alongside requestAgentId, then validate both agentIdRef.current ===
requestAgentId AND requestEpochRef.current === requestEpoch (or token) before
applying results in the try and catch branches; apply the same epoch/token check
in reloadFile and any other read paths that use requestAgentId so only the
latest request for that workspace/agent can mutate setOpenFiles.
- Around line 685-697: The visible-state branch is using agentIdRef.current to
decide remap/close but should use the owner of the open-files state; change the
conditional to compare the requestAgentId against the open-files state's owner
id (e.g. openFiles.owner or the owner id value held by this hook) instead of
agentIdRef.current, and apply the same change in the other helper (the similar
block around the closeOpenPathsByPrefix/remap helper). Update the checks that
gate loadPersistedFiles/persistFilePaths and loadPersistedTab/persistTab so they
run only when requestAgentId differs from the open-files state owner id, keeping
the existing calls to matchesPathPrefix, remapPathPrefix, loadPersistedFiles,
persistFilePaths, loadPersistedTab and persistTab unchanged.
- Around line 299-384: restorePersistedFiles currently snapshots
persistedPaths/persistedTab and then may overwrite newer target-agent changes
when it commits; capture the ownership and live open-files state at start and
validate/merge before writing: record const startOwner =
stateOwnerAgentIdRef.current and get a liveOpenFilesRef (or use the current
setOpenFiles ref) when you begin, and before calling setOpenFiles/setActiveTab
check if restoringAgentIdRef.current!==targetAgentId ||
stateOwnerAgentIdRef.current!==startOwner — if so abort/restart the restore;
additionally, when ownsTargetState is false, derive baseFiles from the current
live open-files (instead of using []), e.g., replace const baseFiles =
ownsTargetState ? prev : [] with baseFiles = ownsTargetState ? prev :
liveOpenFilesRef.current, then call
mergeRestoredFiles/rememberDirtyFiles/persistFiles as before so you merge
against the latest draft rather than clobbering newer changes.
In `@src/features/memory/useMemories.ts`:
- Around line 81-90: The cleanup currently aborts refresh and timers but does
not prevent resolved async mutations (addMemory, deleteMemory) or background
refresh calls from updating state after agentId changes; add a generation/agent
guard similar to useWorkspaceFile: maintain a generationRef (increment it inside
the useEffect cleanup that runs on agentId change), capture const gen =
generationRef.current at the start of every async operation (addMemory,
deleteMemory, refresh and their post-await callbacks), and before calling
setMemories, setError, or enqueue refresh(), check that generationRef.current
=== gen (and that refreshAbortRef.current is not aborted). Apply this pattern
inside the functions named addMemory, deleteMemory, refresh, and any other async
handlers referenced in the hook so late resolutions from the previous agent are
ignored.
---
Outside diff comments:
In `@src/features/memory/useMemories.ts`:
- Around line 61-79: The reconciliation currently merges pending/optimistic rows
into incoming initialMemories without accounting for agent switches; update the
effect in useMemories to also be keyed by agentId so a change of agent either
hard-resets prevInitialRef.current and memories (call
setMemories(initialMemories)) or uses agentId as part of the equality check
before merging. Concretely, reference prevInitialRef, initialMemories,
setMemories and agentId: add agentId to the effect dependencies, detect if
prevAgentIdRef/currentAgentId differs and if so clear prevInitialRef.current and
reset memories to initialMemories; otherwise continue the existing merge logic
but only merge pending items when agentId matches.
In `@src/features/sessions/SpawnAgentDialog.tsx`:
- Around line 23-28: Move the FALLBACK_MODELS constant out of the component file
into a new module and import it back: create a new file exporting
FALLBACK_MODELS (typed as InlineSelectOption[]) — e.g., fallbackModels.ts
exporting the constant — then remove the export of FALLBACK_MODELS from
SpawnAgentDialog.tsx and replace the local constant with an import (import {
FALLBACK_MODELS } from './fallbackModels'); ensure SpawnAgentDialog.tsx only
exports React components to satisfy Fast Refresh/ESLint.
---
Nitpick comments:
In `@server/lib/file-ops.ts`:
- Around line 368-370: Call out that toWorkspaceRelative(targetDirAbs,
workspaceRoot) is intentionally invoked for its validation/side-effect (throws
if path escapes workspace) and its return value is intentionally discarded; add
a brief inline comment above that call in file-ops.ts explaining this intent and
referencing the variables targetDirAbs and workspaceRoot so future readers know
the call is for validation only, not for its return value.
In `@server/routes/gateway.test.ts`:
- Around line 128-129: The test currently only checks the exported constant
MODEL_LIST_TIMEOUT_MS but not that it's actually passed into
child_process.execFile; update the test to import the module
(mod.MODEL_LIST_TIMEOUT_MS), stub/mock child_process.execFile (capture the opts
argument) and then perform an HTTP request to the route '/api/gateway/models'
(or invoke the gateway handler) so you can assert that the captured opts.timeout
=== mod.MODEL_LIST_TIMEOUT_MS; reference the execFile call and the
MODEL_LIST_TIMEOUT_MS constant when adding the assertion.
In `@server/routes/memories.ts`:
- Around line 141-147: The helper resolveWorkspaceOrResponse currently returns
Response | AgentWorkspace which forces instanceof Response checks; change it to
throw a Hono error (e.g., throw new Error or HTTPError) on invalid agentId and
let the global error handler return 400, or refactor to a discriminated union
like { success: true, workspace: AgentWorkspace } | { success: false, response:
Response } and update call sites (where resolveWorkspaceOrResponse is used) to
handle the single typed branch (either catch the thrown error or check the
success flag) instead of instanceof checks; modify resolveWorkspaceOrResponse
and all callers (and any error middleware) accordingly to maintain behavior.
In `@server/routes/workspace.test.ts`:
- Around line 7-99: Add a negative test in the same suite that uses buildApp()
and app.request() to call a workspace route (e.g., GET /api/workspace/tools or
GET /api/workspace) with an invalid agentId containing path-traversal characters
(for example "../bad" or "research/..") and assert the response status is 400
and the JSON indicates failure; this will exercise InvalidAgentIdError handling
in the workspace route handler and ensure malformed agentId inputs are rejected.
In `@src/features/memory/ConfirmDeleteDialog.tsx`:
- Around line 53-83: The in-flight fetch in the useEffect (fetchContent) can
update state after agentId changes; fix by creating an AbortController inside
the effect, pass controller.signal to the fetch call within fetchContent, and on
effect cleanup call controller.abort() so stale requests are cancelled; also
handle AbortError in the catch path before calling
setMarkdownContent/setContentLoading to avoid updating state for aborted
requests (references: useEffect, fetchContent, setMarkdownContent,
setContentLoading).
In `@src/hooks/useDashboardData.ts`:
- Around line 122-129: The variable eventAgentId is typed as string | undefined
so TypeScript complains when passing it to FileChangedHandler (which requires a
string); update the call site inside the check that already ensures eventAgentId
=== agentIdRef.current to convince the compiler by narrowing or asserting
non-null: after confirming data?.path && eventAgentId === agentIdRef.current,
call onFileChangedRef.current with a non-null string (for example use a non-null
assertion or cast: pass eventAgentId! or eventAgentId as string, or assign const
confirmedAgentId = eventAgentId; and pass confirmedAgentId) so the types match;
refer to eventAgentId, agentIdRef.current, and
onFileChangedRef.current/FileChangedHandler to locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b151ba3c-163b-4de5-ab36-3a214abf9c29
📒 Files selected for processing (53)
server/lib/agent-workspace.test.tsserver/lib/agent-workspace.tsserver/lib/file-ops.tsserver/lib/file-utils.tsserver/lib/file-watcher.tsserver/routes/file-browser.test.tsserver/routes/file-browser.tsserver/routes/gateway.test.tsserver/routes/gateway.tsserver/routes/memories.test.tsserver/routes/memories.tsserver/routes/skills.test.tsserver/routes/skills.tsserver/routes/workspace.test.tsserver/routes/workspace.tssrc/App.test.tsxsrc/App.tsxsrc/components/WorkspaceSwitchDialog.tsxsrc/features/dashboard/MemoryList.test.tsxsrc/features/dashboard/MemoryList.tsxsrc/features/file-browser/FileTreePanel.test.tsxsrc/features/file-browser/FileTreePanel.tsxsrc/features/file-browser/ImageViewer.tsxsrc/features/file-browser/TabbedContentArea.tsxsrc/features/file-browser/hooks/useFileTree.test.tssrc/features/file-browser/hooks/useFileTree.tssrc/features/file-browser/hooks/useOpenFiles.test.tssrc/features/file-browser/hooks/useOpenFiles.tssrc/features/file-browser/index.tssrc/features/memory/ConfirmDeleteDialog.tsxsrc/features/memory/MemoryEditor.test.tsxsrc/features/memory/MemoryEditor.tsxsrc/features/memory/useMemories.tssrc/features/sessions/SessionList.tsxsrc/features/sessions/SpawnAgentDialog.test.tsxsrc/features/sessions/SpawnAgentDialog.tsxsrc/features/workspace/WorkspacePanel.test.tsxsrc/features/workspace/WorkspacePanel.tsxsrc/features/workspace/hooks/useSkills.test.tssrc/features/workspace/hooks/useSkills.tssrc/features/workspace/hooks/useWorkspaceFile.test.tssrc/features/workspace/hooks/useWorkspaceFile.tssrc/features/workspace/persistedDrafts.tssrc/features/workspace/tabs/ConfigTab.test.tsxsrc/features/workspace/tabs/ConfigTab.tsxsrc/features/workspace/tabs/MemoryTab.tsxsrc/features/workspace/tabs/SkillsTab.tsxsrc/features/workspace/workspaceScope.test.tssrc/features/workspace/workspaceScope.tssrc/features/workspace/workspaceSwitchGuard.test.tssrc/features/workspace/workspaceSwitchGuard.tssrc/hooks/useDashboardData.test.tssrc/hooks/useDashboardData.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/features/memory/useMemories.test.ts (2)
49-53: AssertagentIdin POST/DELETE payloads to lock in scoping guarantees.These mocks currently match method/path only. Adding body assertions will directly protect the per-agent request contract introduced by this PR.
Proposed assertion additions
vi.mocked(fetch).mockImplementation(async (input, init) => { const url = getRequestUrl(input); if (url.pathname === '/api/memories' && init?.method === 'POST') { + const body = JSON.parse(String(init.body ?? '{}')); + expect(body.agentId).toBe('main'); return addRequest.promise; }vi.mocked(fetch).mockImplementation(async (input, init) => { const url = getRequestUrl(input); if (url.pathname === '/api/memories' && init?.method === 'DELETE') { + const body = JSON.parse(String(init.body ?? '{}')); + expect(body.agentId).toBe('main'); return deleteRequest.promise; }Also applies to: 140-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/memory/useMemories.test.ts` around lines 49 - 53, Update the mocked fetch implementations in useMemories.test.ts (the vi.mocked(fetch) stubs that use getRequestUrl and addRequest) to parse init.body as JSON and assert that payload.agentId equals the expected agent id for both POST and DELETE flows; specifically, inside the branch that checks url.pathname === '/api/memories' and init?.method === 'POST' (and the analogous DELETE branch around lines 140-143), decode init.body, validate the agentId field, and only then return addRequest.promise (or the delete promise) so the tests enforce the per-agent scoping contract introduced by this PR.
55-56: Fail fast on unexpected fetches instead of returning synthetic 500 responses.These fallback branches can mask accidental extra network calls when the hook catches errors. Throwing makes the test fail immediately and keeps failures diagnostic.
Proposed test hardening
- return createJsonResponse({ ok: false, error: 'Unexpected request' }, { ok: false, status: 500 }); + throw new Error(`Unexpected request: ${url.pathname}`);Also applies to: 101-102, 146-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/memory/useMemories.test.ts` around lines 55 - 56, Tests in useMemories.test.ts currently swallow unexpected fetches by returning createJsonResponse({ ok: false, error: 'Unexpected request' }, { ok: false, status: 500 }) which masks accidental extra network calls; change those fallback branches to throw an error instead (e.g., throw new Error('Unexpected fetch in test') or include details) so the test fails immediately. Locate the fallback usages of createJsonResponse in this test file (the instances around the mock fetch handlers) and replace the synthetic 500 return with a thrown Error that clearly identifies the unexpected request and any request info available.src/features/file-browser/hooks/useOpenFiles.test.ts (3)
7-13: PartialResponsemock may cause issues if tests access unmocked properties.The mock only implements
ok,status, andjson. If the hook or future tests access otherResponseproperties (e.g.,headers,text(),blob()), tests will fail with undefined access. Consider adding atextmethod or documenting this limitation.🔧 Optional: Add more complete Response mock
function createJsonResponse(data: unknown, init?: { ok?: boolean; status?: number }): Response { return { ok: init?.ok ?? true, status: init?.status ?? 200, json: async () => data, + text: async () => JSON.stringify(data), + headers: new Headers({ 'content-type': 'application/json' }), } as Response; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/file-browser/hooks/useOpenFiles.test.ts` around lines 7 - 13, The createJsonResponse helper returns a partial Response that only implements ok, status, and json, which can break tests that access other Response members; update createJsonResponse to provide common Response members used in tests—at minimum add an async text() that returns JSON.stringify(data) and a headers object (or a simple get method) and ensure other commonly used methods like blob() or arrayBuffer() are stubbed or documented; locate the createJsonResponse function in useOpenFiles.test.ts and extend its returned object with these additional properties/methods (or add a comment stating the mock's limitations) so future tests won't fail on undefined properties.
2044-2048: Type assertion exposes internal API signature not reflected in public types.The cast
(result.current.handleFileChanged as unknown as (path: string, targetAgentId?: string) => void)suggestshandleFileChangedaccepts an optionaltargetAgentIdparameter that isn't exposed in its TypeScript signature. This makes the test brittle if the internal signature changes, and suggests the public types may be incomplete.Consider either:
- Exposing the
targetAgentIdparameter in the public type if it's intentionally part of the API.- Documenting why this internal API is being tested directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/file-browser/hooks/useOpenFiles.test.ts` around lines 2044 - 2048, The test is using a type assertion to call an internal overload of handleFileChanged with a targetAgentId, exposing a signature not present in the public types; either remove the cast and exercise handleFileChanged using only the public parameters, or update the public type/signature for handleFileChanged to include the optional targetAgentId so tests can call it directly; locate the test in useOpenFiles.test.ts and adjust the call to result.current.handleFileChanged (or the exported handleFileChanged type in the file-browser hook) to match the chosen approach and keep types consistent.
1577-1577: Minor: Extra blank line.There's an extra blank line between tests that's inconsistent with the rest of the file.
🧹 Remove extra blank line
}); - it('keeps own-save bounce-back suppressed after switching away and back before save resolves', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/file-browser/hooks/useOpenFiles.test.ts` at line 1577, Remove the stray blank line in the test file src/features/file-browser/hooks/useOpenFiles.test.ts that breaks consistency between test cases: open the useOpenFiles test suite (the describe/it blocks for the useOpenFiles hook) and delete the extra empty line between the two adjacent tests so the tests follow the same spacing/formatting as the rest of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/file-browser/hooks/useOpenFiles.test.ts`:
- Around line 2195-2204: The Map type for readQueues is too strict: Map<string,
Response[] | Promise<Response>[]> implies each array is homogeneous, but the
arrays actually mix Response and Promise<Response> entries. Change the generic
to Map<string, (Response | Promise<Response>)[]> (or Array<Response |
Promise<Response>>) for readQueues so each array element can be either a
Response or a Promise<Response>; update the readQueues declaration accordingly
where it’s defined.
---
Nitpick comments:
In `@src/features/file-browser/hooks/useOpenFiles.test.ts`:
- Around line 7-13: The createJsonResponse helper returns a partial Response
that only implements ok, status, and json, which can break tests that access
other Response members; update createJsonResponse to provide common Response
members used in tests—at minimum add an async text() that returns
JSON.stringify(data) and a headers object (or a simple get method) and ensure
other commonly used methods like blob() or arrayBuffer() are stubbed or
documented; locate the createJsonResponse function in useOpenFiles.test.ts and
extend its returned object with these additional properties/methods (or add a
comment stating the mock's limitations) so future tests won't fail on undefined
properties.
- Around line 2044-2048: The test is using a type assertion to call an internal
overload of handleFileChanged with a targetAgentId, exposing a signature not
present in the public types; either remove the cast and exercise
handleFileChanged using only the public parameters, or update the public
type/signature for handleFileChanged to include the optional targetAgentId so
tests can call it directly; locate the test in useOpenFiles.test.ts and adjust
the call to result.current.handleFileChanged (or the exported handleFileChanged
type in the file-browser hook) to match the chosen approach and keep types
consistent.
- Line 1577: Remove the stray blank line in the test file
src/features/file-browser/hooks/useOpenFiles.test.ts that breaks consistency
between test cases: open the useOpenFiles test suite (the describe/it blocks for
the useOpenFiles hook) and delete the extra empty line between the two adjacent
tests so the tests follow the same spacing/formatting as the rest of the file.
In `@src/features/memory/useMemories.test.ts`:
- Around line 49-53: Update the mocked fetch implementations in
useMemories.test.ts (the vi.mocked(fetch) stubs that use getRequestUrl and
addRequest) to parse init.body as JSON and assert that payload.agentId equals
the expected agent id for both POST and DELETE flows; specifically, inside the
branch that checks url.pathname === '/api/memories' and init?.method === 'POST'
(and the analogous DELETE branch around lines 140-143), decode init.body,
validate the agentId field, and only then return addRequest.promise (or the
delete promise) so the tests enforce the per-agent scoping contract introduced
by this PR.
- Around line 55-56: Tests in useMemories.test.ts currently swallow unexpected
fetches by returning createJsonResponse({ ok: false, error: 'Unexpected request'
}, { ok: false, status: 500 }) which masks accidental extra network calls;
change those fallback branches to throw an error instead (e.g., throw new
Error('Unexpected fetch in test') or include details) so the test fails
immediately. Locate the fallback usages of createJsonResponse in this test file
(the instances around the mock fetch handlers) and replace the synthetic 500
return with a thrown Error that clearly identifies the unexpected request and
any request info available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8081dfe-ae11-4a28-9524-968b0b4cf96e
📒 Files selected for processing (10)
server/lib/file-ops.tsserver/lib/file-watcher.tsserver/routes/memories.test.tsserver/routes/workspace.test.tssrc/App.tsxsrc/features/file-browser/hooks/useOpenFiles.test.tssrc/features/file-browser/hooks/useOpenFiles.tssrc/features/memory/useMemories.test.tssrc/features/memory/useMemories.tssrc/features/workspace/tabs/SkillsTab.tsx
✅ Files skipped from review due to trivial changes (1)
- src/features/file-browser/hooks/useOpenFiles.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/features/workspace/tabs/SkillsTab.tsx
- server/routes/workspace.test.ts
- server/lib/file-watcher.ts
- server/routes/memories.test.ts
- src/features/memory/useMemories.ts
- server/lib/file-ops.ts
Comment on lines
+2195
to
+2204
| const readQueues = new Map<string, Response[] | Promise<Response>[]>([ | ||
| ['main', [ | ||
| createJsonResponse({ ok: true, content: 'main original', mtime: 11 }), | ||
| mainReload.promise, | ||
| ]], | ||
| ['research', [ | ||
| createJsonResponse({ ok: true, content: 'research original', mtime: 22 }), | ||
| createJsonResponse({ ok: true, content: 'research changed on disk', mtime: 23 }), | ||
| ]], | ||
| ]); |
There was a problem hiding this comment.
Type annotation doesn't match the actual array contents.
The type Map<string, Response[] | Promise<Response>[]> declares arrays containing either all Response OR all Promise<Response>, but the actual values contain mixed elements (both Response and Promise<Response> in the same array).
🔧 Fix type annotation
- const readQueues = new Map<string, Response[] | Promise<Response>[]>([
+ const readQueues = new Map<string, Array<Response | Promise<Response>>>([
['main', [
createJsonResponse({ ok: true, content: 'main original', mtime: 11 }),
mainReload.promise,
]],
['research', [
createJsonResponse({ ok: true, content: 'research original', mtime: 22 }),
createJsonResponse({ ok: true, content: 'research changed on disk', mtime: 23 }),
]],
]);📝 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.
Suggested change
| const readQueues = new Map<string, Response[] | Promise<Response>[]>([ | |
| ['main', [ | |
| createJsonResponse({ ok: true, content: 'main original', mtime: 11 }), | |
| mainReload.promise, | |
| ]], | |
| ['research', [ | |
| createJsonResponse({ ok: true, content: 'research original', mtime: 22 }), | |
| createJsonResponse({ ok: true, content: 'research changed on disk', mtime: 23 }), | |
| ]], | |
| ]); | |
| const readQueues = new Map<string, Array<Response | Promise<Response>>>([ | |
| ['main', [ | |
| createJsonResponse({ ok: true, content: 'main original', mtime: 11 }), | |
| mainReload.promise, | |
| ]], | |
| ['research', [ | |
| createJsonResponse({ ok: true, content: 'research original', mtime: 22 }), | |
| createJsonResponse({ ok: true, content: 'research changed on disk', mtime: 23 }), | |
| ]], | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/file-browser/hooks/useOpenFiles.test.ts` around lines 2195 -
2204, The Map type for readQueues is too strict: Map<string, Response[] |
Promise<Response>[]> implies each array is homogeneous, but the arrays actually
mix Response and Promise<Response> entries. Change the generic to Map<string,
(Response | Promise<Response>)[]> (or Array<Response | Promise<Response>>) for
readQueues so each array element can be either a Response or a
Promise<Response>; update the readQueues declaration accordingly where it’s
defined.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR makes Nerve's workspace surfaces follow the selected top-level agent instead of behaving like one global workspace.
What changed
Agent-scoped workspace context
Dirty cross-agent switch guard
Save and switch,Discard and switch, andCancelVerification
npx vitest run server/lib/agent-workspace.test.ts server/routes/workspace.test.ts server/routes/file-browser.test.ts server/routes/memories.test.ts src/features/workspace/workspaceScope.test.ts src/features/workspace/workspaceSwitchGuard.test.ts src/features/file-browser/hooks/useOpenFiles.test.ts src/features/file-browser/hooks/useFileTree.test.ts src/features/file-browser/FileTreePanel.test.tsx src/App.test.tsx src/hooks/useDashboardData.test.ts src/features/sessions/SpawnAgentDialog.test.tsxnpx eslint src/features/file-browser/hooks/useOpenFiles.ts src/App.tsx src/features/file-browser/hooks/useFileTree.tsnpm run buildNotes
@codemirror/languageimportsact(...)warning noiseSummary by CodeRabbit
New Features
Improvements
Bug Fixes