feat: sync sidebar layout across clients - #99
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a server-owned sidebar layout aggregate for canonical project and pinned-thread ordering. The change spans contracts, event sourcing, SQLite projections, repair flows, client reconciliation, legacy migration, sidebar drag interactions, authorization, and extensive unit, integration, and browser coverage. ChangesSidebar layout architecture
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Sidebar as Sidebar UI
participant Store as sidebarLayoutStore
participant RPC as Orchestration RPC
participant Engine as OrchestrationEngine
participant Projection as Sidebar layout projector
participant Stream as Shell stream
Sidebar->>Store: Enqueue project or pinned-thread intent
Store->>RPC: Dispatch semantic command
RPC->>Engine: Authorize and process command
Engine->>Projection: Persist sidebar-layout.updated
Projection-->>Engine: Return accepted sequence
Engine-->>RPC: Return dispatch receipt
RPC-->>Store: Return sequence
Stream-->>Store: Deliver canonical layout update
Store-->>Sidebar: Reconcile and render confirmed plus pending layout
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
github-actions
Bot
added
size:XXL
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/orchestration/Layers/OrchestrationEngine.ts (1)
389-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSidebar-layout decode failure discards the original cause.
When decoding the persisted
projectOrder/pinnedThreadOrderfails,Effect.mapErrorswaps in a freshOrchestrationCommandInternalErrorwithout settingcause, even though this error class supports an optionalcausefield elsewhere in the codebase. During a repair-flow failure this is the only signal an operator gets — losing the underlyingParseErrordetail makes diagnosing corruptedprojection_sidebar_layoutrows harder.♻️ Suggested fix
Effect.mapError( - () => + (cause) => new OrchestrationCommandInternalError({ commandId: "repair-local-state", commandType: ORCHESTRATION_WS_METHODS.repairState, detail: "The rebuilt sidebar layout could not be decoded.", + cause, }), ),🤖 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 `@apps/server/src/orchestration/Layers/OrchestrationEngine.ts` around lines 389 - 465, Update the sidebar layout decode error handling in the `sidebarLayout` construction to preserve the original decode error as the `cause` when creating `OrchestrationCommandInternalError`. Keep the existing command metadata and detail unchanged while passing through the `Effect.mapError` error value so the underlying `ParseError` remains available.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/server/src/orchestration/Layers/OrchestrationEngine.ts`:
- Around line 50-52: Remove the local SIDEBAR_LAYOUT_PROJECTOR literal and reuse
ORCHESTRATION_PROJECTOR_NAMES.sidebarLayout from ProjectionPipeline.ts wherever
resetDerivedProjectionState or backupDerivedProjectionState needs the projector
name, preserving a single source of truth.
In `@apps/server/src/persistence/Layers/OrchestrationEventStore.test.ts`:
- Around line 16-56: Ensure the test’s inserted event is deleted even when
assertions fail by wrapping the append, replay, and assertion effect in an
Effect.ensuring cleanup. Keep the existing DELETE for event_id
'evt-sidebar-layout-roundtrip' as the finalizer, and preserve the current
round-trip assertions and shared layer usage.
In `@apps/server/src/persistence/Layers/ProjectionRepositories.test.ts`:
- Around line 122-124: Remove the duplicated consecutive rows declaration in the
test’s “only the live member remains pinned” query, keeping a single const rows
assignment and its existing SQL template body.
In `@apps/web/src/sidebarLayoutStore.test.ts`:
- Around line 174-199: Update the test around acceptConfirmedLayout to match the
store’s actual revision-based gating: remove snapshotSequence from
olderReconnectSnapshotLayout and assert only the revision ordering, or move the
reconnect-fence scenario to the router/snapshot layer if that behavior is
intended to be tested.
In `@packages/contracts/src/orchestration.ts`:
- Around line 1102-1105: Extract a generic helper for the repeated
optional/null/decoding-default anchor schema pattern in
SidebarLayoutProjectMoveCommand, SidebarLayoutThreadPinCommand, and
SidebarLayoutPinnedThreadMoveCommand, then use it for each command’s
before-anchor field. Preserve the existing schema types and null decoding
default while keeping all three commands synchronized.
---
Outside diff comments:
In `@apps/server/src/orchestration/Layers/OrchestrationEngine.ts`:
- Around line 389-465: Update the sidebar layout decode error handling in the
`sidebarLayout` construction to preserve the original decode error as the
`cause` when creating `OrchestrationCommandInternalError`. Keep the existing
command metadata and detail unchanged while passing through the
`Effect.mapError` error value so the underlying `ParseError` remains available.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3aaf7692-28ce-4f93-8d74-9f9a5c46753f
📒 Files selected for processing (65)
apps/server/src/main.test.tsapps/server/src/orchestration/Layers/OrchestrationEngine.test.tsapps/server/src/orchestration/Layers/OrchestrationEngine.tsapps/server/src/orchestration/Layers/ProjectionPipeline.test.tsapps/server/src/orchestration/Layers/ProjectionPipeline.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.test.tsapps/server/src/orchestration/Layers/ProjectionSnapshotQuery.tsapps/server/src/orchestration/Schemas.tsapps/server/src/orchestration/Services/ProjectionPipeline.tsapps/server/src/orchestration/commandInvariants.test.tsapps/server/src/orchestration/decider.sidebarLayout.test.tsapps/server/src/orchestration/decider.tsapps/server/src/orchestration/projector.test.tsapps/server/src/orchestration/projector.tsapps/server/src/orchestration/sidebarLayout.test.tsapps/server/src/orchestration/sidebarLayout.tsapps/server/src/orchestration/sidebarLayoutDecider.tsapps/server/src/persistence/Layers/OrchestrationCommandReceipts.test.tsapps/server/src/persistence/Layers/OrchestrationEventStore.test.tsapps/server/src/persistence/Layers/OrchestrationEventStore.tsapps/server/src/persistence/Layers/ProjectionRepositories.test.tsapps/server/src/persistence/Layers/ProjectionSidebarLayout.test.tsapps/server/src/persistence/Layers/ProjectionSidebarLayout.tsapps/server/src/persistence/Layers/ProjectionThreads.tsapps/server/src/persistence/Migrations.tsapps/server/src/persistence/Migrations/041_ProjectionSidebarLayout.test.tsapps/server/src/persistence/Migrations/041_ProjectionSidebarLayout.tsapps/server/src/persistence/Services/OrchestrationCommandReceipts.tsapps/server/src/persistence/Services/ProjectionSidebarLayout.tsapps/server/src/persistence/Services/ProjectionThreads.tsapps/server/src/threadRetention.test.tsapps/server/src/wsRpc.test.tsapps/server/src/wsRpc.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/EventRouter.browser.tsxapps/web/src/components/FirstRunWizard.browser.tsxapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/lib/desktopProjectRecovery.test.tsapps/web/src/lib/threadCreatePromotion.test.tsapps/web/src/pinnedThreadsStore.test.tsapps/web/src/pinnedThreadsStore.tsapps/web/src/routes/__root.tsxapps/web/src/shellEventOrdering.test.tsapps/web/src/shellEventOrdering.tsapps/web/src/sidebarLayout.logic.test.tsapps/web/src/sidebarLayout.logic.tsapps/web/src/sidebarLayoutLegacyMigration.test.tsapps/web/src/sidebarLayoutLegacyMigration.tsapps/web/src/sidebarLayoutRouter.test.tsapps/web/src/sidebarLayoutRouter.tsapps/web/src/sidebarLayoutStore.test.tsapps/web/src/sidebarLayoutStore.tsapps/web/src/storageKeyMigration.test.tsapps/web/src/storageKeyMigration.tsapps/web/src/store.test.tsapps/web/src/store.tsapps/web/src/vite-env.d.tsapps/web/vitest.browser.config.tsdocs/superpowers/specs/2026-07-17-server-owned-sidebar-layout-design.mdpackages/contracts/src/baseSchemas.tspackages/contracts/src/orchestration.test.tspackages/contracts/src/orchestration.ts
💤 Files with no reviewable changes (2)
- apps/web/src/pinnedThreadsStore.ts
- apps/web/src/pinnedThreadsStore.test.ts
| // Then: only the live member remains pinned and every stale flag is cleared. | ||
| const rows = yield* sql<{ readonly threadId: string; readonly isPinned: number }>` | ||
| SELECT thread_id AS "threadId", is_pinned AS "isPinned" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate const rows = ... declaration breaks compilation.
The const rows = yield* sql<{ readonly threadId: string; readonly isPinned: number }>\`` line is duplicated back-to-back before the template literal body, producing a duplicate block-scoped declaration of rows` in the same test. This is a syntax/compile error, not just a style nit.
🐛 Proposed fix
// Then: only the live member remains pinned and every stale flag is cleared.
const rows = yield* sql<{ readonly threadId: string; readonly isPinned: number }>`
- const rows = yield* sql<{ readonly threadId: string; readonly isPinned: number }>`
SELECT thread_id AS "threadId", is_pinned AS "isPinned"
FROM projection_threads
WHERE thread_id IN (${staleId}, ${liveId}, ${deletedId})
ORDER BY thread_id
`;📝 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.
| // Then: only the live member remains pinned and every stale flag is cleared. | |
| const rows = yield* sql<{ readonly threadId: string; readonly isPinned: number }>` | |
| SELECT thread_id AS "threadId", is_pinned AS "isPinned" | |
| // Then: only the live member remains pinned and every stale flag is cleared. | |
| const rows = yield* sql<{ readonly threadId: string; readonly isPinned: number }>` | |
| SELECT thread_id AS "threadId", is_pinned AS "isPinned" |
🤖 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 `@apps/server/src/persistence/Layers/ProjectionRepositories.test.ts` around
lines 122 - 124, Remove the duplicated consecutive rows declaration in the
test’s “only the live member remains pinned” query, keeping a single const rows
assignment and its existing SQL template body.
| beforeProjectId: Schema.optional(Schema.NullOr(ProjectId)).pipe( | ||
| Schema.withDecodingDefault(() => null), | ||
| ), | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate "before-anchor" optional/null/default pattern across three commands.
SidebarLayoutProjectMoveCommand, SidebarLayoutThreadPinCommand, and SidebarLayoutPinnedThreadMoveCommand each repeat Schema.optional(Schema.NullOr(X)).pipe(Schema.withDecodingDefault(() => null)) for their anchor field. Extracting a small generic helper (e.g. optionalBeforeId = <A>(schema: Schema.Schema<A>) => Schema.optional(Schema.NullOr(schema)).pipe(Schema.withDecodingDefault(() => null))) would keep the three commands in sync if this normalization ever changes.
♻️ Proposed helper extraction
+const optionalBeforeId = <A, I, R>(schema: Schema.Schema<A, I, R>) =>
+ Schema.optional(Schema.NullOr(schema)).pipe(Schema.withDecodingDefault(() => null));
+
const SidebarLayoutProjectMoveCommand = Schema.Struct({
type: Schema.Literal("sidebar-layout.project.move"),
commandId: CommandId,
projectId: ProjectId,
- beforeProjectId: Schema.optional(Schema.NullOr(ProjectId)).pipe(
- Schema.withDecodingDefault(() => null),
- ),
+ beforeProjectId: optionalBeforeId(ProjectId),
});Also applies to: 1111-1114, 1126-1129
🤖 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/contracts/src/orchestration.ts` around lines 1102 - 1105, Extract a
generic helper for the repeated optional/null/decoding-default anchor schema
pattern in SidebarLayoutProjectMoveCommand, SidebarLayoutThreadPinCommand, and
SidebarLayoutPinnedThreadMoveCommand, then use it for each command’s
before-anchor field. Preserve the existing schema types and null decoding
default while keeping all three commands synchronized.
1 participant
Summary
Why
Pinned and project ordering were stored locally and continuously mirrored from thread metadata. During an unpin, a stale shell update could repin the thread immediately; ordering also diverged between computers connected to the same JCode server.
The server is now the single authority for both orders, so clients converge without compensating pin commands or local unions.
User impact
T3Code Integration Ideasand other threads can be unpinned reliablyValidation
Summary by CodeRabbit
New Features
Bug Fixes