Conversation
Contributor
WalkthroughAdds a new "app:tabbar" setting (values "top" or "left") and default config/schema entries. Introduces left-side VTabBar support across UI: new tab context menu module, VTab/VTabBar refactors, tab bar preview updates, Mac-specific header/traffic-light handling, and AI panel rounding prop. Implements nested outer/inner panel layout with WorkspaceLayoutModel changes, persistence for vtab width, and new layout helpers. Adds macOS version RPC (MacOSVersion) and client/server wrappers, platform utilities, meta/config constants/fields, and various atom/rpc type surface extensions. Small build/taskfile and gitignore edits included. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying waveterm with
|
| Latest commit: |
aa38645
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://87454df9.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-vtabbar-3.waveterm.pages.dev |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
frontend/wave.ts
Outdated
| initGlobalWaveEventSubs(initOpts); | ||
| subscribeToConnEvents(); | ||
| if (isMacOS()) { | ||
| const macOSVersion = await RpcApi.MacOSVersionCommand(TabRpcClient); |
Contributor
There was a problem hiding this comment.
WARNING: Missing error handling for MacOSVersionCommand RPC call. If this RPC fails (e.g., server not ready), it will throw an unhandled error and could crash the app initialization.
Consider wrapping this in a try-catch to gracefully handle failures:
if (isMacOS()) {
try {
const macOSVersion = await RpcApi.MacOSVersionCommand(TabRpcClient);
setMacOSVersion(macOSVersion);
} catch (e) {
console.warn("Failed to get macOS version:", e);
}
}
Contributor
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Issues already covered in existing comments:
Files Reviewed (28 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/app/tab/vtab.tsx (1)
111-113: MoverenameRefwiring out of render.Assigning
renameRef.currenthere makes the callback publication a render-time side effect. In React's concurrent/aborted renders that can leave callers holding a stale rename handler; set it in an effect and clear it on cleanup instead.Suggested change
- if (renameRef != null) { - renameRef.current = startRename; - } + useEffect(() => { + if (renameRef == null) { + return; + } + renameRef.current = startRename; + return () => { + renameRef.current = null; + }; + }, [renameRef, startRename]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tab/vtab.tsx` around lines 111 - 113, The renameRef.current assignment is done during render (renameRef and startRename in vtab.tsx), causing a render-time side effect; move this wiring into a useEffect that runs when startRename or renameRef changes, set renameRef.current = startRename inside the effect and return a cleanup that clears renameRef.current (or sets it to null) to avoid stale handlers and handle nullish renameRef checks.frontend/app/tab/vtabbar.tsx (2)
79-86: Arrow function wrapper creates new function reference each render.The
() => onClose()wrapper on line 82 creates a new function on each render, which may cause unnecessary context menu rebuilds. SincebuildTabContextMenureceives this asonClose, the dependency array[tabId, onClose, env]won't detect changes to the wrapper itself. Consider usingonClosedirectly if the signature matches, or memoizing the wrapper.Proposed simplification (if onClose signature matches)
const handleContextMenu = useCallback( (e: React.MouseEvent<HTMLDivElement>) => { e.preventDefault(); - const menu = buildTabContextMenu(tabId, renameRef, () => onClose(), env); + const menu = buildTabContextMenu(tabId, renameRef, onClose, env); env.showContextMenu(menu, e); }, [tabId, onClose, env] );Note: This requires verifying that
onCloseinVTabWrapperProps(line 31:onClose: () => void) matches the expected signature inbuildTabContextMenu(which expects(event: React.MouseEvent<HTMLButtonElement, MouseEvent> | null) => void). If they differ, the wrapper is necessary but could be memoized withuseCallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tab/vtabbar.tsx` around lines 79 - 86, The arrow wrapper "() => onClose()" passed into buildTabContextMenu inside handleContextMenu creates a new function each render and defeats the dependency tracking; either pass onClose directly to buildTabContextMenu if its signature matches (verify buildTabContextMenu's expected onClose signature against VTabWrapperProps.onClose), or memoize the wrapper with useCallback (e.g., const memoOnClose = useCallback(() => onClose(), [onClose])) and pass memoOnClose; update handleContextMenu to use the direct onClose or memoOnClose and adjust its dependency array to include only the stable references (tabId, onClose or memoOnClose, env, renameRef as needed).
141-155: Consider consolidating duplicate scroll-into-view effects.These two effects have nearly identical logic—both scroll the active tab into view. The first triggers on
activeTabIdchange, the second ondocumentHasFocuschange. They could be combined into a single effect with both dependencies, reducing code duplication.Proposed consolidation
useEffect(() => { - if (activeTabId == null || scrollContainerRef.current == null) { + if (!documentHasFocus || activeTabId == null || scrollContainerRef.current == null) { return; } const el = scrollContainerRef.current.querySelector(`[data-tabid="${activeTabId}"]`); el?.scrollIntoView({ block: "nearest" }); - }, [activeTabId]); - - useEffect(() => { - if (!documentHasFocus || activeTabId == null || scrollContainerRef.current == null) { - return; - } - const el = scrollContainerRef.current.querySelector(`[data-tabid="${activeTabId}"]`); - el?.scrollIntoView({ block: "nearest" }); - }, [documentHasFocus]); + }, [activeTabId, documentHasFocus]);However, note that combining them changes behavior slightly: the original scrolls on
activeTabIdchange regardless of focus, while this version requires focus. If the intent is to scroll on tab change even when unfocused, keep them separate or adjust the condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/tab/vtabbar.tsx` around lines 141 - 155, Two duplicate useEffect blocks both call scrollContainerRef.current.querySelector(`[data-tabid="${activeTabId}"]`) and el?.scrollIntoView; consolidate them by replacing both with a single useEffect that depends on [activeTabId, documentHasFocus], references the same scrollContainerRef and data-tabid selector, and runs the scroll logic when activeTabId != null and scrollContainerRef.current != null. If you must preserve the original behavior of scrolling on activeTabId changes even when unfocused, keep a separate useEffect for activeTabId only (the existing one) and remove only the redundant documentHasFocus effect; otherwise remove both and add the single combined useEffect around the scrollIntoView call inside the effect body.frontend/app/workspace/workspace-layout-model.ts (1)
156-164: Side effect in getter method.
getResolvedAIWidthmutatesthis.aiPanelWidthon line 161 when the value is null. While this is likely intentional to cache the computed default, it's unexpected behavior for a method with "get" semantics. Consider documenting this behavior or renaming to clarify it initializes state.Alternative: explicit initialization
- private getResolvedAIWidth(windowWidth: number): number { + private getResolvedAIWidth(windowWidth: number): number { this.initializeFromMeta(); let w = this.aiPanelWidth; if (w == null) { w = Math.max(AIPanel_DefaultWidth, windowWidth * AIPanel_DefaultWidthRatio); - this.aiPanelWidth = w; + this.aiPanelWidth = w; // Cache computed default } return clampAIPanelWidth(w, windowWidth); }Or rename to
ensureAndGetResolvedAIWidthto clarify mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/workspace/workspace-layout-model.ts` around lines 156 - 164, The method getResolvedAIWidth currently mutates this.aiPanelWidth when null, which violates "get" semantics; either (A) remove the side-effect: have getResolvedAIWidth call initializeFromMeta(), compute a local w when this.aiPanelWidth is null and return clampAIPanelWidth(w, windowWidth) without assigning to this.aiPanelWidth, or (B) if caching is intended, move the assignment into initializeFromMeta() (so initializeFromMeta sets this.aiPanelWidth) or rename getResolvedAIWidth to ensureAndGetResolvedAIWidth to make the mutation explicit; update references to getResolvedAIWidth accordingly and keep symbols initializeFromMeta, this.aiPanelWidth, getResolvedAIWidth, and clampAIPanelWidth in mind 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/tab/tabbar.tsx`:
- Around line 691-697: The tabs-wrapper currently uses tabsWrapperWidth computed
from tabIds and tabWidthRef even when noTabs is true, causing an empty
horizontal drag region; update the div rendering (className "tabs-wrapper") to
set style.width to "100%" when noTabs is true (instead of `${tabIds.length *
tabWidthRef.current}px`) so the wrapper fills available space; keep the existing
WebkitAppRegion: "drag" behavior when noTabs is true and only change the width
logic that references tabIds/tabWidthRef.
In `@frontend/app/workspace/workspace.tsx`:
- Around line 43-61: The effect that calls workspaceLayoutModel.registerRefs
currently uses showLeftTabBar but has an empty dependency array; update the
useEffect dependencies to include showLeftTabBar so the refs are re-registered
when the tabbar setting changes, keeping the existing guard that checks
aiPanelRef.current, outerPanelGroupRef.current, innerPanelGroupRef.current,
panelContainerRef.current, and aiPanelWrapperRef.current before calling
workspaceLayoutModel.registerRefs (also still pass vtabPanelRef.current ??
undefined and showLeftTabBar as existing).
In `@frontend/wave.ts`:
- Around line 167-170: The macOS probe can throw and block startup; wrap the
call to RpcApi.MacOSVersionCommand(TabRpcClient) inside a try/catch so failures
are swallowed (logged) and do not abort initialization—only call
setMacOSVersion(macOSVersion) on success; keep the surrounding isMacOS() check
and use TabRpcClient as before, but ensure any error is caught and handled
(e.g., console/process logger) so Wave continues rendering with a nil/default
macOS version.
---
Nitpick comments:
In `@frontend/app/tab/vtab.tsx`:
- Around line 111-113: The renameRef.current assignment is done during render
(renameRef and startRename in vtab.tsx), causing a render-time side effect; move
this wiring into a useEffect that runs when startRename or renameRef changes,
set renameRef.current = startRename inside the effect and return a cleanup that
clears renameRef.current (or sets it to null) to avoid stale handlers and handle
nullish renameRef checks.
In `@frontend/app/tab/vtabbar.tsx`:
- Around line 79-86: The arrow wrapper "() => onClose()" passed into
buildTabContextMenu inside handleContextMenu creates a new function each render
and defeats the dependency tracking; either pass onClose directly to
buildTabContextMenu if its signature matches (verify buildTabContextMenu's
expected onClose signature against VTabWrapperProps.onClose), or memoize the
wrapper with useCallback (e.g., const memoOnClose = useCallback(() => onClose(),
[onClose])) and pass memoOnClose; update handleContextMenu to use the direct
onClose or memoOnClose and adjust its dependency array to include only the
stable references (tabId, onClose or memoOnClose, env, renameRef as needed).
- Around line 141-155: Two duplicate useEffect blocks both call
scrollContainerRef.current.querySelector(`[data-tabid="${activeTabId}"]`) and
el?.scrollIntoView; consolidate them by replacing both with a single useEffect
that depends on [activeTabId, documentHasFocus], references the same
scrollContainerRef and data-tabid selector, and runs the scroll logic when
activeTabId != null and scrollContainerRef.current != null. If you must preserve
the original behavior of scrolling on activeTabId changes even when unfocused,
keep a separate useEffect for activeTabId only (the existing one) and remove
only the redundant documentHasFocus effect; otherwise remove both and add the
single combined useEffect around the scrollIntoView call inside the effect body.
In `@frontend/app/workspace/workspace-layout-model.ts`:
- Around line 156-164: The method getResolvedAIWidth currently mutates
this.aiPanelWidth when null, which violates "get" semantics; either (A) remove
the side-effect: have getResolvedAIWidth call initializeFromMeta(), compute a
local w when this.aiPanelWidth is null and return clampAIPanelWidth(w,
windowWidth) without assigning to this.aiPanelWidth, or (B) if caching is
intended, move the assignment into initializeFromMeta() (so initializeFromMeta
sets this.aiPanelWidth) or rename getResolvedAIWidth to
ensureAndGetResolvedAIWidth to make the mutation explicit; update references to
getResolvedAIWidth accordingly and keep symbols initializeFromMeta,
this.aiPanelWidth, getResolvedAIWidth, and clampAIPanelWidth in mind when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 071d6d47-b17a-4fc1-8153-529104e3cebe
📒 Files selected for processing (30)
.gitignoreTaskfile.ymlcmd/wsh/cmd/wshcmd-root.godocs/docs/config.mdxfrontend/app/aipanel/aipanel.tsxfrontend/app/store/wshclientapi.tsfrontend/app/tab/tab.tsxfrontend/app/tab/tabbar.tsxfrontend/app/tab/tabbarenv.tsfrontend/app/tab/tabcontextmenu.tsfrontend/app/tab/vtab.tsxfrontend/app/tab/vtabbar.tsxfrontend/app/tab/vtabbarenv.tsfrontend/app/workspace/workspace-layout-model.tsfrontend/app/workspace/workspace.tsxfrontend/builder/builder-workspace.tsxfrontend/preview/previews/vtabbar.preview.tsxfrontend/types/gotypes.d.tsfrontend/util/platformutil.tsfrontend/util/util.tsfrontend/wave.tspkg/waveobj/metaconsts.gopkg/waveobj/wtypemeta.gopkg/wconfig/defaultconfig/settings.jsonpkg/wconfig/metaconsts.gopkg/wconfig/settingsconfig.gopkg/wshrpc/wshclient/wshclient.gopkg/wshrpc/wshrpctypes.gopkg/wshrpc/wshserver/wshserver.goschema/settings.json
Comment on lines
+691
to
+697
| <div | ||
| className="tabs-wrapper" | ||
| ref={tabsWrapperRef} | ||
| style={{ | ||
| width: `${tabsWrapperWidth}px`, | ||
| ...(noTabs ? ({ WebkitAppRegion: "drag" } as React.CSSProperties) : {}), | ||
| }} |
Contributor
There was a problem hiding this comment.
Don't size the hidden strip by tab count.
When noTabs is true, this wrapper still expands to tabIds.length * tabWidthRef.current, so left-tab mode can end up with an empty horizontal scroller/drag region on large workspaces. Making the wrapper fill the available space avoids that blank strip.
Suggested change
<div
className="tabs-wrapper"
ref={tabsWrapperRef}
style={{
- width: `${tabsWrapperWidth}px`,
+ width: noTabs ? "100%" : `${tabsWrapperWidth}px`,
...(noTabs ? ({ WebkitAppRegion: "drag" } as React.CSSProperties) : {}),
}}
>🤖 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 691 - 697, The tabs-wrapper
currently uses tabsWrapperWidth computed from tabIds and tabWidthRef even when
noTabs is true, causing an empty horizontal drag region; update the div
rendering (className "tabs-wrapper") to set style.width to "100%" when noTabs is
true (instead of `${tabIds.length * tabWidthRef.current}px`) so the wrapper
fills available space; keep the existing WebkitAppRegion: "drag" behavior when
noTabs is true and only change the width logic that references
tabIds/tabWidthRef.
Comment on lines
43
to
61
| useEffect(() => { | ||
| if (aiPanelRef.current && panelGroupRef.current && panelContainerRef.current && aiPanelWrapperRef.current) { | ||
| if ( | ||
| aiPanelRef.current && | ||
| outerPanelGroupRef.current && | ||
| innerPanelGroupRef.current && | ||
| panelContainerRef.current && | ||
| aiPanelWrapperRef.current | ||
| ) { | ||
| workspaceLayoutModel.registerRefs( | ||
| aiPanelRef.current, | ||
| panelGroupRef.current, | ||
| outerPanelGroupRef.current, | ||
| innerPanelGroupRef.current, | ||
| panelContainerRef.current, | ||
| aiPanelWrapperRef.current | ||
| aiPanelWrapperRef.current, | ||
| vtabPanelRef.current ?? undefined, | ||
| showLeftTabBar | ||
| ); | ||
| } | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Missing showLeftTabBar in dependency array.
The useEffect has an empty dependency array [], but the callback uses showLeftTabBar (line 58). This means if the user changes the app:tabbar setting at runtime, the refs won't be re-registered with the updated showLeftTabBar value. While line 68-70 has a separate effect calling setShowLeftTabBar, the initial registration may use a stale value if showLeftTabBar changes before refs are ready.
Proposed fix
workspaceLayoutModel.registerRefs(
aiPanelRef.current,
outerPanelGroupRef.current,
innerPanelGroupRef.current,
panelContainerRef.current,
aiPanelWrapperRef.current,
vtabPanelRef.current ?? undefined,
showLeftTabBar
);
}
-}, []);
+}, [showLeftTabBar]);📝 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 (aiPanelRef.current && panelGroupRef.current && panelContainerRef.current && aiPanelWrapperRef.current) { | |
| if ( | |
| aiPanelRef.current && | |
| outerPanelGroupRef.current && | |
| innerPanelGroupRef.current && | |
| panelContainerRef.current && | |
| aiPanelWrapperRef.current | |
| ) { | |
| workspaceLayoutModel.registerRefs( | |
| aiPanelRef.current, | |
| panelGroupRef.current, | |
| outerPanelGroupRef.current, | |
| innerPanelGroupRef.current, | |
| panelContainerRef.current, | |
| aiPanelWrapperRef.current | |
| aiPanelWrapperRef.current, | |
| vtabPanelRef.current ?? undefined, | |
| showLeftTabBar | |
| ); | |
| } | |
| }, []); | |
| useEffect(() => { | |
| if ( | |
| aiPanelRef.current && | |
| outerPanelGroupRef.current && | |
| innerPanelGroupRef.current && | |
| panelContainerRef.current && | |
| aiPanelWrapperRef.current | |
| ) { | |
| workspaceLayoutModel.registerRefs( | |
| aiPanelRef.current, | |
| outerPanelGroupRef.current, | |
| innerPanelGroupRef.current, | |
| panelContainerRef.current, | |
| aiPanelWrapperRef.current, | |
| vtabPanelRef.current ?? undefined, | |
| showLeftTabBar | |
| ); | |
| } | |
| }, [showLeftTabBar]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/workspace/workspace.tsx` around lines 43 - 61, The effect that
calls workspaceLayoutModel.registerRefs currently uses showLeftTabBar but has an
empty dependency array; update the useEffect dependencies to include
showLeftTabBar so the refs are re-registered when the tabbar setting changes,
keeping the existing guard that checks aiPanelRef.current,
outerPanelGroupRef.current, innerPanelGroupRef.current,
panelContainerRef.current, and aiPanelWrapperRef.current before calling
workspaceLayoutModel.registerRefs (also still pass vtabPanelRef.current ??
undefined and showLeftTabBar as existing).
frontend/wave.ts
Outdated
Comment on lines
+167
to
+170
| if (isMacOS()) { | ||
| const macOSVersion = await RpcApi.MacOSVersionCommand(TabRpcClient); | ||
| setMacOSVersion(macOSVersion); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t let macOS version probing block app startup.
If MacOSVersionCommand fails on Line 168, initialization can abort even though this is non-critical metadata. Guard it so Wave still renders.
💡 Suggested fix
if (isMacOS()) {
- const macOSVersion = await RpcApi.MacOSVersionCommand(TabRpcClient);
- setMacOSVersion(macOSVersion);
+ try {
+ const macOSVersion = await RpcApi.MacOSVersionCommand(TabRpcClient);
+ setMacOSVersion(macOSVersion);
+ } catch (e) {
+ getApi().sendLog("MacOSVersionCommand failed: " + (e?.message ?? e));
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/wave.ts` around lines 167 - 170, The macOS probe can throw and block
startup; wrap the call to RpcApi.MacOSVersionCommand(TabRpcClient) inside a
try/catch so failures are swallowed (logged) and do not abort
initialization—only call setMacOSVersion(macOSVersion) on success; keep the
surrounding isMacOS() check and use TabRpcClient as before, but ensure any error
is caught and handled (e.g., console/process logger) so Wave continues rendering
with a nil/default macOS version.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/vtabbar.tsx`:
- Around line 230-244: startScrollLoop creates a persistent
requestAnimationFrame stored in scrollAnimFrameRef but there is no cleanup on
unmount; add a useEffect cleanup that checks scrollAnimFrameRef.current and
calls cancelAnimationFrame(scrollAnimFrameRef.current) and sets it to null to
prevent the loop from running after unmount (reference scrollAnimFrameRef,
startScrollLoop, and scrollContainerRef in the effect) so any pending animation
frame is cancelled when the component unmounts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a1f78739-ecfb-472e-b647-7b0bd0b34e28
📒 Files selected for processing (8)
frontend/app/tab/tabcontent.tsxfrontend/app/tab/tabcontextmenu.tsfrontend/app/tab/updatebanner.tsxfrontend/app/tab/vtabbar.tsxfrontend/app/tab/vtabbarenv.tsfrontend/app/workspace/workspace-layout-model.tsfrontend/app/workspace/workspace.tsxfrontend/preview/previews/vtabbar.preview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/tab/tabcontextmenu.ts
Comment on lines
+230
to
+244
| const startScrollLoop = useCallback(() => { | ||
| if (scrollAnimFrameRef.current != null) { | ||
| return; | ||
| } | ||
| const loop = () => { | ||
| const container = scrollContainerRef.current; | ||
| if (container == null || scrollDirectionRef.current === 0) { | ||
| scrollAnimFrameRef.current = null; | ||
| return; | ||
| } | ||
| container.scrollTop += scrollDirectionRef.current * scrollSpeedRef.current; | ||
| scrollAnimFrameRef.current = requestAnimationFrame(loop); | ||
| }; | ||
| scrollAnimFrameRef.current = requestAnimationFrame(loop); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Missing cleanup for animation frame on unmount.
If the component unmounts while a drag is in progress (e.g., user switches tabs or closes workspace), scrollAnimFrameRef won't be canceled, leading to a memory leak and potential errors from callbacks on unmounted components.
🛡️ Proposed fix to add cleanup effect
+ useEffect(() => {
+ return () => {
+ if (scrollAnimFrameRef.current != null) {
+ cancelAnimationFrame(scrollAnimFrameRef.current);
+ }
+ };
+ }, []);
+
const updateScrollFromDragY = useCallback(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/tab/vtabbar.tsx` around lines 230 - 244, startScrollLoop creates
a persistent requestAnimationFrame stored in scrollAnimFrameRef but there is no
cleanup on unmount; add a useEffect cleanup that checks
scrollAnimFrameRef.current and calls
cancelAnimationFrame(scrollAnimFrameRef.current) and sets it to null to prevent
the loop from running after unmount (reference scrollAnimFrameRef,
startScrollLoop, and scrollContainerRef in the effect) so any pending animation
frame is cancelled when the component unmounts.
| @@ -1,4 +1,4 @@ | |||
| // Copyright 2025, Command Line Inc. | |||
| // Copyright 2026, Command Line Inc. | |||
| // SPDX-License-Identifier: Apache-2.0s | |||
Contributor
There was a problem hiding this comment.
CRITICAL: Typo in SPDX license identifier - Apache-2.0s should be Apache-2.0
This invalidates the license header. The correct identifier is Apache-2.0.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/app/tab/tabbar.tsx (1)
691-697:⚠️ Potential issue | 🟠 Major
noTabsmode collapses the drag surface to zero width.Line 695 sets
.tabs-wrapperwidth to0whennoTabsis true, but Line 696 applies the drag region to that same element. This effectively removes the intended draggable area in the middle section.Suggested fix
<div className="tabs-wrapper" ref={tabsWrapperRef} style={{ - width: noTabs ? 0 : tabsWrapperWidth, + width: noTabs ? "100%" : tabsWrapperWidth, ...(noTabs ? ({ WebkitAppRegion: "drag" } as React.CSSProperties) : {}), }} >🤖 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 691 - 697, The draggable area is being collapsed when noTabs is true because the same element (class "tabs-wrapper", ref tabsWrapperRef) gets width: 0; to fix, keep the drag surface from collapsing by ensuring the element that receives WebkitAppRegion: "drag" retains a non-zero width: either (A) remove the width: 0 branch and use a separate property to hide tab content (keep width: tabsWrapperWidth and conditionally render/tab-hide inner content), or (B) create a sibling/child draggable overlay element (e.g., a "drag-overlay") that always has WebkitAppRegion: "drag" and a fixed non-zero width while the actual tab content can be collapsed when noTabs is true; update the JSX around the tabs-wrapper element and its styles so the drag region remains available when noTabs is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/app/tab/tabbar.tsx`:
- Around line 691-697: The draggable area is being collapsed when noTabs is true
because the same element (class "tabs-wrapper", ref tabsWrapperRef) gets width:
0; to fix, keep the drag surface from collapsing by ensuring the element that
receives WebkitAppRegion: "drag" retains a non-zero width: either (A) remove the
width: 0 branch and use a separate property to hide tab content (keep width:
tabsWrapperWidth and conditionally render/tab-hide inner content), or (B) create
a sibling/child draggable overlay element (e.g., a "drag-overlay") that always
has WebkitAppRegion: "drag" and a fixed non-zero width while the actual tab
content can be collapsed when noTabs is true; update the JSX around the
tabs-wrapper element and its styles so the drag region remains available when
noTabs is true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3182a3a-50e0-43a7-88ef-15dbc5547cf2
📒 Files selected for processing (3)
frontend/app/tab/tabbar.tsxfrontend/app/workspace/workspace.tsxfrontend/wave.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/wave.ts
- frontend/app/workspace/workspace.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lots of work on the vtabbar UI / UX to make it work and integrate into the Wave UI
Lots of work on the workspace-layout-model to handle two resizable panels.