Skip to content

feat: replace in-app Toast notifications with native OS notifications#334

Merged
Astro-Han merged 9 commits into
devfrom
feat/notification-system-redesign
Apr 29, 2026
Merged

feat: replace in-app Toast notifications with native OS notifications#334
Astro-Han merged 9 commits into
devfrom
feat/notification-system-redesign

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Apr 29, 2026

Copy link
Copy Markdown
Owner

Summary

Replace in-app Toast notifications with native OS notifications + Dock/taskbar flash feedback. This removes the annoying bottom-right popups that block UI elements.

Changes:

  • Remove focus checks from both web and desktop notify functions so system notifications fire regardless of window focus
  • Add flashFrame IPC with platform-specific behavior (macOS: Dock bounce, Windows/Linux: taskbar flash)
  • Remove notification-related Toast calls and associated dead code from layout.tsx
  • Add fallback to Toast when OS notification permission is denied (web platform)

Why

The current notification system uses in-app Toast popups in the bottom-right corner that:

  • Block the send/submit buttons
  • Create visual noise when multiple notifications stack
  • Mix important (permission requests) with less important (status updates) notifications

Users want native OS notifications like Codex App, which don't interfere with the app UI.

Related Issue

Closes #332

How To Verify

bun --cwd packages/desktop-electron typecheck
bun --cwd packages/app typecheck

Manual testing:

  1. Run bun run dev:desktop
  2. Trigger a permission request or question from an agent
  3. Verify system notification appears (not Toast popup)
  4. Verify Dock icon bounces (macOS) or taskbar flashes (Windows)
  5. Verify click notification brings window to front and navigates to session
  6. Deny OS notification permission → verify Toast fallback works

Screenshots or Recordings

N/A — notification behavior change, no visual UI change

Checklist

  • I linked the related issue, or stated why there is no issue
  • This PR has type, scope, and priority labels, or I requested maintainer labeling
  • I listed the relevant verification steps, including tests when behavior changed
  • I manually checked visible UI or copy changes when needed, with screenshots or recordings
  • I considered macOS and Windows impact for desktop, packaging, updater, signing, paths, shell, or permissions changes
  • I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, or generated/local file changes when relevant
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English

Summary by CodeRabbit

  • New Features

    • Desktop attention indicator added (Dock bounce on macOS, taskbar flash on Windows/Linux).
  • Improvements

    • If OS notification permission is explicitly denied, a warning is logged and an in-app toast is shown.
    • Notifications are not sent for the currently active session to avoid distractions; throttling still applies.
    • Native notification delivery now uses a safe fallback and always attempts to attract attention.
  • Bug Fixes

    • Removed persistent per-session toasts so alerts no longer linger.

@Astro-Han Astro-Han added enhancement New feature or request P2 Medium priority app Application behavior and product flows labels Apr 29, 2026
@coderabbitai

coderabbitai Bot commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 723a2192-7368-4ca6-8237-d6376f3055a3

📥 Commits

Reviewing files that changed from the base of the PR and between c959dd0 and c135762.

📒 Files selected for processing (1)
  • packages/desktop-electron/src/renderer/index.tsx
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: unit-windows-opencode-server-tools
  • GitHub Check: unit-windows-opencode-session
  • GitHub Check: unit-app
  • GitHub Check: unit-desktop
  • GitHub Check: smoke-macos-arm64
  • GitHub Check: e2e-artifacts
  • GitHub Check: analyze-js-ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/desktop-electron/src/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)

Renderer process should only call window.api from src/preload

Files:

  • packages/desktop-electron/src/renderer/index.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/src/pages/layout/sidebar-items.tsx:102-107
Timestamp: 2026-04-23T15:26:07.250Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/layout/sidebar-items.tsx`), the `indicator()` function in `SessionRow` intentionally renders `props.leadingSlot` (the pin button) only as a fallback when no status indicator (running/permission/error/unseen) is active. When a higher-priority status wins the slot, the pin button is removed from the DOM — this is a deliberate design choice for the merged leading slot (`#150`). The keyboard unpin path is preserved via: (1) focusing the row anchor triggers `group-focus-within` which reveals the dots menu trigger, then Tab → Enter → "Unpin Session"; (2) the context menu (right-click / Shift+F10) exposes "Unpin Session". The "always render + CSS overlay" approach was considered but rejected due to z-index/pointer-events complexity; residual `...` slot behavior is tracked in `#192`. Do NOT flag the absence of the pin button from the DOM when a status is active as an accessibility regression.
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 208
File: packages/app/src/components/prompt-input.tsx:1569-1611
Timestamp: 2026-04-24T05:39:58.329Z
Learning: In Astro-Han/pawwork `packages/app/src/components/prompt-input.tsx`, after the composer unification in PR `#208` (fixed in commit 5d810aa):
- `SendButton.disabled` does NOT gate on `store.mode !== "normal"`. Shell mode has a fully visible, clickable orange submit button that calls `handleSubmit` directly (same path as the Enter key in `handleKeyDown`). Do NOT suggest re-adding the mode gate.
- `SendButton` does NOT use the `buttons()` spring opacity animation (`style={buttons()}`). It is always fully visible regardless of mode.
- `WorkspaceChip` is gated on `props.homeMode && store.mode === "normal"` so it hides in shell mode (preventing it from appearing isolated/bright while neighboring controls fade).
- The left-side chip group (`aria-hidden={store.mode !== "normal"}`) covers attach/model/variant/workspace controls only; `SendButton` remains in a separate right-side sibling div.
📚 Learning: 2026-04-20T14:36:08.774Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/desktop-electron/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:08.774Z
Learning: Applies to packages/desktop-electron/src/**/*.{ts,tsx,js,jsx} : Renderer process should only call `window.api` from `src/preload`

