Skip to content

feat: Codespaces support, notifications, and build fixes#15

Open
edu-ap wants to merge 1 commit into
T3-Content:mainfrom
edu-ap:pr/codespace-support
Open

feat: Codespaces support, notifications, and build fixes#15
edu-ap wants to merge 1 commit into
T3-Content:mainfrom
edu-ap:pr/codespace-support

Conversation

@edu-ap

@edu-ap edu-ap commented Mar 19, 2026

Copy link
Copy Markdown

What this does

Makes Claude Blocker work with GitHub Codespaces (and any remote dev environment), adds Chrome notifications when Claude needs input, and fixes several build/runtime issues.

Codespaces support

  • Server auto-detects Codespace environment and prints the forwarded WebSocket URL
  • Extension settings page lets users configure a custom server URL (for wss:// forwarded ports)
  • README section with setup instructions and architecture diagram

Notifications

  • Chrome notification fires when Claude stops working and needs your input
  • 30s cooldown to avoid spam
  • Click notification to focus the Codespace/terminal tab
  • Toggle in extension settings

Bug fixes

  • Shared package build: was exporting raw .ts files which broke consumers. Now compiles to JS via tsup
  • Missing dependency: @claude-blocker/shared wasn't listed in server's package.json
  • Hook stdin: $(cat) swallowed stdin on some shells. Changed to curl -d @-
  • Blocking logic: now blocks unless all sessions are actively working (previously allowed distractions if any session was idle but another was waiting for input)
  • --no-setup flag: skip interactive hook prompt for CI/scripted installs

Testing

  • Tested in a GitHub Codespace with Claude Code running behind a forwarded port
  • Verified Chrome notifications fire and clicking focuses the correct tab
  • Build passes with pnpm build

Note

Add GitHub Codespaces support, configurable WebSocket URL, and Chrome notifications to the extension

  • Adds a GitHub Codespaces mode to the server: when Codespaces environment variables are detected, the server logs a wss:// forwarded URL and setup instructions on startup.
  • Makes the WebSocket server URL configurable via the extension options page, persisted in chrome.storage.sync, with the service worker reconnecting automatically when it changes.
  • Adds Chrome notifications (requiring the new notifications permission) that fire when Claude finishes work and is waiting for input, and clear when work resumes. Clicking the notification focuses a relevant Codespaces tab.
  • Adds a --no-setup flag to the server CLI to skip interactive hook setup prompting.
  • Changes the hook curl command in setup.ts to read the request body from stdin via -d @- instead of command substitution.
  • Behavioral Change: the extension now blocks unless all sessions are actively working; idle or waiting-for-input states will also trigger blocking.

Macroscope summarized acc4830.

Summary by CodeRabbit

  • New Features

    • Configurable server URL in extension options and a notifications toggle for input-needed alerts.
    • Extension now displays connection status and supports interactive notifications that focus relevant tabs.
  • Documentation

    • README updated with startup note and step-by-step GitHub Codespaces setup and connection diagram.
  • Chores

    • Extension permissions updated for notifications; server prints Codespaces forwarded URL; setup and packaging/build configs refined.

@coderabbitai

coderabbitai Bot commented Mar 19, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Add configurable WebSocket URL and notifications support to the browser extension, Codespaces-aware server logging and setup tweaks, UI/settings for server URL and notifications, hook-install command change, and shared package build/export configuration updates.

Changes

Cohort / File(s) Summary
Documentation
README.md
Added restart note for Claude Code hooks and a new "Using with GitHub Codespaces" guide with steps, port-forwarding, extension configuration, and ASCII diagram.
Extension Manifest & Workspace
packages/extension/manifest.json, pnpm-workspace.yaml
Added "notifications" permission to the extension manifest; adjusted workspace glob and added ignoredBuiltDependencies entries.
Extension UI & Styling
packages/extension/src/options.html, packages/extension/src/options.css
Added Server Connection and Notifications sections to options UI and new CSS for layout, buttons, and server-status states.
Extension Logic
packages/extension/src/options.ts, packages/extension/src/service-worker.ts
Options: persisted configurable serverUrl, validation, save/reset, and notifications toggle. Service worker: loads runtime config from storage, reacts to storage changes (reconnect), manages notification creation/clearing with cooldown, handles notification clicks to focus GitHub.dev tabs, and adjusts blocked/connected logic.
Server runtime & setup
packages/server/src/server.ts, packages/server/src/bin.ts, packages/server/src/setup.ts
Server: detect GitHub Codespaces and log forwarded wss://...app.github.dev/ws URL. CLI: add --no-setup gating for interactive hook setup. Hooks: change curl payload mode from -d "$(cat)" to -d @-``.
Shared package & server deps
packages/shared/package.json, packages/server/package.json
packages/shared: switch package entrypoints to dist/, add conditional exports and build script with tsup/typescript devDeps. packages/server: add @claude-blocker/shared dependency.

Sequence Diagram(s)

sequenceDiagram
  participant UI as Options UI
  participant SW as Service Worker
  participant Server as WebSocket Server (Codespace)
  participant Notif as Chrome Notifications
  participant Tabs as Browser Tabs

  UI->>SW: Save new serverUrl (chrome.storage.sync)
  SW->>SW: reloadConfig & reconnect WebSocket
  SW->>Server: Open ws connection -> /ws
  Server->>SW: state message (e.g., waitingForInput/working counts)
  alt input needed
    SW->>Notif: maybeNotifyInputNeeded() -> create "claude-input-needed"
    Notif-->>SW: notification created
  else cleared
    SW->>Notif: maybeClearNotification() -> clear notification
  end
  Notif->>Tabs: onClicked -> search/focus `.github.dev` tab
  Tabs-->>Notif: tab focused/activated
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped into Codespaces bright,

WsURLs gleam in morning light,
Notifications chirp, tabs leap to view,
Hooks now sip stdin, clean and true,
Hooray — the blocker’s better, through and through!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the three main changes: Codespaces support, notifications feature, and build fixes, which are all core objectives addressed by the changeset.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai 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.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/extension/src/service-worker.ts (1)

108-124: ⚠️ Potential issue | 🔴 Critical

Blocking logic is inverted—implements stricter behavior than documented product intent.

The code blocks sites when ANY session is idle (shouldBlock = !bypassActive && !allWorking where allWorking = state.working === state.sessions). However, the documented expected behavior for Claude Blocker is to block ONLY when NO sessions are actively working—sites should unblock as soon as ANY session becomes active.

This code change implements the opposite: it blocks if ANY session is not working, making the blocker significantly stricter than intended. Per the Claude Blocker documentation, the intent is to block during multi-session idle periods, not to block whenever one session pauses.

const allWorking = state.serverConnected && state.sessions > 0 && state.working === state.sessions;
const shouldBlock = !bypassActive && !allWorking;

The logic needs to align with: block when working === 0 (no sessions working), not when working < sessions (not all sessions working).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extension/src/service-worker.ts` around lines 108 - 124, The
blocking logic in getPublicState is inverted: it computes allWorking and sets
shouldBlock when not all sessions are working; change it to only block when no
sessions are active. Update getPublicState so bypassActive stays the same but
shouldBlock is computed as !bypassActive && state.serverConnected &&
state.sessions > 0 && state.working === 0 (i.e., block only when working === 0),
and remove/replace the allWorking usage or rename it to reflect the new
semantics; ensure returned blocked still uses this corrected shouldBlock value.
🧹 Nitpick comments (2)
packages/extension/src/service-worker.ts (2)

93-106: Consider consolidating the two chrome.storage.onChanged listeners.

There are two separate listeners for storage changes—one for serverUrl (lines 94-106) and one for notificationsEnabled (lines 276-280). These could be combined into a single listener for slightly cleaner code.

♻️ Optional consolidation
 chrome.storage.onChanged.addListener((changes, areaName) => {
-  if (areaName === "sync" && changes.serverUrl) {
+  if (areaName !== "sync") return;
+
+  if (changes.serverUrl) {
     const newUrl = changes.serverUrl.newValue || DEFAULT_WS_URL;
     if (newUrl !== wsUrl) {
       console.log("[Claude Blocker] Server URL changed, reconnecting...");
       wsUrl = newUrl;
-      // Close existing connection to trigger reconnect
       if (websocket) {
         websocket.close();
       }
     }
   }
+
+  if (changes.notificationsEnabled) {
+    notificationsEnabled = changes.notificationsEnabled.newValue ?? true;
+  }
 });
-
-// Listen for notification setting changes from options page
-chrome.storage.onChanged.addListener((changes, areaName) => {
-  if (areaName === "sync" && changes.notificationsEnabled) {
-    notificationsEnabled = changes.notificationsEnabled.newValue ?? true;
-  }
-});

Also applies to: 275-280

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extension/src/service-worker.ts` around lines 93 - 106, Consolidate
the two chrome.storage.onChanged listeners into one handler: replace separate
listeners that check for changes.serverUrl (updating wsUrl, logging, and closing
websocket) and changes.notificationsEnabled with a single
chrome.storage.onChanged.addListener callback that inspects both
changes.serverUrl and changes.notificationsEnabled; for serverUrl handle newUrl
= changes.serverUrl.newValue || DEFAULT_WS_URL and the reconnect logic (compare
to wsUrl, set wsUrl, console.log and close websocket if present), and for
notificationsEnabled update the existing notificationsEnabled behavior—keep the
same checks and side effects but centralize them in the single listener.

256-273: Tab-finding logic is limited to Codespace URLs.

The click handler only searches for tabs with .github.dev URLs. If users run Claude Code locally (outside Codespaces), clicking the notification won't focus any terminal. Consider documenting this limitation or falling back gracefully.

const claudeTab = tabs.find((t) =>
  t.url?.includes(".github.dev") || t.url?.includes("github.dev/")
);

This is reasonable for the Codespaces feature scope, but users might expect the notification click to do something useful in local setups too.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extension/src/service-worker.ts` around lines 256 - 273, The click
handler registered on chrome.notifications.onClicked only searches tabs via the
tabs.find call for Codespaces (.github.dev), so clicking the
"claude-input-needed" notification does nothing for local setups; update the
handler to fall back when claudeTab is not found — after the existing tabs.find
(variable claudeTab) logic, add a graceful fallback such as searching for a more
general URL pattern (e.g., localhost or file://), or if still not found, open or
focus a known extension page/popup (e.g., chrome.runtime.openOptionsPage or a
dedicated tab) so the user sees where to input text; modify the code in
service-worker.ts inside the chrome.notifications.onClicked listener and keep
the existing tab-focusing logic (chrome.tabs.update / chrome.windows.update)
when a tab is found.
🤖 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/extension/src/options.html`:
- Around line 103-109: The input element with id "server-url" lacks an
accessible name; add one by either adding a visible <label
for="server-url">Server URL</label> immediately associated with the input or by
adding an aria-label="Server URL" (or aria-labelledby pointing to a label
element) to the element so screen readers will announce the field; keep the
existing placeholder but ensure the label text clearly describes the input.

In `@packages/extension/src/options.ts`:
- Around line 313-340: The click handler attached to saveServerBtn uses
serverUrlInput without guarding for null; inside the async listener for
saveServerBtn.addEventListener("click", ...) add an early null check like if
(!serverUrlInput) return; before accessing serverUrlInput.value.trim() to avoid
runtime errors if the DOM element is missing; keep the rest of the flow
(isValidWebSocketUrl, saveServerUrl, updating currentServerUrl and serverStatus)
unchanged and ensure serverStatus is still null-checked where used.

In `@README.md`:
- Around line 99-101: Update the fenced code block in README.md that shows the
WebSocket URL so it includes a language specifier (e.g., use ```text) instead of
a bare ```; locate the block containing
"wss://YOUR-CODESPACE-NAME-8765.app.github.dev/ws" and change the opening fence
to include the specifier to satisfy linters and static analysis.
- Around line 110-125: Update the ASCII diagram code block in README.md by
adding the language specifier "text" to the opening fenced code block (replace
the existing ``` with ```text) so the static analysis no longer flags it as
missing a language; ensure the closing ``` remains unchanged and the diagram
content (the box art) is not modified.

---

Outside diff comments:
In `@packages/extension/src/service-worker.ts`:
- Around line 108-124: The blocking logic in getPublicState is inverted: it
computes allWorking and sets shouldBlock when not all sessions are working;
change it to only block when no sessions are active. Update getPublicState so
bypassActive stays the same but shouldBlock is computed as !bypassActive &&
state.serverConnected && state.sessions > 0 && state.working === 0 (i.e., block
only when working === 0), and remove/replace the allWorking usage or rename it
to reflect the new semantics; ensure returned blocked still uses this corrected
shouldBlock value.

---

Nitpick comments:
In `@packages/extension/src/service-worker.ts`:
- Around line 93-106: Consolidate the two chrome.storage.onChanged listeners
into one handler: replace separate listeners that check for changes.serverUrl
(updating wsUrl, logging, and closing websocket) and
changes.notificationsEnabled with a single chrome.storage.onChanged.addListener
callback that inspects both changes.serverUrl and changes.notificationsEnabled;
for serverUrl handle newUrl = changes.serverUrl.newValue || DEFAULT_WS_URL and
the reconnect logic (compare to wsUrl, set wsUrl, console.log and close
websocket if present), and for notificationsEnabled update the existing
notificationsEnabled behavior—keep the same checks and side effects but
centralize them in the single listener.
- Around line 256-273: The click handler registered on
chrome.notifications.onClicked only searches tabs via the tabs.find call for
Codespaces (.github.dev), so clicking the "claude-input-needed" notification
does nothing for local setups; update the handler to fall back when claudeTab is
not found — after the existing tabs.find (variable claudeTab) logic, add a
graceful fallback such as searching for a more general URL pattern (e.g.,
localhost or file://), or if still not found, open or focus a known extension
page/popup (e.g., chrome.runtime.openOptionsPage or a dedicated tab) so the user
sees where to input text; modify the code in service-worker.ts inside the
chrome.notifications.onClicked listener and keep the existing tab-focusing logic
(chrome.tabs.update / chrome.windows.update) when a tab is found.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6118105b-af74-4c1a-a828-6751f2afe718

📥 Commits

Reviewing files that changed from the base of the PR and between 06f03fa and 8064d91.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (12)
  • README.md
  • packages/extension/manifest.json
  • packages/extension/src/options.css
  • packages/extension/src/options.html
  • packages/extension/src/options.ts
  • packages/extension/src/service-worker.ts
  • packages/server/package.json
  • packages/server/src/bin.ts
  • packages/server/src/server.ts
  • packages/server/src/setup.ts
  • packages/shared/package.json
  • pnpm-workspace.yaml

Comment thread packages/extension/src/options.html
Comment thread packages/extension/src/options.ts
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment on lines +110 to +125
```
┌─────────────────────┐ ┌─────────────────────┐
│ GitHub Codespace │ │ Your Local PC │
│ │ │ │
│ ┌───────────────┐ │ Forwarded │ ┌───────────────┐ │
│ │ Claude Code │ │ Port │ │ Chrome │ │
│ │ (hooks) │──┼──► 8765 ◄────────┼──│ Extension │ │
│ └───────────────┘ │ │ └───────────────┘ │
│ │ │ │ │ │
│ ▼ │ │ ▼ │
│ ┌───────────────┐ │ │ Blocks sites when │
│ │ Server │ │ │ Claude is idle │
│ │ (localhost) │ │ │ │
│ └───────────────┘ │ │ │
└─────────────────────┘ └─────────────────────┘
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add a language specifier to the ASCII diagram code block.

Static analysis flags this code block as missing a language specifier. Use text for ASCII diagrams.

-```
+```text
 ┌─────────────────────┐                    ┌─────────────────────┐
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)

[warning] 110-110: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 110 - 125, Update the ASCII diagram code block in
README.md by adding the language specifier "text" to the opening fenced code
block (replace the existing ``` with ```text) so the static analysis no longer
flags it as missing a language; ensure the closing ``` remains unchanged and the
diagram content (the box art) is not modified.

- Add GitHub Codespaces support: configurable server URL in extension
  settings, Codespace detection in server with forwarded URL instructions
- Add Chrome notifications when Claude finishes and needs input, with
  cooldown and click-to-focus
- Fix shared package build: compile to JS with tsup instead of raw .ts
  exports, add missing @claude-blocker/shared workspace dependency
- Fix hook stdin: use curl -d @- instead of $(cat) for reliable piping
- Fix blocking logic: block unless ALL sessions are actively working
- Add --no-setup flag to skip interactive hook setup prompt
- Add note in README about restarting Claude Code after hook setup
@edu-ap
edu-ap force-pushed the pr/codespace-support branch from 8064d91 to acc4830 Compare March 19, 2026 08:22
@edu-ap

edu-ap commented Mar 19, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review @coderabbitai! Pushed fixes for the minor items (aria-label, null check, markdown lint, consolidated storage listeners).

Re: blocking logic — the stricter behaviour is intentional. The original logic allowed distractions when any session was idle but another was waiting for input. In practice this meant you could browse YouTube while Claude was waiting for your answer in another terminal, which defeats the purpose. The new logic: distractions are only allowed when every session is actively working (i.e., you genuinely have nothing to do). Happy to make this configurable if the maintainer prefers, but I believe this better matches the tool's "if Claude is working, you should be too" ethos.

@coderabbitai

coderabbitai Bot commented Mar 19, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@edu-ap: Thanks for the follow-up and for pushing those fixes!

The blocking logic rationale makes complete sense — allowing distractions while Claude is waiting for your input in another session does undermine the tool's core purpose. The stricter "block unless all sessions are actively working" semantics is a meaningful improvement and the intent is well-documented here. I'm happy to accept that as intentional design.


✏️ Learnings added
Learnt from: edu-ap
URL: https://github.com/T3-Content/claude-blocker/pull/15

Timestamp: 2026-03-19T08:23:36.738Z
Learning: In `packages/extension/src/service-worker.ts` of the `claude-blocker` repo, the blocking logic change (PR `#15`) is **intentional**: distractions are only unblocked when *every* Claude session is actively working (`serverConnected && sessions > 0 && working === sessions`). The original logic incorrectly allowed distractions when any session was idle but another was waiting for user input, which defeated the tool's purpose. The design ethos is "if Claude is working (or waiting for you), you should be too."

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/extension/src/service-worker.ts (1)

115-126: ⚠️ Potential issue | 🟡 Minor

blocked now means more than "idle", but the current UI still treats it as idle-only.

The modal is shown whenever state.blocked is true (packages/extension/src/content-script.ts:229-256), but the renderer in packages/extension/src/content-script.ts:184-209 still turns every blocked state into "Your job finished" / "n sessions idle". With this broader blocked rule, mixed working+waiting states will now show incorrect status copy unless the contract or consumer rendering is updated.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extension/src/service-worker.ts` around lines 115 - 126, The public
state’s blocked boolean from getPublicState() now covers mixed working+waiting
states, but the UI in content-script.ts still treats blocked as “idle” only;
update one side to match: either change getPublicState() to expose an explicit
idle flag (e.g., isIdle = !bypassActive && state.working === 0 && state.sessions
> 0) or add a status field/enum (e.g., status: "idle"|"working"|"waiting"
derived from state.working and state.waitingForInput) so the renderer (the modal
and the renderer logic in packages/extension/src/content-script.ts) can use
working, waitingForInput, or the new status/isIdle to show the correct copy
instead of assuming blocked==idle.
🧹 Nitpick comments (1)
packages/extension/src/service-worker.ts (1)

262-279: Track the Claude tab instead of guessing from github.dev.

Now that serverUrl is configurable, the first .github.dev tab is only a Codespaces heuristic, and it can still be the wrong repo/window when several are open. Persisting the last relevant tab/window from extension traffic would make notification-click focus much more reliable for both Codespaces and other remote setups.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extension/src/service-worker.ts` around lines 262 - 279, The
notification-click handler in chrome.notifications.onClicked uses a brittle URL
heuristic (checking ".github.dev") to find the Claude tab; instead persist the
last active Claude tab/window when the extension interacts with Claude (e.g., in
the code paths that open or message the Claude UI—handlers that create the
notification or handle runtime messages). Add a small helper (e.g.,
saveLastClaudeTab(tabId, windowId) that writes {tabId, windowId} to
chrome.storage.local or a module-level variable) and call it whenever you open
or route traffic to Claude; then update the chrome.notifications.onClicked
listener to first read/validate the stored tab/window (using chrome.tabs.get and
chrome.windows.update) and only fall back to the existing URL heuristic if the
stored tab is missing or invalid.
🤖 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/extension/src/service-worker.ts`:
- Around line 39-72: The notification logic currently gates on changes to
state.working (in maybeNotifyInputNeeded and maybeClearNotification) which
causes false positives/negatives; change the gating to use state.waitingForInput
and a new previousWaiting snapshot instead: in maybeNotifyInputNeeded, check if
state.waitingForInput > previousWaiting (and maintain the
NOTIFY_COOLDOWN_MS/lastNotifyTime logic), then create the "claude-input-needed"
notification and update lastNotifyTime; in maybeClearNotification, clear the
notification when state.waitingForInput < previousWaiting (i.e., no prompts
waiting). Add/update the previousWaiting variable alongside previousWorking (or
replace previousWorking usage where appropriate) so the code compares
waitingForInput deltas, and ensure previousWaiting is updated after handling.

---

Outside diff comments:
In `@packages/extension/src/service-worker.ts`:
- Around line 115-126: The public state’s blocked boolean from getPublicState()
now covers mixed working+waiting states, but the UI in content-script.ts still
treats blocked as “idle” only; update one side to match: either change
getPublicState() to expose an explicit idle flag (e.g., isIdle = !bypassActive
&& state.working === 0 && state.sessions > 0) or add a status field/enum (e.g.,
status: "idle"|"working"|"waiting" derived from state.working and
state.waitingForInput) so the renderer (the modal and the renderer logic in
packages/extension/src/content-script.ts) can use working, waitingForInput, or
the new status/isIdle to show the correct copy instead of assuming
blocked==idle.

---

Nitpick comments:
In `@packages/extension/src/service-worker.ts`:
- Around line 262-279: The notification-click handler in
chrome.notifications.onClicked uses a brittle URL heuristic (checking
".github.dev") to find the Claude tab; instead persist the last active Claude
tab/window when the extension interacts with Claude (e.g., in the code paths
that open or message the Claude UI—handlers that create the notification or
handle runtime messages). Add a small helper (e.g., saveLastClaudeTab(tabId,
windowId) that writes {tabId, windowId} to chrome.storage.local or a
module-level variable) and call it whenever you open or route traffic to Claude;
then update the chrome.notifications.onClicked listener to first read/validate
the stored tab/window (using chrome.tabs.get and chrome.windows.update) and only
fall back to the existing URL heuristic if the stored tab is missing or invalid.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4e7c82e9-2cff-4af6-9e81-151e002b35c8

📥 Commits

Reviewing files that changed from the base of the PR and between 8064d91 and acc4830.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (12)
  • README.md
  • packages/extension/manifest.json
  • packages/extension/src/options.css
  • packages/extension/src/options.html
  • packages/extension/src/options.ts
  • packages/extension/src/service-worker.ts
  • packages/server/package.json
  • packages/server/src/bin.ts
  • packages/server/src/server.ts
  • packages/server/src/setup.ts
  • packages/shared/package.json
  • pnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (6)
  • packages/server/package.json
  • pnpm-workspace.yaml
  • packages/server/src/bin.ts
  • README.md
  • packages/extension/manifest.json
  • packages/extension/src/options.css
🚧 Files skipped from review as they are similar to previous changes (5)
  • packages/server/src/setup.ts
  • packages/server/src/server.ts
  • packages/extension/src/options.ts
  • packages/extension/src/options.html
  • packages/shared/package.json

Comment on lines +39 to +72
// Notify when any Claude session stops working
function maybeNotifyInputNeeded(): void {
if (!notificationsEnabled) return;
if (state.sessions === 0) return;

const now = Date.now();
if (now - lastNotifyTime < NOTIFY_COOLDOWN_MS) return;

// Notify when working count decreases (a session finished)
if (state.working < previousWorking) {
lastNotifyTime = now;
const title = state.waitingForInput > 0
? "Claude is waiting for your answer"
: "Claude needs your input";
// Clear first to ensure Chrome shows a fresh notification
chrome.notifications.clear("claude-input-needed", () => {
chrome.notifications.create("claude-input-needed", {
type: "basic",
iconUrl: "icon-128.png",
title,
message: "Claude Code has finished and is waiting for you.",
priority: 2,
requireInteraction: true,
});
});
}
}

// Clear notification when user submits input (working resumes)
function maybeClearNotification(): void {
if (state.working > previousWorking) {
chrome.notifications.clear("claude-input-needed");
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Gate notification creation/clearing on waitingForInput, not working deltas.

Line 47 fires on any drop in working, including "one session exited while another kept running", so this can show a "needs input" notification when no prompt is pending. The inverse problem exists on Line 69 too: if the waiting session disappears, the notification never clears because working did not increase.

Suggested fix
 function maybeNotifyInputNeeded(): void {
   if (!notificationsEnabled) return;
-  if (state.sessions === 0) return;
+  if (state.waitingForInput === 0) return;

   const now = Date.now();
   if (now - lastNotifyTime < NOTIFY_COOLDOWN_MS) return;

   // Notify when working count decreases (a session finished)
   if (state.working < previousWorking) {
     lastNotifyTime = now;
     const title = state.waitingForInput > 0
       ? "Claude is waiting for your answer"
       : "Claude needs your input";
     // Clear first to ensure Chrome shows a fresh notification
     chrome.notifications.clear("claude-input-needed", () => {
       chrome.notifications.create("claude-input-needed", {
         type: "basic",
         iconUrl: "icon-128.png",
         title,
         message: "Claude Code has finished and is waiting for you.",
         priority: 2,
         requireInteraction: true,
       });
     });
   }
 }

 function maybeClearNotification(): void {
-  if (state.working > previousWorking) {
+  if (state.waitingForInput === 0) {
     chrome.notifications.clear("claude-input-needed");
   }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extension/src/service-worker.ts` around lines 39 - 72, The
notification logic currently gates on changes to state.working (in
maybeNotifyInputNeeded and maybeClearNotification) which causes false
positives/negatives; change the gating to use state.waitingForInput and a new
previousWaiting snapshot instead: in maybeNotifyInputNeeded, check if
state.waitingForInput > previousWaiting (and maintain the
NOTIFY_COOLDOWN_MS/lastNotifyTime logic), then create the "claude-input-needed"
notification and update lastNotifyTime; in maybeClearNotification, clear the
notification when state.waitingForInput < previousWaiting (i.e., no prompts
waiting). Add/update the previousWaiting variable alongside previousWorking (or
replace previousWorking usage where appropriate) so the code compares
waitingForInput deltas, and ensure previousWaiting is updated after handling.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant