From 8a23655f6cb9b7c5d497f409a88d9316c4618b4d Mon Sep 17 00:00:00 2001 From: taylorcox75 Date: Mon, 7 Sep 2026 14:47:53 -0500 Subject: [PATCH] docs: correct the retired develop flow, right-size the run rules, add RN test traps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md had drifted from the repo in ways that actively misled agents: - **`develop` is gone.** The doc mandated it in ten places ("Never commit to develop", "Always cut from develop — there is no hotfix exception", `gh pr create --base develop`), but no such branch exists locally or on origin, and no PR has ever targeted it. Everything since #219 goes straight to main (#218 and earlier used preview). An agent following the doc would have `git switch develop` fail outright. Rewritten around main, with an explicit note that develop/preview are retired so the next reader doesn't re-derive it, plus the release/vX.Y.ZZ branch that does get used. - **docs/RELEASING.md's changelog tree was half-dead** for the same reason — the "On develop → .TESTFLIGHT placeholder" branch can never fire (`grep -c TESTFLIGHT constants/changelog.ts` is 0). Removed; the main-branch rules it kept are unchanged. The run-cost model was off by ~25x, which was buying caution nobody needed: one suite ~18s claimed → 1.2s actual; tsc 25s warm / 85s cold → 0.8s / 3.2s full npm test 4.9s (1095 tests); lint 6.4s; whole batch ~12s The three-tier "exactly three moments to run anything" ceremony was therefore discouraging verification to save ~12 seconds. Replaced with the constraint that actually binds — context, not wall time, since npm test dumps hundreds of lines of unrelated act()/Animated noise. New rule: run the narrow check freely, pipe it through tail. Commit-time batch is unchanged. Also added two rn-project test traps that each cost about an hour to rediscover: a synchronous act(() => ...) around a promise-returning call silently swallows the state update (looks exactly like a product bug), and render() returns a promise so an outside-provider throw needs .rejects.toThrow. Smaller corrections: the Alert deviations list was missing ConfirmModal itself (nine calls across eight files, not eight); File Index gains SkeletonTorrentDetail and notes ServerContext.isReconnecting and how TorrentContext's recovery flag is cleared. --- AGENTS.md | 135 +++++++++++++++++++++++++++++++--------------- docs/RELEASING.md | 32 +++++------ 2 files changed, 107 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ad75cb50..43d5147e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,35 +80,43 @@ version rather than the qBittorrent version: > renames are per-site: handling one does not handle the others. When you add a > version-dependent parameter, gate it *and* verify against the older wiki. -### Don't burn runs +### Run checks freely — but quietly -Every typecheck / test / lint run costs real tokens and wall time. There are -exactly **three** moments to run anything, and no others: +**The checks are fast. The output is not.** Measured on this repo: -**1. Mid-task → run nothing.** Never fire `tsc`, `jest`, or `eslint` after an -individual edit or "just to be safe." Trust the edit and keep working. +| Command | Wall time | +|---|---| +| One suite — `npm test -- tests/utils/format.test.ts` | ~1s | +| `npm test` (both projects, 1000+ tests) | ~5s | +| `npx tsc --noEmit` | ~1s warm, ~3s cold (incremental via `.tsbuildinfo`) | +| `npm run lint` | ~6s | -**2. Handing back a non-trivial change → the impacted suites only.** Never the -full run here. Pick the narrowest command that covers what you actually touched: +So wall time is not the constraint — **context is**. `npm test` prints hundreds +of lines of unrelated `act()` warnings and Animated stack traces from +`Confetti` / `PathAutocompleteInput`, and every line of that stays in the +conversation and is re-sent on every later turn. -| What you changed | What to run | -|---|---| -| One module that has a test | That one suite — `npm test -- tests/utils/format.test.ts` (~18s). Same form for `tests/services/…` and `tests/rn/…`. | -| Pure logic — `utils/`, `services/`, locales | `npm test -- --selectProjects node` (skips the slow jest-expo project) | -| Components, hooks, context | `npm test -- --selectProjects rn` | -| Types, or a change that crosses many files | `npx tsc --noEmit` | +**Therefore: run whatever you need, but pipe it.** -**Trivial edits need nothing** — a comment, a copy tweak, a doc line, a single -string. Use judgment; the point is to catch real breakage, not to perform rigor. +```bash +npm test 2>&1 | tail -5 # summary only +npx tsc --noEmit; echo "EXIT:$?" # silent when clean +npm run lint 2>&1 | tail -3 # the problem count +``` -**3. Before a commit → the full batch.** See [Commit-time checks](#commit-time-checks). +Run the narrow thing while you work — one suite after touching one module, +`tsc` after a type change. Catching a broken test one second after you write it +is far cheaper than discovering it at commit time and re-deriving what you did. +**Trivial edits still need nothing** — a comment, a copy tweak, a doc line. + +Only widen to a full unpiped run when something actually fails and you need the +detail; then read the failure, not the whole log. + +**Before a commit → the full batch.** See [Commit-time checks](#commit-time-checks). ### Read narrowly -Tool output is **permanent and recurring**: whatever a command prints stays in -the conversation and is re-sent on every later turn. A single careless dump of a -few hundred lines is a tax on the whole rest of the session, so the cost of -reading too much is much higher than it looks at the moment you do it. +Same reason as above — output is permanent and re-sent every turn: - **Never `cat` a whole file** to answer a narrow question. Use `grep -n` (with `-A`/`-B` for context) or a ranged read. Reach for the file's shape first — @@ -129,11 +137,13 @@ Only now do you run the whole thing — once, as a batch: | Command | Bar | |---|---| -| `npx tsc --noEmit` | Exit 0. Currently clean. Incremental via `.tsbuildinfo` (~25s warm, ~85s cold). | +| `npx tsc --noEmit` | Exit 0. Currently clean. Incremental via `.tsbuildinfo`. | | `npm test` | All passing, both projects — see [Testing](#7-testing). | -| `npm run lint` | **Zero errors.** Warnings are baseline noise; the count drifts, don't chase it. | +| `npm run lint` | **Zero errors.** Warnings are baseline noise (currently 37); the count drifts, don't chase it. | | `npm run format` | Prettier. Run it last, so it also formats anything you just changed. | +The whole batch is ~12s. Pipe each through `tail` unless it fails. + Then, in the same pre-commit pass: - **Update the [File Index](#5-file-index)** if you added, removed, or renamed a @@ -151,33 +161,37 @@ before. ### Branches — always work on one -**Never commit to `main`. Never commit to `develop`.** Both are protected by -convention: work reaches them only through a PR. There is no exception for a -one-line fix or a "quick" change. +**Never commit to `main`.** It's protected by convention: work reaches it only +through a PR. There is no exception for a one-line fix or a "quick" change. -Every change starts on its own branch cut from `develop`: +Every change starts on its own branch cut from `main`: ```bash -git switch develop && git pull && git switch -c bugfix/#123-short-description +git switch main && git pull && git switch -c bugfix/#123-short-description ``` Naming follows what's already in the repo — `feature/…`, `bugfix/…`, or `fix/…`, usually carrying the issue number (`bugfix/#177`, `feature/#121`). Agent-created branches use a `claude/…` prefix. -**Always cut from `develop` — there is no hotfix exception.** However urgent a -fix is, it goes branch → PR → `develop` → `main`. Never branch from `main`, and -never shortcut a fix straight into it. +**There is no hotfix exception.** However urgent a fix is, it goes branch → PR → +`main`. Never commit straight to `main`. | Branch | Role | |---|---| -| *your branch* | Where every commit goes. Cut from `develop`, merged back by PR. | -| `develop` | Integration branch. Receives work by PR only. Its changelog entry is a `.TESTFLIGHT` placeholder — see [docs/RELEASING.md](docs/RELEASING.md). | -| `main` | Release branch. Receives `develop` by PR. **Pushing it with `"easBuild": true` in `package.json` builds and submits to the App Store** — see [docs/RELEASING.md](docs/RELEASING.md). | +| *your branch* | Where every commit goes. Cut from `main`, merged back by PR. | +| `main` | The only long-lived branch. Receives work by PR. **Pushing it with `"easBuild": true` in `package.json` builds and submits to the App Store** — see [docs/RELEASING.md](docs/RELEASING.md). | +| `release/vX.Y.ZZ` | Occasionally used to gather a release's work before one PR to `main`. Only when the user sets one up — don't create one on your own. | + +> **`develop` and `preview` are retired** (as of 2026-08-14). Neither branch +> exists. Older PRs (#218 and earlier) targeted `preview`; everything since +> #219 goes straight to `main`. If you find guidance anywhere referring to +> either, it's stale. **Commit and push only when asked.** If you're asked to commit and you're sitting -on `main` or `develop`, branch first, then commit — don't ask whether the rule -applies this time. +on `main`, branch first, then commit — don't ask whether the rule applies this +time. If you're already on an unrelated branch (one cut for different work), +cut a fresh one rather than mixing the histories. ### When asked to commit, go all the way to a PR @@ -187,7 +201,7 @@ applies this time. 2. Branch if you aren't already on one. 3. Commit. **No `Co-Authored-By: Claude` trailer** on this repo. 4. `git push -u origin ` -5. `gh pr create --base develop` with a short summary and a test-plan line +5. `gh pr create --base main` with a short summary and a test-plan line covering what you ran. Stop there. **Never merge the PR** — review and merge are the user's. @@ -350,9 +364,18 @@ proxy server incl. auth, IP filtering/banned IPs, I2P (qBit 5.0+ / `connectedAt` tracks when the current connection began (derived from `isConnected` transitions, not each `setIsConnected` call site) for a client-side "Connected For" display — qBittorrent's `server_state` has no - session-uptime field of its own (#232). + session-uptime field of its own (#232). `isReconnecting` is true only while + `checkAndReconnect()` is in flight, set by the run that owns the shared + promise — deliberately *not* folded into `isLoading`/`isConnecting`, whose + consumers shouldn't change behavior for an automatic recovery. It's what + lets the torrents list and torrent detail show a skeleton instead of stale + data or a raw auth error during that window. - **`TorrentContext.tsx`** — rid-based incremental sync, plus the reactive auto-reconnect effect the other providers piggyback on. + `isRecoveringFromBackground` covers the foreground re-sync; it's cleared only + once a fetch *newer than the pre-recovery `dataUpdatedAt`* lands, never by + that timestamp merely being nonzero (which cleared it instantly, since it + holds the last success from potentially hours ago). - **`TransferContext.tsx`** — transfer-info poll; relies on TorrentContext's reconnect. - **`ToastContext.tsx`** + `components/Toast.tsx` — the global toast is a plain view. **Never wrap it in an RN ``** — a Modal captures all touches and @@ -387,7 +410,9 @@ All PascalCase function components taking a `…Props` interface. then defaults then the `avatarColor` fallback), `SearchResultRow` (+ internal ActionPill; the `+` button and the cart-toggle button are independent — see its header comment), `FilterChip`, `EmptyState`, `SkeletonLoader` - (+ `SkeletonTorrentCard`), `PieceMap`, `ServerIconBadge` (per-server tinted + (+ `SkeletonTorrentCard`, `SkeletonTorrentDetail` — the latter covers the + detail screen while a dead session reconnects), `PieceMap`, + `ServerIconBadge` (per-server tinted icon badge — `ServerConfig.icon`/`iconColor` via `utils/server.ts` `getServerIcon`/`getServerIconColor`, falling back to a default icon and `DEFAULT_AVATAR_COLOR` — never the name-derived `avatarColor`, so a badge's @@ -577,6 +602,31 @@ already carry the right imports and mocks: Import app code as `@/…` — both projects map it to the repo root. +**Two `rn`-project traps that will cost you an hour each if you meet them cold:** + +- **Don't wrap a trigger call in synchronous `act(() => …)`.** In this + React/RTL version that *silently swallows* the state update — the component + never re-renders and your assertion sees the old value, with no warning. It + looks exactly like a product bug. Call the function bare and let `waitFor` + observe the result: + + ```ts + const pending = getLatest().checkAndReconnect(); // NOT inside act() + await waitFor(() => expect(getLatest().isReconnecting).toBe(true)); + await act(async () => { resolveIt(true); await pending; }); // async act is fine + ``` + + `await act(async () => …)` around a *fully awaited* operation works normally. + It's the sync form wrapping a promise-returning call that breaks. + +- **`render()` returns a promise here.** Tests `await render(...)`, and a + component that throws during render gives you a *rejected promise*, not a + synchronous throw. The "hook used outside its provider" test therefore reads: + + ```ts + await expect(render()).rejects.toThrow('must be used within'); + ``` + --- ## 7. Testing @@ -632,10 +682,11 @@ rather than a translation gap. 5. **All user-facing strings go through i18n** — `const { t } = useTranslation()`. 6. **Prefer themed dialogs**: `InputModal` over `Alert.prompt`, `ConfirmModal` over `Alert.alert`. Native alerts ignore the app theme. **Don't add a new - one.** *Known deviations* — eight existing sites: `settings/advanced`, - `settings/torrent-defaults` ×2, `search/plugins`, `server/[id]`, `TagsModal`, - `CategoryModal`, `SuperDebugPanel`. Converting one while you're already in - that file is welcome, but it's never required. + one.** *Known deviations* — nine existing calls across eight files: + `settings/advanced`, `settings/torrent-defaults` ×2, `search/plugins`, + `server/[id]`, `TagsModal`, `CategoryModal`, `SuperDebugPanel`, and + `ConfirmModal` itself. Converting one while you're already in that file is + welcome, but it's never required. 7. **Delete superseded files in the same change.** When a component is replaced by a route-level screen or vice versa, remove the old one rather than leaving dead code. Precedent: `components/TorrentDetails.tsx` was deleted once its diff --git a/docs/RELEASING.md b/docs/RELEASING.md index 883cee13..837e95ef 100644 --- a/docs/RELEASING.md +++ b/docs/RELEASING.md @@ -23,24 +23,20 @@ speculative entries, no "while I'm here" additions, no entry per edit. When the user does ask, edit it as the **last step before the commit** — one pass covering everything in the session, not one entry per change. -### Which entry to edit depends on the branch - -The app ships OTA through EAS Update, so the two branches track versions -differently: - -- **On `develop`** — the top entry is a placeholder whose version ends in - `.TESTFLIGHT` (`X.Y.TESTFLIGHT` — the `X.Y` moves with the release train, so - match on the **suffix**, never on a specific number). **Append** your line to - the matching section in that entry. Don't create an entry, don't renumber the - placeholder, don't invent a version. If the top entry *isn't* a `.TESTFLIGHT` - placeholder, stop and ask which entry to use rather than guessing. -- **On `main`** — versions are real. Compare `package.json`'s `version` to - `CHANGELOG[0].version`: - - **They DIFFER** (changelog ahead) → an unreleased entry is already open. - **Append** to it. Don't create an entry, don't change its version. - - **They're EQUAL** → the top entry is already released. Add ONE new entry at - the top with a **patch bump only** and today's date. **Don't touch - `package.json`** — the release process owns the app version. +### Which entry to edit + +All work now branches off `main` and returns by PR (`develop` and `preview` are +retired — see AGENTS.md). Versions in the changelog are therefore always real; +there is no `.TESTFLIGHT` placeholder flow any more. + +Compare `package.json`'s `version` to `CHANGELOG[0].version`: + +- **They DIFFER** (changelog ahead) → an unreleased entry is already open. + **Append** to it. Don't create an entry, don't change its version. +- **They're EQUAL** → the top entry is already released. Add ONE new entry at + the top with a **patch bump only** and today's date. **Don't touch + `package.json`** — the release process owns the app version, unless the user + explicitly asks you to bump it. Patch numbers are always two digits (`3.8.05`, never `3.8.5`).