introduce new models -- specifically tab model which has a react provider for prop drilling #2694
Conversation
WalkthroughThis PR refactors the frontend store architecture by introducing a model-based state management layer to replace direct atom-based global store access. It introduces three new singleton models (ClientModel, GlobalModel, TabModel) that manage client data, window/workspace data, and per-tab state respectively. The TabModel is then propagated through ViewModel constructors across multiple view files, replacing static global tab identifiers with dynamic tab-aware context. Global store atoms are removed or consolidated, and initialization flows are updated to leverage the new model instances. The changes affect approximately 30 files, with most modifications being constructor signature updates and store access pattern replacements. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 (4)
frontend/app/store/global-model.ts (1)
15-16: Definite assignment assertions on atoms require initialization before access.The
!assertions onwindowDataAtomandworkspaceAtomindicate these must be assigned before use. Accessing them beforeinitialize()is called would result inundefined. Consider documenting this requirement or adding a runtime check.🔎 Optional: Add getter with initialization check
- windowDataAtom!: Atom<WaveWindow>; - workspaceAtom!: Atom<Workspace>; + private _windowDataAtom?: Atom<WaveWindow>; + private _workspaceAtom?: Atom<Workspace>; + + get windowDataAtom(): Atom<WaveWindow> { + if (!this._windowDataAtom) { + throw new Error("GlobalModel not initialized"); + } + return this._windowDataAtom; + } + + get workspaceAtom(): Atom<Workspace> { + if (!this._workspaceAtom) { + throw new Error("GlobalModel not initialized"); + } + return this._workspaceAtom; + }frontend/app/view/waveai/waveai.tsx (1)
70-71:nodeModelandtabModelare stored but unused.The constructor accepts and stores
nodeModelandtabModelat lines 70-71 (94-97 in constructor), but neither is referenced anywhere else in the class. Consider removing them if not intended for future use, or add a comment explaining their purpose if they are placeholders.frontend/app/store/global.ts (1)
94-94: Verify inline window data retrieval doesn't impact performance.The window data is now retrieved inline on every access of
workspaceAtominstead of using a cachedwindowDataAtom. While functionally correct, this may recompute the window reference more frequently than the previous approach ifworkspaceAtomis accessed often.Verify whether this inline approach has any measurable performance impact in typical usage patterns. If
workspaceAtomis accessed frequently, consider whether caching the window data in a separate atom (as before) would be beneficial.frontend/app/store/tab-model.ts (1)
12-41: Consider lifecycle management for TabModel instances.The
TabModelclass creates atoms and ametaCachebut provides no disposal or cleanup mechanism. When TabModel instances accumulate in the cache (line 9), their internal structures (atoms, metaCache Map) also persist indefinitely. While Jotai atoms are lightweight, the metaCache can grow as different meta keys are accessed over the tab's lifetime.🔎 Add a disposal method to TabModel
Consider adding a
dispose()method to clean up internal state when a TabModel is no longer needed:class TabModel { tabId: string; tabAtom: Atom<Tab>; tabNumBlocksAtom: Atom<number>; isTermMultiInput = atom(false) as PrimitiveAtom<boolean>; metaCache: Map<string, Atom<any>> = new Map(); constructor(tabId: string) { this.tabId = tabId; this.tabAtom = atom((get) => { return WOS.getObjectValue(WOS.makeORef("tab", this.tabId), get); }); this.tabNumBlocksAtom = atom((get) => { const tabData = get(this.tabAtom); return tabData?.blockids?.length ?? 0; }); } getTabMetaAtom<T extends keyof MetaType>(metaKey: T): Atom<MetaType[T]> { let metaAtom = this.metaCache.get(metaKey); if (metaAtom == null) { metaAtom = atom((get) => { const tabData = get(this.tabAtom); return tabData?.meta?.[metaKey]; }); this.metaCache.set(metaKey, metaAtom); } return metaAtom; } + + dispose(): void { + this.metaCache.clear(); + // Additional cleanup if needed + } }This method can be called when removing a TabModel from the cache (see previous comment about cache cleanup).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
frontend/app/aipanel/aipanel.tsx(3 hunks)frontend/app/app.tsx(2 hunks)frontend/app/block/block.tsx(4 hunks)frontend/app/block/blockframe.tsx(3 hunks)frontend/app/modals/modalsrenderer.tsx(2 hunks)frontend/app/onboarding/onboarding-features.tsx(2 hunks)frontend/app/onboarding/onboarding-upgrade-minor.tsx(5 hunks)frontend/app/onboarding/onboarding-upgrade-patch.tsx(2 hunks)frontend/app/onboarding/onboarding-upgrade.tsx(2 hunks)frontend/app/onboarding/onboarding.tsx(5 hunks)frontend/app/store/client-model.ts(1 hunks)frontend/app/store/global-model.ts(1 hunks)frontend/app/store/global.ts(2 hunks)frontend/app/store/keymodel.ts(2 hunks)frontend/app/store/tab-model.ts(1 hunks)frontend/app/view/aifilediff/aifilediff.tsx(3 hunks)frontend/app/view/helpview/helpview.tsx(2 hunks)frontend/app/view/launcher/launcher.tsx(3 hunks)frontend/app/view/preview/preview-model.tsx(3 hunks)frontend/app/view/quicktipsview/quicktipsview.tsx(1 hunks)frontend/app/view/sysinfo/sysinfo.tsx(4 hunks)frontend/app/view/term/term-model.ts(4 hunks)frontend/app/view/term/term.tsx(3 hunks)frontend/app/view/term/termwrap.ts(3 hunks)frontend/app/view/tsunami/tsunami.tsx(4 hunks)frontend/app/view/vdom/vdom-model.tsx(3 hunks)frontend/app/view/waveai/waveai.tsx(4 hunks)frontend/app/view/waveconfig/waveconfig-model.ts(3 hunks)frontend/app/view/webview/webview.tsx(3 hunks)frontend/app/workspace/workspace.tsx(2 hunks)frontend/types/custom.d.ts(2 hunks)frontend/wave.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/app/modals/modalsrenderer.tsxfrontend/wave.tsfrontend/app/store/global-model.tsfrontend/app/app.tsxfrontend/app/onboarding/onboarding-features.tsxfrontend/app/store/client-model.tsfrontend/types/custom.d.tsfrontend/app/onboarding/onboarding-upgrade-minor.tsxfrontend/app/onboarding/onboarding-upgrade.tsxfrontend/app/onboarding/onboarding.tsxfrontend/app/store/global.ts
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: The onboarding upgrade modal in frontend/app/onboarding/onboarding-upgrade.tsx intentionally freezes the lastVersion at mount using a ref to prevent the modal from changing or disappearing mid-interaction when the user is going through the onboarding flow.
Applied to files:
frontend/app/modals/modalsrenderer.tsxfrontend/app/onboarding/onboarding-upgrade-minor.tsxfrontend/app/onboarding/onboarding-upgrade.tsxfrontend/app/onboarding/onboarding.tsxfrontend/app/onboarding/onboarding-upgrade-patch.tsx
📚 Learning: 2025-11-01T00:57:23.025Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2504
File: frontend/app/aipanel/aipanel-contextmenu.ts:15-16
Timestamp: 2025-11-01T00:57:23.025Z
Learning: In the waveterm codebase, types defined in custom.d.ts are globally available and do not require explicit imports. Backend types defined in gotypes.d.ts are also globally available.
Applied to files:
frontend/app/store/global-model.tsfrontend/app/app.tsxfrontend/types/custom.d.ts
📚 Learning: 2025-10-15T03:18:52.647Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: emain/emain-window.ts:811-828
Timestamp: 2025-10-15T03:18:52.647Z
Learning: In emain/emain-window.ts, within the relaunchBrowserWindows function, ClientService.GetClientData() is guaranteed to return a valid client object and never null/undefined. The backend ensures a client is initialized before startup, so no null-guard is needed when accessing clientData.windowids.
Applied to files:
frontend/app/app.tsxfrontend/app/store/client-model.tsfrontend/app/store/global.ts
🧬 Code graph analysis (21)
frontend/app/view/preview/preview-model.tsx (2)
frontend/app/store/tab-model.ts (1)
TabModel(70-70)frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)
frontend/app/modals/modalsrenderer.tsx (1)
frontend/app/store/client-model.ts (1)
ClientModel(36-36)
frontend/wave.ts (3)
frontend/app/store/global.ts (2)
globalStore(829-829)initGlobal(830-830)frontend/app/store/tab-model.ts (1)
activeTabIdAtom(70-70)frontend/app/store/global-model.ts (1)
GlobalModel(52-52)
frontend/app/view/vdom/vdom-model.tsx (2)
frontend/app/store/tab-model.ts (1)
TabModel(70-70)frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)
frontend/app/onboarding/onboarding-features.tsx (1)
frontend/app/store/client-model.ts (1)
ClientModel(36-36)
frontend/app/view/waveai/waveai.tsx (3)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/store/tab-model.ts (1)
TabModel(70-70)frontend/app/store/client-model.ts (1)
ClientModel(36-36)
frontend/app/view/quicktipsview/quicktipsview.tsx (2)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/store/tab-model.ts (1)
TabModel(70-70)
frontend/app/view/waveconfig/waveconfig-model.ts (2)
frontend/app/store/tab-model.ts (1)
TabModel(70-70)frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)
frontend/app/block/blockframe.tsx (4)
frontend/app/store/tab-model.ts (1)
useTabModel(70-70)frontend/app/store/global.ts (2)
atoms(806-806)getSettingsKeyAtom(825-825)frontend/app/block/block-model.ts (1)
BlockModel(12-51)pkg/waveobj/wtype.go (2)
Block(282-290)Block(292-294)
frontend/app/view/aifilediff/aifilediff.tsx (2)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/store/tab-model.ts (1)
TabModel(70-70)
frontend/app/view/helpview/helpview.tsx (2)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/store/tab-model.ts (1)
TabModel(70-70)
frontend/app/onboarding/onboarding-upgrade.tsx (1)
frontend/app/store/client-model.ts (1)
ClientModel(36-36)
frontend/app/onboarding/onboarding.tsx (1)
frontend/app/store/client-model.ts (1)
ClientModel(36-36)
frontend/app/view/webview/webview.tsx (2)
frontend/app/store/tab-model.ts (1)
TabModel(70-70)frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)
frontend/app/onboarding/onboarding-upgrade-patch.tsx (1)
frontend/app/store/client-model.ts (1)
ClientModel(36-36)
frontend/app/view/tsunami/tsunami.tsx (2)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/store/tab-model.ts (1)
TabModel(70-70)
frontend/app/view/sysinfo/sysinfo.tsx (2)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/store/tab-model.ts (1)
TabModel(70-70)
frontend/app/view/term/term-model.ts (1)
frontend/app/store/tab-model.ts (1)
TabModel(70-70)
frontend/app/view/launcher/launcher.tsx (2)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/store/tab-model.ts (1)
TabModel(70-70)
frontend/app/aipanel/aipanel.tsx (1)
frontend/app/store/tab-model.ts (1)
useTabModel(70-70)
frontend/app/store/tab-model.ts (2)
frontend/layout/lib/nodeRefMap.ts (1)
get(20-24)frontend/app/store/global.ts (1)
globalStore(829-829)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (47)
frontend/app/view/quicktipsview/quicktipsview.tsx (2)
4-5: LGTM! Constructor signature aligns with PR's model refactoring pattern.The new fields (
blockId,nodeModel,tabModel) follow the same pattern described in the PR objectives for threading per-tab context through view models.Also applies to: 12-14
18-41: Constructor implementation is correct; confirm the new fields are intentionally unused.The constructor properly initializes all three new parameters. However, none of the new fields (
blockId,nodeModel,tabModel) are currently accessed in the class or theQuickTipsViewcomponent.If this is intentional infrastructure setup for future use or consistency with other view models, this is fine. Otherwise, consider whether any of these should be utilized (e.g., passing
tabModelto child components, usingnodeModel.onClose()for cleanup, etc.).Can you confirm these fields are being stored for future use or consistency with the view model pattern across the codebase?
frontend/app/view/waveconfig/waveconfig-model.ts (1)
5-5: LGTM! TabModel threading is consistent with the PR's per-tab refactor.The changes properly thread TabModel through the constructor and store it as a field. While
tabModelisn't actively used in this file yet, this is consistent with the broader refactor pattern seen across multiple view models and sets up the infrastructure for per-tab context usage.Also applies to: 146-146, 173-176
frontend/app/block/block.tsx (2)
27-28: LGTM! Proper integration of TabModel into view model creation.The changes correctly:
- Import TabModel type and useTabModel hook
- Update makeViewModel signature to accept tabModel
- Thread tabModel through to view model constructors
This establishes the foundation for per-tab context throughout the block system.
Also applies to: 60-60, 63-63
266-266: LGTM! Proper use of useTabModel hook in Block and SubBlock components.Both components correctly:
- Obtain tabModel via useTabModel hook
- Pass tabModel when creating/updating view models via makeViewModel
This ensures each block instance has access to its tab context.
Also applies to: 271-271, 292-292, 297-297
frontend/app/view/aifilediff/aifilediff.tsx (1)
4-5: LGTM! Consistent TabModel and BlockNodeModel integration.The changes properly:
- Add type-only imports for BlockNodeModel and TabModel
- Declare fields on the view model
- Expand constructor signature to accept both parameters
- Initialize fields in constructor
This aligns with the project-wide refactor to propagate per-tab context through view models.
Also applies to: 22-23, 33-36
frontend/app/view/tsunami/tsunami.tsx (2)
5-6: LGTM! Proper TabModel integration with inheritance.The changes correctly:
- Import TabModel type
- Accept tabModel in constructor
- Forward tabModel to parent WebViewModel via super()
This ensures TsunamiViewModel inherits per-tab context from its parent.
Also applies to: 24-25
110-110: LGTM! Excellent use of per-tab context for RPC calls.The code now uses
this.tabModel.tabIdto provide the correct tab identifier for ControllerResyncCommand, replacing what was likely a global tab ID lookup. This demonstrates the value of the TabModel refactor—ensuring each tsunami instance operates with its own tab context.Also applies to: 138-138
frontend/app/view/sysinfo/sysinfo.tsx (2)
4-5: LGTM! Consistent refactor with intentional constructor signature change.The changes:
- Add type imports for BlockNodeModel and TabModel
- Declare fields on the view model
- Update constructor to accept nodeModel and tabModel (removing viewType parameter)
- Hardcode viewType to "sysinfo"
The removal of the viewType parameter aligns with making SysinfoViewModel a specialized view model rather than a generic one, which is consistent with the pattern in other view models.
Also applies to: 97-98, 121-124
545-549: LGTM! Cosmetic formatting improvement.The className prop formatting change improves readability without affecting behavior.
frontend/types/custom.d.ts (2)
57-65: LGTM! New initialization options type.The GlobalInitOptions type provides a structured way to pass initialization parameters (platform, windowId, clientId, environment, optional tabId/builderId/primaryTabStartup) to the global model infrastructure. This supports the broader refactor to model-based state management.
294-294: LGTM! ViewModelClass signature updated to enforce per-tab context.The updated constructor signature now requires:
blockId: stringnodeModel: BlockNodeModeltabModel: TabModelThis type-level enforcement ensures all view models adopt the new per-tab context pattern, preventing inconsistencies.
frontend/app/onboarding/onboarding-upgrade.tsx (1)
4-4: LGTM! Proper migration to ClientModel singleton.The change correctly migrates from the global
atoms.clienttoClientModel.getInstance().clientAtom, aligning with the broader refactor to centralize client state management in singleton models. Based on learnings, clientData is loaded and awaited before React runs, ensuring availability at mount time.Also applies to: 15-15
frontend/app/view/preview/preview-model.tsx (1)
5-5: LGTM! Consistent TabModel integration into PreviewModel.The changes properly:
- Add type-only import for TabModel
- Declare tabModel field on PreviewModel
- Expand constructor to accept tabModel parameter
- Initialize the field in constructor
This aligns with the project-wide refactor to propagate per-tab context through view models, establishing infrastructure for future per-tab functionality.
Also applies to: 122-122, 172-172, 176-176
frontend/app/workspace/workspace.tsx (1)
10-10: LGTM! Proper TabModel context provider setup.The TabModelContext.Provider correctly wraps TabContent with the per-tab model instance, enabling child components to access tab-specific state. The conditional rendering ensures the provider is only active when a valid tab exists.
Also applies to: 82-84
frontend/app/onboarding/onboarding-upgrade-patch.tsx (1)
8-9: LGTM! Clean migration to ClientModel.The replacement of
globalStore.get(atoms.clientId)withClientModel.getInstance().clientIdaligns with the broader refactor to use singleton models. Based on the retrieved learnings, clientData is loaded before React runs, so the clientId will always be available.Also applies to: 102-102
frontend/app/onboarding/onboarding-upgrade-minor.tsx (1)
9-10: LGTM! Consistent ClientModel usage across handlers.All four event handlers now consistently retrieve clientId from the ClientModel singleton. The pattern is uniform and aligns with the PR's model-based refactor.
Also applies to: 64-64, 82-82, 99-99, 108-108
frontend/app/view/term/term.tsx (2)
269-269: LGTM! Tab ID correctly propagated to TermWrap.Passing
tabModel.tabIdto the TermWrap constructor enables the terminal to maintain proper tab-specific context for RPC calls and state management.
6-6: LGTM! Proper per-tab state integration.The
useTabModel()hook correctly implements per-tab context to the terminal view. The hook is called at the component's top level (line 168 in TerminalView, following React hooks rules), and includes appropriate error handling by throwing when the context is undefined—the standard best practice pattern. ThetabModel.isTermMultiInput(line 173) correctly accesses tab-specific state instead of relying on global atoms.frontend/app/modals/modalsrenderer.tsx (1)
7-7: LGTM! Atom source updated to ClientModel.The replacement of
atoms.clientwithClientModel.getInstance().clientAtommaintains the reactive subscription pattern while aligning with the model-based architecture. The logic and effects remain unchanged.Also applies to: 16-16
frontend/app/onboarding/onboarding-features.tsx (1)
8-8: LGTM! ClientModel integration in mount effect.The replacement of
globalStore.get(atoms.clientId)withClientModel.getInstance().clientIdin the mount effect follows the established pattern for accessing client context.Also applies to: 317-317
frontend/app/app.tsx (1)
4-5: LGTM! Dual model integration with proper null safety.The app correctly retrieves client and window data from their respective singleton models (
ClientModelandGlobalModel). The existing null check at line 282 ensures graceful handling of any initialization issues.Also applies to: 278-279
frontend/app/aipanel/aipanel.tsx (1)
9-9: LGTM! Per-tab context for AI requests.The integration of
useTabModel()correctly provides tab-specific context for AI chat requests. The conditional logic appropriately usestabModel.tabIdfor regular windows while preserving the builder-specific identifiers for builder windows.Also applies to: 258-258, 282-282
frontend/app/store/keymodel.ts (1)
590-602: LGTM! Per-tab multi-input state is correctly implemented.The refactor properly scopes the terminal multi-input toggle to the active tab using
getActiveTabModel()instead of a global atom. The null check on line 592-594 appropriately handles cases where no active tab model exists.frontend/app/view/vdom/vdom-model.tsx (1)
109-109: LGTM! TabModel integration is clean.The constructor signature update correctly accepts and stores the
tabModelparameter, aligning with the broader refactor to provide per-tab context to view models.Also applies to: 143-147
frontend/app/onboarding/onboarding.tsx (1)
33-33: LGTM! ClientModel migration is correct.The changes properly centralize client data access through
ClientModel.getInstance(), replacing direct global store access. This aligns with the PR's goal of introducing singleton models for state management.Also applies to: 160-160, 170-170, 230-230
frontend/app/store/client-model.ts (1)
7-34: LGTM! ClientModel singleton is well-implemented.The singleton pattern is correctly applied with a private constructor and static
getInstance()method. Theinitialize()method properly sets up theclientAtomwith a getter that safely handles nullclientIdvalues. Based on learnings, clientData is guaranteed to be loaded before React runs, so the null check on line 28 is appropriately defensive.frontend/app/block/blockframe.tsx (2)
496-505: LGTM! Per-tab styling is correctly implemented.The BlockMask component now properly sources tab-specific border colors from
tabModel.getTabMetaAtom()instead of global state, enabling per-tab border color customization.
677-680: LGTM! Block count now sourced from tab model.Using
tabModel.tabNumBlocksAtomcorrectly provides the per-tab block count, replacing the previous global computation.frontend/app/view/launcher/launcher.tsx (1)
25-26: LGTM! Constructor signature update is consistent.The LauncherViewModel constructor now accepts
nodeModelandtabModelparameters, aligning with the broader pattern of providing per-tab context to view models across the codebase.Also applies to: 38-42
frontend/app/view/webview/webview.tsx (1)
48-48: LGTM! TabModel parameter added consistently.The WebViewModel constructor correctly accepts and stores the
tabModelparameter, following the same pattern applied across other view models in this PR.Also applies to: 74-76
frontend/app/view/helpview/helpview.tsx (1)
19-20: LGTM! TabModel properly forwarded to superclass.The HelpViewModel correctly extends its constructor to accept
tabModeland forwards it to the WebViewModel superclass viasuper(), maintaining the inheritance chain properly.frontend/wave.ts (3)
5-5: LGTM on new imports.The new imports for
GlobalModelandactiveTabIdAtomalign with the model-based architecture introduced in this PR.Also applies to: 34-34
156-167: Initialization sequence looks correct.The order is appropriate: set
activeTabIdAtomin the store, then awaitGlobalModel.initialize(), then callinitGlobal(). This ensures the tab ID is available before model initialization, and the GlobalModel (which initializes ClientModel internally) is ready before the rest of global setup.Based on learnings, clientData is loaded and awaited in wave.ts before React runs, so atoms.client will have data on first render.
234-243: Builder initialization follows the same pattern consistently.The builder path mirrors the wave init path, which is good for maintainability. Note that
builderIdis set inglobalInitOptshere (vstabIdininitWave), which correctly reflects the different context.frontend/app/view/term/termwrap.ts (2)
344-345: Clean dependency injection pattern.Adding
tabIdas a constructor parameter and storing it as an instance field is a cleaner approach than reading from global state. This makes the class more testable and explicit about its dependencies.Also applies to: 383-391
701-713: Correct usage of instancetabIdin resync.Using
this.tabIdinstead of reading from a global atom properly scopes the tab context to eachTermWrapinstance, supporting multiple tabs correctly.frontend/app/view/term/term-model.ts (3)
6-6: Good integration of TabModel into TermViewModel.The
TabModelis properly imported, stored as an instance field, and initialized in the constructor. This enables per-tab state access throughout the view model.Also applies to: 41-41, 72-75
187-198: Correct usage oftabModel.isTermMultiInput.Both reading (line 187) and writing (line 195) the multi-input state now go through the tab model, which is the correct pattern for per-tab state.
638-654: Inconsistent tab ID access inforceRestartController.This method uses
globalStore.get(atoms.staticTabId)(line 648), while the rest of the class usestabModelfor tab context. Consider usingthis.tabModel.tabIdfor consistency.forceRestartController() { if (globalStore.get(this.isRestarting)) { return; } this.triggerRestartAtom(); const termsize = { rows: this.termRef.current?.terminal?.rows, cols: this.termRef.current?.terminal?.cols, }; const prtn = RpcApi.ControllerResyncCommand(TabRpcClient, { - tabid: globalStore.get(atoms.staticTabId), + tabid: this.tabModel.tabId, blockid: this.blockId, forcerestart: true, rtopts: { termsize: termsize }, }); prtn.catch((e) => console.log("error controller resync (force restart)", e)); }frontend/app/store/global-model.ts (2)
8-27: Singleton pattern correctly implemented.The private constructor and static
getInstance()method properly enforce the singleton pattern. The instance is lazily created on first access.
29-49: No action needed.
ClientModel.initialize()is synchronous with a return type ofvoid, so calling it withoutawaitis correct and does not pose a race condition risk.Likely an incorrect or invalid review comment.
frontend/app/view/waveai/waveai.tsx (2)
4-4: New imports support model-based architecture.The imports for
BlockNodeModel,ClientModel, andTabModelenable the constructor changes and the new client ID access pattern.Also applies to: 8-9
346-348: ClientModel access pattern is correct.Using
ClientModel.getInstance().clientIdaligns with the new singleton-based architecture and replaces the previous global atom access.frontend/app/store/tab-model.ts (2)
62-68: LGTM: Standard React context pattern.The
useTabModel()hook correctly throws an error when used outside aTabModelProvider. This is a standard and appropriate pattern for React context hooks, ensuring that components using the hook are properly nested within the provider.
10-10: Initialization is correct—activeTabIdAtom is set before first render.The atom is properly initialized in
wave.ts:165duringinitWave()before React renders. BothgetActiveTabModel()anduseTabModel()handle null values appropriately: the former returns null when the atom is null, and the latter uses React Context which is provided by a parent component. This follows the same pattern asclientDatamentioned in the codebase.Likely an incorrect or invalid review comment.
frontend/app/store/global.ts (1)
184-184: Type assertion is valid and not masking any structural issues.The atoms object returned in
global.tsmatches theGlobalAtomsTypedefinition exactly. Each property in the atoms object (builderId, builderAppId, waveWindowType, uiContext, workspace, fullConfigAtom, waveaiModeConfigAtom, settingsAtom, hasCustomAIPresetsAtom, staticTabId, isFullScreen, zoomFactorAtom, controlShiftDelayAtom, updaterStatusAtom, prefersReducedMotionAtom, modalOpen, allConnStatus, flashErrors, notifications, notificationPopoverMode, reinitVersion, waveAIRateLimitInfoAtom) is defined in the type, and no extraneous properties are being cast.Additionally, codebase searches found zero references to the atoms claimed to be removed (clientIdAtom, clientAtom, windowDataAtom, tabAtom, typeAheadModalAtom, isTermMultiInput), confirming they are either not part of this refactor or were already removed and properly updated everywhere.
The
as GlobalAtomsTypeassertion is appropriate here—the compiler can verify structural compatibility, and no legitimate type errors are being masked.Likely an incorrect or invalid review comment.
| import { globalStore } from "./jotaiStore"; | ||
| import * as WOS from "./wos"; | ||
|
|
||
| const tabModelCache = new Map<string, TabModel>(); |
There was a problem hiding this comment.
Memory leak: unbounded cache growth.
The tabModelCache Map grows indefinitely as new tabs are created. When tabs are closed, their corresponding TabModel instances remain in the cache and are never removed, leading to a memory leak. Over time, this accumulates stale TabModel instances, each holding references to atoms and metadata caches.
🔎 Add a cleanup mechanism for closed tabs
Consider adding a method to remove TabModel instances when tabs are closed:
+function removeTabModel(tabId: string): void {
+ tabModelCache.delete(tabId);
+}
+
export { activeTabIdAtom, getActiveTabModel, getTabModelByTabId, TabModel, TabModelContext, useTabModel };Then call removeTabModel(tabId) when a tab is closed (e.g., via a wave event subscription for tab deletion).
Alternatively, consider using a WeakMap if the tab lifecycle allows for garbage collection based on reference counting, though this requires careful consideration of how tab references are held.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/app/store/tab-model.ts around line 9, tabModelCache is an unbounded
Map that retains TabModel instances after tabs are closed, causing a memory
leak; add a cleanup API such as removeTabModel(tabId: string) that looks up and
deletes the entry (and optionally calls a dispose/cleanup method on the TabModel
to release internal resources), then invoke removeTabModel when a tab is closed
(e.g., subscribe to the tab deletion/wave event handler), or if lifecycle
semantics permit, replace the Map with a WeakMap keyed by a tab-owned object to
allow GC; ensure any code that retrieved TabModels tolerates missing entries and
re-creates them as needed.
No description provided.