code-quality

# Code Quality Standards Essential reminders for writing consistent, maintainable code in this project. --- ## Mandatory Requirements These are NON-NEGOTIABLE. A feature missing any of these is INCOMPLETE. ### 0. Comments: code is for code, the PR is for explainers Default to zero. Before keeping any comment, rewrite it as a noun phrase. If it survives, keep it; if it cannot, delete it. - Survives: `JVM heap budget: 11 GB total, 16 GB CI runner.` - Cannot: `The Kotlin daemon carries the compile, so it takes the larger share.` A comment states what a declaration IS. The moment it explains why a choice was made, it belongs in the commit message and the PR description, which `git blame` retrieves from the line anyway. Any of these words means the sentence is arguing, so delete it: because, so, since, which is why, rather than, instead of, not just, to avoid, otherwise, this ensures, deliberately, on purpose, note that, we, our. Also delete any number or comparison that came out of a debugging session. "17x", "was 2h40m", "under X it did Y" is history, not contract. Zero inline `//` inside function bodies. No single-line `/* */` or `/** */` anywhere, including declaration-level KDoc: the one-line block form is a rephrased `//`. A genuinely needed KDoc uses the multi-line block form above a non-obvious PUBLIC contract. Never on a private declaration; rename it instead. KDoc states the contract. It never narrates what the body does, argues for a design, or carries history. Two backstops exist. For Claude sessions, `.claude/hooks/smell-guard.py` checks the word list after each edit is written. For every contributor, the blocking `rules` job in `.github/workflows/build.yml` runs `scripts/ci/agentic-smells.py` against `scripts/ci/smell-rules.json`, whose `inline-comment` and `disguised-block-comment` rules flag added lines on a pull request. Passing either is not the goal; not needing them is. ### 1. Input Handling (TV + Touch) All interactive UI components MUST have: **Touch Support:** - Use `Modifier.clickableNoFocus { }` (from `ui/util/Modifiers.kt`) - NEVER use plain `Modifier.clickable()` - it enables Compose TV focus **Controller Support:** - `InputHandler` implementation for D-pad navigation - Proper index wrapping with `.mod(size)` (NOT `% size`) - Visual focus state via `isFocused: Boolean` prop **Focus Management:** - NO Compose focus for navigation or selection. `focusable()` appears once per rendered surface, as that surface's root key sink: `ArgosyApp` for the control surface, and the presentation branch of both `MainActivity` and `SecondaryHomeActivity`. Each activity renders either `ArgosyApp` or `PresentationSlotContent`, so each needs a sink on the presentation side. A surface rendered without one leaves its window with nothing focusable and Android ANRs the activity with "does not have a focused window". The other legitimate exception is `FocusRequester` for soft-keyboard text entry (~20 `ui/` files); any of these becomes a violation the moment focus decides what is SELECTED rather than what is TYPED INTO. - Focus index stored in ViewModel - Manual focus visuals via `FocusIndicators` (`ui/primitives/Focus.kt`): fill/halo/stripe/ring/lift; never movement or scale except lift, which is reserved for cover tiles (`FocusIndicators.Tile` is the only preset that sets `lift`) A component that only handles ONE input modality is INCOMPLETE. Pattern: Check existing components for `InputHandler` + `clickableNoFocus` pairing. **Modal input capture (learned the hard way, 2026-07-06):** a modal/dialog/overlay that RENDERS without taking the input stack is INCOMPLETE - gamepad input drives the screen behind it (dual-focus ghost, drawer-over-dialog). Every modal must use exactly one of: (A) `ModalInputEffect`/`pushModal` (self-contained; `ArgosyConfirmModalHost` does this for you - pushes IMMEDIATELY on open, never deferred to a lifecycle event), (B) the host handler guarding the modal's visibility state in EVERY nav method (up/down/left/right/ confirm/back AND prev/next section/trigger - partial guards leak the unguarded directions), or (C) a handler the host stores and delegates to. `Modal`/`CenteredModal`/ `ModalScaffold` are pure visuals - they capture NOTHING by themselves. Mechanism-B guards MUST return `InputResult.HANDLED`, never UNHANDLED: the app-level fallback treats an unhandled result as "no modal here" and runs global actions (LEFT opens the drawer, Menu toggles it) straight over the modal. A guard that returns UNHANDLED is the bug, not a guard. ### 2. Footer Hints (V2: the control is the guide) Authority: `design-handoff/CONTROL-FOUNDATIONS.md`. The bar surfaces what is NOT obvious from the focused control; the inline affordance carries the interaction. - A / B / d-pad / Back alone never justify a bar - if they are the only candidates, no bar. - Hints that earn the bar: screen-global verbs (search, filter), shoulder paging (LB/RB), non-obvious bound buttons (X/Y, triggers). - When space is limited, shed OBVIOUS guides first (d-pad, then A/B); never drop a non-obvious hint to keep an obvious one. `FooterHint.hidePriority()` implements this order (X/Y highest keep priority, A/B low, d-pad lowest). - A/Confirm means enter/commit/toggle, NEVER adjust; sliders/steppers/enums adjust on Left/Right only. ### 3. Lazy List Scrolling A scrolling collection of repeated items uses LazyColumn, LazyRow or LazyVerticalGrid. Column and Row stay the tools for fixed layout. - Lazy lists compose only visible items and keep scroll state for gamepad navigation - A repeated, scrolling collection gets a lazy list even when it is small today --- ### User-Facing Text Every string a user reads is a resource id. Raw English in a `text =`, `title =`, `subtitle =`, `label =`, `message =` or `contentDescription =` argument is a defect, and `scripts/ci/smell-rules.json` fails the build on one (AS-7). - Add the key to the `res/values/strings_<area>.xml` that owns the screen, named `<area>_<component>_<role>` for the usage site. - **Identical English at two usage sites gets two keys.** Never deduplicate. Seven `LibretroCoreRegistry` display names are byte-identical to save-tree folder names in `EmulatorRegistry.retroArchSaveDirByCore`; fusing them breaks save-path resolution for every user who already has saves on a server. - **A string is a label or a token, never both** (AS-8). Anything compared, stored in DataStore, used as a map key, an `options.indexOf(...)` entry, a route or a path component stays a literal. `indexOf` misses tend to `coerceAtLeast(0)` rather than throw, so getting this wrong resets a user's setting with nothing logged. - **Labels for `data/` and `domain/` types live in `ui/common/`** as extension properties, following `labelRes` in `ui/common/CompletionStatusUi.kt`. `R` never crosses inward. That file is the label pattern only. Its `color` property still holds raw `Color(0xFF...)` literals; completion colors come from `ColorTokens.Domain.Completion`. - **Carriers are `@StringRes Int`, never `(Context) -> String`.** A lambda is not stability-safe under `compose_stability_config.conf`, and ViewModels outlive the activity recreation a locale change triggers. - **Never put a gamepad button letter inside a string.** A/B are user-swappable; render `InputGlyph` beside the text. The sole exception is a row whose purpose is to change that very mapping. - Never translate upstream identifiers: core ids, libretro/RetroArch option tokens and shader names, core and emulator display names, platform slugs, paths, package names. `libretro/coreoptions/**` stays English by decision. - Use `<plurals>` and `pluralStringResource` instead of `"$n item(s)"`. - Add an XML translator comment above any string whose English is ambiguous out of context. This app splits hard on "Save" (verb on an action, noun in save sync), "State" (save state vs completion status), "Play", "Frame", "Core", "Channel", "Collection". The comment is the only context a translator gets. ## Pre-Implementation Questions Before writing code, answer these questions: 1. **Scope**: What are ALL the places this feature touches? (UI, data, settings, navigation) 2. **States**: What are the error, loading, empty, and success states? 3. **Lifecycle**: What happens on rotation? Backgrounding? Return from other app? 4. **First-run**: Does this behave differently on fresh install vs existing data? 5. **Consistency**: What existing patterns should this follow? (Find and cite file:line examples) If you cannot answer these, investigate before writing code. --- ## Core Principles ### SOLID - **Single Responsibility**: Each class/function does one thing well - **Open/Closed**: Extend behavior without modifying existing code - **Liskov Substitution**: Subtypes must be substitutable for base types - **Interface Segregation**: Small, focused interfaces over large ones - **Dependency Inversion**: Depend on abstractions, not concretions ### DRY (Don't Repeat Yourself) - Extract common logic into shared utilities - Reuse existing components before creating new ones - Check for existing patterns in codebase before implementing ### KISS (Keep It Simple, Stupid) - Prefer simple, readable solutions over clever ones - Avoid premature optimization - Don't add features "just in case" ### YAGNI (You Aren't Gonna Need It) - Only implement what's currently needed - Remove unused code rather than commenting it out - Don't build for hypothetical future requirements ## Project-Specific Patterns ### UI State Management - State flows from ViewModel to Composables via `StateFlow` - Use delegates for section-specific logic (e.g., `DisplaySettingsDelegate`) - Update state via `_uiState.update { it.copy(...) }` - Keep UI state in single `UiState` data class per screen ### StateFlow writes MUST be atomic (lost-update trap, learned 2026-07-22) `_uiState.value = _uiState.value.copy(...)` is a read-modify-write that silently loses concurrent updates. On screens with several collectors (feed, profile, prefs), a field written by one collector gets wiped by another and never re-emits - the avatar-doodle fields vanished on exactly the busy social screens this way while quiet screens worked. Rules: - ALWAYS `_uiState.update { it.copy(...) }` - never assign `.value` from a read of `.value`, even though older code in some files still does. - NEVER write back a pre-suspend snapshot: `val s = _uiState.value` ... suspend call ... `_uiState.value = s.copy(...)` clobbers everything that landed during the suspend. Compute the async value first, then `update {}` with fields from the lambda's current state. - When adding a collector to an existing ViewModel, do not copy its legacy write pattern; use `update {}` and convert wide-window writers you touch. ### Compose Stability Contract (NON-NEGOTIABLE) `app/compose_stability_config.conf` declares four packages stable to the Compose compiler: `data.model.**`, `data.local.entity.**`, `domain.model.**` and `ui.**`. It also declares two single classes, `core.game.AchievementUi` and `hardware.CompanionInGameState`. This is what keeps cold compiles at ~4 min instead of 1h+ (StabilityInferencer recursion), but the compiler NO LONGER VERIFIES stability for covered classes -- we promise it. A violation does not crash or warn; it silently skips recompositions and the UI goes stale. Rules for ALL new/edited code in covered packages and classes: - State/model data classes: `val`-only, including constructor params. Never add `var`. - Collections in state are replaced via `copy(...)`, never mutated in place. - Never read a plain `var`/non-State property of a ViewModel or delegate inside composition. UI-visible data reaches composables ONLY via `StateFlow`/`collectAsState` or params. - A data class that genuinely needs mutable fields must live OUTSIDE `ui/`; a `var` or a `MutableList/Set/Map` in one under `ui/` fails `scripts/ci/stability_checks.py`, in the write-time hook and in CI. - The whole of `ui/` is covered, `ui.primitives` included, so a new state class is stable wherever it is written. Writing one outside `ui/` re-opens the inference recursion that produces hour-long compiles. - If the conf file is absent on the current branch, these rules are still the house style; they just aren't load-bearing yet. ### Input Handling (TV/Gamepad) - CRITICAL #### InputHandler Interface All controller input goes through `InputHandler` (`ui/input/InputHandler.kt`). This is the actual interface -- every method has a default `UNHANDLED` body, so implementors override only what they handle: ```kotlin interface InputHandler { fun onUp(): InputResult = InputResult.UNHANDLED fun onDown(): InputResult = InputResult.UNHANDLED fun onLeft(): InputResult = InputResult.UNHANDLED fun onRight(): InputResult = InputResult.UNHANDLED fun onConfirm(): InputResult = InputResult.UNHANDLED fun onBack(): InputResult = InputResult.UNHANDLED fun onMenu(): InputResult = InputResult.UNHANDLED fun onSecondaryAction(): InputResult = InputResult.UNHANDLED fun onContextMenu(): InputResult = InputResult.UNHANDLED fun onPrevSection(): InputResult = InputResult.UNHANDLED fun onNextSection(): InputResult = InputResult.UNHANDLED fun onPrevTrigger(): InputResult = InputResult.UNHANDLED fun onNextTrigger(): InputResult = InputResult.UNHANDLED fun onSelect(): InputResult = InputResult.UNHANDLED fun onLeftStickClick(): InputResult = InputResult.UNHANDLED fun onRightStickClick(): InputResult = InputResult.UNHANDLED fun onLongConfirm(): InputResult = InputResult.UNHANDLED } ``` #### Button Names and Swap Resolution `InputButton` (`ui/components/FooterHint.kt`) uses INTENT names, not physical positions: ```kotlin enum class InputButton { A, B, X, Y, DPAD, DPAD_UP, DPAD_DOWN, DPAD_LEFT, DPAD_RIGHT, DPAD_HORIZONTAL, DPAD_VERTICAL, LB, RB, LB_RB, LT, RT, LT_RT, START, SELECT } ``` There are NO position-named values (`SOUTH`/`EAST`/`WEST`/`NORTH` do not exist). `InputButton.A` means "the confirm intent", wherever the user has mapped it. User button swaps are resolved at exactly two points -- never by choosing a different enum value: 1. **Icon render:** `InputButton.toPainter()` reads `LocalABIconsSwapped` / `LocalXYIconsSwapped` / `LocalSwapStartSelect` and picks the physical glyph (e.g. `InputButton.A` draws `FaceRight` when AB is swapped). 2. **Keyevent dispatch:** `mapKeycodeToGamepadEvent(keyCode, swapAB, swapXY, swapStartSelect)` in `ui/input/GamepadInputHandler.kt` routes the physical keycode to the intended `GamepadEvent` (e.g. `KEYCODE_BUTTON_X` -> `SecondaryAction` when XY is swapped). | InputButton | Handler Method | Typical Use | |-------------|----------------|-------------| | `A` | `onConfirm()` | Select/Confirm (never adjust) | | `B` | `onBack()` | Back/Cancel | | `X` | `onContextMenu()` | Full context menu | | `Y` | `onSecondaryAction()` | Quick action (favorite, add) | #### CRITICAL: Never Hardcode Button Names **WRONG:** a string resource whose English names a button, or a `Text` that draws a button letter as a hand-built hint. Both show the wrong button once the user swaps them. **CORRECT:** `FooterBar` with the `InputButton` enum and a resource label. ```kotlin FooterBar( hints = listOf( InputButton.Y to stringResource(R.string.social_friends_hint_favorite) ) ) ``` `FooterBar` takes `hints: List<Pair<InputButton, String>>`; each hint's icon resolves the swap automatically via `toPainter()`. Labels come from `stringResource` with keys named `<area>_<component>_<role>`. #### Remove Compose Focus System TV UI does NOT use Compose's built-in focus. Always use `clickableNoFocus`: ```kotlin import com.nendo.argosy.ui.util.clickableNoFocus Modifier.clickableNoFocus { onItemClick(index) } Modifier.clickableNoFocus(enabled = isEnabled) { onItemClick(index) } ``` Plain `Modifier.clickable { }` is the wrong form. It enables TV focus and breaks gamepad navigation. **Why `clickableNoFocus`?** Compose's `clickable()` enables TV focus by default, which conflicts with this app's manual InputHandler-based focus system. Using `clickable()` directly causes: - Double focus visuals (Compose ring + manual glow/border) - Unpredictable navigation when Compose and InputHandler fight for control - Broken gamepad input when Compose steals focus The `clickableNoFocus` extension (defined in `ui/util/Modifiers.kt`) disables Compose focus while preserving touch support. **Focus Management:** - Use `isFocused: Boolean` prop for visual focus state - Manage focus index in ViewModel, not Compose focus system - `Modifier.focusable()` belongs only to a surface's root key sink, one per rendered surface (`ArgosyApp` for control; the presentation branch of `MainActivity` and `SecondaryHomeActivity`). Never add one to a row, tile or control - `FocusRequester` is allowed ONLY for soft-keyboard text entry and those root key sinks. Using it to move selection between rows is the violation **Material3 Components with Built-in Focus:** Some Material3 components have built-in TV focus. Disable it: ```kotlin Switch(