more terminal integration for wave ai #2470
Conversation
sawka
commented
Oct 22, 2025
sawka
commented
- mark last command / prompts in xterm.js
- split out term model into its own file
- try to detect repl/shells/ssh/tmux etc commands that stop wave ai integration
- show icons in term headers for whether wave ai integration is available
- keep integration status / last command client side (sync with server on reload)
…implement a get last command output tool + rpc command.
update makeIconClass to deal with suffixes create atoms for shell integration state, show in term header. restore from server on reload.
… report them specifically as incompatible with wave ai...
WalkthroughThis pull request refactors the terminal view model architecture by extracting Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: The diff introduces substantial new modules ( Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/aiusechat/tools.go (1)
128-183: Fix inverted startLine/endLine logic in handle_termgetscrollbacklinesThe "lastcommand" slice calculation has startLine and endLine inverted. Current code uses
startLine = totalLines - markerLineandendLine = totalLines, which retrieves lines before the marker. The correct logic should usestartLine = 0andendLine = totalLines - markerLineto retrieve the command output from the marker to the newest line.Lines 125-132 in frontend/app/view/term/term-wsh.tsx require correction to swap the startLine/endLine assignments.
🧹 Nitpick comments (19)
frontend/util/util.ts (1)
111-122: Consider optimizing the redundant regex match.The while condition and the assignment inside the loop both call
icon.match()with the same pattern, which is inefficient.Apply this diff to optimize:
- while (icon.match(/\+(spin|beat|fade|fw)$/)) { - const modifierMatch = icon.match(/\+(spin|beat|fade|fw)$/); - if (modifierMatch) { - const modifier = modifierMatch[1]; - if (modifier === "fw") { - hasFwModifier = true; - } else { - animation = modifier; - } - icon = icon.replace(/\+(spin|beat|fade|fw)$/, ""); - } - } + let modifierMatch: RegExpMatchArray | null; + while ((modifierMatch = icon.match(/\+(spin|beat|fade|fw)$/))) { + const modifier = modifierMatch[1]; + if (modifier === "fw") { + hasFwModifier = true; + } else { + animation = modifier; + } + icon = icon.replace(/\+(spin|beat|fade|fw)$/, ""); + }pkg/blockcontroller/shellcontroller.go (1)
203-207: Clarify/reset all terminal modes on reload.Comment looks good. Consider also resetting bracketed paste and alt screen via DECSET/DECRST for robustness across terminals, e.g., CSI ? 2004 l and CSI ? 1049 l, in addition to OSC 16162 "R". Safe, idempotent, and avoids sticky states after reload.
frontend/app/view/term/term-model.ts (3)
93-96: Remove no-op branch or set a distinct icon for cmd.The empty if block does nothing; either remove it or choose a cmd-specific icon.
- const isCmd = get(this.isCmdController); - if (isCmd) { - } + const isCmd = get(this.isCmdController); + if (isCmd) { + return "terminal"; // or e.g., "chevron-right" if desired + }
206-247: Avoid “hook” lint false positives for useBlockAtom.Biome flags these as conditional hooks. If useBlockAtom isn’t a React hook, consider renaming to avoid “use*” convention or disable the rule for this file.
- this.termThemeNameAtom = useBlockAtom(blockId, "termthemeatom", () => { + this.termThemeNameAtom = makeBlockAtom(blockId, "termthemeatom", () => { ... - this.termTransparencyAtom = useBlockAtom(blockId, "termtransparencyatom", () => { + this.termTransparencyAtom = makeBlockAtom(blockId, "termtransparencyatom", () => { ... - this.fontSizeAtom = useBlockAtom(blockId, "fontsizeatom", () => { + this.fontSizeAtom = makeBlockAtom(blockId, "fontsizeatom", () => {Or add at top:
/* biome-ignore lint/correctness/useHookAtTopLevel: not a React hook */
250-293: Expose shell-integration icon regardless of AI panel state (optional).If the goal is “icons in terminal headers indicating availability,” consider showing the sparkles indicator even when the AI panel is closed.
- const isAIPanelOpen = get(WorkspaceLayoutModel.getInstance().panelVisibleAtom); - if (isAIPanelOpen) { + { const shellIntegrationButton = this.getShellIntegrationIconButton(get); if (shellIntegrationButton) { rtn.push(shellIntegrationButton); } }pkg/aiusechat/tools.go (1)
176-177: Good addition; tool surfaced only when a term widget exists.Consider further gating or hinting when no terminals have shell integration to reduce user-visible tool errors. Current callback checks and errors cleanly, so this is optional.
frontend/types/gotypes.d.ts (1)
295-301: Align types with “lastcommand” mode.When lastcommand=true, linestart/lineend are ignored and often omitted by callers. Consider a discriminated union or making linestart/lineend optional to reflect reality. If this file is generated, update the generator to support such unions.
-type CommandTermGetScrollbackLinesData = { - linestart: number; - lineend: number; - lastcommand: boolean; -}; +type CommandTermGetScrollbackLinesData = + | { lastcommand: true } + | { linestart: number; lineend: number; lastcommand?: false };frontend/app/view/term/term.tsx (3)
115-121: Stabilize derived Jotai atom to avoid re-creation every renderYou’re creating a new atom on each render; that can cause unnecessary subscriptions/churn. Memoize it.
- const isFocusedAtom = jotai.atom((get) => { - return get(model.nodeModel.isFocused) && get(model.termMode) == "vdom"; - }); + const isFocusedAtom = React.useMemo( + () => + jotai.atom((get) => get(model.nodeModel.isFocused) && get(model.termMode) == "vdom"), + [model] + );
291-293: Avoid leaking TermWrap on window in productionwindow.term is handy for debugging but leaks references. Gate it with a dev/debug flag.
- (window as any).term = termWrap; + if (process.env.NODE_ENV !== "production") { + (window as any).term = termWrap; + }
296-297: Null‑guard ResizeObserver targetconnectElemRef.current should be checked before observe to prevent sporadic NPEs during fast remounts.
- rszObs.observe(connectElemRef.current); + if (connectElemRef.current) { + rszObs.observe(connectElemRef.current); + }frontend/app/view/term/term-wsh.tsx (1)
126-128: Graceful fallback when shell integration is unknownInstead of throwing (which surfaces as an RPC error), consider returning an empty result with a clear message to the caller so the tool can degrade gracefully.
- if (globalStore.get(termWrap.shellIntegrationStatusAtom) == null) { - throw new Error("Cannot get last command data without shell integration"); - } + if (!globalStore.get(termWrap.shellIntegrationStatusAtom)) { + return { + totallines: totalLines, + linestart: 0, + lines: [], + lastupdated: termWrap.lastUpdated, + }; + }frontend/app/view/term/shellblocking.ts (4)
81-85: Wrapper stripping leaves options/assignments as “first” (e.g.,sudo -u root bash).After removing a wrapper token, you should also skip its flags and common env assignments so the base command is detected.
- while (words.length && WRAPPERS.includes(words[0])) { - words.shift(); - } + // Strip known wrappers and their immediate flags/env assignments + while (words.length && WRAPPERS.includes(words[0])) { + words.shift(); + // Skip wrapper flags (e.g., -u, --preserve-env) and their values; also env assignments + while (words.length) { + const t = words[0]; + // stop at next likely command token + if (!t.startsWith("-") && !t.includes("=")) break; + words.shift(); + // heuristically skip one value after short/long options that take a value + if (/^-(?:[A-Za-z]|-[A-Za-z-]+)/.test(t) && words.length && !words[0].startsWith("-")) { + words.shift(); + } + } + }Note: heuristic but materially improves
sudo/doas/env/timechains without full shell parsing.
48-54: Non-interactive shell exec heuristic too broad (includes("/")).
includes("/")on any arg can yield false positives. Prefer the first non-option token.-function isNonInteractiveShellExec(args: string[]): boolean { - return ( - args.includes("-c") || - args.some((a) => a === "-Command" || a.startsWith("-Command")) || - args.some((a) => a.endsWith(".sh") || a.includes("/")) - ); -} +function isNonInteractiveShellExec(args: string[]): boolean { + if (args.includes("-c")) return true; + if (args.some((a) => a === "-Command" || a.startsWith("-Command"))) return true; // pwsh/cmd + const firstNonOpt = args.find((a) => !a.startsWith("-")); + return !!firstNonOpt && (firstNonOpt.endsWith(".sh") || firstNonOpt.includes("/")); +}
111-114: REPLs with explicit interactive flag should also block.
python -i script.pylaunches an interactive session; current logic only blocks with zero args.- if (BARE_REPLS.includes(first)) { - if (args.length === 0) return first; - return null; - } + if (BARE_REPLS.includes(first)) { + if (args.length === 0) return first; + if (args.includes("-i") || args.includes("--interactive")) return first; + return null; + }
78-89: Shellwords/quotes not respected (naive whitespace split).Quoted args (e.g.,
ssh host "htop") and env assignments degrade detection accuracy.Consider a tiny shell-like splitter to honor quotes/escapes, or restrict heuristics to first non-option token and known flag patterns. I can draft a zero-dep splitter if desired.
Also applies to: 105-116
frontend/app/view/term/termwrap.ts (4)
305-309: Reset (“R”) should also clear last command in atoms and RT info.State is cleared, but
lastCommandAtomandshell:lastcmdremain, leaving stale UI.- case "R": - globalStore.set(termWrap.shellIntegrationStatusAtom, null); - if (terminal.buffer.active.type === "alternate") { + case "R": + globalStore.set(termWrap.shellIntegrationStatusAtom, null); + globalStore.set(termWrap.lastCommandAtom, null); + rtInfo["shell:lastcmd"] = null; + rtInfo["shell:lastcmdexitcode"] = null; + if (terminal.buffer.active.type === "alternate") { terminal.write("\x1b[?1049l"); } break;
367-371: Prefer typed Jotai atoms over casts.Use generics to avoid
as PrimitiveAtom<…>.- this.shellIntegrationStatusAtom = jotai.atom(null) as jotai.PrimitiveAtom< - "ready" | "running-command" | null - >; - this.lastCommandAtom = jotai.atom(null) as jotai.PrimitiveAtom<string | null>; + this.shellIntegrationStatusAtom = jotai.atom<"ready" | "running-command" | null>(null); + this.lastCommandAtom = jotai.atom<string | null>(null);
175-200: POC decoration helper should be gated or removed.
addTestMarkerDecorationis unused; shipping dead code risks drift. Gate behind a dev flag or remove.
463-481: RT info load: narrow state assignment defensively.If
shell:stateis an unexpected string, the cast can propagate invalid UI. Guard values.- if (rtInfo["shell:integration"]) { - const shellState = rtInfo["shell:state"] as ShellIntegrationStatus; - globalStore.set(this.shellIntegrationStatusAtom, shellState || null); + if (rtInfo["shell:integration"]) { + const s = rtInfo["shell:state"]; + const shellState: ShellIntegrationStatus | null = + s === "ready" || s === "running-command" ? s : null; + globalStore.set(this.shellIntegrationStatusAtom, shellState);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
frontend/app/block/block.tsx(1 hunks)frontend/app/element/iconbutton.tsx(3 hunks)frontend/app/view/term/shellblocking.ts(1 hunks)frontend/app/view/term/term-model.ts(1 hunks)frontend/app/view/term/term-wsh.tsx(2 hunks)frontend/app/view/term/term.tsx(3 hunks)frontend/app/view/term/termtheme.ts(1 hunks)frontend/app/view/term/termwrap.ts(10 hunks)frontend/types/gotypes.d.ts(1 hunks)frontend/util/util.ts(1 hunks)pkg/aiusechat/tools.go(1 hunks)pkg/aiusechat/tools_term.go(3 hunks)pkg/blockcontroller/shellcontroller.go(1 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
frontend/app/view/term/term-model.ts (6)
frontend/app/block/blocktypes.ts (1)
BlockNodeModel(7-12)frontend/app/view/term/termwrap.ts (1)
TermWrap(329-653)frontend/app/view/term/term-wsh.tsx (1)
TermWshClient(15-183)frontend/app/store/global.ts (9)
WOS(861-861)atoms(811-811)useBlockAtom(854-854)getOverrideConfigAtom(828-828)getConnStatusAtom(824-824)getSettingsKeyAtom(829-829)getBlockMetaKeyAtom(823-823)getBlockComponentModel(822-822)getAllBlockComponentModels(820-820)frontend/app/view/term/shellblocking.ts (1)
getBlockingCommand(75-117)frontend/app/view/vdom/vdom-model.tsx (1)
VDomModel(105-691)
pkg/aiusechat/tools_term.go (7)
pkg/wshrpc/wshrpctypes.go (2)
CommandTermGetScrollbackLinesData(867-871)RpcOpts(299-305)pkg/wcore/wcore.go (1)
ResolveBlockIdFromPrefix(114-131)pkg/wshrpc/wshclient/barerpcclient.go (1)
GetBareRpcClient(30-39)pkg/wshutil/wshrouter.go (1)
MakeFeBlockRouteId(76-78)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)pkg/waveobj/wtype.go (1)
OType_Block(30-30)pkg/wstore/blockrtinfo.go (1)
GetRTInfo(89-99)
pkg/blockcontroller/shellcontroller.go (1)
pkg/util/shellutil/shellutil.go (1)
FormatOSC(425-430)
pkg/aiusechat/tools.go (1)
pkg/aiusechat/tools_term.go (1)
GetTermCommandOutputToolDefinition(245-302)
frontend/app/view/term/termwrap.ts (4)
frontend/app/store/global.ts (2)
globalStore(834-834)WOS(861-861)frontend/app/store/jotaiStore.ts (1)
globalStore(3-3)frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
🪛 Biome (2.1.2)
frontend/app/view/term/term-model.ts
[error] 206-206: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 211-211: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 233-233: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
frontend/app/view/term/termwrap.ts
[error] 240-240: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (7)
frontend/util/util.ts (2)
144-148: LGTM! Clean class assembly logic.The final class construction correctly:
- Combines the fw parameter with the +fw modifier
- Handles both the +spin modifier and the opts.spin parameter
- Applies non-spin animations (beat, fade) separately
- Uses clsx to elegantly handle conditional classes
Note: If multiple animation modifiers are specified (e.g.,
+beat+spin), only the last one will be applied, which is appropriate since Font Awesome only supports one animation at a time.
124-142: No changes required—review comment is based on incorrect assumptions.The codebase loads Font Awesome from local files (
../public/fontawesome) with explicit CSS imports forfontawesome.min.css,brands.min.css,solid.min.css,sharp-solid.min.css,sharp-regular.min.css, andcustom-icons.min.css. This custom Font Awesome distribution supports the exact class structure used in the code (fa fa-solid fa-${icon},fa fa-sharp fa-regular fa-${icon}, etc.). The class structure is correct and intentional for this project's Font Awesome setup.Likely an incorrect or invalid review comment.
pkg/wshrpc/wshrpctypes.go (1)
868-871: API extension is backward-compatible and properly implemented.The handler correctly tolerates missing
linestart/lineendwhenlastcommand=true. When this flag is set, the code uses computed values from shell integration markers (lines 131-137) rather than input fields, so missing input fields pose no risk. The non-lastcommand path properly uses the input fields as expected.frontend/app/view/term/termtheme.ts (1)
4-15: Type-only import update looks goodPath and typing are correct; no runtime effect.
pkg/aiusechat/tools_term.go (2)
84-152: Good consolidation and richer outputShared helper getTermScrollbackOutput centralizes logic and surfaces last-command info; join + bounds + nextStart are coherent.
Confirm the Go toolchain is Go 1.21+ (uses predeclared min/max). If older, replace with helpers:
func imin(a, b int) int { if a < b { return a }; return b } func imax(a, b int) int { if a > b { return a }; return b }
245-302: Term command-output tool: API shape is solid; align last-command bounds with frontend fixOnce the frontend lastcommand range is corrected, this tool will return the intended region between the two most recent prompts. No code change needed here.
After applying the frontend fix, please sanity-check with a long output:
- Run:
yes "line" | head -n 1200then run a short command.- Invoke term_command_output; expect ≤1000 lines from the long command, not content after the prompt.
frontend/app/view/term/term-wsh.tsx (1)
125-161: Semantic concern is valid; marker.line wrapping behavior must be verifiedThe review raises a legitimate semantic issue: the current code returns everything typed after the last prompt, whereas "last command output" semantically means content between the previous and last prompts. The proposed fix correctly structures this by using both markers.
However, the fix's correctness depends entirely on unverified behavior: how
marker.linebehaves when the xterm.js circular buffer wraps. The indexing mathtotalLines - bufIdxassumes consistent marker.line semantics across buffer wrapping—which is not documented in the visible codebase and cannot be tested without running the code.Before applying the proposed diff, confirm:
- Whether
marker.lineremains consistent/valid when buffer content scrolls out- Whether buffer indices ever invert or behave unexpectedly under wrapping
- Whether the clamping logic (Math.max/Math.min) handles all edge cases
| import { isBlank, useAtomValueSafe } from "@/util/util"; | ||
| import { HelpViewModel } from "@/view/helpview/helpview"; | ||
| import { TermViewModel } from "@/view/term/term"; | ||
| import { TermViewModel } from "@/view/term/term-model"; |
There was a problem hiding this comment.
🧩 Analysis chain
Fix TermViewModel import path (build blocker)
Other modules import from "@/app/view/term/term-model"; this file uses "@/view/term/term-model". Likely breaks module resolution.
- import { TermViewModel } from "@/view/term/term-model";
+ import { TermViewModel } from "@/app/view/term/term-model";To verify consistency:
🏁 Script executed:
#!/bin/bash
# Find mismatched imports for term-model
rg -nP '"@/view/term/term-model"|"/app/view/term/term-model"' -g '!**/dist/**' -SLength of output: 87
🏁 Script executed:
#!/bin/bash
# Find the actual term-model file location
fd -e ts -e tsx 'term-model' -t fLength of output: 36
🏁 Script executed:
#!/bin/bash
# Search for all TermViewModel imports across the codebase
rg -nP 'import\s+.*TermViewModel' -g '!**/dist/**'Length of output: 387
🏁 Script executed:
#!/bin/bash
# Check if the "@/view" path exists at all
fd 'view' -t d -g '!**/node_modules/**' -g '!**/dist/**' | head -20Length of output: 164
Fix TermViewModel import path (build blocker)
The import uses an incorrect path that doesn't match the file location or other modules in the codebase. The actual file is at frontend/app/view/term/term-model.ts, which corresponds to the alias @/app/view/term/term-model. Three other files (term-wsh.tsx, termtheme.ts, and term.tsx) correctly import from this path.
- import { TermViewModel } from "@/view/term/term-model";
+ import { TermViewModel } from "@/app/view/term/term-model";📝 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.
| import { TermViewModel } from "@/view/term/term-model"; | |
| import { TermViewModel } from "@/app/view/term/term-model"; |
🤖 Prompt for AI Agents
In frontend/app/block/block.tsx around line 30 the import path for TermViewModel
is incorrect; update the import to use the correct module alias path that
matches the actual file location (frontend/app/view/term/term-model) — replace
the current "@/view/term/term-model" with "@/app/view/term/term-model" to match
the other files (term-wsh.tsx, termtheme.ts, term.tsx) and ensure builds resolve
the module.
| const styleVal: CSSProperties = {}; | ||
| if (decl.iconColor) { | ||
| styleVal.color = decl.iconColor; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Inconsistency: ToggleIconButton still uses the old styling pattern.
The conditional color styling here differs from ToggleIconButton (line 60), which still uses style={{ color: decl.iconColor ?? "inherit" }}. This means when decl.iconColor is undefined:
IconButton: no inline color style (relies on CSS cascade)ToggleIconButton: explicitcolor: inherit
While functionally similar in most cases, this inconsistency could lead to subtle styling differences if CSS specificity or inheritance rules interact differently with the two patterns.
Consider applying the same conditional styling pattern to ToggleIconButton for consistency:
+ const styleVal: CSSProperties = {};
+ if (decl.iconColor) {
+ styleVal.color = decl.iconColor;
+ }
return (
<button
ref={ref}
className={clsx("wave-iconbutton", "toggle", className, decl.className, {
active,
disabled,
"no-action": decl.noAction,
})}
title={title}
aria-label={title}
- style={{ color: decl.iconColor ?? "inherit" }}
+ style={styleVal}
onClick={() => setActive(!active)}
disabled={disabled}
>Also verify that removing the explicit color: inherit default doesn't break existing icon button styling across the application, particularly in contexts where CSS specificity might affect color inheritance.
Also applies to: 31-31
🏁 Script executed:
#!/bin/bash
# Examine the IconButton and ToggleIconButton components
cat frontend/app/element/iconbutton.tsxLength of output: 2677
Verified: ToggleIconButton inconsistency is real and worth addressing.
The review comment accurately identifies an inconsistency. When decl.iconColor is undefined:
- IconButton (lines 18–20, 31): renders with empty
style={}, relying on CSS cascade - ToggleIconButton (line 59): renders with explicit
style={{ color: "inherit" }}
While functionally equivalent in most cases, the inconsistent patterns within the same file warrant consistency. Applying the same conditional styleVal pattern to ToggleIconButton is a reasonable refactoring suggestion to improve maintainability.
🤖 Prompt for AI Agents
In frontend/app/element/iconbutton.tsx around lines 18–21 and line 59,
ToggleIconButton currently passes an explicit style={{ color: "inherit" }} while
IconButton builds a conditional styleVal object when decl.iconColor is present;
update ToggleIconButton to use the same conditional pattern as IconButton by
creating a styleVal: CSSProperties = {} and only setting styleVal.color =
decl.iconColor when decl.iconColor is defined, then pass style={styleVal} to the
rendered element so both components use the same conditional styling approach.
| function isSshInteractive(args: string[]): boolean { | ||
| const hasForcedTty = args.includes("-t") || args.includes("-tt"); | ||
| const hasRemoteCmd = args.some((a) => !a.startsWith("-") && a.includes(" ")); | ||
| return hasForcedTty || !hasRemoteCmd; | ||
| } |
There was a problem hiding this comment.
SSH remote-command detection misclassifies non-interactive invocations.
ssh host cmd (and ssh host cmd arg...) is treated as interactive; isSshInteractive only looks for a space in a single arg. Parse the host position and treat additional args as a remote command.
-function isSshInteractive(args: string[]): boolean {
- const hasForcedTty = args.includes("-t") || args.includes("-tt");
- const hasRemoteCmd = args.some((a) => !a.startsWith("-") && a.includes(" "));
- return hasForcedTty || !hasRemoteCmd;
-}
+function isSshInteractive(args: string[]): boolean {
+ const hasForcedTty = args.includes("-t") || args.includes("-tt");
+ // find first non-option token (host)
+ const hostIdx = args.findIndex((a) => !a.startsWith("-"));
+ if (hostIdx === -1) {
+ // e.g., `ssh -V` → non-interactive unless TTY forced
+ return hasForcedTty;
+ }
+ // any tokens after host imply a remote command
+ const hasRemoteCmd = args.length > hostIdx + 1;
+ return hasForcedTty || !hasRemoteCmd;
+}🤖 Prompt for AI Agents
In frontend/app/view/term/shellblocking.ts around lines 69 to 73,
isSshInteractive currently treats any argument containing a space as a remote
command and thus misclassifies invocations like `ssh host cmd` as interactive.
Change the logic to: scan args left-to-right skipping option tokens (those
starting with "-" and their possible option values until you hit a non-option
token) to identify the host token position, then treat the ssh invocation as
non-interactive if there are any tokens after that host (i.e., remote command
present); still return interactive if -t or -tt is present (forced TTY). Ensure
the function handles a "--" separator (stop option parsing) and common SSH short
options so that host detection is correct.
| viewType: string; | ||
| nodeModel: BlockNodeModel; | ||
| connected: boolean; | ||
| termRef: React.MutableRefObject<TermWrap> = { current: null }; |
There was a problem hiding this comment.
Fix ref type to allow null.
Initialize ref with a nullable type to satisfy TS strict null checks.
- termRef: React.MutableRefObject<TermWrap> = { current: null };
+ termRef: React.MutableRefObject<TermWrap | null> = { current: null };📝 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.
| termRef: React.MutableRefObject<TermWrap> = { current: null }; | |
| termRef: React.MutableRefObject<TermWrap | null> = { current: null }; |
🤖 Prompt for AI Agents
In frontend/app/view/term/term-model.ts around line 39, the ref is declared as
React.MutableRefObject<TermWrap> but initialized with null which violates strict
null checks; change the type to React.MutableRefObject<TermWrap | null> (or use
React.RefObject<TermWrap>) so the ref can legally hold null, and update any
usages to handle the nullable current value (e.g., check current before
accessing).
| setTermMode(mode: "term" | "vdom") { | ||
| if (mode == "term") { | ||
| mode = null; | ||
| } | ||
| RpcApi.SetMetaCommand(TabRpcClient, { | ||
| oref: WOS.makeORef("block", this.blockId), | ||
| meta: { "term:mode": mode }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Type-safe term mode setter.
You assign null to a parameter typed as "term" | "vdom". Use a local variable or widen the type.
- setTermMode(mode: "term" | "vdom") {
- if (mode == "term") {
- mode = null;
- }
+ setTermMode(mode: "term" | "vdom") {
+ const newMode: "vdom" | null = mode === "term" ? null : mode;
RpcApi.SetMetaCommand(TabRpcClient, {
oref: WOS.makeORef("block", this.blockId),
- meta: { "term:mode": mode },
+ meta: { "term:mode": newMode },
});
}📝 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.
| setTermMode(mode: "term" | "vdom") { | |
| if (mode == "term") { | |
| mode = null; | |
| } | |
| RpcApi.SetMetaCommand(TabRpcClient, { | |
| oref: WOS.makeORef("block", this.blockId), | |
| meta: { "term:mode": mode }, | |
| }); | |
| } | |
| setTermMode(mode: "term" | "vdom") { | |
| const newMode: "vdom" | null = mode === "term" ? null : mode; | |
| RpcApi.SetMetaCommand(TabRpcClient, { | |
| oref: WOS.makeORef("block", this.blockId), | |
| meta: { "term:mode": newMode }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In frontend/app/view/term/term-model.ts around lines 393 to 401, the function
parameter mode is typed as "term" | "vdom" but the code assigns null to it;
change to use a local variable or widen the parameter type: create a new local
variable (e.g., let payloadMode = mode === "term" ? null : mode) and pass
payloadMode to RpcApi.SetMetaCommand, or change the parameter type to "term" |
"vdom" | null if callers may pass null; ensure the declared type and actual
assigned value match so TypeScript stays type-safe.
| keyDownHandler(waveEvent: WaveKeyboardEvent): boolean { | ||
| if (keyutil.checkKeyPressed(waveEvent, "Cmd:Escape")) { | ||
| const blockAtom = WOS.getWaveObjectAtom<Block>(`block:${this.blockId}`); | ||
| const blockData = globalStore.get(blockAtom); | ||
| const newTermMode = blockData?.meta?.["term:mode"] == "vdom" ? null : "vdom"; | ||
| const vdomBlockId = globalStore.get(this.vdomBlockId); | ||
| if (newTermMode == "vdom" && !vdomBlockId) { | ||
| return; | ||
| } | ||
| this.setTermMode(newTermMode); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Always return a boolean from keyDownHandler.
The bare return; violates the method’s boolean contract and can break call sites.
- if (newTermMode == "vdom" && !vdomBlockId) {
- return;
- }
+ if (newTermMode == "vdom" && !vdomBlockId) {
+ return false;
+ }📝 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.
| keyDownHandler(waveEvent: WaveKeyboardEvent): boolean { | |
| if (keyutil.checkKeyPressed(waveEvent, "Cmd:Escape")) { | |
| const blockAtom = WOS.getWaveObjectAtom<Block>(`block:${this.blockId}`); | |
| const blockData = globalStore.get(blockAtom); | |
| const newTermMode = blockData?.meta?.["term:mode"] == "vdom" ? null : "vdom"; | |
| const vdomBlockId = globalStore.get(this.vdomBlockId); | |
| if (newTermMode == "vdom" && !vdomBlockId) { | |
| return; | |
| } | |
| this.setTermMode(newTermMode); | |
| return true; | |
| } | |
| keyDownHandler(waveEvent: WaveKeyboardEvent): boolean { | |
| if (keyutil.checkKeyPressed(waveEvent, "Cmd:Escape")) { | |
| const blockAtom = WOS.getWaveObjectAtom<Block>(`block:${this.blockId}`); | |
| const blockData = globalStore.get(blockAtom); | |
| const newTermMode = blockData?.meta?.["term:mode"] == "vdom" ? null : "vdom"; | |
| const vdomBlockId = globalStore.get(this.vdomBlockId); | |
| if (newTermMode == "vdom" && !vdomBlockId) { | |
| return false; | |
| } | |
| this.setTermMode(newTermMode); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
frontend/app/view/term/term-model.ts around lines 466–477: the keyDownHandler
currently uses a bare `return;` inside the Cmd:Escape branch which breaks the
method's boolean contract; replace that bare return with `return false;` (so the
handler consistently returns a boolean when it aborts switching to vdom) and
verify all other code paths in this method return explicit booleans.
| case "A": | ||
| rtInfo["shell:state"] = "ready"; | ||
| globalStore.set(termWrap.shellIntegrationStatusAtom, "ready"); | ||
| const marker = terminal.registerMarker(0); | ||
| if (marker) { | ||
| termWrap.promptMarkers.push(marker); | ||
| // addTestMarkerDecoration(terminal, marker, termWrap); | ||
| marker.onDispose(() => { | ||
| const idx = termWrap.promptMarkers.indexOf(marker); | ||
| if (idx !== -1) { | ||
| termWrap.promptMarkers.splice(idx, 1); | ||
| } | ||
| }); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Fix switch-case declaration scope (noSwitchDeclarations).
const marker = … is declared directly in a case; wrap the case body in braces.
- case "A":
- rtInfo["shell:state"] = "ready";
- globalStore.set(termWrap.shellIntegrationStatusAtom, "ready");
- const marker = terminal.registerMarker(0);
- if (marker) {
- termWrap.promptMarkers.push(marker);
- // addTestMarkerDecoration(terminal, marker, termWrap);
- marker.onDispose(() => {
- const idx = termWrap.promptMarkers.indexOf(marker);
- if (idx !== -1) {
- termWrap.promptMarkers.splice(idx, 1);
- }
- });
- }
- break;
+ case "A": {
+ rtInfo["shell:state"] = "ready";
+ globalStore.set(termWrap.shellIntegrationStatusAtom, "ready");
+ const marker = terminal.registerMarker(0);
+ if (marker) {
+ termWrap.promptMarkers.push(marker);
+ // addTestMarkerDecoration(terminal, marker, termWrap);
+ marker.onDispose(() => {
+ const idx = termWrap.promptMarkers.indexOf(marker);
+ if (idx !== -1) {
+ termWrap.promptMarkers.splice(idx, 1);
+ }
+ });
+ }
+ break;
+ }As per static analysis hints.
📝 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.
| case "A": | |
| rtInfo["shell:state"] = "ready"; | |
| globalStore.set(termWrap.shellIntegrationStatusAtom, "ready"); | |
| const marker = terminal.registerMarker(0); | |
| if (marker) { | |
| termWrap.promptMarkers.push(marker); | |
| // addTestMarkerDecoration(terminal, marker, termWrap); | |
| marker.onDispose(() => { | |
| const idx = termWrap.promptMarkers.indexOf(marker); | |
| if (idx !== -1) { | |
| termWrap.promptMarkers.splice(idx, 1); | |
| } | |
| }); | |
| } | |
| break; | |
| case "A": { | |
| rtInfo["shell:state"] = "ready"; | |
| globalStore.set(termWrap.shellIntegrationStatusAtom, "ready"); | |
| const marker = terminal.registerMarker(0); | |
| if (marker) { | |
| termWrap.promptMarkers.push(marker); | |
| // addTestMarkerDecoration(terminal, marker, termWrap); | |
| marker.onDispose(() => { | |
| const idx = termWrap.promptMarkers.indexOf(marker); | |
| if (idx !== -1) { | |
| termWrap.promptMarkers.splice(idx, 1); | |
| } | |
| }); | |
| } | |
| break; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 240-240: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In frontend/app/view/term/termwrap.ts around lines 237 to 251, the case "A"
contains a block-scoped declaration (const marker = ...) directly in the case
which violates switch-case declaration scope rules; fix it by wrapping the
entire case body in braces (add { after case "A": and a matching } before the
break) so the const marker stays in its own block scope, preserving existing
logic (registerMarker, push, onDispose handler) unchanged.
1 participant
