big simplifications to the preview mocks for rendering blocks#3082
big simplifications to the preview mocks for rendering blocks#3082
Conversation
…new jotaiStore import
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe PR moves many Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: |
b94538e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://010bc6cd.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-mock1.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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/preview/mock/mockwaveenv.ts (1)
311-320:⚠️ Potential issue | 🟡 MinorHardcoded stream handler detection may miss custom stream commands.
The override registration checks only for
"filereadstream"and"fileliststream"by name to decide whether to usesetStreamHandler. If a user registers a custom stream override viaRpcOverrides(e.g.,"CustomStreamCommand"), it would incorrectly be registered as a call handler instead of a stream handler.Consider using the handler's type or a more extensible pattern to determine if it's a stream handler.
🔧 Suggested fix using function type detection
if (overrides) { for (const key of Object.keys(overrides) as (keyof RpcOverrides)[]) { const cmdName = key.slice(0, -"Command".length).toLowerCase(); - if (cmdName === "filereadstream" || cmdName === "fileliststream") { - setStreamHandler(cmdName, overrides[key] as (...args: any[]) => AsyncGenerator<any, void, boolean>); + const handler = overrides[key]; + // Check if handler is an async generator function + if (handler?.constructor?.name === "AsyncGeneratorFunction") { + setStreamHandler(cmdName, handler as (...args: any[]) => AsyncGenerator<any, void, boolean>); } else { - setCallHandler(cmdName, overrides[key] as (...args: any[]) => Promise<any>); + setCallHandler(cmdName, handler as (...args: any[]) => Promise<any>); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/mock/mockwaveenv.ts` around lines 311 - 320, The current override registration uses hardcoded names ("filereadstream"/"fileliststream") to pick setStreamHandler vs setCallHandler; change it to detect the override function type instead: for each overrides[key] (from RpcOverrides) inspect the function's constructor.name (or prototype) to see if it's an "AsyncGeneratorFunction" (and optionally "GeneratorFunction") and call setStreamHandler(cmdName, ...) when true, otherwise call setCallHandler(cmdName, ...); update the loop around overrides, and ensure you reference the existing symbols overrides, RpcOverrides, setStreamHandler, setCallHandler and cmdName when implementing the type check so custom stream commands are correctly registered.
🧹 Nitpick comments (3)
frontend/preview/preview.tsx (1)
101-106: Unnecessary fragment wrapper.The
<></>fragment is redundant sinceTabModelContext.Providercan directly accept multiple children.🧹 Remove unnecessary fragment
<TabModelContext.Provider value={getTabModelByTabId(PreviewTabId, waveEnvRef.current)}> - <> - <PreviewApp /> - <PreviewContextMenu /> - </> + <PreviewApp /> + <PreviewContextMenu /> </TabModelContext.Provider>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/preview.tsx` around lines 101 - 106, The fragment wrapper around the children of TabModelContext.Provider is redundant—remove the <>...</> and pass PreviewApp and PreviewContextMenu directly as children of TabModelContext.Provider (the provider with value={getTabModelByTabId(PreviewTabId, waveEnvRef.current)}). Ensure the JSX now reads TabModelContext.Provider with PreviewApp and PreviewContextMenu nested directly inside, preserving the same order and props.frontend/preview/mock/mockwaveenv.ts (1)
18-23: **Consider documenting ID stability characteristics.**These IDs are generated once at module load time usingcrypto.randomUUID(). This feature has been available across browsers since March 2022, so browser compatibility is not a concern.However, developers should be aware that these IDs will change on each page reload (or HMR in development), which is expected behavior for the preview environment but worth documenting for debugging purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/mock/mockwaveenv.ts` around lines 18 - 23, Add a concise comment above the PreviewTabId, PreviewWindowId, PreviewWorkspaceId, PreviewClientId, WebBlockId, and SysinfoBlockId exports explaining these values are created once at module load time via crypto.randomUUID() and therefore will change on each full page reload or during HMR in development; state they are intentionally ephemeral for the preview environment and should not be relied on for long-term persistence or cross-session debugging so developers know to expect ID churn when investigating issues.frontend/preview/mock/use-rpc-override.ts (1)
8-15: Missing cleanup could leak handlers if component unmounts and remounts with different handler.The hook registers the override but never unregisters it. If a component using this hook unmounts and a new instance mounts with a different handler for the same command, the old handler remains registered. Consider returning a cleanup function or using
useEffectwith cleanup.Additionally, the generic constraint
K extends keyof RpcOverridessuggests type safety, but the handler parameter is alwaysRpcHandlerTyperather than the specific type for commandK. This weakens the type guarantee.♻️ Suggested approach with cleanup
-export function useRpcOverride<K extends keyof RpcOverrides>(command: K, handler: RpcHandlerType): void { +export function useRpcOverride<K extends keyof RpcOverrides>(command: K, handler: RpcHandlerType): void { const mockEnv = useWaveEnv() as MockWaveEnv; - const registeredRef = React.useRef(false); - if (!registeredRef.current) { - registeredRef.current = true; + React.useEffect(() => { mockEnv.addRpcOverride(command, handler); - } + // If MockWaveEnv supported removal, cleanup would go here: + // return () => mockEnv.removeRpcOverride(command); + }, [mockEnv, command, handler]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/mock/use-rpc-override.ts` around lines 8 - 15, The hook useRpcOverride currently registers a handler via mockEnv.addRpcOverride but never unregisters it and also types the handler too loosely; update useRpcOverride to register/unregister inside a React.useEffect that calls mockEnv.addRpcOverride(command, handler) in the effect body and returns a cleanup that calls mockEnv.removeRpcOverride(command, handler) (or remove by command if API requires) so handlers are removed on unmount or when handler changes, and tighten the handler type from RpcHandlerType to the specific handler type for the command (e.g., use the RpcOverrides[K] or a RpcHandlerFor<K> generic) so the handler parameter matches the command-specific signature; keep the existing MockWaveEnv and addRpcOverride/removeRpcOverride symbols to locate the changes.
🤖 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/aifilediff/aifilediff.tsx`:
- Line 9: The import currently referencing "@/store/jotaiStore" should be
updated to the actual module location used elsewhere so the module resolves;
replace the import statement that brings in globalStore from
"@/store/jotaiStore" with the correct module specifier used in the migration
(the module that exports globalStore from frontend/app/store/jotaiStore.ts) so
the named import globalStore continues to be used unchanged.
In `@frontend/preview/mock/mock-node-model.ts`:
- Line 17: The mock defines isMagnifiedAtom and a separate anyMagnified atom
pinned to false, so calling toggleMagnify (which flips isMagnifiedAtom) does not
update anyMagnified and consumers that read anyMagnified never see
magnification; fix by mirroring isMagnifiedAtom into anyMagnified in the mock
(e.g., have anyMagnified derive from or subscribe to isMagnifiedAtom so its
value reflects changes made by toggleMagnify), updating the mock-node-model.ts
atoms for the existing declarations around isMagnifiedAtom and the anyMagnified
definition (also apply same change to the other occurrences noted at lines
31–37).
In `@frontend/preview/previews/aifilediff.preview.tsx`:
- Around line 29-42: The useEffect calling env.createBlock currently ignores
rejection which can leave the component stuck; update the React.useEffect to
handle promise rejections from env.createBlock by adding a .catch or using
try/catch in an async IIFE, and on error call a recovery path such as setting an
error state or logging via console.error/processLogger and avoid leaving blockId
unset; target the createBlock call and setBlockId usage inside the
React.useEffect (or convert to an async function) so failures are surfaced and
the component can render an error or retry UI.
---
Outside diff comments:
In `@frontend/preview/mock/mockwaveenv.ts`:
- Around line 311-320: The current override registration uses hardcoded names
("filereadstream"/"fileliststream") to pick setStreamHandler vs setCallHandler;
change it to detect the override function type instead: for each overrides[key]
(from RpcOverrides) inspect the function's constructor.name (or prototype) to
see if it's an "AsyncGeneratorFunction" (and optionally "GeneratorFunction") and
call setStreamHandler(cmdName, ...) when true, otherwise call
setCallHandler(cmdName, ...); update the loop around overrides, and ensure you
reference the existing symbols overrides, RpcOverrides, setStreamHandler,
setCallHandler and cmdName when implementing the type check so custom stream
commands are correctly registered.
---
Nitpick comments:
In `@frontend/preview/mock/mockwaveenv.ts`:
- Around line 18-23: Add a concise comment above the PreviewTabId,
PreviewWindowId, PreviewWorkspaceId, PreviewClientId, WebBlockId, and
SysinfoBlockId exports explaining these values are created once at module load
time via crypto.randomUUID() and therefore will change on each full page reload
or during HMR in development; state they are intentionally ephemeral for the
preview environment and should not be relied on for long-term persistence or
cross-session debugging so developers know to expect ID churn when investigating
issues.
In `@frontend/preview/mock/use-rpc-override.ts`:
- Around line 8-15: The hook useRpcOverride currently registers a handler via
mockEnv.addRpcOverride but never unregisters it and also types the handler too
loosely; update useRpcOverride to register/unregister inside a React.useEffect
that calls mockEnv.addRpcOverride(command, handler) in the effect body and
returns a cleanup that calls mockEnv.removeRpcOverride(command, handler) (or
remove by command if API requires) so handlers are removed on unmount or when
handler changes, and tighten the handler type from RpcHandlerType to the
specific handler type for the command (e.g., use the RpcOverrides[K] or a
RpcHandlerFor<K> generic) so the handler parameter matches the command-specific
signature; keep the existing MockWaveEnv and addRpcOverride/removeRpcOverride
symbols to locate the changes.
In `@frontend/preview/preview.tsx`:
- Around line 101-106: The fragment wrapper around the children of
TabModelContext.Provider is redundant—remove the <>...</> and pass PreviewApp
and PreviewContextMenu directly as children of TabModelContext.Provider (the
provider with value={getTabModelByTabId(PreviewTabId, waveEnvRef.current)}).
Ensure the JSX now reads TabModelContext.Provider with PreviewApp and
PreviewContextMenu nested directly inside, preserving the same order and props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f080e79e-4e9a-49bf-ae5b-ef6d2ac4f81d
📒 Files selected for processing (22)
.kilocode/skills/create-view/SKILL.mdaiprompts/newview.mdfrontend/app/app.tsxfrontend/app/modals/modalsrenderer.tsxfrontend/app/view/aifilediff/aifilediff.tsxfrontend/app/view/preview/preview-edit.tsxfrontend/app/view/preview/preview-markdown.tsxfrontend/app/view/preview/preview-model.tsxfrontend/app/view/preview/preview-streaming.tsxfrontend/app/view/preview/preview.tsxfrontend/app/view/term/term.tsxfrontend/app/view/waveai/waveai.tsxfrontend/app/view/webview/webview.tsxfrontend/builder/app-selection-modal.tsxfrontend/builder/builder-app.tsxfrontend/preview/mock/mock-node-model.tsfrontend/preview/mock/mockwaveenv.tsfrontend/preview/mock/use-rpc-override.tsfrontend/preview/preview.tsxfrontend/preview/previews/aifilediff.preview.tsxfrontend/preview/previews/sysinfo.preview.tsxfrontend/preview/previews/web.preview.tsx
| import { DiffViewer } from "@/app/view/codeeditor/diffviewer"; | ||
| import type { WaveEnv, WaveEnvSubset } from "@/app/waveenv/waveenv"; | ||
| import { globalStore, WOS } from "@/store/global"; | ||
| import { globalStore } from "@/store/jotaiStore"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "# candidate jotaiStore modules"
fd 'jotaiStore\.(ts|tsx)$'
echo
echo "# globalStore exports / re-exports"
fd 'jotaiStore\.(ts|tsx)$' -x rg -n 'globalStore|export .* from'
echo
echo "# import under review"
sed -n '1,20p' frontend/app/view/aifilediff/aifilediff.tsxRepository: wavetermdev/waveterm
Length of output: 907
Update import path to match the actual module location.
The import on line 9 uses @/store/jotaiStore, but the only jotaiStore module exists at frontend/app/store/jotaiStore.ts. This import will fail module resolution. Update it to match the path used throughout the rest of the migration:
Required change
-import { globalStore } from "@/store/jotaiStore";
+import { globalStore } from "@/app/store/jotaiStore";📝 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.
| import { globalStore } from "@/store/jotaiStore"; | |
| import { globalStore } from "@/app/store/jotaiStore"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/aifilediff/aifilediff.tsx` at line 9, The import currently
referencing "@/store/jotaiStore" should be updated to the actual module location
used elsewhere so the module resolves; replace the import statement that brings
in globalStore from "@/store/jotaiStore" with the correct module specifier used
in the migration (the module that exports globalStore from
frontend/app/store/jotaiStore.ts) so the named import globalStore continues to
be used unchanged.
There was a problem hiding this comment.
in tsconfig.json:
"@/store/*": ["frontend/app/store/*"],
There was a problem hiding this comment.
@sawka, you're absolutely right — my mistake! The @/store/* alias in tsconfig.json maps to frontend/app/store/*, so @/store/jotaiStore correctly resolves to frontend/app/store/jotaiStore.ts. The import is perfectly valid. Sorry for the noise.
✏️ Learnings added
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 3082
File: frontend/app/view/aifilediff/aifilediff.tsx:9-9
Timestamp: 2026-03-18T23:02:25.531Z
Learning: In the wavetermdev/waveterm repository, the TypeScript path alias `"@/store/*"` maps to `"frontend/app/store/*"` (configured in tsconfig.json). So imports like `import { globalStore } from "@/store/jotaiStore"` correctly resolve to `frontend/app/store/jotaiStore.ts` and should NOT be flagged as incorrect import paths.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| React.useEffect(() => { | ||
| env.createBlock( | ||
| { | ||
| meta: { | ||
| view: "aifilediff", | ||
| file: DefaultAiFileDiffFileName, | ||
| "aifilediff:chatid": DefaultAiFileDiffChatId, | ||
| "aifilediff:toolcallid": DefaultAiFileDiffToolCallId, | ||
| }, | ||
| }, | ||
| }); | ||
| }, [baseEnv]); | ||
| false, | ||
| false | ||
| ).then((id) => setBlockId(id)); | ||
| }, []); |
There was a problem hiding this comment.
Missing error handling for createBlock promise.
If env.createBlock rejects, the error will be swallowed silently, leaving the component in a perpetual loading state. Consider adding error handling.
🛡️ Add error handling
React.useEffect(() => {
env.createBlock(
{
meta: {
view: "aifilediff",
file: DefaultAiFileDiffFileName,
"aifilediff:chatid": DefaultAiFileDiffChatId,
"aifilediff:toolcallid": DefaultAiFileDiffToolCallId,
},
},
false,
false
- ).then((id) => setBlockId(id));
+ ).then((id) => setBlockId(id))
+ .catch((err) => console.error("[AiFileDiffPreview] createBlock failed:", err));
}, []);📝 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.
| React.useEffect(() => { | |
| env.createBlock( | |
| { | |
| meta: { | |
| view: "aifilediff", | |
| file: DefaultAiFileDiffFileName, | |
| "aifilediff:chatid": DefaultAiFileDiffChatId, | |
| "aifilediff:toolcallid": DefaultAiFileDiffToolCallId, | |
| }, | |
| }, | |
| }); | |
| }, [baseEnv]); | |
| false, | |
| false | |
| ).then((id) => setBlockId(id)); | |
| }, []); | |
| React.useEffect(() => { | |
| env.createBlock( | |
| { | |
| meta: { | |
| view: "aifilediff", | |
| file: DefaultAiFileDiffFileName, | |
| "aifilediff:chatid": DefaultAiFileDiffChatId, | |
| "aifilediff:toolcallid": DefaultAiFileDiffToolCallId, | |
| }, | |
| }, | |
| false, | |
| false | |
| ).then((id) => setBlockId(id)) | |
| .catch((err) => console.error("[AiFileDiffPreview] createBlock failed:", err)); | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/preview/previews/aifilediff.preview.tsx` around lines 29 - 42, The
useEffect calling env.createBlock currently ignores rejection which can leave
the component stuck; update the React.useEffect to handle promise rejections
from env.createBlock by adding a .catch or using try/catch in an async IIFE, and
on error call a recovery path such as setting an error state or logging via
console.error/processLogger and avoid leaving blockId unset; target the
createBlock call and setBlockId usage inside the React.useEffect (or convert to
an async function) so failures are surfaced and the component can render an
error or retry UI.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge OverviewThe PR introduces a simplified mock environment system for preview testing. After reviewing the diff, no new issues were identified beyond those already flagged in existing comments.
Existing Comments Status
Files Reviewed (23 files)
Note: The existing comment about missing error handling for |
1 participant
No description provided.