Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions skills/jetpack-compose-audit/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,8 @@ Stability deductions from step 5 are measured evidence and should be weighted no

Use the scoring rubric in `references/scoring.md` and the heuristics in `references/search-playbook.md`.

> **Navigation 3:** if the repo uses `rememberNavBackStack`, `NavDisplay`, or `NavKey`, run **playbook section 2b** (Nav3 Detection & Audit) before scoring. Nav3 findings map to State Management and Side Effects — see the scoring note in section 2b.

#### Performance

Focus on:
Expand Down
6 changes: 5 additions & 1 deletion skills/jetpack-compose-audit/references/canonical-sources.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,12 @@ This grounds:
`https://developer.android.com/develop/ui/compose/custom-modifiers`
- Android Developers: `Locally scoped data with CompositionLocal`
`https://developer.android.com/develop/ui/compose/compositionlocal`
- Android Developers: `Navigation with Compose`
- Android Developers: `Navigation with Compose` (Nav2)
`https://developer.android.com/develop/ui/compose/navigation`
- Android Developers: `Navigation 3`
`https://developer.android.com/guide/navigation/navigation-3`
- nav3-recipes (official sample repo, verified API shapes)
`https://github.com/android/nav3-recipes`

These ground:

Expand Down
20 changes: 20 additions & 0 deletions skills/jetpack-compose-audit/references/scoring.md
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,16 @@ Deduct for:

When any of these paging rules affects the State management score, name it in the State section as **Paging load-state handling** rather than folding it into a generic state note.

**Navigation 3 deductions (State management)**

When Nav3 is detected (see playbook section 2b), also deduct for:
- `entryDecorators` list supplied without re-adding `rememberSaveableStateHolderNavEntryDecorator()` — `rememberSaveable` inside entries silently stops persisting state → [Nav3 save state](https://developer.android.com/guide/navigation/navigation-3/save-state)
- Anonymous or non-top-level `NavKey` destination types — breaks process-death restore and `rememberSaveable` for that entry → [Nav3](https://developer.android.com/guide/navigation/navigation-3)
- Feature/screen ViewModel owns or mutates the back stack directly — navigation lifecycle coupled to ViewModel, defeats predictive back and scene strategies → [Nav3](https://developer.android.com/guide/navigation/navigation-3)
- `ResultEventBus` / `conflateAsState` result treated as surviving process death — result is only available within the current navigation lifetime; persist in `SavedStateHandle` if survival is required → [Nav3 save state](https://developer.android.com/guide/navigation/navigation-3/save-state)

Name Nav3 state findings as **Nav3 backstack state** in the State section.

Suggested interpretation:

- `9-10`: strong UDF, clear ownership, minimal ambiguity
Expand Down Expand Up @@ -271,6 +281,16 @@ Deduct for:

When this paging rule affects the Side Effects score, name it in the Side Effects section as **Paging side-effect signals** rather than folding it into a generic effect note — mirroring **Paging list correctness** (Performance) and **Paging load-state handling** (State).

**Navigation 3 deductions (Side Effects)**

When Nav3 is detected (see playbook section 2b), also deduct for:
- `backStack.add` / `backStack.removeLastOrNull` called directly in the composition body instead of inside an event handler or `LaunchedEffect` — navigation fires on every recomposition → [Nav3](https://developer.android.com/guide/navigation/navigation-3)
- Tap handler missing `dropUnlessResumed { backStack.add(...) }` — a queued tap can navigate from a screen already in exit animation, duplicating the back-stack entry → [Nav3](https://developer.android.com/guide/navigation/navigation-3)

Note: "feature ViewModel owns/mutates the back stack" is a **State Management** finding (see above), not Side Effects — do not score it here.

Name Nav3 side-effect findings as **Nav3 navigation side effects** in the Side Effects section.

Suggested interpretation:

- `9-10`: deliberate, lifecycle-aware effect usage
Expand Down
180 changes: 180 additions & 0 deletions skills/jetpack-compose-audit/references/search-playbook.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,186 @@ Report format:

Do not subtract from Performance, State, Side Effects, or Composable API Quality. It may still belong in `Critical Findings` or `Prioritized Fixes` because it is user-visible and usually cheap to fix.

## 2b. Navigation 3 Detection & Audit

Run this section **only when Nav3 is present**. Nav3 ships a different model from Nav2 — many wrong patterns compile fine but break backstack restore, process death recovery, or recomposition safety at runtime. (Note: string routes and typed-key mismatches are often compile errors in Nav3; the runtime risks are primarily state/save-state related.)

### Detect Nav3 Usage

```bash
rg -l 'rememberNavBackStack|NavDisplay|entryProvider|NavKey|androidx\.navigation3' -g '*.kt' -g '*.kts'
```

If any file matches, Nav3 is in scope. Continue below. If none match, skip this section.

### Map Nav3 Entry Points

```bash
# Back stack instantiation
rg 'rememberNavBackStack' -g '*.kt' -n

# NavDisplay host(s)
rg 'NavDisplay\s*\(' -g '*.kt' -n

# Destination key types (multiline: @Serializable often on separate line from : NavKey)
rg -B1 ': NavKey' -g '*.kt' -n

# Custom entry decorators
rg 'entryDecorators\s*=' -g '*.kt' -n

# Result bus usage
rg 'rememberResultEventBusNavEntryDecorator|LocalResultEventBus|ResultEffect|conflateAsState' -g '*.kt' -n
```

### Red Flags To Verify

**Backstack ownership violation — feature ViewModel holds or mutates the back stack**

```bash
rg 'NavBackStack|backStack' -g '*.kt' -l | xargs -r rg -l 'ViewModel|viewModel\(\)' 2>/dev/null
```

A feature/screen ViewModel must never own or directly mutate the back stack. Navigation signals should flow from the ViewModel as state/events that the *route* observes and acts on via `backStack.add` / `removeLastOrNull`. A dedicated app-level nav holder that *owns* the stack is a legitimate pattern; a feature ViewModel that receives `backStack` as a parameter for navigation is not.

**High false-positive rate:** any file with both `backStack` and `viewModel()` matches — including `App.kt` where the nav holder and ViewModel co-exist legitimately. Always read the hit to confirm the ViewModel actually holds or mutates the stack, not merely that both symbols appear in the same file.

Severity: **Blocker** — this couples navigation lifecycle to the ViewModel and defeats predictive back and scene strategies.

---

**`entryDecorators` supplied without re-adding `rememberSaveableStateHolderNavEntryDecorator`**

```bash
rg 'entryDecorators\s*=' -g '*.kt' -n -A 6
```

For each hit: check whether the decorator list includes `rememberSaveableStateHolderNavEntryDecorator()`. Supplying a custom list *replaces all defaults*, so `rememberSaveable` inside entries silently stops working if the default saveable-state decorator is omitted.

Severity: **Blocker** — `rememberSaveable` inside affected entries persists nothing; crash or data loss on config change.

---

**Navigation triggered in composition body (not from event handler or `LaunchedEffect`)**

```bash
rg 'backStack\.(add|removeLastOrNull|clear)\b' -g '*.kt' -n
```

For each hit: confirm it is inside a lambda passed as an event handler (e.g. `onClick`, `onBack`, `onOpenProfile`) or inside a `LaunchedEffect`. A call directly in the composition body triggers navigation on every recomposition.

Severity: **Blocker** — navigation fires on every recomposition, duplicates back-stack entries.

---

**Missing `dropUnlessResumed` click guard**

```bash
# -U enables multiline matching so backStack.add can be on a different line than onClick
rg -U 'onClick\s*=\s*\{[^}]*backStack\.add' -g '*.kt' -n
rg -U 'on\w+\s*=\s*\{[^}]*backStack\.add' -g '*.kt' -n
# Simpler alternative: find all backStack.add call sites and read surrounding context
rg 'backStack\.add\(' -g '*.kt' -n -B3
```

For each navigation-triggering tap handler: check it is wrapped in `dropUnlessResumed { backStack.add(...) }`. Regex matches are hints — multiline lambdas and nested braces require reading the actual code around each hit. Without this guard, a queued tap can navigate from a screen that has already left `RESUMED`, pushing a duplicate entry during the exit animation.

Severity: **Should-fix** — rare in practice but reproducible with fast double-taps and on slow devices.

---

**Anonymous or non-top-level destination keys**

```bash
# Catch anonymous objects (object expression, not object declaration)
rg 'object\s*:\s*NavKey' -g '*.kt' -n
# Catch NavKey usages that are not top-level data class/object or sealed interface/class base types
rg ': NavKey' -g '*.kt' -n | grep -Ev '(data (class|object)|sealed (class|interface)|^.*object\s+\w+\s*:)'
```

All `NavKey` **destination** types must be top-level `@Serializable` data classes or objects. Anonymous inline keys (object expressions) and local classes break `rememberSaveable` and cannot be restored after process death.

Valid patterns that must **not** be flagged: `sealed interface AppNavKey : NavKey`, `object HomeKey : NavKey` (named top-level object), base sealed hierarchies. Read each hit before filing — sealed/interface base types are always valid.

Severity: **Blocker** for anonymous objects and local classes — process-death restore fails; `rememberSaveable` inside that entry silently stops working.

---

**Nav2 `NavController` / `NavHost` used alongside Nav3 code**

```bash
rg 'rememberNavController|NavHost\s*\(' -g '*.kt' -n
```

Check whether any hit coexists in the same navigation graph with `NavDisplay`. Mixing Nav2 and Nav3 in the same back-stack flow is not supported; they must own separate, non-overlapping regions of the UI.

Severity: **Blocker** if mixed in the same flow; **Nit** if clearly isolated sub-graphs.

---

**String routes still used in Nav3 code**

```bash
# Scope to Nav3 files only (from the detection query above) to avoid flagging Nav2 sub-graphs
rg 'backStack\.add\s*\(\s*"' -g '*.kt' -n
# navigate("…") is a Nav2 API; flag only if found in a file that also uses NavDisplay/rememberNavBackStack
rg 'navigate\s*\(\s*"' -g '*.kt' -l | xargs -r rg -l 'NavDisplay|rememberNavBackStack' 2>/dev/null
```

Nav3 destinations are `@Serializable` typed keys — not strings. `backStack.add("…")` won't compile with `NavBackStack<NavKey>`, so a hit almost always indicates dead/migrated code or a type mismatch. `navigate("…")` in a file that also uses `NavDisplay` signals an incomplete Nav2→Nav3 migration.

Severity: **Should-fix** (migration smell / likely dead code) rather than Blocker — Nav3's type system makes the string-route pattern a compile error in practice.

---

**`@Composable` or non-serializable type inside a NavKey destination**

```bash
# Find NavKey destination types first
rg -l ': NavKey' -g '*.kt' | xargs -r rg -n '@Composable|\(\s*\)\s*->\s*\w+|\([^)]+\)\s*->\s*\w+|suspend\s*\(' 2>/dev/null
```

Destination data classes must be serializable plain data. Composable references, lambdas (`() -> Unit`, `(String) -> Unit`, suspend lambdas), or any non-serializable type as a field will crash at serialization time. Scope the search to NavKey files to avoid noise from non-navigation data classes.

Severity: **Blocker** — runtime crash on process death or `rememberSaveable`.

---

**`ResultEventBus` result assumed to survive process death**

```bash
rg 'conflateAsState|ResultEffect' -g '*.kt' -n
```

`conflateAsState` delivers the latest result within the current navigation lifetime — it is **not** persisted across process death. If a result must survive process death, it belongs in a persisted state holder (e.g. `SavedStateHandle`), not just in `conflateAsState`.

Severity: **Should-fix** when the result represents user data that must not be lost on backgrounding.

### Positive Signals

- All destinations are top-level `@Serializable data class` / `object` implementing `NavKey`
- `dropUnlessResumed { backStack.add(...) }` on every tap-driven navigation call
- `rememberSaveableStateHolderNavEntryDecorator()` present in every custom `entryDecorators` list
- App-level nav holder owns the back stack; feature ViewModels expose navigation state as events/state, not by holding `backStack`
- `rememberViewModelStoreNavEntryDecorator()` added when per-entry `viewModel()` scoping is needed (and placed after the saveable-state decorator)
- `ResultEventBus` used for screen-to-screen results; outcomes that must survive process death persisted in `SavedStateHandle`

### Nav3 Scoring Note

Nav3 findings map to existing score categories — do not open a new one:

| Finding | Category |
|---------|----------|
| Feature ViewModel owns/mutates back stack | **State Management** (ownership of state) |
| `entryDecorators` missing saveable-state decorator | **State Management** |
| Anonymous / non-top-level `NavKey` types | **State Management** |
| `ResultEventBus` result assumed to survive process death | **State Management** |
| Navigation triggered in composition body | **Side Effects** |
| Missing `dropUnlessResumed` click guard | **Side Effects** (Should-fix) |

Report State findings as **Nav3 backstack state**; report Side Effects findings as **Nav3 navigation side effects**.

Do not open a new score category for Nav3. If the project has no Nav2 code at all, note in the report that Nav2 navigation patterns are out of scope and Nav3 is the only navigation layer audited.

## 3. Performance Checks

### Search For
Expand Down
Loading