Merge pull request #4362 from CruGlobal/databindingMigration

Migrate ArticlesFragment from DataBinding to Compose

Author: frett

.claude/skills/pr-review/SKILL.md

@@ -1,95 +1,187 @@
 ---
 name: pr-review
-description: This skill should be used when the user asks to review a pull request, check code quality, run a PR review, or asks "is this PR ready to merge?" for the godtools-android project. It performs a structured review checking project-specific patterns learned from historical PR feedback.
-version: 1.0.0
+description: Review a pull request against GodTools Android project conventions. Use when asked to review a PR, check code quality, or audit changes.
+argument-hint: [pr-number]
+allowed-tools: Bash, Read, Grep, Glob, Write, Edit
 ---
 
-# GodTools Android PR Review
+Review pull request $ARGUMENTS against the GodTools Android project conventions.
 
-Perform a structured pull request review for the godtools-android project. This skill applies patterns learned from historical code reviews and the project's established conventions.
+## Steps
 
-## Review Workflow
+1. Check for dismissed issues by reading `.claude/skills/pr-review/dismissed-issues.md` if it exists.
+   Load all dismissed entries — each has a **Pattern** and **Reason**. You will use these to suppress matching findings later.
 
-### Step 1: Get the Diff
+2. Fetch the PR diff and metadata:
+```
+gh pr diff $ARGUMENTS
+gh pr view $ARGUMENTS
+```
 
-```bash
-# If a PR exists
-gh pr diff
+3. Identify all changed files and categorize them (Kotlin, build scripts, resources, manifests, tests).
 
-# Otherwise compare to base branch
-git diff develop...HEAD --name-only
-git diff develop...HEAD
+4. Pre-flight check — run ktlint. This is a hard blocker:
+```
+./gradlew :build-logic:ktlintCheck ktlintCheck
 ```
+If it fails, report as **Must Fix** before reviewing anything else.
 
-Identify all changed files and categorize them by type (Kotlin, build scripts, resources, manifests, tests).
+5. Review each category using the checklist below.
 
-### Step 2: Pre-Flight Check — ktlint
+6. Before outputting, cross-reference every finding against dismissed patterns. A finding matches a dismissed pattern when it describes the same class of issue (not necessarily the exact file/line — match by concept). Move matched findings to a separate suppressed list.
 
-Always verify ktlint passes. This is a hard blocker:
+7. Output a structured review (format below).
+
+8. After the review output, print:
 
-```bash
-./gradlew :build-logic:ktlintCheck ktlintCheck
 ```
+---
+To dismiss a finding so it won't appear in future reviews, say:
+  dismiss: <short title> — <reason>
+```
+
+---
+
+## Review Checklist
+
+### Build Scripts (`build.gradle.kts`)
+
+- [ ] `godtools.library-conventions` / `godtools.application-conventions` applied — do not re-declare `minSdk`, `compileSdk`, `targetSdk`, Kotlin toolchain, or proguard rules
+- [ ] Compose enabled via `configureCompose(project)` in `android {}` — do not manually add Compose BOM or Circuit deps
+- [ ] No redundant `buildFeatures` flags already covered by the convention plugin
+- [ ] New modules use `godtools.library-conventions`, `godtools.application-conventions`, or `godtools.dynamic-feature-conventions` as appropriate
+- [ ] KSP used for new modules; `kapt` only if DataBinding is still present (see TODO comment pattern)
+
+### `gradle/libs.versions.toml`
+
+- [ ] No duplicate version keys
+- [ ] No unused `[plugins]` entries (common AS wizard artifact)
+- [ ] No unused `[libraries]` entries
+- [ ] All new dependencies declared here, not hardcoded in `build.gradle.kts`
+
+### Android Resources & Manifests
+
+- [ ] No AS wizard scaffolding in library modules: `colors.xml`, `strings.xml` with `app_name`, default launcher icons, per-module theme definitions
+- [ ] Library `AndroidManifest.xml` does not declare `application` attributes (theme, label, icon) — those belong in `:app`
+- [ ] Hardcoded strings extracted to `strings.xml` (inline string literals in composables are flagged)
+- [ ] No `app_name` or generic color/theme resources in library modules
+
+### `settings.gradle.kts`
+
+- [ ] No local `includeBuild` substitutions for `kotlin-mpp-godtools-tool-parser` committed — publish artifact and update version in `libs.versions.toml` instead
+
+### Jetpack Compose
+
+- [ ] Every public or internal composable has `modifier: Modifier = Modifier` parameter
+- [ ] Always uses `GodToolsTheme` from `:ui:base` — no per-module `MaterialTheme(colorScheme = …)` wrappers
+- [ ] Colors accessed via `MaterialTheme.colorScheme.*` or `GodToolsTheme.extendedColorScheme`
+- [ ] CPU-heavy work (bitmap generation, etc.) done off the main thread via `produceState { withContext(Dispatchers.IO) { … } }`
 
-If it fails, report as **Critical** — PR cannot merge until resolved.
+### Circuit Presenter/UI Patterns
 
-### Step 3: Run the Checklist
+**Presenter**
+- [ ] Uses `@AssistedInject` constructor; `Navigator`/`Screen` injected via `@Assisted`
+- [ ] Contains nested `Factory` interface annotated with `@AssistedFactory` and `@CircuitInject(<Screen>::class, SingletonComponent::class)`
+- [ ] `UiState` is a `data class` implementing `CircuitUiState`
+- [ ] `UiEvent` is a `sealed interface` implementing `CircuitUiEvent`, marked `internal`
+- [ ] `UiState` and `UiEvent` defined as nested types inside the Presenter
+- [ ] `UiState` exposes `val eventSink: (UiEvent) -> Unit`
+- [ ] Presenter contains no UI logic — pure state/event handling
 
-For each changed file, apply the relevant checks from `references/patterns.md`. Use the priority levels below to triage findings:
+**UI Composable**
+- [ ] Annotated with `@CircuitInject(<Screen>::class, SingletonComponent::class)`
+- [ ] Signature: `(state: <Presenter>.UiState, modifier: Modifier = Modifier)`
+- [ ] All user interactions delegated via `state.eventSink(UiEvent.*)` — no direct function calls from UI
 
-- **Critical** — Hard blocker, must fix before merge
-- **Important** — Should fix in this PR
-- **Suggestion** — Nice to have, can be deferred
+**Screen**
+- [ ] Annotated with `@Parcelize`
+- [ ] Is an `object` or `data class` implementing `Screen`
 
-### Step 4: PR Hygiene Check
+### Cross-Module Activity / Intent Creation
+
+- [ ] `Intent`/`PendingIntent` creation for Activities uses extension functions in `ui/base/src/main/kotlin/org/cru/godtools/base/ui/Activities.kt` (string class names, no hard compile-time deps between sibling UI modules)
+
+### Repository & DAO Patterns
+
+- [ ] DAOs use `suspend fun` for single-shot queries, `Flow<T>` for reactive queries
+- [ ] `@Upsert` for sync operations
+- [ ] `@RewriteQueriesToDropUnusedColumns` when selecting partial projections
+
+### WorkManager
+
+- [ ] Workers annotated with `@HiltWorker` + `@AssistedInject`
+- [ ] Extend `CoroutineWorker` for suspend support
+- [ ] Catch `IOException` (specific), not `Exception` (broad)
+
+### Kotlin Code Quality
+
+- [ ] Logging uses `Timber` — no `println` or `Log.*`
+- [ ] Exception handling catches specific types, not bare `Exception`
+- [ ] Visibility is intentional: `internal` for module-scoped symbols, `private` where possible
+- [ ] Multi-branch conditionals use `when`, not chained `if/else`
+- [ ] `Bundle`/`Intent` extra keys are `const val` shared between producer and consumer
+
+### Testing
+
+- [ ] Presenter tests use `presenterTestOf { }` (Circuit test API)
+- [ ] Paparazzi tests extend `BasePaparazziTest` from `:ui:base` testFixtures with `@TestParameter` night/accessibility matrix
+- [ ] Snapshots not recorded locally — triggered via GitHub Actions workflow on the feature branch
+
+### PR Hygiene
+
+- [ ] No unrelated auto-formatter whitespace changes (check with `git diff develop...HEAD --stat`)
+- [ ] KMP tool UI changes (app bar, lesson/tract rendering) belong in `kotlin-mpp-godtools-tool-parser`, not Android renderer modules
+
+For detailed examples of each pattern, see `references/patterns.md`.
+
+---
+
+## Output Format
 
-```bash
-git diff develop...HEAD --stat
 ```
+## PR Review: <title> (#<number>)
 
