feat(storage): per-drawing storage management#165
Open
OhYee wants to merge 24 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds drawing-scoped storage management tooling to detect and remediate drift between canvas image references, Drawing.files JSON keys, and S3 objects/records—supporting operational cleanup for S3-backed images introduced in the prerequisite PRs.
Changes:
- Backend: introduce storage management routes (
trim,files/diff,files/orphans) plus S3 utilities + centralizedprocessFilesForS3handling during save/import. - Frontend: add “Storage Management” modal UI (diff + destructive actions with name confirmation) and wire it into the drawing card context menu; editors auto-reload on server-side mutations.
- Data/tests: add
S3Filecomposite PK migration + related Prisma schema updates and new tests (route integration + file processing unit tests).
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/Editor.tsx | Reload open editors on drawing-server-update; small save/collab adjustments. |
| frontend/src/components/StorageManageModal.tsx | New UI for three-way file diff + trim/orphan deletion with confirmation. |
| frontend/src/components/DrawingCard.tsx | Adds Storage Management entry in context menu and mounts modal. |
| frontend/src/api/index.ts | Adds /files/config probe caching and storage management API helpers. |
| backend/tsconfig.json | Excludes test files from backend TS compilation. |
| backend/src/security.ts | Tightens fileId sanitization + allows S3/CDN and /api/files/:drawingId/:fileId URLs in dataURL. |
| backend/src/s3.ts | New S3 client + helpers (list/copy/delete/upload + URL helpers). |
| backend/src/routes/storage.ts | New storage management routes (trim/diff/orphan delete) + socket notification. |
| backend/src/routes/importExport/legacySqliteImportRoutes.ts | Upload/process files pre-transaction with bounded concurrency; ensure final drawingId consistency. |
| backend/src/routes/importExport/excalidashImportRoutes.ts | Same as above for excalidash import format. |
| backend/src/routes/files.ts | New /files/config + /files/:drawingId/:fileId redirect route for private-bucket mode. |
| backend/src/routes/dashboard/types.ts | Wires processFilesForS3 dependency into dashboard route deps. |
| backend/src/routes/dashboard/drawings.ts | Moves file upload handling to backend save/create/duplicate/delete flows; adds S3 cleanup/copy logic. |
| backend/src/index.ts | Initializes S3 and registers new file/storage routes; injects processFilesForS3. |
| backend/src/fileProcessing.ts | New centralized base64->S3 upload + preview SVG rewrite helper. |
| backend/src/config.ts | Adds S3-related env config parsing. |
| backend/src/tests/storage.integration.ts | New supertest-based integration coverage for storage routes. |
| backend/src/tests/fileProcessing.test.ts | New unit tests for processFilesForS3 and decodeDataURL. |
| backend/prisma/schema.prisma | Adds S3File model with composite PK (drawingId, fileId). |
| backend/prisma/migrations/20260506130000_s3file_composite_pk/migration.sql | Migration switching S3File PK to composite via drop/recreate. |
| backend/prisma/migrations/20260408060000_add_s3_files/migration.sql | Earlier migration introducing initial S3File table. |
| backend/package.json | Adds AWS SDK S3 dependencies. |
| backend/.env.example | Documents S3-related environment variables (needs alignment with backend-upload model). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+689
to
+705
| /** Cache the result of the /files/config probe so we only call it once. */ | ||
| let s3EnabledCache: boolean | null = null; | ||
|
|
||
| /** | ||
| * Returns true when the backend has S3 configured. | ||
| * The result is cached for the lifetime of the page. | ||
| */ | ||
| export const isS3Enabled = async (): Promise<boolean> => { | ||
| if (s3EnabledCache !== null) return s3EnabledCache; | ||
| try { | ||
| const response = await api.get<{ s3Enabled: boolean }>("/files/config"); | ||
| s3EnabledCache = response.data.s3Enabled === true; | ||
| } catch { | ||
| s3EnabledCache = false; | ||
| } | ||
| return s3EnabledCache; | ||
| }; |
Comment on lines
+495
to
+504
| // Drop entries whose key would not be safe to embed in an S3 object | ||
| // path. Backend file-storage code uses the fileId as a path segment, | ||
| // so a malicious key like "../../foo" must never reach the uploader | ||
| // or the database row. Same regex as backend/src/routes/files.ts. | ||
| const VALID_FILE_ID = /^[\w-]{1,200}$/; | ||
| for (const fileId in sanitizedFiles) { | ||
| if (!VALID_FILE_ID.test(fileId)) { | ||
| delete sanitizedFiles[fileId]; | ||
| } | ||
| } |
Comment on lines
+320
to
+325
| const { confirmName, fileIds } = req.body ?? {}; | ||
|
|
||
| if (!Array.isArray(fileIds) || fileIds.length === 0) { | ||
| return res.status(400).json({ error: "fileIds must be a non-empty array" }); | ||
| } | ||
|
|
Comment on lines
+354
to
+387
| let deletedCount = 0; | ||
| let errorCount = 0; | ||
|
|
||
| // S3File rows are scoped (drawingId, fileId), and each drawing | ||
| // has its own S3 object under its own prefix path — deleting | ||
| // here cannot strand a sibling drawing. | ||
| for (const fileId of fileIds as string[]) { | ||
| try { | ||
| if (isS3Enabled()) { | ||
| const s3Record = await prisma.s3File.findUnique({ | ||
| where: { drawingId_fileId: { drawingId: id, fileId } }, | ||
| }); | ||
| if (s3Record) { | ||
| await deleteS3Object(s3Record.s3Key); | ||
| await prisma.s3File.delete({ | ||
| where: { drawingId_fileId: { drawingId: id, fileId } }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| delete files[fileId]; | ||
| deletedCount++; | ||
| } catch (err: any) { | ||
| console.error( | ||
| `[storage/orphans] Failed to delete fileId=${fileId}`, | ||
| err | ||
| ); | ||
| errorCount++; | ||
| } | ||
| } | ||
|
|
||
| // Also remove deleted elements that reference the orphaned files, | ||
| // so the files disappear from the diff completely. | ||
| const deletedFileIdSet = new Set(fileIds as string[]); |
Comment on lines
+49
to
+52
| // GET /files/:fileId | ||
| // Issues a presigned GET URL and redirects the browser to S3. | ||
| // Used only in private-bucket deployments where S3_PUBLIC_URL is not | ||
| // set and the dataURL stored in the drawing is "/api/files/:fileId". |
Comment on lines
+48
to
+52
| # When S3_BUCKET is set, pasted/dropped images are uploaded directly from the | ||
| # browser to S3, keeping ExcaliDash's server bandwidth and SQLite size small. | ||
| # Compatible with AWS S3, Cloudflare R2, MinIO, Alibaba OSS, and any service | ||
| # that supports presigned PUT URLs. | ||
| # |
Comment on lines
+19
to
+29
| DROP TABLE IF EXISTS "S3File"; | ||
|
|
||
| CREATE TABLE "S3File" ( | ||
| "drawingId" TEXT NOT NULL, | ||
| "fileId" TEXT NOT NULL, | ||
| "userId" TEXT NOT NULL, | ||
| "s3Key" TEXT NOT NULL, | ||
| "mimeType" TEXT NOT NULL, | ||
| "createdAt" DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| PRIMARY KEY ("drawingId", "fileId") | ||
| ); |
Comment on lines
+268
to
+269
| /** File IDs present at initial drawing load — skip S3 re-upload for these. */ | ||
| const initialFileIdsRef = useRef<Set<string>>(new Set()); |
|
|
||
| expect(res.status).toBe(200); | ||
| expect(res.body.summary).toMatchObject({ | ||
| totalCanvasRefs: 2, // active only |
OhYee
changed the title
Excalidraw images are stored as base64 dataURLs inside Drawing.files
(a JSON column in SQLite). One uncompressed image is several MB;
embedding many of them bloats the DB, slows collaborative sync, and
makes backups/exports unwieldy. This commit adds opt-in S3-backed
image storage. When S3_BUCKET is configured, image dataURLs go to
S3 instead of SQLite.
Architecture: backend-driven uploads. The frontend sends the entire
drawing payload (base64 dataURLs intact) to the standard create /
update / import routes; the backend uploads any new dataURLs to S3
and rewrites them to S3 URLs before persisting. Single auth boundary,
uniform coverage of paste / drag / drop / import paths, S3File rows
written by the backend in the same handler as the drawing row.
Configuration (all optional, S3 stays disabled when unset):
S3_BUCKET, S3_REGION (default us-east-1)
S3_ENDPOINT (MinIO / Cloudflare R2 / Alibaba OSS)
S3_PUBLIC_URL (CDN base; unset → private-bucket redirect mode)
S3_FORCE_PATH_STYLE (MinIO needs true; OSS needs false)
S3_KEY_PREFIX (default "excalidash")
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY
Backend module (backend/src/s3.ts):
- Typed S3Config, presigned URL helpers, uploadBuffer / listS3Objects
utilities, shared FILE_KEY_PREFIX / buildS3Key / drawingS3Prefix so
upload and cleanup paths can't drift onto different prefixes.
Backend integration:
- backend/src/fileProcessing.ts: processFilesForS3(files, userId,
drawingId, prisma) scans the file record for base64 dataURLs,
uploads them, upserts a row keyed (drawingId, fileId) in S3File,
returns a rewritten file record (base64 → S3 URL).
- Wired into POST /drawings, PUT /drawings/:id, and both /import/*
routes so every code path that introduces a new image is covered.
New routes (backend/src/routes/files.ts):
- GET /files/config: surfaces s3Enabled for frontend feature
detection.
- GET /files/:fileId: 302 redirects to a presigned GET URL for
private-bucket deployments where the dataURL stored on the drawing
is /api/files/:fileId.
Schema (backend/prisma/schema.prisma + initial migration):
- New S3File table mapping fileId → s3Key + userId + mimeType so the
redirect route can resolve the bucket location. Additive only; no
destructive changes to existing tables.
Sanitiser (backend/src/security.ts):
- Permits the new "https://..." (CDN) and "/api/files/..."
(redirect) shapes as safe dataURL values without otherwise
loosening the existing image-dataURL validation.
Object key layout: {prefix}/{userId}/{drawingId}/{fileId}.{ext},
served with Cache-Control: public, max-age=31536000, immutable since
fileIds are content hashes and the objects are never mutated in
place.
When S3 is enabled there is no silent base64 fallback: upload
failures surface to the user instead of pretending the image was
persisted.
Change-Id: I189e27b1f5459a1782e4a69d1f8c4494dee3921e
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reject file ids that don't match /^[\w-]{1,200}$/ in two layers, so a
request body like
files: { "../../etc/passwd": { dataURL: "data:image/png;base64,..." } }
cannot escape the per-user S3 prefix and write outside it:
- security.ts sanitizeDrawingData drops malformed keys before
persistence, so the drawing.files JSON cannot store entries that
would later be uploaded under attacker-controlled paths.
- fileProcessing.processFilesForS3 also drops them defensively, since
this utility is reachable from import paths that bypass the schema.
Same regex used by the GET /files/:fileId route param. Adds a
regression test covering the path-traversal case end-to-end.
Change-Id: I6c8427b4ab67956a57beb262c182a539a73a14f1
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uessability Excalidraw fileIds are SHA-1 hashes of the file bytes — anyone in possession of the original image can compute the id, so the previous "fileId is unguessable" comment was incorrect and any authenticated user could fetch images uploaded by anyone else. Now the route authorises by drawing access: - the uploader (S3File.userId === req.user.id) can always read; OR - the caller has access to a drawing that references this fileId, either one they own or one explicitly shared with them via DrawingPermission. A single substring query on the JSON columns is enough because fileIds are >= 16 random chars; the wrapping quotes prevent prefix collisions (`abc1` won't match `abc12`). Also adds a 401 fallback in case requireAuth is ever removed. Change-Id: I4c9c0640e39d8d6a791009db289dc3a29e6658df Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
S3_FORCE_PATH_STYLE was parsed in config.ts and surfaced on the typed S3Config but the property was never threaded through to the initS3() call site. MinIO/Alibaba OSS users who set the env var saw no effect — the S3 client always picked the default addressing style. Change-Id: If16d6d8326612d4fb48501a54fea35c2b1e62d1b Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
GET /files/:fileId previously required a logged-in user via requireAuth, which broke private-bucket deployments whenever a drawing was shared with an active link-share: the anonymous viewer could open the drawing scene but every <img src="/api/files/..."> returned 401. Replace requireAuth with optionalAuth and decide access by drawing membership instead: - The uploader (S3File.userId === caller) always has access. - Authenticated callers also pass if they own a drawing referencing the fileId, or were granted access via DrawingPermission. - Anyone (anonymous included) passes if a drawing referencing the fileId has an active DrawingLinkShare row (revokedAt IS NULL and expiresAt either NULL or in the future). - Otherwise return 404 so the response doesn't leak whether a fileId exists for someone who doesn't have access. Change-Id: Id8e6f427d9bd392a0adbefd931a1b219c7b35513 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
S3File previously used the global Excalidraw fileId as its sole primary key. Excalidraw fileIds are content hashes that legitimately repeat across drawings, so two drawings — possibly two different users — uploading the same image silently overwrote each other's row. /files/:fileId then couldn't disambiguate which (drawing, owner) the caller meant. Move S3File to a composite primary key (drawingId, fileId), with matching changes throughout the upload + access paths: - prisma schema + initial migration: PK is now @@id([drawingId, fileId]) with @@index([drawingId]). - s3.ts: shared FILE_KEY_PREFIX + buildS3Key() + drawingS3Prefix() helpers so upload, list, and cleanup paths cannot drift onto different prefixes. - fileProcessing.ts: composite-key upsert; access URL is /api/files/:drawingId/:fileId. - routes/files.ts: route signature is /files/:drawingId/:fileId; the lookup uses { drawingId_fileId } and the access decision is drawing-scoped (owner / explicit grantee / active link-share). - security.ts: the safe-dataURL pattern accepts the new path shape. - Test mock + assertions updated. The DELETE-time S3 cleanup in routes/dashboard/drawings.ts still needs a follow-up to switch from prefix-startswith to drawingId filter; that change rides with the next commit so the upload-side schema change can be reviewed independently. Change-Id: Idccf506bf81b448d29dc75cb24bb3e49481e3279 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Dockerfile production stage runs `npx tsc` to emit dist/. The same tsc invocation was also compiling the __tests__/ tree, which made the build fail whenever a test mock used a partial type (e.g. a Zod schema mock with only safeParse). vitest has its own type-checking pass, so the production build has no business compiling test files. Exclude *.test.ts, *.integration.ts, and the __tests__/ folder. The production tsc now succeeds even when test files have type shortcomings; vitest still catches real test errors at run time. Change-Id: Ica2d5f91c05131b3d9b6674df61e34fcbda60884 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prior versions of this branch shipped the new S3File composite primary key by editing the original `20260408060000_add_s3_files` migration in place. That works for fresh deployments but is invisible to any deployment that already applied the original `id`-only PK migration — Prisma decides what to apply by migration name, so the new shape never reaches the database and runtime upserts crash with "The column `drawingId` does not exist in the current database." Restore the original migration to its pre-bundle shape and add a follow-up migration that drops + recreates S3File with the new shape. Both fresh and upgrade paths converge on the composite PK. Existing S3 objects are untouched; public-bucket deployments are unaffected. Private-bucket deployments will see /api/files/:drawingId/:fileId 404 for legacy rows until those drawings are re-saved (the save flow upserts on every base64 dataURL); operators with valuable legacy rows can rebuild manually before deploying. Change-Id: I9c06c7a46247050521a1d5013ec37eeebeb86247 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PUT /drawings/:id (scene update path): - Version conflict detection now happens BEFORE processFilesForS3, not after the updateMany. Previously, on a stale-version save the upload had already PUT every base64 file to S3 and we returned 409 — the objects became permanent orphans with no DB row. The new check uses a cheap findFirst before the upload; the existing version-filtered updateMany still catches the residual race window. - Non-owner editors (link-share + edit, share-with-user) can no longer introduce new fileIds. Reading the existing files JSON gives us the set of allowed ids; any unknown id in the payload returns 403. Owners retain full control. This prevents a shared editor from filling the owner's S3 bucket via 50MB uploads. DELETE /drawings/:id: - Best-effort cleanup of S3 objects and S3File rows under the per- drawing prefix. Failures log and continue; the drawing row is already gone, so the cleanup is opportunistic. Without this, deleting a drawing leaked every uploaded image permanently. Change-Id: Ia684b408c1eb0c474c04bf8c04eb339f9a497e47 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
Two import flows (excalidash backup + legacy SQLite) both pre-uploaded files to S3 using the imported drawing id, then on a foreign-owner id collision wrote the row under a fresh uuidv4() — leaving every uploaded object as a permanent S3 orphan keyed under an id that no DB row knew about. Now both routes pre-resolve the final drawing id before uploading by peeking at any pre-existing drawing of the same id; if it belongs to another user, we generate the UUID up front and feed it to both the upload and the in-transaction create. The transaction still re-checks authoritatively, but always writes under the same finalId. Also replaces the per-drawing serial await with bounded Promise.all batches of 8. A 5000-drawing import previously serialised every S3 PUT inside the request, easily hitting reverse-proxy timeouts; the bound keeps memory and socket pressure flat without saturating the bucket. Change-Id: I7ae77f642f097c295c71771d59ecea0d045661d3 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
…leted POST /drawings/:id/duplicate previously copied elements/appState/files verbatim, including the dataURLs that point at the *original* drawing's S3 prefix (`/api/files/origId/fileId` or `https://.../excalidash/owner/origId/fileId.ext`). The duplicate's images therefore pointed at storage owned by the original. Deleting the original then ran the new prefix-scoped S3 cleanup and silently broke every image in the duplicate. Now the duplicate handler does a server-side CopyObject for every S3File row of the original under the new drawingId path, writes a fresh (newDrawingId, fileId) S3File row, and rewrites the dataURLs in the new drawing's files JSON to match the new drawing id. Each drawing — original and duplicate — owns its own storage from the moment of duplication, so DELETE /drawings/:origId can no longer strand the duplicate. Adds a bucket-internal copyS3Object() helper to s3.ts (uses S3 CopyObject; no download/re-upload). Change-Id: If57d006fce8964fe04e702680e96866a3ec3b6f0 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…S3 upload Symptom: every save produced a megabyte-scale `Drawing.preview` column. The SVG had `<image href="data:image/png;base64,...">` for each image element instead of the S3 URL. Root cause: the frontend generates the preview SVG from the canvas state at save time, before the round-trip uploads files to S3. The SVG embeds whatever dataURL the file currently has, which at save time is still base64. processFilesForS3 only rewrites the dataURL inside `Drawing.files`, never the parallel preview SVG, so the inlined base64 stays in the preview forever. Add a rewritePreviewForS3 helper in fileProcessing.ts that diffs the original-vs-processed files map and applies the resulting URL substitutions to the preview string. Wire it into the POST and PUT handlers so the preview saved alongside the upload reflects the same S3 URLs as Drawing.files. Best-effort string substitution: it works because the same dataURL string is character-identical in both `files[fileId].dataURL` and the preview SVG's href. If frontend encoding ever diverges, the worst case is the preview is left as-is — never crashes. Change-Id: I427edabcccc1453bf0f69f5dde33c9648cfcb370 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OhYee
force-pushed
the
feat/storage-management
branch
from
4c1e845 to
4cd7e9a
Compare
The duplicate handler used to "log and continue" when copyS3Object threw — the duplicated `Drawing.files` JSON kept the original drawing's dataURL for that fileId, the new drawing landed pointing at the original's S3 prefix, and the next deletion of the original silently broke the duplicate's images. Restructure the flow so the new drawing row is only created after every S3 copy succeeds: 1. Pre-allocate the new drawing id with crypto.randomUUID(). 2. Copy every S3 object to the new prefix; track the destination keys in a list as we go. 3. If any copy throws, delete every key we managed to copy so the bucket isn't left with orphan objects, and return 500. The original drawing is never touched. 4. Only after all copies succeed, open a single Prisma transaction that creates the drawing row and the S3File rows together — so a DB failure can't leave S3 objects with no row, and the new drawing is always observable in a fully-consistent state. Change-Id: Ic930d74a3831ce325aaf843f3dd667d3ddeafbe9 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…flict
The earlier fix pre-resolved a final drawing id before uploading to
S3 so the upload key and the eventual DB row would match. That works
for the common case, but if a drawing with the same id is created
between the pre-check and the in-transaction findUnique (race), the
in-tx tx.drawing.create({ id: finalId }) hits a unique-constraint
error and aborts the whole import — so a stray race kills the whole
batch.
In the conflict branch (existing.userId !== req.user.id), check
whether the pre-resolved finalId still equals the imported id: if so,
the pre-upload check missed the race and we'd be writing under a
guaranteed conflict. Generate a fresh UUID for the row instead. The
S3 objects we already PUT under the imported-id path become orphans
that the storage-management tools can sweep up; that's vastly better
than failing the whole batch.
Applied to both /import/excalidash and /import/sqlite/legacy.
Change-Id: Ie1997da1511b736cee33b03e19ae6e06662df1f4
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous shape of 20260506130000_s3file_composite_pk did
DROP TABLE + CREATE TABLE, which silently lost every existing
(fileId → s3Key, mimeType, userId) mapping. For private-bucket
deployments that turns every /api/files/:drawingId/:fileId into a
404 until each affected drawing is re-saved.
Replace the drop with a copy: extract the drawingId from each
s3Key (which has shape `{prefix}/{userId}/{drawingId}/{fileId}.{ext}`)
by chaining INSTR + SUBSTR three times — SQLite has no native
split() and the bundled engine doesn't have REVERSE either, so we
walk the string from the start. Rows whose s3Key doesn't have at
least three '/' are skipped (they can only come from a custom
S3_KEY_PREFIX containing '/', which would otherwise misattribute the
drawingId; operators in that case still need to re-save).
INSERT OR IGNORE protects against duplicate (drawingId, fileId)
pairs theoretically allowed under the old schema; the survivor wins.
Change-Id: I69a27a5deabdcc43b7bfdcc0b1445d7dcb9fd4db
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dy docs
Round-up of review feedback that doesn't change behaviour for the
common case but plugs operability/correctness gaps:
- security.ts sanitiser:
* accept `http://` in addition to `https://` for S3-style URLs, so
a plain-HTTP local MinIO `S3_PUBLIC_URL` doesn't get blanked on
save and break image loading
* snapshot the file-id list with `Object.keys()` before deleting
invalid ids — `for...in` while mutating can skip entries on some
engines, leaving an unsafe key in the sanitised output
- fileProcessing.processFilesForS3: bound parallel S3 PUTs to a
concurrency of 8. Without this, a paste of N images fired N
parallel uploads, which can saturate S3 connection pools and
produce inconsistent partial-failure states on flaky networks.
Imports were already bounded; this brings the save path in line.
- routes/files.ts: header comment block now reflects the actual
/files/:drawingId/:fileId route shape and the
/api/files/:drawingId/:fileId redirect path.
- backend/.env.example: rewrites the S3 section to describe the
current backend-driven upload flow (no more browser-direct PUT,
no more CORS rules), and documents S3_FORCE_PATH_STYLE,
S3_KEY_PREFIX, plus the http://-publicUrl allowance for local
MinIO.
- frontend Editor.tsx: drop `initialFileIdsRef`. It was only read by
the Excalidraw 0.18 paste-detection block that the backend-direct
refactor removed, so the ref now just leaks state.
Change-Id: I95b8cd0480845aaceeea37aa5ede3e76bd8c479b
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds operator tooling for the inevitable drift between canvas refs, the SQLite files JSON, and the S3 bucket: orphan files from soft-deleted elements, S3 objects with no DB row, ghost file entries left behind by partially-failed saves, etc. Backend routes (require ownership + name confirmation): - POST /drawings/:id/trim: drop isDeleted elements, prune unreferenced file entries, delete the corresponding S3 objects + S3File rows. Requires confirmName matching the drawing name (GitHub-style destructive-action guard). - GET /drawings/:id/files/diff: three-way diff over canvas refs, SQLite files keys, and S3 objects under the per-drawing prefix. Returns one row per fileId with presence flags. - DELETE /drawings/:id/files/orphans: selectively delete fileIds the caller knows are unused. Refuses fileIds still active in non-deleted elements; also strips deleted-element references to the same fileIds so the diff view stops reporting them as ghost canvas references. Frontend: - StorageManageModal renders the three-way diff with checkboxes; surfaces "trim history" and "delete selected orphans" actions, both requiring the user to type the drawing name. - Hooked into DrawingCard right-click menu so the action is one click from the dashboard instead of an API call. - API helpers added in frontend/src/api/index.ts. Change-Id: Ic989d096938a841c4709a6b6a63c2c732e38cf81 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bump Three related defects in trim/orphans handlers: - Stale /drawings cache: trim and orphan-delete mutate the underlying drawing but never called invalidateDrawingsCache(), so the listing endpoint returned pre-trim element/file payloads for up to 5s. - Cross-drawing fileId deletion: Excalidraw fileIds are content hashes, so the same image can appear in multiple drawings. The previous code unconditionally removed the S3 object plus S3File row when trimming one drawing, breaking sibling drawings. Skip the S3-side delete when any other drawing of the same user still references the file (checked via a substring-on-JSON match scoped by quotes to avoid prefix collisions). - Orphan-delete left version unchanged: now bumps version like trim, so concurrent editors reload instead of overwriting. Change-Id: I1a83a5bab7d2354c9a1125142e94c2025f3e2f6d Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
Trim and orphan-delete mutate Drawing.elements / files directly from the dashboard. A collaborator currently in the editor at the same moment never received any signal, so their next save would echo back the trimmed-away elements and undo the cleanup. Now both routes emit drawing-server-update on the drawing's socket room. Editor.tsx subscribes and reloads — coarse but correct, since the cleanup intentionally drops in-progress soft-deleted state that the editor would otherwise try to merge. Change-Id: Id0ce5f4ea7eabdb5af3531e33556d1aaa73ae180 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
The previous storage.integration.ts re-implemented trim/diff/orphan
logic inline against Prisma and asserted on its own work — a tautology
that could not catch real bugs in the route handlers, and one assertion
("Wrong Name" !== "My Drawing") was just a JS string compare.
Now boots the actual Express app via the same supertest+CSRF pattern
used by drawings-shared.integration.ts and exercises:
- POST /drawings/:id/trim: happy path, version monotonicity (regression
for the trim version-reset bug), confirmName mismatch (403),
non-owner (404).
- DELETE /drawings/:id/files/orphans: happy path including soft-deleted
element cleanup and version bump, blocked by active references (400),
empty payload (400), confirmName mismatch (403).
- GET /drawings/:id/files/diff: canvas/sqlite presence columns.
S3-side branches stay uncovered (S3 disabled in tests) — that needs a
mock S3 client and is out of scope for this rewrite.
Change-Id: I6ffa867b9beab7337906bdbfa911824ad77e34ec
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: OhYee <oyohyee@oyohyee.com>
The trim handler wrote `version: 1` literally on completion. Any open
editor with version > 1 would either get a false VERSION_CONFLICT on
their next save, or — worse — silently have their newer version
overwritten by a later updateMany whose version filter happened to
match the reset value. Use { increment: 1 } to match orphan-delete and
the rest of the codebase, so concurrent editors hit a real conflict
and reload.
Caught by the storage integration tests, which now lock in the
post-trim version is the pre-trim version + 1.
Change-Id: I3c76e5838bc2eadd9960b550ff27a38c87b7173a
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: OhYee <oyohyee@oyohyee.com>
…wing check
Storage-management portion of the per-drawing storage isolation fix
that began in feat/s3-image-upload + refactor/backend-direct-upload.
S3File rows are now keyed by (drawingId, fileId) and S3 objects sit
under a per-drawing path, so each drawing's storage is independent of
every other drawing's. Concretely:
- routes/storage.ts: trim/diff/orphans queries are scoped by drawingId
via the shared drawingS3Prefix() helper instead of inline
startsWith() checks against s3Key.
- The findFileIdsStillReferencedElsewhere() helper is gone — it was
protecting against cross-drawing collisions on a global PK that no
longer exists. Each drawing now owns its own row + object exclusively
(duplicates create their own copies in routes/dashboard/drawings.ts).
- DELETE /:fileId in the orphans path uses { drawingId_fileId }.
Change-Id: I4fd81b44384328a29f5890413586aadbfc96f8d1
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three review fixes on the orphan-delete path:
- Validate every entry in the request's `fileIds` array against the
same `^[\w-]{1,200}$` regex used elsewhere. Without this, a non-
string or path-traversal-shaped id slipped past the array-shape
check and would only blow up inside the Prisma / S3 calls below.
- Batch the cleanup. The previous loop did per-file
`s3File.findUnique` + `deleteS3Object` + `s3File.delete` serially.
For large selections that's N+1 round trips and a stalled HTTP
request. Use a single `findMany` keyed by the fileId set, parallel
S3 deletes (concurrency 8) with `Promise.allSettled` so one S3
failure doesn't drop subsequent deletions, and a single
`deleteMany` for the rows.
- The diff test's `totalCanvasRefs: 2` came with the comment "active
only", but the route counts every canvas-referenced fileId
including soft-deleted ones (`file-a` active + `file-b` deleted).
Fix the comment so it documents the real semantic.
Change-Id: Id61e9a2d6f17ccc6e6c8922e6b8e6549b32c73f4
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related bugs in the previous boolean-only cache: 1. Stampede: `isS3Enabled()` was called from every `DrawingCard` that mounts. With several cards mounting in the same render pass, each call hit the cache as `null`, fired its own GET /files/config, and raced. Now we cache the in-flight Promise so concurrent callers coalesce onto a single request. 2. Sticky-false on transient failures: a 401 during the auth-status bootstrap or a one-off network blip permanently latched the cache to `false`, disabling S3 uploads for the rest of the page lifetime even after auth recovered. Only a successful response is cached now; failures resolve `false` for that caller but leave the cache open for retry. Change-Id: Ib38aae45e2724057f12b9a8d03a3603381ecef6a Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OhYee
force-pushed
the
feat/storage-management
branch
from
4cd7e9a to
fdb51b9
Compare
Contributor
Author
|
Thanks @copilot. Both items applied:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #145. Depends on #163, please review/merge that first.
Why
The drift between canvas refs,
Drawing.filesJSON keys, and the actual S3 bucket is inevitable: soft-deleted elements that still reference files, S3 objects with no DB row from partially-failed saves, etc. Operators need tooling to see and clean up that drift per drawing.What
Backend routes (require ownership + name confirmation):
POST /drawings/:id/trim— dropisDeletedelements, prune unreferenced file entries, delete the corresponding S3 objects +S3Filerows. RequiresconfirmNamematching the drawing name (GitHub-style guard). Bumpsversionso concurrent editors getVERSION_CONFLICTand reload (regression-tested: previously hard-codedversion: 1, which silently overwrote concurrent newer versions).GET /drawings/:id/files/diff— three-way diff over canvas refs, SQLite files keys, and S3 objects under the per-drawing prefix.DELETE /drawings/:id/files/orphans— selectively delete fileIds the caller knows are unused. Refuses fileIds active in non-deleted elements; also strips deleted-element references to deleted fileIds so the diff stops reporting them.Frontend:
StorageManageModalrenders the three-way diff with checkboxes.DrawingCardright-click menu.Notable safety fixes (review feedback, separate commits)
fix(storage): invalidate cache, share-aware orphan deletion, version bump— cache invalidated after every mutation; orphan-delete refuses to delete shared S3 objects (the earlier PR already isolates per drawing, kept as belt-and-braces); orphans route also bumps version.fix(storage): notify open editors of server-side mutations— emitsdrawing-server-updateon the socket room after every mutation;Editor.tsxlistens and reloads. Without this, a collaborator's next save would echo the trimmed-away elements back, undoing the cleanup.fix(storage): bump version on trim instead of resetting to 1— regression-locked by the new tests.fix(storage): drawing-scoped S3 queries— composite-PK aware queries for trim/diff/orphans (matches the earlier PR's per-drawing isolation model).test(storage): replace tautological tests with real route integration— the original tests re-implemented the route logic inline and asserted on their own work; replaced with supertest driving the real Express handler (8 cases: confirmName guards, version monotonicity, active-ref protection, empty payload, non-owner 404, etc.).Risk
All write operations are destructive but gated behind a name-confirmation prompt (matching the drawing's actual name). The diff query is read-only. Tests cover the version-bump behaviour so the trim regression that prompted the rewrite cannot recur.
Verification
npm run build,npm test(all pass; 8 storage integration tests cover this PR)