trails.cool · architecture review

Deepening opportunities

A broad repo scan for architectural friction — shallow modules, implicit interfaces, duplicated choreography — with sketches for turning them into deep modules.

2026-06-10 · four parallel codebase surveys (Journal, Planner, packages, cross-cutting) · key claims verified against source · checked against CONTEXT.md and ADR-0001…0006

Method

How to read these slides

Vocabulary

Module — anything with an interface and an implementation.
Deep module — a lot of behaviour behind a small interface.
Shallow module — interface nearly as complex as the implementation.
Seam — where an interface lives; behaviour can change without editing in place.
Locality — change, bugs, and knowledge concentrated in one place.
Leverage — what callers get from depth.

The deletion test

Imagine deleting the module. If complexity vanishes, it was a pass-through. If complexity reappears across N callers, it was earning its keep.

Already settled (not re-proposed)

No unified SyncProvider (ADR-0002)
No AuthMethod polymorphism (ADR-0005)
gpx-save owns geometry atomically (ADR-0006)

Overall map

Where the codebase is deep — and where it isn't

Deep, earning their keep

Journal core seams — gpx-save, completeAuth, ConnectedServiceManager, capability seams (Importer / RoutePusher / WebhookReceiver).
Planner pure modules — BRouter client, route-merge, segment-cache, host election. Well tested.
packages/gpx & packages/db — load-bearing, most-imported, best-tested.
Federation — ~3,200 lines of deliberate Fedify isolation.

Shallow, causing friction

Journal route-handler layer — 110 handlers, ~5 tests; scattered ownership checks, hand-rolled form parsing.
Planner Yjs document schema — the app's most important interface, enforced by convention only (~30 raw call sites).
Domain types — three drifting sources of truth (types / api / db).
Jobs, SSE, notifications — interfaces discovered by grep, not by type.

Agenda

Ten candidates

Closest to a real defect
1. Komoot bulk import smuggles credentials past the manager
Highest-leverage refactors
2. A typed job seam — payloads are unknown end-to-end
3. Planner: the Yjs routeData schema is an implicit interface
5. Journal: route/activity saves duplicate GPX choreography
6. Three drifting sources of truth for Route/Activity

Testability & safety
7. Ownership checks scattered across handlers and lib
8. OAuth connect→callback→resume spread over three routes
Smaller wins
4. Planner GPX assembly duplicated (Save vs Export)
9. Shim & dead-weight packages: map, ui
10. E2E suite: 23 spec files re-derive their own setup

candidate 1 / 10

Komoot bulk import smuggles credentials past the manager defect-adjacent

Files

apps/journal/app/routes/api.sync.komoot.import.ts:34 apps/journal/app/jobs/komoot-bulk-import.ts

Problem

Verified: the route enqueues creds: service.credentials — the raw JSONB — in the pg-boss payload. This bypasses withFreshCredentials, the ConnectedServiceManager's whole reason to exist:

Credentials sit at rest in the jobs table.
No refresh if they go stale between enqueue and execution.
markNeedsRelink never fires on failure.

The Wahoo path goes through the manager correctly; Komoot is the one defector from the seam.

Solution

Jobs carry only the serviceId. The handler resolves fresh credentials through ConnectedServiceManager at execution time — identical to every other capability caller.

Benefits

Locality: all credential-lifecycle bugs live in one module again.
No credentials-at-rest in the jobs table.
Handler becomes testable with a fake manager instead of fixture credential blobs.

candidate 2 / 10

A typed job seam — payloads are `unknown` end-to-end high leverage

Files

packages/jobs/src/types.ts apps/journal/app/jobs/ (~14 handlers) · ~7 enqueue sites

Problem

Verified: JobDefinition.handler is (jobs: Job<unknown>[]) => Promise<unknown>.

Every handler opens with item.data as SomePayload; every enqueue passes a bare string queue name + unchecked object.
A typo or payload-shape change is a runtime failure in a background worker.
unknown exists for a documented contravariance reason — but that's an implementation constraint leaking into the interface.
enqueueOptional swallows queue failures: "saved but fan-out never queued" looks like success.

Solution

A JobPayloads map type ('notifications-fanout': { activityId: string }, …) with typed enqueue<K> and typed handler registration. The contravariance cast happens once, inside the package — not fourteen times at the edges.

Benefits

Leverage: enqueue call sites can't drift from handlers.
Locality: retry/backoff policy and queue-name validity in one registry.
Handlers become unit-testable with typed fixtures.

candidate 3 / 10

Planner: the Yjs routeData schema is an implicit interface high leverage

Files

apps/planner/app/lib/use-yjs.ts use-routing.ts use-waypoint-manager.ts use-elevation-data.ts + components (SessionView, ProfileSelector, ElevationChart, …)

