fix: full app review — editor, menu bar, settings, domain, web/CLI, docs and screenshots

[johnny4young]

A full review of the app (functionality, UI/UX, accessibility, localization, architecture, docs, and published screenshots), with every confirmed finding fixed in one branch. The CHANGELOG [Unreleased] section has the user-facing summary.

Editor

• Annotation undo: color and size edits are single undo steps, a blank text callout leaves no undo step, undo/redo exit inline text editing, and typing in the code editor hands ⌘Z back to the text view.
• Duplicate, Bring to Front and Send to Back are Edit-menu items, so their shortcuts work when the toolbar collapses them into a menu. Delete/Backspace removes the selected mark, Escape deselects, and marks are VoiceOver elements with a Delete action.
• ⌘K palette: no export rows for an empty editor, no Markdown for images, custom themes included, labels match the inspector.
• Only an explicit Diff pick turns on diff bands and line numbers (loaded documents keep their own); the language picker's recent-first order now works app-wide.
• Dropping a file on an image-mode editor offers Replace Image only; Remove image clears its marks.
• Inspector: slider ranges come from SettingsDefaults; secret scan reports "No secrets found" / "N lines redacted" and can rescan; labeled title fields and buttons; the scope note wording is fixed.
• Restored windows keep their destination, scale, format and color profile.
• The safe-area column count ignores ANSI escapes; image secret redaction and its PNG encode run off the main actor; the alt-text field no longer observes every keystroke.
• Whole-document paste detects the language before tidying, external replacements clear the text view's undo stack, and the Format button uses a session-owned operation, so large documents finish formatting.
• Shared BackgroundControls and ValueSlider replace three copies across the inspector, Settings and the social card.

Menu bar, quick capture, recents

• Custom themes resolve everywhere: recents, thumbnails, reopen/copy again, comparison boards and the social card.
• Quick capture: multi-block deferral no longer rewrites the default style; "Open Editor" after an empty clipboard no longer wipes the document; Markdown fences are unwrapped; "nothing copied or saved" is informational, not success.
• The panel returns focus to the previous app, closes before follow-up alerts, announces results to VoiceOver, keeps the newest captures visible, and stays fully on screen next to an edge icon (it is sized before it is shown).
• Relaunch to Apply no longer quits; File ▸ Copy Image follows "Close the editor after copying"; Services use the Export settings; View ▸ Theme shows a checkmark and custom themes.
• A capture made this second shows as "now" rather than "in 0 s".

Settings

• Editing a custom theme keeps its id; recipes with embedded custom themes are adopted; closing Welcome marks it seen.
• Localized panels, tooltips, captions, import/validation errors and window titles; a test fails on unlocalized AppKit panel or window copy.
• Typed license activation results, real launch-at-login status, a Style pane matching the inspector, confirmation before Clear All Web Data, App Store pending/restore feedback, and the correct Help pane reference.

Domain and rendering

• CRLF sources split into real rows (redaction, line numbers, JSON formatting, dedent, asciinema casts).
• Terminal: background-color erase in the grid emulator; line mode handles \r\r\n, cursor-up redraws and escape bytes in erases.
• Foreground images honor their DPI; the hex parser is shared and rejects 0x; language detection recognizes JSON and Rust and stops reading prose as SQL.
• Secret scanner: PGP private keys, bearer headers, URL credentials and :=/=> assignments. The URL rule's scheme is anchored, so a 1 MB dotted line scans in 0.3 s instead of 3.7 s.
• Workspace recipes reject mistyped style values; one unknown annotation kind no longer drops every annotation in a share link or restored window; dead code removed.

Web Snapshot, social card, CLI

• The network-quiet wait counts in-flight fetch/XHR requests; the sign-in window supports popups and HTTP authentication and shows the host; the privacy caption no longer claims nothing is sent.
• Social cards with image backgrounds get the opaque matte; raster PDFs use point-sized pages; "Export all sizes" honors the format and encodes off the main thread.
• CLI: bounded stdin and file input (5 MB) with an explicit error; outputs cannot overwrite inputs; batch skips its own output folder and reports typed failure reasons; --edit rejects options it cannot carry and carries the terminal width; --copy writes the file first; the executable path no longer relies on argv[0]; usage errors exit 2.

