Skip to content

ADFA-5098: Require font-scale verification for new and changed screens #1657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hal-eisen-adfa wants to merge 1 commit into
base: stage
Choose a base branch
from
+26 −3
Open

ADFA-5098: Require font-scale verification for new and changed screens #1657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion .claude/skills/architecture-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,9 @@ For each changed first-party file, check the applicable rules. Each rule cites i
| 9 | **Dependency substitution:** don't add a Maven coordinate for something already vendored/substituted (`build-deps*`); don't add a new dependency without checking `gradle/libs.versions.toml` first. | ADR 0003 |
| 10 | **Strings** live in the `:resources` module's `strings.xml` (not per-module, not inline literals). | REVIEW.md §7 |
| 11 | **UI never drawn over the two system bars** (top status bar, bottom navigation bar). | CLAUDE.md |
| 12 | **Text scales:** new/changed screens hold up at font scale 2.0 — `sp` for text and `dp` for spacing (no `sp` dimen used as margin/padding), no text boxed in a fixed `dp` size, a scroll container on content that can grow, and `maxLines`/`singleLine`/`ellipsize` only on genuinely disposable text. | CLAUDE.md, REVIEW.md §8 |

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Keep the font-scale requirement consistent across all review guidance.

The project contract requires verification at 1.0 and 2.0, but these enforcement points allow a 2.0-only check.

  • .claude/skills/architecture-review/SKILL.md#L59-L59: require verification at font scales 1.0 and 2.0.
  • REVIEW.md#L43-L43: change the checklist item to require 1.0 and 2.0.
  • REVIEW.md#L165-L165: require evidence that names both scales or shows screenshots at both scales.
🧰 Tools
🪛 SkillSpector (2.5.1)

[warning] 56: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.

Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.

(Excessive Agency (EA2))

📍 Affects 2 files
  • .claude/skills/architecture-review/SKILL.md#L59-L59 (this comment)
  • REVIEW.md#L43-L43
  • REVIEW.md#L165-L165
🤖 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 @.claude/skills/architecture-review/SKILL.md at line 59, Update the
font-scale guidance consistently across all sites: in
.claude/skills/architecture-review/SKILL.md lines 59-59, require verification at
both 1.0 and 2.0; in REVIEW.md lines 43-43, change the checklist to require both
scales; and in REVIEW.md lines 165-165, require evidence naming both scales or
showing screenshots at both scales.


Rules 1, 2, 6 apply to UI changes; 4, 5 to data/model changes; 8, 9 to Gradle changes. Judge by what the diff touches — don't flag rules a file doesn't engage.
Rules 1, 2, 6, 11, 12 apply to UI changes; 4, 5 to data/model changes; 8, 9 to Gradle changes. Judge by what the diff touches — don't flag rules a file doesn't engage.

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Complete the applicability mapping.

Rule 10 is defined above but is not assigned to UI changes. A UI review can therefore skip the centralized strings.xml rule. Rules 3 and 7 also have no applicability entry. Add mappings for all rules, or state that unlisted rules are checked when the diff touches their subject.

🧰 Tools
🪛 SkillSpector (2.5.1)

[warning] 56: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.

Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.

(Excessive Agency (EA2))

🤖 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 @.claude/skills/architecture-review/SKILL.md at line 61, Complete the
applicability mapping in the architecture-review skill so every defined rule,
including Rules 3, 7, and 10, has an explicit scope. Assign Rule 10 to UI
changes and add appropriate mappings for Rules 3 and 7, or explicitly state that
unlisted rules are checked whenever the diff touches their subject.


For a **large diff (~15+ first-party files)**, fan out: spawn a subagent per dimension (UI/state, DI, persistence, Gradle/modules), each instructed to read the relevant ADR and report only its dimension's findings; then merge. For a small diff, do it inline.

Expand Down
12 changes: 12 additions & 0 deletions CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ Every module carries `v7` (`armeabi-v7a`) and `v8` (`arm64-v8a`) flavors, so bui

At least one Android emulator or device is available. Find it with `adb devices -l | grep -v offline`, then target it with the `ANDROID_SERIAL` env var. Note the app is **arm-only** (`v7`/`v8` flavors, no x86) — an x86_64 emulator can't run it (not always even via a translation layer), so testing often needs a **physical arm device** or an arm-translation emulator.

**Font-scale check.** Read the current value first so you can put it back. Each change recreates the activity (only `EditorActivityKt` declares `fontScale` in `configChanges`), so this doubles as a state-restoration test:

```bash
adb shell settings get system font_scale # save it (prints "null" if never set)
adb shell settings put system font_scale 2.0
adb exec-out screencap -p > /tmp/scale-2.0.png
adb shell settings put system font_scale 1.0 # restore
```
Comment on lines +32 to +39

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Restore the original font scale.