Problem

Verified: ~15 string-keyed entries (geojson, coordinates, segmentBoundaries, surfaces, highways, maxspeeds, …) read at ~30 raw call sites.

No module owns the schema — consumers do routeData.get("surfaces") as string | undefined and re-parse JSON.
parseJsonArray copy-pasted in two hooks with different signatures; waypoint extraction written four times.
Adding one road attribute touches ~7 files (enrichment → storage → parsing → rendering).

The document schema is the Planner's most important interface — enforced by convention only.

Solution

A routeData schema module: typed read/write of the whole document state, JSON encoding internal — analogous to what waypoint-ymap.ts already does well for waypoints. Hooks and components consume a RouteDataState, never raw Y.Map keys.

Benefits

Locality: schema changes become one-file edits.
Hooks testable against a plain object instead of a live Yjs doc.
Deletion test passes clearly: delete it and key-string knowledge reappears at 30 call sites.

candidate 4 / 10

Planner GPX assembly duplicated between Save and Export small win

Files

apps/planner/app/components/SaveToJournalButton.tsx apps/planner/app/components/ExportButton.tsx

Problem

Both buttons independently extract geojson / waypoints / no-go areas / notes from Yjs and build GPX.
ExportButton additionally handles multi-day splitting.
Same extraction, two implementations — a change to what gets persisted (e.g. waypoint notes in GPX extensions) needs two synchronized edits.
Currently zero tests on either path.

Solution

One buildGpxData(yjs) module — naturally layered on candidate 3 — consumed by both buttons and any future export path.

Benefits

The Planner→Journal handoff payload and the file export can no longer diverge.
The assembly becomes a pure, testable function.

candidate 5 / 10

Journal: `createRoute` / `createActivity` duplicate the GPX choreography high leverage

Files

apps/journal/app/lib/routes.server.ts (~24–78) apps/journal/app/lib/activities.server.ts (~68–122) apps/journal/app/lib/gpx-save.server.ts

Problem

Both callers independently run validateGpx → extract distance/elevation stats → transaction → writeGeom, in near-identical ~45-line blocks.
Includes the "stats may be pre-supplied" handling the demo bot relies on — duplicated too.
The gpx-save module owns validation and geometry but not the stat extraction between them, so the choreography is copied at every save entry point. A third entity type would copy it again.

Solution

Deepen the gpx-save module to own the full validate-and-derive step: one function returning parsed GPX + stats (accepting precomputed stats), leaving callers with only entity-specific row shapes. Extends ADR-0006 rather than contradicting it.

Worth grilling: should basic validation move down into packages/gpx, which already owns parsing and stat computation?

Benefits

New validation rules or stat definitions become one edit.
"Every saved GPX has consistent stats" gets a single testable home.

candidate 6 / 10

Three drifting sources of truth for Route/Activity shapes high leverage

Files

packages/types/src/index.ts packages/api/src/routes.ts (Zod) packages/db/src/schema/journal.ts (Drizzle)

Problem

Route exists three times: hand-written interface (types), Zod RouteSummary/RouteDetail (api), Drizzle columns (db) — each with different fields and nullability.
Route handlers hand-roll the mapping (e.g. api.v1.routes._index.ts constructs response JSON manually, not through the schema).
The api package's contracts are advisory: nothing fails if a handler's response drifts from RouteListResponse.

Solution

Pick one canonical source — likely derive TS types from the Drizzle schema, have the api Zod schemas reference them — and validate responses against the contract at the seam (typed response helpers, or validation in the v1 handlers).

Benefits

Leverage for both apps and mobile: one definition of Route.
The 312 schema tests in packages/api start guarding real traffic instead of just the schemas themselves.

candidate 7 / 10

Ownership checks scattered across handlers and lib modules testability

Files

routes.$id.edit.server.ts push-action.server.ts:54 activities.server.ts (mutators that assume the caller checked) · others

Problem

route.ownerId !== user.id is re-checked in some loaders, re-checked in some lib functions, absent in others where the precondition is implicit.
Nothing in the types distinguishes "a Route" from "a Route this user may mutate" — every new call site re-derives, or forgets, the check.
Contrast: the Terms gate is properly centralized. Ownership deserves the same treatment.

Solution

A loadOwnedRoute(routeId, userId) (and activity twin) as the single enforcement point, returning a branded type that mutators require — making bypass a compile error rather than a code-review catch.

Benefits

Authorization gets the locality that completeAuth gave session minting.
The 403 behavior becomes testable once, instead of per-handler.

candidate 8 / 10

OAuth connect → callback → resume spread over three routes testability

Files

api.sync.connect.$provider.ts api.sync.callback.$provider.ts api.sync.push.$provider.$routeId.ts oauth-state.server.ts

