Perf/parallelize session switch#1158
Conversation
Three changes to speed up session switching: 1. workspace.js: Parallelize expanded-dir pre-fetching in loadDir(). Previously, expanded directories were fetched sequentially with for-await, creating a waterfall of N API calls. Now uses Promise.all() to fetch all expanded dirs concurrently. 2. sessions.js: Overlap loadDir() network requests with highlightCode(). The idle path ran highlightCode() (CPU-bound Prism.js) before loadDir, delaying the file-tree fetch. Now loadDir() is kicked off first so the HTTP request is in-flight while Prism runs, then awaited. 3. workspace.py: Parallelize git_info_for_workspace() subprocess calls. git status, rev-list (ahead), and rev-list (behind) were called serially (3 subprocess.run calls, each up to 3s timeout). Now they run concurrently via ThreadPoolExecutor, reducing wall time by ~60%.
6 tests across 3 classes: - TestLoadDirParallelPrefetch: static analysis of Promise.all in workspace.js - TestLoadSessionIdleOverlap: static analysis of loadDir-before-highlightCode ordering - TestGitInfoParallel: runtime tests with threading.Barrier concurrency proof and wall-clock timing (parallel ~0.1s vs serial ~0.3s for 3 git commands)
Add msg_limit and msg_before parameters to /api/session endpoint: - Initial load fetches only last 30 messages (tail window) - Scroll-to-top triggers lazy loading of older messages via cursor - Payload reduced by ~70% for large sessions (100+ messages) - undo/retry/compress auto-reset truncation state - Export uses server-side generation, unaffected Backend (api/routes.py): - msg_limit: returns only the last N messages with truncation flag - msg_before: cursor-based paging for scroll-to-top loading Frontend (static/sessions.js): - _INITIAL_MSG_LIMIT=30 for initial paginated load - _messagesTruncated tracks pending older messages - _loadOlderMessages() prepends older messages on scroll - _ensureAllMessagesLoaded() for operations needing full history Frontend (static/ui.js): - Scroll-to-top detection (scrollTop < 80px) triggers lazy load - 'Load older messages' visual indicator when truncated Tests: 19 total (13 new for message pagination), all passing
Performance: Parallelize session switch + message paginationProblemSwitching between sessions in the web UI is noticeably slow, especially for long conversations. The delay comes from four serial bottlenecks:
ChangesCommit 1:
Commit 2:
Commit 3:
Expected impact
For a remote deployment switching to a 200+ message session: ~800ms → ~40ms. Compatibility
Tests19 new tests, all passing:
All 2,575 existing tests pass (5 pre-existing failures unrelated to this PR). |
Issues found and fixed: 1. workspace.js: S._expandedDirs||[] fallback produced Array (no .size) → changed to ||new Set() 2. routes.py: _messages_truncated flag was wrong for msg_before paging → separate truncation logic per code path, compare against _slice not _all_msgs 3. routes.py: msg_before used timestamp comparison which fails when messages lack _ts (827 msgs with _ts=0 in real data) or have duplicate timestamps → changed to index-based cursor (0-based position in full array) → added _messages_offset field so frontend knows cursor position → added input validation (try/except, max(1,...), bounds clamping) 4. sessions.js: _oldestIdx tracks cursor position, reset on session switch, updated from _messages_offset on each page load 5. workspace.py: moved concurrent.futures import to module top level 6. tests: updated to match index-based cursor, added 8 new tests: - test_expanded_dirs_fallback_is_set - test_msg_before_index_based_slicing - test_msg_before_zero_returns_empty - test_msg_before_equal_total - test_truncation_flag_with_msg_before - test_messages_offset_initial_load/with_msg_before - test_oldest_idx_tracking/reset_on_session_switch - test_load_older_uses_index_cursor - test_msg_before_bounds_clamping 27/27 tests pass, 2583/2583 total (5 pre-existing failures unrelated)
|
Great performance work, @jasonjcwu! The author's comment gives a thorough breakdown — adding some review notes here: Parallelization changes (commits 1–2) look well-targeted:
Message pagination (commit 3) is the most impactful change and deserves a close review:
One thing worth verifying in review: the The ~800ms → ~40ms expected improvement for remote deployments is a meaningful UX gain. |
Race condition: if user switches sessions while _loadOlderMessages() is in-flight, the stale response could land on the new session's S.messages. Guards verified: - _loadOlderMessages checks _loadingSessionId !== sid after await (bails out) - Guard appears BEFORE S.messages mutation (stale data cannot land) - loadSession resets _loadingOlder=false on switch (no stale lock) - loadSession resets _messagesTruncated=false and _oldestIdx=0 on switch Added: _loadingOlder=false reset in loadSession() reset block. Added: 5 tests in TestSessionSwitchCancellation proving all guards exist and are ordered correctly. 32/32 tests pass.
Hold -- Pagination UX Needs Review Before MergePlacing this PR on hold pending a UX decision on the new message pagination behaviour. Details below. Approved ChangesThe parallelization work looks clean and is approved:
These changes can land as-is once the UX question below is resolved. Hold Reason -- Message Pagination UXThe decision to limit displayed messages to the most-recent 30 on every session switch, with a "click to load older" / scroll-triggered load path, is a meaningful UX change that affects every conversation view. It needs explicit sign-off before merge. Questions for @aronprins
Required Fix --
|
) Merged as v0.50.229. 2678 tests passing. Browser QA 21/21. All three PRs were independently reviewed and approved by @nesquena with reviewer commits pulled in: - #1181 (#1158): `d974388` (stale-response race in _loadOlderMessages) - #1182: `7e20006` (full-scan fallback path consistency) - #1180: `a5ad154` (regression test for iOS zoom threshold) Thanks @jasonjcwu (#1158)!
|
Merged as v0.50.229 via integration PR #1181 (then batched in #1183). Thank you @jasonjcwu — the parallelization and pagination work are live! 🎉 |
…squena#1183) Merged as v0.50.229. 2678 tests passing. Browser QA 21/21. All three PRs were independently reviewed and approved by @nesquena with reviewer commits pulled in: - nesquena#1181 (nesquena#1158): `d974388` (stale-response race in _loadOlderMessages) - nesquena#1182: `7e20006` (full-scan fallback path consistency) - nesquena#1180: `a5ad154` (regression test for iOS zoom threshold) Thanks @jasonjcwu (nesquena#1158)!
No description provided.