Reviewing an Android change

The Kotlin course's review lesson covered the human side of review: correctness before style, comments about the code and never the person, and what to do when somebody misreads your work. This lesson is about what is different when the change is to an Android app. The build passing and the tests going green tell you less than they do in a command-line program, because the bugs that hurt Android users mostly live where tests do not look: an upgrade from last month's version, a process that died in the background, a button TalkBack cannot reach. So the lesson reviews one real, deliberately flawed change to Mandi Bhav, kept in a scratch branch and never merged, and runs every finding before writing it down.

The change

Arjun's pull request: "Let farmers save their favourite prices." A Save button on each card; saved prices sort to the top of the list. The summary:

 .../7.json                                         | 251 +++++++++++++++++++++
 .../mandibhav/TestPriceRepositoryModule.kt         |   2 +
 .../mandibhav/data/PriceRepository.kt              |   2 +
 .../mandibhav/data/RoomPriceRepository.kt          |   2 +
 .../mandibhav/data/local/MandiBhavDatabase.kt      |   2 +-
 .../mandibhav/data/local/PriceDao.kt               |   3 +
 .../mandibhav/data/local/PriceEntity.kt            |   3 +
 .../mandibhav/model/CommodityPrice.kt              |   1 +
 .../com/riztechacademy/mandibhav/ui/PriceCard.kt   |  12 +-
 .../riztechacademy/mandibhav/ui/PricesScreen.kt    |   8 +-
 .../riztechacademy/mandibhav/ui/PricesViewModel.kt |   4 +
 .../mandibhav/data/FakePriceRepository.kt          |   2 +
 12 files changed, 287 insertions(+), 5 deletions(-)

The heart of it, in the order the data flows:

// PriceEntity.kt and CommodityPrice.kt
val isFavourite: Boolean = false,

// MandiBhavDatabase.kt
version = 7,

// PriceDao.kt
@Query("UPDATE prices SET isFavourite = :favourite WHERE id = :id")
suspend fun setFavourite(id: String, favourite: Boolean)

// PricesViewModel.kt
fun toggleFavourite(price: CommodityPrice) {
    viewModelScope.launch { repository.setFavourite(price.id, !price.isFavourite) }
}

// PricesScreen.kt
// Favourites first.
val shown = uiState.prices.sortedByDescending { it.isFavourite }

// PriceCard.kt, inside the card's Row
if (onFavouriteClick != null) {
    androidx.compose.material3.TextButton(onClick = onFavouriteClick) {
        Text(if (price.isFavourite) "Saved" else "Save")
    }
}

// FakePriceRepository.kt
override suspend fun setFavourite(id: String, favourite: Boolean) {}

What the machines said

Before reading a line, see what the tools already know:

Everything is green. Every one of the problems below was still in the change.

Read an Android diff in the order the data flows

A diff tool lists files alphabetically, which here means a test module first and the card's button in the middle. Read it in the order a value travels instead, because a mistake early in that order makes everything after it wrong:

1. Build files and the manifest. New dependencies, permissions, minSdk, R8 rules. (None here.)
2. Schema. Entities, the database version, migrations, the exported schema file.
3. Data. DAO, repository, network: where the value is written, and what else writes the same rows.
4. State. The ViewModel and the UI state.
5. UI. Composables, semantics, strings.
6. Tests. Would any of them fail if the change were wrong?

What the review found

1. The database version went up with no migration (blocking)

version = 7 and a new column, but no AutoMigration(from = 6, to = 7). A fresh install has no old database, so it works; that is the only case the author tried. The scratch build of the app as it is today was installed and opened, so it had a version 6 database with prices in it, and then the change was installed over it:

E AndroidRuntime: FATAL EXCEPTION: main
E AndroidRuntime: java.lang.IllegalStateException: A migration from 6 to 7 was required but not found. Please provide the necessary Migration path via RoomDatabase.Builder.addMigration(...) or allow for destructive migrations via one of the RoomDatabase.Builder.fallbackToDestructiveMigration* functions.