Problem

PKCE verifier cookies, state encoding, and push-resumption are coordinated across three route handlers, each knowing part of the protocol (which cookie, when it's set, what's in the state blob).
State encode/decode is extracted — but the lifecycle isn't.
Adding Garmin or Strava OAuth means re-learning the choreography from the existing routes. CONTEXT.md forecasts three more OAuth providers.

Solution

An OAuth flow module owning initiate (returns redirect with cookies set) and complete (consumes request, returns exchange result + decoded state). Routes shrink to thin adapters.

Benefits

Each future provider reuses the flow, not the pattern.
The PKCE handshake becomes testable without a browser.

candidate 9 / 10

Shim & dead-weight packages: `map` and `ui` cleanup

Files

packages/map (79 lines: re-exports + a 35-line MapView) packages/ui (162 lines)

Problem

Both fail the deletion test in the telling direction — deleting them moves almost nothing:

map adds an import-ambiguity tax: apps import from map and map-core inconsistently (~21 vs ~19 sites).
Verified: @trails-cool/ui is imported only by the two apps' root.tsx — for the stylesheet. Button/Input/Card have zero consumers; both apps roll their own buttons inline.

Solution

Fold MapView into a clearly-React entry of map-core, or make map the only import surface for apps.
Either delete ui or commit to it as a real design system — the half-state is the worst option.

Benefits

Less code falsely suggesting reuse.
One obvious import path for map code.

candidate 10 / 10

E2E suite: 23 spec files re-derive their own setup testability

Files

e2e/ — seed-route boilerplate in 5+ files · auth helpers defined inside auth.test.ts · http://localhost:3000/3001 hardcoded in 10+ files

Problem

The helpers module is nearly empty, so each spec carries its own fixtures.
Virtual-authenticator and register/login utilities live in a test file other specs can't cleanly import.
The stated e2e strategy (shared setup helpers, BRouter mocked by default) is documented — but not embodied in a module.

Solution

A real e2e/lib/ fixtures module: seedRoute(), registerAndLogin(), base-URL constants, authenticator setup — imported by every spec.

Benefits

New specs start at the interesting assertion, not at boilerplate.
URL/port changes become one edit.

Scope discipline

Noted but deliberately dropped

A Result<T,E>-everywhere error-handling rewrite — too sweeping; the narrower wins live inside candidates 5 and 7.
An event-driven capability plugin system for providers — this re-litigates ADR-0002's "capabilities are separate seams" without new friction to justify reopening it.
ADR check: nothing in this deck conflicts with ADR-0001…0006. Candidate 5 deepens ADR-0006's module in place.

Summary

Where to start

#	Candidate	Why it's first-tier
1	Komoot credentials bypass	Closest to a real defect — credentials at rest, no refresh, seam violated
3	Planner routeData schema module	Highest-leverage refactor — 30 call sites, ~7-file blast radius per attribute
2	Typed job seam	Best testability-per-effort — one registry, fourteen handlers gain types
7	Branded ownership loading	Best safety-per-effort — bypass becomes a compile error

Then: 5 & 6 (one source of truth for GPX stats and domain types), 8 (before the next OAuth provider lands), 4 / 9 / 10 as opportunistic cleanups.

Deepening opportunities

How to read these slides

Vocabulary

The deletion test

Already settled (not re-proposed)

Where the codebase is deep — and where it isn't

Deep, earning their keep

Shallow, causing friction

Ten candidates

Komoot bulk import smuggles credentials past the manager defect-adjacent

Files

Problem

Solution

Benefits

A typed job seam — payloads are unknown end-to-end high leverage

Files

Problem

Solution

Benefits

Planner: the Yjs routeData schema is an implicit interface high leverage

Files

Problem

Solution

Benefits

Planner GPX assembly duplicated between Save and Export small win

Files

Problem

Solution

Benefits

Journal: createRoute / createActivity duplicate the GPX choreography high leverage

Files

Problem

Solution

Benefits

Three drifting sources of truth for Route/Activity shapes high leverage

Files

Problem

Solution

Benefits

Ownership checks scattered across handlers and lib modules testability

Files

Problem

Solution

Benefits

OAuth connect → callback → resume spread over three routes testability

Files

Problem

Solution

Benefits

Shim & dead-weight packages: map and ui cleanup

Files

Problem

Solution

Benefits

E2E suite: 23 spec files re-derive their own setup testability

Files

Problem

Solution

Benefits

Noted but deliberately dropped

Where to start

A typed job seam — payloads are `unknown` end-to-end high leverage

Journal: `createRoute` / `createActivity` duplicate the GPX choreography high leverage

Shim & dead-weight packages: `map` and `ui` cleanup