Skip to content

New WaveEnv, for preview server + mocking#3015

Merged
sawka merged 6 commits intomainfrom
sawka/waveenv
Mar 9, 2026
Merged

New WaveEnv, for preview server + mocking#3015
sawka merged 6 commits intomainfrom
sawka/waveenv

Conversation

@sawka
Copy link
Member

@sawka sawka commented Mar 8, 2026

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 8, 2026

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 00d1b9a0-0154-4ac8-91ce-b80d41d38579

📥 Commits

Reviewing files that changed from the base of the PR and between 4def99b and 25b2187.

📒 Files selected for processing (3)
  • frontend/app/app.tsx
  • frontend/app/store/wshclientapi.ts
  • pkg/tsgen/tsgen.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/tsgen/tsgen.go

Walkthrough

Replaces module-scoped mock RPC wiring with an exported RpcApiType that holds an instance-scoped mockClient and setMockRpcClient. Updates generated client code to use this.mockClient. Adds a typed WaveEnv (type, React context, and hook), a production factory (makeWaveEnvImpl), and a preview/mock implementation (makeMockWaveEnv, mock RPC/electron). App and preview roots now provide WaveEnv via context. Introduces MockBoundary component. Refactors many UI components (notably Widgets and floating windows) to consume environment APIs (rpc, createBlock, showContextMenu) instead of global imports. Exposes a preview electron API symbol.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess relevance to the changeset. Add a description explaining the purpose of WaveEnv, its role in the preview server, and how the mocking infrastructure supports the feature.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: introducing a new WaveEnv abstraction with integrated preview server and mocking support across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sawka/waveenv

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

<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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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"

@kilo-code-bot
Copy link
Contributor

kilo-code-bot bot commented Mar 8, 2026

Code Review Summary

Status: No New Issues Found | Recommendation: Merge

Overview

This PR is a refactoring that converts RpcApiType from module-level to class-based, adds WaveEnv context to the app, and refactors widgets to use the env pattern. The changes appear clean and don't introduce new bugs.

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 0

Notes

  • The shink-0 typo mentioned in existing comments has been fixed to shrink-0 in this PR
  • The WaveEnv integration is implemented consistently across widgets
  • The class-based RpcApiType maintains backward compatibility via the exported singleton RpcApi
Files Reviewed (9 files)
  • cmd/generatets/main-generatets.go
  • frontend/app/app.tsx
  • frontend/app/store/wshclientapi.ts
  • frontend/app/workspace/widgets.tsx
  • frontend/preview/mock/mockwaveenv.ts
  • frontend/preview/mock/preview-electron-api.ts
  • frontend/preview/preview.tsx
  • frontend/preview/previews/widgets.preview.tsx
  • pkg/tsgen/tsgen.go

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 8, 2026

Deploying waveterm with  Cloudflare Pages  Cloudflare Pages

Latest commit: 25b2187
Status: ✅  Deploy successful!
Preview URL: https://8fbc9f57.waveterm.pages.dev
Branch Preview URL: https://sawka-waveenv.waveterm.pages.dev

View logs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
frontend/app/workspace/widgets.tsx (1)

139-163: Missing env in dependency array.

The useEffect uses env.rpc.ListAllAppsCommand but only lists [isOpen] as a dependency. While env from useWaveEnv is likely stable, ESLint's react-hooks/exhaustive-deps rule would flag this. Consider adding env to 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 mockRpcApi is created once at module load and reused by all calls to makeMockWaveEnv. This works for basic scenarios but could cause issues if consumers mutate the RPC instance (e.g., overriding ListAllAppsCommand in widgets.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.

WaveEnvContext is created with null as the default value, and useWaveEnv casts the result directly without a null check. If a component accidentally calls useWaveEnv outside a WaveEnvContext.Provider, it will receive null and 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

📥 Commits

Reviewing files that changed from the base of the PR and between 71f7e98 and 0067e8f.

📒 Files selected for processing (12)
  • cmd/generatets/main-generatets.go
  • frontend/app/app.tsx
  • frontend/app/store/wshclientapi.ts
  • frontend/app/waveenv/mockboundary.tsx
  • frontend/app/waveenv/waveenv.ts
  • frontend/app/waveenv/waveenvimpl.ts
  • frontend/app/workspace/widgets.tsx
  • frontend/preview/mock/mockwaveenv.ts
  • frontend/preview/mock/preview-electron-api.ts
  • frontend/preview/preview.tsx
  • frontend/preview/previews/widgets.preview.tsx
  • pkg/tsgen/tsgen.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.createBlock without error handling. If block creation fails, users receive no feedback. Consider wrapping in try-catch or using fireAndForget if 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0067e8f and 9642198.

📒 Files selected for processing (1)
  • frontend/app/workspace/widgets.tsx

Comment on lines +139 to +163
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]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
frontend/preview/previews/widgets.preview.tsx (1)

10-12: Consider making the preview atoms instance-scoped too.

workspaceAtom and fullConfigAtom are still shared across every WidgetsScenario and WidgetsResizable. If any scenario writes to them, the other previews will update as well. Creating those atoms inside makeWidgetsEnv() 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 null here turns a missing mock into a downstream null failure 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9642198 and 4def99b.

📒 Files selected for processing (3)
  • frontend/preview/mock/mockwaveenv.ts
  • frontend/preview/preview.tsx
  • frontend/preview/previews/widgets.preview.tsx

Comment on lines +10 to +25
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);
},
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

@sawka sawka merged commit e6e559c into main Mar 9, 2026
7 of 8 checks passed
@sawka sawka deleted the sawka/waveenv branch March 9, 2026 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant