Add conditional tokens to the menu bar layout editor - #3076
Conversation
- Add required `name` field to `MenuBarLayoutConditional` with backward-compatible Codable (legacy data decodes to empty string) - Add `.hidden` token that renders nothing, used as a conditional branch value to suppress output - Chips in the palette now show the conditional's display name instead of the full summary - Conditional editor enforces a non-empty unique name with inline validation - Add Duplicate action to conditional context menu with auto-generated unique names - Refactor palette section: add button moved to header row, empty-state placeholder when no conditionals exist, wider chip grid - Update all 20 locale files with new string keys - Add renderer and model tests for the new features
Move placed conditional tokens to reference-by-ID: layouts store a conditional's UUID and the library is the single source of truth, so library edits reach every placed copy and removal strips references from all layouts. Pre-resolve conditionals in the renderer so hidden branches leave no orphaned thin-space separators, key the title cache on the library, guard the edit sheet's clause bindings against stale indices, localize duplicate naming, and parenthesize mixed AND/OR summaries to match left-fold evaluation.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
![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: 48faf424a7
ℹ️ 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".
# Conflicts: # Sources/CodexBar/MenuBarLayout.swift # Sources/CodexBar/MenuBarLayoutEditor.swift # Sources/CodexBar/MenuBarLayoutRenderer.swift # Sources/CodexBar/SettingsStore.swift
The Settings scene exists only to own the app-menu Settings command; the real settings window is AppKit-managed. macOS could present or state-restore the scene's empty placeholder window at startup.
wdmitchelluk
changed the title
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs changes before merge. Reviewed August 19, 2026, 5:54 PM ET / 21:54 UTC. ClawSweeper reviewWhat this changesAdds named, reusable conditional rules to the menu-bar layout editor so usage thresholds can show, replace, or hide layout tokens. Merge readinessKeep open: the packaged-bundle proof is convincing, but the branch duplicates an existing main-branch Settings-window guard and still permits visually duplicate conditional names; adopting this persisted feature also needs VISION sign-off. Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — execution before step 1 Assertions:
How this fits togetherThe menu-bar layout editor stores user token arrangements and settings in local preferences, then the status-item renderer turns them into the visible menu-bar title. This PR adds a conditional-rule library between those persisted layouts and the renderer. flowchart LR
A[Usage snapshots] --> B[Conditional rule library]
C[Saved menu bar layout] --> D[Layout editor]
D --> C
B --> E[Layout renderer]
C --> E
A --> E
E --> F[Menu bar title and accessibility label]
Decision needed
Why: VISION.md requires explicit sign-off for new features and behavior that affects stored data; implementation correctness cannot determine whether this added configuration surface is desired. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Reuse the existing placeholder-window guard, normalize duplicate names consistently, then land the conditional library only if a maintainer sponsors its persisted configuration contract. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds a feature rather than repairing a reported failure; its packaged-bundle screenshot and runtime log directly demonstrate the proposed behavior. Is this the best way to solve the issue? No: the conditional implementation is otherwise focused, but it should not add a second placeholder-window handler and needs maintainer approval for its persistent configuration surface. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e7779924f98a. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What 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 (3 earlier review cycles)
|
The dual-write path promises the legacy blob stays decodable by the 0.53.x token surface, but .conditional and .hidden have no case there, so an older decoder dropped the whole saved layout or override. Filter tokens with no legacy representation out of the projection instead of mapping them, drop a line emptied purely by that filtering, and fall back to the default layout when nothing remains.
Resolving a conditional to .hidden removed the token but kept its line, so a stacked layout still emitted that line's newline, held stacked typography, and announced a blank line to VoiceOver. Drop resolved-empty lines before deriving stacking and separators, and keep the debug marker safe when every line collapses.
A plain HStack pushed later chips outside the settings pane once the library grew or names got long, and an adaptive LazyVGrid stretched the chips into equal columns with large gaps. Add a leading-aligned wrapping flow layout that places each chip at its natural width, with the row packing rule covered by a test.
A fresh install showed an empty Conditionals palette, leaving the feature undiscoverable until someone hand-built a rule. Seed five editable conditionals covering the common cases. Identities are fixed so a placed reference keeps resolving across launches, and seeding only happens when no library key exists, so an emptied library is never reseeded.
Captures the packaged-bundle behavior behind this PR: the seeded starter library in the editor, the legacy layout blob staying free of conditional tokens, and a hidden branch collapsing its line in the rendered title.
|
All three actionable findings are fixed, each in its own commit, and the proof ask is satisfied. Head is now
One extra change while in here ( Notes on two items in the checklist:
On the failed live verification: Verification: 562 tests in 32 suites pass across the layout, settings-store, and status-item suites, and @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@codex review |
|
Codex review is not enabled for this repo. Please contact the admins of this repo to enable Codex. |
clawsweeper
Bot
added
proof: sufficient
Two problems made the macOS test shard fail: Restore two '// Provider-specific by design:' markers for the Mistral lanes in the layout preview. The branch's first commit deleted them, so the gatekeeper saw unjustified provider-specific clusters. Realign five allowlist line anchors that this branch shifted, in MenuBarLayout, MenuBarLayoutEditor, SettingsStore, and StatusItemController+MenuBarLayout.
|
CI update for The earlier red gate (
I audited all 391 allowlist/suppression anchors in the gatekeeper against their source files rather than fixing them one failure at a time; all 391 now resolve exactly. Local verification: 600 tests across 33 suites pass ( The one remaining red job is unrelated infrastructure
That it is not this branch:
I do not have re-run rights on this repo, and the workflow has no |
Two behaviors this branch introduced were unproven: a conditional whose branch resolves to .icon in first position has to reach button.image the same way a literal .icon token does, since AppKit only dims that path on inactive displays; and a line the user left empty must survive the older-readable projection, unlike one emptied by token filtering.
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
CI is green on
The push that re-triggered the run also closes two edges my earlier commits introduced but left unproven, rather than being an empty re-trigger:
Both behaviors were already correct; they are now pinned by tests. Local verification: 601 tests across 33 suites pass, and |
2 participants
Summary
Adds conditional tokens to the menu bar layout editor: named, reusable if/then/else rules that decide what the menu bar shows based on live usage. Example: show the session percentage only once it crosses 50%, and nothing (or a countdown) below that.
Found under Settings → Menu bar layout → new "Conditionals" palette group.
What it does
Session / Weekly / Model-scoped weekly / Autometric,> >= < <=comparison, 0–100% threshold) joined by AND/OR, plus a then token and an else token. Thresholds compare the window's used percentage.Hide, rendering nothing — including its would-be separator spacing, and including its whole line if that leaves the line empty.(Session > 10% or Weekly > 20%) and Auto > 30%) so it reads exactly as evaluated.Design
{"conditional":{"id":…}}); the library is the single source of truth. Editing a library entry updates every placed copy immediately (the render cache keys on the library), and removing one strips its references from the global layout and all provider overrides..conditionaland.hiddenhave no case in the 0.53.x token surface, so the legacy dual-write projection drops them rather than emitting a token an older decoder would choke on. A line emptied by that filtering is dropped too, and a layout with nothing left falls back todefaultLayout. Both the global and provider-override paths are covered.–placeholder with a localized accessibility label.MenuBarLayoutChipFlowLayout) rather than an adaptive grid: chips are user-named and variable width, so adaptive columns either stretched them into equal columns with large gaps or, as a plainHStack, pushed later chips out of the pane.%@ (copy),%1$@ (copy %2$d)), the shipped rule names, and all new UI strings are localized across the 23 catalogs.Proof
Packaged release bundle (
Scripts/package_app.sh), driven through the real UI. Artifacts:.github/pr-proof/menubar-conditionals-editor.png,.github/pr-proof/menubar-conditionals-runtime.log.The screenshot shows, in one shot:
Session over 50% used, on line 2 of the strip);87%row — line 2's conditional resolved to.hidden, so the whole line collapsed.Downgrade projection, read back out of the running app's
UserDefaults:The legacy blob carries no
conditionaltoken, so an older release decodes the layout instead of discarding the whole blob.Hidden-line collapse, read off the live status item:
One line announced — no
Line 2segment, no blank trailing line, single-line typography.Testing
swift test --filter 'MenuBarLayout|MenuBar|SettingsStore|StatusItem|MenuLayout'— 562 tests in 32 suites pass.conditional and hidden tokens drop out of the older-readable projection,conditional layouts dual-write an older-readable fallback(global + override + reload),a line emptied by a hidden branch collapses instead of rendering blank,every line hidden renders an empty title without crashing,conditional palette chips wrap instead of overflowing the pane,a fresh install ships an editable conditionals library,clearing the shipped conditionals library survives a reload."5h 25%\u{2009}W 60%"), dangling references, depth cap, library-edit cache invalidation, copy naming, summary parenthesization, and library-removal reference stripping across reload.make checkclean (SwiftFormat + SwiftLint--strict+ locale catalogs: 22 catalogs against 1472 English keys)../.build/release/CodexBarCLI --helpexits 0 locally — the review bot'sswift run CodexBarCLI --helpfailure was its sandbox failingpnpm install(corepack download) before reaching the build.main(lane tokens integrated).Maintainer decision still open
VISION.md requires sign-off for new features and for changes that affect persisted data, and this adds both a feature and persisted global configuration (the conditionals library). That call is yours — the code-level blockers from review are addressed above, but I have not assumed the product decision.