Expand WaveEnv to cover all the deps in sysinfo.tsx#3019
Conversation
WalkthroughThis pull request refactors the view model initialization architecture by consolidating view model constructors from accepting three separate positional parameters (blockId, nodeModel, tabModel) into a single object parameter of type ViewModelInitType. The change introduces WaveEnv as a propagated dependency through the block component factory and view model construction flow. Additionally, the WaveEnv interface is extended with methods for accessing connection status atoms, wave object atoms, and block metadata atoms. Atom APIs are simplified to remove write capabilities from getWaveObjectAtom and getLayoutStateAtomFromTab. The mock environment testing utilities are substantially expanded with new types, helper functions, and lazy initialization patterns. Minor updates include copyright year changes (2025 to 2026) and removal of unused code. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Deploying waveterm with
|
| Latest commit: |
8073e2a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c0401f6c.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-waveenv-2.waveterm.pages.dev |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/block/block.tsx (1)
318-331:⚠️ Potential issue | 🟠 MajorDispose superseded view models before re-registering them.
Line 323 and Line 350 can replace the cached model after the initial loading render, but the cleanup at Line 329 and Line 356 still closes over the instance from that first render. The real model therefore never gets
dispose()on unmount, and view switches leak the old model’s subscriptions/routes.♻️ One way to fix it
const bcm = getBlockComponentModel(props.nodeModel.blockId); let viewModel = bcm?.viewModel; - if (viewModel == null || viewModel.viewType != blockData?.meta?.view) { + if (!loading && blockData != null && (viewModel == null || viewModel.viewType != blockData.meta?.view)) { + bcm?.viewModel?.dispose?.(); viewModel = makeViewModel(props.nodeModel.blockId, blockData?.meta?.view, props.nodeModel, tabModel, waveEnv); registerBlockComponentModel(props.nodeModel.blockId, { viewModel }); } useEffect(() => { return () => { + const current = getBlockComponentModel(props.nodeModel.blockId)?.viewModel; unregisterBlockComponentModel(props.nodeModel.blockId); - viewModel?.dispose?.(); + current?.dispose?.(); }; - }, []); + }, [props.nodeModel.blockId]);Apply the same change in
SubBlock.Also applies to: 345-358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/block/block.tsx` around lines 318 - 331, The cached viewModel can be replaced after initial render causing the effect cleanup to close over the old instance and never dispose the newer model; before calling registerBlockComponentModel(props.nodeModel.blockId, { viewModel }) ensure you dispose any existing cached model for that blockId (retrieve current model via getBlockComponentModel and call its dispose()), or store the active viewModel in a mutable ref and update it when you create a new one so the useEffect cleanup (unregisterBlockComponentModel and viewModel?.dispose()) always disposes the latest instance; apply the same fix in SubBlock as well and reference makeViewModel, registerBlockComponentModel, unregisterBlockComponentModel, viewModel, dispose, and props.nodeModel.blockId when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/sysinfo/sysinfo.tsx`:
- Line 266: The call to this.getDefaultData() is unused and should be removed or
its return value should be consumed; either delete the pointless invocation from
the lifecycle/handler where it appears, or capture its return (e.g., const
defaultData = this.getDefaultData()) and use it to initialize component state or
pass it into the consumer that needs default data (look for getDefaultData in
this class and the call site where this.getDefaultData() is invoked). Ensure you
only keep the call if you actually use the produced array (assign to
state/prop/variable) otherwise remove it to eliminate dead code.
---
Outside diff comments:
In `@frontend/app/block/block.tsx`:
- Around line 318-331: The cached viewModel can be replaced after initial render
causing the effect cleanup to close over the old instance and never dispose the
newer model; before calling registerBlockComponentModel(props.nodeModel.blockId,
{ viewModel }) ensure you dispose any existing cached model for that blockId
(retrieve current model via getBlockComponentModel and call its dispose()), or
store the active viewModel in a mutable ref and update it when you create a new
one so the useEffect cleanup (unregisterBlockComponentModel and
viewModel?.dispose()) always disposes the latest instance; apply the same fix in
SubBlock as well and reference makeViewModel, registerBlockComponentModel,
unregisterBlockComponentModel, viewModel, dispose, and props.nodeModel.blockId
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 35c56c63-a2b3-4e56-9746-bb1a449bce84
📒 Files selected for processing (25)
frontend/app/block/block.tsxfrontend/app/store/global.tsfrontend/app/store/wos.tsfrontend/app/tab/tabbar.tsxfrontend/app/view/aifilediff/aifilediff.tsxfrontend/app/view/helpview/helpview.tsxfrontend/app/view/launcher/launcher.tsxfrontend/app/view/preview/preview-model.tsxfrontend/app/view/quicktipsview/quicktipsview.tsxfrontend/app/view/sysinfo/sysinfo.tsxfrontend/app/view/term/term-model.tsfrontend/app/view/tsunami/tsunami.tsxfrontend/app/view/vdom/vdom-model.tsxfrontend/app/view/waveai/waveai.tsxfrontend/app/view/waveconfig/waveconfig-model.tsfrontend/app/view/webview/webview.tsxfrontend/app/waveenv/waveenv.tsfrontend/app/waveenv/waveenvimpl.tsfrontend/layout/lib/layoutAtom.tsfrontend/layout/lib/layoutModel.tsfrontend/preview/index.htmlfrontend/preview/mock/mockwaveenv.tsfrontend/preview/preview.tsxfrontend/preview/previews/widgets.preview.tsxfrontend/types/custom.d.ts
| return; | ||
| } | ||
| const newData = this.getDefaultData(); | ||
| this.getDefaultData(); |
There was a problem hiding this comment.
Unused return value from getDefaultData().
The call to this.getDefaultData() creates an array but doesn't use the returned value. Since getDefaultData() has no side effects, this appears to be dead code—possibly a leftover from refactoring.
🔧 Suggested fix: remove or use the return value
- this.getDefaultData();
const initialDataItems: DataItem[] = initialData.map(convertWaveEventToDataItem);Or if the default data is needed:
- this.getDefaultData();
+ const defaultData = this.getDefaultData();
+ // Use defaultData as needed...
const initialDataItems: DataItem[] = initialData.map(convertWaveEventToDataItem);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/sysinfo/sysinfo.tsx` at line 266, The call to
this.getDefaultData() is unused and should be removed or its return value should
be consumed; either delete the pointless invocation from the lifecycle/handler
where it appears, or capture its return (e.g., const defaultData =
this.getDefaultData()) and use it to initialize component state or pass it into
the consumer that needs default data (look for getDefaultData in this class and
the call site where this.getDefaultData() is invoked). Ensure you only keep the
call if you actually use the produced array (assign to state/prop/variable)
otherwise remove it to eliminate dead code.
1 participant
No description provided.