feat: drop the frozen icon library, proven by not changing a single golden - #52
Merged
Merged
Conversation
…olden material-icons-extended is frozen at 1.7.8. Google stopped publishing it and material3 1.4.0 stopped depending on material-icons-core transitively, so it was resolving only because the Compose BOM still happened to pin it -- a dependency that would one day break with a missing version rather than a deprecation warning. The 16 icons the app draws are now vector drawables in res/drawable/, converted from the same upstream Material Icons SVGs the Compose artifact is generated from. 30 call sites move from Icons.Default.X to painterResource(R.drawable.ic_x), and the dependency is gone. NOT ONE GOLDEN CHANGED. app/src/test/screenshots/ was recorded from the ImageVectors before this migration and every one of the 16 still matches, byte for byte, rendered from the drawables. That is the entire argument that this is safe, and it is why the goldens were built first. Details that would otherwise have been lost quietly: * ArrowBack was Icons.AutoMirrored, so ic_arrow_back.xml carries android:autoMirrored="true". No golden could have caught dropping it -- they all render LTR. * The converter REFUSES a filled non-path shape rather than approximating it, because a converter that quietly does its best would defeat the gate it feeds. query_stats hit that and was inspected: its <rect> is fill="none", the transparent bounding box, so unfilled shapes of any kind are skipped and only FILLED ones raise. Both directions are controlled. Controls, all failing as required: pointing ic_settings at the close glyph fails verifyRoborazziDebug; a filled <rect> is refused by the converter; a stray Icons. reference no longer even compiles, which is stronger than the test that also guards it. One control PASSED, so it is filed rather than hidden: an orphaned drawable is caught by lint's UnusedResources but only as a WARNING, so nothing fails. Two pre-existing unused strings share that gap. Raising the severity cannot be done in this PR without first resolving those, so it is its own bead. Closes CashPilot-android-vxb
Owner
Author
|
@coderabbitai review |
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe app replaces Compose Material icons with 16 vector drawable resources. Dashboard, settings, and setup screens use drawable painters. The extended Material Icons dependency is removed. Golden tests now validate drawable coverage and reject remaining Material icon references. ChangesIcon migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/test/java/com/cashpilot/android/IconGoldenTest.kt`:
- Around line 173-177: Update the offenders regex in IconGoldenTest so it
rejects any Icons reference, including Icons.AutoMirrored.* and all Compose icon
themes, instead of enumerating only Filled, Outlined, and Default. Keep the
existing source scan and assertion behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e46a41ec-5bf1-4859-9f79-def1b2dbb71e
📒 Files selected for processing (22)
app/build.gradle.ktsapp/src/main/java/com/cashpilot/android/ui/screen/DashboardScreen.ktapp/src/main/java/com/cashpilot/android/ui/screen/SettingsScreen.ktapp/src/main/java/com/cashpilot/android/ui/screen/SetupScreen.ktapp/src/main/res/drawable/ic_arrow_back.xmlapp/src/main/res/drawable/ic_arrow_downward.xmlapp/src/main/res/drawable/ic_arrow_upward.xmlapp/src/main/res/drawable/ic_battery_alert.xmlapp/src/main/res/drawable/ic_check_circle.xmlapp/src/main/res/drawable/ic_chevron_right.xmlapp/src/main/res/drawable/ic_circle.xmlapp/src/main/res/drawable/ic_close.xmlapp/src/main/res/drawable/ic_cloud.xmlapp/src/main/res/drawable/ic_cloud_off.xmlapp/src/main/res/drawable/ic_language.xmlapp/src/main/res/drawable/ic_notifications.xmlapp/src/main/res/drawable/ic_query_stats.xmlapp/src/main/res/drawable/ic_settings.xmlapp/src/main/res/drawable/ic_visibility_off.xmlapp/src/main/res/drawable/ic_warning.xmlapp/src/test/java/com/cashpilot/android/IconGoldenTest.ktgradle/libs.versions.toml
💤 Files with no reviewable changes (1)
- app/build.gradle.kts
Comment on lines
+173
to
+177
| val offenders = Regex("""Icons\.(?:AutoMirrored\.)?(?:Filled|Outlined|Default)\.""") | ||
| .findAll(sourceText()) | ||
| .map { it.value } | ||
| .toSet() | ||
| assert(offenders.isEmpty()) { "the app still draws Material Icons: $offenders" } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import re
pattern = re.compile(r"\bIcons\.")
samples = [
"Icons.Rounded.Settings",
"Icons.Sharp.Warning",
"Icons.TwoTone.Cloud",
"Icons.AutoMirrored.Rounded.ArrowBack",
]
missing = [sample for sample in samples if not pattern.search(sample)]
if missing:
raise SystemExit(f"pattern misses: {missing}")
PY
if rg -nP '\bIcons\.' app/src/main/java --glob '*.kt'; then
echo "Material Icons references remain" >&2
exit 1
fiRepository: GeiserX/CashPilot-android
Length of output: 163
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Target file lines 150-185:\n'
sed -n '150,185p' app/src/test/java/com/cashpilot/android/IconGoldenTest.kt
printf '\nCurrent material icon usages in app sources:\n'
rg -n 'Icons\.|AutoMirrored|Rounded|Sharp|TwoTone' app/src/main --glob '*.kt' || true
printf '\nCurrent test references to material icons guard:\n'
rg -n 'Icons\.(?:AutoMirrored\.)?(?:Filled|Outlined|Default)|sourceText|offenders' app/src/test/java/com/cashpilot/android/IconGoldenTest.kt --glob '*.kt' || trueRepository: GeiserX/CashPilot-android
Length of output: 2407
Expand the Material Icons guard.
Use a single Icons reference check instead of enumerating Filled, Outlined, and Default. This also covers valid Compose themes such as Rounded, Sharp, and TwoTone, including Icons.AutoMirrored.*.
Proposed fixed pattern
- val offenders = Regex("""Icons\.(?:AutoMirrored\.)?(?:Filled|Outlined|Default)\.""")
+ val offenders = Regex("""\bIcons\.""")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val offenders = Regex("""Icons\.(?:AutoMirrored\.)?(?:Filled|Outlined|Default)\.""") | |
| .findAll(sourceText()) | |
| .map { it.value } | |
| .toSet() | |
| assert(offenders.isEmpty()) { "the app still draws Material Icons: $offenders" } | |
| val offenders = Regex("""\bIcons\.""") | |
| .findAll(sourceText()) | |
| .map { it.value } | |
| .toSet() | |
| assert(offenders.isEmpty()) { "the app still draws Material Icons: $offenders" } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/test/java/com/cashpilot/android/IconGoldenTest.kt` around lines 173 -
177, Update the offenders regex in IconGoldenTest so it rejects any Icons
reference, including Icons.AutoMirrored.* and all Compose icon themes, instead
of enumerating only Filled, Outlined, and Default. Keep the existing source scan
and assertion behavior unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes
CashPilot-android-vxb. This is what the golden gate in #51 was built for.Why
androidx.compose.material:material-icons-extendedis frozen at 1.7.8. Google stopped publishing it, and material3 1.4.0 stopped depending on material-icons-core transitively — so it was resolving only because the Compose BOM still happened to pin it. One day a BOM stops, and the build breaks with a missing version, not a deprecation warning.The argument that this is safe
Not one golden changed.
app/src/test/screenshots/was recorded from the ImageVectors, before this migration. All 16 still match — byte for byte — rendered from the drawables.git diff --staton that directory is empty.That is the entire case, and it is why the goldens were built first.
What changed
res/drawable/, converted from the same upstream Material Icons SVGs the Compose artifact is generated from.Icons.Default.XtopainterResource(R.drawable.ic_x).build.gradle.ktsand the version catalog.Two things that would otherwise have been lost quietly
ArrowBackwasIcons.AutoMirrored, soic_arrow_back.xmlcarriesandroid:autoMirrored="true". No golden could have caught dropping this — they all render LTR. It needed noticing by reading, not by testing.query_statshit that and was inspected: its<rect>isfill="none", the transparent bounding box. So unfilled shapes of any kind are skipped and only filled ones raise — and both directions have a control.Controls
ic_settingsat thecloseglyphverifyRoborazziDebugFAILS<rect>alongside a real pathunsupported FILLED shape <rect><rect>alongside a real pathIcons.referenceOne control passed, so it is filed rather than hidden
An orphaned drawable is not caught by anything that fails. Lint's
UnusedResourcesdetects it, but only as a warning:Two pre-existing unused strings (
earnings_shared_detail,earnings_no_reading) share that gap and look like an unwired feature — the more interesting half. Raising the severity can't be done here without resolving those first, so it's its own bead rather than scope creep in a PR whose job is sixteen substitutions that change nothing.Summary by CodeRabbit
UI Improvements
Quality