Skip to content

Split request handling into focused modules - #1847

Merged
stefan-burke merged 3 commits into
mainfrom
split-features-ts
Jul 16, 2026
Merged

stefan-burke merged 3 commits into
mainfrom
split-features-ts

Conversation

@stefan-burke

@stefan-burke stefan-burke commented Jul 16, 2026

Copy link
Copy Markdown
Member

Summary

  • reduce the main request entry from 860 lines to a small request-context wrapper
  • separate routing and request infrastructure from pure request and read-only rules
  • replace the 901-line routing test with focused tests that mirror the source modules
  • move the broad read-only route checks into the integration test tree
  • update references to the moved request code

Checks

  • targeted affected tests passed
  • deno task typecheck
  • deno task lint:ci
  • deno task cpd
  • deno task build:edge
  • deno task unit-tests-report

The full test suite, precommit task, and mutation tests were not run, as requested for this time-sensitive refactor.

Summary by CodeRabbit

  • New Features
    • Strengthened read-only mode protections by blocking mutating requests while allowing explicitly safe operations (with API 403 responses or page redirects as appropriate).
    • Updated the main request routing flow to better handle setup/activation, tracking redirects, and static asset behavior (including custom CSS handling).
  • Bug Fixes
    • Improved handling for database-busy and migration-in-progress states, invalid sessions, and requests with missing/invalid content types.
  • Documentation
    • Refreshed documentation around route ordering and read-only mode behavior.
  • Tests
    • Added coverage for routing, read-only guard logic, request rules, and route responses; removed older test coverage and adjusted read-only renewal-link assertions.

@stefan-burke
stefan-burke enabled auto-merge July 16, 2026 12:01
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cb2da5a2-76f7-4107-9c0e-2c8e65231b90

📥 Commits

Reviewing files that changed from the base of the PR and between 6f5db0a and 67bb3b0.

📒 Files selected for processing (1)
  • test/features/middleware.test.ts

📝 Walkthrough

Walkthrough

The request entrypoint now delegates to dedicated request-processing and application-routing modules. New helpers cover buffering, tracking redirects, setup paths, custom CSS responses, and read-only enforcement. Tests add coverage for routing, request errors, response behavior, session keys, and security headers.

Changes

Request routing refactor

Layer / File(s) Summary
Read-only request policy
src/features/app/read-only.ts, test/features/app/read-only.test.ts, test/integration/routes/read-only.test.ts, BUSINESS_LOGIC_TODOS.md
Read-only request classification now covers API mutations, admin operations, public write allowlists, and read-only page handling.
Lazy application route dispatch
src/features/app/routes.ts, test/features/app/routes.test.ts, test/features/router.test.ts
Application routing adds lazy route groups, prefix handlers, exact route loading, public page dispatch, and 404 fallbacks.
Request processing entrypoint
src/features/app/request.ts, src/features/app/rules.ts, src/features/index.ts, test/features/app/request.test.ts, test/features/app/rules.test.ts, test/features/index.test.ts, test/features/response.test.ts, test/features/middleware.test.ts, test/shared/session-private-key.test.ts, src/features/auth.ts, src/shared/db/settings-audit.ts, docs/external-order-library.md, scripts/mutation/equivalent-mutants.txt
Request setup, buffering, redirects, database-state handling, response finalization, error conversion, session-key handling, and related references move into the application request pipeline.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant handleRequest
  participant processRequest
  participant routeMainApp
  participant prefixHandlers
  Client->>handleRequest: submit request
  handleRequest->>processRequest: process request in request scope
  processRequest->>routeMainApp: route prepared request
  routeMainApp->>prefixHandlers: dispatch matching prefix
  prefixHandlers-->>processRequest: return response or null
  processRequest-->>Client: finalize response
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: request handling was split into focused modules.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch split-features-ts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/features/app/read-only.ts`:
- Around line 49-72: Refactor readOnlyBlock into an ordered declarative rule
table, following the repository’s precedence-table convention and preserving the
current matching order and return values. Define shared rule representations for
the conditions and outcomes, then have readOnlyBlock evaluate the table
sequentially rather than using the existing if-chain, including the default
null/page behavior for unmatched methods and paths.

In `@src/features/app/routes.ts`:
- Around line 279-283: Update the prefix handler invocation in the route
dispatch flow to replace optional chaining with the documented non-null
assertion after Object.hasOwn confirms the handler exists. Preserve the existing
notFoundResponse fallback for handler results that are nullish, while ensuring
an invalid routing-table state fails loudly.

In `@test/features/app/request.test.ts`:
- Around line 26-35: Update the “rejects a body-bearing POST with no content
type” test to use a binary Uint8Array body so Request does not synthesize a
Content-Type header. Assert the constructed request’s content-type header is
null before passing it to handleRequest, while preserving the existing 400
status and error-text assertions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 338c5b28-3d46-4b71-ac4d-0604291f1e00

📥 Commits

Reviewing files that changed from the base of the PR and between d8736b9 and 6f5db0a.

📒 Files selected for processing (20)
  • BUSINESS_LOGIC_TODOS.md
  • docs/external-order-library.md
  • scripts/mutation/equivalent-mutants.txt
  • src/features/app/read-only.ts
  • src/features/app/request.ts
  • src/features/app/routes.ts
  • src/features/app/rules.ts
  • src/features/auth.ts
  • src/features/index.ts
  • src/shared/db/settings-audit.ts
  • test/features/app/read-only.test.ts
  • test/features/app/request.test.ts
  • test/features/app/routes.test.ts
  • test/features/app/rules.test.ts
  • test/features/index.test.ts
  • test/features/response.test.ts
  • test/features/router.test.ts
  • test/integration/routes/read-only.test.ts
  • test/lib/server-misc-routing.test.ts
  • test/shared/session-private-key.test.ts
💤 Files with no reviewable changes (2)
  • test/lib/server-misc-routing.test.ts
  • test/integration/routes/read-only.test.ts

Comment thread src/features/app/read-only.ts
Comment thread src/features/app/routes.ts Outdated
Comment thread test/features/app/request.test.ts
@stefan-burke
stefan-burke added this pull request to the merge queue Jul 16, 2026
Merged via the queue into main with commit 2317bc7 Jul 16, 2026
3 checks passed
@stefan-burke
stefan-burke deleted the split-features-ts branch July 16, 2026 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant