Repository navigation
Conversation
|
@Milofax is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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)
📝 WalkthroughWalkthroughAdjusted layout calculations in GhosttyTerminalView to subtract the overlay scrollbar inset when computing target surface and document widths; synchronizeCoreSurface now derives the core surface width from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@Sources/GhosttyTerminalView.swift`:
- Around line 5266-5271: Clamp the scrollbar-adjusted width into a single
non-negative value and reuse it for both surfaceView.frame.size.width and
documentView.frame.size.width to avoid negative sizes during live resize or
split collapse; compute let adjustedWidth = max(0, targetSize.width -
overlayScrollbarInsetWidth()) (or similar), set surfaceView.frame.size.width =
adjustedWidth and documentView.frame.size.width = scrollView.bounds.width -
overlayScrollbarInsetWidth() replaced with the same clamped adjusted value (or
compute another clamped value for the scrollView case) so the same non-negative
width is stored and subsequent layout/resize logic (including the code path
referenced by the early-exit at the later resize check) sees consistent,
non-negative geometry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0bfbbad5-514c-4627-8df2-f8463900c26c
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
Greptile SummaryThis PR fixes the overlay scrollbar covering terminal text in Key changes:
Issue found: Confidence Score: 3/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[synchronizeGeometryAndContent] --> B[Set scrollView.frame = bounds]
B --> C[Compute targetSize = scrollView.bounds.size]
C --> D[scrollbarInset = overlayScrollbarInsetWidth]
D --> E{scrollerStyle == .overlay\nAND hasVerticalScroller?}
E -- No / legacy --> F[return 0]
E -- Yes --> G{AppKit already reserved\nalreadyReserved > 0.5?}
G -- Yes --> F
G -- No --> H[return measured/fallback scroller width]
F --> I[surfaceView.frame.width = bounds.width]
H --> J[surfaceView.frame.width = bounds.width - scrollbarInset]
I --> K[synchronizeCoreSurface\nwidth = surfaceView.frame.width\n= bounds.width ⚠️ too wide for legacy]
J --> L[synchronizeCoreSurface\nwidth = surfaceView.frame.width\n= bounds.width - inset ✅ correct]
K --> M[pushTargetSurfaceSize\nwrong column count for legacy scrollers]
L --> N[pushTargetSurfaceSize\ncorrect column count for overlay scrollers]
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:5268">
P2: Clamp the computed content width to zero before assigning frame sizes; the new subtraction can produce negative widths during narrow/transition layouts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The overlay scrollbar sat on top of terminal text because surfaceView was sized to the full scrollView bounds while the Metal drawable was told to render at a narrower width. This mismatch caused the rendered content to stretch, losing the scrollbar gutter space. Subtract overlayScrollbarInsetWidth() from both surfaceView and documentView frames so the Metal drawable and view bounds match, and simplify synchronizeCoreSurface() to read surfaceView.frame.width directly instead of double-subtracting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4ba009c to
b0b33b4
Compare
Address review feedback: during narrow live resize or split collapse, subtracting the scrollbar inset could produce negative widths. Clamp to zero and reuse the same adjusted value for both surfaceView and documentView frames. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
teamleaderleo
added
S3: minor
This branch has not been deployed
2 participants
Summary
surfaceViewanddocumentViewframes by the overlay scrollbar width so the Metal drawable and view bounds matchsynchronizeCoreSurface()to readsurfaceView.frame.widthdirectly, avoiding double-subtractionProblem
The overlay scrollbar in
GhosttySurfaceScrollViewsits on top of terminal text instead of text wrapping to leave space for it. This makes text unreadable behind the scrollbar, especially in long-running sessions.Root cause:
synchronizeGeometryAndContent()setssurfaceView.frameto the fullscrollView.bounds.size, thensynchronizeCoreSurface()tells Ghostty to render at a narrower width (minus scrollbar). The Metal layer'sdrawableSizeis narrower than the view's bounds, causing the rendered content to stretch — the scrollbar gutter space is lost.Solution
overlayScrollbarInsetWidth()fromsurfaceView.frame.size.widthso the Metal drawable matches the view boundsdocumentView.frame.size.widthfor consistent scroll view contentsynchronizeCoreSurface()to usesurfaceView.frame.widthdirectlyTest plan
BUILD SUCCEEDEDverified)🤖 Generated with Claude Code
Summary by cubic
Fixes overlay scrollbar covering terminal text by sizing the terminal surface to exclude the scrollbar gutter. Text now wraps before the scrollbar, and the Metal drawable matches the view bounds.
overlayScrollbarInsetWidth()fromsurfaceViewanddocumentView, clamped to >= 0 and using the same adjusted width for both.surfaceView.frame.widthinsynchronizeCoreSurface()to avoid double-subtraction/stretching.Written for commit 5969be6. Summary will update on new commits.
Summary by CodeRabbit