Conversation
WalkthroughThe PR refactors the frontend layout system into a local "write cache" model: runtime state now uses a private localTreeStateAtom with asynchronous persistence to a WaveObject-backed atom. The LayoutModel constructor now accepts a tabAtom and exposes magnifiedNodeIdAtom; generation counters and LayoutTreeState.generation were removed. Multiple APIs (getLayoutModelForTab, getLayoutModelForTabById, useLayoutModel) were removed or made internal and call sites switched to a static-tab model. TileLayout now reads magnifiedNodeId from the new atom. Layout action payloads gained an actionid field (frontend types and Go structs) and the Go backend auto-populates missing ActionId with UUIDs. Tests and .gitignore updated accordingly. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
aiprompts/layout-simplification.md (2)
13-31: Annotate fenced code blocks with a language
markdownlint(MD040) is flagging these code fences because they don’t declare a language. Please add an explicit identifier (e.g.text,mermaid,typescript, etc.) to this block and the other fences introduced in this doc so the linter passes.-``` +```text User action ↓ treeReducer() mutates layoutState
684-684: Resolve duplicate H1 headingWe now have two
# Wave Terminal Layout System - Simplification via Write Cache Patternheadings, which triggers MD024. Please rename or remove the second H1 so the document structure is unique.frontend/layout/lib/layoutModel.ts (1)
407-542: Backend action handling is comprehensive with good error handling.The switch statement correctly:
- Validates required fields for each action type
- Constructs appropriate
LayoutTreeActionobjects- Handles ephemeral nodes specially for
InsertNode- Logs errors for missing/invalid data
- Uses
treeReducerwithsetState=falseto defer persistenceMinor inefficiency: The
DeleteNodecase callscloseNode()which triggerspersistToBackend(), thenprocessPendingBackendActions()calls it again at the end. The debounce prevents duplicate writes, but consider whethercloseNode()needs its own persistence call when invoked from backend action processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.gitignore(1 hunks)aiprompts/layout-simplification.md(1 hunks)frontend/app/store/global.ts(5 hunks)frontend/app/store/keymodel.ts(7 hunks)frontend/layout/index.ts(0 hunks)frontend/layout/lib/TileLayout.tsx(1 hunks)frontend/layout/lib/layoutAtom.ts(1 hunks)frontend/layout/lib/layoutModel.ts(12 hunks)frontend/layout/lib/layoutModelHooks.ts(3 hunks)frontend/layout/lib/layoutTree.ts(11 hunks)frontend/layout/lib/types.ts(0 hunks)frontend/layout/tests/model.ts(0 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/waveobj/wtype.go(1 hunks)pkg/wcore/layout.go(2 hunks)
💤 Files with no reviewable changes (3)
- frontend/layout/lib/types.ts
- frontend/layout/index.ts
- frontend/layout/tests/model.ts
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/layout/lib/layoutAtom.ts (1)
pkg/waveobj/wtype.go (4)
Tab(183-190)Tab(192-194)LayoutState(222-231)LayoutState(233-235)
frontend/app/store/global.ts (2)
frontend/layout/lib/layoutModelHooks.ts (1)
getLayoutModelForStaticTab(45-48)frontend/layout/index.ts (1)
getLayoutModelForStaticTab(37-37)
frontend/app/store/keymodel.ts (2)
frontend/layout/lib/layoutModelHooks.ts (1)
getLayoutModelForStaticTab(45-48)frontend/layout/index.ts (2)
getLayoutModelForStaticTab(37-37)NavigateDirection(40-40)
frontend/layout/lib/layoutModelHooks.ts (2)
frontend/layout/lib/layoutModel.ts (1)
LayoutModel(72-1481)frontend/layout/lib/layoutAtom.ts (1)
getLayoutStateAtomFromTab(7-13)
frontend/layout/lib/layoutTree.ts (1)
frontend/layout/lib/types.ts (1)
LayoutTreeState(291-300)
frontend/layout/lib/layoutModel.ts (3)
frontend/layout/lib/types.ts (1)
LayoutTreeState(291-300)pkg/waveobj/wtype.go (5)
Tab(183-190)Tab(192-194)LayoutState(222-231)LayoutState(233-235)LayoutActionData(204-215)frontend/layout/lib/layoutAtom.ts (1)
getLayoutStateAtomFromTab(7-13)
🪛 markdownlint-cli2 (0.18.1)
aiprompts/layout-simplification.md
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
684-684: Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
frontend/layout/lib/layoutModel.ts (9)
236-351: LGTM! Constructor refactored to support write-cache architecture.The constructor correctly initializes the new persistence infrastructure:
tabAtomreplaces the formertreeStateAtomparameterwaveObjectAtomis derived for backend persistencelocalTreeStateAtomis created as the runtime source of truthpersistDebounceTimerandprocessedActionIdssupport async persistence and deduplicationmagnifiedNodeIdAtomis correctly derived from local state- Initialization is properly deferred to
initializeFromWaveObject()
353-373: LGTM! Initialization logic correctly seeds local state from WaveObject.The method properly:
- Derives initial state from the persisted WaveObject
- Updates both
treeStateandmagnifiedNodeIdfields- Synchronizes
localTreeStateAtom- Handles pending backend actions vs. normal tree updates appropriately
375-381: LGTM! Backend update callback correctly triggers action processing.The method appropriately checks for pending backend actions and processes them asynchronously.
383-405: LGTM! Pending action processing correctly deduplicates and processes actions.The method:
- Properly deduplicates actions using
processedActionIds- Warns when actions lack
actionid- Continues processing remaining actions even if one fails
- Updates local state and persists after all actions are applied
544-561: LGTM! Persistence logic correctly debounces writes to backend.The 100ms debounce is appropriate for coalescing rapid state changes, and the implementation correctly:
- Cancels pending timers before scheduling new ones
- Updates all relevant WaveObject fields
- Clears the timer after execution
649-659: LGTM! State updates correctly separated from persistence.The conditional persistence logic appropriately:
- Updates magnified node tracking
- Defers persistence when
setState=false(backend action processing)- Triggers persistence when
setState=true(user-initiated changes)
665-670: LGTM! Method simplified to match new architecture.The method correctly handles forced updates without the removed generation tracking.
1196-1223: LGTM! Node closure correctly updates local state and persists.The ephemeral node handling properly updates
localTreeStateAtomand triggers persistence.
1023-1031: LGTM! Node model atoms correctly read from local state.The
isFocusedandisMagnifiedatoms properly derive their values fromlocalTreeStateAtom.
| export function getLayoutStateAtomFromTab(tabAtom: Atom<Tab>, get: Getter): WritableWaveObjectAtom<LayoutState> { | ||
| const tabData = get(tabAtom); | ||
| if (!tabData) return; | ||
| const layoutStateOref = WOS.makeORef("layout", tabData.layoutstate); | ||
| const layoutStateAtom = WOS.getWaveObjectAtom<LayoutState>(layoutStateOref); | ||
| return layoutStateAtom; |
There was a problem hiding this comment.
Guard against missing tab layout state
Right now we return undefined when the tab WaveObject hasn’t landed yet, even though the signature guarantees a WritableWaveObjectAtom<LayoutState>. LayoutModel stores that value immediately and then calls this.getter(this.waveObjectAtom) during initialization—if the tab data is still loading we end up dereferencing undefined and crash. Please fail fast (throw) or explicitly widen the return type and make callers handle the empty case.
export function getLayoutStateAtomFromTab(tabAtom: Atom<Tab>, get: Getter): WritableWaveObjectAtom<LayoutState> {
const tabData = get(tabAtom);
- if (!tabData) return;
+ if (!tabData) {
+ throw new Error("layout state atom requested before tab data is loaded");
+ }📝 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.
| export function getLayoutStateAtomFromTab(tabAtom: Atom<Tab>, get: Getter): WritableWaveObjectAtom<LayoutState> { | |
| const tabData = get(tabAtom); | |
| if (!tabData) return; | |
| const layoutStateOref = WOS.makeORef("layout", tabData.layoutstate); | |
| const layoutStateAtom = WOS.getWaveObjectAtom<LayoutState>(layoutStateOref); | |
| return layoutStateAtom; | |
| export function getLayoutStateAtomFromTab(tabAtom: Atom<Tab>, get: Getter): WritableWaveObjectAtom<LayoutState> { | |
| const tabData = get(tabAtom); | |
| if (!tabData) { | |
| throw new Error("layout state atom requested before tab data is loaded"); | |
| } | |
| const layoutStateOref = WOS.makeORef("layout", tabData.layoutstate); | |
| const layoutStateAtom = WOS.getWaveObjectAtom<LayoutState>(layoutStateOref); | |
| return layoutStateAtom; | |
| } |
🤖 Prompt for AI Agents
In frontend/layout/lib/layoutAtom.ts around lines 7 to 12, the function
currently returns undefined when the tab WaveObject isn't ready even though its
signature promises a WritableWaveObjectAtom<LayoutState>; change the behavior to
fail fast by throwing a clear error if tabData or tabData.layoutstate is missing
(e.g., throw new Error with context including the tab Atom identity) so callers
never receive undefined; keep the return type as
WritableWaveObjectAtom<LayoutState> and update any unit tests or call sites
expecting an error path if necessary.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/layout/lib/layoutModel.ts (3)
353-373: Verify error handling for async backend action processing.Line 369 uses
fireAndForgetto process pending backend actions. IfprocessPendingBackendActions()throws an error, it will be silently ignored. Ensure that errors during backend action processing are logged or handled appropriately to prevent silent failures during initialization.Consider wrapping the call in a try-catch or ensuring
processPendingBackendActionshas internal error handling:if (initialState.pendingBackendActions?.length) { - fireAndForget(() => this.processPendingBackendActions()); + fireAndForget(async () => { + try { + await this.processPendingBackendActions(); + } catch (error) { + console.error("Failed to process pending backend actions during initialization:", error); + } + }); } else {
375-381: Verify error handling for async backend action processing.Similar to initialization, Line 379 uses
fireAndForgetforprocessPendingBackendActions(). Ensure errors are logged to prevent silent failures when backend updates trigger action processing.Apply the same error handling pattern as suggested for
initializeFromWaveObject:onBackendUpdate() { const waveObj = this.getter(this.waveObjectAtom); const pendingActions = waveObj?.pendingbackendactions; if (pendingActions?.length) { - fireAndForget(() => this.processPendingBackendActions()); + fireAndForget(async () => { + try { + await this.processPendingBackendActions(); + } catch (error) { + console.error("Failed to process pending backend actions on update:", error); + } + }); } }
407-542: Verify error handling and validation in backend action processing.The method handles multiple action types but lacks error handling around async operations:
DeleteNode (Line 428):
closeNodeis async and may throw. Consider wrapping in try-catch to prevent one failed deletion from aborting the entire batch.Input validation: No validation that
action.blockidis a valid non-empty string before creating nodes. An empty or malformed blockId could cause downstream issues.Add error handling and validation:
case LayoutTreeActionType.DeleteNode: { const leaf = this?.getNodeByBlockId(action.blockid); if (leaf) { - await this.closeNode(leaf.id); + try { + await this.closeNode(leaf.id); + } catch (error) { + console.error("Failed to close node during DeleteNode action:", action.blockid, error); + } } else { console.error( "Cannot apply eventbus layout action DeleteNode, could not find leaf node with blockId", action.blockid ); } break; } case LayoutTreeActionType.InsertNode: { + if (!action.blockid) { + console.error("Cannot apply InsertNode action, blockid is missing"); + break; + } if (action.ephemeral) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/layout/lib/layoutModel.ts(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/layout/lib/layoutModel.ts (5)
frontend/layout/lib/types.ts (1)
LayoutTreeState(291-300)pkg/waveobj/wtype.go (5)
Tab(183-190)Tab(192-194)LayoutState(222-231)LayoutState(233-235)LayoutActionData(204-215)frontend/layout/lib/layoutAtom.ts (1)
getLayoutStateAtomFromTab(7-13)frontend/util/util.ts (1)
fireAndForget(411-411)frontend/layout/lib/layoutNode.ts (1)
newLayoutNode(16-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (4)
frontend/layout/lib/layoutModel.ts (4)
236-351: LGTM! Constructor refactored to support write-cache architecture.The constructor signature change (tabAtom as first parameter) and new field initializations correctly establish the local-atom-as-source-of-truth pattern. The derived magnifiedNodeIdAtom and focusedNode atom properly read from localTreeStateAtom. Initialization flow delegates to initializeFromWaveObject().
581-660: LGTM! TreeReducer correctly implements the write-cache pattern.The modified
treeReducernow updateslocalTreeStateAtom(Line 657) and callspersistToBackend()(Line 658) whensetStateis true. This correctly implements the local-atom-as-source-of-truth architecture with asynchronous persistence.
666-671: LGTM! Remaining changes consistently adopt localTreeStateAtom.The modifications to
onTreeStateAtomUpdated,closeNode, andgetNodeModelcorrectly referencelocalTreeStateAtomthroughout. The persistence calls incloseNode(Lines 1207-1208) ensure ephemeral node changes are persisted. Node model atoms (Lines 1024, 1031) correctly derive focus and magnification state fromlocalTreeStateAtom.Also applies to: 1197-1224, 1000-1051
544-562: Persisting pendingBackendActions is intentional
The round-trip ofpendingBackendActions—including persistingundefinedafter processing—is required to clear them on the backend. Silently skipping whenwaveObjis null is expected.Likely an incorrect or invalid review comment.
| private async processPendingBackendActions() { | ||
| const waveObj = this.getter(this.waveObjectAtom); | ||
| const actions = waveObj?.pendingbackendactions; | ||
| if (!actions?.length) return; | ||
|
|
||
| this.treeState.pendingBackendActions = undefined; | ||
|
|
||
| for (const action of actions) { | ||
| if (!action.actionid) { | ||
| console.warn("Dropping layout action without actionid:", action); | ||
| continue; | ||
| } | ||
| if (this.processedActionIds.has(action.actionid)) { | ||
| continue; | ||
| } | ||
| this.processedActionIds.add(action.actionid); | ||
| await this.handleBackendAction(action); | ||
| } | ||
|
|
||
| this.updateTree(); | ||
| this.setter(this.localTreeStateAtom, { ...this.treeState }); | ||
| this.persistToBackend(); | ||
| } |
There was a problem hiding this comment.
Risk: Pending actions are cleared before processing completes.
Line 388 sets treeState.pendingBackendActions = undefined before processing actions. If handleBackendAction throws an error partway through the loop, remaining actions are lost and will not be retried.
Consider one of these approaches:
Option 1: Clear actions after successful processing
private async processPendingBackendActions() {
const waveObj = this.getter(this.waveObjectAtom);
const actions = waveObj?.pendingbackendactions;
if (!actions?.length) return;
- this.treeState.pendingBackendActions = undefined;
-
for (const action of actions) {
if (!action.actionid) {
console.warn("Dropping layout action without actionid:", action);
continue;
}
if (this.processedActionIds.has(action.actionid)) {
continue;
}
this.processedActionIds.add(action.actionid);
await this.handleBackendAction(action);
}
+ this.treeState.pendingBackendActions = undefined;
this.updateTree();
this.setter(this.localTreeStateAtom, { ...this.treeState });
this.persistToBackend();
}Option 2: Add error handling to preserve unprocessed actions
private async processPendingBackendActions() {
const waveObj = this.getter(this.waveObjectAtom);
const actions = waveObj?.pendingbackendactions;
if (!actions?.length) return;
- this.treeState.pendingBackendActions = undefined;
+ const unprocessedActions: LayoutActionData[] = [];
for (const action of actions) {
if (!action.actionid) {
console.warn("Dropping layout action without actionid:", action);
continue;
}
if (this.processedActionIds.has(action.actionid)) {
continue;
}
- this.processedActionIds.add(action.actionid);
- await this.handleBackendAction(action);
+ try {
+ await this.handleBackendAction(action);
+ this.processedActionIds.add(action.actionid);
+ } catch (error) {
+ console.error("Failed to handle backend action:", action, error);
+ unprocessedActions.push(action);
+ }
}
+ this.treeState.pendingBackendActions = unprocessedActions.length > 0 ? unprocessedActions : undefined;
this.updateTree();
this.setter(this.localTreeStateAtom, { ...this.treeState });
this.persistToBackend();
}📝 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.
| private async processPendingBackendActions() { | |
| const waveObj = this.getter(this.waveObjectAtom); | |
| const actions = waveObj?.pendingbackendactions; | |
| if (!actions?.length) return; | |
| this.treeState.pendingBackendActions = undefined; | |
| for (const action of actions) { | |
| if (!action.actionid) { | |
| console.warn("Dropping layout action without actionid:", action); | |
| continue; | |
| } | |
| if (this.processedActionIds.has(action.actionid)) { | |
| continue; | |
| } | |
| this.processedActionIds.add(action.actionid); | |
| await this.handleBackendAction(action); | |
| } | |
| this.updateTree(); | |
| this.setter(this.localTreeStateAtom, { ...this.treeState }); | |
| this.persistToBackend(); | |
| } | |
| private async processPendingBackendActions() { | |
| const waveObj = this.getter(this.waveObjectAtom); | |
| const actions = waveObj?.pendingbackendactions; | |
| if (!actions?.length) return; | |
| for (const action of actions) { | |
| if (!action.actionid) { | |
| console.warn("Dropping layout action without actionid:", action); | |
| continue; | |
| } | |
| if (this.processedActionIds.has(action.actionid)) { | |
| continue; | |
| } | |
| this.processedActionIds.add(action.actionid); | |
| await this.handleBackendAction(action); | |
| } | |
| this.treeState.pendingBackendActions = undefined; | |
| this.updateTree(); | |
| this.setter(this.localTreeStateAtom, { ...this.treeState }); | |
| this.persistToBackend(); | |
| } |
| private async processPendingBackendActions() { | |
| const waveObj = this.getter(this.waveObjectAtom); | |
| const actions = waveObj?.pendingbackendactions; | |
| if (!actions?.length) return; | |
| this.treeState.pendingBackendActions = undefined; | |
| for (const action of actions) { | |
| if (!action.actionid) { | |
| console.warn("Dropping layout action without actionid:", action); | |
| continue; | |
| } | |
| if (this.processedActionIds.has(action.actionid)) { | |
| continue; | |
| } | |
| this.processedActionIds.add(action.actionid); | |
| await this.handleBackendAction(action); | |
| } | |
| this.updateTree(); | |
| this.setter(this.localTreeStateAtom, { ...this.treeState }); | |
| this.persistToBackend(); | |
| } | |
| private async processPendingBackendActions() { | |
| const waveObj = this.getter(this.waveObjectAtom); | |
| const actions = waveObj?.pendingbackendactions; | |
| if (!actions?.length) return; | |
| const unprocessedActions: LayoutActionData[] = []; | |
| for (const action of actions) { | |
| if (!action.actionid) { | |
| console.warn("Dropping layout action without actionid:", action); | |
| continue; | |
| } | |
| if (this.processedActionIds.has(action.actionid)) { | |
| continue; | |
| } | |
| try { | |
| await this.handleBackendAction(action); | |
| this.processedActionIds.add(action.actionid); | |
| } catch (error) { | |
| console.error("Failed to handle backend action:", action, error); | |
| unprocessedActions.push(action); | |
| } | |
| } | |
| this.treeState.pendingBackendActions = unprocessedActions.length > 0 | |
| ? unprocessedActions | |
| : undefined; | |
| this.updateTree(); | |
| this.setter(this.localTreeStateAtom, { ...this.treeState }); | |
| this.persistToBackend(); | |
| } |
🤖 Prompt for AI Agents
In frontend/layout/lib/layoutModel.ts around lines 383-405, the code clears
treeState.pendingBackendActions before processing, which loses remaining actions
if handleBackendAction throws; either move the assignment
treeState.pendingBackendActions = undefined to after the for-loop (and after
updateTree/setter/persistToBackend) so actions are only cleared on complete
success, or wrap the processing loop in try/catch and on error restore
treeState.pendingBackendActions to the unprocessed slice (or leave it intact
until all actions processed), then only updateTree/set localTreeStateAtom and
call persistToBackend when processing completes successfully.
1 participant
The current layout system uses a complex bidirectional atom architecture that forces every layout change to round-trip through the backend WaveObject, even though the backend never reads this data - it only queues actions via
PendingBackendActions. By switching to a "write cache" pattern where local atoms are the source of truth and backend writes are fire-and-forget, we can eliminate ~70% of the complexity while maintaining full persistence.Every layout change (split, close, focus, magnify) currently follows this flow:
Proposed "Write Cache" Architecture
Core Concept
Key Principles
PendingBackendActions