That is every existing user, on the day the update arrives, crashing on launch. Module 8's migration tests would not have caught it either, because nobody added a 6-to-7 case. The fix is one line in autoMigrations, plus a test in MigrationTest; the error message's other suggestion, a destructive migration, would "fix" the crash by deleting every farmer's saved prices and alerts.

2. A refresh wipes the favourite (blocking)

Read the data layer for every place that writes prices rows, not only the new one. refresh() saves a download with upsertPrices(latest.prices.map { it.asEntity(fetchedAt = now) }), and a price from the network has isFavourite = false. An upsert replaces the whole row. A test against the real repository and an in-memory database:

repository.refresh()
repository.setFavourite(SamplePrices.lasalgaonOnion.id, true)
assertTrue("starred", repository.price(SamplePrices.lasalgaonOnion.id).first()!!.isFavourite)

repository.refresh()

assertTrue("still starred after a refresh", repository.price(SamplePrices.lasalgaonOnion.id).first()!!.isFavourite)

java.lang.AssertionError: still starred after a refresh

The first assertion passed; the favourite was saved. The next refresh, the one the daily worker runs every evening, unsaved it.

3. The favourite belongs to the wrong thing (blocking, found by reading)

Even with the refresh fixed, the flag is on a row of prices, and a row is one day's price: its id carries the date, and module 9's saveDownload moves yesterday's row into price_history and deletes it when a new day arrives. A farmer who saves "onion at Lasalgaon" means the market and commodity, every day. This one was found by reading saveDownload's comment rather than by a run; the run in finding 2 already showed the flag does not survive the data layer, and the design question is bigger than that bug. A favourite needs its own small table keyed by market and commodity, as alerts have. That is a conversation, not a comment, and it should happen before anyone fixes findings 1 and 2.

4. TalkBack cannot reach the button (blocking)

PriceCard uses clearAndSetSemantics so that TalkBack reads a card as one sentence (module 4). That modifier removes everything inside the card from the accessibility tree, including the new button. The fresh install showed it: UI Automator, which reads the same tree as TalkBack, found the cards and no "Save" anywhere. A Compose test said why:

Failed: assertExists.
Reason: Expected exactly '1' node but could not find any node that satisfies: (Text + InputText + EditableText contains 'Save' (ignoreCase: false))
However, the unmerged tree contains '1' node that matches. Are you missing `useUnmergedNode = true` in your finder?

The button exists and is drawn; a blind farmer cannot press it. The card needs a custom accessibility action for saving, beside its existing click action, or the button needs to be outside the cleared node.

5. Sorting in the composable (should fix)

val shown = uiState.prices.sortedByDescending { it.isFavourite } runs in PricesScreen's body, on every recomposition, including every keystroke in the search. It is a rule ("favourites first") with no test, in the one place lesson 2 said rules do not belong, and it interacts with homeMarketFirst in the ViewModel in a way nobody decided: which comes first, the home market or a favourite? It belongs beside homeMarketFirst, with a test that answers that question.

6. A fake that does nothing (should fix)

FakePriceRepository.setFavourite is empty, so no ViewModel test can ever see a favourite. The 206 green tests are green partly because nothing tests the feature. A fake that ignores a write is how a test suite stays green while a feature is broken.

7. Checked, and fine

