Skip to content

Extract RealmServer class methods into per-concern handler modules#4846

Open
habdelra wants to merge 7 commits into
mainfrom
cs-11157-server-refactor
Open

Extract RealmServer class methods into per-concern handler modules#4846
habdelra wants to merge 7 commits into
mainfrom
cs-11157-server-refactor

Conversation

@habdelra
Copy link
Copy Markdown
Contributor

@habdelra habdelra commented May 15, 2026

Summary

packages/realm-server/server.ts shrinks from 1242 → 367 lines. Five orphaned "fake handler" implementations that lived on the class because they closed over private state are now free functions in handlers/ + lib/realm-routing.ts, each with a typed deps interface scoped to what it actually needs.

This is prep work — no behaviour change. The first ticket that benefits is CS-11157 (realm creation hangs during deploy-triggered reindex storms); its fix lands stacked on top of this PR.

What moved where

New module What it pulls out
handlers/create-realm.ts createRealm impl + the Koa middleware (merged from the old handle-create-realm.ts) + errorWithStatus
handlers/send-event.ts sendEvent factory
handlers/serve-from-realm.ts serveFromRealm factory
handlers/serve-index.ts serveIndex / serveHostApp factory — closes over the index-html cache that was previously class state (promiseForIndexHTML / indexHTMLHash)
lib/realm-routing.ts findOrMountRealm, getPublishedRealmInfo, isIndexedCardInstance, hasExtensionlessSourceModule, hasPublicPermissions, candidateRealmURLs

Each new module defines its own deps interface (CreateRealmDeps, SendEventDeps, ServeFromRealmDeps, ServeIndexDeps, RealmRoutingDeps) listing exactly the subset of state it needs, rather than taking the whole CreateRoutesArgs record. That makes the coupling visible at a glance — useful when the next change asks "what does createRealm actually depend on?"

CreateRoutesArgs loses its createRealm field; routes.ts now constructs a CreateRealmDeps bag and passes it straight to the extracted factory. server.testingOnlyFindOrMountRealm calls the lib helper directly.

