Skip to content
Open
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
18 changes: 18 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ flox activate -d flox/local -- ./gradlew <task>
- **Single unit test:** `flox activate -d flox/local -- ./gradlew :module:test --tests "com.itsaky.androidide.SomeTest"`
- **Module unit tests:** `flox activate -d flox/local -- ./gradlew :testing:unit:test`
- **Fast iteration:** during multi-file/multi-module changes, verify with targeted `:module:compileV8DebugKotlin`/`compileV8DebugJavaWithJavac` invocations (batch several modules into one Gradle call) rather than a full assemble. Reserve `:app:assembleV8Debug` for final end-to-end verification — it's slow (multi-minute) in this multi-module project, and running it after every small change adds up.
- **Long-running commands: narrate them, and background only the read-only ones.** Anything that can exceed ~60s — `assembleV8Debug`, a full test sweep, a cold Gradle invocation of any kind (daemon start plus configuring this many modules is ~20s before any task runs, and far worse under memory pressure), and `git push`, which runs Spotless through the hook — gets a line before it starts saying what is running and roughly how long it takes, and a status line every couple of minutes while it runs: elapsed time, the last output line, whether it is still progressing. A silent terminal is indistinguishable from a hang, and saying which it is is your job, not the user's to ask.

Background a command only if it does **not** write to the worktree. A build, a test run, or `spotlessCheck` is safe to background. `spotlessApply` and `git push` (which runs it through the hook) rewrite tracked files, so editing or staging while one is in flight races it — keep those in the foreground and narrate the wait instead.

When the user names a CI/CD job ("the sonar job", "the analyze workflow"), read `.github/workflows/*.yml` — the YAML is the authoritative gradle/shell invocation. Don't reverse-engineer it from gradle tasks or build files.

Expand Down Expand Up @@ -61,12 +64,23 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th
- `.androidide_root` is a sentinel file tests use to locate the project root — don't delete it.
- Avoid http or https links which go off-device. When such links are unavoidable, warn the user beforehand and offer to cancel the action.

## Verify before you claim

Four independent reviews of work already reported as verified each found a real defect in it. Before calling a change done, verified, or behaviour-neutral:

- **Sweep the siblings, not just the site in front of you.** After a fix, grep for the other places the same pattern lives — the `UPDATE` beside the `INSERT` you fixed, the third entry point beside the two you guarded, the `Content-Type` *parameters* beside the media type you sanitised. Say in the PR which sites you checked, and which you deliberately left alone.
- **Prove the regression test fails without the fix.** Revert the fix, run the new test, confirm it fails *for the reason it is named for*, then restore. A test that passes against the unfixed code pins nothing. Watch for expectations that all coincide with one boundary value: if every case equals the minimum, a `MIN()` stub passes the whole suite.
- **Match the handler to the failure the change exists to fix.** `catch (Exception)` does not catch an `Error`. A guard on two of three call sites is not a guard.
- **Every claim needs its check.** "No behaviour change", "this MIME type has rows in the shipped database", "that tool logs a warning" are all testable — run the query, read the sibling repo's source, diff the commits — or don't write them. Re-read the PR body before pushing: a description that was true at commit 1 is often false by commit 3.

## Code style

**Tabs** for indentation, **LF** line endings — enforced by **Spotless**. The `ratchetFrom = origin/stage` ratchet is **file-level, not line-level**: it checks every file that differs from `origin/stage` and reformats each such file *in full*, so editing even one line of a file whose existing indentation doesn't conform (e.g. a layout XML using 4 spaces) pulls the **whole file** under the ratchet and requires reindenting it to tabs — a one-line edit can become a whole-file reformat. Java uses the **Eclipse** formatter (`spotless.eclipse-java.xml`, with member sorting + import ordering); Kotlin and `*.gradle.kts` use **ktlint**; XML uses the **Eclipse WTP** formatter. Run `./gradlew spotlessApply` to fix formatting before pushing — the `.githooks` pre-push hook does this automatically once hooks are installed and enabled (`sh ./scripts/install-git-hooks.sh`, no conflicting `core.hooksPath`). Branch names must match `.../ADFA-#####` (3–5 digits) — see CONTRIBUTING.md; a pre-commit hook enforces it (`sh ./scripts/install-git-hooks.sh`).

