Menu: let metric meta and reset rows wrap instead of truncating - #2742
Conversation
![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: 0c4430a8d7
ℹ️ 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".
| .lineLimit(2) | ||
| .truncationMode(.tail) | ||
| .fixedSize(horizontal: false, vertical: true) |
There was a problem hiding this comment.
When an already-populated metric changes from a short countdown or pace string to a longer one—for example, when a forecast is appended after refresh—these lines now change the card's height. However, Metric.heightFingerprint in MenuCardHeightFingerprint.swift records only whether reset/meta text is present, so makeMenuCardItem can reuse the old measured frame for the same scope and width; the newly added second line is then clipped, defeating this fix until another fingerprint field changes. Include the reset/meta text shape or content in the metric height fingerprint now that these fields can wrap.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed August 8, 2026, 1:51 AM ET / 05:51 UTC. ClawSweeper reviewWhat this changesThis PR allows compact usage-card reset and pace metadata to wrap to two lines and adds their content plus the adjacent title to the card-height cache fingerprint. Regression provenancePossible regression — suspected (reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 3 items remain The patch remains necessary: current main still limits compact reset and pace text to one line. The latest head addresses the matching layout and height-cache path; native-menu proof is still needed before merge. Priority: P3 Review scores
Verification
How this fits togetherCodexBar maps provider usage snapshots into compact SwiftUI menu-card rows, measures them for AppKit, and caches their geometry across refreshes. Reset and pace strings affect both row layout and required card height. flowchart LR
A[Provider usage snapshot] --> B[Metric presentation]
B --> C[Compact metric row]
C --> D[AppKit height measurement]
D --> E[Fingerprint height cache]
E --> F[Menu-bar usage card]
Before merge
Agent review detailsSecurityNone. Review metricsNone. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the bounded two-line layout and merge after a redacted native-menu screenshot or recording visibly shows the formerly truncated reset or pace text without clipping. Do we have a high-confidence way to reproduce the issue? Yes, at source level: current main sets both compact strings to one line, and the branch's fixed-width Is this the best way to solve the issue? Yes; a two-line cap with corresponding height-cache invalidation is the narrowest way to preserve the compact design while keeping localized text visible. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8fc67d7f04ad. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (6 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
Compact usage detail rows (steipete#2620) put all pace detail on one line and moved reset times into the title row; longer locales (e.g. Chinese) truncate tail content. Allow the meta line and reset label to wrap to two lines so all information stays visible while keeping the compact layout.
0c4430a to
a022b65
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a022b65014
ℹ️ 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".
| presentation.metaText == nil ? "meta=0" : "meta=1", | ||
| // Reset and meta rows may wrap to a second line, so their text | ||
| // content (not just presence) can change the measured card height. | ||
| MenuCardHeightFingerprint.field("reset", presentation.resetText), |
There was a problem hiding this comment.
When a stable reset label sits near the wrapping threshold, a width-changing title update—such as 9% left becoming 100% left, or a longer dynamic window title—reduces the space offered to the reset text in the same HStack and can change it from one line to two. This fingerprint still hashes only reset/meta content and intentionally ignores percentage changes, so the old one-line cached height is reused and the new second line can be clipped. Fresh evidence in this revision is that reset wrapping was introduced while the cache key still omits presentation.titleText; include that width-affecting text in the fingerprint as well.
Useful? React with 👍 / 👎.
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Co-authored-by: Yuxin Qiao <104957188+Yuxin-Qiao@users.noreply.github.com>
clawsweeper
Bot
removed
the
merge-risk: 🚨 other
|
Landed. Verified before merge: wrapping is bounded to one extra line at the standard 296pt width; the #2674 hover-deferral pattern is preserved (tracked-parent structural rebuilds stay deferred). A maintainer fix ( |
2 participants
Summary
lineLimit(1)meta line and moved the reset time into the title row. In longer locales (e.g. Chinese) the tail content truncates: "距重置…" cuts off the reset countdown, and "重置于 8月14日 晚…" never shows the time.Metric.heightFingerprint), so a short-to-long refresh re-measures the card instead of reusing a cached one-line frame that clips the second line.Test
swift test --filter MenuCardHeightFingerprintTestsand--filter UsageMenuCardLayoutTests: 13/13 green (live output below).make teston the rebased head: 69/69 groups green (828 selections), 0 retries.make check: 0 violations in 1804 files.Live behavior proof
Focused run on this PR head (
a022b6501), showing both the wrapping layout behavior and the height-cache invalidation:metric detail wraps to a second row instead of truncating as pace content growsmeasures the rendered card viasizeThatFitsat 296pt width: the long localized meta line grows the card by roughly one text line (asserted > heightTolerance and < 20pt), i.e. the second line renders instead of truncating. The fingerprint tests verify the cached height is invalidated whenever the wrapping reset/meta text changes, so the grown card actually gets its new frame on refresh.Rebase
main(cee0fd074); the rebased branch keeps the current percentage-label semantics. Branch is mergeable (no conflicts).