Repository navigation
Conversation
65d4478 to
95811d0
Compare
kmcginnes
left a comment
There was a problem hiding this comment.
Thanks Mario. This needs a rebase and a narrower scope before a detailed review.
Scope: keep this PR to the layout algorithm
I want this PR to cover only #890: save the chosen layout algorithm in the graph file and the previous session, and restore it on load. Please move the exact node positions, viewport pan/zoom, and the restoration transaction (the second commit, the arrangement ADR, and the GLOSSARY.md entry) out of this PR.
Node positions changing between sessions is fine for now. Storing them is a separate and much bigger task, and it should get its own issue and design discussion before any code.
Connector changes: split these out too
Two changes in connector/ aren't about the layout:
fetchEntityDetails.tsbuilds endpoint-only vertices from restored edges. That changes the "not found" counts for every query language.mapSparqlValueToScalar.tsnow returns invalid dates as strings, and an existing test was flipped to match.
I'm not convinced either is the right change, but they're worth discussing. Please open them separately. I expect the endpoint-vertex change is really a prerequisite of the arrangement work, so it can go with that discussion.
Rebase notes
Main has moved quite a bit. In particular:
- #2144 needs its own rebase first (see my comment there); this PR builds on its
core/graphLayout.ts. - #2245 removed Iterator helpers (
.values().map(...),.toArray()) and vitest globals. Use plain loops orArray.from(...), and explicitvitestimports. - Connection types such as
ConfigurationIdandConnectionWithIdnow come from@/connections. CONTEXT.mdis nowGLOSSARY.md.- #1894 added per-tab View Layouts (
createSessionScopedAtom), which affects where the layout choice lives (next section).
Where the layout choice lives
GLOSSARY.md and the per-tab storage ADR say the chosen Layout belongs in the view's View Layout. Right now liveGraphLayoutAtom is an in-memory global and the Session owns the value. I'd do this instead:
- Add
layoutAlgorithmto the Graph View Layout codec, the same way #2144 does for the Schema View. - Back
graphViewLayoutAlgorithmAtomwith it. - Treat the Session and the graph file as holding a snapshot of the choice, and write that snapshot back to the View Layout on restore.
This also fixes a bug in the current version: the live atom is never reset, so after switching connections the next session update writes connection A's layout into connection B's Session.
Validation
- Graph file import uses
z.enum(layoutNames), so a file with a layout this release doesn't know, e.g. one exported from a newer release, fails to import entirely. Fall back to the default instead, the same way stored sessions do, and use one shared schema for both. - That makes
resolveGraphSessionLayoutredundant. Both of its call sites (ImportGraphButton.tsxanduseRestoreGraphSession.ts) receive values that are already validated, so its fallback never runs. Remove it and let the zod schema do the work. transformGraphSessions<ConnectionId>: the generic only ever has one type argument (ConfigurationId), so drop it.
For the arrangement follow-up, not this PR
So these don't get lost when that work picks back up:
- The arrangement schema is defined twice, in
storage.tsand inexportedGraph.ts. mergeGraphArrangementsmakes two extra passes. AMapalready keeps insertion order, soArray.from(byId.values())is enough.- Previous-session restore and graph import repeat the same start, fetch, check, populate, commit, and clear sequence. That should be one shared flow.
useInitCytoscapetracks the connection withconnectionIdRef,layoutTargetRef,dragTargetRef, and aconnectionIdprop onGraph.GraphVieweralready remounts the canvas for each connection withkey={config?.id}, so those guards protect against a case that can't happen. Either drop them or drop the key. Also use lodash'sDebouncedFuncinstead ofReturnType<typeof debounce>.- The "is the session empty? then RESET, otherwise set" check appears three times. It could live in
activeGraphSessionAtom's setter.
9d0fa89 to
f2db768
Compare
|
Thanks Kris. Rebased onto current Scope — layout only. Exact node positions, viewport pan/zoom, the restoration transaction, the arrangement ADR, and its glossary entry are out of this PR. They now live in #2374 for the design discussion you asked for. Node positions changing between sessions is the accepted behavior here for now. Connector changes — split out. Both are gone from this PR. Where the layout choice lives. Done as suggested: Validation. Import no longer uses Rebase notes. All applied: built on the merged #2144 The arrangement follow-up refactors you parked (duplicate schema, |
0111522 to
780c3bc
Compare
…iles Save the chosen Graph View layout algorithm so it is restored when a graph is loaded from the previous session or an exported graph file. The layout choice lives in the Graph View Layout codec (mirroring the Schema View) and is backed by a View Layout atom rather than an in-memory global, so it persists per browser tab across reloads. The per-tab View Layout is the single source of truth: session restore brings back the entities only and never changes the layout, while importing a graph file applies that file's layout since a file is a self-contained artifact. One shared schema validates the layout name on both the file and the stored session, falling back to the default for a missing (quiet) or unknown (warn) value instead of failing the load. Storage keys and persisted field names are unchanged; legacy session and graph data without a layout remain usable.
780c3bc to
8f08974
Compare
Summary
Loading a graph from a previous Session or from a saved graph file always came back on the default Layout, so users had to pick their Layout again every time.
User-facing: The Graph View remembers the chosen Layout per browser tab. Restoring the previous Session brings back the Layout it was last shown in. Save Graph records the Layout in the file, and Load Graph applies it. A Session or file saved by an older version keeps the current Layout.
flowchart LR Picker[Layout picker] --> GVL["Graph View Layout<br/>(per tab)"] Picker -->|graph on screen is the active Session| Session[(Session)] GVL --> Save[Save Graph] --> File[Exported Graph File] Session -->|Restore, after entities load| GVL File -->|Load Graph, after entities load| GVLHow to read:
core/graphLayout.ts:storedLayoutNameSchemafor the per-tab Graph View Layout (missing or unknown falls back toF_COSE), andsavedGraphLayoutNameSchemafor a Session or file (missing or unknown parses as no Layout, so the current one stays; unknown also warns).core/StateProvider/graphSession/graphViewLayoutAlgorithm.ts: the writable atom over the Graph View Layout that replaces the in-memory global. Choosing a Layout while the graph on screen is the active Session also updates that Session. It leaves a Session waiting to be restored alone.core/StateProvider/graphSession/{storage,useUpdateGraphSession,useRestoreGraphSession}.ts: the Session's optionallayout, written on update and applied on restore once the entities load.modules/GraphViewer/{exportedGraph,ExportGraphButton,ImportGraphButton}.ts(x): the optionallayoutfield in the Exported Graph File, written on save and applied on load once the entities load.createExportedGraphnow takes an options object.docs/features/graph-view.mdandGLOSSARY.md: updated Layout docs, and a new Exported Graph File term.Evidence
Before: A restored Session or loaded graph file always showed
F_COSE, and a reload reset the Layout.Session not saved
Export not saved
After:
Session saved
Export saved
Verified live: against a TinkerPop Gremlin Server 3.7.3 container.
Merge Danger
Door: two-way
Both fields are additive and optional, and the graph file's Format Generation stays at 1. Older builds ignore the new
layoutkey, and this build reads older Sessions and files without one.Impact: low
Related
Check List
pnpm checkspasses with no errors.pnpm testpasses with no failures.