robertomachorro/moped/code-review

Reviewing Moped

Moped is a document-based macOS app: SwiftUI shell, AppKit editor. The editor is MopedEditor, a local Swift package with zero external dependencies — 3.0.0 deliberately replaced Highlightr and STTextView/Neon with in-house code. Adding a third-party dependency is a design change, not a cleanup; flag any PR that introduces one unless the PR is explicitly about that.

Bias the review toward invariants that are load-bearing but invisible — the ones below have each already caused a real bug here. Ordinary style nits are handled by swiftlint --strict; do not spend the review on them.

The four gates

CI (.github/workflows/build.yaml) runs all of these on every PR, in this order:

1. ./scripts/check_localized_strings.sh
2. swiftlint --strict over the whole project
3. swift test --package-path MopedEditor
4. the Xcode build

A change is not done until all pass. If a PR touches behavior covered by the package tests and adds none, say so.

Localization

Every user-facing string needs a key in Moped/Localizable.xcstrings. The catalog is at 100% coverage across 13 locales (de, en, es, fi, fr, he, hi, it, ja, nl, pt, pt-BR, uk) — a new key with only English in it silently breaks that, and the check script will not catch it.

Key naming: pref.<setting>.title, pref.section.<section>, option.<group>.<value>, menu.*, alert.*, window.*, status.*, error.*, about.*, default_editor.*. The script's allowlist (scripts/check_localized_strings.sh) rejects anything else.

Where the script is blind, and you should look by hand:

• A string reaching the UI through a variable rather than a literal at the call site.
• A literal passed to a helper rather than to Toggle/Button/Text directly —

PreferencesView.checkbox("pref.…", …) is the standing example. The script's patterns never see it, so a missing catalog key ships as a raw key on screen.

Adding a preference: three legs, and the one that gets forgotten

Preferences (Moped/Preferences.swift) is @unchecked Sendable because it holds no mutable stored state — every property reads and writes UserDefaults. A stored property added to it invalidates that annotation. Flag one.

Booleans are stored as the strings "Yes" / "No" with a do…-prefixed Bool reader. This is deliberate and long-standing; do not suggest converting them to Bool or @AppStorage.

A new editor-affecting preference must be wired in all of:

1. Preferences — the @objc dynamic var plus its do… reader.
2. EditorState.makeEditor(model:delegate:) — the initial build.
3. EditorState.applyPreferences() — the .preferencesChanged observer.
4. PreferencesView — the row, with a catalog key.

Legs 2 and 3 are separate call sites with no shared helper. Missing #3 means the setting only takes effect when some other preference is later written; missing #2 means it only takes effect on an already-open window. Check for both explicitly — this is the single most likely defect in a settings PR.

The editor core

MopedTextView is an NSTextView on an explicit TextKit 1 stack. Temporary attributes and the NSRulerView gutter both depend on NSLayoutManager; "why not TextKit 2" is not a useful review comment.

• Never write rendering state into `NSTextStorage`. Token colors live as layout-manager

temporary attributes precisely so they cannot enter the text, land on the undo stack, or mark the document dirty. A PR that adds an attribute to the storage for display purposes is a bug, however well it renders.

• `SyntaxHighlighter` owns two exclusive resources: textStorage.delegate (one slot

only) and the .foregroundColor temporary attribute, which it clears and rewrites over the whole edited range on every pass. Anything else writing that attribute will be silently wiped.

• `allowsNonContiguousLayout` is on. Any self-computed visible range must call

ensureLayout(forBoundingRect:in:) first, or the glyph range comes back short for a region scrolled into for the first time — the gutter shipped that bug once.

• But never force layout from inside a draw, and never mutate rendering state inside

NSTextStorage.processEditing. Both re-enter TextKit underneath code that has already captured geometry. The established fix is to defer to the next runloop turn (LineNumberRulerView.updateThickness, SyntaxHighlighter.schedulePass).

• Prefer partial invalidation. setNeedsDisplay(rect) over needsDisplay = true where

a bounded region is knowable — a full redraw on every caret move made cursor movement scale with document size. A whole-view invalidation needs a reason in a comment.

• Drawing order matters. The layout manager paints selection and find-bar highlights in

drawBackground(forGlyphRange:at:), before glyphs. Decorations that must survive a selection belong after super.drawGlyphs, not in a background override.

Themes

MopedTheme is Sendable only because every color it carries is a plain sRGB component color. A theme built from NSColor.controlAccentColor or any catalog/dynamic system color reintroduces deferred appearance resolution and breaks the guarantee. Flag it.

Adding a color to MopedTheme is not a small change: it touches the struct, renamed(_:), paired(withDark:), all seven files in Themes/, the System theme, the .mopedtheme read/write maps and fileFormatVersion in EditorTheme+File.swift, plus ThemeFileTests. If a PR can derive its color from an existing one instead (the gutter separator and the whitespace markers both use the palette's own foreground at 30% alpha), prefer that and say so.

Painted colors are pushed out from MopedTextView.applyTheme(), which is the single funnel reached by both a theme change and viewDidChangeEffectiveAppearance(). A new painted color set anywhere else will not survive a light/dark flip.

Conventions

• Tabs for indentation, in Swift and in tests. line_length is disabled but long lines

are still unwelcome.

• Doc comments explain *why*, not *what*. The house style names the bug or constraint

that forced the code to be the way it is. A comment restating the signature adds nothing; a non-obvious decision with no comment is worth flagging.

• Tests name the bug they pin down, and assertion messages are full sentences

explaining the failure. Logic that would otherwise only run inside a graphics context is split into a testable non-drawing method — LineNumberRulerView.visibleLineNumbers(for:) and WhitespaceLayoutManager.whitespaceMarkers(forGlyphRange:) are the precedents.

• Strict concurrency is complete in the app target, and the package mirrors it via

.enableExperimentalFeature("StrictConcurrency") so swift test checks the same rules.

• User-visible changes update the docs: DOCUMENTATION.md (including its settings

tables), CHANGELOG.md, and manual-checklist.md for anything only a human can verify. A feature PR that touches none of them is probably incomplete.

• The Settings window has a fixed, measured .frame(width: 590, height: 246), sized

against the widest locale. The comment above it is a measurement log — a PR that changes the frame without extending that comment, or that adds a pane row without saying whether it still fits, deserves a question.

• Printing (SourcePrintView) is a separate `CTFramesetter` path that does not share

the editor's layout manager. New editor rendering does not print automatically; if a PR implies otherwise, check.

Do not raise these

They are settled decisions, and re-litigating them costs the maintainer time:

• Tabs instead of spaces; the "Yes"/"No" preference encoding; @unchecked Sendable on

Preferences; TextKit 1 instead of TextKit 2.

• "Consider extracting this into a protocol/abstraction" for single-use code. This project

wants the minimum code that solves the problem — no speculative abstraction.

• Line length, given line_length is disabled in .swiftlint.yml.
• Suggesting a third-party library for something the package already does by hand.