From cf5939d9706a32c8bd1886ac12cee3625f641f38 Mon Sep 17 00:00:00 2001 From: Juhana Ilmoniemi Date: Thu, 14 May 2026 09:23:54 +0300 Subject: [PATCH 1/3] spec: unarchive() primitive on ConversationRepository (#96) --- .../architecture/96-unarchive-primitive.md | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 docs/specs/architecture/96-unarchive-primitive.md diff --git a/docs/specs/architecture/96-unarchive-primitive.md b/docs/specs/architecture/96-unarchive-primitive.md new file mode 100644 index 0000000..9909dfb --- /dev/null +++ b/docs/specs/architecture/96-unarchive-primitive.md @@ -0,0 +1,86 @@ +# Spec: `unarchive()` primitive on `ConversationRepository` (#96) + +## Files to read first + +- `app/src/main/java/de/pyryco/mobile/data/repository/ConversationRepository.kt:18-47` — interface contract; new method slots in next to `archive` (line 31) +- `app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt:125-133` — `archive()` impl; `unarchive()` is its inverse with the same `state.update { ... unknown(id) ... }` shape +- `app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt:32-46` — `observeConversations` filter projection; confirms the test assertions (Archived stream filters `conv.archived`, Discussions filters `!conv.isPromoted && !conv.archived`) +- `app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt:203` — `unknown(id)` helper to reuse for the unknown-ID case +- `app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt:326-333` — `seed-discussion-archived` is already in `SEED_RECORDS` with `archived = true`; the round-trip test should use it (no need to create new seed data) +- `app/src/test/java/de/pyryco/mobile/data/repository/FakeConversationRepositoryTest.kt:209-286` — existing `archive_*` tests; the new `unarchive_*` tests follow the same idioms (`runBlocking`, `repo.observeConversations(filter).first()`, `assertTrue`-with-message style) +- `app/src/main/java/de/pyryco/mobile/data/model/Conversation.kt:14` — confirms `archived: Boolean = false` lives on `Conversation` (no domain-model change needed) + +## Context + +#93 introduced the `archived: Boolean` flag and `ConversationFilter.Archived`, plus the flag-based `archive()` operation that retains the row (not delete). The Archived Discussions UI restore action (#94) needs the inverse primitive: clear the flag so the row reappears in `ConversationFilter.Discussions` (or `Channels`, if it had been promoted) and disappears from `ConversationFilter.Archived`. This ticket adds only the data-layer primitive — no UI, no ViewModel. + +The split rationale is in the issue body: bundling into #93 would have pushed the foundation slice over the AC limit, and slicing the inverse out lets #94 start as soon as this primitive lands. + +## Design + +### Interface change + +Add a single method to `ConversationRepository`, placed immediately below `archive` (line 31) to make the inverse-pairing obvious: + +```kotlin +suspend fun unarchive(conversationId: String) +``` + +Signature mirrors `archive` exactly: raw `String` ID, `suspend`, no return value, no extra parameters. No KDoc beyond what the file header already establishes (mutating one-shot; affected streams re-emit). + +### Fake impl + +Add `FakeConversationRepository.unarchive(conversationId)` next to `archive()` (line 125). The body is the exact inverse of `archive()`: + +- Atomic `state.update { records -> ... }` +- Unknown ID → `throw unknown(conversationId)` (reuse the existing helper at line 203) +- Set `conversation.archived = false` via `copy(conversation = record.conversation.copy(archived = false))` +- Behavior on an already-unarchived conversation: **silent success / idempotent**. The `copy(archived = false)` on a record whose flag is already `false` produces an equal `Conversation` value. The `StateFlow` deduplicates equal emissions, so the Archived/Discussions/Channels streams do not re-emit unnecessarily. This matches `archive()`'s established idempotent-no-precondition pattern (the existing `archive_isIdempotent` test, FakeConversationRepositoryTest.kt:274, confirms this is the established convention for this layer). + +Decision rationale for idempotence over explicit precondition: the issue body explicitly delegates this choice to the architect. `archive()` chose silent idempotence and has a test that locks it in; choosing the same for `unarchive()` keeps the pair symmetric and avoids a precondition check that would have to be re-justified later if/when the remote Phase 4 implementation arrives (REST `DELETE`-style unarchive endpoints are conventionally idempotent). + +The whole method is ~6 lines, structurally identical to `archive()`: + +```kotlin +override suspend fun unarchive(conversationId: String) { + state.update { records -> + val record = records[conversationId] ?: throw unknown(conversationId) + records + ( + conversationId to + record.copy(conversation = record.conversation.copy(archived = false)) + ) + } +} +``` + +That snippet is the full body — included here because it IS the contract (mirroring an existing 8-line function), not because it pre-writes complex logic. + +### State + concurrency model + +Identical to `archive()`. `MutableStateFlow>.update { }` is atomic; observers re-emit on every non-equal change. No new flows, no dispatcher concerns, no scope-management. The conversation is a `suspend fun` only because it conforms to the interface — there is no actual suspension point in the Fake. + +### Error handling + +- Unknown `conversationId` → `IllegalArgumentException("Unknown conversation: ")` via the existing `unknown()` helper. Same as `archive()`. +- Conversation exists but is not archived → no-op (silent success). See idempotence rationale above. +- No other failure modes for the Fake impl. + +### Testing strategy + +Unit tests only (no instrumented tests — `./gradlew test`). Three new test cases in `FakeConversationRepositoryTest`, mirroring the `archive_*` cluster (lines 209-286): + +1. **`unarchive_movesConversation_from_Archived_to_Discussions_andRetainsInStore`** — the AC test. + - Use the existing `seed-discussion-archived` seed (no need to call `repo.archive()` first). + - Pre-assert: appears in `Archived`, not in `Discussions`. + - Call `repo.unarchive("seed-discussion-archived")`. + - Assert: leaves `Archived`, appears in `Discussions`, still in `All`, still not in `Channels` (it's a discussion, not a promoted channel). + +2. **`unarchive_onUnknownId_throws`** — mirrors `archive_onUnknownId_throws` (line 263). `repo.unarchive("nope")` inside `runBlocking { }` must throw `IllegalArgumentException`. + +3. **`unarchive_isIdempotent`** — mirrors `archive_isIdempotent` (line 274). Calling `unarchive` twice on the same already-unarchived conversation must leave it in `Discussions` exactly once (i.e. the second call is a silent no-op and does not duplicate). + +These are bullet-pointed scenarios, not test bodies — the developer writes the test code in the project's testing idiom (JUnit4 + `runBlocking { }` + `assertTrue`/`assertEquals` from `org.junit.Assert.*`, matching the existing file). + +## Open questions + +None. The issue body explicitly delegated the no-op-vs-precondition decision to the architect; that is resolved above (silent idempotent, matching `archive()`). From 6ddbd3a44d7bb644d684b0a2e13a04c69575e7b6 Mon Sep 17 00:00:00 2001 From: Juhana Ilmoniemi Date: Thu, 14 May 2026 09:27:08 +0300 Subject: [PATCH 2/3] feat(data): add unarchive() primitive to ConversationRepository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the inverse of archive() so the Archived Discussions UI (#94) can restore an archived discussion. The Fake implementation clears the archived flag; behavior mirrors archive() (unknown ID → IllegalArgumentException, idempotent on already-unarchived conversations). Closes #96 Co-Authored-By: Claude Opus 4.7 --- .../data/repository/ConversationRepository.kt | 2 + .../repository/FakeConversationRepository.kt | 10 +++ .../FakeConversationRepositoryTest.kt | 61 +++++++++++++++++++ .../list/ChannelListViewModelTest.kt | 4 ++ .../list/DiscussionListViewModelTest.kt | 8 +++ 5 files changed, 85 insertions(+) diff --git a/app/src/main/java/de/pyryco/mobile/data/repository/ConversationRepository.kt b/app/src/main/java/de/pyryco/mobile/data/repository/ConversationRepository.kt index c33103c..b2fb4bc 100644 --- a/app/src/main/java/de/pyryco/mobile/data/repository/ConversationRepository.kt +++ b/app/src/main/java/de/pyryco/mobile/data/repository/ConversationRepository.kt @@ -30,6 +30,8 @@ interface ConversationRepository { suspend fun archive(conversationId: String) + suspend fun unarchive(conversationId: String) + suspend fun rename( conversationId: String, name: String, diff --git a/app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt b/app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt index d278f70..66dd48d 100644 --- a/app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt +++ b/app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt @@ -132,6 +132,16 @@ class FakeConversationRepository( } } + override suspend fun unarchive(conversationId: String) { + state.update { records -> + val record = records[conversationId] ?: throw unknown(conversationId) + records + ( + conversationId to + record.copy(conversation = record.conversation.copy(archived = false)) + ) + } + } + override suspend fun rename( conversationId: String, name: String, diff --git a/app/src/test/java/de/pyryco/mobile/data/repository/FakeConversationRepositoryTest.kt b/app/src/test/java/de/pyryco/mobile/data/repository/FakeConversationRepositoryTest.kt index 7855ee1..555e4ae 100644 --- a/app/src/test/java/de/pyryco/mobile/data/repository/FakeConversationRepositoryTest.kt +++ b/app/src/test/java/de/pyryco/mobile/data/repository/FakeConversationRepositoryTest.kt @@ -285,6 +285,67 @@ class FakeConversationRepositoryTest { ) } + @Test + fun unarchive_movesConversation_from_Archived_to_Discussions_andRetainsInStore() = + runBlocking { + val repo = FakeConversationRepository() + val archivedId = "seed-discussion-archived" + + assertTrue( + "seeded archived discussion must appear in Archived before unarchive", + repo.observeConversations(ConversationFilter.Archived).first().any { it.id == archivedId }, + ) + assertTrue( + "seeded archived discussion must not appear in Discussions before unarchive", + repo.observeConversations(ConversationFilter.Discussions).first().none { it.id == archivedId }, + ) + + repo.unarchive(archivedId) + + assertTrue( + "unarchived conversation must leave Archived", + repo.observeConversations(ConversationFilter.Archived).first().none { it.id == archivedId }, + ) + assertTrue( + "unarchived discussion must appear in Discussions", + repo.observeConversations(ConversationFilter.Discussions).first().any { it.id == archivedId }, + ) + assertTrue( + "unarchived discussion must not appear in Channels", + repo.observeConversations(ConversationFilter.Channels).first().none { it.id == archivedId }, + ) + assertTrue( + "unarchived conversation must be retained in All", + repo.observeConversations(ConversationFilter.All).first().any { it.id == archivedId }, + ) + } + + @Test + fun unarchive_onUnknownId_throws() { + val repo = FakeConversationRepository() + try { + runBlocking { repo.unarchive("nope") } + assertTrue("expected IllegalArgumentException", false) + } catch (_: IllegalArgumentException) { + // expected + } + } + + @Test + fun unarchive_isIdempotent() = + runBlocking { + val repo = FakeConversationRepository() + val archivedId = "seed-discussion-archived" + repo.unarchive(archivedId) + repo.unarchive(archivedId) + val discussions = repo.observeConversations(ConversationFilter.Discussions).first() + assertEquals( + "unarchived conversation must appear exactly once after two unarchive() calls", + 1, + discussions.count { it.id == archivedId }, + ) + } + @Test fun rename_updates_name_and_reEmits() = runBlocking { diff --git a/app/src/test/java/de/pyryco/mobile/ui/conversations/list/ChannelListViewModelTest.kt b/app/src/test/java/de/pyryco/mobile/ui/conversations/list/ChannelListViewModelTest.kt index 416a523..17edf96 100644 --- a/app/src/test/java/de/pyryco/mobile/ui/conversations/list/ChannelListViewModelTest.kt +++ b/app/src/test/java/de/pyryco/mobile/ui/conversations/list/ChannelListViewModelTest.kt @@ -363,6 +363,8 @@ class ChannelListViewModelTest { override suspend fun archive(conversationId: String): Unit = TODO("not used") + override suspend fun unarchive(conversationId: String): Unit = TODO("not used") + override suspend fun rename( conversationId: String, name: String, @@ -403,6 +405,8 @@ class ChannelListViewModelTest { override suspend fun archive(conversationId: String): Unit = TODO("not used") + override suspend fun unarchive(conversationId: String): Unit = TODO("not used") + override suspend fun rename( conversationId: String, name: String, diff --git a/app/src/test/java/de/pyryco/mobile/ui/conversations/list/DiscussionListViewModelTest.kt b/app/src/test/java/de/pyryco/mobile/ui/conversations/list/DiscussionListViewModelTest.kt index a3d51d1..d32e386 100644 --- a/app/src/test/java/de/pyryco/mobile/ui/conversations/list/DiscussionListViewModelTest.kt +++ b/app/src/test/java/de/pyryco/mobile/ui/conversations/list/DiscussionListViewModelTest.kt @@ -70,6 +70,8 @@ class DiscussionListViewModelTest { override suspend fun archive(conversationId: String): Unit = TODO("not used") + override suspend fun unarchive(conversationId: String): Unit = TODO("not used") + override suspend fun rename( conversationId: String, name: String, @@ -356,6 +358,8 @@ class DiscussionListViewModelTest { override suspend fun archive(conversationId: String): Unit = TODO("not used") + override suspend fun unarchive(conversationId: String): Unit = TODO("not used") + override suspend fun rename( conversationId: String, name: String, @@ -389,6 +393,8 @@ class DiscussionListViewModelTest { override suspend fun archive(conversationId: String): Unit = TODO("not used") + override suspend fun unarchive(conversationId: String): Unit = TODO("not used") + override suspend fun rename( conversationId: String, name: String, @@ -441,6 +447,8 @@ class DiscussionListViewModelTest { override suspend fun archive(conversationId: String): Unit = TODO("not used") + override suspend fun unarchive(conversationId: String): Unit = TODO("not used") + override suspend fun rename( conversationId: String, name: String, From 7bb6efec858513740645f13def1e8a2ff6af83c0 Mon Sep 17 00:00:00 2001 From: Juhana Ilmoniemi Date: Thu, 14 May 2026 09:30:50 +0300 Subject: [PATCH 3/3] docs: unarchive() primitive on ConversationRepository (#96) Co-Authored-By: Claude Opus 4.7 --- docs/knowledge/INDEX.md | 2 +- docs/knowledge/codebase/96.md | 40 +++++++++++++++++++ .../features/conversation-repository.md | 3 +- 3 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 docs/knowledge/codebase/96.md diff --git a/docs/knowledge/INDEX.md b/docs/knowledge/INDEX.md index 5c52b24..8e51d8b 100644 --- a/docs/knowledge/INDEX.md +++ b/docs/knowledge/INDEX.md @@ -10,7 +10,7 @@ One-line pointers to evergreen docs. Per-ticket implementation notes live under - [Scanner Denied screen](features/scanner-denied-screen.md) — stateless camera-permission-denied surface locked to Figma node `32:2` (#61); `Surface` + `Column` with a 120dp Compose-`Canvas` camera-with-strike illustration (rounded-rect body + viewfinder-bridge `Path` + stroked outer-lens circle + filled inner-lens dot + `colorScheme.error` diagonal `drawLine`), `headlineSmall` headline, `bodyMedium` explainer (`widthIn` 300dp), bottom-pinned `fillMaxWidth` filled `Button` + `TextButton` via a `Spacer(weight=1f)`. Two caller-owned lambdas; no NavHost wiring yet (Phase 4 adds the `TopAppBar` and routes from the CameraX permission flow). Instrumented Compose tests since #101 — three-method `ScannerDeniedScreenTest` at `app/src/androidTest/.../onboarding/ScannerDeniedScreenTest.kt` asserts the `"Camera permission required"` heading (exact match) plus `hasClickAction()` on `"Open settings"` and `"Paste code instead"`; structure only, the two lambdas are passed as `{}` no-ops and callback wiring isn't asserted. - [Scanner Connecting screen](features/scanner-connecting-screen.md) — stateless in-flight-pairing surface locked to Figma node `32:20` (#62); `Surface` + `Column(Arrangement.Center)` of a 48dp indeterminate `CircularProgressIndicator` (zero theming args — M3 defaults bind `colorScheme.primary` arc + `colorScheme.surfaceVariant` track) → `bodyLarge` headline ("Connecting to your pyrycode server…") on `colorScheme.onSurface` → `bodyMedium` server address on `colorScheme.onSurfaceVariant` with `fontFamily = FontFamily.Monospace` as a top-level `Text` parameter (family-only override of the named role). Single `serverAddress: String` parameter, no callbacks; no NavHost wiring yet (Phase 4 owns the handshake state machine). Instrumented Compose tests since #101 — two-method `ScannerConnectingScreenTest` at `app/src/androidTest/.../onboarding/ScannerConnectingScreenTest.kt` asserts `hasText("Connecting to your pyrycode server", substring = true)` (substring deliberately avoids the U+2026 ellipsis) and exact match on the `"home.lan:7117"` literal passed as `serverAddress`. The project's first instrumented test for a no-callback screen — the pattern degenerates cleanly to two render assertions with no counters or event lists. - [Data model](features/data-model.md) — `Conversation` / `Session` / `Message` schema in `data/model/`; shared shape across mobile, Discord, and pyrycode CLI consumers. Sentinel `DefaultScratchCwd` co-lives in `Conversation.kt` as the single-source-of-truth marker for unbound-workspace conversations. `Conversation.isSleeping: Boolean = false` (#20) is derived in Phase 1 and wire-parsed in Phase 4. `Conversation.archived: Boolean = false` (#93) is the authoritative archive bit — flipped by `archive(id)`, parsed from the wire in Phase 4. -- [Conversation repository](features/conversation-repository.md) — `ConversationRepository` interface + `ConversationFilter` (`All` / `Channels` / `Discussions` / `Archived` since #93), `ThreadItem` (`MessageItem` / `SessionBoundary`), `BoundaryReason`; data-layer seam every UI tier binds to. Phase 1 binding is the in-memory `FakeConversationRepository`; the `observeConversations` projection also stamps the derived `Conversation.isSleeping` flag (#20). Filter matrix (#93): `All` returns everything including archived; `Channels` / `Discussions` carry an explicit `!archived` clause; `Archived` returns archived rows regardless of `isPromoted`. `archive(id)` since #93 flips the flag instead of removing the entry — the conversation stays observable through `Archived` (and `All`). +- [Conversation repository](features/conversation-repository.md) — `ConversationRepository` interface + `ConversationFilter` (`All` / `Channels` / `Discussions` / `Archived` since #93), `ThreadItem` (`MessageItem` / `SessionBoundary`), `BoundaryReason`; data-layer seam every UI tier binds to. Phase 1 binding is the in-memory `FakeConversationRepository`; the `observeConversations` projection also stamps the derived `Conversation.isSleeping` flag (#20). Filter matrix (#93): `All` returns everything including archived; `Channels` / `Discussions` carry an explicit `!archived` clause; `Archived` returns archived rows regardless of `isPromoted`. `archive(id)` since #93 flips the flag instead of removing the entry; `unarchive(id)` since #96 is the symmetric inverse (silent-idempotent flag flip back to `false`, same `unknown(id)` throw on missing). - [Dependency injection](features/dependency-injection.md) — Koin scaffold; single `appModule` at `di/AppModule.kt`, `startKoin` owned by `PyryApp.onCreate`. Downstream tickets append `single`/`viewModel` lines. - [App preferences](features/app-preferences.md) — `AppPreferences` wrapper over a shared `DataStore` (file `app_prefs`); typed `Flow` reads + `suspend` setters for non-secret app state. Keys: `pairedServerExists` (#11/#12/#13), `themeMode: Flow` (#86 — `stringPreferencesKey("theme_mode")` holding the enum's `.name`; `entries.firstOrNull { it.name == stored } ?: SYSTEM` fallback for both absent + unparseable; consumed reactively at `MainActivity`'s `setContent` root to resolve `PyrycodeMobileTheme(darkTheme = …)` and inside `composable(Routes.SETTINGS)` to drive the Theme row subtitle). - [ConversationRow](features/conversation-row.md) — stateless M3 `ListItem`-based row consuming `Conversation` + `Message?`; auto-name fallback branched on `isPromoted`, whitespace-normalized 2-line preview, kotlinx-datetime relative-time bucketing (`just now` / `Nm ago` / `Nh ago` / `Yesterday` / `Nd ago` / abbreviated date). Headline is a `Row` with two natural-width decorations: workspace label (#19, trailing) and sleeping-session dot (#20, leading — 8.dp `Box` filled with `colorScheme.outline`, gated on `Conversation.isSleeping`). Optional `onLongClick: (() -> Unit)? = null` (#25) switches the modifier chain from `Modifier.clickable` to `Modifier.combinedClickable` so callers can attach long-press affordances without fanning a screen-specific concept into the shared primitive. Since #68 the `ListItem` carries a `leadingContent = { ConversationAvatar(conversation) }` slot. Shared between the #46 channel list and the discussions drilldown. diff --git a/docs/knowledge/codebase/96.md b/docs/knowledge/codebase/96.md new file mode 100644 index 0000000..ec345c4 --- /dev/null +++ b/docs/knowledge/codebase/96.md @@ -0,0 +1,40 @@ +# #96 — `unarchive()` primitive on `ConversationRepository` + Fake impl + +Adds the trivial inverse of `archive()` introduced in #93: a one-shot mutator that clears `Conversation.archived`, so an archived row leaves `ConversationFilter.Archived` and reappears in the matching live tier (`Discussions` for unpromoted; `Channels` for the hypothetical archived-promoted case). Data-layer-only — the UI restore action (#94) consumes this primitive in a follow-up. + +## Summary + +- `ConversationRepository` gains `suspend fun unarchive(conversationId: String)`, slotted directly under `archive()` (line 32) so the inverse pairing is visually obvious. Signature mirrors `archive()` exactly: raw `String` id, `suspend`, `Unit`, no extra parameters. +- `FakeConversationRepository.unarchive(conversationId)` is a 6-line `state.update { ... }` that copies the existing record with `conversation.copy(archived = false)`. Unknown id throws via the same `unknown(id)` helper as `archive()`. Idempotent by construction — the second call produces an `equals`-identical map and `MutableStateFlow.update` does not re-emit. No precondition check on the current `archived` value (the issue body delegated this choice to the architect; chosen to match `archive()`'s established silent-idempotent shape and stay symmetric with the eventual REST `DELETE`-style remote impl). +- Three new `unarchive_*` tests in `FakeConversationRepositoryTest` mirroring the existing `archive_*` cluster — round-trip from `Archived` → `Discussions` (using the existing `seed-discussion-archived` seed, no `repo.archive()` setup needed), unknown-id-throws, and idempotence-after-double-call. +- Two existing test files (`ChannelListViewModelTest`, `DiscussionListViewModelTest`) gained `override suspend fun unarchive(conversationId: String): Unit = TODO("not used")` arms on five inline `ConversationRepository` stubs so they keep compiling against the widened interface. No production-VM consumer in this slice. + +## Files touched + +- `app/src/main/java/de/pyryco/mobile/data/repository/ConversationRepository.kt:32` — new `unarchive` interface method, two lines below `archive`. +- `app/src/main/java/de/pyryco/mobile/data/repository/FakeConversationRepository.kt:135-143` — new `unarchive` impl, structurally identical to the `archive` block above it. +- `app/src/test/java/de/pyryco/mobile/data/repository/FakeConversationRepositoryTest.kt:288-348` — `unarchive_movesConversation_from_Archived_to_Discussions_andRetainsInStore` (the AC test), `unarchive_onUnknownId_throws`, `unarchive_isIdempotent`. +- `app/src/test/java/de/pyryco/mobile/ui/conversations/list/ChannelListViewModelTest.kt:366,408` — `unarchive` stub override on both inline fake repositories. +- `app/src/test/java/de/pyryco/mobile/ui/conversations/list/DiscussionListViewModelTest.kt:73,361,396,450` — `unarchive` stub override on all four inline fake repositories. + +## Patterns established + +None new — this slice deliberately reuses every pattern #93 set: + +- **Inverse mutators sit immediately next to their forward counterpart in the interface and the impl.** Two-line gap, identical signature shape, identical impl scaffold (`state.update { records -> val record = records[id] ?: throw unknown(id); records + (id to record.copy(...)) }`). When the next paired-inverse mutator lands (e.g. an `unsleep` for the sleeping-session bit, if ever), follow this physical-adjacency rule rather than alphabetising or grouping by lifecycle stage. +- **Reuse the existing `unknown(id)` helper rather than open-coding the error message.** Three call sites today (`archive`, `unarchive`, `rename`); the helper exists at `FakeConversationRepository.kt:217`. New mutators that need an unknown-id branch should reuse it; do not introduce a parallel error-formatting helper. + +## Lessons learned + +- **Widening the `ConversationRepository` interface forces edits to every test file with an inline anonymous-object stub, even if no production VM consumes the new method.** Five `override suspend fun unarchive(conversationId: String): Unit = TODO("not used")` arms across two test files were the bulk of the file count for an XS slice. The reason these stubs aren't shared (as a `BaseFakeConversationRepository` or test-only abstract class) is that each one is scoped to a specific test method and stubs only what that test needs — extracting a shared base would mean every test paying for every method's `TODO`, which isn't a net win at the current ~7 mutator surface. Worth revisiting if the interface grows past ~12 methods or if a test starts needing two stubs that differ on only one method's body. +- **The `archive()` idempotency property generalises cleanly to its inverse.** `archive_isIdempotent` (#93) passed because `MutableStateFlow.update` deduplicates on value equality and `data class` `.copy(archived = true)` on an already-`true` field produces an `equals`-identical record. The same logic applies in reverse: `unarchive_isIdempotent` passes for free because `.copy(archived = false)` on an already-`false` record is also `equals`-identical. This isn't a coincidence — it's the load-bearing property of the `state.update`-with-`data-class-copy` pattern that the file standardised on. Any future paired-inverse mutator using the same shape gets free idempotency on both sides. + +## Links + +- Issue: https://github.com/pyrycode/pyrycode-mobile/issues/96 +- Spec: `docs/specs/architecture/96-unarchive-primitive.md` +- Foundation: #93 ([`./93.md`](./93.md)) — introduced the `archived` flag, `ConversationFilter.Archived`, and the flag-based `archive()` this primitive inverts. +- Split from: #77 (Archived Discussions UI epic). +- Downstream: #94 — Archived Discussions UI restore action consumes this primitive. +- Feature docs touched: [ConversationRepository](../features/conversation-repository.md). +- Prior shape it builds on: #3 ([`./3.md`](./3.md)) — interface; #4 ([`./4.md`](./4.md)) — `state.update { ... }` mutator pattern + `unknown(id)` helper. diff --git a/docs/knowledge/features/conversation-repository.md b/docs/knowledge/features/conversation-repository.md index f200676..03acc99 100644 --- a/docs/knowledge/features/conversation-repository.md +++ b/docs/knowledge/features/conversation-repository.md @@ -16,6 +16,7 @@ interface ConversationRepository { suspend fun createDiscussion(workspace: String? = null): Conversation suspend fun promote(conversationId: String, name: String, workspace: String? = null): Conversation suspend fun archive(conversationId: String) + suspend fun unarchive(conversationId: String) suspend fun rename(conversationId: String, name: String): Conversation suspend fun startNewSession(conversationId: String, workspace: String? = null): Session suspend fun changeWorkspace(conversationId: String, workspace: String): Session @@ -74,7 +75,7 @@ Same package; file `FakeConversationRepository.kt`. In-memory, no persistence, n - **Storage:** `MutableStateFlow>` — a single state-holder keyed by `conversationId`. `ConversationRecord` is a private file-scoped wrapper that pairs the `Conversation` with the full `Session` values (the `Conversation` itself only stores session **ids**). - **`observeConversations(filter)`** is derived from the state-holder via `Flow.map`, `filter`ed per `ConversationFilter` (matrix below), and `sortedByDescending { it.lastUsedAt }`. The sort is filter-agnostic — channel list and discussions drilldown both want most-recently-used first. Initial emission carries the seed records (see "Seed data" below). The projection also stamps `Conversation.isSleeping` from `record.sessions[currentSessionId]?.endedAt != null` (#20) — true exactly when the conversation's current session is closed and no new one has started yet. This is the one place that holds both the conversation and its session map; downstream `UiState` shapes consume an unchanged `List`. Phase 4's Ktor-backed impl will parse `isSleeping` from the wire response instead of deriving it. Filter matrix (#93): `All` = everything (literally — including archived); `Channels` = `isPromoted && !archived`; `Discussions` = `!isPromoted && !archived`; `Archived` = `archived` (regardless of `isPromoted`). The `!archived` clauses on the live tiers are symmetric on purpose — `All` is the only filter that surfaces tombstones unconditionally. - **`observeMessages(conversationId)`** projects from the same state-holder via `state.map { records -> records[id]?.messages?.let(::buildThreadItems) ?: emptyList() }`. Messages live inside `ConversationRecord` as a defaulted `messages: List = emptyList()` field — single source of truth, no sibling map. `buildThreadItems` sorts by `Message.timestamp` ascending, then walks the sorted list inserting a `ThreadItem.SessionBoundary` whenever consecutive messages differ in `sessionId`. Derived boundaries carry `reason = BoundaryReason.Clear` and `occurredAt = message.timestamp` of the new session's first message — both are deliberate defaults that hold until eager boundary emission lands (see "What `observeMessages` does not do" below). Unknown id and known-with-no-messages collapse to the same `emptyList()` path (observation is tolerant; mutation is strict). See `../codebase/9.md`. -- **Mutators** use `state.update { ... }` (atomic CAS, no `Mutex`). `lateinit var` escapes the affected entity out of the lambda so the suspend function can return it. Since #93, `archive(id)` flips the new `Conversation.archived` flag via `record.copy(conversation = record.conversation.copy(archived = true))` instead of removing the entry from the store; unknown id still throws via the existing `unknown(id)` helper. Idempotent by construction — the second `archive(id)` produces an `equals`-identical map and `MutableStateFlow.update` does not re-emit. `lastUsedAt` is deliberately not bumped (archive is a lifecycle transition, not usage); `isPromoted` is not touched (an archived channel routes to `Archived` via the matrix). +- **Mutators** use `state.update { ... }` (atomic CAS, no `Mutex`). `lateinit var` escapes the affected entity out of the lambda so the suspend function can return it. Since #93, `archive(id)` flips the new `Conversation.archived` flag via `record.copy(conversation = record.conversation.copy(archived = true))` instead of removing the entry from the store; unknown id still throws via the existing `unknown(id)` helper. Idempotent by construction — the second `archive(id)` produces an `equals`-identical map and `MutableStateFlow.update` does not re-emit. `lastUsedAt` is deliberately not bumped (archive is a lifecycle transition, not usage); `isPromoted` is not touched (an archived channel routes to `Archived` via the matrix). Since #96, the inverse `unarchive(id)` sits immediately under `archive(id)` (interface line 32, impl block line 135) and shares the same scaffold — `record.copy(conversation = record.conversation.copy(archived = false))`, same `unknown(id)` throw on missing, same free idempotency from `equals`-stable `data class` copy. No precondition check on the current `archived` value; calling `unarchive` on an already-unarchived conversation is a silent no-op. - **Session boundaries:** workspace change is a session-boundary reason per CLAUDE.md. `changeWorkspace` and `startNewSession` share a private `mintNewSession(...)` helper that mints a new `Session`, closes out the previously-active one (`endedAt = now`), and appends the new id to `Conversation.sessionHistory`. - **Seed data:** initial state is three channels (`Pyrycode Mobile`, `Joi Pilates`, `Personal`) plus three discussions (`seed-discussion-a`, `seed-discussion-b`, `seed-discussion-archived`). Channels have stable `seed-channel-*` / `seed-session-*` ids, distinct `cwd`s, and a multi-session message history (one historical session ended days ago + the existing current session, four `User`/`Assistant`-alternating messages per session). Discussions share the scratch sentinel `cwd = "~/.pyrycode/scratch"`, carry `name = null` (auto-named at render time), and ship with `messages = emptyList()`. The third discussion `seed-discussion-archived` has `archived = true` (#93) and a deliberately older `lastUsedAt` so the archived stream is non-empty out of the box without any UI/test setup. All `lastUsedAt` values are fixed `Instant.parse(...)` literals. Built via a private `companion object` (`SEED_RECORDS` / `buildSeedRecords` / `seedChannel` / `seedDiscussion`) so the data is evaluated once at class load; channel histories are authored via `SeedSession` / `SeedMessage` fixture types that the helper expands into real `Session` / `Message` values. Seeds are inserted in source order that is deliberately *not* the sorted order — exercises the sort projection on both tiers. `seedChannel(...)` takes an optional `currentSessionEndedAt: Instant? = null` (#20) — `seed-channel-joi-pilates` opts in so one channel runtime-renders the sleeping indicator without depending on previews. `seedDiscussion(...)` takes an optional `archived: Boolean = false` (#93). See `../codebase/5.md`, `../codebase/6.md`, `../codebase/10.md`, `../codebase/20.md`, `../codebase/93.md`. - **DI binding** landed in #45 alongside the first `ViewModel` consumer (`ChannelListViewModel`): `single { FakeConversationRepository() } bind ConversationRepository::class` in `de/pyryco/mobile/di/AppModule.kt`. Constructor-inject on the interface; Phase 4's remote impl replaces the `single { }` line with no consumer-side change.