Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped desktop notification bug fix that adds a sender-validated IPC call to the existing window reveal implementation while preserving navigation and browser behavior. Focused tests cover both the notification interaction and IPC authorization, and no product defaults or static-analysis overrides are changed. You can add or adjust custom eligibility rules. Learn more. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds a ChangesNotification window reveal
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant ThreadNotificationCoordinator
participant desktopBridge
participant MainProcess
participant MainWindow
ThreadNotificationCoordinator->>desktopBridge: revealWindow()
desktopBridge->>MainProcess: Invoke reveal IPC channel
MainProcess->>MainWindow: Validate sender and reveal window
ThreadNotificationCoordinator->>ThreadNotificationCoordinator: Navigate to completed thread
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Note This comment is posted by Julius' dot The recording shows a minimized window restoring to the completed thread. Does clicking the notification also raise T3 when it is still visible behind another app? Julius's triage calls out that case. Please add a before/after recording of it to complete the verification. |
|
Note This comment is written by Opus 5.5 The description now has before/after recordings for both cases, minimized and behind another app. The behind case exposed a packaging problem outside this PR. The AppImage bundles an old With the system libnotify 0.8.8 loaded instead (dev build launched without the AppImage's The .deb depends on the system get-bb/bb#4483 hit the same thing in another Electron 44 app and fixed it by renaming the bundled library to an unversioned |
Closes #12500
What Changed
Clicking a thread notification now asks the desktop main process to reveal the main window through a new
desktopBridge.revealWindow()IPC method, which calls the existingElectronWindow.reveal(). This covers completed, input, approval, and failed notifications, since they share one click handler. Navigation to the thread is unchanged. The method only reveals the window when the main window's own renderer calls it, the same checkpasteAsTextuses.Why
On Linux (GNOME/Wayland), clicking a notification opened the thread in the background but left T3 Code behind the current app. The handler called renderer
window.focus(), which never raises an Electron window. In Electron it only reachesWebContents::ActivateContents, which hides the auto-hide menu bar and does nothing else.No compositor-specific code is needed. When a notification is clicked, Electron 44's libnotify handler stores the xdg-activation token it received (
base::nix::SetActivationToken). Chromium'sWaylandToplevelWindow::Activate()uses that token the next time the main process activates the window, whichreveal()does throughBrowserWindow.focus(). Nothing activates the window between the click and the IPC, so the token is still there whenreveal()runs.This needs libnotify 0.8 or newer for the token. The AppImage bundles an older
libnotify.so.4withoutnotify_notification_get_activation_token, so there Chromium requests its own token and GNOME refuses it with an "is ready" banner when the window is behind another app. The .deb depends on the systemlibnotify4instead.Tested on Fedora 44, GNOME on Wayland, with the nightly AppImage
0.0.46-nightly.20261003.2623(fed41fa88bb2) for "before" and a dev build for "after":WAYLAND_DEBUGtrace shows Chromium activating with GNOME's token.UI Changes
Minimized, before (nightly
0.0.46-nightly.20261003.2623). Clicking the notification leaves T3 Code minimized.before-minimized.mp4
Minimized, after. The window restores on the completed thread.
linux-notification-reveal.mp4
Behind another app, before (nightly
0.0.46-nightly.20261003.2623). Clicking the notification does nothing; Settings stays in front.before-behind-another-app.mp4
Behind another app, after (with the AppImage's bundled libnotify). GNOME shows "is ready" instead of raising the window, and clicking that banner brings T3 Code forward on the thread. The libnotify limitation above causes this, not this PR.
after-behind-another-app.mp4
Checklist
Opus 5.5 via Claude Code, with GPT-6 Astra and GPT-5.6 Sol via Codex, in T3 Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests