Post-merge fixes from the #150/#151/#152 reviews#153
Merged
Conversation
Recovered submissions for metadata-active experiments were re-enqueued under their original filename instead of data/raw/<name>, producing no derived files and never merging into dataset_description.json. Worse, the queue doc was keyed off the original filename while api-data.ts keys off the transformed one, so a crash between queueUpload and cleanupPending could upload the same file twice under two names. Extract the metadataActive ? rawDataPath(filename) : filename decision into uploadPathFor() and use it in both api-data.ts and scheduled-pending-recovery.ts's promoteToQueue, for both the queued filename and the dedup key, eliminating the double-upload window as a side effect. Metadata/derived files are not regenerated for recovered sessions (documented as a known limitation): recovery has no metadata pipeline and the raw file is the source of truth.
Two cheap hardening fixes, plus the D2 decision (option b): keep returning 400 on a metadata-block failure, but stop deleting the pending-data copy so scheduled-pending-recovery can salvage it later instead of losing the submission outright. Graceful-degrade (accepting the data anyway) is a product decision left for a follow-up. - metadata-production.ts: variableMeasured is now length-checked (`?.length`) so an empty array throws the intended clean error instead of a TypeError on `variableMeasured[0]`. - metadata-production.ts: parseJsonData's result is checked for Array.isArray so a bare JSON object throws a clear "Data must be an array of trials" instead of flowing into generate()/mainRows and failing deeper with a confusing message.
Two submissions with the same leaf name in different subfolders collided at data/raw/<leaf> (the second got a 409 -> 400 OSF_FILE_EXISTS rejection): flattenName discarded the subfolder prefix instead of encoding it. flattenName now encodes path separators as `-` (condition-A/data.json -> condition-A-data.json) instead of dropping everything before the last `/`. The derived main CSV/sidecar stem follows the same encoded name automatically, so those stay collision-free too. Flat data/raw/ still matches the CLI's layout (the alternative — nesting subfolders under data/raw/ — was considered and rejected: it would diverge from the CLI and still need the encoded stem for derived files).
…iption updates Two related defects, one root cause: the uploadQueue was designed for immutable per-submission files, but dataset_description.json is mutable. - metadata-block.ts: when updateFileOSF throws, the catch no longer queues a create-PUT — it was guaranteed to 409 against the existing file, and the retry worker would mark that dead entry completed without ever applying the update. Firestore is the source of truth and every submission re-merges and re-mirrors, so the next submission repairs OSF instead (matches the code's own existing comment). - queue-upload.ts: while an entry is "pending", a newer submission with fresher content used to return early without ever queueing it, so the eventual retry pushed stale metadata. Now a "pending" re-queue overwrites the Cloud Storage payload (keeping the Firestore doc/status/retry schedule), so the retry uploads the freshest content. "processing" entries are left alone since the retry worker owns that payload right now.
uploadDerivedFiles previously uploaded N derived files serially, each independently walking (and possibly racing to create) the OSF data/ folder path — ~2(N+2) sequential OSF round-trips added to the response path before the participant's 201. - uploadDerivedFiles now resolves the data/ folder once up front and fans out the actual uploads with Promise.allSettled. Per-file 409 and queue-on-failure handling was already concurrency-safe; folder-create races among concurrent submissions still resolve via subfolder.ts's existing 409-re-list branch. - putFileOSF takes an optional pre-resolved startUrl to upload directly into (or walk any remaining segments from), skipping the redundant walk for files that share an already-resolved folder.
produceMetadata parsed JSON payloads twice (an isCsv() probe, then parseJsonData) and CSV payloads twice (once inside generate(), once via parseCSV for mainRows). Replace the isCsv probe with parseJsonData itself in a try/catch: success is the JSON path with the parsed array already in hand; a throw means CSV. For CSV, parseCSV runs once up front and its rows are passed into generate() as a pre-parsed array (confirmed in functions/metadata/dist/index.js: generate() short-circuits on Array.isArray(data) for both formats before any internal parsing) and reused as mainRows, instead of generate() re-parsing the same text. Golden metadata-production.test.js output (including byte-verbatim CSV mainContent) is unchanged.
…-read blockMetadata threw a module-level Error singleton out of the merge transaction and compared it by reference identity to detect the bootstrap case (Firestore empty, OSF populated) needing an OSF download before merging — discarding and re-running the whole transaction in that case. osfMetadataId is already known before the transaction even starts, so the download can be decided up front. Replaced with a non-transactional pre-read of metadata_doc_ref: if Firestore is empty and osfMetadataId exists, downloadMetadata runs first; the transaction then runs exactly once. It still re-reads Firestore inside, so a concurrent populate between the pre-read and the transaction still resolves correctly via firestoreMetadata ?? osfMetadata. Deletes NEEDS_OSF_METADATA, the inner runMergeTransaction closure, and the identity-check try/catch. The existing metadata-emulator suite's "in OSF but not in firestore" case already covers the bootstrap path.
…e hash changes
Scroll targeting relied on Chakra v3's private data-controls id format
and a 350ms timer, and only ran on mount, so in-page hash changes (e.g.
clicking another #item-N link without a full navigation) did nothing.
Each FAQItem now wraps its Accordion.Item in a <Box id={value}> — a DOM
node we own that's present regardless of expand/collapse state, so
scrollIntoView-style positioning no longer needs to wait on the
accordion's expand animation or depend on Chakra's internal attribute
naming. The scroll effect also listens for hashchange, not just mount.
Running the emulator suite surfaced a regression from the prior sentinel-abort-transaction removal: the OSF download (which can throw, e.g. on a 404) now happened before metadataMessage was ever set, so a download failure produced an empty metadataMessage instead of reporting which of the four Firestore/OSF states triggered it — unlike before, where the transaction always got to set metadataMessage on its first pass before the sentinel throw. metadataMessage is now set from the pre-read immediately, before the download attempt. The transaction still recomputes it from a fresh read (harmless, and correct if a concurrent write races the pre-read). Caught by functions/src/__tests__/metadata-emulator.test.js's "in OSF but not in Firestore" case under the full emulator suite.
The recovery test's expected raw-data path was written before the D1 (encode) commit landed and still expected the old lossy-flatten behavior (data/raw/data.json). Update it to the encoded name (data/raw/condition-A-data.json), matching flattenName's actual current behavior. Caught by running the full emulator suite.
…sion The "resolve data/ once up front" change moved resolveFolder outside the per-file try/catch, so an OSF/network failure threw out of uploadDerivedFiles — which api-data awaits un-try/caught after the raw file already landed and pending was cleaned up. That lost the derived files (never queued) and returned 500 instead of 201, breaking the best-effort "never fails the submission" contract. Wrap the up-front resolveFolder in try/catch and fall back to an undefined folder link, so each under-data/ file re-walks the path inside its own per-file catch (which queues on failure). Add an emulator test that drives an unreachable OSF and asserts every file is queued. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…letion The pending-refresh path read status once, then saved to Cloud Storage non-atomically. If the retry worker finished the doc (-> completed/failed, which deletes the storage object) in that window, the fresh payload was written to an orphaned path and silently lost. Re-read after the save: if the doc is still pending/processing we're done; otherwise fall through to a clean full re-queue so the fresh data isn't dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ssage The parse-once path signalled "valid JSON but not a trial array" by throwing an Error and re-matching its message text, which is fragile if the vendored library ever throws that same string. Use a private NotATrialArrayError class instead; external behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
derivedFiles can now only exist on a success result, so the 400 path in api-data can send the failure response verbatim without risking a leak — the guarantee is compiler-enforced rather than resting on a defensive strip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Summary
Follow-up fixes and hardening from the code review of #152 and #150 (plus two small #151 leftovers), landed as one commit per item so any can be reverted independently. Sequenced correctness -> queue semantics -> perf -> cleanup -> frontend.
3f4621a): recovered submissions for metadata-active experiments now land atdata/raw/, and the queue dedup key matches api-data.ts's, eliminating a double-upload window. Shared via a newuploadPathForhelper.f7dd079): D2 decision - keep the 400 but stop deleting the pending-data copy, so scheduled-pending-recovery can salvage it. Plus two hardening checks (emptyvariableMeasured, non-array JSON payload) that previously threw confusing errors.79016a6): D1 decision - encode researcher subfolder prefixes into the flattened filename (condition-A/data.json->condition-A-data.json) instead of discarding them, so two submissions with the same leaf name in different subfolders no longer collide atdata/raw/<leaf>.1ade20f): the uploadQueue only supports create-PUT, which is guaranteed to 409 against an existingdataset_description.json- stopped queueing dead update entries, and made pending create-queue entries refresh their payload instead of silently dropping newer content.data/once (73bda2d):Promise.allSettledover derived file uploads instead of serial, with thedata/folder resolved once up front instead of per-file.76b6804): eliminates the double-parse of both JSON and CSV payloads inproduceMetadata.7b4d00a,56f281c): removes the reference-identity sentinel/double-transaction-run pattern inmetadata-block.tsin favor of a non-transactional pre-read.a756618): stops relying on Chakra's privatedata-controlsattribute format and a fixed timer; adds a DOM node we own and ahashchangelistener.891312a): corrects a recovery test's expected path to match the D1 encoding behavior.Two decisions made explicitly during this work (see commit messages for detail): D1 = encode subfolder into the flattened name; D2 = land the smaller "keep the 400, stop deleting pending copy" change now, defer the larger "accept the data anyway" behavior change to a product decision.
Test plan
tscbuild passes forfunctions/npm run buildpasses at repo root (frontend)npm run test-ci, Firestore + Storage + Functions emulators): 18/18 suites, 120/120 tests passingmetadataMessagewent empty on the OSF-download failure path, and a stale test expectation predating the D1 encoding change/code-reviewon the branch diff (recommended before merge)