Applied to files:

  • packages/desktop-electron/src/renderer/index.tsx
📚 Learning: 2026-04-24T17:12:26.774Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 224
File: packages/desktop-electron/electron-builder.config.ts:14-18
Timestamp: 2026-04-24T17:12:26.774Z
Learning: In Astro-Han/pawwork, the `localizedMacDisplayNameByChannel` map in `packages/desktop-electron/electron-builder.config.ts` is intentionally kept separate from `localizedAppDisplayName` in `packages/desktop-electron/src/main/app-display-name.ts`. The former is a build-time packaging helper; the latter is a runtime UI helper that localizes the current app name by locale. Coupling them would introduce a build-time dependency on runtime main logic. Do not suggest deduplicating or sharing this mapping — the explicit local table is covered by focused regression tests in `electron-builder-app-update.test.ts`.

Applied to files:

  • packages/desktop-electron/src/renderer/index.tsx
📚 Learning: 2026-04-25T09:19:33.633Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 231
File: packages/desktop-electron/src/main/index.ts:537-537
Timestamp: 2026-04-25T09:19:33.633Z
Learning: In Astro-Han/pawwork (`packages/desktop-electron/src/main/`), the convention for IPC registration is that `index.ts` (the bootstrap entry) directly calls each registration function. `registerIpcHandlers({...})` in `src/main/ipc.ts` is a single fat options-bag with inline handler bodies. New, cohesive IPC features (e.g., About) are placed in sub-modules like `src/main/ipc/about.ts`, which own their own exported types, helpers, and a `register*Ipc()` function. The bootstrap in `index.ts` calls each `register*Ipc()` directly — this is intentional, not fragmentation. Do NOT suggest routing sub-module IPC registrations through `ipc.ts`.

Applied to files:

  • packages/desktop-electron/src/renderer/index.tsx
🔇 Additional comments (1)
packages/desktop-electron/src/renderer/index.tsx (1)

252-268: Native notification flow + fallback is implemented cleanly.

This block correctly attempts OS notification first, preserves click-to-focus/navigation behavior, falls back to IPC notification when construction fails, and triggers taskbar/Dock attention consistently.

As per coding guidelines, this renderer segment correctly uses window.api (preload bridge) instead of direct Electron APIs.


📝 Walkthrough

Walkthrough

Adds native OS notifications with explicit fallback to in-app toast when OS permission is denied, removes focus/visibility suppression and persistent in-app toast tracking, and introduces a preload↔main IPC pair to trigger Dock/taskbar attention (flash/bounce) from the renderer.

Changes

Cohort / File(s) Summary
Web Notification Fallback
packages/app/src/entry.tsx, packages/desktop-electron/src/renderer/index.tsx
notify now treats "denied" explicitly (console.warn + showToast(title, description) fallback) and no longer suppresses notifications based on document/window focus; native Notification creation is try/catch-protected with IPC fallback.
Layout / Toast Cleanup
packages/app/src/pages/layout.tsx
Removed per-session toast ID tracking, dismissal helpers, and toast action wiring. Reply/reject handlers now only clear throttling. permission.asked and question.asked emit platform.notify(...) only for non-active sessions (workspace + sessionID checks) and continue cooldown via alertedAtBySession.
Electron Main IPC
packages/desktop-electron/src/main/ipc.ts
Added flash-frame IPC handler: maps invoking webContents→BrowserWindow and triggers platform attention — macOS app.dock.bounce(); Windows/Linux win.flashFrame(true) then win.flashFrame(false) after 1s, with per-window timer management.
Preload Bridge & Types
packages/desktop-electron/src/preload/index.ts, packages/desktop-electron/src/preload/types.ts
Exposes flashFrame(): Promise<void> on the preload ElectronAPI, implemented as ipcRenderer.invoke("flash-frame"), and updates the ElectronAPI type accordingly.

