Conversation
|
Caution Review failedPull request was closed or merged during review 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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces module-scoped mock RPC wiring with an exported RpcApiType that holds an instance-scoped Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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 |
frontend/app/workspace/widgets.tsx
Outdated
| <div | ||
| ref={containerRef} | ||
| className="flex flex-col w-12 overflow-hidden py-1 -ml-1 select-none" | ||
| className="flex flex-col w-12 overflow-hidden py-1 -ml-1 select-none shink-0" |
There was a problem hiding this comment.
WARNING: Typo in Tailwind class - shink-0 should be shrink-0
The class shrink-0 is the correct Tailwind CSS class to prevent flex items from shrinking, while shink-0 is not a valid Tailwind class.
| className="flex flex-col w-12 overflow-hidden py-1 -ml-1 select-none shink-0" | |
| className="flex flex-col w-12 overflow-hidden py-1 -ml-1 select-none shrink-0" |
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge OverviewThis PR is a refactoring that converts
Notes
Files Reviewed (9 files)
|
Deploying waveterm with
|
| Latest commit: |
25b2187
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8fbc9f57.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-waveenv.waveterm.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/app/workspace/widgets.tsx (1)
139-163: Missingenvin dependency array.The
useEffectusesenv.rpc.ListAllAppsCommandbut only lists[isOpen]as a dependency. WhileenvfromuseWaveEnvis likely stable, ESLint'sreact-hooks/exhaustive-depsrule would flag this. Consider addingenvto the dependency array for correctness.♻️ Proposed fix
- }, [isOpen]); + }, [isOpen, env]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/workspace/widgets.tsx` around lines 139 - 163, The effect uses env.rpc.ListAllAppsCommand (and TabRpcClient) but only lists isOpen as a dependency; add env to the dependency array of the useEffect (or memoize/stabilize env if it should be omitted) so the hook re-runs when env changes; update the dependency list that wraps the effect which contains fetchApps, setLoading, setApps and isOpen to include env (or replace env with a stable ref) to satisfy react-hooks/exhaustive-deps.frontend/preview/mock/mockwaveenv.ts (1)
57-68: Module-level singleton RPC instance is shared across all mock environments.The
mockRpcApiis created once at module load and reused by all calls tomakeMockWaveEnv. This works for basic scenarios but could cause issues if consumers mutate the RPC instance (e.g., overridingListAllAppsCommandinwidgets.preview.tsx). For preview/testing this is likely acceptable, but be aware that mutations will affect all environments.🤖 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 57 - 68, The module currently creates a singleton mockRpcApi (RpcApiType) at load time which is shared across all makeMockWaveEnv calls; change the implementation to instantiate a new RpcApiType inside makeMockWaveEnv (or otherwise return a fresh RpcApiType per environment) so each mock environment gets its own rpc instance; move the setMockRpcClient call (and the mockWshRpcCall/mockWshRpcStream handlers) to be applied to that per-environment RpcApiType created within makeMockWaveEnv so mutations (e.g., overrides to ListAllAppsCommand in widgets.preview.tsx) do not leak between environments.frontend/app/waveenv/waveenv.ts (1)
21-29: Consider adding a runtime guard or throwing an error when context is missing.
WaveEnvContextis created withnullas the default value, anduseWaveEnvcasts the result directly without a null check. If a component accidentally callsuseWaveEnvoutside aWaveEnvContext.Provider, it will receivenulland fail cryptically when accessing properties.Based on learnings, the codebase ensures context providers are mounted before components render, so this may be intentional. However, a defensive check would provide a clearer error message during development.
🛡️ Optional: Add a runtime check for clearer error messages
export function useWaveEnv<T extends EnvContract<WaveEnv> = WaveEnv>(): T { - return React.useContext(WaveEnvContext) as T; + const env = React.useContext(WaveEnvContext); + if (env == null) { + throw new Error("useWaveEnv must be used within a WaveEnvContext.Provider"); + } + return env as T; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/waveenv/waveenv.ts` around lines 21 - 29, The hook useWaveEnv currently casts React.useContext(WaveEnvContext) to T without checking for null, which yields cryptic failures if used outside a provider; update useWaveEnv to perform a runtime guard: read ctx = React.useContext(WaveEnvContext) and if ctx is null/undefined throw a clear Error (e.g. "useWaveEnv must be used within a WaveEnvContext.Provider") or return a safe default in development, and consider changing the WaveEnvContext default from null to undefined to match the guard; reference WaveEnvContext, useWaveEnv, and the WaveEnv type 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/workspace/widgets.tsx`:
- Line 444: Fix the typo in the Tailwind class string used in the component
where className includes "flex flex-col w-12 overflow-hidden py-1 -ml-1
select-none shink-0": replace the misspelled "shink-0" with the correct
"shrink-0" so the flex item does not shrink; update the className value in the
same JSX/TSX element (look for the string containing "select-none shink-0") to
apply the correct Tailwind utility.
In `@frontend/preview/previews/widgets.preview.tsx`:
- Around line 83-98: makeWidgetsEnv mutates the shared RPC instance returned by
makeMockWaveEnv by assigning baseEnv.rpc.ListAllAppsCommand, causing
cross-contamination between WidgetsScenario instances; instead create a new RPC
object (clone or shallow-copy baseEnv.rpc) and set ListAllAppsCommand on that
copy, then return the env with rpc: copiedRpc so makeWidgetsEnv does not modify
the module-level mockRpcApi used by other scenarios (refer to makeWidgetsEnv,
makeMockWaveEnv, baseEnv.rpc.ListAllAppsCommand, WidgetsPreview,
WidgetsScenario).
---
Nitpick comments:
In `@frontend/app/waveenv/waveenv.ts`:
- Around line 21-29: The hook useWaveEnv currently casts
React.useContext(WaveEnvContext) to T without checking for null, which yields
cryptic failures if used outside a provider; update useWaveEnv to perform a
runtime guard: read ctx = React.useContext(WaveEnvContext) and if ctx is
null/undefined throw a clear Error (e.g. "useWaveEnv must be used within a
WaveEnvContext.Provider") or return a safe default in development, and consider
changing the WaveEnvContext default from null to undefined to match the guard;
reference WaveEnvContext, useWaveEnv, and the WaveEnv type when making the
change.
In `@frontend/app/workspace/widgets.tsx`:
- Around line 139-163: The effect uses env.rpc.ListAllAppsCommand (and
TabRpcClient) but only lists isOpen as a dependency; add env to the dependency
array of the useEffect (or memoize/stabilize env if it should be omitted) so the
hook re-runs when env changes; update the dependency list that wraps the effect
which contains fetchApps, setLoading, setApps and isOpen to include env (or
replace env with a stable ref) to satisfy react-hooks/exhaustive-deps.
In `@frontend/preview/mock/mockwaveenv.ts`:
- Around line 57-68: The module currently creates a singleton mockRpcApi
(RpcApiType) at load time which is shared across all makeMockWaveEnv calls;
change the implementation to instantiate a new RpcApiType inside makeMockWaveEnv
(or otherwise return a fresh RpcApiType per environment) so each mock
environment gets its own rpc instance; move the setMockRpcClient call (and the
mockWshRpcCall/mockWshRpcStream handlers) to be applied to that per-environment
RpcApiType created within makeMockWaveEnv so mutations (e.g., overrides to
ListAllAppsCommand in widgets.preview.tsx) do not leak between environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1c10354d-4551-4e9e-b2da-3bd1727ed0e7
📒 Files selected for processing (12)
cmd/generatets/main-generatets.gofrontend/app/app.tsxfrontend/app/store/wshclientapi.tsfrontend/app/waveenv/mockboundary.tsxfrontend/app/waveenv/waveenv.tsfrontend/app/waveenv/waveenvimpl.tsfrontend/app/workspace/widgets.tsxfrontend/preview/mock/mockwaveenv.tsfrontend/preview/mock/preview-electron-api.tsfrontend/preview/preview.tsxfrontend/preview/previews/widgets.preview.tsxpkg/tsgen/tsgen.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/app/workspace/widgets.tsx (1)
202-212: Consider error handling for block creation.The click handler calls
env.createBlockwithout error handling. If block creation fails, users receive no feedback. Consider wrapping in try-catch or usingfireAndForgetif errors are logged elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/workspace/widgets.tsx` around lines 202 - 212, The onClick handler calls env.createBlock with the constructed blockDef and then onClose but has no error handling; wrap the creation call in a try/catch (or use the project's fireAndForget helper if present) around env.createBlock so failures are caught, log the error (e.g., processLogger or console.error) and surface user feedback (toast/notification) before calling onClose or aborting; update the anonymous onClick function where blockDef is built and env.createBlock is invoked to implement this error handling flow.
🤖 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/workspace/widgets.tsx`:
- Around line 139-163: The effect starting with useEffect that defines fetchApps
should guard against updating state after unmount and include env in its
dependency list: introduce a mounted flag or AbortController inside the effect,
check it before calling setApps and setLoading (i.e., wrap or short-circuit
updates in fetchApps) and return a cleanup that flips the mounted flag or aborts
the request; also add env (and TabRpcClient if it can change) to the dependency
array so the effect re-runs correctly when those change.
---
Nitpick comments:
In `@frontend/app/workspace/widgets.tsx`:
- Around line 202-212: The onClick handler calls env.createBlock with the
constructed blockDef and then onClose but has no error handling; wrap the
creation call in a try/catch (or use the project's fireAndForget helper if
present) around env.createBlock so failures are caught, log the error (e.g.,
processLogger or console.error) and surface user feedback (toast/notification)
before calling onClose or aborting; update the anonymous onClick function where
blockDef is built and env.createBlock is invoked to implement this error
handling flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2e74567b-65ff-4eda-aeea-8214cc542a61
📒 Files selected for processing (1)
frontend/app/workspace/widgets.tsx
| useEffect(() => { | ||
| if (!isOpen) return; | ||
|
|
||
| const fetchApps = async () => { | ||
| setLoading(true); | ||
| try { | ||
| const allApps = await env.rpc.ListAllAppsCommand(TabRpcClient); | ||
| const localApps = allApps | ||
| .filter((app) => !app.appid.startsWith("draft/")) | ||
| .sort((a, b) => { | ||
| const aName = a.appid.replace(/^local\//, ""); | ||
| const bName = b.appid.replace(/^local\//, ""); | ||
| return aName.localeCompare(bName); | ||
| }); | ||
| setApps(localApps); | ||
| } catch (error) { | ||
| console.error("Failed to fetch apps:", error); | ||
| setApps([]); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchApps(); | ||
| }, [isOpen]); |
There was a problem hiding this comment.
Add cleanup to prevent state updates after unmount.
The async fetchApps function lacks cleanup. If the component unmounts or isOpen toggles while the fetch is in-flight, setApps and setLoading will be called on an unmounted component.
Additionally, env is used inside the effect but not included in the dependency array.
🛡️ Proposed fix with abort handling
useEffect(() => {
if (!isOpen) return;
+ let cancelled = false;
const fetchApps = async () => {
setLoading(true);
try {
const allApps = await env.rpc.ListAllAppsCommand(TabRpcClient);
+ if (cancelled) return;
const localApps = allApps
.filter((app) => !app.appid.startsWith("draft/"))
.sort((a, b) => {
const aName = a.appid.replace(/^local\//, "");
const bName = b.appid.replace(/^local\//, "");
return aName.localeCompare(bName);
});
setApps(localApps);
} catch (error) {
+ if (cancelled) return;
console.error("Failed to fetch apps:", error);
setApps([]);
} finally {
+ if (!cancelled) {
setLoading(false);
+ }
}
};
fetchApps();
- }, [isOpen]);
+ return () => {
+ cancelled = true;
+ };
+ }, [isOpen, env]);📝 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.
| useEffect(() => { | |
| if (!isOpen) return; | |
| const fetchApps = async () => { | |
| setLoading(true); | |
| try { | |
| const allApps = await env.rpc.ListAllAppsCommand(TabRpcClient); | |
| const localApps = allApps | |
| .filter((app) => !app.appid.startsWith("draft/")) | |
| .sort((a, b) => { | |
| const aName = a.appid.replace(/^local\//, ""); | |
| const bName = b.appid.replace(/^local\//, ""); | |
| return aName.localeCompare(bName); | |
| }); | |
| setApps(localApps); | |
| } catch (error) { | |
| console.error("Failed to fetch apps:", error); | |
| setApps([]); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| fetchApps(); | |
| }, [isOpen]); | |
| useEffect(() => { | |
| if (!isOpen) return; | |
| let cancelled = false; | |
| const fetchApps = async () => { | |
| setLoading(true); | |
| try { | |
| const allApps = await env.rpc.ListAllAppsCommand(TabRpcClient); | |
| if (cancelled) return; | |
| const localApps = allApps | |
| .filter((app) => !app.appid.startsWith("draft/")) | |
| .sort((a, b) => { | |
| const aName = a.appid.replace(/^local\//, ""); | |
| const bName = b.appid.replace(/^local\//, ""); | |
| return aName.localeCompare(bName); | |
| }); | |
| setApps(localApps); | |
| } catch (error) { | |
| if (cancelled) return; | |
| console.error("Failed to fetch apps:", error); | |
| setApps([]); | |
| } finally { | |
| if (!cancelled) { | |
| setLoading(false); | |
| } | |
| } | |
| }; | |
| fetchApps(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [isOpen, env]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/workspace/widgets.tsx` around lines 139 - 163, The effect
starting with useEffect that defines fetchApps should guard against updating
state after unmount and include env in its dependency list: introduce a mounted
flag or AbortController inside the effect, check it before calling setApps and
setLoading (i.e., wrap or short-circuit updates in fetchApps) and return a
cleanup that flips the mounted flag or aborts the request; also add env (and
TabRpcClient if it can change) to the dependency array so the effect re-runs
correctly when those change.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/preview/previews/widgets.preview.tsx (1)
10-12: Consider making the preview atoms instance-scoped too.
workspaceAtomandfullConfigAtomare still shared across everyWidgetsScenarioandWidgetsResizable. If any scenario writes to them, the other previews will update as well. Creating those atoms insidemakeWidgetsEnv()would keep the examples isolated.Also applies to: 81-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/previews/widgets.preview.tsx` around lines 10 - 12, workspaceAtom and fullConfigAtom are declared at module scope so they are shared across every WidgetsScenario/WidgetsResizable instance; move their creation into makeWidgetsEnv() so each env gets its own atom instances (also relocate any other atoms declared at top-level around lines 81-95, e.g., resizableHeightAtom if present) and return or attach those atoms from makeWidgetsEnv for consumers to use; delete the top-level atom declarations (workspaceAtom, fullConfigAtom, etc.) and update WidgetsScenario/WidgetsResizable to consume the per-instance atoms provided by makeWidgetsEnv.frontend/preview/mock/mockwaveenv.ts (1)
70-87: Fail fast on unmocked RPC commands.Logging and returning
nullhere turns a missing mock into a downstreamnullfailure somewhere else in the UI. Throwing/rejecting with the command name makes preview/test failures much easier to diagnose.💡 Proposed change
mockWshRpcCall(_client, command, data, _opts) { const fn = dispatchMap.get(command); if (fn) { return fn(_client, data, _opts); } - console.log("[mock rpc call]", command, data); - return Promise.resolve(null); + return Promise.reject(new Error(`[mock rpc call] unhandled command: ${command}`)); }, async *mockWshRpcStream(_client, command, data, _opts) { const fn = dispatchMap.get(command); if (fn) { yield* fn(_client, data, _opts); return; } - console.log("[mock rpc stream]", command, data); - yield null; + throw new Error(`[mock rpc stream] unhandled command: ${command}`); },🤖 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 70 - 87, The mock RPC handlers currently log and return null when a command is unmocked; change this to fail fast by rejecting/throwing an Error that includes the command name. In rpc.setMockRpcClient's mockWshRpcCall, replace the console.log + Promise.resolve(null) path with returning Promise.reject(new Error(`Unmocked RPC command: ${command}`)) (or throw) so callers see an immediate rejection; in mockWshRpcStream, replace the console.log + yield null path with throwing an Error(`Unmocked RPC stream command: ${command}`) before yielding so the async generator rejects instead of producing a null value. Reference: rpc.setMockRpcClient, mockWshRpcCall, mockWshRpcStream, dispatchMap.
🤖 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/preview/mock/mockwaveenv.ts`:
- Around line 10-25: makeMockConfigAtoms currently returns real global atoms via
getSettingsKeyAtom for non-overridden keys, causing divergence from the local
mock settingsAtom/fullConfigAtom; update makeMockConfigAtoms to derive
non-overridden atoms from the same mock state used by makeMockGlobalAtoms (the
local settingsAtom/fullConfigAtom) so env.configAtoms and env.atoms.settingsAtom
share the same backing state. Concretely, have makeMockGlobalAtoms create the
mock settingsAtom/fullConfigAtom and pass them (or expose them) into
makeMockConfigAtoms, and change makeMockConfigAtoms so its Proxy.get returns
atoms that read/write against that shared fullConfigAtom (rather than calling
getSettingsKeyAtom). Ensure the unique symbols referenced are
makeMockConfigAtoms, makeMockGlobalAtoms, settingsAtom, fullConfigAtom,
getSettingsKeyAtom, and configAtoms.
---
Nitpick comments:
In `@frontend/preview/mock/mockwaveenv.ts`:
- Around line 70-87: The mock RPC handlers currently log and return null when a
command is unmocked; change this to fail fast by rejecting/throwing an Error
that includes the command name. In rpc.setMockRpcClient's mockWshRpcCall,
replace the console.log + Promise.resolve(null) path with returning
Promise.reject(new Error(`Unmocked RPC command: ${command}`)) (or throw) so
callers see an immediate rejection; in mockWshRpcStream, replace the console.log
+ yield null path with throwing an Error(`Unmocked RPC stream command:
${command}`) before yielding so the async generator rejects instead of producing
a null value. Reference: rpc.setMockRpcClient, mockWshRpcCall, mockWshRpcStream,
dispatchMap.
In `@frontend/preview/previews/widgets.preview.tsx`:
- Around line 10-12: workspaceAtom and fullConfigAtom are declared at module
scope so they are shared across every WidgetsScenario/WidgetsResizable instance;
move their creation into makeWidgetsEnv() so each env gets its own atom
instances (also relocate any other atoms declared at top-level around lines
81-95, e.g., resizableHeightAtom if present) and return or attach those atoms
from makeWidgetsEnv for consumers to use; delete the top-level atom declarations
(workspaceAtom, fullConfigAtom, etc.) and update
WidgetsScenario/WidgetsResizable to consume the per-instance atoms provided by
makeWidgetsEnv.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: daff4f52-39d4-43bd-88d6-6eb336b61dd7
📒 Files selected for processing (3)
frontend/preview/mock/mockwaveenv.tsfrontend/preview/preview.tsxfrontend/preview/previews/widgets.preview.tsx
| function makeMockConfigAtoms(overrides?: Partial<SettingsType>): WaveEnv["configAtoms"] { | ||
| const overrideAtoms = new Map<keyof SettingsType, ReturnType<typeof atom>>(); | ||
| if (overrides) { | ||
| for (const key of Object.keys(overrides) as (keyof SettingsType)[]) { | ||
| overrideAtoms.set(key, atom(overrides[key])); | ||
| } | ||
| } | ||
| return new Proxy({} as WaveEnv["configAtoms"], { | ||
| get<K extends keyof SettingsType>(_target: WaveEnv["configAtoms"], key: K) { | ||
| if (overrideAtoms.has(key)) { | ||
| return overrideAtoms.get(key); | ||
| } | ||
| return getSettingsKeyAtom(key); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Back configAtoms and settingsAtom with the same mock state.
Non-overridden config keys currently come from getSettingsKeyAtom(...), while makeMockGlobalAtoms() creates a separate local settingsAtom/fullConfigAtom. That means env.configAtoms.foo and env.atoms.settingsAtom can diverge inside the same mock env, and separate preview envs can still leak settings through the global store.
Also applies to: 33-54
🤖 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 10 - 25,
makeMockConfigAtoms currently returns real global atoms via getSettingsKeyAtom
for non-overridden keys, causing divergence from the local mock
settingsAtom/fullConfigAtom; update makeMockConfigAtoms to derive non-overridden
atoms from the same mock state used by makeMockGlobalAtoms (the local
settingsAtom/fullConfigAtom) so env.configAtoms and env.atoms.settingsAtom share
the same backing state. Concretely, have makeMockGlobalAtoms create the mock
settingsAtom/fullConfigAtom and pass them (or expose them) into
makeMockConfigAtoms, and change makeMockConfigAtoms so its Proxy.get returns
atoms that read/write against that shared fullConfigAtom (rather than calling
getSettingsKeyAtom). Ensure the unique symbols referenced are
makeMockConfigAtoms, makeMockGlobalAtoms, settingsAtom, fullConfigAtom,
getSettingsKeyAtom, and configAtoms.
No description provided.