Skip to content

Fix z.ai overview submenu recursion - #1279

Merged
steipete merged 1 commit into
steipete:mainfrom
RajvardhanPatil07:fix/zai-overview-submenu-recursion
Jun 7, 2026
Merged

Fix z.ai overview submenu recursion#1279
steipete merged 1 commit into
steipete:mainfrom
RajvardhanPatil07:fix/zai-overview-submenu-recursion

Conversation

@RajvardhanPatil07

@RajvardhanPatil07 RajvardhanPatil07 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Keep overview rows with detail submenus on the passive submenu action path.
  • Add z.ai regression coverage that ensures submenu activation does not switch into provider detail.
  • Keep existing provider-selection coverage on a plain overview row fixture.

Why

Hovering the z.ai overview row could open a nested full menu because the row had both a detail submenu and a provider-selection action. Rows with submenus should let hover open only the detail submenu; plain rows still keep the selection action.

Fixes #1246.

Reviewer validation

A contributor reviewed and tested this locally on macOS with Swift 6.3.2 / Xcode 26.5.

This PR passed:

PASS overview row submenu action does not switch provider detail (0.092s)
PASS selecting overview row switches to provider detail (0.119s)
PASS Test run with 4 tests in 1 suite (0.594s)

Base check, with only the source fix reverted while keeping the new test, failed as expected:

FAIL mergedMenuLastSelectedWasOverview became false
FAIL selectedMenuProvider became .zai instead of staying .claude
FAIL overviewRow-zai disappeared after the rebuild

Restoring the fix made the test green again. The test performs the row action on a z.ai overview row with a confirmed non-nil submenu, so it covers the path that previously rebuilt the menu mid-interaction. The reviewer also reported SwiftFormat and SwiftLint clean on the three changed files.

Unrelated note from local validation: closed attached menu preparation waits for store refresh to finish failed under a parallel run but passed in isolation; that test is not touched by this PR and appears to be a pre-existing timing flake.

Validation

  • git diff --check
  • .build/lint-tools/bin/swiftformat Sources Tests --lint
  • GitHub CI passed: macOS lint/build/test, Linux x64 build, Linux arm64 build, and security.
  • swift test --filter StatusMenuTests/selecting_overview_row_switches_to_provider_detail was blocked locally before project code compiled: the Command Line Tools install cannot load PreviewsMacros for the KeyboardShortcuts dependency #Preview declarations.
  • make check ran SwiftFormat successfully, then was blocked when portable SwiftLint trapped loading sourcekitdInProc.framework under the local Command Line Tools install.

@clawsweeper

clawsweeper Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 7, 2026, 2:21 AM ET / 06:21 UTC.

Summary
The PR changes CodexBar overview menu wiring so rows with detail submenus keep the submenu/no-op action path, then adds z.ai regression coverage and adjusts the plain-row provider-selection test.

Reproducibility: yes. from source inspection: current main overwrites makeMenuCardItem's submenu no-op action with selectOverviewProvider(_:), so submenu-backed overview rows can switch provider and rebuild the menu during activation. I did not run app-bundle validation because this review is read-only and AGENTS.md cautions against live checks that can prompt.

Review metrics: 2 noteworthy metrics.

  • Changed files: 3 files affected. The PR touches one menu implementation file and two focused status-menu test files.
  • Diff size: 76 added, 10 removed. The small diff is reviewable, but it changes activation behavior in a visible menu path.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

  • [P1] Add redacted real behavior proof from a freshly built app showing the z.ai overview row opens only its detail submenu and does not open a nested full menu.
  • [P1] State whether submenu-backed overview rows are intended to be submenu-only on keyboard/accessibility activation, or add an explicit preserved activation path if not.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body/comment include focused red/green Swift test output and local lint/CI claims, but no packaged-app screenshot, recording, redacted runtime log, or desktop proof showing the visible z.ai submenu behavior after the fix. After adding proof, update the PR body so ClawSweeper can re-review automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.

Mantis proof suggestion
A short desktop/menu proof would materially verify the visible AppKit hover/submenu behavior that unit tests cannot fully prove. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify in a freshly built CodexBar app that hovering the z.ai overview row opens only its detail submenu and does not open a nested full menu.

Risk before merge

  • [P1] The PR intentionally changes the NSMenuItem activation path for submenu-backed overview rows; maintainers should accept that those rows keep the submenu/no-op path while plain rows still switch provider detail.
  • [P1] Real packaged-app proof is still missing for the visible z.ai hover/submenu behavior; the submitted evidence is source inspection, focused test output, and CI-style validation.

Maintainer options:

  1. Require visible app proof (recommended)
    Ask for a redacted screenshot, recording, Peekaboo transcript, or runtime log from a freshly built app showing the z.ai overview row opens only the intended detail submenu.
  2. Accept submenu-only activation
    Maintainers can explicitly accept that submenu-backed overview rows no longer use the NSMenuItem provider-selection action path while plain overview rows still do.
  3. Preserve an alternate detail switch
    If keyboard or accessibility activation must still switch provider detail for submenu rows, add a separate affordance or tested activation path before merge.

Next step before merge

  • [P1] Needs contributor real behavior proof and maintainer acceptance of the submenu-backed overview-row activation contract; there is no narrow automated code repair to apply.

Security
Cleared: The PR only changes AppKit menu action wiring and focused XCTest coverage; no security or supply-chain surface was touched.

Review details

Best possible solution:

Merge the narrow submenu-action guard with its regression tests after redacted real app proof shows the z.ai overview row opens only its detail submenu and does not rebuild into a nested full menu.

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

