Conversation
|
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:
📝 WalkthroughWalkthroughAdds persisted appearance selection, adaptive settings styling, local Codex task-state parsing and display, navigation controls, localization and documentation updates, parser tests, and Sparkle signing cleanup. ChangesCodex status and appearance
Packaging signing preparation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsView
participant CodexTaskStatusStore
participant CodexTaskStatusLogParser
participant CodexTaskStatusView
participant CodexApp
User->>SettingsView: Enable Codex task status
SettingsView->>CodexTaskStatusStore: Persist status preferences
CodexTaskStatusStore->>CodexTaskStatusLogParser: Parse recent JSONL events
CodexTaskStatusLogParser-->>CodexTaskStatusStore: Return task state
CodexTaskStatusStore-->>CodexTaskStatusView: Publish task snapshot
User->>CodexTaskStatusView: Select status card
CodexTaskStatusView->>CodexTaskStatusStore: openThread()
CodexTaskStatusStore->>CodexApp: Open Codex thread
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 982ae9ec87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let maxBytes: UInt64 = 512 * 1024 | ||
| try? handle.seek(toOffset: length > maxBytes ? length - maxBytes : 0) |
There was a problem hiding this comment.
For a single long-running turn whose rollout grows beyond 512 KiB, this seek discards the initial task_started/user_message events; if the retained tail contains only token-count, reasoning, or response_item records, parseState starts from .idle and never changes it, so an active task is reported as idle. Preserve the last lifecycle state separately or include the relevant initial events when parsing a truncated file.
Useful? React with 👍 / 👎.
| timer = Timer.scheduledTimer(withTimeInterval: 2, repeats: true) { [weak self] _ in | ||
| Task { @MainActor in self?.refresh() } |
There was a problem hiding this comment.
With the setting enabled by default, this timer runs even when Claude is visible and the task-status UI cannot be shown; every tick recursively enumerates the entire sessions tree and reparses as much as 24 × 512 KiB of unchanged JSONL data. Established Codex installations therefore incur continuous filesystem and JSON-processing work for a hidden feature, so polling should be gated by visibility/file changes or cache unchanged modification dates.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Sources/Views/PanelFooter.swift (1)
74-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCommand-key hint may not read clearly via VoiceOver.
Image(systemName: "command")typically has no built-in accessibility label, so combining it with the plain-text hint under.accessibilityElement(children: .combine)may drop the "Command" cue for VoiceOver users, reading as e.g. "click cycle view" without conveying the modifier key. Consider an explicit.accessibilityLabelon the combined element (e.g. "Command + (keys), (label)").🤖 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/Views/PanelFooter.swift` around lines 74 - 84, The combined accessibility element in shortcutHint must explicitly convey the Command modifier, since the command icon may be omitted by VoiceOver. Add an accessibility label to the combined HStack that includes “Command,” the localized keys text, and the localized label while preserving the existing visual layout.Sources/Window/IslandWindowController.swift (1)
160-165: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueHardcoded
["1","2","3"]is only safe whileScreenPref.Screenhas exactly 3 cases.
ScreenPref.Screen.allCases[index]will crash if the enum's case count ever diverges from this literal array (e.g. a 4th screen added later without updating this line). Currently safe since there are exactly 3 cases, but bounding the index (or deriving the digit set fromallCases.count) would make this refactor-proof.🛡️ Illustrative guard
if modifiers == .command, let character = event.charactersIgnoringModifiers, - let index = ["1", "2", "3"].firstIndex(of: character) { + let index = ["1", "2", "3"].firstIndex(of: character), + index < ScreenPref.Screen.allCases.count { model.showScreen(ScreenPref.Screen.allCases[index]) return nil }🤖 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/Window/IslandWindowController.swift` around lines 160 - 165, Update the command-number handling in the event method around ScreenPref.Screen.allCases so the parsed digit index is validated against allCases.count before indexing. Preserve the existing behavior for valid screen shortcuts and return without indexing when the digit does not correspond to an available screen.
🤖 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/Model/CodexTaskStatusStore.swift`:
- Around line 97-103: Reduce the continuous scan cost driven by start() and its
timer callback: replace the 2-second polling cadence with a substantially
longer, reasonable refresh interval and narrow recentRolloutFiles() so it avoids
recursively enumerating the entire sessions tree, or trigger refresh only on
filesystem changes using an appropriate watcher. Preserve the existing refresh
behavior while ensuring scan() and its tail-reads are not repeatedly executed
when no session files changed.
- Around line 105-126: Keep the existing com.openai.codex bundle identifier in
openCodexApp, but replace the silent failure when urlForApplication returns nil
or opening the app fails with a user-visible fallback or lightweight
notification using clear wording. Preserve the current app-opening behavior when
the application is installed.
In `@Sources/Views/IslandRootView.swift`:
- Around line 464-466: Update the elapsedUpdate label in IslandRootView to
refresh periodically while visible by driving its Date-dependent value from a
SwiftUI timeline or equivalent periodic view update. Preserve the existing
missing-date placeholder and Duration.compact formatting, and scope the refresh
to the visible peek rather than introducing a global timer.
In `@Sources/Views/Settings/StyleTile.swift`:
- Around line 21-23: Update the selected-state branch of the foregroundStyle
expression in StyleTile to use an adaptive semantic foreground color instead of
the fixed pale-blue Color value. Preserve the unselected styling and existing
selection visuals while ensuring selected text remains readable in Light mode.
---
Nitpick comments:
In `@Sources/Views/PanelFooter.swift`:
- Around line 74-84: The combined accessibility element in shortcutHint must
explicitly convey the Command modifier, since the command icon may be omitted by
VoiceOver. Add an accessibility label to the combined HStack that includes
“Command,” the localized keys text, and the localized label while preserving the
existing visual layout.
In `@Sources/Window/IslandWindowController.swift`:
- Around line 160-165: Update the command-number handling in the event method
around ScreenPref.Screen.allCases so the parsed digit index is validated against
allCases.count before indexing. Preserve the existing behavior for valid screen
shortcuts and return without indexing when the digit does not correspond to an
available screen.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 40d2f099-0819-4f91-88de-78c549cfafe8
📒 Files selected for processing (26)
README.mdREADME.zh-CN.mdResources/en.lproj/Localizable.stringsResources/zh-Hans.lproj/Localizable.stringsSources/App.swiftSources/Model/AppearanceStore.swiftSources/Model/CodexTaskStatusStore.swiftSources/Theme/Colors.swiftSources/Views/CodexTaskStatusView.swiftSources/Views/IslandRootView.swiftSources/Views/PageIndicator.swiftSources/Views/PanelFooter.swiftSources/Views/PanelHeader.swiftSources/Views/Settings/BrandHeader.swiftSources/Views/Settings/ChartStylePicker.swiftSources/Views/Settings/CostStylePicker.swiftSources/Views/Settings/SegmentedControl.swiftSources/Views/Settings/SettingsFooter.swiftSources/Views/Settings/SettingsRow.swiftSources/Views/Settings/SettingsToggle.swiftSources/Views/Settings/StyleTile.swiftSources/Views/SettingsView.swiftSources/Views/SettingsWindowController.swiftSources/Views/UsageView.swiftSources/Window/IslandWindowController.swiftbuild.sh
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Model/CodexTaskStatusStore.swift (1)
269-272: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve error status after
task_complete.
parseState(at:)resets all status toidlefortask_complete, so a sequence likeerrorthentask_completereports a failed turn as idle. Track the current turn’s terminal failure state and keep.erroron completion unless a new run starts.🤖 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/Model/CodexTaskStatusStore.swift` around lines 269 - 272, Update parseState(at:) so task_complete preserves an existing .error status instead of unconditionally setting .idle. Track the current turn’s terminal failure state across error, stream_error, and turn_aborted events, and clear it only when a new run starts.
🤖 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.
Outside diff comments:
In `@Sources/Model/CodexTaskStatusStore.swift`:
- Around line 269-272: Update parseState(at:) so task_complete preserves an
existing .error status instead of unconditionally setting .idle. Track the
current turn’s terminal failure state across error, stream_error, and
turn_aborted events, and clear it only when a new run starts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9776230c-0151-464f-915e-34628729e608
📒 Files selected for processing (5)
Resources/en.lproj/Localizable.stringsResources/zh-Hans.lproj/Localizable.stringsSources/Model/CodexTaskStatusStore.swiftSources/Views/IslandRootView.swiftSources/Views/Settings/StyleTile.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Resources/en.lproj/Localizable.strings
- Resources/zh-Hans.lproj/Localizable.strings
- Sources/Views/IslandRootView.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Model/CodexTaskStatusStore.swift (1)
253-281: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve terminal errors across the tail boundary.
parseStatestartscurrentTurnFailedasfalse, buttailData(at:)can begin in the middle of a JSONL record or after the precedingerror/turn_abortedevent. Iftask_completeis present in the suffix, the task is then reported as.idle, losing the terminal error.Read from a complete line boundary and ensure the full current turn—or an equivalent persisted terminal-error marker—is available before applying
task_complete. Add a regression test for a failure immediately before or outside the 512 KiB window.Also applies to: 295-301
🤖 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/Model/CodexTaskStatusStore.swift` around lines 253 - 281, Update parseState and its tailData(at:) input handling so parsing begins at a complete JSONL line boundary and retains the current turn’s terminal failure state when the preceding error, turn_aborted, or stream_error event falls outside the 512 KiB suffix. Ensure task_complete reports .error rather than .idle when that persisted failure marker applies, and add a regression test covering a failure immediately before or outside the tail window.
🤖 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.
Outside diff comments:
In `@Sources/Model/CodexTaskStatusStore.swift`:
- Around line 253-281: Update parseState and its tailData(at:) input handling so
parsing begins at a complete JSONL line boundary and retains the current turn’s
terminal failure state when the preceding error, turn_aborted, or stream_error
event falls outside the 512 KiB suffix. Ensure task_complete reports .error
rather than .idle when that persisted failure marker applies, and add a
regression test covering a failure immediately before or outside the tail
window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b8c95d96-423b-4014-a39a-80a6c95175f2
📒 Files selected for processing (1)
Sources/Model/CodexTaskStatusStore.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/CodexTaskStatusLogParserTests.swift (1)
34-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the normal status transitions too.
These cases only validate preserved errors. Add table-driven assertions for running, approval, user-input, idle completion, and a new
task_started/user_messageclearing a prior error; otherwise regressions in the primary displayed states pass unnoticed.🤖 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 `@Tests/CodexTaskStatusLogParserTests.swift` around lines 34 - 54, Extend the tests around CodexTaskStatusLogParser.parse with table-driven cases covering running, approval, user-input, idle completion, and task_started/user_message clearing a prior error. Keep the existing preserved-error cases, and assert each scenario’s expected parser status so normal transitions and error recovery are validated.
🤖 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/Model/CodexTaskStatusLogParser.swift`:
- Around line 20-21: Bound CodexTaskStatusLogParser I/O to maxBytes: update
completeLineStart and failureMarker so oversized JSONL records and
failure-marker lookups do not scan or reread unbounded session history. Preserve
failure state incrementally by file/offset, or limit backward scanning to the
current turn’s reset/failure marker, and ensure the fallback path is capped at
the configured tail size.
---
Nitpick comments:
In `@Tests/CodexTaskStatusLogParserTests.swift`:
- Around line 34-54: Extend the tests around CodexTaskStatusLogParser.parse with
table-driven cases covering running, approval, user-input, idle completion, and
task_started/user_message clearing a prior error. Keep the existing
preserved-error cases, and assert each scenario’s expected parser status so
normal transitions and error recovery are validated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 69713819-8688-4969-aa60-44dbdeffdb4d
📒 Files selected for processing (4)
Sources/Model/CodexTaskStatusLogParser.swiftSources/Model/CodexTaskStatusStore.swiftTests/CodexTaskStatusLogParserTests.swiftscripts/run-tests.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Model/CodexTaskStatusLogParser.swift`:
- Around line 38-83: Update parse(at:) to detect when completeLines returns no
data and zero consumedBytes because no complete line boundary was available on a
fresh, non-continuing read. Return nil instead of parsing and returning the
hardcoded initial .idle state; preserve cached-state continuation behavior and
normal parsing when complete data is available.
- Around line 15-36: Add a thread-safe removal method to StateCache for deleting
an entry by URL, then invoke it from the store whenever a session file is no
longer tracked, including rotation and task removal from the aggregation set.
Preserve the existing locking pattern used by entry(for:) and set(_:for:) to
keep cache access synchronized.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ba4d6c7-80c7-4198-a49a-fa8e588516cf
📒 Files selected for processing (2)
Sources/Model/CodexTaskStatusLogParser.swiftTests/CodexTaskStatusLogParserTests.swift
| private struct CacheEntry { | ||
| let offset: UInt64 | ||
| let state: CodexTaskLogState | ||
| let currentTurnFailed: Bool | ||
| } | ||
|
|
||
| private final class StateCache: @unchecked Sendable { | ||
| private let lock = NSLock() | ||
| private var entries: [URL: CacheEntry] = [:] | ||
|
|
||
| func entry(for url: URL) -> CacheEntry? { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return entries[url] | ||
| } | ||
|
|
||
| func set(_ entry: CacheEntry, for url: URL) { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| entries[url] = entry | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
StateCache has no eviction path — unbounded growth over the app's lifetime.
StateCache only exposes entry(for:)/set(_:for:); there is no way to remove an entry. Since Codex creates a new session .jsonl file per task, this dictionary grows for as long as the app runs, with no bound.
♻️ Proposed fix: expose a removal path for stale sessions
private final class StateCache: `@unchecked` Sendable {
private let lock = NSLock()
private var entries: [URL: CacheEntry] = [:]
func entry(for url: URL) -> CacheEntry? {
lock.lock()
defer { lock.unlock() }
return entries[url]
}
func set(_ entry: CacheEntry, for url: URL) {
lock.lock()
defer { lock.unlock() }
entries[url] = entry
}
+
+ func remove(for url: URL) {
+ lock.lock()
+ defer { lock.unlock() }
+ entries.removeValue(forKey: url)
+ }
}Have the store call this when a session file is no longer tracked (rotated away, task removed from the aggregation set).
📝 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.
| private struct CacheEntry { | |
| let offset: UInt64 | |
| let state: CodexTaskLogState | |
| let currentTurnFailed: Bool | |
| } | |
| private final class StateCache: @unchecked Sendable { | |
| private let lock = NSLock() | |
| private var entries: [URL: CacheEntry] = [:] | |
| func entry(for url: URL) -> CacheEntry? { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| return entries[url] | |
| } | |
| func set(_ entry: CacheEntry, for url: URL) { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| entries[url] = entry | |
| } | |
| } | |
| private struct CacheEntry { | |
| let offset: UInt64 | |
| let state: CodexTaskLogState | |
| let currentTurnFailed: Bool | |
| } | |
| private final class StateCache: `@unchecked` Sendable { | |
| private let lock = NSLock() | |
| private var entries: [URL: CacheEntry] = [:] | |
| func entry(for url: URL) -> CacheEntry? { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| return entries[url] | |
| } | |
| func set(_ entry: CacheEntry, for url: URL) { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| entries[url] = entry | |
| } | |
| func remove(for url: URL) { | |
| lock.lock() | |
| defer { lock.unlock() } | |
| entries.removeValue(forKey: url) | |
| } | |
| } |
🤖 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/Model/CodexTaskStatusLogParser.swift` around lines 15 - 36, Add a
thread-safe removal method to StateCache for deleting an entry by URL, then
invoke it from the store whenever a session file is no longer tracked, including
rotation and task removal from the aggregation set. Preserve the existing
locking pattern used by entry(for:) and set(_:for:) to keep cache access
synchronized.
| static func parse(at url: URL, maxBytes: UInt64 = 512 * 1024) -> CodexTaskLogState? { | ||
| guard maxBytes > 0, | ||
| let handle = try? FileHandle(forReadingFrom: url) | ||
| else { return nil } | ||
| defer { try? handle.close() } | ||
|
|
||
| let length = (try? handle.seekToEnd()) ?? 0 | ||
| let cached = cache.entry(for: url) | ||
| let canContinue = cached.map { | ||
| length >= $0.offset && length - $0.offset <= maxBytes | ||
| } ?? false | ||
| let readStart: UInt64 | ||
| let initialState: CodexTaskLogState | ||
| let initialFailure: Bool | ||
| if canContinue, let cached { | ||
| readStart = cached.offset | ||
| initialState = cached.state | ||
| initialFailure = cached.currentTurnFailed | ||
| } else { | ||
| readStart = length > maxBytes ? length - maxBytes : 0 | ||
| initialState = .idle | ||
| initialFailure = false | ||
| } | ||
|
|
||
| try? handle.seek(toOffset: readStart) | ||
| let readLimit = Int(min(maxBytes, UInt64(Int.max))) | ||
| guard let raw = try? handle.read(upToCount: readLimit) else { return nil } | ||
| let complete = completeLines( | ||
| in: raw, | ||
| droppingLeadingPartialLine: !canContinue && readStart > 0 | ||
| ) | ||
| let result = parse( | ||
| complete.data, | ||
| initialState: initialState, | ||
| currentTurnFailed: initialFailure | ||
| ) | ||
| cache.set( | ||
| CacheEntry( | ||
| offset: readStart + UInt64(complete.consumedBytes), | ||
| state: result.state, | ||
| currentTurnFailed: result.currentTurnFailed | ||
| ), | ||
| for: url | ||
| ) | ||
| return result.state | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Oversized tail-window record can silently report a stale .idle state.
When the tail read (raw, bounded by maxBytes) is dominated by a single oversized JSONL record such that the window contains only that record's own terminating newline, completeLines hits the lastNewline >= lowerBound guard failure and returns (Data(), 0). parse(at:) then returns initialState unchanged, which on a fresh (non-continuing) read is hardcoded to .idle — even if the true state is .running, .waitingApproval, or .error. This self-heals once the file grows past the record, but until then the UI can show a misleading idle status. This is a residual gap in the oversized-record handling that the earlier review flagged (I/O is now bounded, but correctness for this specific case is not).
Consider returning nil (unknown/no-change) instead of defaulting to .idle when no complete line boundary could be found and there's no prior cached state to preserve, so callers keep the last known state rather than showing a fabricated idle.
🐛 Sketch of the fix
let complete = completeLines(
in: raw,
droppingLeadingPartialLine: !canContinue && readStart > 0
)
+ guard canContinue || complete.consumedBytes > 0 else {
+ // Tail window is entirely consumed by one oversized record with
+ // no other line boundary; report unknown rather than a
+ // fabricated `.idle` until the window advances past it.
+ return nil
+ }
let result = parse(Also applies to: 85-106
🤖 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/Model/CodexTaskStatusLogParser.swift` around lines 38 - 83, Update
parse(at:) to detect when completeLines returns no data and zero consumedBytes
because no complete line boundary was available on a fresh, non-continuing read.
Return nil instead of parsing and returning the hardcoded initial .idle state;
preserve cached-state continuation behavior and normal parsing when complete
data is available.
ericjypark
left a comment
ericjypark
left a comment
There was a problem hiding this comment.
Thanks for this contribution — there's real care in it. The appearance work is thorough (the .white → Color.primary conversion is complete across every settings surface, verified), the parser ships with tests, you responded to four rounds of bot feedback, and the bilingual docs are appreciated. The full-source typecheck and the test suite both pass on my machine, so the toolchain issue on your side didn't leak into the code.
That said, I can't merge this as one PR. It bundles four independent changes, and one of them needs a design-level conversation. Could you split it into separate PRs?
- Appearance (System/Light/Dark) —
AppearanceStore, theColor.primaryconversion,SettingsWindowController. This is close to mergeable as-is. - Keyboard navigation —
IslandWindowController,PageIndicator,PanelFooter. One fix needed (below), plus one product question. - build.sh xattr fix — 4 lines, mergeable immediately with a one-flag hardening (below).
- Codex task status — the big one; needs iteration before it's mergeable.
Splitting lets 1–3 land quickly instead of being held hostage by 4.
Blocking issues on the Codex task status feature
The event vocabulary doesn't match what current Codex writes to rollouts. I checked two months of real ~/.codex/sessions rollouts on a current CLI install (240+ turns): mcp_tool_call_end appears 667×, patch_apply_end 173×, but exec_command_begin, apply_patch_begin, mcp_tool_call_begin, exec_approval_request, apply_patch_approval_request, request_user_input, elicitation_request, and stream_error appear zero times — current rollouts record *_end lifecycle events, not *_begin, and no approval/input-request events at all. That means waitingApproval and waitingUserInput — the feature's two highest-priority states and its main pitch — can never fire, and the five-state design collapses to running/idle/error. Which Codex version emitted the events CodexTaskStatusLogParser listens for? If they're from an older format (or the app-server protocol rather than rollout files), the parser needs to be rebuilt around the vocabulary that rollouts actually contain, and the README claims adjusted to match.
A cancelled task pins the global status for up to 24h. turn_aborted (plain Esc — 6 occurrences in my logs) marks a file .error, and the aggregator ranks error (3) above running (2) with recency only as a tie-break (CodexTaskStatusStore.swift:49, :181). One cancelled session from this morning hides every currently-running session behind a red error glyph, and clicking deep-links to the dead thread. Completed-with-error files need to decay or rank below live running work.
Click behavior in compact/peek hijacks the island. The overlay is a Button, so with Claude hidden, clicking the left side of the island no longer expands the panel — it deep-links to the Codex app. For CLI-only users without com.openai.codex installed, every click produces a modal NSAlert (CodexTaskStatusStore.swift:135) — and the users whose rollouts this feature parses are exactly CLI users. Suggest: plain click keeps expanding the island everywhere; the deep link lives only on the expanded card.
The poller runs 24/7 for users who can never see it. start() is unconditional (App.swift:54), enabled defaults to true, and both providers default visible — but all three render surfaces require !claudeVisible && codexVisible. Default installs pay directory enumeration + up to 24 file stats every 15 s forever with zero render path, in an app that lives in the notch on battery. Please gate the timer on renderability (enabled && !claudeVisible && codexVisible, subscribing to both stores, with an immediate refresh when it flips true) — and default the feature off so existing hide-Claude users don't have their per-model breakdown replaced by an update.
False idle under growth and failure. If a rollout grows >512 KiB between polls (multi-MB screenshot/tool records are normal — see the comment in CodexLogReader.swift:63), the cache is abandoned and the tail parse seeds .idle, showing a running task as idle. Separately, every failure mode (unreadable dir, wrong CODEX_HOME, schema drift) also collapses to idle. An explicit unknown/unavailable state would stop failures from masquerading as "all quiet".
Smaller items, take or leave: StateCache never evicts (mirror LogParseCache.walk's visited-set prune) and has no file-identity check (rollouts can be rewritten — thread_rolled_back exists in real logs); a byte prefilter before JSONSerialization (the CodexLogReader.swift:70 marker pattern) would skip the bulk response_item/token_count lines cheaply; the two-day directory window misses sessions started 2+ days ago but still active; the state-machine mappings and truncation path have no test coverage, and the store's private static methods make it untestable — the CODEX_HOME override is a ready-made seam.
Quick fixes on the other PRs
- Keyboard: the new exact-match modifier check breaks ⌘Q with Caps Lock on (
IslandWindowController.swift:152) — and this app has no menu bar, so that's the only quit path. You already subtract.capsLockfor the arrow keys; do the same before the ⌘ comparisons. (Same fix makes ⌘1/2/3 survive layouts that shift digits.) Product note: replacing the one-shot "⌘-click to cycle" hint with permanent footer hints changes a deliberate quiet-chrome design — let's discuss in the split PR.hasCycledStylealso becomes write-only after this change; either remove it or keep the one-shot behavior. - build.sh:
xattr -cfollows symlinks; usexattr -cs(orfind -type f) so a vendored tree can never redirect the sweep outside the framework. - Localization: the
"click"/"1 2 3"hint keys aren't in either.stringsfile, so zh users see raw English; "Icon" display mode currently only affects one label on the expanded card — peek and header always render text.
Happy to review the split PRs quickly — the appearance and build ones especially should be fast merges.
|
已按 review 建议将这个综合 PR 拆分为四个独立、可单独审阅的 PR：
任务状态 PR 特意保持为 Draft，便于继续讨论设计和数据语义。原 PR 现由以上四个 PR 取代，感谢详细 review。 Following the review, this combined PR has been split into four independently reviewable PRs:
The task-status PR intentionally remains a draft for further design and data-semantics discussion. This PR is superseded by the four PRs above. Thank you for the detailed review. |
2 participants
Summary
Why / Design rationale
Not every user subscribes to both Claude and Codex. When someone uses only Codex and hides Claude, leaving half of the island empty wastes its most glanceable space.
This change turns that freed column into an optional, privacy-preserving Codex companion. It reads only local lifecycle events from
~/.codex/sessions/**/*.jsonland reduces them to five states; it does not display prompts, commands, or output, and it does not modify Codex configuration. The compact status group mirrors the Codex quota group, while the expanded view fills the otherwise unused provider column. The original per-model breakdown remains available when the feature is disabled.设计动机
并不是每位用户都会同时订阅 Claude 和 Codex。只使用 Codex 并隐藏 Claude 时，如果左半区一直留空，灵动岛中最适合随时查看的一块空间就没有得到利用。
因此，这次改动把空出的栏位设计成可选的 Codex 任务状态视图。它只读取
~/.codex/sessions/**/*.jsonl中的本地生命周期事件，并归纳为运行中、等待审批、等待用户输入、空闲和异常五种状态；不会展示提示词、命令或输出，也不会修改 Codex 配置。收起状态与右侧额度信息采用镜像布局，展开后填充原本空置的服务商栏位。关闭此功能后，仍会恢复原有的按模型 Token 用量视图。Validation
swiftc -frontend -parsepassed for all Swift sourcesLocalizable.stringspassedplutil -lintgit diff --checkpassed./scripts/run-tests.shcould not run in this local environment because the installed Swift compiler is 6.3.3 while the Command Line Tools SDK Swift interfaces are 6.3.2Summary by CodeRabbit