refactor: standardize file storage and metadata management#446
Open
harbassan wants to merge 13 commits into
Open
refactor: standardize file storage and metadata management#446harbassan wants to merge 13 commits into
harbassan wants to merge 13 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR migrates file handling from GridFS and client-side Firebase storage to Firebase Admin Storage with ChangesFile and resource migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AuthoringUI
participant FilesAPI
participant FirebaseStorage
participant ResourcesAPI
participant MongoDB
AuthoringUI->>FilesAPI: Upload file for scenario
FilesAPI->>FirebaseStorage: Save buffer and metadata
FirebaseStorage-->>FilesAPI: Return path and URL
FilesAPI->>MongoDB: Create UploadedFile
FilesAPI-->>AuthoringUI: Return uploaded file
AuthoringUI->>ResourcesAPI: Create resource with fileId
ResourcesAPI->>MongoDB: Increment file refCount
ResourcesAPI-->>AuthoringUI: Return resource
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/features/resources/ManageResourcesPage.jsx (1)
92-114: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd
userto the load effect.api.getcallsuser.getIdToken(), so this runs too early when auth is still resolving, the first fetch fails, and it never reruns whenuserbecomes available. Guard onuserand depend on[scenarioId, user].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/resources/ManageResourcesPage.jsx` around lines 92 - 114, Update the groups/files-loading useEffect to return early when user is unavailable, add user to its dependency array alongside scenarioId, and only call api.get after authentication resolves so the request reruns with the available user.backend/src/routes/api/collections.js (1)
85-104: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDecrement file refs before deleting a group.
Resource.deleteMany({ groupId })bypasses theapplyReferenceDeltapath used inbackend/src/routes/api/resources.js, so the deleted group's files keep theirrefCountand never reachdeletedAt/cleanup. Build afileIddelta map fromfilesand apply it before removing the resources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/api/collections.js` around lines 85 - 104, Update the DELETE /groups/:groupId handler to build a fileId delta map from the queried files and apply those decrements through the existing applyReferenceDelta path before Resource.deleteMany and group.deleteOne. Preserve the current response counts and not-found behavior.
🧹 Nitpick comments (9)
frontend/src/features/authoring/images.tsx (1)
39-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInvalidate the images query cache after a successful upload.
addNewImageuploads and adds the image directly to the scene, but doesn't invalidate the["images", scenarioId]query. SinceimagesQuerylives in the parentImageCreateMenu(it doesn't remount when the "Select Existing Image" modal opens), a newly uploaded image won't show up in that picker later in the same session without a manual refetch.♻️ Proposed fix
+import { useQuery, useQueryClient } from "`@tanstack/react-query`"; ... function ImageCreateMenu() { const { scenarioId } = useParams<{ scenarioId: string }>(); const [selectedImage, setSelectedImage] = useState<UploadedFile | null>(null); + const queryClient = useQueryClient(); ... function handleFileChange(event: React.ChangeEvent<HTMLInputElement>) { const file = event.target.files?.[0]; - if (file) addNewImage(file, scenarioId, user).catch(handleGeneric); + if (file) + addNewImage(file, scenarioId, user) + .then(() => queryClient.invalidateQueries({ queryKey: ["images", scenarioId] })) + .catch(handleGeneric); }Also applies to: 77-104
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/images.tsx` around lines 39 - 64, Update addNewImage so that after a successful upload and scene insertion, it invalidates the ["images", scenarioId] query cache. Reuse the existing query-client/cache mechanism used by ImageCreateMenu, and keep invalidation out of the failure path.backend/src/firebase/storage.js (2)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrapping swallows the original error's stack/type.
Rethrowing as
new Error(upload failed: ${err.message})discardserr's stack trace and any structured fields (e.g. GCSerr.code), making production diagnosis harder.🐛 Suggested fix
} catch (err) { - throw new Error(`upload failed: ${err.message}`); + throw new Error(`upload failed: ${err.message}`, { cause: err }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/firebase/storage.js` around lines 21 - 23, Update the catch handling around the upload operation to preserve the original error object, including its stack trace and structured fields such as code, while retaining the “upload failed” context. Avoid creating a plain Error from only err.message; use the existing upload failure path and nearest relevant symbol.
12-20: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDisable resumable uploads for these small buffers.
All uploads here are capped by
MAX_FILE_SIZE_MB(backend/src/routes/api/files.js, default 10MB) yetfile.save()defaults to a resumable upload, which adds an extra initiate round-trip per request. The Node Cloud Storage client recommendsresumable: falsefor files under 10MB.⚡ Suggested fix
await file.save(buffer, { metadata: { contentType, metadata: { firebaseStorageDownloadTokens: downloadToken, }, }, + resumable: false, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/firebase/storage.js` around lines 12 - 20, Update the options passed to file.save in the upload flow to set resumable to false, while preserving the existing buffer and metadata handling for files capped by MAX_FILE_SIZE_MB.backend/src/firebase/firebase.js (1)
1-14: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding against double
initializeApp()calls.
admin.initializeApp()runs unconditionally at import time; calling it twice without a name throwsThe default Firebase app already exists. A one-line guard is the standard mitigation:🛡️ Suggested guard
-admin.initializeApp({ - credential: admin.credential.cert({ - projectId: process.env.FIREBASE_PROJECT_ID, - clientEmail: process.env.FIREBASE_CLIENT_EMAIL, - privateKey: process.env.FIREBASE_PRIVATE_KEY?.replace(/\\n/g, "\n"), - }), - storageBucket: process.env.FIREBASE_STORAGE_BUCKET, -}); +if (!admin.apps.length) { + admin.initializeApp({ + credential: admin.credential.cert({ + projectId: process.env.FIREBASE_PROJECT_ID, + clientEmail: process.env.FIREBASE_CLIENT_EMAIL, + privateKey: process.env.FIREBASE_PRIVATE_KEY?.replace(/\\n/g, "\n"), + }), + storageBucket: process.env.FIREBASE_STORAGE_BUCKET, + }); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/firebase/firebase.js` around lines 1 - 14, Guard the unconditionally executed admin.initializeApp call in the Firebase setup so initialization occurs only when the default app does not already exist. Preserve the existing credential and storageBucket configuration, and leave getBucket and getAuth unchanged.backend/src/db/models/uploadedFile.js (1)
16-21: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant single-field index on
scenarioId.
scenarioIdhas its own index (line 20) and also leads the compound index{ scenarioId: 1, createdAt: -1 }(line 28). The compound index already serves scenarioId-only queries as a prefix, so the standalone index adds write overhead without additional query benefit.♻️ Proposed fix
scenarioId: { type: Schema.Types.ObjectId, ref: "Scenario", required: true, - index: true, },Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/db/models/uploadedFile.js` around lines 16 - 21, Remove the standalone index declaration from the scenarioId field in the uploaded file schema, while preserving the compound { scenarioId: 1, createdAt: -1 } index for scenarioId-based queries.backend/src/db/daos/fileDao.js (1)
8-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate pipeline logic between
applyReferenceDeltasandapplyReferenceDelta.Both functions build an identical two-stage
refCount/deletedAtpipeline. Consider having the single-item version delegate to the bulk version (e.g.applyReferenceDeltas(new Map([[fileId, delta]]))), or extracting the pipeline array into a shared helper, so any future change to this logic (e.g. tweaking thedeletedAtcondition) doesn't need to be kept in sync in two places.♻️ Proposed fix
+function buildRefCountPipeline(delta) { + return [ + { $set: { refCount: { $add: ["$refCount", delta] } } }, + { + $set: { + deletedAt: { $cond: [{ $lte: ["$refCount", 0] }, "$$NOW", "$$REMOVE"] }, + }, + }, + ]; +} + export async function applyReferenceDeltas(fileRefDeltas) { if (fileRefDeltas.size === 0) return; const fileOperations = Array.from(fileRefDeltas.entries()) .filter(([, delta]) => delta !== 0) .map(([fileId, delta]) => ({ updateOne: { filter: { _id: fileId }, - update: [ - { $set: { refCount: { $add: ["$refCount", delta] } } }, - { - $set: { - deletedAt: { - $cond: [{ $lte: ["$refCount", 0] }, "$$NOW", "$$REMOVE"], - }, - }, - }, - ], + update: buildRefCountPipeline(delta), }, })); ... export async function applyReferenceDelta(fileId, delta) { if (delta === 0) return; - await UploadedFile.updateOne({ _id: fileId }, [ - { $set: { refCount: { $add: ["$refCount", delta] } } }, - { - $set: { - deletedAt: { $cond: [{ $lte: ["$refCount", 0] }, "$$NOW", "$$REMOVE"] }, - }, - }, - ]); + await UploadedFile.updateOne({ _id: fileId }, buildRefCountPipeline(delta)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/db/daos/fileDao.js` around lines 8 - 50, Remove the duplicated update pipeline from applyReferenceDelta by delegating its nonzero single-file operation to applyReferenceDeltas, using a Map containing fileId and delta. Preserve the existing zero-delta early return and ensure the bulk helper remains the single source of truth for refCount and deletedAt updates.backend/src/db/models/resource.js (1)
5-10: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant single-field index on
scenarioId.Same pattern as
uploadedFile.js:scenarioIdhas its own index (line 9) and is also the leading field of the compound index (lines 41-45), making the standalone index redundant.Also applies to: 41-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/db/models/resource.js` around lines 5 - 10, Remove the standalone index declaration from the scenarioId field in the Resource schema, while preserving scenarioId as the leading field of the existing compound index.backend/src/db/models/scene.js (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCommented-out
pre("remove")hook was already dead code under Mongoose 8.Mongoose 7+ removed
Document.prototype.remove()entirely (doc.remove()no longer exists; migration path isdoc.deleteOne()/schema.pre('deleteOne', { document: true, query: false }, ...)). Since this project is on mongoose 8.7.0,pre("remove")could never have fired here regardless of this diff, so commenting it out is not a functional regression. That said, leaving a large commented block (with the removedtryDeleteFileimport gone) is dead weight — prefer deleting it and tracking the "scheduled remove of unused files" follow-up in an issue/ticket rather than an inline comment.♻️ Suggested cleanup
-// NOTE: this will be replaced by a scheduled remove of unused files -// -// before removal of scene from the database, first attempt to delete all user-uploaded images from firebase -// sceneSchema.pre("remove", function () { -// this.components.forEach((c) => { -// if (c.type === "image" || c.type === "audio") { -// tryDeleteFile(c.href ?? c.url); -// } -// }); -// }); +// File cleanup on scene deletion is handled by a scheduled sweep of +// unreferenced UploadedFile records (see <tracking-issue-link>).Also applies to: 39-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/db/models/scene.js` at line 1, Delete the commented-out pre("remove") hook block and any associated dead-code comments or remnants in the scene model; leave the active Mongoose schema behavior unchanged and track the unused-file removal follow-up separately.backend/src/middleware/firebaseAuth.js (1)
8-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden
authagainst unhandled rejections; the promise chain is fire-and-forget.The
.then()/.catch()chain is neither returned nor awaited, soauth()'s own promise resolves independently of token verification. IfgetAuth()throws synchronously (e.g. Firebase Admin not yet initialized), that exception is never caught anywhere — Express doesn't await middleware return values, so it becomes an unhandled rejection that can crash the process. Usingawait+try/catchfixes this and also removes the reliance on manual microtask flushing in tests.🔒️ Suggested fix
export default async function auth(req, res, next) { if (!req.headers.authorization) { res.sendStatus(HttpStatusCode.Unauthorized); return; } const token = req.headers.authorization.split(" ")[1]; - getAuth() - .verifyIdToken(token) - .then((decoded) => { - const { uid } = decoded; - req.body.uid = uid; - req.uid = uid; // to ensure other middleware (multer) doesn't overwrite - next(); - }) - .catch(() => res.sendStatus(HttpStatusCode.Unauthorized)); + try { + const { uid } = await getAuth().verifyIdToken(token); + req.body.uid = uid; + req.uid = uid; // to ensure other middleware (multer) doesn't overwrite + next(); + } catch { + res.sendStatus(HttpStatusCode.Unauthorized); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/middleware/firebaseAuth.js` around lines 8 - 24, Update the auth middleware function to await getAuth().verifyIdToken(token) inside a try/catch, handling both synchronous initialization errors and rejected verification promises with an Unauthorized response. Preserve the existing uid assignments and next() behavior on successful verification, and remove the fire-and-forget then/catch chain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/db/daos/fileDao.js`:
- Around line 52-59: Update retrieveImageList to exclude soft-deleted files by
filtering for documents whose deletedAt field is unset or null, while preserving
the existing scenarioId and image-type filters.
In `@backend/src/db/daos/sceneDao.js`:
- Around line 248-278: Update computeFileRefDeltas so modified existing
components compare their previous and current fileId values: decrement the old
file reference and increment the new one when they differ, while preserving
new-component handling and avoiding changes when the fileId is unchanged.
- Around line 296-302: Guard the result of Scene.findById in the scene update
flow before accessing existingScene.components. When no scene is found, return
the existing clean not-found response, and preserve computeFileRefDeltas
behavior for valid scenes.
In `@backend/src/routes/api/__tests__/collectionsApi.test.js`:
- Around line 59-78: Update the DELETE /api/collections/groups/:groupId handling
to decrement linked UploadedFile reference counts when removing Resource
documents, allowing the existing refCount/deletedAt lifecycle to update
correctly. Extend the related collections API test around applyReferenceDelta
and the group deletion request to assert the attached file’s resulting state.
In `@backend/src/routes/api/files.js`:
- Around line 17-18: Validate the parsed value used by MAX_FILE_SIZE_MB before
calculating MAX_FILE_SIZE_BYTES, and fall back to the existing 10 MB default
when parsing produces NaN or another invalid value. Keep the byte conversion
unchanged for valid positive configuration values so multer always receives a
usable size limit.
- Around line 104-118: Validate fileId in the GET /:scenarioId/:fileId handler
before calling retrieveFile, using the existing ObjectId validation or HttpError
conventions, and reject malformed values with a client-facing 400 BadRequest.
Keep the not-found response for valid IDs that retrieveFile does not find, and
remove or replace the unreachable !fileId check.
- Around line 70-90: Wrap the upload and UploadedFile.create flow in the
relevant handler with error handling that tracks firebaseInfo.path and deletes
the uploaded blob if database creation fails. Preserve the existing successful
response, and ensure cleanup errors do not mask the original failure.
In `@backend/src/routes/api/resources.js`:
- Around line 128-145: Update the DELETE conditional handler to include
`"stateConditionals._id": conditionalId` in the `Resource.findOneAndUpdate`
filter, matching the PUT handler’s validation. Preserve the existing 404
response when either the resource or conditional does not exist, and continue
returning the updated resource for successful deletions.
- Around line 74-93: Enable schema validation on both conditional update routes
by adding runValidators: true to the options object passed to each
findOneAndUpdate call, including the POST handler shown and its corresponding
PUT handler. Preserve the existing new: true behavior and other update options.
- Around line 19-46: Validate fileId before Resource.create by reusing
retrieveFile(scenarioId, fileId) in the POST handler, ensuring the file exists
and belongs to the requested scenarioId. Reject missing or mismatched files with
the established 4xx error handling, and only create the resource and call
applyReferenceDelta after validation succeeds.
In `@frontend/src/features/authoring/images.tsx`:
- Around line 31-37: Restore error handling for addExistingImage by catching
failures from getImageDimensions and routing them through the existing
handleGeneric feedback handler. Ensure the invocation that currently uses void
addExistingImage(selectedImage) does not leave the promise rejection unhandled,
while preserving the existing image creation flow on success.
In `@frontend/src/features/playScenario/components/ResourcePreview.jsx`:
- Around line 75-78: Update the isText error-rendering branch in ResourcePreview
to display a safe string derived from text.error rather than rendering the raw
Error object; preserve the existing warning alert and fallback behavior while
ensuring rejected loadText fetches cannot crash rendering.
- Around line 1-14: Update ResourcePreview’s loadText and useQuery configuration
to match the established Preview behavior in ManageResourcesPage: guard
contentType before calling startsWith, treat application/json as previewable
alongside text types, and check res.ok in loadText before returning the response
text. Preserve the existing file?.url gating and query key.
In `@frontend/src/features/resources/ManageResourcesPage.jsx`:
- Around line 408-417: Update Preview’s text-preview eligibility to safely
handle missing file.contentType and include application/json alongside text,
CSV, and Markdown so JSON queries actually run; keep the existing isText
behavior aligned with the same supported-content classification. Update loadText
to validate the fetch response and reject or throw when res.ok is false before
returning the response body.
- Around line 463-466: Update the isText && text.isError rendering branch in
ManageResourcesPage to display a string derived from text.error, such as its
message or a safe fallback, instead of rendering the error object directly.
Preserve the existing warning alert and ensure fetch failures cannot produce an
invalid React child.
---
Outside diff comments:
In `@backend/src/routes/api/collections.js`:
- Around line 85-104: Update the DELETE /groups/:groupId handler to build a
fileId delta map from the queried files and apply those decrements through the
existing applyReferenceDelta path before Resource.deleteMany and
group.deleteOne. Preserve the current response counts and not-found behavior.
In `@frontend/src/features/resources/ManageResourcesPage.jsx`:
- Around line 92-114: Update the groups/files-loading useEffect to return early
when user is unavailable, add user to its dependency array alongside scenarioId,
and only call api.get after authentication resolves so the request reruns with
the available user.
---
Nitpick comments:
In `@backend/src/db/daos/fileDao.js`:
- Around line 8-50: Remove the duplicated update pipeline from
applyReferenceDelta by delegating its nonzero single-file operation to
applyReferenceDeltas, using a Map containing fileId and delta. Preserve the
existing zero-delta early return and ensure the bulk helper remains the single
source of truth for refCount and deletedAt updates.
In `@backend/src/db/models/resource.js`:
- Around line 5-10: Remove the standalone index declaration from the scenarioId
field in the Resource schema, while preserving scenarioId as the leading field
of the existing compound index.
In `@backend/src/db/models/scene.js`:
- Line 1: Delete the commented-out pre("remove") hook block and any associated
dead-code comments or remnants in the scene model; leave the active Mongoose
schema behavior unchanged and track the unused-file removal follow-up
separately.
In `@backend/src/db/models/uploadedFile.js`:
- Around line 16-21: Remove the standalone index declaration from the scenarioId
field in the uploaded file schema, while preserving the compound { scenarioId:
1, createdAt: -1 } index for scenarioId-based queries.
In `@backend/src/firebase/firebase.js`:
- Around line 1-14: Guard the unconditionally executed admin.initializeApp call
in the Firebase setup so initialization occurs only when the default app does
not already exist. Preserve the existing credential and storageBucket
configuration, and leave getBucket and getAuth unchanged.
In `@backend/src/firebase/storage.js`:
- Around line 21-23: Update the catch handling around the upload operation to
preserve the original error object, including its stack trace and structured
fields such as code, while retaining the “upload failed” context. Avoid creating
a plain Error from only err.message; use the existing upload failure path and
nearest relevant symbol.
- Around line 12-20: Update the options passed to file.save in the upload flow
to set resumable to false, while preserving the existing buffer and metadata
handling for files capped by MAX_FILE_SIZE_MB.
In `@backend/src/middleware/firebaseAuth.js`:
- Around line 8-24: Update the auth middleware function to await
getAuth().verifyIdToken(token) inside a try/catch, handling both synchronous
initialization errors and rejected verification promises with an Unauthorized
response. Preserve the existing uid assignments and next() behavior on
successful verification, and remove the fire-and-forget then/catch chain.
In `@frontend/src/features/authoring/images.tsx`:
- Around line 39-64: Update addNewImage so that after a successful upload and
scene insertion, it invalidates the ["images", scenarioId] query cache. Reuse
the existing query-client/cache mechanism used by ImageCreateMenu, and keep
invalidation out of the failure path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 03e25548-aea7-47d3-9787-906b024ac10a
📒 Files selected for processing (41)
backend/jest.config.jsbackend/jest.setup.env.jsbackend/src/db/daos/fileDao.jsbackend/src/db/daos/imageDao.jsbackend/src/db/daos/resourcesDao.jsbackend/src/db/daos/sceneDao.jsbackend/src/db/models/StoredFile.jsbackend/src/db/models/resource.jsbackend/src/db/models/scene.jsbackend/src/db/models/uploadedFile.jsbackend/src/firebase/__tests__/storage.test.jsbackend/src/firebase/firebase.jsbackend/src/firebase/storage.jsbackend/src/index.jsbackend/src/middleware/__tests__/firebaseAuth.test.jsbackend/src/middleware/firebaseAuth.jsbackend/src/routes/api/__tests__/collectionsApi.test.jsbackend/src/routes/api/__tests__/filesApi.test.jsbackend/src/routes/api/__tests__/imageApi.test.jsbackend/src/routes/api/__tests__/navigateGroupApi.test.jsbackend/src/routes/api/__tests__/resourcesApi.test.jsbackend/src/routes/api/collections.jsbackend/src/routes/api/files.jsbackend/src/routes/api/image.jsbackend/src/routes/api/index.jsbackend/src/routes/api/navigate/group.jsbackend/src/routes/api/navigate/index.jsbackend/src/routes/api/resources.jsbackend/src/util/gridfs.jsfrontend/src/components/ListContainer/ImageListContainer.jsxfrontend/src/components/StateVariables/StateConditionalMenu.jsxfrontend/src/features/authoring/CanvasSideBar/AudioManager.jsxfrontend/src/features/authoring/audio/EditAudioComponent.tsxfrontend/src/features/authoring/images.tsxfrontend/src/features/authoring/types.tsfrontend/src/features/playScenario/components/MDTextViewer.jsxfrontend/src/features/playScenario/components/ResourcePreview.jsxfrontend/src/features/playScenario/components/ResourcesPanel.jsxfrontend/src/features/playScenario/hooks/useDownloadUrl.jsfrontend/src/features/resources/ManageResourcesPage.jsxfrontend/src/features/resources/util.js
💤 Files with no reviewable changes (10)
- backend/src/db/models/StoredFile.js
- backend/src/db/daos/imageDao.js
- frontend/src/features/playScenario/hooks/useDownloadUrl.js
- backend/src/db/daos/resourcesDao.js
- backend/src/routes/api/tests/imageApi.test.js
- backend/src/routes/api/image.js
- backend/src/util/gridfs.js
- frontend/src/features/authoring/audio/EditAudioComponent.tsx
- backend/src/routes/api/tests/navigateGroupApi.test.js
- backend/src/routes/api/navigate/group.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/src/util/validation.js (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer Mongoose's built-in
isValidObjectId.The current regex validation will incorrectly return
truefor an array containing a single valid ObjectId string (e.g.,["5f4d8e8b2b5d4c2b9c8b4567"]), becauseRegExp.test()coerces arrays to strings. Relying on Mongoose's built-in validator prevents edge cases like this and leverages the existing ecosystem.♻️ Proposed refactor
+import mongoose from "mongoose"; + export function isValidObjectId(id) { - return /^[0-9a-fA-F]{24}$/.test(id); + return mongoose.isValidObjectId(id); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/util/validation.js` around lines 1 - 3, Replace the regex-based validation in isValidObjectId with Mongoose’s built-in isValidObjectId validator, passing the input through unchanged so arrays and other non-scalar values are handled correctly.backend/src/firebase/storage.js (1)
31-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake file deletion idempotent.
Firebase Storage's
delete()method throws an error if the file does not exist. It's highly recommended to make this operation idempotent by passing{ ignoreNotFound: true }. This prevents errors if a deletion flow is retried or if the file was already removed out-of-band.♻️ Proposed refactor
export async function deleteFile(path) { if (!path) throw new Error("path is required"); try { - await getBucket().file(path).delete(); + await getBucket().file(path).delete({ ignoreNotFound: true }); } catch (err) { throw new Error(`delete failed: ${err.message}`); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/firebase/storage.js` around lines 31 - 38, Update deleteFile to pass the Firebase Storage delete option ignoreNotFound: true when invoking getBucket().file(path).delete(), while preserving the existing path validation and error-wrapping behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/src/firebase/storage.js`:
- Around line 31-38: Update deleteFile to pass the Firebase Storage delete
option ignoreNotFound: true when invoking getBucket().file(path).delete(), while
preserving the existing path validation and error-wrapping behavior.
In `@backend/src/util/validation.js`:
- Around line 1-3: Replace the regex-based validation in isValidObjectId with
Mongoose’s built-in isValidObjectId validator, passing the input through
unchanged so arrays and other non-scalar values are handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 56f28291-bcdf-4e75-8c7d-484eede6d462
📒 Files selected for processing (11)
backend/src/db/daos/sceneDao.jsbackend/src/firebase/__tests__/storage.test.jsbackend/src/firebase/storage.jsbackend/src/routes/api/__tests__/filesApi.test.jsbackend/src/routes/api/collections.jsbackend/src/routes/api/files.jsbackend/src/routes/api/resources.jsbackend/src/util/validation.jsfrontend/src/features/authoring/images.tsxfrontend/src/features/playScenario/components/ResourcePreview.jsxfrontend/src/features/resources/ManageResourcesPage.jsx
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/src/routes/api/collections.js
- frontend/src/features/playScenario/components/ResourcePreview.jsx
- backend/src/routes/api/files.js
- backend/src/routes/api/tests/filesApi.test.js
- frontend/src/features/authoring/images.tsx
- backend/src/routes/api/resources.js
- frontend/src/features/resources/ManageResourcesPage.jsx
harbassan
force-pushed
the
vps-137-standardize-file-storage-and-metadata-management
branch
from
July 17, 2026 03:51
19d1e8b to
6f00e5f
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/features/authoring/images.tsx (2)
100-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the file input to support same-file retries.
After a failed upload, selecting the same file again may not trigger
onChangebecause the input value is unchanged. Clearevent.currentTarget.valueafter capturing the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/images.tsx` around lines 100 - 103, Update handleFileChange to capture the selected file, then clear event.currentTarget.value before starting the upload so selecting the same file again triggers onChange after failures. Preserve the existing addNewImage and handleGeneric flow.
94-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRefresh the images query after upload
frontend/src/features/authoring/images.tsx:addNewImageonly adds the new image to the canvas; it never invalidates or updates["images", scenarioId], so the existing-image modal can keep showing stale data until a later refetch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/images.tsx` around lines 94 - 103, Update handleFileChange to refresh the imagesQuery after addNewImage completes by invalidating or updating the ["images", scenarioId] query through the existing query client mechanism. Preserve handleGeneric error handling and ensure the refresh occurs only after a successful upload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/features/authoring/images.tsx`:
- Around line 100-103: Update handleFileChange to capture the selected file,
then clear event.currentTarget.value before starting the upload so selecting the
same file again triggers onChange after failures. Preserve the existing
addNewImage and handleGeneric flow.
- Around line 94-103: Update handleFileChange to refresh the imagesQuery after
addNewImage completes by invalidating or updating the ["images", scenarioId]
query through the existing query client mechanism. Preserve handleGeneric error
handling and ensure the refresh occurs only after a successful upload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ec1e252-4bf9-4eef-bd91-24cf9a0c9370
📒 Files selected for processing (11)
backend/src/db/daos/sceneDao.jsbackend/src/firebase/__tests__/storage.test.jsbackend/src/firebase/storage.jsbackend/src/routes/api/__tests__/filesApi.test.jsbackend/src/routes/api/collections.jsbackend/src/routes/api/files.jsbackend/src/routes/api/resources.jsbackend/src/util/validation.jsfrontend/src/features/authoring/images.tsxfrontend/src/features/playScenario/components/ResourcePreview.jsxfrontend/src/features/resources/ManageResourcesPage.jsx
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/src/firebase/tests/storage.test.js
- backend/src/firebase/storage.js
- backend/src/routes/api/collections.js
- backend/src/routes/api/resources.js
- backend/src/util/validation.js
- backend/src/db/daos/sceneDao.js
- backend/src/routes/api/files.js
- backend/src/routes/api/tests/filesApi.test.js
- frontend/src/features/playScenario/components/ResourcePreview.jsx
- frontend/src/features/resources/ManageResourcesPage.jsx
5 tasks
Contributor
|
alot of changes, I like the approach reminds me of hard inode pointers from OS, with the ref counters. will review fully soon. |
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.
Issue
The file upload system we use is inconsistent, which makes development unnecessarily difficult and increases our consumption of external resources (firestore, gridfs).
Essentially, we use both firebase storage and gridfs for file storage, and both firestore and mongodb for metadata storage. Even within firebase storage, the method we use to upload files and the paths they are stored with vary across our different upload endpoints. The ticket description has more info.
Solution
There is now a single upload endpoint to create files, which handles the storage on firebase alone and the metadata on mongodb alone. This metadata object contains a ref counter, which is in sync with all referencing documents (resource, scene component). This system means that we can minimise duplicate files in storage.
This is a soft delete system, so files are only marked for deletion, not actually deleted. The actual deletion will be handled later, as a service that occasionally checks for ready to delete files and prunes the metadata and file.
Again for more info read the ticket description.
Risk
THIS IS NOT BACKWARDS COMPATIBLE WITH EXISTING RESOURCES
This refactor required a major modification to the resources backend, given the previously tight coupling of the file concept and resource cocnept, and the use of gridfs.
Ive added many tests for the new and modified functionalities, coverage is close to 100%.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests