--- name: code-review description: Review guidance for Moped, a sandboxed macOS SwiftUI text editor with a homegrown TextKit 1 editor core in the local MopedEditor package. Use when reviewing pull requests in this repository — it carries the invariants that a generic Swift review misses, and the suggestions this project does not want. --- # 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..title`, `pref.section.
`, `option..`, `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.