Merge branch 'feat/58c3-history-nav'

Author: alphaleonis

web/src/App.svelte

@@ -15,7 +15,8 @@
   import { SelectionState } from "./lib/selection.svelte";
   import { DragState } from "./lib/drag.svelte";
   import type { DropZone } from "./lib/drag.svelte";
-  import { provideSelection, provideDrag, provideConfirmDialog, provideEditorOrchestration } from "./lib/contexts";
+  import { provideSelection, provideDrag, provideConfirmDialog, provideEditorOrchestration, provideHistoryNav } from "./lib/contexts";
+  import { createHistoryNav } from "./lib/composables/useHistoryNav.svelte";
   import { createConfirmDialog } from "./lib/composables/useConfirmDialog.svelte";
   import { createEditorOrchestration } from "./lib/composables/useEditorOrchestration.svelte";
   import { useKeyboardShortcuts } from "./lib/composables/useKeyboardShortcuts.svelte";
@@ -47,11 +48,32 @@
   const prefs = new Preferences();
   const selection = new SelectionState();
   const drag = new DragState();
+  // isBlocked reads editor/confirmDialog, created below — the closure is only
+  // invoked at popstate time, by which point both exist. While a blocking overlay
+  // is open, Back/Forward must not navigate the panel behind it (nibs-g1fy).
+  const nav = createHistoryNav({
+    selection,
+    isBlocked: () => editor.editorOpen || editor.typePickerOpen || confirmDialog.open,
+  });
   provideSelection(selection);
   provideDrag(drag);
+  provideHistoryNav(nav);
+
+  // Wire browser history: sync selection from the initial URL, then let
+  // Back/Forward drive selection via popstate. syncFromUrl reads/writes only
+  // non-reactive deps (window.location/history), so this effect runs once
+  // (no reactive reads). handlePopState is only registered as a listener
+  // here, never invoked in the effect body.
+  $effect(() => {
+    nav.syncFromUrl();
+    window.addEventListener("popstate", nav.handlePopState);
+    return () => window.removeEventListener("popstate", nav.handlePopState);
+  });
   const confirmDialog = createConfirmDialog();
   provideConfirmDialog(confirmDialog);
-  const editor = createEditorOrchestration({ client, selection });
+  // Pass `nav` so editor-save auto-select records a Back-stop (creating/opening
+  // a nib after save routes through history, per nibs-58c3 "all open paths").
+  const editor = createEditorOrchestration({ client, nav });
   provideEditorOrchestration(editor);
   // Collect unique tags from the query results via TreeTable callback
   let availableTags: string[] = $state([]);
@@ -101,9 +123,10 @@
   let contextMenuNib: TreeTableNib | null = $state(null);
 
   function handleRowContextMenu(nibId: string, event: MouseEvent, nib: TreeTableNib) {
-    // If the right-clicked nib is not in the selection, select it first
+    // If the right-clicked nib is not in the selection, select it first —
+    // route through nav so the URL/history stay in sync (nibs-58c3).
     if (!selection.isSelected(nibId)) {
-      selection.select(nibId);
+      nav.navigateToNib(nibId);
     }
     contextMenuNibId = nibId;
     contextMenuNib = nib;
@@ -114,6 +137,7 @@
   // --- Global keyboard shortcuts ---
   useKeyboardShortcuts({
     selection,
+    nav,
     editor,
     confirmDialog,
     mutations,
@@ -239,15 +263,23 @@
         collapsible={true}
         collapsedSize={0}
         onResize={handleDetailPaneResize}
-        onCollapse={() => { if (selection.panelOpen) requestAnimationFrame(() => selection.close()); }}
+        onCollapse={() => {
+          if (!selection.panelOpen) return;
+          // Capture the open nib at schedule time; only close if it's still the one
+          // showing when the frame fires. A different nib opened during the rAF window
+          // (near-impossible resize-drag race) must not be closed / push a spurious
+          // {nibId:null}. nibs-58c3.
+          const openedId = selection.selectedNibId;
+          requestAnimationFrame(() => { if (selection.selectedNibId === openedId) nav.closePanel(); });
+        }}
         bind:this={detailPaneComponent}
         data-testid="detail-pane"
       >
         {#if selection.panelOpen && selection.selectedNibId}
           <DetailPanel
             nibId={selection.selectedNibId}
-            onclose={() => selection.close()}
-            onnibselect={(nibId) => selection.select(nibId)}
+            onclose={() => nav.closePanel()}
+            onnibselect={(nibId) => nav.navigateToNib(nibId)}
             onedit={editor.handleEditNib}
             onaddchild={editor.handleAddChild}
           />

web/src/App.test.ts

@@ -78,6 +78,10 @@ const mockQueryStore = vi.mocked(queryStore);
 describe("App", () => {
   beforeEach(() => {
     mockQueryStore.mockClear();
+    // App now syncs selection from the URL on mount, and clicks push history.
+    // jsdom shares window.history/location across tests in a file, so reset to a
+    // clean URL before each test to mirror a fresh page load.
+    window.history.replaceState(null, "", "/");
   });
 
   it("renders with dark theme shell containing Toolbar and TreeTable", () => {

web/src/lib/components/RowContextMenu.svelte

@@ -3,7 +3,7 @@
   import { STATUSES, PRIORITIES } from "../constants";
   import { canHaveChildren } from "../typeHierarchy";
   import * as DropdownMenu from "$lib/components/ui/dropdown-menu/index.js";
-  import { useSelection, useConfirmDialog, useEditorOrchestration } from "$lib/contexts";
+  import { useSelection, useConfirmDialog, useEditorOrchestration, useHistoryNav } from "$lib/contexts";
   import { getMutationStore } from "$lib/mutations";
   import {
     setStatusBatch,
@@ -30,6 +30,7 @@
   const selection = useSelection();
   const confirmDialog = useConfirmDialog();
   const editor = useEditorOrchestration();
+  const nav = useHistoryNav();
   const mutations = getMutationStore();
 
   let isBulk = $derived(selectedCount > 1);
@@ -51,7 +52,8 @@
 
   function handleOpen() {
     if (nib) {
-      selection.select(nib.id);
+      // Route through nav so the URL/history stay in sync (nibs-58c3).
+      nav.navigateToNib(nib.id);
     }
   }
 

web/src/lib/components/TreeTable.svelte

@@ -10,7 +10,7 @@
   import TreeTableRow from "./TreeTableRow.svelte";
   import { Plus, Minus } from "@lucide/svelte";
   import type { DropZone } from "../drag.svelte";
-  import { useSelection, useDrag } from "../contexts";
+  import { useSelection, useDrag, useHistoryNav } from "../contexts";
   import { useColumnResize } from "../composables/useColumnResize.svelte";
   import { useTreeDrag } from "../composables/useTreeDrag.svelte";
   import { useKeyboardNav } from "../composables/useKeyboardNav.svelte";
@@ -49,6 +49,7 @@
 
   const selection = useSelection();
   const drag = useDrag();
+  const nav = useHistoryNav();
 
   // Resolve values: prefs takes precedence over individual props
   let resolvedFilter = $derived(resolveFilter(prefs, filter));
@@ -141,6 +142,15 @@
   let parentIds = $derived(tableData.parentIds);
   let visibleRowIds = $derived(rows.map(r => r.nib.id));
 
+  // Structural equality for two string sets (size + membership).
+  function sameSet(a: Set<string>, b: Set<string>): boolean {
+    if (a.size !== b.size) return false;
+    for (const x of a) {
+      if (!b.has(x)) return false;
+    }
+    return true;
+  }
+
   // --- Ensure-visible: expand collapsed ancestors and scroll into view ---
   $effect(() => {
     const nibId = selection.pendingEnsureVisibleId;
@@ -152,25 +162,44 @@
       nibMap.set(nib.id, nib);
     }
 
-    // If the nib doesn't exist in the dataset, clear and bail
+    // The nib isn't in the dataset. Two distinct cases must NOT be conflated:
+    //   - Query still loading (cold deep-link fires syncFromUrl before the
+    //     GraphQL result lands, so allNibs is []): keep the pending request so
+    //     the expand/scroll runs once data arrives (nibs-58c3 AC3). Reading
+    //     $result.fetching also subscribes the effect to re-run on settle.
+    //   - Query settled and the nib is genuinely absent (archived/bad URL):
+    //     clear and bail — there is nothing to scroll to.
     if (!nibMap.has(nibId)) {
-      selection.clearEnsureVisible();
+      if (!$result.fetching) {
+        selection.clearEnsureVisible();
+      }
       return;
     }
 
-    // If the nib is not currently visible, expand collapsed ancestors
+    // The nib is in the dataset but not currently visible. Try to expand its
+    // collapsed ancestors so it becomes reachable.
     if (!visibleRowIds.includes(nibId)) {
       const next = new Set(collapsedIds);
       let current = nibMap.get(nibId);
       while (current?.parentId) {
-        if (next.has(current.parentId)) {
-          next.delete(current.parentId);
-        }
+        next.delete(current.parentId);
         current = nibMap.get(current.parentId);
       }
+      // If expansion changes nothing yet the nib is still not visible, it is in
+      // the dataset but excluded from the visible rows — by an active client
+      // filter OR the current view level (e.g. viewLevel='milestones' drops a
+      // top-level epic via buildViewTree), regardless of collapse state.
+      // Ancestor-expansion can never reveal it, so clear and bail — otherwise
+      // reassigning `collapsedIds` to a new Set every pass would loop forever
+      // (effect_update_depth_exceeded).
+      if (sameSet(next, collapsedIds)) {
+        selection.clearEnsureVisible();
+        return;
+      }
+      // Ancestors were collapsed — expand them and let the effect re-run once
+      // visibleRowIds updates (either the nib appears, or the next pass hits
+      // the filtered-out guard above and clears).
       collapsedIds = next;
-      // After expanding, the nib will appear in the next reactive cycle.
-      // We return here and let the effect re-run once visibleRowIds updates.
       return;
     }
 
@@ -258,6 +287,7 @@
     toggleNode,
     getScrollContainer: () => scrollContainerEl ?? null,
     onDragKeyDown: treeDrag.onDragKeyDown,
+    navigateToNib: nav.navigateToNib,
   });
 
   // --- Event delegation helpers ---
@@ -296,7 +326,7 @@
     }
 
     if (action === "title") {
-      selection.select(nibId);
+      nav.navigateToNib(nibId);
       return; // Title click selects, not row click
     }
 
@@ -306,13 +336,20 @@
       return; // Don't fire row click for add-child
     }
 
-    // Default: row click with modifier handling
+    // Default: row click with modifier handling.
+    // Shift/Ctrl-Cmd are bulk-selection gestures — intentionally NOT routed
+    // through nav, so they record no Back/Forward history entry. Note that a
+    // collapse-to-exactly-one still opens the single-nib panel (and collapse-to-
+    // zero closes it) without a history push, so URL/history can lag selection
+    // after these gestures; that's accepted because multi-select is a bulk
+    // gesture, not detail-panel navigation (nibs-58c3).
+    // Only a plain click is treated as navigation.
     if (e.shiftKey) {
       selection.rangeSelect(nibId, visibleRowIds);
     } else if (e.ctrlKey || e.metaKey) {
       selection.toggleSelect(nibId);
     } else {
-      selection.select(nibId);
+      nav.navigateToNib(nibId);
     }
   }
 
@@ -322,7 +359,7 @@
     const nibId = getNibIdFromEvent(e);
     if (!nibId) return;
 
-    selection.select(nibId);
+    nav.navigateToNib(nibId);
   }
 
   function handleDelegatedContextMenu(e: MouseEvent) {

web/src/lib/components/TreeTable.test.ts

@@ -1174,6 +1174,74 @@ describe("TreeTable", () => {
         expect(sel.pendingEnsureVisibleId).toBeNull();
       });
     });
+
+    // Regression (nibs-58c3, review #1): ensureVisible for a nib that is in the
+    // dataset but excluded by an active client filter used to spin the effect
+    // forever — every pass reassigned collapsedIds to a fresh Set that could
+    // never make the filtered-out nib visible (effect_update_depth_exceeded).
+    // The effect must now detect that expansion changes nothing and clear.
+    it("settles (does not loop) when ensureVisible targets a filtered-out nib", async () => {
+      const sel = new SelectionState();
+      const nibs: TreeTableNib[] = [
+        makeTreeTableNib({ id: "nibs-m1", title: "Milestone", type: "milestone" }),
+        makeTreeTableNib({ id: "nibs-e1", title: "Epic", type: "epic", parentId: "nibs-m1" }),
+        makeTreeTableNib({ id: "nibs-bug1", title: "Matching Bug", type: "bug", parentId: "nibs-e1" }),
+        makeTreeTableNib({ id: "nibs-task1", title: "Filtered Task", type: "task", parentId: "nibs-e1" }),
+      ];
+
+      // Active client filter (type: bug) drops the task from `rows` while it
+      // stays in `allNibs`. Expanding ancestors can never reveal it.
+      setupWithNibs(nibs, { filter: { type: ["bug"] } }, { selection: sel });
+
+      expect(screen.getByText("Matching Bug")).toBeInTheDocument();
+      expect(screen.queryByText("Filtered Task")).not.toBeInTheDocument();
+
+      sel.ensureVisible("nibs-task1");
+
+      // Must terminate by clearing, not loop forever reassigning collapsedIds.
+      await waitFor(() => {
+        expect(sel.pendingEnsureVisibleId).toBeNull();
+      });
+
+      // Still filtered out — expansion did not (and cannot) reveal it.
+      expect(screen.queryByText("Filtered Task")).not.toBeInTheDocument();
+    });
+
+    // Regression (nibs-58c3, review #3): a cold deep-link runs syncFromUrl on
+    // mount before the GraphQL query resolves (allNibs === []). The effect must
+    // NOT clear the pending request as "absent" while the query is still
+    // fetching — it must wait for data, then expand/scroll.
+    it("keeps the pending request while the query is fetching, then resolves it once data arrives", async () => {
+      const sel = new SelectionState();
+      const nibs: TreeTableNib[] = [
+        makeTreeTableNib({ id: "nibs-m1", title: "Milestone", type: "milestone" }),
+        makeTreeTableNib({ id: "nibs-t1", title: "Deferred Task", type: "task", parentId: "nibs-m1" }),
+      ];
+
+      // Query in-flight, no data yet.
+      const store = writable<any>({ fetching: true, error: undefined, data: undefined, stale: false });
+      mockQueryStore.mockReturnValue(store as any);
+
+      // Deep-link request for a nib not yet in the (empty) dataset.
+      sel.ensureVisible("nibs-t1");
+
+      renderTreeTable(
+        { filter: {}, viewLevel: "milestones" as ViewLevel },
+        { selection: sel },
+      );
+      await tick();
+
+      // Still fetching → preserve the request (do NOT clear as absent).
+      expect(sel.pendingEnsureVisibleId).toBe("nibs-t1");
+
+      // Data arrives.
+      store.set({ fetching: false, error: undefined, data: { nibs }, stale: false });
+
+      // Now present and visible → effect clears after scrolling.
+      await waitFor(() => {
+        expect(sel.pendingEnsureVisibleId).toBeNull();
+      });
+    });
   });
 
   describe("reactive filter re-query", () => {

web/src/lib/composables/useEditorOrchestration.svelte.ts

@@ -11,7 +11,7 @@ import { toast } from "svelte-sonner";
 import { NIB_DETAIL_QUERY } from "../queries";
 import { getValidChildTypes } from "../typeHierarchy";
 import type { NibData } from "../components/EditorModal.svelte";
-import type { SelectionState } from "../selection.svelte";
+import type { HistoryNav } from "./useHistoryNav.svelte";
 
 export interface EditorOrchestrationState {
   readonly editorOpen: boolean;
@@ -34,9 +34,9 @@ export interface EditorOrchestrationState {
 
 export function createEditorOrchestration(opts: {
   client: Client;
-  selection: SelectionState;
+  nav: HistoryNav;
 }): EditorOrchestrationState {
-  const { client, selection } = opts;
+  const { client, nav } = opts;
 
   // Editor modal state
   let editorOpen = $state(false);
@@ -126,7 +126,10 @@ export function createEditorOrchestration(opts: {
   }
 
   function handleEditorSave(nibId: string) {
-    selection.select(nibId);
+    // Route through nav so the URL/history stay in sync (nibs-58c3). This
+    // intentionally records a Back-stop: creating/opening a nib after save is
+    // a navigation, per "all open paths flow through nav".
+    nav.navigateToNib(nibId);
   }
 
   function closeTypePicker() {