From a8893f81f7b7c3ecb2dbb96f9ab3708cb241197a Mon Sep 17 00:00:00 2001 From: Ethan Graf Date: Tue, 28 Jul 2026 22:04:04 -0400 Subject: [PATCH] Derive the sidebar highlight from the open document, not from clicks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a document by any route other than clicking the sidebar tree left the sidebar highlight stale — internal markdown links, New File, open-in-split, and tab switching/cycling all did it. AppSidebar owned `selection` as local state, written in exactly two places: its own tree-click handler and its own inline-create. So the highlight answered "what did you last click in the sidebar", not "what document is open". Two sources of truth, and the sidebar's was an input event rather than state. Inverts the dependency. Workspaces now report the document they are actually showing via `WorkspaceProps.onActiveDocumentChange`; App holds that as `activeDocument` and passes it to AppSidebar, which renders it read-only and no longer tracks selection at all. The sidebar's click handler just calls onOpenEntry and lets the highlight follow the document opening. This is what makes the class of bug unreachable rather than fixed once: there is no longer anything a navigation feature must remember to update. A new way to reach a document keeps the highlight correct because the highlight is derived from what the workspace shows, not announced by whoever caused the change. The tiling side is `tabsModel.activeDocument(map, tileId)` — pure, and unit tested including the case that motivates the design: a tab switch is not an "open", yet must still move the highlight. Loading and errored slots count as shown, since the tile is displaying that request either way. Remaining seam, deliberately not closed: a *new workspace kind* could still forget to call onActiveDocumentChange. The contract is documented on the prop. Closing it structurally would require the shell to infer focus across a workspace's slots, which only the workspace knows. Also extracts `treeQueryKey` so the sidebar's file-tree query key has one definition; a second consumer is coming and a drifted copy would silently double the IPC. Co-Authored-By: Claude Opus 5 --- src/App.tsx | 13 +++++++++ src/components/AppSidebar.tsx | 36 +++++++++++++++--------- src/filesystem/treeQuery.ts | 11 ++++++++ src/workspaces/tiling/tabsModel.test.ts | 37 +++++++++++++++++++++++++ src/workspaces/tiling/tabsModel.ts | 21 ++++++++++++++ src/workspaces/tiling/tiling.tsx | 11 ++++++++ src/workspaces/workspace.tsx | 17 ++++++++++++ src/workspaces/zen/zen.tsx | 10 +++++++ 8 files changed, 143 insertions(+), 13 deletions(-) create mode 100644 src/filesystem/treeQuery.ts diff --git a/src/App.tsx b/src/App.tsx index 89c0e95..db44378 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -307,6 +307,17 @@ function AppShell() { setOpenRequestId((id) => id + 1); }, []); + /** + * The document the active workspace is showing, reported up by that + * workspace. This — not the last thing clicked in the sidebar — is what the + * sidebar highlights, so every route to a document (sidebar, wikilink, + * internal link, new file, split, tab switch, restored layout) keeps the + * highlight correct without having to know the sidebar exists. + */ + const [activeDocument, setActiveDocument] = useState( + null, + ); + // Split the active tile, then open the file — the new tile becomes active, so // the open lands in it. Splitting is a no-op when no tiling workspace is shown // (e.g. Zen), in which case the file simply opens normally. @@ -430,6 +441,7 @@ function AppShell() { onSelectProviderId={setSelectedProviderId} onOpenVaultManager={() => setVaultManagerOpen(true)} onOpenEntry={handleOpenEntry} + activeDocument={activeDocument} onOpenInSplit={handleOpenInSplit} unsyncedVaults={unsyncedVaults} onSyncRemote={setSyncTarget} @@ -449,6 +461,7 @@ function AppShell() { tilingHandleRef={tilingHandleRef} onNewFile={handleNewFile} onOpenEntry={handleOpenEntry} + onActiveDocumentChange={setActiveDocument} /> } /> diff --git a/src/components/AppSidebar.tsx b/src/components/AppSidebar.tsx index 9406c76..aa90421 100644 --- a/src/components/AppSidebar.tsx +++ b/src/components/AppSidebar.tsx @@ -11,6 +11,7 @@ import { import type { RemoteVault } from '../vault/registry'; import type { OpenDocRequest } from '../workspaces/workspace'; import { useContextMenu, type ContextMenuItem } from '../platform/contextMenu'; +import { treeQueryKey } from '../filesystem/treeQuery'; import { FileSystemTree } from './VaultTree'; import type { InlineCreateState } from './VaultTree'; @@ -35,21 +36,32 @@ type AppSidebarProps = { unsyncedVaults?: RemoteVault[]; /** Emitted when the user clicks an unsynced remote vault to sync it. */ onSyncRemote?: (vault: RemoteVault) => void; + /** + * The document the active workspace is currently showing, which the tree + * highlights. Read-only: the sidebar never sets it, so the highlight tracks + * what is open rather than what was last clicked here. + */ + activeDocument: Selection; }; +/** + * Which document the tree highlights. + * + * Derived, never owned here: it comes from the workspace's report of what it is + * actually showing (`App`'s `activeDocument`). The sidebar deliberately does + * NOT set this when you click a file — it calls `onOpenEntry` and lets the + * highlight follow the document actually opening. That is what keeps the + * highlight correct for documents opened from anywhere else (wikilinks, + * internal links, new file, tab switches) instead of only from this tree. + */ type Selection = { providerId: string; entryId: string } | null; const MIN_SIDEBAR_WIDTH = 112; const DEFAULT_SIDEBAR_WIDTH = 224; -function treeQueryKey(providerId: string) { - return ['tree', providerId] as const; -} - type ProviderSectionProps = { provider: FileSystemProvider; selection: Selection; - onSelect: (sel: Selection) => void; onOpenEntry?: (request: OpenDocRequest) => void; renameNodeId: string | null; renameValue: string; @@ -75,7 +87,6 @@ type ProviderSectionProps = { function ProviderSection({ provider, selection, - onSelect, onOpenEntry, renameNodeId, renameValue, @@ -113,10 +124,11 @@ function ProviderSection({ }); }, [provider, queryClient]); + // Only asks for the document to open. The highlight follows from the + // workspace actually showing it — see the `Selection` docblock. const handleSelect = useCallback( (node: { id: string; name: string; type: 'file' | 'folder' }) => { if (node.type === 'file') { - onSelect({ providerId: provider.id, entryId: node.id }); onOpenEntry?.({ providerId: provider.id, entryId: node.id, @@ -124,7 +136,7 @@ function ProviderSection({ }); } }, - [provider.id, onSelect, onOpenEntry], + [provider.id, onOpenEntry], ); const handleMove = useCallback( @@ -335,9 +347,9 @@ export function AppSidebar({ onOpenInSplit, unsyncedVaults, onSyncRemote, + activeDocument, }: AppSidebarProps) { const queryClient = useQueryClient(); - const [selection, setSelection] = useState(null); const [createError, setCreateError] = useState(null); const [inlineCreate, setInlineCreate] = useState( null, @@ -454,7 +466,6 @@ export function AppSidebar({ trimmed || undefined, ); setInlineCreate(null); - setSelection({ providerId: selectedProvider.id, entryId: entry.id }); onOpenEntry?.({ providerId: selectedProvider.id, entryId: entry.id, @@ -488,7 +499,7 @@ export function AppSidebar({ } } }, - [selectedProvider, inlineCreate, queryClient, onOpenEntry, setSelection], + [selectedProvider, inlineCreate, queryClient, onOpenEntry], ); const handleInlineCreateCancel = useCallback(() => { @@ -653,8 +664,7 @@ export function AppSidebar({ {selectedProvider ? ( { }); }); }); + +describe('activeDocument', () => { + it('reports the request shown by the active tab', () => { + const map = tileWithDocs('tile-1', ['a', 'b'], 'b'); + expect(activeDocument(map, 'tile-1')).toMatchObject({ entryId: 'b' }); + }); + + it('follows a tab switch — no navigation event required', () => { + // The whole point: switching tabs is not an "open", yet the shell's + // notion of the current document must still move. + const map = tileWithDocs('tile-1', ['a', 'b'], 'a'); + const switched = setActiveTab(map, 'tile-1', 'tab-b'); + expect(activeDocument(switched, 'tile-1')).toMatchObject({ entryId: 'b' }); + }); + + it('returns null for an empty tab, a missing tile, and an emptied tile', () => { + expect( + activeDocument(ensureTile({}, 'tile-1', 'tab-1'), 'tile-1'), + ).toBeNull(); + expect(activeDocument(tileWithDocs('tile-1', ['a']), 'nope')).toBeNull(); + const closed = closeTab(tileWithDocs('tile-1', ['a']), 'tile-1', 'tab-a'); + expect(activeDocument(closed.map, 'tile-1')).toBeNull(); + }); + + it('still reports the request while the document is loading or errored', () => { + const loading = tileWithDocs('tile-1', ['a']); + expect(activeDocument(loading, 'tile-1')).toMatchObject({ entryId: 'a' }); + + const errored = setTabSlot(loading, 'tile-1', 'tab-a', { + kind: 'error', + request: request('a'), + message: 'boom', + }); + expect(activeDocument(errored, 'tile-1')).toMatchObject({ entryId: 'a' }); + }); +}); diff --git a/src/workspaces/tiling/tabsModel.ts b/src/workspaces/tiling/tabsModel.ts index eaf5375..0a3f3b6 100644 --- a/src/workspaces/tiling/tabsModel.ts +++ b/src/workspaces/tiling/tabsModel.ts @@ -76,6 +76,27 @@ export function getActiveSlot( ); } +/** + * The document a tile is currently showing, or null when it shows none. + * + * This is what the shell renders its "currently open" affordances from (the + * sidebar highlight). Deriving it from tab state — rather than having each + * navigation action remember to announce itself — is what keeps those + * affordances correct for *every* way a document can come to be shown: + * sidebar clicks, wikilinks, internal links, new-file, tab switches, tab + * closes, and whatever gets added next. + * + * A loading or errored slot still counts as the shown document: the tile is + * displaying that request either way, and the highlight should follow it. + */ +export function activeDocument( + map: TileTabsMap, + tileId: string, +): OpenDocRequest | null { + const slot = getActiveSlot(map, tileId); + return slot.kind === 'empty' ? null : slot.request; +} + /** Activate an existing tab. No-op if the tile/tab is missing or already active. */ export function setActiveTab( map: TileTabsMap, diff --git a/src/workspaces/tiling/tiling.tsx b/src/workspaces/tiling/tiling.tsx index 82e99d1..3f7b738 100644 --- a/src/workspaces/tiling/tiling.tsx +++ b/src/workspaces/tiling/tiling.tsx @@ -23,6 +23,7 @@ import type { OpenDocRequest, WorkspaceProps } from '../workspace'; import { useTilingDocumentSlots } from './useTilingDocumentSlots'; import { TileTabStrip, type TabAction } from './TileTabStrip'; import { TileStartView } from './TileStartView'; +import { activeDocument } from './tabsModel'; import type { TileTab } from './tabsModel'; import { computeDropZone, @@ -284,6 +285,7 @@ export function TilingWorkspace({ tilingHandleRef, onNewFile, onOpenEntry, + onActiveDocumentChange, }: WorkspaceProps) { // Restore this vault's persisted layout once (null when there is none). const [persisted] = useState(() => loadLayout(instanceKey)); @@ -328,6 +330,15 @@ export function TilingWorkspace({ initialTiles: persisted?.tiles ?? null, }); + // Report the focused tile's document to the shell. Derived from tab state, so + // it stays right no matter *how* the document got there — an open intent, a + // tab switch, a tile focus change, a close, or a restored layout. + const shownDocument = active ? activeDocument(tileTabs, activeTileId) : null; + useEffect(() => { + if (!active) return; + onActiveDocumentChange?.(shownDocument); + }, [active, shownDocument, onActiveDocumentChange]); + // Persist layout changes for this vault (skips no-op rewrites). const lastSavedRef = useRef(null); useEffect(() => { diff --git a/src/workspaces/workspace.tsx b/src/workspaces/workspace.tsx index 830fa2d..9ae4872 100644 --- a/src/workspaces/workspace.tsx +++ b/src/workspaces/workspace.tsx @@ -76,6 +76,20 @@ export type WorkspaceProps = { * a tile. Because the clicked tile is already active, the open lands in it. */ onOpenEntry?: (request: OpenDocRequest) => void; + /** + * Report which document this workspace is currently showing, or null for + * none. Only the `active` workspace reports; inactive ones stay mounted and + * stay quiet. + * + * This is the shell's ONLY source of truth for "the current document", and + * the sidebar highlight is rendered from it. It is deliberately a report of + * *state* rather than a notification fired by each navigation action: a new + * way to reach a document (wikilinks, internal links, quick-open, tab + * cycling, restoring a layout) cannot forget to announce itself, because + * there is nothing to announce — the workspace simply keeps reporting what + * it shows. + */ + onActiveDocumentChange?: (request: OpenDocRequest | null) => void; }; export type WorkspaceComponent = ComponentType; @@ -130,6 +144,7 @@ type WorkspaceHostProps = { tilingHandleRef?: MutableRefObject; onNewFile?: () => void; onOpenEntry?: (request: OpenDocRequest) => void; + onActiveDocumentChange?: (request: OpenDocRequest | null) => void; }; /** @@ -150,6 +165,7 @@ export function WorkspaceHost({ tilingHandleRef, onNewFile, onOpenEntry, + onActiveDocumentChange, }: WorkspaceHostProps) { // Always render at least one slot — even before any vault is mounted — so the // empty start page is visible. Guarantee exactly one active vault key. @@ -187,6 +203,7 @@ export function WorkspaceHost({ } onNewFile={onNewFile} onOpenEntry={onOpenEntry} + onActiveDocumentChange={onActiveDocumentChange} /> diff --git a/src/workspaces/zen/zen.tsx b/src/workspaces/zen/zen.tsx index f07fa68..7c7924c 100644 --- a/src/workspaces/zen/zen.tsx +++ b/src/workspaces/zen/zen.tsx @@ -1,3 +1,5 @@ +import { useEffect } from 'react'; + import { DocumentSlotView } from '../../editors/DocumentSlotView'; import { useDocumentSlot } from '../../documents/useDocumentSlot'; import { useDocumentTitle } from '../../documents/useDocumentTitle'; @@ -10,6 +12,7 @@ export function ZenWorkspace({ openRequest, resolveProvider, onOpenEntry, + onActiveDocumentChange, }: WorkspaceProps) { const { state, close } = useDocumentSlot({ openRequestId, @@ -21,6 +24,13 @@ export function ZenWorkspace({ const handle = state.kind === 'open' ? state.handle : null; const title = useDocumentTitle(handle); + // Same contract as tiling: report what is shown, derived from slot state. + const shownDocument = active && state.kind !== 'empty' ? state.request : null; + useEffect(() => { + if (!active) return; + onActiveDocumentChange?.(shownDocument); + }, [active, shownDocument, onActiveDocumentChange]); + return ( <>
-- 2.51.2