Repository navigation
refactor(cli): centralize annotation state and preserve diagnostic order - #259
Merged
Merged
Conversation
johnny4young
marked this pull request as ready for review
October 6, 2026 03:50
johnny4young
commented
Oct 6, 2026
johnny4young
left a comment
Owner
Author
There was a problem hiding this comment.
Review: centralize CLI annotation state
What it does: replaces 22 loose annotation fields on CLIArgumentParser with one invocation-local CLIAnnotationArguments value. Two position/style groups (callout, counter) and five geometry groups (arrow, line, rectangle, highlighter, blur) each own their own validation and projection into CLIOptions.
Verdict: ready. Nothing blocking found.
What I checked
- Diagnostics:
validate()reproduces the old checks one-for-one, with the same messages in the same kind order (callout → counter → arrow → line → rectangle → highlighter), and runs at the same point in the compatibility pass. The precedence tests cover argv-order independence. hasContent: matches the oldstyleOptionsRequesteddisjunction for--editexactly.- Projection:
materializedattaches each group's final color and size to every point, as before.highlighters.sizeand the blur group's style are never set by any flag, so they staynil, matching the old hard-codedsize: nil/color: nil. - Positions:
callout.positionandcounter.positionkeep the "both or neither" semantics. - Isolation: the parser builds a fresh value per invocation (
var annotations = CLIAnnotationArguments()), so no state leaks between runs. - CI was green on
82fc47e(all 5 required jobs).
🟡 Nit: the ARCHITECTURE.md section says "a later style flag applies to every mark of that kind, including earlier marks". That's accurate and long-standing, but it may surprise CLI users. A one-line note in --help or the README for --arrow-color and friends would make it discoverable.
Generated by Claude Code
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
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.
Finding
Annotation parsing carried twenty-two loose fields, while the compatibility pass and option construction repeated the same position/style and geometry decisions. This made per-kind final-style application and error precedence depend on several separate mappings.
Refactor
Regression coverage
Five new native tests cover interleaved repeated marks and final per-kind styles, last assignment of point fields, semantic diagnostic precedence independent of argv order, syntax/cross-feature precedence, and isolation between invocations. Existing per-kind annotation parsing/model tests remain intact. Visual blur stays visual only and does not sanitize sidecars.
Verification
Published source head: 82fc47e
Uploaded tree: f9852179b729792a4fd0bb0953934220236b993d
Explicit base: main at bb8fea0
Scope and integration
Independent main-based PR, separate from open app-review, file-reload/read, CLI batch-preflight and GUI export-publication changes. No public API break, overwrite policy, merge, release, deployment or credential access.
Exact-head hosted evidence
Source head: 82fc47e
Verified remote/local tree: f9852179b729792a4fd0bb0953934220236b993d
Verified synthetic PR merge checkout: ef6211b
No gates, baselines or expected skips were relaxed. No merge, release or deployment.
Human-review readiness
Scoped implementation and regressions are complete at unchanged head 82fc47e. Fresh exact-head checks confirm all five required CI jobs plus GitGuardian successful. No submitted reviews, discussion comments or unresolved inline review threads are pending. Human-review readiness does not qualify an untested combined integration or authorize merge, release or deployment.
Review follow-ups (2026-10-06)
validate()reproduces the old checks one-for-one, with the same messages and kind order.hasContentmatches the old--editdisjunction.materializedattaches the final per-kind style to every point, and highlighter size and blur style staynil.--help/README that a later--<kind>-color/--<kind>-sizeapplies to every mark of that kind, including earlier ones. That is long-standing behavior, unchanged by this PR.Generated by Claude Code