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
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ interface ConversationRepository {

suspend fun archive(conversationId: String)

suspend fun unarchive(conversationId: String)

suspend fun rename(
conversationId: String,
name: String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion docs/knowledge/INDEX.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 entrythe 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<Preferences>` (file `app_prefs`); typed `Flow<T>` reads + `suspend` setters for non-secret app state. Keys: `pairedServerExists` (#11/#12/#13), `themeMode: Flow<ThemeMode>` (#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.
Expand Down
40 changes: 40 additions & 0 deletions docs/knowledge/codebase/96.md
Original file line number Diff line number Diff line change
@@ -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.
Loading
Loading