Keep docs, tickets, commit messages, and PR descriptions crisp — say it once, lead with the point, cut hedging and restated context. Brevity is the soul of wit; a reader's attention is the scarce resource.

Brevity governs the artifact, not the reasoning. When you recommend something to the user — a design choice, a library, a way to split a PR — give the one-line *why* and the alternative you rejected, not the conclusion alone. A bare recommendation costs a round-trip of "tell me more", and the user should never have to ask twice to see the trade-off.

**Code comments** follow the same discipline:
- Short and to-the-point: comment the non-obvious *why* (a workaround, a constraint, a subtle invariant), not what the code already states. Cut restated context.
- **No separator or decorative comments.** No banner bars, `// ====` rules, or ASCII-art dividers; let structure, naming, and small functions carry organization.
Expand Down Expand Up @@ -96,6 +110,10 @@ Read tickets with the local authenticated `jira` CLI (e.g. `jira issue view ADFA

The sonarqube MCP server runs in Docker, so Docker must be up before launching Claude Code. Its first launch pulls a ~225MB image (`mcp/sonarqube:latest`) that exceeds Claude Code's 30s MCP handshake timeout — so the first connect reports a timeout though nothing is broken. Pre-pull the image (or let one launch finish) so later `/mcp` reconnects succeed. `docker system prune` removes it and brings back the slow first launch.

### Staging commits — no `git add -A`

Stage by explicit path (`git add path/one path/two`). `git add -A` sweeps in whatever else the working tree happens to hold, untracked files included — regenerated test fixtures, multi-MB binaries, files carrying machine-local absolute paths — and buries them in an unrelated commit. `git add -u` is narrower (tracked paths only, and it stages deletions), but it still picks up any tracked file something rewrote behind your back, which is how a 12 MB regenerated fixture reached a commit here. The untracked half of that risk is live here: a `:gradle-plugin:test` run leaves files under `tests/`, and `tests/test-home` is not currently ignored. Run `git status --short` first and account for every line; if a regenerated file genuinely belongs in the change, say so in the commit message.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
### Multi-line git/gh messages

Default to writing the body to a tempfile via the Write tool, then `git commit -F /tmp/msg.txt` or `gh pr create --body-file /tmp/body.md`. Use heredoc/`--body "$(cat <<EOF ...)"` only for short messages with no shell-special characters.
Expand Down
11 changes: 11 additions & 0 deletions docs/process/learnings.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
# Learnings

## Spotless / formatting
- **A tripped pre-push hook costs two Gradle invocations, not one.** The hook runs `spotlessApply`, and if that *changes* anything it fails the push and leaves the tree dirty — so you commit the formatting and pay it again. Run `spotlessApply` yourself before committing.
- **Spotless is not the slow part; a cold Gradle invocation is.** Measured on this repo: `spotlessCheck` is ~6.5s warm and ~22s cold, of which ~20s is daemon start plus configuration (`gradlew help` alone costs the same). `:spotlessShell` is 750ms — it targets `.githooks/**` and `scripts/**`, which together are 38 files. A multi-minute Spotless run means something else is wrong: a cold daemon under memory pressure, or a machine with orphaned JVMs holding gigabytes (see ADFA-5265, filed on the wrong premise and corrected by measurement).
- Root `spotlessCheck` passing alone proves little: it only fails on the implicit-dependency check when a task that *writes into the source tree* is in the same invocation (e.g. `:common:compileV8DebugKotlin spotlessCheck`). CI ran it standalone, so that class of failure reproduced only on developer machines.
- The Spotless ratchet (`ratchetFrom = origin/stage`) is **file-level, not line-level**: a one-line edit to a file whose indentation doesn't already conform reformats the *whole* file (e.g. a 4-space layout XML gets fully reindented to tabs). Run `spotlessApply` after editing so the whole-file reformat lands in your commit, not in CI.
- The `.githooks/pre-push/0001-run-spotless` hook runs `spotlessCheck`/`spotlessApply` locally and blocks a push on violations — but only if git hooks are actually enabled: no conflicting global `core.hooksPath` (it silently overrides `.git/hooks`), and a portable dispatcher (`find -executable` is GNU-only and no-ops on macOS BSD find — see ADFA-4833).

## Git / GitHub
- **An `APPROVED` badge does not mean the current code was approved.** This repo does not dismiss stale reviews on push, so an approval can predate the commits it appears to cover — five PRs at once, in one case, one of them approved two weeks before the code it labelled. Compare the commit each approval points at with the PR head: `gh pr view <n> --json reviews,headRefOid` and check the approving review's `commit.oid` against `headRefOid`. Timestamps are a weaker proxy, and `latestReviews` can come back with an empty `commit.oid`, so use `reviews`.
- Before pushing a follow-up commit to a community PR, check `gh pr view <n> --json headRepositoryOwner` — the PR head is usually on the contributor's **fork**, so a same-named push to `origin` doesn't touch the PR and just creates a confusing dead branch that has to be deleted.

## Android / Kotlin
Expand All @@ -24,6 +28,13 @@
- `.bail on` is required for a `BEGIN;...COMMIT;`-wrapped script to actually be atomic: without it, a mid-script SQL error prints to stderr but the script keeps going, including reaching the final `COMMIT`, which persists whatever succeeded before the error. `.bail` also can't see `.system` shell failures directly — if a step's success depends on a shell command's exit status, assert it in SQL (e.g. a temp table with a `CHECK` constraint) rather than relying on `.bail` to catch it.
- Don't write a `.system` command's output to a fixed, guessable filename directly under `/tmp` (CWE-377) — another local user could pre-plant a symlink there or race the write against your later read. Create an owner-only working directory instead (`rm -rf` it, then `mkdir -m 700` it — the mode is set atomically at creation, so there's no window where it's briefly wider), write everything under that, and remove it when done. `mkdir` itself can fail (e.g. another user recreates the path between the `rm -rf` and the `mkdir`) — that's a `.system` failure `.bail` won't catch either, so assert the directory's mode in SQL before trusting it, the same way you'd guard the Brotli step above. A fresh `mktemp -d` per run would be even better, but it doesn't fit this script shape: each `.system` line is its own subshell, so a path it generates can't be carried into later `.system`/`READFILE()` calls without writing it to another fixed, guessable file first.

## Instrumented (androidTest) runs
- `./gradlew :module:connectedV8DebugAndroidTest` fails here before any test runs: `NoClassDefFoundError: org/bouncycastle/asn1/edec/EdECObjectIdentifiers`, thrown while AGP's Unified Test Platform mints a TLS cert for its result-listener server. Not the device, the APK, or the tests (ADFA-5258). Workaround that does work: `adb install -r <module>/build/outputs/apk/androidTest/.../*-androidTest.apk`, then `adb shell am instrument -w -e class <TestClass> <testPackage>/<runner>` — find the runner with `aapt2 dump xmltree --file AndroidManifest.xml <apk>`.
- Assertions in an instrumented test tell you the return value, not the log. To check a diagnostic actually fires (and stays quiet when it should), clear logcat before the run and grep it after — a warning that cries wolf is worse than none.

## Test fixtures that write themselves
- `testing/resources/test-project/.cg/gradle-sync/{project,sync}.pb` used to be **tracked**, and a test run rewrote them with the local machine's absolute paths — so they arrived in unrelated commits, a 12 MB binary among them. Reverting before a run didn't stick, because the next run re-dirtied them. ADFA-5264 (#1740) ignored the whole `.cg/` directory, so this is fixed; it is recorded because the shape recurs. A tracked file that a test run rewrites cannot be kept clean by discipline, only by untracking it.

## Kotlin LSP test harness
- Disposing the `KtLspTestEnvironment` in a unit test (`env.close()`, or `Disposer.dispose(env.project)`) throws `AssertionError: Write access is allowed inside write-action only`. IntelliJ requires model teardown to run inside a write action. This is why `KtLspTestRule`'s teardown has `env.close()` commented out as "fails in test cases". To dispose deterministically in a test, wrap it: `ApplicationManager.getApplication().runWriteAction { env.close() }`.
- The index/compilation environment lifecycle is racy: background `IndexWorker` coroutines call `PsiManager.findFile(project)` and will crash with `Project is already disposed` if the project is disposed before the workers are stopped. Always stop & join `KtSymbolIndex.close()` (and cancel related scopes) before `Disposer.dispose(...)`.
Loading
Loading