The text says to save the current value, but the commands never save it and always write 1.0. This overwrites users' existing settings. If the value is null, delete the setting instead of writing 1.0. Capture the value and restore it with a shell trap.

Proposed shell update 
- adb shell settings get system font_scale          # save it (prints "null" if never set)
+ original_scale="$(adb shell settings get system font_scale | tr -d '\r')"
+ restore_font_scale() {
+   if [ "$original_scale" = "null" ]; then
+     adb shell settings delete system font_scale
+   else
+     adb shell settings put system font_scale "$original_scale"
+   fi
+ }
+ trap restore_font_scale EXIT
  adb shell settings put system font_scale 2.0
  adb exec-out screencap -p > /tmp/scale-2.0.png
- adb shell settings put system font_scale 1.0      # restore
📝 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
**Font-scale check.** Read the current value first so you can put it back. Each change recreates the activity (only `EditorActivityKt` declares `fontScale` in `configChanges`), so this doubles as a state-restoration test:
```bash
adb shell settings get system font_scale # save it (prints "null" if never set)
adb shell settings put system font_scale 2.0
adb exec-out screencap -p > /tmp/scale-2.0.png
adb shell settings put system font_scale 1.0 # restore
```
**Font-scale check.** Read the current value first so you can put it back. Each change recreates the activity (only `EditorActivityKt` declares `fontScale` in `configChanges`), so this doubles as a state-restoration test:
🤖 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 `@CLAUDE.md` around lines 32 - 39, Update the “Font-scale check” commands in
CLAUDE.md to capture the current font_scale value, install a shell trap that
restores that exact value on exit, and delete the setting when the captured
value is null; remove the hard-coded 1.0 restoration while preserving the 2.0
screenshot step.


At 2.0, look for text cut off mid-word, labels overrunning their control, actions pushed off the bottom with no way to scroll to them, and overlapping rows.

## Architecture

See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for the module map, layering/data flow, dependency rules, tech stack (DI, async, persistence, networking), state management, and testing strategy. Don't re-document those here; update ARCHITECTURE.md.
Expand All @@ -38,6 +49,7 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th
- **Avoid new dependencies** — the build almost certainly already has what's needed. Check `gradle/libs.versions.toml` and `build.gradle.kts` first.
- **Persistence:** prefer **Room** for relational data and the filesystem/preferences for settings; raw SQLite only for justified exceptions — see [ADR 0001](docs/adr/0001-prefer-room-for-persistence.md).
- **Protect the two Android system bars** in any UI work: the top status bar (clock, notifications, status icons) and the bottom navigation bar (home, back, recents). Don't draw over or intercept them.
- **Every screen must survive 2x font scale.** Users with low vision run large system fonts, and a screen that clips or hides content at 2.0 is broken for them. Verify any new or changed screen at font scale **1.0 and 2.0** (see Build & test, Emulator / device) and say in the PR that you did. Text grows, so: use `sp` for text and `dp` for spacing — never an `sp` dimen as a margin or padding; don't box text in a fixed `dp` height or width; give content that can grow somewhere to scroll; and reserve `maxLines`/`singleLine`/`ellipsize` for text that is genuinely disposable.
- **Plan and size before building.** Prefer **one PR per ticket/use case** — don't force-split a coherent change (splitting has its own overhead when later edits span the pieces). When a change is large, break it into **reviewable commits** — mechanical/refactor commits separate from behavioral ones — and offer review-by-commit. Treat ~500 LOC / ~10 files as a signal to reach for that commit structure, not a hard cap; the ceiling rises as LLM-assisted review matures. For staged multi-commit refactors (e.g. removing a dependency across many files/modules), order stages easiest-to-hardest and independently compile/test each stage (see Build & test's fast-iteration guidance) before moving to the next, so a failure is isolated to the stage that caused it.
- **Keep docs in step with code.** When you change code, update the docs that describe it in the same change — a module's `README.md`, `ARCHITECTURE.md`, or an ADR — so a doc never outlives the API it documents (see REVIEW.md, Code quality). If the doc fix is out of scope, file a ticket rather than let it drift.
- `.androidide_root` is a sentinel file tests use to locate the project root — don't delete it.
Expand Down
14 changes: 12 additions & 2 deletions REVIEW.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ A review isn't done because it *looks* fine; it's done when you can **show what
| §4 Security | Which untrusted inputs were validated; secrets checked |
| §5 Tests & coverage | JaCoCo numbers for new non-UI code (line & branch) |
| §7 Code quality | Duplication/cohesion pass done; no reimplementation of existing helpers |
| §8–§9 A11y & help | contentDescription + long-press help on new interactive elements |
| §8–§9 A11y & help | contentDescription + long-press help on new interactive elements; font scale 1.0/2.0 verified on new or changed screens |
| §10 Architecture | Checklist below, each item pass/fail |
| §13 Plugins | API-surface touched? impact check result |

