add workspace directory feature with shell quoting and tests #2763
Conversation
rnunley-nmg
commented
Jan 10, 2026
rnunley-nmg
commented
- Add directory field to workspaces for setting default working directory
- Terminals cd to workspace directory on change; preview blocks navigate to it
- Add folder picker dialog (Electron showOpenDialog)
- Add shellQuote utility for safe shell command construction
- Add path validation to reject null bytes in directory paths
- Debounce directory updates to prevent rapid API calls
- Expose WAVETERM_WORKSPACE_DIR environment variable to shells
- Add frontend tests for shellQuote function
- Add Go tests for workspace directory validation
|
|
WalkthroughAdds end-to-end workspace directory support and a folder picker. Main process: new IPC handler Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In @frontend/app/tab/workspaceeditor.tsx:
- Around line 123-144: The folder Browse button click handler calls
getApi().showOpenFolderDialog() without error handling; wrap the async call in a
try/catch inside the onClick handler (the anonymous async function for the
Button) to catch API/IPC/permission errors, log the error (use console.error or
an existing logger) and optionally surface a user-facing message (e.g., via a
toast) instead of letting the promise reject; ensure you still call
onDirectoryChange(path) when a valid path is returned and do not change other
behavior of the directory-input elements.
In @pkg/blockcontroller/shellcontroller.go:
- Around line 393-395: The getWorkspaceDirectory call can hang and is
duplicated; wrap that DB call in a short cancellable context (e.g.,
context.WithTimeout ~2s) and cache the result in a local wsDir variable when
cmdOpts.Cwd == "" and connUnion.ConnType == ConnType_Local, then pass wsDir into
makeSwapToken instead of letting makeSwapToken refetch; update makeSwapToken
signature/usage in ShellController.setupAndStartShellProcess (and any callers)
to accept an optional workspaceDir string so the workspace is fetched once and
reused, or at minimum ensure getWorkspaceDirectory is invoked with a timeout
context to prevent indefinite hangs.
🧹 Nitpick comments (4)
pkg/blockcontroller/shellcontroller.go (2)
721-734: Consider logging errors and validating the directory path.The function silently returns an empty string for all error cases, which might hide legitimate issues like database connectivity problems or data inconsistencies. Consider adding error logging for debugging purposes.
♻️ Suggested improvement
func getWorkspaceDirectory(ctx context.Context, tabId string) string { if tabId == "" { return "" } wsId, err := wstore.DBFindWorkspaceForTabId(ctx, tabId) if err != nil { + log.Printf("error finding workspace for tab %s: %v\n", tabId, err) return "" } ws, err := wstore.DBGet[*waveobj.Workspace](ctx, wsId) if err != nil || ws == nil { + if err != nil { + log.Printf("error getting workspace %s: %v\n", wsId, err) + } return "" } return ws.Directory }Additionally, verify that the returned directory path doesn't contain null bytes or other invalid characters (as mentioned in the PR objectives).
758-761: Code duplication withgetWorkspaceDirectoryfunction.This code block duplicates the workspace fetching logic from
getWorkspaceDirectory(lines 721-734). When both functions are called in the same execution path (as happens for local shell connections), the workspace is queried twice from the database.Consider refactoring to fetch the workspace once and pass it through, or cache the result. This aligns with the earlier comment on lines 393-395 about duplicate queries.
Option: Fetch workspace once in
setupAndStartShellProcessand pass directory to both locations:// In setupAndStartShellProcess, before line 393: var wsDir string if connUnion.ConnType == ConnType_Local { ctx, cancelFn := context.WithTimeout(context.Background(), 2*time.Second) defer cancelFn() wsDir = getWorkspaceDirectory(ctx, bc.TabId) if cmdOpts.Cwd == "" { cmdOpts.Cwd = wsDir } } // Then pass wsDir to makeSwapToken: swapToken := bc.makeSwapToken(ctx, logCtx, blockMeta, remoteName, connUnion.ShellType, wsDir) // In makeSwapToken, use the passed wsDir instead of re-queryingfrontend/app/tab/workspaceswitcher.tsx (2)
216-244: Use strict equality operator.Line 223 uses
!=instead of!==. For consistency with TypeScript best practices, use strict equality.♻️ Proposed fix
- if (newWorkspace.name != "") { + if (newWorkspace.name !== "") {
223-239: Consider simplifying the update logic.The branching logic handles directory changes differently from other field changes, which adds complexity. The current implementation:
- Debounces only directory changes
- Immediately saves non-directory changes
- Only saves directory when it changes (line 224-227)
- Saves all fields together when directory is unchanged (line 228-238)
This creates an asymmetry where directory changes are debounced but other field changes are immediate. Consider whether all workspace field changes should use the same debounced approach for consistency.
📜 Review details
Configuration used: Repository 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 (17)
emain/emain-ipc.tsemain/preload.tsfrontend/app/store/services.tsfrontend/app/tab/workspaceeditor.scssfrontend/app/tab/workspaceeditor.tsxfrontend/app/tab/workspaceswitcher.tsxfrontend/app/view/preview/preview-model.tsxfrontend/types/custom.d.tsfrontend/types/gotypes.d.tsfrontend/util/util.test.tsfrontend/util/util.tspkg/blockcontroller/shellcontroller.gopkg/service/workspaceservice/workspaceservice.gopkg/waveobj/wtype.gopkg/wconfig/defaultconfig/widgets.jsonpkg/wcore/workspace.gopkg/wcore/workspace_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
📚 Learning: 2024-12-27T22:06:04.948Z
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
Applied to files:
pkg/wcore/workspace_test.gofrontend/app/tab/workspaceswitcher.tsx
🧬 Code graph analysis (7)
frontend/util/util.test.ts (1)
frontend/util/util.ts (1)
shellQuote(547-547)
emain/emain-ipc.ts (1)
emain/emain-window.ts (1)
focusedWaveWindow(100-100)
pkg/blockcontroller/shellcontroller.go (2)
pkg/wstore/wstore_dbops.go (2)
DBFindWorkspaceForTabId(368-385)DBGet(133-136)pkg/waveobj/wtype.go (2)
Workspace(171-181)Workspace(183-185)
frontend/app/tab/workspaceeditor.tsx (3)
frontend/util/util.ts (1)
fireAndForget(532-532)frontend/app/store/services.ts (1)
WorkspaceService(190-190)frontend/app/store/global.ts (1)
getApi(826-826)
pkg/wcore/workspace_test.go (1)
pkg/wcore/workspace.go (1)
ValidateWorkspaceDirectory(27-35)
pkg/wcore/workspace.go (2)
frontend/app/store/services.ts (1)
UpdateWorkspace(185-187)pkg/waveobj/wtype.go (2)
Workspace(171-181)Workspace(183-185)
frontend/app/tab/workspaceswitcher.tsx (7)
frontend/app/store/services.ts (1)
WorkspaceService(190-190)pkg/service/workspaceservice/workspaceservice.go (1)
WorkspaceService(23-23)pkg/waveobj/wtype.go (4)
Block(284-292)Block(294-296)Workspace(171-181)Workspace(183-185)frontend/app/store/global.ts (1)
getAllBlockComponentModels(825-825)frontend/app/store/wshclientapi.ts (1)
RpcApi(717-717)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(38-38)frontend/util/util.ts (3)
stringToBase64(550-550)shellQuote(547-547)fireAndForget(532-532)
🔇 Additional comments (23)
frontend/app/tab/workspaceeditor.scss (1)
64-89: LGTM! Well-structured directory selector styles.The directory selector styles are well-organized and consistent with the existing patterns in the file. The flexbox layout appropriately handles the input expansion and button sizing.
frontend/types/custom.d.ts (1)
137-137: LGTM! Type definition matches implementation.The showOpenFolderDialog type signature correctly reflects the async nature of the folder picker with appropriate nullable return type for cancellation scenarios. This aligns with the implementation in emain/preload.ts.
pkg/waveobj/wtype.go (1)
177-177: LGTM! Directory field addition is properly implemented with comprehensive validation.The Directory field at line 177 is correctly added to the Workspace struct with the omitempty JSON tag. The null byte validation is properly implemented in the workspace service layer via
ValidateWorkspaceDirectory()in pkg/wcore/workspace.go, which usesstrings.ContainsRune(directory, 0)to reject paths containing null bytes. Test coverage is comprehensive, including test cases for null bytes at the end, in the middle, multiple occurrences, and valid unicode paths.emain/preload.ts (1)
71-71: LGTM! IPC handler implementation verified.The showOpenFolderDialog API is correctly exposed through the contextBridge with proper async handling via ipcRenderer.invoke. The corresponding IPC handler in emain-ipc.ts is properly implemented using ipcMain.handle with electron.dialog.showOpenDialog, appropriate dialog options for directory selection, and correct null-checking for the window and dialog results. The implementation matches the type definition in custom.d.ts.
frontend/util/util.ts (1)
466-483: LGTM! Well-designed shell escaping implementation.The shellQuote function correctly implements POSIX shell escaping using single quotes with proper handling of embedded quotes via the
'\\''pattern. The safe character regex is appropriately conservative, and the function effectively prevents command injection. It is consistently used for directory paths passed to shell commands (seeworkspaceswitcher.tsxline 180:cd ${shellQuote(newDirectory)}), and comprehensive test coverage validates all edge cases including empty strings, spaces, special shell characters, and embedded quotes.pkg/wcore/workspace_test.go (1)
10-103: Well-structured test coverage for directory validation.The table-driven tests comprehensively cover the
ValidateWorkspaceDirectoryfunction including edge cases for null bytes at various positions, Unicode/emoji paths, and Windows-style paths. The test structure follows Go conventions and properly validates both success and error scenarios.emain/emain-ipc.ts (1)
261-274: LGTM!The IPC handler follows the established patterns in the file, properly checks for
focusedWaveWindow, and correctly handles the dialog cancellation case by returningnull.frontend/util/util.test.ts (1)
7-78: Comprehensive security-focused test coverage.The test suite thoroughly validates the
shellQuotefunction, including critical command injection prevention scenarios (;,$(), backticks,&&). The single-quote escaping logic ('it'\''s'pattern) is correctly tested. Good coverage of edge cases including empty strings, whitespace characters, and complex paths.frontend/types/gotypes.d.ts (1)
1865-1872: Generated type correctly reflects backend changes.The optional
directoryfield aligns with theDirectory stringfield added to the GoWorkspacestruct withjson:"directory,omitempty".frontend/app/store/services.ts (1)
185-187: Generated service signature correctly updated.The
directoryparameter is correctly positioned to match the backendUpdateWorkspacefunction signature.frontend/app/view/preview/preview-model.tsx (1)
376-389: Good integration of workspace directory as preview fallback.The logic correctly prioritizes: (1) explicit file path, (2) workspace directory for local connections, (3) fallback to
~. The checkif (!connName)appropriately limits workspace directory usage to local contexts, avoiding unexpected behavior for remote connections.pkg/wcore/workspace.go (2)
25-35: Minimal but effective null byte validation.The validation correctly rejects directory paths containing null bytes, which prevents potential security issues with path manipulation. The function appropriately allows empty strings for clearing/unsetting the directory.
122-128: Add test coverage for UpdateWorkspace directory clearing behavior.The code correctly allows empty directory strings to clear the workspace directory—
ValidateWorkspaceDirectoryexplicitly permits this, and the conditiondirectory != ws.Directorywill trigger the update. This appears intentional based on frontend usage (initialization with empty string whenapplyDefaults=true), but the behavior lacks test coverage.Additionally, the frontend protection is incomplete: while line 208 in
workspaceswitcher.tsxprevents the empty directory from affecting the current workspace, other named workspaces can have their directory cleared without explicit guard conditions. Add tests to verify:
- That
UpdateWorkspacecorrectly clears a workspace directory when an empty string is passed- That non-empty workspaces can be modified including directory changes
- That the
applyDefaultsflag doesn't interfere with explicit directory updatespkg/service/workspaceservice/workspaceservice.go (1)
42-42: LGTM! Service signature correctly extended.The method signature and metadata have been properly updated to include the directory parameter. The parameter ordering (directory before applyDefaults) is consistent with the method signature.
Also applies to: 46-48
frontend/app/tab/workspaceeditor.tsx (3)
17-35: LGTM! Improved component naming for debugging.Refactoring
ColorSelectorandIconSelectorto use named functions withinmemo()improves the display names in React DevTools.Also applies to: 44-64
70-70: LGTM! Component signature properly extended.The component props and signature have been correctly updated to include directory handling.
Also applies to: 75-75, 78-89
95-102: LGTM! Service calls updated correctly.The WorkspaceService API calls have been properly updated to use the new method signatures.
frontend/app/tab/workspaceswitcher.tsx (5)
144-159: LGTM! Type guards properly implemented.The type guard functions correctly distinguish between different ViewModel types using both type checks and property existence checks. The
isPreviewViewModelguard appropriately checks forviewType === "preview", the baseBlockAwareViewModelinterface, and thegoHistorymethod.
200-214: LGTM! Debounce implementation is correct.The debounced directory update properly:
- Uses a 300ms delay to prevent rapid API calls
- Clears the pending reference before executing
- Checks if the workspace is current before updating blocks
- Compares new vs old directory to avoid unnecessary updates
90-90: LGTM! Service calls correctly updated.All
WorkspaceService.UpdateWorkspacecalls have been properly updated to include the directory parameter in the correct position (beforeapplyDefaults).Also applies to: 207-207, 229-236
312-312: LGTM! Directory props properly wired.The directory value and change handler are correctly passed to the
WorkspaceEditorcomponent, with appropriate fallback to empty string for undefined values.Also applies to: 317-317
172-174: Connection detection is reliable and comprehensive.The
blockData?.meta?.connectionproperty correctly identifies all remote connection types throughout the codebase. All remote connections—SSH, WSL, AWS, containers, and others—consistently use this same metadata key. Local connections are identified via theisLocalConnName()utility, which returns true only for undefined/blank values or names starting with "local". This pattern is used uniformly across all block types (term, preview, sysinfo), confirming the approach is sound. No remote connection types use alternative metadata keys.pkg/wconfig/defaultconfig/widgets.json (1)
13-21: This change is safe and requires no additional verification.The preview widget properly handles missing
meta.filethrough a fallback chain infrontend/app/view/preview/preview-model.tsx(lines 376-389):
- Uses
meta.fileif present- Falls back to
workspace.directory(when no connection is specified)- Ultimately defaults to
"~"(home directory)Removing
"file": "~"fromdefwidget@filesis appropriate since the workspace directory feature now provides the initial context, and the home directory default is preserved as the final fallback. No code depends on this field being present in the widget definition.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/app/tab/workspaceeditor.tsx (1)
134-143: Consider adding user feedback for folder dialog errors.The error handling only logs to the console. While this might be acceptable for a native folder dialog that rarely fails, consider whether users should be notified of errors (e.g., through a toast notification).
💡 Optional: Add user feedback
If your app has a notification system, consider adding user feedback:
onClick={async () => { try { const path = await getApi().showOpenFolderDialog(); if (path) { onDirectoryChange(path); } } catch (e) { console.error("error opening folder dialog:", e); + // Optional: show user-facing error notification + // showErrorToast("Failed to open folder dialog"); } }}pkg/blockcontroller/shellcontroller.go (3)
389-391: Use defer for context cancellation.The immediate cancellation works but is less idiomatic. Using
defer wsCancel()right after line 389 would be cleaner and more maintainable.♻️ Refactor to use defer
wsCtx, wsCancel := context.WithTimeout(context.Background(), 2*time.Second) + defer wsCancel() wsId, wsDir := getWorkspaceInfo(wsCtx, bc.TabId) - wsCancel()
724-737: Add error logging for consistency and debugging.The function silently swallows errors, which is inconsistent with the error handling pattern used throughout this file (e.g., lines 751, 763, 770 all use
log.Printf). While returning empty strings allows graceful degradation, adding error logging would help with debugging issues related to workspace directory features.📝 Add error logging
func getWorkspaceInfo(ctx context.Context, tabId string) (wsId string, wsDir string) { if tabId == "" { return "", "" } wsId, err := wstore.DBFindWorkspaceForTabId(ctx, tabId) if err != nil { + log.Printf("error finding workspace for tab %s: %v\n", tabId, err) return "", "" } ws, err := wstore.DBGet[*waveobj.Workspace](ctx, wsId) if err != nil || ws == nil { + if err != nil { + log.Printf("error getting workspace %s: %v\n", wsId, err) + } return wsId, "" } return wsId, ws.Directory }
396-398: Null byte validation is properly implemented, but comprehensive path validation is incomplete.The null byte rejection (a PR objective) is correctly implemented in
ValidateWorkspaceDirectory(workspace.go:31-32) and tested comprehensively. However, the validation is limited to that check. Missing validations include:
- Path existence and accessibility checks
- Path traversal attack prevention (e.g.,
../sequences after home dir expansion)- Symlink handling
Consider whether these additional protections are needed in the workspace layer or if they're intentionally deferred to the OS/shell execution layer.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/tab/workspaceeditor.tsxpkg/blockcontroller/shellcontroller.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
🧬 Code graph analysis (1)
pkg/blockcontroller/shellcontroller.go (6)
pkg/blockcontroller/blockcontroller.go (1)
BlockController_Shell(28-28)pkg/waveobj/metaconsts.go (1)
MetaKey_CmdCwd(56-56)pkg/wstore/wstore_dbops.go (2)
DBFindWorkspaceForTabId(368-385)DBGet(133-136)pkg/waveobj/wtype.go (2)
Workspace(171-181)Workspace(183-185)pkg/waveobj/metamap.go (1)
MetaMapType(8-8)pkg/util/shellutil/tokenswap.go (1)
TokenSwapEntry(19-25)
🔇 Additional comments (10)
frontend/app/tab/workspaceeditor.tsx (8)
17-35: LGTM: Clean refactor to named function.The ColorSelector refactor improves debugging by using a named function in
memoand simplifies the click handler. No functional changes.
44-64: LGTM: Consistent refactor pattern.IconSelector follows the same refactor pattern as ColorSelector, maintaining consistency and improving code quality.
70-70: LGTM: Props correctly defined for directory feature.The new
directoryandonDirectoryChangeprops are properly typed and integrate well with the existing prop structure.Also applies to: 75-75
78-89: LGTM: Improved export pattern for React 19.The new memo export pattern with named function and inline props destructuring is cleaner and removes the need for type assertions. This aligns with React 19 best practices.
97-100: LGTM: Improved variable naming.The explicit
fetchedColorsandfetchedIconsvariable names improve code clarity without changing functionality.
1-1: TheshowOpenFolderDialogAPI is properly defined and implemented.Type definition exists in
frontend/types/custom.d.tsas() => Promise<string | null>, with corresponding preload implementation inemain/preload.tsand IPC handler inemain/emain-ipc.ts. The usage in workspaceeditor.tsx correctly awaits the async result.
123-148: Path validation and debouncing are correctly implemented.Null byte validation is implemented in the backend (
pkg/wcore/workspace.go:31-32) with comprehensive test coverage. Directory updates are debounced at the parent component level (WorkspaceSwitcher) using a 300ms debounce. TheWorkspaceEditorcomponent correctly passes directory changes to the parent, where validation and debouncing are handled before backend updates.
129-129: The backend properly expands tilde paths in workspace directories. When the workspace directory is used for shell execution, it passes throughwavebase.ExpandHomeDir()(line 400 of shellcontroller.go) before being assigned to the command options. The placeholder~/projects/myworkspaceis appropriate and will be correctly expanded when the workspace is used.pkg/blockcontroller/shellcontroller.go (2)
417-417: LGTM! Good refactoring to avoid duplicate database calls.Passing workspace info as parameters instead of looking it up inside
makeSwapTokenimproves performance and makes the data flow clearer, as stated in the PR objectives.
739-760: LGTM! Clean implementation of workspace environment variables.The signature change and environment variable handling are well implemented:
- Workspace info is passed as parameters instead of being looked up internally (avoiding duplicate DB calls)
- Environment variables are only set when values are non-empty (safe handling)
- Variable names follow existing conventions (WAVETERM_* prefix)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @frontend/app/tab/workspaceswitcher.tsx:
- Around line 190-193: The code is using shellQuote (POSIX-only) when building
the cd command before sending via RpcApi.ControllerInputCommand; update the
logic around RpcApi.ControllerInputCommand/TabRpcClient so the directory is
quoted according to the terminal shell (not always POSIX). Detect the shell for
the target block (e.g., from viewModel or block settings) and apply a
PowerShell/cmd quoting function for newDirectory when appropriate, falling back
to shellQuote for POSIX shells; ensure the quoted string is then passed to
stringToBase64 and used in the input payload with viewModel.blockId.
- Around line 173-196: The skip logic in updateBlocksWithNewDirectory
incorrectly skips any truthy connection string (including local ones); change
the check that reads the block connection (from
globalStore.get(viewModel.blockAtom).meta.connection) to only skip when the
connection exists and is not local by using the isLocalConnName helper (e.g.,
continue when connection && !isLocalConnName(connection)); ensure
isLocalConnName is imported from "@/util/util" if not already.
In @pkg/blockcontroller/shellcontroller.go:
- Line 417: makeSwapToken currently exports WAVETERM_WORKSPACE_DIR whenever
wsDir != "" which can leak a local absolute path into remote shells like
SSH/WSL; modify the call/site and/or makeSwapToken so that
WAVETERM_WORKSPACE_DIR is only set for truly local shells (check
connUnion.ShellType or equivalent enum/flag) and do not include the env var for
remote shell types (e.g., SSH/WSL); ensure any other call sites that pass wsDir
(see the other occurrences around makeSwapToken) follow the same rule or
sanitize/remove wsDir before creating the swap token.
🧹 Nitpick comments (5)
pkg/blockcontroller/shellcontroller.go (2)
389-399: Guard against empty wsId and missing/non-existent wsDir (avoid noisy logs + potential start failures).
DBFindWorkspaceForTabIdcan return""withnilerror (no row). Today we’ll still callDBGetwith an empty OID and log an error. Also, usingwsDirascmdOpts.Cwdcan fail shell start if the directory doesn’t exist anymore.Proposed fix
wsCtx, wsCancel := context.WithTimeout(context.Background(), 2*time.Second) wsId, wsDir := getWorkspaceInfo(wsCtx, bc.TabId) wsCancel() ... cmdOpts.Cwd = blockMeta.GetString(waveobj.MetaKey_CmdCwd, "") if cmdOpts.Cwd == "" && connUnion.ConnType == ConnType_Local { - cmdOpts.Cwd = wsDir + // Only use wsDir as cwd if it still exists. + if wsDir != "" { + if st, err := os.Stat(wsDir); err == nil && st.IsDir() { + cmdOpts.Cwd = wsDir + } + } }func getWorkspaceInfo(ctx context.Context, tabId string) (wsId string, wsDir string) { if tabId == "" { return "", "" } wsId, err := wstore.DBFindWorkspaceForTabId(ctx, tabId) if err != nil { - log.Printf("error finding workspace for tab %s: %v\n", tabId, err) + log.Printf("error finding workspace for tab %q: %v\n", tabId, err) return "", "" } + if wsId == "" { + return "", "" + } ws, err := wstore.DBGet[*waveobj.Workspace](ctx, wsId) if err != nil { - log.Printf("error getting workspace %s: %v\n", wsId, err) + log.Printf("error getting workspace %q: %v\n", wsId, err) return wsId, "" } if ws == nil { return wsId, "" } return wsId, ws.Directory }
724-744:getWorkspaceInfobehavior is reasonable, but consider returning expanded/normalized directory (or documenting expectations).Right now it returns the stored
Workspace.Directoryverbatim; that’s fine, but note: if this ever accepts~or relative paths,WAVETERM_WORKSPACE_DIRwon’t be directlycd-able via$WAVETERM_WORKSPACE_DIRin POSIX shells (tilde expansion won’t occur after variable expansion). Consider either normalizing here (for local-only usage) or explicitly documenting thatdirectoryshould be absolute.frontend/app/tab/workspaceswitcher.tsx (3)
88-93: “Save workspace” call likely wipes directory unintentionally.
UpdateWorkspace(activeWorkspace.oid, "", "", "", "", true)now passes an explicit empty directory. If the active workspace can ever have a directory set before saving, this will clear it.Proposed fix
- await WorkspaceService.UpdateWorkspace(activeWorkspace.oid, "", "", "", "", true); + await WorkspaceService.UpdateWorkspace( + activeWorkspace.oid, + "", + "", + "", + activeWorkspace.directory ?? "", + true + );
212-223: Debounced updater: consider cancellation on unmount;pendingDirectoryRefappears unused.
- If the popover closes/unmounts while a debounce is pending, you may still send a
cdburst later (maybe fine, but typically you cancel).pendingDirectoryRefis set/cleared but never read in this file.Possible cleanup (depends on throttle-debounce API)
const debouncedBlockUpdate = useMemo( () => debounce(300, (newDirectory: string) => { pendingDirectoryRef.current = null; fireAndForget(async () => { await updateBlocksWithNewDirectory(newDirectory); }); }), [] ); +useEffect(() => { + return () => { + // If supported by throttle-debounce. + (debouncedBlockUpdate as any)?.cancel?.(); + }; +}, [debouncedBlockUpdate]);-const pendingDirectoryRef = useRef<string | null>(null); +// Remove if not used for UI/logic.
225-252: Directory propagation path looks coherent; small optimization: avoid redundant UpdateWorkspace calls.The
setWorkspaceflow correctly triggers a debounced block update only for the active workspace + only on directory change. IfWorkspaceEditorcallsonDirectoryChangeon every keystroke, consider debouncing the UpdateWorkspace call too (not just block updates), unless it’s already debounced insideWorkspaceEditor.Also applies to: 320-326
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/tab/workspaceswitcher.tsxpkg/blockcontroller/shellcontroller.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
📚 Learning: 2024-12-27T22:06:04.948Z
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
Applied to files:
frontend/app/tab/workspaceswitcher.tsx
🧬 Code graph analysis (1)
pkg/blockcontroller/shellcontroller.go (4)
pkg/waveobj/metaconsts.go (1)
MetaKey_CmdCwd(56-56)pkg/wstore/wstore_dbops.go (1)
DBFindWorkspaceForTabId(368-385)pkg/waveobj/wtype.go (2)
Workspace(171-181)Workspace(183-185)pkg/util/shellutil/tokenswap.go (1)
TokenSwapEntry(19-25)
🔇 Additional comments (1)
pkg/blockcontroller/shellcontroller.go (1)
746-747: All call sites have been properly updated.The call site at line 417 includes both new parameters (
wsId,wsDir) in the correct order matching the function signature. No stale references or test file calls remain.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app/tab/workspaceswitcher.tsx (1)
90-90: Consider documenting why empty string is passed for directory.Passing an empty string for
directorywhen saving a workspace is correct per the signature, but a brief comment explaining this represents "no directory specified" or "use default" would improve clarity.📝 Suggested comment addition
await WorkspaceService.UpdateWorkspace(activeWorkspace.oid, "", "", "", "", true); + // Empty strings for name/icon/color/directory means "keep existing values" await updateWorkspaceList();frontend/app/tab/workspaceeditor.tsx (1)
123-148: Consider user-facing error feedback for folder dialog failures.The directory input UI is well-structured with:
- Clear label and placeholder
- Browse button that correctly handles the folder dialog
- Proper null checking (line 137)
However, the error handling only logs to console (line 141). Consider whether users should receive visual feedback when the folder dialog fails, such as a toast notification.
The current implementation is acceptable, but enhanced error handling could improve UX:
💡 Optional enhancement for error feedback
If you have a toast/notification system available, consider:
} catch (e) { console.error("error opening folder dialog:", e); + // Optional: Show user-facing error + // showToast("Failed to open folder dialog", "error"); }
📜 Review details
Configuration used: Repository 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 (17)
emain/emain-ipc.tsemain/preload.tsfrontend/app/store/services.tsfrontend/app/tab/workspaceeditor.scssfrontend/app/tab/workspaceeditor.tsxfrontend/app/tab/workspaceswitcher.tsxfrontend/app/view/preview/preview-model.tsxfrontend/types/custom.d.tsfrontend/types/gotypes.d.tsfrontend/util/util.test.tsfrontend/util/util.tspkg/blockcontroller/shellcontroller.gopkg/service/workspaceservice/workspaceservice.gopkg/waveobj/wtype.gopkg/wconfig/defaultconfig/widgets.jsonpkg/wcore/workspace.gopkg/wcore/workspace_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/util/util.ts
- emain/preload.ts
- frontend/util/util.test.ts
- emain/emain-ipc.ts
- frontend/app/view/preview/preview-model.tsx
- pkg/waveobj/wtype.go
- frontend/types/gotypes.d.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
📚 Learning: 2024-12-27T22:06:04.948Z
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
Applied to files:
pkg/wcore/workspace_test.go
🧬 Code graph analysis (5)
frontend/app/tab/workspaceswitcher.tsx (7)
frontend/app/store/services.ts (1)
WorkspaceService(190-190)pkg/service/workspaceservice/workspaceservice.go (1)
WorkspaceService(23-23)pkg/waveobj/wtype.go (4)
Block(284-292)Block(294-296)Workspace(171-181)Workspace(183-185)frontend/app/store/global.ts (1)
getAllBlockComponentModels(825-825)frontend/app/store/wshclientapi.ts (1)
RpcApi(717-717)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(38-38)frontend/util/util.ts (3)
stringToBase64(550-550)shellQuote(547-547)fireAndForget(532-532)
pkg/wcore/workspace_test.go (1)
pkg/wcore/workspace.go (1)
ValidateWorkspaceDirectory(27-35)
pkg/blockcontroller/shellcontroller.go (6)
pkg/blockcontroller/blockcontroller.go (1)
BlockController_Shell(28-28)pkg/waveobj/metaconsts.go (1)
MetaKey_CmdCwd(56-56)pkg/wstore/wstore_dbops.go (2)
DBFindWorkspaceForTabId(368-385)DBGet(133-136)pkg/waveobj/wtype.go (2)
Workspace(171-181)Workspace(183-185)pkg/waveobj/metamap.go (1)
MetaMapType(8-8)pkg/util/shellutil/tokenswap.go (1)
TokenSwapEntry(19-25)
pkg/service/workspaceservice/workspaceservice.go (3)
frontend/app/store/services.ts (1)
UpdateWorkspace(185-187)pkg/wcore/workspace.go (1)
UpdateWorkspace(90-133)pkg/waveobj/wtype.go (1)
UpdatesRtnType(12-12)
frontend/app/tab/workspaceeditor.tsx (2)
frontend/util/util.ts (1)
fireAndForget(532-532)frontend/app/store/global.ts (1)
getApi(826-826)
🔇 Additional comments (21)
pkg/blockcontroller/shellcontroller.go (4)
389-398: LGTM! Workspace directory integration is well-implemented.The workspace directory lookup and conditional Cwd setting are correctly implemented:
- 2-second timeout is appropriate for the DB lookup
- Only applies workspace directory to Local connections when no explicit Cwd is set
- Properly preserves user-specified Cwd values
724-744: LGTM! Workspace info retrieval is robust.The
getWorkspaceInfofunction properly handles all edge cases:
- Returns empty strings for invalid/missing tab IDs
- Logs errors without failing
- Safely returns partial results if workspace exists but has no directory
746-784: LGTM! Environment variable propagation is correct.The updated
makeSwapTokensignature and implementation properly:
- Exposes workspace context to spawned shells via
WAVETERM_WORKSPACEIDandWAVETERM_WORKSPACE_DIR- Conditionally sets environment variables only when values are available
- Maintains backward compatibility with empty workspace info
417-417: All makeSwapToken call sites have been correctly updated.The search found only one call site at line 417, which correctly uses the new 7-parameter signature with
wsIdandwsDirparameters. No instances of the old signature remain in the codebase.frontend/types/custom.d.ts (1)
137-137: LGTM! Type definition is correct.The
showOpenFolderDialogAPI signature correctly represents a folder selection dialog that can be cancelled:
- Returns
Promise<string | null>wherestringis the selected path andnullindicates cancellation- Follows the established pattern for async Electron IPC methods
frontend/app/tab/workspaceeditor.scss (1)
64-89: LGTM! Styling follows established patterns.The directory selector styles are well-structured:
- Proper visual separation with border-top
- Flexbox layout ensures the input field expands while the browse button maintains its size
- Color and spacing are consistent with the existing workspace editor styles
pkg/wcore/workspace_test.go (1)
10-103: LGTM! Excellent test coverage for directory validation.The test suite comprehensively covers:
- Valid paths: absolute, home directory notation, spaces, special characters, Windows-style, Unicode, emoji
- Invalid paths: null byte injection in various positions (end, middle, embedded, multiple)
- Edge cases: empty directory (valid), various null byte attack vectors
The table-driven test structure is idiomatic and makes it easy to add more test cases in the future.
Based on learnings, consider adding test coverage for workspace operations (creation/update) that validate directory paths at the service layer, if not already covered elsewhere in the test suite.
pkg/wconfig/defaultconfig/widgets.json (1)
18-20: The removal of the hardcoded "file" field is safe and properly handled by the workspace directory feature.The frontend code (preview-model.tsx) dynamically determines the preview file path by checking: (1) if meta.file exists, use it; (2) otherwise, if workspace has a directory, use that; (3) otherwise, default to "~". The backend code in tools.go gracefully handles the missing field by checking if it exists before using it. Preview blocks continue to function correctly with the directory now determined through workspace configuration.
frontend/app/store/services.ts (1)
185-187: LGTM! Signature correctly updated.The
directoryparameter is properly positioned betweencolorandapplyDefaults, matching the backend signature inpkg/service/workspaceservice/workspaceservice.go.pkg/service/workspaceservice/workspaceservice.go (2)
40-44: LGTM! Metadata correctly updated.The
ArgNamesarray properly includes"directory"in the correct position, matching the updated function signature.
46-54: LGTM! Parameter threading is correct.The
directoryparameter is correctly accepted and forwarded towcore.UpdateWorkspace. The existing error handling and update logic remain intact.pkg/wcore/workspace.go (2)
122-128: LGTM! Directory update logic is correct.The implementation properly:
- Only updates when directory changes (line 122)
- Validates before applying changes (line 123)
- Returns early on validation failure (line 124)
- Marks as updated for persistence (line 127)
25-35: No changes needed - the validation design is intentional and well-tested.The function correctly implements a minimal backend validation strategy. The test suite confirms that empty paths, absolute/relative paths, Windows-style paths, Unicode, and special characters are intentionally allowed—only null bytes are blocked for injection prevention. Path existence and other filesystem checks belong elsewhere in the application flow, not in this basic validator.
frontend/app/tab/workspaceswitcher.tsx (4)
144-165: LGTM! Type guards are well-implemented.The interfaces and type guards provide clear type safety for block-aware and preview view models. The guards correctly check for required properties before type narrowing.
214-223: Good use of debouncing to prevent rapid API calls.The 300ms debounce with
pendingDirectoryRefefficiently throttles directory updates during rapid changes, improving both UX and reducing backend load.
225-252: LGTM! Directory change propagation is correctly implemented.The logic properly:
- Detects directory changes (line 230)
- Updates the workspace via
WorkspaceService.UpdateWorkspace(lines 234-241)- Only triggers block updates for the current workspace (line 243)
- Uses debouncing to batch rapid changes (line 245)
The nullish coalescing on line 239 (
newWorkspace.directory ?? "") safely handles undefined directory values.
173-196: The shellQuote implementation is secure and well-tested.The function correctly:
- Skips remote connections to avoid impacting non-local blocks
- Handles preview blocks with
goHistory- Sends
cdcommands to terminal blocks usingshellQuoteThe
shellQuoteimplementation atfrontend/util/util.ts:472-483uses single-quote escaping, a standard Unix security pattern that prevents command injection. Unsafe strings are wrapped in single quotes with embedded single quotes properly escaped via'\'\'. The test suite infrontend/util/util.test.tscomprehensively covers edge cases including explicit command injection scenarios (e.g.,; rm -rf /,$(whoami), backticks), confirming the implementation's safety.frontend/app/tab/workspaceeditor.tsx (4)
17-35: LGTM! ColorSelector refactored to named function.The refactoring to a named memo function improves debugging in React DevTools and removes the unnecessary wrapper handler by calling
onSelectdirectly.
44-64: LGTM! IconSelector refactored to named function.Consistent with
ColorSelector, this refactoring improves debugging and simplifies the click handler.
70-76: LGTM! Props interface correctly extended.The addition of
directoryandonDirectoryChangeproperly extends the component's API to support directory management.
97-101: LGTM! Variable naming improves clarity.Renaming to
fetchedColorsandfetchedIconsbefore setting state makes the data flow clearer.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/tab/workspaceswitcher.tsx (1)
88-94: Directory parameter will unintentionally clear the workspace directory.The
UpdateWorkspacecall passes an empty string fordirectory, which will setws.Directoryto""if the workspace currently has a directory set. This differs from thename,icon, andcolorparameters, which preserve existing values when passed as empty strings. Either pass the current directory value or modify the backend logic to treat empty directory strings as "no change" (consistent with the other parameters).
🤖 Fix all issues with AI agents
In @frontend/app/tab/workspaceswitcher.tsx:
- Around line 173-202: The updateBlocksWithNewDirectory function lacks error
handling and user feedback around RPC calls: wrap the await
RpcApi.GetRTInfoCommand and the RpcApi.ControllerInputCommand call in try-catch
blocks, log or surface errors to the user (use the project’s notification/Toast
utility) and ensure consistent awaiting — either await ControllerInputCommand or
call the existing fireAndForget helper for a deliberate fire-and-forget pattern;
also handle failures from viewModel.goHistory similarly (it is currently
awaited) so callers get reliable success/failure behavior.
🧹 Nitpick comments (1)
frontend/app/tab/workspaceswitcher.tsx (1)
220-229: Consider cleanup of debounced function on unmount.The debounced function should be cancelled when the component unmounts to prevent stale updates and potential memory leaks.
♻️ Proposed cleanup using useEffect
const debouncedBlockUpdate = useMemo( () => debounce(300, (newDirectory: string) => { pendingDirectoryRef.current = null; fireAndForget(async () => { await updateBlocksWithNewDirectory(newDirectory); }); }), [] ); + +useEffect(() => { + return () => { + debouncedBlockUpdate.cancel?.(); + }; +}, [debouncedBlockUpdate]);Note: Verify that the
throttle-debouncelibrary's debounce function provides acancelmethod.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/tab/workspaceswitcher.tsxfrontend/util/util.test.tsfrontend/util/util.tspkg/blockcontroller/shellcontroller.go
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/util/util.test.ts
- frontend/util/util.ts
- pkg/blockcontroller/shellcontroller.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
🔇 Additional comments (4)
frontend/app/tab/workspaceswitcher.tsx (4)
13-15: LGTM: Imports support the new directory feature.The new imports correctly support the workspace directory feature, including RPC APIs for block updates, utilities for shell quoting and connection checks, React hooks for memoization and refs, debouncing for performance, and access to block models and global store.
Also applies to: 17-17, 20-21, 24-24
144-165: LGTM: Well-structured type definitions and guards.The type hierarchy and guards are well-designed. The type guards follow best practices by checking properties before attempting to access them.
231-258: LGTM: Directory change handling is well-implemented.The callback correctly:
- Detects directory changes
- Persists updates to the backend via
UpdateWorkspace- Triggers debounced block updates only for the current workspace
- Uses null coalescing to handle undefined directories
The dependency array is complete and the conditional logic prevents unnecessary updates.
326-326: LGTM: WorkspaceEditor correctly wired for directory support.The directory prop and change handler are properly connected, with appropriate null handling.
Also applies to: 331-331
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @frontend/app/tab/workspaceswitcher.tsx:
- Around line 246-286: The debouncedBlockUpdate created with debounce needs to
be cancelled on unmount to avoid pending callbacks; add a useEffect that returns
a cleanup which calls debouncedBlockUpdate.cancel() (and clears
pendingDirectoryRef.current if you want) to prevent stray invocations;
additionally consider wrapping WorkspaceService.UpdateWorkspace calls inside a
debounced function (or only call it when directoryChanged) instead of calling
immediately on every setWorkspace keystroke to avoid excessive backend updates.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/tab/workspaceswitcher.tsx
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
🧬 Code graph analysis (1)
frontend/app/tab/workspaceswitcher.tsx (6)
frontend/app/store/services.ts (1)
WorkspaceService(190-190)frontend/app/store/global.ts (2)
getAllBlockComponentModels(825-825)pushFlashError(846-846)frontend/util/util.ts (4)
isLocalConnName(596-596)shellQuoteForShellType(608-608)fireAndForget(590-590)stringToBase64(611-611)frontend/app/store/wshclientapi.ts (1)
RpcApi(717-717)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(38-38)frontend/app/store/wos.ts (1)
makeORef(317-317)
🔇 Additional comments (3)
frontend/app/tab/workspaceswitcher.tsx (3)
13-25: Imports look consistent with new directory propagation flow.
No issues spotted with the new dependencies and wiring.
88-94: Double-checkUpdateWorkspace(...)arg order + consider passing the current directory when saving.
IfUpdateWorkspacetreats""as “clear”, this will always clear the directory on “Save workspace”. If it treats""as “no change”, this is fine—just worth confirming given the newdirectoryparameter.
350-361: WorkspaceEditor directory wiring looks correct.
Passingdirectory+onDirectoryChangethrough is straightforward and matches the feature intent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app/tab/workspaceswitcher.tsx (2)
260-269: Consider cleanup for the debounced function to prevent memory leaks.The debounced function is created but never cancelled. If the component unmounts while a debounced call is pending, it may still execute, potentially causing state updates on unmounted components.
♻️ Recommended cleanup implementation
const debouncedBlockUpdate = useMemo( () => debounce(300, (newDirectory: string) => { pendingDirectoryRef.current = null; fireAndForget(async () => { await updateBlocksWithNewDirectory(newDirectory); }); }), [] ); + +useEffect(() => { + return () => { + debouncedBlockUpdate.cancel?.(); + }; +}, [debouncedBlockUpdate]);
278-293: Consider debouncing theUpdateWorkspaceservice calls to reduce API traffic.Currently,
UpdateWorkspaceis called immediately on every directory change (line 280), while only the block updates are debounced. If a user rapidly modifies the directory field (e.g., typing or pasting), this will generate multiple API calls in quick succession.Consider debouncing the
UpdateWorkspacecall as well to batch rapid changes.♻️ Possible debouncing implementation for service calls
Create a separate debounced function for workspace updates:
+const debouncedWorkspaceUpdate = useMemo( + () => + debounce(300, (workspaceOid: string, name: string, icon: string, color: string, directory: string) => { + fireAndForget(async () => { + await WorkspaceService.UpdateWorkspace(workspaceOid, name, icon, color, directory, false); + }); + }), + [] +); + const setWorkspace = useCallback( (newWorkspace: Workspace) => { setWorkspaceEntry((prev) => { const oldDirectory = prev.workspace.directory; const newDirectory = newWorkspace.directory; const directoryChanged = newDirectory !== oldDirectory; if (newWorkspace.name !== "") { - fireAndForget(async () => { - await WorkspaceService.UpdateWorkspace( - prev.workspace.oid, - newWorkspace.name, - newWorkspace.icon, - newWorkspace.color, - newWorkspace.directory ?? "", - false - ); - }); + debouncedWorkspaceUpdate( + prev.workspace.oid, + newWorkspace.name, + newWorkspace.icon, + newWorkspace.color, + newWorkspace.directory ?? "" + ); if (directoryChanged && isCurrentWorkspace && newDirectory) { pendingDirectoryRef.current = newDirectory; debouncedBlockUpdate(newDirectory); } } return { ...prev, workspace: newWorkspace }; }); }, - [debouncedBlockUpdate, isCurrentWorkspace, setWorkspaceEntry] + [debouncedBlockUpdate, debouncedWorkspaceUpdate, isCurrentWorkspace, setWorkspaceEntry] );Note: You'll also need to add cleanup for this new debounced function similar to the previous suggestion.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/tab/workspaceswitcher.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
📚 Learning: 2024-12-27T22:06:04.948Z
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
Applied to files:
frontend/app/tab/workspaceswitcher.tsx
🔇 Additional comments (5)
frontend/app/tab/workspaceswitcher.tsx (5)
13-24: LGTM! Imports align with the new directory propagation functionality.All new imports are used appropriately for RPC communication, shell command construction, debouncing, and block model access.
144-165: LGTM! Type guards provide robust type safety.The interfaces and type guards properly distinguish between preview and terminal blocks, enabling type-safe directory propagation logic.
366-371: LGTM! WorkspaceEditor integration is correct.The directory prop properly defaults to an empty string, and the onDirectoryChange callback correctly propagates updates through the setWorkspace handler.
90-90: LGTM! UpdateWorkspace signature updated consistently.The call correctly includes the new
directoryparameter (as an empty string) before the final boolean flag, aligning with the updated service signature.
173-242: This review concern is not valid. The code is safe as implemented.The
updateBlocksWithNewDirectoryfunction properly handles all shell type cases, including undefined and unknown types. The key detail the original concern missed is thatshellQuoteForShellType()(line 207) quotes the directory path before it's used in the cd command (line 216). Because the directory is already properly quoted via POSIX quoting for unknown shell types, the absence of--in the fallbackcdprefix poses no safety risk.For POSIX shells with quoted paths like
cd '/path-starting-with-dash', the leading dash is not interpreted as an option flag because it's inside quotes. The--option terminator is only necessary for unquoted arguments. The existing code structure withshellQuoteForShellType()handling all quoting ensures paths are safe regardless of whether the shell type is known or not.Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/tab/workspaceswitcher.tsx (1)
88-94: Critical: Empty strings will overwrite workspace metadata on save.The
saveWorkspacefunction passes empty strings for all metadata fields (name, icon, color, directory), which will erase the existing workspace configuration instead of preserving it. This defeats the purpose of the "Save workspace" action.🐛 Proposed fix
const saveWorkspace = () => { fireAndForget(async () => { - await WorkspaceService.UpdateWorkspace(activeWorkspace.oid, "", "", "", "", true); + await WorkspaceService.UpdateWorkspace( + activeWorkspace.oid, + activeWorkspace.name ?? "", + activeWorkspace.icon ?? "", + activeWorkspace.color ?? "", + activeWorkspace.directory ?? "", + true + ); await updateWorkspaceList(); setEditingWorkspace(activeWorkspace.oid); }); };
🧹 Nitpick comments (2)
frontend/app/tab/workspaceswitcher.tsx (2)
199-205: Consider adding expiration to flash error messages.Setting
expiration: nullmeans error messages persist indefinitely in the UI. If multiple blocks fail to navigate, this could clutter the interface.♻️ Suggested improvement
pushFlashError({ id: null, icon: "triangle-exclamation", title: "Directory Change Failed", message: `Could not navigate preview to ${newDirectory}`, - expiration: null, + expiration: Date.now() + 5000, // 5 seconds });Apply the same pattern to the other flash errors at Lines 227-233 and 238-244.
264-275: Consider removing unusedpendingDirectoryRef.The
pendingDirectoryRefis set at Line 311 and cleared at Line 269, but it's never read elsewhere in the component. If it's not needed for tracking state between debounced calls, it can be safely removed to simplify the code.♻️ Simplified version if ref is unnecessary
- const pendingDirectoryRef = useRef<string | null>(null); - const debouncedBlockUpdate = useMemo( () => debounce(300, (newDirectory: string) => { - pendingDirectoryRef.current = null; fireAndForget(async () => { await updateBlocksWithNewDirectory(newDirectory); }); }), [] );And remove Line 291:
useEffect(() => { return () => { debouncedBlockUpdate.cancel(); debouncedWorkspaceUpdate.cancel(); - pendingDirectoryRef.current = null; }; }, [debouncedBlockUpdate, debouncedWorkspaceUpdate]);And remove Line 311:
if (directoryChanged && isCurrentWorkspace && newDirectory) { - pendingDirectoryRef.current = newDirectory; debouncedBlockUpdate(newDirectory); }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/tab/workspaceswitcher.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
📚 Learning: 2024-12-27T22:06:04.948Z
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
Applied to files:
frontend/app/tab/workspaceswitcher.tsx
🧬 Code graph analysis (1)
frontend/app/tab/workspaceswitcher.tsx (8)
frontend/app/store/services.ts (1)
WorkspaceService(190-190)pkg/service/workspaceservice/workspaceservice.go (1)
WorkspaceService(23-23)pkg/waveobj/wtype.go (4)
Block(284-292)Block(294-296)Workspace(171-181)Workspace(183-185)frontend/app/store/global.ts (2)
getAllBlockComponentModels(825-825)pushFlashError(846-846)frontend/util/util.ts (4)
isLocalConnName(596-596)shellQuoteForShellType(608-608)fireAndForget(590-590)stringToBase64(611-611)frontend/app/store/wshclientapi.ts (1)
RpcApi(717-717)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(38-38)frontend/app/store/wos.ts (1)
makeORef(317-317)
🔇 Additional comments (5)
frontend/app/tab/workspaceswitcher.tsx (5)
144-171: LGTM! Type definitions and guards are well-structured.The type definitions and guards correctly identify block-aware and preview view models with appropriate runtime checks. The JSDoc comments provide clear documentation.
295-319: VerifyisCurrentWorkspacedependency inuseCallback.The
isCurrentWorkspacevariable (Line 262) is used inside thesetWorkspacecallback at Line 310, but it's derived fromactiveWorkspace.oid === workspace.oid. Sinceworkspacecomes fromworkspaceEntry.workspacewhich can change, this could cause stale closure issues.Consider one of these approaches:
- Include
workspace.oidin the dependency array- Read
isCurrentWorkspaceinside the callback from the latestworkspaceEntry♻️ Option 2: Calculate isCurrentWorkspace inside callback
const setWorkspace = useCallback( (newWorkspace: Workspace) => { setWorkspaceEntry((prev) => { + const isCurrentWorkspace = activeWorkspace.oid === prev.workspace.oid; const oldDirectory = prev.workspace.directory; const newDirectory = newWorkspace.directory; const directoryChanged = newDirectory !== oldDirectory; if (newWorkspace.name !== "") { debouncedWorkspaceUpdate( prev.workspace.oid, newWorkspace.name, newWorkspace.icon, newWorkspace.color, newWorkspace.directory ?? "" ); if (directoryChanged && isCurrentWorkspace && newDirectory) { pendingDirectoryRef.current = newDirectory; debouncedBlockUpdate(newDirectory); } } return { ...prev, workspace: newWorkspace }; }); }, - [debouncedBlockUpdate, debouncedWorkspaceUpdate, isCurrentWorkspace, setWorkspaceEntry] + [debouncedBlockUpdate, debouncedWorkspaceUpdate, activeWorkspace.oid, setWorkspaceEntry] );
383-395: LGTM! WorkspaceEditor integration is correct.The directory prop and callback are properly wired, with appropriate null handling using the nullish coalescing operator.
214-217: Good defensive practice withcd --for POSIX shells.Using
cd --for bash/zsh/sh/fish prevents directories starting with-from being interpreted as flags. The fallback tocdfor other shell types (e.g., PowerShell, cmd) is appropriate.
207-245: The code at this location is safe and does not require modification. TheshellQuoteForShellTypefunction is explicitly designed to acceptundefinedandnullvalues for theshellTypeparameter. WhenshellTypeis undefined, the function safely defaults to empty string via the nullish coalescing operator (??) and then falls back to POSIX shell quoting (shellQuote). This behavior is validated by comprehensive unit tests, including explicit test cases for null and undefined shell types.Likely an incorrect or invalid review comment.
Allows setting a default directory for workspaces. New shells opened in the workspace will start in this directory. Includes validation, error handling, and debounced updates in the workspace editor.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/blockcontroller/shellcontroller.go (1)
389-405: Good local-only wsDir propagation; add a guard for emptywsId.Nice job keeping
wsDirout of remote shell env (Lines 417-421) while still using it as local default CWD (Lines 396-398). One edge case:getWorkspaceInfoshould likely short-circuit ifwsId == ""afterDBFindWorkspaceForTabIdto avoid aDBGetwith an empty id.Proposed guard
func getWorkspaceInfo(ctx context.Context, tabId string) (wsId string, wsDir string) { if tabId == "" { return "", "" } wsId, err := wstore.DBFindWorkspaceForTabId(ctx, tabId) if err != nil { log.Printf("error finding workspace for tab %s: %v\n", tabId, err) return "", "" } + if wsId == "" { + return "", "" + } ws, err := wstore.DBGet[*waveobj.Workspace](ctx, wsId) if err != nil { log.Printf("error getting workspace %s: %v\n", wsId, err) return wsId, "" }Also applies to: 417-423, 729-749, 769-774
frontend/app/tab/workspaceeditor.tsx (1)
66-77: PreventInputfrom receivingundefinedfordirectory.Since
Workspace.directoryis optional in the generated types, please ensure the prop passed here is always a string (or normalize in-place) to avoid controlled/uncontrolled warnings.Minimal safe normalization
export const WorkspaceEditor = memo(function WorkspaceEditor({ @@ - directory, + directory, @@ }: WorkspaceEditorProps) { @@ return ( <div className="workspace-editor"> @@ <div className="directory-selector"> @@ <Input - value={directory} + value={directory ?? ""} onChange={onDirectoryChange}Also applies to: 123-148
🤖 Fix all issues with AI agents
In @emain/emain-ipc.ts:
- Around line 261-274: The IPC handler for "show-open-folder-dialog" must
validate the sender like other handlers: change the handler signature to include
the event parameter (e.g., async (event) => { ... }), call
getWaveWindowByWebContentsId(event.sender.id) to retrieve and verify the window
(instead of using focusedWaveWindow), return null if it’s not found, and then
call electron.dialog.showOpenDialog with that verified window; ensure you still
check result.canceled and result.filePaths before returning the selected path.
In @frontend/app/view/preview/preview-model.tsx:
- Line 9: The workspace-directory fallback currently only runs when
meta.connection is falsy, causing explicit local connection names like "local"
or "local:*" to be treated as remote; import and use isLocalConnName from
frontend/util/util.ts and change the checks that currently test !meta.connection
to instead test ( !meta.connection || isLocalConnName(meta.connection) ), and
apply this same update to both fallback locations around the workspace.directory
logic (the block handling meta.connection/connName near preview-model.tsx lines
~376–389).
🧹 Nitpick comments (2)
frontend/app/tab/workspaceswitcher.tsx (2)
179-248: Well-designed async directory propagation with comprehensive error handling.The function properly filters local blocks, uses appropriate navigation methods (goHistory for previews, cd command for terminals), and provides user feedback on failures. The fire-and-forget pattern for terminal cd commands is appropriate to avoid blocking.
One minor consideration: For unknown shell types (line 214-217), the code defaults to
"cd "without the--flag. While this is reasonable, it might fail for directories starting with-.🔧 Optional: Add logging for unknown shell types
const cdPrefix = shellType === "bash" || shellType === "zsh" || shellType === "sh" || shellType === "fish" ? "cd -- " : "cd "; +// Log unknown shell types for debugging +if (shellType && !["bash", "zsh", "sh", "fish"].includes(shellType)) { + console.warn(`Unknown shell type "${shellType}", using basic cd command`); +}
264-319: Excellent debouncing implementation with proper cleanup.The dual-debounce pattern (workspace metadata vs. block navigation) is well-designed:
- 300ms debounce prevents excessive API calls and terminal floods
- Cleanup cancels pending updates on unmount
- Directory change detection correctly triggers block updates only for the current workspace
- Dependencies are properly tracked in the useCallback
Minor observation:
pendingDirectoryRefis set but never read. If it's intended for future use or debugging, consider adding a comment.
📜 Review details
Configuration used: Repository 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 (17)
emain/emain-ipc.tsemain/preload.tsfrontend/app/store/services.tsfrontend/app/tab/workspaceeditor.scssfrontend/app/tab/workspaceeditor.tsxfrontend/app/tab/workspaceswitcher.tsxfrontend/app/view/preview/preview-model.tsxfrontend/types/custom.d.tsfrontend/types/gotypes.d.tsfrontend/util/util.test.tsfrontend/util/util.tspkg/blockcontroller/shellcontroller.gopkg/service/workspaceservice/workspaceservice.gopkg/waveobj/wtype.gopkg/wconfig/defaultconfig/widgets.jsonpkg/wcore/workspace.gopkg/wcore/workspace_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/app/tab/workspaceeditor.scss
- emain/preload.ts
- pkg/wcore/workspace.go
- frontend/app/store/services.ts
- frontend/util/util.test.ts
- pkg/waveobj/wtype.go
- pkg/wcore/workspace_test.go
- frontend/types/custom.d.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
📚 Learning: 2024-12-27T22:06:04.948Z
Learnt from: Jalileh
Repo: wavetermdev/waveterm PR: 1623
File: pkg/wcore/workspace.go:0-0
Timestamp: 2024-12-27T22:06:04.948Z
Learning: We should confirm test coverage for "force deletion vs. protected named workspaces" in the workspace tests (e.g., verifying that named, non-empty workspaces cannot be deleted without force).
Applied to files:
frontend/app/tab/workspaceswitcher.tsx
🧬 Code graph analysis (4)
pkg/blockcontroller/shellcontroller.go (6)
pkg/blockcontroller/blockcontroller.go (1)
BlockController_Shell(28-28)pkg/waveobj/metaconsts.go (1)
MetaKey_CmdCwd(56-56)pkg/wstore/wstore_dbops.go (1)
DBFindWorkspaceForTabId(368-385)pkg/waveobj/wtype.go (2)
Workspace(171-181)Workspace(183-185)pkg/waveobj/metamap.go (1)
MetaMapType(8-8)pkg/util/shellutil/tokenswap.go (1)
TokenSwapEntry(19-25)
frontend/app/view/preview/preview-model.tsx (2)
frontend/util/util.ts (1)
isBlank(595-595)frontend/app/store/global.ts (1)
atoms(816-816)
frontend/app/tab/workspaceswitcher.tsx (7)
frontend/app/store/services.ts (1)
WorkspaceService(190-190)pkg/service/workspaceservice/workspaceservice.go (1)
WorkspaceService(23-23)frontend/app/store/global.ts (2)
getAllBlockComponentModels(825-825)pushFlashError(846-846)frontend/util/util.ts (4)
isLocalConnName(596-596)shellQuoteForShellType(608-608)fireAndForget(590-590)stringToBase64(611-611)frontend/app/store/wshclientapi.ts (1)
RpcApi(717-717)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(38-38)frontend/app/store/wos.ts (1)
makeORef(317-317)
emain/emain-ipc.ts (1)
emain/emain-window.ts (1)
focusedWaveWindow(100-100)
🔇 Additional comments (9)
pkg/wconfig/defaultconfig/widgets.json (1)
13-22: Good cleanup; ensure Preview has a fallback whenmeta.fileis absent.Given
defwidget@filesno longer setsmeta.file, the new fallback behavior infrontend/app/view/preview/preview-model.tsxbecomes load-bearing—this looks consistent with the PR intent.frontend/app/tab/workspaceeditor.tsx (1)
17-35: Selector refactor looks clean.Also applies to: 44-64
frontend/types/gotypes.d.ts (1)
1864-1873: Type update aligns with the new feature.frontend/app/tab/workspaceswitcher.tsx (4)
13-21: LGTM!The new imports are well-organized and all support the directory propagation feature being added.
144-171: LGTM!The ViewModel interfaces and type guards are well-designed with clear documentation. The type narrowing pattern properly checks all required properties before casting.
90-90: Correct signature update for saveWorkspace.The empty directory parameter aligns with the new
UpdateWorkspacesignature where all workspaces receive default values on save.
387-392: LGTM!The WorkspaceEditor integration correctly propagates directory state with null-safe defaults.
pkg/service/workspaceservice/workspaceservice.go (2)
32-32: LGTM!Public method documentation is clear, concise, and follows Go conventions. The comments properly describe each method's purpose and behavior.
Also applies to: 47-49, 80-80, 97-97, 122-122, 142-142, 153-153, 158-158
41-71: UpdateWorkspace correctly extended with directory parameter and all callers updated.The signature change properly threads the directory parameter through to
wcore.UpdateWorkspace, the metadata accurately reflects the new parameter order, and the implementation validates the directory viaValidateWorkspaceDirectorybefore applying changes. Both frontend callers in workspaceswitcher.tsx and the backend call in CreateWorkspace have been updated to include the directory argument.
| electron.ipcMain.handle("show-open-folder-dialog", async () => { | ||
| const ww = focusedWaveWindow; | ||
| if (ww == null) { | ||
| return null; | ||
| } | ||
| const result = await electron.dialog.showOpenDialog(ww, { | ||
| title: "Select Workspace Directory", | ||
| properties: ["openDirectory", "createDirectory"], | ||
| }); | ||
| if (result.canceled || result.filePaths.length === 0) { | ||
| return null; | ||
| } | ||
| return result.filePaths[0]; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and examine the exact handler implementation
find . -name "emain-ipc.ts" -type fRepository: wavetermdev/waveterm
Length of output: 84
🏁 Script executed:
# Get an overview of the repository structure
git ls-files | head -30Repository: wavetermdev/waveterm
Length of output: 791
🏁 Script executed:
# Check the actual implementation at lines 261-274
if [ -f "emain/emain-ipc.ts" ]; then
sed -n '255,280p' emain/emain-ipc.ts
fiRepository: wavetermdev/waveterm
Length of output: 904
🏁 Script executed:
# Search for all ipcMain.handle patterns to understand security patterns used in the codebase
rg "ipcMain\.handle" -A 3 -B 1 --max-count 10Repository: wavetermdev/waveterm
Length of output: 977
🏁 Script executed:
# Look for WebContents validation patterns and window management
rg "event\.sender|event\.senderFrame|webContents" -B 2 -A 2 | head -50Repository: wavetermdev/waveterm
Length of output: 3303
🏁 Script executed:
# Search for getWaveWindowByWebContentsId or similar window lookup functions
rg "getWaveWindowByWebContentsId|getWaveTabViewByWebContentsId" -B 2 -A 5Repository: wavetermdev/waveterm
Length of output: 8316
🏁 Script executed:
# Look for focusedWaveWindow definition and how it's managed
rg "focusedWaveWindow" -B 2 -A 2Repository: wavetermdev/waveterm
Length of output: 4512
🏁 Script executed:
# Check what windows/webContents can invoke IPC handlers - look at preload/context bridge setup
fd "preload|contextBridge|ipcRenderer" -type fRepository: wavetermdev/waveterm
Length of output: 234
🏁 Script executed:
# Search for WaveBrowserWindow and related window classes to understand architecture
rg "class.*BrowserWindow|focusedWaveWindow\s*=" -A 3Repository: wavetermdev/waveterm
Length of output: 896
Validate the IPC handler sender before showing the native dialog.
The handler lacks sender validation, which is inconsistent with security patterns used elsewhere in the same file. Multiple other handlers (e.g., "capture-screenshot", "update-window-controls-overlay") validate the sender using getWaveWindowByWebContentsId(event.sender.id). Since webviews exist in this app and may be able to invoke this channel, any untrusted renderer could trigger native file dialogs without proper authorization. Add the event parameter and validate it against an expected window/webContents:
Suggested hardening pattern
- electron.ipcMain.handle("show-open-folder-dialog", async () => {
- const ww = focusedWaveWindow;
+ electron.ipcMain.handle("show-open-folder-dialog", async (event) => {
+ const ww = getWaveWindowByWebContentsId(event.sender.id) ?? focusedWaveWindow;
if (ww == null) {
return null;
}
const result = await electron.dialog.showOpenDialog(ww, {
title: "Select Workspace Directory",
properties: ["openDirectory", "createDirectory"],
});
if (result.canceled || result.filePaths.length === 0) {
return null;
}
return result.filePaths[0];
});📝 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.
| electron.ipcMain.handle("show-open-folder-dialog", async () => { | |
| const ww = focusedWaveWindow; | |
| if (ww == null) { | |
| return null; | |
| } | |
| const result = await electron.dialog.showOpenDialog(ww, { | |
| title: "Select Workspace Directory", | |
| properties: ["openDirectory", "createDirectory"], | |
| }); | |
| if (result.canceled || result.filePaths.length === 0) { | |
| return null; | |
| } | |
| return result.filePaths[0]; | |
| }); | |
| electron.ipcMain.handle("show-open-folder-dialog", async (event) => { | |
| const ww = getWaveWindowByWebContentsId(event.sender.id) ?? focusedWaveWindow; | |
| if (ww == null) { | |
| return null; | |
| } | |
| const result = await electron.dialog.showOpenDialog(ww, { | |
| title: "Select Workspace Directory", | |
| properties: ["openDirectory", "createDirectory"], | |
| }); | |
| if (result.canceled || result.filePaths.length === 0) { | |
| return null; | |
| } | |
| return result.filePaths[0]; | |
| }); |
🤖 Prompt for AI Agents
In @emain/emain-ipc.ts around lines 261 - 274, The IPC handler for
"show-open-folder-dialog" must validate the sender like other handlers: change
the handler signature to include the event parameter (e.g., async (event) => {
... }), call getWaveWindowByWebContentsId(event.sender.id) to retrieve and
verify the window (instead of using focusedWaveWindow), return null if it’s not
found, and then call electron.dialog.showOpenDialog with that verified window;
ensure you still check result.canceled and result.filePaths before returning the
selected path.
| import { RpcApi } from "@/app/store/wshclientapi"; | ||
| import { TabRpcClient } from "@/app/store/wshrpcutil"; | ||
| import { getConnStatusAtom, getOverrideConfigAtom, getSettingsKeyAtom, globalStore, refocusNode } from "@/store/global"; | ||
| import { atoms, getConnStatusAtom, getOverrideConfigAtom, getSettingsKeyAtom, globalStore, refocusNode } from "@/store/global"; |
There was a problem hiding this comment.
Consider treating "local" / "local:*" as “no remote” for workspace-dir fallback.
Right now the workspace directory fallback only runs when meta.connection is falsy (Line 382). If a preview block sets connection: "local" explicitly, it will fall back to "~" instead of workspace.directory. Suggest using isLocalConnName(connName) (from frontend/util/util.ts) for this check.
Proposed tweak
this.metaFilePath = atom<string>((get) => {
const file = get(this.blockAtom)?.meta?.file;
if (!isBlank(file)) {
return file;
}
const connName = get(this.blockAtom)?.meta?.connection;
- if (!connName) {
+ if (isBlank(connName) || isLocalConnName(connName)) {
const workspace = get(atoms.workspace);
if (workspace?.directory) {
return workspace.directory;
}
}
return "~";
});Also applies to: 376-389
🤖 Prompt for AI Agents
In @frontend/app/view/preview/preview-model.tsx at line 9, The
workspace-directory fallback currently only runs when meta.connection is falsy,
causing explicit local connection names like "local" or "local:*" to be treated
as remote; import and use isLocalConnName from frontend/util/util.ts and change
the checks that currently test !meta.connection to instead test (
!meta.connection || isLocalConnName(meta.connection) ), and apply this same
update to both fallback locations around the workspace.directory logic (the
block handling meta.connection/connName near preview-model.tsx lines ~376–389).
2 participants
