Skip to content

Improve typing and sidebar performance in large workspaces - #516

Open
gschier wants to merge 2 commits into
mainfrom
worktree-perf
Open

Improve typing and sidebar performance in large workspaces#516
gschier wants to merge 2 commits into
mainfrom
worktree-perf

Conversation

@gschier

@gschier gschier commented Jul 25, 2026

Copy link
Copy Markdown
Member

Performance work targeting large workspaces (thousands of requests, eg. a big OpenAPI/Postman import).

Typing lag

  • Debounce model writes from editors (patchModelDebounced, 400ms). Sends, duplicates, and WS/gRPC connects flush pending patches first. gRPC message stays un-debounced since its send path reads from the frontend store.
  • Key useParentFolders on folderId so keystrokes no longer rebuild every editor's language/autocomplete extensions.
  • Debounce the per-update editor state cache (full-doc serialize + md5).

Sidebar

  • Virtualize the tree with @tanstack/react-virtual (existing pattern from the event viewer). Only visible rows mount, which also shrinks dnd-kit registration/measuring and the per-row drag monitor fan-out.
  • Filter collapsed/hidden rows out of the render list entirely.
  • Memoize Sidebar so route navigations don't re-render the subtree.

Request switching

  • Identity-stable URL autocomplete options (shared atom, no dependency on the active request id).

Follow-ups (not in this PR): lazy-mount editors for inactive request-pane tabs (~64 CodeMirror mounts per switch), batch model_write events after import, SQLite indexes on workspace_id/folder_id.

@gschier
gschier marked this pull request as ready for review August 2, 2026 15:06
@greptile-apps

greptile-apps Bot commented Aug 2, 2026

Copy link
Copy Markdown

Greptile Summary

This PR addresses typing lag and sidebar sluggishness in large workspaces through a coherent set of targeted optimizations. Model writes from editors are debounced (400 ms) via a new patchModelDebounced that coalesces rapid patches per model and flushes before every send/connect operation; the debounce utility gains a flush() method to support this. The sidebar tree is virtualized with @tanstack/react-virtual, collapsed/hidden nodes are filtered from the render list before they reach TreeItem, and the component is wrapped in memo to block re-renders from route navigation.

  • patchModelDebounced coalesces per-keystroke patches into a single DB write; flushAllModelWrites (called before HTTP/WS/gRPC sends and on beforeunload) guarantees no stale data reaches the backend.
  • useParentFolders now keys on folderId instead of the full model object, so typing in the URL bar no longer invalidates language/autocomplete extension memos.
  • allRequestUrlsAtom and allRequestIdsAtom are new identity-stable selectAtom derivations that suppress re-renders when unrelated request fields change.

Confidence Score: 4/5

Safe to merge; all flush-before-send paths are correctly wired and the debounce logic is sound.

The core debounce + flush contract is correctly implemented: coalesced patches are always flushed synchronously before any backend operation, and the beforeunload handler guards against window-close data loss. The sidebar virtualization and collapsed-row filtering follow an established pattern. The two open items — a stateSavers map that accumulates dead closures on request deletion, and a scrollMargin offset that is not refreshed when the sidebar layout shifts — are real but narrow: the memory overhead is tiny and scrollToIndex is only invoked when keyboard-navigating to an off-screen row.

Files Needing Attention: packages/ui/src/components/tree/TreeItemList.tsx (stale scrollMargin) and apps/yaak-client/components/core/Editor/Editor.tsx (stateSavers cleanup).

Important Files Changed

