Two /simplify slices, and five bugs they turned up - #49
Merged
Conversation
…er lines 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.
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.
…ently rewritten
`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.
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 <denied>` happily followed a file the policy forbids. Now policy-checked like import/get/set/bind, and refused with exit 4. 2. `gx sync <typo>` 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 <ambiguous> --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.
…per line
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-<ProjectId>` 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.
…anch #48 landed on viewer while this branch was open and touched the same two files this branch rewrote: Cli.scala and CliSpec.scala. Cli.scala auto-merged and is semantically clean: #48 added a `skill` command — a new dispatch arm plus a self-contained handler that goes nowhere near ref resolution, sync or watch, which is all this branch restructured. Verified the merged dispatch carries all thirteen arms and that resolveRef / selectedRefs / SyncOutcome / reportRef all survive. CliSpec.scala had one conflict, in the import block, where both sides added imports: `Paths` and `scala.jdk.CollectionConverters.*` from #48, and `PosixFilePermissions` from this branch's unwritable-store test. Resolved as the union — no test on either side changed. Full suite on the merged tree: 0 failures. gx-cli is 81 tests (71 here + 10 from #48) and gx-core 181 (175 + 6), so both sides' tests are present and passing rather than one side having quietly displaced the other.
✅ Deploy Preview for graph-explorer-net ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two quality passes (
viewer/state,gx-cli), plus the bugs the reviews surfaced — each fixed separately, with regression tests verified to fail against the code they fix.Cleanup (behaviour-preserving)
52fb68fcviewer/state— 9 files, net −70 lines. One rankdir reader instead of two (the local copy also defaulted toRankdir.TBby hand instead ofRankdir.default); 8 members collapsed to oneselectRelated(outgoing, transitive);beyond(b)was exactlygap(b) >= 0; dead members removed;RightPanelSection's four hand-assigned numbers looked like the persisted encoding and weren't (persistence usesordinal). Two per-mouse-move wins:concealedCountsNow()is nowlazy, and a document-wideelementsFromPointhit test is now by-name.877128f5gx-cli— onepersisthelper for six copies of the save ladder, one--listprinter, one bound-target rule.Target.OnDiskcarried anOriginUriall four match sites discarded, costing a redundanttoRealPathper invocation.Fixes (behaviour changes, each with tests)
790605f3CRLF origins.base/remoteare hashes of file bytes;localwas hashed with a hardcodedLineEnding.Lf. Any CRLF-authored origin wasAheadforever — and in--mode syncthat triggers a push, sogx syncsilently rewrote untouched Windows-authored files as LF. A byte-identical regeneration also landed onDivergedinstead ofConverged. Every pre-existing sync test used text with no newline, where LF and CRLF are identical bytes; that is how it shipped.00e0a4fbfour ways a bad ref or failed write passed for success.watchskippedcheckPolicy(the one path-taking command that bypassed the guardrail);gx sync <typo>exited 0; ambiguity was reported as absence, and at the fall-through sites as a path, sogx set <ambiguous> --stdincreated a file of that name;syncOnediscarded threestore.saveresults, reporting success after failing to persist.df623d1aaudit clock + dead traps.Auditstamped its owncurrentTimeMillis— the one hole in the CLI's clock seam — and re-chmodded per line, whichwatchpays per event forever. DeletedLibraryStore.inFolder(exact-match semantics disagreeing withgx ls's recursive query) andChannelError.describe(terser than the CLI messagesCliSpecasserts on): both zero-caller, both traps rather than reusable helpers.Not changed, deliberately
watch --jsonrenders compact because it emits NDJSON;opencorrectly does not mapNO_SESSIONtoNeedsDesktop;DiagramId.derivedFromdoes no sanitising despite a comment claiming it does, but every reachable id is already file-safe.Integration
8c6998d0merges #48, which landed mid-branch and touched the same two files.Cli.scalaauto-merged and was checked semantically (skillis independent of ref resolution/sync/watch);CliSpec.scalahad one import-block conflict, resolved as the union. Both sides' tests are present: gx-cli 81, gx-core 181.Full suite green across all modules.
Note on merge strategy: the recent convention here is squash, which would collapse the two cleanup commits and three fix commits into one. The fixes each carry their own regression tests and are independently revertible — worth keeping separate if you'd rather not squash.