Yes from source inspection: current main overwrites makeMenuCardItem's submenu no-op action with selectOverviewProvider(_:), so submenu-backed overview rows can switch provider and rebuild the menu during activation. I did not run app-bundle validation because this review is read-only and AGENTS.md cautions against live checks that can prompt.

Is this the best way to solve the issue?

Yes for the code path: guarding the provider-selection action to plain rows preserves the helper's existing submenu no-op behavior and the PR adds focused z.ai and plain-row tests. The merge decision still needs real packaged-app proof and maintainer acceptance of the submenu-backed activation contract.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: The PR fixes a normal-priority z.ai menu recursion bug with limited blast radius.
  • merge-risk: 🚨 compatibility: The PR changes the NSMenuItem activation contract for submenu-backed overview rows, which can affect keyboard or accessibility workflows.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body/comment include focused red/green Swift test output and local lint/CI claims, but no packaged-app screenshot, recording, redacted runtime log, or desktop proof showing the visible z.ai submenu behavior after the fix. After adding proof, update the PR body so ClawSweeper can re-review automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

Likely related people:

  • Peter Steinberger: Blame shows the current overview-row action wiring and adjacent overview-row tests at the release-boundary commit. (role: introduced behavior; confidence: medium; commits: 723734ef3422; files: Sources/CodexBar/StatusItemController+Menu.swift, Tests/CodexBarTests/StatusMenuTests.swift)
  • Larry Hao(郝卓远): Recent merged-menu work extracted makeMenuCardItem and the submenu no-op wiring that this PR relies on. (role: recent area contributor; confidence: medium; commits: 65e39f4dcb3a; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+MenuCardItems.swift)
  • hhh2210: Recent commits touched menu-card height caching and fingerprints in the same overview-menu construction path. (role: recent adjacent contributor; confidence: medium; commits: 7c083fab0c08, 10239cc617cf; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+MenuCardItems.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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 2, 2026
@RajvardhanPatil07
RajvardhanPatil07 force-pushed the fix/zai-overview-submenu-recursion branch 2 times, most recently from 9e60be8 to 24e34db Compare June 2, 2026 15:29
@RajvardhanPatil07
RajvardhanPatil07 marked this pull request as ready for review June 2, 2026 16:08
@devYRPauli

Copy link
Copy Markdown
Contributor

Reviewed and tested this locally on macOS (Swift 6.3.2 / Xcode 26.5). Fix looks correct, and since CI here wants real-behavior proof, here it is.

Root cause is as described: addOverviewRows unconditionally set item.action = #selector(selectOverviewProvider(_:)), overwriting the no-op action makeMenuCardItem already wires up for rows that have a submenu. So activating a z.ai row (which always gets a submenu) fired selectOverviewProvider, flipped mergedMenuLastSelectedWasOverview and the selected provider, and rebuilt the menu mid-interaction. Gating it behind if submenu == nil is the right minimal fix and matches how makeMenuCardItem already handles the submenu case.

GREEN on this PR:

✔ "overview row submenu action does not switch provider detail" passed (0.092s)
✔ "selecting overview row switches to provider detail" passed (0.119s)
✔ Test run with 4 tests in 1 suite passed (0.594s)

RED on base (reverted only the source-fix hunk, kept the new test):

✘ mergedMenuLastSelectedWasOverview → false
✘ (selectedMenuProvider → .zai) == .claude
✘ menu.items.contains { ... }   // overviewRow-zai gone after the rebuild

Restoring the fix makes it green again. The test drives target.perform(action, with: zaiRow) on a row with a confirmed non-nil submenu, so it hits the real path. swiftformat --lint and swiftlint are both clean on the three changed files.

One unrelated note: closed attached menu preparation waits for store refresh to finish (StatusMenuOpenRefreshTests) failed under the parallel run but passes in isolation. It is not touched by this PR (last changed in #1277) and looks like a pre-existing timing flake, not a regression here.

LGTM.

@RajvardhanPatil07

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 5, 2026

Copy link
Copy Markdown

🦞🧹
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 fix/zai-overview-submenu-recursion branch from 24e34db to 2ea4521 Compare June 7, 2026 06:14
@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 7, 2026
@steipete
steipete merged commit 7cd8690 into steipete:main Jun 7, 2026
4 checks passed
@steipete

steipete commented Jun 7, 2026

Copy link
Copy Markdown
Owner

Landed this PR and the linked issue is closed.

How I tested the landed head:

  • Rebased/fixed PR head 2ea452184038de7811a3b0ae8e178b324b76ed06 on current main.
  • Ran swift test --filter StatusMenuOverviewSubmenuTests.
  • Ran swift test --filter StatusMenuTests.
  • Ran git diff --check origin/main...HEAD.
  • Ran make check.
  • Ran /Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode branch --base origin/main; result was clean with no accepted/actionable findings.
  • Packaged the debug app with CODEXBAR_SIGNING=adhoc ./Scripts/package_app.sh debug.
  • Launched the packaged debug bundle with a temporary config, dummy z.ai token, local quota fixture on 127.0.0.1:54555, and debug keychain access disabled. No real z.ai credentials were used.
  • Verified visually with Peekaboo that the merged Overview menu stays selected while hovering the z.ai row and only opens the z.ai detail submenu.
  • Watched exact-head CI run 27084665619 go green for lint-build-test, build-linux-cli (linux-x64, ubuntu-24.04), build-linux-cli (linux-arm64, ubuntu-24.04-arm), and GitGuardian.

Landed commits:

  • 7cd8690648bd0804888e7a3fc6facfadfce43eea - PR squash merge.
  • 574c158c - maintainer changelog follow-up.

Verification image:

zai-hover-proof-cropped.png
zai-hover-proof-cropped.png

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

zai triggers a recursion

3 participants