A review should also say what was checked and found correct, so the author knows it was looked at. The favourite never leaves the phone, so PRIVACY.md, privacySummary and the Data safety form (module 14's rule) do not change. No new permission, dependency or R8 rule. The strings "Save" and "Saved" are hard-coded, as all of Mandi Bhav's are; translation is a separate decision, not this change's problem.

Writing it up

Label each comment so the author knows what blocks the merge. The review for this change, in full:

blocking: MandiBhavDatabase goes to version 7 without a migration. Installing this over
today's build crashes on launch ("A migration from 6 to 7 was required"). Please add
AutoMigration(6, 7) and a case in MigrationTest. Not fallbackToDestructiveMigration:
that deletes every user's alerts.

blocking: refresh() upserts whole rows with isFavourite = false, so the evening refresh
unsaves everything. Test attached that fails on this branch.

question (blocking until answered): a favourite is on today's price row, which is
deleted when tomorrow's arrives. Should it be keyed by market and commodity in its own
table, like alerts? Happy to pair on the design before you fix the two above.

blocking: the Save button is inside clearAndSetSemantics, so TalkBack and UI Automator
cannot see it. A customActions entry on the card would keep the one-sentence reading.

should fix: favourites-first sorting is in PricesScreen's body. Could it live next to
homeMarketFirst, with a test that decides which comes first?

should fix: FakePriceRepository.setFavourite does nothing, so no test can see the feature.

checked: no privacy, permission or R8 impact; local only.

Notice what is not in it: the variable names, the import style, the fully qualified androidx.compose.material3.TextButton. Those are worth a nit: on a change that is otherwise ready. On a change with four blocking problems they are noise that buries the four.

Review your own diff first

Every finding above is one the author could have found alone, in ten minutes, with this checklist:

A diff is easiest to review before anyone else has seen it. When the reviewer misreads your code, that is a fact about the code: answer by changing the code or adding the comment, not by explaining in the thread.

Check your work

Why did a fresh install of the change work when an upgrade crashed? A fresh install creates a version 7 database directly; an upgrade finds version 6 and needs a migration that was not there.

Why is fallbackToDestructiveMigration the wrong fix for that crash? It deletes the database to rebuild it, so every farmer loses saved prices, alerts and sale slips.

What does the evening refresh do to a favourite, and why? Clears it: refresh() upserts whole rows from the network, where isFavourite is false.

Why could UI Automator not find the Save button? clearAndSetSemantics on the card removes the card's children from the merged accessibility tree, which both TalkBack and UI Automator read.

Why did 206 tests pass? None of them exercised the feature, and the fake repository ignored setFavourite.

In what order should an Android diff be read? Build files and manifest, schema, data, state, UI, tests: the order data flows.

Practice

1. Make the change in a scratch copy with its own applicationIdSuffix: the lines above, plus passing onFavouriteClick from PricesRoute through PricesScreen to each PriceCard. Reproduce findings 1, 2 and 4. Uninstall the scratch app afterwards.
2. Fix finding 1 with AutoMigration(from = 6, to = 7) and write the MigrationTest case. Install the old build, then the fixed one, and confirm that the saved prices survive.
3. Write the design for finding 3: the table, its key, its DAO, and what the Prices screen's state gains. Compare it with how price_alerts is stored.
4. Break it on purpose: add a customActions entry to PriceCard for saving, then turn on TalkBack on the emulator and save a price without touching the screen except to swipe and double-tap.
5. Harder: take Mandi Bhav's module 15 commit (git show on the checkpoint) and review it as if somebody else wrote it, using the checklist. It renames a function on the startup path; find what that leaves stale, and say whether it should block.

Official documentation

• How to do a code review — Google's guide for reviewers: what to look for, in what order, and how to write comments.
• Migrate your Room database — Automated and manual migrations, and what a destructive migration costs.
• Semantics — Merged and unmerged trees, clearAndSetSemantics, and custom actions.
• Improve your code with lint checks — What lint can catch before a reviewer does, and how to add your own checks.

Next: the capstone, a medicine reminder app.

---

That is module 15. You can now:

• follow one concept from the JSON to the screen and keep its name the same, change a name only when its meaning changes, and know which renames in an Android app are really schema, saved-state or profile changes;
• decide whether a piece of logic belongs in a composable, a ViewModel, a use case or the repository, and recognise logic that gives a right answer only with the data you tested with;
• run five interruptions against any screen: process death, configuration changes, the background, the main thread and incomplete intents, and fix what they find without hiding it behind a guard;
• read the Compose compiler's report as a list of suspects, count recompositions before changing anything, and defer fast-changing reads to the phase that needs them;
• review an Android change in the order its data flows, run every finding before writing it, and label what blocks the merge.

Mandi Bhav now names its fetch time and its alert prices for what they are, says what its refresh really throws, and keeps its slow-connection note across a rotation. The module's real finding is how little needed to change after fourteen modules of building it carefully; most of what this module showed was planted in scratch copies to be found. The capstone, a medicine reminder app, is where you build an app from an empty project to Google Play on your own, to the standard these five lessons set.