Add OSC 9;4 progress reporting (pane bar + tab indicator)#28
Merged
Conversation
Receives GHOSTTY_ACTION_PROGRESS_REPORT from libghostty and renders a 2px progress bar at the top of the active terminal surface. Tab dots reflect the progress state (accent/red/orange). State is auto-cleared after 15 seconds of inactivity, matching Ghostty's own behavior.
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds per-surface progress reporting: new SurfaceProgressReport model, UI components (SurfaceProgressBar and tab-dot coloring), workspace manager tracking with auto-clear timers, and a progress-report callback threaded from backend (Ghostty) through GhosttyRuntime → LibghosttySurfaceView → GhosttyTerminalView into the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Backend as Backend (Ghostty)
participant Runtime as GhosttyRuntime
participant Surface as LibghosttySurfaceView
participant Manager as WorkspaceManager
participant UI as PaneLeafView / SurfaceProgressBar
Backend->>Runtime: GHOSTTY_ACTION_PROGRESS_REPORT payload
Runtime->>Runtime: Convert payload -> SurfaceProgressReport?
Runtime->>Surface: callbacks.onProgressReport(report)
Surface->>Manager: setProgressReport(workspaceId, surfaceId, report)
Manager->>Manager: Store progressBySurfaceId[surfaceId], start 15s timer
Manager->>UI: Publish progressBySurfaceId update
UI->>UI: Render/refresh SurfaceProgressBar and tab-dot
Note over Manager: After 15s timer triggers
Manager->>Manager: clearProgressReport(surfaceId)
Manager->>UI: Publish removal → UI hides progress indicator
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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: 3
🧹 Nitpick comments (4)
Sources/Shellraiser/Services/Workspaces/WorkspaceManager+SurfaceOperations.swift (1)
148-154: Consider extracting the auto-clear delay into a named constant.This keeps behavior easier to tune and avoids scattering timing literals.
♻️ Proposed refactor
+private enum ProgressReporting { + static let autoClearDelay: TimeInterval = 15 +} + /// Stores or removes an OSC 9;4 progress report for a surface, resetting the 15-second auto-clear timer. func setProgressReport(workspaceId: UUID, surfaceId: UUID, report: SurfaceProgressReport?) { if let report { progressBySurfaceId[surfaceId] = report progressClearTimers[surfaceId]?.invalidate() - progressClearTimers[surfaceId] = Timer.scheduledTimer(withTimeInterval: 15, repeats: false) { [weak self] _ in + progressClearTimers[surfaceId] = Timer.scheduledTimer(withTimeInterval: ProgressReporting.autoClearDelay, repeats: false) { [weak self] _ in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Shellraiser/Services/Workspaces/WorkspaceManager`+SurfaceOperations.swift around lines 148 - 154, Extract the hard-coded 15-second auto-clear delay in setProgressReport into a named constant (e.g., progressAutoClearInterval or kProgressAutoClearInterval) and use that constant when scheduling Timer.scheduledTimer; update any other occurrences of the same literal (such as other Timer.scheduledTimer uses related to progress clearing) to reference the constant and add it near WorkspaceManager (as a static let or file-level private constant) so the timing is easy to tune and annotated.Tests/ShellraiserTests/GhosttyTerminalViewTests.swift (1)
203-224: Consider adding one assertion thatonProgressReportis actually propagated.Current updates only ensure the parameter is accepted. A small callback-forwarding assertion would lock in the new behavior and prevent silent regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/ShellraiserTests/GhosttyTerminalViewTests.swift` around lines 203 - 224, Add an assertion in the update(...) test helper to verify that the onProgressReport closure is forwarded: in the update function (the one that currently assigns workingDirectoryChangeHandler and appends to updatedSurfaceIds) accept and store a local variable or call site-provided stub for onProgressReport and trigger it with a sample SurfaceProgressReport, then assert the receiver/handler (or a test-recorded variable) gets that same value; reference the update(...) function and the onProgressReport parameter and updatedSurfaceIds to locate where to add the forwarding/assertion.Sources/Shellraiser/Features/WorkspaceDetail/SurfaceProgressBar.swift (1)
11-17: Centralize progress-state color mapping to avoid drift across views.
SurfaceProgressBarandSurfaceTabButtonboth encode the same state→color logic. A shared helper keeps them synchronized.♻️ Proposed refactor
+extension SurfaceProgressState { + var tintColor: Color { + switch self { + case .error: .red + case .pause: .orange + default: .accentColor + } + } +} + struct SurfaceProgressBar: View { let report: SurfaceProgressReport private var color: Color { - switch report.state { - case .error: return .red - case .pause: return .orange - default: return .accentColor - } + report.state.tintColor }Also apply
report.state.tintColorinSources/Shellraiser/Features/WorkspaceDetail/SurfaceTabButton.swiftLine 32-36.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Shellraiser/Features/WorkspaceDetail/SurfaceProgressBar.swift` around lines 11 - 17, Add a centralized color mapping by adding a computed property named tintColor on the shared state type (e.g., Report.State or whatever type report.state is) that returns .red for .error, .orange for .pause, and .accentColor otherwise, then replace the duplicated logic inside SurfaceProgressBar (the private var color) and SurfaceTabButton (lines where state→color is implemented) to use report.state.tintColor instead so both views reference the single source of truth.Sources/Shellraiser/Infrastructure/Ghostty/GhosttyRuntime.swift (1)
138-150: Collapse host updates into one write path.
acquireHostView(...)mutates existing hosts here, andGhosttyTerminalView.syncHostView(...)inSources/Shellraiser/Features/Terminal/GhosttyTerminalView.swift, Lines 236-247 immediately callshost.update(...)again. Every new callback now has to be threaded through both mutation paths, which is getting easy to drift. Consider making acquisition pure and keeping host mutation in one place.As per coding guidelines, "Prefer small, composable methods and keep methods focused and readable".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Shellraiser/Infrastructure/Ghostty/GhosttyRuntime.swift` around lines 138 - 150, The host acquisition currently mutates host state in acquireHostView (via existing.update(...) on hostViewsBySurfaceId) while GhosttyTerminalView.syncHostView also calls host.update(...), causing duplicated mutation paths; change acquireHostView to be pure/side-effect-free (return the existing host or create a new host without invoking update callbacks) and consolidate all state mutations to a single place (e.g., GhosttyTerminalView.syncHostView), so only syncHostView calls host.update(...) (or a single canonical update method) and all callback wiring (onActivate, onInput, onTitleChange, etc.) is applied there; update references to existing.update and host.update usages accordingly and remove the duplicate mutation in acquireHostView to ensure callbacks are only threaded through the single write path.
🤖 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/Shellraiser/Infrastructure/Ghostty/GhosttyRuntime.swift`:
- Around line 27-28: The enum case progressReport(SurfaceProgressReport?)
conflates an explicit "remove" (nil) with unsupported/unknown states because the
switch's default branch also returns nil; change the model to distinguish
explicit remove from ignored/unsupported states (e.g., add a dedicated case like
progressRemoved or make the associated value non-optional:
progressReport(SurfaceProgressReport) plus a separate progressRemoved case) and
update all switch handling (the branches that currently return nil in the
default path) to leave progress unchanged for unsupported states while only
treating the explicit remove case as a removal; update any code that constructs
or matches progressReport to use the new explicit remove/non-optional pattern so
unknown states do not clear progress.
In `@Sources/Shellraiser/Models/SurfaceProgressReport.swift`:
- Around line 10-15: SurfaceProgressReport currently allows progress values
101...255 because it's a bare UInt8; add a validating initializer on
SurfaceProgressReport (e.g., init(state: SurfaceProgressState, progress:
UInt8?)) that checks if progress is nil or within 0...100 and otherwise
rejects/handles the value (use a failable init returning nil or
assert/precondition depending on desired behavior), and make callers use this
initializer so the invariant is enforced; reference SurfaceProgressReport, its
progress property, and SurfaceProgressState when implementing the validation.
In
`@Sources/Shellraiser/Services/Workspaces/WorkspaceManager`+SurfaceOperations.swift:
- Around line 149-156: In setProgressReport, avoid writing progress/timer
entries for surfaces that no longer exist and prevent the timer closure from
re-inserting them: before assigning progressBySurfaceId[surfaceId] and creating
progressClearTimers[surfaceId], verify the surface is still present in the
workspace (e.g., check workspace.surfaces contains surfaceId or call a
surfaceExists(workspaceId:surfaceId) helper); and inside the Timer scheduled
closure, re-check that the surface still exists before removing or modifying
progressBySurfaceId/progressClearTimers (and early-return if the surface was
removed) so stale callbacks cannot reinsert entries for closed surfaces. Ensure
you reference setProgressReport, progressBySurfaceId, progressClearTimers and
the workspace surface presence check when implementing the guards.
---
Nitpick comments:
In `@Sources/Shellraiser/Features/WorkspaceDetail/SurfaceProgressBar.swift`:
- Around line 11-17: Add a centralized color mapping by adding a computed
property named tintColor on the shared state type (e.g., Report.State or
whatever type report.state is) that returns .red for .error, .orange for .pause,
and .accentColor otherwise, then replace the duplicated logic inside
SurfaceProgressBar (the private var color) and SurfaceTabButton (lines where
state→color is implemented) to use report.state.tintColor instead so both views
reference the single source of truth.
In `@Sources/Shellraiser/Infrastructure/Ghostty/GhosttyRuntime.swift`:
- Around line 138-150: The host acquisition currently mutates host state in
acquireHostView (via existing.update(...) on hostViewsBySurfaceId) while
GhosttyTerminalView.syncHostView also calls host.update(...), causing duplicated
mutation paths; change acquireHostView to be pure/side-effect-free (return the
existing host or create a new host without invoking update callbacks) and
consolidate all state mutations to a single place (e.g.,
GhosttyTerminalView.syncHostView), so only syncHostView calls host.update(...)
(or a single canonical update method) and all callback wiring (onActivate,
onInput, onTitleChange, etc.) is applied there; update references to
existing.update and host.update usages accordingly and remove the duplicate
mutation in acquireHostView to ensure callbacks are only threaded through the
single write path.
In
`@Sources/Shellraiser/Services/Workspaces/WorkspaceManager`+SurfaceOperations.swift:
- Around line 148-154: Extract the hard-coded 15-second auto-clear delay in
setProgressReport into a named constant (e.g., progressAutoClearInterval or
kProgressAutoClearInterval) and use that constant when scheduling
Timer.scheduledTimer; update any other occurrences of the same literal (such as
other Timer.scheduledTimer uses related to progress clearing) to reference the
constant and add it near WorkspaceManager (as a static let or file-level private
constant) so the timing is easy to tune and annotated.
In `@Tests/ShellraiserTests/GhosttyTerminalViewTests.swift`:
- Around line 203-224: Add an assertion in the update(...) test helper to verify
that the onProgressReport closure is forwarded: in the update function (the one
that currently assigns workingDirectoryChangeHandler and appends to
updatedSurfaceIds) accept and store a local variable or call site-provided stub
for onProgressReport and trigger it with a sample SurfaceProgressReport, then
assert the receiver/handler (or a test-recorded variable) gets that same value;
reference the update(...) function and the onProgressReport parameter and
updatedSurfaceIds to locate where to add the forwarding/assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e63da255-4ba1-484b-ba32-1562f0975d35
📒 Files selected for processing (11)
Shellraiser.xcodeproj/project.pbxprojSources/Shellraiser/Features/Terminal/GhosttyTerminalView.swiftSources/Shellraiser/Features/WorkspaceDetail/PaneLeafView.swiftSources/Shellraiser/Features/WorkspaceDetail/SurfaceProgressBar.swiftSources/Shellraiser/Features/WorkspaceDetail/SurfaceTabButton.swiftSources/Shellraiser/Infrastructure/Ghostty/GhosttyRuntime.swiftSources/Shellraiser/Infrastructure/Ghostty/LibghosttySurfaceView.swiftSources/Shellraiser/Models/SurfaceProgressReport.swiftSources/Shellraiser/Services/Workspaces/WorkspaceManager+SurfaceOperations.swiftSources/Shellraiser/Services/Workspaces/WorkspaceManager.swiftTests/ShellraiserTests/GhosttyTerminalViewTests.swift
Comment on lines
+10
to
+15
| struct SurfaceProgressReport: Equatable { | ||
| /// Current progress state. | ||
| let state: SurfaceProgressState | ||
| /// Percentage completion (0–100), or nil when not quantified. | ||
| let progress: UInt8? | ||
| } |
There was a problem hiding this comment.
Enforce the documented 0–100 invariant for progress.
UInt8 permits 101...255, but the model contract says 0–100. That allows invalid state into UI/accessibility paths.
🔧 Proposed fix
struct SurfaceProgressReport: Equatable {
/// Current progress state.
let state: SurfaceProgressState
/// Percentage completion (0–100), or nil when not quantified.
let progress: UInt8?
+
+ init(state: SurfaceProgressState, progress: UInt8?) {
+ self.state = state
+ self.progress = progress.map { min($0, 100) }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Shellraiser/Models/SurfaceProgressReport.swift` around lines 10 - 15,
SurfaceProgressReport currently allows progress values 101...255 because it's a
bare UInt8; add a validating initializer on SurfaceProgressReport (e.g.,
init(state: SurfaceProgressState, progress: UInt8?)) that checks if progress is
nil or within 0...100 and otherwise rejects/handles the value (use a failable
init returning nil or assert/precondition depending on desired behavior), and
make callers use this initializer so the invariant is enforced; reference
SurfaceProgressReport, its progress property, and SurfaceProgressState when
implementing the validation.
Sources/Shellraiser/Services/Workspaces/WorkspaceManager+SurfaceOperations.swift
Show resolved
Hide resolved
- Skip dispatch for unknown progress states instead of clearing progress - Clamp progress values to 100 at parse boundary and in model init - Guard surface existence in setProgressReport before writing state - Extract tintColor onto SurfaceProgressState to remove duplicate color mapping - Remove redundant host.update() call from acquireHostView - Extract progressAutoClearInterval constant for magic number 15 - Store onProgressReport in test mock; add closure-forwarding test
Combines OSC 9;4 progress reporting with in-terminal search overlay. Both features coexist: SurfaceCallbacks carries both onProgressReport and onSearchStateChange; GhosttyRuntime handles both PROGRESS_REPORT and START/END/TOTAL/SELECTED_SEARCH actions; PaneLeafView renders both the progress bar and search overlay in the terminal ZStack.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Shellraiser/Services/Workspaces/WorkspaceManager+SurfaceOperations.swift (1)
159-164: Consider usingRunLoop.mainfor timer scheduling to ensure firing during UI tracking.
Timer.scheduledTimerschedules on the current run loop's default mode. If the user is actively scrolling or dragging (tracking mode), the timer won't fire. This is likely acceptable for a 15-second auto-clear, but worth noting.Optional: Schedule in common modes for guaranteed firing
- progressClearTimers[surfaceId] = Timer.scheduledTimer(withTimeInterval: Self.progressAutoClearInterval, repeats: false) { [weak self] _ in + let timer = Timer(timeInterval: Self.progressAutoClearInterval, repeats: false) { [weak self] _ in Task { `@MainActor` [weak self] in self?.progressBySurfaceId.removeValue(forKey: surfaceId) self?.progressClearTimers.removeValue(forKey: surfaceId) } } + RunLoop.main.add(timer, forMode: .common) + progressClearTimers[surfaceId] = timer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Shellraiser/Services/Workspaces/WorkspaceManager`+SurfaceOperations.swift around lines 159 - 164, The current Timer created in the progressClearTimers assignment uses Timer.scheduledTimer which attaches to the current run loop mode and may not fire during UI tracking; change to create a Timer with the same time interval and closure but explicitly add it to RunLoop.main in common modes so it fires during scrolling/dragging (use the same Self.progressAutoClearInterval, the same closure that removes entries from progressBySurfaceId and progressClearTimers, and store the resulting Timer back into progressClearTimers[surfaceId]).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@Sources/Shellraiser/Services/Workspaces/WorkspaceManager`+SurfaceOperations.swift:
- Around line 159-164: The current Timer created in the progressClearTimers
assignment uses Timer.scheduledTimer which attaches to the current run loop mode
and may not fire during UI tracking; change to create a Timer with the same time
interval and closure but explicitly add it to RunLoop.main in common modes so it
fires during scrolling/dragging (use the same Self.progressAutoClearInterval,
the same closure that removes entries from progressBySurfaceId and
progressClearTimers, and store the resulting Timer back into
progressClearTimers[surfaceId]).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 316d6fdd-bb10-4073-9300-90574c906234
📒 Files selected for processing (5)
Shellraiser.xcodeproj/project.pbxprojSources/Shellraiser/Features/Terminal/GhosttyTerminalView.swiftSources/Shellraiser/Features/WorkspaceDetail/PaneLeafView.swiftSources/Shellraiser/Infrastructure/Ghostty/GhosttyRuntime.swiftSources/Shellraiser/Services/Workspaces/WorkspaceManager+SurfaceOperations.swift
Use .common run loop mode so the timer fires during scroll/drag events, not just in .default mode which pauses when the run loop switches to .tracking mode.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Shellraiser/Services/Workspaces/WorkspaceManager`+SurfaceOperations.swift:
- Around line 173-178: focusedSurfaceProgress currently returns nil when
workspace.focusedSurfaceId is unset; change it to mirror other flows by falling
back to workspace.rootPane.firstActiveSurfaceId() before looking up
progressBySurfaceId. In other words, inside focusedSurfaceProgress use
workspace.focusedSurfaceId ?? workspace.rootPane.firstActiveSurfaceId() (or
equivalent) to compute the surface id, then return
progressBySurfaceId[surfaceId]; reference function focusedSurfaceProgress,
property workspace.focusedSurfaceId, method
workspace.rootPane.firstActiveSurfaceId(), and dictionary progressBySurfaceId
when making the change.
- Around line 159-166: The timer callback can race with a newer timer and clear
a freshly set report; when creating the Timer in setProgressReport, capture the
created Timer instance in the closure and, inside the `@MainActor` Task, compare
that captured timer to the current progressClearTimers[surfaceId] before
removing entries from progressBySurfaceId and progressClearTimers; only clear
state if they are identical to avoid deleting a newer report/timer (refer to
Timer, Self.progressAutoClearInterval, progressBySurfaceId, progressClearTimers
and the setProgressReport path).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b5ce9cf-3b63-4524-afee-9a934886531f
📒 Files selected for processing (1)
Sources/Shellraiser/Services/Workspaces/WorkspaceManager+SurfaceOperations.swift
Sources/Shellraiser/Services/Workspaces/WorkspaceManager+SurfaceOperations.swift
Show resolved
Hide resolved
Sources/Shellraiser/Services/Workspaces/WorkspaceManager+SurfaceOperations.swift
Outdated
Show resolved
Hide resolved
- focusedSurfaceProgress now falls back to firstActiveSurfaceId() when focusedSurfaceId is nil, matching the pattern used by other surface lookup functions - Replace timer identity check (unsound due to Timer not being Sendable and forward-reference restrictions) with a generation counter; stale timer callbacks are now ignored if a newer timer has been registered for the same surface
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.
Receives GHOSTTY_ACTION_PROGRESS_REPORT from libghostty and renders a 2px progress bar at the top of the active terminal surface. Tab dots reflect the progress state (accent/red/orange). State is auto-cleared after 15 seconds of inactivity, matching Ghostty's own behavior.
Summary by CodeRabbit