Expanding WaveEnv to cover all of frontend/app/block components#3024
Expanding WaveEnv to cover all of frontend/app/block components#3024
Conversation
sawka
commented
Mar 10, 2026
sawka
commented
- lots of updates to WaveEnv to make it cover more functionality
- Create BlockEnv as a narrowing of WaveEnv that covers all of frontend/app/block functionality
- Fixed a lot of dependencies in the block components that caused unnecessarily re-renders
- Added atom caching to WOS
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR shifts block data access from global/WOS atoms into a WaveEnv/BlockEnv abstraction and updates mocks and types. Block rendering is refactored so view models are created from a viewType (makeDefaultViewModel(viewType)) and Block/SubBlock now route through internal BlockInner/SubBlockInner with viewType propagation. WaveEnv gains a wos namespace (getWaveObjectAtom/getWaveObjectLoadingAtom/isWaveObjectNullAtom/useWaveObjectValue), getSettingsKeyAtom, getConnConfigKeyAtom, getLocalHostDisplayNameAtom, and mockTabModel; wos caching and isWaveObjectNullAtom were added. Many UI components and store utilities were updated to use useWaveEnv, plus a new SKILL.md doc. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge OverviewThis PR introduces WaveEnv narrowing for better type safety and testability. It refactors multiple components to use the
Changes ReviewedThe refactoring is well-structured:
The existing comments on this PR already cover the areas of concern. The current code in Files Reviewed (13 files)
|
Deploying waveterm with
|
| Latest commit: |
fc0c580
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://353844f0.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-mock-for-block.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: 6
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)
306-323:⚠️ Potential issue | 🔴 CriticalRegistry lookup must match remount strategy to prevent disposing active viewModels.
When
viewTypechanges, React remountsBlockInnerandSubBlockInnerby key (blockId:viewType), but the components still read from a registry keyed onblockIdalone. The new instance can grab the stale model from the registry before the old instance's cleanup runs, then that cleanup disposes the model while the new tree uses it.Prevent reuse by gating on
viewModel.viewType === props.viewType, and guard cleanup to unregister only if the registry entry still points to the instance being torn down. UpdateuseEffectdependencies to[props.nodeModel.blockId, viewModel]to properly track which instance owns the cleanup.🔁 Minimal guard for the remount path
- const bcm = getBlockComponentModel(props.nodeModel.blockId); - let viewModel = bcm?.viewModel; - if (viewModel == null) { + const bcm = getBlockComponentModel(props.nodeModel.blockId); + let viewModel = bcm?.viewModel; + if (viewModel == null || viewModel.viewType !== props.viewType) { // viewModel gets the full waveEnv viewModel = makeViewModel(props.nodeModel.blockId, props.viewType, props.nodeModel, tabModel, waveEnv); registerBlockComponentModel(props.nodeModel.blockId, { viewModel }); } useEffect(() => { return () => { - unregisterBlockComponentModel(props.nodeModel.blockId); + if (getBlockComponentModel(props.nodeModel.blockId)?.viewModel === viewModel) { + unregisterBlockComponentModel(props.nodeModel.blockId); + } viewModel?.dispose?.(); }; - }, []); + }, [props.nodeModel.blockId, viewModel]);Apply the same guard to
SubBlockInner.Apply to both
BlockInner(lines 306–323) andSubBlockInner(lines 363–370).🤖 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 306 - 323, The registry lookup and cleanup in BlockInner and SubBlockInner can cause the new remounted instance to reuse a viewModel that the old instance will dispose; fix by ensuring registry entries are keyed/validated by viewType: when reading viewModel from getBlockComponentModel(props.nodeModel.blockId) only accept it if viewModel?.viewType === props.viewType, otherwise create a new viewModel via makeViewModel(...) and register it with registerBlockComponentModel(...); in the cleanup useEffect, only call unregisterBlockComponentModel(...) and viewModel.dispose() if the current registry entry still points to this instance’s viewModel (i.e., the registered model === viewModel) and change the effect deps to [props.nodeModel.blockId, viewModel] so the cleanup is associated with the correct instance; apply identical guards to SubBlockInner.
🧹 Nitpick comments (1)
frontend/app/block/connectionbutton.tsx (1)
24-24: UseuseSetAtominstead of subscribing to the modal state.Line 24 still subscribes this component to
changeConnModalAtomeven though the value is discarded. That reintroduces a rerender path the PR is trying to remove.const setConnModalOpen = jotai.useSetAtom(changeConnModalAtom)avoids the extra subscription.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/block/connectionbutton.tsx` at line 24, The component is unnecessarily subscribing to changeConnModalAtom by calling jotai.useAtom and discarding the value via _connModalOpen; replace that subscription with a setter-only hook by using jotai.useSetAtom(changeConnModalAtom) so the component no longer re-renders on atom value changes—update the declaration that currently reads const [_connModalOpen, setConnModalOpen] = jotai.useAtom(changeConnModalAtom) to use useSetAtom and keep the existing setConnModalOpen reference.
🤖 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/block/blockenv.ts`:
- Around line 17-19: The BlockEnv type currently defines api.openExternal but
the actual env implementations (makeWaveEnvImpl, makeMockWaveEnv) expose
electron.openExternal, so useWaveEnv<BlockEnv>() will yield undefined for
waveEnv.api; fix by aligning the shape: either rename BlockEnv.api to electron
(update the BlockEnv interface and all consumers to use electron.openExternal)
or add a real api property to makeWaveEnvImpl and makeMockWaveEnv that forwards
to electron.openExternal (e.g., set env.api.openExternal =
env.electron.openExternal) so WaveEnvContext, useWaveEnv, BlockEnv,
makeWaveEnvImpl, and makeMockWaveEnv all share the same property names.
In `@frontend/app/block/durable-session-flyover.tsx`:
- Around line 29-34: The runtime error is caused by calling
waveEnv.api.openExternal where waveEnv.api is undefined; change the call to use
the actual runtime shape—invoke waveEnv.electron.openExternal(...) instead of
waveEnv.api.openExternal (locate the use in durable-session-flyover.tsx where
useWaveEnv<BlockEnv>() returns waveEnv and reference BlockEnv and
makeWaveEnvImpl to confirm the runtime fields), or alternatively add an api
wrapper onto the WaveEnv implementation in makeWaveEnvImpl so waveEnv.api is
defined; prefer the simpler fix of replacing waveEnv.api.openExternal with
waveEnv.electron.openExternal to match the real object shape.
In `@frontend/app/store/tab-model.ts`:
- Around line 49-53: Cache key for TabModel must include the WaveEnv so an
env-less instance doesn't win; update getTabModelByTabId to derive a composite
key from tabId plus a canonical identifier from waveEnv (e.g., env.id or a
stable string) instead of using tabId alone, and use that composite key when
calling tabModelCache.get(...) and tabModelCache.set(...). Ensure TabModel
construction still receives the passed waveEnv and update any helper like
getActiveTabModel to use the same key derivation so env-specific models are
returned consistently.
- Around line 80-87: Rename the hook function maybeUseTabModel to
useMaybeTabModel to follow React hook naming, change its return type from
TabModel to TabModel | undefined, and update its implementation signature
accordingly (it already calls useWaveEnv() and useContext(TabModelContext));
then update all call sites (notably the caller in aipanel.tsx) to handle the
possible undefined return (add a null/undefined check or early return) so
consumers no longer assume a guaranteed TabModel.
In `@frontend/app/store/wos.ts`:
- Around line 221-258: The derived-atom cache entries (created by
getWaveObjectAtom, getWaveObjectLoadingAtom, and isWaveObjectNullAtom) close
over the original wov and must be evicted when the backing WaveObjectValue is
dropped; update the cache-clearing logic
(cleanWaveObjectCache/clearWaveObjectCache) to also remove
waveObjectDerivedAtomCache entries for the same oref by deleting keys with the
suffixes ":value", ":loading", and ":isnull" so that subsequent getWaveObject*
calls recreate atoms bound to the new wov.dataAtom instead of returning stale,
disconnected atoms.
In `@frontend/preview/mock/mockwaveenv.ts`:
- Around line 175-183: getLocalHostDisplayNameAtom currently constructs a new
atom on every call, breaking atom identity; change it to return a cached
singleton like connStatusAtomCache: create a module-/mock-init-scoped variable
(e.g., localHostDisplayNameAtomCache) and initialize it once with the same
atom<string>((get) => { const configValue =
get(atoms.settingsAtom)?.["conn:localhostdisplayname"]; return configValue ??
"user@localhost"; }) then have getLocalHostDisplayNameAtom return that cached
instance instead of creating a new atom on each invocation so consumers (e.g.,
connectionbutton.tsx) subscribe to the same atom.
---
Outside diff comments:
In `@frontend/app/block/block.tsx`:
- Around line 306-323: The registry lookup and cleanup in BlockInner and
SubBlockInner can cause the new remounted instance to reuse a viewModel that the
old instance will dispose; fix by ensuring registry entries are keyed/validated
by viewType: when reading viewModel from
getBlockComponentModel(props.nodeModel.blockId) only accept it if
viewModel?.viewType === props.viewType, otherwise create a new viewModel via
makeViewModel(...) and register it with registerBlockComponentModel(...); in the
cleanup useEffect, only call unregisterBlockComponentModel(...) and
viewModel.dispose() if the current registry entry still points to this
instance’s viewModel (i.e., the registered model === viewModel) and change the
effect deps to [props.nodeModel.blockId, viewModel] so the cleanup is associated
with the correct instance; apply identical guards to SubBlockInner.
---
Nitpick comments:
In `@frontend/app/block/connectionbutton.tsx`:
- Line 24: The component is unnecessarily subscribing to changeConnModalAtom by
calling jotai.useAtom and discarding the value via _connModalOpen; replace that
subscription with a setter-only hook by using
jotai.useSetAtom(changeConnModalAtom) so the component no longer re-renders on
atom value changes—update the declaration that currently reads const
[_connModalOpen, setConnModalOpen] = jotai.useAtom(changeConnModalAtom) to use
useSetAtom and keep the existing setConnModalOpen reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 38ce212c-2005-42cb-b115-6f6083b0fa0d
📒 Files selected for processing (14)
frontend/app/block/block.tsxfrontend/app/block/blockenv.tsfrontend/app/block/blockframe-header.tsxfrontend/app/block/blockframe.tsxfrontend/app/block/blockutil.tsxfrontend/app/block/connectionbutton.tsxfrontend/app/block/connstatusoverlay.tsxfrontend/app/block/durable-session-flyover.tsxfrontend/app/store/global.tsfrontend/app/store/tab-model.tsfrontend/app/store/wos.tsfrontend/app/waveenv/waveenv.tsfrontend/app/waveenv/waveenvimpl.tsfrontend/preview/mock/mockwaveenv.ts
| export function getTabModelByTabId(tabId: string, waveEnv?: WaveEnv): TabModel { | ||
| let model = tabModelCache.get(tabId); | ||
| if (model == null) { | ||
| model = new TabModel(tabId); | ||
| model = new TabModel(tabId, waveEnv); | ||
| tabModelCache.set(tabId, model); |
There was a problem hiding this comment.
Include waveEnv in the TabModel cache key.
tabModelCache is still keyed only by tabId, so the first model created for a tab wins. frontend/app/app.tsx:42-57 currently seeds the provider with getTabModelByTabId(tabId) without an env, which means later getTabModelByTabId(tabId, waveEnv) / getActiveTabModel(waveEnv) calls can still get the env-less instance and bypass the new mock/WaveEnv-backed path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/store/tab-model.ts` around lines 49 - 53, Cache key for TabModel
must include the WaveEnv so an env-less instance doesn't win; update
getTabModelByTabId to derive a composite key from tabId plus a canonical
identifier from waveEnv (e.g., env.id or a stable string) instead of using tabId
alone, and use that composite key when calling tabModelCache.get(...) and
tabModelCache.set(...). Ensure TabModel construction still receives the passed
waveEnv and update any helper like getActiveTabModel to use the same key
derivation so env-specific models are returned consistently.
frontend/app/store/tab-model.ts
Outdated
| export function maybeUseTabModel(): TabModel { | ||
| return useContext(TabModelContext); | ||
| const waveEnv = useWaveEnv(); | ||
| const ctxModel = useContext(TabModelContext); | ||
| if (waveEnv?.mockTabModel != null) { | ||
| return waveEnv.mockTabModel; | ||
| } | ||
| return ctxModel; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/app/store/tab-model.ts | head -100Repository: wavetermdev/waveterm
Length of output: 3498
🏁 Script executed:
rg "maybeUseTabModel" --type ts --type tsx -B 2 -A 2Repository: wavetermdev/waveterm
Length of output: 91
🏁 Script executed:
fd tsconfig.json -o exec cat {} \;Repository: wavetermdev/waveterm
Length of output: 191
🏁 Script executed:
rg "maybeUseTabModel" -A 2 -B 2Repository: wavetermdev/waveterm
Length of output: 1377
🏁 Script executed:
cat tsconfig.jsonRepository: wavetermdev/waveterm
Length of output: 1138
🏁 Script executed:
cat -n frontend/app/aipanel/aipanel.tsx | grep -A 5 "maybeUseTabModel()"Repository: wavetermdev/waveterm
Length of output: 484
Rename maybeUseTabModel to useMaybeTabModel and update return type to TabModel | undefined.
The function calls hooks (useWaveEnv() and useContext()) on lines 81–82 but doesn't follow the React hooks naming convention. Line 86 returns ctxModel, which has type TabModel | undefined per the TabModelContext definition (line 66), creating a type mismatch with the declared TabModel return type. The caller in frontend/app/aipanel/aipanel.tsx (line 260) doesn't check for undefined.
🧰 Tools
🪛 Biome (2.4.6)
[error] 81-81: This hook is being called from within a function or method that is not a hook or component.
(lint/correctness/useHookAtTopLevel)
[error] 82-82: This hook is being called from within a function or method that is not a hook or component.
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/store/tab-model.ts` around lines 80 - 87, Rename the hook
function maybeUseTabModel to useMaybeTabModel to follow React hook naming,
change its return type from TabModel to TabModel | undefined, and update its
implementation signature accordingly (it already calls useWaveEnv() and
useContext(TabModelContext)); then update all call sites (notably the caller in
aipanel.tsx) to handle the possible undefined return (add a null/undefined check
or early return) so consumers no longer assume a guaranteed TabModel.
| const waveObjectDerivedAtomCache = new Map<string, Atom<any>>(); | ||
|
|
||
| function getWaveObjectAtom<T extends WaveObj>(oref: string): Atom<T> { | ||
| const cacheKey = oref + ":value"; | ||
| let cachedAtom = waveObjectDerivedAtomCache.get(cacheKey) as Atom<T>; | ||
| if (cachedAtom != null) { | ||
| return cachedAtom; | ||
| } | ||
| const wov = getWaveObjectValue<T>(oref); | ||
| return atom((get) => get(wov.dataAtom).value); | ||
| cachedAtom = atom((get) => get(wov.dataAtom).value); | ||
| waveObjectDerivedAtomCache.set(cacheKey, cachedAtom); | ||
| return cachedAtom; | ||
| } | ||
|
|
||
| function getWaveObjectLoadingAtom(oref: string): Atom<boolean> { | ||
| const cacheKey = oref + ":loading"; | ||
| let cachedAtom = waveObjectDerivedAtomCache.get(cacheKey) as Atom<boolean>; | ||
| if (cachedAtom != null) { | ||
| return cachedAtom; | ||
| } | ||
| const wov = getWaveObjectValue(oref); | ||
| return atom((get) => { | ||
| cachedAtom = atom((get) => { | ||
| const dataValue = get(wov.dataAtom); | ||
| if (dataValue.loading) { | ||
| return null; | ||
| } | ||
| return dataValue.loading; | ||
| }); | ||
| waveObjectDerivedAtomCache.set(cacheKey, cachedAtom); | ||
| return cachedAtom; | ||
| } | ||
|
|
||
| function isWaveObjectNullAtom(oref: string): Atom<boolean> { | ||
| const cacheKey = oref + ":isnull"; | ||
| let cachedAtom = waveObjectDerivedAtomCache.get(cacheKey) as Atom<boolean>; | ||
| if (cachedAtom != null) { | ||
| return cachedAtom; | ||
| } | ||
| cachedAtom = atom((get) => get(getWaveObjectAtom(oref)) == null); | ||
| waveObjectDerivedAtomCache.set(cacheKey, cachedAtom); | ||
| return cachedAtom; |
There was a problem hiding this comment.
Evict derived atoms when the backing wov is dropped.
These cached atoms close over the wov.dataAtom captured at creation time. cleanWaveObjectCache() / clearWaveObjectCache() only remove waveObjectValueCache entries, so a later read can return a stale derived atom that's disconnected from the newly created wov and never sees fresh loads or updates. Clear the :value / :loading / :isnull entries alongside the corresponding oref.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/store/wos.ts` around lines 221 - 258, The derived-atom cache
entries (created by getWaveObjectAtom, getWaveObjectLoadingAtom, and
isWaveObjectNullAtom) close over the original wov and must be evicted when the
backing WaveObjectValue is dropped; update the cache-clearing logic
(cleanWaveObjectCache/clearWaveObjectCache) to also remove
waveObjectDerivedAtomCache entries for the same oref by deleting keys with the
suffixes ":value", ":loading", and ":isnull" so that subsequent getWaveObject*
calls recreate atoms bound to the new wov.dataAtom instead of returning stale,
disconnected atoms.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/block/durable-session-flyover.tsx (1)
199-199: Consider removing the redundant constant.
showRestartButtonis alwaystrue, making the conditional on line 250{showRestartButton && ...}redundant. If this is placeholder logic for future conditional behavior, consider adding a comment explaining the intent. Otherwise, the constant and conditional can be removed for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/block/durable-session-flyover.tsx` at line 199, The constant showRestartButton is always true, making the conditional render around it redundant; remove the declaration of showRestartButton and the surrounding conditional (the {showRestartButton && ...} JSX) and render the restart button directly, or if this was intended as future conditional behavior, replace the const showRestartButton = true with a short comment (e.g. // toggle to control restart button visibility) and keep the {showRestartButton && ...} check so the intent is documented; refer to the symbol showRestartButton and the JSX conditional where the restart button is rendered to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/block/durable-session-flyover.tsx`:
- Line 199: The constant showRestartButton is always true, making the
conditional render around it redundant; remove the declaration of
showRestartButton and the surrounding conditional (the {showRestartButton &&
...} JSX) and render the restart button directly, or if this was intended as
future conditional behavior, replace the const showRestartButton = true with a
short comment (e.g. // toggle to control restart button visibility) and keep the
{showRestartButton && ...} check so the intent is documented; refer to the
symbol showRestartButton and the JSX conditional where the restart button is
rendered to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 655d30e9-9e21-4b0a-ad8d-b3320575298d
📒 Files selected for processing (2)
frontend/app/block/blockenv.tsfrontend/app/block/durable-session-flyover.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/block/blockenv.ts
…for creating waveenv narrowing
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.kilocode/skills/waveenv/SKILL.md (1)
121-122: Consider using anchors or removing line numbers from documentation links.Line number references (
:12,:23) will become stale as files evolve. Consider linking to the file without line numbers, or use a named export/heading anchor if supported. Alternatively, use a comment marker in the source files that documentation can reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.kilocode/skills/waveenv/SKILL.md around lines 121 - 122, The documentation links in SKILL.md point to specific line numbers for BlockEnv and WidgetsEnv which will become stale; update the links to target the file or a stable anchor instead by removing the ":12" and ":23" line suffixes from the BlockEnv and WidgetsEnv links, or add named export/heading anchors (or a comment marker) in the source near the BlockEnv and WidgetsEnv declarations and update the SKILL.md links to use those anchors so references stay stable as the files change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.kilocode/skills/waveenv/SKILL.md:
- Around line 121-122: The documentation links in SKILL.md point to specific
line numbers for BlockEnv and WidgetsEnv which will become stale; update the
links to target the file or a stable anchor instead by removing the ":12" and
":23" line suffixes from the BlockEnv and WidgetsEnv links, or add named
export/heading anchors (or a comment marker) in the source near the BlockEnv and
WidgetsEnv declarations and update the SKILL.md links to use those anchors so
references stay stable as the files change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a123c0af-94f6-414c-b8c0-94a39c25dcba
📒 Files selected for processing (4)
.kilocode/skills/waveenv/SKILL.mdfrontend/app/block/blockenv.tsfrontend/app/waveenv/waveenv.tsfrontend/app/workspace/widgets.tsx
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const waveEnv = useWaveEnv<BlockEnv>(); | ||
| const handleDisconnect = React.useCallback(() => { | ||
| const prtn = RpcApi.ConnDisconnectCommand(TabRpcClient, connName, { timeout: 5000 }); | ||
| const prtn = waveEnv.rpc.ConnDisconnectCommand(TabRpcClient, connName, { timeout: 5000 }); |
There was a problem hiding this comment.
CRITICAL: Missing waveEnv in useCallback dependency array. The callback uses waveEnv but only includes connName. This can cause stale closures.
| @@ -138,13 +140,13 @@ export const ConnStatusOverlay = React.memo( | |||
| }, [width, connStatus, setShowError]); | |||
|
|
|||
There was a problem hiding this comment.
CRITICAL: Missing waveEnv in useCallback dependency array. The callback uses waveEnv but only includes connName and nodeModel.blockId. This can cause stale closures.
| @@ -155,19 +157,19 @@ export const ConnStatusOverlay = React.memo( | |||
| metamaptype: metamaptype, | |||
| }; | |||
There was a problem hiding this comment.
CRITICAL: Missing waveEnv in useCallback dependency array. The callback uses waveEnv but only includes connName. This can cause stale closures.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.kilocode/skills/waveenv/SKILL.md (1)
16-18: Optional: Consider rephrasing to reduce repetition.The phrase "you want to" appears twice in the same sentence. For stylistic variety, consider:
"Create a narrowing when writing a component (or group of components) for testing in the preview server, or to make the environmental dependencies of a component tree explicit."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.kilocode/skills/waveenv/SKILL.md around lines 16 - 18, The sentence under the "When To Create One" heading repeats the phrase "you want to"; edit the sentence to improve style and remove duplication (for example: "Create a narrowing when writing a component (or group of components) for testing in the preview server, or to make the environmental dependencies of a component tree explicit."). Update the content in SKILL.md under the "When To Create One" section to use the revised wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.kilocode/skills/waveenv/SKILL.md:
- Around line 16-18: The sentence under the "When To Create One" heading repeats
the phrase "you want to"; edit the sentence to improve style and remove
duplication (for example: "Create a narrowing when writing a component (or group
of components) for testing in the preview server, or to make the environmental
dependencies of a component tree explicit."). Update the content in SKILL.md
under the "When To Create One" section to use the revised wording.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d0ea686-e420-4b46-ae00-3367c58d29df
📒 Files selected for processing (1)
.kilocode/skills/waveenv/SKILL.md
…y unnecessary and adds overhead/complexity
1 participant