Filename Overview
crates/yaak-models/guest-js/store.ts Adds patchModelDebounced with per-model coalescing, flushAllPendingPatches, and wires into existing flushAllModelWrites; logic is sound and flush ordering is correct
packages/common-lib/debounce.ts Adds flush() method and fixes cancel() to clear lastArgs; implementation is correct
apps/yaak-client/components/core/Editor/Editor.tsx Adds per-stateKey debounced state saving and flushes on unmount; the module-level stateSavers map accumulates dead entries for deleted requests and is never pruned
packages/ui/src/components/tree/TreeItemList.tsx Introduces VirtualTreeItemList using @tanstack/react-virtual; scrollMargin offset is measured once on mount and not refreshed if the container layout changes later
packages/ui/src/components/tree/Tree.tsx Filters hidden/collapsed items out of the render list, virtualizes via TreeItemList, and updates tryFocus to scroll virtualized rows into range before focusing
apps/yaak-client/components/Sidebar.tsx Wraps export with memo to prevent route-navigation re-renders from churning DndContext; straightforward single-line change
apps/yaak-client/hooks/useAllRequests.ts Adds allRequestIdsAtom and allRequestUrlsAtom as identity-stable selectAtom derivations; equality functions are correct
apps/yaak-client/hooks/useParentFolders.ts Keys useMemo on folderId instead of the full model object so URL keystrokes no longer invalidate the memo; correct and clean refactor

Sequence Diagram

sequenceDiagram
    participant User
    participant Editor
    participant patchModelDebounced
    participant debounceTimer as Debounce 400ms
    participant pendingPatches as pendingPatches Map
    participant flushAllModelWrites
    participant Backend

    User->>Editor: keystroke
    Editor->>patchModelDebounced: patch
    patchModelDebounced->>pendingPatches: merge patch into pending entry
    patchModelDebounced->>debounceTimer: reset timer

    User->>Editor: keystroke
    Editor->>patchModelDebounced: patch
    patchModelDebounced->>pendingPatches: merge patch coalesced
    patchModelDebounced->>debounceTimer: reset timer

    alt 400ms idle timer fires
        debounceTimer->>pendingPatches: writePendingPatch key
        pendingPatches->>Backend: patchModelById invoke models_upsert
    else User clicks Send or Connect
        User->>flushAllModelWrites: send clicked
        flushAllModelWrites->>pendingPatches: flushAllPendingPatches sync
        pendingPatches->>Backend: patchModelById invoke models_upsert
        flushAllModelWrites->>flushAllModelWrites: await Promise.allSettled
        flushAllModelWrites->>Backend: invoke cmd_send_http_request
    end
Loading

Comments Outside Diff (2)

  1. apps/yaak-client/components/core/Editor/Editor.tsx, line 357-360 (link)

    P2 stateSavers accumulates entries and never cleans up

    flushCachedEditorState calls flush() on unmount but never removes the entry from the module-level stateSavers map. Each unique stateKey ever mounted adds a permanent closure to the map. For a typical workflow — importing a large OpenAPI spec and then pruning requests — this means hundreds of dead debounce closures accumulate for the lifetime of the window. Calling stateSavers.delete(stateKey) inside flushCachedEditorState, after the flush, would keep the map bounded to currently-mounted editors.

  2. apps/yaak-client/components/HttpRequestPane.tsx, line 93-95 (link)

    P2 Identical requestUrlOptionsAtom duplicated across two modules

    HttpRequestPane.tsx and WebsocketRequestPane.tsx each define their own module-level requestUrlOptionsAtom with identical bodies. Since atoms are module singletons, two separate atom instances are created, meaning two independent subscriptions to allRequestUrlsAtom and two copies of the resulting array. Moving the shared atom to useAllRequests.ts would eliminate the duplication and reduce downstream subscriptions to one.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "Merge branch 'main' into worktree-perf" | Re-trigger Greptile

Comment on lines +98 to +105
useLayoutEffect(() => {
const list = listRef.current;
const scroller = getScrollElement();
if (list == null || scroller == null) return;
const offset =
list.getBoundingClientRect().top - scroller.getBoundingClientRect().top + scroller.scrollTop;
setScrollMargin(offset);
}, [getScrollElement]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 scrollMargin computed only once and can go stale

The useLayoutEffect that measures the list-to-scroller offset only reruns when getScrollElement changes, which never happens (it's a stable useCallback with no deps). If the sidebar header grows or shrinks after mount, the cached offset will be wrong and scrollToIndex will land on a different row than intended. A ResizeObserver on the scroll container — or adding nodes.length as a second dependency — would keep this accurate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant