diff --git a/CLAUDE.md b/CLAUDE.md index a56755b..d73ef23 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -138,44 +138,6 @@ Create/update `.env` file: AUTH_TOKEN=your-secure-token-here ``` -#### Security Notes - -- Authentication is **enabled by default** (secure by default) -- Server **crashes on startup** if `AUTH_TOKEN` is missing (fail-safe); release builds also require `DATABASE_PASSWORD` -- All endpoints on port 8080 require authentication, including the adapter WebSocket (`/adapter/v1`) -- Tokens are compared in constant time (`TokenAuthenticationMiddleware.constantTimeEquals`) — keep it that way -- `limit` query parameters are clamped server-side to the bounds declared in `openapi.yaml` (`OpenAPIController.clampedLimit`); the generator does not enforce them -- The adapter WebSocket rejects any `call`/`reply` frame that arrives before the peer's `hello` handshake frame -- `HealthCheck` automations only accept public `https` URLs (`HealthCheck.validate`) so `POST /config` cannot be used to reach internal services -- Set `DATABASE_TLS_VERIFY=true` when the MySQL connection leaves the private container network (see `docs/setup-server.md` for all environment variables) -- The adapter stores the server token in the keychain (`AdapterCredentialStore`), never in `UserDefaults`; enable **Use TLS (wss://)** in the adapter settings when it connects over an untrusted network -- Never commit secrets: `docker-compose.yml` must reference `${AUTH_TOKEN}` etc. from a git-ignored `.env` - -## Pre-Commit Checklist - -Before committing changes that affect iOS apps: - -1. Run SwiftLint: - ```bash - swiftlint --fix - ``` - -2. Build Swift Package: - ```bash - swift build - ``` - -3. Build affected iOS app(s): - ```bash - # For FlowKitAdapter changes - cd Apps/FlowKitAdapter && xcodebuild -project "FlowKit Adapter.xcodeproj" -scheme "FlowKit Adapter" -sdk iphonesimulator -configuration Debug build - - # For FlowKitController changes - cd Apps/FlowKitController && xcodebuild -project "FlowKit Controller.xcodeproj" -scheme "FlowKit Controller" -sdk iphonesimulator -configuration Debug build - ``` - -4. Ensure all builds pass before committing - ## Project Structure ### SPM Modules (`Sources/`) @@ -204,142 +166,7 @@ Before committing changes that affect iOS apps: - `Dockerfile` — Multi-stage Docker build (Swift 6.2, Ubuntu Noble, jemalloc) - `openapi.yaml` — OpenAPI spec; both `Server` and `ServerClient` use `swift-openapi-generator` -## Git Workflow - -This project follows the **Git-Flow** branching model: - -### Branch Structure - -- **`main`** - Production-ready releases only - - Receives git tags for version releases - - Never commit directly to this branch - -- **`develop`** - Integration branch for features - - Never work directly on this branch - - All feature branches merge here via pull requests - -### Feature Development - -1. **Always branch from `develop`:** - ```bash - git checkout develop - git pull - git checkout -b feature/-description - ``` - -2. **Branch naming convention:** - - Format: `feature/-description` - - Example: `feature/99-prevent-server-shutdown` - - Use lowercase and hyphens for readability - -3. **Creating pull requests:** - - Base branch: `develop` (NOT `main`) - - Target: Merge your feature branch into `develop` - - After approval, features are merged to `develop` - -4. **Release process:** - - When ready for release, `develop` is merged to `main` - - Git tags are created on `main` for version tracking - -### Quick Reference - -```bash -# Start new feature -git checkout develop && git pull && git checkout -b feature/123-my-feature - -# Keep feature branch updated -git checkout develop && git pull && git checkout feature/123-my-feature && git merge develop - -# Create PR (always target develop) -gh pr create --base develop --head feature/123-my-feature -``` - -## Database & API Schema Updates - -### ⚠️ CRITICAL: When Adding New Entity Fields - -When adding new sensor fields or entity properties, you MUST update **all three layers**: - -#### 1. Database Migration (REQUIRED) -Create a new migration file in `Sources/Server/Migrations/`: - -```bash -# Example: 04_AddNewSensorFields.swift -``` - -**Template:** -```swift -import Fluent - -struct AddNewSensorFields: AsyncMigration { - func prepare(on database: Database) async throws { - try await database.schema("entityItems") - .field("newFieldName", .double) // or .int, .bool, .string - .update() - } - - func revert(on database: Database) async throws { - try await database.schema("entityItems") - .deleteField("newFieldName") - .update() - } -} -``` - -**Don't forget to register it in `Sources/Server/configure.swift`:** -```swift -app.migrations.add(AddNewSensorFields()) -``` - -#### 2. Update Database Model -File: `Sources/Server/Models/EntityStorageDbItem.swift` - -- Add `@Field` property to the model -- Update `map()` method to convert FROM `EntityStorageItem` -- Update `mapDbItem()` function to convert TO `EntityStorageItem` - -#### 3. Update OpenAPI Schema (REQUIRED) -File: `openapi.yaml` - -Update the `EntityHistoryItem` schema with ALL fields: -```yaml -EntityHistoryItem: - type: object - properties: - id: - type: string - format: uuid - timestamp: - type: string - format: date-time - newFieldName: # ADD YOUR NEW FIELD HERE - type: number - format: double - nullable: true -``` - -#### 4. Update API Response Model -File: `Sources/HAModels/EntityHistoryItem.swift` - -- Add the new field as an optional property -- Update the `Codable` mapping if needed - -#### 5. Update Chart Display Logic (Optional) -File: `Sources/Controller/Extensions/EntityHistoryItem+Chart.swift` - -Add your field to `primaryValue` getter with appropriate priority: -```swift -var primaryValue: Double? { - if let newField { return newField } // Add here - if let temperatureInC { return temperatureInC } - // ... rest of priority list -} -``` - -#### 6. Update Server Controller -File: `Sources/Server/Controllers/OpenAPIController.swift` - -In `getEntityHistory()`, ensure the field is mapped from `EntityStorageItem` to the OpenAPI response. +## Model layers ### Model Architecture @@ -371,31 +198,6 @@ In `getEntityHistory()`, ensure the field is mapped from `EntityStorageItem` to └─────────────────────────────────────────────────────────────┘ ``` -**When to update each:** -- **EntityStorageItem**: When HomeKit adapter reads new characteristics -- **EntityStorageDbItem**: ALWAYS when adding fields that need persistence -- **EntityHistoryItem**: ALWAYS when fields should appear in history charts - -### Migration Checklist - -When adding new entity fields, verify ALL of these: - -- [ ] Created migration file in `Sources/Server/Migrations/` -- [ ] Registered migration in `Sources/Server/configure.swift` -- [ ] Updated `EntityStorageDbItem` model with `@Field` property -- [ ] Updated `map()` method in EntityStorageDbItem (TO database) -- [ ] Updated `mapDbItem()` function (FROM database) -- [ ] Updated `openapi.yaml` schema with new fields -- [ ] Updated `EntityHistoryItem.swift` with new properties -- [ ] Updated OpenAPIController mapping logic -- [ ] Updated chart display logic (if visualization needed) -- [ ] Tested with `swift build` and app builds - -**Example PRs that missed migrations:** -- PR #89: Added entity history but no migration for new fields -- PR #90: Server changes but OpenAPI schema incomplete -- PR #93: ✅ Correct implementation with migration + full schema - ## CI / GitHub Actions PR checks run on `main` and `develop` branches (`.github/workflows/pr-checks.yml`): @@ -406,10 +208,12 @@ PR checks run on `main` and `develop` branches (`.github/workflows/pr-checks.yml Additional workflows: `docker-publish.yaml` (amd64 + arm64 image for pushes to `develop`/`main` and for release tags, which also get a GitHub Release), `renovate-package-resolved.yaml` (dependency updates). -## Documentation +## Where things are +- [CODING_STANDARDS.md](CODING_STANDARDS.md): the code rules (security, pre-commit checks, Git flow, adding entity fields), the shared ones linked from it and this repo's own. Read it before writing code. - `docs/setup-FlowKitAdapter.md` — Adapter setup: launchctl, iOS guided access, logging - `docs/setup-server.md` — Server Docker setup, log levels, migration +- More on how the features work: the project's folder in the knowledge base (the Obsidian vault), under `references/`. Without the vault (a cloud session, CI), go on without it. ## Common Issues diff --git a/CODING_STANDARDS.md b/CODING_STANDARDS.md new file mode 100644 index 0000000..f54ad68 --- /dev/null +++ b/CODING_STANDARDS.md @@ -0,0 +1,217 @@ +# Coding standards + +What code in this repo has to look like. Read it before writing code; a reviewer checks a diff against it. + +## Shared standards + +Shared standards are generated from the Obsidian vault into `docs/coding-standards/` and never edited in the repo. Start at [docs/coding-standards/readme.md](docs/coding-standards/readme.md). + + +- [`docs/coding-standards/general.md`](docs/coding-standards/general.md) +- [`docs/coding-standards/logging.md`](docs/coding-standards/logging.md) +- [`docs/coding-standards/macos-design.md`](docs/coding-standards/macos-design.md) +- [`docs/coding-standards/readme.md`](docs/coding-standards/readme.md) +- [`docs/coding-standards/swift-deprecations.md`](docs/coding-standards/swift-deprecations.md) +- [`docs/coding-standards/swift-testing.md`](docs/coding-standards/swift-testing.md) +- [`docs/coding-standards/swift.md`](docs/coding-standards/swift.md) + + +## This repo + +### Security + +- Authentication is **enabled by default** (secure by default) +- Server **crashes on startup** if `AUTH_TOKEN` is missing (fail-safe); release builds also require `DATABASE_PASSWORD` +- All endpoints on port 8080 require authentication, including the adapter WebSocket (`/adapter/v1`) +- Tokens are compared in constant time (`TokenAuthenticationMiddleware.constantTimeEquals`) — keep it that way +- `limit` query parameters are clamped server-side to the bounds declared in `openapi.yaml` (`OpenAPIController.clampedLimit`); the generator does not enforce them +- The adapter WebSocket rejects any `call`/`reply` frame that arrives before the peer's `hello` handshake frame +- `HealthCheck` automations only accept public `https` URLs (`HealthCheck.validate`) so `POST /config` cannot be used to reach internal services +- Set `DATABASE_TLS_VERIFY=true` when the MySQL connection leaves the private container network (see `docs/setup-server.md` for all environment variables) +- The adapter stores the server token in the keychain (`AdapterCredentialStore`), never in `UserDefaults`; enable **Use TLS (wss://)** in the adapter settings when it connects over an untrusted network +- Never commit secrets: `docker-compose.yml` must reference `${AUTH_TOKEN}` etc. from a git-ignored `.env` + +### Before committing + +Before committing changes that affect iOS apps: + +1. Run SwiftLint: + ```bash + swiftlint --fix + ``` + +2. Build Swift Package: + ```bash + swift build + ``` + +3. Build affected iOS app(s): + ```bash + # For FlowKitAdapter changes + cd Apps/FlowKitAdapter && xcodebuild -project "FlowKit Adapter.xcodeproj" -scheme "FlowKit Adapter" -sdk iphonesimulator -configuration Debug build + + # For FlowKitController changes + cd Apps/FlowKitController && xcodebuild -project "FlowKit Controller.xcodeproj" -scheme "FlowKit Controller" -sdk iphonesimulator -configuration Debug build + ``` + +4. Ensure all builds pass before committing + +### Git workflow + +This project follows the **Git-Flow** branching model: + +#### Branch Structure + +- **`main`** - Production-ready releases only + - Receives git tags for version releases + - Never commit directly to this branch + +- **`develop`** - Integration branch for features + - Never work directly on this branch + - All feature branches merge here via pull requests + +#### Feature Development + +1. **Always branch from `develop`:** + ```bash + git checkout develop + git pull + git checkout -b feature/-description + ``` + +2. **Branch naming convention:** + - Format: `feature/-description` + - Example: `feature/99-prevent-server-shutdown` + - Use lowercase and hyphens for readability + +3. **Creating pull requests:** + - Base branch: `develop` (NOT `main`) + - Target: Merge your feature branch into `develop` + - After approval, features are merged to `develop` + +4. **Release process:** + - When ready for release, `develop` is merged to `main` + - Git tags are created on `main` for version tracking + +#### Quick Reference + +```bash +# Start new feature +git checkout develop && git pull && git checkout -b feature/123-my-feature + +# Keep feature branch updated +git checkout develop && git pull && git checkout feature/123-my-feature && git merge develop + +# Create PR (always target develop) +gh pr create --base develop --head feature/123-my-feature +``` + +### Database and API schema updates + +When adding new sensor fields or entity properties, you MUST update **all three layers**: + +##### 1. Database Migration (REQUIRED) +Create a new migration file in `Sources/Server/Migrations/`: + +```bash +# Example: 04_AddNewSensorFields.swift +``` + +**Template:** +```swift +import Fluent + +struct AddNewSensorFields: AsyncMigration { + func prepare(on database: Database) async throws { + try await database.schema("entityItems") + .field("newFieldName", .double) // or .int, .bool, .string + .update() + } + + func revert(on database: Database) async throws { + try await database.schema("entityItems") + .deleteField("newFieldName") + .update() + } +} +``` + +**Don't forget to register it in `Sources/Server/configure.swift`:** +```swift +app.migrations.add(AddNewSensorFields()) +``` + +##### 2. Update Database Model +File: `Sources/Server/Models/EntityStorageDbItem.swift` + +- Add `@Field` property to the model +- Update `map()` method to convert FROM `EntityStorageItem` +- Update `mapDbItem()` function to convert TO `EntityStorageItem` + +##### 3. Update OpenAPI Schema (REQUIRED) +File: `openapi.yaml` + +Update the `EntityHistoryItem` schema with ALL fields: +```yaml +EntityHistoryItem: + type: object + properties: + id: + type: string + format: uuid + timestamp: + type: string + format: date-time + newFieldName: # ADD YOUR NEW FIELD HERE + type: number + format: double + nullable: true +``` + +##### 4. Update API Response Model +File: `Sources/HAModels/EntityHistoryItem.swift` + +- Add the new field as an optional property +- Update the `Codable` mapping if needed + +##### 5. Update Chart Display Logic (Optional) +File: `Sources/Controller/Extensions/EntityHistoryItem+Chart.swift` + +Add your field to `primaryValue` getter with appropriate priority: +```swift +var primaryValue: Double? { + if let newField { return newField } // Add here + if let temperatureInC { return temperatureInC } + // ... rest of priority list +} +``` + +##### 6. Update Server Controller +File: `Sources/Server/Controllers/OpenAPIController.swift` + +In `getEntityHistory()`, ensure the field is mapped from `EntityStorageItem` to the OpenAPI response. + +**When to update each:** +- **EntityStorageItem**: When HomeKit adapter reads new characteristics +- **EntityStorageDbItem**: ALWAYS when adding fields that need persistence +- **EntityHistoryItem**: ALWAYS when fields should appear in history charts + +#### Migration Checklist + +When adding new entity fields, verify ALL of these: + +- [ ] Created migration file in `Sources/Server/Migrations/` +- [ ] Registered migration in `Sources/Server/configure.swift` +- [ ] Updated `EntityStorageDbItem` model with `@Field` property +- [ ] Updated `map()` method in EntityStorageDbItem (TO database) +- [ ] Updated `mapDbItem()` function (FROM database) +- [ ] Updated `openapi.yaml` schema with new fields +- [ ] Updated `EntityHistoryItem.swift` with new properties +- [ ] Updated OpenAPIController mapping logic +- [ ] Updated chart display logic (if visualization needed) +- [ ] Tested with `swift build` and app builds + +**Example PRs that missed migrations:** +- PR #89: Added entity history but no migration for new fields +- PR #90: Server changes but OpenAPI schema incomplete +- PR #93: ✅ Correct implementation with migration + full schema diff --git a/docs/coding-standards/.manifest b/docs/coding-standards/.manifest new file mode 100644 index 0000000..d3bc416 --- /dev/null +++ b/docs/coding-standards/.manifest @@ -0,0 +1,8 @@ +# Generated by sync-coding-standards.py from ~/KnowledgeBase/Resources/Coding/coding-standards. Do not edit this folder. +d65aa08021d382e3afcf1d72cdce71d96eadaa1e2cd48fb3269f6eca956712b0 general.md +1bf569306e20213df89287f2ccbeb01db2ff418e9eaa1880e3254552c2e50641 logging.md +e9c206d7e8ab7f3ee76a650e315f332cb419643d9a883a3fec06057c9f5e1a2e macos-design.md +e2327be14549704db046f6ad259326db87dd4c0d1234354be718463b945d54b5 readme.md +c9f9ce842aa1a6aaa4edbda4ca9907a88bf42b2028682c5a6280b5ef0249d392 swift-deprecations.md +aa33c54020f7a3db98cd22f2f8adf38147518f01bd40dec3b6341ddb77fe07b2 swift-testing.md +41f968144c1f549c4ed6ad12bbb25bf4966d46971de3f1dd14b6a373118962f5 swift.md diff --git a/docs/coding-standards/general.md b/docs/coding-standards/general.md new file mode 100644 index 0000000..58162d8 --- /dev/null +++ b/docs/coding-standards/general.md @@ -0,0 +1,175 @@ + +# General coding standards + +Rules for any change to source code, in any language. Language-specific rules live next to this file: Swift in `swift.md`, Swift Testing in `swift-testing.md`, logging in `logging.md`. The process around a change (diagnose first, TDD loop, verification, commits, PRs) is `jk:builder`'s. + +Four rules outrank everything further down whenever they collide: + +1. **The simplest thing that works wins.** Little code is the optimum, not a compromise. +2. **SDK before hand-rolled.** Check the platform first; every line not written needs no tests. +3. **A bug gets a failing test first.** Red proves you understood it; green proves you fixed it. +4. **Comments are for humans: the WHY, in at most 2 lines.** + +## Simplicity — little code is the optimum + +- Solve the problem in front of you, at the size it actually has. No speculative generality: no extra parameter, protocol, generic, config flag, or indirection for a caller that does not exist. +- **Pass the narrowest input the logic reads** — the field, not the object holding it. Handing over a whole struct to read one field is **stamp coupling**: the signature claims a dependency on every field, callers and tests have to build the full value, and the next change quietly reaches for a second field instead of announcing it in the signature. Take the whole object once three or more of its fields are read (Fowler's *Preserve Whole Object*). + + ```swift + // FORBIDDEN — stamp coupling: the signature claims 13 fields, the body reads one + func bubbleColor(for content: MessageRowContent) -> Color { + content.isOwnMessage ? .brandTinted : .neutral + } + + // Correct — the call site passes what the decision needs: `bubbleColor(for: content.isOwnMessage)` + func bubbleColor(for isOwnMessage: Bool) -> Color { + isOwnMessage ? .brandTinted : .neutral + } + ``` + +- **Duplication beats premature abstraction** when it reads better. Two clear call sites can be better than one abstract one; abstract on the third occurrence, once the shared shape is known. +- **A value used once is written where it is used.** A named or `static` constant earns its name only when a second call site has to stay in sync with it — before that it is indirection plus a name to maintain, and the reader has to jump to find a literal. Count the call sites before extracting; the rationale that would have gone into the constant's doc comment goes into the comment next to the literal. +- **Smallest diff that solves it.** Unrelated cleanups and refactors are a separate change — mention them instead of smuggling them in. +- Delete dead code rather than commenting it out; version control remembers it. +- Do not add a dependency, layer, file, or type when something already in the project fits. +- If the solution turned out large, say so and offer the smaller version — a big diff for a small problem is a design signal, not an achievement. + +## SDK first — check, don't recall + +- Before writing any helper, look for the platform, framework, or standard-library API that already does it. **Look it up** (`jk:sdk-documentation` for a symbol dump, the framework reference, the LSP) — "I don't remember one" is not evidence that none exists. +- A marginal difference is not a licence to reimplement. One missing option, slightly different wording, a test that wants a fixed instant → adapt the **call site or the test**, not the API. Hand-writing a relative-date formatter when `Date.RelativeFormatStyle` exists costs tests, maintenance, and localisation for free features. +- If the SDK genuinely cannot do it, name the API you checked and what it lacks before writing the substitute. + +## Comments + +- **Comments are written for humans**, not for the compiler and not to narrate the diff. +- **Keep them to 2 lines, ideally 1.** Go longer only when the reason genuinely cannot be said shorter (a subtle invariant, a linked workaround) or the user asked for more detail. +- **A comment that does earn more than one line is shaped for scanning**, because it is text for a human and structure carries as much of the meaning as the wording: + - **A list beats prose** as soon as three or more things are enumerated. One item per line, the distinguishing word first, so the reader finds their case down the left edge instead of parsing a sentence to the end. + - **The point goes in the first line.** A reader who stops there still has it; the rest is detail. + - **One idea per comment, at the line it explains.** Two unrelated reasons are two comments in two places, never one paragraph stacked on top of the block. + - **Name the thing** — the symbol in backticks, the real value, the issue URL. "This", "it" and "the object" make the reader reconstruct the referent from context. + - **No banner lines or ASCII-art boxes.** Wrap at the file's column limit and break at clause boundaries, not mid-phrase. + + ```swift + // FORBIDDEN — three cases welded into one sentence, each read past to reach the next + // Retry on 429 and 503 but never on the other 4xx codes because the request itself is wrong, + // and never once the deadline has passed since the caller has stopped waiting by then. + + // Correct — the cases are the left edge + // Retry only while the failure is transient: + // - 429, 503: the server asked us to back off + // - other 4xx: the request is wrong, a retry cannot fix it + // - deadline passed: the caller has stopped waiting + ``` + +- **The WHY, never the WHAT.** Good names already say what the code does. Comments carry hidden constraints, invariants, workarounds, business rules, and behavior that would surprise a reader. A comment that restates well-named code gets deleted. +- **Skip them entirely for** simple getters, obvious one-liners, and private helpers with descriptive names — the signature already says it. Separate from the `///` DocC rule, which follows architectural relevance rather than access level (see `swift.md`). +- **Do not match surrounding comment density.** Heavily commented neighbours are not a target; judge every comment on its own merit. +- **Never comment at every spot where *you* made a decision.** Having just weighed an option makes it feel noteworthy, but that is a log of the author's reasoning, not something the reader needs — they never had the discussion and see only code that explains itself. The test before writing one: **would a plausible refactor break this without the compiler or the UI showing it?** If not, the code already says it. + + ```swift + // FORBIDDEN — narrates the three lines below; interesting to decide, not to read + /// The font reports its size, the padding grows it — no fixed frame. + private func controlGlyph(_ systemName: String) -> some View { + Image(systemName: systemName) + .font(.body) + .padding(6) + } + + // FORBIDDEN — reads like an invariant, but a shared function already says that both + // callers share it, and breaking the equality changes the layout visibly + /// Both round controls share this treatment — equal glyph heights are what keep the + /// attachment button as tall as the collapsed composer. + private func controlGlyph(_ systemName: String) -> some View { … } + + // Correct — moving the line compiles, looks identical, and only shrinks the tap target + // Inside the label: moved outside the Button, the hit area silently shrinks to the glyph. + .contentShape(.rect) + ``` + +- **Never comment on access levels** — not why something is not `private`, not which file reads it. The access level is the statement; such notes go stale the moment code moves. + + ```swift + // FORBIDDEN — access-level narration, and it is a WHAT + // Not `private`: read by the formatting extension in the sibling file. + let dateFormatter: DateFormatter + + // Correct + let dateFormatter: DateFormatter + ``` + +- Declarations get `///` (DocC); notes inside a function body get `//`. Avoid `/* */`. +- All comments in English. `TODO:` prefix for known-incomplete work. +- Update or delete comments with the code they describe — an outdated comment is worse than none. +- **Swift only — Xcode marker comments** ([SwiftLee](https://www.avanderlee.com/xcode/xcode-mark-line-comment/)): `// MARK: - Title` (titled section + separator), `// MARK: -` (separator only), `// MARK: Title`, plus `// FIXME:` and `// TODO:`, which Xcode shows with an icon in the jump bar. Uppercase, two slashes, colon — otherwise Xcode ignores them. Use them **only where a file really has sections a reader jumps between** (typically one per conformance extension), never as decoration above every second function. Needing many MARKs to make a type navigable means the type is too large — split it instead. Pair a `FIXME:` with `#warning("…")` when the placeholder must not ship. +- No AI attribution, no "generated by" headers, in code, commits, or PRs. + +## Naming + +Names reveal responsibility and read clearly at the call site. No abbreviations beyond universal ones (URL, ID, UUID). Avoid `Manager`, `Helper`, `Handler`, `Coordinator` — prefer `PaymentProcessor`, `ImageCache`, `EventDispatcher`. For **user-facing** names (features, buttons, labels, settings), use `jk:naming`. + +## Errors — surface them, don't swallow them + +- No empty `catch`, no `try?` used to make a path compile, no error discarded to keep a signature clean. If a layer cannot handle a failure, propagate it with enough context to diagnose. +- Fail fast on programmer errors (precondition violations); degrade gracefully on expected ones and give the user an actionable message. +- Never silence a compiler warning or a lint rule instead of fixing the cause. If a suppression is genuinely correct, the comment next to it says why. +- Validate data crossing a boundary (network, file, user input) — it is untrusted. No secrets or personal data in source or logs. +- **Handle the absent value where it can occur.** Unwrap with `guard`/`if let`, optional chaining or an explicit fallback; a force unwrap (`!`, `try!`) is only for a programmer-error invariant, never for data from outside. A fallback default must not hide a `nil` that is really a bug. +- **Fail the way the neighbouring API fails.** A function that returns `nil` where its siblings throw (or the reverse) makes every caller guess; keep one failure contract per type or module, or say in the doc comment why this one differs. + +## Security + +- **Never build a query, command, or path from strings.** SQL takes bound parameters, shell commands take an argument array, file paths are joined by the path API and checked against the directory they must stay in. String interpolation of outside data into any of them is an injection. +- **Encode output for the sink it goes into**: HTML-escape what lands in a page, URL-encode what lands in a URL, quote what lands in CSV. Validation on the way in does not replace encoding on the way out. +- **Authentication and authorization are checked where the data is served**, on every request, not only in the client or the UI that hides a button. + +## Concurrency — nothing you read before an `await` is still true after it + +Language-agnostic rule, because every cooperative-concurrency runtime has it: a suspension point hands control away. State you read before it may have been changed by someone else by the time you resume, so **never write shared state based on a value you read before suspending** — re-read it, or prove you still own what you are about to overwrite. Swift specifics, including the actor-reentrancy catalogue: `jk:swift-concurrency` (`references/bug-patterns.md`). + +**Before adding a new `Mutex`, `NSLock`, `os_unfair_lock`, `DispatchSemaphore`, or any other lock**, load `jk:swift-concurrency` and run the choice past it rather than reaching for one from habit — that skill's default is async/await and actors; a lock is the documented exception (a synchronous API you cannot change, a C/ObjC callback, a single scalar), never a reflex. State the reason a suspension-based primitive does not fit before writing the lock, the same way an escape hatch like `@unchecked Sendable` needs a receipt. + +The frequent instance: a shared in-flight task is cached to deduplicate callers, and every caller clears it on failure. Each `catch` runs *after* a suspension, so a late one evicts a **newer** task that a different caller has since installed — the dedupe silently stops working. + +```swift +// FORBIDDEN — every caller clears; this one may no longer own what it nils out +} catch { + cachedTask = nil + return .fallback +} + +// Correct — only the creator evicts, and only while it still owns the entry +} catch { + if existing == nil, cachedTask == task { cachedTask = nil } + return .fallback +} +``` + +It is a race condition, not a data race, so neither the compiler nor strict concurrency checking will find it — only reasoning about the suspension will. A test that fails the shared task and then asserts a *single* underlying read is what pins it down. + +## Architecture + +Follow the architecture the project already uses, and keep domain logic free of infrastructure and UI concerns so it stays testable. Do not introduce layers, modules, or indirection a project does not have — that is the simplicity rule applied to structure. When a project documents its own conventions (the "This repo" part of its `CODING_STANDARDS.md`, its `CLAUDE.md`), those win over these shared files. + +- **One function, one job.** A function that validates, transforms, persists and notifies in one body is split into named steps the caller runs in order; each step is then testable alone, and the caller reads as the outline. +- **No shared mutable state without an owner.** A global or static `var` that several call sites write is a hidden parameter to all of them; pass the value in, return a new one, or give the state one owner (an actor, an injected dependency) that serializes access. + +## Performance + +These are what a reviewer flags in a diff without a profiler; anything beyond them needs a measurement first, not a guess. + +- **No query or request per loop iteration (N+1).** Fetch the related rows in one query (a join, an `IN (…)`, a batch call) and look them up in memory. +- **Bound every fetch whose size the user controls.** A list read from a database or an API gets a limit or pagination; "all rows" is a size that grows until it is a bug. +- **No blocking I/O on the main thread or an event loop.** File, network and database work runs off the UI thread through the platform's async API. +- **No retain cycles.** A closure stored by the object it captures captures it `weak` or `unowned`; an observer, timer or task started by an object is cancelled when the object goes. +- **No cache without a measured reason.** A cache adds invalidation bugs; it comes with the measurement that showed the uncached path is slow. + +## Tests + +- Assert observable behavior through the public API, not implementation details, so a refactor does not break the suite. +- Keep tests deterministic: inject clock, randomness, and network; no sleeps, no shared mutable fixture, no dependence on execution order. +- One reason to fail per test, and a name that says what breaks. +- **Test the edges and the failures, not only the happy path**: empty input, `nil`, the boundary values on both sides of a limit, and every error the code under test throws or returns. +- **Fake only the seams** — network, clock, file system, the system frameworks behind a dependency client. A mock of the code under test, or of a pure function it calls, tests the mock. +- **A test run must have no side effects on the machine running it.** A suite that opens a real window, plays sound, or reaches shared state is not safe inside an automated `test` command and must be isolated and excluded by default. Swift: any suite that constructs an `NSWindow`/`UIWindow` goes in a target suffixed `*UITests`, kept out of the default `swift test` run via `--skip UITests`. **Verify the exclusion by running it and confirming no window appears** — a `--skip` argument that doesn't match the actual target name silently runs the suite anyway, and reading the skip name back proves nothing. diff --git a/docs/coding-standards/logging.md b/docs/coding-standards/logging.md new file mode 100644 index 0000000..25357ac --- /dev/null +++ b/docs/coding-standards/logging.md @@ -0,0 +1,79 @@ + +# Logging + +How to log so the output stays aggregatable, filterable, and free of secrets. For *temporary* instrumentation during a bug hunt, use `jk:builder`'s `references/debug-logging.md` instead — different rules apply there. + +## The rule in short + +Use the project's logger — `swift-log` (`OSLog` if `swift-log` is unavailable). **Never** `print`, `Swift.print`, `dump`, `debugPrint`, or `os_log`. **Never interpolate dynamic values into the message**: keep the message static and pass values via `metadata:`, otherwise every call site produces a unique string and log aggregation breaks. Levels, `error:` handling, and full examples follow below. + +## Setup — one logger per file + +```swift +private let logger = Logger(label: "com.example.ChannelRepository") +``` + +If the project ships its own logging package with a convenience factory (e.g. a label derived from the file), prefer that over hand-written labels. + +A file-scoped `private let` is enough: no logger passed through initializers, no logger stored on types. The label identifies the source, so it never has to be repeated in the message. + +**Never** use `print`, `Swift.print`, `dump`, `debugPrint`, or `os_log` — SwiftLint enforces this. They bypass levels, labels, and metadata, and they ship to production silently. + +## The one rule: static message, dynamic values in `metadata:` + +Interpolating a value into the message makes every call site produce a unique string, so the log backend can no longer group occurrences of the same event — the message is the aggregation key. + +```swift +// FORBIDDEN — message varies per call, defeats aggregation +logger.debug("Initialized ChannelCollectionView.ViewModel for \(dutyIDs.count) duty ids") + +// Correct — static message, dynamic values in metadata +logger.debug("Initialized ChannelCollectionView.ViewModel", metadata: ["dutyIdCount": "\(dutyIDs.count)"]) +``` + +Metadata keys are `lowerCamelCase` and stable across call sites (`dutyId`, not `duty_id` here and `dutyID` there) — a query filtering on a key only works if the key is spelled the same everywhere. Put every value you might later want to *correlate on* into metadata: entity IDs, counts, states. + +## Levels + +| Level | Use for | +| --- | --- | +| `logger.debug(...)` | Init, internal state transitions, fine-grained flow ("Fetching channels for duty ids …"). | +| `logger.info(...)` | User-visible or cross-layer events ("Button tapped", "Resolved channels"). | +| `logger.warning(...)` | Recoverable problems, missing optional data — the flow continues. | +| `logger.error(...)` | Failures the user or an operator needs to know about. | + +`trace` and `critical` exist but are rarely the right answer: `trace` belongs to a temporary probe, and a `critical` that does not page someone is just an `error`. + +## Errors + +Always attach the error itself — the message says what failed, the error says why. + +```swift +// swift-log has no `error:` parameter — put the error in metadata +logger.error("Failed to load channels", metadata: ["error": "\(error)", "dutyId": "\(dutyID)"]) +``` + +If the project's logging wrapper has a dedicated `error:` parameter, use it instead of metadata. + +Add a correlation ID whenever one exists, so the failure can be tied back to the request or entity that caused it. Log an error **once**, at the layer that decides how to handle it — an error logged at every level on the way up looks like several failures. + +## `OSLog` variant + +When `swift-log` is not available: + +```swift +import OSLog + +private let logger = Logger(subsystem: "com.example.app", category: "ChannelRepository") + +logger.error("Failed to load channels: \(error.localizedDescription, privacy: .public)") +``` + +`OSLog` has no `metadata:`, so values go into the message — that is fine here, because `OSLog` keeps the format string and the arguments separate internally, so aggregation survives. Dynamic values are redacted to `` unless annotated, so add `privacy: .public` to anything that must appear in a captured log — and deliberately *omit* it for anything sensitive. + +## What not to log + +- No secrets, tokens, passwords, or auth headers — not even at `debug`. +- No personal data beyond an opaque identifier; log the user ID, not the email or name. +- Nothing inside a hot loop or a SwiftUI `body` — that is a probe (`jk:builder`'s `references/debug-logging.md`), not a log. +- No log line that only restates the previous one. One line per meaningful decision or boundary crossing, not one per statement. diff --git a/docs/coding-standards/macos-design.md b/docs/coding-standards/macos-design.md new file mode 100644 index 0000000..73893db --- /dev/null +++ b/docs/coding-standards/macos-design.md @@ -0,0 +1,211 @@ + +# macOS Design — Layout Metrics, Window Chrome, Mac Interaction + +A Mac app is not an iOS app with a pointer. It owes the user a full menu bar, resizable windows, keyboard parity, and a layout built on **center equalization** rather than a touch grid. This file carries the concrete numbers and the rules that iOS habits get wrong. + +Sources: Mario Guzman's Mac design guidelines (layout, toolbar, sidebar, inspector) and the `platform-design-skills` macOS HIG ruleset — both linked at the bottom. Where they disagree on spacing, the metrics below win: they are AppKit's actual layout values. + +## The iOS reflexes that break on Mac + +- **There is no 8-point grid.** Mac spacing is `6` between stacked controls, `12` between groups, `20` at window margins. Applying an 8pt rhythm produces a layout that is subtly wrong everywhere. +- **Controls are compact** (roughly 22–28 pt tall), because the pointer is precise. Touch-sized buttons read as a lazy port. +- **No hamburger menu, no bottom tab bar, no floating action button.** The menu bar is the command surface, the sidebar is the navigation surface, the toolbar holds frequent actions. +- **Never sheet-by-default.** Sheets are for multi-step or consequential flows; use popovers, inspectors, or inline editing for everything else. +- **A fixed or non-resizable window is a bug.** So is a custom title bar that hides the traffic lights. + +## Layout metrics + +`20` and `6` carry most layouts. Consistency outranks the exact value: a reader never notices an 18-point margin, but always notices a left margin wider than the right. + +### Regular-size controls (the default — start here) + +| Distance | Value | +|---|---| +| Left / right / bottom window margin | 20 | +| Titlebar or toolbar → first control (not in a group box or tab view) | 14 | +| Label colon → its stacked control | 6 | +| Between stacked controls | 6 minimum | +| Above and below a separator | +12 (on top of the 6) | +| Extra separation inside a group (e.g. checkbox under a radio group) | +8 (on top of the 6) | +| Bottom control group → button row | 12 | + +Tab views: `12` from the top of the tab view to the titlebar/toolbar, `20` window margins, and at least `16` inside a group box and between controls and the tab-view edge. Always center a tab view in its window. + +### Small and mini controls + +Reach for these **only** where space is genuinely at a premium — tool palettes, toolbar accessory views, HUDs, inspector sidebars, utility windows. Never mix control sizes in one window or pane. + +| Distance | Small | Mini | +|---|---|---| +| Window margins | 20 (10 when using group boxes) | 10 left/top/right, 14 bottom | +| Titlebar/toolbar → first control | 14 | — | +| Between sections | 12 | 12 (last control → next section title) | +| Section title → first control | 8 | 8 | +| Between related checkboxes / radio groups | 6 | 4 | +| Slider: title → slider, and below its labels | 4 | — | + +Both examples use a **bold label per section** instead of separators or group boxes, which is how they buy the space back. + +### Grouping — pick one and stay with it + +- **White space**: `12`–`24` between groups, `12` minimum. Best for small groups. +- **Separators**: equal spacing above and below, `12` for regular controls, less for small/mini. Preferred when space is tight. +- **Group boxes**: `16` margin top and bottom inside the box, at least `16` left and right. The strongest visual separation and the most expensive in space; the box title takes over the section name, so rename controls to kill the redundancy. + +### Bottom button row + +`12` above, `20` left/bottom/right. Help button always bottom-left. The confirmation button sits **to the right of Cancel**, matches its width (longer only if it must), and fires on Return. + +### Bottom bars + +A window-frame strip below the body, holding controls that act on the content directly above it — frequently used, but less important than the toolbar. Use a **recessed, momentary-push-in** button so it highlights on hover like a toolbar item. All controls vertically centered; labels in Secondary Label Color. + +| | Large | Small | +|---|---|---| +| Bar height (including separator) | 32 | 22 | +| Control size | Regular | Mini | +| Button size | 31 × 18 | ≥ 25 × 14 | +| Space to leading/trailing edge | 8 | 6 | +| Between buttons | 1 | 1 | + +For mini bars, prefer the `fill` variant of a symbol — it stays legible at that size. + +## Center equalization and alignment + +**Center equalization is visual balance, not center justification**: the visual weight left and right of the window's invisible center axis matches. A label column on the left balances a denser control column on the right. It applies to windows, panes, and the inside of group boxes. + +- Section labels **right-aligned** (their colons line up); their stacked controls **left-aligned**. +- Checkboxes and radio buttons left-aligned to each other. +- **First-baseline align every element in a row**, so label text lines up with pop-up button and combo-box text. Pop-up buttons, text fields, pickers, and combo boxes are taller than checkboxes — give them extra room when stacking. +- Similar controls get **equal widths even across different group boxes**. +- A control that depends on another's state goes below it, indented so its leading edge aligns with the parent's text label. +- Alignment carries across group boxes: the section titles of one box align with the next. + +**Optional description labels** under a control (when the control's own label cannot say enough): small-size, multi-line, Secondary Label Color, `4` below the control. Left-aligned with the control — except under a checkbox or radio button, where it aligns with the control's *title*, so indent at least `14`. + +## Windows + +- Freely resizable with a sensible minimum; a maximum only when the content truly cannot scale. +- Opt into native fullscreen and Split View (`.fullScreenPrimary`). +- Support multiple windows unless the app is a genuine single-purpose utility; document-based apps use `DocumentGroup` and allow many documents at once. +- Document windows show the document name, the proxy icon (draggable), and the edited dot — `representedURL`, `title`, `isDocumentEdited`. +- Persist frame and state across launches (`setFrameAutosaveName`, or SwiftUI scene restoration). +- Traffic lights stay top-left, visible, functional — including under `titlebarAppearsTransparent` + `.fullSizeContentView`. + +## Menu bar and keyboard + +The menu bar is where a user looks for a command they cannot find. Every toolbar item must also exist as a menu command; the reverse does not hold. + +- Standard menus: **App, File, Edit, View, Window, Help** (File only if document-based). App-specific menus go between Edit and View, or View and Window. Never rename or drop the standard App-menu items (About, Settings ⌘, Services, Hide, Quit). +- **Every action reachable by mouse needs a key equivalent.** ⌘+letter for primary, ⌘⇧ for a variant, ⌘⌥ for an alternative mode, ⌘⌃ for window/view controls. +- Menu items track state: disable what does not apply, retitle for context ("Undo Typing", "Delete 3 Items"), check what is toggled. AppKit does this in `validateMenuItem(_:)`. +- Esc cancels and dismisses (popover, sheet, dialog, in-progress operation; reverts a text field). Return fires the default button — `.keyboardShortcut(.defaultAction)` / `.cancelAction`. +- Delete removes the selection, ⌘Delete for the destructive variant, and **⌘Z undoes it**. Space invokes Quick Look. Arrows move selection; left/right collapse/expand or change column; ⌘↑/⌘↓ jump to the ends. Tab and ⇧Tab reach every control — no keyboard traps. +- ⌘-click extends a non-contiguous selection, ⇧-click a range. Right-click opens a contextual menu on **every** interactive element. +- Sidebar toggling has **no HIG-standard shortcut** — ⌘⌃S is common but not reserved. Pick one and put "Show/Hide Sidebar" in the View menu. + +## Toolbar + +Only frequent actions, and only in the **default** set — the user adds the rest. Order items by the user's mental model: highest-visibility first (leading), grouped by task, ranked inside each group. Items enable and disable with the First Responder. + +- **Anchored ends, never a moving target.** The sidebar toggle is pinned leading, the inspector toggle trailing. Expanding either column pushes the *other* items inward; a toggle that slides with it makes the user chase it. Trailing order, outermost first: **inspector toggle → Share → global Search** (Search expands into a field, so it cannot be outermost). +- **The sidebar's toolbar section holds at most two items** — realistically just the sidebar toggle. A narrow sidebar pushes the rest into the overflow menu, where they vanish. Sidebar-specific actions belong in the sidebar's **bottom bar** instead, where they cannot be hidden. +- Overflow order: **lowest `visibilityPriority` goes first regardless of position**, then remaining items right-to-left. +- Centered items (`centeredItemIdentifiers`) are for one special control — a view picker, an address bar, a now-playing view. `NSToolbarItemGroup` renders as a segmented control and collapses to a pop-up when space runs out. +- A title/subtitle is optional and costs item space; use it for the current section and its context (mailbox name + message count), never the app name. +- Styles: **Unified** (or Unified Compact) is the default for most apps; **Expanded** for document-based apps; **Preference** for Settings windows. +- Use the built-ins rather than rebuilding them — `.toggleSidebar`, `.sidebarTrackingSeparator`, `.toggleInspector`, `.inspectorTrackingSeparator`, `.cloudSharing`, `.writingToolsItemIdentifier`, `NSSharingServicePickerToolbarItem`, `NSSearchToolbarItem`, `NSMenuToolbarItem`, `NSToolbarItemGroup`, `NSTrackingSeparatorToolbarItem`. They arrive with the right icon, title, localization, and action. (SDK-before-hand-rolled, applied to chrome.) +- Toolbar items carry an icon *and* a label (`Label`); labels are what make them discoverable. +- An **accessory bar** (`NSTitlebarAccessoryViewController`) is the secondary surface: content-scoped controls only, no global ones. It spans the content columns but not the sidebar or inspector. + +## Sidebar (source list) + +Navigation for "shoebox" apps — the user picks an item, the detail view shows it. It abstracts the file system away (libraries and playlists, not paths); document-based apps rarely need one. + +- Width: **min 225–275, max 350–400**. Needing no maximum is a sign the layout is wrong. +- **At most two levels of hierarchy.** Deeper data gets a second source list between sidebar and detail (as in Notes), or a browser view for deeply nested objects. +- Section headers are optional but fine for distinguishing logical groups ("Folders"). +- Bottom bar for sidebar-scoped actions: add, remove, edit, get info, refresh, sync status — `NSSplitViewItemAccessoryViewController` / `addBottomAlignedAccessoryViewController(_:)` in AppKit, `tabViewSidebarBottomBar(content:)` or `safeAreaBar(edge:alignment:spacing:content:)` in SwiftUI. +- Contextual menus carry the actions that matter but do not earn a bottom-bar button; put important ones in **both**. +- Add a search field when the list can get long. Let the user customize the contents. +- **macOS 26 (Tahoe): sidebar icons tint black in light mode, white in dark** — no longer the app's accent color, because the translucent glass sidebar now floats over the content and tinted icons clash with it. +- Respect the system sidebar icon size (Small/Medium/Large) by using standard API behavior. In AppKit set `allowsExpansionToolTips` so a truncated row reveals itself on hover (SwiftUI needs an `NSViewRepresentable` wrapper for that). + +```swift +// Sidebar and inspector both need a clamped width; the ideal is what the user first sees. +NavigationSplitView { + SidebarList() + .navigationSplitViewColumnWidth(min: 250, ideal: 260, max: 375) +} detail: { + DetailView() +} +``` + +## Inspector + +Auxiliary metadata or controls for the current selection, shown or hidden by the user. Contextual and dynamic: content updates, enables, or hides the moment selection or focus changes. + +- **Sidebar is the modern form** — slides in from the trailing edge, part of the main window. A floating **panel** becomes key while the app stays main, and closes on Esc. +- Same widths as a sidebar: min 225–275, max 350–400. +- Group content in collapsible disclosure sections, tabs, or both — so the user can remember the way back to a control. Inspectors use the **small control size**, densely packed. +- A menu-bar item must open it; a sidebar inspector also needs its toolbar toggle. +- Panel title: the selected tab's name, otherwise just "Inspector". Disable the minimize button. +- Never say "panel" in the UI — "Show Inspector", or "Show Inspector Window". +- HUD style (dark, translucent, small white/gray controls) only when the app is media-centric, lives in a dark or immersive environment, or a standard panel would cover what the user is adjusting. One inspector keeps one style; never switch by mode. + +## Visual design + +- System font at semantic sizes (`.title`, `.headline`, `.body`, `.caption`), SF Mono for code. Never hardcode point sizes. +- System materials for sidebar and toolbar surfaces (`.listStyle(.sidebar)`, `NSVisualEffectView` with `.sidebar` / `.behindWindow`) — vibrancy is what anchors a window to the Mac. +- **Respect the system accent color.** Do not force a brand color onto standard controls. +- Semantic colors only (`.primary`, `.secondary`, `windowBackgroundColor`, `controlBackgroundColor`), so Light and Dark both work. Never `Color.white` / `.black` for a surface. Test both appearances. + +## Accessibility + +- Every icon-only button and toolbar item gets an `accessibilityLabel` — "trash" alone is meaningless to VoiceOver. +- Full keyboard access: everything the mouse can do, the keyboard can do; logical VoiceOver traversal order (fix divergence with `accessibilitySortPriority` / `accessibilityElement(children:)`). +- Honour **Reduce Motion** (drop decorative animation), **Reduce Transparency** (solid background instead of the material), **Bold Text** (`\.legibilityWeight` for custom rendering), and **Increase Contrast** (`\.colorSchemeContrast`, or `accessibilityDisplayShouldIncreaseContrast`). + +```swift +struct SidebarBackground: View { + @Environment(\.accessibilityReduceTransparency) private var reduceTransparency + + var body: some View { + // Reduce Transparency must replace the material outright — dimming it is not enough. + if reduceTransparency { + Color(nsColor: .windowBackgroundColor) + } else { + Rectangle().fill(.ultraThinMaterial) + } + } +} +``` + +- Hover state on every interactive element, and a cursor that names the affordance (I-beam, crosshair, resize, grab). +- Drag and drop both ways: reorder, move between containers, accept Finder drops, export content. + +## System integration + +Dock icon at 1024×1024 plus a Dock menu with recent documents; Core Spotlight indexing for app content; Quick Look previews for custom file types; the Share menu (`ShareLink`) in both directions; the Services menu; App Intents for Shortcuts, and an `.sdef` where AppleScript support matters. + +Notify only for events the user must know about from outside the app, and make it actionable. Success gets inline status, not an alert; modal alerts are for stopping the user before something consequential. Recurring alerts carry a suppression checkbox (`showsSuppressionButton`) and the choice is persisted. Clear the Dock badge as soon as it is addressed. + +## Review checklist + +- [ ] Margins `20`, control spacing `6`, groups `12`–`24` — and the same on both sides +- [ ] Center-equalized; labels right-aligned, controls left-aligned, rows first-baseline aligned +- [ ] Regular control size, unless the pane is a palette / inspector / HUD — and no mixing +- [ ] Complete menu bar; every action has a shortcut; items enable/disable and retitle +- [ ] Window resizable, minimum set, frame restored, traffic lights untouched +- [ ] Sidebar and inspector toggles anchored; trailing order inspector → Share → Search +- [ ] Sidebar ≤ 2 hierarchy levels, width clamped, its actions in the bottom bar +- [ ] ⌘Z undoes every destructive action; Esc dismisses; Space previews; Delete removes +- [ ] Hover states, contextual menus, drag and drop, ⌘/⇧-click multi-selection +- [ ] Dark Mode, system accent, semantic colors, materials +- [ ] VoiceOver labels; Reduce Motion / Reduce Transparency / Bold Text / Increase Contrast honoured + +## Sources + +- [Mac Design Guidelines — Mario Guzman](https://marioaguzman.github.io/design/): [Layout](https://marioaguzman.github.io/design/layoutguidelines/), [Toolbar](https://marioaguzman.github.io/design/toolbarguidelines/), [Sidebar](https://marioaguzman.github.io/design/sidebarguidelines/), [Inspector](https://marioaguzman.github.io/design/inspectorguidelines/) — every metric above +- [platform-design-skills / macOS](https://github.com/ehmo/platform-design-skills/blob/main/skills/macos/SKILL.md) — menu bar, keyboard, accessibility, anti-patterns +- [Apple HIG](https://developer.apple.com/design/human-interface-guidelines/) — the authority when either source is stale diff --git a/docs/coding-standards/readme.md b/docs/coding-standards/readme.md new file mode 100644 index 0000000..d17b9e4 --- /dev/null +++ b/docs/coding-standards/readme.md @@ -0,0 +1,15 @@ + +# Coding standards + +The shared coding standards for Julian's private repos: what the code looks like, the rules a reviewer can check in a diff. Each repo carries a copy of these files in `docs/coding-standards/` and links them from its `CODING_STANDARDS.md`, followed by the repo's own rules. This folder is the only place they are edited; the copies are regenerated from it. + +How to work on a change (diagnose first, the TDD loop, verification, commits, PRs) is not here: that is the `jk:builder` skill. + +| File | Covers | Read when | +|---|---|---| +| `general.md` | Simplicity, SDK first, comments (incl. Xcode `MARK:`), naming, errors and absent values, security, the concurrency rule, architecture, performance, tests (isolation, edge cases, fakes at the seams) | Every change, any language | +| `swift.md` | Swift and SwiftUI style: naming, golden path, no `if`/`switch` expressions, access control, doc comments, typography, layout, availability | Writing or reviewing Swift | +| `swift-testing.md` | Swift Testing: `ext:swift-testing-pro` first, custom assertions with `sourceLocation`, helper naming | Any work with `import Testing` | +| `logging.md` | Static message with values in `metadata:`, levels, errors, the `OSLog` variant, what not to log | Adding, reviewing, or fixing a log call | +| `swift-deprecations.md` | `@available(…, deprecated:)` on availability workarounds, `renamed:` for an Xcode Fix button, `obsoleted:` for an error at a later floor, and how to verify they fire | Writing or reviewing an `if #available` branch or a shim a higher floor would delete | +| `macos-design.md` | Mac layout metrics, center equalization, window chrome, menu bar and keyboard parity, toolbar, sidebar and inspector | Writing or reviewing macOS UI, or porting an iOS layout | diff --git a/docs/coding-standards/swift-deprecations.md b/docs/coding-standards/swift-deprecations.md new file mode 100644 index 0000000..2f5fea4 --- /dev/null +++ b/docs/coding-standards/swift-deprecations.md @@ -0,0 +1,142 @@ + +# Availability Workarounds — Make the Compiler Remind You + +A workaround that exists **only** because the minimum deployment target is too low is debt with no due date. `if #available` branches, shim types and hand-rolled back-ports all keep compiling silently long after the floor has moved past them — nobody greps for them, and a `TODO` comment is not enforced by anything. + +**Rule: every such workaround carries `@available(, deprecated: , message: …)`.** Raising the floor then turns each call site into a compiler warning that says what to do instead. + +## The pattern + +Annotate the smallest symbol that would be **deleted** once the floor moves — usually the shim type or function, never the `if #available` statement itself (statements cannot be annotated). + +```swift +/// Filled toolbar action. Mirrors `NewChatButton`'s availability split for a text button. That split +/// is the only reason the type exists, so raising the package to `.iOS(.v26)` turns every call site +/// into a warning to delete it — the floor is `.iOS(.v18)` today, where the deprecation stays silent. +@available(iOS, deprecated: 26.0, message: "Inline .buttonStyle(.glassProminent) and delete this type") +struct ProminentToolbarButton: View { + var body: some View { + if #available(iOS 26.0, *) { + button.buttonStyle(.glassProminent) + } else { + button.buttonStyle(.borderedProminent) + } + } +} +``` + +Three things make it work: + +- **Platform-scoped, versioned** — `@available(iOS, deprecated: 26.0)`, not `@available(*, deprecated)`. The version is what keeps it quiet until the floor reaches it. +- **The version is where the workaround dies** — the OS version whose API you would inline, not the next release you happen to know about. +- **The message names the replacement**, so whoever hits the warning does not have to re-derive the fix. Keep the whole attribute inside the line-length limit. + +## Getting a Fix button in Xcode — `renamed:` + +`message:` only prints text. `renamed:` additionally attaches a **fix-it**, which Xcode renders as a **Fix** button that rewrites the call site. Both fit in one attribute: the `message:` wins the warning text and the fix-it survives as a `note: use 'X' instead`. + +```swift +/// Pre-26 card. `GlassCard` replaces it one-for-one once the floor allows it, so the removal is a +/// plain rename — that earns a `renamed:` on top of the message. +@available(iOS, deprecated: 26.0, message: "Use GlassCard, which needs iOS 26", renamed: "GlassCard") +struct FlatCard: View { … } +``` + +The fix-it does more than swap an identifier, and it survives the module boundary (shim in a package, call site in the app): + +| `renamed:` value | Call site before → after pressing Fix | +| --- | --- | +| `"GlassCard"` (on a type) | `FlatCard()` → `GlassCard()` | +| `"modernHelper()"` | `legacyHelper()` → `modernHelper()` | +| `"modernFoo(first:second:)"` | `legacyFoo(1, 2)` → `modernFoo(first: 1, second: 2)` | +| `"Int.doubled(self:_:)"` | `legacyDouble(3, 4)` → `3.doubled(4)`; `self:` marks the new receiver | + +`@available(*, unavailable, renamed:)` carries the same fix-it on an error instead of a warning — for a cutover that must stop compiling. + +Verify the fix-it rather than assuming it: `-diagnostic-style llvm` prints the replacement text under the note's caret, and that text is exactly what the Fix button inserts. Type-check the shim on its own with a target at or above the deprecation version, so the warning fires at all (see the traps below): + +```bash +xcrun swiftc -typecheck -diagnostic-style llvm \ + -target arm64-apple-ios26.0-simulator -sdk "$(xcrun --sdk iphonesimulator --show-sdk-path)" Shim.swift +``` + +### Shape the shim so its removal is a rename + +Whether a Fix button is possible at all is decided by the **form** of the shim, not by the attribute. A shim that is a *value* in a position the new API also occupies — a `ButtonStyle`, `ShapeStyle`, `Transition`, a formatter — differs from its replacement by one identifier, so `renamed:` carries the whole fix. A shim that is a *structure* which disappears — a wrapper view, a duplicated modifier chain — has no name to rename to. Where both forms are open, pick the first: + +```swift +struct CompatProminentButtonStyle: PrimitiveButtonStyle { … } + +extension PrimitiveButtonStyle where Self == CompatProminentButtonStyle { + /// Exists only for the pre-26 floor, and `.glassProminent` sits in exactly this position — so the + /// removal is a one-identifier rename that the Fix button can perform. + @available(iOS, deprecated: 26.0, renamed: "glassProminent") + static var compatProminent: CompatProminentButtonStyle { .init() } +} +``` + +`.buttonStyle(.compatProminent)` → Fix → `.buttonStyle(.glassProminent)`, which compiles. The bare identifier is enough; a type prefix works too — the fix-it's `.Type.member` result is a valid implicit member chain (SE-0287) — but buys nothing. + +### When to leave `renamed:` off + +- **The value must be a declaration name** — "an operator, identifier, or full function name, optionally prefixed by a type name". `renamed: "buttonStyle(.glassProminent)"` is a **compile error**, not a silent no-op. So `ProminentToolbarButton` above, whose fix is *inline the modifier and delete the type*, cannot have a Fix button; there `message:` is the whole answer. +- **The fix-it rewrites the name and the argument labels, never the arity.** A no-argument shim pointed at `renamed: "modernWithArg(_:)"` produces `modernWithArg()` — the Fix button leaves code that does not compile. Add `renamed:` only when the replacement's call shape matches, or when labels and `self:` express the whole difference. + +## Stopping the build at a later floor — `obsoleted:` + +`deprecated:` never does more than warn, and a warning can sit in the build log for as long as nobody reads it. `obsoleted: ` in the same attribute adds a second floor: once the deployment target reaches it, every call site is a compile error. + +```swift +@available(iOS, deprecated: 26.0, obsoleted: 27.0, renamed: "GlassCard") +struct FlatCard: View { … } +``` + +| Deployment target | A `FlatCard()` call site | +| --- | --- | +| below 26.0 | compiles silently | +| from 26.0, below 27.0 | `warning: 'FlatCard' was deprecated in iOS 26.0: renamed to 'GlassCard'` | +| 27.0 and later | `error: 'FlatCard' has been renamed to 'GlassCard'`, plus `note: 'FlatCard' was obsoleted in iOS 27.0` | + +`renamed:` keeps its fix-it on the error, and a `message:` is appended to it (`error: 'X' is unavailable in iOS: `). The declaration itself still compiles; only its uses fail. + +The price: raising the floor to the obsoleted version breaks the build until every such shim is gone, in that same change. Add `obsoleted:` when a shim has to be gone by a known floor, and leave it off where a leftover shim is merely untidy. + +To verify it, raise the floor to the obsoleted version as in the traps below and grep for `was obsoleted in iOS`. + +## What not to use instead + +| Instead of | Why it fails | +| --- | --- | +| `@available(*, deprecated, message:)` | Unconditional — warns immediately, on every platform, today. | +| `#warning("remove when iOS 26")` | Fires on every build from now on; becomes noise and gets ignored. | +| A `TODO:` comment | Nothing enforces it. This is exactly the case the compiler can enforce for you. | + +## Verifying it — two traps + +The annotation is worthless if it never fires, so verify it once instead of assuming. + +**Trap 1: `swift build` compiles for the host, i.e. macOS.** An `@available(iOS, …)` never fires there. Verify with a real iOS build: + +```bash +xcodebuild -scheme -sdk iphonesimulator -destination 'generic/platform=iOS Simulator' build +``` + +**Trap 2: it is supposed to be silent right now.** A deprecation warns only once the deployment target the file is compiled with reaches the version — for a SwiftPM package that is its own `platforms:` floor. So "no warning" on the current floor is the correct result, not a failed setup. + +Prove both halves by temporarily raising the floor: + +```bash +cp Package.swift /tmp/Package.swift.bak +sed -i '' 's/\.iOS(\.v18),/.iOS(.v26),/' Package.swift +xcodebuild -scheme -sdk iphonesimulator -destination 'generic/platform=iOS Simulator' build 2>&1 \ + | grep "was deprecated in iOS" +cp /tmp/Package.swift.bak Package.swift && rm /tmp/Package.swift.bak +``` + +Expected: the grep prints the warning with your message, and the floor is back where it was. + +## Where else this applies + +The same annotation fits any deliberate, version-bound workaround — a back-ported formatter, a `UIViewRepresentable` wrapping a control SwiftUI gained later, a duplicated modifier chain behind `if #available`. If you can name the OS version that makes the code unnecessary, annotate it. + +It does **not** fit code that is merely old or unloved: deprecation is a promise that a specific version removes the need. Without that version, write the fix or a tracked `TODO:` instead. diff --git a/docs/coding-standards/swift-testing.md b/docs/coding-standards/swift-testing.md new file mode 100644 index 0000000..408c88a --- /dev/null +++ b/docs/coding-standards/swift-testing.md @@ -0,0 +1,146 @@ + +# Swift Testing + +Rules for any work with `import Testing` — writing, reviewing, or migrating from XCTest. Test rules for every language (behavior through the public API, determinism, isolation of window-opening suites) are in `general.md`. + +## Swift Testing + +1. **Invoke `ext:swift-testing-pro` first.** It carries the full, up-to-date Swift Testing ruleset (structs over classes, `#expect`/`#require`, async tests, traits, migration); load whichever of its reference files fit the request. Its review process and output format are the default here. +2. **Layer custom-assertion judgment** — whenever the work involves a shared, reusable, or multi-part assertion / verification helper (or the user asks about one), apply *Custom assertions* below: decide whether a helper is even warranted, pick `#expect` vs `#require` vs `Issue.record`, and thread `sourceLocation` so failures report at the call site. +3. **Run the detector when reviewing existing tests** — `scripts/find-assertion-helpers.sh` in the `jk:builder` skill, over the test sources, flags helpers missing a `sourceLocation:` parameter. Treat hits as leads to confirm, then fix per Pattern 1/2 below. +4. **Report** in the base skill's output format (findings by file, rule named, brief before/after), folding the custom-assertion findings in alongside. + +Nothing here contradicts `ext:swift-testing-pro`. Where it already covers a point (e.g. the `sourceLocation` passthrough for verification methods), defer to it rather than restating it. + +## Custom assertions + +Reusable assertion helpers — `expectValidEmail(_:)`, `requireUser(from:)`, `verifyDivision(_:…)` — that wrap one or more `#expect` / `#require` / `Issue.record` calls so a multi-part or complex check reads as a single line at the call site. + +Source: Jon Reid, *"Custom Test Assertions in Swift Testing"* (qualitycoding.org). All snippets below were compile- and behaviour-verified on Swift 6.3.3 (macOS 26 target) against the `Testing` module. + +### 1. Extract sparingly — the bar is higher than in XCTest + +Custom assertions matter **less** in Swift Testing than they did in XCTest, because `#expect` already prints the operands of a failed comparison (`(user.name → "Bob") == "Taylor"`). You rarely need a helper just to get a readable message. Extract one only when it buys something `#expect` can't: + +- **Multi-part validation** — several checks that always travel together (a valid email needs an `@` *and* no spaces *and* a domain). +- **Navigating complex objects** — digging into JSON / nested models where `==` on the whole value isn't meaningful and you assert on extracted fields. + +If a single `#expect(a == b)` says it clearly, don't wrap it. A helper that hides one comparison adds a stack frame and a name to learn for no diagnostic gain. + +### 2. The call-site rule (and how it relates to `ext:swift-testing-pro`) + +Any helper that contains `#expect` / `#require` / `Issue.record` **must** take a `sourceLocation` parameter defaulting to `#_sourceLocation` and pass it to every assertion inside — otherwise a failure is reported at the line *inside the helper*, and you fire up the debugger to find which test actually failed. + +`ext:swift-testing-pro` (`references/writing-better-tests.md` → *verification methods*) already teaches this passthrough mechanic. Don't re-derive it; this section adds the *decision* layer (§1), the `Issue.record`-in-`if` diagnostic pattern (§3 Pattern 2), the value-returning and async variants (Patterns 3–4), and the detector script (§4). + +> `#_sourceLocation` keeps the leading underscore — that is the current spelling, not a typo. + +### 3. The patterns + +#### ❌ Anti-pattern — no `sourceLocation` + +Failures land inside the helper, not at the test that called it: + +```swift +func expectValidEmail(_ email: String) { + #expect(email.contains("@"), "Missing @ in \"\(email)\"") + #expect(!email.contains(" "), "Should not contain spaces: \"\(email)\"") +} +``` + +#### ✅ Pattern 1 — thread `#_sourceLocation` through every assertion + +```swift +func expectValidEmail( + _ email: String, + sourceLocation: SourceLocation = #_sourceLocation +) { + #expect( + email.contains("@"), + "Missing @ in \"\(email)\"", + sourceLocation: sourceLocation + ) + #expect( + !email.contains(" "), + "Should not contain spaces: \"\(email)\"", + sourceLocation: sourceLocation + ) +} +``` + +`Testing.SourceLocation` is the same type, fully qualified — useful when `SourceLocation` is ambiguous in scope. The message is the optional `Comment` (second positional arg); a string literal is enough. + +#### ✅ Pattern 2 — `Issue.record()` inside an `if`, for clean conditional checks + +When the check is conditional or multi-step, an expanded `#expect(complexBool)` produces noisy diagnostics. Decide in plain Swift and record a tidy message: + +```swift +func expectValidEmail( + _ email: String, + sourceLocation: SourceLocation = #_sourceLocation +) { + if !email.contains("@") { + Issue.record( + "Missing @ in \"\(email)\"", + sourceLocation: sourceLocation + ) + } +} +``` + +`Issue.record(_:sourceLocation:)` is also the primitive for failures that aren't a single boolean — it's the Swift Testing replacement for `XCTFail`. + +#### ✅ Pattern 3 — `#require`-style helper that unwraps and returns + +Use `#require` (not `#expect`) when later lines depend on the value: it throws and stops the test on failure. Make the helper `throws` and return the unwrapped value: + +```swift +func requireUser( + fromJSON json: [String: Any], + sourceLocation: SourceLocation = #_sourceLocation +) throws -> User { + let name = try #require( + json["name"] as? String, + "Expected String 'name' in \(json)", + sourceLocation: sourceLocation + ) + let email = try #require( + json["email"] as? String, + "Expected String 'email' in \(json)", + sourceLocation: sourceLocation + ) + return User(name: name, email: email) +} +``` + +Call it with `let user = try requireUser(fromJSON: payload)`. + +#### ✅ Pattern 4 — async helper + +Mark the helper `async`, `await` inside, still thread `sourceLocation`: + +```swift +func expectUserCount( + _ expected: Int, + in store: UserStore, + sourceLocation: SourceLocation = #_sourceLocation +) async { + let actual = await store.count + #expect(actual == expected, sourceLocation: sourceLocation) +} +``` + +### 4. Detecting helpers that forgot `sourceLocation` + +`scripts/find-assertion-helpers.sh [path …]`, which ships with the `jk:builder` skill (`~/.claude/skills/jk/skills/builder/scripts/`), flags every non-`@Test` function that calls `#expect` / `#require` / `Issue.record` without a `sourceLocation` parameter. It's a heuristic lead-finder (naive about braces inside string literals), so confirm each hit — but it's the fast first pass on a review: + +``` +$ scripts/find-assertion-helpers.sh Tests/ +Tests/EmailTests.swift:42: helper 'expectValidEmail' uses #expect/#require/Issue.record but has no `sourceLocation:` parameter — failures will point inside the helper, not the call site +``` + +Exit code is 1 when any lead is found, 0 when clean — usable as a CI gate. + +### 5. Naming + +The article uses `assert…`; `ext:swift-testing-pro` uses `verify…`; the Swift Testing idiom is to mirror the underlying macro — `expect…` for non-throwing checks, `require…` for throwing helpers that unwrap/return. Lead with `expect…` / `require…` because the prefix tells the reader at the call site whether the helper can throw and stop the test. Whichever you pick, match the surrounding test target rather than mixing conventions. diff --git a/docs/coding-standards/swift.md b/docs/coding-standards/swift.md new file mode 100644 index 0000000..bf92a5c --- /dev/null +++ b/docs/coding-standards/swift.md @@ -0,0 +1,191 @@ + +# Swift and SwiftUI + +**Clarity > Brevity > Consistency.** Code compiles without warnings. Comment rules live in `general.md`; doc comments belong to the `jk:swift-docc` skill. + +## Swift / SwiftUI + +- Doc comments: use the `jk:swift-docc` skill (it ships the rules plus a lint script). What earns a `///` is **architectural relevance, not the access level** — domain models and the protocols that serve as the contract between components get one; a declaration does not earn one by being `public`, and an `internal` cross-component seam still deserves one. +- Concurrency and SwiftUI specifics: `jk:swift-concurrency` and the SwiftUI skills; tests: `swift-testing.md`. +- **macOS UI: `macos-design.md`** — layout metrics, window chrome, and Mac interaction. Mac layout is **not** an 8-point grid (`20` margins, `6` between controls, `12` between groups), and iOS reflexes — hamburger menu, bottom tab bar, touch-sized controls, sheet-per-action — are the visible tell of a port. +- **An element that appears and disappears keeps its space — `.opacity(…)`, not a conditional `if` that removes it from the hierarchy.** An unread indicator in a list row, a `ProgressView` during a load, a validation hint: removing the view makes the surrounding layout jump the moment it comes back. Reserve the space by default; reflow it only when the developer asked for the jump explicitly. +- Always add `#Preview` macros for views. Prefer type inference for simple properties. Never write `internal` explicitly. TCA: keep Feature and View in the same file. +- **A workaround that only exists because the deployment target is too low gets `@available(, deprecated: , message: …)`**, so raising the floor turns it into a compiler warning instead of silent debt — plus `renamed:` where the fix is a plain rename, which gives Xcode a **Fix** button, and `obsoleted: ` where a later floor has to turn the warning into an error: `swift-deprecations.md`. +- Never use `if`/`switch` expressions (SE-0380) — `let x = if p { … } else { … }` and `let y: Float = switch value { … }` are forbidden; use the statement form (examples under *No `if`/`switch` expressions* below). + +## Naming + +- `UpperCamelCase` for types and protocols, `lowerCamelCase` for everything else. +- Clear at the call site — read the declaration as it will be *used*. +- No abbreviations except the universal ones (URL, ID, UUID). Spell out `cfg`, `mgr`, `ctx`, `desc` as `configuration`, `manager`, `context`, `description`. +- Avoid `Manager`, `Handler`, `Helper`, `Coordinator` — they hide the responsibility. Prefer `PaymentProcessor`, `EventDispatcher`, `ImageCache`. + +```swift +let maximumWidgetCount = 100 +func fetchUser(byID id: String) -> User +``` + +## Golden path + +The left-hand margin is the happy path. Guard and return early instead of nesting `if`s — deep nesting is what makes a function hard to follow. + +```swift +func process(value: Int?) throws -> Result { + guard let value else { throw ProcessError.nilValue } + guard value > 0 else { throw ProcessError.invalidValue } + return compute(value) +} +``` + +## No `if`/`switch` expressions (SE-0380) + +Never use `if` or `switch` as an expression ([SE-0380](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0380-if-switch-expressions.md)) — not to initialize a `let`/`var`, not as a single-expression function or closure body. Use the statement form that assigns in each branch, or extract a function. + +```swift +// FORBIDDEN — if/switch expressions +let x = if p { + [1] +} else { + [1].map(expensiveOperation) +} + +let y: Float = switch value { +case 0..<0x80: 1 +case 0x80..<0x0800: 2.0 +default: 4.5 +} + +// Correct — statement form, each branch assigns +let x: [Int] +if p { + x = [1] +} else { + x = [1].map(expensiveOperation) +} + +let y: Float +switch value { +case 0..<0x80: y = 1 +case 0x80..<0x0800: y = 2.0 +default: y = 4.5 +} +``` + +## Access control + +- `private` over `fileprivate`. +- Never write `internal` — it is the default, and stating it adds noise. +- Access control is the leading specifier on a declaration. + +## `self` + +Omit `self` unless the compiler requires it, and be consistent — mixing the two styles obscures capture semantics at a glance. + +```swift +func configure() { + backgroundColor = .systemBackground +} +``` + +## Type inference & sugar + +Let the compiler infer when the type is obvious. Empty collections need the annotation. + +```swift +var names: [String] = [] +var cache: [String: Int] +var name: String? +``` + +## Computed properties + +Omit `get` for read-only: + +```swift +var diameter: Double { + radius * 2 +} +``` + +## Closures + +Trailing-closure syntax only when there is a single closure parameter. In escaping closures: + +```swift +resource.request().onComplete { [weak self] response in + guard let self else { return } + self.updateModel(response) +} +``` + +## Constants + +Once a value has earned a name (see *Simplicity* in `general.md` — one call site means no constant), namespace it with a case-less enum, which cannot be instantiated by accident: + +```swift +enum Math { + static let pi = 3.14159 +} +``` + +## Organization + +Group conformances into extensions with `MARK:` headers: + +```swift +// MARK: - UITableViewDataSource +extension MyViewController: UITableViewDataSource { } +``` + +Sparingly — see *Comments* in `general.md`: a file needing many MARKs to stay navigable is too large. + +Braces open on the same line, one blank line between methods, no space before a colon and one after. + +## Typography (SwiftUI) + +Prefer semantic text styles so type scales with Dynamic Type: `.font(.footnote.weight(.medium))` instead of `.font(.system(size: 12, weight: .medium))`. Fall back to `.system(size:)` only when a design genuinely cannot be expressed semantically. See [Apple HIG — Typography](https://developer.apple.com/design/human-interface-guidelines/typography). + +## Layout (SwiftUI) + +A view is sized **from the inside out**: the parent proposes a size, the child determines its own, and the child's answer is final — *"the parent cannot alter this size unilaterally"* (*Thinking in SwiftUI*, ch. Layout). Grow a view with `.padding`, which adds its insets to whatever the content reported. Avoid `.frame(width:height:)`: it proposes a fixed size and reports it *independent of the subview*, which hard-codes a magic number Dynamic Type cannot move — the same book calls fixed frames in production code "actually quite rare". + +Watch for `.resizable()`. It strips an `Image` of its reported size so the view accepts any proposal, and from then on a fixed frame is the only thing that *can* size it. That is a self-inflicted problem, not a justification for the frame. + +```swift +// FORBIDDEN — the glyph has no size of its own, so a magic number has to supply it +Image(systemName: "paperplane.fill") + .resizable() + .scaledToFit() + .frame(width: 32, height: 32) + +// Correct — the font reports the size, the padding grows it, both follow Dynamic Type +Image(systemName: "paperplane.fill") + .font(.title3) + .padding(6) +``` + +Two sizes that must match are expressed structurally rather than computed: give both the same content treatment and add the difference as padding, so the equality survives every text size. Fixed frames stay legitimate for design elements that genuinely cannot or should not scale. + +## API availability & deprecation + +When a newer OS version offers an API that replaces a legacy fallback, isolate the fallback behind its own helper so the **compiler** tells you when it is safe to delete ([Swift with Majid](https://swiftwithmajid.com/2023/05/17/api-availability-in-swift/)): + +```swift +extension View { + @ViewBuilder + func scrollBounceDisabled() -> some View { + if #available(iOS 16.4, *) { + self.scrollBounceBehavior(.basedOnSize) + } else { + legacyScrollBounceDisabled() + } + } + + @available(iOS, deprecated: 16.4, message: "Use scrollBounceBehavior(.basedOnSize) directly.") + private func legacyScrollBounceDisabled() -> some View { + // legacy implementation + } +} +``` + +The deprecation version is the one that introduced the replacement: once the deployment target reaches it, every call site warns and the helper is provably dead code. The `message:` names the replacement so removal is mechanical. A `renamed:` would additionally give Xcode a **Fix** button, but only where the replacement's call shape matches — `scrollBounceBehavior(_:)` takes an argument the shim does not, so the button would insert non-compiling code (`swift-deprecations.md`). Never inline an indefinite legacy branch — cleanup should be compiler-driven, not memory-driven.