TabBar full preview + much more FE mocking via WaveEnv to enable it#3028
TabBar full preview + much more FE mocking via WaveEnv to enable it#3028
Conversation
Co-authored-by: sawka <2722291+sawka@users.noreply.github.com>
Deploying waveterm with
|
| Latest commit: |
3675696
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://54d06025.waveterm.pages.dev |
| Branch Preview URL: | https://copilot-create-waveenv-for-t.waveterm.pages.dev |
Co-authored-by: sawka <2722291+sawka@users.noreply.github.com>
…ace deps in tab.tsx
…n calls to the backend in wps when in preview mode...
sawka
changed the title
Code Review SummaryStatus: 6 Known Issues | Recommendation: Address before merge Overview
Known Issues (Previously Identified)All issues below were previously identified in existing inline comments:
Changes ReviewedThis PR introduces:
The core architectural changes appear sound. No new issues were found beyond those already documented above. Files Reviewed (23 files)
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR moves many frontend modules to an environment-aware WaveEnv model: service classes are instance-based with a callBackendService wrapper and exported AllServiceTypes/AllServiceImpls; numerous UI components and stores (tab bar, tab model, badges, workspace switcher, previews) now accept and use WaveEnv subsets; mock preview envs and RPC mocks are expanded; two RPC commands (UpdateTabNameCommand, UpdateWorkspaceTabIdsCommand) are added across client/server layers; ObjectService.UpdateTabName and WorkspaceService.UpdateTabIds methods were removed; tsgen now emits service classes wired to WaveEnv; type surfaces (waveenv, atoms, mocks) and previews updated accordingly. 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 |
![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 (4)
frontend/app/tab/updatebanner.tsx (1)
15-49:⚠️ Potential issue | 🟡 Minor
dismissBanneris never set totrue, making the timeout logic dead code.The
dismissBannervariable is initialized tofalse(line 17) and never assignedtruein any switch case. This means the timeout logic at lines 39-45 will never execute, and lines 33-48 are effectively dead code.If auto-dismissal is no longer needed, consider removing the unused code:
🧹 Proposed cleanup
useEffect(() => { let message: string; - let dismissBanner = false; switch (appUpdateStatus) { case "ready": message = "Update"; break; case "downloading": message = "Downloading"; break; case "installing": message = "Installing"; break; default: break; } setUpdateStatusMessage(message); - - // Clear any existing timeout - if (dismissBannerTimeout) { - clearTimeout(dismissBannerTimeout); - } - - // If we want to dismiss the banner, set the new timeout, otherwise clear the state - if (dismissBanner) { - setDismissBannerTimeout( - setTimeout(() => { - setUpdateStatusMessage(null); - setDismissBannerTimeout(null); - }, 10000) - ); - } else { - setDismissBannerTimeout(null); - } }, [appUpdateStatus]);If auto-dismissal is still planned, consider removing the variable and directly checking the condition that should trigger dismissal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tab/updatebanner.tsx` around lines 15 - 49, The local flag dismissBanner is never set true so the timeout branch is dead; either remove the auto-dismiss logic (clear references to dismissBanner, dismissBannerTimeout, setDismissBannerTimeout and the timeout handling inside the useEffect) or set dismissBanner based on appUpdateStatus (e.g., in the useEffect switch set dismissBanner = true for the status that should auto-dismiss such as "ready" or whichever statuses you want) and keep the existing timeout/clear logic using dismissBannerTimeout, setDismissBannerTimeout and setUpdateStatusMessage.frontend/app/tab/tabbar.tsx (1)
477-489:⚠️ Potential issue | 🔴 CriticalTab reorders can be written to the wrong workspace after a workspace switch.
This debounced callback is memoized once, so it keeps the first
workspace.oid. If the sameTabBarinstance later receives another workspace, dragging tabs will still callUpdateWorkspaceTabIdsCommandfor the old workspace and can corrupt its tab order.🐛 Proposed fix
-import { createRef, memo, useCallback, useEffect, useRef, useState } from "react"; +import { createRef, memo, useCallback, useEffect, useMemo, useRef, useState } from "react"; @@ - const setUpdatedTabsDebounced = useCallback( - debounce(300, (tabIds: string[]) => { + const setUpdatedTabsDebounced = useMemo( + () => + debounce(300, (nextTabIds: string[]) => { // Reset styles tabRefs.current.forEach((ref) => { ref.current.style.zIndex = "0"; ref.current.classList.remove("animate"); }); // Reset dragging state setDraggingTab(null); // Update workspace tab ids - fireAndForget(() => env.rpc.UpdateWorkspaceTabIdsCommand(TabRpcClient, workspace.oid, tabIds)); - }), - [] + fireAndForget(() => env.rpc.UpdateWorkspaceTabIdsCommand(TabRpcClient, workspace.oid, nextTabIds)); + }), + [env.rpc, workspace.oid] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tab/tabbar.tsx` around lines 477 - 489, The debounced callback setUpdatedTabsDebounced captures the initial workspace.oid and never updates, so calls to env.rpc.UpdateWorkspaceTabIdsCommand(TabRpcClient, workspace.oid, tabIds) may target the wrong workspace; fix by recreating the debounced callback when workspace changes (or by reading the latest workspace.oid at call time). Concretely, move the debounce(...) creation into a useEffect that lists workspace.oid (and any other changing refs like TabRpcClient) in its dependency array and rebind setUpdatedTabsDebounced there, and ensure you clean up/cancel the previous debounced function on effect cleanup; keep the existing reset logic that touches tabRefs.current and setDraggingTab(null).pkg/tsgen/tsgen.go (1)
415-449:⚠️ Potential issue | 🟠 MajorServices are created as singletons with undefined
waveEnv, making env-scoped routing ineffective.Generated service methods depend on
this.waveEnvto dispatch through the environment's backend, butAllServiceImplsexports zero-argument singletons wherewaveEnvis never initialized. All service calls fall through toWOS.callBackendService, bypassing environment-specific dispatch needed for mocks and previews. Either create service instances perWaveEnv(passing env to constructor), or initializewaveEnvon singletons when environments are created.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tsgen/tsgen.go` around lines 415 - 449, The generated services are exported as zero-arg singletons with waveEnv unset (see GenerateServiceClass, the waveEnv field and constructor, and the final "export const %s = new %sType()" line), which breaks env-scoped routing; change the generation to either export a factory that accepts a WaveEnv and returns a new tsServiceNameType with waveEnv initialized (e.g. export function New<tsServiceName>(waveEnv: WaveEnv) { return new <tsServiceName>Type(waveEnv); }) or set waveEnv on the singleton when an environment is created; update the constructor signature in GenerateServiceClass to accept waveEnv (and assign it) and replace the "export const" singleton emission with a factory function (or an exported function to setWaveEnv) so each service instance is created/initialized with the correct WaveEnv.frontend/app/store/badge.ts (1)
116-146:⚠️ Potential issue | 🟡 MinorPotential cache inconsistency when mixing env/non-env calls.
getTabBadgeAtomcaches the result bytabId(line 120-123, 144), but the internaltabAtomis created based on whetherenvis provided (line 126-127). If called first withoutenv(production path), it caches an atom usingWOS.getWaveObjectAtom. Subsequent calls withenv(mock path) will return the cached production atom instead of creating a mock-aware one.This may not be an issue if production and mock environments are never mixed in the same runtime, but it could cause subtle bugs during development or testing if both paths are exercised.
💡 Possible fix: include env identity in cache key
function getTabBadgeAtom(tabId: string, env?: TabBadgesEnv): Atom<Badge[]> { if (tabId == null) { return NullAtom as Atom<Badge[]>; } - let rtn = TabBadgeAtomCache.get(tabId); + const cacheKey = env?.isMock ? `mock:${tabId}` : tabId; + let rtn = TabBadgeAtomCache.get(cacheKey); if (rtn != null) { return rtn; } // ... rest of function - TabBadgeAtomCache.set(tabId, rtn); + TabBadgeAtomCache.set(cacheKey, rtn); return rtn; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/store/badge.ts` around lines 116 - 146, The cached atom key must include the env identity so we don't mix production and mock atoms: change getTabBadgeAtom to derive a cache key from tabId plus an env discriminator (e.g., env?.id or a boolean like envProvided) before looking up TabBadgeAtomCache, and use that composite key when setting/getting the cache; ensure the branch that creates tabAtom still uses env?.wos.getWaveObjectAtom vs WOS.getWaveObjectAtom (referencing getTabBadgeAtom, TabBadgeAtomCache, tabAtom, WOS.getWaveObjectAtom, env.wos.getWaveObjectAtom) so mock and non-mock atoms are cached separately.
🧹 Nitpick comments (1)
eslint.config.js (1)
89-95: Consider documenting whyno-unused-varsis disabled for this file.Completely disabling
@typescript-eslint/no-unused-varsforservices.tsis a broad suppression. If this is due to generated code or intentional service mappings, a brief comment would help future maintainers understand the rationale.📝 Proposed documentation
{ + // services.ts contains generated service types and mappings where + // unused vars are intentional for type completeness files: ["frontend/app/store/services.ts"], rules: { "@typescript-eslint/no-unused-vars": "off", "prefer-rest-params": "off", }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 89 - 95, Add a short explanatory comment next to the override that disables "@typescript-eslint/no-unused-vars" for "frontend/app/store/services.ts" in eslint.config.js explaining why the rule is suppressed (e.g., generated file, intentional exported-but-unused service mappings), and if possible prefer a narrower approach (use file-level eslint-disable in the generated file or configure ignorePatterns for generated artifacts) or justify why a global override for that single file is required; reference the rule name "@typescript-eslint/no-unused-vars" and the affected path "frontend/app/store/services.ts" so maintainers can locate and understand the rationale.
🤖 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/store/tab-model.ts`:
- Around line 62-67: The code assumes waveEnv.mockModels exists when creating a
TabModel, which can be undefined per waveenv.ts; update the TabModel creation
path to defensively ensure mockModels is present (e.g., if waveEnv.mockModels is
null/undefined, assign a new Map and store it on waveEnv) before calling
get/set, and/or use a safe-get pattern; reference the TabModel constructor usage
and waveEnv.mockModels access in the block shown and ensure makeMockWaveEnv (or
its callers) initializes mockModels or the TabModel factory initializes it when
missing.
In `@frontend/app/tab/tabbar.tsx`:
- Around line 181-193: The tab bar doesn't recalc when chrome widths change
because changes to zoomFactor, app-menu/Wave visibility, or the new right-side
container don't call setSizeAndPosition; add a useEffect in tabbar.tsx that
installs a ResizeObserver on rightContainerRef, workspaceSwitcherRef,
waveAIButtonRef, and appMenuButtonRef (and/or subscribes to the atoms that
control their visibility like zoomFactor and the relevant env.atoms keys) to
call setSizeAndPosition whenever any observed element resizes or those atoms
change, and ensure you disconnect the observer on cleanup; reference
setSizeAndPosition, rightContainerRef, workspaceSwitcherRef, waveAIButtonRef,
appMenuButtonRef, and zoomFactor to locate the code to update.
In `@frontend/preview/mock/mockwaveenv.ts`:
- Around line 340-343: The mock exposes useWaveObjectValue as an arrow property
that calls the React hook useAtomValue, which is unconventional for custom
hooks; refactor it into a proper hook function named useWaveObjectValue<T
extends WaveObj>(oref: string): [T, boolean] { ... } that internally calls
env.wos.getWaveObjectAtom<T>(oref) and useAtomValue(objAtom) and returns the
value/flag; replace the property assignment with this function export so the
mock signature and usage match the real implementation in
frontend/app/store/wos.ts while keeping the same return behavior.
---
Outside diff comments:
In `@frontend/app/store/badge.ts`:
- Around line 116-146: The cached atom key must include the env identity so we
don't mix production and mock atoms: change getTabBadgeAtom to derive a cache
key from tabId plus an env discriminator (e.g., env?.id or a boolean like
envProvided) before looking up TabBadgeAtomCache, and use that composite key
when setting/getting the cache; ensure the branch that creates tabAtom still
uses env?.wos.getWaveObjectAtom vs WOS.getWaveObjectAtom (referencing
getTabBadgeAtom, TabBadgeAtomCache, tabAtom, WOS.getWaveObjectAtom,
env.wos.getWaveObjectAtom) so mock and non-mock atoms are cached separately.
In `@frontend/app/tab/tabbar.tsx`:
- Around line 477-489: The debounced callback setUpdatedTabsDebounced captures
the initial workspace.oid and never updates, so calls to
env.rpc.UpdateWorkspaceTabIdsCommand(TabRpcClient, workspace.oid, tabIds) may
target the wrong workspace; fix by recreating the debounced callback when
workspace changes (or by reading the latest workspace.oid at call time).
Concretely, move the debounce(...) creation into a useEffect that lists
workspace.oid (and any other changing refs like TabRpcClient) in its dependency
array and rebind setUpdatedTabsDebounced there, and ensure you clean up/cancel
the previous debounced function on effect cleanup; keep the existing reset logic
that touches tabRefs.current and setDraggingTab(null).
In `@frontend/app/tab/updatebanner.tsx`:
- Around line 15-49: The local flag dismissBanner is never set true so the
timeout branch is dead; either remove the auto-dismiss logic (clear references
to dismissBanner, dismissBannerTimeout, setDismissBannerTimeout and the timeout
handling inside the useEffect) or set dismissBanner based on appUpdateStatus
(e.g., in the useEffect switch set dismissBanner = true for the status that
should auto-dismiss such as "ready" or whichever statuses you want) and keep the
existing timeout/clear logic using dismissBannerTimeout, setDismissBannerTimeout
and setUpdateStatusMessage.
In `@pkg/tsgen/tsgen.go`:
- Around line 415-449: The generated services are exported as zero-arg
singletons with waveEnv unset (see GenerateServiceClass, the waveEnv field and
constructor, and the final "export const %s = new %sType()" line), which breaks
env-scoped routing; change the generation to either export a factory that
accepts a WaveEnv and returns a new tsServiceNameType with waveEnv initialized
(e.g. export function New<tsServiceName>(waveEnv: WaveEnv) { return new
<tsServiceName>Type(waveEnv); }) or set waveEnv on the singleton when an
environment is created; update the constructor signature in GenerateServiceClass
to accept waveEnv (and assign it) and replace the "export const" singleton
emission with a factory function (or an exported function to setWaveEnv) so each
service instance is created/initialized with the correct WaveEnv.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 89-95: Add a short explanatory comment next to the override that
disables "@typescript-eslint/no-unused-vars" for
"frontend/app/store/services.ts" in eslint.config.js explaining why the rule is
suppressed (e.g., generated file, intentional exported-but-unused service
mappings), and if possible prefer a narrower approach (use file-level
eslint-disable in the generated file or configure ignorePatterns for generated
artifacts) or justify why a global override for that single file is required;
reference the rule name "@typescript-eslint/no-unused-vars" and the affected
path "frontend/app/store/services.ts" so maintainers can locate and understand
the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1dca2d06-04da-4de7-9c9c-0c1499945c4e
📒 Files selected for processing (36)
.kilocode/skills/add-rpc/SKILL.md.kilocode/skills/waveenv/SKILL.mdcmd/generatets/main-generatets.goeslint.config.jsfrontend/app/store/badge.tsfrontend/app/store/contextmenu.tsfrontend/app/store/global-atoms.tsfrontend/app/store/global.tsfrontend/app/store/keymodel.tsfrontend/app/store/services.tsfrontend/app/store/tab-model.tsfrontend/app/store/wps.tsfrontend/app/store/wshclientapi.tsfrontend/app/tab/tab.tsxfrontend/app/tab/tabbar.scssfrontend/app/tab/tabbar.tsxfrontend/app/tab/tabbarenv.tsfrontend/app/tab/updatebanner.tsxfrontend/app/tab/workspaceswitcher.tsxfrontend/app/view/term/osc-handlers.tsfrontend/app/waveenv/waveenv.tsfrontend/app/waveenv/waveenvimpl.tsfrontend/app/workspace/widgets.tsxfrontend/preview/mock/mockwaveenv.tsfrontend/preview/preview.tsxfrontend/preview/previews/tabbar.preview.tsxfrontend/preview/previews/widgets.preview.tsxfrontend/types/custom.d.tsfrontend/util/platformutil.tspkg/service/objectservice/objectservice.gopkg/service/workspaceservice/workspaceservice.gopkg/tsgen/tsgen.gopkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshrpctypes.gopkg/wshrpc/wshserver/wshserver.gotsconfig.json
💤 Files with no reviewable changes (4)
- pkg/service/workspaceservice/workspaceservice.go
- pkg/service/objectservice/objectservice.go
- frontend/app/tab/tabbar.scss
- frontend/preview/previews/widgets.preview.tsx
| const key = `TabModel:${tabId}`; | ||
| let model = waveEnv.mockModels.get(key); | ||
| if (model == null) { | ||
| model = new TabModel(tabId, waveEnv); | ||
| tabModelCache.set(tabId, model); | ||
| waveEnv.mockModels.set(key, model); | ||
| } |
There was a problem hiding this comment.
Potential null reference if mockModels is undefined.
When waveEnv.isMock is true, the code accesses waveEnv.mockModels.get/set without a null check. While mockModels is always provided in makeMockWaveEnv, the type definition in waveenv.ts marks it as optional (mockModels?: Map<any, any>). This could cause a runtime error if a mock environment is created without mockModels.
🛡️ Proposed defensive fix
const key = `TabModel:${tabId}`;
- let model = waveEnv.mockModels.get(key);
+ let model = waveEnv.mockModels?.get(key);
if (model == null) {
model = new TabModel(tabId, waveEnv);
- waveEnv.mockModels.set(key, model);
+ waveEnv.mockModels?.set(key, model);
}
return model;📝 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.
| const key = `TabModel:${tabId}`; | |
| let model = waveEnv.mockModels.get(key); | |
| if (model == null) { | |
| model = new TabModel(tabId, waveEnv); | |
| tabModelCache.set(tabId, model); | |
| waveEnv.mockModels.set(key, model); | |
| } | |
| const key = `TabModel:${tabId}`; | |
| let model = waveEnv.mockModels?.get(key); | |
| if (model == null) { | |
| model = new TabModel(tabId, waveEnv); | |
| waveEnv.mockModels?.set(key, model); | |
| } |
🤖 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 62 - 67, The code assumes
waveEnv.mockModels exists when creating a TabModel, which can be undefined per
waveenv.ts; update the TabModel creation path to defensively ensure mockModels
is present (e.g., if waveEnv.mockModels is null/undefined, assign a new Map and
store it on waveEnv) before calling get/set, and/or use a safe-get pattern;
reference the TabModel constructor usage and waveEnv.mockModels access in the
block shown and ensure makeMockWaveEnv (or its callers) initializes mockModels
or the TabModel factory initializes it when missing.
| const rightContainerRef = useRef<HTMLDivElement>(null); | ||
| const workspaceSwitcherRef = useRef<HTMLDivElement>(null); | ||
| const waveAIButtonRef = useRef<HTMLDivElement>(null); | ||
| const appMenuButtonRef = useRef<HTMLDivElement>(null); | ||
| const tabWidthRef = useRef<number>(TabDefaultWidth); | ||
| const scrollableRef = useRef<boolean>(false); | ||
| const updateStatusBannerRef = useRef<HTMLButtonElement>(null); | ||
| const configErrorButtonRef = useRef<HTMLElement>(null); | ||
| const prevAllLoadedRef = useRef<boolean>(false); | ||
| const activeTabId = useAtomValue(atoms.staticTabId); | ||
| const isFullScreen = useAtomValue(atoms.isFullScreen); | ||
| const zoomFactor = useAtomValue(atoms.zoomFactorAtom); | ||
| const settings = useAtomValue(atoms.settingsAtom); | ||
| const activeTabId = useAtomValue(env.atoms.staticTabId); | ||
| const isFullScreen = useAtomValue(env.atoms.isFullScreen); | ||
| const zoomFactor = useAtomValue(env.atoms.zoomFactorAtom); | ||
| const settings = useAtomValue(env.atoms.settingsAtom); | ||
| const confirmClose = useAtomValue(env.getSettingsKeyAtom("tab:confirmclose")) ?? false; | ||
|
|
There was a problem hiding this comment.
Recalculate tab widths when the added chrome changes size.
spaceForTabs now depends on zoomFactor, app-menu visibility, Wave AI visibility, and the new right-side banner/error container, but those state changes do not trigger setSizeAndPosition(). The bar stays sized against stale widths until the window is manually resized.
A ResizeObserver on the chrome refs, or an effect keyed off the new atoms that affect those widths, would keep the layout in sync.
Also applies to: 238-252, 613-633
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/tab/tabbar.tsx` around lines 181 - 193, The tab bar doesn't
recalc when chrome widths change because changes to zoomFactor, app-menu/Wave
visibility, or the new right-side container don't call setSizeAndPosition; add a
useEffect in tabbar.tsx that installs a ResizeObserver on rightContainerRef,
workspaceSwitcherRef, waveAIButtonRef, and appMenuButtonRef (and/or subscribes
to the atoms that control their visibility like zoomFactor and the relevant
env.atoms keys) to call setSizeAndPosition whenever any observed element resizes
or those atoms change, and ensure you disconnect the observer on cleanup;
reference setSizeAndPosition, rightContainerRef, workspaceSwitcherRef,
waveAIButtonRef, appMenuButtonRef, and zoomFactor to locate the code to update.
| useWaveObjectValue: <T extends WaveObj>(oref: string): [T, boolean] => { | ||
| const obj = (overrides.mockWaveObjs?.[oref] ?? null) as T; | ||
| return [obj, false]; | ||
| const objAtom = env.wos.getWaveObjectAtom<T>(oref); | ||
| return [useAtomValue(objAtom), false]; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that useWaveObjectValue is only called from React components/hooks
# and follows the hooks naming convention in its call sites.
# Search for usages of useWaveObjectValue to ensure it's called correctly
rg -n "useWaveObjectValue" --type=ts -A2 -B2Repository: wavetermdev/waveterm
Length of output: 4321
🏁 Script executed:
# Get the full context of the actual useWaveObjectValue implementation
sed -n '251,270p' frontend/app/store/wos.ts
# Check if mockwaveenv.ts is used only in preview/test scenarios
fd "mockwaveenv" --type f
rg -l "mockwaveenv" --type=tsRepository: wavetermdev/waveterm
Length of output: 940
🏁 Script executed:
# Check how mockwaveenv is used in preview.tsx
sed -n '1,50p' frontend/preview/preview.tsx | head -30
# Check the structure - is this a storybook or similar preview system?
cat frontend/preview/preview.tsx | head -50
# Also check if the mock is meant to be a hook or just a utility object
sed -n '300,360p' frontend/preview/mock/mockwaveenv.tsRepository: wavetermdev/waveterm
Length of output: 6915
🏁 Script executed:
# Check if there's a way the mock could be restructured to follow hooks rules more strictly
# Look at how the mock wos object is used in preview components
sed -n '1,100p' frontend/preview/previews/widgets.preview.tsx
# Also check if there are any TypeScript or ESLint warnings about this pattern
rg "rules of hooks|useWaveObjectValue" --type=ts frontend/preview/Repository: wavetermdev/waveterm
Length of output: 3309
The mock implementation works correctly but uses an unconventional pattern.
While useWaveObjectValue calls useAtomValue (a React hook), the pattern is functionally sound because it's invoked from React components during render. However, for consistency with the actual implementation in frontend/app/store/wos.ts, consider refactoring this arrow function property into a proper custom hook function. The current pattern, while working, doesn't follow standard custom hook conventions where the function itself would be declared as function useWaveObjectValue(...) rather than assigned as a property.
🤖 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 340 - 343, The mock
exposes useWaveObjectValue as an arrow property that calls the React hook
useAtomValue, which is unconventional for custom hooks; refactor it into a
proper hook function named useWaveObjectValue<T extends WaveObj>(oref: string):
[T, boolean] { ... } that internally calls env.wos.getWaveObjectAtom<T>(oref)
and useAtomValue(objAtom) and returns the value/flag; replace the property
assignment with this function export so the mock signature and usage match the
real implementation in frontend/app/store/wos.ts while keeping the same return
behavior.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
frontend/app/tab/tabbar.tsx (1)
245-250:⚠️ Potential issue | 🟠 MajorRelayout still misses app-menu visibility changes.
spaceForTabssubtractsappMenuButtonWidth, and Line 623 can flipshowAppMenuButtonwhenwindow:showmenubarchanges, but the relayout effect never keys off that state. In the new preview, turning "Show menu bar" on/off leaves tab widths calculated against the previous chrome until the window is resized.🩹 Minimal fix
- }, [tabIds, tabsLoaded, newTabId, saveTabsPosition, hideAiButton, appUpdateStatus, hasConfigErrors, zoomFactor]); + }, [ + tabIds, + tabsLoaded, + newTabId, + saveTabsPosition, + hideAiButton, + showAppMenuButton, + appUpdateStatus, + hasConfigErrors, + zoomFactor, + ]);Also applies to: 333-345, 623-623
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tab/tabbar.tsx` around lines 245 - 250, The relayout effect that computes spaceForTabs uses appMenuButtonWidth but does not re-run when the computed visibility state showAppMenuButton changes, so toggling "Show menu bar" doesn't update tab widths until a resize; update the relevant useEffect(s) that compute tab widths (the effect reading tabbarWrapperRef, draggerLeftRef, rightContainerRef, addBtnRef, appMenuButtonRef, workspaceSwitcherRef and computing spaceForTabs) to include showAppMenuButton (or the derived appMenuButtonWidth state) in its dependency array so the effect reruns when showAppMenuButton flips; ensure any other width-calculation effects (the ones around the block that reads those refs and the handler that flips showAppMenuButton) also observe that state to keep layouts in sync.
🧹 Nitpick comments (1)
frontend/app/waveenv/waveenv.ts (1)
39-52: KeepWaveEnvSubsetnarrow by making mock fields opt-in.
WaveEnvSubset<T>now injectsisMock/mockSetWaveObj/mockModelsinto every named env, so contracts likefrontend/app/view/sysinfo/sysinfo.tsx:20-30,frontend/app/store/tab-model.ts:6-8,frontend/app/store/badge.ts:9-22, andfrontend/app/workspace/widgets.tsx:11-18silently pick up preview-only APIs they never asked for. That defeats the narrowing this PR is trying to introduce.♻️ Suggested shape
-type WaveEnvMockFields = { +export type WaveEnvMockFields = { isMock: WaveEnv["isMock"]; mockSetWaveObj: WaveEnv["mockSetWaveObj"]; mockModels: WaveEnv["mockModels"]; }; -export type WaveEnvSubset<T> = WaveEnvMockFields & - OmitNever<{ +export type WaveEnvSubset<T> = OmitNever<{ [K in keyof T]: K extends keyof ComplexWaveEnvKeys ? Subset<T[K], ComplexWaveEnvKeys[K]> : K extends keyof WaveEnv ? T[K] : never; }>;Then only the few preview-facing env types that really need these helpers can intersect
WaveEnvMockFieldsexplicitly.🤖 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 39 - 52, WaveEnvSubset currently always injects WaveEnvMockFields (isMock, mockSetWaveObj, mockModels) into every resulting type; remove that unconditional inclusion so mock helpers are opt-in. Update WaveEnvSubset<T> to only construct the mapped subset (the OmitNever<{ ... }>) and export WaveEnvMockFields separately, or alternatively add a second generic flag (e.g., WaveEnvSubset<T, WithMocks extends boolean = false>) that only intersects WaveEnvMockFields when WithMocks is true; adjust call sites that need preview helpers to explicitly intersect with WaveEnvMockFields (or pass WithMocks=true) rather than relying on automatic injection from WaveEnvSubset.
🤖 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/tab/updatebanner.tsx`:
- Around line 10-20: The function getUpdateStatusMessage currently declares a
return type of string but returns null in the default branch; change its
signature to return string | null to match the implementation (update the
function declaration getUpdateStatusMessage(status: string): string | null) and
verify any callers handle a possible null result (or narrow the return to always
string if you prefer, but the immediate fix is to change the annotated return
type to string | null).
In `@frontend/preview/mock/mockwaveenv.ts`:
- Around line 374-380: callBackendService currently calls the override
synchronously via Promise.resolve(fn(...args)), which means exceptions thrown by
fn propagate synchronously; change the invocation to create the promise first
and then call the override so thrown errors become rejected promises (e.g.
return Promise.resolve().then(() =>
overrides.services?.[service]?.[method](...args))). Update the call in
callBackendService and reference overrides.services and the local variable fn to
locate the change.
In `@frontend/types/custom.d.ts`:
- Around line 14-15: The type declarations in custom.d.ts are incorrect for
nullable runtime atoms; change workspaceId: jotai.Atom<string> to
jotai.Atom<string | null> and workspace: jotai.Atom<Workspace | null> so the
types reflect that global-atoms.ts can return null, then update any callers that
assume non-null (e.g., WorkspaceElem which accesses ws.oid) to handle the null
case or use the existing useAtomValueSafe helper; ensure workspaceIdAtom and
workspaceAtom are the referenced symbols that get the nullable types so
TypeScript prevents unsafe accesses.
---
Duplicate comments:
In `@frontend/app/tab/tabbar.tsx`:
- Around line 245-250: The relayout effect that computes spaceForTabs uses
appMenuButtonWidth but does not re-run when the computed visibility state
showAppMenuButton changes, so toggling "Show menu bar" doesn't update tab widths
until a resize; update the relevant useEffect(s) that compute tab widths (the
effect reading tabbarWrapperRef, draggerLeftRef, rightContainerRef, addBtnRef,
appMenuButtonRef, workspaceSwitcherRef and computing spaceForTabs) to include
showAppMenuButton (or the derived appMenuButtonWidth state) in its dependency
array so the effect reruns when showAppMenuButton flips; ensure any other
width-calculation effects (the ones around the block that reads those refs and
the handler that flips showAppMenuButton) also observe that state to keep
layouts in sync.
---
Nitpick comments:
In `@frontend/app/waveenv/waveenv.ts`:
- Around line 39-52: WaveEnvSubset currently always injects WaveEnvMockFields
(isMock, mockSetWaveObj, mockModels) into every resulting type; remove that
unconditional inclusion so mock helpers are opt-in. Update WaveEnvSubset<T> to
only construct the mapped subset (the OmitNever<{ ... }>) and export
WaveEnvMockFields separately, or alternatively add a second generic flag (e.g.,
WaveEnvSubset<T, WithMocks extends boolean = false>) that only intersects
WaveEnvMockFields when WithMocks is true; adjust call sites that need preview
helpers to explicitly intersect with WaveEnvMockFields (or pass WithMocks=true)
rather than relying on automatic injection from WaveEnvSubset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2ce79d5-ff32-4e2b-aa6e-9542cbd00129
📒 Files selected for processing (9)
frontend/app/store/global-atoms.tsfrontend/app/tab/tabbar.tsxfrontend/app/tab/tabbarenv.tsfrontend/app/tab/updatebanner.tsxfrontend/app/view/sysinfo/sysinfo.tsxfrontend/app/waveenv/waveenv.tsfrontend/preview/mock/mockwaveenv.tsfrontend/preview/previews/tabbar.preview.tsxfrontend/types/custom.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/store/global-atoms.ts
- frontend/app/tab/tabbarenv.ts
| function getUpdateStatusMessage(status: string): string { | ||
| switch (status) { | ||
| case "ready": | ||
| return "Update"; | ||
| case "downloading": | ||
| return "Downloading"; | ||
| case "installing": | ||
| return "Installing"; | ||
| default: | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the frontend TypeScript config enables strict null checking,
# which would make the current signature invalid.
fd 'tsconfig*.json' -t f | while read -r file; do
echo "== $file =="
rg -n '"strict"\s*:|"strictNullChecks"\s*:' "$file"
done
echo
echo "== updatebanner helper =="
sed -n '10,20p' frontend/app/tab/updatebanner.tsxRepository: wavetermdev/waveterm
Length of output: 567
Change getUpdateStatusMessage return type to string | null.
The function is declared as returning string, but the default case at line 19 returns null. Although strictNullChecks is disabled in the frontend tsconfig, the annotation is still incorrect and misleading—the actual return type is string | null. Update the signature to match the implementation:
-function getUpdateStatusMessage(status: string): string {
+function getUpdateStatusMessage(status: string): string | null {📝 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.
| function getUpdateStatusMessage(status: string): string { | |
| switch (status) { | |
| case "ready": | |
| return "Update"; | |
| case "downloading": | |
| return "Downloading"; | |
| case "installing": | |
| return "Installing"; | |
| default: | |
| return null; | |
| } | |
| function getUpdateStatusMessage(status: string): string | null { | |
| switch (status) { | |
| case "ready": | |
| return "Update"; | |
| case "downloading": | |
| return "Downloading"; | |
| case "installing": | |
| return "Installing"; | |
| default: | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/tab/updatebanner.tsx` around lines 10 - 20, The function
getUpdateStatusMessage currently declares a return type of string but returns
null in the default branch; change its signature to return string | null to
match the implementation (update the function declaration
getUpdateStatusMessage(status: string): string | null) and verify any callers
handle a possible null result (or narrow the return to always string if you
prefer, but the immediate fix is to change the annotated return type to string |
null).
| callBackendService: (service: string, method: string, args: any[], noUIContext?: boolean) => { | ||
| const fn = overrides.services?.[service]?.[method]; | ||
| if (fn) { | ||
| return Promise.resolve(fn(...args)); | ||
| } | ||
| console.log("[mock callBackendService]", service, method, args, noUIContext); | ||
| return Promise.resolve(null); |
There was a problem hiding this comment.
Preserve async error semantics in callBackendService.
Promise.resolve(fn(...args)) still invokes fn before a promise exists. If an override throws, callBackendService will throw synchronously instead of returning a rejected promise, so previews won't exercise the same failure path as production.
🩹 Minimal fix
callBackendService: (service: string, method: string, args: any[], noUIContext?: boolean) => {
const fn = overrides.services?.[service]?.[method];
if (fn) {
- return Promise.resolve(fn(...args));
+ return Promise.resolve().then(() => fn(...args));
}
console.log("[mock callBackendService]", service, method, args, noUIContext);
return Promise.resolve(null);
},🤖 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 374 - 380,
callBackendService currently calls the override synchronously via
Promise.resolve(fn(...args)), which means exceptions thrown by fn propagate
synchronously; change the invocation to create the promise first and then call
the override so thrown errors become rejected promises (e.g. return
Promise.resolve().then(() => overrides.services?.[service]?.[method](...args))).
Update the call in callBackendService and reference overrides.services and the
local variable fn to locate the change.
| workspaceId: jotai.Atom<string>; // derived from window WOS object | ||
| workspace: jotai.Atom<Workspace>; // driven from workspaceId via WOS |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for direct property accesses on workspace atom value without null checks
rg -n -C3 'useAtomValue\(.*workspace\)' --type=ts --type=tsx -g '!*.d.ts' | head -50Repository: wavetermdev/waveterm
Length of output: 91
🏁 Script executed:
# Search for workspace atom usages without null checks
rg -n -C3 'useAtomValue\(.*workspace' --type=ts -g '!*.d.ts' | head -100Repository: wavetermdev/waveterm
Length of output: 2867
🏁 Script executed:
# Locate global-atoms.ts and inspect the implementations
fd 'global-atoms' --type f -x cat {} \; -printRepository: wavetermdev/waveterm
Length of output: 294
🏁 Script executed:
# Verify the actual type declarations in custom.d.ts
rg -n 'workspaceId|workspace.*Atom' frontend/types/custom.d.tsRepository: wavetermdev/waveterm
Length of output: 699
🏁 Script executed:
# Find WorkspaceElem and check for the unsafe ws.oid access
rg -n 'WorkspaceElem' --type=ts -A 20 -B 2 | head -80Repository: wavetermdev/waveterm
Length of output: 2243
🏁 Script executed:
# Find and inspect global-atoms.ts
fd 'global-atoms' -t fRepository: wavetermdev/waveterm
Length of output: 138
🏁 Script executed:
# Check what happens with ws in WorkspaceElem after line 26
sed -n '26,80p' frontend/app/workspace/workspace.tsxRepository: wavetermdev/waveterm
Length of output: 2646
🏁 Script executed:
# Search for useAtomValueSafe to understand the null-safe pattern
rg -n 'useAtomValueSafe' --type=ts | head -20Repository: wavetermdev/waveterm
Length of output: 2225
🏁 Script executed:
# Check if ws is accessed directly or with null checks in WorkspaceElem
rg -n 'ws\.' frontend/app/workspace/workspace.tsxRepository: wavetermdev/waveterm
Length of output: 117
🏁 Script executed:
cat frontend/app/store/global-atoms.tsRepository: wavetermdev/waveterm
Length of output: 6795
Type declarations don't match nullable runtime behavior.
The implementation in global-atoms.ts returns null for both atoms:
workspaceIdAtomreturnswindowData?.workspaceid ?? nullbut is typed asAtom<string>workspaceAtomreturnsnullwhenworkspaceId == nullbut is typed asAtom<Workspace>
This causes unsafe accesses like ws.oid in WorkspaceElem (line 56 of workspace.tsx) to pass type-checking while being vulnerable to runtime crashes. Other components use useAtomValueSafe to handle nulls, confirming the runtime possibility.
Proposed fix
- workspaceId: jotai.Atom<string>; // derived from window WOS object
- workspace: jotai.Atom<Workspace>; // driven from workspaceId via WOS
+ workspaceId: jotai.Atom<string | null>; // derived from window WOS object
+ workspace: jotai.Atom<Workspace | null>; // driven from workspaceId via WOS📝 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.
| workspaceId: jotai.Atom<string>; // derived from window WOS object | |
| workspace: jotai.Atom<Workspace>; // driven from workspaceId via WOS | |
| workspaceId: jotai.Atom<string | null>; // derived from window WOS object | |
| workspace: jotai.Atom<Workspace | null>; // driven from workspaceId via WOS |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/types/custom.d.ts` around lines 14 - 15, The type declarations in
custom.d.ts are incorrect for nullable runtime atoms; change workspaceId:
jotai.Atom<string> to jotai.Atom<string | null> and workspace:
jotai.Atom<Workspace | null> so the types reflect that global-atoms.ts can
return null, then update any callers that assume non-null (e.g., WorkspaceElem
which accesses ws.oid) to handle the null case or use the existing
useAtomValueSafe helper; ensure workspaceIdAtom and workspaceAtom are the
referenced symbols that get the nullable types so TypeScript prevents unsafe
accesses.
tabbar.tsxdid not expose its WaveEnv dependencies explicitly, which made it awkward to preview in isolation. This change introduces a dedicatedTabBarEnvcontract and adds a fulltabbar.preview.tsxthat renders the tab bar with preview-safe mocked state for visual inspection.WaveEnv narrowing
frontend/app/tab/tabbarenv.tswith a focusedTabBarEnvsubset covering only the tab bar’s actual environment usage:tabbar.tsxto read those dependencies throughuseWaveEnv<TabBarEnv>()instead of reaching into broader global helpers for the narrowed surface.Tab bar preview
frontend/preview/previews/tabbar.preview.tsxwith a full visual mock of the tab bar shell.TabV,WaveAIButton,UpdateStatusBanner,ConfigErrorIcon) where that provides useful fidelity.Preview controls
Consistency cleanup
tab:confirmcloseaccess intabbar.tsxto the narrowedgetSettingsKeyAtom(...)path so settings reads follow the same WaveEnv pattern throughout the component.Example of the new narrowed env contract:
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.