Removed the route-settings activation bridge - #29744
Merged
Merged
Conversation
ref https://linear.app/ghost/issue/HKG-1898 - the bridge was the last transformation between the RouteSettings domain model and the routing layer; PRs for HKG-1895/1896/1897 relocated all four of its original conversions, leaving only a taxonomy map-to-array flatten - RouterManager now iterates the domain `{tag, author}` map directly and DynamicRoutingService returns the store's model untouched, so the domain model flows store -> service -> routers with no intermediate shape - the bridge also defaulted `templates` to `[]`, which is redundant: both collection-router and static-routes-router already default it themselves - dropped the bridge's falsy-permalink guard rather than porting it — the parser rejects empty taxonomy permalinks outright, so it was unreachable - integrity.test.js now hashes the parsed model instead of bridge output; `yamlSource` is excluded so the canary keeps tracking route structure rather than tripping on comment or whitespace edits to default-routes.yaml
ref https://linear.app/ghost/issue/HKG-1898 - the parser carried a comment justifying its rss handling in terms of the activation bridge and validate.js, both of which are now deleted; the reason stands on its own as parse/serialize round-trip fidelity - dropped a redundant taxonomies assertion that could never fail independently of the deepEqual above it - described the RouterConfig typedef as the domain model rather than "JSON config", matching the typedef now pointing at the domain package
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 2m 25s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 18s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 54s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 43s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 21s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 27s | View ↗ |
nx run-many -t lint -p ghost,ghost-monorepo |
✅ Succeeded | 20s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 3s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-04 11:36:18 UTC
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe route settings pipeline now passes parsed domain Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ref https://linear.app/ghost/issue/HKG-1898 - iterating the domain taxonomies map is the only production behaviour change in this branch, and it had no unit-level test: RouterManager has never had one, so it was covered only indirectly by the legacy and e2e suites - asserts against the registry rather than the routers themselves, which keeps the setup to a urlService stub and avoids standing up the other seven router types the start() path mounts - all three cases fail against the previous array iteration, so they pin the new contract rather than passing vacuously
There was a problem hiding this comment.
Pull request overview
This PR completes the route-settings DDD cleanup by removing the route-settings “activation bridge” layer so the routing system now consumes the RouteSettings domain model directly (store → service → RouterManager → routers), without intermediate reshaping.
Changes:
- Removed the route-settings activation bridge and updated
DynamicRoutingService.loadRouteSettings()to return the store’s parsed domain model directly. - Updated
RouterManager.start()to iterate the domaintaxonomiesmap directly instead of consuming{key, permalink}entries. - Repointed/updated tests and the schema integrity routes canary hash to match the new (bridge-free) hashing input, excluding
yamlSource.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ghost/core/test/unit/server/services/route-settings/route-settings-parser.test.ts | Adds determinism assertions for the parsed domain model serialization stability. |
| ghost/core/test/unit/server/services/route-settings/dynamic-routing-service.test.ts | Updates unit expectations so loadRouteSettings() returns the untransformed domain model. |
| ghost/core/test/unit/server/services/route-settings/activation-bridge.test.ts | Deletes bridge characterization/determinism tests along with the bridge. |
| ghost/core/test/unit/server/data/schema/integrity.test.js | Rehashes default routes canary to hash parsed model (omitting yamlSource) instead of bridge output. |
| ghost/core/test/legacy/mock-express-style/api-vs-frontend.test.js | Updates legacy fixture reshaping helper to stub the domain RouteSettings shape. |
| ghost/core/test/integration/services/route-settings/dynamic-routing-service.test.ts | Updates integration expectations for domain-model return shape (incl. taxonomies map + yamlSource). |
| ghost/core/core/server/services/route-settings/route-settings-parser.ts | Minor comment update; continues producing the domain model with stable property ordering. |
| ghost/core/core/server/services/route-settings/dynamic-routing-service.js | Stops calling the activation bridge; returns store.get() result directly. |
| ghost/core/core/server/services/route-settings/activation-bridge.ts | Removes the route-settings activation bridge implementation. |
| ghost/core/core/frontend/services/routing/static-routes-router.js | Updates internal comment to reference parseRouteSettings as the source of type. |
| ghost/core/core/frontend/services/routing/router-manager.js | Switches start() to accept domain RouteSettings and iterates taxonomy map entries directly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ref https://linear.app/ghost/issue/HKG-1898 - the previous wording claimed parse -> serialize round-trips the author's file unchanged, which overclaims: serializeRouteSettings emits canonical key ordering, so a file whose keys are ordered differently comes back reordered - what the explicit-rss handling actually buys is narrower and worth stating precisely: serializing never writes back an `rss` key the author never wrote
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #29744 +/- ##
==========================================
+ Coverage 75.47% 75.52% +0.04%
==========================================
Files 1613 1612 -1
Lines 142161 142034 -127
Branches 17583 17575 -8
==========================================
- Hits 107296 107265 -31
+ Misses 33820 33724 -96
Partials 1045 1045
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
ref https://linear.app/ghost/issue/HKG-1898
Closes out the DDD Architecture Cleanup milestone: the
RouteSettingsdomain model now flows store → service → RouterManager → routers with no intermediate transformation.Why now
The activation bridge was a temporary adapter introduced in Phase 1 of the refactor. Its four original conversions have all been relocated by earlier PRs in this milestone:
pathRouterManager(#29617){slug}→:slugpermalink-adapter.ts(#29662)resource→ controller nameapi-adapter.ts(#29742)data→{query, router}api-adapter.ts+resolveResourceRead(#29742)That left the bridge doing only two things, both removed here:
taxonomiesfrom the domain{tag, author}map into{key, permalink}[]—RouterManager.start()now iterates the map directly, which is what step 3.7 of the design doc specifies.templatesto[]— redundant, sincecollection-router.js:36andstatic-routes-router.js:17each already default it.Two behaviours deliberately not carried over
if (value)) was unreachable:parseRouteSettingsthrows on an empty taxonomy permalink (route-settings-parser.ts:370, pinned byvalidate-route-settings.test.ts:190), and both stores —FileStoreandS3RouteSettingsStore— produce their model through that parser. Dropping it is a no-op.templates: []default, per above.loadRouteSettings()now returnsyamlSourceThe bridge used to strip it. Both callers (
dynamic-routing-service.js:52,bridge.js:118) hand the object straight torouterManager.start(), which reads only.routes/.collections/.taxonomies, so the extra property is inert.integrity.test.js
currentRoutesHashchanges because it now hashes the parsed model rather than bridge output.yamlSourceis excluded so the canary keeps tracking route structure — including it would trip the test on comment and whitespace edits todefault-routes.yamlthat change no route. The constant is test-local: theroutes_hashsetting was removed in #29643, so nothing at runtime or in the DB consumes it.Testing
Built test-first: the
loadRouteSettingscontract tests and the legacy suite's fixture helper were changed to expect the domain model, then the production code was changed to satisfy them.test/unit/**(route-settings, frontend routing, schema)test/integration/services/route-settings--project legacy(full)test/e2e-frontend/(full)The taxonomy-loop rewrite is the only production behaviour change. It has no unit-level test because no
router-manager.test.jsexists (mockingstart()means standing up eight router types); it is covered byapi-vs-frontend.test.js— which stubsloadRouteSettingsdirectly and so only runs under--project legacy— with real taxonomy maps at:372,:809and:1137, plus the e2e-frontend routing suites.Out of scope
toDomainNotation()inpermalink-adapter.tsis dead production code (only its own unit test calls it). It is unrelated to the bridge and left for a follow-up rather than bundled here.