Repository navigation
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThe PR replaces the boolean workspace-port setting with inline and topbar modes, centralizes port data at the dashboard layout, removes the old sidebar ports list, and adds conditional top-bar ports controls with grouped port management. ChangesWorkspace ports state and provisioning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DashboardLayout
participant TopBar
participant TopBarPortsDropdown
participant useDashboardSidebarPortsData
participant useDashboardSidebarPortKill
DashboardLayout->>TopBar: provide ports context
TopBar->>TopBarPortsDropdown: render mode-gated control
TopBarPortsDropdown->>useDashboardSidebarPortsData: read grouped ports and count
TopBarPortsDropdown->>useDashboardSidebarPortKill: close selected or all ports
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
Ready to review this PR? Stage has broken it down into 8 individual chapters for you: 
Chapters generated by Stage for commit 4c9e0d7 on Jul 29, 2026 11:06pm UTC. |
![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: 2
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/inline-workspace-ports.ts (1)
57-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMigrate function: inconsistent typing between branches.
The
version === 0branch returns a bare{ mode: ... }object while the fallback branch explicitly castsas InlineWorkspacePortsState. Given zustand infers the persisted-state type frommigrate's return type (andmerge'spersistedStateparam is untypedunknown), this likely compiles, but the asymmetry is confusing and fragile if a stricterPersistedStategeneric is added later.♻️ Suggested consistency fix
migrate: (persisted, version) => { if (version === 0 && persisted && typeof persisted === "object") { const { enabled } = persisted as { enabled?: boolean }; - return { mode: enabled === false ? "panel" : "inline" }; + return { + mode: enabled === false ? "panel" : "inline", + } as InlineWorkspacePortsState; } return persisted as InlineWorkspacePortsState; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/stores/inline-workspace-ports.ts` around lines 57 - 63, Make the return typing in the migrate function consistent across both branches: explicitly treat the version-0 `{ mode: ... }` result as InlineWorkspacePortsState, matching the fallback cast. Keep the existing migration behavior and mode selection unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortRow/TopBarPortRow.tsx`:
- Around line 39-42: Update handleCopy so it awaits
navigator.clipboard.writeText(portUrl) and displays the success toast only after
the write resolves; catch rejected writes to prevent unhandled promise
rejections and avoid showing a false success notification.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/TopBarPortsDropdown.tsx`:
- Around line 26-213: Split TopBarPortsGroup and TopBarPortsFooter out of
TopBarPortsDropdown.tsx into separate component folders, each containing its
component file and index.ts, matching the existing TopBarPortRow structure. Move
each component’s dependencies with it and update imports/exports accordingly;
optionally extract TopBarPortsDropdownInner using the same pattern.
---
Nitpick comments:
In `@apps/desktop/src/renderer/stores/inline-workspace-ports.ts`:
- Around line 57-63: Make the return typing in the migrate function consistent
across both branches: explicitly treat the version-0 `{ mode: ... }` result as
InlineWorkspacePortsState, matching the fallback cast. Keep the existing
migration behavior and mode selection unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 29e19286-d6fa-4814-ac7d-d96865aa7a4d
📒 Files selected for processing (10)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/TopBarPortsDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortRow/TopBarPortRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortRow/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/stores/inline-workspace-ports.ts
Comment on lines
+39
to
+42
| const handleCopy = () => { | ||
| void navigator.clipboard.writeText(portUrl); | ||
| toast.success(`Copied ${portUrl.replace(/^https?:\/\//, "")}`); | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Copy toast fires before/regardless of clipboard write success.
toast.success runs unconditionally right after the fire-and-forget writeText call; if the write rejects, the user sees a false "Copied" confirmation and the rejection goes unhandled.
🐛 Suggested fix
const handleCopy = () => {
- void navigator.clipboard.writeText(portUrl);
- toast.success(`Copied ${portUrl.replace(/^https?:\/\//, "")}`);
+ navigator.clipboard
+ .writeText(portUrl)
+ .then(() => {
+ toast.success(`Copied ${portUrl.replace(/^https?:\/\//, "")}`);
+ })
+ .catch(() => {
+ toast.error("Failed to copy to clipboard");
+ });
};📝 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
| const handleCopy = () => { | |
| void navigator.clipboard.writeText(portUrl); | |
| toast.success(`Copied ${portUrl.replace(/^https?:\/\//, "")}`); | |
| }; | |
| const handleCopy = () => { | |
| navigator.clipboard | |
| .writeText(portUrl) | |
| .then(() => { | |
| toast.success(`Copied ${portUrl.replace(/^https?:\/\//, "")}`); | |
| }) | |
| .catch(() => { | |
| toast.error("Failed to copy to clipboard"); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortRow/TopBarPortRow.tsx`
around lines 39 - 42, Update handleCopy so it awaits
navigator.clipboard.writeText(portUrl) and displays the success toast only after
the write resolves; catch rejected writes to prevent unhandled promise
rejections and avoid showing a false success notification.
Collaborator
Author
There was a problem hiding this comment.
Skipping: in an Electron renderer a clipboard write from a click handler (focused document, granted permission) effectively can't reject, so the false-success window is unreachable in practice; keeping the handler synchronous.
Contributor
There was a problem hiding this comment.
@AviPeltz, the Comment images show the Electron app’s top-bar live-port indicator: a signal-style icon with “2” and tooltip “2 ports across 1 workspace.” The expanded dropdown is for workspace jira-sync-card, listing :5174 and :5175 as node, with “2 live ports” and “Close all.” The terminal behind it shows repeated Vite startup output, port collisions on 5173/5174, error: script "dev" exited with code 143, and missing Bun scripts.
Acknowledged—keeping the synchronous clipboard handler as-is per the Electron renderer assumptions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsx (1)
30-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Close all ports" gives no feedback when every kill fails. Both
handleCloseAllimplementations onlytoast.successwhenclosedCount > 0; ifkillPortsreturns all failures, the button just stops spinning with no indication anything went wrong.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsx#L30-L39: add anelse/failure branch (e.g.toast.error("Failed to close ports")) whenclosedCount === 0 && results.length > 0.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsFooter/TopBarPortsFooter.tsx#L19-L28: apply the same failure-toast branch here.🐛 Proposed fix (apply to both files)
const results = await killPorts(/* ...ports... */); const closedCount = results.filter((result) => result.success).length; if (closedCount > 0) { toast.success( closedCount === 1 ? "Closed 1 port" : `Closed ${closedCount} ports`, ); + } else if (results.length > 0) { + toast.error("Failed to close ports"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsx` around lines 30 - 39, Update handleCloseAll in TopBarPortsGroup.tsx and the corresponding close-all handler in TopBarPortsFooter.tsx to show a failure toast when results.length > 0 and closedCount === 0, while preserving the existing success toasts for partial or complete success.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts (1)
62-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard the live-query sync effect against transient unmatched rows.
After a workspace switch,
localWorkspaceRowscan temporarily contain the previous workspace’s row while the live query for the newworkspaceIdis still loading, solocalWorkspaceState?.workspaceId === workspaceIdis false andpersistedPaneLayoutbecomesEMPTY_STATE. Since the store now starts with the real layout andlastSyncedSnapshotis seeded to it, this fallback wipes the seeded state before the row for the current workspace arrives. Don’t sync unless the live query is ready and the returned row matches the current workspace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts around lines 62 - 89, Update the persistedPaneLayout synchronization effect to return early while the live query is not ready or localWorkspaceState does not match workspaceId. Only compare snapshots and call store.getState().replaceState after the current workspace row is available, preserving the seeded state during transient unmatched results.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.ts`:
- Around line 67-155: The useEffect managing port:changed subscriptions
currently rebuilds every host’s listener whenever hostsToQuery changes. Refactor
the lifecycle around the per-host target identity—using host.hostUrl or
equivalent diffing—so only added, removed, or changed hosts tear down and
recreate their listener, timer, and retained event bus; preserve existing cache
updates and cleanup behavior for unaffected hosts.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts:
- Around line 33-46: The workspaceRuntime useMemo should depend only on
workspaceId, not collections, to avoid recreating the pane store and
seededSnapshot when collection references change. Keep seeding from
collections.v2WorkspaceLocalState inside the memo while removing collections
from its dependency array, preserving existing in-flight workspace state across
organization or collection reloads.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsx`:
- Around line 30-39: Update handleCloseAll in TopBarPortsGroup.tsx and the
corresponding close-all handler in TopBarPortsFooter.tsx to show a failure toast
when results.length > 0 and closedCount === 0, while preserving the existing
success toasts for partial or complete success.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts:
- Around line 62-89: Update the persistedPaneLayout synchronization effect to
return early while the live query is not ready or localWorkspaceState does not
match workspaceId. Only compare snapshots and call store.getState().replaceState
after the current workspace row is available, preserving the seeded state during
transient unmatched results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95b1bccd-daaa-4dd1-9b64-5afe5ff446b3
📒 Files selected for processing (31)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/DashboardSidebarPortsList.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortBadge/DashboardSidebarPortBadge.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortBadge/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/DashboardSidebarPortGroup.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/components/DashboardSidebarWorkspaceChips/components/DashboardSidebarPortsChip/DashboardSidebarPortsChip.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/components/DashboardSidebarWorkspaceChips/components/DashboardSidebarPortsChip/components/DashboardSidebarPortHoverRow/DashboardSidebarPortHoverRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortKill/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortKill/useDashboardSidebarPortKill.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/usePortOpenActions/usePortOpenActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarPortsProvider/DashboardSidebarPortsProvider.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/TopBarPortsDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortRow/TopBarPortRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsFooter/TopBarPortsFooter.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/stores/inline-workspace-ports.tsapps/desktop/src/renderer/stores/workspace-sidebar-state.ts
💤 Files with no reviewable changes (6)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortBadge/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/DashboardSidebarPortGroup.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/DashboardSidebarPortsList.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortBadge/DashboardSidebarPortBadge.tsx
Comment on lines
+33
to
46
| const workspaceRuntime = useMemo(() => { | ||
| const seededLayout = | ||
| (collections.v2WorkspaceLocalState.get(workspaceId)?.paneLayout as | ||
| | WorkspaceState<PaneViewerData> | ||
| | undefined) ?? EMPTY_STATE; | ||
| return { | ||
| workspaceId, | ||
| seededSnapshot: getSnapshot(seededLayout), | ||
| store: createWorkspaceStore<PaneViewerData>({ | ||
| initialState: EMPTY_STATE, | ||
| initialState: seededLayout, | ||
| }), | ||
| }), | ||
| [workspaceId], | ||
| ); | ||
| }; | ||
| }, [collections, workspaceId]); | ||
| const { store } = workspaceRuntime; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'CollectionsProvider' apps/desktop/src/renderer/routes/_authenticated/providers | xargs -I{} sed -n '1,150p' {}Repository: superset-sh/superset
Length of output: 3079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the hook and nearby state usage to determine whether adding collections
# to the dependency array can recreate workspaceRuntime/store unnecessarily.
file='apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts'
sed -n '1,140p' "$file"
printf '\n-- getCollections definition/usages --\n'
rg -n "function getCollections|const getCollections|getCollections|\nexport function getCollections" apps/desktop/src/renderer/routes/_authenticated/providers/shared apps/desktop/src/renderer/routes/_authenticated/providers -S
fd -t f 'collections' apps/desktop/src/renderer/routes/_authenticated/providers | while read -r f; do
echo "--- $f ---"
sed -n '1,220p' "$f"
doneRepository: superset-sh/superset
Length of output: 4316
Don’t add collections to the workspace useMemo dependencies.
CollectionsProvider memoizes collections by activeOrganizationId, so adding collections here makes the pane store depend on the current org’s collection reference as well as the workspace. This can recreate createWorkspaceStore and its seededSnapshot on org/collection reloads, losing in-flight pane/store state; seed state from the collection row without adding collections to the dependency array.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts
around lines 33 - 46, The workspaceRuntime useMemo should depend only on
workspaceId, not collections, to avoid recreating the pane store and
seededSnapshot when collection references change. Keep seeding from
collections.v2WorkspaceLocalState inside the memo while removing collections
from its dependency array, preserving existing in-flight workspace state across
organization or collection reloads.
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsx (1)
30-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Close all ports" gives no feedback when every kill fails. Both
handleCloseAllimplementations onlytoast.successwhenclosedCount > 0; ifkillPortsreturns all failures, the button just stops spinning with no indication anything went wrong.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsx#L30-L39: add anelse/failure branch (e.g.toast.error("Failed to close ports")) whenclosedCount === 0 && results.length > 0.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsFooter/TopBarPortsFooter.tsx#L19-L28: apply the same failure-toast branch here.🐛 Proposed fix (apply to both files)
const results = await killPorts(/* ...ports... */); const closedCount = results.filter((result) => result.success).length; if (closedCount > 0) { toast.success( closedCount === 1 ? "Closed 1 port" : `Closed ${closedCount} ports`, ); + } else if (results.length > 0) { + toast.error("Failed to close ports"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsx` around lines 30 - 39, Update handleCloseAll in TopBarPortsGroup.tsx and the corresponding close-all handler in TopBarPortsFooter.tsx to show a failure toast when results.length > 0 and closedCount === 0, while preserving the existing success toasts for partial or complete success.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts (1)
62-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard the live-query sync effect against transient unmatched rows.
After a workspace switch,
localWorkspaceRowscan temporarily contain the previous workspace’s row while the live query for the newworkspaceIdis still loading, solocalWorkspaceState?.workspaceId === workspaceIdis false andpersistedPaneLayoutbecomesEMPTY_STATE. Since the store now starts with the real layout andlastSyncedSnapshotis seeded to it, this fallback wipes the seeded state before the row for the current workspace arrives. Don’t sync unless the live query is ready and the returned row matches the current workspace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts around lines 62 - 89, Update the persistedPaneLayout synchronization effect to return early while the live query is not ready or localWorkspaceState does not match workspaceId. Only compare snapshots and call store.getState().replaceState after the current workspace row is available, preserving the seeded state during transient unmatched results.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.ts`:
- Around line 67-155: The useEffect managing port:changed subscriptions
currently rebuilds every host’s listener whenever hostsToQuery changes. Refactor
the lifecycle around the per-host target identity—using host.hostUrl or
equivalent diffing—so only added, removed, or changed hosts tear down and
recreate their listener, timer, and retained event bus; preserve existing cache
updates and cleanup behavior for unaffected hosts.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts:
- Around line 33-46: The workspaceRuntime useMemo should depend only on
workspaceId, not collections, to avoid recreating the pane store and
seededSnapshot when collection references change. Keep seeding from
collections.v2WorkspaceLocalState inside the memo while removing collections
from its dependency array, preserving existing in-flight workspace state across
organization or collection reloads.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsx`:
- Around line 30-39: Update handleCloseAll in TopBarPortsGroup.tsx and the
corresponding close-all handler in TopBarPortsFooter.tsx to show a failure toast
when results.length > 0 and closedCount === 0, while preserving the existing
success toasts for partial or complete success.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts:
- Around line 62-89: Update the persistedPaneLayout synchronization effect to
return early while the live query is not ready or localWorkspaceState does not
match workspaceId. Only compare snapshots and call store.getState().replaceState
after the current workspace row is available, preserving the seeded state during
transient unmatched results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95b1bccd-daaa-4dd1-9b64-5afe5ff446b3
📒 Files selected for processing (31)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/DashboardSidebarPortsList.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortBadge/DashboardSidebarPortBadge.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortBadge/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/DashboardSidebarPortGroup.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/components/DashboardSidebarWorkspaceChips/components/DashboardSidebarPortsChip/DashboardSidebarPortsChip.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/components/DashboardSidebarWorkspaceChips/components/DashboardSidebarPortsChip/components/DashboardSidebarPortHoverRow/DashboardSidebarPortHoverRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortKill/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortKill/useDashboardSidebarPortKill.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/usePortOpenActions/usePortOpenActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/providers/DashboardSidebarPortsProvider/DashboardSidebarPortsProvider.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/TopBarPortsDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortRow/TopBarPortRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsFooter/TopBarPortsFooter.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/TopBarPortsDropdown/components/TopBarPortsGroup/TopBarPortsGroup.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/stores/inline-workspace-ports.tsapps/desktop/src/renderer/stores/workspace-sidebar-state.ts
💤 Files with no reviewable changes (6)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortBadge/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortGroup/DashboardSidebarPortGroup.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/DashboardSidebarPortsList.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/components/DashboardSidebarPortBadge/DashboardSidebarPortBadge.tsx
🛑 Comments failed to post (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.ts (1)
67-155: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Event-bus subscriptions and query cache are churned for every host whenever any single host's target changes.
The single
useEffect(lines 98-155) loops over the entirehostsToQueryarray and tears down + rebuilds every host's event-bus listener/timer whenever the array's identity changes. Combined withderiveHostPortQueryTargetsunconditionally merging all visible workspace ids (including remote-hosted ones) into the local host's target, any workspace becoming visible/hidden anywhere in the sidebar changes the local target'sworkspaceIds, which:
- Changes
getHostPortsQueryKey(host)(it embedsworkspaceIds), forcing a fresh network fetch under a new cache key instead of an incremental update.- Re-runs the effect, disconnecting/reconnecting the
port:changedevent-bus subscription for every host (local and remote), not just the one whose target actually changed.This can repeatedly reset real-time port tracking for unrelated hosts on ordinary workspace-list changes, working against the PR's goal of avoiding duplicate polling/churn.
Consider scoping the subscription lifecycle per host (e.g. keyed effects per
host.hostUrl, or diffing the previous vs. next target list so only the changed host's listener is torn down/rebuilt) instead of one effect that treats the whole list as a single unit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.ts` around lines 67 - 155, The useEffect managing port:changed subscriptions currently rebuilds every host’s listener whenever hostsToQuery changes. Refactor the lifecycle around the per-host target identity—using host.hostUrl or equivalent diffing—so only added, removed, or changed hosts tear down and recreate their listener, timer, and retained event bus; preserve existing cache updates and cleanup behavior for unaffected hosts.
…sidebar chip Extend the ports-layout experiment from a single inline chip to a two-way display mode: "inline" (a chip under each workspace item, unchanged default) or "topbar" (a compact pill with the live port count in the persistent sidebar-header chrome — or the TopBar/tab-bar trailing slot when that chrome isn't visible). Its dropdown lists every detected port grouped by workspace with per-row open/copy/close, a per-workspace close-all on each group header, and a global close-all footer. A single DashboardSidebarPortsProvider in the dashboard layout backs both modes so port data survives the workspace-navigation remounts that the TopBar/tab-bar mounts sit inside — the pane store is now also seeded synchronously from its persisted layout instead of starting empty, so the tab bar (and the pill/background-terminals button inside it) no longer blinks on every workspace switch. The old three-mode design (a "panel" consolidated at the sidebar bottom) was prototyped and dropped before shipping; this history skips straight to the two-mode result, so useDashboardSidebarPortsData and useDashboardSidebarPortKill live directly under DashboardSidebar/hooks/ alongside usePortOpenActions rather than modeling their interim home under the never-shipped panel component. Claude-Session: https://claude.ai/code/session_018hKiDErNgDrZCGMwQcRjdn
AviPeltz
force-pushed
the
review-orca-improvements
branch
from
August 3, 2026 01:55
4c9e0d7 to
45bf888
Compare
This was referenced Aug 10, 2026
# Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsx # apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx # apps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsx
3 of 4 tasks
This branch has not been deployed
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
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.
Summary
:portcolumn,label · processtext, and a hover-revealed action cluster (open in browser via the existing port-open preference, copy URL, close port). Group headers navigate to the workspace, show aremotetag for non-local hosts, and expose a hover ✕ that closes all ports for that workspace; the footer has a global close-all.DashboardSidebarPortsProvidergoes inert so port polling andport:changedsubscriptions aren't duplicated — the dropdown readsuseDashboardSidebarPortsDatadirectly and shares the same react-query keys.Screenshots
Test plan
bun run lintand desktoptypecheckpass (verified locally).https://claude.ai/code/session_018hKiDErNgDrZCGMwQcRjdn
Summary by cubic
Adds a top‑bar ports dropdown and reduces the workspace‑ports experiment to two modes: inline chips under each workspace (default) or a top‑bar dropdown; the previous bottom panel is removed. The pill renders once in the active chrome (sidebar header when expanded, otherwise in the TopBar or the v2 workspace tab bar) and keeps live counts/actions consistent across navigation.
modestring; persisted “panel” and the old boolean are migrated to “topbar”.Written for commit 26cb232. Summary will update on new commits.
Summary by CodeRabbit