Sequence Diagrams

sequenceDiagram
    participant User
    participant Renderer as Renderer Process
    participant Preload
    participant Main as Main Process
    participant OS as OS Notification
    participant DockTask as Dock/Taskbar

    User->>Renderer: Trigger notification event
    Renderer->>OS: Create native Notification()
    alt OS Permission Granted
        OS-->>User: Show native notification
        Renderer->>Preload: call flashFrame()
        Preload->>Main: invoke "flash-frame"
        alt macOS
            Main->>DockTask: app.dock.bounce()
        else Windows/Linux
            Main->>DockTask: win.flashFrame(true)
            Note over Main: wait 1000ms
            Main->>DockTask: win.flashFrame(false)
        end
    else OS Permission Denied
        Renderer->>Renderer: console.warn + showToast(title, description)
        Renderer-->>User: Display in-app fallback toast
    end
Loading
sequenceDiagram
    participant Session as Session System
    participant Layout as Layout Component
    participant Notifier as Notification Emission

    Session->>Layout: question.asked / permission.asked
    Layout->>Layout: check alertedAtBySession throttle & active session match
    alt Background session & throttle expired
        Layout->>Notifier: emit notification (native or fallback)
        Layout->>Layout: update alertedAtBySession timestamp
    else Active session or throttle active
        Layout->>Layout: skip emitting notification
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

P1, desktop

Poem

🐇
I twitched my whiskers, gave a hop,
Native bells ring—no toast to stop,
Dock gives a bounce, taskbars flash bright,
If OS says no, a toast takes flight.
Hooray, the rabbit's on the hop!

🚥 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: replacing in-app Toast notifications with native OS notifications.
Description check ✅ Passed The PR description follows the template structure with comprehensive Summary, Why, Related Issue, How to Verify, and Checklist sections, providing clear context and verification steps.
Linked Issues check ✅ Passed All five key changes from issue #332 are implemented: focus checks removed [entry.tsx, renderer/index.tsx], Toast calls removed [layout.tsx], flashFrame IPC added [preload bridge], flash-frame handler added [main/ipc.ts], and platform-specific flash behavior implemented.
Out of Scope Changes check ✅ Passed All code changes directly address the linked issue objectives. Changes are scoped to notification system refactoring, IPC bridge extensions, and removal of obsolete Toast-related code with no unrelated modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/notification-system-redesign

Review rate limit: 0/3 reviews remaining, refill in 51 minutes and 2 seconds.

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a fallback to in-app toasts when OS notification permissions are denied and introduces a window flashing feature for desktop notifications to attract user attention. It also refactors notification handling by removing legacy session-based toast logic. Feedback focuses on restoring missing persistence and actions in the new fallback toast, preventing a potential crash in the Electron main process by checking if the window is destroyed before resetting the flash state, and ensuring the window only flashes when it is not currently focused to avoid unnecessary distractions.

Comment thread packages/app/src/entry.tsx
Comment thread packages/desktop-electron/src/main/ipc.ts Outdated
Comment thread packages/desktop-electron/src/renderer/index.tsx

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one inline comment for a notification regression that is not covered by the existing comments.

