feat: Tab base directory with VS Code style redesign #2789
Conversation
## Tab Base Directory Feature - Per-tab base directory context (tab:basedir, tab:basedirlock metadata) - Smart auto-detection from terminal OSC 7 sequences - All terminals and widgets inherit the tab's base directory - Tab context menu for configuration - Stale path detection and recovery after restart ## Security & Validation - Path validation for OSC 7 terminal escape sequences - Backend metadata validation to prevent injection attacks - Optimistic locking for metadata updates (race condition fix) - Preset metadata validation ## VS Code Style Tab Redesign - Tab backgrounds use relative white overlays (active/hover/inactive) - Tab stripe moved from left to top horizontal (manual color only) - Status indicated via text color (running=blue, finished=green, stopped=red) - Breadcrumb bar below tabs showing active tab's base directory - App menu moved from tab bar to breadcrumb bar - Transparent button styling with subtle hover effects ## Other Improvements - Tab color customization via context menu - Reactive tab status icons using Jotai atoms - Reusable preset menu builder - showOpenDialog API for file picker dialogs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
|
WalkthroughAdds optimistic-locking support with two new errors (ErrVersionMismatch, ErrObjectLocked) and two new store APIs (UpdateObjectMetaWithVersion, UpdateObjectMetaIfNotLocked) exposed via RPC and TypeScript bindings. Implements per-tab OSC 7 sanitization, 300ms debounce with cleanup, and lock-aware atomic metadata updates. Introduces tab base-directory validation/handling (local + network retry), tab UI features (base-dir, lock, color, terminal status, breadcrumb, presets menu), extensive metadata validation and schema/presets, path and preset utilities, Electron showOpenDialog IPC + preload API, multiple frontend atoms/hooks/models, and moves several backend metadata validations and update-event dispatches to asynchronous, panic-safe paths. Documentation added (CLAUDE.md). Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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
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 |
Code reviewFound 2 issues:
waveterm/frontend/app/view/term/termwrap.ts Lines 40 to 46 in 5622d24
waveterm/frontend/app/view/term/termwrap.ts Lines 296 to 313 in 5622d24 Lines 349 to 351 in 5622d24 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/view/term/termwrap.ts (1)
187-277: Use the emitting tab’s id for OSC 7 basedir updates.
atoms.activeTabmay not correspond to the tab that emitted the OSC 7 sequence, which can update the wrong tab’s basedir. Passthis.tabIdinto the handler and use it directly.🔧 Proposed fix
-function handleOsc7Command(data: string, blockId: string, loaded: boolean): boolean { +function handleOsc7Command(data: string, blockId: string, tabId: string, loaded: boolean): boolean { @@ - const tabData = globalStore.get(atoms.activeTab); - - // CRITICAL: Null safety check for tabData - if (!tabData?.oid) { - return; // Early return if no tab or no oid - } - - const tabId = tabData.oid; + if (!tabId) { + return; // Early return if no tab id + } const tabORef = WOS.makeORef("tab", tabId);- this.terminal.parser.registerOscHandler(7, (data: string) => { - return handleOsc7Command(data, this.blockId, this.loaded); - }); + this.terminal.parser.registerOscHandler(7, (data: string) => { + return handleOsc7Command(data, this.blockId, this.tabId, this.loaded); + });Also applies to: 615-617
🤖 Fix all issues with AI agents
In `@CHANGES.md`:
- Around line 67-69: Fix the typo in the "### 3. OSC 7 Debouncing" section:
update the second bullet that currently reads "300ms debounce window for rapid
directory cha..." to "300ms debounce window for rapid directory changes" so the
full word "changes" is used; locate the bullet under the "OSC 7 Debouncing"
heading and correct the text.
In `@emain/emain-ipc.ts`:
- Around line 456-541: The allowedProperties computation in the
electron.ipcMain.handle "show-open-dialog" handler can end up without
"openDirectory" (e.g., when only "showHiddenFiles" is passed), so always force
"openDirectory" into the array while optionally preserving "showHiddenFiles" and
"multiSelections"; update the logic around options.properties (used in the
allowedProperties variable and later checks) to build allowedProperties =
Array.from(new Set(["openDirectory", ...options.properties?.filter(p =>
["openDirectory","showHiddenFiles","multiSelections"].includes(p)) || []])) so
"openDirectory" is always present and duplicates are removed, then use that
allowedProperties in the validation and dialog call.
In `@emain/preload.ts`:
- Around line 71-76: The showOpenDialog wrapper's options type is missing
Electron 38.5.0 fields; update the parameter type in the showOpenDialog
declaration so its properties union (the properties?: Array<...>) also includes
"createDirectory" and "promptToCreate", and add the top-level options
buttonLabel?: string and securityScopedBookmarks?: boolean to the options object
passed to ipcRenderer.invoke (keeping the same function name showOpenDialog and
call to ipcRenderer.invoke("show-open-dialog", options)). Ensure type additions
are only for typing (no runtime behavior changes) and maintain existing
filter/flags shape.
In `@frontend/app/store/global.ts`:
- Around line 119-125: The atom activeTabAtom should guard against empty string
IDs before creating an ORef; update the resolver for staticTabId (from
staticTabIdAtom) to treat both null/undefined and empty string ("") as "no
active tab" and return null instead of calling WOS.makeORef/WOS.getObjectValue;
specifically, in the atom getter around staticTabId check add a condition like
staticTabId == null || staticTabId === "" so WOS.makeORef("tab", staticTabId)
and WOS.getObjectValue<Tab>(...) are only called with a non-empty id.
In `@frontend/app/store/tab-basedir-validation-hook.ts`:
- Around line 13-23: In validateActiveTabBasedir, guard against
getTabModelByTabId(tabId) returning null/undefined before accessing
tabModel.lastValidationTimeAtom: if tabModel is falsy, return early (or handle
accordingly) to avoid runtime exceptions; then continue using
globalStore.get(tabModel.lastValidationTimeAtom) and the
DEBOUNCE_INTERVAL_MS-based debounce logic as before.
In `@frontend/app/store/tab-model.ts`:
- Around line 77-87: The setters clearFinishedUnread and setFinishedUnread
currently only flip finishedUnreadAtom via globalStore.set and must also trigger
a recompute/update of terminalStatusAtom so the aggregate "finished" indicator
is recalculated; modify both methods to, after updating finishedUnreadAtom,
invoke the code path that recomputes terminalStatusAtom (e.g., call the existing
terminal status recompute helper or update terminalStatusAtom via
globalStore.set with the recomputed value) so terminalStatusAtom reflects the
new finishedUnread state immediately.
In `@frontend/app/tab/tab.tsx`:
- Around line 76-88: The call to ObjectService.UpdateObjectMeta inside the
useEffect timeout can reject and currently creates an unhandled promise; wrap
that async call with the already-imported fireAndForget helper (or prefix with
void) so rejections are swallowed/handled. Locate the useEffect block that sets
timer for tab status clearing (references active, isDocVisible, tabStatus, id)
and change the timeout callback to call
fireAndForget(ObjectService.UpdateObjectMeta(makeORef("tab", id), {
"tab:termstatus": null })) (or void await pattern) while keeping the existing
clearTimeout cleanup.
In `@frontend/app/view/term/term-model.ts`:
- Around line 460-472: The updateTabTerminalStatus method currently calls
globalStore.get(this.termRef.current.shellIntegrationStatusAtom) which will
throw if shellIntegrationStatusAtom is undefined; change the read to use the
existing readAtom helper to safely read the atom (guard null/undefined termRef
or shellIntegrationStatusAtom), e.g. obtain shellIntegrationStatus via
readAtom(this.termRef?.current?.shellIntegrationStatusAtom) before calling
tabModel.updateBlockTerminalStatus, and keep the existing procStatus handling
and the call to tabModel.updateBlockTerminalStatus(this.blockId, {...}) intact.
In `@frontend/app/workspace/widgets.tsx`:
- Around line 34-76: The code mutates the cached widget configuration by
directly changing widget.blockdef.meta inside handleWidgetSelect; fix by cloning
the block definition at the start (e.g., create a deep copy of widget.blockdef
into blockDef) and perform all validations and meta mutations on that cloned
blockDef, then pass the cloned blockDef to createBlock so fullConfig.widgets
remains unmodified.
In `@frontend/app/workspace/workspace.tsx`:
- Around line 64-77: The menu control uses a non-interactive div (class
"menu-button") which prevents keyboard and screen-reader activation; replace
that div with a semantic <button type="button"> (keeping the
onClick={handleMenuClick}, title/aria-label and inner <i className="fa
fa-ellipsis" />) so it is focusable and activatable via keyboard, and adjust CSS
if necessary to preserve styling; ensure handleMenuClick remains the click
handler and remove any manual key handlers or tabindex hacks in workspace.tsx.
In `@frontend/util/pathutil.ts`:
- Around line 178-195: The isBlockedPath function can be bypassed when
separators differ; normalize path separators before comparing by converting
normalizedPath and each blocked entry (from BLOCKED_PATHS_WINDOWS /
BLOCKED_PATHS_UNIX) to a consistent separator (e.g., replace backslashes with
forward slashes) and then lowercase for comparison; update the loop in
isBlockedPath to compare against normalizedBlocked (and check exact match or
startsWith(normalizedBlocked + "/")) so "C:/Windows" correctly matches
backslash-style blocked entries.
- Around line 210-286: The quickValidatePath() function currently rejects
empty/whitespace and prevents OSC 7 from clearing the basedir; change it to
allow empty (trimmed) paths by removing or altering the initial empty/whitespace
check so it returns valid: true for an empty string (so
sanitizeOsc7Path(rawPath) will accept empty and defer enforcement to backend).
Keep all other checks (length, null bytes, traversal, UNC, Windows rules)
unchanged so non-empty inputs are still validated.
In `@frontend/util/presetutil.ts`:
- Around line 19-123: The allowlist currently treats entries in
PRESET_KEY_ALLOWLISTS (e.g. "bg:*") as exact keys, so validatePresetBeforeApply
and sanitizePreset reject/strip keys like "bg:color"; update both functions to
honor wildcard/prefix entries by treating any allowlist string that ends with
"*" (or ":*") as a prefix matcher: when resolving allowedKeys for a preset type
(used in validatePresetBeforeApply and sanitizePreset), build a structure of
exactAllowed (Set) and allowedPrefixes (string[] derived from entries with
trailing '*'), then change the checks that currently call allowedKeys.has(key)
to instead check exactAllowed.has(key) || allowedPrefixes.some(p =>
key.startsWith(p)), ensuring display: keys handling remains unchanged.
In `@pkg/service/objectservice/objectservice.go`:
- Around line 163-176: UpdateObjectMetaWithVersion (and the sibling RPC at
184-196) skip the metadata validation step and therefore can bypass validation;
before calling wstore.UpdateObjectMetaWithVersion, run the existing metadata
validation routine (use the project's canonical validator—e.g.,
waveobj.ValidateMeta / ValidateMetaMap or add/ call svc.validateMeta) on the
incoming meta and return a validation error if it fails, and include any context
info (uiContext / oref) in the error to mirror other update paths; apply the
same change to the other update RPC so both paths enforce identical metadata
validation.
In `@pkg/util/shellutil/shellintegration/pwsh_wavepwsh.sh`:
- Around line 99-119: The PSReadLine Enter binding wrapper is overwriting and
always calling AcceptLine(), which breaks custom Enter handlers; change how
$existingHandler is captured by using Get-PSReadLineKeyHandler -Chord Enter
without filtering so you preserve the first handler, store it in
$existingHandler, then inside the Set-PSReadLineKeyHandler ScriptBlock invoke
_waveterm_si_send_command -Command $line as before and conditionally call the
preserved handler: if $existingHandler has a ScriptBlock property invoke that
ScriptBlock, elseif it has a Function property invoke that function, and only
call [Microsoft.PowerShell.PSConsoleReadLine]::AcceptLine() as a last-resort
fallback; keep references to Get-PSReadLineKeyHandler, Set-PSReadLineKeyHandler,
$existingHandler, AcceptLine, and _waveterm_si_send_command to locate and modify
the code.
In `@pkg/waveobj/metaconsts.go`:
- Around line 97-101: The MetaKey_BgText constant was added but the TypeScript
type MetaTSType is missing a matching BgText field; update MetaTSType to include
a BgText?: string (or appropriate type) so the new "bg:text" metadata is typed,
or remove MetaKey_BgText if it’s unused; locate the MetaTSType declaration in
wtypemeta.go and add the BgText optional field to keep the Go/TS metadata shape
in sync.
In `@pkg/waveobj/validators.go`:
- Around line 176-228: In checkPathTraversal, the UNC-path validation uses
parts[0..3] which are wrong due to leading empty segments; update the logic to
skip leading empty parts (or directly use parts[2] and parts[3] for server and
share) when building sharePath and comparing prefixes (use
filepath.Clean(absPath) or the cleaned UNC path for the HasPrefix check) so that
sharePath correctly represents "\\server\share" and valid UNC paths are not
falsely flagged as traversal; adjust the code that builds sharePath and the
subsequent HasPrefix comparison accordingly (refer to symbols:
checkPathTraversal, parts, sharePath, filepath.Clean, absPath/absPathClean).
In `@pkg/wstore/wstore.go`:
- Around line 73-80: The DBUpdate call's error is being ignored causing metadata
merge failures to appear successful; update both places where
DBUpdate(tx.Context(), obj) is called (the block using waveobj.GetMeta/SetMeta
and the similar block at lines 113-116) to capture and return the error from
DBUpdate so the outer transaction can abort; ensure the function returns that
error (propagate it up) rather than unconditionally returning nil after
waveobj.SetMeta.
🧹 Nitpick comments (14)
CLAUDE.md (1)
330-342: Add a language to the fenced code block.The ASCII diagram block is missing a language tag (MD040). Consider
textfor clarity.♻️ Suggested change
-``` +```text ┌─────────────────────────────────────────────────────────────┐ │ TAB │ │ tab:basedir = "/home/user/project" │ │ tab:basedirlock = false │ │ │ │ ┌─────────────────┐ ┌─────────────────┐ ┌──────────────┐ │ │ │ Terminal 1 │ │ Terminal 2 │ │ File View │ │ │ │ cmd:cwd = ... │ │ cmd:cwd = ... │ │ file = ... │ │ │ │ (inherits tab) │ │ (inherits tab) │ │ (inherits) │ │ │ └─────────────────┘ └─────────────────┘ └──────────────┘ │ └─────────────────────────────────────────────────────────────┘frontend/app/tab/workspaceswitcher.scss (1)
15-20: Consider whether!importantis necessary.Using
!importanttwice (lines 16 and 20) can make future style overrides difficult and may indicate specificity conflicts elsewhere. If these are needed to override styles from a parent component or library, the current approach is acceptable. Otherwise, consider increasing selector specificity instead.That said, for a scoped component stylesheet like this, the pragmatic use is understandable.
frontend/app/view/waveconfig/waveconfig-model.ts (1)
46-131: Use shared path validation helpers to avoid false positives.The inline checks duplicate logic and
value.includes("..")can reject legitimate paths with..inside a segment. Consider using the shared helpers infrontend/util/pathutil.tsto stay consistent with OSC 7 sanitization and reduce false positives.♻️ Suggested refactor
-import { base64ToString, stringToBase64 } from "@/util/util"; +import { base64ToString, stringToBase64 } from "@/util/util"; +import { containsPathTraversal, hasNullBytes } from "@/util/pathutil"; ... - if (value.includes("\x00")) { + if (hasNullBytes(value)) { return { error: `Invalid characters in "tab:basedir" in "${key}"` }; } // Path traversal check (basic) - if (value.includes("..")) { + if (containsPathTraversal(value)) { return { error: `Path traversal not allowed in "tab:basedir" in "${key}"` }; }frontend/app/tab/tab.scss (1)
7-47: Consider honoring reduced-motion for status animations.
text-pulse(and any future uses ofspin) should be disabled for users who prefer reduced motion.♿ Optional accessibility tweak
+@media (prefers-reduced-motion: reduce) { + .tab.status-running .name, + .tab.status-attention .name { + animation: none; + } +}Also applies to: 253-269
pkg/waveobj/validators.go (1)
296-322: Integer validation may lose precision for large values.When JSON numbers arrive as
float64, converting tointcan lose precision for values larger than 2^53 (JavaScript's safe integer limit). For the current use cases (font sizes, scrollback, etc.), this is unlikely to be an issue, but consider documenting this limitation.Additionally, the validation doesn't check if the float64 has a fractional component (e.g.,
12.5would be accepted and truncated to12).Optional: Add fractional check for stricter validation
func ValidateInt(key string, value interface{}, minVal, maxVal int) error { if value == nil { return nil } // JSON numbers come as float64 f, ok := value.(float64) if !ok { return &ValidationError{ Key: key, Value: value, Message: fmt.Sprintf("must be a number, got %T", value), } } + // Check for fractional component + if f != float64(int(f)) { + return &ValidationError{ + Key: key, + Value: f, + Message: "must be a whole number", + } + } + i := int(f) if i < minVal || i > maxVal { return &ValidationError{ Key: key, Value: i, Message: fmt.Sprintf("must be between %d and %d", minVal, maxVal), } } return nil }frontend/wave.ts (1)
193-196: Consider adding error handling for the dynamic import.While dynamic imports of internal modules are generally reliable, adding error handling would improve resilience and provide better debugging if the module fails to load.
Optional: Add error handling for robustness
// Initialize tab base directory validation - const { initTabBasedirValidation } = await import("@/store/tab-basedir-validation-hook"); - initTabBasedirValidation(); + try { + const { initTabBasedirValidation } = await import("@/store/tab-basedir-validation-hook"); + initTabBasedirValidation(); + } catch (e) { + console.error("Failed to initialize tab basedir validation:", e); + }frontend/app/store/keymodel.ts (1)
347-362: Consider extracting shared validation logic to reduce duplication.This validation pattern (lines 347-362) is nearly identical to the one in
widgets.tsx(lines 49-62). Both:
- Check if basedir is non-empty
- Dynamically import the validator
- Call
validateTabBasedir- Set basedir to null on failure
Consider extracting a shared helper like
getValidatedTabBasedir(tabData)to reduce duplication and ensure consistent behavior.frontend/app/store/tab-basedir-validation-hook.ts (1)
63-76: Consider storing the unsubscribe function for cleanup.
globalStore.sub()returns an unsubscribe function that isn't captured. While this hook is likely initialized once at startup and never torn down, storing the unsubscribe function would be cleaner and enable proper cleanup if the module lifecycle ever changes.Suggested improvement
+let unsubscribe: (() => void) | null = null; + // Initialize tab validation hook export function initTabBasedirValidation(): void { + // Clean up any existing subscription + if (unsubscribe) { + unsubscribe(); + } + // Subscribe to activeTabIdAtom changes - globalStore.sub(activeTabIdAtom, () => { + unsubscribe = globalStore.sub(activeTabIdAtom, () => { const activeTabId = globalStore.get(activeTabIdAtom); if (activeTabId) { fireAndForget(() => validateActiveTabBasedir(activeTabId)); } });frontend/app/store/tab-basedir-validator.ts (6)
32-42: Remove duplicatesleeputility and unusedtotalWindowconfig.
- The
sleepfunction duplicates the one available in@/util/util(per the relevant code snippets). Import it instead.- The
totalWindowproperty inRetryConfigis never used invalidateWithNetworkRetry.♻️ Suggested changes
Update the import at line 6:
-import { fireAndForget } from "@/util/util"; +import { fireAndForget, sleep } from "@/util/util";Remove the local sleep function (lines 39-42) and either use
totalWindowin the retry logic or remove it from the config:interface RetryConfig { maxAttempts: number; timeoutPerAttempt: number; delayBetweenRetries: number; - totalWindow: number; } const defaultRetryConfig: RetryConfig = { maxAttempts: 3, timeoutPerAttempt: 10000, // 10 seconds per attempt delayBetweenRetries: 1000, // 1 second delay between retries - totalWindow: 30000, // Maximum 30 seconds total }; - -// Sleep utility -function sleep(ms: number): Promise<void> { - return new Promise((resolve) => setTimeout(resolve, ms)); -}
73-95: Consider excluding URL schemes from network path detection.The regex
/^[^\/\\]+:\//at line 90 would also matchhttp://,https://,ftp://, etc. While these would ultimately fail validation, treating them as network paths triggers unnecessary retry logic.♻️ Optional: exclude common URL schemes
// NFS paths (common patterns) // - server:/path (NFS) // - /net/server/path (automounter) -if (/^[^\/\\]+:\//.test(path) || path.startsWith("/net/")) { +// Exclude common URL schemes that aren't network file paths +if (!/^(https?|ftp|file):\/\//i.test(path) && /^[^\/\\]+:\//.test(path) || path.startsWith("/net/")) { return true; }
128-168: The catch block (lines 154-163) appears unreachable.
validatePathWithTimeoutcatches all errors internally and always returns aPathValidationResult. The outer try/catch block will never hit the catch clause since the inner function never throws.♻️ Simplify by removing unreachable catch block
async function validateWithNetworkRetry( basedir: string, config: RetryConfig = defaultRetryConfig ): Promise<PathValidationResult> { let lastError: StalePathReason | null = null; for (let attempt = 1; attempt <= config.maxAttempts; attempt++) { - try { - const result = await validatePathWithTimeout(basedir, config.timeoutPerAttempt); + const result = await validatePathWithTimeout(basedir, config.timeoutPerAttempt); - if (result.valid) { - return result; // Success on any attempt - } + if (result.valid) { + return result; // Success on any attempt + } - // Non-network errors fail immediately (no retry) - if (result.reason !== "network_error") { - return result; - } + // Non-network errors fail immediately (no retry) + if (result.reason !== "network_error") { + return result; + } - lastError = result.reason; + lastError = result.reason; - // Don't delay after final attempt - if (attempt < config.maxAttempts) { - await sleep(config.delayBetweenRetries); - } - } catch (error) { - lastError = classifyError(error); - - // Only retry network errors - if (lastError !== "network_error" || attempt === config.maxAttempts) { - return { valid: false, path: basedir, reason: lastError }; - } - - await sleep(config.delayBetweenRetries); + // Don't delay after final attempt + if (attempt < config.maxAttempts) { + await sleep(config.delayBetweenRetries); } } // All retries exhausted return { valid: false, path: basedir, reason: "network_error" }; }
170-189: UnusedtabIdparameter.The
tabIdparameter is declared but never used invalidateTabBasedir. Either remove it or use it for logging/tracing purposes.♻️ Option 1: Remove unused parameter
export async function validateTabBasedir( - tabId: string, basedir: string ): Promise<PathValidationResult> {Note: This would require updating call sites (e.g., in
tab-basedir-validation-hook.ts).♻️ Option 2: Use for logging
export async function validateTabBasedir( tabId: string, basedir: string ): Promise<PathValidationResult> { if (!basedir || basedir.trim() === "") { return { valid: true, path: basedir }; // Empty path is considered valid (no validation needed) } // Detect if this is a network path const isNetwork = isNetworkPath(basedir); + console.log(`[TabBasedir] Validating ${isNetwork ? "network" : "local"} path for tab ${tabId}: ${basedir}`); if (isNetwork) {
243-281: Batch clearing doesn't handle partial failures gracefully.
Promise.allat line 259 will reject if any single update fails, preventing the notification from being shown and potentially leaving some tabs in an inconsistent state (some cleared, some not).♻️ Use Promise.allSettled for resilient batch processing
- try { - await Promise.all(clearPromises); + const results = await Promise.allSettled(clearPromises); + const failures = results.filter((r) => r.status === "rejected"); + if (failures.length > 0) { + console.error(`[TabBasedir] Failed to clear ${failures.length}/${staleTabs.length} stale basedirs`); + } + try { // Push batched notification const { pushNotification } = await import("./global"); pushNotification({ id: "stale-basedir-batch", icon: "triangle-exclamation", type: "warning", - title: `Cleared base directory for ${staleTabs.length} tabs`, - message: "Multiple tabs had stale paths. See logs for details.", + title: `Cleared base directory for ${staleTabs.length - failures.length} tabs`, + message: failures.length > 0 + ? `${failures.length} tab(s) failed to clear. See logs for details.` + : "Multiple tabs had stale paths. See logs for details.", timestamp: new Date().toISOString(), expiration: Date.now() + 15000, // 15 second auto-dismiss persistent: false, });
283-341: Batching logic can delay indefinitely if stale tabs keep arriving.Each new stale tab resets the 5-second timer (lines 321-323). In a pathological scenario where stale paths are detected continuously, the batch will never be processed and
staleTabsarray grows unbounded.♻️ Add a maximum batch age to ensure processing
interface BatchingState { staleTabs: Array<{ tabId: string; path: string; reason: StalePathReason }>; timer: NodeJS.Timeout | null; + batchStartTime: number | null; } const batchingState: BatchingState = { staleTabs: [], timer: null, + batchStartTime: null, }; const BATCHING_WINDOW_MS = 5000; // 5 second window for batching const BATCH_THRESHOLD = 4; // Batch if 4+ tabs have stale paths +const MAX_BATCH_AGE_MS = 30000; // Force processing after 30 seconds export async function validateAndHandleStale(tabId: string): Promise<void> { // ... validation code ... if (!result.valid && result.reason) { // Add to batching queue batchingState.staleTabs.push({ tabId, path: basedir, reason: result.reason }); + + // Track when batch started + if (batchingState.batchStartTime === null) { + batchingState.batchStartTime = Date.now(); + } + + // Check if batch has been pending too long + const batchAge = Date.now() - batchingState.batchStartTime; + const shouldForceProcess = batchAge >= MAX_BATCH_AGE_MS; // Clear existing timer if any if (batchingState.timer) { clearTimeout(batchingState.timer); } + if (shouldForceProcess) { + // Process immediately if batch is too old + const staleTabs = [...batchingState.staleTabs]; + batchingState.staleTabs = []; + batchingState.timer = null; + batchingState.batchStartTime = null; + + if (staleTabs.length >= BATCH_THRESHOLD) { + fireAndForget(() => handleMultipleStaleBasedirs(staleTabs)); + } else { + staleTabs.forEach(({ tabId, path, reason }) => { + fireAndForget(() => handleStaleBasedir(tabId, path, reason)); + }); + } + return; + } // Set timer to process batch batchingState.timer = setTimeout(() => { const staleTabs = [...batchingState.staleTabs]; batchingState.staleTabs = []; batchingState.timer = null; + batchingState.batchStartTime = null; // ... rest of processing ... }, BATCHING_WINDOW_MS); } }
| ### 3. OSC 7 Debouncing | ||
| - 300ms debounce window for rapid directory changes | ||
| - Per-tab debounce timers in a Map |
There was a problem hiding this comment.
Fix typo in OSC 7 debouncing bullet.
"directory cha..." should read "directory changes".
✏️ Proposed fix
-- 300ms debounce window for rapid directory cha...
+- 300ms debounce window for rapid directory changes🧰 Tools
🪛 LanguageTool
[grammar] ~67-~67: Ensure spelling is correct
Context: ... error handling ### 3. OSC 7 Debouncing - 300ms debounce window for rapid directory cha...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@CHANGES.md` around lines 67 - 69, Fix the typo in the "### 3. OSC 7
Debouncing" section: update the second bullet that currently reads "300ms
debounce window for rapid directory cha..." to "300ms debounce window for rapid
directory changes" so the full word "changes" is used; locate the bullet under
the "OSC 7 Debouncing" heading and correct the text.
|
|
||
| electron.ipcMain.handle( | ||
| "show-open-dialog", | ||
| async ( | ||
| event: electron.IpcMainInvokeEvent, | ||
| options: { | ||
| title?: string; | ||
| defaultPath?: string; | ||
| properties?: Array<"openFile" | "openDirectory" | "multiSelections" | "showHiddenFiles">; | ||
| filters?: Array<{ name: string; extensions: string[] }>; | ||
| } | ||
| ): Promise<string[]> => { | ||
| // SECURITY: Restrict to directory selection only for this feature | ||
| const allowedProperties = | ||
| options.properties?.filter((p) => ["openDirectory", "showHiddenFiles"].includes(p)) || | ||
| ["openDirectory"]; | ||
|
|
||
| // SECURITY: Sanitize defaultPath | ||
| let sanitizedDefaultPath = options.defaultPath; | ||
| if (sanitizedDefaultPath) { | ||
| // CRITICAL SECURITY: Block UNC paths on Windows to prevent network attacks | ||
| // UNC paths like \\attacker.com\share can leak credentials or data | ||
| if (process.platform === "win32" && /^[\\/]{2}[^\\/]/.test(sanitizedDefaultPath)) { | ||
| console.warn("show-open-dialog: blocked UNC path in defaultPath:", sanitizedDefaultPath); | ||
| sanitizedDefaultPath = electronApp.getPath("home"); | ||
| } else { | ||
| // Expand home directory shorthand | ||
| if (sanitizedDefaultPath.startsWith("~")) { | ||
| sanitizedDefaultPath = sanitizedDefaultPath.replace(/^~/, electronApp.getPath("home")); | ||
| } | ||
| // Normalize path to resolve any .. components | ||
| sanitizedDefaultPath = path.normalize(sanitizedDefaultPath); | ||
|
|
||
| // Validate the path exists and is accessible | ||
| try { | ||
| await fs.promises.access(sanitizedDefaultPath, fs.constants.R_OK); | ||
| } catch { | ||
| // Fall back to home directory if path doesn't exist or isn't readable | ||
| sanitizedDefaultPath = electronApp.getPath("home"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Get the appropriate parent window | ||
| const ww = getWaveWindowByWebContentsId(event.sender.id); | ||
| const parentWindow = ww ?? electron.BrowserWindow.getFocusedWindow(); | ||
|
|
||
| const result = await electron.dialog.showOpenDialog(parentWindow, { | ||
| title: options.title ?? "Select Directory", | ||
| defaultPath: sanitizedDefaultPath, | ||
| properties: allowedProperties as electron.OpenDialogOptions["properties"], | ||
| filters: options.filters, | ||
| }); | ||
|
|
||
| // Return empty array if canceled | ||
| if (result.canceled || !result.filePaths) { | ||
| return []; | ||
| } | ||
|
|
||
| // SECURITY: Validate returned paths | ||
| const validPaths: string[] = []; | ||
| for (const filePath of result.filePaths) { | ||
| // CRITICAL SECURITY: Block UNC paths in returned values on Windows | ||
| if (process.platform === "win32" && /^[\\/]{2}[^\\/]/.test(filePath)) { | ||
| console.warn("show-open-dialog: blocked UNC path in result:", filePath); | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| const stats = await fs.promises.stat(filePath); | ||
| if (allowedProperties.includes("openDirectory")) { | ||
| if (stats.isDirectory()) { | ||
| validPaths.push(filePath); | ||
| } | ||
| } else { | ||
| validPaths.push(filePath); | ||
| } | ||
| } catch { | ||
| // Skip paths that can't be accessed | ||
| console.warn("show-open-dialog: skipping inaccessible path:", filePath); | ||
| } | ||
| } | ||
|
|
||
| return validPaths; | ||
| } | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron showOpenDialog properties behavior openDirectory openFile default behavior
💡 Result:
Short answer
-
The options.properties array controls what the open dialog allows; supported values include "openFile" and "openDirectory" (plus others like "multiSelections", "showHiddenFiles", platform-specific ones). To allow selecting files you should include "openFile"; to allow selecting folders include "openDirectory". [1]
-
On Windows and Linux an open dialog cannot be both a file selector and a directory selector simultaneously. If you set properties to ['openFile','openDirectory'] on those platforms the dialog will show a directory selector. (macOS behaves differently and can present both behaviors). [1]
-
properties is optional; explicitly set the value(s) you need (e.g., ['openFile'] or ['openDirectory','multiSelections']) to get predictable cross‑platform behavior. [1]
References
[1] Electron dialog.showOpenDialog API documentation.
🌐 Web query:
Electron dialog showOpenDialog empty properties array default behavior behavior
💡 Result:
Short answer: If you omit the properties option (or pass an empty array), showOpenDialog behaves as a normal file-open dialog (i.e. equivalent to allowing file selection/openFile). The docs list properties as optional and describe openFile as the file-selection mode; on Windows/Linux the dialog cannot be both file+directory at once so you must explicitly set openDirectory when you need a folder picker. [1][2]
Sources:
- Electron dialog.showOpenDialog API (properties is optional; openFile/openDirectory listed). [1]
- Note about Windows/Linux behavior for file vs directory selectors. [2]
Always enforce openDirectory in show-open-dialog.
If a caller passes only showHiddenFiles, the filtered allowedProperties becomes ["showHiddenFiles"] (a truthy value, so the || fallback is bypassed). Passing properties without openDirectory or openFile causes Electron to default to file-selection mode on Windows and Linux, defeating the directory-only security constraint. Always include openDirectory, and optionally preserve showHiddenFiles and multiSelections if provided.
🔒 Suggested fix
- const allowedProperties =
- options.properties?.filter((p) => ["openDirectory", "showHiddenFiles"].includes(p)) ||
- ["openDirectory"];
+ const allowedProperties = new Set<"openDirectory" | "showHiddenFiles" | "multiSelections">([
+ "openDirectory",
+ ]);
+ if (options.properties?.includes("showHiddenFiles")) {
+ allowedProperties.add("showHiddenFiles");
+ }
+ if (options.properties?.includes("multiSelections")) {
+ allowedProperties.add("multiSelections");
+ }
...
- const result = await electron.dialog.showOpenDialog(parentWindow, {
+ const properties = Array.from(allowedProperties) as electron.OpenDialogOptions["properties"];
+ const result = await electron.dialog.showOpenDialog(parentWindow, {
title: options.title ?? "Select Directory",
defaultPath: sanitizedDefaultPath,
- properties: allowedProperties as electron.OpenDialogOptions["properties"],
+ properties,
filters: options.filters,
});
...
- if (allowedProperties.includes("openDirectory")) {
+ if (properties.includes("openDirectory")) {
if (stats.isDirectory()) {
validPaths.push(filePath);
}
} else {
validPaths.push(filePath);
}🤖 Prompt for AI Agents
In `@emain/emain-ipc.ts` around lines 456 - 541, The allowedProperties computation
in the electron.ipcMain.handle "show-open-dialog" handler can end up without
"openDirectory" (e.g., when only "showHiddenFiles" is passed), so always force
"openDirectory" into the array while optionally preserving "showHiddenFiles" and
"multiSelections"; update the logic around options.properties (used in the
allowedProperties variable and later checks) to build allowedProperties =
Array.from(new Set(["openDirectory", ...options.properties?.filter(p =>
["openDirectory","showHiddenFiles","multiSelections"].includes(p)) || []])) so
"openDirectory" is always present and duplicates are removed, then use that
allowedProperties in the validation and dialog call.
| showOpenDialog: (options: { | ||
| title?: string; | ||
| defaultPath?: string; | ||
| properties?: Array<"openFile" | "openDirectory" | "multiSelections" | "showHiddenFiles">; | ||
| filters?: Array<{ name: string; extensions: string[] }>; | ||
| }) => ipcRenderer.invoke("show-open-dialog", options), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron 38.5.0 dialog.showOpenDialog options API documentation
💡 Result:
Here are the showOpenDialog options and supported properties (as documented for Electron v38.x). Key options:
- title: string.
- defaultPath: string.
- buttonLabel: string.
- filters: FileFilter[] — [{ name, extensions: [...] }].
- properties: string[] — supported entries:
- openFile
- openDirectory
- multiSelections
- showHiddenFiles
- createDirectory (macOS)
- promptToCreate (Windows)
- securityScopedBookmarks: boolean (macOS, returns bookmarks when enabled).
- Return (async): Promise resolving to an object with canceled (boolean) and filePaths (string[]); may include bookmarks when securityScopedBookmarks is used.
Example:
const { dialog } = require('electron')
dialog.showOpenDialog(win, {
title: 'Choose files',
defaultPath: '/home/user',
filters: [{ name: 'Images', extensions: ['png','jpg'] }],
properties: ['openFile','multiSelections']
}).then(result => { console.log(result.canceled, result.filePaths) })
Sources: Electron API docs (dialog.showOpenDialog options). [1][2]
[1] Electron v38.5.0 release notes. [2] Electron dialog.showOpenDialog API docs.
🏁 Script executed:
# Search for the main process handler to understand how options are used
rg "show-open-dialog" --type ts --type js -A 5 -B 2Repository: wavetermdev/waveterm
Length of output: 3278
🏁 Script executed:
# Check if there are Electron type definitions in the project
fd -e d.ts -e ts | grep -i electron | head -20Repository: wavetermdev/waveterm
Length of output: 87
Add missing Electron 38.5.0 dialog options to showOpenDialog wrapper.
The preload options are incomplete. Electron 38.5.0 supports:
- Properties:
createDirectory(macOS) andpromptToCreate(Windows) are missing from the current enum - Options:
buttonLabel(string) andsecurityScopedBookmarks(boolean, macOS) are not exposed
Update the type definition to include these missing options so all platforms can be fully supported.
🤖 Prompt for AI Agents
In `@emain/preload.ts` around lines 71 - 76, The showOpenDialog wrapper's options
type is missing Electron 38.5.0 fields; update the parameter type in the
showOpenDialog declaration so its properties union (the properties?: Array<...>)
also includes "createDirectory" and "promptToCreate", and add the top-level
options buttonLabel?: string and securityScopedBookmarks?: boolean to the
options object passed to ipcRenderer.invoke (keeping the same function name
showOpenDialog and call to ipcRenderer.invoke("show-open-dialog", options)).
Ensure type additions are only for typing (no runtime behavior changes) and
maintain existing filter/flags shape.
| async function validateActiveTabBasedir(tabId: string): Promise<void> { | ||
| if (!tabId) return; | ||
|
|
||
| const tabModel = getTabModelByTabId(tabId); | ||
| const lastValidationTime = globalStore.get(tabModel.lastValidationTimeAtom); | ||
| const now = Date.now(); | ||
|
|
||
| // Debounce: skip if validated recently | ||
| if (now - lastValidationTime < DEBOUNCE_INTERVAL_MS) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Add null check for tabModel before accessing its atoms.
If getTabModelByTabId(tabId) returns null or undefined (e.g., for a tab that was just closed or doesn't exist yet), accessing tabModel.lastValidationTimeAtom on line 17 will throw.
Proposed fix
async function validateActiveTabBasedir(tabId: string): Promise<void> {
if (!tabId) return;
const tabModel = getTabModelByTabId(tabId);
+ if (!tabModel) return;
+
const lastValidationTime = globalStore.get(tabModel.lastValidationTimeAtom);
const now = Date.now();📝 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.
| async function validateActiveTabBasedir(tabId: string): Promise<void> { | |
| if (!tabId) return; | |
| const tabModel = getTabModelByTabId(tabId); | |
| const lastValidationTime = globalStore.get(tabModel.lastValidationTimeAtom); | |
| const now = Date.now(); | |
| // Debounce: skip if validated recently | |
| if (now - lastValidationTime < DEBOUNCE_INTERVAL_MS) { | |
| return; | |
| } | |
| async function validateActiveTabBasedir(tabId: string): Promise<void> { | |
| if (!tabId) return; | |
| const tabModel = getTabModelByTabId(tabId); | |
| if (!tabModel) return; | |
| const lastValidationTime = globalStore.get(tabModel.lastValidationTimeAtom); | |
| const now = Date.now(); | |
| // Debounce: skip if validated recently | |
| if (now - lastValidationTime < DEBOUNCE_INTERVAL_MS) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@frontend/app/store/tab-basedir-validation-hook.ts` around lines 13 - 23, In
validateActiveTabBasedir, guard against getTabModelByTabId(tabId) returning
null/undefined before accessing tabModel.lastValidationTimeAtom: if tabModel is
falsy, return early (or handle accordingly) to avoid runtime exceptions; then
continue using globalStore.get(tabModel.lastValidationTimeAtom) and the
DEBOUNCE_INTERVAL_MS-based debounce logic as before.
| # Hook into PSReadLine to detect when commands are executed | ||
| # This is called just before a command is accepted and executed | ||
| if (Get-Module PSReadLine) { | ||
| # Save any existing AcceptLine handler | ||
| $existingHandler = (Get-PSReadLineKeyHandler -Chord Enter | Where-Object { $_.Function -eq 'AcceptLine' }) | ||
| # Create a wrapper that sends command notification before executing | ||
| Set-PSReadLineKeyHandler -Chord Enter -ScriptBlock { | ||
| $line = $null | ||
| $cursor = $null | ||
| [Microsoft.PowerShell.PSConsoleReadLine]::GetBufferState([ref]$line, [ref]$cursor) | ||
| # Send command started notification | ||
| if (-not [string]::IsNullOrWhiteSpace($line)) { | ||
| _waveterm_si_send_command -Command $line | ||
| } | ||
| # Call the original AcceptLine function | ||
| [Microsoft.PowerShell.PSConsoleReadLine]::AcceptLine() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "pwsh_wavepwsh.sh" -type fRepository: wavetermdev/waveterm
Length of output: 118
🌐 Web query:
PSReadLine Set-PSReadLineKeyHandler existing handler Function ScriptBlock invocation
💡 Result:
Short answer — difference and how to invoke an existing handler:
-Set-PSReadLineKeyHandler -Function
- Binds the named, built‑in PSReadLine key handler (e.g. BackwardChar, HistorySearchBackward). This is the preferred way to rebind PSReadLine functions because Get-PSReadLineKeyHandler will clearly show which PSReadLine function is bound. [1][2]
-Set-PSReadLineKeyHandler -ScriptBlock { ... }
- Binds an arbitrary ScriptBlock that PSReadLine will call when the chord is pressed. The ScriptBlock is invoked with two parameters: $key (a System.ConsoleKeyInfo for the triggering key) and $arg (an optional argument such as a digit-argument). You can do anything in that ScriptBlock (inspect/modify the buffer, call PSReadLine APIs, run commands, etc.). [2][3]
Invoking an existing handler from a ScriptBlock
- If you want the ScriptBlock to call a PSReadLine built-in handler, do it explicitly inside the block (for example call the underlying PSReadLine/PSConsoleReadLine API methods or call the handler function). The ScriptBlock gives you full control; using -Function is simpler when you only want to bind an existing handler. Documentation notes you can achieve the same effect with a ScriptBlock by calling the method directly. [1][2][3]
References
[1] Set-PSReadLineKeyHandler (Microsoft docs).
[2] PSReadLine about help / custom key bindings (PSReadLine package help).
[3] Historical examples / blog showing ScriptBlock usage and parameters.
🏁 Script executed:
cat -n ./pkg/util/shellutil/shellintegration/pwsh_wavepwsh.sh | sed -n '90,125p'Repository: wavetermdev/waveterm
Length of output: 1469
Preserve any existing PSReadLine Enter binding.
The current code at line 103 filters handlers using $_.Function -eq 'AcceptLine', which means custom Enter bindings (like ValidateAndAcceptLine or user-defined ScriptBlocks) are not captured. Line 106 then unconditionally overwrites the Enter binding with a new ScriptBlock, and line 117 always calls AcceptLine() directly without invoking the preserved handler. This breaks user customizations.
Capture the first Enter handler without filtering, then invoke it conditionally within the ScriptBlock (checking for both ScriptBlock and Function properties), falling back to AcceptLine() only if no handler exists.
🔧 Proposed fix (preserve existing Enter handler)
- # Save any existing AcceptLine handler
- $existingHandler = (Get-PSReadLineKeyHandler -Chord Enter | Where-Object { $_.Function -eq 'AcceptLine' })
+ # Save any existing Enter handler (function or scriptblock)
+ $existingHandler = Get-PSReadLineKeyHandler -Chord Enter | Select-Object -First 1
@@
- # Call the original AcceptLine function
- [Microsoft.PowerShell.PSConsoleReadLine]::AcceptLine()
+ # Call the original Enter handler if it exists
+ if ($existingHandler -and $existingHandler.ScriptBlock) {
+ & $existingHandler.ScriptBlock
+ } elseif ($existingHandler -and $existingHandler.Function) {
+ [Microsoft.PowerShell.PSConsoleReadLine]::$($existingHandler.Function)()
+ } else {
+ [Microsoft.PowerShell.PSConsoleReadLine]::AcceptLine()
+ }📝 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.
| # Hook into PSReadLine to detect when commands are executed | |
| # This is called just before a command is accepted and executed | |
| if (Get-Module PSReadLine) { | |
| # Save any existing AcceptLine handler | |
| $existingHandler = (Get-PSReadLineKeyHandler -Chord Enter | Where-Object { $_.Function -eq 'AcceptLine' }) | |
| # Create a wrapper that sends command notification before executing | |
| Set-PSReadLineKeyHandler -Chord Enter -ScriptBlock { | |
| $line = $null | |
| $cursor = $null | |
| [Microsoft.PowerShell.PSConsoleReadLine]::GetBufferState([ref]$line, [ref]$cursor) | |
| # Send command started notification | |
| if (-not [string]::IsNullOrWhiteSpace($line)) { | |
| _waveterm_si_send_command -Command $line | |
| } | |
| # Call the original AcceptLine function | |
| [Microsoft.PowerShell.PSConsoleReadLine]::AcceptLine() | |
| } | |
| } | |
| # Hook into PSReadLine to detect when commands are executed | |
| # This is called just before a command is accepted and executed | |
| if (Get-Module PSReadLine) { | |
| # Save any existing Enter handler (function or scriptblock) | |
| $existingHandler = Get-PSReadLineKeyHandler -Chord Enter | Select-Object -First 1 | |
| # Create a wrapper that sends command notification before executing | |
| Set-PSReadLineKeyHandler -Chord Enter -ScriptBlock { | |
| $line = $null | |
| $cursor = $null | |
| [Microsoft.PowerShell.PSConsoleReadLine]::GetBufferState([ref]$line, [ref]$cursor) | |
| # Send command started notification | |
| if (-not [string]::IsNullOrWhiteSpace($line)) { | |
| _waveterm_si_send_command -Command $line | |
| } | |
| # Call the original Enter handler if it exists | |
| if ($existingHandler -and $existingHandler.ScriptBlock) { | |
| & $existingHandler.ScriptBlock | |
| } elseif ($existingHandler -and $existingHandler.Function) { | |
| [Microsoft.PowerShell.PSConsoleReadLine]::$($existingHandler.Function)() | |
| } else { | |
| [Microsoft.PowerShell.PSConsoleReadLine]::AcceptLine() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/util/shellutil/shellintegration/pwsh_wavepwsh.sh` around lines 99 - 119,
The PSReadLine Enter binding wrapper is overwriting and always calling
AcceptLine(), which breaks custom Enter handlers; change how $existingHandler is
captured by using Get-PSReadLineKeyHandler -Chord Enter without filtering so you
preserve the first handler, store it in $existingHandler, then inside the
Set-PSReadLineKeyHandler ScriptBlock invoke _waveterm_si_send_command -Command
$line as before and conditionally call the preserved handler: if
$existingHandler has a ScriptBlock property invoke that ScriptBlock, elseif it
has a Function property invoke that function, and only call
[Microsoft.PowerShell.PSConsoleReadLine]::AcceptLine() as a last-resort
fallback; keep references to Get-PSReadLineKeyHandler, Set-PSReadLineKeyHandler,
$existingHandler, AcceptLine, and _waveterm_si_send_command to locate and modify
the code.
1. OSC 7 debounce cleanup: Call cleanupOsc7DebounceForTab() when tabs are closed to prevent memory leaks from orphaned timers. 2. Smart auto-detection logic: Add missing basedir value check per CLAUDE.md spec. OSC 7 now only updates tab:basedir when it's empty or equals "~", respecting user-set directories. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The previous fix only addressed simpleCloseStaticTab() in keymodel.ts, but missed handleCloseTab() in tabbar.tsx which handles the UI close button. This ensures cleanup for all tab close paths. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code review - Follow-upAll previously identified issues have been addressed in commits 4421ab3 and fb2ab9e:
No issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/view/term/termwrap.ts`:
- Around line 269-277: The code currently reads the active tab via
globalStore.get(atoms.activeTab) (tabData) to compute tabId and update
tab:basedir, which can mis-target updates when a background tab emits OSC 7 or
the user switches tabs; change the logic to use the emitting tab's id provided
by TermWrap (or resolve the id from the emitting block) instead of
atoms.activeTab: locate where TermWrap provides the emitter id (e.g., this.tabId
or block.tabId) and use that as tabId, keep the null-safety check on tabData
only as a fallback, and ensure WOS.makeORef("tab", tabId) and subsequent
tab:basedir updates consistently use the emitting tab id rather than
atoms.activeTab.
🧹 Nitpick comments (1)
frontend/app/store/keymodel.ts (1)
320-385: Defer tab‑basedir validation until it’s actually needed.
Validation currently runs even when a focused terminal already providescmd:cwd, adding async overhead to every new block. Consider moving validation to the branch that appliestab:basedir.♻️ Suggested refactor
- // Pre-use validation: quickly validate tab basedir before using it - if (tabBaseDir && tabBaseDir.trim() !== "") { - try { - const { validateTabBasedir } = await import("@/store/tab-basedir-validator"); - const validationResult = await validateTabBasedir(tabData.oid, tabBaseDir); - if (!validationResult.valid) { - console.warn( - `[keymodel] Tab basedir validation failed at use-time: ${tabBaseDir} (${validationResult.reason}). Falling back to home directory.` - ); - tabBaseDir = null; // Fall back to home directory - } - } catch (error) { - console.error("[keymodel] Failed to validate tab basedir:", error); - tabBaseDir = null; // Fall back to home directory on error - } - } @@ - if (termBlockDef.meta["cmd:cwd"] == null && tabBaseDir != null) { - termBlockDef.meta["cmd:cwd"] = tabBaseDir; - } + if (termBlockDef.meta["cmd:cwd"] == null && tabBaseDir && tabBaseDir.trim() !== "") { + try { + const { validateTabBasedir } = await import("@/store/tab-basedir-validator"); + const validationResult = await validateTabBasedir(tabData?.oid, tabBaseDir); + if (validationResult.valid) { + termBlockDef.meta["cmd:cwd"] = tabBaseDir; + } else { + console.warn( + `[keymodel] Tab basedir validation failed at use-time: ${tabBaseDir} (${validationResult.reason}). Falling back to home directory.` + ); + } + } catch (error) { + console.error("[keymodel] Failed to validate tab basedir:", error); + } + }
- tab-model.ts: Call recomputeTerminalStatus() when finishedUnread changes - objectservice.go: Add metadata validation to UpdateObjectMetaWithVersion and UpdateObjectMetaIfNotLocked - wstore.go: Properly handle DBUpdate errors in optimistic locking functions - Various improvements to WPS broadcast handling and terminal status tracking Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@frontend/app/store/tab-model.ts`:
- Around line 129-171: updateBlockTerminalStatus is only clearing finishedUnread
when shellIntegrationStatus goes from not-"running-command" to
"running-command", so background tabs that only change shellProcStatus (e.g.,
"done" -> "running") keep finishedUnread set; update the commandJustStarted
detection to also consider a proc-status start transition (e.g.,
prevStatus?.shellProcStatus === "done" && status.shellProcStatus === "running"
or more generally prev !== "running" && status === "running"), and use that
combined condition where you currently check commandJustStarted before calling
globalStore.set(this.finishedUnreadAtom, false); reference
updateBlockTerminalStatus, previousStatusMap, shellIntegrationStatus,
shellProcStatus, and finishedUnreadAtom.
In `@frontend/app/view/term/term.tsx`:
- Around line 342-354: The wasFocusedRef isn't updated when the effect
early-returns, causing missed focus transitions; update wasFocusedRef.current
unconditionally so it always reflects the latest isFocused. In the useEffect
that depends on [isFocused, model], set wasFocusedRef.current = isFocused
outside the early-return path (e.g., at the start or in a finally/after block)
while keeping the existing logic that calls model.updateTabTerminalStatus() when
isFocused transitions from false to true and clears the timeout on cleanup.
In `@pkg/service/objectservice/objectservice.go`:
- Around line 91-94: The deferred call currently passes recover() at goroutine
spawn time so panics inside the new goroutine won't be caught; wrap the
PanicHandler call inside a deferred anonymous function so recover() is invoked
inside the goroutine's deferred context (e.g., replace defer
panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents",
recover()) with defer func(){
panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents",
recover()) }()), and apply the same change to all other goroutine blocks in this
file (the patterns around SendUpdateEvents and similar anonymous goroutines at
the other noted locations).
- Around line 271-278: The deferred call currently passes recover() directly to
panichandler.PanicHandler and does an unsafe type assertion on waveObj; change
the defer to capture the panic inside the goroutine (defer func(){ if r :=
recover(); r != nil {
panichandler.PanicHandler("ObjectService:UpdateObject:SendUpdateEvents", r) }
}()) and replace the direct assertion waveObj.(*waveobj.Workspace) with a safe
check: if waveObj.GetOType() == waveobj.OType_Workspace { if w, ok :=
waveObj.(*waveobj.Workspace); ok && w.Name != "" {
wps.Broker.Publish(wps.WaveEvent{Event: wps.Event_WorkspaceUpdate}) } } before
calling wps.Broker.SendUpdateEvents(updates).
In `@pkg/waveobj/validators.go`:
- Around line 296-348: ValidateInt and ValidateNullableInt currently cast
float64 to int which can truncate fractional values and overflow; instead first
assert the number is an integer (e.g. math.Trunc(f) == f or use math.Modf to
ensure no fractional part) and verify the float64 value falls within
float64(minVal) and float64(maxVal) bounds before converting to int; return a
ValidationError mentioning "must be an integer" (or "must be an integer or null"
for ValidateNullableInt) when fractional, and keep the existing range error if
out of bounds, then safely cast to int only after these checks.
♻️ Duplicate comments (4)
pkg/service/objectservice/objectservice.go (1)
162-165: Previous review finding addressed: metadata validation now implemented.Validation is correctly added before persistence in all update paths (
UpdateObjectMeta,UpdateObjectMetaWithVersion,UpdateObjectMetaIfNotLocked, andUpdateObject).pkg/waveobj/validators.go (1)
205-214: Fix UNC share prefix handling to avoid rejecting valid paths.The UNC logic builds
sharePathfrom the first four split elements, but UNC paths split to["", "", "server", "share", ...]. This causes valid UNC paths to fail the prefix check and be flagged as traversal.🔧 Proposed fix
- parts := strings.Split(filepath.Clean(path), string(filepath.Separator)) - if len(parts) >= 4 { // \\server\share\... - sharePath := filepath.Join(parts[0], parts[1], parts[2], parts[3]) - if !strings.HasPrefix(filepath.Clean(path), sharePath) { + uncPath := filepath.Clean(path) + parts := strings.Split(uncPath, string(filepath.Separator)) + if len(parts) >= 4 && parts[0] == "" && parts[1] == "" { // \\server\share\... + sharePath := string(filepath.Separator) + string(filepath.Separator) + filepath.Join(parts[2], parts[3]) + if !strings.HasPrefix(uncPath, sharePath) { return fmt.Errorf("path traversal detected in UNC path") } }frontend/app/view/term/term-model.ts (1)
474-476: UsereadAtomhelper for null-safe atom access.While the ternary guards against
termRef?.currentbeing null, ifshellIntegrationStatusAtomis somehow undefined,globalStore.get()could throw. ThereadAtomhelper (already imported) handles this safely.♻️ Suggested fix
// First, try to get shell integration status from local atom (for active tabs) // This is the most up-to-date value since it's set synchronously on OSC 16162 - const localShellIntegrationStatus = this.termRef?.current - ? globalStore.get(this.termRef.current.shellIntegrationStatusAtom) - : null; + const localShellIntegrationStatus = readAtom(this.termRef?.current?.shellIntegrationStatusAtom);frontend/app/store/global.ts (1)
119-125: Guard against empty string tab IDs before resolving ORef.The null check on line 121 doesn't handle the case where
staticTabIdis an empty string"". This could create an invalid ORef"tab:"and cause issues downstream.🛠️ Suggested fix
const activeTabAtom: Atom<Tab> = atom((get) => { const staticTabId = get(staticTabIdAtom); - if (staticTabId == null) { + if (isBlank(staticTabId)) { return null; } return WOS.getObjectValue<Tab>(WOS.makeORef("tab", staticTabId), get); });
🧹 Nitpick comments (5)
Taskfile.yml (1)
11-14:DATEvariable appears to be unused dead code.The
DATEvariable at line 12 is defined but no longer used anywhere in the file—BUILD_TIMEhas replaced it for embedding timestamps in ldflags. Additionally, the Windows branch ofDATEis incomplete (missing format specifier after-UFormat).Consider removing the unused
DATEvariable to reduce confusion.♻️ Suggested cleanup
vars: APP_NAME: "Wave" BIN_DIR: "bin" VERSION: sh: node version.cjs RMRF: '{{if eq OS "windows"}}pwsh -NoProfile -Command Remove-Item -Force -Recurse -ErrorAction SilentlyContinue{{else}}rm -rf{{end}}' - DATE: '{{if eq OS "windows"}}pwsh -NoProfile -Command "Get-Date -UFormat"{{else}}date{{end}}' BUILD_TIME: sh: '{{if eq OS "windows"}}pwsh -NoProfile -Command "Get-Date -UFormat +%Y%m%d%H%M"{{else}}date +%Y%m%d%H%M{{end}}'pkg/wstore/wstore.go (1)
86-110: Docstring slightly inconsistent with error behavior.The docstring states "Returns ErrObjectLocked if locked" but the actual return is
ErrVersionMismatchwrappingErrObjectLocked. Whileerrors.Is(err, ErrObjectLocked)will still work due to Go's error unwrapping, the primary error type isErrVersionMismatch.Consider updating the docstring for clarity:
// UpdateObjectMetaIfNotLocked atomically checks lock and updates. -// Returns ErrObjectLocked if locked, or ErrVersionMismatch if version doesn't match. +// Returns ErrVersionMismatch (wrapping ErrObjectLocked) if locked, or ErrVersionMismatch if version doesn't match. // This eliminates the TOCTOU vulnerability in lock checking.frontend/app/view/term/term-model.ts (1)
488-510: Consider logging RTInfo query failures at debug level.The catch block silently handles errors but also logs nothing when RTInfo fails. For debugging tab status issues, a
dlogcall would help trace failures without cluttering the console.♻️ Suggested improvement
.catch((err) => { + dlog("RTInfo query failed for terminal status:", err); // If RTInfo query fails, update with null shell integration status this.tabModel.updateBlockTerminalStatus(this.blockId, {frontend/app/view/term/termwrap.ts (1)
272-338: Avoid dynamic import inside the debounce callback.The dynamic import on line 323 introduces unnecessary overhead on every OSC 7 update. Since
getTabModelByTabIdis a simple utility, consider importing it statically at the top of the file.♻️ Suggested fix
Add to imports at top of file:
import { getTabModelByTabId } from "@/app/store/tab-model";Then simplify the callback:
// Update validation state to "valid" after successful OSC 7 update - const { getTabModelByTabId } = await import("@/store/tab-model"); const tabModel = getTabModelByTabId(tabId); globalStore.set(tabModel.basedirValidationAtom, "valid"); globalStore.set(tabModel.lastValidationTimeAtom, Date.now());frontend/app/store/tab-model.ts (1)
193-229: Avoid redundant metadata writes when status is unchanged.
UpdateObjectMetaruns on every recompute, even if the computed status is identical. Guarding the persist call reduces RPC noise and avoids unnecessary churn.♻️ Suggested refactor
- // Update local atom for immediate reactivity within this webview - globalStore.set(this.terminalStatusAtom, newStatus); - - // Persist to tab metadata for cross-webview sync - // This allows other tab webviews to see the status in the tabbar - ObjectService.UpdateObjectMeta(WOS.makeORef("tab", this.tabId), { - "tab:termstatus": newStatus, - }).catch((err) => { - console.error("Failed to persist tab terminal status:", err); - }); + const prevStatus = globalStore.get(this.terminalStatusAtom); + if (prevStatus === newStatus) { + return; + } + + // Update local atom for immediate reactivity within this webview + globalStore.set(this.terminalStatusAtom, newStatus); + + // Persist to tab metadata for cross-webview sync + // This allows other tab webviews to see the status in the tabbar + ObjectService.UpdateObjectMeta(WOS.makeORef("tab", this.tabId), { + "tab:termstatus": newStatus, + }).catch((err) => { + console.error("Failed to persist tab terminal status:", err); + });
| updateBlockTerminalStatus(blockId: string, status: BlockTerminalStatus): void { | ||
| const prevStatus = this.previousStatusMap.get(blockId); | ||
|
|
||
| // Detect command completion via shell integration | ||
| const shellIntegrationFinished = | ||
| prevStatus?.shellIntegrationStatus === "running-command" && | ||
| status.shellIntegrationStatus !== "running-command"; | ||
|
|
||
| // Detect command completion via proc status (fallback for background tabs) | ||
| // Background tabs don't process OSC 16162, so we also check shellProcStatus | ||
| const procStatusFinished = | ||
| prevStatus?.shellProcStatus === "running" && | ||
| status.shellProcStatus === "done"; | ||
|
|
||
| const commandJustFinished = shellIntegrationFinished || procStatusFinished; | ||
|
|
||
| // Detect when a new command starts (to clear old "finished" state) | ||
| const commandJustStarted = | ||
| prevStatus?.shellIntegrationStatus !== "running-command" && | ||
| status.shellIntegrationStatus === "running-command"; | ||
|
|
||
| // Clear "finished" state when a new command starts | ||
| if (commandJustStarted && globalStore.get(this.finishedUnreadAtom)) { | ||
| globalStore.set(this.finishedUnreadAtom, false); | ||
| } | ||
|
|
||
| // Detect error exit | ||
| const hasError = | ||
| status.shellProcStatus === "done" && | ||
| status.shellProcExitCode != null && | ||
| status.shellProcExitCode !== 0; | ||
|
|
||
| // Show "finished" status for successful completions | ||
| // "stopped" status is handled by recomputeTerminalStatus based on exit code | ||
| if (commandJustFinished && !hasError) { | ||
| this.setFinishedUnread(); | ||
| // Status will be cleared by tab.tsx when user views it (2-3 second delay) | ||
| } | ||
|
|
||
| // Store current status for next comparison | ||
| this.previousStatusMap.set(blockId, { ...status }); | ||
| this.terminalStatusMap.set(blockId, status); | ||
| this.recomputeTerminalStatus(); |
There was a problem hiding this comment.
Clear “finished” on proc-start for background tabs.
If background tabs don’t emit shellIntegrationStatus updates, a new command can start (proc done → running) while the tab still shows “finished.” Consider also using shellProcStatus transitions to clear finishedUnread.
🔧 Suggested adjustment
- const commandJustStarted =
- prevStatus?.shellIntegrationStatus !== "running-command" &&
- status.shellIntegrationStatus === "running-command";
+ const shellIntegrationStarted =
+ prevStatus?.shellIntegrationStatus !== "running-command" &&
+ status.shellIntegrationStatus === "running-command";
+ const procStatusStarted =
+ prevStatus?.shellProcStatus !== "running" &&
+ status.shellProcStatus === "running";
+ const commandJustStarted = shellIntegrationStarted || procStatusStarted;📝 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.
| updateBlockTerminalStatus(blockId: string, status: BlockTerminalStatus): void { | |
| const prevStatus = this.previousStatusMap.get(blockId); | |
| // Detect command completion via shell integration | |
| const shellIntegrationFinished = | |
| prevStatus?.shellIntegrationStatus === "running-command" && | |
| status.shellIntegrationStatus !== "running-command"; | |
| // Detect command completion via proc status (fallback for background tabs) | |
| // Background tabs don't process OSC 16162, so we also check shellProcStatus | |
| const procStatusFinished = | |
| prevStatus?.shellProcStatus === "running" && | |
| status.shellProcStatus === "done"; | |
| const commandJustFinished = shellIntegrationFinished || procStatusFinished; | |
| // Detect when a new command starts (to clear old "finished" state) | |
| const commandJustStarted = | |
| prevStatus?.shellIntegrationStatus !== "running-command" && | |
| status.shellIntegrationStatus === "running-command"; | |
| // Clear "finished" state when a new command starts | |
| if (commandJustStarted && globalStore.get(this.finishedUnreadAtom)) { | |
| globalStore.set(this.finishedUnreadAtom, false); | |
| } | |
| // Detect error exit | |
| const hasError = | |
| status.shellProcStatus === "done" && | |
| status.shellProcExitCode != null && | |
| status.shellProcExitCode !== 0; | |
| // Show "finished" status for successful completions | |
| // "stopped" status is handled by recomputeTerminalStatus based on exit code | |
| if (commandJustFinished && !hasError) { | |
| this.setFinishedUnread(); | |
| // Status will be cleared by tab.tsx when user views it (2-3 second delay) | |
| } | |
| // Store current status for next comparison | |
| this.previousStatusMap.set(blockId, { ...status }); | |
| this.terminalStatusMap.set(blockId, status); | |
| this.recomputeTerminalStatus(); | |
| updateBlockTerminalStatus(blockId: string, status: BlockTerminalStatus): void { | |
| const prevStatus = this.previousStatusMap.get(blockId); | |
| // Detect command completion via shell integration | |
| const shellIntegrationFinished = | |
| prevStatus?.shellIntegrationStatus === "running-command" && | |
| status.shellIntegrationStatus !== "running-command"; | |
| // Detect command completion via proc status (fallback for background tabs) | |
| // Background tabs don't process OSC 16162, so we also check shellProcStatus | |
| const procStatusFinished = | |
| prevStatus?.shellProcStatus === "running" && | |
| status.shellProcStatus === "done"; | |
| const commandJustFinished = shellIntegrationFinished || procStatusFinished; | |
| // Detect when a new command starts (to clear old "finished" state) | |
| const shellIntegrationStarted = | |
| prevStatus?.shellIntegrationStatus !== "running-command" && | |
| status.shellIntegrationStatus === "running-command"; | |
| const procStatusStarted = | |
| prevStatus?.shellProcStatus !== "running" && | |
| status.shellProcStatus === "running"; | |
| const commandJustStarted = shellIntegrationStarted || procStatusStarted; | |
| // Clear "finished" state when a new command starts | |
| if (commandJustStarted && globalStore.get(this.finishedUnreadAtom)) { | |
| globalStore.set(this.finishedUnreadAtom, false); | |
| } | |
| // Detect error exit | |
| const hasError = | |
| status.shellProcStatus === "done" && | |
| status.shellProcExitCode != null && | |
| status.shellProcExitCode !== 0; | |
| // Show "finished" status for successful completions | |
| // "stopped" status is handled by recomputeTerminalStatus based on exit code | |
| if (commandJustFinished && !hasError) { | |
| this.setFinishedUnread(); | |
| // Status will be cleared by tab.tsx when user views it (2-3 second delay) | |
| } | |
| // Store current status for next comparison | |
| this.previousStatusMap.set(blockId, { ...status }); | |
| this.terminalStatusMap.set(blockId, status); | |
| this.recomputeTerminalStatus(); |
🤖 Prompt for AI Agents
In `@frontend/app/store/tab-model.ts` around lines 129 - 171,
updateBlockTerminalStatus is only clearing finishedUnread when
shellIntegrationStatus goes from not-"running-command" to "running-command", so
background tabs that only change shellProcStatus (e.g., "done" -> "running")
keep finishedUnread set; update the commandJustStarted detection to also
consider a proc-status start transition (e.g., prevStatus?.shellProcStatus ===
"done" && status.shellProcStatus === "running" or more generally prev !==
"running" && status === "running"), and use that combined condition where you
currently check commandJustStarted before calling
globalStore.set(this.finishedUnreadAtom, false); reference
updateBlockTerminalStatus, previousStatusMap, shellIntegrationStatus,
shellProcStatus, and finishedUnreadAtom.
| // Refresh terminal status when tab becomes focused | ||
| // This catches OSC data that was buffered while tab was in background | ||
| const wasFocusedRef = React.useRef(isFocused); | ||
| React.useEffect(() => { | ||
| if (isFocused && !wasFocusedRef.current) { | ||
| // Tab just became focused - refresh status after a delay to let xterm process buffered data | ||
| const timer = setTimeout(() => { | ||
| model.updateTabTerminalStatus(); | ||
| }, 300); | ||
| return () => clearTimeout(timer); | ||
| } | ||
| wasFocusedRef.current = isFocused; | ||
| }, [isFocused, model]); |
There was a problem hiding this comment.
Update wasFocusedRef unconditionally to track state correctly.
The ref update on line 353 only executes when the early return path isn't taken. If isFocused becomes false, the ref won't update, causing the next focus transition to be missed.
🛠️ Suggested fix
const wasFocusedRef = React.useRef(isFocused);
React.useEffect(() => {
+ const wasFocused = wasFocusedRef.current;
+ wasFocusedRef.current = isFocused;
+
- if (isFocused && !wasFocusedRef.current) {
+ if (isFocused && !wasFocused) {
// Tab just became focused - refresh status after a delay to let xterm process buffered data
const timer = setTimeout(() => {
model.updateTabTerminalStatus();
}, 300);
return () => clearTimeout(timer);
}
- wasFocusedRef.current = isFocused;
}, [isFocused, model]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Refresh terminal status when tab becomes focused | |
| // This catches OSC data that was buffered while tab was in background | |
| const wasFocusedRef = React.useRef(isFocused); | |
| React.useEffect(() => { | |
| if (isFocused && !wasFocusedRef.current) { | |
| // Tab just became focused - refresh status after a delay to let xterm process buffered data | |
| const timer = setTimeout(() => { | |
| model.updateTabTerminalStatus(); | |
| }, 300); | |
| return () => clearTimeout(timer); | |
| } | |
| wasFocusedRef.current = isFocused; | |
| }, [isFocused, model]); | |
| // Refresh terminal status when tab becomes focused | |
| // This catches OSC data that was buffered while tab was in background | |
| const wasFocusedRef = React.useRef(isFocused); | |
| React.useEffect(() => { | |
| const wasFocused = wasFocusedRef.current; | |
| wasFocusedRef.current = isFocused; | |
| if (isFocused && !wasFocused) { | |
| // Tab just became focused - refresh status after a delay to let xterm process buffered data | |
| const timer = setTimeout(() => { | |
| model.updateTabTerminalStatus(); | |
| }, 300); | |
| return () => clearTimeout(timer); | |
| } | |
| }, [isFocused, model]); |
🤖 Prompt for AI Agents
In `@frontend/app/view/term/term.tsx` around lines 342 - 354, The wasFocusedRef
isn't updated when the effect early-returns, causing missed focus transitions;
update wasFocusedRef.current unconditionally so it always reflects the latest
isFocused. In the useEffect that depends on [isFocused, model], set
wasFocusedRef.current = isFocused outside the early-return path (e.g., at the
start or in a finally/after block) while keeping the existing logic that calls
model.updateTabTerminalStatus() when isFocused transitions from false to true
and clears the timeout on cleanup.
| go func() { | ||
| defer panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents", recover()) | ||
| wps.Broker.SendUpdateEvents(updates) | ||
| }() |
There was a problem hiding this comment.
Critical: recover() called outside goroutine scope won't catch panics.
The recover() is evaluated at goroutine spawn time (in the parent goroutine), not when the deferred function executes inside the new goroutine. This means panics inside the goroutine will not be caught.
🐛 Proposed fix
go func() {
- defer panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents", recover())
+ defer func() {
+ panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents", recover())
+ }()
wps.Broker.SendUpdateEvents(updates)
}()This same issue applies to all similar patterns in this file at lines 119-122, 141-144, 171-174, 201-204, 231-234, and 271-278.
📝 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.
| go func() { | |
| defer panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents", recover()) | |
| wps.Broker.SendUpdateEvents(updates) | |
| }() | |
| go func() { | |
| defer func() { | |
| panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents", recover()) | |
| }() | |
| wps.Broker.SendUpdateEvents(updates) | |
| }() |
🤖 Prompt for AI Agents
In `@pkg/service/objectservice/objectservice.go` around lines 91 - 94, The
deferred call currently passes recover() at goroutine spawn time so panics
inside the new goroutine won't be caught; wrap the PanicHandler call inside a
deferred anonymous function so recover() is invoked inside the goroutine's
deferred context (e.g., replace defer
panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents",
recover()) with defer func(){
panichandler.PanicHandler("ObjectService:UpdateTabName:SendUpdateEvents",
recover()) }()), and apply the same change to all other goroutine blocks in this
file (the patterns around SendUpdateEvents and similar anonymous goroutines at
the other noted locations).
| go func() { | ||
| defer panichandler.PanicHandler("ObjectService:UpdateObject:SendUpdateEvents", recover()) | ||
| if (waveObj.GetOType() == waveobj.OType_Workspace) && (waveObj.(*waveobj.Workspace).Name != "") { | ||
| wps.Broker.Publish(wps.WaveEvent{ | ||
| Event: wps.Event_WorkspaceUpdate}) | ||
| } | ||
| wps.Broker.SendUpdateEvents(updates) | ||
| }() |
There was a problem hiding this comment.
Same recover() issue; also consider safer type assertion.
In addition to the recover() bug mentioned earlier, the type assertion at line 273 could panic if the object is malformed (OType is "workspace" but actual type differs). With the broken panic handler, this would crash the goroutine silently.
🔧 Proposed fix with safe type assertion
go func() {
- defer panichandler.PanicHandler("ObjectService:UpdateObject:SendUpdateEvents", recover())
- if (waveObj.GetOType() == waveobj.OType_Workspace) && (waveObj.(*waveobj.Workspace).Name != "") {
+ defer func() {
+ panichandler.PanicHandler("ObjectService:UpdateObject:SendUpdateEvents", recover())
+ }()
+ if ws, ok := waveObj.(*waveobj.Workspace); ok && ws.Name != "" {
wps.Broker.Publish(wps.WaveEvent{
Event: wps.Event_WorkspaceUpdate})
}
wps.Broker.SendUpdateEvents(updates)
}()🤖 Prompt for AI Agents
In `@pkg/service/objectservice/objectservice.go` around lines 271 - 278, The
deferred call currently passes recover() directly to panichandler.PanicHandler
and does an unsafe type assertion on waveObj; change the defer to capture the
panic inside the goroutine (defer func(){ if r := recover(); r != nil {
panichandler.PanicHandler("ObjectService:UpdateObject:SendUpdateEvents", r) }
}()) and replace the direct assertion waveObj.(*waveobj.Workspace) with a safe
check: if waveObj.GetOType() == waveobj.OType_Workspace { if w, ok :=
waveObj.(*waveobj.Workspace); ok && w.Name != "" {
wps.Broker.Publish(wps.WaveEvent{Event: wps.Event_WorkspaceUpdate}) } } before
calling wps.Broker.SendUpdateEvents(updates).
| // ValidateInt ensures value is an integer within range | ||
| func ValidateInt(key string, value interface{}, minVal, maxVal int) error { | ||
| if value == nil { | ||
| return nil | ||
| } | ||
|
|
||
| // JSON numbers come as float64 | ||
| f, ok := value.(float64) | ||
| if !ok { | ||
| return &ValidationError{ | ||
| Key: key, | ||
| Value: value, | ||
| Message: fmt.Sprintf("must be a number, got %T", value), | ||
| } | ||
| } | ||
|
|
||
| i := int(f) | ||
| if i < minVal || i > maxVal { | ||
| return &ValidationError{ | ||
| Key: key, | ||
| Value: i, | ||
| Message: fmt.Sprintf("must be between %d and %d", minVal, maxVal), | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // ValidateNullableInt ensures value is nil or integer within range | ||
| func ValidateNullableInt(key string, value interface{}, minVal, maxVal int) error { | ||
| if value == nil { | ||
| return nil // nil is valid for pointer types | ||
| } | ||
|
|
||
| // JSON numbers come as float64 | ||
| f, ok := value.(float64) | ||
| if !ok { | ||
| return &ValidationError{ | ||
| Key: key, | ||
| Value: value, | ||
| Message: fmt.Sprintf("must be a number or null, got %T", value), | ||
| } | ||
| } | ||
|
|
||
| i := int(f) | ||
| if i < minVal || i > maxVal { | ||
| return &ValidationError{ | ||
| Key: key, | ||
| Value: i, | ||
| Message: fmt.Sprintf("must be between %d and %d", minVal, maxVal), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Reject non-integer and overflow-prone numeric inputs before int conversion.
Casting float64 to int first can silently truncate (e.g., 12.7 → 12) and can overflow for very large values. Validate integer-ness and range before conversion.
🔧 Proposed fix
import (
"fmt"
"log"
+ "math"
"net/url"
"os"
"path/filepath"
"runtime"
"strings"
@@
- // JSON numbers come as float64
f, ok := value.(float64)
if !ok {
return &ValidationError{
Key: key,
Value: value,
Message: fmt.Sprintf("must be a number, got %T", value),
}
}
- i := int(f)
- if i < minVal || i > maxVal {
+ if math.IsNaN(f) || math.IsInf(f, 0) || math.Trunc(f) != f {
+ return &ValidationError{
+ Key: key,
+ Value: f,
+ Message: "must be an integer",
+ }
+ }
+ if f < float64(minVal) || f > float64(maxVal) {
return &ValidationError{
Key: key,
- Value: i,
+ Value: f,
Message: fmt.Sprintf("must be between %d and %d", minVal, maxVal),
}
}
+ i := int(f)- // JSON numbers come as float64
f, ok := value.(float64)
if !ok {
return &ValidationError{
Key: key,
Value: value,
Message: fmt.Sprintf("must be a number or null, got %T", value),
}
}
- i := int(f)
- if i < minVal || i > maxVal {
+ if math.IsNaN(f) || math.IsInf(f, 0) || math.Trunc(f) != f {
+ return &ValidationError{
+ Key: key,
+ Value: f,
+ Message: "must be an integer or null",
+ }
+ }
+ if f < float64(minVal) || f > float64(maxVal) {
return &ValidationError{
Key: key,
- Value: i,
+ Value: f,
Message: fmt.Sprintf("must be between %d and %d", minVal, maxVal),
}
}
+ i := int(f)🤖 Prompt for AI Agents
In `@pkg/waveobj/validators.go` around lines 296 - 348, ValidateInt and
ValidateNullableInt currently cast float64 to int which can truncate fractional
values and overflow; instead first assert the number is an integer (e.g.
math.Trunc(f) == f or use math.Modf to ensure no fractional part) and verify the
float64 value falls within float64(minVal) and float64(maxVal) bounds before
converting to int; return a ValidationError mentioning "must be an integer" (or
"must be an integer or null" for ValidateNullableInt) when fractional, and keep
the existing range error if out of bounds, then safely cast to int only after
these checks.
Critical: - validators.go: Fix UNC path validation - use correct indices for server/share segments Major: - widgets.tsx: Clone blockdef before modification to avoid mutating cached config - workspace.tsx/scss: Make menu button keyboard-accessible using semantic button element - pathutil.ts: Normalize Windows separators for blocked path checking - pathutil.ts: Allow empty paths through quick validation for OSC 7 basedir clearing - presetutil.ts: Implement wildcard prefix matching for bg:* allowlist entries Minor: - global.ts: Guard against empty string tab IDs in activeTabAtom - tab.tsx: Wrap UpdateObjectMeta with fireAndForget to avoid unhandled promise - term-model.ts: Use readAtom helper for safe atom reads Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@frontend/app/view/term/term-model.ts`:
- Around line 486-507: The RTInfo promise callback closes over the original
procStatus and can overwrite newer status when it resolves; change the callback
in RpcApi.GetRTInfoCommand so it re-reads the current terminal status before
calling this.tabModel.updateBlockTerminalStatus: inside the .then and .catch
handlers, fetch the latest procStatus (e.g., via
this.tabModel.getBlockTerminalStatus(this.blockId) or another current-state
accessor) or compare a version/id to ensure the fetched RTInfo applies, and only
call updateBlockTerminalStatus with values derived from the up-to-date
procStatus (and this.blockId) to avoid stale overwrites.
In `@frontend/app/workspace/workspace.tsx`:
- Around line 12-16: TabBreadcrumb currently builds an ORef using staticTabId
which can be an empty string when there is no active tab; switch to using the
guarded atom value atoms.activeTab (via useAtomValue(atoms.activeTab) or the
existing atoms usage) when constructing the ORef and ensure you skip or return a
safe fallback when the active tab id is falsy so you never create an ORef with
an empty id; update any other occurrences (e.g., the similar logic around lines
52-56) to use the same guarded atoms.activeTab value and short-circuit when it
is empty.
In `@frontend/util/pathutil.ts`:
- Around line 146-154: The Biome linter flags the control-character regex
/[\x00-\x1F]/ in pathutil.ts; to fix add an inline disable comment immediately
before the test that uses that regex in the function that computes
pathWithoutDrive (where the code currently does if (/[\x00-\x1F]/.test(path)) {
return true; }); insert the Biome inline rule disable (e.g., // biome-ignore
suspicious.noControlCharactersInRegex) directly above that line so the specific
regex is exempt, leaving the rest of the logic unchanged.
♻️ Duplicate comments (1)
pkg/waveobj/validators.go (1)
307-361: Validate integer-ness and range before casting float64 to int.This issue was flagged in a previous review and remains unaddressed. Casting
float64directly tointsilently truncates fractional values (e.g.,12.7 → 12) and can cause undefined behavior for values outside the int range.,
🔧 Proposed fix
+import "math" + // ValidateInt ensures value is an integer within range func ValidateInt(key string, value interface{}, minVal, maxVal int) error { if value == nil { return nil } - // JSON numbers come as float64 f, ok := value.(float64) if !ok { return &ValidationError{ Key: key, Value: value, Message: fmt.Sprintf("must be a number, got %T", value), } } + if math.IsNaN(f) || math.IsInf(f, 0) || math.Trunc(f) != f { + return &ValidationError{ + Key: key, + Value: f, + Message: "must be an integer", + } + } + if f < float64(minVal) || f > float64(maxVal) { + return &ValidationError{ + Key: key, + Value: f, + Message: fmt.Sprintf("must be between %d and %d", minVal, maxVal), + } + } i := int(f) - if i < minVal || i > maxVal { - return &ValidationError{ - Key: key, - Value: i, - Message: fmt.Sprintf("must be between %d and %d", minVal, maxVal), - } - } return nil }Apply the same pattern to
ValidateNullableInt.
🧹 Nitpick comments (1)
pkg/waveobj/validators.go (1)
363-413: Consider rejecting NaN and Inf float values.The float validators don't check for
math.IsNaN(f)ormath.IsInf(f, 0). These special values would pass validation and could cause issues downstream.🔧 Proposed fix
func ValidateFloat(key string, value interface{}, minVal, maxVal float64) error { if value == nil { return nil } f, ok := value.(float64) if !ok { return &ValidationError{ Key: key, Value: value, Message: fmt.Sprintf("must be a number, got %T", value), } } + if math.IsNaN(f) || math.IsInf(f, 0) { + return &ValidationError{ + Key: key, + Value: f, + Message: "must be a finite number", + } + } + if f < minVal || f > maxVal {Apply the same to
ValidateNullableFloat.
| // For background tabs or when termRef isn't ready, query RTInfo | ||
| // RTInfo stores the shell state written by the tab that processes OSC 16162 | ||
| RpcApi.GetRTInfoCommand(TabRpcClient, { | ||
| oref: WOS.makeORef("block", this.blockId), | ||
| }) | ||
| .then((rtInfo) => { | ||
| const shellIntegrationStatus = rtInfo?.["shell:state"] as string | null; | ||
|
|
||
| this.tabModel.updateBlockTerminalStatus(this.blockId, { | ||
| shellProcStatus: procStatus?.shellprocstatus ?? null, | ||
| shellProcExitCode: procStatus?.shellprocexitcode ?? null, | ||
| shellIntegrationStatus: shellIntegrationStatus ?? null, | ||
| }); | ||
| }) | ||
| .catch((err) => { | ||
| // If RTInfo query fails, update with null shell integration status | ||
| this.tabModel.updateBlockTerminalStatus(this.blockId, { | ||
| shellProcStatus: procStatus?.shellprocstatus ?? null, | ||
| shellProcExitCode: procStatus?.shellprocexitcode ?? null, | ||
| shellIntegrationStatus: null, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Avoid stale status overwrites from async RTInfo fetch.
The RTInfo callback uses procStatus captured at call time. If a newer status arrives before the promise resolves, the late callback can revert the tab status. Re-read the latest status in the callback (or guard by version).
🛠️ Suggested fix
- .then((rtInfo) => {
- const shellIntegrationStatus = rtInfo?.["shell:state"] as string | null;
-
- this.tabModel.updateBlockTerminalStatus(this.blockId, {
- shellProcStatus: procStatus?.shellprocstatus ?? null,
- shellProcExitCode: procStatus?.shellprocexitcode ?? null,
- shellIntegrationStatus: shellIntegrationStatus ?? null,
- });
- })
- .catch((err) => {
- // If RTInfo query fails, update with null shell integration status
- this.tabModel.updateBlockTerminalStatus(this.blockId, {
- shellProcStatus: procStatus?.shellprocstatus ?? null,
- shellProcExitCode: procStatus?.shellprocexitcode ?? null,
- shellIntegrationStatus: null,
- });
- });
+ .then((rtInfo) => {
+ const shellIntegrationStatus = rtInfo?.["shell:state"] as string | null;
+ const latestProcStatus = globalStore.get(this.shellProcFullStatus);
+ this.tabModel.updateBlockTerminalStatus(this.blockId, {
+ shellProcStatus: latestProcStatus?.shellprocstatus ?? null,
+ shellProcExitCode: latestProcStatus?.shellprocexitcode ?? null,
+ shellIntegrationStatus: shellIntegrationStatus ?? null,
+ });
+ })
+ .catch(() => {
+ const latestProcStatus = globalStore.get(this.shellProcFullStatus);
+ // If RTInfo query fails, update with null shell integration status
+ this.tabModel.updateBlockTerminalStatus(this.blockId, {
+ shellProcStatus: latestProcStatus?.shellprocstatus ?? null,
+ shellProcExitCode: latestProcStatus?.shellprocexitcode ?? null,
+ shellIntegrationStatus: null,
+ });
+ });🤖 Prompt for AI Agents
In `@frontend/app/view/term/term-model.ts` around lines 486 - 507, The RTInfo
promise callback closes over the original procStatus and can overwrite newer
status when it resolves; change the callback in RpcApi.GetRTInfoCommand so it
re-reads the current terminal status before calling
this.tabModel.updateBlockTerminalStatus: inside the .then and .catch handlers,
fetch the latest procStatus (e.g., via
this.tabModel.getBlockTerminalStatus(this.blockId) or another current-state
accessor) or compare a version/id to ensure the fetched RTInfo applies, and only
call updateBlockTerminalStatus with values derived from the up-to-date
procStatus (and this.blockId) to avoid stale overwrites.
| import { atoms, getApi } from "@/store/global"; | ||
| import * as WOS from "@/store/wos"; | ||
| import { useAtomValue } from "jotai"; | ||
| import { memo, useEffect, useRef } from "react"; | ||
| import React, { memo, useEffect, useMemo, useRef } from "react"; | ||
| import "./workspace.scss"; |
There was a problem hiding this comment.
Guard against empty tab IDs in TabBreadcrumb.
When no active tab, staticTabId can be "", so building an ORef can point to an invalid object. Using atoms.activeTab (already guarding empty IDs) avoids this edge case.
🛠️ Suggested fix
-import * as WOS from "@/store/wos";
-import React, { memo, useEffect, useMemo, useRef } from "react";
+import React, { memo, useEffect, useRef } from "react";
@@
- const tabId = useAtomValue(atoms.staticTabId);
const ws = useAtomValue(atoms.workspace);
- const tabAtom = useMemo(() => WOS.getWaveObjectAtom<Tab>(WOS.makeORef("tab", tabId)), [tabId]);
- const tabData = useAtomValue(tabAtom);
+ const tabData = useAtomValue(atoms.activeTab);Also applies to: 52-56
🤖 Prompt for AI Agents
In `@frontend/app/workspace/workspace.tsx` around lines 12 - 16, TabBreadcrumb
currently builds an ORef using staticTabId which can be an empty string when
there is no active tab; switch to using the guarded atom value atoms.activeTab
(via useAtomValue(atoms.activeTab) or the existing atoms usage) when
constructing the ORef and ensure you skip or return a safe fallback when the
active tab id is falsy so you never create an ORef with an empty id; update any
other occurrences (e.g., the similar logic around lines 52-56) to use the same
guarded atoms.activeTab value and short-circuit when it is empty.
| // < > : " | ? * and control characters | ||
| // Note: : is allowed as second char for drive letter | ||
| const pathWithoutDrive = path.length >= 2 && path[1] === ":" ? path.substring(2) : path; | ||
| if (/[<>"|?*]/.test(pathWithoutDrive)) { | ||
| return true; | ||
| } | ||
| // Control characters (0x00-0x1F) | ||
| if (/[\x00-\x1F]/.test(path)) { | ||
| return true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "pathutil.ts" -type fRepository: wavetermdev/waveterm
Length of output: 91
🏁 Script executed:
sed -n '140,160p' ./frontend/util/pathutil.tsRepository: wavetermdev/waveterm
Length of output: 816
🏁 Script executed:
find . -maxdepth 2 -name "biome.json*" -o -name ".biomerc*"Repository: wavetermdev/waveterm
Length of output: 76
🏁 Script executed:
cat ./biome.jsonRepository: wavetermdev/waveterm
Length of output: 1838
🌐 Web query:
Biome regex control characters lint error \x00-\x1F
💡 Result:
What you’re seeing is Biome’s noControlCharactersInRegex rule (lint/suspicious/noControlCharactersInRegex) flagging control-character escapes (hex \x00–\x1F, \u0000–\u001F, or \u{0}–\u{1F}) as suspicious — it’s the same check as ESLint’s no-control-regex. The rule is enabled by default and reports these escapes because ASCII control characters (0–31) are rarely intended in JS regexes. [1][2]
Quick options to resolve it
- Remove or replace the control escape with a visible/explicit alternative (e.g., use \t or \n where appropriate).
- If you really need to match that range, disable the rule for that line/block:
- Inline: add a disable comment (example: // biome-disable-next-line lint/suspicious/noControlCharactersInRegex) before the regex.
- Config: turn the rule off in biome.json under linter.rules.suspicious.noControlCharactersInRegex.
- Rework your logic (e.g., match a character class produced at runtime) so the pattern doesn’t contain literal control escapes.
Examples
- Problematic: /\x00/ (flagged)
- Allowed alternatives: /\n/ or /\t/ (if matching newline/tab)
- Disable inline:
// biome-disable-next-line lint/suspicious/noControlCharactersInRegex
const re = /\x00/;
Docs
- Biome rule: noControlCharactersInRegex. [1]
- ESLint rule: no-control-regex (same behavior). [2]
If you want, tell me the exact regex you’re linting and I’ll suggest the safest rewrite or the proper disable placement.
🌐 Web query:
Biome noControlCharactersInRegex RegExp constructor string vs regex literal
💡 Result:
Short answer: Biome's noControlCharactersInRegex flags control-character escapes whether they appear in a regex literal or (when the string actually contains those control characters/escapes) in a RegExp-constructor string. The rule treats raw control chars and the escape forms \x00–\x1F, \u0000–\u001F and \u{0}–\u{1F} as suspicious. UseRegexLiterals is a separate rule that simply prefers regex literals over new RegExp("...") when the pattern is static (partly because literals avoid extra string escaping).
Why this matters practically:
- Regex literal: /\x0C/ — the control escape is directly in the pattern and is flagged by noControlCharactersInRegex. [1]
- RegExp constructor with an actual control escape in the string: new RegExp("\x0C") — that string produces the same control character and Biome flags it too. (The docs include this example.) [2]
- If you use new RegExp("\x0C") the pattern will contain the sequence \x0C (two characters: backslash + x) which is different; but because of string escaping you must be careful — the rule analyzes the resulting pattern and flags actual control-character escapes. [1][2]
References:
- Biome noControlCharactersInRegex (description + examples). [1]
- Biome useRegexLiterals / prefer-regex-literals explanation. [2]
🌐 Web query:
Biome noControlCharactersInRegex disable comment example solution
💡 Result:
Use a Biome suppression comment above the offending line with the rule path lint/suspicious/noControlCharactersInRegex. Example:
// biome-ignore lint/suspicious/noControlCharactersInRegex: needed for legacy binary pattern
const pattern = /\x00/;
(See Biome linter ignore-comments and the noControlCharactersInRegex rule.) [1][2]
Sources
- noControlCharactersInRegex rule — Biome. [1]
- Linter — Biome (ignore comments syntax). [2]
Fix Biome noControlCharactersInRegex error with inline disable comment.
Biome's noControlCharactersInRegex rule (enabled by default) flags the literal /[\x00-\x1F]/ because it contains control-character escapes. Converting to a RegExp constructor does not resolve this—Biome flags control-character escapes regardless of syntax. Use an inline disable comment instead:
🛠️ Suggested fix
// Control characters (0x00-0x1F)
+ // biome-ignore lint/suspicious/noControlCharactersInRegex: needed to match control chars
if (/[\x00-\x1F]/.test(path)) {
return true;
}Alternatively, disable the rule in biome.json under linter.rules.suspicious.noControlCharactersInRegex if this pattern is used elsewhere.
🧰 Tools
🪛 Biome (2.1.2)
[error] 153-153: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 153-153: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🤖 Prompt for AI Agents
In `@frontend/util/pathutil.ts` around lines 146 - 154, The Biome linter flags the
control-character regex /[\x00-\x1F]/ in pathutil.ts; to fix add an inline
disable comment immediately before the test that uses that regex in the function
that computes pathWithoutDrive (where the code currently does if
(/[\x00-\x1F]/.test(path)) { return true; }); insert the Biome inline rule
disable (e.g., // biome-ignore suspicious.noControlCharactersInRegex) directly
above that line so the specific regex is exempt, leaving the rest of the logic
unchanged.
3 participants
Summary
This PR adds per-tab base directory context and redesigns the tab UI to match VS Code style.
Tab Base Directory Feature
tab:basedir,tab:basedirlockmetadata)Security & Validation
VS Code Style Tab Redesign
Other Improvements
Screenshots
Test plan
cdcommands🤖 Generated with Claude Code