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
17 changes: 11 additions & 6 deletions .claude/agents/circuit-scaffold.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,17 @@ not a finished feature.

## Before writing anything

Read these two files, in this order. They are the contract:
Read these files, in this order. They are the contract:

1. `.claude/skills/circuit-screen/SKILL.md` — the pattern, the required declaration order, and the
traps.
2. `domain/src/commonMain/kotlin/com/scottolcott/recipe/domain/presenter/CategoriesPresenter.kt` —
1. `.claude/skills/circuit-screen/SKILL.md` — the pattern and the required declaration order.
2. `.claude/rules/circuit.md` — the traps: `@Redacted` sinks, per-state events,
`@CircuitSerializable`, and URL mapping.
3. `domain/src/commonMain/kotlin/com/scottolcott/recipe/domain/presenter/CategoriesPresenter.kt` —
the canonical example. Match its structure, import style, and naming exactly.

If the screen reads a repository, also grep the repository interface in `repository/` and confirm
the method you intend to call actually exists. **Do not invent repository methods.** If nothing
If the screen reads a repository, also read `.claude/rules/data-flow.md` (`ContentState`,
`foldToState`, and why `hasAnswer` beats `data.isEmpty()`), and grep the repository interface in
`repository/` to confirm the method you intend to call actually exists. **Do not invent repository methods.** If nothing
suitable exists, leave a TODO naming what's needed and carry on.

