Repository navigation
feat(native): add menu-bar popover and background attention - #682
Farhan (farhankhalaf) wants to merge 17 commits into
Conversation
📋 PR Overview
🔬 Coverage
|
🎨 Storybook previewUpdated for 328b8bb 🧭 Story changesCompared to ✏️ Changed stories (2)
|
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![prelint[bot]](https://avatars.githubusercontent.com/in/2822344?s=60&v=4){kind=small}
|
| Option | What it gives users | What it costs | Effort to change later |
|---|---|---|---|
| Current approach (no indicator) | Simpler tray icon | Dismissed silent notice leaves no reminder of pending work | Medium: new tray badge or icon-color logic needed |
| Tray badge or icon color change | Persistent signal survives notification dismissal | Small added visual complexity in the menu bar | Low: additive, does not change existing behavior |
| Re-enable preview indicator | Full parity with control mode for pending-change awareness | Floating window conflicts with the popover ambient-operation model | Low: gated by the existing flag |
3. Active work (AI requests, builds, restores) continues running after the user dismisses or hides the popover.
This is the core product promise of the feature. A popover that cancels work on hide would be worse than the existing control window. The implementation keeps the view mounted across hide cycles, so the user returns to exactly the screen they left. The decision is clearly correct and the test suite covers it thoroughly.
4. Starting a new AI request or build clears all pending completion and failure notifications from the previous work session.
Clearing stale notifications at the start of new work prevents a confusing accumulation of outdated results. The narrow risk is a race: a user who sees a ChangesReady notification, begins reviewing it, and simultaneously triggers a new request will see the notification disappear from the notification center. The result they were about to act on is gone permanently. For a developer tool in alpha this is an acceptable edge case. At general availability, a confirm-before-clear or superseded-label policy for unreviewed results would avoid the data-loss feel.
5. The popover window is fixed at 800x600 with no user resize.
A fixed size is standard for tray popovers and keeps positioning logic predictable on multi-display setups. The PR includes a native test confirming that a 401-line diff, a 120-section reply, and 500 lines of build console output all remain scrollable with primary controls reachable at that size. The constraint is appropriate for the current scope.
6. Apply finalization errors are now shown to the user instead of being silently written to the console.
The previous behavior swallowed finalization errors with a console log, leaving the user with no indication that the Save step failed. Surfacing the error lets the user take corrective action. This change is correct in both window modes and is a straightforward improvement.
7. The Escape key routes through a fixed priority stack (Radix dialogs first, then app-owned overlays, then the popover dismiss) rather than relying on listener registration order.
Priority-ordered routing matches the visual depth of each layer and makes the behavior predictable regardless of how React mounts components. Radix layers handle their own events before the app dispatcher sees them, which is the correct split between library and application responsibilities. The test coverage for edge cases (IME composition, default-prevented events, native close tokens, delayed dismissal) is thorough.
8. In popover mode, the tray menu removes the "Open nixmac" menu item and uses a left-click on the tray icon to toggle the window directly.
Left-click-to-toggle is the standard model for tray popovers (Dropbox, 1Password, and similar apps all use it). Removing the redundant "Open nixmac" menu item declutters the right-click menu. A user migrating from control mode may briefly look for the menu item, but the left-click behavior is discoverable immediately on first use.
9. PostHog-backed and local-only feature flags share one developer settings panel, with per-flag description text to distinguish them.
Putting all developer overrides in one place is convenient. The concern is that the "PostHog default" reset label means different things depending on the flag: for PostHog-backed flags it reverts to the server-side value; for local-only flags it reverts to the app default. The defaultLabel field mitigates this by showing "Default (control)" for local-only flags. The distinction is still easy to miss for a developer who skips the description text. A visual separator or section header between the two categories would make the difference obvious at a glance.
10. The mode (popover versus control window) is a launch-time setting that requires a full restart to change.
A launch-time mode avoids the complexity of hot-switching two fundamentally different window management strategies, including tray behavior, position tracking, notification backend, and indicator windows. The restart requirement is clearly labeled in the developer settings. For a developer alpha with a small audience this is an appropriate tradeoff.
Since the last review
- Addressed by explanation: All attention notifications reopen to the same last-visible screen regardless of which event triggered them. (PR conversation: "Routing to a particular diff or blocker remains a separate follow-up, including how old notifications interact with newer work and unsaved edits.")
- Still open: The popover notification backend uses the deprecated NSUserNotificationCenter API with no documented migration plan. (attention.rs retains the #[allow(deprecated)] macos module and uses NSUserNotificationCenter throughout; no migration comment or linked issue appears in the diff.)
- Still open: ChangesReady and BuildSucceeded are silent background notifications and the preview indicator is disabled in popover mode, leaving no persistent ambient signal after a dismissed notice. (attention.rs sets request_attention: false for ChangesReady and BuildSucceeded; peek.rs gates preview-indicator creation and updates on shows_detached_indicators(), which returns false in popover mode.)
- Addressed by explanation: Is a persistent ambient indicator planned before the popover flag exits the developer gate? (PR conversation: "A persistent menu-bar pending-change indicator remains follow-up scope for wider rollout." The feature remains developer-only with an explicit restart gate.)
- Still open: Is there a documented migration plan from the deprecated NSUserNotificationCenter to UNUserNotificationCenter before the feature reaches general availability? (No migration plan, code comment, or linked issue appears in this PR or in attention.rs.)
- Addressed in code: When a new AI request starts, clear_work dismisses any pending notifications from the previous session. Is this the intended behavior? (commands/evolve.rs calls crate::attention::clear_work(&app) at the top of run_evolve; state/rebuild_status.rs calls it in record_start. The pattern is consistent across both entry points, confirming the choice is deliberate.)
Open questions
-
Before the popover flag exits the developer gate, will a persistent ambient indicator (tray badge or icon state) be in place to cover the gap left by the disabled preview indicator?
-
Is there a tracked issue or written plan to migrate from the deprecated notification API before the feature reaches general availability, so that a future macOS release does not silently break all popover notifications?
-
When a new request begins and clears a pending ChangesReady notification that the user has not yet acted on, should the user see any acknowledgment that the previous result has been superseded?
Recommendation
Ship with changes
The feature is solidly built and correctly gated behind a developer-only, restart-required flag, which limits all current user-facing risk. Two prior concerns remain unaddressed: the deprecated notification API has no documented migration path, and silent notifications leave no ambient signal after dismissal. Neither blocks the developer alpha, but both need a written resolution plan before the feature exits the gate. Adding a code comment or linked issue for the API migration path, and a one-line note in the developer settings description about the missing ambient indicator, would close both gaps without requiring new implementation work in this PR.
![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: 2354ad63c5
ℹ️ 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 (@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 (@codex) address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c059cfe856
ℹ️ 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 (@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 (@codex) address that feedback".
|
Thanks, I checked each open question against the branch:
Failure copy is updated: evolution failure says “Your request stopped with an error”; the shared build/restore path says “Build or restore failed” with recovery guidance. Finalization errors retain the distinction that changes may already be active. The full History restore path now also notifies on finalization success or failure. The feature stays a local developer opt-in with an explicit restart requirement. A dedicated preference and migration of the local override belong with wider rollout. Normal-window shortcuts and resizing passed their native checks; dismissing the popover keeps active work running. The popover implementation at |
cbc6b61 to
46f2d04
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 297f149b04
ℹ️ 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 (@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 (@codex) address that feedback".
012c7e7 to
e139f0e
Compare
Compile Cocoa notification bookkeeping and geometry helpers only on macOS or in unit tests. This keeps Linux cargo check clean while preserving cross-platform unit coverage.
e139f0e to
6cda456
Compare
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Escape/Cmd+W without relying on a later native event to reveal mode. | ||
| if (modeProbeAttempts < 3) { | ||
| modeProbeRetry = setTimeout(probePopoverMode, 250); | ||
| } else if (import.meta.env.PROD) { |
There was a problem hiding this comment.
The new error handlers read import.meta.env.PROD directly in widget.tsx. The repository requires all environment reads to go through env.ts. Export the needed build check there and use it here, including in the dismissal and native-listener error handlers. This requirement must be satisfied before merging.
Context Used: Environment access — only through env.ts (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in db5e569. env.ts now exports isProductionBuild and isDevelopmentBuild, and every build-mode read in widget.tsx uses them.
| let popoverModePromise: Promise<boolean> | undefined; | ||
|
|
||
| /** Cache the immutable launch mode, but let a failed transport request retry. */ | ||
| export function isMainWindowPopover(): Promise<boolean> { | ||
| popoverModePromise ??= client.mainWindow.isPopover().catch((error) => { | ||
| popoverModePromise = undefined; | ||
| throw error; | ||
| }); | ||
| return popoverModePromise; |
There was a problem hiding this comment.
main-window.ts adds a module-level cache for data fetched from Rust. The repository requires TypeScript state, data fetching, and cache code to live in packages/state. Move the launch-mode cache there and expose it through @nixmac/state; keep the window actions separate. This requirement must be satisfied before merging.
Context Used: Where TypeScript state and data-fetching code lives (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Changed in 328b8bb, but not moved into packages/state. That package has no runtime oRPC or React Query dependency (it only imports types from native), so moving the cache there would need a native to state to native runtime cycle or a new injection pattern. Instead, the hand-rolled module cache is gone and the launch-mode answer lives in the shared React Query client (no retry, infinite stale and gc time), with its query options in apps/native/src/lib next to the other native query options such as lib/account-status.ts. Concurrent callers share one request, and a failed lookup leaves no data, so the next call retries. Leaving this thread open in case the team wants the move with an injected client.
There was a problem hiding this comment.
Agreed. The updated implementation addresses my concern: the launch-mode lookup now uses the shared React Query client and keeps its query options alongside the other native query options, consistent with account-status.ts. Given that packages/state has no runtime oRPC/React Query dependency, moving this cache there would introduce an undesirable dependency cycle or require an injection layer. I’m withdrawing the original requirement to move it; no further change is needed.
cooper (czxtm)
left a comment
cooper (czxtm)
left a comment
There was a problem hiding this comment.
Not ready to merge: this is blocked on #681, which has to be rebased onto main (it conflicts with #691) and merged first. After that this needs to be retargeted to main, since it currently points at fix/helper-recovery, and re-checked against the rebased code.
Being off by default limits the risk of the feature, which helps, but it does not change the points below.
Blocking
- Size. About 5,000 lines on top of #681, with no Linear ID, covers several separable pieces. Please split it, for example into: the
rebuild_statusand apply/rollback plumbing; the attention and notification backend; window lifecycle plus frontend close and escape routing; and the developer-flag UI.main_window.rsis 1,588 lines andattention.rsis 1,158, each mixing several responsibilities with tests inline, so the split should also break up those modules. - Deprecated API. The macOS backend sits behind a module-wide
#[allow(deprecated)]and usesNSUserNotification(attention.rs#L614-L624), with no comment or issue on the migration. That attribute will also hide any other deprecation in the module. Either move to UserNotifications or thetauri-plugin-notificationdependency already in Cargo.toml, or scope the allow to the specific call sites with a comment and a tracking issue.
Two Greptile threads are still open (env reads in widget.tsx and the cache in lib/main-window.ts); please address or answer both.
Smaller things
apply_end_attentionin rebuild/darwin.rs turnsetc_clobber,app_managementandauthorization_deniedinto "build failed" notices, because onlyuser_cancelledandactivation_refusedare special-cased. The first two are user-actionable blockers and the last is a user decision, so they should map to blocked or silent, with tests.- The question and answered routing in
emit_evolve_eventhas no test for the popover-versus-normal branch that I can find.
A finished apply only special-cased user cancellation and a refused activation, so etc_clobber, app_management and authorization_denied fell through to the "Build or restore failed" notice. etc_clobber, app_management and authorization_denied are blockers the user can resolve (rename files in /etc, grant App Management, supply administrator credentials), so they now raise the Build & Test blocked notice. authorization_denied is not silenced: its classifier also matches broad text such as "not permitted", so silence could hide an activation the user never declined. User cancellation stays silent. Tests cover every mapped error type, unknown types, and the payloads the rebuild paths actually emit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
emit_evolve_event decided inline whether a question or an answer touched native attention, so the popover-versus-control branch had no test. The decision is now a pure function of the event type and window mode. A question in the control window uses the plugin notification when the window is unfocused; in the popover it goes through the owned attention backend, which holds the process-wide macOS delegate. An answer clears pending work attention only in the popover. Behavior is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The popover's notification backend sat behind a module-wide #[allow(deprecated)], which also hid any other deprecation in it. The allow now covers only the imports and the struct that name the deprecated types; the functions and delegate impl already carried their own. A comment records why the backend stays on NSUserNotificationCenter for now: tauri-plugin-notification posts through the same center on macOS and exposes no click callback, and UNUserNotificationCenter needs its own authorization and delegate work, tracked separately. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The widget's new dismissal, mode-probe and native-listener error handlers read import.meta.env.PROD directly, and its test helpers read import.meta.env.DEV. App code reads deployment settings only through lib/env.ts, which now exports isProductionBuild and isDevelopmentBuild. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
isMainWindowPopover kept a hand-rolled module-level promise for data fetched from Rust. It now reads through the app's React Query client with an infinite stale and GC time and no retry, so concurrent callers share one request, a success lasts for the process, and a failed request leaves no data, so the next caller asks again. The query has no persist meta, so it never reaches the on-disk cache, where a value from an earlier launch could be wrong after a restart. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
9383d71 to
4e5f802
Compare
|
|
Pushed fixes for the smaller points (head 328b8bb):
Still open: the split into smaller PRs, the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 39 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/native/src-tauri/src/types.rs">
<violation number="1" location="apps/native/src-tauri/src/types.rs:602">
P2: Answering a question clears the whole Work attention scope, not just the question's notice. `clear_work` retires every pending work notice (changes-ready, build failures, and so on), so those reminders disappear too. Scope the clear to the input-required notice, or document that answering clears all work attention.</violation>
</file>
<file name="apps/native/src/hooks/use-apply.test.ts">
<violation number="1" location="apps/native/src/hooks/use-apply.test.ts:187">
P2: This test name claims the rebuild coordinator surfaces the failure, but `triggerRebuild` is mocked and `onSuccess` is called directly, so the coordinator's catch-and-`setError` is never exercised. Either rename it to describe what it checks (e.g. `rejects from onSuccess when apply finalization fails`) or assert the error reaches `useUiState` through the real `useRebuildStream`.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
View guided diff | Turn on auto-fix | Re-trigger cubic
| EvolveEventAttention::QuestionNotificationIfUnfocused => { | ||
| notify_question(app, &window, &event.summary); | ||
| } | ||
| EvolveEventAttention::ClearWork => crate::attention::clear_work(app), |
There was a problem hiding this comment.
P2: Answering a question clears the whole Work attention scope, not just the question's notice. clear_work retires every pending work notice (changes-ready, build failures, and so on), so those reminders disappear too. Scope the clear to the input-required notice, or document that answering clears all work attention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/native/src-tauri/src/types.rs, line 602:
<comment>Answering a question clears the whole Work attention scope, not just the question's notice. `clear_work` retires every pending work notice (changes-ready, build failures, and so on), so those reminders disappear too. Scope the clear to the input-required notice, or document that answering clears all work attention.</comment>
<file context>
@@ -589,26 +589,68 @@ pub(crate) fn emit_evolve_event<R: tauri::Runtime>(app: &tauri::AppHandle<R>, ev
+ EvolveEventAttention::QuestionNotificationIfUnfocused => {
+ notify_question(app, &window, &event.summary);
+ }
+ EvolveEventAttention::ClearWork => crate::attention::clear_work(app),
+ }
+ }
</file context>
There was a problem hiding this comment.
Valid, and it predates this round of fixes: the PR's original commit already cleared all work attention when a question is answered in the popover, and ca0193c only moved that routing into a pure function without changing behavior. An answer should retire only the question's notice. I'll scope it when the attention backend is split out of this PR (ENG-1448).
| resolveCommitMessage?.(); | ||
| }); | ||
|
|
||
| it("lets the rebuild coordinator surface apply finalization failures", async () => { |
There was a problem hiding this comment.
P2: This test name claims the rebuild coordinator surfaces the failure, but triggerRebuild is mocked and onSuccess is called directly, so the coordinator's catch-and-setError is never exercised. Either rename it to describe what it checks (e.g. rejects from onSuccess when apply finalization fails) or assert the error reaches useUiState through the real useRebuildStream.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/native/src/hooks/use-apply.test.ts, line 187:
<comment>This test name claims the rebuild coordinator surfaces the failure, but `triggerRebuild` is mocked and `onSuccess` is called directly, so the coordinator's catch-and-`setError` is never exercised. Either rename it to describe what it checks (e.g. `rejects from onSuccess when apply finalization fails`) or assert the error reaches `useUiState` through the real `useRebuildStream`.</comment>
<file context>
@@ -183,4 +183,29 @@ describe("useApply", () => {
resolveCommitMessage?.();
});
+
+ it("lets the rebuild coordinator surface apply finalization failures", async () => {
+ mocks.finalizeApply.mockRejectedValue(new Error("finalization failed"));
+ const { result } = renderHook(() => useApply());
</file context>
| it("lets the rebuild coordinator surface apply finalization failures", async () => { | |
| it("rejects from onSuccess when apply finalization fails", async () => { |
There was a problem hiding this comment.
Valid, and it predates this round of fixes. I'll rename it to describe what it checks when this PR is split (ENG-1448).
Summary
Stacked on helper recovery #681. The base is
fix/helper-recoveryso this diff contains only menu-bar/background-attention changes. Retarget to main after the helper PR merges.Adds an opt-in menu-bar popover so queries and builds can continue while nixmac is hidden, with retained state and actionable native attention for questions, completion, failures and configuration drift.
The change handles tray/Dock/notification reopening, display positioning, and close/Escape routing through dismissible dialogs and Settings. Active work and unsaved editor contents stay mounted. Explicit dismissal returns keyboard focus to another app; normal-window behavior stays within the existing rollout boundary.
Rollout
Off by default. Enable Developer settings by clicking the version seven times under Settings > General, then choose Developer > Feature flags > Menu bar popover > Popover and fully quit/reopen nixmac.
The flag is a persisted local launch-time override,
menu-bar-popover=popover. PostHog does not control it, diagnostics may remain off, and hiding Developer settings does not reset it. Choose Default (control) and restart to return to the normal window.Review fixes
Close fallback ownership now survives failed, false and unanswered dismissal calls. Only the current native close token can hide the popover, and successful hide retires that token. Overlay-first handling and retained editor state stay intact.
Completed activation ownership now expires after an intervening external build, preserving notifications and external-build state for a later rollback.
Transient launch-mode lookup failures retry without poisoning the cache. Full History restores now notify after finalization succeeds or fails. Failure copy names the failed request or build/restore operation.
Test Plan
cbc6b6146: 1,412 Rust tests passed (14 ignored), 454 frontend tests passed; strict workspace Clippy with codegen, TypeScript and frontend production build passed. Generated oRPC bindings are idempotent.cbc6b6146: Escape/Cmd+W/Close focus handoff and retained prompt; History retention and overlay-first Close; rejected/false/unanswered dismissal fallback; delayed close invalidated by reopen; an 8-second unresponsive WebView and recovery. Tray show/hide also passes from a clean launch.9383d7139; all five popover commits are unchanged by range-diff. This includes the reviewed diagnostics fix from fix(native): recover unattended sync helper setup #681. The shared helper code and permission panel passed real signed-app Retry/Disable/recovery checks on MacinCloud, plus their regression tests.012c7e753: all 58 state tests and strict workspace Clippy pass. Regressions cover an intervening external build followed by rollback, pre-completion sampling, unavailable reads and stale success events.6cda456b9: macOS build and Linux tests passed, including 1,302 macOS Rust tests (10 ignored), 1,266 Linux Rust tests (3 ignored), 460 frontend tests, strict Clippy, signing, notarization and portability. Evaluate passed. Final Danger validation after the description updates is blocked waiting for an ARC runner; CI maintainers need to inspect runner capacity and dispatch. The earlier Danger run passed. Storybook completed with the same 24 inherited Evolve snapshot failures and alucide-reacterror generating Creevey baseline images. The five-image gallery contains only after images; full snapshot success is not claimed.Native scope: isolated debug/e2e app with local fixtures and mocked system activation. Restore bookkeeping is real. The one-failure startup test uses a separate frontend-copy bootstrap; the original
cbc6b6146bundle was preserved and restored. Release signing, real system activation, display disconnection and additional screen sizes are outside this pass.Standalone Filesystem and the editable NixEditor have no reachable entry point in this build. Their state-retention coverage remains automated; no native presentation claim is made for those surfaces.
Docs
#no-linear