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.