[codex] Update sidebar main workspace icons#3800
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPasses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Greptile SummaryThis PR propagates a new Confidence Score: 5/5Safe to merge — all findings are P2 style/edge-case observations The implementation is correct for the described scope. The only finding (offline state hidden behind the laptop icon for a remote-device main workspace) is a P2 edge case that may not even be a reachable state in the application. DashboardSidebarWorkspaceIcon.tsx — ordering of workspaceType guard vs. isRemoteDeviceOffline branch
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx | Adds workspaceType prop; inserts CgLaptop icon for main workspaces before host-type checks, which skips the offline indicator path for remote-device main workspaces |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx | Passes workspace.type to both icon instances; swaps close button icon to HiMiniMinus for main workspaces and keeps HiMiniXMark for worktrees — looks correct |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarCollapsedWorkspaceButton/DashboardSidebarCollapsedWorkspaceButton.tsx | Threads new workspaceType prop through to DashboardSidebarWorkspaceIcon — straightforward and correct |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx | Passes workspace.type to DashboardSidebarCollapsedWorkspaceButton — minimal, correct change |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DashboardSidebarWorkspaceItem] -->|workspaceType| B[DashboardSidebarCollapsedWorkspaceButton]
A --> C[DashboardSidebarExpandedWorkspaceRow]
B -->|workspaceType| D[DashboardSidebarWorkspaceIcon]
C -->|workspace.type| D
D --> E{pullRequestState?}
E -->|yes| F[PR icon]
E -->|no| G{workspaceType === 'main'?}
G -->|yes| H[CgLaptop icon]
G -->|no| I{hostType?}
I -->|local-device| J[RxDot]
I -->|remote-device offline| K[TbCloudOff]
I -->|cloud| L[TbCloud]
C --> M{isMainWorkspace?}
M -->|yes| N[HiMiniMinus — Remove from sidebar]
M -->|no| O[HiMiniXMark — Close workspace]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx
Line: 77-79
Comment:
**Offline state not reflected for main workspaces on remote devices**
The `workspaceType === "main"` guard short-circuits before the `isRemoteDeviceOffline` branch, so a main workspace whose host is a remote device that is currently offline will display the `CgLaptop` icon with no visual degradation (no `TbCloudOff` / opacity-60 treatment). If main workspaces can only be on local devices in practice this is harmless, but if the type system allows remote-device main workspaces the offline state becomes invisible to the user.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Update sidebar main workspace icons" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+77
to
+79
| if (workspaceType === "main") { | ||
| return <CgLaptop className={cn("size-4 transition-colors", iconColor)} />; | ||
| } |
There was a problem hiding this comment.
The workspaceType === "main" guard short-circuits before the isRemoteDeviceOffline branch, so a main workspace whose host is a remote device that is currently offline will display the CgLaptop icon with no visual degradation (no TbCloudOff / opacity-60 treatment). If main workspaces can only be on local devices in practice this is harmless, but if the type system allows remote-device main workspaces the offline state becomes invisible to the user.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx
Line: 77-79
Comment:
**Offline state not reflected for main workspaces on remote devices**
The `workspaceType === "main"` guard short-circuits before the `isRemoteDeviceOffline` branch, so a main workspace whose host is a remote device that is currently offline will display the `CgLaptop` icon with no visual degradation (no `TbCloudOff` / opacity-60 treatment). If main workspaces can only be on local devices in practice this is harmless, but if the type system allows remote-device main workspaces the offline state becomes invisible to the user.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 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
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx`:
- Around line 287-290: In DashboardSidebarExpandedWorkspaceRow, prevent keyboard
events from reaching the parent row onKeyDown by stopping propagation for
Enter/Space on the button handlers (the same places you already call
event.stopPropagation() in the onClick block for the remove and other button
elements); update the button event handlers (e.g., the onKeyDown for the remove
button and the other button at the same area) to check for event.key === 'Enter'
|| event.key === ' ' (or 'Spacebar' for broad support) and call
event.preventDefault() and event.stopPropagation() before invoking
onRemoveFromSidebarClick (or the corresponding action), so keyboard activation
won't bubble to the parent DashboardSidebarExpandedWorkspaceRow onKeyDown
handler.
🪄 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
Run ID: a25a4da7-992e-463c-a7ed-04e2889550ad
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
9 tasks
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.
Summary
CgLaptopfor local main workspace sidebar icons.Validation
bun run --cwd apps/desktop typecheckbunx @biomejs/biome@2.4.2 checkon the changed sidebar filesSummary by CodeRabbit