diff --git a/lib/public/modules/app-messages.js b/lib/public/modules/app-messages.js index 0d4c2ab..a49c9b9 100644 --- a/lib/public/modules/app-messages.js +++ b/lib/public/modules/app-messages.js @@ -19,7 +19,7 @@ import { handleFindInSessionResults } from './session-search.js'; import { handleInputSync, autoResize, resetAutoResize, builtinCommands, setScheduleBtnDisabled } from './input.js'; import { startThinking, appendThinking, stopThinking, resetThinkingGroup, createToolItem, updateToolExecuting, updateToolResult, markAllToolsDone, closeToolGroup, removeToolFromGroup, resetToolState, getTools, getPlanContent, setPlanContent, renderPlanBanner, renderPlanCard, getTodoTools, handleTodoWrite, handleTaskCreate, handleTaskUpdate, applyDeadSessionTodoCompaction, isPlanFilePath, enableMainInput, addTurnMeta, updateSubagentActivity, addSubagentToolEntry, markSubagentDone, initSubagentStop, updateSubagentProgress, updateSubagentTaskStatus, renderAskUserQuestion, markAskUserAnswered, renderPermissionRequest, markPermissionCancelled, markPermissionResolved, renderElicitationRequest, markElicitationResolved } from './tools.js'; import { showDoneNotification, playDoneSound, isNotifAlertEnabled, isNotifSoundEnabled } from './notifications.js'; -import { handleFsList, handleFsRead, handleFileChanged, handleDirChanged, handleFileHistory, handleGitDiff, handleFileAt, refreshIfOpen, getPendingNavigate, handleFsSearch } from './filebrowser.js'; +import { handleFsList, handleFsRead, handleFileChanged, handleDirChanged, handleFileHistory, handleGitDiff, handleFileAt, refreshIfOpen, getPendingNavigate, peekPendingNavigate, handleFsSearch } from './filebrowser.js'; import { isProjectSettingsOpen, refreshProjectSettingsModels, handleInstructionsRead, handleInstructionsWrite, handleProjectEnv, handleProjectEnvSaved, handleProjectSharedEnv, handleProjectSharedEnvSaved, handleProjectOwnerChanged } from './project-settings.js'; import { updateSettingsModels, updateSettingsStats, updateDaemonConfig, handleSetPinResult, handleKeepAwakeChanged, handleAutoContinueChanged, handleRestartResult, handleShutdownResult, handleSharedEnv, handleSharedEnvSaved, handleGlobalClaudeMdRead, handleGlobalClaudeMdWrite } from './server-settings.js'; import { handleTermList, handleTermCreated, sendTerminalCommand, handleTermOutput, handleTermResized, handleTermExited, handleTermClosed } from './terminal.js'; @@ -69,7 +69,11 @@ export function processMessage(msg) { // append sequence. Without this, scroll events fired during DOM growth // can set isUserScrolledUp=true before history_done's arm fires, leaving // the user stranded mid-conversation on resume (especially on mobile). - if (!getPendingNavigate() || !(getPendingNavigate().toolId || getPendingNavigate().assistantUuid)) { + // Peek without consuming — getPendingNavigate() is the single consumer + // in the history_done branch below. Calling getPendingNavigate() here + // would clear the value before history_done can use it. + var _metaNav = peekPendingNavigate(); + if (!_metaNav || !(_metaNav.toolId || _metaNav.assistantUuid)) { armStickyBottom(5000, true); // force: history replay must scroll to bottom } break; diff --git a/lib/public/modules/filebrowser.js b/lib/public/modules/filebrowser.js index 8c2e543..c68b7e0 100644 --- a/lib/public/modules/filebrowser.js +++ b/lib/public/modules/filebrowser.js @@ -5,6 +5,12 @@ import { closeSidebar } from './sidebar.js'; import { closeTerminal } from './terminal.js'; import { renderUnifiedDiff, renderSplitDiff } from './diff.js'; import { initFileIcons, getFileIconSvg, getFolderIconSvg } from './fileicons.js'; +import { + setPendingNavigate as _setPendingNavigate, + clearPendingNavigate as _clearPendingNavigate, + peekPendingNavigate, + getPendingNavigate, +} from './pending-navigate.js'; var ctx; var showDropHint = function () {}; @@ -15,7 +21,7 @@ var isRendered = false; // markdown render toggle state var currentIsMarkdown = false; var historyVisible = false; var currentHistoryEntries = []; -var pendingNavigate = null; // { sessionLocalId, assistantUuid } +// pendingNavigate state delegated to ./pending-navigate.js (DOM-free, testable) var selectedEntries = []; // up to 2 selected for compare var compareMode = false; var inlineDiffActive = false; @@ -419,7 +425,7 @@ export function resetFileBrowser() { currentIsMarkdown = false; historyVisible = false; currentHistoryEntries = []; - pendingNavigate = null; + _clearPendingNavigate(); selectedEntries = []; compareMode = false; inlineDiffActive = false; @@ -1929,11 +1935,11 @@ function navigateToEdit(edit) { return; } - pendingNavigate = { + _setPendingNavigate({ sessionLocalId: edit.sessionLocalId, assistantUuid: edit.assistantUuid, toolId: edit.toolId, - }; + }); if (ctx.ws && ctx.connected) { ctx.ws.send(JSON.stringify({ type: "switch_session", id: edit.sessionLocalId })); @@ -1959,8 +1965,5 @@ function scrollToToolElement(toolId, assistantUuid) { }); } -export function getPendingNavigate() { - var nav = pendingNavigate; - pendingNavigate = null; - return nav; -} +// peekPendingNavigate and getPendingNavigate are imported from +// ./pending-navigate.js and re-exported from the import statement at the top. diff --git a/lib/public/modules/pending-navigate.js b/lib/public/modules/pending-navigate.js new file mode 100644 index 0000000..edba2f1 --- /dev/null +++ b/lib/public/modules/pending-navigate.js @@ -0,0 +1,28 @@ +// pendingNavigate state machine — no DOM dependencies. +// Imported by filebrowser.js (browser) and by test/pending-navigate.test.js +// (Node ESM import — no DOM shim needed). +// +// Extracted for lr-a3ca: history_meta must peek without consuming so that +// history_done remains the sole consumer. + +var pendingNavigate = null; + +export function setPendingNavigate(nav) { + pendingNavigate = nav; +} + +export function clearPendingNavigate() { + pendingNavigate = null; +} + +// Non-consuming read — use in history_meta guard. +export function peekPendingNavigate() { + return pendingNavigate; +} + +// Consuming read — use in history_done only. +export function getPendingNavigate() { + var nav = pendingNavigate; + pendingNavigate = null; + return nav; +} diff --git a/test/pending-navigate.test.js b/test/pending-navigate.test.js new file mode 100644 index 0000000..d102b8d --- /dev/null +++ b/test/pending-navigate.test.js @@ -0,0 +1,112 @@ +// Regression tests for the pendingNavigate peek/consume contract (lr-a3ca). +// +// Background: history_meta must inspect the pending nav target WITHOUT +// consuming it (peekPendingNavigate), so that history_done can later consume +// it exactly once (getPendingNavigate). +// +// The bug (before the fix) was three calls to getPendingNavigate() in the +// history_meta branch of app-messages.js: +// if (!getPendingNavigate() || !(getPendingNavigate().toolId || getPendingNavigate().assistantUuid)) +// First call returned the nav and cleared pendingNavigate. Second call +// returned null → null.toolId TypeError in the WS onmessage handler. +// history_done then received null and never navigated. +// +// The fix: history_meta uses peekPendingNavigate() (non-consuming); only +// history_done calls the consuming getPendingNavigate(). +// +// These tests import the real lib/public/modules/pending-navigate.js — the +// DOM-free module that filebrowser.js imports its peek/get implementations +// from. If pending-navigate.js reverts peekPendingNavigate to a consuming +// read, the "peek does not consume" and "nav survives history_meta" tests fail. + +import { test } from "node:test"; +import assert from "node:assert/strict"; + +// Node can import ESM modules directly. pending-navigate.js has no DOM deps. +// We append a cache-busting query param so each test file invocation gets a +// fresh module instance; within a single test run the module is loaded once and +// state is reset manually between tests using the exported clearPendingNavigate. +import { + setPendingNavigate, + clearPendingNavigate, + peekPendingNavigate, + getPendingNavigate, +} from "../lib/public/modules/pending-navigate.js"; + +// Reset state before each test by clearing any leftover nav target. +function reset() { clearPendingNavigate(); } + +test("peekPendingNavigate returns the nav object without consuming it", () => { + reset(); + const nav = { sessionLocalId: "s1", assistantUuid: "u1", toolId: "t1" }; + setPendingNavigate(nav); + + const first = peekPendingNavigate(); + const second = peekPendingNavigate(); + + assert.deepEqual(first, nav, "first peek returns nav"); + assert.deepEqual(second, nav, "second peek returns same object (non-consuming)"); + reset(); +}); + +test("getPendingNavigate returns the nav object and clears it", () => { + reset(); + const nav = { sessionLocalId: "s2", assistantUuid: "u2", toolId: "t2" }; + setPendingNavigate(nav); + + const first = getPendingNavigate(); + const second = getPendingNavigate(); + + assert.deepEqual(first, nav, "first get returns nav"); + assert.equal(second, null, "second get returns null (consumed)"); +}); + +test("peek does not consume — nav survives history_meta and is available for history_done", () => { + reset(); + const nav = { sessionLocalId: "s3", assistantUuid: "u3", toolId: "t3" }; + setPendingNavigate(nav); + + // Simulate the FIXED history_meta branch: peek, no consume + const metaNav = peekPendingNavigate(); + assert.deepEqual(metaNav, nav, "history_meta (peek) sees the nav target"); + + // Nav must still be available after history_meta + assert.deepEqual(peekPendingNavigate(), nav, "nav survives history_meta (non-consuming)"); + + // Simulate history_done: consume once + const doneNav = getPendingNavigate(); + assert.deepEqual(doneNav, nav, "history_done receives the nav target"); + assert.equal(getPendingNavigate(), null, "nav is null after history_done consumes it"); +}); + +test("regression: consuming get in history_meta clears nav before history_done (pre-fix bug)", () => { + reset(); + const nav = { sessionLocalId: "s4", assistantUuid: "u4", toolId: "t4" }; + setPendingNavigate(nav); + + // Simulate the BUGGY history_meta: getPendingNavigate() called twice + // (matches the original `!getPendingNavigate() || !(getPendingNavigate().toolId ...)`) + const first = getPendingNavigate(); // consumes nav + const second = getPendingNavigate(); // returns null — this was .toolId accessed → TypeError + assert.deepEqual(first, nav); + assert.equal(second, null, "second consuming get returns null — the bug"); + + // history_done now gets null because the bug consumed nav too early + assert.equal(getPendingNavigate(), null, "history_done receives null when nav consumed in history_meta"); +}); + +test("clearPendingNavigate resets state (used by resetFileBrowser)", () => { + setPendingNavigate({ sessionLocalId: "s5" }); + clearPendingNavigate(); + assert.equal(getPendingNavigate(), null, "clearPendingNavigate empties the state"); +}); + +test("peekPendingNavigate on empty state returns null", () => { + reset(); + assert.equal(peekPendingNavigate(), null); +}); + +test("getPendingNavigate on empty state returns null", () => { + reset(); + assert.equal(getPendingNavigate(), null); +});