refactor(brands): migrate brand-loader to StorageAdapter#51
Conversation
Replace direct fs calls with StorageAdapter interface so brand loading works against both local filesystem and Azure Blob Storage. - brand-loader.ts: all I/O through getStorageAdapter() - BrandProfile paths now store container-relative keys - Logo proxy and generate routes use adapter for reads - Tests rewritten to mock StorageAdapter instead of node:fs
Use Awaited<ReturnType<typeof fs.readdir>> instead of Dirent[] to match the NonSharedBuffer generic parameter in newer @types/node.
There was a problem hiding this comment.
Pull request overview
This PR refactors brand-related I/O to route through StorageAdapter so the same codepaths can run on local filesystem or Azure Blob Storage, and updates BrandProfile to store container-relative keys instead of absolute filesystem paths.
Changes:
- Migrates
brand-loaderto read/validate brand fixtures and assets viagetStorageAdapter(), returning container-relative keys in the hydrated profile. - Updates the generate pipeline and logo proxy route to read brand assets through
StorageAdapterrather thannode:fs. - Rewrites brand-loader tests to mock
StorageAdapter, and adjusts storage-adapter tests for newer Nodereaddirtypings.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/storage-adapter.test.ts | Adjusts readdir mock typing to align with updated Node type overloads. |
| tests/brand-loader.test.ts | Reworks tests to mock getStorageAdapter() and validate container-relative keys. |
| lib/cast/server/pipeline/resolve.ts | Updates resolver docs to reflect container-relative keys + adapter-based reads. |
| lib/cast/server/brand-loader.ts | Switches brand fixture reads/existence checks from fs to StorageAdapter; returns keys instead of absolute paths. |
| lib/cast/schemas.ts | Updates BrandProfile docs to describe container-relative key fields. |
| app/api/generate/route.ts | Reads product-image (“products” source) assets via StorageAdapter. |
| app/api/brands/[slug]/logos/[id]/route.ts | Reads logo bytes via StorageAdapter instead of direct filesystem reads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reject item.file values containing .., ., backslashes, or absolute paths before building container-relative keys. Surfaces violations as BrandInvalidError (400) instead of an unexpected PathTraversalError. Addresses Copilot review on PR #51: - brand-loader.ts — "item.file contains backslashes or traversal segments" - brand-loader.ts — "backgrounds.json item.file is not constrained"
Copilot Review — Round 1 Summary
All 4 threads resolved. Requesting re-review. |
Addresses Copilot review on PR #51 (round 2): - brand-loader.ts:validateItemFile — reject backslashes upfront instead of silently splitting on them; container keys are forward-slash only - brand-loader.test.ts — rename traversal-via-backslash test, add pure backslash test case (products\\can.png)
PR #51 — Copilot review addressed (round 2)Pre-flight: round 2/3 | CI: no checks configured | pending review: no Commits: Details:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
app/api/brands/[slug]/logos/[id]/route.ts:62
- The error handling after
storage.readFile()assumes a Node-style errno witherr.code. With the AzureBlobAdapter, missing blobs typically throw an AzureRestError(e.g.statusCode === 404/code === "BlobNotFound"), so this route may throw instead of returning a 404. Consider normalizing StorageAdapter not-found errors to{ code: "ENOENT" }across backends (preferred), or update this catch block to also treat Azure 404-style errors as "not found" (or preflight withfileExists()and map false to 404).
buf = await storage.readFile("inputs", variant.path)
} catch (err) {
const code = (err as NodeJS.ErrnoException).code
if (code === "ENOENT" || code === "EACCES" || code === "EPERM") {
return NextResponse.json(
…y brand.json - validateItemFile: reject inputs that split to zero segments (defense-in-depth) - listBrandSlugs: only list slugs whose brands/<slug>/brand.json key exists, consistent with loadBrandProfile semantics - Add tests: empty-segments rejection, partial-brand exclusion (224 pass)
Round 3 — Copilot Review Summary3 threads triaged → 2 fixed, 1 deferred
Tests: 224 passing (+2 new: empty-segments rejection, partial-brand exclusion) Cumulative across 3 rounds: 9 threads → 6 fixed, 1 reply-only (intentional semantic change), 2 deferred (#52 |
Changes
getStorageAdapter()instead of directnode:fscallsbrands/acme/font.otf) instead of absolute filesystem pathsreadBrandAsset()uses adapter for product image readsnode:fsVerification
pnpm typecheckpasses