getting vtabbar into shape, share mock between the tab bars#3047
Conversation
WalkthroughThis pull request refactors the VTabBar component from an explicit props-based API (accepting tabs, activeTabId, and multiple callbacks) to a workspace-driven architecture. The component now accepts a workspace object and internalizes tab management through environment/electron RPC calls. A new VTabWrapper component encapsulates per-tab rendering with drag-and-drop handlers. The changes include introducing tab ordering state synchronized with workspace.tabids, adding per-tab data resolution with CSS color validation for flags, and replacing direct event handlers with environment-driven actions. A new VTabBarEnv type defines the environment surface, and preview/mock infrastructure is updated to support the new architecture with TabBarMockEnvProvider for test environments. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Changes Overview:
This pattern is consistent with other components in the codebase. No runtime errors, null/undefined access issues, or logic bugs were found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
frontend/preview/mock/tabbar-mock.tsx (2)
167-172: Missing dependency inuseMemoforbaseEnv.The
useMemohas an empty dependency array but usesbaseEnv. IfbaseEnvchanges (unlikely in practice but possible), the memoized value won't update. Consider addingbaseEnvto the dependency array for correctness.♻️ Proposed fix
- const tabEnv = useMemo(() => makeTabBarMockEnv(baseEnv, envRef, PlatformMacOS), []); + const tabEnv = useMemo(() => makeTabBarMockEnv(baseEnv, envRef, PlatformMacOS), [baseEnv]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/mock/tabbar-mock.tsx` around lines 167 - 172, The useMemo in TabBarMockEnvProvider currently captures baseEnv but uses an empty dependency array, so update the memoization to include baseEnv (and keep envRef and PlatformMacOS if desired) so the result of makeTabBarMockEnv(baseEnv, envRef, PlatformMacOS) is recalculated when baseEnv changes; modify the dependency array passed to useMemo accordingly to reference baseEnv (and any other stable refs you want to observe).
116-134: Consider return type consistency forcreateTab.The
createTabfunction returnsvoidimplicitly, whilecloseTabreturnsPromise<boolean>andsetActiveTabreturnsvoid. This matches the usage patterns, but note that the actualcreateTabmight be expected to return a promise or the new tab ID in some scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/mock/tabbar-mock.tsx` around lines 116 - 134, createTab currently returns void while closeTab returns Promise<boolean>; make createTab's return type explicit and consistent by returning the created tab id (string) — or if you prefer async parity with closeTab, return Promise<string>. Update the createTab implementation in the TabBarMock (function createTab) to compute newTabId from crypto.randomUUID(), perform the same envRef.current/mockSetWaveObj/globalStore.set operations (references: envRef.current, mockSetWaveObj, globalStore.get, globalStore.set, TabBarMockWorkspaceId) and then return newTabId (or wrap in a resolved Promise if choosing Promise<string>), and update any call sites that expect the new value or a promise accordingly.frontend/preview/previews/tabbar.preview.tsx (1)
38-52: Missing dependency inuseEffectforloadBadges.The
useEffectat lines 50-52 has an empty dependency array but usesloadBadgesEnv. While the env reference is stable, React's exhaustive-deps rule would flag this. Consider adding the dependency for lint compliance.♻️ Proposed fix
useEffect(() => { loadBadges(loadBadgesEnv); - }, []); + }, [loadBadgesEnv]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/previews/tabbar.preview.tsx` around lines 38 - 52, The useEffect inside TabBarPreviewInner calls loadBadges(loadBadgesEnv) but has an empty dependency array, which triggers exhaustive-deps linting; update the effect to include the dependency (e.g., add loadBadgesEnv or the loadBadges function reference) in the dependency array so React knows when to re-run the effect — locate the useEffect in TabBarPreviewInner and add loadBadgesEnv (or loadBadges if that is imported/defined) to the dependency list to satisfy lint rules.frontend/preview/previews/vtabbar.preview.tsx (1)
26-33: Missing dependency inuseEffectforloadBadges.Same issue as in
tabbar.preview.tsx- theuseEffecthas an empty dependency array but usesloadBadgesEnv.♻️ Proposed fix
useEffect(() => { loadBadges(loadBadgesEnv); - }, []); + }, [loadBadgesEnv]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/preview/previews/vtabbar.preview.tsx` around lines 26 - 33, The useEffect in VTabBarPreviewInner uses loadBadges(loadBadgesEnv) but has an empty dependency array; update the effect to include loadBadgesEnv (and loadBadges if it's not stable) in the dependency array so React/ESLint won't warn and the effect runs when the env changes; locate the useEffect inside function VTabBarPreviewInner and either add loadBadgesEnv to the dependencies or memoize loadBadges before use to ensure stable references.frontend/app/tab/vtabbar.tsx (1)
119-127: Potential stale closure withworkspace?.tabidsdependency.The first
useEffectusesworkspace?.tabidsas a dependency, but this creates a new array reference on each render, causing unnecessary effect runs. The seconduseEffect(lines 123-127) forreinitVersionalso readsworkspace?.tabidsdirectly inside but doesn't list it as a dependency.Consider using a stable reference or memoizing the tabIds comparison:
♻️ Proposed fix using JSON comparison for stable dependency
+ const tabIdsKey = JSON.stringify(tabIds); + useEffect(() => { setOrderedTabIds(tabIds); - }, [workspace?.tabids]); + }, [tabIdsKey]); useEffect(() => { if (reinitVersion > 0) { setOrderedTabIds(workspace?.tabids ?? []); } - }, [reinitVersion]); + }, [reinitVersion, workspace?.tabids]);🤖 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 119 - 127, The effects are re-running due to unstable workspace?.tabids reference and the reinitVersion effect reads workspace?.tabids without it in deps; fix by creating a stable memoizedTabIds (e.g., via useMemo or a stable serialization like JSON.stringify) derived from workspace?.tabids and use that stable value (or tabIds if that is already stable) in both useEffect dependency arrays, and update both effects to call setOrderedTabIds(memoizedTabIds) (references: setOrderedTabIds, tabIds, workspace?.tabids, reinitVersion, the two useEffect hooks).
🤖 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/tab/vtabbar.tsx`:
- Around line 119-127: The effects are re-running due to unstable
workspace?.tabids reference and the reinitVersion effect reads workspace?.tabids
without it in deps; fix by creating a stable memoizedTabIds (e.g., via useMemo
or a stable serialization like JSON.stringify) derived from workspace?.tabids
and use that stable value (or tabIds if that is already stable) in both
useEffect dependency arrays, and update both effects to call
setOrderedTabIds(memoizedTabIds) (references: setOrderedTabIds, tabIds,
workspace?.tabids, reinitVersion, the two useEffect hooks).
In `@frontend/preview/mock/tabbar-mock.tsx`:
- Around line 167-172: The useMemo in TabBarMockEnvProvider currently captures
baseEnv but uses an empty dependency array, so update the memoization to include
baseEnv (and keep envRef and PlatformMacOS if desired) so the result of
makeTabBarMockEnv(baseEnv, envRef, PlatformMacOS) is recalculated when baseEnv
changes; modify the dependency array passed to useMemo accordingly to reference
baseEnv (and any other stable refs you want to observe).
- Around line 116-134: createTab currently returns void while closeTab returns
Promise<boolean>; make createTab's return type explicit and consistent by
returning the created tab id (string) — or if you prefer async parity with
closeTab, return Promise<string>. Update the createTab implementation in the
TabBarMock (function createTab) to compute newTabId from crypto.randomUUID(),
perform the same envRef.current/mockSetWaveObj/globalStore.set operations
(references: envRef.current, mockSetWaveObj, globalStore.get, globalStore.set,
TabBarMockWorkspaceId) and then return newTabId (or wrap in a resolved Promise
if choosing Promise<string>), and update any call sites that expect the new
value or a promise accordingly.
In `@frontend/preview/previews/tabbar.preview.tsx`:
- Around line 38-52: The useEffect inside TabBarPreviewInner calls
loadBadges(loadBadgesEnv) but has an empty dependency array, which triggers
exhaustive-deps linting; update the effect to include the dependency (e.g., add
loadBadgesEnv or the loadBadges function reference) in the dependency array so
React knows when to re-run the effect — locate the useEffect in
TabBarPreviewInner and add loadBadgesEnv (or loadBadges if that is
imported/defined) to the dependency list to satisfy lint rules.
In `@frontend/preview/previews/vtabbar.preview.tsx`:
- Around line 26-33: The useEffect in VTabBarPreviewInner uses
loadBadges(loadBadgesEnv) but has an empty dependency array; update the effect
to include loadBadgesEnv (and loadBadges if it's not stable) in the dependency
array so React/ESLint won't warn and the effect runs when the env changes;
locate the useEffect inside function VTabBarPreviewInner and either add
loadBadgesEnv to the dependencies or memoize loadBadges before use to ensure
stable references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 93ca1d8b-2d2c-46db-bb5c-0462ff0a0fa4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
frontend/app/tab/vtabbar.tsxfrontend/app/tab/vtabbarenv.tsfrontend/preview/mock/tabbar-mock.tsxfrontend/preview/previews/tabbar.preview.tsxfrontend/preview/previews/vtabbar.preview.tsx
1 participant
No description provided.