loosen up the requirements to show the File Browser in term context menu#3232
loosen up the requirements to show the File Browser in term context menu#3232
Conversation
WalkthroughThis pull request refactors the File Browser menu item visibility logic in the terminal view. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying waveterm with
|
| Latest commit: |
bc56458
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b71f4249.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-term-filebrowser.waveterm.pages.dev |
![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)
frontend/app/view/term/term-model.ts (1)
965-984:⚠️ Potential issue | 🟡 MinorFile Browser heuristic is insufficient for SSH detection; only checks
sshprefix.The File Browser currently uses
isLikelyOnSameHost()which only checks!cmd.startsWith("ssh "). This heuristic fails in common scenarios:
- Misses
moshandautosshcommands- Most critically: fails when user runs any command after SSH'ing (e.g., user SSH's, runs
ls, then opens File Browser—cmd:cwdbecomes remote butisLikelyOnSameHost("ls")returnstrue)When the heuristic incorrectly allows File Browser creation with a remote
cmd:cwdon the original (local) connection, the preview block attempts to browse the remote path on the wrong host.Consider checking additional context like
shell:lastcmdexitcodebehavior or expanding the heuristic to detect SSH wrappers. Alternatively, include the targeted host in the label (e.g.,File Browser ({connection or "local"})) to surface the actual target.🤖 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 965 - 984, The File Browser opens on the wrong host because the current heuristic (isLikelyOnSameHost) only checks cmd.startsWith("ssh ") and misses wrappers (mosh, autossh) and cases where the last interactive command was run after an SSH session; update the logic in the File Browser click handler that reads blockData?.meta (referenced via this.blockAtom and createBlock/BlockDef) to: 1) prefer an explicit meta.connection if present and pass it into BlockDef.meta.connection so the preview uses the correct host; 2) improve isLikelyOnSameHost to also detect common SSH wrappers (e.g., /^\\s*(ssh|mosh|autossh)\\b/i), detect remote cwd patterns (user@host: or /ssh: or other remote path markers), and consider shell:lastcmdexitcode or other meta flags when available; and 3) update the menu label to include the target (e.g., `File Browser (${connection ?? "local"})`) so users see which host will be browsed.
🧹 Nitpick comments (1)
frontend/app/view/term/termutil.ts (1)
400-406: Type signature should allownull.
lastCommandis read fromlastCommandAtom, which is explicitly set tonullin several places (seeosc-handlers.tsandtermwrap.ts), and the function body already handlesnull. Typing the parameter asstringmisrepresents the contract and will trip up strict null-checking callers.Proposed fix
-export function isLikelyOnSameHost(lastCommand: string): boolean { +export function isLikelyOnSameHost(lastCommand: string | null | undefined): boolean { if (lastCommand == null) { return true; } const cmd = lastCommand.trimStart(); return !cmd.startsWith("ssh "); }🤖 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 parameter type for isLikelyOnSameHost should allow null since callers set lastCommandAtom to null; change the function signature from (lastCommand: string) to (lastCommand: string | null) so the type matches the runtime check and satisfies strict null-checking, leaving the existing null handling in the body intact (update any TypeScript usages/call sites if they rely on it being just string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/view/term/term-model.ts`:
- Around line 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.
In `@frontend/app/view/term/termutil.ts`:
- Around line 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).
---
Outside diff comments:
In `@frontend/app/view/term/term-model.ts`:
- Around line 965-984: The File Browser opens on the wrong host because the
current heuristic (isLikelyOnSameHost) only checks cmd.startsWith("ssh ") and
misses wrappers (mosh, autossh) and cases where the last interactive command was
run after an SSH session; update the logic in the File Browser click handler
that reads blockData?.meta (referenced via this.blockAtom and
createBlock/BlockDef) to: 1) prefer an explicit meta.connection if present and
pass it into BlockDef.meta.connection so the preview uses the correct host; 2)
improve isLikelyOnSameHost to also detect common SSH wrappers (e.g.,
/^\\s*(ssh|mosh|autossh)\\b/i), detect remote cwd patterns (user@host: or /ssh:
or other remote path markers), and consider shell:lastcmdexitcode or other meta
flags when available; and 3) update the menu label to include the target (e.g.,
`File Browser (${connection ?? "local"})`) so users see which host will be
browsed.
---
Nitpick comments:
In `@frontend/app/view/term/termutil.ts`:
- Around line 400-406: The parameter type for isLikelyOnSameHost should allow
null since callers set lastCommandAtom to null; change the function signature
from (lastCommand: string) to (lastCommand: string | null) so the type matches
the runtime check and satisfies strict null-checking, leaving the existing null
handling in the body intact (update any TypeScript usages/call sites if they
rely on it being just string).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2d6af3b8-0d30-4ac7-81c5-f5c12021c59c
📒 Files selected for processing (2)
frontend/app/view/term/term-model.tsfrontend/app/view/term/termutil.ts
| 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); |
There was a problem hiding this comment.
🧩 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/*.tsRepository: 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 -60Repository: 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.jsonRepository: 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.tsRepository: 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 -60Repository: 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.tsRepository: 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.
| 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.
| export function isLikelyOnSameHost(lastCommand: string): boolean { | ||
| if (lastCommand == null) { | ||
| return true; | ||
| } | ||
| const cmd = lastCommand.trimStart(); | ||
| return !cmd.startsWith("ssh "); | ||
| } |
There was a problem hiding this comment.
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, butSSH host,/usr/bin/ssh host, orenv FOO=1 ssh hostare not), etc.- Conversely, once the user runs any command inside the ssh session (and shell integration is active on the remote),
lastCommandbecomes that remote command andisLikelyOnSameHostreturnstrueagain — 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).
1 participant
No description provided.