chadtmiller.com/pds.js

Switch repository

Loading…

# Decomposing core/src/pds.js

**Status:** complete. Fourteen clusters moved and three helper modules
extracted, 1,337 lines left in `pds.js` from 10,205. What stays in the class is
the dispatch pipeline, authentication, and the commit and blob machinery. The
static route table is empty; every route comes from a handler module.

Read `CLAUDE.md` first for the conventions this follows. This file is the
record: what moved, what each move needed, and the mistakes that cost time.

A second phase followed. The feature routes that `handlers/account.js` held
for git, drive, sites, spaces, OCI and npm moved into those packages, each
mounted through the extension seam described in
[architecture.md](architecture.md#extensions), along with the per-feature
constructor options that wired them. The table below is the snapshot at the
end of the first phase; `account.js` has since lost those routes and the
adapters their feature construction.

## Where it stands

| module | lines | statements | branches | functions |
| --- | --- | --- | --- | --- |
| `handlers/account.js` | 2,998 | 88.4 | 74.6 | 96.5 |
| `handlers/oauth.js` | 1,682 | 83.7 | 73.8 | 100 |
| `handlers/xrpc-server.js` | 1,036 | 98.0 | 94.3 | 95.0 |
| `handlers/xrpc-repo.js` | 810 | 89.2 | 80.0 | 100 |
| `handlers/xrpc-identity.js` | 679 | 95.3 | 82.9 | 100 |
| `handlers/account-backup.js` | 535 | 97.5 | 90.3 | 100 |
| `handlers/migration.js` | 356 | 96.6 | 83.9 | 84.6 |
| `handlers/xrpc-sync.js` | 312 | 95.7 | 79.4 | 100 |
| `handlers/server-meta.js` | 252 | 100 | 100 | 100 |
| `handlers/xrpc-blob.js` | 189 | 100 | 100 | 100 |
| `handlers/xrpc-firehose.js` | 155 | 100 | 86.1 | 100 |
| `handlers/xrpc-preferences.js` | 81 | 100 | 100 | 100 |
| `session.js` | 331 | 96.4 | 84.2 | 100 |
| `proxy.js` | 282 | 98.8 | 96.9 | 100 |
| `http.js` | 103 | 92.6 | 88.9 | 100 |
| `passkey.js` | 85 | 100 | 80.0 | 100 |

Repo-wide statements are 78.1%, from 59.87% at the start. The unit suite is
1,318 tests, up from 941.

`proxy.js`, `session.js`, `http.js` and `passkey.js` are not under `handlers/`
because they contribute no routes. Name a module for what it knows when that
is true, and put it in `core/src`.

## What each cluster needs before it moves

Two numbers decide the work, and they are not correlated. Coverage under the unit
suite against coverage with a `PLATFORM=node` e2e run says where the untested code
is. The count of instance members a cluster reaches says how much of a seam it
has. `xrpc-blob` was 0% covered and reached six members. The account surface
reaches forty-seven.

Measure both before starting. The commands are in the commits; the shape is:

```
awk 'NR>=LO && NR<HI' packages/core/src/pds.js \
  | grep -oE "this\.[a-zA-Z_][a-zA-Z0-9_]*" | sort -u
```

A member count that is too high is a signal to extract a helper module first,
not to widen the context. `xrpc-server` reached 24 members and six module-level
names shared with the rest of the file. Three extractions — `session.js` for the
lifetimes and the registry, `http.js` for CORS, cookies and `isSameOriginPost`,
`decodeJwtClaims` into `auth.js` — brought it to a 17-member context with no
edge in either direction, and the move itself was then mechanical.

## The last rounds

### The account surface, 2,696 lines, 47 members

`Account Pages` and its `JSON API` sub-section moved as one project into
`handlers/account.js`. Most of the methods in the surface are shared machinery
rather than route handlers — `requireAccountSession` alone has fifteen callers —
so splitting the sub-sections would have traded a few hundred lines for fifty
lines of indirection. `accountApi`, the spine of 29 routes, moved once.

Of the 47 members, 27 were the surface's own methods. The email-state helpers
(`sessionSeesEmail`, `readEmailState`, `emailSessionFields`) went into
`email.js` first: the PLC endpoints and the xrpc-server session context need
them on the other side of the seam, and as module functions taking
`actorStorage` neither side carries them in a context. That left a 20-member
context, all ports, config values, and closures over the class.

The factory exposes six functions inward: `readAccountSession`,
`renewAccountSession`, `readAccountUiState` and `accountAppResponse` for
dispatch and the OAuth consent flow, `accountApi` for the backup routes, and
`listConnectedApps` for its unit test, which builds the factory directly.

### The identity and migration residue, 580 lines, 17 members

The ten route handlers left after the account surface had the worst
payoff-per-line in the file: a fifth of that surface's lines reaching nearly
the same seam width, and sitting directly on the signing-key cache and the
commit and blob traversals. They also span three route families, so they went
three ways. The PLC operation flow and `getRecommendedDidCredentials` joined
`xrpc-identity.js`, whose `renameHandle` was their only sibling, with
`derivePublicKey` going to `crypto.js`. `getServiceAuth` joined
`xrpc-server.js`. The migration sequence — `checkAccountStatus`, `importRepo`,
`listMissingBlobs`, `activateAccount`, `deactivateAccount` — became
`handlers/migration.js`, taking the account-active helpers with it; the sync,
repo and account contexts read `isDeactivated`, `checkAccountActive` and
`setAccountActive` from its return. The traversals themselves
(`collectRepoBlocks`, `collectReferencedBlobs`, `relinkBlobRecords`) stay on
the class, which uses them for orphan cleanup, and reach the module as
closures. With the residue gone the static route table held nothing and is
deleted; `this.routes` is built from the module fragments alone.

## Things that cost time here

- **Check inbound references from elsewhere in the same file.** Not just other
  packages. `handleApiOverview` called `this.backupsAvailable()` from 1,500 lines
  away and broke three tests.
- **A test DID must satisfy `DID_REGEX`.** `did:plc:` takes exactly 24 characters
  from `[a-z2-7]`. A short one is not rejected as a bad DID; `isValidDid` returns
  false and the failure surfaces as an unrelated assertion.
- **A fake has to match arity.** `listRecords(collection, cursor, limit)` given a
  two-parameter fake receives the cursor as its limit, which reads as an empty
  collection rather than a wrong call.
- **A default parameter fires on `undefined`.** A test helper that models "no
  password configured" by passing `password: undefined` gets the default back
  and tests the opposite branch. Give the helper a named flag instead.
- **A blanket `this.member` → `member` rename collides with locals.** The
  sign-in handler holds the submitted form value in a local named `password`;
  renaming `this.password` there produced `password !== password`, a check that
  passes every value. Biome's `noSelfCompare` and tsc's use-before-declaration
  errors found all four collisions; run both before trusting the transform.
- **A file missing from the coverage table is not necessarily complete.** Read
  `coverage/coverage-final.json` when it matters. `xrpc-preferences.js` was absent
  and was at 13.3%.
- **Glob threshold keys aggregate.** A well-covered file in the same key hides a
  bare one. One entry per module.
- **Do not type a context member off the class.**
  `PersonalDataServer['buildFullRepoCar']` broke the moment that method moved.
  Structural types instead.
- **A test that calls a handler as a method needs a new entry point.**
  `pds.handleRefreshSession(request)` stops resolving once the handler is a
  closure in a module. `pds.fetch(request)` drives the same route through the
  table, which is what `test/onboarding.test.js` already does.
- The e2e suite binds port 2471. A `just dev` stack in any checkout blocks it.

## Two fixes came out of this, and how

Neither was found by reading the code. Both came from writing a test for a
specific property of a handler that had just moved.

`buildFullRepoCar` threw `Missing block` from inside a `try` whose `catch`
discarded it, so only a missing *root* reached the caller and
`com.atproto.sync.getRepo` answered 200 with part of a repository. Fixed by
walking the structure it knows rather than probing field names on every block.

`putRecord` and `applyWrites` did not check the repo scope that `createRecord` and
`deleteRecord` did, so a narrowed OAuth grant reached any collection through a
different endpoint. Fixed to match `packages/pds` in `bluesky-social/atproto`.

That second fix then exposed a third thing: app-password sessions were being
refused every write, because `com.atproto.appPass` names no granular permission
and `allowsRepo` found nothing to allow. The reference gates its assertions on
`auth.credentials.type === 'oauth'`; `skipsGranularChecks()` is the equivalent
here.

## Open questions

- **`allowsSpace` and `allowsSpaceManage` still refuse app-password sessions.**
  Permissioned data has no counterpart in the reference, so there was nothing to
  match and the change left it strict. If an app-password client should reach
  spaces, that is one line.
- **`skipsGranularChecks` infers the credential type from the scope string.** The
  reference carries `credentials.type` explicitly. Ours holds only because
  `com.atproto.appPass` is set at session creation and never appears in an OAuth
  grant. Carrying the type on `auth` would make it structural instead of implied.
- **`SCOPE_APP_PASS_PRIVILEGED` is issued and never read.** Nothing distinguishes
  a privileged app password from an ordinary one. The reference uses that
  distinction to gate chat access.
- **No e2e test drives an app-password client through a write.** The fix for it is
  covered by unit tests against a fake context, which is a weaker guarantee than
  the rest of this work. This is the first thing to verify.