Test plan

  • pnpm lint:types clean
  • pnpm lint:js clean on touched files (server.ts, routes.ts, the new handlers/*.ts + lib/realm-routing.ts)
  • TEST_MODULES=lazy-mount-test.ts ./tests/scripts/run-qunit-with-test-pg.sh — exercises testingOnlyFindOrMountRealm → the lib findOrMountRealm. All 7 lazy-mount tests pass locally.
  • Full realm-server test suite in CI (realm-endpoints-test.ts and friends need Chrome which isn't installed locally on this machine).

🤖 Generated with Claude Code

server.ts went from 1242 lines to 367. Five orphaned "fake handler"
methods that lived on the class because they closed over private
state are now free functions in handlers/ + lib/realm-routing.ts,
each with a typed deps interface scoped to what it actually needs.

  handlers/create-realm.ts   — createRealm impl + the Koa middleware
                               (merged from the old handle-create-realm.ts)
  handlers/send-event.ts     — sendEvent factory
  handlers/serve-from-realm.ts — serveFromRealm factory
  handlers/serve-index.ts    — serveIndex / serveHostApp factory
                               (closes over the index-html cache that
                               previously lived as class state)
  lib/realm-routing.ts       — findOrMountRealm + getPublishedRealmInfo
                               + isIndexedCardInstance +
                               hasExtensionlessSourceModule +
                               hasPublicPermissions + candidateRealmURLs

No behaviour change. CreateRoutesArgs loses createRealm; the
realm-creation route now wires a CreateRealmDeps bag directly to the
extracted factory. testingOnlyFindOrMountRealm calls the lib helper.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extracts several RealmServer method implementations into focused handler and routing modules to reduce server.ts size and make dependencies more explicit, without intending behavior changes.

Changes:

  • Moved create-realm handling, send-event, serve-index, and serve-from-realm logic into handlers/.
  • Moved realm lookup/routing helpers into lib/realm-routing.ts.
  • Updated server.ts and routes.ts to wire extracted handlers with dependency bags.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/realm-server/server.ts Instantiates extracted handler factories and delegates test-only routing lookup to the new helper.
packages/realm-server/routes.ts Removes createRealm from route args and builds CreateRealmDeps for the new handler.
packages/realm-server/lib/realm-routing.ts Adds extracted realm resolution, published-realm lookup, and routing helper functions.
packages/realm-server/handlers/serve-index.ts Adds extracted host app/index HTML serving logic and index HTML cache closure.
packages/realm-server/handlers/serve-from-realm.ts Adds extracted realm request dispatch middleware.
packages/realm-server/handlers/send-event.ts Adds extracted Matrix event sender factory.
packages/realm-server/handlers/handle-create-realm.ts Removes the old create-realm handler module.
packages/realm-server/handlers/create-realm.ts Adds combined create-realm implementation and Koa request handler.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/realm-server/handlers/send-event.ts
Comment thread packages/realm-server/handlers/create-realm.ts
Comment thread packages/realm-server/handlers/serve-index.ts Outdated
Comment thread packages/realm-server/handlers/serve-index.ts
createServeIndex never reads virtualNetwork — it was carried over by
reflex from the union of class state. Per-module deps should reflect
actual coupling, so remove it from both the type and the call site.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@habdelra habdelra requested a review from a team May 15, 2026 13:42
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

Host Test Results

    1 files  ±0      1 suites  ±0   1h 40m 27s ⏱️ + 10m 54s
2 659 tests ±0  2 644 ✅ ±0  15 💤 ±0  0 ❌ ±0 
2 678 runs  ±0  2 663 ✅ ±0  15 💤 ±0  0 ❌ ±0 

Results for commit 0c9a30c. ± Comparison against earlier commit be11127.

Realm Server Test Results

    1 files  ±0      1 suites  ±0   9m 26s ⏱️ +24s
1 377 tests ±0  1 377 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 458 runs  ±0  1 458 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 0c9a30c. ± Comparison against earlier commit be11127.

habdelra and others added 2 commits May 15, 2026 09:52
…tory

server-config-test was reaching into RealmServer via `(server as any)
.retrieveIndexHTML()` — a private accessor that disappeared with the
factory-closure extraction. Add it back to the ServeIndexHandlers
return type and rewrite the test to construct the factory directly
with the deps it actually exercises, instead of going through a fully
mocked RealmServer.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Threads a skipFromScratchIndex option through
reconciler.lookupOrMount → ensureMounted → realm.start → #startup.
When set, #startup mounts the realm without enqueuing its own
from-scratch-index job.

createRealm now:
  1. enqueues one from-scratch-index job at userInitiatedPriority
  2. lookupOrMount({ skipFromScratchIndex: true }) — mounts without
     enqueuing a duplicate
  3. awaits the priority-10 job's completion before returning

Prior behaviour had two enqueue sites (explicit at priority 10, plus
the implicit one via realm.start at default priority). They were
intended to coalesce via chooseFromScratch keeping maxPriority, but a
worker claim landing between the two inserts moved the first job into
the in-flight bucket — which the from-scratch coalesce ignores — so
the second job survived as a separate priority-0 row that could sit
behind any backlog of system-priority indexing work.

Tightened realm-lifecycle-test to assert exactly one job exists at
userInitiatedPriority (was: "at least one, at least one at p10").

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
habdelra and others added 2 commits May 15, 2026 10:32
A worker claiming the priority-10 job between publish and the
lookupOrMount call would fetch the new realm's _mtimes against this
server-instance. That fetch would land in findOrMountRealm, whose
lazy-mount path calls reconciler.lookupOrMount(url) without the
skipFromScratchIndex option — re-introducing the priority-0 duplicate
enqueue this PR is trying to eliminate.

Reorder so the realm is mounted (via skipFromScratchIndex
lookupOrMount) before the index job is published. Once the realm is
in realms[] / virtualNetwork / the reconciler's `mounted` map, a
worker fetch routed here resolves via the existing mount and never
triggers the lazy-mount path. The sibling-instance race (reconciler
NOTIFY in flight) remains but is the same pre-existing window that
exists for any newly-created realm.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Earlier shape (skipFromScratchIndex + explicit enqueueReindexRealmJob
in createRealm) bypassed RealmIndexUpdater.publishFullIndex, so the
mounted realm's #stats, #ignoreData, and #ignoreDataVersion never
updated when the job completed.

Replace skipFromScratchIndex with fromScratchIndexPriority. The
mount pipeline itself drives the one-and-only from-scratch job at
the chosen priority:

  lookupOrMount(url, { fromScratchIndexPriority })
    → ensureMounted(row, opts)
      → realm.start(opts)
        → #startup(opts)
          → #realmIndexUpdater.fullIndex(opts.fromScratchIndexPriority
                                         ?? this.#fromScratchIndexPriority)
            → publishFullIndex(...)  // .then updates #stats/#ignoreData/...

publishFullIndex remains the single source of truth for full-index
state updates; createRealm just picks the priority. prepareRealmFromRow
publishes the realm into realms[] / virtualNetwork synchronously, so
worker self-fetches that race the mount still resolve via the existing
mount and never re-enter the lazy-mount path.

Drop the now-unused enqueueReindexRealmJob + queue dep from
create-realm.ts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@backspace backspace left a comment

Choose a reason for hiding this comment

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

server.ts has been increasingly unwieldy, I appreciate this cleanup

createRealm: enqueue exactly one priority-10 index job (CS-11157)
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.

3 participants