Comment thread packages/app/src/pages/layout.tsx Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/app/src/entry.tsx`:
- Around line 64-73: The current check treats any non-"granted" result the same;
update the logic around the permission variable returned by
Notification.requestPermission() so that you only fall back to showToast(...)
when permission === "denied" (explicit denial); if permission === "default"
simply return silently and do not show the in-app toast. Locate the block using
permission, showToast, title, and description and change the conditional
accordingly so denied triggers the toast and default exits without UI.

In `@packages/desktop-electron/src/main/ipc.ts`:
- Around line 403-409: The flash timer is overwritten unconditionally causing
earlier timers to cancel newer flashes; before calling setTimeout to turn off
win.flashFrame, clear any existing timer for that BrowserWindow (e.g., check and
clear a stored timer id on the window or in a Map keyed by win.id), then assign
the new timer id back to that storage; in the timeout callback call
win.flashFrame(false) and remove/clear the stored timer id so subsequent
notifications properly reset the stop-timer per window (use clearTimeout on the
stored id and ensure unique storage name like flashTimer or a dedicated Map).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0ec34922-b277-47c5-a2df-89eab0621ee2

📥 Commits

Reviewing files that changed from the base of the PR and between 24a3f37 and 665e9aa.

📒 Files selected for processing (6)
  • packages/app/src/entry.tsx
  • packages/app/src/pages/layout.tsx
  • packages/desktop-electron/src/main/ipc.ts
  • packages/desktop-electron/src/preload/index.ts
  • packages/desktop-electron/src/preload/types.ts
  • packages/desktop-electron/src/renderer/index.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: unit-windows-desktop
  • GitHub Check: unit-windows-opencode-config-project
  • GitHub Check: unit-windows-app
  • GitHub Check: unit-windows-opencode-session
  • GitHub Check: unit-opencode
  • GitHub Check: unit-windows-opencode-server-tools
🧰 Additional context used
📓 Path-based instructions (3)
packages/desktop-electron/src/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)

Renderer process should only call window.api from src/preload

Files:

  • packages/desktop-electron/src/preload/types.ts
  • packages/desktop-electron/src/preload/index.ts
  • packages/desktop-electron/src/renderer/index.tsx
  • packages/desktop-electron/src/main/ipc.ts
packages/desktop-electron/src/main/ipc.ts

📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)

Main process should register IPC handlers in src/main/ipc.ts

Files:

  • packages/desktop-electron/src/main/ipc.ts
packages/app/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (packages/app/AGENTS.md)

Always prefer createStore over multiple createSignal calls in SolidJS

Files:

  • packages/app/src/entry.tsx
  • packages/app/src/pages/layout.tsx
🧠 Learnings (8)
📚 Learning: 2026-04-20T14:36:08.774Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/desktop-electron/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:08.774Z
Learning: Applies to packages/desktop-electron/src/**/*.{ts,tsx,js,jsx} : Renderer process should only call `window.api` from `src/preload`

Applied to files:

  • packages/desktop-electron/src/preload/types.ts
  • packages/desktop-electron/src/preload/index.ts
  • packages/desktop-electron/src/renderer/index.tsx
📚 Learning: 2026-04-25T09:19:33.633Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 231
File: packages/desktop-electron/src/main/index.ts:537-537
Timestamp: 2026-04-25T09:19:33.633Z
Learning: In Astro-Han/pawwork (`packages/desktop-electron/src/main/`), the convention for IPC registration is that `index.ts` (the bootstrap entry) directly calls each registration function. `registerIpcHandlers({...})` in `src/main/ipc.ts` is a single fat options-bag with inline handler bodies. New, cohesive IPC features (e.g., About) are placed in sub-modules like `src/main/ipc/about.ts`, which own their own exported types, helpers, and a `register*Ipc()` function. The bootstrap in `index.ts` calls each `register*Ipc()` directly — this is intentional, not fragmentation. Do NOT suggest routing sub-module IPC registrations through `ipc.ts`.

Applied to files:

  • packages/desktop-electron/src/preload/index.ts
📚 Learning: 2026-04-24T17:12:26.774Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 224
File: packages/desktop-electron/electron-builder.config.ts:14-18
Timestamp: 2026-04-24T17:12:26.774Z
Learning: In Astro-Han/pawwork, the `localizedMacDisplayNameByChannel` map in `packages/desktop-electron/electron-builder.config.ts` is intentionally kept separate from `localizedAppDisplayName` in `packages/desktop-electron/src/main/app-display-name.ts`. The former is a build-time packaging helper; the latter is a runtime UI helper that localizes the current app name by locale. Coupling them would introduce a build-time dependency on runtime main logic. Do not suggest deduplicating or sharing this mapping — the explicit local table is covered by focused regression tests in `electron-builder-app-update.test.ts`.

Applied to files:

  • packages/desktop-electron/src/renderer/index.tsx
📚 Learning: 2026-04-25T09:19:30.734Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 231
File: packages/desktop-electron/src/main/index.ts:537-537
Timestamp: 2026-04-25T09:19:30.734Z
Learning: In Astro-Han/pawwork (packages/desktop-electron/src/main/), follow the IPC registration convention: the bootstrap entry (packages/desktop-electron/src/main/index.ts) should directly call each module’s exported register*Ipc() function. Do not route/centralize these sub-module IPC registrations through src/main/ipc.ts. Keep sub-module IPC features cohesive (e.g., src/main/ipc/about.ts should own its types/helpers and expose register*Ipc()), and allow index.ts to aggregate by calling each register*Ipc() directly.

Applied to files:

  • packages/desktop-electron/src/main/ipc.ts
📚 Learning: 2026-04-25T12:52:35.631Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 234
File: packages/desktop-electron/src/main/ipc.ts:238-263
Timestamp: 2026-04-25T12:52:35.631Z
Learning: In Astro-Han/pawwork (`packages/desktop-electron/src/main/ipc.ts`), `deps.getServerReadyData()` (backed by `serverReady.promise` in `index.ts`) resolves once at server startup and remains settled; it is not expected to reject in practice. Do not flag the absence of a try-catch around it in the `export-session` IPC handler — the network/fetch layer in `server-client.ts` already has a 10-second AbortController timeout and returns a typed `{ok: false, error}` payload, covering the real failure modes.

Applied to files:

  • packages/desktop-electron/src/main/ipc.ts
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.

Applied to files:

  • packages/app/src/entry.tsx
  • packages/app/src/pages/layout.tsx
📚 Learning: 2026-04-23T15:10:21.635Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 191
File: packages/app/src/components/session/pawwork-skill-meta.ts:38-39
Timestamp: 2026-04-23T15:10:21.635Z
Learning: This repo configures Tailwind v4 with `--color-*: initial`, which effectively breaks standard Tailwind palette utilities (e.g., `text-violet-500` can resolve to no CSS variable and render as a no-op/black). For brand/accent colors that are not backed by semantic design tokens, use inline styles with the exact hex value (e.g., `style={{ color: '#8B5FBF' }}` / `homeIconStyle: { color: '#8B5FBF' }`) and add a short comment explaining that Tailwind palette utilities won’t work due to the `--color-*: initial` setup. Do not suggest replacing these inline hex colors with Tailwind palette classes anywhere in this repo.

Applied to files:

  • packages/app/src/entry.tsx
  • packages/app/src/pages/layout.tsx
📚 Learning: 2026-04-24T03:51:56.211Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 206
File: packages/app/e2e/prompt/prompt-footer-focus.spec.ts:131-143
Timestamp: 2026-04-24T03:51:56.211Z
Learning: In Astro-Han/pawwork E2E tests (packages/app/e2e/fixtures.ts), `project.prompt(text)` internally calls `trackSession(next.sessionID, active.directory)` after the prompt submission is observed and the active session is resolved. Tests that obtain a `sessionID` from `project.prompt()` do NOT need to call `project.trackSession(sessionID)` manually — it is already registered for teardown. Calling it again would be duplicate ownership.

Applied to files:

  • packages/app/src/pages/layout.tsx

Comment thread packages/app/src/entry.tsx Outdated
Comment thread packages/desktop-electron/src/main/ipc.ts Outdated

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please close the remaining notification UX gaps before this PR lands. The direction matches #332, but the implementation still needs to make the failure paths as reassuring as the main path: 1. Web fallback: when OS notification permission is denied, the fallback Toast should preserve the old ability to go to the affected session instead of only showing a passive message. 2. Desktop fallback: if desktop OS notifications are unavailable or denied, PawWork should still give the user an in-app fallback instead of silently losing the permission/question/session alert. 3. Windows/Linux flash cleanup: the delayed stop for taskbar flashing should not call into a destroyed window, and repeated notifications should reset the stop timer cleanly per window. These three items are the difference between replacing the blocking Toasts and accidentally losing important alerts when system notifications cannot be delivered.

Comment thread packages/desktop-electron/src/renderer/index.tsx
@Astro-Han

Copy link
Copy Markdown
Owner Author

Review Comments Addressed

P0 - Critical Issues

1. Restore current-session guard (Astro-Han, line 486)

  • Fixed: Restored the current-session and child-session suppression before calling platform.notify. Now only background sessions trigger OS notifications, preventing unnecessary alerts when the user is already viewing the relevant session.

P1 - High Priority Issues

2. Only fallback to Toast for denied permission (CodeRabbit, line 64-73)

  • Fixed: Changed the condition to only show Toast fallback when permission === "denied". For default state (user dismissed prompt), we now silently return without showing a Toast.

3. Check win.isDestroyed() in flash timer (Gemini, line 407-409)

  • Fixed: Added win.isDestroyed() check before calling win.flashFrame(false) in the timeout callback to prevent crashes when the window is closed before the timer fires.

4. Reset flash timer per window (CodeRabbit, line 403-409)

  • Fixed: Implemented a flashFrameTimers Map to track timers per window ID. Now clears existing timer before setting a new one, preventing earlier timers from canceling newer flashes under burst conditions.

P2 - Medium Priority Issues (YAGNI Analysis)

5. Desktop fallback for failed notifications (Astro-Han, line 260)

  • Won't fix (YAGNI): On Electron, Notification API is typically reliable as it uses the system's native notification. If it fails, the user likely sees a system-level error. Adding fallback logic would increase complexity for an edge case that rarely occurs. If users report issues, we can add this later.

6. Flash only when window not focused (Gemini, line 263)

  • Won't fix (YAGNI): The current implementation already provides good UX - flashing when focused is a minor distraction, and the 1-second timer ensures it stops quickly. Adding focus checks would add complexity without significant benefit.

7. Restore Toast actions and persistence (Gemini, line 67-71)

  • Won't fix (YAGNI): The PR's primary goal is to replace Toasts with native OS notifications. The fallback Toast is a minimal safety net for denied permissions. Adding actions and persistence would partially defeat the purpose of removing Toasts. Users should enable OS notifications for the best experience.

P3 - Low Priority Issues

8. Icon inconsistency (Gemini, line 67-71)

  • Won't fix (YAGNI): The /icon.png in web fallback is a minor inconsistency. The primary notification path (OS notifications) uses the app icon automatically. This is a cosmetic issue that doesn't affect functionality.

Summary: Fixed 4 critical/high priority issues that could cause crashes, UX regressions, or logic errors. Declined 4 medium/low priority issues based on YAGNI principle - they add complexity for edge cases that rarely occur or don't significantly improve UX.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/app/src/pages/layout.tsx`:
- Around line 480-498: The child-session suppression logic incorrectly treats
undefined params.id as a match (undefined === undefined) and suppresses
root-session notifications; in the question.asked branch replicate the guard
used above by only computing currentSession/isCurrentSession and checking
session?.parentID when params.id is truthy (i.e., require params.id before
comparing to session?.parentID), so update the logic around currentSession,
isCurrentSession, workspaceKey(currentDir()), props.sessionID and
session?.parentID in the question.asked block and only call platform.notify when
!isCurrentSession after applying that guard.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3bf451eb-3bb5-4137-bf46-e200a818e043

📥 Commits

Reviewing files that changed from the base of the PR and between 665e9aa and fb2b831.

📒 Files selected for processing (3)
  • packages/app/src/entry.tsx
  • packages/app/src/pages/layout.tsx
  • packages/desktop-electron/src/main/ipc.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
  • GitHub Check: smoke-macos-arm64
  • GitHub Check: unit-windows-opencode-config-project
  • GitHub Check: unit-windows-opencode-session
  • GitHub Check: unit-opencode
  • GitHub Check: unit-windows-app
  • GitHub Check: unit-windows-opencode-server-tools
  • GitHub Check: unit-windows-desktop
  • GitHub Check: unit-desktop
  • GitHub Check: unit-app
  • GitHub Check: typecheck
  • GitHub Check: analyze-js-ts
  • GitHub Check: e2e-artifacts
🧰 Additional context used
📓 Path-based instructions (3)
packages/desktop-electron/src/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)

Renderer process should only call window.api from src/preload

Files:

  • packages/desktop-electron/src/main/ipc.ts
packages/desktop-electron/src/main/ipc.ts

📄 CodeRabbit inference engine (packages/desktop-electron/AGENTS.md)

Main process should register IPC handlers in src/main/ipc.ts

Files:

  • packages/desktop-electron/src/main/ipc.ts
packages/app/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (packages/app/AGENTS.md)

Always prefer createStore over multiple createSignal calls in SolidJS

Files:

  • packages/app/src/entry.tsx
  • packages/app/src/pages/layout.tsx
🧠 Learnings (13)
📓 Common learnings
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 126
File: packages/ui/src/theme/context.tsx:11-16
Timestamp: 2026-04-22T09:32:58.310Z
Learning: In Astro-Han/pawwork (`packages/ui/src/theme/context.tsx` and related files), the renaming of localStorage theme keys from `opencode-*` to `pawwork-*` (THEME_ID, COLOR_SCHEME, THEME_CSS_LIGHT, THEME_CSS_DARK) is intentional and should NOT include a migration path from the old keys. Migrating would re-couple PawWork and OpenCode browser storage namespaces, which the PR is explicitly designed to avoid. A reset to the PawWork default theme on upgrade is acceptable by design.
📚 Learning: 2026-04-25T09:19:30.734Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 231
File: packages/desktop-electron/src/main/index.ts:537-537
Timestamp: 2026-04-25T09:19:30.734Z
Learning: In Astro-Han/pawwork (packages/desktop-electron/src/main/), follow the IPC registration convention: the bootstrap entry (packages/desktop-electron/src/main/index.ts) should directly call each module’s exported register*Ipc() function. Do not route/centralize these sub-module IPC registrations through src/main/ipc.ts. Keep sub-module IPC features cohesive (e.g., src/main/ipc/about.ts should own its types/helpers and expose register*Ipc()), and allow index.ts to aggregate by calling each register*Ipc() directly.

Applied to files:

  • packages/desktop-electron/src/main/ipc.ts
📚 Learning: 2026-04-25T12:52:35.631Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 234
File: packages/desktop-electron/src/main/ipc.ts:238-263
Timestamp: 2026-04-25T12:52:35.631Z
Learning: In Astro-Han/pawwork (`packages/desktop-electron/src/main/ipc.ts`), `deps.getServerReadyData()` (backed by `serverReady.promise` in `index.ts`) resolves once at server startup and remains settled; it is not expected to reject in practice. Do not flag the absence of a try-catch around it in the `export-session` IPC handler — the network/fetch layer in `server-client.ts` already has a 10-second AbortController timeout and returns a typed `{ok: false, error}` payload, covering the real failure modes.

Applied to files:

  • packages/desktop-electron/src/main/ipc.ts
