Repository navigation
fix: keep reminders when notification navigation is not admitted - #2055
Conversation
Preserve reminders on refused, redirected, throwing and deferred navigation. Pass chat session keys directly through the existing navigation owners. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesThe PR dismisses notification reminders only after navigation is admitted and preserves reminders updated during navigation. Example: Click Open Channels on the Slack channel needs attention reminder when browser launch is refused.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this maintainer-authored draft open: it fixes reminder loss still present on main, resolves the earlier integration concerns, and has no identified actionable patch defects. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherNotificationsPage and the Settings banner send reminder actions through AppNotificationService to the existing Workspace and Dashboard navigation owners; admission results control dismissal in the in-memory notification store. flowchart TD
A[Notifications page] --> C[Notification service]
B[Settings banner] --> C
C --> D[Window manager and route policy]
D --> E[Native Workspace destination]
D --> F[Dashboard browser launcher]
E --> G[Admission result]
F --> G
G --> C
Technical reviewBest possible solution: Land the focused reminder-lifetime fix when the author marks the draft ready. Do we have a high-confidence way to reproduce the issue? Fetched main's page and banner handlers dismiss immediately after void navigation, establishing the failure path from source; no target code, builds or tests were executed during this read-only review. Is this the best way to solve the issue? The patch fixes the cause by returning admission through existing navigation owners and protecting replacement reminders with an atomic identity check, without adding a competing state owner. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 1c7901f6cb1e. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Preflight sourceSource unknown. Source comparison at the observed PR snapshot only. Completion is self-reported; body, policy, tests, execution, and report bytes are not verified. LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed November 8, 2026, 11:53 PM ET / November 9, 2026, 04:53 UTC (Revision 2). |
…utes Integrate main while preserving Dashboard dispatch and the separate About page. Await confirmed browser launch before dismissing reminders, and retain reminders replaced or updated during navigation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add native same-window and browser-refusal admission coverage. Preserve the validated Clipboard lifecycle and elapsed-render prerequisites. Co-authored-by: bkudiess <7658216+bkudiess@users.noreply.github.com>
Co-authored-by: bkudiess <7658216+bkudiess@users.noreply.github.com>
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
What Problem This Solves
Fixes: notification reminders disappear when their action is refused, redirected or deferred, and an older action can dismiss a reminder updated during navigation.
User Impact
User impact: inbox and Settings-banner reminders survive unsuccessful navigation. Dashboard actions dismiss only after Windows accepts the browser launch; updated or replaced reminders survive completion of an older action. Native modal-deferred requests retain reminders even if navigation later runs. Opening a reminder never approves or aborts the underlying action.
Current status: integration commit
745eea3b05e3b1a3061614a88d6366eeac26341bis published, preserving both notification admission and main’s Settings-return/focus changes. Its exact source tree passed the native Debug and unsigned non-Dev Release selections below. The PR is ready for maintainer review; current-head CI run 42558071958, attempt 1 has passed, including the UI job and required CI Gate. The only manual merge resolution retained both required imports inNotificationsPage.xaml.cs; no methods or proof assertions were changed. This is not a full-suite, actual-browser-launch or merge-readiness claim.Why This Change Was Made
The existing notification service shares inbox/banner dismissal policy through the existing navigation owners. Review fixes await
GatewayDashboardLauncher.OpenAsyncfor Dashboard admission, preserve About's separate section identity, and compare the captured immutable reminder under the store lock before dismissal. UI handlers useAsyncEventHandlerGuard.The prior head
071b364c4c3eb333f388d97e8c8a242bf0e17d1cintegrates main2812fe5187f64105e297de21377f4a8daae5703d. It preserves the newer Channels removal, Settings reorganization and agent-avatar owners. No independent redesign of Pair-device, history/notification aliases, command-center startup compatibility or Activity destinations is included. Chat keys travel on the request instead of being written into ambient pending-chat state; existing downstream consumption of ambient state is unchanged.Evidence
Current integrated-head proof
Fresh ClawSweeper review completed for
745eea3b05e3b1a3061614a88d6366eeac26341bon November 9 at 04:53 UTC: ready for maintainer review, zero actionable findings and no before-merge items. With current-head CI Gate passing, the PR was marked ready under the author’s instruction. This is not merge approval; the proof limitations below remain explicit.Both Debug and unsigned non-Dev Release passed 3 notification cases, 3 Settings-return cases, 1 Clipboard case, 2 elapsed-time cases, 3 shared-root-dialog cases and 34 accessibility/Axe cases, with zero selected skips or failures. This is 46 executions and 45 unique cases per configuration: Clipboard is intentionally exercised both standalone and in the accessibility category. All 14 TRX inventories were checked against their retained reports.
Full closeout · Debug and Axe · Unsigned Release build · Release and Axe · Source/binary export · Preservation check.
Independent Astra integration source and implementation/proof-plan review found no actionable P0-P3 integration findings. This is bounded Astra review of the integration, not a new Opus or dual-model verdict. The earlier negative control and broad-hosted crash below remain bound to their original source; neither was rerun for this integration.
Current integrated build, before and after the reminder’s own admitted action. These are interaction states from the same build, not a code-before/code-after comparison.
Before its own action
After its own admission
Earlier notification proof, negative control, failures and review history
This retained history predates integration head
745eea3…. Its counts, source identities and review scopes are not promoted into fresh integration executions.November 9 current-source native proof
0xC000027B; all four StartupWindow cases executed and passed. This is not a green full-suite result. A separate unmodified maina39bd70aab55bfdc0053b278756a87c8a3305bcfrun reproduced the same exception/module/hash/offset. The inherited crash is investigated independently rather than attributed to this notification change. Required CI is not bypassed.Required native closeout · Final Debug and Axe · Final Release · Faithful negative control · Unmodified-main baseline
Independent final implementation/proof-plan review (native OpenClaw subagent,
openai/gpt-6-astra, default context, reasoning off) found no actionable P0-P3 findings in this five-file update. It independently checked all 2,174 source hashes, both new admission test files, retained TRX rows, negative control and restoration. This is bounded Astra review, not refreshed dual-model consensus. No additional simplification was recommended.Historical November 6 validation
November 6 integrated validation: full build passed; 361 focused, 4,875 Shared passed / 25 skipped, 5,317 Tray passed, and 16 telemetry diagnostic tests passed. Native UI-test project compilation passed with zero warnings/errors; no native UI tests were executed.
The committed tree
e7213df09f74ba42722b132608e69eda2b3fef19exactly matches the source validated before the merge commit. Tests were not rerun merely for committing, pushing or editing this description.Review history:
reviewer_unavailable,report_produced=false); no clean result is claimed for it.claude-opus-5.5andgpt-6-astra, both requested high reasoning, with long context requested for Astra. Both reviewed the old head28a9346157bffcaad1102cc2f856e608ad4cdb92against122bda71b2211a8fdb40bb7c8589efd19beb5b2e. They found no significant candidate defect but identified Dashboard admission and About identity integration risks. Those are now fixed; the additional same-ID reminder guard addresses the disputed/pre-existing race defensively.code-reviewfollow-up found no verified P0-P3 bug in the integrated changes. It preceded the final event-handler guard correction, which passed the subsequent automated gates. This is not a refreshed dual-model verdict on the final head.CI snapshot observed November 6 at approximately 21:00 Pacific: CI run 42340777734 had change-classification and Proof-pool contracts passing, fast-validation in progress, and release/package jobs skipped. Dispatch passed; C# and Actions security checks were skipped. These observations do not establish that all required CI passed.
Change Type
Scope
winnodeRequired proof pools
windows-winui-interactive: integrated notification admission, Settings return/focus, modal deferral and accessibility. All selected native cases passed in Debug and unsigned non-Dev Release. This establishes those named contracts, not complete pool coverage, actual external-browser behavior or settled Connection-content pixels.Prior declaration and scope
windows-winui-interactive: mounted inbox/banner browser-refusal, separate retry, own-action admission, same-window Connection admission and selective reminder dismissal. Three focused cases pass in both configurations, with 34 real Axe cases. This does not claim the entire pool or every setup/modal/focus/draft scenario was executed.Validation
The published merge commit
745eea3b05e3b1a3061614a88d6366eeac26341bhas parents86615d8a2ea0aa2eff56d55fc827dc35264b2922anda39bd70aab55bfdc0053b278756a87c8a3305bcf. Its treeebeaf64f299e6573a7c1b036f395c195ea36d4f4exactly matches the tree validated before committing. The 2,175-file manifest SHA256 is0cc0cd341b10de1c5c51add1a7592b34745a454257a72d8fe9293ac42336a65b. Committing and pushing did not create another test execution../build.ps1dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restoredotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restoredotnet build ./tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -c Debug -r win-x64./scripts/validate-docs.ps1Every selected row passed; 46 executions / 45 unique cases per configuration, not 46 distinct cases. Ordinary Shared skips remain skips, not runtime/MXC proof. Current-head CI run 42558071958, attempt 1 passed, including the UI job and required CI Gate. No broad-hosted replay or crash experiment was included in this integration run.
Exact current focused commands, filters and binary bindings
For each retained Debug/Release configuration and filter below, the native owner ran:
FullyQualifiedName~NotificationAction_BrowserRefusalAndSeparateRetryKeepReminderUntilItsOwnActionSucceeds|FullyQualifiedName~NotificationAction_NativeConnectionDismissesOnlyItsReminderInTheSameWindowFullyQualifiedName~SettingsBackToApp_ReturnsToAppDestinationInSameWindowFullyQualifiedName~ClipboardExpander_SupportsKeyboardSelectionAndCollapseFullyQualifiedName~ActiveRun_AfterAssistantPreamble_KeepsClawAndElapsedTimeVisibleFullyQualifiedName~SharedRootDialog_DefersNavigationAndRejectsNativeBindingCategory=AccessibilityDebug accessibility used the inventory-owned wrapper:
The wrapper has no Configuration parameter. Release accessibility used the explicit selected command above with
-c Release,--no-buildand--no-restore; its nonzero inventory and all-selected-pass outcome were checked independently.Unsigned local Release was built with process-scoped
RequireSignedCleanupHost=false,./build.ps1 -Project WinUI -Configuration Release, anddotnet build ./tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -c Release -r win-x64 -p:DevBuild=false -p:RequireSignedCleanupHost=false. The override was restored; this was not a machine setting or shipping-signing waiver. Both configurations used isolated profiles, explicit fixture/keyboard opt-ins and their own freshly built app path.02dd86fc516615115d4a68b59038915198bc998be2c5e34afa5e6e8e49b872523cc65b12574e41c0268231b268d95208e6858b47442b822e07bd2ce047e00127426b14dd4ff3936860bc03d607b3e2678446a3e7cedb8df2c5c66df4800057fc0c0eaa37b08368535bfd1f8bce8434e66a4148390be1849a672de2ac2b0f109eBoth app identities report release (non-Dev), independently of compilation configuration. Final receipts verify these same files after the runs.
Pre-integration validation and retained failure history
The following original results belong to earlier source identities. They are preserved, not counted as integration reruns.
November 9 validated update
The published update is byte-bound to the native-tested 2,174-file source manifest
545b704d85e4d2636af5d019feb47f7fb5aa6e51e68931e4f5ad390005b4e748. Source equality is not a new execution after commit../build.ps1dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restoredotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restoredotnet build ./tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -c Debug -r win-x64pwsh -NoProfile -File ./scripts/run-proof-tests.ps1 -Project tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -Filter Category=Accessibility -ResultName winui-accessibility -RuntimeIdentifier win-x64 -ResultsDirectory ./TestResults/ProofPools/pr2055-current-accessibilityFor each
$Configurationin Debug and Release, the actual focused command was:dotnet test ./tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -c $Configuration -r win-x64 --no-build --no-restore --filter $filter --logger "trx;LogFileName=$name.trx" --results-directory "$proof/$name" --diag "$proof/$name/vstest.log" --blame --blame-hang --blame-hang-timeout 300s --blame-hang-dump-type noneFilters and actual counts per configuration:
FullyQualifiedName~NotificationAction_BrowserRefusalAndSeparateRetryKeepReminderUntilItsOwnActionSucceeds|FullyQualifiedName~NotificationAction_NativeConnectionDismissesOnlyItsReminderInTheSameWindow: 3/3.FullyQualifiedName~ClipboardExpander_SupportsKeyboardSelectionAndCollapse: 1/1.FullyQualifiedName~ActiveRun_AfterAssistantPreamble_KeepsClawAndElapsedTimeVisible: 2/2.Both fixture and keyboard opt-ins were explicitly set; profiles were isolated and exact app/test binaries bound. Release is unsigned local behavior proof, not signing/distribution proof. The broad hosted selector
Category!=Accessibility&Category!=NativeOnboardingProof&Category!=GatewayFixtureaborted at 219 passed / 1 skip. Original build/text-assertion failures, negative-control failure, and crash remain retained.Historical validation
Fresh integrated runs on November 6, 2026, superseding the original pre-integration counts:
.\build.ps1dotnet build .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --verbosity quietdotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --logger 'trx;LogFileName=shared.trx' --results-directory $results --verbosity quietdotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --logger 'trx;LogFileName=tray.trx' --results-directory $results --verbosity quietdotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --filter 'FullyQualifiedName~McpHttpServerTelemetryTests' --logger 'trx;LogFileName=telemetry-diagnostic.trx' --results-directory $results --verbosity quietdotnet build .\tests\OpenClaw.Tray.UITests\OpenClaw.Tray.UITests.csproj -r win-x64 --verbosity quiet.\scripts\validate-docs.ps1git diff --cached --check 2812fe5187f64105e297de21377f4a8daae5703dFocused command: 361 passed, 0 failed, 0 skipped.
$resultswas a session-artifact output directory. Runs used isolated tray data, roaming and local roots,OPENCLAW_REPO_ROOT, unset runtime opt-ins andOPENCLAW_STATE_DIR, and a bounded process-only PATH.Preserved failures: an unreadable task-owned FunctionalUI reference assembly was regenerated with
dotnet build .\src\OpenClawTray.FunctionalUI\OpenClawTray.FunctionalUI.csproj -r win-x64 -t:Rebuild --verbosity quiet(passed). Handler/source-guard failures were corrected before final closeout. An unchanged Shared test failed inFreePortbefore product code because Windows lacked socket buffer space; its 16-test batch and full required build/Shared/Tray sequence then passed without source edits or machine changes. The original candidate's IPC timeout is also retained in historical evidence. No timeout or assertion was weakened.Real Behavior Proof
745eea3b05e3b1a3061614a88d6366eeac26341b. Its treeebeaf64f299e6573a7c1b036f395c195ea36d4f4is the exact 2,175-file tree exercised before the merge commit, not a post-push rerun. Both parent identities, manifest and binary hashes are recorded above.Yes/No/N/A) Native run URLs match retained receipts and local artifact hashes. Integrated archive SHA256:1e690623b0bae77a956ac0d9d4152beddef138eaad99bfda96f3b88b3f15080d. Two current native interaction frames are embedded under Evidence; they depict before/after the reminder’s own action, not before/after code versions. Current images have the explicit transition/focus limitations above.Previous head’s native proof and source-bound limits
86615d8a2ea0aa2eff56d55fc827dc35264b2922. All 2,174 committed file bytes match the source tested before committing, based on parent071b364c4c3eb333f388d97e8c8a242bf0e17d1cplus the five-file test update.Yes/No/N/A) Yes for retained native run receipts and local artifact hashes. Archive SHA256:b87de0fdf1b09ccc7448860dd4422fb316c2f98090136910a17c58430184fa17. Connection captures are transitional, not settled pixel proof; no polished screenshot comparison is claimed.Security Impact
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain the risk and mitigation: N/A. The existing credential/browser owner is reused; navigation failures log exception type, not routes, keys or messages.Compatibility and Migration
Yes/No) Yes. Newer main routes and owners are preserved; reminder-retention semantics change intentionally.Yes/No) No product configuration changes.Yes/No) NoReview Conversations