feat: command completion notifications (sound, OS notify, highlight)#3294
Open
hyi1233 wants to merge 2 commits intowavetermdev:mainfrom
Open
feat: command completion notifications (sound, OS notify, highlight)#3294hyi1233 wants to merge 2 commits intowavetermdev:mainfrom
hyi1233 wants to merge 2 commits intowavetermdev:mainfrom
Conversation
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughIntroduces end-to-end block completion support. Adds a new CLI command Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
Add three notification features when a background block's command finishes: 1. **Sound** - plays system bell when command completes in an unfocused block 2. **OS Notification** - shows desktop notification with last terminal line as body, clickable to jump to the completed block 3. **Visual Highlight** - blue border glow for success (exit 0), red for failure, auto-clears on focus Also adds `wsh done` CLI command to explicitly signal command completion, publishing a `block:done` WPS event. Includes bell badge on block header that focuses the block when clicked. Three new settings: term:donenotify, term:donesound, term:doneautofocus.
hyi1233
force-pushed
the
feat/command-completion-notifications
branch
from
d695e0b to
1e3e2ec
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/view/term/term-model.ts (1)
672-679:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
blockDoneUnsubFnis never called indispose()— subscription leaks.The constructor at lines 354-360 stores the WPS unsubscribe in
this.blockDoneUnsubFn, butdispose()only invokes the other unsub functions. Each time a terminal block is destroyed and recreated, a staleblock:donehandler is left registered, which will continue to firetriggerCompletionNotificationsagainst a disposed model (causing duplicate notifications/sounds and possibly errors when readingthis.termRef.currentor the static layout).🐛 Proposed fix
dispose() { DefaultRouter.unregisterRoute(makeFeBlockRouteId(this.blockId)); this.shellProcStatusUnsubFn?.(); + this.blockDoneUnsubFn?.(); this.blockJobStatusUnsubFn?.(); this.termBPMUnsubFn?.(); this.termCursorUnsubFn?.(); this.termCursorBlinkUnsubFn?.(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/view/term/term-model.ts` around lines 672 - 679, The dispose() method currently misses calling the stored WebSocket unsubscribe, causing block:done handlers to leak; update dispose() to invoke this.blockDoneUnsubFn?.() alongside the other unsub calls so the subscription created in the constructor (where blockDoneUnsubFn is set) is properly removed and will stop calling triggerCompletionNotifications (which references termRef.current and static layout) after the model is disposed.
🧹 Nitpick comments (3)
cmd/wsh/cmd/wshcmd-done.go (1)
48-56: ⚡ Quick winUse
wps.Event_BlockDoneconstant andwshrpc.BlockDoneEventDatastruct instead of raw literals.The hardcoded string
"block:done"bypasses the existing constant. Additionally,map[string]anyfor the event data is not type-safe — ifBlockDoneEventDatafields are renamed or extended, this code silently diverges. Using the typed struct also respectsomitemptyso emptytitle/messagestrings are excluded from the wire payload.♻️ Proposed refactor
err := wshclient.EventPublishCommand(RpcClient, wps.WaveEvent{ - Event: "block:done", + Event: wps.Event_BlockDone, Scopes: []string{fmt.Sprintf("block:%s", blockId)}, - Data: map[string]any{ - "blockid": blockId, - "exitcode": doneExitCode, - "title": doneTitle, - "message": doneMessage, - }, + Data: wshrpc.BlockDoneEventData{ + BlockId: blockId, + ExitCode: doneExitCode, + Title: doneTitle, + Message: doneMessage, + }, }, &wshrpc.RpcOpts{NoResponse: true})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/wsh/cmd/wshcmd-done.go` around lines 48 - 56, Replace the raw event name and untyped data map in the EventPublishCommand call with the typed constants/structs: use wps.Event_BlockDone for the WaveEvent.Event and populate WaveEvent.Data with a wshrpc.BlockDoneEventData value (set BlockId, ExitCode, Title, Message accordingly) instead of map[string]any; keep calling EventPublishCommand(RpcClient, wps.WaveEvent{...}) and ensure field names match wshrpc.BlockDoneEventData so omitempty on Title/Message is respected and the payload is type-safe.frontend/app/view/term/term-model.ts (1)
580-591: 💤 Low value
getLastTerminalLinescans entire scrollback buffer in worst case.For an empty terminal this iterates
buf.lengthlines (which can be large with scrollback enabled) just to return"". In practice command-completion implies non-empty output, so the loop will return early — but consider bounding the search (e.g. last N=200 lines) defensively, since this runs synchronously on the event handler when the block is unfocused and may include large scrollback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/view/term/term-model.ts` around lines 580 - 591, The getLastTerminalLine method currently scans the entire scrollback (using termRef.current?.terminal.buffer.active and iterating from buf.length-1 down to 0) which can be expensive; limit the search to a bounded tail (e.g., last N = 200 lines) by computing a start index like Math.max(0, buf.length - N) and iterate down to that start instead of 0, keeping the existing null checks (line == null) and translateToString(true).trim() logic and returning "" if nothing found.frontend/app/block/block-model.ts (1)
18-18: 💤 Low valueRedundant cast on the atom initializer.
jotai.atom(new Map())already returns aPrimitiveAtom<Map<...>>; theas jotai.PrimitiveAtom<Map<string, number>>cast just papers over the missing type argument onnew Map(). Prefer typing the value directly.♻️ Cleaner typing
- completionHighlightAtom: jotai.PrimitiveAtom<Map<string, number>> = jotai.atom(new Map()) as jotai.PrimitiveAtom<Map<string, number>>; + completionHighlightAtom: jotai.PrimitiveAtom<Map<string, number>> = jotai.atom(new Map<string, number>());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/block/block-model.ts` at line 18, The completionHighlightAtom line uses a redundant cast; instead of casting the result of jotai.atom(new Map()) to jotai.PrimitiveAtom<Map<string, number>>, initialize the atom with a correctly typed Map by constructing new Map<string, number>() and let jotai.atom infer the PrimitiveAtom<Map<string, number>> type for completionHighlightAtom; update the declaration referencing completionHighlightAtom and jotai.atom accordingly so the explicit "as jotai.PrimitiveAtom<...>" cast is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emain/emain-ipc.ts`:
- Around line 523-527: The code calls ww.setActiveTab(tabId, false)
synchronously then immediately reads ww.allLoadedTabViews and sends
"focus-block", which races with the async tab activation/load; update the flow
to wait for the tab switch/load to complete before sending the message — either
await the promise returned by ww.setActiveTab(tabId, false) if it returns one,
or attach a one-time listener for the tab-ready event emitted by the window
manager (e.g. a "tab-activated" / "tab-loaded" event) and then retrieve
ww.allLoadedTabViews.get(tabId) and call tabView.webContents.send("focus-block",
blockId) only after that completion signal; reference ww.setActiveTab,
ww.allLoadedTabViews, and tabView.webContents.send("focus-block", ...) when
making the change.
In `@emain/preload.ts`:
- Around line 76-79: The preload exposes showCompletionNotification(tabId,
blockId, title, body) which forwards the raw body (often from
getLastTerminalLine()) to the main IPC handler that constructs an OS
Notification; update the main-side "show-completion-notification" handler (the
listener in emain-ipc.ts) to sanitize and truncate the body before calling new
Notification — strip control/ANSI sequences, collapse excessive whitespace,
enforce a safe max length (e.g., ~200 chars) and provide a short fallback like
"[output trimmed]" if truncated or empty; keep the preload's
showCompletionNotification unchanged but document that bodies are clipped by the
main handler.
In `@frontend/app/block/block-model.ts`:
- Around line 53-58: The setCompletionHighlight function currently clears the
entire Map which breaks per-block storage—remove the currentMap.clear() call so
you only set the new entry and preserve other blocks' exit codes; update
setCompletionHighlight(blockId: string, exitCode: number) to clone the atom map
from globalStore.get(this.completionHighlightAtom), set the single key with
currentMap.set(blockId, exitCode), and write it back with
globalStore.set(this.completionHighlightAtom, currentMap) so
getCompletionHighlightAtom(blockId) continues to return per-block state.
In `@frontend/app/block/blockframe.tsx`:
- Around line 76-79: The block that clears style.borderColor and style.boxShadow
when isFocused && completionHighlight != null is redundant and causes a
one-frame visual glitch; remove that conditional clearing (the four lines
referencing style.borderColor/style.boxShadow guarded by isFocused and
completionHighlight) so the active border set by
tabActiveBorderColor/frameActiveBorderColor remains intact while
completionHighlight is handled elsewhere (e.g., the existing guard using
!isFocused and the useEffect).
In `@frontend/app/view/term/term-model.ts`:
- Around line 606-646: triggerCompletionNotifications is repeatedly constructing
derived atoms and using a racy setTimeout-based autofocus; hoist/cache the three
getOverrideConfigAtom reads (term:donesound, term:donenotify,
term:doneautofocus) into local variables by reading the atoms once (use the same
pattern as termThemeNameAtom/termTransparencyAtom via readAtom or useBlockAtom)
before branching, and replace the setActiveTab + setTimeout +
getLayoutModelForStaticTab.focusNode logic with a single IPC-based focus action:
call getApi().setActiveTab(this.tabModel.tabId) and then emit the same
focus-block IPC used by wave.ts's onFocusBlock handler to focus this.blockId (so
focus is performed by the main/process handler rather than relying on timing).
---
Outside diff comments:
In `@frontend/app/view/term/term-model.ts`:
- Around line 672-679: The dispose() method currently misses calling the stored
WebSocket unsubscribe, causing block:done handlers to leak; update dispose() to
invoke this.blockDoneUnsubFn?.() alongside the other unsub calls so the
subscription created in the constructor (where blockDoneUnsubFn is set) is
properly removed and will stop calling triggerCompletionNotifications (which
references termRef.current and static layout) after the model is disposed.
---
Nitpick comments:
In `@cmd/wsh/cmd/wshcmd-done.go`:
- Around line 48-56: Replace the raw event name and untyped data map in the
EventPublishCommand call with the typed constants/structs: use
wps.Event_BlockDone for the WaveEvent.Event and populate WaveEvent.Data with a
wshrpc.BlockDoneEventData value (set BlockId, ExitCode, Title, Message
accordingly) instead of map[string]any; keep calling
EventPublishCommand(RpcClient, wps.WaveEvent{...}) and ensure field names match
wshrpc.BlockDoneEventData so omitempty on Title/Message is respected and the
payload is type-safe.
In `@frontend/app/block/block-model.ts`:
- Line 18: The completionHighlightAtom line uses a redundant cast; instead of
casting the result of jotai.atom(new Map()) to jotai.PrimitiveAtom<Map<string,
number>>, initialize the atom with a correctly typed Map by constructing new
Map<string, number>() and let jotai.atom infer the PrimitiveAtom<Map<string,
number>> type for completionHighlightAtom; update the declaration referencing
completionHighlightAtom and jotai.atom accordingly so the explicit "as
jotai.PrimitiveAtom<...>" cast is removed.
In `@frontend/app/view/term/term-model.ts`:
- Around line 580-591: The getLastTerminalLine method currently scans the entire
scrollback (using termRef.current?.terminal.buffer.active and iterating from
buf.length-1 down to 0) which can be expensive; limit the search to a bounded
tail (e.g., last N = 200 lines) by computing a start index like Math.max(0,
buf.length - N) and iterate down to that start instead of 0, keeping the
existing null checks (line == null) and translateToString(true).trim() logic and
returning "" if nothing found.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f5720e1-2b55-4914-b02c-fa416659f232
📒 Files selected for processing (22)
cmd/wsh/cmd/wshcmd-done.goemain/emain-ipc.tsemain/preload.tsfrontend/app/block/block-model.tsfrontend/app/block/block.scssfrontend/app/block/blockframe-header.tsxfrontend/app/block/blockframe.tsxfrontend/app/view/term/term-model.tsfrontend/preview/mock/preview-electron-api.tsfrontend/types/custom.d.tsfrontend/types/gotypes.d.tsfrontend/types/waveevent.d.tsfrontend/wave.tspkg/tsgen/tsgenevent.gopkg/waveobj/metaconsts.gopkg/waveobj/wtypemeta.gopkg/wconfig/defaultconfig/settings.jsonpkg/wconfig/metaconsts.gopkg/wconfig/settingsconfig.gopkg/wps/wpstypes.gopkg/wshrpc/wshrpctypes.goschema/settings.json
Comment on lines
+523
to
+527
| ww.setActiveTab(tabId, false); | ||
| const tabView = ww.allLoadedTabViews.get(tabId); | ||
| if (tabView) { | ||
| tabView.webContents.send("focus-block", blockId); | ||
| } |
Contributor
There was a problem hiding this comment.
Await tab switch completion before sending focus-block.
Line 523 triggers an async tab switch, but Lines 524-527 execute immediately. If the tab isn’t loaded yet, notification click won’t focus the target block.
Suggested fix
- notification.on("click", () => {
+ notification.on("click", async () => {
const ww = getWaveWindowByWebContentsId(senderWcId);
if (ww == null) return;
if (ww.isMinimized()) {
ww.restore();
}
ww.focus();
- ww.setActiveTab(tabId, false);
+ try {
+ await ww.setActiveTab(tabId, false);
+ } catch (err) {
+ console.error("Failed to activate tab from completion notification:", err);
+ return;
+ }
const tabView = ww.allLoadedTabViews.get(tabId);
- if (tabView) {
- tabView.webContents.send("focus-block", blockId);
- }
+ tabView?.webContents.send("focus-block", blockId);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@emain/emain-ipc.ts` around lines 523 - 527, The code calls
ww.setActiveTab(tabId, false) synchronously then immediately reads
ww.allLoadedTabViews and sends "focus-block", which races with the async tab
activation/load; update the flow to wait for the tab switch/load to complete
before sending the message — either await the promise returned by
ww.setActiveTab(tabId, false) if it returns one, or attach a one-time listener
for the tab-ready event emitted by the window manager (e.g. a "tab-activated" /
"tab-loaded" event) and then retrieve ww.allLoadedTabViews.get(tabId) and call
tabView.webContents.send("focus-block", blockId) only after that completion
signal; reference ww.setActiveTab, ww.allLoadedTabViews, and
tabView.webContents.send("focus-block", ...) when making the change.
Comment on lines
+76
to
+79
| showCompletionNotification: (tabId, blockId, title, body) => | ||
| ipcRenderer.send("show-completion-notification", tabId, blockId, title, body), | ||
| onFocusBlock: (callback) => | ||
| ipcRenderer.on("focus-block", (_event, blockId) => callback(blockId)), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the main-process IPC handler for show-completion-notification to confirm any length/sanitization handling.
rg -nP -C3 'show-completion-notification' --type=tsRepository: wavetermdev/waveterm
Length of output: 1764
🏁 Script executed:
sed -n '507,530p' emain/emain-ipc.tsRepository: wavetermdev/waveterm
Length of output: 1108
🏁 Script executed:
rg -nP 'showCompletionNotification\(' --type=tsRepository: wavetermdev/waveterm
Length of output: 202
🏁 Script executed:
sed -n '610,635p' frontend/app/view/term/term-model.tsRepository: wavetermdev/waveterm
Length of output: 1212
🏁 Script executed:
sed -n '570,625p' frontend/app/view/term/term-model.tsRepository: wavetermdev/waveterm
Length of output: 2277
Consistent with existing preload patterns; one heads-up on downstream handling.
The wiring is correct and matches the conventions used by neighboring API methods (sync send for fire-and-forget, ipcRenderer.on for subscriptions).
One thing to double-check on the main side: the body argument carries arbitrary terminal output (from getLastTerminalLine() in term-model.ts). The handler in emain-ipc.ts currently passes this directly to Electron's Notification constructor without sanitization or length checks. A runaway command could output very long strings or problematic content that then flows into the OS notification subsystem—consider truncating or stripping the body before constructing the Notification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@emain/preload.ts` around lines 76 - 79, The preload exposes
showCompletionNotification(tabId, blockId, title, body) which forwards the raw
body (often from getLastTerminalLine()) to the main IPC handler that constructs
an OS Notification; update the main-side "show-completion-notification" handler
(the listener in emain-ipc.ts) to sanitize and truncate the body before calling
new Notification — strip control/ANSI sequences, collapse excessive whitespace,
enforce a safe max length (e.g., ~200 chars) and provide a short fallback like
"[output trimmed]" if truncated or empty; keep the preload's
showCompletionNotification unchanged but document that bodies are clipped by the
main handler.
Comment on lines
+606
to
+646
| triggerCompletionNotifications(exitCode: number, title: string, notifyBody?: string) { | ||
| const focusManager = FocusManager.getInstance(); | ||
| const focusedBlockId = globalStore.get(focusManager.blockFocusAtom); | ||
| if (focusedBlockId === this.blockId) { | ||
| return; | ||
| } | ||
|
|
||
| const doneSoundEnabled = globalStore.get(getOverrideConfigAtom(this.blockId, "term:donesound")) ?? true; | ||
| if (doneSoundEnabled) { | ||
| fireAndForget(() => | ||
| RpcApi.ElectronSystemBellCommand(TabRpcClient, { route: "electron" }) | ||
| ); | ||
| } | ||
|
|
||
| const doneNotifyEnabled = globalStore.get(getOverrideConfigAtom(this.blockId, "term:donenotify")) ?? true; | ||
| if (doneNotifyEnabled) { | ||
| const body = notifyBody || `exit code ${exitCode}`; | ||
| getApi().showCompletionNotification(this.tabModel.tabId, this.blockId, title, body); | ||
| } | ||
|
|
||
| const doneAutoFocusEnabled = globalStore.get(getOverrideConfigAtom(this.blockId, "term:doneautofocus")) ?? false; | ||
| if (doneAutoFocusEnabled) { | ||
| getApi().setActiveTab(this.tabModel.tabId); | ||
| setTimeout(() => { | ||
| const layoutModel = getLayoutModelForStaticTab(); | ||
| const node = layoutModel?.getNodeByBlockId(this.blockId); | ||
| if (node?.id) { | ||
| layoutModel.focusNode(node.id); | ||
| } | ||
| }, 150); | ||
| } | ||
|
|
||
| BlockModel.getInstance().setCompletionHighlight(this.blockId, exitCode); | ||
|
|
||
| setBadge(this.blockId, { | ||
| badgeid: `done-${this.blockId}`, | ||
| icon: "bell", | ||
| color: exitCode === 0 ? "#3b82f6" : "#ef4444", | ||
| priority: 5, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify wsh done CLI semantics for -t and -m flags and how empty values are handled.
fd -t f 'wshcmd-done.go' | xargs -I{} cat {}Repository: wavetermdev/waveterm
Length of output: 1805
🏁 Script executed:
rg -n "block:done" --type ts --type tsx -B 5 -A 15Repository: wavetermdev/waveterm
Length of output: 92
🏁 Script executed:
rg "block:done" -B 5 -A 15 --type tsRepository: wavetermdev/waveterm
Length of output: 3736
🏁 Script executed:
rg "handleBlockDoneEvent" -B 2 -A 20 --type tsRepository: wavetermdev/waveterm
Length of output: 3691
🏁 Script executed:
rg "termThemeNameAtom|termTransparencyAtom" -B 3 -A 3 --type tsRepository: wavetermdev/waveterm
Length of output: 3998
🏁 Script executed:
rg "useBlockAtom" -B 2 -A 10 --type ts | head -60Repository: wavetermdev/waveterm
Length of output: 4784
🏁 Script executed:
rg "getLayoutModelForStaticTab" -B 3 -A 8 --type tsRepository: wavetermdev/waveterm
Length of output: 35136
🏁 Script executed:
cat -n frontend/app/view/term/term-model.ts | sed -n '593,610p'Repository: wavetermdev/waveterm
Length of output: 978
🏁 Script executed:
rg "setActiveTab" -B 2 -A 5 --type ts | head -40Repository: wavetermdev/waveterm
Length of output: 2425
🏁 Script executed:
rg "staticTabId" -B 2 -A 2 --type ts | head -50Repository: wavetermdev/waveterm
Length of output: 3569
🏁 Script executed:
rg "onFocusBlock" -B 5 -A 10 --type tsRepository: wavetermdev/waveterm
Length of output: 4387
A few concerns in triggerCompletionNotifications.
getOverrideConfigAtom(this.blockId, "term:donesound" | "term:donenotify" | "term:doneautofocus")is invoked three separate times per completion event (lines 613, 620, 626).getOverrideConfigAtomconstructs a derived atom on each call; the established pattern elsewhere in this file (seetermThemeNameAtom,termTransparencyAtom) is to cache viauseBlockAtom. For an event handler the impact is small, but consider hoisting these reads or usingreadAtomconsistently.- The autofocus block (lines 627-636) calls
getApi().setActiveTab(this.tabModel.tabId)and then schedulesgetLayoutModelForStaticTab().focusNode(...)150 ms later.getLayoutModelForStaticTab()resolves against the currently active static tab, so this is racy across tabs (the tab switch may not yet have propagated, especially with reinit). The IPConFocusBlockhandler inwave.tsalready does block focus correctly via the main process and is used for notification clicks; consider routing autofocus through the same mechanism (e.g. emit a focus-block IPC aftersetActiveTab) to keep both paths consistent and avoid the timing hack.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/app/view/term/term-model.ts` around lines 606 - 646,
triggerCompletionNotifications is repeatedly constructing derived atoms and
using a racy setTimeout-based autofocus; hoist/cache the three
getOverrideConfigAtom reads (term:donesound, term:donenotify,
term:doneautofocus) into local variables by reading the atoms once (use the same
pattern as termThemeNameAtom/termTransparencyAtom via readAtom or useBlockAtom)
before branching, and replace the setActiveTab + setTimeout +
getLayoutModelForStaticTab.focusNode logic with a single IPC-based focus action:
call getApi().setActiveTab(this.tabModel.tabId) and then emit the same
focus-block IPC used by wave.ts's onFocusBlock handler to focus this.blockId (so
focus is performed by the main/process handler rather than relying on timing).
- Add blockDoneUnsubFn call in dispose() to prevent subscription leak - Remove currentMap.clear() from setCompletionHighlight to allow multiple blocks to be highlighted simultaneously - Use typed wps.Event_BlockDone constant and wshrpc.BlockDoneEventData struct instead of raw string/map[string]any in wsh done command - Remove redundant style-clearing block in blockframe.tsx - Bound getLastTerminalLine to last 200 lines of scrollback - Remove redundant type cast on completionHighlightAtom
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds three notification features for when a background block's command finishes, similar to what cmux provides:
Also adds a bell badge on the block header that focuses the block when clicked.
New
wsh donecommandPublishes a
block:doneWPS event that the frontend subscribes to.New settings
term:donenotifytrueterm:donesoundtrueterm:doneautofocusfalseAll settings support block-level overrides via meta.
Files Changed
Go backend (5 files)
pkg/wshrpc/wshrpctypes.go—BlockDoneEventDatatypepkg/wps/wpstypes.go—block:doneevent registrationpkg/tsgen/tsgenevent.go— TS type generation mappingpkg/wconfig/settingsconfig.go+pkg/waveobj/wtypemeta.go— 3 new config fieldspkg/wconfig/defaultconfig/settings.json— defaultsNew file (1 file)
cmd/wsh/cmd/wshcmd-done.go—wsh doneCLI commandElectron main process (2 files)
emain/emain-ipc.ts— notification IPC handler with click-to-focusemain/preload.ts— expose IPC methodsFrontend (6 files)
frontend/app/view/term/term-model.ts— core: subscribes toblock:done, reads last terminal line, triggers sound/notify/highlight/badgefrontend/app/block/block-model.ts— completion highlight state (one block at a time)frontend/app/block/blockframe.tsx— renders highlight glow + auto-clear on focusfrontend/app/block/blockframe-header.tsx— clickable bell badgefrontend/app/block/block.scss— highlight transition CSSfrontend/wave.ts—onFocusBlockcallback for notification click-to-jumpGenerated (5 files) — via
task generateTest Plan
sleep 3 && wsh donein one, focus the othersleep 2 && exit 1 && wsh done -e 1→ verify red highlight and "Command Failed" titlewsh done -m "custom message"→ verify notification shows custom messageterm:donenotify=false→ verify no OS notificationterm:donesound=false→ verify no bell soundterm:doneautofocus=true→ verify auto-focus on completion