Skip to content

Defer closed menu rebuilds during refresh#1261

Merged
steipete merged 7 commits into
steipete:mainfrom
ProspectOre:codex/prepare-closed-menus-after-refresh
Jun 2, 2026
Merged

Defer closed menu rebuilds during refresh#1261
steipete merged 7 commits into
steipete:mainfrom
ProspectOre:codex/prepare-closed-menus-after-refresh

Conversation

@ProspectOre
Copy link
Copy Markdown
Contributor

@ProspectOre ProspectOre commented Jun 1, 2026

Summary

Prepare attached closed menus after refresh work settles, and keep stale nonempty menu content on open while store or token refreshes are active.

Context

Part of the UI hang/lag cleanup set: this keeps refresh-time menu rebuild work off the open-menu path.

Validation

  • swift test --filter StatusMenuOpenRefreshTests (25 tests passed)
  • make check
  • git diff --check

Runtime Proof

Runtime proof was captured from a freshly packaged debug build on June 1, 2026 at 00:05 PDT, driven through macOS Accessibility. Logs are redacted to lifecycle state, item counts, provider id, and refresh state only.

[DEBUG] menu open kept existing content during refresh items=13 provider=claude storeRefreshing=1
[DEBUG] closed menu rebuild completed items=17 provider=claude

Interpretation: while refresh was active, opening the stale nonempty menu kept the existing 13 items instead of rebuilding synchronously; after close and refresh settle, the closed-menu rebuild path populated the next version with 17 items.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

Codex review: needs maintainer review before merge. Reviewed June 1, 2026, 10:59 PM ET / 02:59 UTC.

Summary
The PR defers attached closed-menu rebuilds until refresh work settles, preserves stale nonempty menu content only for data-refresh invalidations, forces rebuilds for privacy/structure invalidations, and adds focused status-menu tests.

Reproducibility: yes. Current main’s source path rebuilds a stale menu synchronously on open, and the PR body provides after-fix runtime logs from a freshly packaged debug build showing stale content kept during refresh and rebuilt after refresh settles.

Review metrics: none identified.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] This read-only review did not run Swift tests or the packaged app; final merge should still wait for the repository’s required checks on the exact head.

Maintainer options:

  1. Decide the mitigation before merge
    Land the PR after required checks and maintainer review, keeping stale-menu reuse limited to data-refresh invalidations while privacy and structure changes force a fresh rebuild.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] No repair lane is needed because no concrete patch defect is evident; the remaining action is normal maintainer merge/check handling.

Security
Cleared: The diff touches menu scheduling, tests, and release-note text only; it does not add dependency, script, credential, permission, or supply-chain changes.

Review details

Best possible solution:

Land the PR after required checks and maintainer review, keeping stale-menu reuse limited to data-refresh invalidations while privacy and structure changes force a fresh rebuild.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main’s source path rebuilds a stale menu synchronously on open, and the PR body provides after-fix runtime logs from a freshly packaged debug build showing stale content kept during refresh and rebuilt after refresh settles.

Is this the best way to solve the issue?

Yes. The version-gated approach is narrow: it avoids refresh-time open-menu rebuilds for data changes while forcing rebuilds for privacy and structure changes that must not reuse stale content.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against dc4e4835bc6e.

Label changes

Label justifications:

  • P2: The PR fixes a normal-priority menu responsiveness issue with limited blast radius and focused regression coverage.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body includes redacted runtime logs from a freshly packaged debug build driven through macOS Accessibility that show the changed menu-refresh behavior after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted runtime logs from a freshly packaged debug build driven through macOS Accessibility that show the changed menu-refresh behavior after the fix.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its menu-testing, privacy/provider-data, and validation guidance informed the review. (AGENTS.md:1, dc4e4835bc6e)
  • Open-menu path uses the new refresh gate: The PR head cancels pending closed-menu rebuild work on menu open and routes stale-menu decisions through refreshMenuForOpenIfNeeded instead of always rebuilding synchronously. (Sources/CodexBar/StatusItemController+Menu.swift:85, 2178073d6aec)
  • Data-refresh stale-content gate is scoped by required rebuild version: The branch increments menuContentVersion on invalidation, advances latestRequiredMenuRebuildVersion only for non-data-refresh invalidations, and preserves stale content only when an in-flight data refresh exists and the menu version is still at or beyond the required rebuild version. (Sources/CodexBar/StatusItemController+MenuTracking.swift:26, 2178073d6aec)
  • Closed-menu rebuilds wait for refresh work to settle: prepareAttachedClosedMenusIfNeeded and rebuildClosedMenuIfNeeded both avoid rebuilding while store or token refresh work is in flight, then schedule delayed closed-menu preparation when attached menus are stale and closed. (Sources/CodexBar/StatusItemController+MenuTracking.swift:50, 2178073d6aec)
  • Privacy setting participates in the rebuild signature: The PR head includes hidePersonalInfo in menuLocalizationSignature so privacy changes are treated as structure-sensitive and cannot be bypassed by stale data-refresh reuse. (Sources/CodexBar/StatusItemController+MenuLocalization.swift:4, 2178073d6aec)
  • Focused regression coverage added: The branch adds tests for closed attached-menu preparation, waiting during store/token refresh, stale content preservation during refresh, and forced rebuild after hide-personal-info changes during refresh. (Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift:118, 2178073d6aec)

Likely related people:

  • Peter Steinberger: Recent current-main commits changed the menu refresh/defer/rebuild paths, and the provided PR discussion shows Peter reviewed and added follow-up fixes on this branch. (role: recent area contributor and likely follow-up owner; confidence: high; commits: 482f1da5adbb, d5a5796a9844, 07ed3facdd4e; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+MenuTracking.swift, Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift)
  • soumikbhatta: Recent current-main work on preserving Codex web refresh behavior touched StatusItemController menu and tracking files that this PR builds on. (role: recent adjacent contributor; confidence: medium; commits: 96745231187f; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+MenuTracking.swift, Sources/CodexBar/StatusItemController.swift)
  • Theo Browne: The hide-personal-info menu behavior relevant to the maintainer blocker appears to date to the privacy redaction commit. (role: introduced related privacy behavior; confidence: medium; commits: 0bc3fda43385; files: Sources/CodexBar/MenuDescriptor.swift, Sources/CodexBar/PersonalInfoRedactor.swift, Sources/CodexBar/StatusItemController+Menu.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jun 1, 2026
@ProspectOre
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 1, 2026
@ProspectOre
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 1, 2026
@ProspectOre
Copy link
Copy Markdown
Contributor Author

Addressed the P3 delay-seam comment in 3e9360a.

  • DEBUG closed-menu prep delay now defaults/resets to the release delay.
  • Focused closed-menu tests explicitly opt into .zero delay.
  • Moved a DEBUG test helper out of the class body for lint only.

Validation:

  • swift test --filter StatusMenuOpenRefreshTests (25 tests passed)
  • make check
  • git diff --check

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 1, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 1, 2026
@steipete
Copy link
Copy Markdown
Owner

steipete commented Jun 1, 2026

Thanks for the detailed PR and tests. I ran the focused menu suites locally (swift test --filter StatusMenuOpenRefreshTests --filter StatusMenuTests) and autoreview.

Holding this one for a fix before landing: the stale-menu preservation path can keep previously rendered personal details visible when a non-data setting invalidates the menu during an in-flight refresh. Concrete case: populate/open with account email visible, start a long refresh, enable Hide Personal Info, then reopen before refresh finishes. refreshMenuForOpenIfNeeded keeps stale nonempty content, so the old unredacted rows can remain visible until refresh completes.

The likely fix is to make the skip apply only to data-refresh invalidations, or force rebuilds for privacy/structure settings even while provider data refresh is active.

@ProspectOre
Copy link
Copy Markdown
Contributor Author

Addressed the privacy stale-menu blocker in 9bec53f.

  • Privacy/structure setting invalidations now mark the current menu version as requiring a fresh rebuild on next open.
  • Data-refresh invalidations still preserve nonempty stale menus during provider refreshes.
  • Added regression coverage for Hide Personal Info during an in-flight refresh and descriptor-level email redaction.

Validation:

  • swift test --filter StatusMenuOpenRefreshTests
  • swift test --filter CodexPresentationCharacterizationTests
  • make check

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 2, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@steipete steipete force-pushed the codex/prepare-closed-menus-after-refresh branch from 9bec53f to 2178073 Compare June 2, 2026 02:51
@steipete steipete marked this pull request as ready for review June 2, 2026 02:51
@steipete steipete merged commit 085319c into steipete:main Jun 2, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants