diff --git a/backend/canvas.py b/backend/canvas.py index 3456ffe..4a7116d 100644 --- a/backend/canvas.py +++ b/backend/canvas.py @@ -755,9 +755,12 @@ class SceneDocument: # loadChat (backend/chat_library.py), left None for a fresh session, and # reset to None by clear_for_load/new_chat below (a freshly loaded OR # freshly cleared scene has no id of its own yet until the next load/ - # save assigns one). Deliberately NOT part of scene_payload() - purely - # server-side bookkeeping the frontend never needs to read directly, - # same posture as gitlink_imported_root/code_sandbox_sandbox_id. + # save assigns one). The id ITSELF stays server-side - the frontend has + # no use for a chats.db row number, same posture as + # gitlink_imported_root/code_sandbox_sandbox_id. R7.5c does expose one + # derived bit of it as scene_payload()'s "hasSavedChat", because legacy's + # New Chat confirm skips only when the canvas is empty AND there is no + # current chat - a predicate the frontend cannot evaluate without it. current_chat_id: int | None = None _counter: itertools.count = field(default_factory=itertools.count, repr=False) @@ -2807,6 +2810,10 @@ def scene_payload(self) -> dict[str, Any]: "fadeConnectionsEnabled": self.fade_connections_enabled, "orthogonalRouting": self.orthogonal_routing, "smartGuides": self.smart_guides, + # R7.5c: the one derived bit of current_chat_id the frontend needs + # (see that field's own comment) - whether this scene corresponds + # to a saved chats.db row. Never the id itself. + "hasSavedChat": self.current_chat_id is not None, "dragFactor": self.drag_factor, "fontFamily": self.font_family, "fontSizePt": self.font_size_pt, diff --git a/backend/tests/test_canvas.py b/backend/tests/test_canvas.py index 5000202..6c66b3f 100644 --- a/backend/tests/test_canvas.py +++ b/backend/tests/test_canvas.py @@ -2204,6 +2204,24 @@ async def run(): asyncio.run(run()) +def test_has_saved_chat_tracks_current_chat_id_without_exposing_it(): + # R7.5c: the frontend's New Chat confirm needs legacy's full skip + # predicate ("empty canvas AND no current chat"), so scene_payload + # publishes the boolean shadow of current_chat_id - and only that. The + # row id itself stays server-side. + document = SceneDocument() + assert document.scene_payload()["hasSavedChat"] is False + + document.current_chat_id = 7 + payload = document.scene_payload() + assert payload["hasSavedChat"] is True + assert "currentChatId" not in payload + assert 7 not in payload.values() + + document.current_chat_id = None + assert document.scene_payload()["hasSavedChat"] is False + + # -- R4.4a: Generate/Regenerate Image - domain-level resolvers --------------- diff --git a/graphlink_app/graphlink_scene_payload.py b/graphlink_app/graphlink_scene_payload.py index 6cbc2b7..d4c540e 100644 --- a/graphlink_app/graphlink_scene_payload.py +++ b/graphlink_app/graphlink_scene_payload.py @@ -330,6 +330,11 @@ class SceneStatePayload: # R7.5b-3: the third and final canvas-visual parity fix - populated 1:1 # from backend/canvas.py's SceneDocument.smart_guides. smartGuides: bool + # R7.5c: True when this scene corresponds to a saved chats.db row - + # derived from backend/canvas.py's SceneDocument.current_chat_id, which + # itself stays server-side. Lets the frontend evaluate legacy's New Chat + # confirm-skip predicate ("empty canvas AND no current chat"). + hasSavedChat: bool dragFactor: float fontFamily: str fontSizePt: int diff --git a/web_ui/src/app/App.tsx b/web_ui/src/app/App.tsx index 54f4ce5..8cccf1f 100644 --- a/web_ui/src/app/App.tsx +++ b/web_ui/src/app/App.tsx @@ -1,10 +1,14 @@ -import { ReactFlowProvider } from "@xyflow/react"; +import { ReactFlowProvider, useReactFlow } from "@xyflow/react"; import { useEffect, useMemo, useState } from "react"; import { TOPIC_VALIDATORS } from "../lib/api-contract/topics"; import type { AppSettingsState } from "../lib/bridge-core/generated/app-settings-state"; +import { isTextEditable } from "../lib/bridge-core/textFocus"; import { ConnectionStatus, WsTransport, defaultWsUrl } from "../lib/ws/transport"; -import { SceneCanvas } from "./canvas/SceneCanvas"; +import { SceneCanvas, measuredNodeSize } from "./canvas/SceneCanvas"; import { SceneStore } from "./canvas/sceneStore"; +import { resolveTreeNavigationTarget, type TreeNavigationDirection } from "./canvas/treeNavigation"; +import { requestNewChat } from "./chrome/commands"; +import { isGatedWhileTyping, resolveShortcut, type ShortcutId } from "./chrome/shortcuts"; import { AboutDialog } from "./chrome/AboutDialog"; import { AppBar } from "./chrome/AppBar"; import { ChatLibraryDialog } from "./chrome/ChatLibraryDialog"; @@ -42,26 +46,112 @@ interface SettingsVisibilityState { showTokenCounter?: boolean; } -// Ctrl/Cmd+K opens the command palette, Ctrl/Cmd+F the canvas search - -// the conventional bindings the legacy islands' own keyPressEvent handlers -// used. Lives inside OverlayProvider (needs useOverlays()), so it is its -// own small component rather than inline in App(). -function GlobalShortcuts() { +/** + * The global keyboard shortcuts (Qt-removal plan R7.5c) - the SPA successor + * to legacy's QShortcut block (graphlink_window.py:307-318) and its + * AcceleratorForwardingFilter typing arbitration. Key matching and the + * suppression rule are pure functions in chrome/shortcuts.ts; branch + * traversal is pure in canvas/treeNavigation.ts. This component owns only + * the dispatch, which needs the live store/overlays/React Flow instance. + * + * Lives inside BOTH OverlayProvider and ReactFlowProvider (see App's tree), + * so useOverlays() and useReactFlow() are both legal here. + * + * R2 shipped Ctrl+K/Ctrl+F here ungated; the recon of legacy's + * GATED_SHORTCUTS confirmed Ctrl+F must be suppressed while typing and + * Ctrl+K must NOT be - both now go through the same table as the rest. + */ +function GlobalShortcuts({ store }: { store: SceneStore }) { const overlays = useOverlays(); + const reactFlow = useReactFlow(); + useEffect(() => { - function onKeyDown(event: KeyboardEvent) { - const mod = event.ctrlKey || event.metaKey; - if (mod && event.key.toLowerCase() === "k") { - event.preventDefault(); - overlays.toggle("palette", "dialog"); - } else if (mod && event.key.toLowerCase() === "f") { - event.preventDefault(); - overlays.toggle("search", "popover"); + function navigate(direction: TreeNavigationDirection) { + const scene = store.getScene(); + // store.getSelectedNodeId() mirrors React Flow's own selection, but + // only ever tracks ONE id - so re-derive from React Flow to honor + // legacy's "exactly one item selected" precondition, which a + // multi-selection must fail. + const selected = reactFlow.getNodes().filter((n) => n.selected); + const currentId = selected.length === 1 ? selected[0].id : null; + const targetId = resolveTreeNavigationTarget(scene, currentId, direction); + if (!targetId) return; // every legacy boundary is a pure no-op + + // Legacy clears the whole selection, selects the target, and centers + // the view on it (instant, zoom unchanged). Selecting through + // setNodes also flows out via onSelectionChange -> the store's + // selected-node mirror, so the composer's context anchor follows. + reactFlow.setNodes((nodes) => nodes.map((n) => ({ ...n, selected: n.id === targetId }))); + const target = scene.nodes.find((n) => n.id === targetId); + if (!target) return; + // Legacy centerOn() takes the item's bounding-rect CENTER, so the node + // half-size matters. It has to come from measuredNodeSize: React Flow's + // internal `measured` is empty here far more often than not (every + // scene snapshot rebuilds the node objects it is derived from - see + // that helper's own comment), and a naive `?? 0` would silently degrade + // to centering on the top-left corner, throwing the target half off + // screen at high zoom. Only a node absent from the DOM lands on the + // zero fallback, which is the legacy no-size behavior anyway. + const size = measuredNodeSize(reactFlow, targetId) ?? { width: 0, height: 0 }; + reactFlow.setCenter(target.x + size.width / 2, target.y + size.height / 2, { + zoom: reactFlow.getZoom(), + }); + } + + function dispatch(id: ShortcutId) { + switch (id) { + case "new-chat": + return requestNewChat(store); + case "toggle-library": + // Legacy's Ctrl+L is overlay_manager.toggle("library"), not open. + return overlays.toggle("library", "dialog"); + case "save-chat": + return store.saveChat(); + case "create-frame": + // Legacy createFrame/createContainer need only ONE eligible node + // (graphlink_scene.py:689-730) and silently no-op on zero - the + // shortcuts match that exactly. The palette's own Create Frame + // entry keeps its stricter 2+ gate; see the ledger note. + return applyGrouping("frame"); + case "create-container": + return applyGrouping("container"); + case "toggle-palette": + return overlays.toggle("palette", "dialog"); + case "toggle-search": + return overlays.toggle("search", "popover"); + case "navigate-up": + return navigate("up"); + case "navigate-down": + return navigate("down"); + case "navigate-left": + return navigate("left"); + case "navigate-right": + return navigate("right"); } } + + function applyGrouping(kind: "frame" | "container") { + const ids = reactFlow.getNodes().filter((n) => n.selected).map((n) => n.id); + if (ids.length === 0) return; // legacy: bare return, no message + if (kind === "frame") store.createFrame(ids); + else store.createContainer(ids); + } + + function onKeyDown(event: KeyboardEvent) { + const id = resolveShortcut(event); + if (!id) return; + // The AcceleratorForwardingFilter port: while a text input has focus, + // the gated shortcuts are left entirely alone so the keystroke reaches + // the field, and only the exempt ones (Save, palette) still fire. + if (isGatedWhileTyping(id) && isTextEditable(document.activeElement)) return; + event.preventDefault(); + dispatch(id); + } + document.addEventListener("keydown", onKeyDown); return () => document.removeEventListener("keydown", onKeyDown); - }, [overlays]); + }, [overlays, reactFlow, store]); + return null; } @@ -109,7 +199,7 @@ function App() { return ( - +
Graphlink diff --git a/web_ui/src/app/canvas/SceneCanvas.test.tsx b/web_ui/src/app/canvas/SceneCanvas.test.tsx index a5ac305..784e033 100644 --- a/web_ui/src/app/canvas/SceneCanvas.test.tsx +++ b/web_ui/src/app/canvas/SceneCanvas.test.tsx @@ -7,6 +7,7 @@ import { makeDebouncedViewportReport, toFlowEdges, toFlowNodes, + withPreservedSelection, type SceneFlowNode, } from "./SceneCanvas"; import { SceneStore, initialSceneState } from "./sceneStore"; @@ -1608,3 +1609,49 @@ describe("toFlowEdges (R7.5b-2 orthogonal routing)", () => { expect(edge.style).toEqual({ opacity: 0.08 }); }); }); + +// R7.5c: found live, not by a test - Ctrl+Arrow's setCenter round-trips a +// viewport report through the backend, the echoed snapshot rebuilt every +// node, and the selection the keystroke had just made disappeared. Without +// this, branch navigation worked for exactly one hop. +describe("withPreservedSelection (R7.5c snapshot-rebuild selection wipe)", () => { + const node = (id: string, selected?: boolean) => + ({ id, selected, position: { x: 0, y: 0 }, data: {} }) as unknown as SceneFlowNode; + + it("re-applies the selection onto the freshly rebuilt nodes", () => { + const rebuilt = [node("a"), node("b"), node("c")]; + const current = [node("a"), node("b", true), node("c")]; + const merged = withPreservedSelection(rebuilt, current); + expect(merged.map((n) => [n.id, !!n.selected])).toEqual([ + ["a", false], + ["b", true], + ["c", false], + ]); + }); + + it("preserves a multi-node selection, not just a single id", () => { + const merged = withPreservedSelection( + [node("a"), node("b"), node("c")], + [node("a", true), node("b"), node("c", true)], + ); + expect(merged.filter((n) => n.selected).map((n) => n.id)).toEqual(["a", "c"]); + }); + + it("returns the rebuilt array untouched when nothing was selected", () => { + const rebuilt = [node("a"), node("b")]; + expect(withPreservedSelection(rebuilt, [node("a"), node("b")])).toBe(rebuilt); + }); + + it("cannot resurrect a node the backend deleted - it is simply absent from the rebuild", () => { + const merged = withPreservedSelection([node("a")], [node("a"), node("gone", true)]); + expect(merged.map((n) => n.id)).toEqual(["a"]); + expect(merged.some((n) => n.selected)).toBe(false); + }); + + it("does not mutate the node objects it was handed", () => { + const current = [node("a", true)]; + const rebuilt = [node("a")]; + withPreservedSelection(rebuilt, current); + expect(rebuilt[0].selected).toBeUndefined(); + }); +}); diff --git a/web_ui/src/app/canvas/SceneCanvas.tsx b/web_ui/src/app/canvas/SceneCanvas.tsx index d808e39..9891e47 100644 --- a/web_ui/src/app/canvas/SceneCanvas.tsx +++ b/web_ui/src/app/canvas/SceneCanvas.tsx @@ -844,8 +844,10 @@ export function isOrthogonalEligible(sourceKind: string | undefined, targetKind: // R7.5b-3: one node's current rendered size in flow units, for smart-guide // rects - see the comment on CanvasInner's reactFlow hoist for why the // internal store alone is not enough and the DOM is the reliable fallback. -type MeasuredSizeSource = { getInternalNode: (id: string) => { measured?: { width?: number; height?: number } } | undefined }; -function measuredNodeSize(reactFlow: MeasuredSizeSource, id: string): { width: number; height: number } | null { +// Exported since R7.5c: App.tsx's Ctrl+Arrow navigation has to center on a +// node's MIDDLE, and hit the exact same empty-`measured` trap this solves. +export type MeasuredSizeSource = { getInternalNode: (id: string) => { measured?: { width?: number; height?: number } } | undefined }; +export function measuredNodeSize(reactFlow: MeasuredSizeSource, id: string): { width: number; height: number } | null { const measured = reactFlow.getInternalNode(id)?.measured; if (measured?.width !== undefined && measured?.height !== undefined) { return { width: measured.width, height: measured.height }; @@ -855,6 +857,34 @@ function measuredNodeSize(reactFlow: MeasuredSizeSource, id: string): { width: n return { width: el.offsetWidth, height: el.offsetHeight }; } +/** + * R7.5c: carry the current selection across a snapshot rebuild. + * + * toFlowNodes mints brand-new node objects from every scene snapshot, so + * anything React Flow keeps on the node object rather than in the scene - + * selection above all - is dropped unless copied over explicitly. + * + * Found live, not by a test: Ctrl+Arrow calls setCenter, setCenter fires + * onMove, onMove reports the viewport to the backend, the backend echoes a + * fresh scene, and the selection the keystroke had just made vanished about + * 300ms later. That left the NEXT Ctrl+Arrow with no single selected node, + * so branch-walking died after exactly one hop. The same wipe hits any + * selection that merely overlaps a snapshot - an autosave tick, a streaming + * token - which is why it is fixed here at the rebuild rather than inside + * the shortcut handler. + * + * A node the backend actually removed is simply absent from `rebuilt`, so a + * stale id can never resurrect one. + */ +export function withPreservedSelection( + rebuilt: SceneFlowNode[], + current: SceneFlowNode[], +): SceneFlowNode[] { + const selectedIds = new Set(current.filter((n) => n.selected).map((n) => n.id)); + if (selectedIds.size === 0) return rebuilt; + return rebuilt.map((n) => (selectedIds.has(n.id) ? { ...n, selected: true } : n)); +} + // Exported standalone for direct unit testing, same posture as toFlowNodes // above - R7.5b-1's fade-connections opacity logic doesn't need a mounted // to verify. @@ -960,7 +990,8 @@ function CanvasInner({ store }: { store: SceneStore }) { ); useEffect(() => { - if (!draggingRef.current) setNodes(toFlowNodes(scene, store)); + if (draggingRef.current) return; + setNodes((current) => withPreservedSelection(toFlowNodes(scene, store), current)); }, [scene, store]); const edges = useMemo(() => toFlowEdges(scene, hoveredEdgeId), [scene, hoveredEdgeId]); diff --git a/web_ui/src/app/canvas/sceneStore.test.ts b/web_ui/src/app/canvas/sceneStore.test.ts index 6b4ccf1..525ebc1 100644 --- a/web_ui/src/app/canvas/sceneStore.test.ts +++ b/web_ui/src/app/canvas/sceneStore.test.ts @@ -99,6 +99,7 @@ function validScenePayload(overrides: Record = {}) { fadeConnectionsEnabled: false, orthogonalRouting: false, smartGuides: false, + hasSavedChat: false, dragFactor: 0.5, fontFamily: "Segoe UI", fontSizePt: 9, diff --git a/web_ui/src/app/canvas/sceneStore.ts b/web_ui/src/app/canvas/sceneStore.ts index eb235d5..7a2c224 100644 --- a/web_ui/src/app/canvas/sceneStore.ts +++ b/web_ui/src/app/canvas/sceneStore.ts @@ -26,6 +26,7 @@ export const initialSceneState: SceneState = { fadeConnectionsEnabled: false, orthogonalRouting: false, smartGuides: false, + hasSavedChat: false, dragFactor: 1, fontFamily: "Segoe UI", fontSizePt: 9, diff --git a/web_ui/src/app/canvas/treeNavigation.test.ts b/web_ui/src/app/canvas/treeNavigation.test.ts new file mode 100644 index 0000000..b5d9f48 --- /dev/null +++ b/web_ui/src/app/canvas/treeNavigation.test.ts @@ -0,0 +1,160 @@ +import { describe, expect, it } from "vitest"; +import { + resolveTreeNavigationTarget, + type TreeNavigationScene, +} from "./treeNavigation"; + +// Direct coverage of the legacy _navigate_up/_down/_left/_right port - each +// documented rule (navigable-kind filter, x-position sibling ordering, +// leftmost-child, parent requirement, pure no-op boundaries) gets its own +// named test so a regression in any one fails loudly. + +/** + * chat-root + * / | \ + * chat-b chat-a chat-c (declared out of order on purpose; + * x=200 x=100 x=300 visual order is a, b, c) + */ +function branchScene(): TreeNavigationScene { + return { + nodes: [ + { id: "root", kind: "chat", x: 150 }, + { id: "b", kind: "chat", x: 200 }, + { id: "a", kind: "chat", x: 100 }, + { id: "c", kind: "chat", x: 300 }, + ], + edges: [ + { source: "root", target: "b" }, + { source: "root", target: "a" }, + { source: "root", target: "c" }, + ], + }; +} + +describe("resolveTreeNavigationTarget", () => { + it("no-ops when nothing is selected", () => { + expect(resolveTreeNavigationTarget(branchScene(), null, "down")).toBeNull(); + }); + + it("no-ops when the selected node is not a navigable kind", () => { + const scene: TreeNavigationScene = { + nodes: [ + { id: "chat-1", kind: "chat", x: 0 }, + { id: "code-1", kind: "code", x: 0 }, + ], + edges: [{ source: "chat-1", target: "code-1" }], + }; + // Standing on the code node: legacy's _get_single_selected_node filter + // rejects it outright, so even "up" (which has a real parent) no-ops. + expect(resolveTreeNavigationTarget(scene, "code-1", "up")).toBeNull(); + }); + + it("moves up to the parent", () => { + expect(resolveTreeNavigationTarget(branchScene(), "a", "up")).toBe("root"); + }); + + it("no-ops going up from a root (no parent)", () => { + expect(resolveTreeNavigationTarget(branchScene(), "root", "up")).toBeNull(); + }); + + it("moves down to the LEFTMOST child by x, not the first-created one", () => { + // "b" is the first edge declared, but "a" sits further left. + expect(resolveTreeNavigationTarget(branchScene(), "root", "down")).toBe("a"); + }); + + it("no-ops going down from a leaf (no children)", () => { + expect(resolveTreeNavigationTarget(branchScene(), "a", "down")).toBeNull(); + }); + + it("moves left/right through siblings in x order", () => { + const scene = branchScene(); + expect(resolveTreeNavigationTarget(scene, "a", "right")).toBe("b"); + expect(resolveTreeNavigationTarget(scene, "b", "right")).toBe("c"); + expect(resolveTreeNavigationTarget(scene, "c", "left")).toBe("b"); + expect(resolveTreeNavigationTarget(scene, "b", "left")).toBe("a"); + }); + + it("no-ops at both sibling boundaries - never wraps around", () => { + const scene = branchScene(); + expect(resolveTreeNavigationTarget(scene, "a", "left")).toBeNull(); + expect(resolveTreeNavigationTarget(scene, "c", "right")).toBeNull(); + }); + + it("no-ops sideways from a parentless root - siblings need a parent", () => { + const scene = branchScene(); + expect(resolveTreeNavigationTarget(scene, "root", "left")).toBeNull(); + expect(resolveTreeNavigationTarget(scene, "root", "right")).toBeNull(); + }); + + it("skips non-navigable children entirely when moving down", () => { + // A code node sits furthest left, but was never in legacy's children + // graph - so "down" must land on the leftmost NAVIGABLE child instead. + const scene: TreeNavigationScene = { + nodes: [ + { id: "root", kind: "chat", x: 0 }, + { id: "code-1", kind: "code", x: 10 }, + { id: "chat-1", kind: "chat", x: 50 }, + ], + edges: [ + { source: "root", target: "code-1" }, + { source: "root", target: "chat-1" }, + ], + }; + expect(resolveTreeNavigationTarget(scene, "root", "down")).toBe("chat-1"); + }); + + it("excludes non-navigable siblings from the left/right ordering", () => { + const scene: TreeNavigationScene = { + nodes: [ + { id: "root", kind: "chat", x: 0 }, + { id: "chat-1", kind: "chat", x: 100 }, + { id: "img-1", kind: "image", x: 200 }, + { id: "chat-2", kind: "chat", x: 300 }, + ], + edges: [ + { source: "root", target: "chat-1" }, + { source: "root", target: "img-1" }, + { source: "root", target: "chat-2" }, + ], + }; + // The image node sits between them visually but must be stepped over. + expect(resolveTreeNavigationTarget(scene, "chat-1", "right")).toBe("chat-2"); + }); + + it("treats conversation and html nodes as navigable, like legacy", () => { + const scene: TreeNavigationScene = { + nodes: [ + { id: "root", kind: "chat", x: 0 }, + { id: "conv", kind: "conversation", x: 10 }, + { id: "html", kind: "html", x: 20 }, + ], + edges: [ + { source: "root", target: "conv" }, + { source: "conv", target: "html" }, + ], + }; + expect(resolveTreeNavigationTarget(scene, "root", "down")).toBe("conv"); + expect(resolveTreeNavigationTarget(scene, "conv", "down")).toBe("html"); + expect(resolveTreeNavigationTarget(scene, "html", "up")).toBe("conv"); + }); + + it("falls back to declaration order for siblings tied on x (stable sort)", () => { + const scene: TreeNavigationScene = { + nodes: [ + { id: "root", kind: "chat", x: 0 }, + { id: "first", kind: "chat", x: 50 }, + { id: "second", kind: "chat", x: 50 }, + ], + edges: [ + { source: "root", target: "first" }, + { source: "root", target: "second" }, + ], + }; + expect(resolveTreeNavigationTarget(scene, "root", "down")).toBe("first"); + expect(resolveTreeNavigationTarget(scene, "first", "right")).toBe("second"); + }); + + it("no-ops for a selected id that isn't in the scene at all", () => { + expect(resolveTreeNavigationTarget(branchScene(), "ghost", "up")).toBeNull(); + }); +}); diff --git a/web_ui/src/app/canvas/treeNavigation.ts b/web_ui/src/app/canvas/treeNavigation.ts new file mode 100644 index 0000000..2607c92 --- /dev/null +++ b/web_ui/src/app/canvas/treeNavigation.ts @@ -0,0 +1,127 @@ +/** + * Ctrl+Arrow branch navigation (Qt-removal plan R7.5c) - a pure port of + * graphlink_window_navigation.py's _navigate_up/_navigate_down/_navigate_left/ + * _navigate_right (lines 65-91) plus their shared _get_single_selected_node + * filter (graphlink_window.py:1419-1422). Framework-free so the whole + * traversal is unit-testable without mounting a canvas - same posture as + * smartGuides.ts / scaleDragPosition / toFlowNodes. + * + * Ported semantics, each confirmed against the legacy source: + * - Navigation only STARTS from a single selected node whose kind is one of + * the three legacy "navigable" classes (ChatNode/ConversationNode/ + * HtmlViewNode). Multi-selection, no selection, or any other kind -> no-op. + * - Targets are restricted to those same kinds. This is the faithful + * translation of legacy's `children` LIST, which only ever contained those + * three classes: code/document/image/thinking/chart/note nodes are real + * EDGE targets in this backend but were never in legacy's children graph + * (confirmed: CodeNode has no `children` attribute at all), so they stay + * unreachable by Ctrl+Arrow here too. + * - Up -> parent. Down -> the LEFTMOST child by x-position (legacy sorts + * children by pos().x() on every keypress, so order is purely visual, not + * creation order). Left/Right -> previous/next sibling in that same + * x-sorted order, and both REQUIRE a parent (legacy guards on + * current.parent_node). + * - Every boundary is a pure no-op: no wrap-around, no beep, no message. + * No parent -> Up/Left/Right do nothing; no children -> Down does nothing; + * leftmost/rightmost sibling -> Left/Right do nothing. + * + * Ties in x fall back to insertion order because both legacy's Python + * `sorted` and JS `Array.prototype.sort` are stable. + */ + +export type TreeNavigationDirection = "up" | "down" | "left" | "right"; + +/** The three legacy classes that carry a `children`/`parent_node` graph. */ +const NAVIGABLE_KINDS = new Set(["chat", "conversation", "html"]); + +export interface TreeNavigationNode { + id: string; + kind: string; + x: number; +} + +export interface TreeNavigationEdge { + source: string; + target: string; +} + +export interface TreeNavigationScene { + nodes: TreeNavigationNode[]; + edges: TreeNavigationEdge[]; +} + +function navigableById(scene: TreeNavigationScene): Map { + const map = new Map(); + for (const node of scene.nodes) { + if (NAVIGABLE_KINDS.has(node.kind)) map.set(node.id, node); + } + return map; +} + +/** The navigable parent of `id`, or null. A node has at most one parent edge + * in this model; if several exist the first is taken, matching legacy's + * single `parent_node` reference. */ +function parentOf( + scene: TreeNavigationScene, + navigable: Map, + id: string, +): TreeNavigationNode | null { + for (const edge of scene.edges) { + if (edge.target !== id) continue; + const parent = navigable.get(edge.source); + if (parent) return parent; + } + return null; +} + +/** Navigable children of `id`, sorted leftmost-first by x - legacy's + * `sorted(current.children, key=lambda c: c.pos().x())`. */ +function childrenOf( + scene: TreeNavigationScene, + navigable: Map, + id: string, +): TreeNavigationNode[] { + const children: TreeNavigationNode[] = []; + for (const edge of scene.edges) { + if (edge.source !== id) continue; + const child = navigable.get(edge.target); + if (child) children.push(child); + } + return children.sort((a, b) => a.x - b.x); +} + +/** + * The id of the node Ctrl+ should move selection to, or null when + * legacy would no-op. `selectedNodeId` is the single currently-selected node + * (null for none, and callers must pass null for a multi-selection - see + * legacy's exactly-one-item filter). + */ +export function resolveTreeNavigationTarget( + scene: TreeNavigationScene, + selectedNodeId: string | null, + direction: TreeNavigationDirection, +): string | null { + if (!selectedNodeId) return null; + const navigable = navigableById(scene); + const current = navigable.get(selectedNodeId); + if (!current) return null; + + if (direction === "up") { + return parentOf(scene, navigable, current.id)?.id ?? null; + } + + if (direction === "down") { + return childrenOf(scene, navigable, current.id)[0]?.id ?? null; + } + + // left/right: siblings are the PARENT's x-sorted children, so a parentless + // (root) node has no siblings to move between - legacy guards on + // current.parent_node before even building the list. + const parent = parentOf(scene, navigable, current.id); + if (!parent) return null; + const siblings = childrenOf(scene, navigable, parent.id); + const index = siblings.findIndex((s) => s.id === current.id); + if (index === -1) return null; + const target = direction === "left" ? siblings[index - 1] : siblings[index + 1]; + return target?.id ?? null; +} diff --git a/web_ui/src/app/chrome/commands.test.ts b/web_ui/src/app/chrome/commands.test.ts index d18acbc..ca07e5c 100644 --- a/web_ui/src/app/chrome/commands.test.ts +++ b/web_ui/src/app/chrome/commands.test.ts @@ -9,12 +9,15 @@ vi.mock("../canvas/exportCanvasPng", () => ({ exportCanvasAsPng: (...args: unknown[]) => exportCanvasAsPngMock(...args), })); -import { buildCommands } from "./commands"; +import { buildCommands, requestNewChat } from "./commands"; import { initialSceneState } from "../canvas/sceneStore"; import type { OverlayContextValue } from "../overlays/overlays"; -function makeStore(nodes: Array<{ id: string; x: number; y: number; title: string; kind: string }> = []) { - const scene = { ...initialSceneState, nodes, pins: [] }; +function makeStore( + nodes: Array<{ id: string; x: number; y: number; title: string; kind: string }> = [], + hasSavedChat = false, +) { + const scene = { ...initialSceneState, nodes, pins: [], hasSavedChat }; return { getScene: () => scene, organizeNodes: vi.fn(), @@ -25,6 +28,7 @@ function makeStore(nodes: Array<{ id: string; x: number; y: number; title: strin createFrame: vi.fn(), createContainer: vi.fn(), newChat: vi.fn(), + saveChat: vi.fn(), }; } @@ -153,6 +157,8 @@ describe("buildCommands", () => { }); it("new-chat is always enabled and calls store.newChat (R7.5a)", () => { + // Empty scene -> R7.5c's confirm is skipped entirely, so this still + // reaches newChat without any modal. const store = makeStore(); // @ts-expect-error - test double const commands = buildCommands(store, makeRf(), makeOverlays()); @@ -162,6 +168,17 @@ describe("buildCommands", () => { expect(store.newChat).toHaveBeenCalledTimes(1); }); + it("save-chat is always enabled and calls store.saveChat (R7.5c)", () => { + const store = makeStore(); + // @ts-expect-error - test double + const commands = buildCommands(store, makeRf(), makeOverlays()); + const save = commands.find((c) => c.id === "save-chat")!; + expect(save.name).toBe("Save Chat"); + expect(save.enabled()).toBe(true); + save.run(); + expect(store.saveChat).toHaveBeenCalledTimes(1); + }); + it("focus-selection is disabled with no selection and calls fitView scoped to the selected ids when run (R7.5a)", () => { const store = makeStore([ { id: "n0", x: 0, y: 0, title: "A", kind: "placeholder" }, @@ -219,3 +236,46 @@ describe("buildCommands", () => { expect(exportCanvasAsPngMock).toHaveBeenCalledWith(rf, "--gl-surface-window"); }); }); + +// R7.5c: the New Chat confirm, restored from legacy's blocking QMessageBox. +describe("requestNewChat", () => { + it("skips the confirm on an empty, never-saved canvas - nothing to lose", () => { + const store = makeStore([]); + const confirmFn = vi.fn(() => true); + // @ts-expect-error - test double + requestNewChat(store, confirmFn); + expect(confirmFn).not.toHaveBeenCalled(); + expect(store.newChat).toHaveBeenCalledTimes(1); + }); + + it("still asks when the canvas is empty but the scene IS a saved chat", () => { + // Legacy's skip needs BOTH halves (graphlink_window.py:1429). Emptying a + // loaded chat and hitting Ctrl+T must not silently detach it: newChat() + // drops current_chat_id, so the next Save would INSERT a new row instead + // of updating the one the user believes they are editing. + const store = makeStore([], true); + const confirmFn = vi.fn(() => false); + // @ts-expect-error - test double + requestNewChat(store, confirmFn); + expect(confirmFn).toHaveBeenCalledOnce(); + expect(store.newChat).not.toHaveBeenCalled(); + }); + + it("asks before discarding a canvas that has content, and proceeds on yes", () => { + const store = makeStore([{ id: "n0", x: 0, y: 0, title: "A", kind: "chat" }]); + const confirmFn = vi.fn(() => true); + // @ts-expect-error - test double + requestNewChat(store, confirmFn); + expect(confirmFn).toHaveBeenCalledWith("Start a new chat? Any unsaved changes will be lost."); + expect(store.newChat).toHaveBeenCalledTimes(1); + }); + + it("does NOT clear the canvas when the confirm is declined", () => { + const store = makeStore([{ id: "n0", x: 0, y: 0, title: "A", kind: "chat" }]); + const confirmFn = vi.fn(() => false); + // @ts-expect-error - test double + requestNewChat(store, confirmFn); + expect(confirmFn).toHaveBeenCalledOnce(); + expect(store.newChat).not.toHaveBeenCalled(); + }); +}); diff --git a/web_ui/src/app/chrome/commands.ts b/web_ui/src/app/chrome/commands.ts index 884b536..94518ba 100644 --- a/web_ui/src/app/chrome/commands.ts +++ b/web_ui/src/app/chrome/commands.ts @@ -22,6 +22,40 @@ export interface PaletteCommand { enabled: () => boolean; } +/** + * R7.5c: New Chat, with legacy's confirm step restored. + * graphlink_window.py's new_chat (1427-1442) always showed a blocking + * QMessageBox ("Start a new chat? Any unsaved changes will be lost.", + * default No) before clearing the canvas, and only skipped it when there was + * genuinely nothing to lose. R7.5a shipped the palette command without that + * guard; both the palette and R7.5c's Ctrl+T now route through here so the + * confirm can never apply to one surface and not the other. + * + * Legacy's skip condition is BOTH halves of "scene is empty AND there is no + * current chat id" (graphlink_window.py:1429). The first draft of this used + * only the empty-canvas half, which quietly inverted the guard in the unsafe + * direction: emptying a loaded chat and pressing Ctrl+T skipped the confirm, + * and newChat() drops current_chat_id, so the next Save would INSERT a new + * row rather than update the loaded one - a silent detach from the chat the + * user thought they were in. hasSavedChat (R7.5c) is the wire-side derivation + * of current_chat_id added for exactly this predicate; the id itself stays + * server-side. + * + * confirmFn is injectable purely so tests never touch a real modal; the + * default is the browser's own blocking confirm, which matches legacy's + * modal semantics (synchronous, blocks until answered) - the command + * registry's run() is sync, so an async dialog could not be awaited here. + */ +export function requestNewChat( + store: SceneStore, + confirmFn: (message: string) => boolean = (message) => window.confirm(message), +): void { + const scene = store.getScene(); + const nothingToLose = scene.nodes.length === 0 && !scene.hasSavedChat; + if (!nothingToLose && !confirmFn("Start a new chat? Any unsaved changes will be lost.")) return; + store.newChat(); +} + export function buildCommands( store: SceneStore, // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -185,7 +219,17 @@ export function buildCommands( id: "new-chat", name: "New Chat", aliases: ["new session", "clear canvas", "start over"], - run: () => store.newChat(), + run: () => requestNewChat(store), + enabled: () => true, + }, + { + // R7.5c: Save had a real app-bar button and a real store method since + // R6.5 but no palette entry - a genuine gap this increment closes, + // since Ctrl+S now needs a palette twin to stay discoverable. + id: "save-chat", + name: "Save Chat", + aliases: ["save", "save session", "persist chat"], + run: () => store.saveChat(), enabled: () => true, }, { diff --git a/web_ui/src/app/chrome/shortcuts.test.ts b/web_ui/src/app/chrome/shortcuts.test.ts new file mode 100644 index 0000000..663d88a --- /dev/null +++ b/web_ui/src/app/chrome/shortcuts.test.ts @@ -0,0 +1,93 @@ +import { describe, expect, it } from "vitest"; +import { isGatedWhileTyping, resolveShortcut, type ShortcutId } from "./shortcuts"; + +function key(k: string, mods: Partial<{ ctrl: boolean; meta: boolean; shift: boolean; alt: boolean }> = {}) { + return { + key: k, + ctrlKey: mods.ctrl ?? true, + metaKey: mods.meta ?? false, + shiftKey: mods.shift ?? false, + altKey: mods.alt ?? false, + }; +} + +describe("resolveShortcut", () => { + it("maps each legacy binding to its shortcut id", () => { + expect(resolveShortcut(key("t"))).toBe("new-chat"); + expect(resolveShortcut(key("l"))).toBe("toggle-library"); + expect(resolveShortcut(key("s"))).toBe("save-chat"); + expect(resolveShortcut(key("k"))).toBe("toggle-palette"); + expect(resolveShortcut(key("f"))).toBe("toggle-search"); + expect(resolveShortcut(key("g"))).toBe("create-frame"); + expect(resolveShortcut(key("ArrowUp"))).toBe("navigate-up"); + expect(resolveShortcut(key("ArrowDown"))).toBe("navigate-down"); + expect(resolveShortcut(key("ArrowLeft"))).toBe("navigate-left"); + expect(resolveShortcut(key("ArrowRight"))).toBe("navigate-right"); + }); + + it("treats Ctrl+Shift+G as Container - a separate legacy binding from Ctrl+G", () => { + expect(resolveShortcut(key("g", { shift: true }))).toBe("create-container"); + expect(resolveShortcut(key("G", { shift: true }))).toBe("create-container"); + }); + + it("is case-insensitive, since Shift/CapsLock change event.key's case", () => { + expect(resolveShortcut(key("T"))).toBe("new-chat"); + }); + + it("accepts Cmd as well as Ctrl", () => { + expect(resolveShortcut(key("t", { ctrl: false, meta: true }))).toBe("new-chat"); + }); + + it("ignores keys with no Ctrl/Cmd modifier", () => { + expect(resolveShortcut(key("t", { ctrl: false }))).toBeNull(); + expect(resolveShortcut(key("ArrowUp", { ctrl: false }))).toBeNull(); + }); + + it("ignores Alt combinations so AltGr-produced characters still type", () => { + expect(resolveShortcut(key("t", { alt: true }))).toBeNull(); + expect(resolveShortcut(key("g", { alt: true, shift: true }))).toBeNull(); + }); + + it("ignores Shift on bindings legacy did not define with Shift", () => { + expect(resolveShortcut(key("t", { shift: true }))).toBeNull(); + expect(resolveShortcut(key("s", { shift: true }))).toBeNull(); + expect(resolveShortcut(key("ArrowUp", { shift: true }))).toBeNull(); + }); + + it("returns null for unbound keys", () => { + expect(resolveShortcut(key("q"))).toBeNull(); + expect(resolveShortcut(key("Enter"))).toBeNull(); + }); +}); + +describe("isGatedWhileTyping", () => { + // The exact membership of legacy's GATED_SHORTCUTS + // (graphlink_web_island_host.py:735-745), which legacy itself + // contract-tests. Mirrored here so a future edit to the set has to be + // deliberate rather than incidental. + const GATED: ShortcutId[] = [ + "new-chat", + "toggle-library", + "toggle-search", + "create-frame", + "create-container", + "navigate-up", + "navigate-down", + "navigate-left", + "navigate-right", + ]; + const EXEMPT: ShortcutId[] = ["save-chat", "toggle-palette"]; + + it.each(GATED)("suppresses %s while a text field has focus", (id) => { + expect(isGatedWhileTyping(id)).toBe(true); + }); + + it.each(EXEMPT)("lets %s through while typing - a documented legacy exemption", (id) => { + expect(isGatedWhileTyping(id)).toBe(false); + }); + + it("gates exactly the 9 legacy combinations, no more and no fewer", () => { + const all: ShortcutId[] = [...GATED, ...EXEMPT]; + expect(all.filter(isGatedWhileTyping)).toHaveLength(9); + }); +}); diff --git a/web_ui/src/app/chrome/shortcuts.ts b/web_ui/src/app/chrome/shortcuts.ts new file mode 100644 index 0000000..6352f50 --- /dev/null +++ b/web_ui/src/app/chrome/shortcuts.ts @@ -0,0 +1,110 @@ +/** + * Global keyboard shortcuts (Qt-removal plan R7.5c) - the SPA successor to + * the QShortcut block at graphlink_window.py:307-318 plus the + * AcceleratorForwardingFilter focus arbitration at + * graphlink_web_island_host.py:692-755. + * + * Key MATCHING and the typing-suppression RULE live here as pure functions so + * both are unit-testable without mounting the app; the dispatch half (which + * needs the scene store, overlay context and React Flow instance) stays in + * App.tsx's GlobalShortcuts. Same split posture as smartGuides.ts / + * treeNavigation.ts. + * + * Modifier note: legacy compared raw Qt ControlModifier and shipped + * Windows-only (no Cmd handling anywhere in that codebase). This keeps the + * SPA's pre-existing ctrl-or-meta matching so the bindings also work on a + * Mac keyboard - a deliberate superset of legacy, not a divergence in + * behavior on the platform legacy actually targeted. + */ + +export type ShortcutId = + | "new-chat" + | "toggle-library" + | "save-chat" + | "create-frame" + | "create-container" + | "toggle-palette" + | "toggle-search" + | "navigate-up" + | "navigate-down" + | "navigate-left" + | "navigate-right"; + +/** + * The shortcuts legacy SUPPRESSES while a text input has focus - ported + * verbatim from AcceleratorForwardingFilter's GATED_SHORTCUTS + * (graphlink_web_island_host.py:735-745), whose exact membership is itself + * contract-tested in graphlink_app/tests/test_keyboard_arbitration.py. + * + * Save and the command palette are deliberately ABSENT: legacy documents both + * as intentional exemptions - Ctrl+S is non-destructive and reflexively + * expected mid-sentence, and Ctrl+K is the palette's own summon key (its + * handler was made idempotent specifically so the exemption is safe). + */ +const GATED_WHILE_TYPING = new Set([ + "new-chat", + "toggle-library", + "toggle-search", + "create-frame", + "create-container", + "navigate-up", + "navigate-down", + "navigate-left", + "navigate-right", +]); + +export function isGatedWhileTyping(id: ShortcutId): boolean { + return GATED_WHILE_TYPING.has(id); +} + +/** Just the modifier/key shape of a keyboard event - so tests can pass plain + * objects instead of constructing real KeyboardEvents. */ +export interface ShortcutKeyEvent { + key: string; + ctrlKey: boolean; + metaKey: boolean; + shiftKey: boolean; + altKey: boolean; +} + +const ARROW_DIRECTIONS: Record = { + ArrowUp: "navigate-up", + ArrowDown: "navigate-down", + ArrowLeft: "navigate-left", + ArrowRight: "navigate-right", +}; + +/** + * Which shortcut (if any) this key event means. Returns null for anything + * unbound, so the caller leaves the event completely alone. + * + * Alt is required to be UP: legacy matched an exact modifier mask, and + * letting Ctrl+Alt through would also swallow AltGr combinations that + * produce real characters on several keyboard layouts. + */ +export function resolveShortcut(event: ShortcutKeyEvent): ShortcutId | null { + if (!(event.ctrlKey || event.metaKey)) return null; + if (event.altKey) return null; + + const arrow = ARROW_DIRECTIONS[event.key]; + if (arrow) return event.shiftKey ? null : arrow; + + switch (event.key.toLowerCase()) { + case "t": + return event.shiftKey ? null : "new-chat"; + case "l": + return event.shiftKey ? null : "toggle-library"; + case "s": + return event.shiftKey ? null : "save-chat"; + case "k": + return event.shiftKey ? null : "toggle-palette"; + case "f": + return event.shiftKey ? null : "toggle-search"; + // Ctrl+G -> Frame, Ctrl+Shift+G -> Container: two distinct legacy + // bindings (graphlink_window.py:312-313), not one with a modifier. + case "g": + return event.shiftKey ? "create-container" : "create-frame"; + default: + return null; + } +} diff --git a/web_ui/src/lib/bridge-core/generated/scene-state.schema.json b/web_ui/src/lib/bridge-core/generated/scene-state.schema.json index 6cc02d0..9210fce 100644 --- a/web_ui/src/lib/bridge-core/generated/scene-state.schema.json +++ b/web_ui/src/lib/bridge-core/generated/scene-state.schema.json @@ -40,6 +40,9 @@ "fontSizePt": { "type": "integer" }, + "hasSavedChat": { + "type": "boolean" + }, "minCompatibleSchemaVersion": { "type": "integer" }, @@ -520,6 +523,7 @@ "fadeConnectionsEnabled", "orthogonalRouting", "smartGuides", + "hasSavedChat", "dragFactor", "fontFamily", "fontSizePt", diff --git a/web_ui/src/lib/bridge-core/generated/scene-state.ts b/web_ui/src/lib/bridge-core/generated/scene-state.ts index 02d26f6..519c387 100644 --- a/web_ui/src/lib/bridge-core/generated/scene-state.ts +++ b/web_ui/src/lib/bridge-core/generated/scene-state.ts @@ -134,6 +134,7 @@ export interface SceneState { fadeConnectionsEnabled: boolean; orthogonalRouting: boolean; smartGuides: boolean; + hasSavedChat: boolean; dragFactor: number; fontFamily: string; fontSizePt: number; @@ -730,6 +731,11 @@ function checkSceneState(value: unknown, path: string, errors: string[]): void { if (fieldValue === undefined || fieldValue === null) errors.push(`${path}.smartGuides: missing required field`); else { if (typeof fieldValue !== "boolean") errors.push(`${path}.smartGuides` + ": expected boolean"); } } + { + const fieldValue = value["hasSavedChat"]; + if (fieldValue === undefined || fieldValue === null) errors.push(`${path}.hasSavedChat: missing required field`); + else { if (typeof fieldValue !== "boolean") errors.push(`${path}.hasSavedChat` + ": expected boolean"); } + } { const fieldValue = value["dragFactor"]; if (fieldValue === undefined || fieldValue === null) errors.push(`${path}.dragFactor: missing required field`);