Expand All @@ -40,6 +40,7 @@ Keep it proportional — a two-line change needs a two-line ledger.
- [ ] **Docs:** public classes/functions have KDoc/Javadoc explaining *why*, not *what*; any module `README`/`ARCHITECTURE.md`/ADR the change affects is updated in the same PR.
- [ ] **Strings** are in the **`:resources`** module's `strings.xml` (not per-module, not inline literals) — keeps localization centralized.
- [ ] **Accessibility:** every actionable view has a `contentDescription` (XML *or* programmatic); decorative views are marked `importantForAccessibility="no"`.
- [ ] **Font scale:** new or changed screens verified at **2.0** — nothing clipped, nothing unreachable — or explicitly noted as not applicable.
- [ ] **Contextual help:** new interactive elements (and any new screen/panel) have long-press help wired to the 3-tier tooltip system.
- [ ] **Analytics:** meaningful user/build actions emit an event (see below).
- [ ] **Scope/size:** PR is focused on one ticket/use case; if large, it's split into **reviewable commits** (mechanical separate from behavioral) rather than force-split into multiple PRs (`CLAUDE.md`).
Expand Down Expand Up @@ -142,7 +143,7 @@ Keep event names/params stable and low-cardinality; **no PII, file paths with us
- **Strings in `strings.xml`.** User-facing text must be a string resource, never an inline literal — lint flags `HardcodedText`, and externalized strings feed our Crowdin translation flow. Use plurals/`getQuantityString` and positional args for formatting. Log messages and analytics keys are *not* user-facing and stay in code.
- **Dependencies:** don't add one without checking `gradle/libs.versions.toml` first — we probably already have it (`CLAUDE.md`).

## 8. Accessibility — every actionable view speaks
## 8. Accessibility — every actionable view speaks, and every screen scales

CoGo serves visually-impaired developers, so TalkBack support is a correctness requirement, not a nice-to-have (pattern set by ADFA-2667). New UI is Compose ([§10](#10-architecture-alignment) / [ADR 0009](docs/adr/0009-jetpack-compose-for-new-ui.md)), so each rule gives the View and Compose form — the requirement is the same in either.

Expand All @@ -159,6 +160,15 @@ CoGo serves visually-impaired developers, so TalkBack support is a correctness r
- **Externalize, with the `cd_` convention.** Content descriptions live in `strings.xml` as `cd_*` — greppable, translatable, reusable; check for an existing one first. `HardcodedText` lint does **not** catch Compose literals, so reviewers must.
- **Bonus — it stabilizes tests.** Screen-reader semantics are what UI tests match on (`ACTION_CLICK` for Views, `onNodeWithContentDescription(…)` for Compose), so a11y and reliable instrumentation tests are the same work.

**Text scales, so layouts must too.** Low vision means large system fonts as often as it means TalkBack. A screen isn't done until it works at **2x**.

- **Verify new or changed screens at font scale 1.0 and 2.0**, and put the result in the PR — a screenshot at 2.0, or one line saying what you checked. Recipe in `CLAUDE.md` (Build & test → Emulator / device). "No visual change" or "no text on this surface" is a valid one-line opt-out; silence is not.
- **Spacing in `dp`, text in `sp`.** An `sp` dimension used as a margin or padding grows with the font scale and squeezes the text it was meant to frame — as `layout-land/fragment_onboarding_greeting.xml` does today with `@dimen/_32sp`.
- **Don't box text in a fixed size.** A control sized `40dp x 40dp` can't hold a label that doubled. Let the container wrap its content and set a `minWidth`/`minHeight` for the touch target instead of a fixed one.
- *Compose:* the same trap is `Modifier.height(44.dp)`/`.size(36.dp)` on chrome that contains text — use `defaultMinSize` and let it grow.
- **Give growth somewhere to go.** Content that can reflow past the viewport needs a `NestedScrollView` (Compose: `verticalScroll`/`LazyColumn`). Only 14 of the 108 layouts in `app/src/main/res/layout/` have one today — don't add to the pile.
- **`maxLines`/`singleLine`/`ellipsize` are a decision, not a default.** Clamping is fine for a preview line, wrong for anything the user must read to proceed. `ellipsize="none"` with `maxLines` clips mid-glyph and is almost never what you want.

## 9. Contextual help — long-press works everywhere

Help in CoGo is reached by **long-press**, anywhere: a progressive three-tier experience — **Tiers 1 & 2 are tooltips** (anchored popups from `idetooltips`), **Tier 3 is a full help web page** via the tooltip's "See More" link. A long-press should never be met with silence.
Expand Down
Loading