UX Improvements: Folder-based tab titles, scroll enhancements, and file preview features #2674
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request:
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
pkg/wcore/workspace.go (1)
202-205: Avoid double workspace fetch (CreateTab + createTabObj) to reduce staleness and overhead.
CreateTabalready hasws;createTabObjfetches again and updates. Consider passingwsintocreateTabObj(or extracting a helper that does a single fetch + update) to reduce TOCTOU weirdness and make tab naming/active-tab reads consistent with the update you’re about to write.Also applies to: 231-231, 273-297
frontend/app/view/preview/preview-streaming.tsx (1)
50-57: Refresh callback pattern implemented correctly.The effect properly registers and cleans up the refresh callback, following the established pattern across preview components (preview-edit.tsx, preview-markdown.tsx).
Consider including
modelin the dependency array for clarity, even though it should be stable:- }, []); + }, [model]);This makes the effect's dependencies explicit and aligns with React exhaustive-deps best practices, though the current implementation is safe given that
modelis a stable prop.frontend/app/aipanel/aipanelmessages.tsx (2)
26-37: Consider extracting the timeout delay as a named constant.The 100ms delay for resetting
isAutoScrollingis reasonable for distinguishing programmatic scrolls from user scrolls, but a named constant would improve readability.+const AUTO_SCROLL_DEBOUNCE_MS = 100; + const scrollToBottom = () => { const container = messagesContainerRef.current; if (container) { isAutoScrolling.current = true; container.scrollTop = container.scrollHeight; container.scrollLeft = 0; userHasScrolledUp.current = false; setTimeout(() => { isAutoScrolling.current = false; - }, 100); + }, AUTO_SCROLL_DEBOUNCE_MS); } };
39-61: Consider using a passive event listener for better scroll performance.Scroll event listeners can impact performance. Since this handler doesn't call
preventDefault(), marking it as passive can improve scrolling smoothness.- container.addEventListener("scroll", handleScroll); - return () => container.removeEventListener("scroll", handleScroll); + container.addEventListener("scroll", handleScroll, { passive: true }); + return () => container.removeEventListener("scroll", handleScroll);frontend/app/tab/tabbar.tsx (1)
45-64: Consider removing unused WaveAIButton component.The
WaveAIButtoncomponent is defined but, according to the PR summary, was removed from the tab bar UI. If it's no longer rendered anywhere, consider removing the component definition to reduce dead code.Verify if WaveAIButton is still used:
#!/bin/bash # Description: Check if WaveAIButton is still referenced in the render tree # Search for WaveAIButton usage in tabbar.tsx rg -n "WaveAIButton" frontend/app/tab/tabbar.tsxpkg/blockcontroller/blockcontroller.go (1)
321-326: Consider error handling for title generation.The call to
CheckAndGenerateTitlehas no error handling. If title generation fails, errors will be silently ignored. Consider logging errors to aid debugging, especially during initial rollout of this feature.Apply this diff to add error logging:
// Check if we should auto-generate a tab title if blockFile == wavebase.BlockFile_Term { - CheckAndGenerateTitle(blockId, data) + if err := CheckAndGenerateTitle(blockId, data); err != nil { + log.Printf("error generating tab title for block %s: %v", blockId, err) + } }frontend/types/custom.d.ts (1)
133-133: Comment doesn't match implementation.The comment
// get-path-for-filesuggests an IPC channel, but the implementation inemain/preload.tsdirectly callswebUtils.getPathForFile(file)synchronously without IPC. Consider updating the comment or removing it for clarity.Apply this diff to clarify the comment:
- getPathForFile: (file: File) => string; // get-path-for-file + getPathForFile: (file: File) => string; // uses webUtils.getPathForFilefrontend/app/view/term/termwrap.ts (1)
852-860: Scroll restoration condition may miss some cases.The condition
currentScrollY < scrollY - 10only restores scroll if the viewport jumped upward by more than 10 lines. Unexpected downward jumps won't trigger restoration. Consider also handling cases where scroll position changed significantly in either direction.// Restore scroll position after paste if it changed unexpectedly setTimeout(() => { const currentScrollY = this.terminal.buffer.active.viewportY; - // Only restore if we've scrolled significantly (not just normal paste scroll) - if (currentScrollY < scrollY - 10) { + // Only restore if we've scrolled significantly in either direction + if (Math.abs(currentScrollY - scrollY) > 10) { this.terminal.scrollToLine(scrollY); } }, 50);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
emain/emain-platform.ts(1 hunks)emain/preload.ts(2 hunks)frontend/app/aipanel/aipanelmessages.tsx(2 hunks)frontend/app/modals/confirmclosetab.tsx(1 hunks)frontend/app/modals/modalregistry.tsx(2 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/tab/tabbar.tsx(1 hunks)frontend/app/view/preview/preview-edit.tsx(1 hunks)frontend/app/view/preview/preview-markdown.tsx(1 hunks)frontend/app/view/preview/preview-model.tsx(2 hunks)frontend/app/view/preview/preview-streaming.tsx(2 hunks)frontend/app/view/term/term-model.ts(1 hunks)frontend/app/view/term/term.tsx(2 hunks)frontend/app/view/term/termwrap.ts(5 hunks)frontend/types/custom.d.ts(1 hunks)frontend/types/gotypes.d.ts(2 hunks)pkg/blockcontroller/blockcontroller.go(1 hunks)pkg/blockcontroller/tabtitle_trigger.go(1 hunks)pkg/waveai/tabtitle.go(1 hunks)pkg/wcore/workspace.go(2 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(2 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/app/view/term/term.tsx
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
pkg/wshrpc/wshclient/wshclient.go
🧬 Code graph analysis (13)
pkg/waveai/tabtitle.go (3)
pkg/wstore/wstore_dbops.go (1)
DBMustGet(138-149)pkg/waveobj/waveobj.go (1)
GetMeta(216-226)pkg/waveobj/metaconsts.go (1)
MetaKey_CmdCwd(56-56)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (1)
GenerateTabTitleCommand(291-293)pkg/wshrpc/wshclient/wshclient.go (1)
GenerateTabTitleCommand(354-357)
frontend/app/aipanel/aipanelmessages.tsx (1)
frontend/app/aipanel/waveai-model.tsx (1)
scrollToBottom(288-290)
frontend/app/view/preview/preview-markdown.tsx (1)
frontend/app/view/preview/preview.tsx (1)
SpecializedViewProps(20-23)
pkg/wcore/workspace.go (6)
pkg/waveobj/metamap.go (1)
MetaMapType(8-8)pkg/wstore/wstore_dbops.go (1)
DBGet(133-136)pkg/waveobj/wtype.go (5)
Tab(185-192)Tab(194-196)Block(282-290)Block(292-294)BlockDef(244-247)pkg/waveobj/waveobj.go (1)
GetMeta(216-226)pkg/waveobj/metaconsts.go (1)
MetaKey_CmdCwd(56-56)pkg/wcore/layout.go (2)
GetNewTabLayout(63-72)ApplyPortableLayout(116-142)
frontend/app/view/term/term.tsx (2)
frontend/app/store/global.ts (1)
getApi(846-846)electron-builder.config.cjs (1)
path(4-4)
frontend/app/modals/confirmclosetab.tsx (3)
frontend/app/store/global.ts (2)
atoms(836-836)getApi(846-846)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/modals/modal.tsx (1)
Modal(149-149)
pkg/wshrpc/wshserver/wshserver.go (3)
frontend/app/store/wshclientapi.ts (1)
GenerateTabTitleCommand(291-293)pkg/wshrpc/wshclient/wshclient.go (1)
GenerateTabTitleCommand(354-357)pkg/waveai/tabtitle.go (1)
GenerateTabTitle(17-46)
pkg/wshrpc/wshclient/wshclient.go (2)
frontend/app/store/wshclientapi.ts (1)
GenerateTabTitleCommand(291-293)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)
frontend/app/view/preview/preview-model.tsx (2)
frontend/layout/lib/nodeRefMap.ts (1)
get(20-24)frontend/util/util.ts (1)
isBlank(511-511)
frontend/app/modals/modalregistry.tsx (1)
frontend/app/modals/confirmclosetab.tsx (1)
ConfirmCloseTabModal(39-39)
pkg/blockcontroller/tabtitle_trigger.go (4)
pkg/wstore/wstore_dbops.go (1)
DBGet(133-136)pkg/waveobj/waveobj.go (1)
ParseORef(80-98)pkg/waveobj/wtype.go (1)
OType_Tab(28-28)pkg/waveai/tabtitle.go (1)
GenerateTabTitle(17-46)
emain/emain-platform.ts (1)
frontend/app/store/global.ts (1)
isDev(862-862)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (34)
pkg/wcore/workspace.go (2)
202-205: Good: fail fast if workspace doesn’t exist (and enables correct tab numbering).
This makes the error path deterministic instead of relying on downstream DB calls.
244-255: Verify meta precedence: preset meta may override inheritedcmd:cwd.
You merge inherited cwd into the first block meta (good), then applypresetMetaonto the tab. If presets can includecmd:cwd, confirm whether that should win over inheritance (and howwstore.UpdateObjectMeta(..., presetMeta, true)resolves conflicts).Also applies to: 258-264
frontend/app/view/preview/preview-markdown.tsx (2)
5-7: LGTM!The import additions are correct and necessary for the new refresh callback functionality.
11-18: LGTM!The refresh callback lifecycle management is correctly implemented. The pattern of setting
model.refreshCallbackon mount and clearing it on unmount aligns with the established pattern used in other preview components and properly supports the manual refresh feature for Markdown previews.frontend/app/view/preview/preview-streaming.tsx (1)
6-6: LGTM!The new imports are necessary for the refresh callback mechanism and are correctly added.
Also applies to: 10-10
frontend/app/aipanel/aipanelmessages.tsx (3)
23-24: Good use of refs for scroll tracking state.Using refs instead of state is the correct approach here since these values don't need to trigger re-renders and are only used for control flow logic.
74-78: LGTM!Correctly preserves the user's scroll position when reopening the panel.
80-91: LGTM!The logic correctly respects user scroll position during status transitions, enabling users to read earlier messages while the AI continues generating without being interrupted by auto-scroll.
frontend/types/gotypes.d.ts (1)
1224-1224: LGTM!The addition of the optional
"settings:transparent"boolean field to both telemetry types is consistent with the existing pattern for settings tracking.Also applies to: 1309-1309
frontend/app/view/preview/preview-model.tsx (2)
373-385: Good UX: Defaulting to terminal's current working directory.When no file is selected, the preview now defaults to showing the terminal's current working directory from block metadata. This provides better context to users.
432-433: Correct reactive pattern for refresh.Subscribing to
refreshVersionviaget(this.refreshVersion)properly establishes a Jotai dependency, ensuring the file data re-fetches when a refresh is triggered.emain/emain-platform.ts (1)
29-34: LGTM: Improved branding in macOS menu bar.Setting the app name to "Wave (Dev)" or "Wave" based on the environment provides better branding than the previous approach. The explanatory comments clarify why the
envPathsconfiguration is sufficient for data directory organization.frontend/app/view/term/term-model.ts (1)
492-505: LGTM: Terminal scroll keyboard shortcuts.The new Cmd+ArrowDown (scroll to bottom) and Cmd+ArrowUp (scroll to top) shortcuts provide convenient terminal navigation. The implementation properly checks for terminal existence and returns true to indicate the events were handled.
pkg/wshrpc/wshserver/wshserver.go (1)
1397-1403: LGTM: Clean RPC command implementation.The
GenerateTabTitleCommandproperly delegates towaveai.GenerateTabTitleand wraps errors appropriately. The implementation follows the established pattern for WshServer command methods.frontend/app/modals/modalregistry.tsx (1)
10-10: LGTM: Modal registration follows existing pattern.The
ConfirmCloseTabModalis properly imported and registered in the modal registry, consistent with other modal registrations.Also applies to: 19-19
frontend/app/tab/tabbar.tsx (1)
621-626: Good separation of concerns with confirmation modal.Moving the tab close logic into
ConfirmCloseTabModalprovides a cleaner architecture and enables user confirmation before closing tabs.frontend/app/view/preview/preview-edit.tsx (1)
66-73: LGTM: Proper refresh callback lifecycle management.The
refreshCallbackis correctly set in the effect and cleaned up on unmount. The atomic update pattern(v) => v + 1ensures race-condition-free increments of the refresh version.emain/preload.ts (1)
4-4: LGTM!Clean integration using Electron's built-in
webUtils.getPathForFileAPI. The implementation properly exposes the file path resolution functionality via contextBridge for use in the renderer process.Also applies to: 71-71
frontend/app/store/wshclientapi.ts (1)
290-293: LGTM!The
GenerateTabTitleCommandfollows the established pattern for RPC commands and correctly aligns with the server-side interface.pkg/wshrpc/wshrpctypes.go (2)
188-190: LGTM!The command constant is properly defined and logically grouped under tab-related commands.
266-266: LGTM!The interface method signature is consistent with the pattern established for other RPC commands and properly returns
(string, error)for the generated title.pkg/wshrpc/wshclient/wshclient.go (1)
353-357: LGTM!The generated
GenerateTabTitleCommandcorrectly follows the established pattern for RPC client commands and properly uses type-safe helper with string return type.frontend/app/modals/confirmclosetab.tsx (1)
13-35: The implementation is correct as-is and does not require the suggested error handling.The
closeTabAPI returnsvoid(not a Promise), as it usesipcRenderer.send()which is a fire-and-forget operation. ThedeleteLayoutModelForTabfunction is synchronous and only performs a map deletion—no error handling is possible or needed. This pattern is consistent throughout the codebase, where modal confirm handlers remain synchronous and don't wrap IPC operations in try-catch blocks.Likely an incorrect or invalid review comment.
pkg/waveai/tabtitle.go (1)
48-70: No action needed; current implementation handles Windows paths reasonably for UI title generation.The use of
filepath.Cleanand splitting by the platform-specific separator correctly handles typical Windows paths (drive letters, UNC shares) and returns sensible tab titles. For a drive root likeC:\, returning"C:"is appropriate; for\\server\share, returning"share"appropriately shows the share name. These behaviors are suitable for a UI component generating tab names from the working directory.frontend/app/view/term/term.tsx (4)
10-10: LGTM!Import of
getApiis appropriate for accessing the Electron API to resolve file paths.
353-358: LGTM!Standard drag-over handler correctly prevents default and indicates copy operation.
405-413: LGTM!Handlers correctly prevent default browser drag behavior.
416-423: LGTM!Event handlers are correctly wired to the terminal view container.
pkg/blockcontroller/tabtitle_trigger.go (2)
18-21: LGTM!Constants are well-documented and provide reasonable defaults for title generation behavior.
86-104: LGTM!The async title generation goroutine has proper timeout handling and error logging.
frontend/app/view/term/termwrap.ts (4)
539-542: LGTM!Selection change handler correctly uses markdown conversion for copy-on-select.
547-561: LGTM!Copy handler correctly intercepts events and provides markdown-formatted text. Disposal is properly registered.
566-590: LGTM!IME composition handling is well-implemented with proper event listener registration, blur handling, and cleanup disposal.
776-788: LGTM!Scroll preservation logic correctly captures viewport position and restores it after resize when user wasn't at bottom.
| useEffect(() => { | ||
| scrollToBottom(); | ||
| // Only auto-scroll if user hasn't manually scrolled up | ||
| if (!userHasScrolledUp.current) { | ||
| scrollToBottom(); | ||
| } | ||
| }, [messages]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the file and check its existence
fd -t f "aipanelmessages.tsx" --type f
# Check how large it is
wc -l frontend/app/aipanel/aipanelmessages.tsx 2>/dev/null || echo "File not found, searching..."
# Search for userHasScrolledUp usage
rg -n "userHasScrolledUp" -t tsx -t ts 2>/dev/null || rg -n "userHasScrolledUp"Repository: wavetermdev/waveterm
Length of output: 837
🏁 Script executed:
#!/bin/bash
# Search for conversation reset or message clearing logic
rg -n "messages.*\[\]|clearMessages|resetConversation|conversation.*reset" -t tsx -t ts 2>/dev/null || rg -n "messages.*\[\]|clearMessages|resetConversation|conversation.*reset"Repository: wavetermdev/waveterm
Length of output: 3173
🏁 Script executed:
#!/bin/bash
# Read the full file to understand the complete context
cat -n frontend/app/aipanel/aipanelmessages.tsxRepository: wavetermdev/waveterm
Length of output: 5151
🏁 Script executed:
#!/bin/bash
# Check the parent component to see how messages are managed and if there's any reset logic
rg -n "AIPanelMessages" -t tsx -t ts -B 5 -A 5 | head -100Repository: wavetermdev/waveterm
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Look for the model.clearMessages or conversation reset in waveai-model.tsx
sed -n '370,390p' frontend/app/aipanel/waveai-model.tsxRepository: wavetermdev/waveterm
Length of output: 1039
🏁 Script executed:
#!/bin/bash
# Check if there's any reset logic in parent or if messages completely empty means new conversation
rg -n "clearChat|newChat|clearMessages" frontend/app/aipanel/ -A 3Repository: wavetermdev/waveterm
Length of output: 2001
Edge case: userHasScrolledUp persists across conversation resets.
When a user scrolls up in one conversation, then clears the chat or starts a new conversation (via clearChat() which calls useChatSetMessages([]) and resets the messages array), the userHasScrolledUp flag remains true. This prevents auto-scrolling to new messages in the fresh conversation since the effect at lines 67-72 only calls scrollToBottom() when !userHasScrolledUp.current. The flag only resets when the user manually scrolls near the bottom or scrollToBottom() is explicitly called. Consider resetting this flag when the conversation changes—for instance, by detecting when messages.length === 0 within the messages effect, or by adding an explicit reset trigger.
🤖 Prompt for AI Agents
In frontend/app/aipanel/aipanelmessages.tsx around lines 67 to 72, the
userHasScrolledUp ref persists across conversation resets so auto-scroll is
blocked for new/cleared conversations; update the messages effect to detect a
conversation reset (e.g., messages.length === 0 or a conversationId change) and
reset userHasScrolledUp.current = false (or call scrollToBottom()) when a
new/empty conversation is detected so subsequent incoming messages will
auto-scroll as expected.
| { | ||
| elemtype: "iconbutton", | ||
| icon: "arrows-rotate", | ||
| title: "Refresh", | ||
| click: () => this.refreshCallback?.(), | ||
| }, | ||
| ] as IconButtonDecl[]; | ||
| } else if (!isCeView && mimeType) { | ||
| // For all other file types (text, code, etc.), add refresh button | ||
| return [ | ||
| { | ||
| elemtype: "iconbutton", | ||
| icon: "arrows-rotate", | ||
| title: "Refresh", | ||
| click: () => this.refreshCallback?.(), | ||
| }, | ||
| ] as IconButtonDecl[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "refreshCallback\s*=" frontend/app/view/preview/Repository: wavetermdev/waveterm
Length of output: 789
🏁 Script executed:
rg -n "refreshCallback" frontend/app/view/preview/ -A 3 -B 3Repository: wavetermdev/waveterm
Length of output: 9670
🏁 Script executed:
fd . frontend/app/view/preview -name "*.tsx" -o -name "*.ts" | head -20Repository: wavetermdev/waveterm
Length of output: 294
Fix unsafe refreshCallback invocations in preview-directory operations.
The refresh buttons correctly use optional chaining refreshCallback?.(), but internal directory operations directly invoke model.refreshCallback() without null safety. In preview-directory-utils.tsx (lines 132, 173) and preview-directory.tsx (lines 758, 818, 835), these direct calls will throw if the callback is null. While all specialized views initialize the callback in their useEffect hooks, the direct invocations should also use optional chaining to match the defensive pattern established in the button handlers.
🤖 Prompt for AI Agents
In frontend/app/view/preview/preview-model.tsx around lines 353-369, and in
preview-directory-utils.tsx (around lines 132 and 173) and preview-directory.tsx
(around lines 758, 818, 835), direct invocations of refreshCallback are unsafe
(they call model.refreshCallback() or this.refreshCallback() without null
checks); change those direct calls to use optional chaining (e.g.,
model.refreshCallback?.() or this.refreshCallback?.()) so the callback is only
invoked when present, matching the defensive pattern used in the icon button
handlers.
| const handleDrop = React.useCallback((e: React.DragEvent) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
|
|
||
| // Get files from the drop | ||
| const files = Array.from(e.dataTransfer.files); | ||
| if (files.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| console.log("Drop files:", files); | ||
|
|
||
| // Get the file path(s) using the Electron API | ||
| const paths = files.map((file: File) => { | ||
| try { | ||
| // Use the exposed Electron API to get the full path | ||
| const fullPath = getApi().getPathForFile(file); | ||
| console.log("File:", file.name, "-> Full path:", fullPath); | ||
| return fullPath; | ||
| } catch (err) { | ||
| console.error("Could not get path for file:", file.name, err); | ||
| return file.name; | ||
| } | ||
| }); | ||
|
|
||
| console.log("Paths to insert:", paths); | ||
|
|
||
| // Insert the path(s) into the terminal | ||
| // If multiple files, separate with spaces and quote if necessary | ||
| const pathString = paths.map(path => { | ||
| // Quote paths that contain spaces | ||
| if (path.includes(" ")) { | ||
| return `"${path}"`; | ||
| } | ||
| return path; | ||
| }).join(" "); | ||
|
|
||
| console.log("Final path string:", pathString); | ||
|
|
||
| // Send the path to the terminal | ||
| if (model.termRef.current && pathString) { | ||
| model.sendDataToController(pathString); | ||
| } | ||
| }, [model]); |
There was a problem hiding this comment.
Incomplete shell escaping and debug logs should be removed.
Two issues:
-
Shell metacharacters: Quoting paths with spaces using double quotes (line 391-393) is insufficient. Paths containing
",$,`, or\will cause shell parsing issues or unintended variable expansion. -
Debug logs: The
console.logstatements at lines 370, 377, 385, 397 should be removed for production.
Apply this diff for proper escaping and cleanup:
- console.log("Drop files:", files);
-
// Get the file path(s) using the Electron API
const paths = files.map((file: File) => {
try {
// Use the exposed Electron API to get the full path
const fullPath = getApi().getPathForFile(file);
- console.log("File:", file.name, "-> Full path:", fullPath);
return fullPath;
} catch (err) {
console.error("Could not get path for file:", file.name, err);
return file.name;
}
});
- console.log("Paths to insert:", paths);
-
// Insert the path(s) into the terminal
- // If multiple files, separate with spaces and quote if necessary
- const pathString = paths.map(path => {
- // Quote paths that contain spaces
- if (path.includes(" ")) {
- return `"${path}"`;
- }
- return path;
- }).join(" ");
+ // Shell-escape paths for safe insertion
+ const pathString = paths.map(path => {
+ // Use single quotes and escape any embedded single quotes
+ if (/["\s$`\\!*?#~<>|;&(){}[\]']/.test(path)) {
+ return "'" + path.replace(/'/g, "'\\''") + "'";
+ }
+ return path;
+ }).join(" ");
- console.log("Final path string:", pathString);
-
// Send the path to the terminal📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDrop = React.useCallback((e: React.DragEvent) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| // Get files from the drop | |
| const files = Array.from(e.dataTransfer.files); | |
| if (files.length === 0) { | |
| return; | |
| } | |
| console.log("Drop files:", files); | |
| // Get the file path(s) using the Electron API | |
| const paths = files.map((file: File) => { | |
| try { | |
| // Use the exposed Electron API to get the full path | |
| const fullPath = getApi().getPathForFile(file); | |
| console.log("File:", file.name, "-> Full path:", fullPath); | |
| return fullPath; | |
| } catch (err) { | |
| console.error("Could not get path for file:", file.name, err); | |
| return file.name; | |
| } | |
| }); | |
| console.log("Paths to insert:", paths); | |
| // Insert the path(s) into the terminal | |
| // If multiple files, separate with spaces and quote if necessary | |
| const pathString = paths.map(path => { | |
| // Quote paths that contain spaces | |
| if (path.includes(" ")) { | |
| return `"${path}"`; | |
| } | |
| return path; | |
| }).join(" "); | |
| console.log("Final path string:", pathString); | |
| // Send the path to the terminal | |
| if (model.termRef.current && pathString) { | |
| model.sendDataToController(pathString); | |
| } | |
| }, [model]); | |
| const handleDrop = React.useCallback((e: React.DragEvent) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| // Get files from the drop | |
| const files = Array.from(e.dataTransfer.files); | |
| if (files.length === 0) { | |
| return; | |
| } | |
| // Get the file path(s) using the Electron API | |
| const paths = files.map((file: File) => { | |
| try { | |
| // Use the exposed Electron API to get the full path | |
| const fullPath = getApi().getPathForFile(file); | |
| return fullPath; | |
| } catch (err) { | |
| console.error("Could not get path for file:", file.name, err); | |
| return file.name; | |
| } | |
| }); | |
| // Shell-escape paths for safe insertion | |
| const pathString = paths.map(path => { | |
| // Use single quotes and escape any embedded single quotes | |
| if (/["\s$`\\!*?#~<>|;&(){}[\]']/.test(path)) { | |
| return "'" + path.replace(/'/g, "'\\''") + "'"; | |
| } | |
| return path; | |
| }).join(" "); | |
| // Send the path to the terminal | |
| if (model.termRef.current && pathString) { | |
| model.sendDataToController(pathString); | |
| } | |
| }, [model]); |
🤖 Prompt for AI Agents
In frontend/app/view/term/term.tsx around lines 360 to 403, the drop handler
uses weak quoting and left debug logs; replace the ad-hoc double-quote wrapping
with proper shell-escaping for POSIX shells (wrap each path in single quotes and
escape any embedded single quotes by replacing ' with '\'' ), remove all
console.log/console.error debug statements introduced in this block, keep the
same behavior for multiple files (join escaped paths with spaces), and retain
the existing guard that sends the final escaped pathString to
model.sendDataToController only when termRef.current and pathString are present.
| type tabTitleTracker struct { | ||
| mu sync.Mutex | ||
| tabLineCounts map[string]int // tabId -> line count | ||
| lastTitleGenTime map[string]time.Time // tabId -> last time title was generated | ||
| } | ||
|
|
||
| var titleTracker = &tabTitleTracker{ | ||
| tabLineCounts: make(map[string]int), | ||
| lastTitleGenTime: make(map[string]time.Time), | ||
| } |
There was a problem hiding this comment.
Potential memory leak: map entries are never cleaned up.
The tabLineCounts and lastTitleGenTime maps grow as new tabs are created but entries are never removed when tabs close. Over long-running sessions, this could accumulate stale entries.
Consider adding a cleanup mechanism, such as:
- A method to remove entries when tabs are closed
- Periodic cleanup of entries older than a threshold
- Using the cooldown time to prune old entries during
CheckAndGenerateTitle
🤖 Prompt for AI Agents
In pkg/blockcontroller/tabtitle_trigger.go around lines 24 to 33, the
tabLineCounts and lastTitleGenTime maps never have entries removed, which can
leak memory for long-running processes; add a cleanup strategy: implement a
RemoveTab(tabID string) method that locks the tracker and deletes both map
entries and call that from tab-close logic, and/or prune old entries during
CheckAndGenerateTitle by iterating keys under the same mutex and deleting
entries whose lastTitleGenTime is older than a chosen TTL (for example the
cooldown or a separate threshold); ensure all map deletes happen while holding
t.mu to avoid races.
| titleTracker.mu.Lock() | ||
| titleTracker.tabLineCounts[tabId] += newlines | ||
| lineCount := titleTracker.tabLineCounts[tabId] | ||
| lastGenTime, exists := titleTracker.lastTitleGenTime[tabId] | ||
| titleTracker.mu.Unlock() | ||
|
|
||
| // Check if we've hit the threshold | ||
| if lineCount < LinesThresholdForTitle { | ||
| return | ||
| } | ||
|
|
||
| // Check cooldown period | ||
| if exists && time.Since(lastGenTime).Seconds() < TitleCooldownSeconds { | ||
| return | ||
| } | ||
|
|
||
| // Reset counter and update last gen time before generating (to prevent duplicates) | ||
| titleTracker.mu.Lock() | ||
| titleTracker.tabLineCounts[tabId] = 0 | ||
| titleTracker.lastTitleGenTime[tabId] = time.Now() | ||
| titleTracker.mu.Unlock() |
There was a problem hiding this comment.
Race condition: TOCTOU between check and state reset.
Between the first critical section (lines 63-67) and the second (lines 80-83), multiple goroutines could pass the threshold and cooldown checks simultaneously, leading to duplicate title generations. This is a time-of-check to time-of-use issue.
Consolidate into a single critical section:
// Update line count and check threshold
titleTracker.mu.Lock()
+ defer titleTracker.mu.Unlock()
+
titleTracker.tabLineCounts[tabId] += newlines
lineCount := titleTracker.tabLineCounts[tabId]
- lastGenTime, exists := titleTracker.lastTitleGenTime[tabId]
- titleTracker.mu.Unlock()
// Check if we've hit the threshold
if lineCount < LinesThresholdForTitle {
return
}
// Check cooldown period
+ lastGenTime, exists := titleTracker.lastTitleGenTime[tabId]
if exists && time.Since(lastGenTime).Seconds() < TitleCooldownSeconds {
return
}
// Reset counter and update last gen time before generating (to prevent duplicates)
- titleTracker.mu.Lock()
titleTracker.tabLineCounts[tabId] = 0
titleTracker.lastTitleGenTime[tabId] = time.Now()
- titleTracker.mu.Unlock()
// Generate title asynchronouslyCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/blockcontroller/tabtitle_trigger.go around lines 63 to 83: there is a
TOCTOU race where the code unlocks between checking lineCount/cooldown and
resetting state, allowing multiple goroutines to pass the checks and generate
duplicate titles; fix by consolidating the check and state update inside a
single mutex critical section — acquire the lock, read/update tabLineCounts and
lastTitleGenTime, compute whether threshold/cooldown allow generation, if
allowed reset the counter and set lastTitleGenTime = time.Now() while still
holding the lock, then release and proceed to generate the title outside the
lock if needed.
| func GenerateTabTitle(ctx context.Context, tabId string) (string, error) { | ||
| // Get the tab | ||
| tab, err := wstore.DBMustGet[*waveobj.Tab](ctx, tabId) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error getting tab: %w", err) | ||
| } | ||
|
|
||
| // If no blocks, return default | ||
| if len(tab.BlockIds) == 0 { | ||
| return "", fmt.Errorf("tab has no blocks") | ||
| } | ||
|
|
||
| // Get the first block (usually the primary terminal) | ||
| blockId := tab.BlockIds[0] | ||
| block, err := wstore.DBMustGet[*waveobj.Block](ctx, blockId) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error getting block: %w", err) | ||
| } | ||
|
|
||
| // Get the current working directory from block metadata | ||
| meta := waveobj.GetMeta(block) | ||
| cwd, ok := meta[waveobj.MetaKey_CmdCwd].(string) | ||
| if !ok || cwd == "" { | ||
| return "", fmt.Errorf("no working directory available") | ||
| } | ||
|
|
||
| // Generate title from the last 2 folders | ||
| title := generateTitleFromPath(cwd) | ||
| return title, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for BlockIds ordering and usage patterns
rg -n --type=go 'BlockIds\[0\]|BlockIds.*append' -A 2 -B 2Repository: wavetermdev/waveterm
Length of output: 1678
🏁 Script executed:
# Search for block type definitions and controller constants
rg -n --type=go 'BlockController_|const.*Block.*=' | head -20Repository: wavetermdev/waveterm
Length of output: 1439
🏁 Script executed:
# Search for tab creation and initialization logic
rg -n --type=go 'BlockIds\s*=' -A 3 -B 2 | head -30Repository: wavetermdev/waveterm
Length of output: 1737
🏁 Script executed:
# Check for any block type or ordering comments
rg -n --type=go 'primary|terminal|first.*block' -i | head -20Repository: wavetermdev/waveterm
Length of output: 1810
🏁 Script executed:
#!/bin/bash
# Search for tsunami block usage and metadata handling
rg -n --type=go 'BlockController_Tsunami' -A 3 -B 3 | head -40Repository: wavetermdev/waveterm
Length of output: 1470
🏁 Script executed:
# Search for MetaKey_CmdCwd usage to see which blocks use it
rg -n --type=go 'MetaKey_CmdCwd' -A 2 -B 2 | head -30Repository: wavetermdev/waveterm
Length of output: 2009
🏁 Script executed:
# Look for block creation order or any comments about block ordering
rg -n --type=go 'CreateBlock|AddBlock' -A 5 | head -40Repository: wavetermdev/waveterm
Length of output: 2713
🏁 Script executed:
# Check for any tests that show tab/block creation patterns
fd -e '_test.go' --type f | xargs grep -l 'BlockIds\|CreateBlock' | head -5Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for MakeTsunamiController usage to understand when tsunami blocks are created
rg -n --type=go 'MakeTsunamiController|BlockController_Tsunami' -B 5 -A 5Repository: wavetermdev/waveterm
Length of output: 3200
🏁 Script executed:
# Search for tab initialization to see if there's default block creation
rg -n --type=go 'func.*NewTab|CreateTab|Tab{' -A 10 | head -50Repository: wavetermdev/waveterm
Length of output: 181
🏁 Script executed:
# Look for any validation or ordering logic when adding blocks to tabs
rg -n --type=go 'BlockIds.*=|append.*BlockIds' -B 3 -A 3 | head -60Repository: wavetermdev/waveterm
Length of output: 3364
🏁 Script executed:
#!/bin/bash
# Search for where tsunami blocks are typically created
rg -n --type=go -i 'tsunami' -B 3 -A 3 | grep -E '(Create|Add|Block|Controller)' | head -40Repository: wavetermdev/waveterm
Length of output: 4083
🏁 Script executed:
# Look for comments or documentation about block ordering
rg -n --type=go 'first.*block|block.*order|primary' -i | head -20Repository: wavetermdev/waveterm
Length of output: 1727
🏁 Script executed:
# Check if there's any special handling for initial block creation
rg -n --type=go 'BlockIds\[0\]' -B 5 -A 2Repository: wavetermdev/waveterm
Length of output: 1126
🏁 Script executed:
#!/bin/bash
# Search for default tab/block creation to see what controller type is used initially
rg -n --type=go 'NewTab|CreateTab|initializ' -i -A 8 | head -60Repository: wavetermdev/waveterm
Length of output: 3772
🏁 Script executed:
# Look for the Block struct definition to see metadata types
rg -n --type=go 'type Block struct' -A 15 | head -30Repository: wavetermdev/waveterm
Length of output: 967
🏁 Script executed:
# Check if there's any defensive handling of missing cwd in callers
rg -n --type=go 'GenerateTabTitle' -B 2 -A 5Repository: wavetermdev/waveterm
Length of output: 3476
🏁 Script executed:
#!/bin/bash
# Search for how users/code creates blocks in tabs - look for block creation commands
rg -n --type=go 'CreateBlock|AddBlock' -B 5 -A 8 | head -80Repository: wavetermdev/waveterm
Length of output: 6246
🏁 Script executed:
# Look for any block type checking before accessing cwd
rg -n --type=go 'GetController|ControllerType|Meta\[' -A 2 | grep -A 2 'CmdCwd' | head -30Repository: wavetermdev/waveterm
Length of output: 46
Add defensive check for block type or working directory availability.
The function assumes tab.BlockIds[0] contains MetaKey_CmdCwd, but this is not guaranteed. Tsunami blocks (non-terminal visualization blocks) can be first in a tab and do not have working directory metadata. The code will fail with "no working directory available" if BlockIds[0] is a Tsunami or other non-terminal block. Consider iterating through blocks to find the first one with working directory metadata, or add validation to ensure the first block is a terminal-type controller (Shell/Cmd).
🤖 Prompt for AI Agents
In pkg/waveai/tabtitle.go around lines 17 to 46, the code assumes
tab.BlockIds[0] is a terminal block with MetaKey_CmdCwd and returns an error
when it isn’t; instead, iterate through tab.BlockIds and retrieve each block
until you find one whose metadata contains a non-empty MetaKey_CmdCwd (or
validate the block.Type is a terminal type like Shell/Cmd), then use that cwd to
generate the title; if no block with a cwd is found, return the existing "no
working directory available" error.
| // Try to inherit cwd from the active tab | ||
| var inheritedMeta waveobj.MetaMapType | ||
| if ws.ActiveTabId != "" && !isInitialLaunch { | ||
| activeTab, _ := wstore.DBGet[*waveobj.Tab](ctx, ws.ActiveTabId) | ||
| if activeTab != nil && len(activeTab.BlockIds) > 0 { | ||
| // Get the first block from the active tab | ||
| firstBlock, _ := wstore.DBGet[*waveobj.Block](ctx, activeTab.BlockIds[0]) | ||
| if firstBlock != nil { | ||
| meta := waveobj.GetMeta(firstBlock) | ||
| if cwd, ok := meta[waveobj.MetaKey_CmdCwd].(string); ok && cwd != "" { | ||
| // Inherit the cwd for the new tab | ||
| inheritedMeta = waveobj.MetaMapType{ | ||
| waveobj.MetaKey_CmdCwd: cwd, | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Don’t silently ignore DB errors when inheriting cwd.
Right now wstore.DBGet errors are dropped for both the active tab and its first block, which can make cwd inheritance flaky with no observability (and can mask real DB issues). Consider capturing err and logging at least at debug/info (or return error if this must be consistent).
🤖 Prompt for AI Agents
In pkg/wcore/workspace.go around lines 211-229 the errors returned by
wstore.DBGet for the active tab and the first block are being ignored; capture
the err return values for both calls, check if err != nil and log it (include
context like ws.ActiveTabId / block id and a clear message) at debug/info level
so DB problems are observable; only proceed with cwd inheritance when err == nil
and the object is non-nil, and if cwd inheritance must be strict consider
returning the error instead of just logging.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Update the tab name | ||
| err = wstore.UpdateTabName(genCtx, tabId, title) | ||
| if err != nil { | ||
| log.Printf("Error updating tab name for tab %s: %v", tabId, err) |
There was a problem hiding this comment.
Auto title generation now unconditionally calls wstore.UpdateTabName whenever a terminal block appends data (see HandleAppendBlockFile → CheckAndGenerateTitle), so any user-supplied tab name is replaced with the generated folder name after the next newline of output. Previously user-renamed tabs persisted; with this change they are silently reset, which is a user-facing regression for anyone who customizes tab titles. Consider skipping auto-generation when the tab name was manually set or adding an opt-out flag.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/view/term/termwrap.ts (1)
529-561: Add error handling for clipboard operations and use capture flag for copy event listenerThe
navigator.clipboard.writeText()call at line 541 can reject due to permissions or clipboard access constraints and is currently unhandled. Additionally, the copy event listener at line 553 should use the capture flag (third argumenttrue) to reliably intercept clipboard events before other handlers, matching the pattern already used for the paste handler on line [paste handler line].--- a/frontend/app/view/term/termwrap.ts +++ b/frontend/app/view/term/termwrap.ts @@ - if (markdownText.length > 0) { - navigator.clipboard.writeText(markdownText); - } + if (markdownText.length > 0) { + navigator.clipboard.writeText(markdownText).catch((err) => { + dlog("clipboard write failed", err); + }); + } @@ - this.connectElem.addEventListener("copy", copyHandler); + this.connectElem.addEventListener("copy", copyHandler, true); @@ - this.connectElem.removeEventListener("copy", copyHandler); + this.connectElem.removeEventListener("copy", copyHandler, true);
♻️ Duplicate comments (3)
frontend/app/aipanel/aipanelmessages.tsx (1)
70-75: Edge case:userHasScrolledUppersists across conversation resets.When the user clears the chat or starts a new conversation,
userHasScrolledUpremainstrueif they had scrolled up previously. This blocks auto-scroll for the new conversation. Consider resetting the flag whenmessages.length === 0:useEffect(() => { + // Reset scroll flag when conversation is cleared + if (messages.length === 0) { + userHasScrolledUp.current = false; + } // Only auto-scroll if user hasn't manually scrolled up if (!userHasScrolledUp.current) { scrollToBottom(); } }, [messages]);pkg/wcore/workspace.go (1)
211-228: DB errors still silently ignored during cwd inheritance.The errors from
wstore.DBGeton lines 214 and 217 are still being dropped without logging, which matches the earlier review feedback. While the inheritance is best-effort, capturing and logging these errors (even at debug level) would improve observability and help catch DB issues during development.Apply this diff to add basic error logging:
// Try to inherit cwd from the active tab var inheritedMeta waveobj.MetaMapType if ws.ActiveTabId != "" && !isInitialLaunch { - activeTab, _ := wstore.DBGet[*waveobj.Tab](ctx, ws.ActiveTabId) + activeTab, err := wstore.DBGet[*waveobj.Tab](ctx, ws.ActiveTabId) + if err != nil { + log.Printf("error fetching active tab %s for cwd inheritance: %v", ws.ActiveTabId, err) + } if activeTab != nil && len(activeTab.BlockIds) > 0 { // Get the first block from the active tab - firstBlock, _ := wstore.DBGet[*waveobj.Block](ctx, activeTab.BlockIds[0]) + firstBlock, err := wstore.DBGet[*waveobj.Block](ctx, activeTab.BlockIds[0]) + if err != nil { + log.Printf("error fetching block %s for cwd inheritance: %v", activeTab.BlockIds[0], err) + } if firstBlock != nil { meta := waveobj.GetMeta(firstBlock) if cwd, ok := meta[waveobj.MetaKey_CmdCwd].(string); ok && cwd != "" {frontend/app/view/term/termwrap.ts (1)
31-84: Bold detection is incorrect (uses color mode, not style); also verify selection bounds & avoid lossy trimming
The currentcellBoldcalculation readsgetBgColorMode/getFgColorMode(Line 58), which represents color mode (default/palette/RGB), not the bold attribute—so markdown**...**will be applied incorrectly. Also, theendCol/loop bound likely drops the last selected cell depending on whetherselection.end.xis inclusive, andtrimEnd()(Line 77) can remove meaningful spaces from terminal output.--- a/frontend/app/view/term/termwrap.ts +++ b/frontend/app/view/term/termwrap.ts @@ - // Check if cell has bold attribute (bit 0 of fg color flags) - const cellBold = (cell.getBgColorMode() & 0x01) !== 0 || (cell.getFgColorMode() & 0x01) !== 0; + // Bold is a cell attribute; color *mode* is unrelated. + const cellBold = cell.isBold() === 1; @@ - markdown += lineText.trimEnd(); + // Avoid trimming by default; terminal selections often rely on trailing spaces. + markdown += lineText;Verification items:
- Confirm @xterm/xterm 5.5.0
IBufferCell.isBold(): numberis available and the correct predicate.- Confirm whether
selection.end.xis inclusive; if it is,endColshould beselection.end.x + 1to avoid dropping the last cell.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/aipanel/aipanelmessages.tsx(3 hunks)frontend/app/tab/tabbar.tsx(1 hunks)frontend/app/view/term/termwrap.ts(3 hunks)frontend/types/custom.d.ts(1 hunks)pkg/wcore/workspace.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/types/custom.d.ts
- frontend/app/tab/tabbar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/aipanel/aipanelmessages.tsx (1)
frontend/app/aipanel/waveai-model.tsx (1)
scrollToBottom(288-290)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (8)
frontend/app/aipanel/aipanelmessages.tsx (6)
11-12: LGTM!Well-defined constants with clear naming. The 100ms debounce and 50px threshold are reasonable values for scroll detection.
26-27: LGTM!Using refs for scroll state is appropriate since changes don't need to trigger re-renders.
29-40: LGTM!The auto-scroll tracking pattern correctly prevents the scroll handler from misinterpreting programmatic scrolls as user actions.
42-64: LGTM!The scroll detection is well-implemented with passive listener for performance, proper cleanup, and correct distance-from-bottom calculation.
77-81: LGTM!Correctly respects user scroll position when panel opens.
83-94: LGTM!The streaming completion scroll is well-implemented with
requestAnimationFrametiming and proper respect for user scroll position.pkg/wcore/workspace.go (2)
202-205: LGTM!Good practice to fetch and validate the workspace early, failing fast if the workspace doesn't exist.
244-254: LGTM!The logic to merge inherited metadata into the new tab layout is correct. The array bounds and nil map checks are properly handled.
| handleResize() { | ||
| const oldRows = this.terminal.rows; | ||
| const oldCols = this.terminal.cols; | ||
|
|
||
| // Preserve scroll position before resize | ||
| const wasAtBottom = this.terminal.buffer.active.baseY + this.terminal.rows >= this.terminal.buffer.active.length; | ||
| const scrollY = this.terminal.buffer.active.viewportY; | ||
|
|
||
| this.fitAddon.fit(); | ||
|
|
||
| // Restore scroll position after resize | ||
| if (!wasAtBottom && scrollY > 0) { | ||
| // If user wasn't at bottom, try to keep them at the same content | ||
| setTimeout(() => { | ||
| this.terminal.scrollToLine(scrollY); | ||
| }, 0); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@xterm/xterm 5.5.0 scrollToLine API documentation absolute buffer line
💡 Result:
The Terminal.scrollToLine(line: number) method takes a 0‑based absolute buffer line index (not a viewport row). Call terminal.scrollToLine(y) with y being the buffer line you want shown at the top of the viewport. [1][2]
Sources:
- Terminal API (scrollToLine). [1]
- Buffer coordinate explanation (ybase / ydisp = buffer absolute indices). [2]
🌐 Web query:
xterm.js buffer viewportY baseY bottom detection scroll restoration
💡 Result:
Short answer
- viewportY (ydisp) = top line index shown in the viewport. baseY (ybase) = index of the buffer line that represents the bottom page (the logical bottom). You're at the bottom when viewportY === baseY. [1][2]
How to detect & preserve/restore scroll state
- Track scroll changes with terminal.onScroll(fn) to get the current viewport value. If fn receives n, that n is the new viewportY. Use this to detect user scroll-away or return-to-bottom. [3]
- To test "at bottom": read terminal.buffer.active.viewportY (or buffer.normal/alt as appropriate) and compare to terminal.buffer.active.baseY. If equal → at bottom. [1]
- To keep the terminal pinned to bottom when new data arrives: before writing, check if at bottom; if true, write and then call terminal.scrollToBottom(); if false, leave viewport alone (or restore saved viewportY via terminal.scrollToLine(savedViewportY)). Example:
- isAtBottom = terminal.buffer.active.viewportY === terminal.buffer.active.baseY
- term.write(data)
- if (isAtBottom) term.scrollToBottom() else term.scrollToLine(savedViewportY)
API: scrollToBottom(), scrollToLine(line). [2]
Notes / caveats
- There are two buffers (normal & alternate); use the active buffer you care about (terminal.buffer.active / terminal.buffer.normal / .alt). [1]
- onScroll historically had edge-cases in old versions; if you see missing events test with your xterm.js version and renderer. [3]
Sources
- xterm.js IBuffer docs (viewportY, baseY). [https://xtermjs.org/docs/api/terminal/interfaces/ibuffer/] [1]
- xterm.js Terminal API (scrollToBottom, scrollToLine, scrollLines, buffer access). [https://xtermjs.org/docs/api/terminal/classes/terminal/] [2]
- Discussion/issue about onScroll behavior (notes about user-scroll edge cases). [https://lightrun.com/answers/xtermjs-xterm-js-onscroll-doesnt-emit-when-user-is-scrolling] [3]
Resize scroll preservation: bottom detection and restoration condition are incorrect
The bottom-of-buffer check should use viewportY === baseY (line 777); xterm.js defines "at bottom" when the viewport top equals the buffer bottom index. The current formula baseY + rows >= length doesn't match xterm semantics.
Additionally, the condition scrollY > 0 (line 782) skips restoration for users at the very top (scrollY === 0), breaking scroll preservation for that case. Remove the scrollY > 0 check entirely, as scrollToLine(viewportY) correctly restores any saved viewport position (including 0).
--- a/frontend/app/view/term/termwrap.ts
+++ b/frontend/app/view/term/termwrap.ts
@@
- const wasAtBottom = this.terminal.buffer.active.baseY + this.terminal.rows >= this.terminal.buffer.active.length;
+ const wasAtBottom = this.terminal.buffer.active.viewportY === this.terminal.buffer.active.baseY;
const scrollY = this.terminal.buffer.active.viewportY;
@@
- if (!wasAtBottom && scrollY > 0) {
+ if (!wasAtBottom) {
// If user wasn't at bottom, try to keep them at the same content
setTimeout(() => {
this.terminal.scrollToLine(scrollY);
}, 0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handleResize() { | |
| const oldRows = this.terminal.rows; | |
| const oldCols = this.terminal.cols; | |
| // Preserve scroll position before resize | |
| const wasAtBottom = this.terminal.buffer.active.baseY + this.terminal.rows >= this.terminal.buffer.active.length; | |
| const scrollY = this.terminal.buffer.active.viewportY; | |
| this.fitAddon.fit(); | |
| // Restore scroll position after resize | |
| if (!wasAtBottom && scrollY > 0) { | |
| // If user wasn't at bottom, try to keep them at the same content | |
| setTimeout(() => { | |
| this.terminal.scrollToLine(scrollY); | |
| }, 0); | |
| } | |
| handleResize() { | |
| const oldRows = this.terminal.rows; | |
| const oldCols = this.terminal.cols; | |
| // Preserve scroll position before resize | |
| const wasAtBottom = this.terminal.buffer.active.viewportY === this.terminal.buffer.active.baseY; | |
| const scrollY = this.terminal.buffer.active.viewportY; | |
| this.fitAddon.fit(); | |
| // Restore scroll position after resize | |
| if (!wasAtBottom) { | |
| // If user wasn't at bottom, try to keep them at the same content | |
| setTimeout(() => { | |
| this.terminal.scrollToLine(scrollY); | |
| }, 0); | |
| } |
🤖 Prompt for AI Agents
In frontend/app/view/term/termwrap.ts around lines 772 to 788, the
bottom-of-buffer detection and restoration guard are incorrect: replace the
current bottom check (baseY + rows >= length) with the xterm.js semantic check
(viewportY === baseY) to determine "at bottom", and remove the `scrollY > 0`
condition so restoration runs even when viewportY is 0; keep the setTimeout(()
=> this.terminal.scrollToLine(scrollY), 0) to restore the saved viewport
position after fit().
Implements LLM-based tab title generation that automatically creates meaningful, short tab titles based on terminal activity. Features: - Analyzes last 4KB of terminal output using configured AI backend - Auto-triggers after 10 lines of terminal output - Respects 14-character UI limit with optimized prompt - 5-minute cooldown to prevent excessive regeneration - Only auto-generates for default tab names (T1, T2, etc.) - Works with Vertex AI Anthropic, OpenAI, and other backends Example titles: npm install → NPM Setup, git commit → Git Commit New files: - pkg/waveai/tabtitle.go: Core title generation logic - pkg/blockcontroller/tabtitle_trigger.go: Automatic trigger system 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Implements drag-and-drop functionality for terminal, matching macOS Terminal behavior. Users can now drag files or folders from Finder directly into the terminal to insert their paths. Features: - Automatically inserts file paths when files are dropped - Handles multiple files (space-separated) - Auto-quotes paths containing spaces - Uses full file paths from Electron File API - Works with files, folders, and multiple selections Usage: Simply drag a file from Finder and drop it into any terminal block. The file path will be inserted at the cursor position. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Use Electron's webUtils.getPathForFile() API to get the actual full file path instead of just the file name. This matches macOS Terminal behavior where dragging a file inserts its complete path. Before: CV_Document.pdf After: /Users/steven/Downloads/CV_Document.pdf 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Expose webUtils.getPathForFile through the Electron preload API so the renderer process can get full file system paths from File objects during drag-and-drop operations. Changes: - Added getPathForFile method to preload script using webUtils - Updated ElectronApi TypeScript interface - Simplified terminal drag-drop to use new API via getApi() This properly implements macOS Terminal-style drag-and-drop with full file paths instead of just filenames. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Added missing import of getApi from @/store/global to fix 'getApi is not defined' error when dropping files. Also added debug logging to help troubleshoot file path retrieval. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
- Replace AI-powered tab title generation with simple folder name from cwd - Show last folder name from current working directory as tab title - Reduce auto-title trigger threshold from 10 to 1 line for immediate updates - Remove default-name-only restriction (updates any tab name) - Reduce cooldown from 5 minutes to 10 seconds for faster updates - Add manual refresh button to all file preview types (code, markdown, PDFs, images) - Inherit working directory when creating new tabs (Cmd+T preserves cwd) - Fix terminal paste scroll position jumping to top 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…kdown copy - Fix Electron name in menu bar (remove waveterm/electron setName) - Prevent AI panel auto-scroll when user manually scrolls up to read - Add confirmation dialog before closing tabs to prevent accidental closures - Preserve terminal scroll position during resize and window changes - Add markdown formatting to terminal copy (converts bold text to **bold**) - Terminal copy now preserves formatting when pasting into markdown editors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When opening the file browser from a terminal block, it now starts in the terminal's current working directory instead of always defaulting to the home directory. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add default-light theme (white background, black text) similar to macOS Terminal - Change default theme from default-dark to default-light - Fix file browser to start in terminal's current working directory 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Changed from github-dark-dimmed to github (light) theme for better readability and GitHub-style code block appearance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This reverts commit bf6928d74dbef09a4fd85cf4d5dd786cae7fdf6c.
This reverts commit d6b685a94521b960b204c96f5dd52d02198400ab.
Added bg@white preset for a clean white background (100% opacity). Appears first in the Backgrounds menu after Default. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Added macOS-style light theme with black text on white background, matching the default macOS Terminal.app appearance. Use this theme together with the White background preset for a clean light terminal. To use: Right-click tab → select a light theme when it's available in UI 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Changed default font from Hack to SF Mono (with fallbacks to Menlo, Monaco) - Changed default font size from 12pt to 11pt to match macOS Terminal.app - Font stack: SF Mono, Menlo, Monaco, Courier New, monospace 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Disabled transparent background override in terminal - Terminal now uses the theme's own background color - macOS Basic theme will show its white background - Default background preset changed to white - Removes dependency on background presets for basic white terminal 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Changed font weight from 'normal'/'bold' to 400/600 - Added line height 1.2 for better spacing - Better matches macOS Terminal's font rendering 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Changed bold font weight from 600 to 700 to make bold text more prominent and closer to macOS Terminal appearance. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This reverts commit b92c35730fa1d8b3a6611b4d3fd044320d88e918.
This reverts commit c217bcddabb94d21918750b4363a3134c1a5fd52.
This reverts commit 445f0ef75926e3797fbce1b04a229e9fe8aa8f61.
…nal" This reverts commit 4cb6e9337a67d77ea361f6c470d3bf87be84e340.
This reverts commit 2a2777b8ce94d9b776fadbf7e29c611d6a18d125.
This reverts commit 468dbfbd7648e216f6743593564e371c10b6c75b.
Prevent unwanted scroll-to-top when commands start executing (like Claude Code). Preserves scroll position if user has scrolled up, while still allowing auto-scroll when at bottom. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This reverts commit 8f313955259f7da7cc65d19c27675843bdc9c2c5.
- Cmd+Down: Scroll to bottom of terminal - Cmd+Up: Scroll to top of terminal - Quick navigation for long terminal output 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Removed the AI sparkle button from the tab bar. AI panel can still be accessed via other methods if needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Auto-generated TypeScript and Go RPC client code after rebase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This reverts commit a10900e.
This reverts commit 589cc0b.
- Optimize CreateTab to avoid double workspace fetch (pass workspace to createTabObj) - Remove paste scroll preservation (user wants auto-scroll to bottom on paste) - Add constants for magic numbers in AI panel scroll (AUTO_SCROLL_DEBOUNCE_MS, SCROLL_BOTTOM_THRESHOLD_PX) - Add passive event listener for better scroll performance - Update comment in custom.d.ts to clarify getPathForFile implementation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
stevenvo
force-pushed
the
main
branch
2 times, most recently
from
be96b08 to
18ab50f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/types/custom.d.ts (1)
77-134: Update type signature to account for empty string return value.
The API returns an empty string for JS-constructed or non-disk-backed File objects (not an error); the current typing(file: File) => stringshould reflect this is always a valid return rather than implying a path is guaranteed. Consider clarifying the return value semantics or updating the type to(file: File) => stringwith a comment noting empty string indicates a non-disk-backed file.
♻️ Duplicate comments (8)
frontend/app/aipanel/aipanelmessages.tsx (1)
70-75: Edge case:userHasScrolledUppersists across conversation resets (blocks auto-scroll in new chat).
This still appears: clearingmessageswon’t resetuserHasScrolledUp.current, so a new conversation may never auto-scroll until the user manually scrolls near bottom.pkg/blockcontroller/tabtitle_trigger.go (3)
62-84: Fix TOCTOU race that can trigger duplicate title generations (extra AI calls + churn).
Current lock split lets multiple goroutines pass the threshold/cooldown check before the reset happens. Consolidate “check + state update” into one critical section and only spawn the goroutine ifshouldGenerate == true.// Update line count and check threshold - titleTracker.mu.Lock() - titleTracker.tabLineCounts[tabId] += newlines - lineCount := titleTracker.tabLineCounts[tabId] - lastGenTime, exists := titleTracker.lastTitleGenTime[tabId] - titleTracker.mu.Unlock() - - // Check if we've hit the threshold - if lineCount < LinesThresholdForTitle { - return - } - - // Check cooldown period - if exists && time.Since(lastGenTime).Seconds() < TitleCooldownSeconds { - return - } - - // Reset counter and update last gen time before generating (to prevent duplicates) - titleTracker.mu.Lock() - titleTracker.tabLineCounts[tabId] = 0 - titleTracker.lastTitleGenTime[tabId] = time.Now() - titleTracker.mu.Unlock() + now := time.Now() + shouldGenerate := false + titleTracker.mu.Lock() + titleTracker.tabLineCounts[tabId] += newlines + lineCount := titleTracker.tabLineCounts[tabId] + lastGenTime, exists := titleTracker.lastTitleGenTime[tabId] + if lineCount >= LinesThresholdForTitle && + (!exists || now.Sub(lastGenTime) >= time.Duration(TitleCooldownSeconds)*time.Second) { + titleTracker.tabLineCounts[tabId] = 0 + titleTracker.lastTitleGenTime[tabId] = now + shouldGenerate = true + } + titleTracker.mu.Unlock() + + if !shouldGenerate { + return + }
23-33: Prevent unbounded growth oftabLineCounts/lastTitleGenTime(stale tabs leak).
Entries are never removed, so long-running sessions can accumulate dead tab IDs. Add a cleanup API (e.g.,RemoveTab(tabId string)) called on tab close, and/or opportunistic TTL pruning under the same mutex.
85-104: Don’t overwrite user-renamed tab titles (user-facing regression).
This always callswstore.UpdateTabName(...)once the trigger fires, which can silently replace a manual tab name after any newline of output. Please gate auto-updates (e.g., only if the tab name is empty/default/auto-managed, or if an “autoTitleEnabled / userRenamed” flag says it’s allowed). Also consider skipping updates whentitle == ""or when it’s unchanged to avoid churn.title, err := waveai.GenerateTabTitle(genCtx, tabId) if err != nil { log.Printf("Error generating tab title for tab %s: %v", tabId, err) return } + if title == "" { + return + } - // Update the tab name - err = wstore.UpdateTabName(genCtx, tabId, title) + // TODO: only update if tab title is still auto-managed / not user-renamed. + err = wstore.UpdateTabName(genCtx, tabId, title)In waveterm, where is the “tab name” stored and is there any field/flag indicating it was set by the user vs auto-generated? Search for UpdateTabName implementation and Tab model fields related to name/title.pkg/wcore/workspace.go (1)
211-228: DB errors silently discarded during cwd inheritance.The errors from
wstore.DBGetcalls on lines 214 and 217 are discarded, which can mask real database issues and make cwd inheritance failures unobservable. Consider capturing and logging these errors at debug level.frontend/app/view/term/term.tsx (1)
352-403: Remove debug logs and shell-escape dropped paths before inserting.const handleDrop = React.useCallback((e: React.DragEvent) => { e.preventDefault(); e.stopPropagation(); @@ - console.log("Drop files:", files); - // Get the file path(s) using the Electron API const paths = files.map((file: File) => { try { // Use the exposed Electron API to get the full path const fullPath = getApi().getPathForFile(file); - console.log("File:", file.name, "-> Full path:", fullPath); return fullPath; } catch (err) { - console.error("Could not get path for file:", file.name, err); return file.name; } }); - - console.log("Paths to insert:", paths); // Insert the path(s) into the terminal - // If multiple files, separate with spaces and quote if necessary - const pathString = paths.map(path => { - // Quote paths that contain spaces - if (path.includes(" ")) { - return `"${path}"`; - } - return path; - }).join(" "); - - console.log("Final path string:", pathString); + // Shell-escape for POSIX shells (best-effort) + const pathString = paths + .map((path) => { + // Wrap in single quotes and escape embedded single quotes: ' -> '\''. + if (/["\s$`\\!*?#~<>|;&(){}[\]']/.test(path)) { + return "'" + path.replace(/'/g, "'\\''") + "'"; + } + return path; + }) + .join(" "); // Send the path to the terminal if (model.termRef.current && pathString) { model.sendDataToController(pathString); } }, [model]);frontend/app/view/term/termwrap.ts (2)
31-84: Bold detection is using the wrong xterm.js API.- // Check if cell has bold attribute (bit 0 of fg color flags) - const cellBold = (cell.getBgColorMode() & 0x01) !== 0 || (cell.getFgColorMode() & 0x01) !== 0; + // Check if cell has bold attribute + const cellBold = cell.isBold() === 1;xterm.js 5.5.0 IBufferCell isBold() API and meaning of getBgColorMode/getFgColorMode
772-788: Resize scroll preservation: bottom detection + restoration guard are incorrect.- const wasAtBottom = this.terminal.buffer.active.baseY + this.terminal.rows >= this.terminal.buffer.active.length; + const wasAtBottom = this.terminal.buffer.active.viewportY === this.terminal.buffer.active.baseY; const scrollY = this.terminal.buffer.active.viewportY; @@ - if (!wasAtBottom && scrollY > 0) { + if (!wasAtBottom) { // If user wasn't at bottom, try to keep them at the same content setTimeout(() => { this.terminal.scrollToLine(scrollY); }, 0); }xterm.js Terminal buffer semantics: viewportY/baseY and when "at bottom" is true; scrollToLine() expects absolute buffer line index
🧹 Nitpick comments (7)
frontend/app/aipanel/aipanelmessages.tsx (2)
29-40: Debounce viasetTimeoutcan stack and misclassify auto-scroll windows.
IfscrollToBottom()is called repeatedly (streaming updates), multiple timeouts race to flipisAutoScrolling.current = false, which can re-enable user scroll detection too early/late.+ const autoScrollTimeoutRef = useRef<number | null>(null); + const scrollToBottom = () => { const container = messagesContainerRef.current; if (container) { isAutoScrolling.current = true; container.scrollTop = container.scrollHeight; container.scrollLeft = 0; userHasScrolledUp.current = false; - setTimeout(() => { + if (autoScrollTimeoutRef.current != null) { + window.clearTimeout(autoScrollTimeoutRef.current); + } + autoScrollTimeoutRef.current = window.setTimeout(() => { isAutoScrolling.current = false; + autoScrollTimeoutRef.current = null; }, AUTO_SCROLL_DEBOUNCE_MS); } };
42-65: Set initialuserHasScrolledUpstate on mount (don’t wait for first scroll event).
Right nowuserHasScrolledUp.currentstaysfalseuntil the user scrolls, even if the container starts far from bottom (e.g., restored scroll position).const handleScroll = () => { // Ignore scroll events triggered by our auto-scroll if (isAutoScrolling.current) return; const { scrollTop, scrollHeight, clientHeight } = container; const distanceFromBottom = scrollHeight - scrollTop - clientHeight; // If user is more than threshold from the bottom, they've scrolled up if (distanceFromBottom > SCROLL_BOTTOM_THRESHOLD_PX) { userHasScrolledUp.current = true; } else { userHasScrolledUp.current = false; } }; container.addEventListener("scroll", handleScroll, { passive: true }); + handleScroll(); return () => container.removeEventListener("scroll", handleScroll); }, []);pkg/blockcontroller/tabtitle_trigger.go (1)
18-21: Prefertime.Durationconstants for cooldown/threshold config.
Avoid float math (Seconds()) and make the unit explicit (also makes future refactors safer).const ( LinesThresholdForTitle = 1 // Generate title after N lines of output - TitleCooldownSeconds = 10 // Don't regenerate title more often than every 10 seconds + TitleCooldown = 10 * time.Second // Don't regenerate title more often than every 10 seconds )frontend/app/modals/confirmclosetab.tsx (1)
29-30: Reconsider the "cannot be undone" message.The message states "This action cannot be undone," which may be overly dramatic for closing a tab. In most terminal applications, tabs can be reopened or recreated, and the workspace state is typically preserved.
Consider softening the language:
- Are you sure you want to close this tab? This action cannot be undone. + Are you sure you want to close this tab?pkg/wcore/workspace.go (1)
273-295: DB operation errors not propagated increateTabObjWithWorkspace.Lines 291-293 ignore errors from
DBInsertandDBUpdate. While the function signature allows returning errors, failures in tab/layout creation or workspace update would go unnoticed, potentially leaving the database in an inconsistent state.Consider propagating these errors:
- wstore.DBInsert(ctx, tab) - wstore.DBInsert(ctx, layoutState) - wstore.DBUpdate(ctx, ws) + if err := wstore.DBInsert(ctx, tab); err != nil { + return nil, fmt.Errorf("error inserting tab: %w", err) + } + if err := wstore.DBInsert(ctx, layoutState); err != nil { + return nil, fmt.Errorf("error inserting layout state: %w", err) + } + if err := wstore.DBUpdate(ctx, ws); err != nil { + return nil, fmt.Errorf("error updating workspace: %w", err) + } return tab, nilpkg/wshrpc/wshrpctypes.go (1)
189-192: Good RPC surface addition; prefer using the constant end-to-end.
Consider switching any client callsites using"generatetabtitle"towshrpc.Command_GenerateTabTitleto avoid drift.Also applies to: 199-268
frontend/app/view/term/termwrap.ts (1)
529-561: Copy interception: consider also settingtext/markdown(optional).
Some targets prefer the explicit MIME type when present.const copyHandler = (e: ClipboardEvent) => { const selection = this.terminal.getSelection(); if (selection.length > 0) { e.preventDefault(); const markdownText = convertSelectionToMarkdown(this.terminal); e.clipboardData?.setData("text/plain", markdownText); + e.clipboardData?.setData("text/markdown", markdownText); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
emain/emain-platform.ts(1 hunks)emain/preload.ts(2 hunks)frontend/app/aipanel/aipanelmessages.tsx(3 hunks)frontend/app/modals/confirmclosetab.tsx(1 hunks)frontend/app/modals/modalregistry.tsx(2 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/tab/tabbar.tsx(1 hunks)frontend/app/view/preview/preview-edit.tsx(1 hunks)frontend/app/view/preview/preview-markdown.tsx(1 hunks)frontend/app/view/preview/preview-model.tsx(2 hunks)frontend/app/view/preview/preview-streaming.tsx(2 hunks)frontend/app/view/term/term-model.ts(1 hunks)frontend/app/view/term/term.tsx(2 hunks)frontend/app/view/term/termwrap.ts(3 hunks)frontend/types/custom.d.ts(1 hunks)frontend/types/gotypes.d.ts(2 hunks)pkg/blockcontroller/blockcontroller.go(1 hunks)pkg/blockcontroller/tabtitle_trigger.go(1 hunks)pkg/waveai/tabtitle.go(1 hunks)pkg/wcore/workspace.go(4 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(2 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- emain/preload.ts
- pkg/blockcontroller/blockcontroller.go
- pkg/wshrpc/wshclient/wshclient.go
- frontend/app/tab/tabbar.tsx
- emain/emain-platform.ts
- pkg/waveai/tabtitle.go
- frontend/types/gotypes.d.ts
- frontend/app/view/preview/preview-markdown.tsx
- frontend/app/store/wshclientapi.ts
- frontend/app/view/preview/preview-streaming.tsx
- frontend/app/view/term/term-model.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/app/view/term/term.tsx
🧬 Code graph analysis (6)
pkg/wshrpc/wshserver/wshserver.go (4)
pkg/wshrpc/wshclient/barerpcclient.go (2)
WshServer(14-14)WshServer(16-16)frontend/app/store/wshclientapi.ts (1)
GenerateTabTitleCommand(296-298)pkg/wshrpc/wshclient/wshclient.go (1)
GenerateTabTitleCommand(360-363)pkg/waveai/tabtitle.go (1)
GenerateTabTitle(17-46)
frontend/app/modals/modalregistry.tsx (1)
frontend/app/modals/confirmclosetab.tsx (1)
ConfirmCloseTabModal(39-39)
pkg/blockcontroller/tabtitle_trigger.go (4)
pkg/wstore/wstore_dbops.go (1)
DBGet(133-136)pkg/waveobj/waveobj.go (1)
ParseORef(80-98)pkg/waveobj/wtype.go (1)
OType_Tab(28-28)pkg/waveai/tabtitle.go (1)
GenerateTabTitle(17-46)
frontend/app/view/preview/preview-model.tsx (2)
frontend/layout/lib/nodeRefMap.ts (1)
get(20-24)frontend/util/util.ts (1)
isBlank(518-518)
frontend/app/view/term/term.tsx (1)
frontend/app/store/global.ts (1)
getApi(852-852)
frontend/app/aipanel/aipanelmessages.tsx (1)
frontend/app/aipanel/waveai-model.tsx (1)
scrollToBottom(288-290)
🔇 Additional comments (8)
frontend/app/aipanel/aipanelmessages.tsx (1)
66-69: VerifyregisterScrollToBottomlifecycle (missing unregister/cleanup).
IfWaveAIModel.registerScrollToBottom()doesn’t safely overwrite/cleanup, this can retain stale callbacks across mounts.frontend/app/modals/modalregistry.tsx (1)
10-10: LGTM! Modal registration follows the established pattern.The import and registry entry for
ConfirmCloseTabModalcorrectly follow the existing pattern used by other modals in this file.Also applies to: 19-19
pkg/wcore/workspace.go (2)
202-205: LGTM!Pre-fetching the workspace early in
CreateTabis necessary for the cwd inheritance logic and follows the existing error handling pattern.
231-231: LGTM!Correctly passes the pre-fetched workspace and inherited metadata to avoid redundant database lookups.
frontend/app/view/preview/preview-edit.tsx (1)
64-74: Refresh callback wiring looks correct and cleaned up properly.
Hookingmodel.refreshCallbackto bumpmodel.refreshVersionand clearing it on unmount is consistent with the new refresh flow.frontend/app/view/preview/preview-model.tsx (3)
321-372: Refresh button logic is consistent and null-safe.
Usingthis.refreshCallback?.()in the new header buttons is the right defensive pattern.
373-385: CWD fallback formetaFilePathmatches the UX goal.
Defaulting tocmd:cwdwhen no file is set should make “open file” flows land in the terminal’s current directory.
432-453:refreshVersionsubscription is the right mechanism to force re-fetch.
Subscribing insidefullFileAtomwill reliably triggerFileReadCommandon refresh.
| const handleConfirmClose = () => { | ||
| const ws = globalStore.get(atoms.workspace); | ||
| getApi().closeTab(ws.oid, tabId); | ||
| deleteLayoutModelForTab(tabId); | ||
| modalsModel.popModal(); | ||
| }; |
There was a problem hiding this comment.
Add error handling and await async operations.
The closeTab API call is likely asynchronous but isn't awaited, causing the modal to close immediately regardless of whether the operation succeeds. If the API call fails, the user won't receive feedback.
Additionally, there's no validation that the workspace exists or that the tabId is valid.
Consider this refactor:
- const handleConfirmClose = () => {
+ const handleConfirmClose = async () => {
+ try {
const ws = globalStore.get(atoms.workspace);
- getApi().closeTab(ws.oid, tabId);
+ if (!ws?.oid || !tabId) {
+ console.error("Invalid workspace or tabId");
+ modalsModel.popModal();
+ return;
+ }
+ await getApi().closeTab(ws.oid, tabId);
deleteLayoutModelForTab(tabId);
modalsModel.popModal();
+ } catch (error) {
+ console.error("Failed to close tab:", error);
+ // Consider showing an error message to the user
+ modalsModel.popModal();
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleConfirmClose = () => { | |
| const ws = globalStore.get(atoms.workspace); | |
| getApi().closeTab(ws.oid, tabId); | |
| deleteLayoutModelForTab(tabId); | |
| modalsModel.popModal(); | |
| }; | |
| const handleConfirmClose = async () => { | |
| try { | |
| const ws = globalStore.get(atoms.workspace); | |
| if (!ws?.oid || !tabId) { | |
| console.error("Invalid workspace or tabId"); | |
| modalsModel.popModal(); | |
| return; | |
| } | |
| await getApi().closeTab(ws.oid, tabId); | |
| deleteLayoutModelForTab(tabId); | |
| modalsModel.popModal(); | |
| } catch (error) { | |
| console.error("Failed to close tab:", error); | |
| // Consider showing an error message to the user | |
| modalsModel.popModal(); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/app/modals/confirmclosetab.tsx around lines 14 to 19, the code calls
getApi().closeTab(...) without awaiting it, lacks validation for workspace and
tabId, and provides no error feedback; update handleConfirmClose to: validate
that globalStore.get(atoms.workspace) exists and that tabId is present, set a
local loading state, await the async closeTab call inside a try/catch, only call
deleteLayoutModelForTab(tabId) and modalsModel.popModal() on success, and in the
catch block surface the error to the user (e.g., set an error state or call a
notification logger); ensure loading is cleared in finally so the UI reflects
operation progress.
| newTabLayout := GetNewTabLayout() | ||
| // Merge inherited cwd into the terminal block's meta | ||
| if len(inheritedMeta) > 0 && len(newTabLayout) > 0 { | ||
| if newTabLayout[0].BlockDef.Meta == nil { | ||
| newTabLayout[0].BlockDef.Meta = make(waveobj.MetaMapType) | ||
| } | ||
| for k, v := range inheritedMeta { | ||
| newTabLayout[0].BlockDef.Meta[k] = v | ||
| } | ||
| } | ||
| err = ApplyPortableLayout(ctx, tab.OID, newTabLayout, true) |
There was a problem hiding this comment.
Potential nil pointer dereference if BlockDef is nil.
The code checks newTabLayout[0].BlockDef.Meta == nil but doesn't guard against newTabLayout[0].BlockDef being nil, which would panic.
Apply this diff to add the nil check:
newTabLayout := GetNewTabLayout()
// Merge inherited cwd into the terminal block's meta
- if len(inheritedMeta) > 0 && len(newTabLayout) > 0 {
+ if len(inheritedMeta) > 0 && len(newTabLayout) > 0 && newTabLayout[0].BlockDef != nil {
if newTabLayout[0].BlockDef.Meta == nil {
newTabLayout[0].BlockDef.Meta = make(waveobj.MetaMapType)
}📝 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.
| newTabLayout := GetNewTabLayout() | |
| // Merge inherited cwd into the terminal block's meta | |
| if len(inheritedMeta) > 0 && len(newTabLayout) > 0 { | |
| if newTabLayout[0].BlockDef.Meta == nil { | |
| newTabLayout[0].BlockDef.Meta = make(waveobj.MetaMapType) | |
| } | |
| for k, v := range inheritedMeta { | |
| newTabLayout[0].BlockDef.Meta[k] = v | |
| } | |
| } | |
| err = ApplyPortableLayout(ctx, tab.OID, newTabLayout, true) | |
| newTabLayout := GetNewTabLayout() | |
| // Merge inherited cwd into the terminal block's meta | |
| if len(inheritedMeta) > 0 && len(newTabLayout) > 0 && newTabLayout[0].BlockDef != nil { | |
| if newTabLayout[0].BlockDef.Meta == nil { | |
| newTabLayout[0].BlockDef.Meta = make(waveobj.MetaMapType) | |
| } | |
| for k, v := range inheritedMeta { | |
| newTabLayout[0].BlockDef.Meta[k] = v | |
| } | |
| } | |
| err = ApplyPortableLayout(ctx, tab.OID, newTabLayout, true) |
🤖 Prompt for AI Agents
In pkg/wcore/workspace.go around lines 244 to 254, the code assumes
newTabLayout[0].BlockDef is non-nil before accessing its Meta, which can cause a
nil pointer panic; guard against this by checking if newTabLayout[0].BlockDef is
nil and if so initialize it (ensuring Meta can be assigned) or skip merging,
then ensure Meta is non-nil before copying entries from inheritedMeta; after
these guards proceed to copy entries and call ApplyPortableLayout as before.
| // createTabObj is a wrapper that fetches the workspace and calls createTabObjWithWorkspace | ||
| func createTabObj(ctx context.Context, workspaceId string, name string, pinned bool, meta waveobj.MetaMapType) (*waveobj.Tab, error) { | ||
| ws, err := GetWorkspace(ctx, workspaceId) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("workspace %s not found: %w", workspaceId, err) | ||
| } | ||
| return createTabObjWithWorkspace(ctx, ws, name, pinned, meta) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of createTabObj across the codebase
rg -n --type go 'createTabObj\(' -g '!*_test.go'Repository: wavetermdev/waveterm
Length of output: 225
Remove unused createTabObj wrapper function.
The createTabObj function at lines 297-304 is not called anywhere in the codebase outside this file. If it's not intended for future use or external package access, it should be removed. If it is meant to be exported, consider making it public (capitalize first letter) and documenting its purpose.
🤖 Prompt for AI Agents
In pkg/wcore/workspace.go around lines 297 to 304, the createTabObj wrapper is
unused; remove the entire private function if there are no external callers or
planned future use, or alternatively make it public (capitalize CreateTabObj)
and add a doc comment if it must be exported for other packages — ensure you
update any callers or tests accordingly and run go vet/go test to confirm no
references remain.
| func (ws *WshServer) GenerateTabTitleCommand(ctx context.Context, tabId string) (string, error) { | ||
| title, err := waveai.GenerateTabTitle(ctx, tabId) | ||
| if err != nil { | ||
| return "", fmt.Errorf("error generating tab title: %w", err) | ||
| } | ||
| return title, nil | ||
| } |
There was a problem hiding this comment.
Add a fast-fail guard for empty tabId.
This improves error clarity and avoids unnecessary DB work.
func (ws *WshServer) GenerateTabTitleCommand(ctx context.Context, tabId string) (string, error) {
+ if tabId == "" {
+ return "", fmt.Errorf("tabId is required")
+ }
title, err := waveai.GenerateTabTitle(ctx, tabId)
if err != nil {
return "", fmt.Errorf("error generating tab title: %w", err)
}
return title, nil
}🤖 Prompt for AI Agents
In pkg/wshrpc/wshserver/wshserver.go around lines 1411 to 1417, add a fast-fail
guard at the start of GenerateTabTitleCommand to immediately return an error
when tabId is empty (e.g., check if tabId == "" and return "", fmt.Errorf("tabId
cannot be empty")). This avoids invoking waveai.GenerateTabTitle with an invalid
id and provides a clear, early error for callers instead of doing unnecessary
DB/remote work.
|
Thanks for the big contribution -- there are some genuinely useful improvements in here. That said, I can’t merge or properly review this PR as a single bundle. It mixes bug fixes with major, opinionated UX/default behavior changes (tab close confirmation, automatic tab renaming/title generation, preview following terminal CWD, removing the AI button, etc.). Changes that affect default workflows need design discussion first and, in many cases, must be opt-in via preferences/flags rather than new defaults. If you’re up for it, the path forward would be to split this into focused PRs (ideally one feature or fix per PR), for example:
A few of the other changes are more problematic and/or need clearer semantics or discussion before implementation:
If you open the smaller PRs, I’m happy to review and merge the ones that fit. If splitting isn’t feasible, I’m going to close this PR for now and we can revisit individual pieces later. |
|
@sawka |
|
Thanks for the feedback! I've split this into focused PRs as requested:
Each PR is now focused on a single feature. The more opinionated features (markdown copy, auto tab renaming, preview follows CWD) are not included and can be discussed separately if there's interest. |
3 participants
Summary
This PR adds several UX improvements to enhance the Wave Terminal user experience:
Tab Management
File Preview Enhancements
Terminal Improvements
AI Panel
Test plan
🤖 Generated with Claude Code