📚 Learning: 2026-04-20T14:36:08.774Z
Learnt from: CR
Repo: Astro-Han/pawwork PR: 0
File: packages/desktop-electron/AGENTS.md:0-0
Timestamp: 2026-04-20T14:36:08.774Z
Learning: Applies to packages/desktop-electron/src/**/*.{ts,tsx,js,jsx} : Renderer process should only call `window.api` from `src/preload`

Applied to files:

  • packages/desktop-electron/src/main/ipc.ts
📚 Learning: 2026-04-27T10:33:12.228Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 264
File: packages/opencode/src/session/prompt.ts:108-169
Timestamp: 2026-04-27T10:33:12.228Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/session/prompt.ts` and `packages/opencode/src/session/processor.ts`, PR `#264`), the loop-gate race condition between `buildLoopContext()` and `recordSyntheticBlock`/`recordSyntheticStop` is intentionally handled via idempotence guards (re-check sigKey presence / `hasStopped` inside the record helpers) rather than a full per-parent `Effect.Mutex`. Threading a `Map<MessageID, Mutex>` through the processor was considered too large a surface change for this edge case; the residual TOCTOU window only produces extra synthetic parts with no behavioral drift on the "turn ends" contract. A code comment documents the trade-off and points to a full-mutex follow-up if the race is observed in practice. Do NOT re-flag the absence of a per-parent mutex as a blocking issue in future reviews.

Applied to files:

  • packages/desktop-electron/src/main/ipc.ts
📚 Learning: 2026-04-24T13:03:14.694Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 222
File: packages/desktop-electron/src/main/index.ts:686-692
Timestamp: 2026-04-24T13:03:14.694Z
Learning: In `packages/desktop-electron/src/main/index.ts`, the `checkForUpdates()` function intentionally uses recursive self-invocation for the "Retry" path in the update-check failure dialog. This is mandated by the v5.2 design spec (`#213`): "Await the retry recursion and log any rejection so support can see repeated failures." Because retries are user-paced (require a button click), all prior async frames have already unwound through microtasks before the next attempt, so there is no stack/frame-nesting problem in practice. Do not suggest refactoring this to an iterative loop.

Applied to files:

  • packages/desktop-electron/src/main/ipc.ts
📚 Learning: 2026-04-21T16:57:25.580Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 102
File: packages/opencode/src/config/agent.ts:108-119
Timestamp: 2026-04-21T16:57:25.580Z
Learning: In `packages/opencode/src/config/agent.ts` (Astro-Han/pawwork), `ConfigPermission.Info` only accepts permission objects or the three action strings `"ask"`, `"allow"`, `"deny"`, and transforms those action strings into `{ "*": action }` before `normalize()` runs. By the time `normalize()` is reached, `configuredPermission` is always either `undefined` or a `Record<string, Rule>` — never a raw arbitrary string. The `Object.assign(permission, configuredPermission)` pattern is therefore safe. Do not flag it as corrupting string permission references.

Applied to files:

  • packages/app/src/entry.tsx
📚 Learning: 2026-04-24T05:39:58.329Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 208
File: packages/app/src/components/prompt-input.tsx:1569-1611
Timestamp: 2026-04-24T05:39:58.329Z
Learning: In Astro-Han/pawwork `packages/app/src/components/prompt-input.tsx`, after the composer unification in PR `#208` (fixed in commit 5d810aa):
- `SendButton.disabled` does NOT gate on `store.mode !== "normal"`. Shell mode has a fully visible, clickable orange submit button that calls `handleSubmit` directly (same path as the Enter key in `handleKeyDown`). Do NOT suggest re-adding the mode gate.
- `SendButton` does NOT use the `buttons()` spring opacity animation (`style={buttons()}`). It is always fully visible regardless of mode.
- `WorkspaceChip` is gated on `props.homeMode && store.mode === "normal"` so it hides in shell mode (preventing it from appearing isolated/bright while neighboring controls fade).
- The left-side chip group (`aria-hidden={store.mode !== "normal"}`) covers attach/model/variant/workspace controls only; `SendButton` remains in a separate right-side sibling div.

Applied to files:

  • packages/app/src/entry.tsx
📚 Learning: 2026-04-26T15:35:36.505Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 245
File: packages/opencode/src/question/index.ts:21-24
Timestamp: 2026-04-26T15:35:36.505Z
Learning: In Astro-Han/pawwork (`packages/opencode/src/question/index.ts`), `Question.Option.description` is intentionally **required** (`z.string()`, not `.optional()`). This matches the upstream opencode contract and all current callers (e.g. `PlanExitTool`) always supply a description. The defensive `<Show when={props.description}>` rendering in `session-question-dock.tsx` is a standard guard, not a signal that the field is intended to be optional. Do NOT suggest making `Option.description` optional without a dedicated follow-up that covers schema + tool description + dock copy + tests.

