Skip to content

fix(desktop): raise the window when a thread notification is clicked … - #12567

Open
zytact wants to merge 1 commit into
pingdotgg:mainfrom
zytact:fix/linux-notification-reveal
Open

zytact wants to merge 1 commit into
pingdotgg:mainfrom
zytact:fix/linux-notification-reveal

Conversation

@zytact

@zytact zytact commented Sep 19, 2026 •

Copy link
Copy Markdown

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 existing ElectronWindow.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 check pasteAsText uses.

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 reaches WebContents::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's WaylandToplevelWindow::Activate() uses that token the next time the main process activates the window, which reveal() does through BrowserWindow.focus(). Nothing activates the window between the click and the IPC, so the token is still there when reveal() runs.

This needs libnotify 0.8 or newer for the token. The AppImage bundles an older libnotify.so.4 without notify_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 system libnotify4 instead.

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":

  • Minimized, on a different thread: clicking "Thread completed" restores the window and opens the completed thread.
  • Behind another app: with the AppImage's bundled libnotify, clicking shows GNOME's "is ready" banner, and clicking that raises the window on the thread. With the system libnotify 0.8.8 loaded, the click raises the window directly; a WAYLAND_DEBUG trace 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

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

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

    • Clicking a desktop notification now brings the application window to the front and focuses it before opening the selected thread.
    • Added support for safely requesting window reveal behavior from the desktop application.
  • Bug Fixes

    • Improved navigation from desktop notifications when the application window is hidden or out of focus.
  • Tests

    • Added coverage for window reveal authorization and notification-click behavior.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Sep 19, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 11ab902

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.

@coderabbitai

coderabbitai Bot commented Sep 19, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: fa97c180-5d9b-4144-b0ca-2b4a4e52146e

📥 Commits

Reviewing files that changed from the base of the PR and between dfbb11b and 11ab902.

📒 Files selected for processing (8)
  • apps/desktop/src/ipc/DesktopIpcHandlers.ts
  • apps/desktop/src/ipc/channels.ts
  • apps/desktop/src/ipc/methods/window.test.ts
  • apps/desktop/src/ipc/methods/window.ts
  • apps/desktop/src/preload.ts
  • apps/web/src/components/ThreadNotificationCoordinator.test.tsx
  • apps/web/src/components/ThreadNotificationCoordinator.tsx
  • packages/contracts/src/ipc.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a revealWindow IPC method, exposes it through the desktop bridge, and invokes it when a completed-thread notification is clicked. The main process validates the sender before revealing the main window.

Changes

Notification window reveal

Layer / File(s) Summary
IPC contract and preload bridge
apps/desktop/src/ipc/channels.ts, packages/contracts/src/ipc.ts, apps/desktop/src/preload.ts
Defines the reveal channel and exposes the optional desktopBridge.revealWindow method.
Main-process reveal handler
apps/desktop/src/ipc/methods/window.ts, apps/desktop/src/ipc/DesktopIpcHandlers.ts, apps/desktop/src/ipc/methods/window.test.ts
Adds the handler, registers it, validates the sender web contents, and reveals the main window for matching senders.
Notification click integration
apps/web/src/components/ThreadNotificationCoordinator.tsx, apps/web/src/components/ThreadNotificationCoordinator.test.tsx
Requests window reveal before navigating to the completed thread and ignores reveal failures. Tests cover the notification click flow.

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
Loading

Suggested reviewers: bil0000

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 8 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes satisfy #12500. ThreadNotificationCoordinator calls window.desktopBridge?.revealWindow?.() from the system notification click handler before navigation. The preload bridge and IPC chan…
Out of Scope Changes check ✅ Passed The changes stay within #12500. They add the required renderer bridge, IPC channel, main-process reveal handler, notification click integration, and automated tests. No unrelated product behavior is c…
Title check ✅ Passed The title clearly summarizes the main change: revealing the desktop window when a thread notification is clicked.
Description check ✅ Passed The description explains the problem, the change, and focused verification results, and includes UI evidence. It references the linked issue, but does not explicitly address the Scope and approval sec…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Member

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.

@zytact

zytact commented Oct 3, 2026 •

Copy link
Copy Markdown
Author

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 libnotify.so.4 without notify_notification_get_activation_token, and it shadows the system copy. Electron 44 only forwards GNOME's xdg-activation token from a notification click when that function exists, so with the bundled library Chromium requests its own token and Mutter refuses it with the "is ready" banner. The behind/after recording shows exactly that: the click raises T3 only after clicking the banner.

With the system libnotify 0.8.8 loaded instead (dev build launched without the AppImage's LD_LIBRARY_PATH), the click raises the window directly. A WAYLAND_DEBUG trace confirms Chromium activates with GNOME's token (gnome-shell//…) and never requests its own. There's no recording of that here, since the fix is an AppImage packaging change rather than part of this PR.

The .deb depends on the system libnotify4 instead of bundling it, so it most likely doesn't hit this on distros with libnotify 0.8+. I haven't tested the .deb.

get-bb/bb#4483 hit the same thing in another Electron 44 app and fixed it by renaming the bundled library to an unversioned libnotify.so, so Electron tries the system's libnotify.so.4 first.

This branch has not been deployed

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

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Clicking a completed-thread notification does not reveal T3 Code on Linux

2 participants