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
3 changes: 2 additions & 1 deletion .claude/skills/architecture-review/SKILL.md
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 verified at font scale 1.0 and 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 |

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, 10, 11, 12 apply to UI changes; 4, 5 to data/model changes; 8, 9 to Gradle changes; 3 wherever a dependency, singleton, or ViewModel is introduced; 7 wherever a module's dependencies or cross-module imports change. Any rule not listed here is still checked whenever the diff touches its subject. Judge by what the diff touches — don't flag rules a file doesn't engage.

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
17 changes: 17 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,22 @@ 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
orig=$(adb shell settings get system font_scale | tr -d '\r') # "null" if never set
trap 'if [ "$orig" = null ]; then
adb shell settings delete system font_scale
else
adb shell settings put system font_scale "$orig"
fi' EXIT
Comment thread
hal-eisen-adfa marked this conversation as resolved.

adb shell settings put system font_scale 2.0
adb exec-out screencap -p > /tmp/scale-2.0.png
```
Comment thread
hal-eisen-adfa marked this conversation as resolved.

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 @@ -39,6 +55,7 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th
- **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).
- **Don't treat a large binary asset's on-disk content as ground truth without checking its provenance first.** Run `git ls-files <path>` / `git check-ignore -v <path>`, and grep the build files for how it's provisioned, before relying on its current schema or row content. Several assets here (e.g. `assets/documentation.db`, and the SDK/bootstrap/Gradle zips alongside it) are `.gitignore`d and fetched by a Gradle task from an external URL (see the `Asset(...)` list in `app/build.gradle.kts`) — a locally-cached copy can be stale independent of git commit history and silently diverge from the maintained original.
- **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
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 **1.0 and 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 — screenshots at both scales, or one line naming both scales and 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