tighten v2 diff file header layout#3776
Conversation
- size the file badge to its content instead of stretching full-width - move +/- LOC out of the badge and next to the right-side actions - drop redundant Open in editor button (badge ⌘ click already does it) - simplify file badge tooltip to just the click hint
📝 WalkthroughWalkthroughThe DiffFileHeader component UI is refactored with adjusted flex sizing, simplified tooltip messaging, removal of an external editor button, relocation of the additions/deletions counter to a conditional right-side badge, and condensed imports. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 tightens the diff file header layout: the file-path badge is shrunk to fit its content ( Confidence Score: 5/5Safe to merge; changes are purely cosmetic/layout with one minor UX edge case around the tooltip hint when the external editor is unavailable. All findings are P2 style/UX concerns. No logic errors, data-integrity issues, or security risks were identified. The layout changes are straightforward Tailwind class adjustments, and the removed button's functionality is preserved via the existing keyboard-modifier shortcut. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx | UI refactor: badge now sizes to content (min-w-0), LOC stats moved to right-side action bar, redundant "Open in editor" button removed; tooltip simplified to CLICK_HINT_TOOLTIP but still advertises ⌘-click-to-editor even when onOpenInExternalEditor is unavailable. |
Sequence Diagram
sequenceDiagram
participant User
participant Badge as File Badge (button)
participant getSidebarClickIntent
participant onOpenFile
participant onOpenInExternalEditor
User->>Badge: plain click
Badge->>getSidebarClickIntent: event
getSidebarClickIntent-->>Badge: "openInViewer"
Badge->>onOpenFile: onOpenFile?.(false)
User->>Badge: ⇧ click
Badge->>getSidebarClickIntent: event
getSidebarClickIntent-->>Badge: "openInNewTab"
Badge->>onOpenFile: onOpenFile?.(true)
User->>Badge: ⌘ click
Badge->>getSidebarClickIntent: event
getSidebarClickIntent-->>Badge: "openInEditor"
Badge->>onOpenInExternalEditor: onOpenInExternalEditor?.()
Note over onOpenInExternalEditor: No-op if prop is undefined,<br/>but tooltip still shows ⌘ click hint
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx
Line: 83-85
Comment:
**Tooltip hints ⌘-click even when editor unavailable**
`CLICK_HINT_TOOLTIP` is now always shown as-is, but it still advertises `⌘ click: editor`. When `onOpenInExternalEditor` is `undefined`, that modifier silently does nothing (`onOpenInExternalEditor?.()`) while the tooltip continues to promise it works. The old code addressed this by showing "Open in editor unavailable" on the dedicated button. Consider either suppressing the `⌘ click` line from the tooltip when `onOpenInExternalEditor` is falsy, or accepting that the hint will occasionally be a no-op.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "tighten v2 diff file header layout" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <TooltipContent side="bottom" showArrow={false}> | ||
| {CLICK_HINT_TOOLTIP} | ||
| </TooltipContent> |
There was a problem hiding this comment.
CLICK_HINT_TOOLTIP is now always shown as-is, but it still advertises ⌘ click: editor. When onOpenInExternalEditor is undefined, that modifier silently does nothing (onOpenInExternalEditor?.()) while the tooltip continues to promise it works. The old code addressed this by showing "Open in editor unavailable" on the dedicated button. Consider either suppressing the ⌘ click line from the tooltip when onOpenInExternalEditor is falsy, or accepting that the hint will occasionally be a no-op.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx
Line: 83-85
Comment:
**Tooltip hints ⌘-click even when editor unavailable**
`CLICK_HINT_TOOLTIP` is now always shown as-is, but it still advertises `⌘ click: editor`. When `onOpenInExternalEditor` is `undefined`, that modifier silently does nothing (`onOpenInExternalEditor?.()`) while the tooltip continues to promise it works. The old code addressed this by showing "Open in editor unavailable" on the dedicated button. Consider either suppressing the `⌘ click` line from the tooltip when `onOpenInExternalEditor` is falsy, or accepting that the hint will occasionally be a no-op.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx (1)
65-75:⚠️ Potential issue | 🟡 Minor⌘-click silently no-ops when
onOpenInExternalEditoris not provided.The button stays enabled as long as either callback is present (line 73), but the click handler returns early on
intent === "openInEditor"without falling back toonOpenFile. If a caller wires uponOpenFileonly (e.g. environments without an external editor configured), the tooltip still advertises⌘ click: editorand ⌘-click does nothing — no viewer fallback, no error — which is a confusing dead end now that the explicit "Open in editor" button is gone.Consider either falling back to the viewer or disabling the editor branch of the tooltip when the handler is missing.
🛡️ Proposed fallback
onClick={(event) => { const intent = getSidebarClickIntent(event); - if (intent === "openInEditor") { + if (intent === "openInEditor" && onOpenInExternalEditor) { onOpenInExternalEditor?.(); return; } onOpenFile?.(intent === "openInNewTab"); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx around lines 65 - 75, The click handler for the button (using getSidebarClickIntent) returns early when intent === "openInEditor", causing a silent no-op if onOpenInExternalEditor is not provided; update the handler in DiffFileHeader.tsx so that when intent === "openInEditor" it calls onOpenInExternalEditor if available otherwise falls back to onOpenFile (passing true when appropriate), and ensure the disabled prop/tooltip logic reflects whether the editor action is actually available (i.e., hide or disable the ⌘-click editor hint when onOpenInExternalEditor is undefined) so users get either a viewer fallback or correct tooltip state.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx (1)
47-87: Minor:ml-autoon the right-side group is redundant with parentjustify-between.Parent at line 47 already uses
justify-between, so the explicitml-autoat line 87 is belt-and-suspenders. Not harmful (it actually helps keep layout stable if a future change adds a third left-side item), so feel free to leave as-is — flagging only for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx around lines 47 - 87, The right-side container in the DiffFileHeader component uses a redundant ml-auto alongside the parent container's justify-between; open the DiffFileHeader.tsx file, locate the div with className "ml-auto flex shrink-0 items-center gap-1.5" inside the DiffFileHeader component, and remove the ml-auto token from that className (leaving "flex shrink-0 items-center gap-1.5") to rely on the parent's justify-between; alternatively, if you prefer to preserve defensive layout for future changes, add a short comment above the div explaining why ml-auto is intentionally kept.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx:
- Around line 65-75: The click handler for the button (using
getSidebarClickIntent) returns early when intent === "openInEditor", causing a
silent no-op if onOpenInExternalEditor is not provided; update the handler in
DiffFileHeader.tsx so that when intent === "openInEditor" it calls
onOpenInExternalEditor if available otherwise falls back to onOpenFile (passing
true when appropriate), and ensure the disabled prop/tooltip logic reflects
whether the editor action is actually available (i.e., hide or disable the
⌘-click editor hint when onOpenInExternalEditor is undefined) so users get
either a viewer fallback or correct tooltip state.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx:
- Around line 47-87: The right-side container in the DiffFileHeader component
uses a redundant ml-auto alongside the parent container's justify-between; open
the DiffFileHeader.tsx file, locate the div with className "ml-auto flex
shrink-0 items-center gap-1.5" inside the DiffFileHeader component, and remove
the ml-auto token from that className (leaving "flex shrink-0 items-center
gap-1.5") to rely on the parent's justify-between; alternatively, if you prefer
to preserve defensive layout for future changes, add a short comment above the
div explaining why ml-auto is intentionally kept.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63402a1c-22e9-4c54-b22d-c6bcc04937a9
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/DiffFileHeader/DiffFileHeader.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
Test plan
⇧ click: new tab · ⌘ click: editorSummary by CodeRabbit
Summary by cubic
Tightened the v2 diff file header to reduce clutter and make interactions clearer. The file badge now fits its content, the tooltip is concise, and LOC counts sit with the right-side controls.
Written for commit dea7480. Summary will update on new commits.