Merged
Conversation
Contributor
WalkthroughAdded a new JSON specification file providing plan dataset schemas and metadata, and introduced middleware that loads this fallback and replaces Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/middleware/common.middleware.js (1)
395-406: Consider pre-computing the JSON string for performance.
JSON.stringify(planFallback.datasets)is called on every request matching the condition. SinceplanFallbackis a module-level constant, this could be pre-computed once at module load time to avoid repeated serialisation.♻️ Proposed optimisation
const planFallback = JSON.parse(readFileSync(new URL('../../config/plan-fallback.json', import.meta.url), 'utf8')) +const planFallbackDatasetsJson = JSON.stringify(planFallback.datasets)Then in the middleware:
if (fallbackDataset) { - req.specification = { json: JSON.stringify(planFallback.datasets) } + req.specification = { json: planFallbackDatasetsJson } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/common.middleware.js` around lines 395 - 406, Pre-compute the JSON string for planFallback.datasets at module load and use that cached value inside checkSpecificationFallback to avoid re-serialising on every request: create a module-level constant (e.g., PLAN_FALLBACK_DATASETS_JSON) set to JSON.stringify(planFallback.datasets) and replace the inline JSON.stringify(planFallback.datasets) in the checkSpecificationFallback middleware so req.specification = { json: PLAN_FALLBACK_DATASETS_JSON } when the fallbackDataset match is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/middleware/common.middleware.js`:
- Around line 395-406: Pre-compute the JSON string for planFallback.datasets at
module load and use that cached value inside checkSpecificationFallback to avoid
re-serialising on every request: create a module-level constant (e.g.,
PLAN_FALLBACK_DATASETS_JSON) set to JSON.stringify(planFallback.datasets) and
replace the inline JSON.stringify(planFallback.datasets) in the
checkSpecificationFallback middleware so req.specification = { json:
PLAN_FALLBACK_DATASETS_JSON } when the fallbackDataset match is found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe7dfff3-e21a-43b2-8a97-fa05d1ea007e
📒 Files selected for processing (2)
config/plan-fallback.jsonsrc/middleware/common.middleware.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/middleware/common.middleware.js (2)
520-523: Please add regression coverage for this middleware ordering.The new behaviour depends on
checkSpecificationFallbackrunning beforepullOutDatasetSpecification; a small reorder or guard tweak will quietly revert to the old dataset-field path. A targeted test for one matching plan dataset and one non-plan control would lock this down.Based on learnings, tests for
pullOutDatasetSpecificationhave been moved tocommon.middleware.test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/common.middleware.js` around lines 520 - 523, Add regression tests to lock in middleware ordering: create tests in common.middleware.test that exercise processSpecificationMiddlewares ensuring checkSpecificationFallback runs before pullOutDatasetSpecification; specifically add one test with a "plan" dataset spec that should be transformed by checkSpecificationFallback then correctly handled by pullOutDatasetSpecification, and one control test with a non-plan dataset to confirm unchanged behavior. Invoke the middlewares in the same sequence as processSpecificationMiddlewares and assert final request/spec state to catch any accidental reordering or guard changes affecting pullOutDatasetSpecification.
399-403: Use the dataset metadata as the switch, notspecification.specification.This branch only runs after the DB has already returned a row named
local-plan. If the shared row is stillplan— which matchesconfig/plan-fallback.json— gets renamed, or is missing, the override is skipped and we quietly drop back to the dataset-field fallback. Please verify the actual plan collection/spec name here and key this offreq.dataset.collection/req.dataset.datasetinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/common.middleware.js` around lines 399 - 403, The branch should switch on the dataset metadata, not specification.specification: replace the condition "if (specification && specification.specification === 'local-plan')" with a check against the dataset metadata (e.g. "if (req.dataset && (req.dataset.collection === 'local-plan' || req.dataset.dataset === 'local-plan'))"), then keep the existing fallback lookup using planFallback.datasets.find(d => d.dataset === req.dataset.dataset) and set req.specification = { json: PLAN_FALLBACK_DATASETS_JSON } when a fallbackDataset is found; verify and use the actual collection/spec name you expect (req.dataset.collection or req.dataset.dataset) instead of specification.specification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/middleware/common.middleware.js`:
- Around line 520-523: Add regression tests to lock in middleware ordering:
create tests in common.middleware.test that exercise
processSpecificationMiddlewares ensuring checkSpecificationFallback runs before
pullOutDatasetSpecification; specifically add one test with a "plan" dataset
spec that should be transformed by checkSpecificationFallback then correctly
handled by pullOutDatasetSpecification, and one control test with a non-plan
dataset to confirm unchanged behavior. Invoke the middlewares in the same
sequence as processSpecificationMiddlewares and assert final request/spec state
to catch any accidental reordering or guard changes affecting
pullOutDatasetSpecification.
- Around line 399-403: The branch should switch on the dataset metadata, not
specification.specification: replace the condition "if (specification &&
specification.specification === 'local-plan')" with a check against the dataset
metadata (e.g. "if (req.dataset && (req.dataset.collection === 'local-plan' ||
req.dataset.dataset === 'local-plan'))"), then keep the existing fallback lookup
using planFallback.datasets.find(d => d.dataset === req.dataset.dataset) and set
req.specification = { json: PLAN_FALLBACK_DATASETS_JSON } when a fallbackDataset
is found; verify and use the actual collection/spec name you expect
(req.dataset.collection or req.dataset.dataset) instead of
specification.specification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e19a0d8a-827f-425b-82be-48977b5462c6
📒 Files selected for processing (1)
src/middleware/common.middleware.js
gibahjoe
approved these changes
Apr 7, 2026
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
2 participants
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.
Description
config/plan-fallback.jsonplandataset into 4 separate datasets:local-plan— includeslocal-planning-authorities,required-housingsupplementary-plan— includeslocal-planning-authoritiesminerals-plan— includesmineral-planning-authorities,document-countwaste-plan— includeswaste-planning-authorities,document-countreference,name,datasets,period-start-date,period-end-date,documentation-url,document-url,entry-date,notes) present in each datasetplan-timetablekept unchangedsrc/middleware/common.middleware.jsplan-fallback.jsonviareadFileSync+JSON.parse(avoids ESM JSON import attribute requirement)checkSpecificationFallbackmiddleware: whenspecification.specification === 'local-plan', looks upreq.dataset.datasetin the fallback JSON and overridesreq.specificationsopullOutDatasetSpecificationextracts the correct dataset-specific fields from the fallbackWhat type of PR is this? (check all applicable)
Future
This will needed to be added as tech debt - to be removed when it works correctly with the Specification
Summary by CodeRabbit
New Features
Improvements