Tab background glow on process exit/bell#2835
Tab background glow on process exit/bell#2835JustHereToHelp wants to merge 1 commit intowavetermdev:mainfrom
Conversation
|
|
WalkthroughAdds a terminal exit indicator feature. New config key Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/config.mdx (1)
108-108:⚠️ Potential issue | 🟡 MinorStale version reference in the default config example.
The header says "v0.11.5" but the config block now includes keys from v0.14 (e.g.,
term:bellsound,term:bellindicator,term:exitindicator). Consider updating the version reference to match.
🤖 Fix all issues with AI agents
In `@frontend/app/tab/tab.tsx`:
- Around line 227-233: The background glow fails when indicator exists but
indicator.color is falsy because the style omits --tab-indicator-color; update
the style expression around the indicator handling so that when indicator !=
null you always set "--tab-indicator-color" to indicator.color ??
"<amber-default>" (the same amber used for the icon) instead of only when
indicator.color is truthy, preserving the existing className usage
("has-indicator") and ensuring the CSS variable is present for the SCSS rule;
modify the style logic in the Tab component where indicator and style are
computed to use this fallback.
In `@pkg/blockcontroller/shellcontroller.go`:
- Around line 630-633: The block-level check for the term exit indicator
currently reads only blockData.Meta.GetBool(waveobj.MetaKey_TermExitIndicator,
false) and returns early, so global config is ignored; update the logic in the
same area to fall back to the global settings from
wconfig.GetWatcher().GetFullConfig().Settings when the block meta key is unset
(mirror the pattern used by getLocalShellPath/getLocalShellOpts). Specifically,
read settings := wconfig.GetWatcher().GetFullConfig().Settings, keep the initial
exitIndicatorEnabled from
blockData.Meta.GetBool(waveobj.MetaKey_TermExitIndicator, false), and if that is
false/unspecified and settings.TermExitIndicator != nil then set
exitIndicatorEnabled = *settings.TermExitIndicator before using it.
🧹 Nitpick comments (2)
frontend/app/tab/tab.scss (1)
99-106: Indicator glow is overridden on hover.The hover rule at lines 137–139 (
body:not(.nohover) .tab:hover .tab-inner) has higher specificity than.tab.has-indicator .tab-inner, so the colored glow disappears when the user hovers over an indicator tab. If you want the glow to persist on hover, you could add a.has-indicatoroverride inside the hover block, e.g.:♻️ Optional: preserve glow on hover
body:not(.nohover) .tab:hover, body:not(.nohover) .tab.dragging { ... .tab-inner { border-color: transparent; background: rgb(from var(--main-text-color) r g b / 0.1); } + &.has-indicator .tab-inner { + background: rgb(from var(--tab-indicator-color) r g b / 0.15); + } ... }pkg/blockcontroller/shellcontroller.go (1)
619-665: Duplicated block-data fetch and close-on-exit logic withcheckCloseOnExit.This goroutine fetches
blockDataand evaluates the samecloseOnExit/closeOnExitForceconditions thatcheckCloseOnExit(Line 666→697) also fetches and evaluates independently. Consider extracting the shared block-data fetch and close-on-exit predicate into a helper, or restructuring so both consumers share a single DB read.Not a correctness issue — just duplication that could drift over time.
JustHereToHelp
force-pushed
the
feat/tab-exit-glow
branch
3 times, most recently
from
7b7eaf2 to
7014aa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/config.mdx (1)
108-108:⚠️ Potential issue | 🟡 MinorStale version reference in the default config block.
The header says "current default configuration (v0.11.5)" but now includes settings like
term:exitindicator,term:bellsound,term:bellindicator, etc. that were introduced in v0.14. Consider updating this version string.
🧹 Nitpick comments (3)
pkg/blockcontroller/shellcontroller.go (3)
647-717: Duplicated clear-event block can be hoisted.Lines 671-678 and 683-690 publish the exact same clear event. Since both the early-return path (closeOnExit) and the normal exit path need to clear the running indicator first, hoist the clear event before the
if closeOnExitForce || ...check to remove duplication.♻️ Suggested refactor
- if closeOnExitForce || (closeOnExit && exitCode == 0) { - // Clear running indicator before block gets deleted - if bc.ControllerType == BlockController_Cmd { - clearEvent := wps.WaveEvent{ - Event: wps.Event_TabIndicator, - Scopes: []string{waveobj.MakeORef(waveobj.OType_Tab, bc.TabId).String()}, - Data: wshrpc.TabIndicatorEventData{TabId: bc.TabId, Indicator: nil}, - } - wps.Broker.Publish(clearEvent) - } - return - } - // Clear running indicator before exit indicator to prevent - // PersistentIndicator from resurrecting the amber glow - if bc.ControllerType == BlockController_Cmd { - clearEvent := wps.WaveEvent{ - Event: wps.Event_TabIndicator, - Scopes: []string{waveobj.MakeORef(waveobj.OType_Tab, bc.TabId).String()}, - Data: wshrpc.TabIndicatorEventData{TabId: bc.TabId, Indicator: nil}, - } - wps.Broker.Publish(clearEvent) + // Clear running indicator before block deletion or exit indicator + // to prevent PersistentIndicator from resurrecting the amber glow + if bc.ControllerType == BlockController_Cmd { + clearEvent := wps.WaveEvent{ + Event: wps.Event_TabIndicator, + Scopes: []string{waveobj.MakeORef(waveobj.OType_Tab, bc.TabId).String()}, + Data: wshrpc.TabIndicatorEventData{TabId: bc.TabId, Indicator: nil}, + } + wps.Broker.Publish(clearEvent) + } + if closeOnExitForce || (closeOnExit && exitCode == 0) { + return }
531-536: Consider extracting the exit-indicator-enabled check into a helper.The same pattern — read from block meta, then fall back to global settings — appears at both start (lines 531-536) and exit (lines 658-663). A small helper like
isExitIndicatorEnabled(meta MetaMapType) boolwould reduce duplication and keep the fallback logic in one place.Also applies to: 658-663
667-680: Potential race withcheckCloseOnExit— benign but worth a note.Both this goroutine (line 647) and
checkCloseOnExit(line 718) run concurrently, each independently fetching block data and evaluatingcloseOnExit/closeOnExitForce. The indicator-clear event here may race with block deletion incheckCloseOnExit. This is benign (clearing an indicator on a soon-to-be-deleted block is a no-op), but a comment noting the intentional concurrency would help future readers.
When term:exitindicator is enabled, tabs show a colored background glow reflecting process state: - Amber spinning indicator while a cmd block is running - Green glow when process exits successfully (exit 0) - Red glow when process errors (non-zero exit or signal kill) Uses the existing tab indicator system with a priority hierarchy: bell (1) < running (1.5) < exit (2). Running indicator has ClearOnFocus=false so it persists while the process is active. Exit indicators auto-clear on focus via ClearOnFocus. Skips the indicator when cmd:closeonexit would auto-delete the block. Clears the running indicator before setting exit indicator to prevent PersistentIndicator from resurrecting the amber glow. The glow effect also applies to existing bell indicators, giving them a colored background tint for better visibility. Closes wavetermdev#2834
JustHereToHelp
force-pushed
the
feat/tab-exit-glow
branch
from
7014aa5 to
29a2424
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/blockcontroller/shellcontroller.go (2)
647-717: Exit indicator goroutine is well-structured. Fresh DB read, proper panic handling, parameter capture forexitCode/exitSignal, and the clear-before-set pattern to avoid PersistentIndicator resurrection are all solid.Minor DRY note: the clear-event construction is duplicated at lines 672-678 and 684-690. A small helper (or hoisting to a local closure) would reduce the repetition, but this is optional.
667-680: Potential race withcheckCloseOnExiton the same DB read.Both this goroutine and
checkCloseOnExit(line 718) independently fetchblockDataand evaluatecloseOnExit/closeOnExitForce. SincecheckCloseOnExitsleeps forcloseonexitdelay(default 2s) before deleting the block, the ordering is safe in practice—but ifcloseonexitdelayis set to0, the block could be deleted before this goroutine finishes publishing the clear event. Currently harmless (publishing to a deleted tab is a no-op on the frontend), but worth being aware of.
| // Fire amber "running" indicator for cmd blocks | ||
| if bc.ControllerType == BlockController_Cmd { | ||
| exitIndicatorEnabled := blockMeta.GetBool(waveobj.MetaKey_TermExitIndicator, false) | ||
| if !blockMeta.HasKey(waveobj.MetaKey_TermExitIndicator) { | ||
| if globalVal := wconfig.GetWatcher().GetFullConfig().Settings.TermExitIndicator; globalVal != nil { | ||
| exitIndicatorEnabled = *globalVal | ||
| } | ||
| } | ||
| if exitIndicatorEnabled { | ||
| indicator := wshrpc.TabIndicator{ | ||
| Icon: "spinner+spin", | ||
| Color: "#f59e0b", | ||
| Priority: 1.5, | ||
| ClearOnFocus: false, | ||
| } | ||
| eventData := wshrpc.TabIndicatorEventData{ | ||
| TabId: bc.TabId, | ||
| Indicator: &indicator, | ||
| } | ||
| event := wps.WaveEvent{ | ||
| Event: wps.Event_TabIndicator, | ||
| Scopes: []string{waveobj.MakeORef(waveobj.OType_Tab, bc.TabId).String()}, | ||
| Data: eventData, | ||
| } | ||
| wps.Broker.Publish(event) | ||
| } | ||
| } |
There was a problem hiding this comment.
Running indicator logic looks good. Global settings fallback is correctly implemented, and the indicator is appropriately scoped to BlockController_Cmd blocks only.
One edge case: if the user disables term:exitindicator while a command is still running, the exit goroutine (line 664) returns early without clearing this amber spinner—leaving it stuck on the tab. Consider unconditionally clearing the running indicator for Cmd blocks before the exitIndicatorEnabled early-return:
Proposed fix
+ // Always clear the running indicator for cmd blocks
+ if blockData.Meta.GetString(waveobj.MetaKey_Controller, "") == BlockController_Cmd {
+ clearEvent := wps.WaveEvent{
+ Event: wps.Event_TabIndicator,
+ Scopes: []string{waveobj.MakeORef(waveobj.OType_Tab, bc.TabId).String()},
+ Data: wshrpc.TabIndicatorEventData{TabId: bc.TabId, Indicator: nil},
+ }
+ wps.Broker.Publish(clearEvent)
+ }
if !exitIndicatorEnabled {
return
}
|
Hey, thanks for the thorough review! Just pushed a fix for both actionable items: CSS variable fallback — good catch, I updated the condition to check Global config fallback — this was actually already fixed in the latest push (the review might've run against the earlier commit). The exit goroutine checks block meta first with Hover glow — fixed that too, added Duplicated block-data fetch — yeah I noticed that. Kept it separate intentionally since the exit indicator goroutine runs async and the close-on-exit check has its own lifecycle, but I agree it could be cleaner. Happy to refactor if the team prefers. Docs version reference — that v0.11.5 header was already there before my change, didn't want to touch unrelated stuff in this PR. Can update it in a follow-up if you want. |
2 participants
Follows up on #2834 — I wanted the tab indicator system to be a bit more noticeable, so I added a subtle background color glow that uses the indicator's color, plus a running state for cmd blocks.
What it does:
Three states for cmd blocks when
term:exitindicatoris enabled:The existing bell indicator also picks up the background glow since the CSS applies to any active indicator.
Priority hierarchy:
The running indicator is cleared before setting the exit indicator to prevent the PersistentIndicator logic from resurrecting the amber glow after the exit indicator clears. Also skips both indicators when
cmd:closeonexitwould auto-delete the block.Only cmd blocks get the running indicator — regular shells are always "running" so they'd be permanently amber.
Config:
Files changed (10 files, 114 insertions):
tab:indicatorevents on cmd start and shell process exit.has-indicatorclass withrgb(from ... / 0.15)background tintThe glow uses 15% opacity (20% for active tab) so it's visible but not overwhelming.