From 52fb68fc1c5cdd8a4b4f1d48c32ad4af0e6a500a Mon Sep 17 00:00:00 2001 From: Juan Pablo Romero Date: Thu, 20 Aug 2026 23:50:24 -0700 Subject: [PATCH 1/5] refactor(viewer/state): one rankdir reader, one selectRelated, 70 fewer lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A quality-only pass over viewer/state (23 files, 4238 L), reviewed along four independent angles and applied where the fix was contained and behaviour- preserving. Tests: 149 passed, 0 failed — identical to the pre-pass baseline. Reuse: - rankdirNow() re-implemented graphRankDirNow(), and defaulted to Rankdir.TB by hand instead of Rankdir.default. Flagged independently by all three reviewing angles. - DefFontSize/DefWidthIn/DefHeightIn duplicated FontSize/Width/Height.default. DefFontName stays local: FontName.default is the UI spelling, not gv's. - clientToLocal hand-rolled the inverse-CTM step that ClientPoint.toSvgPoint already owns, and returned a bare tuple where the package passes SvgPoint. Simplification: - selectRelated: 4 partially-applied vals + 4 wrappers -> 1 method taking the two booleans it already had. The selector is derivable from them, so a mismatched pair can no longer compile. - beyond(b) was exactly gap(b) >= 0 — two enum matches that had to stay sign-consistent, now one. - ThumbnailRenderer: 8 sites repeating the telemetryContext splat -> one local log(). Kept context-only so no event's payload changes. - RightPanelSection carried four hand-assigned numbers that looked like the persisted encoding; persistence uses ordinal, and they disagreed. Dead code (each confirmed by the compiler, not just by grep): - ViewerState.nodeById, VisibilityOps.hiddenElements.toggle, six unused ProjectOps members, two commented-out blocks. - promptLabelBeforeNewGroup: never written, no UI, no ViewerSettings field — unlike its persisted sibling promptLabelBeforeNewNode. Removing it and its unreachable else branch leaves behaviour identical (the prompt always shows). Efficiency (both on paths that run per mouse-move): - concealedCountsNow() is now lazy: it is O(visible nodes) with 4 set-folds each, and every drag computed it and threw it away. - elementsFromRectEnd is now by-name: a document-wide hit test that forces layout, read only in the click branch. Deliberately NOT in this commit, and why: - selectAll() misses folded boxes (proxyIds), and six selection ops classify raw canvas-spelled ids. Real bugs, not cleanup — separate commits. - The `format == Mermaid` branch in reverseArrowsStyle belongs on DiagramBackend. Contained, but it changes the backend seam on purpose. - Everything touching Persistence/ProjectsStorage/ThumbnailDiskCache: a dev server is live against the real library. - The successor/predecessor merge in VisibilityOps: the two halves have already diverged, so merging them is a behaviour decision. --- .../viewer/state/DiagramSelectionOps.scala | 59 ++++++++----------- .../viewer/state/KeyboardNavOps.scala | 13 ++-- .../viewer/state/ProjectOps.scala | 21 +------ .../viewer/state/RecordCellOps.scala | 46 ++++++--------- .../viewer/state/ThumbnailRenderer.scala | 53 ++++++----------- .../graphexplorer/viewer/state/UIState.scala | 8 +-- .../viewer/state/ViewerState.scala | 15 +---- .../viewer/state/VisibilityOps.scala | 3 - .../state/mouseActions/AddNewArrowOps.scala | 6 +- 9 files changed, 77 insertions(+), 147 deletions(-) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/DiagramSelectionOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/DiagramSelectionOps.scala index 797bc2cf..0bb0173b 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/DiagramSelectionOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/DiagramSelectionOps.scala @@ -25,9 +25,7 @@ trait DiagramSelectionOps: val editingElementV = Var[Option[ElementId]](None) object selection: - val signal = selectionV.signal - .distinct -// .tapEach(sel => println(s"[selection] $sel")) + val signal = selectionV.signal.distinct val selectionChanges: EventStream[(toUnselect: ElementIds, toSelect: ElementIds)] = selection.signal @@ -41,24 +39,20 @@ trait DiagramSelectionOps: .distinct .changes - val _selectSuccessors = - selectRelated(outgoing = true, transitive = true)((graph, nodes) => graph.allSuccessorsGraph(nodes.nodeIds)) - val _selectPredecessors = - selectRelated(outgoing = false, transitive = true)((graph, nodes) => graph.allPredecessorsGraph(nodes.nodeIds)) - val _selectDirectSuccessors = - selectRelated(outgoing = true, transitive = false)((graph, nodes) => graph.directSuccessorsGraph(nodes.nodeIds)) - val _selectDirectPredecessors = - selectRelated(outgoing = false, transitive = false)((graph, nodes) => graph.directPredecessorsGraph(nodes.nodeIds)) - /** `outgoing` and `transitive` describe the same operation the selector * already performs; they exist because the ROW-scoped path has to take the * first hop itself (only that hop is constrained by the port) and therefore * needs to know which way it points and whether to keep going. */ - private def selectRelated(outgoing: Boolean, transitive: Boolean)( - selector: (ViewerGraph, Selection) => ViewerGraph - )(fullGraph: ViewerGraph, hiddenNodes: HiddenElements): Unit = - val visibleSubGraph: ViewerGraph = fullGraph.removeElements(hiddenNodes) + private def selectRelated(outgoing: Boolean, transitive: Boolean): Unit = + def selector(graph: ViewerGraph, nodes: Selection): ViewerGraph = + (outgoing, transitive) match + case (true, true) => graph.allSuccessorsGraph(nodes.nodeIds) + case (true, false) => graph.directSuccessorsGraph(nodes.nodeIds) + case (false, true) => graph.allPredecessorsGraph(nodes.nodeIds) + case (false, false) => graph.directPredecessorsGraph(nodes.nodeIds) + + val visibleSubGraph: ViewerGraph = fullGraphNow().removeElements(hiddenElements.now()) recordCells.selectedCellHop(visibleSubGraph, outgoing) match // A row of a record/table is the subject: only the arrows attached at // its port count, so take that hop by hand. A PORT constrains the first @@ -99,14 +93,14 @@ trait DiagramSelectionOps: def toggle(ss: ElementId*): Unit = selectionV.update(ss.foldLeft(_)(_.toggle(_))) - def set(ss: Selection)(using name: sourcecode.FullName): Unit = + def set(ss: Selection): Unit = selectionV.set(ss) @targetName("setElementIds") - def set1(ss: Set[? <: ElementId])(using name: sourcecode.FullName): Unit = + def set1(ss: Set[? <: ElementId]): Unit = set(ElementIds(ss)) - def set2(ss: ElementId*)(using name: sourcecode.FullName): Unit = + def set2(ss: ElementId*): Unit = set1(ss.toSet) @targetName("addElementIds") @@ -130,7 +124,7 @@ trait DiagramSelectionOps: def keepOnly(p: ElementId => Boolean): Unit = selectionV.update(_.filter(p)) - def clear()(using name: sourcecode.FullName): Unit = + def clear(): Unit = set(ElementIds()) def contains(id: ElementId) = @@ -181,17 +175,10 @@ trait DiagramSelectionOps: // Keep the original groups/clusters in the selection and add all members set(s ++ memberNodeIds) - def selectSuccessors() = - _selectSuccessors(fullGraphNow(), hiddenElements.now()) - - def selectPredecessors() = - _selectPredecessors(fullGraphNow(), hiddenElements.now()) - - def selectDirectSuccessors() = - _selectDirectSuccessors(fullGraphNow(), hiddenElements.now()) - - def selectDirectPredecessors() = - _selectDirectPredecessors(fullGraphNow(), hiddenElements.now()) + def selectSuccessors() = selectRelated(outgoing = true, transitive = true) + def selectPredecessors() = selectRelated(outgoing = false, transitive = true) + def selectDirectSuccessors() = selectRelated(outgoing = true, transitive = false) + def selectDirectPredecessors() = selectRelated(outgoing = false, transitive = false) def addToGroup() = val classified = now().classify @@ -361,10 +348,14 @@ trait DiagramSelectionOps: set(ElementIds.from(nodeId)) if now().contains(nodeId) then navCursorSet(nodeId) + /** `elementsFromRectEnd` is BY-NAME: it is a document-wide hit test that + * forces layout, and only the click branch below reads it — this runs on + * every mouse-move of a rubber-band drag. + */ def selectExtendSelectionOverlappingElements( - rect: MouseActionRect, - selectableElements: Seq[SelectableElement], - elementsFromRectEnd: js.Array[dom.Element] + rect: MouseActionRect, + selectableElements: Seq[SelectableElement], + elementsFromRectEnd: => js.Array[dom.Element] ) = if rect.isEmpty then // Equivalent to an onClick event diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/KeyboardNavOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/KeyboardNavOps.scala index 0b14082d..4cb0618c 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/KeyboardNavOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/KeyboardNavOps.scala @@ -231,13 +231,10 @@ trait KeyboardNavOps: boxes: Map[ElementId, NavBox] ): Option[NodeId] = boxes.get(n).flatMap: o => - // Strictly past this node's edge, so an overlapping neighbour never - // counts as "beside" — and this node can never be its own answer. - def beyond(b: NavBox) = dir match - case NavDirection.NavLeft => b.r <= o.l - case NavDirection.NavRight => b.l >= o.r - case NavDirection.NavUp => b.b <= o.t - case NavDirection.NavDown => b.t >= o.b + // Signed distance from this node's edge to the candidate's facing edge. + // Non-negative means strictly past that edge, so an overlapping + // neighbour never counts as "beside" — and this node is never its own + // answer. One definition, so the two uses cannot drift apart. def gap(b: NavBox) = dir match case NavDirection.NavLeft => o.l - b.r case NavDirection.NavRight => b.l - o.r @@ -246,7 +243,7 @@ trait KeyboardNavOps: visibleGraphNow().nodeIds.iterator .filter(_ != n) .flatMap(id => boxes.get(id).map(id -> _)) - .filter((_, b) => overlaps(o, b, dir) && beyond(b)) + .filter((_, b) => overlaps(o, b, dir) && gap(b) >= 0) .minByOption((_, b) => gap(b)) .map(_._1) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ProjectOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ProjectOps.scala index 1d58f2fe..f8cdeeb2 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ProjectOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ProjectOps.scala @@ -1,6 +1,5 @@ package org.jpablo.graphexplorer.viewer.state -import com.raquo.airstream.core.Signal import com.raquo.airstream.ownership.Owner import com.raquo.airstream.state.Var import com.softwaremill.quicklens.* @@ -10,17 +9,11 @@ import org.jpablo.graphexplorer.viewer.models.GroupId */ case class ProjectOps(project: Var[Project])(using Owner): - // export project.{signal, update, updater} - val signal = project.signal - val update = project.update - val updater = project.updater + val signal = project.signal val name: Var[String] = project.zoomLazy(_.name)((p, n) => p.copy(name = n)).distinct - val page: Var[Page] = - project.zoomLazy(_.page)((p, page) => p.copy(page = page)).distinct - val hiddenElements: Var[HiddenElements] = project .zoomLazy(_.page.hiddenElements)((p, s) => p.modify(_.page.hiddenElements).setTo(s)) @@ -31,16 +24,4 @@ case class ProjectOps(project: Var[Project])(using Owner): .zoomLazy(_.page.collapsedGroups)((p, s) => p.modify(_.page.collapsedGroups).setTo(s)) .distinct -// hiddenElements.signal.foreach: hidden => -// dom.console.debug(s"hidden elements changed: $hidden") - - val basePaths: Signal[List[String]] = - project.signal.map(_.projectSettings.basePaths).distinct - - val projectSettings: Signal[ProjectSettings] = - project.signal.map(_.projectSettings).distinct - - val diagramOptions: Signal[DiagramOptions] = - project.signal.map(_.page.diagramOptions) - end ProjectOps diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/RecordCellOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/RecordCellOps.scala index 69ea5452..a48d5fac 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/RecordCellOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/RecordCellOps.scala @@ -5,11 +5,13 @@ import com.raquo.airstream.state.Var import org.jpablo.graphexplorer.graphviz.html.{HtmlTable, HtmlTableLayout} import org.jpablo.graphexplorer.graphviz.layout.RecordLabel import org.jpablo.graphexplorer.viewer.components.selection.SelectableElement +import org.jpablo.graphexplorer.viewer.components.toSvgPoint +import org.jpablo.graphexplorer.viewer.utils.{ClientPoint, SvgPoint} import org.jpablo.graphexplorer.viewer.components.svgCanvas.RecordCellOverlay import org.jpablo.graphexplorer.viewer.domUtils.querySelectorAllT import org.jpablo.graphexplorer.viewer.formats.dot.{HtmlLabelOps, HtmlLabels, RecordTree} import org.jpablo.graphexplorer.viewer.formats.dot.ast.{AttrEq, AttrValue} -import org.jpablo.graphexplorer.viewer.formats.dot.attributes.Rankdir +import org.jpablo.graphexplorer.viewer.formats.dot.attributes.{FontSize, Height, Width} import org.jpablo.graphexplorer.viewer.graph.ViewerGraph import org.jpablo.graphexplorer.viewer.models.* @@ -62,11 +64,13 @@ trait RecordCellOps: val editingCellV = Var[Option[SelectedCell]](None) object recordCells: - // gv const.h defaults — private in NodeSize, mirrored here (stable). - private val DefFontSize = 14.0 + // gv const.h defaults. The numeric three come from the shared attribute + // objects; DefFontName does NOT — FontName.default is the UI spelling + // ("Times New Roman"), while layout needs the gv font name. + private val DefFontSize = FontSize.default private val DefFontName = "Times-Roman" - private val DefWidthIn = 0.75 - private val DefHeightIn = 0.5 + private val DefWidthIn = Width.default + private val DefHeightIn = Height.default private val PointsPerInch = 72.0 private enum CellKind derives CanEqual: @@ -97,25 +101,21 @@ trait RecordCellOps: def selectedCellIsHtml: Boolean = selectedCellV.now().exists(c => kindOf(c.nodeId).contains(CellKind.Html)) - private def rankdirNow(): Rankdir = - fullGraphNow().elements.graphAttributes.values - .get(Rankdir.attrId) - .flatMap(attr => Rankdir.values.find(_.toString == attr.toString)) - .getOrElse(Rankdir.TB) - - def topLRNow(): Boolean = RecordTree.topLRFor(rankdirNow()) + def topLRNow(): Boolean = RecordTree.topLRFor(graphRankDirNow()) private def getNodeNow(nodeId: NodeId): Option[ViewerNode] = fullGraphNow().getNode(nodeId) /** The record tree of a RECORD node (record ops only). */ def cellTreeOf(nodeId: NodeId): Option[RecordTree.Group] = - Option.when(kindOf(nodeId).contains(CellKind.Record))(()).flatMap: _ => + if kindOf(nodeId).contains(CellKind.Record) then getNodeNow(nodeId).map(node => RecordTree.parse(node.label.toString)) + else None private def htmlTableOf(nodeId: NodeId): Option[HtmlTable] = - Option.when(kindOf(nodeId).contains(CellKind.Html))(()).flatMap: _ => + if kindOf(nodeId).contains(CellKind.Html) then getNodeNow(nodeId).flatMap(node => HtmlLabelOps.parseTable(node.label.toString)) + else None /** Node-local cell boxes, from the SAME layout the engine used. */ def cellBoxes(nodeId: NodeId): Vector[RecordCellBox] = @@ -181,11 +181,11 @@ trait RecordCellOps: if !isCellEditable(nodeId) then None else for - group <- nodeGroupInDom(nodeId) - (lx, ly) <- clientToLocal(group, clientX, clientY) + group <- nodeGroupInDom(nodeId) + local <- clientToLocal(group, clientX, clientY) path <- { val bbox = RecordCellOverlay.ownGeometryBBox(group) - cellNearestLocalPoint(nodeId, lx - (bbox.x + bbox.width / 2), (bbox.y + bbox.height / 2) - ly) + cellNearestLocalPoint(nodeId, local.x - (bbox.x + bbox.width / 2), (bbox.y + bbox.height / 2) - local.y) } yield path @@ -196,16 +196,8 @@ trait RecordCellOps: .filterNot(_.closest(s".${SelectableElement.exitGhostClass}") != null) .collectFirst { case g: dom.svg.G => g } - private def clientToLocal(group: dom.svg.G, clientX: Double, clientY: Double): Option[(Double, Double)] = - for - svgEl <- Option(group.ownerSVGElement) - ctm <- Option(group.getScreenCTM()) - yield - val pt = svgEl.createSVGPoint() - pt.x = clientX - pt.y = clientY - val local = pt.matrixTransform(ctm.inverse()) - (local.x, local.y) + private def clientToLocal(group: dom.svg.G, clientX: Double, clientY: Double): Option[SvgPoint] = + Option(group.getScreenCTM()).map(ctm => ClientPoint(clientX, clientY).toSvgPoint(ctm)) /** The port of the cell at `path`, MINTED into the label when the cell has * none (a fresh `f`). Pure on the given graph, so arrow ops can compose diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ThumbnailRenderer.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ThumbnailRenderer.scala index 58b1e48f..20c5fe46 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ThumbnailRenderer.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ThumbnailRenderer.scala @@ -72,25 +72,21 @@ object ThumbnailRenderer: telemetryContext: Seq[(String, Any)] = Nil )(using ExecutionContext): Signal[ReactiveSvgElement[SVG]] = val format = DiagramFormat.detect(dot.value) + + // Every event from here carries the caller's context; going through one + // place is what keeps a site from silently dropping the project id. + def log(name: String, extra: (String, Any)*): Unit = + Telemetry.log(name, (telemetryContext ++ extra)*) + + def cacheFields = Seq("format" -> format.toString, "cacheSize" -> ThumbnailSvgCache.size) + ThumbnailSvgCache.get(format, dot.value) match case Some(proto) => - Telemetry.log( - "thumb.cache.hit", - (telemetryContext ++ Seq( - "format" -> format.toString, - "cacheSize" -> ThumbnailSvgCache.size - ))* - ) + log("thumb.cache.hit", cacheFields*) Signal.fromValue(ThumbnailSvgCache.cloneSvg(proto)) case None => - Telemetry.log( - "thumb.cache.miss", - (telemetryContext ++ Seq( - "format" -> format.toString, - "cacheSize" -> ThumbnailSvgCache.size - ))* - ) + log("thumb.cache.miss", cacheFields*) // Adopt an SVG string (from the persistent cache, or fresh off a // render) as this card's element, refilling the in-memory cache on the @@ -98,10 +94,7 @@ object ThumbnailRenderer: def adopt(svgHtml: String): ReactiveSvgElement[SVG] = val proto = parseSVG(svgHtml).ref ThumbnailSvgCache.put(format, dot.value, proto) - Telemetry.log( - "thumb.cache.store", - (telemetryContext ++ Seq("format" -> format.toString, "cacheSize" -> ThumbnailSvgCache.size))* - ) + log("thumb.cache.store", cacheFields*) ThumbnailSvgCache.cloneSvg(proto) // ONE svg-only render (`textToSvgOnly`) straight from the source text. @@ -116,14 +109,8 @@ object ThumbnailRenderer: onIdle { () => val svgStartedAt = Telemetry.nowMs() val resultTry = graphviz.textToSvgOnly(dot) - Telemetry.log( - "thumb.dot.textToSvg", - (telemetryContext ++ Seq("dtMs" -> (Telemetry.nowMs() - svgStartedAt), "ok" -> resultTry.isSuccess))* - ) - Telemetry.log( - "thumb.dot.total", - (telemetryContext ++ Seq("dtMs" -> (Telemetry.nowMs() - startedAt), "ok" -> resultTry.isSuccess))* - ) + log("thumb.dot.textToSvg", "dtMs" -> (Telemetry.nowMs() - svgStartedAt), "ok" -> resultTry.isSuccess) + log("thumb.dot.total", "dtMs" -> (Telemetry.nowMs() - startedAt), "ok" -> resultTry.isSuccess) resultTry.get.outerHTML } @@ -135,15 +122,12 @@ object ThumbnailRenderer: // containment reduces it. Not rendering at all is the only real cure, // which is what the persistent cache buys on every visit after the first. def renderMermaid(startedAt: Double): Future[String] = - Telemetry.log("thumb.mermaid.start", (telemetryContext ++ Seq("sourceChars" -> dot.value.length))*) + log("thumb.mermaid.start", "sourceChars" -> dot.value.length) val backend = MermaidBackend() onIdle(() => backend.textToSvg(dot.value)) .flatMap(identity) .map: r => - Telemetry.log( - "thumb.mermaid.done", - (telemetryContext ++ Seq("dtMs" -> (Telemetry.nowMs() - startedAt), "ok" -> true))* - ) + log("thumb.mermaid.done", "dtMs" -> (Telemetry.nowMs() - startedAt), "ok" -> true) r.svg.ref.outerHTML val startedAt = Telemetry.nowMs() @@ -156,13 +140,10 @@ object ThumbnailRenderer: val html: Future[String] = ThumbnailDiskCache.get(format, dot.value).flatMap: case Some(stored) => - Telemetry.log( - "thumb.disk.hit", - (telemetryContext ++ Seq("format" -> format.toString, "bytes" -> stored.length))* - ) + log("thumb.disk.hit", "format" -> format.toString, "bytes" -> stored.length) Future.successful(stored) case None => - Telemetry.log("thumb.disk.miss", (telemetryContext ++ Seq("format" -> format.toString))*) + log("thumb.disk.miss", "format" -> format.toString) val rendered = format match case DiagramFormat.DOT => renderDot(startedAt) case DiagramFormat.Mermaid => renderMermaid(startedAt) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/UIState.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/UIState.scala index c66f0fca..8f460179 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/UIState.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/UIState.scala @@ -4,11 +4,9 @@ import com.raquo.airstream.core.Signal import com.raquo.airstream.eventbus.EventBus import com.raquo.airstream.state.Var -enum RightPanelSection(idx: Int) derives CanEqual: - case none extends RightPanelSection(-1) - case diagramAttributes extends RightPanelSection(0) - case elements extends RightPanelSection(1) - case sources extends RightPanelSection(2) +/** Persisted by `ordinal` (see Persistence), NOT by any hand-assigned number. */ +enum RightPanelSection derives CanEqual: + case none, diagramAttributes, elements, sources def isVisible = this != none diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ViewerState.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ViewerState.scala index 3337bd64..e16ed1e6 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ViewerState.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/ViewerState.scala @@ -285,9 +285,6 @@ case class ViewerState( // If true, prompt for label before creating a new node (default: true) val promptLabelBeforeNewNode: Var[Boolean] = Var(true) - // If true, prompt for label before creating a new group (default: true) - val promptLabelBeforeNewGroup: Var[Boolean] = Var(true) - // ------------- New node flow ------------- case class PendingNewNode(attributes: Attributes, direction: ArrowDirection) val pendingNewNodeV: Var[Option[PendingNewNode]] = Var(None) @@ -306,12 +303,9 @@ case class ViewerState( else addNodeWithSmartConnection(attributes, direction) - /** Creates a new group from the current selection, optionally prompting for the label before creation based on settings. */ + /** Creates a new group from the current selection, prompting for its label. */ def createGroupMaybePrompt(elementIds: ElementIds): Unit = - if promptLabelBeforeNewGroup.now() then - pendingNewGroupV.set(Some(PendingNewGroup(elementIds))) - else - createGroupWithLabel(elementIds, "") + pendingNewGroupV.set(Some(PendingNewGroup(elementIds))) /** Creates a new group with the specified elements and label. */ def createGroupWithLabel(elementIds: ElementIds, label: String): Unit = @@ -326,9 +320,6 @@ case class ViewerState( // -------- storage ------------ initializePersistence() - def nodeById(ids: Seq[NodeId]): Seq[ViewerNode] = - ids.flatMap(fullGraphNow().getNode) - def allNodeIds(): Set[NodeId] = fullGraphNow().nodeIds @@ -369,7 +360,7 @@ case class ViewerState( to: NodeId, fromCell: Option[List[Int]] = None, toCell: Option[List[Int]] = None - )(using name: sourcecode.FullName) = + ) = phases.fullGraphV.update: g => val (g1, fromPort) = recordCells.resolvePortIn(g, from, fromCell) val (g2, toPort) = recordCells.resolvePortIn(g1, to, toCell) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/VisibilityOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/VisibilityOps.scala index aa856bdb..5c4d8f95 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/VisibilityOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/VisibilityOps.scala @@ -20,9 +20,6 @@ trait VisibilityOps: val signal = _hiddenElements.signal - def toggle(s: NodeId): Unit = - _hiddenElements.update(_.toggle(s)) - def add(ss: Set[NodeId]): Unit = _hiddenElements.update(_ ++ ss) diff --git a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/mouseActions/AddNewArrowOps.scala b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/mouseActions/AddNewArrowOps.scala index 5e0111ce..fd33637d 100644 --- a/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/mouseActions/AddNewArrowOps.scala +++ b/viewer/src/main/scala/org/jpablo/graphexplorer/viewer/state/mouseActions/AddNewArrowOps.scala @@ -68,8 +68,10 @@ trait AddNewArrowOps: def handleNewArrowControls(parent: dom.svg.G, selection: Option[SelectableElement], action: MouseAction): Unit = // Read the badge model ONCE, and read the SAME one CountBadges drew from: // a control decides where to stand by which edges carry a count, so the two - // must never disagree about that. - val concealed = concealedCountsNow() + // must never disagree about that. LAZY because this runs on every + // mouse-move of every drag, where no control is built and the whole + // O(visible nodes) count would be computed and thrown away. + lazy val concealed = concealedCountsNow() val controls = for elem <- selection.toSeq From 877128f5921dbfde56a72c450554f4daa959d96a Mon Sep 17 00:00:00 2001 From: Juan Pablo Romero Date: Fri, 21 Aug 2026 00:00:18 -0700 Subject: [PATCH 2/5] refactor(gx-cli): one save ladder, one --list, one dead field fewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second quality-only slice, same four-angle review. Tests: 61 passed, 0 failed — identical to the pre-pass baseline. Net line count barely moves because three documented helpers replace duplication that was cheaper per copy than to state once; the count that dropped is the number of places a rule lives. Reuse / simplification: - persist(): the store.save result ladder was spelled out six times, each deciding independently that a failed write means ExitCode.Unknown. Now one helper. (The three saves inside syncOne still drop their error — that is a BUG, not duplication, and is deliberately left for its own commit.) - listNames(): `run --list` and `session --list` printed a Vector[String] with four identical lines each. - boundTargets(): "--all, or no arguments, means every bound diagram" is documented in Usage as shared by sync and watch, and was stated twice. Dead / no-op: - Target.OnDisk carried an OriginUri that all four match sites discarded as `case OnDisk(path, _)`. Building it cost a toRealPath walk on a path that checkPolicy had already canonicalised, on every path-target invocation of get, set and run. - `.left.map(identity)` in pathToShow, and `Right(()).flatMap(_ => ...)` in the Args parser, both pure identity wrappers. - import called store.initialize() before saving; no other save site does, and AtomicFiles.write already creates the parent directory. A caller compensating for a precondition the callee guarantees. Output contract: - emitWatchEvent wrapped a TOTAL ContentHash in Some and unwrapped it with getOrElse(""), so the emitted JSON advertised `"hash": ""` as a reachable state. Every WatchEvent case carries a hash; typed as such, same output. Deliberately NOT in this commit: - The LF/CRLF hash disagreement in syncOne (phantom "Ahead" forever on any CRLF-authored origin), watch skipping checkPolicy, syncOne swallowing save errors, `gx sync typo` exiting 0, the --json indent/"[]" drift, and the open-vs-session NO_SESSION exit code. All real bugs; each wants its own fix. - Extracting the shared import/bind path prelude. It is the biggest remaining simplification, but it reorders "cannot read this file" against "this scheme cannot support this mode", and I could not establish from the code that the combination is unreachable. Ordering changes are not cleanup. --- .../org/jpablo/graphexplorer/gx/Args.scala | 2 +- .../org/jpablo/graphexplorer/gx/Cli.scala | 113 +++++++++--------- 2 files changed, 59 insertions(+), 56 deletions(-) diff --git a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala index 945a9fd8..e5b27d8c 100644 --- a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala +++ b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Args.scala @@ -41,7 +41,7 @@ object Args: if name.isEmpty then Left("empty flag: '--'") else if name.contains('=') then val (k, v) = name.span(_ != '=') - Right(()).flatMap(_ => loop(tail, positional, values + (k -> v.drop(1)), switches)) + loop(tail, positional, values + (k -> v.drop(1)), switches) else if ValueFlags.contains(name) then tail match case v :: more if !v.startsWith("--") => diff --git a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala index badb0dfb..df2b49f7 100644 --- a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala +++ b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala @@ -29,7 +29,7 @@ private enum Target derives CanEqual: * writing a file needs no registration first. v1 could not do this — `get` * failed with "path is not currently watched" until you had called `watch`. */ - case OnDisk(path: Path, origin: OriginUri) + case OnDisk(path: Path) /** The `gx` command surface (§8). * @@ -215,13 +215,10 @@ object Cli: createdAt = env.now(), updatedAt = env.now() ) - env.store.initialize() - env.store.save(d) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(saved) => - env.audit.record(AuditEvent.Allowed(resolved.toString, "import")) - printDiagram(saved, args, env) - ExitCode.Ok + persist(d, env): saved => + env.audit.record(AuditEvent.Allowed(resolved.toString, "import")) + printDiagram(saved, args, env) + ExitCode.Ok // ----------------------------------------------------------------- ls @@ -247,7 +244,7 @@ object Cli: case Target.InLibrary(d) => if args.json then env.out(summaryJson(d).render(indent = 2)) else env.out(d.text) ExitCode.Ok - case Target.OnDisk(path, _) => + case Target.OnDisk(path) => Documents.read(path) match case Left(err) => env.err(s"gx: ${describe(err)}") @@ -279,7 +276,7 @@ object Cli: */ private def applyText(target: Target, text: String, args: Args, env: CliEnv, source: String): Int = target match - case Target.OnDisk(path, _) => writeFile(path, text, args, env, source) + case Target.OnDisk(path) => writeFile(path, text, args, env, source) case Target.InLibrary(d) => val updated = d.copy(text = text, updatedAt = env.now()) d.binding.filter(b => b.mode.pushes) match @@ -288,9 +285,8 @@ object Cli: // never writes them back (§5.3). Saving the record is the whole // operation, and the divergence it creates is reported by // `gx sync` rather than resolved behind the user's back. - env.store.save(updated) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(_) => reportSet(updated, wroteOrigin = false, args, env); ExitCode.Ok + persist(updated, env): _ => + reportSet(updated, wroteOrigin = false, args, env); ExitCode.Ok case Some(binding) => binding.origin.filePath.map(Paths.get(_)) match case None => @@ -308,12 +304,10 @@ object Cli: text = doc.text, binding = Some(binding.copy(baseHash = doc.hash, lastSyncAt = env.now())) ) - env.store.save(synced) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(_) => - env.audit.record(AuditEvent.Written(path.toString, doc.hash, source)) - reportSet(synced, wroteOrigin = true, args, env) - ExitCode.Ok + persist(synced, env): _ => + env.audit.record(AuditEvent.Written(path.toString, doc.hash, source)) + reportSet(synced, wroteOrigin = true, args, env) + ExitCode.Ok private def writeFile(path: Path, text: String, args: Args, env: CliEnv, source: String): Int = checkPolicy(path, env) match @@ -373,9 +367,8 @@ object Cli: binding = Some(Binding(origin, mode, base, env.now())), updatedAt = env.now() ) - env.store.save(bound) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(_) => printDiagram(bound, args, env); ExitCode.Ok + persist(bound, env): _ => + printDiagram(bound, args, env); ExitCode.Ok case _ => env.err("gx: bind needs a diagram and a path") ExitCode.Usage @@ -387,16 +380,13 @@ object Cli: ExitCode.InvalidPathOrPolicy case Some(d) => val detached = d.copy(binding = None, updatedAt = env.now()) - env.store.save(detached) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(_) => printDiagram(detached, args, env); ExitCode.Ok + persist(detached, env): _ => + printDiagram(detached, args, env); ExitCode.Ok // --------------------------------------------------------------- sync private def sync(args: Args, env: CliEnv): Int = - val targets = - if args.has("all") || args.positional.isEmpty then env.store.list().filter(_.isBound) - else args.positional.flatMap(findInLibrary(_, env)).filter(_.isBound) + val targets = boundTargets(args, env) if targets.isEmpty then env.out(if args.json then "[]" else "(nothing bound to sync)") @@ -479,9 +469,7 @@ object Cli: * discarded"). */ private def watch(args: Args, env: CliEnv): Int = - val diagrams = - if args.has("all") || args.positional.isEmpty then env.store.list().filter(_.isBound) - else args.positional.flatMap(findInLibrary(_, env)).filter(_.isBound) + val diagrams = boundTargets(args, env) val fromPaths = args.positional.filter(r => findInLibrary(r, env).isEmpty).map: raw => FileOrigins.originOf(env.cwd.resolve(raw), env.cwd) @@ -507,15 +495,11 @@ object Cli: private def emitWatchEvent(event: WatchEvent, args: Args, env: CliEnv): Unit = val (kind, uri, hash) = event match - case WatchEvent.Changed(u, h) => ("changed", u, Some(h)) - case WatchEvent.Restored(u, h) => ("restored", u, Some(h)) - case WatchEvent.Deleted(u, h) => ("deleted", u, Some(h)) + case WatchEvent.Changed(u, h) => ("changed", u, h) + case WatchEvent.Restored(u, h) => ("restored", u, h) + case WatchEvent.Deleted(u, h) => ("deleted", u, h) if args.json then - env.out( - ujson - .Obj("event" -> kind, "origin" -> uri.value, "hash" -> hash.map(_.hex).getOrElse("")) - .render() - ) + env.out(ujson.Obj("event" -> kind, "origin" -> uri.value, "hash" -> hash.hex).render()) else env.out(s"$kind\t${uri.value}") // ---------------------------------------------------------------- run @@ -531,9 +515,7 @@ object Cli: if args.has("list") then // Discoverability is part of the vocabulary being a vocabulary: a name // nobody can enumerate is not addressable in any useful sense. - if args.json then env.out(ujson.Arr.from(AnyCommand.names.map(ujson.Str(_))).render(indent = 2)) - else AnyCommand.names.foreach(env.out) - ExitCode.Ok + listNames(AnyCommand.names, args, env) else (args.positionalAt(0), args.positionalAt(1)) match case (Some(_), Some(commandName)) => @@ -569,7 +551,7 @@ object Cli: */ private def executeRecord(target: Target, command: RecordCommand, args: Args, env: CliEnv): Int = target match - case Target.OnDisk(path, _) => + case Target.OnDisk(path) => env.err(s"gx: '${path.getFileName}' is not in the library, so it has no record to change") env.err(s"gx: import it first: gx import ${path.getFileName}") ExitCode.InvalidPathOrPolicy @@ -588,12 +570,10 @@ object Cli: // Metadata only: the record is saved, and the ORIGIN is untouched // whatever the sync mode says. That is §5.3.1's split doing its job // — hiding a node must never make a regenerating origin conflict. - env.store.save(updated.copy(updatedAt = env.now())) match - case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown - case Right(saved) => - if args.json then env.out(summaryJson(saved).render(indent = 2)) - else env.out(s"${saved.id.value} ${command.name}") - ExitCode.Ok + persist(updated.copy(updatedAt = env.now()), env): saved => + if args.json then env.out(summaryJson(saved).render(indent = 2)) + else env.out(s"${saved.id.value} ${command.name}") + ExitCode.Ok private def executeDocument(target: Target, command: DocumentCommand, args: Args, env: CliEnv): Int = textOf(target, env) match @@ -651,7 +631,7 @@ object Cli: private def textOf(target: Target, env: CliEnv): Either[Int, String] = target match case Target.InLibrary(d) => Right(d.text) - case Target.OnDisk(path, _) => + case Target.OnDisk(path) => Documents.read(path) match case Right(doc) => Right(doc.text) case Left(err) => @@ -687,9 +667,7 @@ object Cli: */ private def sessionCommand(args: Args, env: CliEnv): Int = if args.has("list") then - if args.json then env.out(ujson.Arr.from(SessionCommand.names.map(ujson.Str(_))).render(indent = 2)) - else SessionCommand.names.foreach(env.out) - ExitCode.Ok + listNames(SessionCommand.names, args, env) else args.positionalAt(0) match case None => @@ -810,7 +788,7 @@ object Cli: env.err(s"gx: bind it first: gx bind ${d.id.value} ") Left(ExitCode.InvalidPathOrPolicy) case None => - checkPolicy(env.cwd.resolve(ref), env).left.map(identity) + checkPolicy(env.cwd.resolve(ref), env) // ---------------------------------------------------------- resolution @@ -826,7 +804,7 @@ object Cli: val path = env.cwd.resolve(ref) checkPolicy(path, env) match case Left(code) => code - case Right(resolved) => f(Target.OnDisk(resolved, FileOrigins.originOf(resolved, env.cwd))) + case Right(resolved) => f(Target.OnDisk(resolved)) /** id, then exact name, then the origin path. Ambiguity is reported rather * than resolved by picking one, because "gx set" on the wrong diagram is not @@ -848,6 +826,31 @@ object Cli: // ------------------------------------------------------------- helpers + /** Save, or report the failure the same way everywhere. A store write that + * fails is `Unknown`, not a success with a warning — the six call sites that + * spelled this out by hand could each have answered differently. + */ + private def persist(d: Diagram, env: CliEnv)(onSaved: Diagram => Int): Int = + env.store.save(d) match + case Left(e) => env.err(s"gx: $e"); ExitCode.Unknown + case Right(saved) => onSaved(saved) + + /** The vocabulary of a command tier, for `--list`. Both tiers print it the + * same way; two copies is how the two `--list` flags start differing. + */ + private def listNames(names: Vector[String], args: Args, env: CliEnv): Int = + if args.json then env.out(ujson.Arr.from(names.map(ujson.Str(_))).render(indent = 2)) + else names.foreach(env.out) + ExitCode.Ok + + /** `--all`, or no arguments at all, means every bound diagram. `sync` and + * `watch` are documented as sharing that rule, so they read it from here + * rather than each restating it. + */ + private def boundTargets(args: Args, env: CliEnv): Vector[Diagram] = + if args.has("all") || args.positional.isEmpty then env.store.list().filter(_.isBound) + else args.positional.flatMap(findInLibrary(_, env)).filter(_.isBound) + private def checkPolicy(path: Path, env: CliEnv): Either[Int, Path] = env.policy.evaluate(path, env.cwd) match case Right(resolved) => Right(resolved) From 790605f3cadccaa1de28937b2cef8012d4afb890 Mon Sep 17 00:00:00 2001 From: Juan Pablo Romero Date: Fri, 21 Aug 2026 00:06:32 -0700 Subject: [PATCH 3/5] =?UTF-8?q?fix(gx):=20a=20CRLF=20origin=20is=20no=20lo?= =?UTF-8?q?nger=20Ahead=20forever=20=E2=80=94=20and=20no=20longer=20silent?= =?UTF-8?q?ly=20rewritten?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `base` and `remote` are hashes of file BYTES (Hashing.ofBytes, via Documents). `local` was hashed from the record's text with a hardcoded LineEnding.Lf. For any CRLF-authored origin the three were never in the same space, so: localMoved = (local != base) = ALWAYS TRUE `gx sync` therefore reported Ahead on a file nobody had touched — and in --mode sync that is not just a wrong label, it triggers a PUSH: the user's untouched CRLF file was rewritten as LF, changing every line's bytes. The new first test asserts the file is byte-identical after a no-op sync, and that assertion is what fails on the old code. The same mismatch made `local == remote` unreachable, so a generator rewriting a CRLF file to byte-identical content landed on Diverged instead of Converged — precisely the "conflict machine" SyncState.Converged's scaladoc exists to prevent. Fix: read the origin ONCE and hash the record's text with that file's own convention (V-04), so all three hashes describe bytes: val origin = path.flatMap(Documents.read(_).toOption) val remote = origin.map(_.hash) val local = Hashing.ofText(d.text, origin.map(_.lineEnding).getOrElse(Lf)) Hashing.ofText already demanded the convention explicitly and its scaladoc names this failure ("a phantom conflict, the least debuggable failure this design has", V-16); the CLI was the caller it was warning about. Documents.read has always returned the lineEnding — the CLI discarded it. Reading once also removes a second full read+SHA of the same file in the Pull branch, which had re-read what the hash check had just read. `bind` needs no change: it takes base from Documents.hashOf on an existing file (already byte-space), and falls back to an LF hash only when the file does not exist yet — which is what Documents.create writes. Tests: three new cases covering InSync / Converged / Behind on CRLF origins. They fail 3/3 on the previous code. Every pre-existing sync test used text with NO newline in it, where LF and CRLF are identical bytes — which is exactly how this shipped. 64 passed, 0 failed. --- .../org/jpablo/graphexplorer/gx/Cli.scala | 27 +++++++--- .../org/jpablo/graphexplorer/gx/CliSpec.scala | 54 +++++++++++++++++++ 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala index df2b49f7..5f31021d 100644 --- a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala +++ b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala @@ -412,15 +412,30 @@ object Cli: case None => (d, SyncState.InSync) case Some(binding) => val path = binding.origin.filePath.map(Paths.get(_)) - val remote = path.flatMap(Documents.hashOf) - val local = Hashing.ofText(d.text, LineEnding.Lf) - val state = SyncState.of(binding.baseHash, local, remote) + // ONE read of the origin: it answers both questions below, and the Pull + // branch reuses it rather than reading and re-hashing the same bytes. + val origin = path.flatMap(Documents.read(_).toOption) + val remote = origin.map(_.hash) + + // `base` and `remote` are hashes of FILE BYTES, so `local` has to be + // measured the same way: the record's text as it would be written into + // THIS file, using the convention that file already uses (V-04). + // + // Hashing with a fixed LF made every CRLF-authored origin read `Ahead` + // forever — nothing had been edited, the bytes simply could not agree — + // and made a byte-identical regeneration land on `Diverged` instead of + // `Converged`, which is the conflict machine SyncState.Converged exists + // to prevent. Hashing.ofText demands the convention explicitly for this + // exact reason; see its scaladoc and V-16. + // + // With no origin on disk the state is OriginMissing whatever `local` + // says, and Lf is what Documents.create would write if it reappears. + val local = Hashing.ofText(d.text, origin.map(_.lineEnding).getOrElse(LineEnding.Lf)) + val state = SyncState.of(binding.baseHash, local, remote) binding.mode.autoAction(state) match case Some(SyncAction.Pull) => - (for - p <- path - doc <- Documents.read(p).toOption + (for doc <- origin yield val updated = d.copy( text = doc.text, diff --git a/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala b/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala index 5b57347d..8c675198 100644 --- a/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala +++ b/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala @@ -254,6 +254,60 @@ class CliSpec extends FunSuite: assert(r.stdout.contains("nothing bound"), r.stdout) } + // ------------------------------------------------- sync × line endings + // + // `base` and `remote` are hashes of file BYTES, so `local` has to be measured + // the same way. Hashing the record's text with a fixed LF made every one of + // these read as a local edit that never happened. + // + // Note every OTHER sync test above uses text with no newline in it, where LF + // and CRLF are the same bytes — which is exactly how this survived. + + private def crlf: String = "digraph G {\r\n a -> b\r\n}\r\n" + + tmp.test("a CRLF origin nobody has touched is InSync, not Ahead") { dir => + dot(dir, "win.dot", crlf) + val i = Run(dir) + assertEquals(i("import", "win.dot", "--mode", "sync"), ExitCode.Ok, i.stderr) + + // Nothing at all happens here. Both sides are exactly as imported. + val s = Run(dir) + assertEquals(s("sync", "--all"), ExitCode.Ok, s.stderr) + assert(s.stdout.contains("InSync"), s.stdout) + assertEquals(Files.readString(dir.resolve("win.dot")), crlf, "sync rewrote an untouched origin") + } + + tmp.test("a byte-identical CRLF regeneration is Converged, not Diverged") { dir => + val f = dot(dir, "win.dot", crlf) + val i = Run(dir) + i("import", "win.dot", "--mode", "sync") + val id = i.store.list().head.id + + // The generator rewrites the same bytes; the store independently agrees. + val d = i.store.get(id).fold(x => fail(s"$x"), identity) + val next = "digraph G {\r\n a -> c\r\n}\r\n" + i.store.save(d.copy(text = next)) + Files.writeString(f, next) + + val s = Run(dir) + assertEquals(s("sync", "--all"), ExitCode.Ok, s.stdout) + assert(s.stdout.contains("Converged"), s.stdout) + } + + tmp.test("a CRLF origin that moves is Behind, and pull follows it") { dir => + val f = dot(dir, "win.dot", crlf) + val i = Run(dir) + i("import", "win.dot", "--mode", "pull") + + val next = "digraph G {\r\n a -> b\r\n b -> c\r\n}\r\n" + Files.writeString(f, next) + + val s = Run(dir) + assertEquals(s("sync", "--all"), ExitCode.Ok, s.stderr) + assert(s.stdout.contains("Behind"), s.stdout) + assertEquals(s.store.list().head.text, next) + } + // --------------------------------------------------------------- watch /** v1 had no way to observe changes without a window. This is the primitive a From 00e0a4fb47fbc76ea5010ca80f4131bc04cd589d Mon Sep 17 00:00:00 2001 From: Juan Pablo Romero Date: Fri, 21 Aug 2026 08:41:24 -0700 Subject: [PATCH 4/5] fix(gx): four ways a bad ref or a failed write passed for success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each of these was found by the four-angle review of gx-cli and deliberately left out of the cleanup commits. All four are behaviour changes with tests; the five new cases fail 5/5 against the previous code. 70 passed, 0 failed. 1. `watch` skipped the access policy. A ref that was not in the library became an origin via FileOrigins.originOf(cwd.resolve(raw)) with no checkPolicy — the one path-taking command that bypassed the guardrail every other one applies, so `gx watch ` happily followed a file the policy forbids. Now policy-checked like import/get/set/bind, and refused with exit 4. 2. `gx sync ` exited 0. boundTargets flat-mapped unresolvable refs away, so a mistyped name became an empty selection: "(nothing bound to sync)" and success. A script that typo'd a diagram name was told it had synced. selectedRefs now RETURNS the failures, and sync reports every bad ref before failing. 3. Ambiguity was reported as absence — and worse, as a path. findInLibrary's own doc says "Ambiguity is reported rather than resolved by picking one", but it returned None for both "nothing matched" and "several matched". So callers said "no diagram matches" for a name two diagrams share, and the callers that fall back to a PATH did so on an ambiguous ref: `gx set --stdin` created a FILE of that name instead of refusing. resolveRef now returns RefError.NotFound | RefError.Ambiguous; only NotFound may become a path. Tier order (id, name, origin) is unchanged. 4. `syncOne` discarded three save results. env.store.save(updated) with the Either dropped, at all three write sites, while six other call sites treated a failed save as ExitCode.Unknown. gx printed Behind/Ahead and exited 0 having failed to persist the record it had just reconciled — then redid the same work from the same stale baseline on the next run, silently. syncOne now returns a SyncOutcome carrying the failure, and an unsaved record outranks divergence in the exit code. Two findings from the same review that are NOT bugs, and are left alone: - `watch --json` renders compact while other commands use indent = 2. That is correct: it emits NDJSON, one object per line, and pretty-printing would break line-delimited parsing. - `open` does not map NO_SESSION to NeedsDesktop the way `session` does. Also correct: for `session` the tier genuinely needs a window, while for `open` the desktop IS running and the show call failed — reporting "no desktop" would be false. --- .../org/jpablo/graphexplorer/gx/Cli.scala | 235 ++++++++++++------ .../org/jpablo/graphexplorer/gx/CliSpec.scala | 104 ++++++++ 2 files changed, 265 insertions(+), 74 deletions(-) diff --git a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala index 5f31021d..7e5253fc 100644 --- a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala +++ b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Cli.scala @@ -20,6 +20,24 @@ import org.jpablo.graphexplorer.gxcore.rpc.ChannelError import java.nio.file.{Path, Paths} import scala.util.control.NonFatal +/** Why a reference resolved to no single diagram. Two cases, not one: "you + * typed a name nothing has" and "you typed a name several things share" need + * different answers from the user. + */ +private enum RefError derives CanEqual: + case NotFound(ref: String) + case Ambiguous(ref: String, matches: Vector[Diagram]) + +/** What one reconciliation did. + * + * `failure` is a store write that did NOT land. It used to be discarded — + * `env.store.save(updated)` with the Either thrown away at three sites — so + * `gx sync` printed Behind/Ahead and exited 0 after failing to persist the + * record it had just reconciled. The next run then redid the same work from + * the same stale baseline, silently, forever. + */ +private case class SyncOutcome(diagram: Diagram, state: SyncState, failure: Option[String]) + /** What a reference on the command line turned out to mean. */ private enum Target derives CanEqual: case InLibrary(diagram: Diagram) @@ -343,11 +361,9 @@ object Cli: modeOf(args, default = SyncMode.Pull) match case Left(why) => env.err(s"gx: $why"); ExitCode.Usage case Right(mode) => - findInLibrary(ref, env) match - case None => - env.err(s"gx: no diagram matches '$ref'") - ExitCode.InvalidPathOrPolicy - case Some(d) => + resolveRef(ref, env) match + case Left(err) => reportRef(err, env) + case Right(d) => val path = env.cwd.resolve(rawPath) checkPolicy(path, env) match case Left(code) => code @@ -374,42 +390,64 @@ object Cli: ExitCode.Usage private def unbind(args: Args, env: CliEnv): Int = - args.positionalAt(0).flatMap(findInLibrary(_, env)) match + args.positionalAt(0) match case None => env.err("gx: unbind needs a diagram") ExitCode.InvalidPathOrPolicy - case Some(d) => - val detached = d.copy(binding = None, updatedAt = env.now()) - persist(detached, env): _ => - printDiagram(detached, args, env); ExitCode.Ok + case Some(ref) => + resolveRef(ref, env) match + case Left(err) => reportRef(err, env) + case Right(d) => + val detached = d.copy(binding = None, updatedAt = env.now()) + persist(detached, env): _ => + printDiagram(detached, args, env); ExitCode.Ok // --------------------------------------------------------------- sync private def sync(args: Args, env: CliEnv): Int = - val targets = boundTargets(args, env) + val (unknown, found) = selectedRefs(args, env) + // Report every bad ref, not just the first: a script passing ten names + // wants all the typos at once. + if unknown.nonEmpty then unknown.map(reportRef(_, env)).last + else syncTargets(found.filter(_.isBound), args, env) + private def syncTargets(targets: Vector[Diagram], args: Args, env: CliEnv): Int = if targets.isEmpty then env.out(if args.json then "[]" else "(nothing bound to sync)") ExitCode.Ok else val results = targets.map(syncOne(_, env)) - val diverged = results.count(_._2 == SyncState.Diverged) + val diverged = results.count(_.state == SyncState.Diverged) + val failures = results.flatMap(o => o.failure.map(o.diagram.id.value -> _)) if args.json then env.out( ujson.Arr - .from(results.map((d, state) => - ujson.Obj("id" -> d.id.value, "state" -> state.toString) + .from(results.map(o => + ujson.Obj("id" -> o.diagram.id.value, "state" -> o.state.toString) )) .render(indent = 2) ) - else for (d, state) <- results do env.out(f"${d.id.value}%-28s $state") - // Divergence is a state, not an error (§5.2) — but a script that just - // pushed and wants to know whether it landed deserves a non-zero code. - if diverged > 0 then ExitCode.Conflict else ExitCode.Ok + else for o <- results do env.out(f"${o.diagram.id.value}%-28s ${o.state}") + + for (id, why) <- failures do env.err(s"gx: $id reconciled but could not be saved: $why") + + // A write that did not land outranks divergence: divergence is a state + // the user can act on, an unsaved record is one they have not been told + // about. Divergence is a state, not an error (§5.2) — but a script that + // just pushed and wants to know whether it landed deserves a non-zero code. + if failures.nonEmpty then ExitCode.Unknown + else if diverged > 0 then ExitCode.Conflict + else ExitCode.Ok + + private def syncOne(d: Diagram, env: CliEnv): SyncOutcome = + // Save, and KEEP the answer. Every early return below goes through here. + def store(updated: Diagram, state: SyncState): SyncOutcome = + env.store.save(updated) match + case Left(e) => SyncOutcome(updated, state, Some(e.toString)) + case Right(saved) => SyncOutcome(saved, state, None) - private def syncOne(d: Diagram, env: CliEnv): (Diagram, SyncState) = d.binding match - case None => (d, SyncState.InSync) + case None => SyncOutcome(d, SyncState.InSync, None) case Some(binding) => val path = binding.origin.filePath.map(Paths.get(_)) // ONE read of the origin: it answers both questions below, and the Pull @@ -442,9 +480,8 @@ object Cli: binding = Some(binding.copy(baseHash = doc.hash, lastSyncAt = env.now())), updatedAt = env.now() ) - env.store.save(updated) - (updated, state) - ).getOrElse((d, state)) + store(updated, state) + ).getOrElse(SyncOutcome(d, state, None)) case Some(SyncAction.Push) => (for @@ -453,25 +490,24 @@ object Cli: yield val updated = d.copy(binding = Some(binding.copy(baseHash = doc.hash, lastSyncAt = env.now()))) - env.store.save(updated) env.audit.record(AuditEvent.Written(p.toString, doc.hash, "sync")) - (updated, state) - ).getOrElse((d, state)) + store(updated, state) + ).getOrElse(SyncOutcome(d, state, None)) case Some(SyncAction.AdvanceBase) => // Converged: both sides moved to the same content, so only the // agreed baseline is stale. No I/O — this is what stops a // byte-identical regeneration from looking like a change. val updated = d.copy(binding = Some(binding.copy(baseHash = local, lastSyncAt = env.now()))) - env.store.save(updated) - (updated, state) + store(updated, state) case None => if state == SyncState.Diverged then env.audit.record( AuditEvent.Conflict(binding.origin.value, binding.baseHash, local, "sync") ) - (d, state) + // Nothing was written, so there is nothing that could fail to save. + SyncOutcome(d, state, None) // -------------------------------------------------------------- watch @@ -484,29 +520,45 @@ object Cli: * discarded"). */ private def watch(args: Args, env: CliEnv): Int = - val diagrams = boundTargets(args, env) - - val fromPaths = args.positional.filter(r => findInLibrary(r, env).isEmpty).map: raw => - FileOrigins.originOf(env.cwd.resolve(raw), env.cwd) - - val origins = (diagrams.flatMap(_.binding.map(_.origin)) ++ fromPaths).distinct - - if origins.isEmpty then - env.err("gx: nothing to watch") - ExitCode.Usage + // Resolve each ref ONCE. This used to run findInLibrary twice per argument + // — once to collect the diagrams, once to test emptiness — and each call + // scans the whole library. + val (unresolved, found) = selectedRefs(args, env) + val diagrams = found.filter(_.isBound) + + // Only "nothing matched" can mean "this is a path"; an ambiguous ref is a + // ref, and watching a FILE of that name is not what was asked for. + val (ambiguous, missing) = unresolved.partitionMap: + case err: RefError.Ambiguous => Left(err) + case RefError.NotFound(ref) => Right(ref) + + // A ref that is not in the library is a PATH, and every other path-taking + // command runs it past the access policy first. watch did not, which made + // it the one way to point gx at a file the policy forbids. + val (denied, allowed) = missing.partitionMap(raw => checkPolicy(env.cwd.resolve(raw), env)) + + if ambiguous.nonEmpty then ambiguous.map(reportRef(_, env)).last + else if denied.nonEmpty then denied.head else - if args.has("open") && !env.desktopRunning() then - env.err("gx: --open needs a running desktop; watching anyway") - - val interval = args.value("interval").flatMap(_.toLongOption).getOrElse(50L) - val registry = WatchRegistry(env.audit, debounceMs = interval) - origins.foreach(registry.watch) - for o <- origins do env.err(s"watching ${o.value}") + val fromPaths = allowed.map(FileOrigins.originOf(_, env.cwd)) + val origins = (diagrams.flatMap(_.binding.map(_.origin)) ++ fromPaths).distinct - while env.keepWatching() do - for event <- registry.poll() do emitWatchEvent(event, args, env) - env.sleep(interval) - ExitCode.Ok + if origins.isEmpty then + env.err("gx: nothing to watch") + ExitCode.Usage + else + if args.has("open") && !env.desktopRunning() then + env.err("gx: --open needs a running desktop; watching anyway") + + val interval = args.value("interval").flatMap(_.toLongOption).getOrElse(50L) + val registry = WatchRegistry(env.audit, debounceMs = interval) + origins.foreach(registry.watch) + for o <- origins do env.err(s"watching ${o.value}") + + while env.keepWatching() do + for event <- registry.poll() do emitWatchEvent(event, args, env) + env.sleep(interval) + ExitCode.Ok private def emitWatchEvent(event: WatchEvent, args: Args, env: CliEnv): Unit = val (kind, uri, hash) = event match @@ -794,15 +846,16 @@ object Cli: */ private def pathToShow(args: Args, env: CliEnv): Either[Int, Path] = val ref = args.positional.head - findInLibrary(ref, env) match - case Some(d) => + resolveRef(ref, env) match + case Left(err: RefError.Ambiguous) => Left(reportRef(err, env)) + case Right(d) => d.binding.flatMap(_.origin.filePath).map(Paths.get(_)) match case Some(path) => Right(path) case None => env.err(s"gx: '${d.name}' is not bound to a file, so there is nothing to open") env.err(s"gx: bind it first: gx bind ${d.id.value} ") Left(ExitCode.InvalidPathOrPolicy) - case None => + case Left(_: RefError.NotFound) => checkPolicy(env.cwd.resolve(ref), env) // ---------------------------------------------------------- resolution @@ -813,9 +866,13 @@ object Cli: env.err("gx: this command needs a diagram or a path") ExitCode.Usage case Some(ref) => - findInLibrary(ref, env) match - case Some(d) => f(Target.InLibrary(d)) - case None => + resolveRef(ref, env) match + case Right(d) => f(Target.InLibrary(d)) + // Only "nothing matched" can mean "this is a path". An ambiguous ref + // falling through here is how `gx set ` ended up writing + // to a FILE of that name instead of refusing. + case Left(err: RefError.Ambiguous) => reportRef(err, env) + case Left(_: RefError.NotFound) => val path = env.cwd.resolve(ref) checkPolicy(path, env) match case Left(code) => code @@ -824,20 +881,46 @@ object Cli: /** id, then exact name, then the origin path. Ambiguity is reported rather * than resolved by picking one, because "gx set" on the wrong diagram is not * a mistake the user can see happening. + * + * That promise used to be unkept: this returned None for "nothing matched" + * AND for "several matched", so every caller rendered both as "no diagram + * matches" — and the ones that fall back to treating the ref as a PATH did + * so on an ambiguous name, which is the wrong-diagram write the paragraph + * above is about. */ - private def findInLibrary(ref: String, env: CliEnv): Option[Diagram] = + private def resolveRef(ref: String, env: CliEnv): Either[RefError, Diagram] = val all = env.store.list() - all - .find(_.id.value == ref) - .orElse: - all.filter(_.name == ref) match - case Vector(one) => Some(one) - case _ => None - .orElse: - val origin = FileOrigins.originOf(env.cwd.resolve(ref), env.cwd) - all.filter(_.binding.exists(_.origin == origin)) match - case Vector(one) => Some(one) - case _ => None + lazy val byName = all.filter(_.name == ref) + lazy val byOrigin = + val origin = FileOrigins.originOf(env.cwd.resolve(ref), env.cwd) + all.filter(_.binding.exists(_.origin == origin)) + + all.find(_.id.value == ref) match + case Some(d) => Right(d) + case None => + // Tier order is preserved: a unique name beats an origin match, and an + // origin match still answers when the name tier found nothing usable. + (byName, byOrigin) match + case (Vector(one), _) => Right(one) + case (_, Vector(one)) => Right(one) + case (Vector(), Vector()) => Left(RefError.NotFound(ref)) + case (many, others) => Left(RefError.Ambiguous(ref, (many ++ others).distinct)) + + /** True when the ref names something in the library. Callers that fall back + * to a path use this; an AMBIGUOUS ref is not a path and must not fall + * through, so they check [[resolveRef]] rather than this. + */ + private def findInLibrary(ref: String, env: CliEnv): Option[Diagram] = + resolveRef(ref, env).toOption + + private def reportRef(err: RefError, env: CliEnv): Int = err match + case RefError.NotFound(ref) => + env.err(s"gx: no diagram matches '$ref'") + ExitCode.InvalidPathOrPolicy + case RefError.Ambiguous(ref, matches) => + env.err(s"gx: '$ref' matches ${matches.size} diagrams — name one by id:") + for d <- matches do env.err(s"gx: ${d.id.value} ${d.name}") + ExitCode.InvalidPathOrPolicy // ------------------------------------------------------------- helpers @@ -858,13 +941,17 @@ object Cli: else names.foreach(env.out) ExitCode.Ok - /** `--all`, or no arguments at all, means every bound diagram. `sync` and - * `watch` are documented as sharing that rule, so they read it from here - * rather than each restating it. + /** `--all`, or no arguments at all, means the whole library; otherwise the + * positional refs, resolved. `sync` and `watch` are documented as sharing + * that rule, so they read it from here rather than each restating it. + * + * Failures are RETURNED, not dropped. Flat-mapping them away is what made + * `gx sync typo` print "(nothing bound to sync)" and exit 0 — a script that + * mistyped a name was told it had succeeded. */ - private def boundTargets(args: Args, env: CliEnv): Vector[Diagram] = - if args.has("all") || args.positional.isEmpty then env.store.list().filter(_.isBound) - else args.positional.flatMap(findInLibrary(_, env)).filter(_.isBound) + private def selectedRefs(args: Args, env: CliEnv): (Vector[RefError], Vector[Diagram]) = + if args.has("all") || args.positional.isEmpty then (Vector.empty, env.store.list()) + else args.positional.partitionMap(resolveRef(_, env)) private def checkPolicy(path: Path, env: CliEnv): Either[Int, Path] = env.policy.evaluate(path, env.cwd) match diff --git a/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala b/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala index 8c675198..782a5c8e 100644 --- a/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala +++ b/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala @@ -6,6 +6,7 @@ import org.jpablo.graphexplorer.gxcore.rpc.ChannelError import org.jpablo.graphexplorer.gxcore.store.LibraryStore import java.nio.file.{Files, Path} +import java.nio.file.attribute.PosixFilePermissions /** V-09: every command except `open` works with no desktop running. * @@ -254,6 +255,94 @@ class CliSpec extends FunSuite: assert(r.stdout.contains("nothing bound"), r.stdout) } + /** A ref that resolves to nothing is a typo, not an empty selection. This + * used to be flat-mapped away: "(nothing bound to sync)" and exit 0, which + * tells a script that mistyped a name that it succeeded. + */ + tmp.test("sync on a ref that matches nothing fails, it does not report success") { dir => + dot(dir, "gen.dot", "v1") + val i = Run(dir) + i("import", "gen.dot", "--mode", "sync") + + val s = Run(dir) + assertNotEquals(s("sync", "typo"), ExitCode.Ok, s.stdout) + assert(s.stderr.contains("no diagram matches"), s.stderr) + assert(!s.stdout.contains("nothing bound"), s.stdout) + } + + /** A reconciliation that cannot be persisted is not a success. The three + * saves inside syncOne discarded their Either, so gx printed `Behind`, + * exited 0, and left the record on its old baseline — and the next run + * redid the same work from the same stale state, silently. + */ + tmp.test("a sync that cannot save the record reports it and does not exit 0") { dir => + val f = dot(dir, "gen.dot", "v1") + val i = Run(dir) + i("import", "gen.dot", "--mode", "pull") + + Files.writeString(f, "v2") // the origin moves, so Pull has something to save + + // Readable and listable, but nothing new can be created in it — so the + // scan still finds the record and only the WRITE fails. + val diagrams = dir.resolve("library").resolve("diagrams") + val restore = Files.getPosixFilePermissions(diagrams) + Files.setPosixFilePermissions(diagrams, PosixFilePermissions.fromString("r-xr-xr-x")) + try + // Root ignores the mode bits; there is nothing to assert on such a box. + assume( + scala.util.Try(Files.createTempFile(diagrams, "probe", null)).isFailure, + "this user can write to a read-only directory" + ) + + val s = Run(dir) + assertNotEquals(s("sync", "--all"), ExitCode.Ok, s.stdout) + assert(s.stderr.contains("could not be saved"), s.stderr) + finally Files.setPosixFilePermissions(diagrams, restore) + } + + // ------------------------------------------------------- ambiguous refs + // + // findInLibrary's own doc says ambiguity is reported rather than resolved by + // picking one. It returned None for "nothing matched" AND "several matched", + // so callers rendered both as "no diagram matches" — and the ones that fall + // back to a PATH did so on an ambiguous name. + + /** Two records deliberately sharing a name; only their ids differ. */ + private def twoNamed(dir: Path, name: String): Run = + dot(dir, "a.dot", "digraph { a }") + dot(dir, "b.dot", "digraph { b }") + val i = Run(dir) + i("import", "a.dot", "--name", name) + i("import", "b.dot", "--name", name) + i + + tmp.test("an ambiguous ref is reported as ambiguous, with the ids to pick from") { dir => + twoNamed(dir, "Shared Thing") + val r = Run(dir) + assertNotEquals(r("get", "Shared Thing"), ExitCode.Ok, r.stdout) + assert(r.stderr.contains("matches 2 diagrams"), r.stderr) + assert(!r.stderr.contains("no diagram matches"), "ambiguity reported as absence") + } + + /** The wrong-diagram write the doc warns about, in its real form: with no + * ambiguity case, `set` fell through to treating the ref as a path and + * created a FILE called `shared` instead of refusing. + */ + tmp.test("an ambiguous ref does not fall through to being a path") { dir => + twoNamed(dir, "Shared Thing") + val r = Run(dir, stdinText = "digraph { c }") + assertNotEquals(r("set", "Shared Thing", "--stdin"), ExitCode.Ok, r.stdout) + assert(r.stderr.contains("matches 2 diagrams"), r.stderr) + assert(!Files.exists(dir.resolve("Shared Thing")), "set wrote a file named after an ambiguous ref") + } + + tmp.test("an unambiguous id still resolves when its name is shared") { dir => + val i = twoNamed(dir, "Shared Thing") + val id = i.store.list().head.id + val r = Run(dir) + assertEquals(r("get", id.value), ExitCode.Ok, r.stderr) + } + // ------------------------------------------------- sync × line endings // // `base` and `remote` are hashes of file BYTES, so `local` has to be measured @@ -528,6 +617,21 @@ class CliSpec extends FunSuite: assert(r.stderr.contains("denied root"), r.stderr) } + /** The guardrail has to hold on EVERY path-taking command, not most of them. + * `watch` took a ref that was not in the library, turned it straight into an + * origin, and started following it — the one way to point gx at a file the + * policy forbids. + */ + tmp.test("watch refuses a denied path too, like every other command") { dir => + val secret = Files.createDirectories(dir.resolve("secrets")) + Files.writeString(secret.resolve("a.dot"), "x") + val r = Run(dir) + val env = r.env.copy(policy = AccessPolicy(Nil, List(secret))) + assertEquals(Cli.run(Vector("watch", "secrets/a.dot"), env), ExitCode.InvalidPathOrPolicy) + assert(r.stderr.contains("denied root"), r.stderr) + assert(!r.stderr.contains("watching "), "watch followed a denied path anyway") + } + // ----------------------------------------------------------------- run /** The document tier, headless (D7.2). These are V-09's point restated for From df623d1aec0bb0d333b7caeec37032fa92b0ce19 Mon Sep 17 00:00:00 2001 From: Juan Pablo Romero Date: Fri, 21 Aug 2026 09:21:52 -0700 Subject: [PATCH 5/5] fix(gx-core): audit gets the real clock, and stops re-chmodding once per line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit was the one hole in the CLI's clock seam. Every timestamp in gx comes from the injected `env.now()` — every `updatedAt`, every `lastSyncAt` — except the audit line describing the very same operation, which stamped its own `System.currentTimeMillis()` inside toJson. A record and the line explaining it could disagree about when it happened, and no test could pin either. Audit now takes the clock (defaulted, so existing callers are unaffected) and Main hands the same `clock` to both Audit and CliEnv. The new test asserts the stamp falls inside the fixture's 1000-and-counting range; against the previous code it reports a thirteen-digit wall-clock value. Same file: `record` called Files.createDirectories AND setPosixFilePermissions around EVERY appended line — two extra syscalls per event, which `watch` pays once per change for as long as it runs. Both now happen only when the file is actually being created, tested with one isRegularFile stat. That still re-restricts a log deleted underneath us, which a "do it once per process" flag would not. Also deleted two dead members of gx-core that were not merely unused but actively misleading, each sitting next to a live hand-rolled answer to the same question: - LibraryStore.inFolder — zero callers, and filters `_.folder == folder` (exact) while the live folder query in `gx ls` uses `_.folder.isUnder(f)` (recursive). Whoever "reused the store helper" would have got results that silently disagree with gx ls. - ChannelError.describe — zero callers, and its messages are terser than the CLI's, which deliberately add the next step ("Start Graph Explorer Desktop", "only `gx open` needs it") and are asserted on in CliSpec. Reusing it would have regressed them. An unused helper beside two hand-rolled copies is a worse signal than plain duplication: it says the right seam was identified and then routed around. In both these cases the helper was also wrong for the job, so deleting is the fix rather than adopting. Not changed, deliberately: `DiagramId.derivedFrom` does no sanitising despite LibraryStore.sanitize's comment claiming it "produces ids that are already safe". Its only caller passes `ls-` and ProjectId.random is a dashless UUID, so every reachable id is already file-safe and no collision exists today. Worth tightening when something else starts deriving ids. Full suite: 2193 tests across all modules, 0 failures. --- .../org/jpablo/graphexplorer/gx/Main.scala | 9 ++++++-- .../org/jpablo/graphexplorer/gx/CliSpec.scala | 21 +++++++++++++++++- .../graphexplorer/gxcore/fs/Audit.scala | 22 ++++++++++++++----- .../gxcore/rpc/ControlChannel.scala | 5 ----- .../gxcore/store/LibraryStore.scala | 3 --- 5 files changed, 43 insertions(+), 17 deletions(-) diff --git a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala index 9c48e2bc..5251dc42 100644 --- a/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala +++ b/gx-cli/src/main/scala/org/jpablo/graphexplorer/gx/Main.scala @@ -38,10 +38,15 @@ object Main: if debug then message => System.err.println(s"gx[protocol] $message") else _ => () + // One clock for the whole process: the audit log stamps its lines from the + // same source as every `updatedAt` and `lastSyncAt`, so a record and the + // line describing it cannot disagree about when it happened. + val clock: () => Long = () => System.currentTimeMillis() + val env = CliEnv( store = LibraryStore.default(gxHome), policy = AccessPolicy.fromEnv(), - audit = Audit(runtime.resolve("audit.log.jsonl")), + audit = Audit(runtime.resolve("audit.log.jsonl"), clock), // The user's shell, not the process's idea of it. v1 learned this the hard // way: the desktop's working directory is an artifact of how it was // launched, so paths must be resolved where the human typed them. @@ -49,7 +54,7 @@ object Main: out = println, err = System.err.println, stdin = () => String(System.in.readAllBytes(), StandardCharsets.UTF_8), // V-16 - now = () => System.currentTimeMillis(), + now = clock, desktopRunning = () => Main.desktopRunning(control, trace), rpc = (method, params) => Main.call(control, trace, method, params) ) diff --git a/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala b/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala index 782a5c8e..efd50537 100644 --- a/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala +++ b/gx-cli/src/test/scala/org/jpablo/graphexplorer/gx/CliSpec.scala @@ -53,7 +53,9 @@ class CliSpec extends FunSuite: val env: CliEnv = CliEnv( store = store, policy = AccessPolicy(Nil, Nil), - audit = Audit(dir.resolve("audit.jsonl")), + // Same clock the CLI uses, so an audit line's timestamp is a fact a test + // can assert rather than whatever the wall clock said. + audit = Audit(dir.resolve("audit.jsonl"), () => clock), cwd = dir, out = s => out.append(s).append('\n'), err = s => err.append(s).append('\n'), @@ -617,6 +619,23 @@ class CliSpec extends FunSuite: assert(r.stderr.contains("denied root"), r.stderr) } + /** The audit log is the only place `source` is recorded, so WHEN an event + * happened has to come from the same clock as the record it describes. + * Audit stamped its own `System.currentTimeMillis()` instead — the one hole + * in a seam every other timestamp goes through. + */ + tmp.test("an audit line is stamped from the injected clock, not the wall clock") { dir => + dot(dir, "a.dot") + val r = Run(dir) + assertEquals(r("import", "a.dot"), ExitCode.Ok, r.stderr) + + val line = r.env.audit.entries.headOption.getOrElse(fail("nothing was audited")) + val stamp = ujson.read(line).obj("timestampMs").num.toLong + // The fixture's clock starts at 1000 and ticks by one per read, so a real + // wall-clock stamp is thirteen digits and this assertion is unmissable. + assert(stamp > 1000L && stamp < 2000L, s"audit used a clock the test does not control: $stamp") + } + /** The guardrail has to hold on EVERY path-taking command, not most of them. * `watch` took a ref that was not in the library, turned it straight into an * origin, and started following it — the one way to point gx at a file the diff --git a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala index 726bbbc1..ad7c8c06 100644 --- a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala +++ b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/fs/Audit.scala @@ -28,21 +28,31 @@ enum AuditEvent derives CanEqual: * Writes are best-effort by construction. Failing to record an action must * never fail the action: a full disk should not make the editor read-only. */ -final class Audit(path: Path): +/** @param now the clock, injected for the same reason every other timestamp in + * the CLI is: an event stamped from a hidden `currentTimeMillis` + * cannot be pinned by a test, and this was the one hole left in an + * otherwise complete seam. + */ +final class Audit(path: Path, now: () => Long = () => System.currentTimeMillis()): private val lock = Object() def record(event: AuditEvent): Unit = lock.synchronized: try - Option(path.getParent).foreach(Files.createDirectories(_)) - val line = Audit.toJson(event) + "\n" + // Only a file we are about to CREATE needs its directory made and its + // mode set. Doing both per line cost two extra syscalls on every event, + // which `watch` pays once per change for as long as it runs — and + // testing existence still re-restricts a log deleted underneath us. + val existed = Files.isRegularFile(path) + if !existed then Option(path.getParent).foreach(Files.createDirectories(_)) + val line = Audit.toJson(event, now()) + "\n" Files.write( path, line.getBytes(StandardCharsets.UTF_8), StandardOpenOption.CREATE, StandardOpenOption.APPEND ) - Audit.restrictToOwner(path) + if !existed then Audit.restrictToOwner(path) catch case NonFatal(_) => () // never fail the operation being audited def entries: Vector[String] = @@ -74,7 +84,7 @@ object Audit: private def obj(fields: (String, String)*): String = fields.map((k, v) => s""""$k":"${escape(v)}"""").mkString("{", ",", "}") - private[fs] def toJson(event: AuditEvent): String = + private[fs] def toJson(event: AuditEvent, timestampMs: Long): String = import AuditEvent.* val base = event match case Allowed(path, action) => obj("event" -> "allowed", "path" -> path, "action" -> action) @@ -93,4 +103,4 @@ object Audit: case WatchRemoved(uri) => obj("event" -> "watch.removed", "uri" -> uri) case OriginMissing(path) => obj("event" -> "origin.missing", "path" -> path) // Timestamp is prepended rather than threaded through every case. - s"""{"timestampMs":${System.currentTimeMillis()},${base.drop(1)}""" + s"""{"timestampMs":$timestampMs,${base.drop(1)}""" diff --git a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala index 8073370f..4738c9d3 100644 --- a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala +++ b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/rpc/ControlChannel.scala @@ -20,11 +20,6 @@ enum ChannelError derives CanEqual: case Io(message: String) case Rpc(code: String, message: String, details: ujson.Obj) - def describe: String = this match - case NoDesktop(_) => "no desktop is running" - case Io(message) => message - case Rpc(_, message, _) => message - /** The desktop's control channel: a unix socket carrying one JSON object per * line, request then response (D4). * diff --git a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala index c2861c99..c6dc8501 100644 --- a/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala +++ b/gx-core/jvm/src/main/scala/org/jpablo/graphexplorer/gxcore/store/LibraryStore.scala @@ -110,9 +110,6 @@ final class LibraryStore(val root: Path) extends DiagramSink: def findByOrigin(origin: OriginUri): Vector[Diagram] = list().filter(_.binding.exists(_.origin == origin)) - def inFolder(folder: FolderPath): Vector[Diagram] = - list().filter(_.folder == folder) - // ------------------------------------------------------------ folders /** The tree, including folders that hold nothing.