Repository navigation
Diff viewer: responsive toolbar that never overlaps at small widths - #6550
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a toolbar overflow system to the diff viewer: a new ChangesToolbar Overflow and Popover Positioning
Sequence Diagram(s)sequenceDiagram
participant User
participant Toolbar
participant useToolbarWidth
participant resolveToolbarOverflow
participant OptionsMenu
User->>Toolbar: window resize
Toolbar->>useToolbarWidth: observe toolbar element
useToolbarWidth-->>Toolbar: measured pixel width
Toolbar->>resolveToolbarOverflow: {available, reserved, items}
resolveToolbarOverflow-->>Toolbar: {visible, overflow} id lists
Toolbar-->>Toolbar: render conditionally visible controls
Toolbar->>OptionsMenu: pass externalURL, onSetLayout
OptionsMenu-->>OptionsMenu: render secondary items for overflowed controls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (21 passed)
✨ 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 makes the diff-viewer toolbar responsive using a priority+ overflow pattern, so toolbar controls never overlap at narrow panel widths. A
Confidence Score: 5/5Safe to merge — all toolbar layout changes are structural (CSS minmax + overflow-x:clip) with the JS overflow resolver as a graceful degradation layer on top, so neither component can cause overlap even if estimates are stale. The overflow resolver is a pure function with six explicit unit tests covering every edge case, the CSS clip is the hard structural guarantee, the portal change in BranchBasePicker correctly handles both the outside-click contract and the viewport anchor lifecycle, and the ResizeObserver hook tears down cleanly. No timing-based synchronization, no hacky sleeps, no data-loss paths. No files require special attention. All changed files are self-contained webview TypeScript/CSS — no Swift, no data persistence, no auth paths. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ResizeObserver fires on #toolbar] --> B[useToolbarWidth returns width px]
B --> C{width == null?}
C -- yes --> D[overflow = empty Set\nrender all controls]
C -- no --> E[resolveToolbarOverflow\navailable = width\nreserved = always-present + repo-select-min\nitems = files-toggle · layout-toggle · external-link]
E --> F{budget fits item?}
F -- yes, keep --> G[visible list]
F -- no, or already dropping --> H[overflow list]
G --> I[render icon button in toolbar]
H --> J[hide icon button from toolbar\nkeep entry in ⋯ OptionsMenu]
D --> I
K[Base button clicked] --> L[open = true\npopoverStyle = null\nportal not painted]
L --> M[useEffect fires\ncomputePopoverStyle from getBoundingClientRect]
M --> N{more space below or above?}
N -- below --> O[top = rect.bottom + gap\nbottom = auto\nmaxHeight = spaceBelow]
N -- above --> P[top = auto\nbottom = viewportH - rect.top + gap\nmaxHeight = spaceAbove]
O --> Q[createPortal to document.body\nposition: fixed]
P --> Q
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[ResizeObserver fires on #toolbar] --> B[useToolbarWidth returns width px]
B --> C{width == null?}
C -- yes --> D[overflow = empty Set\nrender all controls]
C -- no --> E[resolveToolbarOverflow\navailable = width\nreserved = always-present + repo-select-min\nitems = files-toggle · layout-toggle · external-link]
E --> F{budget fits item?}
F -- yes, keep --> G[visible list]
F -- no, or already dropping --> H[overflow list]
G --> I[render icon button in toolbar]
H --> J[hide icon button from toolbar\nkeep entry in ⋯ OptionsMenu]
D --> I
K[Base button clicked] --> L[open = true\npopoverStyle = null\nportal not painted]
L --> M[useEffect fires\ncomputePopoverStyle from getBoundingClientRect]
M --> N{more space below or above?}
N -- below --> O[top = rect.bottom + gap\nbottom = auto\nmaxHeight = spaceBelow]
N -- above --> P[top = auto\nbottom = viewportH - rect.top + gap\nmaxHeight = spaceAbove]
O --> Q[createPortal to document.body\nposition: fixed]
P --> Q
Reviews (4): Last reviewed commit: "Diff toolbar: portal the base picker pop..." | Re-trigger Greptile |
| @@ -386,9 +412,95 @@ extension CMUXCLI { | |||
| var executablePath: String? | |||
There was a problem hiding this comment.
File size sprawl in an already-oversized file
CLI/cmux_open.swift grows from 6,196 → 8,017 lines (+1,821) in this diff, and the file-length budget ceiling is raised to match. The entire new branch-picker subsystem (ref resolution heuristics, PR-base cache, refs group builder, stale-while-revalidate disk cache, session descriptor, and the two new CLI commands) is self-contained and independently testable — exactly the shape the SwiftPM-package-boundary rule targets. Appending it to an already 6k-line file makes the file harder to navigate and grows what is already the second-largest file in the budget. Consider splitting the branch-picker logic into a dedicated file (e.g. cmux_open+branch_picker.swift or a separate extension file) to keep responsibilities separated and bring the parent file's line count back toward a manageable level.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@Sources/Panels/BrowserPanel.swift`:
- Around line 2257-2265: The jsEscaped variable in BrowserPanel.swift currently
only escapes backslashes and double quotes before inserting the URL into
JavaScript code. Add escaping for newline characters (\n) and line separator
characters (\u2028 and \u2029) by adding additional replacingOccurrences calls
to the jsEscaped string preparation. These characters can break out of the
JavaScript string context if present in malformed CLI output, so escape newlines
to \\n, carriage returns to \\r, and the Unicode line/paragraph separators to
their escaped equivalents to prevent potential injection vulnerabilities.
In `@webviews/src/App.tsx`:
- Around line 576-579: The repo-select item in the overflowItems array can
become hidden when the toolbar narrows (when hasRepoSelect returns true and
width is set to 110 or when it's 0), but the overflow menu fallback rendering
does not provide an alternative way to access repo selection. Either remove
repo-select from the overflowItems array to keep it always visible, or add
proper handling in the overflow menu rendering logic (at the locations marked in
the comment: lines 668-677 and 932-940) to display and handle repo selection
when the selector is not visible in the main toolbar. Ensure users always have
at least one entrypoint to change repositories regardless of toolbar width.
- Around line 779-787: The `isValidBranchPickerPayload` type guard function is
incomplete and only validates some properties (refsURL, regenerateURLTemplate,
currentRef, headRef) while missing validation for required properties like
`repoRoot`, `currentReason`, `confidence`, and `aheadBehind`. This allows
partial or invalid payloads to pass validation and render a broken branch picker
instead of falling back to the legacy select. Extend the validation logic in the
`isValidBranchPickerPayload` function to include type checks and value
validation for all required properties of the BranchPickerPayload contract,
including `repoRoot`, `currentReason`, `confidence`, and `aheadBehind`, ensuring
that only complete and valid payloads are accepted.
In `@webviews/src/BranchBasePicker.tsx`:
- Around line 476-504: The filtering logic in the loop that iterates through all
groups and their rows (starting at line 478) rescans the entire dataset on every
query update, creating O(totalRows) complexity for cases with no or sparse
matches. Refactor this filtering path to use a precomputed index or single-pass
plan approach instead of calling fuzzyMatchSpan on every row during each
keystroke. Replace the current brute-force row-by-row scanning pattern with a
bounded-work algorithm that avoids repeatedly traversing the full groups and
rows collection for every query change.
In `@webviews/src/styles.css`:
- Around line 141-157: The `.toolbar-left` class clips horizontal overflow which
prevents the `.base-picker-popover` (a 320px absolute child of `#base-picker`)
from displaying properly when the left track is narrower than the popover width.
To fix this, either change `.base-picker-popover` to be portal or
fixed-positioned to remove it from the clipping context, or modify the
overflow-x clipping on `.toolbar-left` to allow it to escape (consider using CSS
`:has()` selector or a modifier class to relax clipping conditionally). Apply
the same fix to the other occurrence mentioned at lines 423-428.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78737c36-2dd0-49c4-be70-ad0381763467
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (17)
CLI/CMUXCLI+Process.swiftCLI/cmux.swiftCLI/cmux_open.swiftResources/Localizable.xcstringsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/Panels/BrowserPanel.swiftwebviews/src/App.tsxwebviews/src/BranchBasePicker.tsxwebviews/src/actions.tswebviews/src/icons.tsxwebviews/src/labels.tswebviews/src/styles.csswebviews/src/toolbar-overflow.tswebviews/src/useToolbarWidth.tswebviews/test/actions.test.tswebviews/test/branch-base-picker.test.tsxwebviews/test/toolbar-overflow.test.ts
| let metaEscaped = Self.htmlAttributeEscaped(viewerURLString) | ||
| let jsEscaped = viewerURLString | ||
| .replacingOccurrences(of: "\\", with: "\\\\") | ||
| .replacingOccurrences(of: "\"", with: "\\\"") | ||
| let html = """ | ||
| <!doctype html><html><head><meta charset="utf-8">\ | ||
| <meta http-equiv="refresh" content="0;url=\(metaEscaped)"></head>\ | ||
| <body><script>window.location.replace("\(jsEscaped)");</script></body></html> | ||
| """ |
There was a problem hiding this comment.
Harden JS string escaping for defense in depth.
The current escaping only handles \ and ". While the URL is validated to have the custom scheme and comes from the trusted bundled CLI, adding newline/line-separator escaping provides defense in depth against malformed CLI output.
🛡️ Suggested fix
- let jsEscaped = viewerURLString
- .replacingOccurrences(of: "\\", with: "\\\\")
- .replacingOccurrences(of: "\"", with: "\\\"")
+ let jsEscaped = viewerURLString
+ .replacingOccurrences(of: "\\", with: "\\\\")
+ .replacingOccurrences(of: "\"", with: "\\\"")
+ .replacingOccurrences(of: "\n", with: "\\n")
+ .replacingOccurrences(of: "\r", with: "\\r")
+ .replacingOccurrences(of: "\u{2028}", with: "\\u2028")
+ .replacingOccurrences(of: "\u{2029}", with: "\\u2029")🤖 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 `@Sources/Panels/BrowserPanel.swift` around lines 2257 - 2265, The jsEscaped
variable in BrowserPanel.swift currently only escapes backslashes and double
quotes before inserting the URL into JavaScript code. Add escaping for newline
characters (\n) and line separator characters (\u2028 and \u2029) by adding
additional replacingOccurrences calls to the jsEscaped string preparation. These
characters can break out of the JavaScript string context if present in
malformed CLI output, so escape newlines to \\n, carriage returns to \\r, and
the Unicode line/paragraph separators to their escaped equivalents to prevent
potential injection vulnerabilities.
| function isValidBranchPickerPayload(value: any): value is BranchPickerPayload { | ||
| return Boolean( | ||
| value && | ||
| typeof value === "object" && | ||
| typeof value.refsURL === "string" && value.refsURL !== "" && | ||
| typeof value.regenerateURLTemplate === "string" && value.regenerateURLTemplate !== "" && | ||
| typeof value.currentRef === "string" && | ||
| typeof value.headRef === "string", | ||
| ); |
There was a problem hiding this comment.
Validate the full branch-picker contract before opting in.
This accepts partial branchPicker objects missing repoRoot, currentReason, confidence, or valid aheadBehind, and String.replace("{ref}", …) does not throw when the placeholder is absent—it silently keeps the original URL. Invalid payloads should fall back to the legacy select instead of rendering a broken picker.
Proposed validation tightening
+function isValidAheadBehind(value: unknown): value is BranchPickerPayload["aheadBehind"] {
+ if (value === null) {
+ return true;
+ }
+ if (!value || typeof value !== "object") {
+ return false;
+ }
+ const candidate = value as { ahead?: unknown; behind?: unknown };
+ return Number.isFinite(candidate.ahead) && Number.isFinite(candidate.behind);
+}
+
function isValidBranchPickerPayload(value: any): value is BranchPickerPayload {
+ const aheadBehind = value?.aheadBehind;
return Boolean(
value &&
typeof value === "object" &&
+ typeof value.repoRoot === "string" &&
typeof value.refsURL === "string" && value.refsURL !== "" &&
- typeof value.regenerateURLTemplate === "string" && value.regenerateURLTemplate !== "" &&
+ typeof value.regenerateURLTemplate === "string" &&
+ value.regenerateURLTemplate.includes("{ref}") &&
typeof value.currentRef === "string" &&
- typeof value.headRef === "string",
+ typeof value.headRef === "string" &&
+ typeof value.currentReason === "string" &&
+ (value.confidence === "high" || value.confidence === "low") &&
+ isValidAheadBehind(aheadBehind),
);
}🤖 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 `@webviews/src/App.tsx` around lines 779 - 787, The
`isValidBranchPickerPayload` type guard function is incomplete and only
validates some properties (refsURL, regenerateURLTemplate, currentRef, headRef)
while missing validation for required properties like `repoRoot`,
`currentReason`, `confidence`, and `aheadBehind`. This allows partial or invalid
payloads to pass validation and render a broken branch picker instead of falling
back to the legacy select. Extend the validation logic in the
`isValidBranchPickerPayload` function to include type checks and value
validation for all required properties of the BranchPickerPayload contract,
including `repoRoot`, `currentReason`, `confidence`, and `aheadBehind`, ensuring
that only complete and valid payloads are accepted.
| // Filtering: match across ALL rows of every group, then cap the total | ||
| // rendered set so a query that matches thousands of rows stays cheap. | ||
| for (const group of groups) { | ||
| if (result.length >= FILTERED_TOTAL_CAP) { | ||
| break; | ||
| } | ||
| const groupLabel = resolveGroupLabel(group, label); | ||
| let firstInGroup = true; | ||
| for (const row of group.rows) { | ||
| if (result.length >= FILTERED_TOTAL_CAP) { | ||
| break; | ||
| } | ||
| const match = fuzzyMatchSpan(row.label, trimmed); | ||
| if (match == null) { | ||
| continue; | ||
| } | ||
| anyMatch = true; | ||
| result.push({ | ||
| row, | ||
| groupId: group.id, | ||
| groupLabel, | ||
| firstInGroup, | ||
| match, | ||
| raw: false, | ||
| moreCount: 0, | ||
| }); | ||
| firstInGroup = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid rescanning the full refs corpus on every query update.
The filtered path performs row-by-row fuzzy matching across all groups per keystroke (Lines 478-504). For large ref sets this makes no-match/sparse-match input O(totalRows) each render on the interactive path.
Suggested direction
- const flat = buildFlatRows(groups, query, label);
+ const indexed = useMemo(() => buildPickerIndex(groups, label), [groups, label]);
+ const flat = buildFlatRowsFromIndex(indexed, query);
+ // build once when refs load:
+ // - pre-lowercase labels
+ // - optional prefix bucket map (first char -> candidate rows)
+ // - preserve group metadata for headersAs per coding guidelines, this picker path should avoid repeated full scans over scalable collections and prefer precomputed index/single-pass plans with bounded work per event.
🤖 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 `@webviews/src/BranchBasePicker.tsx` around lines 476 - 504, The filtering
logic in the loop that iterates through all groups and their rows (starting at
line 478) rescans the entire dataset on every query update, creating
O(totalRows) complexity for cases with no or sparse matches. Refactor this
filtering path to use a precomputed index or single-pass plan approach instead
of calling fuzzyMatchSpan on every row during each keystroke. Replace the
current brute-force row-by-row scanning pattern with a bounded-work algorithm
that avoids repeatedly traversing the full groups and rows collection for every
query change.
Source: Coding guidelines
…at small widths The toolbar was a rigid grid (minmax(0,1.1fr) minmax(124px,0.9fr) auto) with a non-shrinking toolbar-actions, so at small widths the left controls overflowed their cell and overlaid the accessory icons. - No-overlap guarantee: all toolbar grid tracks are minmax(0,...) and the cells use overflow-x:clip (overflow-y:visible so the base/options popovers still escape below the bar); toolbar-actions can shrink. - Measured priority overflow: a ResizeObserver on #toolbar drives a pure resolver (toolbar-overflow.ts) that keeps the highest-priority controls and drops the rest as a clean priority suffix into the always-present options menu. Drop order (lowest first): external link -> layout -> files -> repo select. The base picker (primary) and the ⋯ button are always visible. The options menu always lists layout + external so dropped actions stay reachable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…at small widths The @pierre/diffs per-file header is a flex space-between of the file-path side ([data-header-content], min-width:0 + truncating) and the diffstat side ([data-metadata]: status icon + +N/-N counts). The library leaves [data-metadata] flex-shrinkable with white-space:nowrap and no overflow handling, so when the panel is narrow it gets squished below its content width and its text spills left, overlapping the change-status icon. Pin the diffstat (flex-shrink:0) so the path side absorbs all shrinking and truncates, and clip the header as a no-overlap safety net. Same priority+ idea as the toolbar fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9c141ab to
bbf187b
Compare
…reachable Two regressions from the responsive-toolbar change, caught by review: - The base picker popover (320px absolute child of #base-picker in .toolbar-left) was clipped by the new overflow-x:clip safety net. Make it a viewport-anchored position:fixed floating element (JS-anchored to the button rect, clamped to the viewport, flips above when short on space below), so it escapes the toolbar clip while the clip stays as the controls' no-overlap guarantee. - The repo <select> could overflow into the options menu, but a native select can't live there, so multi-repo users lost the switcher. Stop overflowing it: it's always rendered and truncates in place. Only the accessory icon controls overflow into the menu. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… container clip) position:fixed alone did not free the popover: .toolbar-left has container-type:inline-size (for the picker's @container queries), which makes it the containing block for fixed descendants AND still clips them. Render the popover via createPortal(document.body) so it leaves the container/clip subtree entirely; the existing fixed + viewport-anchored positioning then resolves against the viewport. Outside-click now checks both the container and a new popoverRef so clicks inside the portaled popover don't dismiss it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
At very small diff-panel widths, the toolbar controls overlapped/overlaid the accessory icon buttons and looked broken (reported with a screenshot). This makes the toolbar degrade gracefully via the priority+ overflow pattern: low-priority controls collapse into the existing
⋯options menu as width shrinks, and nothing ever overlaps.Root cause
#toolbarwas a rigid gridminmax(0,1.1fr) minmax(124px,0.9fr) autowith a non-shrinkingtoolbar-actions. At small widths the middle 124px floor + the fixed actions column starved the left cell, whose contents overflowed (grid cells don't clip) and overlaid the accessory icons.How
minmax(0,…)and the cells useoverflow-x: clip(withoverflow-y: visibleso the base picker / options popovers still drop below the bar);toolbar-actionscan shrink. Overlap is now structurally impossible regardless of measurement.ResizeObserveron#toolbardrives a pure resolver (toolbar-overflow.ts, unit-tested) that keeps the highest-priority controls and drops the rest as a clean priority suffix into the always-present⋯menu. Drop order (lowest first): external link → layout toggle → files toggle → repo select. The Base picker (primary, with its own internal shedding) and the⋯button are always visible. The options menu always lists the layout + external actions so anything dropped from the bar stays reachable.At the reported extreme width the bar cleanly shows roughly
[source] [Base… ▾] [⋯], accessories in the menu, no overlap.Verification
⋯, no overlap (vs the prior overlapping layout).Note
This branch is stacked on #6484 (the branch picker), so the diff currently includes those commits. Once #6484 merges, I'll rebase this onto
mainso it shows only the toolbar change.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Make the diff viewer toolbar responsive with priority+ overflow so controls never overlap at small widths. Only accessory icons collapse into
⋯; the repo selector and Base picker stay usable at any width.New Features
⋯as width shrinks (drop order: external → layout → files). The repo select is always shown and truncates; Base and⋯are always visible. Measured withResizeObserverand a pureresolveToolbarOverflowresolver; overflowed actions remain in the options menu.cmux-diff-viewer://pages.Bug Fixes
minmax(0, …)and cellsoverflow-x: clip(popovers still escape). Per‑file diff headers pin the diffstat and clip the header so counts never overlap the status icon.position: fixed), portaled todocument.body, clamped to the viewport, and flips above when space is tight. Outside‑click handling respects the portaled popover.Written for commit 2c18e75. Summary will update on new commits.
Summary by CodeRabbit
Release Notes