Address review feedback and plan changes#3
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements and bug fixes, including adaptive dark mode colors for the menu bar icon, safe handling of duplicate limit IDs using dictionary uniquing keys, and immediate acceptance of usage limits upon a plan change. Additionally, TimelineView in UsageMenuView is replaced with a periodic task, and timeout.cancel() is simplified using defer. The reviewer feedback suggests simplifying the SwiftUI .task implementation by removing redundant isVisible state tracking, and warns that using localizedCapitalized might incorrectly lowercase acronyms like 'API' to 'Api'.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| .onDisappear { | ||
| isVisible = false | ||
| } | ||
| .task(id: isVisible) { | ||
| guard isVisible else { return } | ||
| while !Task.isCancelled { | ||
| try? await Task.sleep(for: .seconds(1)) | ||
| guard !Task.isCancelled else { break } | ||
| now = Date() | ||
| } | ||
| } |
There was a problem hiding this comment.
The .task modifier in SwiftUI is automatically started when the view appears and cancelled when it disappears. Therefore, managing the isVisible state manually via .onAppear and .onDisappear is redundant and adds unnecessary complexity. We can simplify this by removing the isVisible state variable entirely and letting .task handle the lifecycle automatically.
.task {
while !Task.isCancelled {
guard (try? await Task.sleep(for: .seconds(1))) != nil else { break }
now = Date()
}
}| .split(separator: " ") | ||
| .map { $0.prefix(1).uppercased() + $0.dropFirst() } | ||
| .joined(separator: " ") | ||
| value.replacingOccurrences(of: "_", with: " ").localizedCapitalized |
There was a problem hiding this comment.
Using localizedCapitalized lowercases all characters in each word except the first one. If the input string contains acronyms or specific casing (e.g., API, CPU, OAuth), they will be incorrectly transformed (e.g., Api, Cpu, Oauth). If preserving the original casing of the remaining characters is desired, it is safer to stick to the previous manual capitalization approach.
value.replacingOccurrences(of: "_", with: " ")
.split(separator: " ")
.map { $0.prefix(1).uppercased() + $0.dropFirst() }
.joined(separator: " ")There was a problem hiding this comment.
Pull request overview
This PR updates CodexBar’s usage tracking and UI behavior to address prior review feedback, improve reliability of probing/polling, and handle plan changes without triggering false “reset” celebrations.
Changes:
- Adds plan-aware stabilization so usage is accepted immediately when the Codex plan type changes, and hardens stabilizer logic against duplicate limit IDs.
- Adjusts the menu UI update loop so the “last updated” timestamp only advances while the menu is visible, and ensures polling stops when the view model is released.
- Improves visuals (RTL marker placement, dark-mode flame contrast) and refines naming/threshold behavior (default severity naming + tests).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/CodexBarCoreTests/UsageSnapshotStabilizerTests.swift | Adds regression/plan-change/duplicate-ID coverage for snapshot stabilization. |
| Tests/CodexBarCoreTests/UsageModelsTests.swift | Adds tests for the default severity thresholds at boundary values. |
| Sources/CodexBarCore/UsageSnapshotStabilizer.swift | Adds plan-type awareness and de-dupes dictionary keying to avoid crashes on duplicate IDs. |
| Sources/CodexBarCore/UsageModels.swift | Renames severity to defaultSeverity and updates string humanization behavior. |
| Sources/CodexBar/UsageViewModel.swift | Stops polling when deallocated; integrates plan-aware stabilization and reseeds celebrations on plan change. |
| Sources/CodexBar/UsageMenuView.swift | Replaces TimelineView with a visibility-scoped task-driven clock update. |
| Sources/CodexBar/MenuBarIconRenderer.swift | Uses an appearance-aware flame color for improved dark-mode contrast. |
| Sources/CodexBar/LimitRowView.swift | Uses leading padding for the pace marker to better respect RTL layout direction. |
| Sources/CodexBar/CodexAppServerClient.swift | Switches to throwing pipe writes and changes timeout cancellation behavior. |
Comments suppressed due to low confidence (1)
Sources/CodexBarCore/UsageModels.swift:42
- Renaming a
publicAPI fromseveritytodefaultSeverityis source-breaking for any downstream clients ofCodexBarCore. If external consumers exist, consider keepingseverityas a deprecated shim (renamed todefaultSeverity) to preserve compatibility while still clarifying the naming.
public var defaultSeverity: Severity {
if percent >= 90 {
return .critical
}
if percent >= 75 {
return .warning
}
return .normal
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let timeout = DispatchWorkItem { | ||
| if process.isRunning { | ||
| process.terminate() | ||
| } | ||
| } | ||
| DispatchQueue.global(qos: .utility).asyncAfter(deadline: .now() + 15, execute: timeout) | ||
| defer { timeout.cancel() } | ||
|
|
![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: dfdfe277a1
ℹ️ 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".
| private static let height: CGFloat = 18 | ||
| static let strongFlameColor = NSColor(srgbRed: 0.75, green: 0.12, blue: 0.12, alpha: 1) | ||
| static let strongFlameColor = NSColor(name: nil) { appearance in | ||
| if appearance.bestMatch(from: [.darkAqua, .vibrantDark]) != nil { |
There was a problem hiding this comment.
In light appearances this condition can still be true because NSAppearance.bestMatch(from:) returns the closest name from the supplied candidates; with only dark candidates available, a light appearance can pick one of them. That makes the weekly flame use the brighter dark-mode red even in light mode, regressing the previous light-mode color. Include a light candidate such as .aqua/.vibrantLight and compare the result to a dark name instead.
Useful? React with 👍 / 👎.
What changed
Review context
Addresses the ten valid unresolved threads from #1. The
base64 --decodecomment was not changed because the livemacos-15release run successfully imported the certificate, signed the app, and completed notarization.Validation