Skip to content

Commit 18ab50f

Browse files
stevenvoclaude
andcommitted
Address CodeRabbit review feedback
- 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]>
1 parent 4970ac4 commit 18ab50f

File tree

4 files changed

+20
-23
lines changed

4 files changed

+20
-23
lines changed

frontend/app/aipanel/aipanelmessages.tsx

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ import { AIModeDropdown } from "./aimode";
88
import { type WaveUIMessage } from "./aitypes";
99
import { WaveAIModel } from "./waveai-model";
1010

11+
const AUTO_SCROLL_DEBOUNCE_MS = 100;
12+
const SCROLL_BOTTOM_THRESHOLD_PX = 50;
13+
1114
interface AIPanelMessagesProps {
1215
messages: WaveUIMessage[];
1316
status: string;
@@ -32,7 +35,7 @@ export const AIPanelMessages = memo(({ messages, status, onContextMenu }: AIPane
3235
userHasScrolledUp.current = false;
3336
setTimeout(() => {
3437
isAutoScrolling.current = false;
35-
}, 100);
38+
}, AUTO_SCROLL_DEBOUNCE_MS);
3639
}
3740
};
3841

@@ -48,15 +51,15 @@ export const AIPanelMessages = memo(({ messages, status, onContextMenu }: AIPane
4851
const { scrollTop, scrollHeight, clientHeight } = container;
4952
const distanceFromBottom = scrollHeight - scrollTop - clientHeight;
5053

51-
// If user is more than 50px from the bottom, they've scrolled up
52-
if (distanceFromBottom > 50) {
54+
// If user is more than threshold from the bottom, they've scrolled up
55+
if (distanceFromBottom > SCROLL_BOTTOM_THRESHOLD_PX) {
5356
userHasScrolledUp.current = true;
5457
} else {
5558
userHasScrolledUp.current = false;
5659
}
5760
};
5861

59-
container.addEventListener("scroll", handleScroll);
62+
container.addEventListener("scroll", handleScroll, { passive: true });
6063
return () => container.removeEventListener("scroll", handleScroll);
6164
}, []);
6265

frontend/app/view/term/termwrap.ts

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -830,9 +830,6 @@ export class TermWrap {
830830
e?.preventDefault();
831831
e?.stopPropagation();
832832

833-
// Preserve scroll position before paste
834-
const scrollY = this.terminal.buffer.active.viewportY;
835-
836833
try {
837834
const clipboardData = await extractAllClipboardData(e);
838835
let firstImage = true;
@@ -849,15 +846,6 @@ export class TermWrap {
849846
this.terminal.paste(data.text);
850847
}
851848
}
852-
853-
// Restore scroll position after paste if it changed unexpectedly
854-
setTimeout(() => {
855-
const currentScrollY = this.terminal.buffer.active.viewportY;
856-
// Only restore if we've scrolled significantly (not just normal paste scroll)
857-
if (currentScrollY < scrollY - 10) {
858-
this.terminal.scrollToLine(scrollY);
859-
}
860-
}, 50);
861849
} catch (err) {
862850
console.error("Paste error:", err);
863851
} finally {

frontend/types/custom.d.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ declare global {
130130
openBuilder: (appId?: string) => void; // open-builder
131131
setBuilderWindowAppId: (appId: string) => void; // set-builder-window-appid
132132
doRefresh: () => void; // do-refresh
133-
getPathForFile: (file: File) => string; // get-path-for-file
133+
getPathForFile: (file: File) => string; // uses webUtils.getPathForFile
134134
};
135135

136136
type ElectronContextMenuItem = {

pkg/wcore/workspace.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ func CreateTab(ctx context.Context, workspaceId string, tabName string, activate
228228
}
229229

230230
// The initial tab for the initial launch should be pinned
231-
tab, err := createTabObj(ctx, workspaceId, tabName, pinned || isInitialLaunch, inheritedMeta)
231+
tab, err := createTabObjWithWorkspace(ctx, ws, tabName, pinned || isInitialLaunch, inheritedMeta)
232232
if err != nil {
233233
return "", fmt.Errorf("error creating tab: %w", err)
234234
}
@@ -270,11 +270,8 @@ func CreateTab(ctx context.Context, workspaceId string, tabName string, activate
270270
return tab.OID, nil
271271
}
272272

273-
func createTabObj(ctx context.Context, workspaceId string, name string, pinned bool, meta waveobj.MetaMapType) (*waveobj.Tab, error) {
274-
ws, err := GetWorkspace(ctx, workspaceId)
275-
if err != nil {
276-
return nil, fmt.Errorf("workspace %s not found: %w", workspaceId, err)
277-
}
273+
// createTabObjWithWorkspace creates a tab object with an already-fetched workspace
274+
func createTabObjWithWorkspace(ctx context.Context, ws *waveobj.Workspace, name string, pinned bool, meta waveobj.MetaMapType) (*waveobj.Tab, error) {
278275
layoutStateId := uuid.NewString()
279276
tab := &waveobj.Tab{
280277
OID: uuid.NewString(),
@@ -297,6 +294,15 @@ func createTabObj(ctx context.Context, workspaceId string, name string, pinned b
297294
return tab, nil
298295
}
299296

297+
// createTabObj is a wrapper that fetches the workspace and calls createTabObjWithWorkspace
298+
func createTabObj(ctx context.Context, workspaceId string, name string, pinned bool, meta waveobj.MetaMapType) (*waveobj.Tab, error) {
299+
ws, err := GetWorkspace(ctx, workspaceId)
300+
if err != nil {
301+
return nil, fmt.Errorf("workspace %s not found: %w", workspaceId, err)
302+
}
303+
return createTabObjWithWorkspace(ctx, ws, name, pinned, meta)
304+
}
305+
300306
// Must delete all blocks individually first.
301307
// Also deletes LayoutState.
302308
// recursive: if true, will recursively close parent window, workspace, if they are empty.

0 commit comments

Comments
 (0)