Applied to files:

  • packages/app/src/entry.tsx
📚 Learning: 2026-04-23T07:23:23.849Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 180
File: packages/app/src/components/session/session-new-view.tsx:13-18
Timestamp: 2026-04-23T07:23:23.849Z
Learning: In pawwork (Astro-Han/pawwork), prefer using `createStore` instead of multiple `createSignal` calls only when the signals represent **coupled** object state that is updated together (i.e., there is at least one shared batch-update site where the state is changed in the same transaction). If the state fields are **independent** and are mutated by separate handlers (e.g., one handler updates only `selectedSkill` while another updates only `mode`), keep them as individual `createSignal` calls—using `createStore` for truly independent fields adds boilerplate without behavioral benefit.

Applied to files:

  • packages/app/src/entry.tsx
  • packages/app/src/pages/layout.tsx
📚 Learning: 2026-04-23T15:10:21.635Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 191
File: packages/app/src/components/session/pawwork-skill-meta.ts:38-39
Timestamp: 2026-04-23T15:10:21.635Z
Learning: This repo configures Tailwind v4 with `--color-*: initial`, which effectively breaks standard Tailwind palette utilities (e.g., `text-violet-500` can resolve to no CSS variable and render as a no-op/black). For brand/accent colors that are not backed by semantic design tokens, use inline styles with the exact hex value (e.g., `style={{ color: '#8B5FBF' }}` / `homeIconStyle: { color: '#8B5FBF' }`) and add a short comment explaining that Tailwind palette utilities won’t work due to the `--color-*: initial` setup. Do not suggest replacing these inline hex colors with Tailwind palette classes anywhere in this repo.

Applied to files:

  • packages/app/src/entry.tsx
  • packages/app/src/pages/layout.tsx
📚 Learning: 2026-04-29T13:27:28.494Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 329
File: packages/app/src/pages/session/session-view-controller.test.ts:5-45
Timestamp: 2026-04-29T13:27:28.494Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/session/session-view-controller.test.ts` and related Solid + Bun test files), the Bun + Solid test environment does NOT reliably exercise `createMemo((current) => ...)` signal recomputation the way a browser runtime does. Adding signal-driven transition tests (e.g., using `createSignal` + `createRoot` to flip reactive inputs) is misleading in this environment because the reactive invalidation/recomputation path is not faithfully replicated. The correct strategy is: cover pure transition logic with plain unit tests (e.g., `nextSessionViewState`), and cover the browser reactive path with E2E tests (e.g., session-switch E2E spec). Do NOT re-flag the absence of signal-driven `createSessionViewController` tests in this environment as a gap.

Applied to files:

  • packages/app/src/pages/layout.tsx
📚 Learning: 2026-04-23T15:26:07.250Z
Learnt from: Astro-Han
Repo: Astro-Han/pawwork PR: 193
File: packages/app/src/pages/layout/sidebar-items.tsx:102-107
Timestamp: 2026-04-23T15:26:07.250Z
Learning: In Astro-Han/pawwork (`packages/app/src/pages/layout/sidebar-items.tsx`), the `indicator()` function in `SessionRow` intentionally renders `props.leadingSlot` (the pin button) only as a fallback when no status indicator (running/permission/error/unseen) is active. When a higher-priority status wins the slot, the pin button is removed from the DOM — this is a deliberate design choice for the merged leading slot (`#150`). The keyboard unpin path is preserved via: (1) focusing the row anchor triggers `group-focus-within` which reveals the dots menu trigger, then Tab → Enter → "Unpin Session"; (2) the context menu (right-click / Shift+F10) exposes "Unpin Session". The "always render + CSS overlay" approach was considered but rejected due to z-index/pointer-events complexity; residual `...` slot behavior is tracked in `#192`. Do NOT flag the absence of the pin button from the DOM when a status is active as an accessibility regression.

Applied to files:

  • packages/app/src/pages/layout.tsx
🔇 Additional comments (2)
packages/desktop-electron/src/main/ipc.ts (2)

395-395: Per-window timer map is the right fix for burst notifications.

This prevents older stop-timers from turning off newer flashes on the same window.


397-419: flash-frame handler correctly handles window scoping and timer cleanup.

Good use of BrowserWindow.fromWebContents(event.sender) for multi-window correctness, plus guarded unflash on destroyed windows.

As per coding guidelines: Main process should register IPC handlers in src/main/ipc.ts.

Comment thread packages/app/src/pages/layout.tsx
@Astro-Han Astro-Han merged commit 8514d96 into dev Apr 29, 2026
25 checks passed
@Astro-Han Astro-Han deleted the feat/notification-system-redesign branch April 29, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app Application behavior and product flows enhancement New feature or request P2 Medium priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Replace in-app Toast notifications with native OS notifications

1 participant