Architecture

• One WindowPlacement helper for all windows (the comparison board window now fits small screens), one atomic save-panel writer, one activation path, injected stores instead of = .shared defaults, and production uses the tested URLRenderer.configured mapping.

Docs and screenshots

• README, SECURITY, ARCHITECTURE, RELEASING, PRO, CAPABILITIES, TERMINAL and design docs, issue templates, and the site's PRO list and CLI reference now match the code.
• Refreshed editor.png, settings.png, menu-bar.png, annotated.png (the secret line is truly redacted; the blur covers an ordinary line) and og-card.png. A Debug-only --brand-accent launch hook keeps tour screenshots on the brand accent whatever the Mac's accent setting is.

Found while verifying

• Validating the App menu created Sparkle's updater, so its consent prompt took focus in every UI test (and wherever launch deliberately skips the updater); validation no longer creates it.
• Cooperative NSApp.activate() is refused while another app stays active, which left windows opened from Terminal (render --edit), a hotkey, Services or Shortcuts behind it. Activation keeps the forcing call in one place, pinned by a test.
• The editor body is split so older compilers type-check it.
• The menu-bar panel is sized before it is shown, so an edge icon keeps it on screen; a capture made this second reads "now", not "in 0 s".
• Full-screen terminal captures keep the last frame of apps that clear with ESC[H ESC[J before exiting (current lazygit).
• The editor stage is an accessibility container, so the canvas text and the inline callout field keep their own identifiers; UI tests updated.
• The controlled WebKit lane covers the network-quiet wait with a late fetch and a positive control.
• The terminal screenshots (htop, lazygit, Neovim) are recaptured from a neutral demo project, with no personal process list or paths.

Completed after review

• App-wide output behavior (auto-copy, save, close after copy, confidential clipboard) moved out of ExportSettings into one shared OutputBehavior; editor sessions reference it instead of holding stale copies, and the four image-copy paths share one helper.
• CustomThemeRenderer and the theme chrome lookups moved out of HighlightManager.swift.
• Test-only helpers, Bool clipboard twins and unused design tokens removed; tests now exercise the production paths.
• Persisted annotations drop only an unreadable element; unused catalog keys removed, with a test that fails on new ones.
• What's New release notes are translated to Spanish; a test requires every future note to ship translated, and the release checklist says so.
• In-app Help (hotkey, presets) matches docs/HELP.md.
• Hosting sizing for the Social Card, Web Snapshot and Comparison Board windows was measured earlier with the headless harness and showed no per-update cost, so they keep the default sizing.

Verification

• make lint passes.
• make test: 1964 app tests plus the domain, rendering and repository suites, all passing.
• make test-web-capture (diagnostic host): 13/13 controlled WebKit tests, including the new network-quiet scenario.
• cd site && npm test: 58/58.
• UI tests run in CI.

Review follow-ups (2026-10-06)

• Reviewed in depth with no defects found:

  • CLI input/output, --copy + --out ordering, --edit handoff;
  • terminal erase with background color, scanner progress, image decode/DPI;
  • OutputBehavior (same defaults keys, so no preference loss);
  • Web Snapshot network-quiet, HTTP auth and popups, license activation results.

  ruff check scripts qa passes.

• Applied:

  1. 11c97c5 fix(cli): report output conflicts as runtime failures and fold case.
     • New error. An output or sidecar that would replace an input or image resource is found against the filesystem, not in the command line. It now raises CLIError.outputConflict (exit 1, message only) instead of incompatibleOptions (exit 2 plus the full usage text).
     • Case folding. Claimed paths are case-folded, so NOTES.md cannot replace notes.md on case-insensitive APFS. On a case-sensitive volume this fails closed.
     • Tests. Two regression tests: the conflict is a runtime error with exit 1, and a case-only difference is refused. A CHANGELOG entry is included.
  2. 982c47c fix(web): release sign-in popup observations with their popup. Each popup now owns its subtitle KVO observations; they are invalidated when that popup closes. Before, they were held in a controller-wide array until the main sign-in window closed, so they kept firing for popups that no longer existed.

• Not applied (follow-up): distinguishing !persisted from persisted && !isPro in the license activation message. It needs a new user-facing string with its Spanish translation. Both cases already fail closed (no PRO without verified state); only the wording differs.

• Validation of the new commits:

  • Passed: swiftc -parse and swift format lint --strict on every changed file (Swift 6.0.3 on Linux), and scripts/check-repository-hygiene.sh.
  • Not compiled locally: AppKit/WebKit targets cannot build on Linux, so native build and tests for this head rely on macOS CI.

• Release note: foreground images now honor their DPI, so 144-DPI screenshots lay out at half their previous on-canvas size. This is already in the CHANGELOG.

• Merge order: this PR first, then fix(cli): protect batch artifacts and skipped inputs during preflight #254 (retarget to main; it already contains this head). fix(site): patch build-toolchain advisories in the website lockfile #260 (site build advisories) is independent.

Release-aware first-use follow-up (2026-10-07)

The documentation update extends this existing PR without replacing its earlier review fixes (11c97c5 and 982c47c).

• FIRST-CAPTURE now distinguishes current-source free editor handoff from released v1.2.3, which still requires PRO for render including --edit; Open Editor and paste remain the free graphical fallback.
• English and Spanish CLI getting-started pages lead with a self-contained free vgrab --no-context printf sample, using the zsh helper for the current session only. No project file is assumed.
• Automated PNG output is separately labeled PRO and uses synthetic stdin plus --no-overwrite; both troubleshooting translations retain the release qualification.
• Five new site contracts cover source/guide text and built English/Spanish HTML, including shell escaping.

Exact source head: 4871ddb
Parent before this follow-up: 982c47c
Tested tree: 5e1d8e4fc58cfb00dff0614fe52c9df5e33b3266
Main base: bb8fea0

Fresh Linux validation: full site image generation, Astro check/build, HTML validation, and 63/63 tests pass with zero skips; git diff --check, hygiene, changelog/version checks, and portable lint prerequisites pass. Existing local dependencies were reused, with no manifest/lockfile changes. Native make format/lint/build were attempted but require unavailable xcrun/Xcode/XcodeGen. The archive sub-check explicitly skips without ditto/unzip. Exact-head hosted CI subsequently completed successfully; see the evidence below.

Released v1.2.3 source contracts and mock shell examples were checked during preparation. Actual signed-release clipboard capture and browser/native first-use interaction have not been executed here; those remain a separate native acceptance limitation. No real clipboard/user-content capture was performed. This follow-up changes no production Swift, licensing, installer, dependencies, workflow gates, or release state.

Reviewer focus: inspect both languages and usable copied shell examples; confirm the free/released/current-source/PRO boundaries; retain existing installer recovery guidance. No merge, release, deployment, or force-push.

Exact-head qualification (2026-10-07)

Verified unchanged head 4871ddb2f1d2f79213a23afa010e13f3031630b3, tree 5e1d8e4fc58cfb00dff0614fe52c9df5e33b3266, against main bb8fea04d62c82194b8a05cb353d13b3336984bf.

CI run 37553818314 completed successfully: all five jobs passed. Controlled WebKit capture also passed.

• Native strict Swift formatting, Debug and universal Release builds, coverage, performance, and golden gates pass on both supported macOS rows.
• Linux clean site CI confirms 63/63 tests, zero skips, and Astro check with zero errors/warnings.
• Sequoia production coverage: 28,686/48,562 = 59.07%, above the unchanged 47.87% floor. Tahoe: 28,748/48,562 = 59.20%, above the 47.93% floor. Critical changed-line coverage: 715/814 = 87.84% on both rows. These values cover the whole PR, not just its documentation follow-up.
• Each native ordinary UI run reports 108 cases with zero failures and 26 intentional tour skips; the separate strict tour executes all 26 successfully. Both jobs export and validate all 53 required screenshots.
• Tahoe retains strict golden comparison; Sequoia retains explicitly render-only golden smoke. No gate, threshold, baseline, or expected skip was relaxed.

The local Linux formatter/build limitations above remain historical facts; current-source native compilation and testing are now qualified by this exact-head CI. Actual signed v1.2.3 first-use clipboard interaction and rendered browser acceptance of the new copy remain unperformed. This is not a claim that the published release was exercised, nor a combined qualification with other PRs. The PR remains open, conflict-free and ready for human review; no merge/release/deployment occurred.

[johnny4young]

Review: full app review (editor, menu bar, settings, domain, web/CLI, docs, screenshots)

Size: 274 files, about +10.4k/−4.3k lines, 52 commits including worktree merges. I prioritized the parts with real logic or contract risk over copy, screenshots and the string catalog.

Verdict: ready. I found no blocking defects. There are four non-blocking inline notes, and #254 builds on this branch and should merge right after it.

Reviewed in depth (no defects found)

• CLI:
  • Bounded stdin (readBoundedStandardInput stops at limit+1).
  • inputTooLarge, and unexpectedArgument for an extra positional.
  • With both --copy and --out, the file is written first and the same raster is reused for the clipboard, so a failed write no longer replaces what the user had copied.
  • --copy rejects a non-PNG --format when there is no --out.
  • --alt-text now requires a sidecar.
  • --edit rejects output and theme options, refuses empty input, and passes the terminal width through EditorHandoff.Payload, with the columns hint range-checked on both sides.
  • Batch discovery skips the output folder and report files and runs before the output folder is created.
  • _NSGetExecutablePath replaces argv[0].
• Domain:
  • RGBAColor.hexComponents takes ASCII-only hex and rejects 0x/fullwidth digits.
  • SecretScanner: the new bearer, URL-credential and PGP rules. The URL-credential look-behind keeps scanning linear.
  • LanguageDetector: JSON and Rust detection, and SQL now requires a statement verb plus a clause keyword.
  • StyleSnapshot strict decoding is opt-in through userInfo, so presets stay tolerant.
• Terminal:
  • Erases and inserts now fill with the pen's background color (bce), and padding to the right margin happens only when that color is set.
  • ED 0 from the origin snapshots the alt screen.
  • isPopulated ignores colored blanks.
  • Scanner continue on an aborted CSI: scanCSI always advances (C0 abort returns cursor >= start), so the loop cannot spin.
• Images:
  • readBoundedImageData now goes through BoundedFileReader, which adds FIFO and descriptor checks.
  • ImageDecodePolicy still checks the frame count before the per-frame loop.
• Settings:
  • OutputBehavior persists under the same keys, so there's no preference migration or data loss, and editor sessions share the app-wide instance.
  • resetToDefaults resets the shared instance only from its owner.
• Web:
  • NetworkQuietTracker logic. The injected fetch/XHR counter clamps at 0, and a probe failure counts as busy.
  • HTTP auth sheet: password methods only, cancelled after 3 failures.
  • Popup lifecycle, including re-entrant windowWillClose through closePopups(), which is safe because popups is cleared first.
• Licensing: LicenseActivationResult mapping. Deactivation now lives in Entitlements, so switching panes no longer drops its result.
• Python QA scripts: ruff check scripts qa passes.
• CI: all 8 checks are green on b3d7d8e: Linux static, Build & test and UI on Sequoia and Tahoe, WebKit integration on both, and GitGuardian.

Non-blocking notes (inline)

1. 🟡 incompatibleOptions now exits 2 with the full usage text, even for conflicts found at run time against the filesystem (output overwriting an input, and #254's report conflicts).
2. 🟡 KVO observations for sign-in popups outlive closed popups.
3. 🟡 .persistenceFailed also covers "stored but not verified as PRO".
4. ℹ️ DPI-aware point size makes high-DPI foreground images lay out smaller. That's intended, but it's a visible change to call out in release notes.

Process notes

• At this size, a second human pass over the editor and undo changes (EditorCommandResponder, EditorView*, AnnotationToolbar) is advisable. They're hard to verify statically, and I relied on the UI and screenshot-tour lanes for them.
• Future review rounds would be easier to bisect as several focused PRs, which is the pattern #255–#259 already follow.

---

Generated by Claude Code