Skip to content

loosen up the requirements to show the File Browser in term context menu #3232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sawka merged 1 commit into from
Apr 17, 2026
+11 −3
Merged

loosen up the requirements to show the File Browser in term context menu #3232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions frontend/app/view/term/term-model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import { boundNumber, fireAndForget, stringToBase64 } from "@/util/util";
import * as jotai from "jotai";
import * as React from "react";
import { getBlockingCommand } from "./shellblocking";
import { computeTheme, DefaultTermTheme, trimTerminalSelection } from "./termutil";
import { computeTheme, DefaultTermTheme, isLikelyOnSameHost, trimTerminalSelection } from "./termutil";
import { TermWrap, WebGLSupported } from "./termwrap";

export class TermViewModel implements ViewModel {
Expand Down Expand Up @@ -958,9 +958,9 @@ export class TermViewModel implements ViewModel {
});
fullMenu.push({ type: "separator" });

const shellIntegrationStatus = globalStore.get(this.termRef?.current?.shellIntegrationStatusAtom);
const lastCommand = globalStore.get(this.termRef?.current?.lastCommandAtom);
const cwd = blockData?.meta?.["cmd:cwd"];
const canShowFileBrowser = shellIntegrationStatus === "ready" && cwd != null;
const canShowFileBrowser = cwd != null && isLikelyOnSameHost(lastCommand);
Comment on lines +961 to +963
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm globalStore.get behavior with undefined and check how other call sites guard lastCommandAtom access.
rg -nP -C2 '\blastCommandAtom\b'
rg -nP -C2 'globalStore\.get\(\s*this\.termRef'

Repository: wavetermdev/waveterm

Length of output: 5605

🏁 Script executed:

#!/bin/bash
# Get context around line 961 in term-model.ts
sed -n '945,975p' frontend/app/view/term/term-model.ts | cat -n

# Search for where this function is called from to understand preconditions
rg -B5 -A15 'canShowFileBrowser' frontend/app/view/term/term-model.ts | head -40

# Check how globalStore.get is typed/documented
rg -B3 -A3 'globalStore.*=.*new' frontend/app/store/*.ts

Repository: wavetermdev/waveterm

Length of output: 2535

🏁 Script executed:

#!/bin/bash
# Find where globalStore is defined and check its type definition
rg -B3 -A10 'const globalStore' frontend/app/store/*.ts

# Check jotai atom and store usage patterns
rg -l 'jotai' frontend/app/store/*.ts | head -3

# Search for error handling around globalStore.get calls
rg -B3 -A3 'globalStore\.get\(' frontend/app/view/term/term-model.ts | head -50

# Check if termRef is initialized in constructor/lifecycle
rg -B5 -A10 'termRef.*=' frontend/app/view/term/term-model.ts | head -60

Repository: wavetermdev/waveterm

Length of output: 4568

🏁 Script executed:

#!/bin/bash
# Check if there are examples in code where undefined atoms are caught/handled
rg -B2 -A2 'try.*globalStore\.get|catch.*globalStore' frontend/app/

# Look for jotai types or TypeScript warnings about undefined atoms
rg 'Atom<' frontend/app/view/term/term-model.ts | head -10

# Check if this context menu function is called early (before mount)
rg -B10 'getMenuItemsForBlockContextMenu' frontend/app/view/term/term-model.ts | head -40

# Check for jotai usage documentation in package.json or comments
rg -i 'jotai' frontend/package.json

Repository: wavetermdev/waveterm

Length of output: 947

🏁 Script executed:

#!/bin/bash
# Find where getMenuItemsForBlockContextMenu is called from
rg 'getMenuItemsForBlockContextMenu' frontend/

# Look at the broader context - is this part of a lifecycle hook or event handler?
rg -B20 'const lastCommand = globalStore\.get.*lastCommandAtom' frontend/app/view/term/term-model.ts | head -50

# Check if there are any type errors or comments about this
rg -B5 'lastCommandAtom.*undefined|undefined.*lastCommandAtom' frontend/

Repository: wavetermdev/waveterm

Length of output: 926

🏁 Script executed:

#!/bin/bash
# Find the function definition containing the problematic code
rg -B5 'getMenuItemsForBlockContextMenu|const lastCommand = globalStore\.get.*termRef.*lastCommandAtom' frontend/app/view/term/term-model.ts | grep -E '(getMenuItemsForBlockContextMenu|private|public|async)' | head -5

# Search for function calls to this menu function across the entire repo
rg 'getMenuItemsForBlockContextMenu\(' frontend/ | head -20

# Get full function signature and context around line 961
sed -n '920,970p' frontend/app/view/term/term-model.ts

Repository: wavetermdev/waveterm

Length of output: 2348

🏁 Script executed:

#!/bin/bash
# Get the full function name and signature
sed -n '890,920p' frontend/app/view/term/term-model.ts

# Find where this context menu function is invoked
rg 'getContextMenuItems|getMenuItems' frontend/app/view/term/term-model.ts

# Look for right-click handler or context menu trigger
rg -B3 -A3 'onContextMenu|contextmenu' frontend/app/view/term/

Repository: wavetermdev/waveterm

Length of output: 2234

🏁 Script executed:

#!/bin/bash
# Find where handleContextMenu is defined in term.tsx
rg -B5 -A10 'handleContextMenu' frontend/app/view/term/term.tsx

# Check when termRef.current is assigned (when TermWrap mounts)
rg -B3 -A3 'ref={termRef}|termRef\.current\s*=' frontend/app/view/term/termwrap.ts

# Find the React component useEffect/mount cycle for TermWrap
rg -B5 -A15 'componentDidMount|useEffect.*termRef|React\.forwardRef' frontend/app/view/term/termwrap.ts | head -60

Repository: wavetermdev/waveterm

Length of output: 1482

🏁 Script executed:

#!/bin/bash
# Find where termRef is assigned in term.tsx (the component that passes termRef down)
rg -B10 -A10 'termRef.*current\s*=' frontend/app/view/term/term.tsx

# Check TermWrap class to understand lifecycle
rg -B5 -A15 'class TermWrap|forwardRef.*TermWrap' frontend/app/view/term/termwrap.ts | head -80

# Look at the componentDidMount or constructor for TermWrap
sed -n '120,160p' frontend/app/view/term/termwrap.ts

Repository: wavetermdev/waveterm

Length of output: 3322

Guard against a null lastCommandAtom.

this.termRef?.current?.lastCommandAtom can be undefined if termRef.current isn't set yet or the TermWrap hasn't initialized its atoms. globalStore.get(undefined) will throw, breaking the context menu. Other code sites (e.g., tabrpcclient.ts line 165) explicitly guard against this pattern.

Proposed fix 
-        const lastCommand = globalStore.get(this.termRef?.current?.lastCommandAtom);
+        const lastCommandAtom = this.termRef?.current?.lastCommandAtom;
+        const lastCommand = lastCommandAtom ? globalStore.get(lastCommandAtom) : null;
         const cwd = blockData?.meta?.["cmd:cwd"];
         const canShowFileBrowser = cwd != null && isLikelyOnSameHost(lastCommand);
📝 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.

Suggested change
const lastCommand = globalStore.get(this.termRef?.current?.lastCommandAtom);
const cwd = blockData?.meta?.["cmd:cwd"];
const canShowFileBrowser = shellIntegrationStatus === "ready" && cwd != null;
const canShowFileBrowser = cwd != null && isLikelyOnSameHost(lastCommand);
const lastCommandAtom = this.termRef?.current?.lastCommandAtom;
const lastCommand = lastCommandAtom ? globalStore.get(lastCommandAtom) : null;
const cwd = blockData?.meta?.["cmd:cwd"];
const canShowFileBrowser = cwd != null && isLikelyOnSameHost(lastCommand);
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@frontend/app/view/term/term-model.ts` around lines 961 - 963, Guard against
this.termRef?.current?.lastCommandAtom being undefined before calling
globalStore.get: retrieve the atom into a local (e.g., lastCommandAtom =
this.termRef?.current?.lastCommandAtom), only call
globalStore.get(lastCommandAtom) if that atom is truthy, otherwise set
lastCommand to undefined; then compute cwd = blockData?.meta?.["cmd:cwd"] and
keep canShowFileBrowser = cwd != null && isLikelyOnSameHost(lastCommand) so
callers of lastCommandAtom (and globalStore.get) no longer throw when the
TermWrap or termRef is uninitialized.


if (canShowFileBrowser) {
fullMenu.push({
Expand Down
8 changes: 8 additions & 0 deletions frontend/app/view/term/termutil.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,14 @@ export function bufferLinesToText(buffer: TermTypes.IBuffer, startIndex: number,
return lines;
}

export function isLikelyOnSameHost(lastCommand: string): boolean {
if (lastCommand == null) {
return true;
}
const cmd = lastCommand.trimStart();
return !cmd.startsWith("ssh ");
}
Comment on lines +400 to +406
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Heuristic has known blind spots — worth documenting.

The check only matches a leading literal "ssh ". Common remote-session invocations will slip through and the File Browser will be offered against a remote host it can't actually reach:

  • mosh user@host, autossh ..., et user@host, kubectl exec, docker exec -it ... bash, gcloud compute ssh, ssh host (multiple spaces is fine, but SSH host, /usr/bin/ssh host, or env FOO=1 ssh host are not), etc.
  • Conversely, once the user runs any command inside the ssh session (and shell integration is active on the remote), lastCommand becomes that remote command and isLikelyOnSameHost returns true again — so the File Browser re-appears and opens the local/original connection's filesystem against a cwd that belongs to the remote host.

Given the function is named isLikely…, the fuzziness is acknowledged, but please at least add a brief JSDoc noting it's a best-effort heuristic gated on the immediately prior command, so future readers don't expect stronger guarantees. If tightening is desired, matching /(^|\s)(ssh|mosh|autossh|et)\s/ on a trimmed command would catch more cases.

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

In `@frontend/app/view/term/termutil.ts` around lines 400 - 406, The heuristic in
isLikelyOnSameHost only checks for a literal leading "ssh " and has blind spots;
add a concise JSDoc above export function isLikelyOnSameHost(lastCommand:
string): boolean explaining this is a best-effort, immediate-prior-command
heuristic (not a guarantee) and list examples of common false negatives (mosh,
autossh, gcloud compute ssh, docker/kubectl exec, prefixed env/absolute-path
ssh, varied spacing, and the case where lastCommand becomes a remote command) so
future readers won't assume stronger semantics; optionally, if you want to
broaden detection, replace the simple startsWith check with a trimmed regex
match like /(^|\s)(ssh|mosh|autossh|et)\s/ as noted in the review (but keep the
JSDoc regardless).


export function quoteForPosixShell(filePath: string): string {
return "'" + filePath.replace(/'/g, "'\\''") + "'";
}
Loading