-Look for signs of unrelated auto-formatter changes:
-- Large line-count diffs on files tangentially touched by the PR
-- Whitespace-only changes in unrelated sections
-- Reformatted import blocks or trailing commas not part of the feature
+### Summary
+<1–2 sentence summary of what the PR does>
 
-If found, flag as **Important**: instruct to use git patch-mode staging or scope-selective reformatting in Android Studio.
+### Checklist Findings
 
-### Step 5: Report Findings
+#### ✅ Looks Good
+- <item>
 
-Structure the output as:
+#### ⚠️ Minor Issues
+- <file:line> — <issue> — <suggested fix>
 
+#### ❌ Must Fix
+- <file:line> — <issue> — <suggested fix>
+
+#### ⏭️ Suppressed
+- <short title> — dismissed: <reason>
+(omit this section entirely if nothing was suppressed)
+
+### Overall Verdict
+APPROVE / REQUEST CHANGES / COMMENT
+<brief rationale>
 ```
-## PR Review: <branch or PR title>
 
-### Critical Issues (must fix before merge)
-- [FILE:LINE] Issue description
+Be specific. Reference file paths and line numbers. Cite the relevant convention when flagging an issue.
+
+---
+
+## Handling Dismissals
 
-### Important Issues (should fix)
-- [FILE:LINE] Issue description
+When the user says `dismiss: <title> — <reason>` (in any form — "dismiss the X issue because Y", etc.):
 
-### Suggestions
-- [FILE:LINE] Suggestion
+1. Read `.claude/skills/pr-review/dismissed-issues.md` if it exists (create it if not).
+2. Run `git config user.name` to get the current user's name.
+3. Append a new entry in this format:
 
-### Looks Good
-- What's well done in this PR
+```markdown
+## <title>
+**Pattern**: <describe the class of issue broadly enough to match future occurrences>
+**Reason**: <reason the user gave>
+**Dismissed**: <today's date as YYYY-MM-DD>
+**Dismissed by**: <git user.name>
 ```
 
-## Quick Reference: Top Issues from Historical Reviews
-
-These are the patterns most frequently flagged in past reviews. Check these first:
-
-1. **ktlint failures** — Run check before reporting anything else.
-2. **AS wizard scaffolding left in** — Delete generated `colors.xml`, `strings.xml` with `app_name`, default launcher icons, and per-module theme definitions from library modules.
-3. **Unrelated auto-formatter changes** — Flag if diff contains formatting-only changes in untouched sections.
-4. **Convention plugin boilerplate in `build.gradle.kts`** — `godtools.library-conventions` already provides minSdk, namespace (via `android {}` block), Kotlin toolchain, proguard, and Compose setup. Don't re-declare them.
-5. **`libs.versions.toml` clutter** — No duplicate version entries, no unused plugin declarations.
-6. **Composables missing `modifier: Modifier = Modifier`** — Required on all public/internal composables.
-7. **Wrong theme** — Use `GodToolsTheme` from `:ui:base`, never define a per-module theme.
-8. **Cross-module Activity intents** — Intent/PendingIntent creation for Activities must use extension functions in `ui/base/src/main/kotlin/org/cru/godtools/base/ui/Activities.kt`.
-9. **Circuit actions as direct calls** — User-triggered actions must flow through named `UiEvent` entries in `UiState.eventSink`, not direct function calls from the UI composable.
-10. **Local `includeBuild` in settings.gradle.kts** — Never commit local KMP project substitutions. Publish the KMP artifact and update the version in `libs.versions.toml` instead.
-11. **`println` for logging** — Use `Timber` instead.
-12. **Overly broad exception catching** — Catch specific exception types, not `Exception`.
-13. **Visibility too wide** — Prefer `internal` for module-scoped composables/functions, `private` where possible.
-
-For full pattern details, consult `references/patterns.md`.
+4. Confirm to the user what was added and that it will be suppressed in future reviews.

.claude/skills/pr-review/dismissed-issues.md

@@ -0,0 +1,5 @@
+# Dismissed Review Issues
+
+Issues listed here are suppressed in future PR reviews.
+
+---

gradle/libs.versions.toml

@@ -95,6 +95,7 @@ circuitx-gesture-navigation = { module = "com.slack.circuit:circuitx-gesture-nav
 coil-compose = { module = "io.coil-kt:coil-compose", version.ref = "coil" }
 coil-test = { module = "io.coil-kt:coil-test", version.ref = "coil" }
 colormath-android-colorint = { module = "com.github.ajalt.colormath:colormath-ext-android-colorint", version.ref = "colormath" }
+colormath-jetpack-compose = { module = "com.github.ajalt.colormath:colormath-ext-jetpack-compose", version.ref = "colormath" }
 compose-reorderable = "org.burnoutcrew.composereorderable:reorderable:0.9.6"
 dagger = { module = "com.google.dagger:dagger", version.ref = "dagger" }
 dagger-compiler = { module = "com.google.dagger:dagger-compiler", version.ref = "dagger" }
@@ -160,6 +161,7 @@ gtoSupport-play-auth = { module = "org.ccci.gto.android:gto-support-play-auth",
 gtoSupport-retrofit2 = { module = "org.ccci.gto.android:gto-support-retrofit2", version.ref = "gtoSupport" }
 gtoSupport-scarlet = { module = "org.ccci.gto.android:gto-support-scarlet", version.ref = "gtoSupport" }
 gtoSupport-scarlet-actioncable = { module = "org.ccci.gto.android:gto-support-scarlet-actioncable", version.ref = "gtoSupport" }
+gtoSupport-sync = { module = "org.ccci.gto.android:gto-support-sync", version.ref = "gtoSupport" }
 gtoSupport-testing-dagger = { module = "org.ccci.gto.android.testing:gto-support-dagger", version.ref = "gtoSupport" }
 gtoSupport-testing-picasso = { module = "org.ccci.gto.android.testing:gto-support-picasso", version.ref = "gtoSupport" }
 gtoSupport-testing-timber = { module = "org.ccci.gto.android.testing:gto-support-timber", version.ref = "gtoSupport" }

ui/article-aem-renderer/src/main/kotlin/org/cru/godtools/article/aem/db/ArticleDao.kt

@@ -8,6 +8,7 @@ import androidx.room.Insert
 import androidx.room.OnConflictStrategy
 import androidx.room.Query
 import java.util.Locale
+import kotlinx.coroutines.flow.Flow
 import org.cru.godtools.article.aem.model.Article
 
 private const val GET_ARTICLES_FROM = """
@@ -38,7 +39,7 @@ interface ArticleDao {
         ORDER BY a.title
         """
     )
-    fun getArticles(tool: String, locale: Locale): LiveData<List<Article>>
+    fun getArticlesFlow(tool: String, locale: Locale): Flow<List<Article>>
     @AnyThread
     @Query(
         """
@@ -50,7 +51,7 @@ interface ArticleDao {
         ORDER BY a.title
         """
     )
-    fun getArticles(tool: String, locale: Locale, tags: List<@JvmSuppressWildcards String>): LiveData<List<Article>>
+    fun getArticlesFlow(tool: String, locale: Locale, tags: List<@JvmSuppressWildcards String>): Flow<List<Article>>
 
     @Insert(onConflict = OnConflictStrategy.IGNORE)
     suspend fun insertOrIgnore(article: Article)

ui/article-renderer/build.gradle.kts

@@ -7,6 +7,7 @@ plugins {
 android {
     namespace = "org.cru.godtools.tool.article"
 
+    configureCompose(project)
     configureQaBuildType(project)
     configureGodToolsCustomUri()
 
@@ -21,19 +22,22 @@ dependencies {
     implementation(projects.library.base)
     implementation(projects.ui.baseTool)
 
+    implementation(libs.androidx.compose.material3)
     implementation(libs.androidx.fragment.ktx)
     implementation(libs.androidx.lifecycle.livedata.ktx)
     implementation(libs.androidx.constraintlayout)
     implementation(libs.androidx.recyclerview)
-    implementation(libs.androidx.swiperefreshlayout)
 
     implementation(libs.gtoSupport.androidx.fragment)
     implementation(libs.gtoSupport.androidx.lifecycle)
     implementation(libs.gtoSupport.androidx.recyclerview)
     implementation(libs.gtoSupport.core)
+    implementation(libs.gtoSupport.kotlin.coroutines)
     implementation(libs.gtoSupport.picasso)
+    implementation(libs.gtoSupport.sync)
     implementation(libs.gtoSupport.util)
 
+    implementation(libs.colormath.jetpack.compose)
     implementation(libs.dagger)
     implementation(libs.hilt)
     implementation(libs.splitties.fragmentargs)