From 0dc88b51195ade06d00148fbd691d9151ba9ef76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sat, 15 Aug 2026 21:52:21 +0000 Subject: [PATCH] leaving a folder closes what was open in it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pane shows a file from the folder you were in. Walking into another folder kept it open, so the listing said one place and the pane said another — and the URL carried a `?view=` that no longer belonged to its own pathname. Navigating now drops the overlay params. Only those: anything else in the query is left alone, because `useGlobalQueryString` puts real state there and a wholesale reset takes it with it. ── The actual fix is that the list has an owner now ── Which params belong to the overlay was a private array inside `useFileViewerPanels`, so every other site that touched the query worked from memory. That is exactly how `onCloseViewer` came to wipe the folder along with the pane, and it is the thing TODO.md flagged as the reason a fifth handler would get it wrong the same way. `VIEWER_PARAMS` and `withoutViewerParams` now live in `files-route.ts` — a leaf with no imports, so the hook and the browser can both depend on it without depending on each other. Three readers, one list. Writing this caught a fourth site immediately: `setViewerParams` had been REPLACING the whole search since the folder left the query string, so opening a file would have discarded global UI state. It clears the overlay and keeps the rest, like everything else now does. 3 more tests, including that every key is covered rather than the obvious two. tsgo clean, frontend builds, 840 pass / 7 fail — +3 new, same 7. Co-Authored-By: Claude Opus 5 --- .../FileBrowserApp/files-route.test.ts | 25 ++++++++++++++ .../FileBrowser/FileBrowserApp/files-route.ts | 33 +++++++++++++++++++ .../FileBrowserApp/useFileBrowserApp.ts | 20 ++++++++--- .../useFileViewerPanels.tsx | 19 ++--------- 4 files changed, 75 insertions(+), 22 deletions(-) diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/files-route.test.ts b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/files-route.test.ts index f4fdf22b..faf2812b 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/files-route.test.ts +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/files-route.test.ts @@ -6,6 +6,8 @@ import { folderHref, relativeToFolder, resolveInFolder, + VIEWER_PARAMS, + withoutViewerParams, } from './files-route'; // The folder is in the URL path now, so every character a filesystem allows has to survive a round trip @@ -150,3 +152,26 @@ describe('view param, relative to the folder in the path', () => { expect(resolveInFolder('/docs', '')).toBe(''); }); }); + +describe('withoutViewerParams', () => { + test('drops the overlay, keeps everything else', () => { + // The "everything else" is the point: useGlobalQueryString puts arbitrary state in the URL, and a + // wholesale reset to '' would take it with it — which is the bug that sent people home on close. + const out = new URLSearchParams(withoutViewerParams('view=a.md&ephemeral=b.mp3&chatType=file&theme=dark&x=1')); + expect(out.get('view')).toBeNull(); + expect(out.get('ephemeral')).toBeNull(); + expect(out.get('chatType')).toBeNull(); + expect(out.get('theme')).toBe('dark'); + expect(out.get('x')).toBe('1'); + }); + + test('every overlay key is covered, not just the obvious two', () => { + const all = VIEWER_PARAMS.map((k) => `${k}=x`).join('&'); + expect(withoutViewerParams(all)).toBe(''); + }); + + test('an empty or overlay-free query is unchanged', () => { + expect(withoutViewerParams('')).toBe(''); + expect(withoutViewerParams('a=1')).toBe('a=1'); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/files-route.ts b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/files-route.ts index 645f3cbe..ff5e9872 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/files-route.ts +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/files-route.ts @@ -91,3 +91,36 @@ export function relativeToFolder(folder: string, absPath: string): string { const rest = absPath.slice(prefix.length); return rest && !rest.includes('/') ? rest : absPath; } + +// ── Which params belong to the overlay, in one place ── +// +// The list used to live inside `useFileViewerPanels` and nowhere else, so every other site that touched +// the query string worked from memory. That is how `onCloseViewer` came to wipe the whole search — the +// folder included — and why walking into a folder had to decide for itself what to carry. One list, three +// readers, no memory. +// +// This module is a leaf with no imports of its own, which is why the list lives here rather than in the +// hook: both the hook and the browser can depend on it without depending on each other. + +export const VIEWER_PARAMS = [ + 'view', + 'ephemeral', + 'ephemeralRoot', + 'ephemeral2', + 'ephemeral2Root', + 'ephemeral2Auto', + 'chatContext', + 'chatType', +] as const; + +/** + * The query string with every overlay param removed, and everything else left alone. + * + * "Everything else" is not hypothetical — `useGlobalQueryString` puts arbitrary state in the URL, and a + * wholesale `''` would take it with it. + */ +export function withoutViewerParams(search: string): string { + const next = new URLSearchParams(search); + VIEWER_PARAMS.forEach((key) => next.delete(key)); + return next.toString(); +} diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/useFileBrowserApp.ts b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/useFileBrowserApp.ts index b3470898..16c96e65 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/useFileBrowserApp.ts +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/useFileBrowserApp.ts @@ -5,7 +5,7 @@ import { useFilesAPI, type DirEntry } from '../../../hooks/useFilesAPI'; import { usePinnedFiles } from '../usePinnedFiles'; import { EDITOR_FILE_PARAM } from '../../CodeEditor/useEditorState'; import { getFileType } from '../../FileViewer'; -import { folderFromPathname, folderHref, relativeToFolder } from './files-route'; +import { folderFromPathname, folderHref, relativeToFolder, withoutViewerParams } from './files-route'; import { useTasks, type TaskSummary } from '../useTasks'; import { useAgents, type AgentSummary } from '../useAgents'; import { useUserState } from 'state/useUserState'; @@ -37,9 +37,11 @@ export const useFileBrowserApp = (basePath: string, rootOverride?: string, urlPa // default a new dashboard's cwd to it. const currentPath = urlPath ? folderFromPathname(location.pathname) : localPath; const setCurrentPath = (path: string) => { - // The search string rides along, so walking into a folder does not close an open pane — the same - // property the old `withPath` had, now expressed as "change the pathname, keep the query". - if (urlPath) navigate({ pathname: folderHref(path), search: location.search }); + // Leaving a folder closes what was open in it. A pane shows a file from the folder you were in, so + // carrying it into the next one leaves the two disagreeing — the listing says one place, the pane + // another. Only the OVERLAY params go; anything else in the query (global UI state) is left alone, + // which is the distinction a wholesale reset kept getting wrong. + if (urlPath) navigate({ pathname: folderHref(path), search: withoutViewerParams(location.search) }); else setLocalPath(path); if (!scoped) setGlobalPath(path); }; @@ -55,7 +57,15 @@ export const useFileBrowserApp = (basePath: string, rootOverride?: string, urlPa * open. The folder is no longer among them — it is the pathname — so it survives without being copied * across, which is what used to be needed here. */ - const setViewerParams = (params: Record) => setSearchParams(new URLSearchParams(params)); + const setViewerParams = (params: Record) => + setSearchParams((prev) => { + // Clear the previous overlay — opening a file closes whatever was open — but only the overlay. + // Replacing the whole search would take global UI state with it, which is the mistake this file + // has now made twice in two different places. + const next = new URLSearchParams(withoutViewerParams(prev.toString())); + for (const [key, value] of Object.entries(params)) next.set(key, value); + return next; + }); /** * Links from before the folder moved into the path still say `?path=/Tests`. Rewrite them once, in diff --git a/src/workspaces/officerdev/src/hooks/useFileViewerPanels/useFileViewerPanels.tsx b/src/workspaces/officerdev/src/hooks/useFileViewerPanels/useFileViewerPanels.tsx index 2efab051..7d700d74 100644 --- a/src/workspaces/officerdev/src/hooks/useFileViewerPanels/useFileViewerPanels.tsx +++ b/src/workspaces/officerdev/src/hooks/useFileViewerPanels/useFileViewerPanels.tsx @@ -9,17 +9,7 @@ import { singleChatLayout, } from './layouts'; import { ViewerProvider, EphemeralProvider, Ephemeral2Provider, ChatEphemeralBody } from './Providers'; - -const EPHEMERAL_KEYS = [ - 'view', - 'ephemeral', - 'ephemeralRoot', - 'ephemeral2', - 'ephemeral2Root', - 'ephemeral2Auto', - 'chatContext', - 'chatType', -]; +import { withoutViewerParams } from '../../apps/FileBrowser/FileBrowserApp/files-route'; export const useFileViewerPanels = (): EphemeralPanels | null => { const [searchParams, setSearchParams] = useSearchParams(); @@ -82,12 +72,7 @@ export const useFileViewerPanels = (): EphemeralPanels | null => { * `onCloseEphemeral` directly below has always done. */ const onCloseViewer = useCallback( - () => - setSearchParams((prev) => { - const next = new URLSearchParams(prev); - EPHEMERAL_KEYS.forEach((k) => next.delete(k)); - return next; - }), + () => setSearchParams((prev) => new URLSearchParams(withoutViewerParams(prev.toString()))), [setSearchParams], );