Revert "feat(desktop): workspace pane store registry (v2 PR 3)" (#3940)#4017
Merged
Revert "feat(desktop): workspace pane store registry (v2 PR 3)" (#3940)#4017
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (8)
📝 WalkthroughWalkthroughThe workspace pane store initialization was changed to start from an EMPTY_STATE and be seeded via v2WorkspaceLocalState.subscribeChanges (includeInitialState). Registry initialization/wiring was removed from the CollectionsProvider; several tests/files and re-exports were removed, and hooks were updated to create local stores and synchronize with collections via snapshot-guarded subscribe/writebacks. ChangesWorkspace pane store initialization & sync
Sequence Diagram(s)sequenceDiagram
participant Hook as useV2WorkspacePaneLayout (UI Hook)
participant Store as Workspace Store
participant Col as v2WorkspaceLocalState (Collections)
participant LS as localStorage
Note over Hook,Store: Initialization (cold-start)
Hook->>Store: createWorkspaceStore(EMPTY_STATE)
Hook->>Col: useLiveQuery/get(workspaceId)
Col->>LS: read persisted row
LS-->>Col: persisted paneLayout (initial)
Col-->>Hook: includeInitialState event (subscribeChanges)
Hook->>Store: replaceState(persistedPaneLayout) [if snapshot differs]
Note over Store,Col: Ongoing sync
Store->>Col: update row with version/tabs/activeTabId (on store change)
Col->>LS: write persisted row
LS-->>Col: persisted row saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Greptile SummaryThis PR fixes a cold-boot race where Confidence Score: 4/5Safe to merge; fix is well-reasoned, all 944 tests pass, and the only concern is a P2 style issue in CollectionsProvider. No P0 or P1 issues found. The core fix (replacing a racing .get() with subscribeChanges({ includeInitialState: true })) is structurally correct and validated by two targeted regression tests. The three-way reconciliation handles all seeding and echo-prevention edge cases. The sole concern is startSyncImmediate() being called as a side effect inside useMemo, which is non-idiomatic React and can double-fire in Strict Mode, but is acknowledged in comments and the call appears idempotent. CollectionsProvider.tsx — startSyncImmediate() side effect in useMemo should be monitored if startSyncImmediate ever becomes non-idempotent.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/workspace-pane-registry/workspace-pane-registry.ts | Core fix: removes the racing synchronous .get() seed, replaces it with subscribeChanges({ includeInitialState: true }) delivering the persisted row as the first event; three-way reconciliation and all sync paths preserved correctly. |
| apps/desktop/src/renderer/lib/workspace-pane-registry/workspace-pane-registry.test.ts | Adds two targeted regression tests (cold-boot seeding and cold-boot tab-stacking) using isolated Storage mocks and real createCollection; both verify the fix end-to-end without async workarounds, confirming includeInitialState fires synchronously. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx | Moves startSyncImmediate() into the useMemo as belt-and-suspenders for non-registry consumers; the side effect in useMemo is intentional and acknowledged but remains non-idiomatic React and could cause double-invocations in Strict Mode. |
Sequence Diagram
sequenceDiagram
participant CP as CollectionsProvider (useMemo)
participant Reg as WorkspacePaneRegistry
participant TDB as TanStack DB Collection
participant Store as Zustand Store
CP->>TDB: startSyncImmediate()
Note over TDB: Collection loads localStorage data synchronously
CP->>Reg: initWorkspacePaneRegistry(deps)
Note over CP: Later — workspace route mounts
CP->>Reg: getOrCreateWorkspacePaneStore(workspaceId)
Reg->>Store: createWorkspaceStore(EMPTY_STATE)
Reg->>TDB: subscribeChanges({ includeInitialState: true })
TDB-->>Reg: initial-state event (persisted row)
Note over Reg: 3-way reconciliation:<br/>incoming ≠ storeSnapshot (EMPTY)<br/>storeSnapshot === lastSyncedSnapshot (EMPTY)<br/>→ replaceState(persistedLayout)
Reg->>Store: replaceState(persistedLayout)
Note over Store: Store now holds persisted layout ✓
Note over Store: User action (e.g. addTab)
Store-->>Reg: store subscriber fires
Reg->>TDB: update(workspaceId, draft.paneLayout = newState)
Note over Reg: lastSyncedSnapshot = newSnapshot
TDB-->>Reg: change event (echo of store→row write)
Note over Reg: incoming === storeSnapshot → no-op
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx:85
**Side effect inside `useMemo` is non-idiomatic**
`startSyncImmediate()` is a side effect placed directly in `useMemo`. React reserves the right to discard and recompute memo work (and in Strict Mode explicitly does so in development), which means `startSyncImmediate()` can be called multiple times per `activeOrganizationId`. The comment acknowledges this, and the mitigation (idempotency of `startSyncImmediate`) makes this tolerable in practice, but it's worth noting that if `startSyncImmediate` is ever not idempotent (e.g., triggers duplicate event listeners), the double-invoke in Strict Mode could surface subtle bugs. Consider whether a `useEffect` with a dedicated synchrony workaround or a `useRef` guard would be a safer long-term home for this call.
Reviews (1): Last reviewed commit: "fix(desktop): seed pane store from initi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const collections = useMemo(() => { | ||
| if (!activeOrganizationId) return null; | ||
| const next = getCollections(activeOrganizationId); | ||
| next.v2WorkspaceLocalState.startSyncImmediate(); |
There was a problem hiding this comment.
useMemo is non-idiomatic
startSyncImmediate() is a side effect placed directly in useMemo. React reserves the right to discard and recompute memo work (and in Strict Mode explicitly does so in development), which means startSyncImmediate() can be called multiple times per activeOrganizationId. The comment acknowledges this, and the mitigation (idempotency of startSyncImmediate) makes this tolerable in practice, but it's worth noting that if startSyncImmediate is ever not idempotent (e.g., triggers duplicate event listeners), the double-invoke in Strict Mode could surface subtle bugs. Consider whether a useEffect with a dedicated synchrony workaround or a useRef guard would be a safer long-term home for this call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx
Line: 85
Comment:
**Side effect inside `useMemo` is non-idiomatic**
`startSyncImmediate()` is a side effect placed directly in `useMemo`. React reserves the right to discard and recompute memo work (and in Strict Mode explicitly does so in development), which means `startSyncImmediate()` can be called multiple times per `activeOrganizationId`. The comment acknowledges this, and the mitigation (idempotency of `startSyncImmediate`) makes this tolerable in practice, but it's worth noting that if `startSyncImmediate` is ever not idempotent (e.g., triggers duplicate event listeners), the double-invoke in Strict Mode could surface subtle bugs. Consider whether a `useEffect` with a dedicated synchrony workaround or a `useRef` guard would be a safer long-term home for this call.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Kitenite
force-pushed
the
debug-pane-registry-prese
branch
from
c5c4956 to
266c746
Compare
This reverts commit f7db05c.
Kitenite
force-pushed
the
debug-pane-registry-prese
branch
from
266c746 to
7e78255
Compare
Kitenite
changed the title
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
Recorded as integrated via -s ours after batch PRs #455-#464. Taken via individual PRs: - PR 1 (#455): v2 polish 前半 safe set (9 commits) - PR 2 (#456): v2/host-service polish 中盤 (12 commits) - PR 3 (#457): sidebar polish + jwt API (5 commits) - PR 4 (#458): host-service tRPC retry/cache/timeout (3 commits) - PR 5 (#459): v2 diff pane / file pane polish (2 commits) - PR 7 (#462): host-service v2 canonical workspace.create + attachment store (PR1 superset-sh#3893 + PR2 superset-sh#3916) - PR 11 (#463): agents API + onboarding (7 commits + 1 cleanup) - PR 12 (#464): v1→v2 import flow rewrite (11 commits + 2 follow-ups) - PR 13 (#460): host-service shell env probe + typo (2 commits) - PR 16 (#461): marketplace 19 themes (1 commit) Skipped / deferred (recorded as integrated for behind=0): - PR 6: CLI v1 launch (superset-sh#3898 + 30+ CLI/SDK followups) — defer to dedicated migration - PR 9: v2 PR3 (superset-sh#3940) + revert (superset-sh#4017) — net-zero pair - PR 10: pty-daemon (superset-sh#3896, superset-sh#3971, superset-sh#4054) — fork keeps its terminal-host - PR 14: Slack MCP-v2 (superset-sh#4197, superset-sh#4208) — depends on mcp-v2/sdk divergence - PR 15: onboarding remaining (superset-sh#4115, superset-sh#4125, superset-sh#4214, superset-sh#4213, superset-sh#4222, superset-sh#4225) — depends on fork's deleted setup pages Behind: 0 after this merge.
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
Reverts #3940 (
f7db05ce8).The pane-store registry / bidirectional persistence sync introduced regressions that aren't worth its current scope:
<Pane>remounts when the layout tree restructures (single-pane → split, split-depth growing) anduseRef(paneData.initialCommand)reinitializes against already-cleared data.subscribeChanges+ 3-way reconciliation againstlastSyncedSnapshotis hard to reason about and traces revealed it was firing many redundant writes per pane operation.Going back to per-mount
useState(() => createWorkspaceStore(...))+useLiveQuery-driven seeding restores the pre-PR3 behavior, which was reliable. PR4'sworkspace.create()flow can re-add a singleton later with a different design that doesn't putinitialCommandon persisted pane data.Test plan
bun test src/renderer— 927/927 passbunx tsc --noEmit— no new errorsSummary by CodeRabbit
Bug Fixes
Behavior Change
Tests
Refactor