refactor: service layers for documents, projects, tabular, chat, user and workflows (ADR) - #42
Open
amal66 wants to merge 1 commit into
Open
refactor: service layers for documents, projects, tabular, chat, user and workflows (ADR)#42amal66 wants to merge 1 commit into
amal66 wants to merge 1 commit into
Conversation
Port of the amal66/mike fork's backend architecture refactor onto the upstream layout: the monolithic backend/src/routes/*.ts files are replaced by per-domain modules under backend/src/modules/, each split into thin route handlers (HTTP concerns only) and service functions (business logic + data access, taking an explicit db client and returning typed results). - modules: chat, project-chat, projects, documents (access/upload/ versions/download/edits facade), tabular (reviews/extract/generate/ chats), user (profile/mfa/apiKeys/mcp/account/export), workflows, library, downloads, case-law - src/index.ts split into app.ts (exported express app, testable with supertest) + index.ts (process concerns: listen, signals, crash exits) - supporting libs the modules are written against: lib/http (sendError/ parseBody), lib/logger (pino + request-id correlation), lib/asyncErrors, lib/sseHeartbeat, lib/pdfjs, middleware/httpLogger + requestContext; lib/upload gains magic-byte validation used by the upload routes - integration tests (supertest against the exported app, all IO mocked) and unit tests ported alongside the modules; tsconfig excludes test files from the build so tsc output is unchanged - deps: pino/pino-http/@opentelemetry/api (logger), zod 3 -> 4 (module validation schemas), supertest (dev) Fork-only features NOT ported (hunks omitted): BullMQ job queues, orgs/ multi-tenant RBAC, message credits, RAG/embeddings, DMS connectors, Word add-in document context + prompt spotlighting, demo-model fallback, air-gap model gating, docx redline extraction, workflow pack import/ export, /ready probe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC
amal66
added a commit
that referenced
this pull request
Aug 6, 2026
…ice layers
WHY THIS MATTERS
The backend's route files were monoliths — routes/tabular.ts (1,649
lines), routes/documents.ts (1,504), routes/projects.ts (1,139),
routes/user.ts (1,132) — each interleaving HTTP parsing, auth checks,
storage IO, and business logic inline in a dozen unrelated handlers.
In a monolith, every change lands in a giant file where the blast
radius is unclear, business logic can only be exercised through a live
HTTP stack, and a new contributor cannot tell which lines are "the
endpoint" and which are "the feature". For an open-source repo this is
the difference between a drive-by contributor shipping a fix and giving
up: small files with one concern each are reviewable; 1,600-line route
files are not.
WHAT IS A SERVICE LAYER
A service layer separates WHAT the application does from HOW it is
reached. The route (HTTP layer) owns request parsing, validation, and
mapping results onto status codes; the service owns business logic and
data access, takes its database handle as an explicit parameter, returns
typed results (discriminated unions like { ok: false, kind: "not_found" }
instead of writing to `res`), and never touches the HTTP request or
response. That inversion is what makes logic unit-testable (call the
function with a fake db — no server needed), reusable (the async
extraction worker calls the exact same functions the SSE route calls),
and safe to change (the compiler knows every result shape a route must
handle).
HOW IT WORKS
- src/routes/*.ts (11 files, 7,853 lines) is replaced by
src/modules/<domain>/ — chat, project-chat, projects, documents,
tabular, user, workflows, library, downloads, case-law, models —
each a thin <name>.routes.ts plus <name>.service.ts. Large domains
split the service into topic files behind a named-re-export facade
(documents: access/upload/versions/download/edits; projects:
crud/folders/documents/chats; user: profile/mfa/apiKeys/mcp/account/
export; tabular: reviews/rows/extract/extractRow/generate/
generateStream/chats) so intra-module helpers cannot leak.
- lib/tabular/* (from the durable-queues change this builds on) moves
into modules/tabular/ — the domain's extraction core, row loaders and
route layer now live together; src/lib/ keeps only cross-domain
infrastructure (storage, llm, chat, queue, access...).
- Streaming endpoints keep their SSE loops in the routes file; only
their non-streaming prepare/persist logic moved into services —
streaming lifetime and client-abort handling are HTTP concerns.
- Pure motion, verified three ways: the endpoint inventory
(method+path multiset, 67 endpoints) is byte-identical before and
after; tsc is clean; the full suite — 510 tests, including the 11
route-level integration suites that exercise the real express app —
passes unchanged. Handler bodies moved verbatim; the only rewrites
are the mechanical seam (res.status(...) inside moved code became
typed returns mapped back to the identical status/JSON in the route).
- DRY within domains only: helpers duplicated across handlers in the
same domain (shared_with normalization in projects, the doc-access
guard sequence in documents, findSystemWorkflow) now have one copy in
their service; similar-but-not-identical code was left alone rather
than force-merged.
- Zero new dependencies. No logging framework, no validation framework,
no observability hooks — organization only, so the diff is reviewable
as motion and each future concern can be its own decision.
Re-derived against this branch's code from the fork's service-layer
refactor (#42, running in the amal66 fork), whose module
boundaries and routes/service contract this follows; the fork's
pino/OTel/zod adoption was deliberately NOT ported to keep this
dependency-free pure motion.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Design (ADR)
Context. The backend's route files are monoliths:
routes/tabular.ts(1,837 lines),routes/documents.ts(1,464),routes/user.ts(1,132),routes/projects.ts(1,027) each interleave HTTP parsing, auth/access checks, storage IO, and business logic inline in the handlers (the fork's equivalent of the 3,900-line chatTools file was already split intolib/chat/upstream; the route layer is the remaining monolith). Handlers can't be unit-tested without a live HTTP stack, and every change touches a file with a dozen unrelated endpoints.Decision. Decompose each route file into a module under
src/modules/<domain>/: thin*.routes.ts(HTTP concerns: parse, validate, map typed results onto status codes) delegating to*.service.tsfunctions (business logic + data access, explicitdbparameter, typed results, never touch req/res). Larger domains split the service behind a facade (documents.service.tsre-exports access/upload/versions/download/edits; tabular splits reviews/extract/generate/chats; user splits profile/mfa/apiKeys/mcp/account/export).index.tsis split intoapp.ts(exported express app) +index.ts(listen/signals), which is what makes the ported supertest integration suites possible — tests-first was the fork's discipline for this refactor and the tests ride along.Consequences. Reviewability (each concern is a few-hundred-line file), testability (93 tests in this PR exercise routes with all IO mocked), a stable seam for future work. Cost: many more files, and a big one-time diff.
Alternatives considered. Status quo (rejected: the monoliths are where changes go to be risky); partial split of only the worst files (rejected: two conventions coexisting indefinitely).
Summary
Ports the amal66/mike fork's service-layer architecture refactor onto upstream's
backend/layout:routes/*.tsmonoliths are replaced bysrc/modules/{chat,project-chat,projects,documents,tabular,user,workflows,library,downloads,case-law}/, plus the supporting libs the modules are written against and the module test suites.Changes
src/modules/— 10 domain modules (routes + services), replacing 10 deletedsrc/routes/files. Endpoint surface is unchanged (verified by extracting method+path pairs from both trees; only deliberate omissions differ, see Provenance).src/app.ts+ slimsrc/index.ts— app/process split; central error handler;lib/asyncErrorspatches express so thrown async errors reach it.lib/http(sendError/parseBody),lib/logger(pino with request-id/trace mixin) +lib/observability/requestContext,middleware/httpLogger+middleware/requestContext,lib/sseHeartbeat,lib/pdfjs;lib/uploadgains magic-byte validation (hasMagicBytes) called by the upload routes.src/__tests__/integration/) +tabular.extractDocunit test — 93 tests, all IO mocked.tsconfig.jsonexcludes test files sonpm run buildoutput is unchanged (same exclude the test-harness PR adds; duplicated so this branch builds green standalone).pino,pino-http,@opentelemetry/api(logger; no OTel SDK — the mixin no-ops without one),zod3→4 (module validation schemas use v4 APIs; upstreamsrc/has no zod call sites), dev:pino-pretty,supertest,@types/supertest.Why
This is the fork's architecture refactor offered upstream as design-review material: it makes the backend testable at the route level and turns the monoliths into per-domain modules with a uniform routes/service contract.
Testing
npm install+npm run build(tsc) green from a clean checkout of this branch (node_modules removed first).upstream-pr/test-harness) merged locally (not part of this PR):npm test→ 9 files, 105 tests passed (93 from this branch: health, chat, project-chat, projects, documents-upload, tabular, user integration suites + tabular.extractDoc unit; 12 from the harness's own tests).Provenance
All added lines are mechanical ports of amal66/mike@origin/main (b3166dd): path moves (
apps/api/src→backend/src), import rewrites, and omission of fork-only feature hunks. Exceptions, each deliberate:GET /:reviewId/generate/streamview;tabular.generateStream.tsnot ported), orgs/multi-tenant RBAC (org_idcolumns andgetOrgRole/getPersonalOrgId/resolveContentOrgId/listUserOrgIdscall sites), message credits (chat + project-chat reserve/refund), DMS connectors (user.dms.ts+ its routes), Word add-indocumentContext+ spotlight nonce-fencing, demo-model fallback and air-gapassertModelAvailablegating, docx redline summaries in tabular extraction, workflow pack import/export (workflowFormat.ts+ 2 endpoints),/readyprobe, Sentry/metrics/OTel init, secretGuard, and theorgs+ guest-authmodules.documents.filename; inserts seed offilename/org_idremoved and two deadselect("filename")calls deleted. Project PATCH management check maps the fork's org-awareaccess.canManageto upstream's owner-onlyaccess.isOwner.any-typed one):ApiKeyProviderparam type inuser.apiKeys.ts, oneas unknown as DocRowcast for a dynamic select,applyKindFiltergeneric loosened inlibrary.service.ts(TS2589 otherwise).lib/env-based versions (fork's typed-env module not ported).lib/env/lib/creditsmocks removed; credit-limit, org-access,/ready, and invalid-bearer-token cases dropped with the features they exercised.Credits & prior art
lib/logger(pino) +middleware/httpLogger+requestContextslice implements the same operational contract with pino instead of a hand-rolled logger.req.bodyunvalidated; the fork's route-modularization commit records fix: validate request body in POST /projects/:projectId/chat Open-Legal-Products/mike#155 (together with the CWE-639 access-scoping fixes independently made by ~10 community forks) as the precedent for the validate-at-the-edge, access-guard-in-service discipline these modules follow.lib/uploadhasMagicBytes) is adapted from fix: magic-byte upload validation, CSP headers, upgrade xmldom Open-Legal-Products/mike#78's file-signature validation (same PDF/ZIP/OLE2 signature set, extended to the Office formats); the fork's upload-validation commit records fix: magic-byte upload validation, CSP headers, upgrade xmldom Open-Legal-Products/mike#78 as its precedent.🤖 Generated with Claude Code
https://claude.ai/code/session_01CEguyEgXa9JjCciXCcVemC