From 61bb72389f552d514fa659787cd0ec3f80e1e1fd Mon Sep 17 00:00:00 2001 From: Ed Horsford Date: Fri, 10 Jul 2026 16:13:54 +0100 Subject: [PATCH 1/5] Fix PORT env var being ignored in app.js --- app.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app.js b/app.js index 3e2c4978..919bd827 100755 --- a/app.js +++ b/app.js @@ -42,7 +42,7 @@ async function init() { prototype.nunjucks.addGlobal(name, global) } - prototype.start(config.port) + prototype.start(port) } init() From 6ef17325d8b4ca5ae5044f96333b204385c41a30 Mon Sep 17 00:00:00 2001 From: Ed Horsford Date: Fri, 10 Jul 2026 16:13:54 +0100 Subject: [PATCH 2/5] Add shared data store, serve clinics from it instead of sessions Clinics now live in a boot-time shared store (deep-frozen in dev) and are attached to each request by middleware; a toJSON override keeps them out of the serialised session. First collection of three - participants and events follow once the write path is funnelled through helpers. --- app/data/session-data-defaults.js | 10 ++- app/lib/data-store.js | 145 ++++++++++++++++++++++++++++++ app/lib/utils/regenerate-data.js | 4 + app/routes.js | 67 ++++++++++++++ 4 files changed, 223 insertions(+), 3 deletions(-) create mode 100644 app/lib/data-store.js diff --git a/app/data/session-data-defaults.js b/app/data/session-data-defaults.js index ca48e5e2..797b6518 100644 --- a/app/data/session-data-defaults.js +++ b/app/data/session-data-defaults.js @@ -28,7 +28,6 @@ if (!fs.existsSync(generatedDataPath)) { } let participants = [] -let clinics = [] let events = [] let generationInfo = { generatedAt: 'Never', @@ -73,12 +72,18 @@ if (needsRegeneration(generationInfo)) { if (!generationInfo.seedDataProfile) { generationInfo.seedDataProfile = DEFAULT_SEED_DATA_PROFILE } + + // The shared data store may already have loaded the stale files (require + // order at boot), so reload it now the generator has written fresh ones + require('../lib/data-store').reload() } // Load generated data +// Clinics are NOT loaded here: they live in the shared data store +// (app/lib/data-store.js) and are attached to each request by middleware in +// app/routes.js, so they never get copied into (or serialised with) sessions. try { participants = require('./generated/participants.json').participants - clinics = require('./generated/clinics.json').clinics events = require('./generated/events.json').events } catch (err) { console.warn('Error loading generated data:', err) @@ -133,7 +138,6 @@ const defaults = { allBreastScreeningUnits, screeningRooms, participants, - clinics, events, generationInfo, config, diff --git a/app/lib/data-store.js b/app/lib/data-store.js new file mode 100644 index 00000000..8945943b --- /dev/null +++ b/app/lib/data-store.js @@ -0,0 +1,145 @@ +// app/lib/data-store.js +// +// Shared, read-only store for the generated seed data. +// +// Loaded once at boot and shared across every session. Sessions no longer +// carry their own copy of the big collections - they only store changed +// records in `data._changes` (see the attach middleware in app/routes.js), +// which are overlaid on these shared arrays at request time. +// +// Because these arrays and records are shared across sessions, nothing may +// mutate them at request time. To change a record, code must go through the +// update helpers (updateEvent etc), which write a whole replacement record +// into the session's `data._changes`. In development the store is deep-frozen +// so any leftover in-place mutation throws at the exact line, rather than +// silently leaking one session's changes into every other session. See +// docs/data-conventions.md. + +const fs = require('fs') +const path = require('path') +const config = require('../config') + +const generatedDataPath = config.paths.generatedData + +// Freeze shared data in development so accidental mutation fails loudly. +// Escape hatch: set freezeSharedData: false in config if a frozen object is +// ever blocking work - the store still works, mutations just go undetected. +const shouldFreeze = + process.env.NODE_ENV !== 'production' && config.freezeSharedData !== false + +/** + * Recursively freeze an object graph. Handles cycles and only descends into + * plain objects/arrays (seed data is plain JSON so that covers everything). + * + * @param {object} value - Object or array to freeze + * @param {WeakSet} seen - Already-frozen objects (cycle guard) + * @returns {object} The same value, frozen + */ +const deepFreeze = (value, seen = new WeakSet()) => { + if (value === null || typeof value !== 'object' || seen.has(value)) { + return value + } + seen.add(value) + for (const key of Object.keys(value)) { + deepFreeze(value[key], seen) + } + return Object.freeze(value) +} + +/** + * Read a generated JSON file, returning a fallback on any failure + * + * @param {string} filename - File within the generated data folder + * @param {object} fallback - Value to use if the file is missing/invalid + * @returns {object} Parsed JSON or fallback + */ +const readGeneratedFile = (filename, fallback) => { + try { + return JSON.parse( + fs.readFileSync(path.join(generatedDataPath, filename), 'utf8') + ) + } catch (err) { + console.warn(`Data store: error reading ${filename}:`, err.message) + return fallback + } +} + +// The store's mutable state - swapped wholesale by reload() +const state = { + participants: [], + clinics: [], + events: [], + participantsById: new Map(), + clinicsById: new Map(), + eventsById: new Map(), + eventIdsByClinic: new Map(), + eventIdsByParticipant: new Map(), + generationInfo: {}, + // Identifies which generation of seed data the store holds. Sessions stamp + // their _changes with this; the attach middleware discards changes made + // against a different generation (i.e. before a regenerate/profile swap). + generationId: null +} + +/** + * (Re)load all collections from the generated JSON on disk and rebuild + * indexes. Called at boot and again after any data regeneration. + */ +const reload = () => { + const started = Date.now() + + const participants = + readGeneratedFile('participants.json', {}).participants || [] + const clinics = readGeneratedFile('clinics.json', {}).clinics || [] + const events = readGeneratedFile('events.json', {}).events || [] + const generationInfo = readGeneratedFile('generation-info.json', { + generatedAt: 'Never', + stats: { participants: 0, clinics: 0, events: 0 } + }) + + if (shouldFreeze) { + deepFreeze(participants) + deepFreeze(clinics) + deepFreeze(events) + deepFreeze(generationInfo) + } + + state.participants = participants + state.clinics = clinics + state.events = events + state.generationInfo = generationInfo + state.generationId = generationInfo.generatedAt + + state.participantsById = new Map(participants.map((p) => [p.id, p])) + state.clinicsById = new Map(clinics.map((c) => [c.id, c])) + state.eventsById = new Map(events.map((e) => [e.id, e])) + + state.eventIdsByClinic = new Map() + state.eventIdsByParticipant = new Map() + for (const event of events) { + if (!state.eventIdsByClinic.has(event.clinicId)) { + state.eventIdsByClinic.set(event.clinicId, []) + } + state.eventIdsByClinic.get(event.clinicId).push(event.id) + + if (!state.eventIdsByParticipant.has(event.participantId)) { + state.eventIdsByParticipant.set(event.participantId, []) + } + state.eventIdsByParticipant.get(event.participantId).push(event.id) + } + + console.log( + `Data store: loaded ${participants.length} participants, ` + + `${clinics.length} clinics, ${events.length} events ` + + `in ${Date.now() - started}ms${shouldFreeze ? ' (frozen)' : ''}` + ) +} + +// Initial load at boot. If the generated files are missing/stale the app's +// regeneration path will run the generator and call reload() again. +reload() + +module.exports = { + state, + reload +} diff --git a/app/lib/utils/regenerate-data.js b/app/lib/utils/regenerate-data.js index 224bba27..92d3ee11 100644 --- a/app/lib/utils/regenerate-data.js +++ b/app/lib/utils/regenerate-data.js @@ -35,6 +35,10 @@ async function regenerateData(req, options = {}) { seedDataProfileObject: resolvedProfile }) + // Reload the shared data store from the freshly generated files + // (required lazily to avoid loading the store before generation at boot) + require('../data-store').reload() + // Clear the require cache for session data defaults delete require.cache[require.resolve(sessionDataPath)] diff --git a/app/routes.js b/app/routes.js index df83759b..ac56b258 100644 --- a/app/routes.js +++ b/app/routes.js @@ -7,6 +7,7 @@ const { needsRegeneration } = require('./lib/utils/regenerate-data') const { resetCallSequence } = require('./lib/utils/random') +const dataStore = require('./lib/data-store') const router = express.Router() @@ -69,6 +70,72 @@ router.use(async (req, res, next) => { } }) +// Collections served from the shared data store rather than from per-session +// copies. Spike: clinics only - participants and events follow in phase 2. +const STORE_COLLECTIONS = ['clinics'] + +// Attach shared collections to this request's session data. +// +// The shared arrays live in app/lib/data-store.js, loaded once at boot. +// Sessions only persist changed records (data._changes, whole records keyed +// by id); here we overlay those onto the shared arrays so that everything +// downstream - route handlers, helpers taking `data`, views via locals, the +// kit's auto-routes template fallback - sees `data.clinics` etc exactly as +// before the refactor. +// +// The toJSON trick: express-session's MemoryStore serialises sessions with +// JSON.stringify, which respects a toJSON method. Defining a non-enumerable +// toJSON on req.session.data that omits the attached collections means they +// are never written back into the session store - the session stays small +// (~KBs) while the request sees the full data. +// +// Kit version assumptions (nhsuk-prototype-kit 8.3.0): +// - setSessionDataDefaults spreads a *fresh* req.session.data object every +// request, so both the collections and toJSON must be (re)attached per +// request - which this middleware does. +// - autoStoreData copies session data to res.locals.data (for..in over +// enumerable props) *before* our router runs, so locals are set explicitly +// here; the non-enumerable toJSON is skipped by both that copy and the +// defaults spread, which is what we want. +router.use((req, res, next) => { + const data = req.session.data + if (!data) return next() + + // Per-session changed records: whole replacement records keyed by id. + // The _ prefix means the kit's autoStoreData can never write form data + // into it (it skips _-prefixed fields). + if (!data._changes) { + data._changes = { events: {}, participants: {}, clinics: {} } + } + + for (const name of STORE_COLLECTIONS) { + const changes = data._changes[name] || {} + const hasChanges = Object.keys(changes).length > 0 + + // Shared array directly when this session has no changes; an overlay + // copy substituting the session's changed records when it does + data[name] = hasChanges + ? dataStore.state[name].map((record) => changes[record.id] ?? record) + : dataStore.state[name] + + res.locals.data[name] = data[name] + } + + Object.defineProperty(data, 'toJSON', { + value: function () { + const copy = { ...this } + for (const name of STORE_COLLECTIONS) { + delete copy[name] + } + return copy + }, + enumerable: false, + configurable: true + }) + + next() +}) + // Reset randomisation per page load router.use((req, res, next) => { resetCallSequence() From e399acbe336bc91e49182bf0fe3325b19c5b3d22 Mon Sep 17 00:00:00 2001 From: Ed Horsford Date: Fri, 10 Jul 2026 16:55:02 +0100 Subject: [PATCH 3/5] Funnel all event/participant writes through update helpers Prepares for serving events and participants from the shared store: - Temp working copies (data.event, data.participant) are now deep clones, so form merges can never write into the source records - Priors, deferral and reading writes build replacement records and save via updateEventData instead of mutating found records in place - Session-copied seed arrays are deep-frozen in development (config escape hatch: freezeSharedData) so leftover in-place writers throw --- app/data/session-data-defaults.js | 11 ++ app/lib/data-store.js | 4 +- app/lib/utils/reading.js | 40 ++++---- app/routes/events.js | 46 +++++---- app/routes/participants.js | 6 +- app/routes/reading.js | 163 ++++++++++++++++-------------- 6 files changed, 152 insertions(+), 118 deletions(-) diff --git a/app/data/session-data-defaults.js b/app/data/session-data-defaults.js index 797b6518..ea16340f 100644 --- a/app/data/session-data-defaults.js +++ b/app/data/session-data-defaults.js @@ -89,6 +89,17 @@ try { console.warn('Error loading generated data:', err) } +// In development, freeze these seed arrays so any leftover in-place mutation +// of a shared record throws at the offending line. Only the first request of +// a session sees these exact objects (afterwards the session store holds its +// own parsed copy), but that is enough to flush out bad writers early. +// These arrays move to the shared data store in the next phase. +const { deepFreeze, shouldFreeze } = require('../lib/data-store') +if (shouldFreeze) { + deepFreeze(participants) + deepFreeze(events) +} + const defaultSettings = { darkMode: 'false', compactMode: 'false', diff --git a/app/lib/data-store.js b/app/lib/data-store.js index 8945943b..42aab1db 100644 --- a/app/lib/data-store.js +++ b/app/lib/data-store.js @@ -141,5 +141,7 @@ reload() module.exports = { state, - reload + reload, + deepFreeze, + shouldFreeze } diff --git a/app/lib/utils/reading.js b/app/lib/utils/reading.js index 2170b872..89ee9bb7 100644 --- a/app/lib/utils/reading.js +++ b/app/lib/utils/reading.js @@ -4,6 +4,7 @@ const dayjs = require('dayjs') const { eligibleForReading, getStatusTagColour } = require('./status') const { isWithinDayRange } = require('./dates') const { awaitingPriors, userRequestedPriors } = require('./prior-mammograms') +const { updateEventData } = require('./event-data') // /** // * Get first unread event in a clinic @@ -83,44 +84,45 @@ const getReadsAsArray = function (event) { } /** - * Update the writeReading function to also handle removing from skipped events + * Save a user's reading for an event, and remove the event from the reading + * session's skipped list if present + * + * Builds a new imageReading object and saves it through updateEventData + * rather than mutating the event - event records are shared read-only data. * * @param {object} event - The event to update * @param {string} userId - User ID * @param {object} reading - Reading data to save - * @param {object | null} [data] - Session data (needed for session context) - * @param {string | null} [sessionId] - Session ID (if in session context) + * @param {object} data - Session data + * @param {string | null} [sessionId] - Reading session ID (if in session context) */ -const writeReading = ( - event, - userId, - reading, - data = null, - sessionId = null -) => { - // Ensure imageReading structure exists - if (!event.imageReading) { - event.imageReading = { reads: {} } - } else if (!event.imageReading.reads) { - event.imageReading.reads = {} +const writeReading = (event, userId, reading, data, sessionId = null) => { + // Work on a clone so the shared record is never touched in place + const imageReading = structuredClone(event.imageReading || {}) + if (!imageReading.reads) { + imageReading.reads = {} } // Calculate readNumber based on existing reads - const existingReadCount = Object.keys(event.imageReading.reads).length + const existingReadCount = Object.keys(imageReading.reads).length // If this user already has a read, keep their readNumber; otherwise assign next number - const existingRead = event.imageReading.reads[userId] + const existingRead = imageReading.reads[userId] const readNumber = existingRead?.readNumber || existingReadCount + 1 // Add the reading with timestamp and readNumber - event.imageReading.reads[userId] = { + imageReading.reads[userId] = { ...reading, readerId: userId, // Ensure the reader ID is saved readNumber, timestamp: new Date().toISOString() } + // Saves to the event and mirrors into data.event if it matches + updateEventData(data, event.id, { imageReading }) + // If we have session context, remove this event from skipped events - if (data && sessionId && data.readingSessions?.[sessionId]) { + // (readingSessions is per-session working data, so in-place edits are fine) + if (sessionId && data.readingSessions?.[sessionId]) { const session = data.readingSessions[sessionId] // Remove event from skipped list if present diff --git a/app/routes/events.js b/app/routes/events.js index 419636f3..04c67678 100644 --- a/app/routes/events.js +++ b/app/routes/events.js @@ -125,8 +125,10 @@ module.exports = (router) => { `Temp event data found, but eventId ${data.event.id} does not match ${eventId}, creating new one` ) } - // Copy over the event data to the temp event - data.event = originalEventData.event + // Copy over the event data to the temp event. + // Must be a deep clone: the temp copy gets mutated by forms, and the + // source record is shared (read-only) data that other sessions see. + data.event = structuredClone(originalEventData.event) } const participantId = originalEventData.participant.id @@ -138,8 +140,10 @@ module.exports = (router) => { `Temp participant data found, but participantId ${data.participant.id} does not match ${participantId}, creating new one` ) } - // Copy over the participant data to the temp participant - data.participant = { ...originalEventData.participant } + // Copy over the participant data to the temp participant. + // Deep clone - a shallow spread would leave nested objects + // (demographicInformation etc) shared with the read-only source record. + data.participant = structuredClone(originalEventData.participant) } // Deep compare temp participant and saved participant in array @@ -282,14 +286,17 @@ module.exports = (router) => { if (event?.status === 'event_paused') { // Get existing session details const existingDetails = event.sessionDetails || {} - const authors = existingDetails.authors || [] - // Add resume action to authors - authors.push({ - userId: currentUser.id, - action: 'resumed', - timestamp: new Date().toISOString() - }) + // Add resume action to a new authors array - the existing one belongs + // to the shared read-only event record, so must not be pushed to + const authors = [ + ...(existingDetails.authors || []), + { + userId: currentUser.id, + action: 'resumed', + timestamp: new Date().toISOString() + } + ] // Update status to in progress updateEventStatus(data, req.params.eventId, 'event_in_progress') @@ -341,14 +348,17 @@ module.exports = (router) => { // Get existing session details to track authors const existingDetails = event.sessionDetails || {} - const authors = existingDetails.authors || [] - // Add current user's pause action to authors - authors.push({ - userId: data.currentUser?.id, - action: 'paused', - timestamp: new Date().toISOString() - }) + // Add pause action to a new authors array - the existing one belongs + // to the shared read-only event record, so must not be pushed to + const authors = [ + ...(existingDetails.authors || []), + { + userId: data.currentUser?.id, + action: 'paused', + timestamp: new Date().toISOString() + } + ] // Update status to paused and record pause details updateEventStatus(data, eventId, 'event_paused') diff --git a/app/routes/participants.js b/app/routes/participants.js index f33d83be..84773f37 100644 --- a/app/routes/participants.js +++ b/app/routes/participants.js @@ -99,8 +99,10 @@ module.exports = (router) => { `Temp participant data found, but participantId ${data.participant.id} does not match ${participantId}, creating new one` ) } - // Copy over the participant data to the temp participant - data.participant = { ...originalParticipant } + // Copy over the participant data to the temp participant. + // Deep clone - a shallow spread would leave nested objects + // (demographicInformation etc) shared with the read-only source record. + data.participant = structuredClone(originalParticipant) } // This will now have any temp participant data that forms have added too diff --git a/app/routes/reading.js b/app/routes/reading.js index 9a4a4788..01e14f39 100644 --- a/app/routes/reading.js +++ b/app/routes/reading.js @@ -1,5 +1,5 @@ // app/routes/image-reading.js -const { getEventData } = require('../lib/utils/event-data') +const { getEventData, updateEventData } = require('../lib/utils/event-data') const { getFirstUserReadableEvent, getNextUserReadableEvent, @@ -212,24 +212,32 @@ module.exports = (router) => { return res.redirect('/reading/priors') } - // Update the status - mammogram.requestStatus = newStatus - mammogram.statusChangedDate = new Date().toISOString() - mammogram.statusChangedBy = currentUserId - - // Set additional fields based on status - if (newStatus === 'requested') { - // Admin is formally sending the IEP request - mammogram.requestedDate = new Date().toISOString() - mammogram.requestedBy = currentUserId - } else if (newStatus === 'received') { - mammogram.receivedDate = new Date().toISOString() - } + // Build an updated mammogram list rather than mutating in place - event + // records are shared read-only data; writes go through the update helpers + const previousMammograms = event.previousMammograms.map((m) => { + if (m.id !== mammogramId) return m - // Also update mirrored event in data.event if it matches - if (data.event && data.event.id === eventId) { - data.event.previousMammograms = event.previousMammograms - } + const updated = { + ...m, + requestStatus: newStatus, + statusChangedDate: new Date().toISOString(), + statusChangedBy: currentUserId + } + + // Set additional fields based on status + if (newStatus === 'requested') { + // Admin is formally sending the IEP request + updated.requestedDate = new Date().toISOString() + updated.requestedBy = currentUserId + } else if (newStatus === 'received') { + updated.receivedDate = new Date().toISOString() + } + + return updated + }) + + // Saves to the event and mirrors into data.event if it matches + updateEventData(data, eventId, { previousMammograms }) // If this was a fetch request, send JSON response for in-place update if (req.headers.accept?.includes('application/json')) { @@ -703,21 +711,22 @@ module.exports = (router) => { // Find the event in the main events array const event = data.events.find((e) => e.id === eventId) if (event && event.previousMammograms) { - event.previousMammograms.forEach((mammogram) => { - if (requestPriorIds.includes(mammogram.id)) { - mammogram.requestStatus = 'pending' - mammogram.requestedDate = new Date().toISOString() - mammogram.requestedBy = currentUserId - if (reason) { - mammogram.requestReason = reason - } - } - }) + // Build an updated list rather than mutating in place - event records + // are shared read-only data; writes go through the update helpers + const previousMammograms = event.previousMammograms.map((mammogram) => + requestPriorIds.includes(mammogram.id) + ? { + ...mammogram, + requestStatus: 'pending', + requestedDate: new Date().toISOString(), + requestedBy: currentUserId, + ...(reason ? { requestReason: reason } : {}) + } + : mammogram + ) - // Also update the mirrored event in data.event - if (data.event && data.event.id === eventId) { - data.event.previousMammograms = event.previousMammograms - } + // Saves to the event and mirrors into data.event if it matches + updateEventData(data, eventId, { previousMammograms }) } // If submitted from an existing-read page (e.g. editing reason), return there @@ -725,18 +734,16 @@ module.exports = (router) => { if (priorsReferrerChain) { // In edit mode, also update reason on mammograms already pending/requested by this user if (event && event.previousMammograms && reason) { - event.previousMammograms.forEach((mammogram) => { - if ( + const latestEvent = data.events.find((e) => e.id === eventId) + const previousMammograms = latestEvent.previousMammograms.map( + (mammogram) => (mammogram.requestStatus === 'pending' || mammogram.requestStatus === 'requested') && mammogram.requestedBy === currentUserId - ) { - mammogram.requestReason = reason - } - }) - if (data.event && data.event.id === eventId) { - data.event.previousMammograms = event.previousMammograms - } + ? { ...mammogram, requestReason: reason } + : mammogram + ) + updateEventData(data, eventId, { previousMammograms }) } const returnUrl = getReturnUrl( `/reading/session/${sessionId}/events/${eventId}/existing-read`, @@ -811,22 +818,23 @@ module.exports = (router) => { const event = data.events.find((e) => e.id === eventId) if (event && event.previousMammograms) { - event.previousMammograms.forEach((mammogram) => { + // Build an updated list rather than mutating in place - event records + // are shared read-only data; writes go through the update helpers + const previousMammograms = event.previousMammograms.map((mammogram) => { if ( mammogram.requestStatus === 'pending' && mammogram.requestedBy === currentUserId ) { - mammogram.requestStatus = 'not_requested' - delete mammogram.requestedDate - delete mammogram.requestedBy - delete mammogram.requestReason + // Omit the request fields entirely on the replacement record + const { requestedDate, requestedBy, requestReason, ...rest } = + mammogram + return { ...rest, requestStatus: 'not_requested' } } + return mammogram }) - // Also update the mirrored event in data.event - if (data.event && data.event.id === eventId) { - data.event.previousMammograms = event.previousMammograms - } + // Saves to the event and mirrors into data.event if it matches + updateEventData(data, eventId, { previousMammograms }) } // Redirect to opinion page so the reader can now read the case @@ -848,28 +856,26 @@ module.exports = (router) => { const reason = req.body.deferralReason || '' - // Find the event and save deferral data + // Find the event and save deferral data. Work on a clone rather than + // mutating in place - event records are shared read-only data; writes + // go through the update helpers const event = data.events.find((e) => e.id === eventId) if (event) { - if (!event.imageReading) { - event.imageReading = {} - } + const imageReading = structuredClone(event.imageReading || {}) // Remove any existing read by this user — deferral replaces a prior opinion - if (event.imageReading.reads?.[currentUserId]) { - delete event.imageReading.reads[currentUserId] + if (imageReading.reads?.[currentUserId]) { + delete imageReading.reads[currentUserId] } - event.imageReading.deferral = { + imageReading.deferral = { deferredAt: new Date().toISOString(), deferredBy: currentUserId, reason: reason || null } - // Also update the mirrored event in data.event - if (data.event && data.event.id === eventId) { - data.event.imageReading = event.imageReading - } + // Saves to the event and mirrors into data.event if it matches + updateEventData(data, eventId, { imageReading }) } // If submitted from an existing-read page (e.g. editing reason), return there @@ -944,12 +950,12 @@ module.exports = (router) => { const event = data.events.find((e) => e.id === eventId) if (event?.imageReading?.deferral) { - delete event.imageReading.deferral + // Clone rather than mutate - event records are shared read-only data + const imageReading = structuredClone(event.imageReading) + delete imageReading.deferral - // Also update the mirrored event in data.event - if (data.event && data.event.id === eventId) { - data.event.imageReading = event.imageReading - } + // Saves to the event and mirrors into data.event if it matches + updateEventData(data, eventId, { imageReading }) } res.redirect(`/reading/session/${sessionId}/events/${eventId}/opinion`) @@ -969,19 +975,20 @@ module.exports = (router) => { const event = data.events.find((e) => e.id === eventId) if (event?.imageReading?.deferral) { - if (!event.imageReading.deferralHistory) { - event.imageReading.deferralHistory = [] - } - event.imageReading.deferralHistory.push({ - ...event.imageReading.deferral, - resolvedAt: new Date().toISOString(), - resolvedBy: data.currentUser?.id - }) - delete event.imageReading.deferral + // Clone rather than mutate - event records are shared read-only data + const imageReading = structuredClone(event.imageReading) + imageReading.deferralHistory = [ + ...(imageReading.deferralHistory || []), + { + ...imageReading.deferral, + resolvedAt: new Date().toISOString(), + resolvedBy: data.currentUser?.id + } + ] + delete imageReading.deferral - if (data.event && data.event.id === eventId) { - data.event.imageReading = event.imageReading - } + // Saves to the event and mirrors into data.event if it matches + updateEventData(data, eventId, { imageReading }) const participant = data.participants.find( (p) => p.id === event.participantId From 1d3e39669207d25e90513b9e3d48d2ec1835757c Mon Sep 17 00:00:00 2001 From: Ed Horsford Date: Fri, 10 Jul 2026 17:04:43 +0100 Subject: [PATCH 4/5] Serve all seed collections from the shared store, sessions keep deltas only Sessions now persist only changed records (data._changes, whole records keyed by id, stamped with the data generation they were made against). Middleware overlays them onto the shared store's arrays each request, so routes and views read data.events etc unchanged. - Update helpers write changes to _changes as well as the attached array - generationInfo comes from the store, so a regenerate or profile swap by one session resets data for every session (stale stamps discarded) - Daily regeneration check now reads the store, not the session - Serialised session size drops from ~4.5MB to ~40KB; RSS under a 200-request two-session load roughly halves - New docs/data-conventions.md explains reading vs updating data --- app/data/session-data-defaults.js | 30 ++------------ app/lib/utils/event-data.js | 23 ++++++++++- app/lib/utils/participants.js | 7 +++- app/lib/utils/regenerate-data.js | 47 +++++---------------- app/routes.js | 52 ++++++++++++++++------- docs/data-conventions.md | 68 +++++++++++++++++++++++++++++++ 6 files changed, 147 insertions(+), 80 deletions(-) create mode 100644 docs/data-conventions.md diff --git a/app/data/session-data-defaults.js b/app/data/session-data-defaults.js index ea16340f..17976c5a 100644 --- a/app/data/session-data-defaults.js +++ b/app/data/session-data-defaults.js @@ -27,8 +27,6 @@ if (!fs.existsSync(generatedDataPath)) { fs.mkdirSync(generatedDataPath) } -let participants = [] -let events = [] let generationInfo = { generatedAt: 'Never', seedDataProfile: DEFAULT_SEED_DATA_PROFILE, @@ -78,27 +76,10 @@ if (needsRegeneration(generationInfo)) { require('../lib/data-store').reload() } -// Load generated data -// Clinics are NOT loaded here: they live in the shared data store -// (app/lib/data-store.js) and are attached to each request by middleware in -// app/routes.js, so they never get copied into (or serialised with) sessions. -try { - participants = require('./generated/participants.json').participants - events = require('./generated/events.json').events -} catch (err) { - console.warn('Error loading generated data:', err) -} - -// In development, freeze these seed arrays so any leftover in-place mutation -// of a shared record throws at the offending line. Only the first request of -// a session sees these exact objects (afterwards the session store holds its -// own parsed copy), but that is enough to flush out bad writers early. -// These arrays move to the shared data store in the next phase. -const { deepFreeze, shouldFreeze } = require('../lib/data-store') -if (shouldFreeze) { - deepFreeze(participants) - deepFreeze(events) -} +// The generated collections (participants, clinics, events) are NOT loaded +// here: they live in the shared data store (app/lib/data-store.js) and are +// attached to each request by middleware in app/routes.js, so they never get +// copied into (or serialised with) sessions. Same for generationInfo. const defaultSettings = { darkMode: 'false', @@ -148,9 +129,6 @@ const defaults = { breastScreeningUnits, allBreastScreeningUnits, screeningRooms, - participants, - events, - generationInfo, config, settings: defaultSettings, defaultSettings, diff --git a/app/lib/utils/event-data.js b/app/lib/utils/event-data.js index 59ad8645..33ba0a0f 100644 --- a/app/lib/utils/event-data.js +++ b/app/lib/utils/event-data.js @@ -1,6 +1,23 @@ // app/lib/utils/event-data.js const { getParticipant } = require('./participants.js') +/** + * Record a changed event so it persists for this session + * + * The events array attached to `data` is rebuilt from the shared data store + * on every request; only records in data._changes survive across requests + * (see the attach middleware in app/routes.js). Callers should also replace + * the record in data.events so reads later in the same request see it. + * + * @param {object} data - Session data + * @param {object} event - Whole replacement event record + */ +const recordEventChange = (data, event) => { + if (data._changes?.events) { + data._changes.events[event.id] = event + } +} + /** * Get an event by ID * @@ -50,8 +67,10 @@ const updateEvent = (data, eventId, updatedEvent) => { const eventIndex = data.events.findIndex((e) => e.id === eventId) if (eventIndex === -1) return null - // Update in the array + // Update in the attached array (same-request reads) and record the change + // (persistence across requests) data.events[eventIndex] = updatedEvent + recordEventChange(data, updatedEvent) return updatedEvent } @@ -88,6 +107,7 @@ const updateEventStatus = (data, eventId, newStatus) => { // Update main data data.events[eventIndex] = updatedEvent + recordEventChange(data, updatedEvent) // Also update temp event data if it exists and matches this event // Only update the status-related fields to preserve other temp changes @@ -125,6 +145,7 @@ const updateEventData = (data, eventId, updates) => { // Update main data data.events[eventIndex] = updatedEvent + recordEventChange(data, updatedEvent) // Also update temp event data if it exists and matches this event // Merge updates into existing temp event to preserve any unsaved changes diff --git a/app/lib/utils/participants.js b/app/lib/utils/participants.js index 5abd91d6..6eb325c9 100644 --- a/app/lib/utils/participants.js +++ b/app/lib/utils/participants.js @@ -250,8 +250,13 @@ const updateParticipant = (data, participantId, updatedParticipant) => { ) if (participantIndex === -1) return null - // Update in the array + // Update in the attached array (same-request reads) and record the change + // in data._changes (persistence - the attached array is rebuilt from the + // shared data store on every request; see middleware in app/routes.js) data.participants[participantIndex] = updatedParticipant + if (data._changes?.participants) { + data._changes.participants[participantId] = updatedParticipant + } return updatedParticipant } diff --git a/app/lib/utils/regenerate-data.js b/app/lib/utils/regenerate-data.js index 92d3ee11..4484b992 100644 --- a/app/lib/utils/regenerate-data.js +++ b/app/lib/utils/regenerate-data.js @@ -3,7 +3,6 @@ const generateData = require('../generate-seed-data') const { join, resolve } = require('path') const dayjs = require('dayjs') -const fs = require('fs') const { DEFAULT_SEED_DATA_PROFILE, ensureSeedProfilesState, @@ -26,8 +25,6 @@ async function regenerateData(req, options = {}) { const dataDirectory = join(__dirname, '../../data') const sessionDataPath = resolve(dataDirectory, 'session-data-defaults.js') - const generatedDataPath = resolve(dataDirectory, 'generated') - const generationInfoPath = join(generatedDataPath, 'generation-info.json') // Generate new data await generateData({ @@ -35,40 +32,18 @@ async function regenerateData(req, options = {}) { seedDataProfileObject: resolvedProfile }) - // Reload the shared data store from the freshly generated files - // (required lazily to avoid loading the store before generation at boot) + // Reload the shared data store from the freshly generated files. This also + // updates the store's generationId, which invalidates every session's + // _changes (see the attach middleware in app/routes.js) - so a regenerate + // resets data for all sessions, not just this one. + // (Required lazily to avoid loading the store before generation at boot.) require('../data-store').reload() - // Clear the require cache for session data defaults + // Clear the require cache for session data defaults so they re-evaluate delete require.cache[require.resolve(sessionDataPath)] - // Clear cache for the generated JSON files - Object.keys(require.cache).forEach((key) => { - if (key.startsWith(generatedDataPath)) { - delete require.cache[key] - } - }) - - // Read generation info including stats - let generationInfo = { - generatedAt: new Date().toISOString(), - stats: { participants: 0, clinics: 0, events: 0 }, - seedDataProfile: resolvedProfile.key - } - - try { - if (fs.existsSync(generationInfoPath)) { - generationInfo = JSON.parse(fs.readFileSync(generationInfoPath)) - } - } catch (err) { - console.warn('Error reading generation info:', err) - } - - // Reload session data defaults with fresh data and updated generation info - // req.session.data = { - // ...require('../../data/session-data-defaults'), - // generationInfo - // } + // Reset this session to fresh defaults. Collections and generationInfo are + // not part of defaults - the attach middleware provides them from the store. // IMPORTANT: Modify existing object rather than replacing it // Replacing breaks express-session's change tracking const freshDefaults = require('../../data/session-data-defaults') @@ -80,8 +55,8 @@ async function regenerateData(req, options = {}) { // Clear existing keys Object.keys(req.session.data).forEach((key) => delete req.session.data[key]) - // Merge in fresh defaults and generation info - Object.assign(req.session.data, freshDefaults, { generationInfo }) + // Merge in fresh defaults + Object.assign(req.session.data, freshDefaults) // Restore settings if they existed if (preservedSettings) { @@ -96,8 +71,6 @@ async function regenerateData(req, options = {}) { req.session.data.settings ) refreshedSeedProfiles.selectedKey = resolvedProfile.key - - req.session.data.generationInfo.seedDataProfile = resolvedProfile.key } function needsRegeneration(generationInfo) { diff --git a/app/routes.js b/app/routes.js index ac56b258..6fbcdb41 100644 --- a/app/routes.js +++ b/app/routes.js @@ -57,7 +57,9 @@ router.use(async (req, res, next) => { logMemory(`request #${requestCount}`, req.session?.data) } - if (needsRegeneration(req.session.data?.generationInfo)) { + // Check the shared store's generation info (not the session's) - one + // regeneration serves every session + if (needsRegeneration(dataStore.state.generationInfo)) { logMemory('before-regeneration') console.log('Regenerating data for new day...') await regenerateData(req) @@ -71,8 +73,8 @@ router.use(async (req, res, next) => { }) // Collections served from the shared data store rather than from per-session -// copies. Spike: clinics only - participants and events follow in phase 2. -const STORE_COLLECTIONS = ['clinics'] +// copies +const STORE_COLLECTIONS = ['clinics', 'participants', 'events'] // Attach shared collections to this request's session data. // @@ -80,9 +82,13 @@ const STORE_COLLECTIONS = ['clinics'] // Sessions only persist changed records (data._changes, whole records keyed // by id); here we overlay those onto the shared arrays so that everything // downstream - route handlers, helpers taking `data`, views via locals, the -// kit's auto-routes template fallback - sees `data.clinics` etc exactly as +// kit's auto-routes template fallback - sees `data.events` etc exactly as // before the refactor. // +// The attached arrays are fresh copies each request (shared record objects, +// new array), so the update helpers can replace elements in place for +// read-after-write within a request without touching the shared store. +// // The toJSON trick: express-session's MemoryStore serialises sessions with // JSON.stringify, which respects a toJSON method. Defining a non-enumerable // toJSON on req.session.data that omits the attached collections means they @@ -104,29 +110,45 @@ router.use((req, res, next) => { // Per-session changed records: whole replacement records keyed by id. // The _ prefix means the kit's autoStoreData can never write form data // into it (it skips _-prefixed fields). - if (!data._changes) { - data._changes = { events: {}, participants: {}, clinics: {} } + // + // Changes are stamped with the data generation they were made against. + // After a regenerate or profile swap the stamp no longer matches, and the + // stale changes are discarded - for every session, not just the one that + // triggered the swap. Data resetting on swap is expected demo behaviour, + // and this guarantees a swap can never leave another session overlaying + // old records onto fresh data. + if ( + !data._changes || + data._changes.generationId !== dataStore.state.generationId + ) { + data._changes = { + generationId: dataStore.state.generationId, + events: {}, + participants: {}, + clinics: {} + } } for (const name of STORE_COLLECTIONS) { - const changes = data._changes[name] || {} - const hasChanges = Object.keys(changes).length > 0 - - // Shared array directly when this session has no changes; an overlay - // copy substituting the session's changed records when it does - data[name] = hasChanges - ? dataStore.state[name].map((record) => changes[record.id] ?? record) - : dataStore.state[name] - + const changes = data._changes[name] + data[name] = dataStore.state[name].map( + (record) => changes[record.id] ?? record + ) res.locals.data[name] = data[name] } + // Generation info also comes from the store, so every session sees the + // current generation (not the one it was created under) + data.generationInfo = dataStore.state.generationInfo + res.locals.data.generationInfo = data.generationInfo + Object.defineProperty(data, 'toJSON', { value: function () { const copy = { ...this } for (const name of STORE_COLLECTIONS) { delete copy[name] } + delete copy.generationInfo return copy }, enumerable: false, diff --git a/docs/data-conventions.md b/docs/data-conventions.md new file mode 100644 index 00000000..e3908c4e --- /dev/null +++ b/docs/data-conventions.md @@ -0,0 +1,68 @@ +# Data conventions: reading and updating seed data + +How participant, clinic and event data works in this prototype, and the rules +to follow when changing it. + +## How it works + +The generated seed data (`data.participants`, `data.clinics`, `data.events`) +is **shared and read-only**. It is loaded once at boot into a shared store +([app/lib/data-store.js](../app/lib/data-store.js)) and attached to every +request by middleware in [app/routes.js](../app/routes.js) - it is not copied +into each session. + +A user's session only stores **changed records** in `data._changes` (whole +replacement records, keyed by id). The attach middleware overlays those onto +the shared arrays on each request, so `data.events` etc always look exactly +like a plain per-session copy - reading them needs no special knowledge. + +## Reading data + +No change from what you'd expect. In views and routes, read `data.events`, +`data.participants`, `data.clinics` as normal. Lookups, filters, maps and the +Nunjucks `sort` filter are all fine. + +## Updating data + +Never assign to properties of a record you got from one of these collections - +in development they are frozen, and the assignment will be **silently +ignored** (this codebase runs in sloppy mode, so frozen objects don't throw). +If a change you made mysteriously doesn't stick, this is almost certainly why. + +Instead, do one of: + +- **Use the update helpers** - `updateEvent`, `updateEventStatus`, + `updateEventData` ([app/lib/utils/event-data.js](../app/lib/utils/event-data.js)) + and `updateParticipant` ([app/lib/utils/participants.js](../app/lib/utils/participants.js)). + Build a whole replacement record (spread the old one, change what you need) + and pass it in. The helpers write it to the session's `_changes` for you. + +- **Use the temp working copy** - the screening flow checks out + `data.event` / `data.participant` (deep clones) for multi-page forms, and + `saveTempEventToEvent` / `saveTempParticipantToParticipant` commit them + back. Mutating the temp copies is fine - they're session-local clones. + +```js +// Wrong - mutates a shared record, silently dropped in dev +const event = data.events.find((e) => e.id === eventId) +event.status = 'event_complete' + +// Right - build a replacement and save through a helper +updateEventData(data, eventId, { status: 'event_complete' }) +``` + +Arrays and nested objects inside records are shared too: don't `push` to +them - copy first (`[...event.things, newThing]`). + +## Regeneration and profile swaps + +Regenerating data (daily, or via `/settings`) reloads the shared store and +changes its `generationId`. Session changes are stamped with the generation +they were made against, and stale changes are discarded automatically - so a +profile swap resets everyone's data. This is intentional demo behaviour. + +## Escape hatch + +If the development freeze ever blocks you mid-task, set +`freezeSharedData: false` in [app/config.js](../app/config.js) - the app +works identically, you just lose the mutation protection. From 62e5d5e1330212d42e3fc4b6e52b77c8a7960c00 Mon Sep 17 00:00:00 2001 From: Ed Horsford Date: Fri, 10 Jul 2026 17:06:01 +0100 Subject: [PATCH 5/5] Generate seed data at build time on Heroku npm run generate writes the seed files ahead of boot; heroku-postbuild calls it so dynos start with data on disk. Daily staleness check at runtime is unchanged (dyno cycling keeps dates fresh). --- package.json | 2 ++ scripts/generate-data.js | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 scripts/generate-data.js diff --git a/package.json b/package.json index b1eb5483..a8bd4c47 100644 --- a/package.json +++ b/package.json @@ -5,6 +5,8 @@ "main": "app.js", "scripts": { "start": "node .", + "generate": "node scripts/generate-data.js", + "heroku-postbuild": "npm run generate", "build:static-error-page": "node scripts/build-static-error-page.js", "docs": "node scripts/generate-utils-reference.js" }, diff --git a/scripts/generate-data.js b/scripts/generate-data.js new file mode 100644 index 00000000..6203e18c --- /dev/null +++ b/scripts/generate-data.js @@ -0,0 +1,18 @@ +// scripts/generate-data.js +// +// Generate the seed data files (app/data/generated/*.json) ahead of boot. +// Run via `npm run generate`; called from heroku-postbuild so dynos start +// with data already on disk instead of generating on first require. + +const generateData = require('../app/lib/generate-seed-data') + +const started = Date.now() + +generateData() + .then(() => { + console.log(`Seed data generated in ${Date.now() - started}ms`) + }) + .catch((err) => { + console.error('Seed data generation failed:', err) + process.exit(1) + })