Open workspace drafts optimistically and dedupe draft writes#29
Merged
Open workspace drafts optimistically and dedupe draft writes#29
Conversation
- Start workspace drafts before setActive ack and roll back on failure - Consolidate composer draft upsert and sticky state into one write - Add sidebar grouping helper and new-thread latency diagnostics
📝 WalkthroughWalkthroughAdds dev-only new-thread latency tracking and debug UI; implements optimistic workspace activation with token-guarded async persistence and rollback on failure; introduces logical project indexing for sidebar rendering; adds atomic create-or-reuse composer-draft semantics with sticky-state application; and includes extensive tests for races, dedupe, and persistence behaviors. (36 words) Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Sidebar as Sidebar UI
participant Store as AppStore
participant Router as Router/Handler
participant API as Workspace API
participant Latency as NewThreadLatency
User->>Sidebar: Click "New Thread" / press shortcut
Sidebar->>Latency: startNewThreadLatency(flow)
Latency-->>Sidebar: tracker
Sidebar->>Store: setActiveWorkspaceForProject (optimistic)
Store-->>Sidebar: optimistic state update
Sidebar->>Store: createOrReuseProjectDraft(...)
Store-->>Sidebar: draft created/reused
Sidebar->>Router: router.navigate(newDraft) (via startTransition)
Router->>Latency: markRouteReady()
Latency-->>Latency: append sample (partial)
Note right of API: async persistence (token-guarded)
Sidebar->>API: workspaces.setActive (async)
alt server ack success (token matches)
API-->>Sidebar: success
Sidebar->>Latency: markWorkspaceAck("success")
else server ack error (token matches)
API-->>Sidebar: error
Sidebar->>Store: revert active workspace (if token matches)
Sidebar->>Sidebar: show "Workspace sync failed" toast
Sidebar->>Latency: markWorkspaceAck("error")
else stale response (token mismatch)
API-->>Sidebar: ignored (no-op)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
github-actions
Bot
added
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/perf/newThreadLatency.test.ts (1)
64-78: Strengthen the eviction test with distinct sample durations.Line 76 and Line 77 both asserting
1doesn’t prove oldest samples were evicted in order. Make per-sample durations unique and assert boundary values.Suggested test hardening
for (let index = 0; index < 55; index += 1) { const tracker = startNewThreadLatency("shortcut"); - nowMs += 1; + nowMs += index + 1; tracker.markRouteReady(); } const samples = getNewThreadLatencySamples(); expect(samples).toHaveLength(50); - expect(samples[0]?.durationMs).toBe(1); - expect(samples[49]?.durationMs).toBe(1); + expect(samples[0]?.durationMs).toBe(6); + expect(samples[49]?.durationMs).toBe(55);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/perf/newThreadLatency.test.ts` around lines 64 - 78, The test currently uses identical durations so eviction order isn’t verified; update the loop in the "keeps only the latest 50 samples" test to produce unique durations per sample by advancing nowMs differently between creating the tracker (startNewThreadLatency) and calling tracker.markRouteReady(), using __setNewThreadLatencyNowProviderForTests to control time; after pushing 55 samples, use getNewThreadLatencySamples() and assert samples.length === 50 and that samples[0].durationMs equals the duration of the 6th-created sample and samples[49].durationMs equals the duration of the 55th-created sample (reference functions: __setNewThreadLatencyNowProviderForTests, startNewThreadLatency, tracker.markRouteReady, getNewThreadLatencySamples).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 2494-2504: The optimistic workspace switch via
setActiveWorkspaceForProject is done before awaiting handleNewThread, but
there's no rollback if handleNewThread rejects; modify the flow to capture the
current active workspace (previous environmentId/projectId/workspaceId) before
calling setActiveWorkspaceForProject, then call setActiveWorkspaceForProject and
await handleNewThread inside a try block, and in the catch for handleNewThread
revert by calling setActiveWorkspaceForProject with the saved previous ids (and
rethrow or return the error) so the sidebar reflects the original workspace;
keep the existing api.workspaces.setActive(...).catch(...) logic intact for the
server update.
In `@apps/web/src/composerDraftStore.ts`:
- Around line 1158-1170: The current cleanup block deletes the previous thread's
entries even if that draft is mid-promotion; modify the conditional so we do not
evict a draft whose state has promotedTo set. Specifically, when checking
previousThreadKeyForLogicalProject before deleting nextDraftThreadsByThreadKey
and nextDraftsByThreadKey, also check
state.draftsByThreadKey[previousThreadKeyForLogicalProject]?.promotedTo ===
undefined (or falsy); if promotedTo exists, skip the delete logic for that
previousThreadKeyForLogicalProject to preserve in-flight promotion data in
nextLogicalProjectDraftThreadKeyByLogicalProject, nextDraftThreadsByThreadKey,
and nextDraftsByThreadKey.
In `@apps/web/src/perf/newThreadLatency.ts`:
- Around line 158-172: getNewThreadLatencySamples currently builds a new array
on every call which breaks useSyncExternalStore's snapshot stability; change it
to return a cached snapshot reference and only recreate that cached array when
samplesWithId changes. Concretely, add a module-level cachedSnapshot variable
(e.g., cachedNewThreadLatencySamples) and a function to refresh it when
samplesWithId is mutated (or compute it lazily and update on writes), update
getNewThreadLatencySamples to return cachedNewThreadLatencySamples instead of
mapping each time, and ensure any code that mutates samplesWithId also updates
the cache so useNewThreadLatencySamples (which passes getNewThreadLatencySamples
as the snapshot getter) will receive a stable reference between real updates.
---
Nitpick comments:
In `@apps/web/src/perf/newThreadLatency.test.ts`:
- Around line 64-78: The test currently uses identical durations so eviction
order isn’t verified; update the loop in the "keeps only the latest 50 samples"
test to produce unique durations per sample by advancing nowMs differently
between creating the tracker (startNewThreadLatency) and calling
tracker.markRouteReady(), using __setNewThreadLatencyNowProviderForTests to
control time; after pushing 55 samples, use getNewThreadLatencySamples() and
assert samples.length === 50 and that samples[0].durationMs equals the duration
of the 6th-created sample and samples[49].durationMs equals the duration of the
55th-created sample (reference functions:
__setNewThreadLatencyNowProviderForTests, startNewThreadLatency,
tracker.markRouteReady, getNewThreadLatencySamples).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a77f48b1-62f2-40ec-a896-64caf7f956a7
📒 Files selected for processing (13)
apps/web/src/components/ChatView.browser.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/WebSocketConnectionSurface.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.tsapps/web/src/hooks/useHandleNewThread.tsapps/web/src/perf/newThreadLatency.test.tsapps/web/src/perf/newThreadLatency.tsapps/web/src/routes/_chat.tsxapps/web/src/store.test.tsapps/web/src/store.ts
- Avoid evicting promoted drafts during project thread remaps - Cache new-thread latency snapshots for stable references - Restore the previous active workspace if new thread creation fails
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/composerDraftStore.ts`:
- Around line 1131-1146: The early-return path rebuilds the thread with
createDraftThreadState (assigned to nextDraftThread) which clears promotedTo,
causing promotion state to be lost when draftThreadsEqual triggers; modify the
logic so that before comparing/returning you copy existingThread.promotedTo into
the rebuilt thread (i.e. set nextDraftThread.promotedTo =
existingThread.promotedTo) or preserve promotedTo when constructing
nextDraftThread, ensuring callers like setLogicalProjectDraftThreadId,
createOrReuseProjectDraft, getDraftSessionByLogicalProjectKey, and
finalizePromotedDraftThreadByRef still see the promotion marker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2407804-b395-4419-be2b-f4c39acfd83c
📒 Files selected for processing (5)
apps/web/src/components/Sidebar.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.tsapps/web/src/perf/newThreadLatency.test.tsapps/web/src/perf/newThreadLatency.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/perf/newThreadLatency.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/composerDraftStore.test.ts
- keep promoted thread metadata when reusing an existing logical draft mapping - wait for workspace-set-active requests in chat view tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/composerDraftStore.ts`:
- Around line 1149-1152: The current merge into
nextLogicalProjectDraftThreadKeyByLogicalProjectKey blindly adds
[input.normalizedLogicalProjectKey]: input.draftId without removing any existing
entries that point to the same draftId, leaving stale aliases; update the
construction to first copy state.logicalProjectDraftThreadKeyByLogicalProjectKey
then remove any keys whose value === input.draftId before assigning the new
mapping for input.normalizedLogicalProjectKey so old logicalProjectKey ->
draftId aliases are cleared; apply the same change in the other occurrence (the
block around lines referencing the same pattern) and keep references to
nextLogicalProjectDraftThreadKeyByLogicalProjectKey,
state.logicalProjectDraftThreadKeyByLogicalProjectKey,
input.normalizedLogicalProjectKey, input.draftId,
getDraftSessionByLogicalProjectKey and isComposerThreadKeyInUse when locating
the spots to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43ff07c9-c7d2-4c08-bef2-aadbf9944010
📒 Files selected for processing (3)
apps/web/src/components/ChatView.browser.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/composerDraftStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/ChatView.browser.tsx
- Keep only the latest logical-project mapping for a reused draft id - Normalize persisted mappings to avoid stale aliases
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/composerDraftStore.ts (1)
291-306: Extract the repeated draft-upsert options shape into a shared type.That inline
optionsobject now appears in the public store interface and again in the helper signatures below. Pulling it into a singleDraftThreadUpsertOptionsalias will make future field additions much harder to miss.♻️ Example cleanup
+type DraftThreadUpsertOptions = { + threadId?: ThreadId; + branch?: string | null; + worktreePath?: string | null; + createdAt?: string; + envMode?: DraftThreadEnvMode; + runtimeMode?: RuntimeMode; + interactionMode?: ProviderInteractionMode; +}; + interface ComposerDraftStoreState { ... setLogicalProjectDraftThreadId: ( logicalProjectKey: string, projectRef: ScopedProjectRef, draftId: DraftId, - options?: { - threadId?: ThreadId; - branch?: string | null; - worktreePath?: string | null; - createdAt?: string; - envMode?: DraftThreadEnvMode; - runtimeMode?: RuntimeMode; - interactionMode?: ProviderInteractionMode; - }, + options?: DraftThreadUpsertOptions, ) => void; ... createOrReuseProjectDraft: (input: { logicalProjectKey: string; projectRef: ScopedProjectRef; draftId: DraftId; - options?: { - threadId?: ThreadId; - branch?: string | null; - worktreePath?: string | null; - createdAt?: string; - envMode?: DraftThreadEnvMode; - runtimeMode?: RuntimeMode; - interactionMode?: ProviderInteractionMode; - }; + options?: DraftThreadUpsertOptions; applyStickyState?: boolean; }) => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/composerDraftStore.ts` around lines 291 - 306, The public interface method createOrReuseProjectDraft repeats the inline options shape; extract that options object into a shared type alias (e.g., DraftThreadUpsertOptions) and replace all inline occurrences with that alias (including the createOrReuseProjectDraft signature and any helper functions that accept the same options), update imports/exports as needed, and run type checks to ensure fields like threadId, branch, worktreePath, createdAt, envMode, runtimeMode, and interactionMode are preserved exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/composerDraftStore.ts`:
- Around line 291-306: The public interface method createOrReuseProjectDraft
repeats the inline options shape; extract that options object into a shared type
alias (e.g., DraftThreadUpsertOptions) and replace all inline occurrences with
that alias (including the createOrReuseProjectDraft signature and any helper
functions that accept the same options), update imports/exports as needed, and
run type checks to ensure fields like threadId, branch, worktreePath, createdAt,
envMode, runtimeMode, and interactionMode are preserved exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14a1a8e3-f18e-427e-9a89-75e2e2c2de9a
📒 Files selected for processing (2)
apps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/composerDraftStore.test.ts
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
setActivefailure.Testing
bun fmt,bun lint,bun typecheck.Summary by CodeRabbit
Bug Fixes
New Features
Tests
Refactor