Tab background glow on process exit/bell #2835
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a terminal exit indicator feature (config key Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
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. |
Updated: Tab indicator states in actionThree states visible — grey (done, already viewed), amber breathing (working), green (done, needs attention). Using this daily across 8-10 Claude Code tabs and it's been a massive workflow improvement. |
JustHereToHelp
force-pushed
the
feat/tab-exit-glow
branch
from
cb2fc47 to
4f17e5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/tab/tab.tsx`:
- Line 232: The two fallback hex colors for the tab indicator are inconsistent
(one uses "#f59e0b" and the other "#fbbf24"); change both places to use a single
shared constant (e.g., DEFAULT_INDICATOR_COLOR) and replace the inline fallbacks
in the expressions that read indicator.color || ... so both the background glow
and icon color use the same fallback value; update usages where the ternary/OR
fallback appears (the indicator color expressions) to reference that constant.
🧹 Nitpick comments (1)
frontend/app/tab/tab.scss (1)
121-137: Consider extracting indicator background rules into a shared mixin to reduce duplication.The indicator background rules are repeated in both the default
.tabcontext (Lines 121-137) and the hover/drag context (Lines 172-183). While the duplication is necessary for specificity overrides, a SCSS mixin could keep them in sync:♻️ Suggested mixin
+@mixin indicator-backgrounds { + &.has-indicator .tab-inner { + background: rgb(from var(--tab-indicator-color) r g b / 0.15); + } + &.has-indicator.active .tab-inner { + background: rgb(from var(--tab-indicator-color) r g b / 0.2); + } + &.indicator-breathing .tab-inner { + background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14)); + } + &.indicator-breathing.active .tab-inner { + background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14)); + } +}Then use
@include indicator-backgrounds;in both contexts.Also applies to: 172-183
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@frontend/app/tab/tab.scss`:
- Around line 123-139: Change the hardcoded alpha decimals to percentage
notation and add blank lines before the nested active rules: update the
.has-indicator .tab-inner background alpha from 0.15 to 15% and the
.has-indicator &.active .tab-inner from 0.2 to 20%; ensure there is an empty
line separating the .tab-inner block and the &.active .tab-inner block; leave
the .indicator-breathing calc(...) alpha expressions unchanged but also add the
empty line before its &.active .tab-inner rule for consistency.
- Around line 174-185: The hover/drag block uses the wrong alpha notation and
misses required empty lines: for the selectors like `.has-indicator .tab-inner`,
`.has-indicator.active .tab-inner`, `.indicator-breathing .tab-inner`, and
`.indicator-breathing.active .tab-inner` replace the fractional alpha values
(e.g. 0.15, 0.2, calc(...) with fractional) with percentage-style alpha values
(e.g. 15%, 20%, calc(... * 100% or equivalent) to satisfy the
alpha-value-notation rule) and ensure there is a blank line inserted between
these rule blocks to satisfy rule-empty-line-before.
- Around line 6-24: Rename the keyframes identifier from indicatorBreathePhase
to kebab-case indicator-breathe-phase and update its usage: change the
`@keyframes` rule name (indicatorBreathePhase → indicator-breathe-phase) and
update the animation declaration on :root (animation: indicatorBreathePhase →
animation: indicator-breathe-phase) so the keyframes-name-pattern Stylelint rule
is satisfied.
In `@frontend/app/view/term/termwrap.ts`:
- Around line 541-564: The dragover and drop event handlers attached to
connectElem are not removed on disposal; add their removal to the component's
disposal tracking by creating named handler functions (e.g., onConnectDragOver,
onConnectDrop), attach them with connectElem.addEventListener, and push
disposables to toDispose that call connectElem.removeEventListener for each
handler (similar to how the paste listener is tracked). This ensures
connectElem's dragover/drop listeners are cleaned up when the component is
disposed.
- Line 557: The manual POSIX-only quoting in termwrap.ts (const quoted = "'" +
filePath.replace(/'/g, "'\\''") + "'") breaks on Windows; replace this with a
platform-aware quoting call using the existing shell-quote utility (the same
approach used in preview-directory.tsx) or detect process.platform and apply
Windows cmd/PowerShell-safe quoting. Update the code that builds the quoted
variable (referencing filePath and the quoted identifier in termwrap.ts) to call
the shared shell-quote function (or a thin wrapper that switches behavior by
platform) so the generated command is safe for both POSIX shells and Windows
shells listed in shellblocking.ts.
🧹 Nitpick comments (2)
pkg/wshrpc/wshremote/wshremote_file.go (1)
61-71:.Info()called repeatedly in the sort comparator — potential O(n log n) stat syscalls.Each comparison invokes
.Info()on both entries. The GoDirEntry.Info()contract does not guarantee caching, so on some filesystems/platforms each call may re-stat. For large directories this is expensive, and the results are thrown away only to be re-fetched at line 92.Consider pre-fetching
Info()once per entry into a parallel slice (or a[]struct{ entry; modTime }) before sorting, which also eliminates the second.Info()call in the iteration loop below.♻️ Suggested approach — cache mod times before sorting
- sort.Slice(innerFilesEntries, func(i, j int) bool { - iInfo, iErr := innerFilesEntries[i].Info() - jInfo, jErr := innerFilesEntries[j].Info() - if iErr != nil { - return false - } - if jErr != nil { - return true - } - return iInfo.ModTime().After(jInfo.ModTime()) - }) + type dirEntryWithTime struct { + entry os.DirEntry + modTime time.Time + err error + } + enriched := make([]dirEntryWithTime, len(innerFilesEntries)) + for i, e := range innerFilesEntries { + info, err := e.Info() + if err != nil { + enriched[i] = dirEntryWithTime{entry: e, err: err} + } else { + enriched[i] = dirEntryWithTime{entry: e, modTime: info.ModTime()} + } + } + sort.Slice(enriched, func(i, j int) bool { + if enriched[i].err != nil { + return false + } + if enriched[j].err != nil { + return true + } + return enriched[i].modTime.After(enriched[j].modTime) + }) + // Rebuild the slice so downstream code is unchanged. + for i, e := range enriched { + innerFilesEntries[i] = e.entry + }frontend/app/tab/tab.scss (1)
12-14: Unconditional:rootanimation runs even when no indicator is active.The 16s animation on
:rootruns continuously for all users, including those who never enable indicators. This is a minor performance/battery concern. Consider scoping it so it only runs when at least one tab has the.indicator-breathingclass, or accept the cost as negligible.
| @property --breathe-phase { | ||
| syntax: "<number>"; | ||
| initial-value: 0; | ||
| inherits: true; | ||
| } | ||
|
|
||
| :root { | ||
| animation: indicatorBreathePhase 16s ease-in-out infinite; | ||
| } | ||
|
|
||
| @keyframes indicatorBreathePhase { | ||
| 0%, | ||
| 100% { | ||
| --breathe-phase: 0; | ||
| } | ||
| 50% { | ||
| --breathe-phase: 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Rename keyframes to kebab-case to satisfy Stylelint.
indicatorBreathePhase triggers the keyframes-name-pattern rule. Rename to indicator-breathe-phase and update the reference on Line 13.
Proposed fix
-@property --breathe-phase {
+@property --breathe-phase {
syntax: "<number>";
initial-value: 0;
inherits: true;
}
:root {
- animation: indicatorBreathePhase 16s ease-in-out infinite;
+ animation: indicator-breathe-phase 16s ease-in-out infinite;
}
-@keyframes indicatorBreathePhase {
+@keyframes indicator-breathe-phase {
0%,
100% {
--breathe-phase: 0;
}
+
50% {
--breathe-phase: 1;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @property --breathe-phase { | |
| syntax: "<number>"; | |
| initial-value: 0; | |
| inherits: true; | |
| } | |
| :root { | |
| animation: indicatorBreathePhase 16s ease-in-out infinite; | |
| } | |
| @keyframes indicatorBreathePhase { | |
| 0%, | |
| 100% { | |
| --breathe-phase: 0; | |
| } | |
| 50% { | |
| --breathe-phase: 1; | |
| } | |
| } | |
| `@property` --breathe-phase { | |
| syntax: "<number>"; | |
| initial-value: 0; | |
| inherits: true; | |
| } | |
| :root { | |
| animation: indicator-breathe-phase 16s ease-in-out infinite; | |
| } | |
| `@keyframes` indicator-breathe-phase { | |
| 0%, | |
| 100% { | |
| --breathe-phase: 0; | |
| } | |
| 50% { | |
| --breathe-phase: 1; | |
| } | |
| } |
🧰 Tools
🪛 Stylelint (17.2.0)
[error] 16-16: Expected keyframe name "indicatorBreathePhase" to be kebab-case (keyframes-name-pattern)
(keyframes-name-pattern)
[error] 21-23: Expected empty line before rule (rule-empty-line-before)
(rule-empty-line-before)
🤖 Prompt for AI Agents
In `@frontend/app/tab/tab.scss` around lines 6 - 24, Rename the keyframes
identifier from indicatorBreathePhase to kebab-case indicator-breathe-phase and
update its usage: change the `@keyframes` rule name (indicatorBreathePhase →
indicator-breathe-phase) and update the animation declaration on :root
(animation: indicatorBreathePhase → animation: indicator-breathe-phase) so the
keyframes-name-pattern Stylelint rule is satisfied.
| &.has-indicator { | ||
| .tab-inner { | ||
| background: rgb(from var(--tab-indicator-color) r g b / 0.15); | ||
| } | ||
| &.active .tab-inner { | ||
| background: rgb(from var(--tab-indicator-color) r g b / 0.2); | ||
| } | ||
| } | ||
|
|
||
| &.indicator-breathing { | ||
| .tab-inner { | ||
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14)); | ||
| } | ||
| &.active .tab-inner { | ||
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stylelint: use percentage alpha notation and add empty lines before nested rules.
Static analysis flags alpha-value-notation (use 15%/20% instead of 0.15/0.2) and missing empty lines before the &.active rules. The calc() expressions in .indicator-breathing can't easily use % notation, so those are fine as-is, but the static values on Lines 125 and 128 should use percentages.
Proposed fix
&.has-indicator {
.tab-inner {
- background: rgb(from var(--tab-indicator-color) r g b / 0.15);
+ background: rgb(from var(--tab-indicator-color) r g b / 15%);
}
+
&.active .tab-inner {
- background: rgb(from var(--tab-indicator-color) r g b / 0.2);
+ background: rgb(from var(--tab-indicator-color) r g b / 20%);
}
}
&.indicator-breathing {
.tab-inner {
background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14));
}
+
&.active .tab-inner {
background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14));
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &.has-indicator { | |
| .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / 0.15); | |
| } | |
| &.active .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / 0.2); | |
| } | |
| } | |
| &.indicator-breathing { | |
| .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14)); | |
| } | |
| &.active .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14)); | |
| } | |
| } | |
| &.has-indicator { | |
| .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / 15%); | |
| } | |
| &.active .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / 20%); | |
| } | |
| } | |
| &.indicator-breathing { | |
| .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14)); | |
| } | |
| &.active .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14)); | |
| } | |
| } |
🧰 Tools
🪛 Stylelint (17.2.0)
[error] 125-125: Expected "0.15" to be "15%" (alpha-value-notation)
(alpha-value-notation)
[error] 128-128: Expected "0.2" to be "20%" (alpha-value-notation)
(alpha-value-notation)
[error] 127-129: Expected empty line before rule (rule-empty-line-before)
(rule-empty-line-before)
[error] 136-138: Expected empty line before rule (rule-empty-line-before)
(rule-empty-line-before)
🤖 Prompt for AI Agents
In `@frontend/app/tab/tab.scss` around lines 123 - 139, Change the hardcoded alpha
decimals to percentage notation and add blank lines before the nested active
rules: update the .has-indicator .tab-inner background alpha from 0.15 to 15%
and the .has-indicator &.active .tab-inner from 0.2 to 20%; ensure there is an
empty line separating the .tab-inner block and the &.active .tab-inner block;
leave the .indicator-breathing calc(...) alpha expressions unchanged but also
add the empty line before its &.active .tab-inner rule for consistency.
| &.has-indicator .tab-inner { | ||
| background: rgb(from var(--tab-indicator-color) r g b / 0.15); | ||
| } | ||
| &.has-indicator.active .tab-inner { | ||
| background: rgb(from var(--tab-indicator-color) r g b / 0.2); | ||
| } | ||
| &.indicator-breathing .tab-inner { | ||
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14)); | ||
| } | ||
| &.indicator-breathing.active .tab-inner { | ||
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14)); | ||
| } |
There was a problem hiding this comment.
Same Stylelint fixes needed in the hover/drag block.
The same alpha-value-notation and rule-empty-line-before violations apply here. Apply the matching percentage notation and empty-line fixes.
Proposed fix
+
&.has-indicator .tab-inner {
- background: rgb(from var(--tab-indicator-color) r g b / 0.15);
+ background: rgb(from var(--tab-indicator-color) r g b / 15%);
}
+
&.has-indicator.active .tab-inner {
- background: rgb(from var(--tab-indicator-color) r g b / 0.2);
+ background: rgb(from var(--tab-indicator-color) r g b / 20%);
}
+
&.indicator-breathing .tab-inner {
background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14));
}
+
&.indicator-breathing.active .tab-inner {
background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &.has-indicator .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / 0.15); | |
| } | |
| &.has-indicator.active .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / 0.2); | |
| } | |
| &.indicator-breathing .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14)); | |
| } | |
| &.indicator-breathing.active .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14)); | |
| } | |
| &.has-indicator .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / 15%); | |
| } | |
| &.has-indicator.active .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / 20%); | |
| } | |
| &.indicator-breathing .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.08 + var(--breathe-phase) * 0.14)); | |
| } | |
| &.indicator-breathing.active .tab-inner { | |
| background: rgb(from var(--tab-indicator-color) r g b / calc(0.12 + var(--breathe-phase) * 0.14)); | |
| } |
🧰 Tools
🪛 Stylelint (17.2.0)
[error] 175-175: Expected "0.15" to be "15%" (alpha-value-notation)
(alpha-value-notation)
[error] 178-178: Expected "0.2" to be "20%" (alpha-value-notation)
(alpha-value-notation)
[error] 174-176: Expected empty line before rule (rule-empty-line-before)
(rule-empty-line-before)
[error] 177-179: Expected empty line before rule (rule-empty-line-before)
(rule-empty-line-before)
[error] 180-182: Expected empty line before rule (rule-empty-line-before)
(rule-empty-line-before)
[error] 183-185: Expected empty line before rule (rule-empty-line-before)
(rule-empty-line-before)
🤖 Prompt for AI Agents
In `@frontend/app/tab/tab.scss` around lines 174 - 185, The hover/drag block uses
the wrong alpha notation and misses required empty lines: for the selectors like
`.has-indicator .tab-inner`, `.has-indicator.active .tab-inner`,
`.indicator-breathing .tab-inner`, and `.indicator-breathing.active .tab-inner`
replace the fractional alpha values (e.g. 0.15, 0.2, calc(...) with fractional)
with percentage-style alpha values (e.g. 15%, 20%, calc(... * 100% or
equivalent) to satisfy the alpha-value-notation rule) and ensure there is a
blank line inserted between these rule blocks to satisfy rule-empty-line-before.
frontend/app/view/term/termwrap.ts
Outdated
| this.connectElem.addEventListener("dragover", (e: DragEvent) => { | ||
| e.preventDefault(); | ||
| if (e.dataTransfer) { | ||
| e.dataTransfer.dropEffect = "copy"; | ||
| } | ||
| }); | ||
| this.connectElem.addEventListener("drop", (e: DragEvent) => { | ||
| e.preventDefault(); | ||
| if (!e.dataTransfer || e.dataTransfer.files.length === 0) { | ||
| return; | ||
| } | ||
| const paths: string[] = []; | ||
| for (let i = 0; i < e.dataTransfer.files.length; i++) { | ||
| const file = e.dataTransfer.files[i]; | ||
| const filePath = getApi().getPathForFile(file); | ||
| if (filePath) { | ||
| const quoted = "'" + filePath.replace(/'/g, "'\\''") + "'"; | ||
| paths.push(quoted); | ||
| } | ||
| } | ||
| if (paths.length > 0) { | ||
| this.terminal.paste(paths.join(" ")); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Drag-and-drop event listeners are not cleaned up on dispose.
The dragover and drop listeners on connectElem are never removed, unlike the paste listener (Line 566-572) which is properly tracked in toDispose. This creates a minor leak if the terminal is destroyed while the DOM element persists.
Proposed fix — track listeners for disposal
- this.connectElem.addEventListener("dragover", (e: DragEvent) => {
+ const dragOverHandler = (e: DragEvent) => {
e.preventDefault();
if (e.dataTransfer) {
e.dataTransfer.dropEffect = "copy";
}
- });
- this.connectElem.addEventListener("drop", (e: DragEvent) => {
+ };
+ const dropHandler = (e: DragEvent) => {
e.preventDefault();
if (!e.dataTransfer || e.dataTransfer.files.length === 0) {
return;
}
const paths: string[] = [];
for (let i = 0; i < e.dataTransfer.files.length; i++) {
const file = e.dataTransfer.files[i];
const filePath = getApi().getPathForFile(file);
if (filePath) {
const quoted = "'" + filePath.replace(/'/g, "'\\''") + "'";
paths.push(quoted);
}
}
if (paths.length > 0) {
this.terminal.paste(paths.join(" "));
}
- });
+ };
+ this.connectElem.addEventListener("dragover", dragOverHandler);
+ this.connectElem.addEventListener("drop", dropHandler);
+ this.toDispose.push({
+ dispose: () => {
+ this.connectElem.removeEventListener("dragover", dragOverHandler);
+ this.connectElem.removeEventListener("drop", dropHandler);
+ },
+ });🤖 Prompt for AI Agents
In `@frontend/app/view/term/termwrap.ts` around lines 541 - 564, The dragover and
drop event handlers attached to connectElem are not removed on disposal; add
their removal to the component's disposal tracking by creating named handler
functions (e.g., onConnectDragOver, onConnectDrop), attach them with
connectElem.addEventListener, and push disposables to toDispose that call
connectElem.removeEventListener for each handler (similar to how the paste
listener is tracked). This ensures connectElem's dragover/drop listeners are
cleaned up when the component is disposed.
frontend/app/view/term/termwrap.ts
Outdated
| const file = e.dataTransfer.files[i]; | ||
| const filePath = getApi().getPathForFile(file); | ||
| if (filePath) { | ||
| const quoted = "'" + filePath.replace(/'/g, "'\\''") + "'"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file and its context around line 557
if [ -f "frontend/app/view/term/termwrap.ts" ]; then
echo "=== Context around line 557 ==="
sed -n '545,570p' frontend/app/view/term/termwrap.ts
echo ""
echo "=== File size check ==="
wc -l frontend/app/view/term/termwrap.ts
else
echo "File not found"
git ls-files | grep -i termwrap
fiRepository: wavetermdev/waveterm
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Search for platform-aware quoting and shell escaping utilities
echo "=== Searching for shell quoting utilities ==="
rg -n "shellQuote|quoteForShell|escapePath|quoteArg" --type=ts --type=js -i
echo ""
echo "=== Searching for platform detection (Windows checks) ==="
rg -n "windows|win32|process\.platform" --type=ts --type=js | head -30Repository: wavetermdev/waveterm
Length of output: 3268
🏁 Script executed:
#!/bin/bash
# Search for context of where this quoted path is used
echo "=== Searching for usage context of the quoted variable ==="
rg -n -B5 -A5 "replace.*'\\\\\\\\''" --type=tsRepository: wavetermdev/waveterm
Length of output: 1051
🏁 Script executed:
#!/bin/bash
# Check if Wave supports Windows terminal and what kind of shells
echo "=== Checking for terminal type references ==="
rg -n "cmd\.exe|powershell|terminal.*windows|windows.*shell" --type=ts --type=js -i | head -20Repository: wavetermdev/waveterm
Length of output: 631
🏁 Script executed:
#!/bin/bash
# Check the terminal.paste() implementation
echo "=== Looking for terminal.paste() implementation ==="
rg -n "paste\(" --type=ts -A3 frontend/app/view/term/termwrap.ts | head -20
echo ""
echo "=== Check what type 'terminal' is ==="
rg -n "this\.terminal\s*=" --type=ts frontend/app/view/term/termwrap.ts -B5 -A2
echo ""
echo "=== Search for Terminal class definition ==="
rg -n "class.*Terminal|interface.*Terminal" --type=ts frontend/app/view/term/ | head -20Repository: wavetermdev/waveterm
Length of output: 1511
🏁 Script executed:
#!/bin/bash
# Check if there's existing platform-aware shell quoting or if Windows terminals get special handling
echo "=== Check for platform-specific path/quoting handling ==="
rg -n "platform|unamePlatform|win32" --type=ts frontend/app/view/term/
echo ""
echo "=== Search for how shell-quote is used ==="
rg -n -B3 -A3 "shell-quote\|shellQuote" --type=ts --type=tsxRepository: wavetermdev/waveterm
Length of output: 505
🏁 Script executed:
#!/bin/bash
# Check if paste() does any shell detection/handling
echo "=== Search for Terminal implementation ==="
fd "terminal" --type f | grep -E "\.(ts|tsx)$" | grep -v node_modules | head -10
echo ""
echo "=== Check xterm terminal implementation ==="
rg -n "class.*Terminal\|interface.*Terminal" --type=ts --type=tsx | grep -v ".d.ts"Repository: wavetermdev/waveterm
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Let's search for import statements to understand Terminal type
echo "=== Looking for Terminal import ==="
rg -n "import.*Terminal" --type=ts frontend/app/view/term/termwrap.ts
echo ""
echo "=== Check how platform is used in termwrap ==="
rg -n "PLATFORM|isWindows" --type=ts frontend/app/view/term/termwrap.ts -B2 -A2
echo ""
echo "=== Look at the actual paste handling in term-model ==="
rg -n "paste\|Ctrl-V" --type=ts frontend/app/view/term/term-model.ts -B3 -A3 | head -40Repository: wavetermdev/waveterm
Length of output: 1016
🏁 Script executed:
#!/bin/bash
# Check what getPathForFile does - understand if paths are already sanitized
echo "=== Search for getPathForFile implementation ==="
rg -n "getPathForFile" --type=ts --type=js -B2 -A5 | head -50
echo ""
echo "=== Look for file path handling in API ==="
rg -n "function.*getPathForFile\|getPathForFile.*=" --type=ts --type=js -B2 -A3Repository: wavetermdev/waveterm
Length of output: 1986
🏁 Script executed:
#!/bin/bash
# Understand the architecture - is this quoting sent to shell or processed elsewhere?
echo "=== Check if paths are sent to shell or processed by Wave ==="
rg -n "terminal.paste\|this\.terminal\.paste" --type=ts -B5 -A5
echo ""
echo "=== Check shell integration ==="
rg -n "shellIntegration\|send.*command" --type=ts frontend/app/view/term/ | head -15Repository: wavetermdev/waveterm
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check what shell is typically used on Windows
echo "=== Check for shell selection/connection logic ==="
rg -n "shell.*select\|getShell\|activeShell" --type=ts | head -20
echo ""
echo "=== Check cmd.exe references and how it's handled ==="
rg -n "cmd\.exe\|cmd\|powershell" --type=ts -B2 -A2 | head -40
echo ""
echo "=== Look for OS-specific shell selection ==="
rg -n "defaultShell\|shell.*detect" --type=ts -B2 -A2Repository: wavetermdev/waveterm
Length of output: 215
🏁 Script executed:
#!/bin/bash
# Check if this terminal.paste data goes through any sanitization after
echo "=== Check xterm.js paste behavior ==="
rg -n "onData\|onPaste\|paste" --type=ts frontend/app/view/term/termwrap.ts -B2 -A2
echo ""
echo "=== Search for where paste data is actually sent ==="
rg -n "sendData\|send.*command\|executeCommand" --type=ts frontend/app/view/term/ | head -20Repository: wavetermdev/waveterm
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Key question: is shell-quote used anywhere for similar purposes?
echo "=== Compare with shell-quote usage ==="
cat frontend/app/view/preview/preview-directory.tsx | head -40
echo ""
echo "=== Look at the context where shell-quote is used ==="
rg -n -B5 -A5 'shellQuote\(' frontend/app/view/preview/preview-directory.tsxRepository: wavetermdev/waveterm
Length of output: 2477
🏁 Script executed:
#!/bin/bash
# Let's understand the architecture better - where does terminal.paste data go?
echo "=== Check for terminal write or send handlers ==="
rg -n "terminal\.write\|_writeData\|sendData" --type=ts frontend/app/view/term/termwrap.ts -B2 -A2
echo ""
echo "=== Look at xterm addon usage ==="
rg -n "SerializeAddon\|addon" --type=ts frontend/app/view/term/termwrap.ts | head -20
echo ""
echo "=== Check if there's any documentation or comments about Windows support ==="
rg -n "windows.*shell\|cmd.*support\|Windows.*terminal" --type=ts --type=js frontend/ | grep -v node_modules | head -20Repository: wavetermdev/waveterm
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Check if paste data goes through any filtering or shell detection
echo "=== Check term-model for shell-specific handling ==="
cat frontend/app/view/term/term-model.ts | head -100
echo ""
echo "=== Look for shell type in term-model ==="
rg -n "shellType\|shellName\|shell.*type\|cmd\|PowerShell\|bash" --type=ts frontend/app/view/term/term-model.ts | head -30Repository: wavetermdev/waveterm
Length of output: 4203
🏁 Script executed:
#!/bin/bash
# Check if there's any actual issue reported or platform-specific handling for file paths
echo "=== Check issues or comments about Windows file handling ==="
rg -n "TODO\|FIXME\|BUG.*windows\|windows.*path" --type=ts -i | head -20
echo ""
echo "=== Check how cmd shell is actually invoked/supported ==="
rg -n "cmd\.exe\|windows.*shell" --type=ts --type=js --type=go | head -30Repository: wavetermdev/waveterm
Length of output: 183
Use platform-aware shell quoting for Windows compatibility.
The POSIX single-quote escaping is correct for bash/zsh but will fail on Windows cmd.exe or PowerShell. Since Wave Terminal supports both (cmd listed in shellblocking.ts), use the shell-quote library already in the codebase (see preview-directory.tsx) instead of manual escaping, or detect the platform and apply appropriate quoting.
🤖 Prompt for AI Agents
In `@frontend/app/view/term/termwrap.ts` at line 557, The manual POSIX-only
quoting in termwrap.ts (const quoted = "'" + filePath.replace(/'/g, "'\\''") +
"'") breaks on Windows; replace this with a platform-aware quoting call using
the existing shell-quote utility (the same approach used in
preview-directory.tsx) or detect process.platform and apply Windows
cmd/PowerShell-safe quoting. Update the code that builds the quoted variable
(referencing filePath and the quoted identifier in termwrap.ts) to call the
shared shell-quote function (or a thin wrapper that switches behavior by
platform) so the generated command is safe for both POSIX shells and Windows
shells listed in shellblocking.ts.
Adds a term:exitindicator config that shows colored tab indicators when processes exit — amber breathing while running, green check on success, red X on error. Uses CSS @Property for a synchronized breathing phase across all tabs (16s ease-in-out cycle). Tab rendering: icon="none" hides the icon for glow-only working state, has-indicator class adds colored background, indicator-breathing class adds the breathing pulse. Active tabs get slightly higher opacity. Go backend: ShellController publishes amber running indicator for cmd blocks on start, green/red exit indicator on process exit. Respects per-block and global term:exitindicator setting. Clears indicators before block deletion on close-on-exit.
JustHereToHelp
force-pushed
the
feat/tab-exit-glow
branch
from
b3ef51b to
41721a5
Compare
2 participants
Tab Background Glow + Indicator Improvements
Been using tab indicators with Claude Code across 8-10 tabs daily and kept running into friction — spinning icons are distracting, indicators go out of sync, and there's no way to tell at a glance which tabs are working vs done. With these fixes it is clear as day which tabs are running a command, which are done and need me to look at them, which are done and I have already looked at. Such a major help to my workflow.
This PR adds a
term:exitindicatorconfig option that lights up tab backgrounds based on process state, plus some quality-of-life improvements to how indicators feel:What it does:
icon="none"support — sets the background glow without showing an icon, for tools that just want a color state@propertyvariable on:root(16s cycle, barely perceptible)term:exitindicator(default false)The breathing sync was the tricky part. Per-tab CSS animations drift out of phase. Solved it by animating a single
--breathe-phasecustom property on:rootand having tabs read from it viacalc()in the alpha channel. One clock, all tabs.Screenshot
Three states visible: grey check (done, already viewed), amber breathing (working), green check (done, needs attention).
Files changed:
pkg/blockcontroller/shellcontroller.go— running + exit indicators for cmd blocksfrontend/app/tab/tab.tsx— indicator-breathing class, icon="none" supportfrontend/app/tab/tab.scss— @Property breathing, static vs animated statesterm:exitindicator