refactor(cli): address PR review feedback on image storage

znat · claude · znat · commit e14811881b15 · 2026-05-11T09:16:27.000+02:00
- Drop unused storeId field on VercelBlobStorage (greptile) - URL-encode path segments in urlFor() so keys with spaces, ?, # or unicode produce valid URLs — required once PR 2/3 introduce branch names in key paths (coderabbit) - Harden storeIdToHost(): trim whitespace, throw on empty or prefix-only input (coderabbit) - Retry the post-delete 404 check up to 10s in the integration test; Vercel Blob is eventually consistent at the CDN, so an immediate re-fetch can briefly return cached bytes (coderabbit) - Extend (not replace) vitest default excludes via configDefaults.exclude so node_modules, dist, .idea, etc. stay excluded by default (coderabbit) Skipped: SHA-pinning GitHub Actions in storage-integration.yml — would be inconsistent with the rest of the repo (ci.yml, deploy-vercel*.yml all use floating @v6 tags). Belongs in a separate repo-wide PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
diff --git a/cli/src/image/storage/vercel-blob.integration.test.ts b/cli/src/image/storage/vercel-blob.integration.test.ts
@@ -38,7 +38,21 @@ describe.skipIf(!hasCreds)('VercelBlobStorage integration', () => {
     await storage.delete([key]);
     createdKeys.length = 0;
 
-    const afterDelete = await fetch(url, { cache: 'no-store' });
-    expect(afterDelete.status).toBe(404);
+    // Vercel Blob is eventually consistent at the CDN (up to ~60s propagation
+    // per docs). Retry briefly so CI doesn't flake on the post-delete check.
+    const status = await waitFor404(url, 10_000);
+    expect(status).toBe(404);
   });
 });
+
+async function waitFor404(url: string, timeoutMs: number): Promise<number> {
+  const deadline = Date.now() + timeoutMs;
+  let status = 0;
+  while (Date.now() < deadline) {
+    const res = await fetch(url, { cache: 'no-store' });
+    status = res.status;
+    if (status === 404) return status;
+    await new Promise((r) => setTimeout(r, 250));
+  }
+  return status;
+}
diff --git a/cli/src/image/storage/vercel-blob.test.ts b/cli/src/image/storage/vercel-blob.test.ts
@@ -25,6 +25,21 @@ describe('storeIdToHost', () => {
   it('handles ids without store_ prefix (defensive)', () => {
     expect(storeIdToHost('XyZ')).toBe('xyz.public.blob.vercel-storage.com');
   });
+
+  it('trims whitespace', () => {
+    expect(storeIdToHost('  store_abc  ')).toBe(
+      'abc.public.blob.vercel-storage.com',
+    );
+  });
+
+  it('throws on empty or whitespace-only input', () => {
+    expect(() => storeIdToHost('')).toThrow(/storeId is required/);
+    expect(() => storeIdToHost('   ')).toThrow(/storeId is required/);
+  });
+
+  it('throws when the prefix is the entire value', () => {
+    expect(() => storeIdToHost('store_')).toThrow(/Invalid storeId/);
+  });
 });
 
 describe('VercelBlobStorage', () => {
@@ -66,6 +81,18 @@ describe('VercelBlobStorage', () => {
     );
   });
 
+  it('urlFor encodes special characters in path segments but preserves slashes', () => {
+    const storage = new VercelBlobStorage(
+      { storeId: STORE_ID },
+      { BLOB_READ_WRITE_TOKEN: TOKEN },
+    );
+    // Branch names in PR 2 keys can include spaces, ?, #, etc. Each segment
+    // gets percent-encoded; slashes stay as path separators.
+    expect(storage.urlFor('feature/foo bar/#hash?.webp')).toBe(
+      'https://abcdef123.public.blob.vercel-storage.com/feature/foo%20bar/%23hash%3F.webp',
+    );
+  });
+
   it('list returns pathnames and follows cursors across pages', async () => {
     vi.mocked(blobList)
       .mockResolvedValueOnce({
diff --git a/cli/src/image/storage/vercel-blob.ts b/cli/src/image/storage/vercel-blob.ts
@@ -6,7 +6,6 @@ export interface VercelBlobStorageOptions {
 }
 
 export class VercelBlobStorage implements ImageStorage {
-  private readonly storeId: string;
   private readonly token: string;
   private readonly host: string;
 
@@ -17,7 +16,6 @@ export class VercelBlobStorage implements ImageStorage {
         'BLOB_READ_WRITE_TOKEN env var is required for vercel-blob storage',
       );
     }
-    this.storeId = opts.storeId;
     this.token = token;
     this.host = storeIdToHost(opts.storeId);
   }
@@ -33,7 +31,7 @@ export class VercelBlobStorage implements ImageStorage {
   }
 
   urlFor(key: string): string {
-    return `https://${this.host}/${key}`;
+    return `https://${this.host}/${encodePath(key)}`;
   }
 
   async list(prefix: string): Promise<string[]> {
@@ -58,6 +56,16 @@ export class VercelBlobStorage implements ImageStorage {
 // https://<lowercase-storeId-without-prefix>.public.blob.vercel-storage.com/<key>
 // confirmed by derisking against the real API.
 export function storeIdToHost(storeId: string): string {
-  const slug = storeId.replace(/^store_/, '').toLowerCase();
+  const trimmed = storeId.trim();
+  if (!trimmed) throw new Error('storeId is required');
+  const slug = trimmed.replace(/^store_/, '').toLowerCase();
+  if (!slug) throw new Error(`Invalid storeId: ${storeId}`);
   return `${slug}.public.blob.vercel-storage.com`;
 }
+
+// Encode a slash-delimited key as URL path segments, preserving the slashes
+// as path separators. Keys can include branch names or other arbitrary
+// strings in PR 2/3, so we must handle spaces, ?, #, and unicode safely.
+function encodePath(key: string): string {
+  return key.split('/').map(encodeURIComponent).join('/');
+}
diff --git a/cli/vitest.config.ts b/cli/vitest.config.ts
@@ -1,7 +1,7 @@
-import { defineConfig } from 'vitest/config';
+import { configDefaults, defineConfig } from 'vitest/config';
 
 export default defineConfig({
   test: {
-    exclude: ['**/node_modules/**', '**/dist/**', '**/*.integration.test.ts'],
+    exclude: [...configDefaults.exclude, '**/*.integration.test.ts'],
   },
 });
