Add smooth terminal scrollback#4419
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughUnifies wheel-forwarding and live-scroll reconciliation in Ghostty and adds a new terminal smooth-scrolling preference with UI, persistence, schema, docs, localization, settings-file support, and tests. ChangesScroll Wheel and Live Scroll Synchronization
Terminal Smooth-Scrolling Setting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (13 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds pixel-precise terminal scrollback by routing normal wheel gestures through
Confidence Score: 4/5Safe to merge once the missing locale translations for the search alias string are filled in; the scroll-routing logic itself is well-guarded. The settings.search.alias.setting.terminal.smooth-scrolling string-catalog entry is missing ~15 locale translations that the equivalent scrollbar alias carries. Users of those locales will get no search results or fall back to English when looking for the smooth-scrolling setting. The scroll-event routing and fractional-offset logic look sound, and all other wiring (settings UI, Command Palette, JSON import, schema) is consistent with existing patterns. Resources/Localizable.xcstrings — the new search alias string needs the same set of locale translations as settings.search.alias.setting.terminal.scrollbar. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User gesture
participant GSV as GhosttyScrollView
participant GNV as GhosttyNSView
participant GSSV as GhosttySurfaceScrollView
participant Ghostty as libghostty
User->>GSV: scrollWheel(event)
GSV->>GNV: shouldForwardWheelToGhostty?
alt smooth scroll path (no mouse capture, scrollback exists)
GNV-->>GSV: false
GSV->>GNV: recordWheelScrollActivity(event)
GSV->>GSV: super.scrollWheel (NSScrollView handles)
GSV->>GSSV: handleScrollChange (bounds notification)
GSSV->>GSSV: handleLiveScroll → currentRowOffset()
GSSV->>GNV: scrollToOffset(fractionalOffset)
GNV->>Ghostty: ghostty_surface_scroll_to_offset(surface, offset)
else TUI / mouse-capture path
GNV-->>GSV: true
GSV->>GNV: sendWheelScrollToGhostty(event)
GNV->>Ghostty: ghostty_surface_mouse_scroll(surface, x, y, mods)
end
Ghostty-->>GSSV: scrollbar update notification
GSSV->>GSSV: handleScrollbarUpdate → synchronizeScrollView
Reviews (3): Last reviewed commit: "Add smooth terminal scrollback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func handleEndLiveScroll() { | ||
| isLiveScrolling = false | ||
| if needsLiveScrollReconciliation { | ||
| needsLiveScrollReconciliation = false | ||
| DispatchQueue.main.async { [weak self] in | ||
| self?.handleLiveScroll(force: true) | ||
| } | ||
| return | ||
| } | ||
| handleLiveScroll(force: true) | ||
| } |
There was a problem hiding this comment.
needsLiveScrollReconciliation is a new boolean side-channel whose sole purpose is to gate a one-run-loop DispatchQueue.main.async deferral in handleEndLiveScroll. That deferral is the classic symptom of an ordering race: NSScrollViewDidEndLiveScroll fires while some internal NSScrollView state hasn't yet settled, but neither the flag nor the deferral names the invariant or why a real completion signal cannot replace it. The source of truth for "correct position after live scroll ends" is split between a synchronous path and a deferred path with no documented coordination rule.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
| override func scrollWheel(with event: NSEvent) { | ||
| guard shouldForwardWheelToGhostty(event) else { | ||
| recordWheelScrollActivity(event) | ||
| super.scrollWheel(with: event) | ||
| return | ||
| } | ||
| sendWheelScrollToGhostty(event) |
There was a problem hiding this comment.
recordWheelScrollActivity called twice per smooth-scroll event
When shouldForwardWheelToGhostty returns false, GhosttyNSView.scrollWheel calls recordWheelScrollActivity and then super.scrollWheel. Because GhosttyNSView lives inside the documentView of GhosttyScrollView (NSScrollView), the event propagates up the responder chain to GhosttyScrollView.scrollWheel, which hits the same guard and calls surfaceView.recordWheelScrollActivity a second time. The double call cancels and re-schedules the scrollEndTimer in GhosttyApp.shared.markScrollActivity on every smooth-scroll tick.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@ghostty`:
- Line 1: CI is failing because scripts/ghosttykit-checksums.txt lacks a
checksum entry for the new GhosttyKit submodule commit
8577c2b5162a63eb0d12ffbb1f94c7b33ca9ced7; add a new line in
scripts/ghosttykit-checksums.txt that pins the correct checksum for the prebuilt
GhosttyKit artifact corresponding to commit
8577c2b5162a63eb0d12ffbb1f94c7b33ca9ced7 (use the repository’s checksum format
used by existing entries so the checksum lookup logic for GhosttyKit can
validate this commit in CI).
In `@Sources/GhosttyTerminalView.swift`:
- Around line 13280-13289: handleEndLiveScroll currently clears isLiveScrolling
before the deferred reconciliation runs, allowing a scrollbar packet to enter
the non-live path and snap the view; to fix, delay clearing isLiveScrolling
until after handleLiveScroll(force: true) completes: in the
DispatchQueue.main.async branch, call handleLiveScroll(force: true) inside the
closure first (using [weak self]) and then set self?.isLiveScrolling = false and
self?.needsLiveScrollReconciliation = false; in the synchronous path, call
handleLiveScroll(force: true) first and then set isLiveScrolling = false (and
needsLiveScrollReconciliation = false if relevant). Apply the same change to the
other occurrence around lines 13302–13304 so isLiveScrolling remains true until
reconciliation finishes.
- Around line 9747-9756: The wheel-handling logic currently returns true from
shouldForwardWheelToGhostty when surface is nil but sendWheelScrollToGhostty
immediately returns if surface is nil, dropping events; change
shouldForwardWheelToGhostty to fall back to AppKit by returning false when
surface is missing (i.e., make the initial guard for surface return false
instead of true) so that NSScrollView/AppKit receives wheel events during
attach/recreate, and verify sendWheelScrollToGhostty still safely guards on
surface (keep its guard) to avoid dereferencing a nil surface.
🪄 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: fe1fae91-e0e4-46f0-a6bc-9fb9e6c4ce69
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftghostty
| fileprivate func shouldForwardWheelToGhostty(_: NSEvent) -> Bool { | ||
| guard let surface else { return true } | ||
| guard let scrollbar, scrollbar.total > scrollbar.len else { return true } | ||
| return ghostty_surface_mouse_captured(surface) | ||
| } | ||
|
|
||
| fileprivate func sendWheelScrollToGhostty(_ event: NSEvent) { | ||
| NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object: self) | ||
| guard let surface = surface else { return } | ||
| lastScrollEventTime = CACurrentMediaTime() | ||
| recordWheelScrollActivity(event) |
There was a problem hiding this comment.
Fall back to AppKit when the runtime surface is missing.
Line 9748 returns true when surface is nil, but Line 9755 immediately bails on that same condition. In that state the wheel event is consumed by neither Ghostty nor NSScrollView, so scroll gestures are dropped during attach/recreate windows.
🛠️ Suggested fix
fileprivate func shouldForwardWheelToGhostty(_: NSEvent) -> Bool {
- guard let surface else { return true }
+ guard let surface else { return false }
guard let scrollbar, scrollbar.total > scrollbar.len else { return true }
return ghostty_surface_mouse_captured(surface)
}📝 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.
| fileprivate func shouldForwardWheelToGhostty(_: NSEvent) -> Bool { | |
| guard let surface else { return true } | |
| guard let scrollbar, scrollbar.total > scrollbar.len else { return true } | |
| return ghostty_surface_mouse_captured(surface) | |
| } | |
| fileprivate func sendWheelScrollToGhostty(_ event: NSEvent) { | |
| NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object: self) | |
| guard let surface = surface else { return } | |
| lastScrollEventTime = CACurrentMediaTime() | |
| recordWheelScrollActivity(event) | |
| fileprivate func shouldForwardWheelToGhostty(_: NSEvent) -> Bool { | |
| guard let surface else { return false } | |
| guard let scrollbar, scrollbar.total > scrollbar.len else { return true } | |
| return ghostty_surface_mouse_captured(surface) | |
| } | |
| fileprivate func sendWheelScrollToGhostty(_ event: NSEvent) { | |
| NotificationCenter.default.post(name: .ghosttyDidReceiveWheelScroll, object: self) | |
| guard let surface = surface else { return } | |
| recordWheelScrollActivity(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 `@Sources/GhosttyTerminalView.swift` around lines 9747 - 9756, The
wheel-handling logic currently returns true from shouldForwardWheelToGhostty
when surface is nil but sendWheelScrollToGhostty immediately returns if surface
is nil, dropping events; change shouldForwardWheelToGhostty to fall back to
AppKit by returning false when surface is missing (i.e., make the initial guard
for surface return false instead of true) so that NSScrollView/AppKit receives
wheel events during attach/recreate, and verify sendWheelScrollToGhostty still
safely guards on surface (keep its guard) to avoid dereferencing a nil surface.
| private func handleEndLiveScroll() { | ||
| isLiveScrolling = false | ||
| if needsLiveScrollReconciliation { | ||
| needsLiveScrollReconciliation = false | ||
| DispatchQueue.main.async { [weak self] in | ||
| self?.handleLiveScroll(force: true) | ||
| } | ||
| return | ||
| } | ||
| handleLiveScroll(force: true) |
There was a problem hiding this comment.
Keep live-scroll mode active until the forced reconciliation finishes.
handleEndLiveScroll() clears isLiveScrolling before the deferred handleLiveScroll(force: true) runs. If a scrollbar packet lands in that gap, synchronizeScrollView() re-enters the non-live path and snaps the clip view back to integer scrollbar.offset, so the final fractional resting position is lost.
🛠️ Suggested fix
private func handleEndLiveScroll() {
- isLiveScrolling = false
- if needsLiveScrollReconciliation {
- needsLiveScrollReconciliation = false
- DispatchQueue.main.async { [weak self] in
- self?.handleLiveScroll(force: true)
- }
- return
- }
- handleLiveScroll(force: true)
+ let finishReconciliation = { [weak self] in
+ guard let self else { return }
+ self.handleLiveScroll(force: true)
+ self.needsLiveScrollReconciliation = false
+ self.isLiveScrolling = false
+ }
+
+ if needsLiveScrollReconciliation {
+ DispatchQueue.main.async(execute: finishReconciliation)
+ return
+ }
+
+ finishReconciliation()
}Also applies to: 13302-13304
🤖 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/GhosttyTerminalView.swift` around lines 13280 - 13289,
handleEndLiveScroll currently clears isLiveScrolling before the deferred
reconciliation runs, allowing a scrollbar packet to enter the non-live path and
snap the view; to fix, delay clearing isLiveScrolling until after
handleLiveScroll(force: true) completes: in the DispatchQueue.main.async branch,
call handleLiveScroll(force: true) inside the closure first (using [weak self])
and then set self?.isLiveScrolling = false and
self?.needsLiveScrollReconciliation = false; in the synchronous path, call
handleLiveScroll(force: true) first and then set isLiveScrolling = false (and
needsLiveScrollReconciliation = false if relevant). Apply the same change to the
other occurrence around lines 13302–13304 so isLiveScrolling remains true until
reconciliation finishes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
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:9748">
P2: Return `false` when `surface` is missing so wheel events fall back to `NSScrollView`; returning `true` here routes into `sendWheelScrollToGhostty` and then exits early, dropping scroll input during surface attach/recreate windows.</violation>
</file>
<file name="ghostty">
<violation number="1" location="ghostty:1">
P1: Add the GhosttyKit checksum entry for this submodule commit; without it, `download-prebuilt-ghosttykit.sh` fails and CI is blocked.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| @@ -1 +1 @@ | |||
| Subproject commit ff6e1260d2e7767de55b8d9307b328e4060545b7 | |||
| Subproject commit 8577c2b5162a63eb0d12ffbb1f94c7b33ca9ced7 | |||
There was a problem hiding this comment.
P1: Add the GhosttyKit checksum entry for this submodule commit; without it, download-prebuilt-ghosttykit.sh fails and CI is blocked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ghostty, line 1:
<comment>Add the GhosttyKit checksum entry for this submodule commit; without it, `download-prebuilt-ghosttykit.sh` fails and CI is blocked.</comment>
<file context>
@@ -1 +1 @@
-Subproject commit ff6e1260d2e7767de55b8d9307b328e4060545b7
+Subproject commit 8577c2b5162a63eb0d12ffbb1f94c7b33ca9ced7
</file context>
| } | ||
|
|
||
| fileprivate func shouldForwardWheelToGhostty(_: NSEvent) -> Bool { | ||
| guard let surface else { return true } |
There was a problem hiding this comment.
P2: Return false when surface is missing so wheel events fall back to NSScrollView; returning true here routes into sendWheelScrollToGhostty and then exits early, dropping scroll input during surface attach/recreate windows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GhosttyTerminalView.swift, line 9748:
<comment>Return `false` when `surface` is missing so wheel events fall back to `NSScrollView`; returning `true` here routes into `sendWheelScrollToGhostty` and then exits early, dropping scroll input during surface attach/recreate windows.</comment>
<file context>
@@ -9736,9 +9736,24 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations {
+ }
+
+ fileprivate func shouldForwardWheelToGhostty(_: NSEvent) -> Bool {
+ guard let surface else { return true }
+ guard let scrollbar, scrollbar.total > scrollbar.len else { return true }
+ return ghostty_surface_mouse_captured(surface)
</file context>
| guard let surface else { return true } | |
| guard let surface else { return false } |
lawrencecchen
force-pushed
the
feat-smooth-scrolling
branch
from
18b2526 to
1f05f5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/GhosttyTerminalView.swift`:
- Around line 13260-13295: The bottom-detection logic in handleLiveScroll is
inverted: currentRowOffset() returns 0 at bottom and maxRowOffset() at top, so
change reachedBottom to compare rowOffset against 0 (e.g., abs(rowOffset - 0) <
epsilon or rowOffset <= threshold) instead of maxRowOffset(); update the branch
that handles reachedBottom to use bottomOffset = 0 (and set
lastSentRow/lastSentOffset accordingly) before calling
surfaceView.scrollToOffset(0); ensure the dedupe logic that computes
needsBottomCommit still uses lastSentOffset vs the new offset threshold so final
tiny positive offsets commit to exact bottom.
🪄 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: 536d497b-abc9-4834-b9ea-5f4956f9c901
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftghostty
lawrencecchen
force-pushed
the
feat-smooth-scrolling
branch
from
1f05f5b to
a0cd913
Compare
lawrencecchen
force-pushed
the
feat-smooth-scrolling
branch
from
a0cd913 to
3007edb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghostty (1)
1-1:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing GhosttyKit checksum blocks CI (confirmed by pipeline failures).
This issue has already been flagged in previous review comments. All CI jobs continue to fail because
scripts/ghosttykit-checksums.txtlacks the checksum entry for commit8577c2b5162a63eb0d12ffbb1f94c7b33ca9ced7.🤖 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 `@ghostty` at line 1, The CI failures are caused by a missing checksum entry for commit 8577c2b5162a63eb0d12ffbb1f94c7b33ca9ced7 in scripts/ghosttykit-checksums.txt; open scripts/ghosttykit-checksums.txt and add the correct checksum line for that commit (matching the format of the other entries), verify the checksum value against the artifact for commit 8577c2b...ced7, commit the updated checksums file, and push so the CI can validate the new entry.
🤖 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 `@Resources/Localizable.xcstrings`:
- Around line 77108-77124: The four new localization keys
(settings.search.alias.setting.terminal.smooth-scrolling,
settings.terminal.smoothScrolling,
settings.terminal.smoothScrolling.subtitleOff,
settings.terminal.smoothScrolling.subtitleOn) only include en/ja; add entries
for all 19 supported locales (ar, bs, da, de, en, es, fr, it, ja, ko, nb, pl,
pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant). For high-confidence locales provide
appropriate translated "value" and mark "state":"translated"; for
lower-confidence locales (ar, bs, da, pl, th, tr) use the English text as the
fallback value but still set "state":"translated". Ensure each key’s
"localizations" object contains an object for every locale with a "stringUnit"
containing "state" and "value".
---
Duplicate comments:
In `@ghostty`:
- Line 1: The CI failures are caused by a missing checksum entry for commit
8577c2b5162a63eb0d12ffbb1f94c7b33ca9ced7 in scripts/ghosttykit-checksums.txt;
open scripts/ghosttykit-checksums.txt and add the correct checksum line for that
commit (matching the format of the other entries), verify the checksum value
against the artifact for commit 8577c2b...ced7, commit the updated checksums
file, and push so the CI can validate the new entry.
🪄 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: c539eeac-f2af-4458-a38b-890741baebd6
📒 Files selected for processing (16)
Resources/Localizable.xcstringsSources/App/WorkspaceRuntimeSettings.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/CommandPaletteSettingsToggleTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swiftcmuxTests/SettingsSearchIndexTests.swiftghosttyweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux.schema.json
| "settings.search.alias.setting.terminal.smooth-scrolling": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "terminal.smoothScrolling smooth scroll scrolling pixel precise trackpad magic mouse scrollback" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "terminal.smoothScrolling smooth scroll scrolling pixel precise trackpad magic mouse scrollback ターミナル スムーズ スクロール トラックパッド" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Extract all unique locale codes from the Localizable.xcstrings catalog
# and check if the new keys are missing any of them.
# Extract all unique locale keys from the catalog
echo "=== All locales found in Localizable.xcstrings ==="
cat Resources/Localizable.xcstrings | jq -r '
.. | objects | select(has("localizations")) | .localizations | keys[]
' | sort -u
echo ""
echo "=== Locales present in new smooth-scrolling keys ==="
cat Resources/Localizable.xcstrings | jq -r '
.["settings.terminal.smoothScrolling"].localizations | keys[]
' | sort
echo ""
echo "=== Expected: All locales from first list should appear in second list ==="Repository: manaflow-ai/cmux
Length of output: 384
🏁 Script executed:
#!/bin/bash
# Search for the new keys in the catalog and show their locale coverage
echo "=== Searching for the four new keys ==="
keys=("settings.search.alias.setting.terminal.smooth-scrolling"
"settings.terminal.smoothScrolling"
"settings.terminal.smoothScrolling.subtitleOff"
"settings.terminal.smoothScrolling.subtitleOn")
for key in "${keys[@]}"; do
echo ""
echo "--- Key: $key ---"
# Extract section for this key including next line to count locales
grep -A 50 "\"$key\"" Resources/Localizable.xcstrings | head -60 | grep -E "(\"(ar|bs|da|de|en|es|fr|it|ja|ko|nb|pl|pt-BR|ru|th|tr|uk|zh-Hans|zh-Hant)\":|\"state\"|\"value\")" | head -20
done
echo ""
echo "=== Verify the key exists at all ==="
grep -c "settings.terminal.smoothScrolling" Resources/Localizable.xcstringsRepository: manaflow-ai/cmux
Length of output: 4233
All four new localization keys must include entries for every locale supported by this catalog.
The catalog supports 19 locales (ar, bs, da, de, en, es, fr, it, ja, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant), but all four new keys—settings.search.alias.setting.terminal.smooth-scrolling, settings.terminal.smoothScrolling, settings.terminal.smoothScrolling.subtitleOff, and settings.terminal.smoothScrolling.subtitleOn—only include entries for en and ja.
Per coding guidelines and established convention, newly added keys must include entries for all 19 supported locales. For lower-confidence locales (ar, bs, da, pl, th, tr), use English-language fallback with state: "translated".
Missing locales for each key: ar, bs, da, de, es, fr, it, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant.
🤖 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 `@Resources/Localizable.xcstrings` around lines 77108 - 77124, The four new
localization keys (settings.search.alias.setting.terminal.smooth-scrolling,
settings.terminal.smoothScrolling,
settings.terminal.smoothScrolling.subtitleOff,
settings.terminal.smoothScrolling.subtitleOn) only include en/ja; add entries
for all 19 supported locales (ar, bs, da, de, en, es, fr, it, ja, ko, nb, pl,
pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant). For high-confidence locales provide
appropriate translated "value" and mark "state":"translated"; for
lower-confidence locales (ar, bs, da, pl, th, tr) use the English text as the
fallback value but still set "state":"translated". Ensure each key’s
"localizations" object contains an object for every locale with a "stringUnit"
containing "state" and "value".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3007edb. Configure here.
lawrencecchen
force-pushed
the
feat-smooth-scrolling
branch
from
3007edb to
6eb4ebb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Resources/Localizable.xcstrings (1)
77108-77124:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAll four new localization keys are missing 17 required locale entries.
This issue was already flagged in a previous review and remains unresolved. All four keys—
settings.search.alias.setting.terminal.smooth-scrolling,settings.terminal.smoothScrolling,settings.terminal.smoothScrolling.subtitleOff, andsettings.terminal.smoothScrolling.subtitleOn—only includeenandjalocalizations.Per coding guidelines, the catalog supports 19 locales and all new keys must include entries for every supported locale. Missing locales: ar, bs, da, de, es, fr, it, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant.
For high-confidence locales (de, es, fr, it, ko, nb, pt-BR, ru, uk, zh-Hans, zh-Hant), provide appropriate translated values with
state: "translated". For lower-confidence locales (ar, bs, da, pl, th, tr), use English-language fallback text as the value but still setstate: "translated"per established catalog convention.As per coding guidelines: "Every new Swift localization key must be backed by a matching
Resources/*.xcstringsentry with translated values for every locale already supported by that catalog." Based on learnings: For lower-confidence locales (ar, bs, da, pl, th, tr), the established convention is to use English-language fallback withstate: "translated"for newly added keys.Also applies to: 107123-107173
🤖 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 `@Resources/Localizable.xcstrings` around lines 77108 - 77124, The four new localization keys (settings.search.alias.setting.terminal.smooth-scrolling, settings.terminal.smoothScrolling, settings.terminal.smoothScrolling.subtitleOff, settings.terminal.smoothScrolling.subtitleOn) only include en and ja and must be populated for all 19 supported locales: ar, bs, da, de, es, fr, it, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant plus the existing en/ja; add a localization entry for each missing locale in the Resources/xcstrings entries, setting state: "translated" for every locale, and for high-confidence locales (de, es, fr, it, ko, nb, pt-BR, ru, uk, zh-Hans, zh-Hant) provide real translated values while for lower-confidence locales (ar, bs, da, pl, th, tr) use the English fallback text as the value (still state: "translated"); apply the same fix to the other affected keys/ranges called out in the comment (107123-107173).
🤖 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/GhosttyTerminalView.swift`:
- Around line 13217-13225: When increasing documentView height in
GhosttyTerminalView, the live-scroll adjustment uses previousRowOffset +
(maxRowOffset - previousMaxRowOffset) which moves a bottom-pinned session up
when previousRowOffset == 0; change the scroll calculation in the
isLiveScrolling branch (where previousRowOffset and previousMaxRowOffset are
unwrapped and maxRowOffset() is available) to compute the delta = maxRowOffset -
previousMaxRowOffset but keep the resulting row offset at 0 if previousRowOffset
== 0 (i.e., newOffset = 0 when previously pinned, otherwise newOffset =
previousRowOffset + delta) and call scroll(toRowOffset:) with that newOffset.
---
Duplicate comments:
In `@Resources/Localizable.xcstrings`:
- Around line 77108-77124: The four new localization keys
(settings.search.alias.setting.terminal.smooth-scrolling,
settings.terminal.smoothScrolling,
settings.terminal.smoothScrolling.subtitleOff,
settings.terminal.smoothScrolling.subtitleOn) only include en and ja and must be
populated for all 19 supported locales: ar, bs, da, de, es, fr, it, ko, nb, pl,
pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant plus the existing en/ja; add a
localization entry for each missing locale in the Resources/xcstrings entries,
setting state: "translated" for every locale, and for high-confidence locales
(de, es, fr, it, ko, nb, pt-BR, ru, uk, zh-Hans, zh-Hant) provide real
translated values while for lower-confidence locales (ar, bs, da, pl, th, tr)
use the English fallback text as the value (still state: "translated"); apply
the same fix to the other affected keys/ranges called out in the comment
(107123-107173).
🪄 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: 5ad7eb74-90cd-4e98-b8d5-2730122ea8cb
📒 Files selected for processing (16)
Resources/Localizable.xcstringsSources/App/WorkspaceRuntimeSettings.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/cmuxApp.swiftcmuxTests/CommandPaletteSettingsToggleTests.swiftcmuxTests/KeyboardShortcutSettingsFileStoreStartupTests.swiftcmuxTests/SettingsSearchIndexTests.swiftghosttyweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux.schema.json
| if abs(documentView.frame.height - targetDocumentHeight) > 0.5 { | ||
| documentView.frame.size.height = targetDocumentHeight | ||
| didChangeGeometry = true | ||
| if isLiveScrolling, | ||
| let previousRowOffset, | ||
| let previousMaxRowOffset, | ||
| let maxRowOffset = maxRowOffset() { | ||
| scroll(toRowOffset: previousRowOffset + maxRowOffset - previousMaxRowOffset) | ||
| } |
There was a problem hiding this comment.
Keep bottom-pinned live scroll at offset 0 when geometry grows.
If previousRowOffset is already 0, adding maxRowOffset - previousMaxRowOffset pushes the clip view upward by every newly appended row. A session receiving output while the user is still at the bottom of a live scroll ends up marked as scrolled away from bottom.
🛠️ Proposed fix
if abs(documentView.frame.height - targetDocumentHeight) > 0.5 {
documentView.frame.size.height = targetDocumentHeight
didChangeGeometry = true
if isLiveScrolling,
let previousRowOffset,
let previousMaxRowOffset,
let maxRowOffset = maxRowOffset() {
- scroll(toRowOffset: previousRowOffset + maxRowOffset - previousMaxRowOffset)
+ if previousRowOffset <= 0.001 {
+ scroll(toRowOffset: 0)
+ } else {
+ scroll(toRowOffset: previousRowOffset + maxRowOffset - previousMaxRowOffset)
+ }
}
}🤖 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/GhosttyTerminalView.swift` around lines 13217 - 13225, When
increasing documentView height in GhosttyTerminalView, the live-scroll
adjustment uses previousRowOffset + (maxRowOffset - previousMaxRowOffset) which
moves a bottom-pinned session up when previousRowOffset == 0; change the scroll
calculation in the isLiveScrolling branch (where previousRowOffset and
previousMaxRowOffset are unwrapped and maxRowOffset() is available) to compute
the delta = maxRowOffset - previousMaxRowOffset but keep the resulting row
offset at 0 if previousRowOffset == 0 (i.e., newOffset = 0 when previously
pinned, otherwise newOffset = previousRowOffset + delta) and call
scroll(toRowOffset:) with that newOffset.
| } | ||
| } | ||
| }, | ||
| "settings.search.alias.setting.terminal.smooth-scrolling": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "terminal.smoothScrolling smooth scroll scrolling pixel precise trackpad magic mouse scrollback" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "terminal.smoothScrolling smooth scroll scrolling pixel precise trackpad magic mouse scrollback ターミナル スムーズ スクロール トラックパッド" | ||
| } | ||
| } |
There was a problem hiding this comment.
The new settings.search.alias.setting.terminal.smooth-scrolling entry only has en and ja translations, but the directly comparable settings.search.alias.setting.terminal.scrollbar entry carries translations for ar, bs, da, de, es, fr, it, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant. Users of those locales will get no results (or English fallback) when they search for the smooth-scrolling setting in their own language. The three settings.terminal.smoothScrolling UI-label strings (title, subtitleOn, subtitleOff) have the same gap, though that matches the existing en+ja-only pattern for settings.terminal.scrollBar; the alias string deviation is the concrete regression.
Rule Used: Flag production user-facing text that is not fully... (source)
1 participant
Adds smooth scrollback for regular terminal scroll gestures by letting AppKit track fractional scroll position and sending that fractional row offset into libghostty. Mouse-reporting terminal apps still receive wheel events directly.
Ghostty submodule PR: manaflow-ai/ghostty#61
Changes:
NSScrollViewwhen scrollback is available and mouse capture is off.ghostty_surface_scroll_to_offset.Testing:
./scripts/ensure-ghosttykit.shnm -gU ghostty/macos/GhosttyKit.xcframework/macos-arm64_x86_64/ghostty-internal.a | rg "ghostty_surface_scroll_to_offset|ghostty_surface_mouse_scroll"./scripts/reload.sh --tag smooth-scrollsmooth-scrolland preparedsmooth scroll testwith scrollback through/tmp/cmux-debug-smooth-scroll.sock.Need help on this PR? Tag
@codesmithwith what you need.Note
Medium Risk
Changes core terminal wheel/scrollback routing and scroll synchronization logic, which could introduce regressions in scroll position, TUI/mouse-capture behavior, or scrollbar syncing across surfaces. Adds a new persisted setting (UI, command palette, config file import/schema) that must remain consistent across app and web/docs.
Overview
Adds a new "Smooth Terminal Scrolling" setting (default on) that enables pixel-precise trackpad/Magic Mouse scrollback by letting AppKit manage fractional scroll position and forwarding the resulting offset into libghostty via
ghostty_surface_scroll_to_offset.When enabled, wheel events are conditionally routed through
NSScrollViewfor normal scrollback (and still forwarded directly to Ghostty for mouse-captured/TUI apps), with updated scroll-view synchronization to track fractional offsets, avoid feedback loops during programmatic scroll updates, and reconcile live-scroll geometry changes.Plumbs the setting through Settings UI, Command Palette toggle, settings search/navigation aliases,
cmux.jsonsupport (parser/template/schema/docs), localization strings, and adds tests covering notifications, settings-file import/side effects, and search anchors.Reviewed by Cursor Bugbot for commit 6eb4ebb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds smooth terminal scrollback with pixel-precise AppKit scrolling and fractional row offsets, controlled by a new “Smooth Terminal Scrolling” setting (on by default). Non-captured wheel gestures use the AppKit scroll view when scrollback exists; TUIs/mouse-captured apps still receive raw wheel events.
New Features
ghostty_surface_scroll_to_offset; clamp/track last offset, commit an exact bottom offset, prevent echo loops, preserve “scrolled away” state, keep relative row offset during live-resize, reconcile at scroll end, and snap to integer rows when the setting is turned off.cmux.jsonsupport (schema/template), settings search aliases/anchors, and a docs example.Dependencies
ghosttysubmodule to include the offset scrolling API (ghostty_surface_scroll_to_offset).Written for commit 6eb4ebb. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests