feat(platform): 优化Windows平台路径处理和SSH配置文件支持#3220
Open
Cure217 wants to merge 5 commits intowavetermdev:mainfrom
Open
feat(platform): 优化Windows平台路径处理和SSH配置文件支持#3220Cure217 wants to merge 5 commits intowavetermdev:mainfrom
Cure217 wants to merge 5 commits intowavetermdev:mainfrom
Conversation
Cure217
commented
Apr 15, 2026
Cure217
commented
- 在SSH配置解析中添加Windows系统配置文件支持
- 实现Windows驱动器根目录和UNC路径的正确处理
- 修复历史导航中的路径分隔符问题
- 添加Windows虚拟根目录的支持
- 增强文件系统操作的跨平台兼容性
- 为终端滚动添加滚轮事件处理功能
- 实现对创建条目的权限控制检查
|
|
Contributor
|
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:
WalkthroughThe PR adds Windows-specific build gating and optional local Electron dist usage; improves platform-aware config and path handling (SSH config discovery, Windows virtual/drive roots, history parent-dir logic); introduces wheel-scroll conversion and terminal wheel handling with fractional accumulation; gates directory entry creation in the file preview UI; adds multiple unit tests across frontend and Go packages; and minor repo housekeeping (.gitignore, README newline). Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
.idea/vcs.xml (1)
1-6: Keep IDE project metadata out of this feature PR unless explicitly required by repo policy.This file is IntelliJ-local configuration and appears unrelated to the Windows/SSH/path behavior changes in this PR. Consider removing
.idea/*project metadata from this PR (or documenting why these files are intentionally versioned).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.idea/vcs.xml around lines 1 - 6, Remove the accidental IntelliJ metadata from this feature PR by reverting or deleting the .idea/vcs.xml change and ensure the repository ignores IDE files: remove .idea/vcs.xml from the commit (or run git rm --cached .idea/vcs.xml) and add .idea/ (or at least .idea/vcs.xml) to .gitignore so future commits won’t include VcsDirectoryMappings; if this file must remain, add a short justification in the PR description explaining why it’s intentionally versioned.frontend/util/historyutil.test.ts (1)
5-28: Good test coverage for the new path handling logic.The tests adequately cover POSIX, Windows drive, and UNC path scenarios. Consider adding tests for edge cases in a follow-up:
- Mixed separators:
C:\Users/wave/Downloads- UNC with forward slashes:
//server/share/folder- Bare drive letter:
C:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/util/historyutil.test.ts` around lines 5 - 28, Add tests to frontend/util/historyutil.test.ts that exercise getParentDirectory with the suggested edge cases: mixed separators (e.g., "C:\\Users\\wave/Downloads"), UNC using forward slashes (e.g., "//server/share/folder"), and a bare drive letter (e.g., "C:"); assert expected parent behavior consistent with existing cases (normalize separators, return root/share when appropriate, and handle "C:" as its own root or parent per current implementation) so edge-case parsing is covered alongside the POSIX/Windows/UNC tests.frontend/app/view/preview/preview-directory.tsx (2)
881-881: Similar dependency array concern.
setRefreshVersionis listed but not used in this callback either. SincenewFileandnewDirectoryare already in the dependencies (which internally handle refresh),setRefreshVersionappears to be unnecessary here.♻️ Suggested fix
- [canCreateEntries, setRefreshVersion, conn, newFile, newDirectory, dirPath] + [canCreateEntries, conn, newFile, newDirectory, dirPath, finfo]Note:
finfois used viaaddOpenMenuItems(menu, conn, finfo)and should be included.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-directory.tsx` at line 881, The callback's dependency array includes an unused setter and misses a used variable: remove setRefreshVersion from the dependency array (it's not referenced inside the callback) and add finfo (used via addOpenMenuItems(menu, conn, finfo)); ensure the dependency array becomes [canCreateEntries, conn, newFile, newDirectory, dirPath, finfo] so the useCallback/useEffect reflects actual internal references.
438-438: Dependency array has a mismatch.
setRefreshVersionis included but not used in the callback. The callback usessetErrorMsg(line 432) which isn't listed. SincesetErrorMsgfromuseSetAtomis stable, this won't cause bugs, but the dependencies could be more accurate.♻️ Suggested fix
- [canCreateEntries, setRefreshVersion, conn] + [canCreateEntries, setErrorMsg, conn]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-directory.tsx` at line 438, The callback's dependency array incorrectly includes setRefreshVersion while omitting setErrorMsg: remove setRefreshVersion and add setErrorMsg so the useCallback/useEffect for the preview-directory callback references the actual reactive values (update the dependency array that currently lists [canCreateEntries, setRefreshVersion, conn] to [canCreateEntries, conn, setErrorMsg]); locate the callback in preview-directory.tsx that uses setErrorMsg (around line 432) and adjust its dependency array accordingly.frontend/app/view/term/termutil.test.ts (1)
5-24: Add guard-branch tests for zero/non-finite deltas.Current tests are good, but Line 398’s early-return path (
0,NaN,Infinity) isn’t exercised yet.✅ Suggested test additions
describe("getWheelLineDelta", () => { + it("returns 0 for zero or non-finite deltaY", () => { + expect(getWheelLineDelta(0, 0, 16, 40)).toBe(0); + expect(getWheelLineDelta(Number.NaN, 0, 16, 40)).toBe(0); + expect(getWheelLineDelta(Number.POSITIVE_INFINITY, 0, 16, 40)).toBe(0); + }); + it("converts pixel deltas using cell height", () => { expect(getWheelLineDelta(32, 0, 16, 40)).toBe(2); expect(getWheelLineDelta(-24, 0, 12, 40)).toBe(-2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/termutil.test.ts` around lines 5 - 24, Add tests exercising the early-return guard in getWheelLineDelta for zero and non-finite delta values: add cases calling getWheelLineDelta with delta = 0, delta = NaN, and delta = Infinity (and -Infinity) and assert they return the expected early-return value (e.g., 0 or the function's defined fallback) to cover the branch at line handling `0`, `NaN`, `Infinity`; place these assertions alongside the existing describe("getWheelLineDelta") tests so the guard-branch is exercised during unit runs.pkg/wshrpc/wshremote/wshremote_file.go (1)
410-431: Consider simplifying the redundant root check.Lines 426-429 check
path == "/"afterfilepath.ToSlash, but on non-Windows this path would already have been/if it were a root. The check is harmless but slightly redundant since the function would have returned at line 428 for the actual Unix root case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/wshrpc/wshremote/wshremote_file.go` around lines 410 - 431, computeDirPart contains a redundant root check after converting to slashes: remove the path == "/" branch and its return so the function relies on filepath.Dir(path) (which already returns "/" for the Unix root) after filepath.ToSlash; update function computeDirPart to drop the redundant condition and keep the Windows-specific logic and the final return of filepath.Dir(path).
🤖 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/termwrap.ts`:
- Around line 331-343: The wheel handler computes a fractional lineDelta and
early-returns when wholeLines === 0, but fails to call
event.preventDefault()/event.stopPropagation(), allowing parent scrolling;
update the handler around the code that updates this.wheelScrollRemainder and
computes wholeLines (the block referencing wheelScrollRemainder, wholeLines and
this.terminal.scrollLines) so that if lineDelta !== 0 (i.e.,
wheelScrollRemainder changed) you call event.preventDefault() and
event.stopPropagation() before returning when wholeLines === 0; keep the
existing behavior of subtracting wholeLines and calling
this.terminal.scrollLines(wholeLines) when wholeLines !== 0.
- Around line 322-324: The code calls target.closest(".xterm-viewport") on the
assumed Element in the event handler in termwrap.ts; modify the guard so you
first verify target is an Element (e.g., using `target instanceof Element` or
checking `target.nodeType === Node.ELEMENT_NODE`) before calling `.closest()`
and only return early if that Element.closest(...) check is truthy; update the
`target` handling around the existing `const target = event.target as Element |
null` and the `if (target?.closest(".xterm-viewport") != null) { return; }` to
perform the explicit Element check first.
---
Nitpick comments:
In @.idea/vcs.xml:
- Around line 1-6: Remove the accidental IntelliJ metadata from this feature PR
by reverting or deleting the .idea/vcs.xml change and ensure the repository
ignores IDE files: remove .idea/vcs.xml from the commit (or run git rm --cached
.idea/vcs.xml) and add .idea/ (or at least .idea/vcs.xml) to .gitignore so
future commits won’t include VcsDirectoryMappings; if this file must remain, add
a short justification in the PR description explaining why it’s intentionally
versioned.
In `@frontend/app/view/preview/preview-directory.tsx`:
- Line 881: The callback's dependency array includes an unused setter and misses
a used variable: remove setRefreshVersion from the dependency array (it's not
referenced inside the callback) and add finfo (used via addOpenMenuItems(menu,
conn, finfo)); ensure the dependency array becomes [canCreateEntries, conn,
newFile, newDirectory, dirPath, finfo] so the useCallback/useEffect reflects
actual internal references.
- Line 438: The callback's dependency array incorrectly includes
setRefreshVersion while omitting setErrorMsg: remove setRefreshVersion and add
setErrorMsg so the useCallback/useEffect for the preview-directory callback
references the actual reactive values (update the dependency array that
currently lists [canCreateEntries, setRefreshVersion, conn] to
[canCreateEntries, conn, setErrorMsg]); locate the callback in
preview-directory.tsx that uses setErrorMsg (around line 432) and adjust its
dependency array accordingly.
In `@frontend/app/view/term/termutil.test.ts`:
- Around line 5-24: Add tests exercising the early-return guard in
getWheelLineDelta for zero and non-finite delta values: add cases calling
getWheelLineDelta with delta = 0, delta = NaN, and delta = Infinity (and
-Infinity) and assert they return the expected early-return value (e.g., 0 or
the function's defined fallback) to cover the branch at line handling `0`,
`NaN`, `Infinity`; place these assertions alongside the existing
describe("getWheelLineDelta") tests so the guard-branch is exercised during unit
runs.
In `@frontend/util/historyutil.test.ts`:
- Around line 5-28: Add tests to frontend/util/historyutil.test.ts that exercise
getParentDirectory with the suggested edge cases: mixed separators (e.g.,
"C:\\Users\\wave/Downloads"), UNC using forward slashes (e.g.,
"//server/share/folder"), and a bare drive letter (e.g., "C:"); assert expected
parent behavior consistent with existing cases (normalize separators, return
root/share when appropriate, and handle "C:" as its own root or parent per
current implementation) so edge-case parsing is covered alongside the
POSIX/Windows/UNC tests.
In `@pkg/wshrpc/wshremote/wshremote_file.go`:
- Around line 410-431: computeDirPart contains a redundant root check after
converting to slashes: remove the path == "/" branch and its return so the
function relies on filepath.Dir(path) (which already returns "/" for the Unix
root) after filepath.ToSlash; update function computeDirPart to drop the
redundant condition and keep the Windows-specific logic and the final return of
filepath.Dir(path).
🪄 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: 5fea7e20-e620-4e64-81b5-d6b768d59c65
📒 Files selected for processing (19)
.idea/.gitignore.idea/inspectionProfiles/Project_Default.xml.idea/misc.xml.idea/modules.xml.idea/prettier.xml.idea/vcs.xml.idea/waveterm.imlelectron-builder.config.cjsemain/emain-platform.tsfrontend/app/view/preview/preview-directory.tsxfrontend/app/view/term/termutil.test.tsfrontend/app/view/term/termutil.tsfrontend/app/view/term/termwrap.tsfrontend/util/historyutil.test.tsfrontend/util/historyutil.tspkg/remote/conncontroller/conncontroller.gopkg/remote/conncontroller/conncontroller_test.gopkg/wshrpc/wshremote/wshremote_file.gopkg/wshrpc/wshremote/wshremote_file_test.go
- 在SSH配置解析中添加Windows系统配置文件支持 - 实现Windows驱动器根目录和UNC路径的正确处理 - 修复历史导航中的路径分隔符问题 - 添加Windows虚拟根目录的支持 - 增强文件系统操作的跨平台兼容性 - 为终端滚动添加滚轮事件处理功能 - 实现对创建条目的权限控制检查
- 在SSH配置解析中添加Windows系统配置文件支持 - 实现Windows驱动器根目录和UNC路径的正确处理 - 修复历史导航中的路径分隔符问题 - 添加Windows虚拟根目录的支持 - 增强文件系统操作的跨平台兼容性 - 为终端滚动添加滚轮事件处理功能 - 实现对创建条目的权限控制检查
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/app/view/term/termwrap.ts (2)
331-342:⚠️ Potential issue | 🟠 MajorPrevent default before the fractional early return.
Line 336-Line 338 returns when
wholeLines === 0without suppressing the event, so trackpad micro-scroll can bubble and scroll parent containers/pages.🔧 Suggested fix
this.wheelScrollRemainder += lineDelta; const wholeLines = this.wheelScrollRemainder > 0 ? Math.floor(this.wheelScrollRemainder) : Math.ceil(this.wheelScrollRemainder); + event.preventDefault(); + event.stopPropagation(); if (wholeLines === 0) { return; } this.wheelScrollRemainder -= wholeLines; this.terminal.scrollLines(wholeLines); - event.preventDefault(); - event.stopPropagation();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/termwrap.ts` around lines 331 - 342, The handler currently returns early when wholeLines === 0 without suppressing the original wheel event, allowing fractional trackpad scrolls to bubble; modify the logic in the method that uses wheelScrollRemainder/wholeLines (the block that calls this.terminal.scrollLines) so that event.preventDefault() and event.stopPropagation() are invoked before the early return (or invoked unconditionally for this wheel handler), then keep the existing subtraction of wholeLines and terminal.scrollLines(wholeLines) path unchanged.
322-324:⚠️ Potential issue | 🟡 MinorGuard
.closest()with an actualElementruntime check.Line 322-Line 324 uses a type cast, but non-
Elementevent targets can still reach runtime and make.closest(...)throw.🔧 Suggested fix
- const target = event.target as Element | null; - if (target?.closest(".xterm-viewport") != null) { + const target = event.target; + if (target instanceof Element && target.closest(".xterm-viewport") != null) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/termwrap.ts` around lines 322 - 324, The guard uses a type cast for event.target then calls target?.closest(...), which can still throw if target is not actually an Element at runtime; change the check to ensure event.target is an Element (e.g., use "if (!(event.target instanceof Element)) return" or similar) before calling closest, updating the code around the target variable in termwrap.ts (the const target = event.target as Element | null and subsequent target?.closest(".xterm-viewport") check) so closest is only invoked on real Element instances.
🧹 Nitpick comments (1)
pkg/wshrpc/wshremote/wshremote_file.go (1)
70-102: Consider potential latency when probing unmounted drives.Iterating through all 26 drive letters and calling
os.Staton each is a common Windows pattern. However, probing non-existent or unmounted drives (like network drives or removable media) can incur timeout delays on Windows, potentially making this function slow (several seconds in worst case).For typical desktop systems with only a few local drives, this should be acceptable. If latency becomes an issue, consider parallelizing the probes or caching results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/wshrpc/wshremote/wshremote_file.go` around lines 70 - 102, The loop in listWindowsDriveInfos currently calls statFn serially for 'A'..'Z', which can block on unmounted/network drives; make the probes concurrent and bounded to avoid long latency: spawn goroutines for each drive that call statFn(root) but enforce a per-probe timeout (context with deadline or select on time.After) and use a semaphore or worker pool to limit concurrency (e.g., max 4-8 workers), collect successful results via channel and join with a WaitGroup, then append those results to entries; optionally add an in-memory cache keyed by drive root in listWindowsDriveInfos to short-circuit frequent calls. Ensure you keep using the statFn parameter and preserve the FileInfo construction logic when moving into the goroutine.
🤖 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/preview/preview-directory.tsx`:
- Line 190: Guard access to model.statFile when deriving supportsmkdir: change
the canCreateEntries read to safely handle null/undefined from
useAtomValue(model.statFile) (e.g., const stat = useAtomValue(model.statFile);
const canCreateEntries = stat?.supportsmkdir ?? true) and apply the same pattern
in both DirectoryTable and TableBody where useAtomValue(model.statFile) is
dereferenced so rendering won't crash when the atom is temporarily
null/undefined.
- Around line 374-397: The code currently uses FileInfo.supportsmkdir (exposed
as canCreateEntries) to gate creation, rename and delete actions; change the
logic so creation actions ("New File"/"New Folder") remain gated by
canCreateEntries (or FileInfo.supportsmkdir) but allow rename
(table.options.meta.updateName(finfo.path, finfo.isdir)) and delete handlers to
be gated by separate capability flags (e.g., canModifyEntries /
canDeleteEntries) or left unconditional if the API supports them; update the
menu push so only the creation menu items check canCreateEntries while the
Rename and Delete menu items check the appropriate new flags (or no flag), and
ensure you reference table.options.meta.updateName and the delete callback when
making the change.
---
Duplicate comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 331-342: The handler currently returns early when wholeLines === 0
without suppressing the original wheel event, allowing fractional trackpad
scrolls to bubble; modify the logic in the method that uses
wheelScrollRemainder/wholeLines (the block that calls this.terminal.scrollLines)
so that event.preventDefault() and event.stopPropagation() are invoked before
the early return (or invoked unconditionally for this wheel handler), then keep
the existing subtraction of wholeLines and terminal.scrollLines(wholeLines) path
unchanged.
- Around line 322-324: The guard uses a type cast for event.target then calls
target?.closest(...), which can still throw if target is not actually an Element
at runtime; change the check to ensure event.target is an Element (e.g., use "if
(!(event.target instanceof Element)) return" or similar) before calling closest,
updating the code around the target variable in termwrap.ts (the const target =
event.target as Element | null and subsequent target?.closest(".xterm-viewport")
check) so closest is only invoked on real Element instances.
---
Nitpick comments:
In `@pkg/wshrpc/wshremote/wshremote_file.go`:
- Around line 70-102: The loop in listWindowsDriveInfos currently calls statFn
serially for 'A'..'Z', which can block on unmounted/network drives; make the
probes concurrent and bounded to avoid long latency: spawn goroutines for each
drive that call statFn(root) but enforce a per-probe timeout (context with
deadline or select on time.After) and use a semaphore or worker pool to limit
concurrency (e.g., max 4-8 workers), collect successful results via channel and
join with a WaitGroup, then append those results to entries; optionally add an
in-memory cache keyed by drive root in listWindowsDriveInfos to short-circuit
frequent calls. Ensure you keep using the statFn parameter and preserve the
FileInfo construction logic when moving into the goroutine.
🪄 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: 0470bf05-2f5c-4350-a9a5-7539fc1f5095
📒 Files selected for processing (20)
.idea/.gitignore.idea/inspectionProfiles/Project_Default.xml.idea/misc.xml.idea/modules.xml.idea/prettier.xml.idea/vcs.xml.idea/waveterm.imlREADME.mdelectron-builder.config.cjsemain/emain-platform.tsfrontend/app/view/preview/preview-directory.tsxfrontend/app/view/term/termutil.test.tsfrontend/app/view/term/termutil.tsfrontend/app/view/term/termwrap.tsfrontend/util/historyutil.test.tsfrontend/util/historyutil.tspkg/remote/conncontroller/conncontroller.gopkg/remote/conncontroller/conncontroller_test.gopkg/wshrpc/wshremote/wshremote_file.gopkg/wshrpc/wshremote/wshremote_file_test.go
✅ Files skipped from review due to trivial changes (11)
- README.md
- .idea/vcs.xml
- .idea/prettier.xml
- .idea/misc.xml
- .idea/inspectionProfiles/Project_Default.xml
- .idea/.gitignore
- .idea/waveterm.iml
- .idea/modules.xml
- frontend/util/historyutil.test.ts
- pkg/remote/conncontroller/conncontroller_test.go
- pkg/wshrpc/wshremote/wshremote_file_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- electron-builder.config.cjs
- pkg/remote/conncontroller/conncontroller.go
- frontend/util/historyutil.ts
- frontend/app/view/term/termutil.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/app/view/preview/preview-directory.tsx (2)
190-190:⚠️ Potential issue | 🔴 CriticalGuard
model.statFilebefore readingsupportsmkdir.Line 190 and Line 330 still dereference
useAtomValue(model.statFile)directly; this can crash render during transient null/undefined state.💡 Suggested fix
-const canCreateEntries = useAtomValue(model.statFile).supportsmkdir ?? true; +const statFile = useAtomValue(model.statFile); +const canCreateEntries = statFile?.supportsmkdir ?? true;Apply this in both
DirectoryTableandTableBody.Also applies to: 330-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-directory.tsx` at line 190, The code dereferences useAtomValue(model.statFile).supportsmkdir directly which can be null/undefined during transient states; update the logic in DirectoryTable and TableBody to first read const stat = useAtomValue(model.statFile) and then guard access with stat?.supportsmkdir (or default to true) when computing canCreateEntries (and any similar checks), e.g., compute const canCreateEntries = stat ? stat.supportsmkdir ?? true : true so rendering never accesses properties on null/undefined.
374-397:⚠️ Potential issue | 🟠 MajorDon’t gate Rename/Delete behind create-only capability.
Line 389 (Rename) and Line 431 (Delete) are still conditioned by
canCreateEntries(supportsmkdir), so non-create scenarios can lose unrelated actions.💡 Suggested direction
if (canCreateEntries) { menu.push( { label: "New File", click: () => table.options.meta.newFile() }, { label: "New Folder", click: () => table.options.meta.newDirectory() }, - { label: "Rename", click: () => table.options.meta.updateName(finfo.path, finfo.isdir) }, { type: "separator" } ); } + +menu.push({ + label: "Rename", + click: () => table.options.meta.updateName(finfo.path, finfo.isdir), +}); ... -if (canCreateEntries) { +if (canDeleteEntries /* or equivalent capability */) { menu.push( { type: "separator" }, { label: "Delete", click: () => handleFileDelete(model, finfo.path, false, setErrorMsg) } ); }Also applies to: 425-435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/preview/preview-directory.tsx` around lines 374 - 397, The Rename and Delete menu items are incorrectly placed inside the canCreateEntries (supportsmkdir) conditional, causing rename/delete to be hidden when create permission is false; move the menu entries that call table.options.meta.updateName (Rename) and the Delete handler out of the canCreateEntries block so they are always added, or instead gate them with the correct permissions (e.g., canRename / canDelete) if those exist, ensuring the separator logic is adjusted accordingly and the Delete menu entry referenced around lines 425-435 is handled similarly.
🧹 Nitpick comments (1)
frontend/app/view/term/termwrap.ts (1)
318-351: Well-structured wheel scroll handler with proper event management.The implementation correctly:
- Guards against non-Element targets before calling
closest()- Prevents default for both whole-line scrolls and sub-line accumulations
- Defers to the terminal when mouse tracking is active
- Uses capturing phase with
passive: falseto enablepreventDefault()One minor observation on Line 326: accessing
_core._renderService.dimensions.css.cell.heightrelies on xterm.js internal APIs that could change between versions. The fallback to16is a reasonable safeguard, but consider adding a comment documenting this dependency for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/termwrap.ts` around lines 318 - 351, The wheelHandler uses xterm.js internals when reading cell height via (this.terminal as any)?._core?._renderService?.dimensions?.css?.cell?.height with a numeric fallback of 16; add an inline comment next to that access (near wheelHandler and the _core/_renderService reference) documenting that this relies on private xterm.js APIs which may change across versions, why the 16 fallback is chosen, and a note to revisit/replace with a public API if/when xterm exposes cell dimensions.
🤖 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/preview/preview-directory.tsx`:
- Line 583: The current optimistic defaulting (const canCreateEntries =
finfo?.supportsmkdir ?? true) treats missing SupportsMkdir as true and overrides
backend intent; update the checks to require an explicit true (e.g., replace
uses of "finfo?.supportsmkdir ?? true" with a strict equality check like
"finfo?.supportsmkdir === true") for canCreateEntries (and the other occurrences
around the earlier checks noted) so a missing/omitted field does not enable
creation; alternatively, if you control the backend, ensure SupportsMkdir is
always serialized (remove omitempty) so the field presence accurately reflects
the capability.
---
Duplicate comments:
In `@frontend/app/view/preview/preview-directory.tsx`:
- Line 190: The code dereferences useAtomValue(model.statFile).supportsmkdir
directly which can be null/undefined during transient states; update the logic
in DirectoryTable and TableBody to first read const stat =
useAtomValue(model.statFile) and then guard access with stat?.supportsmkdir (or
default to true) when computing canCreateEntries (and any similar checks), e.g.,
compute const canCreateEntries = stat ? stat.supportsmkdir ?? true : true so
rendering never accesses properties on null/undefined.
- Around line 374-397: The Rename and Delete menu items are incorrectly placed
inside the canCreateEntries (supportsmkdir) conditional, causing rename/delete
to be hidden when create permission is false; move the menu entries that call
table.options.meta.updateName (Rename) and the Delete handler out of the
canCreateEntries block so they are always added, or instead gate them with the
correct permissions (e.g., canRename / canDelete) if those exist, ensuring the
separator logic is adjusted accordingly and the Delete menu entry referenced
around lines 425-435 is handled similarly.
---
Nitpick comments:
In `@frontend/app/view/term/termwrap.ts`:
- Around line 318-351: The wheelHandler uses xterm.js internals when reading
cell height via (this.terminal as
any)?._core?._renderService?.dimensions?.css?.cell?.height with a numeric
fallback of 16; add an inline comment next to that access (near wheelHandler and
the _core/_renderService reference) documenting that this relies on private
xterm.js APIs which may change across versions, why the 16 fallback is chosen,
and a note to revisit/replace with a public API if/when xterm exposes cell
dimensions.
🪄 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: 18e13afe-61c7-4523-b38f-0b090c2519ce
📒 Files selected for processing (6)
.gitignorefrontend/app/view/preview/preview-directory.tsxfrontend/app/view/term/termutil.test.tsfrontend/app/view/term/termwrap.tsfrontend/util/historyutil.test.tspkg/wshrpc/wshremote/wshremote_file.go
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- frontend/util/historyutil.test.ts
- frontend/app/view/term/termutil.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/termwrap.ts`:
- Around line 330-331: The current cellHeight calculation using nullish
coalescing can still yield 0 which can cause division-by-zero in
getWheelLineDelta; modify the logic around the cellHeight variable (where
cellHeight is computed from (this.terminal as
any)?._core?._renderService?.dimensions?.css?.cell?.height) to explicitly guard
against 0 — either return early if height === 0 (like the fit addon) or replace
a falsy/zero value with a sensible default (e.g., 16) before calling
getWheelLineDelta; ensure the change references the same symbol names
(cellHeight, this.terminal, getWheelLineDelta) so the guard is applied in that
calculation site.
🪄 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: cf74b5ad-49d3-45a2-b6a9-a2c95fc06fc1
📒 Files selected for processing (2)
frontend/app/view/preview/preview-directory.tsxfrontend/app/view/term/termwrap.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/view/preview/preview-directory.tsx
Comment on lines
+330
to
+331
| const cellHeight = (this.terminal as any)?._core?._renderService?.dimensions?.css?.cell?.height ?? 16; | ||
| const lineDelta = getWheelLineDelta(event.deltaY, event.deltaMode, cellHeight, this.terminal.rows); |
Contributor
There was a problem hiding this comment.
Nullish coalescing doesn't guard against cellHeight of 0.
The ?? operator only falls back for null/undefined, not for 0. If the renderer dimensions return height: 0 (e.g., before terminal fully renders), cellHeight will be 0, potentially causing division-by-zero issues in getWheelLineDelta.
The pattern in fitaddon.ts (lines 80-82) explicitly checks if (dims.css.cell.height === 0) and returns early. Consider a similar guard here, or use a logical OR with a truthy fallback.
🛡️ Suggested fix
- const cellHeight = (this.terminal as any)?._core?._renderService?.dimensions?.css?.cell?.height ?? 16;
+ const cellHeight = (this.terminal as any)?._core?._renderService?.dimensions?.css?.cell?.height || 16;Alternatively, add an explicit guard:
const cellHeight = (this.terminal as any)?._core?._renderService?.dimensions?.css?.cell?.height ?? 16;
+ if (cellHeight === 0) {
+ return;
+ }
const lineDelta = getWheelLineDelta(event.deltaY, event.deltaMode, cellHeight, this.terminal.rows);📝 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 cellHeight = (this.terminal as any)?._core?._renderService?.dimensions?.css?.cell?.height ?? 16; | |
| const lineDelta = getWheelLineDelta(event.deltaY, event.deltaMode, cellHeight, this.terminal.rows); | |
| const cellHeight = (this.terminal as any)?._core?._renderService?.dimensions?.css?.cell?.height || 16; | |
| const lineDelta = getWheelLineDelta(event.deltaY, event.deltaMode, cellHeight, this.terminal.rows); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/termwrap.ts` around lines 330 - 331, The current
cellHeight calculation using nullish coalescing can still yield 0 which can
cause division-by-zero in getWheelLineDelta; modify the logic around the
cellHeight variable (where cellHeight is computed from (this.terminal as
any)?._core?._renderService?.dimensions?.css?.cell?.height) to explicitly guard
against 0 — either return early if height === 0 (like the fit addon) or replace
a falsy/zero value with a sensible default (e.g., 16) before calling
getWheelLineDelta; ensure the change references the same symbol names
(cellHeight, this.terminal, getWheelLineDelta) so the guard is applied in that
calculation site.
Author
|
请查阅 |
- 在网页视图右键菜单中添加刷新选项，支持 CmdOrCtrl+R 快捷键 - 为 webview 上下文菜单添加刷新功能 - 优化预加载脚本中的上下文菜单触发逻辑，过滤可编辑元素和选中文本 - 添加默认终端字体大小配置项，默认值为 15 - 在终端模型中使用统一的默认字体大小常量 - 更新分隔拖拽手柄样式，改进透明度和颜色过渡效果 - 实现窗口标题与标签页名称同步功能，动态更新页面标题显示
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.