## What you produce
Expand All @@ -34,6 +36,9 @@ Given a screen name `Xxx` and a description of what it shows:
last (`com.slack.circuit.serialization.CircuitSerializable` — it implies `@Serializable`, so
don't add that too; `Screen`s are not `Parcelable`).
- `@CircuitInject(XxxScreen::class, AppScope::class)` and `@Inject` on the presenter
- `@Immutable` on the sealed `XxxState` (`@Stable` instead if a case holds an observable holder
such as a `TextFieldState`) — `composeStabilityCheck` fails an unannotated one; see
`.claude/rules/compose-stability.md`
- `@Redacted` on every `eventSink` property
- `retain { }` for retained state, never `rememberSaveable`
- `ContentState.foldToState(onLoading, onError, onContent)` to map producer output onto the
Expand Down
5 changes: 4 additions & 1 deletion .claude/agents/recipes-gradle-runner.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,12 @@ check, don't guess.
| Check formatting without writing | `ktfmtCheck` |
| Static analysis | `detektAll` |
| Sort build-file dependency blocks | `sortDependencies` / `checkSortDependencies` |
| Compose stability (`:ui :domain :shared :app :webApp :desktopApp`) | `composeStabilityCheck` |
| Everything | `build` |

`detektAll` is a custom aggregate task, not stock detekt. `:<module>:build` is the project's
`detektAll` is a custom aggregate task, not stock detekt. Run `composeStabilityCheck` as its own
invocation, never combined with `build` in one command: every target writes the same reports
directory, so its result is only trustworthy on its own. On failure, relay the offenders it names. `:<module>:build` is the project's
standard compile check — prefer it over `assemble` unless the caller asked for something narrower.

Run from the repo root with `./gradlew`. Add `--console=plain`; it strips the progress-bar control
Expand Down
79 changes: 79 additions & 0 deletions .claude/rules/circuit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
---
paths:
- "domain/**"
- "ui/**"
---

# Circuit screens, state, and navigation

A presenter (`:domain`) and its UI (`:ui`) are joined only by
`@CircuitInject(XxxScreen::class, AppScope::class)` on each side. `:domain` never imports from
`:ui`. For the step-by-step of adding a screen, use `/circuit-screen`.

## Presenter file layout

`domain/.../presenter/XxxPresenter.kt`, in this order: presenter class → sealed
`XxxState : CircuitUiState` (Loading/Error/Success) → sealed `XxxEvent` with nested per-state
sub-interfaces → the `Screen` last, as `@CircuitSerializable(AppScope::class) data object XxxScreen : Screen`.

- **`@Redacted` on every `eventSink`.** Comes from `dev.zacsweers.redacted.annotations.Redacted`;
keeps lambdas out of `toString`.
- **Events are nested per state**, not one flat sealed interface. A `Success` state's sink only
accepts `XxxEvent.Success`.
- A retry is conventionally an `Int` counter (`retryTrigger`) passed into the producer as a key,
incremented by `XxxEvent.Error.RetryClicked`.

## Retained state

State survives recomposition/config change via `androidx.compose.runtime.retain.retain { }`,
**not** `rememberSaveable`. One sanctioned exception: `HomePresenter` persists the selected tab
with `rememberSerializable`, because that selection is cheap to store and worth surviving process
death so the app reopens on the tab the user left. Don't "fix" it to `retain { }`.

**`:domain` depends on Material3 (`api`), deliberately.** `SearchState` holds a `SearchBarState`
and `RecipeScaffoldState`/`SearchScreen` carry a `SearchBarValue`, so that the presenter — not the
composable — owns whether the search bar is expanded. This is the *only* sanctioned place for
Material3 widget state in `:domain`; other widget state belongs in `:ui`.

## URL mapping

A new screen must also be added to `domain/.../navigation/ScreenUrlMapper.kt` — both the
`Screen.toUrlPath()` and `urlPathToScreen()` directions. It backs `recipes://app/...` deep links
and browser history on web. Home tabs are the exception to writing anything there by hand: each
`HomeTabScreen` carries its own `urlSegment` and the mapper reads `HOME_TABS`, so adding a tab to
that one list makes it addressable in both directions.

## Navigation persistence

Since Circuit 0.38, `Screen` and `PopResult` are no longer `Parcelable`; the back stack is
persisted with kotlinx-serialization.

- **`@CircuitSerializable(AppScope::class)` on the `Screen`**, from
`com.slack.circuit.serialization`. It is `@MetaSerializable`, so it already implies
`@Serializable` — don't add that too.
- Screens with parameters are a sealed interface whose **concrete cases** carry the annotation —
see `RecipesScreen.ByCategory` / `.ByArea` / `.BySearch` / `.Favorites`. The sealed parent is
never annotated.
- Every parameter of a screen must be serializable. `RecipeId`, `CategoryId` and `IngredientId`
already are; a new parameter type in `:model` needs `@Serializable` added there.
- `SubScreen`s from `circuitx-subcircuit` never enter the back stack and get **no** annotation —
they may hold unserializable state such as a `TextFieldState`.

Circuit codegen turns each `@CircuitSerializable` screen into a `CircuitSerializerRegistration`
multibinding, and `domain/.../circuit/CircuitProviders.kt` folds the whole set into a
`SerializableCircuitSaver` on `Circuit.Builder`. Nothing to register by hand — but the failure
modes are runtime, not compile time:

- The saver is installed with the static `setCircuitSaver(saver)` overload rather than the
`setCircuitSaver { fallback -> ... }` transform, so **saving** an unregistered or unserializable
screen **throws** instead of quietly falling through to the registry-backed saver. That is
deliberate.
- **Restoring** one can only return null, and the nav stack drops that record; `CircuitProviders`
passes `onRestoreError` to the logger so it isn't silent.

**The nav stack is `rememberSaveableNavStack`, not `rememberSaveableBackStack`.** `BackStack`
stubs `forward()` and `backward()` to `false`, and `NavigatorImpl` delegates straight to them, so
switching to it silently turns the browser back/forward buttons on web into no-ops —
`ui/src/webMain/.../BrowserHistoryEffect.web.kt` drives navigation through
`Navigator.backward()`/`forward()`. Both `RecipeApp.kt` and `RecipeScaffoldPresenter.kt` must stay
on `rememberSaveableNavStack`.
79 changes: 79 additions & 0 deletions .claude/rules/compose-stability.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
---
paths:
- "ui/**"
- "domain/**"
- "model/**"
- "repository/**"
- "shared/**"
- "app/**"
- "desktopApp/**"
- "webApp/**"
- "config/compose/**"
---

# Compose stability

Everything a presenter hands to a composable has to be *stable*, or Compose re-runs the UI on every
recomposition. Two rules keep it that way:

- **Collections headed for the UI are `ImmutableList`**, from the repository's public signature
onward — `SourceOfTruth.reader`, `toModel()`, the `Flow<Outcome<…>>` return type, the producer's
`ContentState<ImmutableList<T>>`, and the screen state's field. Entities and DTOs stay `List`:
Store's local type is `List<Entity>` and nothing below `toModel()` ever reaches composition, so
converting there is a copy that buys nothing. `:model` carries
`api(libs.kotlinx.collections.immutable)` for this.
- **`@Immutable` when it is true, `@Stable` when it is not.** `@Immutable` promises no public
property ever changes: right for the `:model` classes and for a screen state whose fields are all
values (`AreasState`, `RecipesState`, `RecipeDetailsState`, `SearchTabState`, `RecipesScreen`).
A state that carries an observable holder — `SearchBarState` and `TextFieldState` in `SearchState`,
`NavStack`/`Navigator` in `RecipeScaffoldState` and `HomeState`, the Metro graph in
`DesktopAppGraph` — gets `@Stable`. Both make the type skippable; only one of them is honest, and
a class marked `@Stable` must back its mutable properties with snapshot state (this is why
`BackShortcutHost.onBack` is a `MutableState`).

Annotate the *declared parameter type*: `@Stable` on a supertype does not propagate, so
`DesktopAppGraph` carries its own annotation rather than inheriting one from `AppGraph`.

A sealed `CircuitUiState` interface is unstable until annotated — the compiler cannot see the
implementations — so every one of them carries a marker.

## `composeStabilityCheck`

**None of the above is taken on trust — `./gradlew composeStabilityCheck` enforces it.** The
`compose.stability` convention plugin turns on the Compose compiler's own metrics and reports for
the six modules that apply the compiler plugin (`:ui`, `:domain`, `:shared`, `:app`, `:webApp`,
`:desktopApp`) and registers a per-project check that reads them. It fails on a class the compiler
inferred as `unstable`, and on a `restartable` composable that is not `skippable`. A `runtime`
class passes: its stability depends on a generic argument and is settled at run time. There is no
aggregate task — Gradle's cross-project name matching fans `composeStabilityCheck` out to all six.

Exceptions live in `config/compose/unstable-classes.txt` and
`config/compose/unskippable-composables.txt`, one `<module>:<fully.qualified.Name>` per line, each
with a comment saying why. Today only the Metro graph impls and the two Android entry points are
listed, none of which is ever a composable parameter.

Reports land in `<module>/build/reports/compose/reports/` (`-classes.txt`, `-composables.txt`) with
`-module.json` counts under `.../metrics/<target>/main/`. Read them for *why* something failed —
the entry lists each property or parameter and its verdict. The compiler names them after the
Kotlin module, which is the Gradle project path — so they carry a colon, `Recipes:domain-classes.txt`,
and CI stages a renamed copy before uploading them because `upload-artifact` rejects one.

Two traps in those reports:

- **Only the metrics path is per-target; the reports path is not.** The Compose plugin appends
`<target>/<compilation>` to `metricsDestination` but hands `reportsDestination` to every
compilation verbatim, and a KMP module's targets share a Kotlin module name. So all six targets
write the same `Recipes_ui-classes.txt` and the last to run wins. After a full `./gradlew build`
you are reading whichever target finished last. `composeStabilityCheck` is the invocation to
trust: it depends on exactly one compile task (JVM where there is one, `compileDebugKotlin` for
`:app`, `compileKotlinJs` for `:webApp`), so it has a single writer. That is also why it is not
wired into `check`, and why CI runs it as its own step *before* `build`.
- **`reportsDestination` is a compiler *input*, not an output.** A compile task whose reports have
been deleted still counts as up to date and quietly writes nothing. `compose.stability` declares
the directory as an output of that one designated compile task to fix it, which also means the
task re-runs after a full `build` and stays the last writer.

Detekt's `UnstableCollections` covers the same ground at the source level and is on. It is right
about composables that emit UI and wrong about ones that return a value — those are neither
restartable nor skippable, so a parameter's stability is inert. `RecipesProducer.produceByIngredients`
is the single `@Suppress` for that reason.
63 changes: 63 additions & 0 deletions .claude/rules/data-flow.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
---
paths:
- "repository/**"
- "domain/**"
---

# Reading data: `Outcome` and `ContentState`

Repositories return `Flow<Outcome<T>>`, not Store's own `StoreReadResponse`. **Store 5 is an
implementation detail of `:repository`** — it is an `implementation` dependency there, and nothing
above that module compiles against it. `io.github.solcott:dataresult-store5` does the translation
in `asOutcomes(fetching = refresh) { … }`, at the end of each repository's chain. It sits *inside*
the `flatMapLatest { refresh -> … }` so it can see whether a fetch is coming.

The types come from [kmp-dataresult](https://github.com/solcott/kmp-dataresult), shared with the
`Countries` project. Resolving them needs `gpr.user`/`gpr.key` in `~/.gradle/gradle.properties` (a
classic PAT with `read:packages`) — GitHub Packages authenticates even public reads.

The path from a repository to a screen state is three steps:

1. `Flow<Outcome<T>>` — `Loading`, `Data(value, origin)` or `Error(cause, origin)`. Store's
`Initial` and `Loading` both become `Outcome.Loading`; `NoNewData` is dropped. An empty first
read from cache while a fetch is pending is held back until the fetch answers. Room returns `[]`
for a key it has never fetched, and that's a cache miss, not an empty result.
`getFavoritesAsFlow` passes no `fetching`, because nothing is coming to answer instead.
2. `produceRetainedContentState(initial, keys) { … }` from `libs.uistateCircuit`
(`io.github.solcott.uistate.circuit`) folds those emissions into a retained `ContentState<T>`.
**`ContentState.data` is what holds the last loaded value** — the separate retained
`lastItems`/`lastRecipes` caches that used to sit beside the response are gone, and so is
`ListUi`. Shared with `Countries`; the local copy in `:domain`'s `producer` package is gone.
3. `ContentState.foldToState(onLoading, onError, onContent)` in `presenter/ListContent.kt` maps it
onto a screen's own three-case `CircuitUiState`.

A screen fed by several sources at once uses the group versions of the first two steps. The
repository returns `combineOutcomes(a, b, c)`, a `Flow<Outcomes3<A, B, C>>` with each source seeded
with `Loading` so the fastest one shows at once. The presenter folds it with
`produceRetainedContentStates(contentStatesOf(…))` into one `ContentState` per source. Destructure
the group for names; `isAnyLoading`/`errorOrNull` on it answer for all of them. `SearchPresenter` is
the example.

Two things are easy to get wrong:

- **Check `hasAnswer`, never `data.isEmpty()`**, to tell "nothing has loaded yet" from "loaded and
empty". `hasLoaded` reads `origin`, which stays null until the first value arrives. `hasAnswer`
also refuses an empty read *from cache* while its request is in flight or has failed. That's a key
the database has never seen, not an empty result. The repositories already hold that read back
with `asOutcomes(fetching = refresh) { … }`, and `hasAnswer` is the backstop. Testing the data for
emptiness leaves a spinner over a legitimately empty tab forever. `hasAnswer` can't, because it is
only false while a request is outstanding or has failed.
- **Having data beats being in flight.** `foldToState` checks `hasAnswer` first, which is what
renders a background refresh as `isRefreshing` over the existing grid instead of dropping back to
a full-screen spinner. `domain`'s `refreshKeepsPreviousAreas` test pins this — don't reorder those
branches.

`DataError` is structured; screen states carry `String`. `DataError.toMessage()` in
`ListContent.kt` bridges the two, and belongs in `:ui` with the rest of the user-facing copy once
these get localized.

**A test fake for an in-flight request must not complete.** `produceRetainedContentState` settles a still
loading status when its source completes, so a spinner can never hang — which means
`flowOf(Outcome.Loading)` is a *settled empty result*, not a pending one. Store streams never
complete; use the `inFlight()` helper in the presenter tests, which emits `Loading` and then awaits
cancellation.
50 changes: 50 additions & 0 deletions .claude/rules/gradle-build.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
---
paths:
- "**/*.gradle.kts"
- "build-logic/**"
- "gradle/**"
- "gradle.properties"
---

# Gradle build and conventions

## JDK 25, bytecode 17

Everything runs on **JDK 25** and everything compiles to **JVM 17** — those are two different
settings and the split is deliberate. The daemon uses Amazon Corretto 25, auto-provisioned via
foojay from `gradle/gradle-daemon-jvm.properties`; `jvm-toolchain = "25"` in the catalog is the
JDK that compiles the code and runs it (`:desktopApp:run`, tests, the jpackage runtime image), while
`jvmSourceCompatibility`/`jvmTargetCompatibility = "17"` is the bytecode and API level.
Don't override the JDK.

A toolchain silently sets `jvmTarget` to its own version unless a target pins one, so **every JVM
compilation pins it explicitly** — `jvm {}` and `android {}` in `kmp.library`, plus `:app` and
`:desktopApp` — each with `jvmTarget` *and* `-Xjdk-release=17`, which additionally hides post-17 JDK
APIs so nothing that would crash on Android can compile. Drop either and you silently get bytecode
25. `-Xjdk-release` is JVM-only: never add it to the project-wide `compilerOptions`, which also
feeds Native, JS, and WasmJS. `kotlin.jvm.target.validation.mode` is unset (default `error`), so a
Kotlin/Java target mismatch fails the build rather than shipping — that check is a feature, not an
obstacle.

## Module conventions

- New library modules apply the **`kmp.library`** convention plugin
(`build-logic/src/main/kotlin/kmp.library.gradle.kts`), not raw KMP plugins. It sets the targets,
toolchain, Android namespace (`com.scottolcott.recipe.<module>`), ktfmt, detekt, and sort-dependencies.
- **No `iosX64`** — Intel Macs can't run the iOS simulator build.
- `-Xexpect-actual-classes` is set project-wide by `kmp.library`. Don't re-declare it per file.
The `-opt-in=kotlin.time.ExperimentalTime` that used to sit beside it is gone: `kotlin.time.Clock`
and `Instant` went Stable in Kotlin 2.3.0, so neither the flag nor a per-file `@OptIn` is needed.
- `@Parcelize` comes from `io.github.solcott.kmp.parcelize` (a multiplatform plugin), **not**
kotlin-parcelize. It applies only to the `:model` id value classes (`RecipeId`, `CategoryId`,
`IngredientId`) — **`Screen`s are not Parcelable** (see `.claude/rules/circuit.md`).
- Any module declaring `Screen`s needs the kotlinx-serialization Gradle plugin
(`libs.plugins.kotlinx.serialization`); `:domain` already applies it.

## Tooling tasks

- `detektAll` is a custom aggregate task from `build-logic/src/main/kotlin/detekt.gradle.kts`.
Plain `detekt` does not cover all source sets.
- `buildHealth` (dependency-analysis) is noisy about Compose artifacts. Most modules already carry
`dependencyAnalysis { issues { onUnusedDependencies { exclude(...) } } }` — add to those rather
than deleting a dependency it flags.
Loading
Loading