feat(cli): pluggable image storage with Vercel Blob impl (PR 1/3)#52
Merged
Conversation
Lays groundwork for AI-generated PR illustrations: a four-method ImageStorage interface (upload, urlFor, list, delete) audited against Vercel Blob, R2, S3, and Supabase. PR 1 ships only the Vercel Blob implementation; the StorageConfig discriminated union reserves the other three so .gitpulse.json schemas can stage migrations later. Includes a vitest config split so the network-touching integration test runs only on demand (yarn test:integration) and a CI workflow that exercises it on PRs from the same repo. Fork PRs skip via describe.skipIf without failing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a pluggable ImageStorage contract, a Vercel Blob provider implementation, Zod project config support, unit and integration Vitest suites, Vitest configs separating integration tests, and a GitHub Actions workflow that runs integration tests with Node 22 and Yarn. ChangesPluggable Image Storage with Vercel Blob
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Contributor
|
Vercel preview — built from |
|
| Filename | Overview |
|---|---|
| cli/src/image/storage/vercel-blob.ts | Core VercelBlobStorage implementation — clean constructor token guard, deterministic public URL construction, paginated list, and URL-mapped delete. Logic is sound. |
| cli/src/image/storage/types.ts | Thin ImageStorage interface and StorageConfig discriminated union — well-structured and forward-compatible with future providers. |
| cli/src/image/storage/index.ts | Factory createStorage() with exhaustive switch and never guard — correct and type-safe. |
| cli/src/image/storage/vercel-blob.integration.test.ts | Real round-trip integration test — comprehensive flow (upload → fetch → list → delete → 404 check), but waitFor404 timeout (10 s) is shorter than documented CDN propagation ceiling (~60 s), creating a flake risk. |
| cli/src/image/storage/vercel-blob.test.ts | Thorough unit tests covering storeIdToHost edge cases, URL encoding, pagination, and no-op delete — all mocked correctly. |
| cli/src/project-config.ts | Adds StorageProviderSchema discriminated union to the project config with proper validation — clean extension that doesn't break existing parsers. |
| .github/workflows/storage-integration.yml | CI workflow correctly gates on same-repo PRs to protect secrets, passes required env vars, and separates integration from unit runs. |
| cli/vitest.integration.config.ts | Integration vitest config with 30 s timeout — margin may be tight if CDN propagation reaches its documented upper bound during the 404 poll. |
| cli/vitest.config.ts | Unit test config correctly excludes *.integration.test.ts files so network-free tests stay isolated. |
Sequence Diagram
sequenceDiagram
participant Caller
participant createStorage
participant VercelBlobStorage
participant VercelBlobSDK as @vercel/blob SDK
participant CDN as Vercel CDN
Caller->>createStorage: createStorage(config)
createStorage->>VercelBlobStorage: "new VercelBlobStorage({ storeId })"
Note over VercelBlobStorage: validates BLOB_READ_WRITE_TOKEN<br/>derives host via storeIdToHost()
Caller->>VercelBlobStorage: upload(key, body, contentType)
VercelBlobStorage->>VercelBlobSDK: "put(key, body, { access:'public', token })"
VercelBlobSDK-->>CDN: store blob
Caller->>VercelBlobStorage: urlFor(key)
VercelBlobStorage-->>Caller: https://host/encodePath(key)
Caller->>VercelBlobStorage: list(prefix)
loop paginated
VercelBlobStorage->>VercelBlobSDK: "blobList({ prefix, cursor, token })"
VercelBlobSDK-->>VercelBlobStorage: "{ blobs[], cursor }"
end
VercelBlobStorage-->>Caller: pathname[]
Caller->>VercelBlobStorage: delete(keys[])
VercelBlobStorage->>VercelBlobStorage: keys.map(urlFor)
VercelBlobStorage->>VercelBlobSDK: "del(urls[], { token })"
VercelBlobSDK-->>CDN: remove blobs
Reviews (3): Last reviewed commit: "refactor(cli): tighten storage integrati..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 4
🤖 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 `@cli/src/image/storage/vercel-blob.integration.test.ts`:
- Around line 41-42: The test currently asserts an immediate 404 after deletion
(variable afterDelete and the fetch(url, { cache: 'no-store' }) call), which
flakes due to eventual consistency; replace the single
expect(afterDelete.status).toBe(404) with retry logic that re-fetches the URL
until a 404 is observed or a timeout elapses (e.g., up to 60s with a short
interval like 1s), returning success as soon as response.status === 404 and
throwing/failing the test if the timeout is reached; keep using cache:
'no-store' and ensure the retry loop wraps the fetch call used to set
afterDelete.
In `@cli/src/image/storage/vercel-blob.ts`:
- Around line 35-37: The urlFor method currently interpolates the raw key into
the URL which breaks for spaces, #, ?, or unicode; update urlFor(key: string) to
URL-encode path segments (e.g., split key on '/' and apply encodeURIComponent to
each segment, then join with '/') before building `https://${this.host}/${...}`
and apply the same encoding approach wherever the key is used to build public
URLs (including the delete() URL construction referenced in the diff) so both
urlFor and delete use the same encoded-key logic.
- Around line 60-63: storeIdToHost should validate and normalize storeId: trim
whitespace, ensure it's not empty, remove a case-insensitive "store_" prefix,
convert to lowercase, replace any characters not allowed in DNS labels with
hyphens, collapse consecutive hyphens and trim leading/trailing hyphens, and
throw an error if the resulting slug is empty; update the function storeIdToHost
to perform these checks and normalization before returning
`${slug}.public.blob.vercel-storage.com` (refer to the storeIdToHost function to
locate the change).
In `@cli/vitest.config.ts`:
- Around line 1-6: The current defineConfig call replaces Vitest's default
exclude patterns by setting test.exclude directly; update the configuration to
merge your additional patterns with Vitest's defaults by importing and using
configDefaults.exclude and spreading it into test.exclude so that defineConfig({
test: { exclude: [...configDefaults.exclude, '**/*.integration.test.ts'] } })
preserves defaults while adding your custom patterns (reference defineConfig,
test.exclude, and configDefaults.exclude when making the change).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6f7b706-3eda-4166-a189-d04885d20a2d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.github/workflows/storage-integration.ymlcli/package.jsoncli/src/image/storage/index.tscli/src/image/storage/types.tscli/src/image/storage/vercel-blob.integration.test.tscli/src/image/storage/vercel-blob.test.tscli/src/image/storage/vercel-blob.tscli/src/project-config.tscli/vitest.config.tscli/vitest.integration.config.ts
- 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>
Owner
Author
Review fixes (commit e148118)Addressed:
Skipped:
70 unit tests pass; integration test still passes locally and in CI. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cli/src/image/storage/vercel-blob.integration.test.ts (1)
41-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPost-delete wait window is still shorter than the documented propagation window.
Line 43 waits only
10_000mseven though Line 41 notes cache/CDN propagation can be ~60s. This can still flake in CI; increase the wait budget (or make it configurable) and align integration test timeout accordingly.What does Vercel Blob documentation state about cache/CDN propagation time after delete or overwrite operations, and is a 10-second post-delete retry window typically sufficient?Also applies to: 48-58
🤖 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 `@cli/src/image/storage/vercel-blob.integration.test.ts` around lines 41 - 44, The post-delete retry window used by waitFor404(url, 10_000) is too short compared to Vercel Blob's documented CDN propagation (~60s); update the test to increase the wait budget to at least 60_000ms (or make the timeout configurable via a constant/env var) and adjust the integration test timeout accordingly so the expect(status).toBe(404) is retried long enough; apply the same change to the other similar block around lines 48-58 that also calls waitFor404 or similar post-delete checks.cli/src/image/storage/vercel-blob.ts (1)
58-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden
storeIdToHostnormalization before composing hostname.Line 61 strips only lowercase
store_. Inputs likeSTORE_abckeep the underscore in the host label and produce malformed URLs. Make prefix removal case-insensitive and normalize/validate slug characters before returning the host.♻️ Proposed fix
export function storeIdToHost(storeId: string): string { const trimmed = storeId.trim(); if (!trimmed) throw new Error('storeId is required'); - const slug = trimmed.replace(/^store_/, '').toLowerCase(); + const slug = trimmed + .replace(/^store_/i, '') + .toLowerCase() + .replace(/[^a-z0-9-]/g, '-') + .replace(/-+/g, '-') + .replace(/^-|-$/g, ''); if (!slug) throw new Error(`Invalid storeId: ${storeId}`); return `${slug}.public.blob.vercel-storage.com`; }🤖 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 `@cli/src/image/storage/vercel-blob.ts` around lines 58 - 63, The storeIdToHost function currently only strips a lowercase "store_" prefix and can produce invalid hostnames for inputs like "STORE_abc"; update storeIdToHost to remove the "store_" prefix case-insensitively, then normalize and validate the resulting slug (lowercase it, replace or remove invalid hostname characters, collapse consecutive non-alphanumerics into single hyphens, trim leading/trailing hyphens) and throw an error if the final slug is empty or contains invalid characters; make these changes inside the storeIdToHost implementation so the returned value is always a valid hostname label (used when composing `${slug}.public.blob.vercel-storage.com`).
🤖 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.
Duplicate comments:
In `@cli/src/image/storage/vercel-blob.integration.test.ts`:
- Around line 41-44: The post-delete retry window used by waitFor404(url,
10_000) is too short compared to Vercel Blob's documented CDN propagation
(~60s); update the test to increase the wait budget to at least 60_000ms (or
make the timeout configurable via a constant/env var) and adjust the integration
test timeout accordingly so the expect(status).toBe(404) is retried long enough;
apply the same change to the other similar block around lines 48-58 that also
calls waitFor404 or similar post-delete checks.
In `@cli/src/image/storage/vercel-blob.ts`:
- Around line 58-63: The storeIdToHost function currently only strips a
lowercase "store_" prefix and can produce invalid hostnames for inputs like
"STORE_abc"; update storeIdToHost to remove the "store_" prefix
case-insensitively, then normalize and validate the resulting slug (lowercase
it, replace or remove invalid hostname characters, collapse consecutive
non-alphanumerics into single hyphens, trim leading/trailing hyphens) and throw
an error if the final slug is empty or contains invalid characters; make these
changes inside the storeIdToHost implementation so the returned value is always
a valid hostname label (used when composing
`${slug}.public.blob.vercel-storage.com`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1965fca1-05ea-4dee-a1bc-6af4a00ad00d
📒 Files selected for processing (4)
cli/src/image/storage/vercel-blob.integration.test.tscli/src/image/storage/vercel-blob.test.tscli/src/image/storage/vercel-blob.tscli/vitest.config.ts
- Pin actions/checkout to v6.0.2 and actions/setup-node to v6.4.0 (latest releases). Tighter than floating @v6 without going full SHA. - Drop describe.skipIf — the test now hard-fails when env vars are missing instead of silently passing as skipped. Both BLOB_READ_WRITE_TOKEN and GITPULSE_TEST_STORE_ID are required at import time. - Add a job-level if-guard to the workflow so fork PRs (which can't read secrets) skip the workflow entirely rather than failing every PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli/src/image/storage/vercel-blob.integration.test.ts (1)
42-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncrease post-delete polling window to match documented propagation behavior.
Line 42 uses a 10s wait, but Lines 40-41 state propagation can be ~60s. This can still produce flaky failures in CI.
Proposed patch
- it('uploads, fetches, lists, and deletes a blob', async () => { + it('uploads, fetches, lists, and deletes a blob', async () => { @@ - const status = await waitFor404(url, 10_000); + const status = await waitFor404(url, 60_000); expect(status).toBe(404); - }); + }, 75_000);What does Vercel Blob document as cache/CDN propagation delay after delete, and can reads still succeed during that window?🤖 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 `@cli/src/image/storage/vercel-blob.integration.test.ts` around lines 42 - 43, The test's post-delete polling window is too short: replace the 10_000 timeout used in waitFor404(url, 10_000) with a longer window that matches Vercel Blob's documented ~60s propagation (e.g., 60_000 or 65_000) so CI doesn't flake; update the call to waitFor404 and any nearby comment that mentions propagation to reflect the new timeout and rationale.
🤖 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 @.github/workflows/storage-integration.yml:
- Line 13: The job currently declares "runs-on: ubuntu-latest" without a
timeout; add a job-level timeout by inserting a "timeout-minutes: <N>" entry
(e.g. 20–60 minutes) alongside the "runs-on" key in the same job definition so
the network integration workflow cannot hang indefinitely; update the job that
contains runs-on: ubuntu-latest (the network integration job) to include the
timeout-minutes setting.
---
Duplicate comments:
In `@cli/src/image/storage/vercel-blob.integration.test.ts`:
- Around line 42-43: The test's post-delete polling window is too short: replace
the 10_000 timeout used in waitFor404(url, 10_000) with a longer window that
matches Vercel Blob's documented ~60s propagation (e.g., 60_000 or 65_000) so CI
doesn't flake; update the call to waitFor404 and any nearby comment that
mentions propagation to reflect the new timeout and rationale.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0d42e85-555c-41c2-9cde-ce8b5f1f2bc4
📒 Files selected for processing (2)
.github/workflows/storage-integration.ymlcli/src/image/storage/vercel-blob.integration.test.ts
|
|
||
| jobs: | ||
| test: | ||
| runs-on: ubuntu-latest |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a job timeout for this network integration workflow.
Line 13 defines a network-dependent job without timeout-minutes; a stuck call can tie up runners unnecessarily.
Proposed patch
test:
runs-on: ubuntu-latest
+ timeout-minutes: 15📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| runs-on: ubuntu-latest | |
| test: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 |
🤖 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 @.github/workflows/storage-integration.yml at line 13, The job currently
declares "runs-on: ubuntu-latest" without a timeout; add a job-level timeout by
inserting a "timeout-minutes: <N>" entry (e.g. 20–60 minutes) alongside the
"runs-on" key in the same job definition so the network integration workflow
cannot hang indefinitely; update the job that contains runs-on: ubuntu-latest
(the network integration job) to include the timeout-minutes setting.
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
First slice of the AI-image work — introduces a pluggable storage layer for binary assets. No image generation yet; that lands in PR 2.
ImageStorageinterface (cli/src/image/storage/types.ts): four methods —upload,urlFor,list,delete— audited against Vercel Blob, Cloudflare R2, AWS S3, and Supabase Storage. All four providers implement all four methods natively.VercelBlobStorage(cli/src/image/storage/vercel-blob.ts): only provider implemented in this PR. Uses deterministic public URLs (https://<lowercase-storeId>.public.blob.vercel-storage.com/<key>) so we never need to round-trip through the SDK just to know where a blob lives.StorageConfigdiscriminated union reservesr2 | s3 | supabaseshapes in.gitpulse.jsonso future providers slot in without breaking parsers.createStorage()factory throws clearly for unimplemented providers.yarn testruns unit tests (network-free, fully mocked).yarn test:integrationruns a real round-trip against a Vercel Blob store, gated bydescribe.skipIfso missing creds are a clean skip, not a failure..github/workflows/storage-integration.yml) runs the integration test onpull_requestandpush: main. SecretBLOB_READ_WRITE_TOKENis already configured on the repo; the store IDstore_ryXhp9N7MsNEbMh1is hardcoded in the workflow (it's public — appears in every blob URL).Architecture context
The original plan considered per-branch Blob stores for preview isolation, but Vercel's auto-link mechanism only allows one
BLOB_READ_WRITE_TOKENenv var per project — multi-store-per-project fails on env var conflict. We pivoted to one shared store, path-namespaced by branch, which preserves the isolation property without fighting the platform. PR 2 will introduce the_preview/<branch>/...key convention; PR 3 will add anon: pull_request.closedcleanup workflow that callslist(prefix)+delete(keys).Test plan
Storage Integrationworkflow passes on this PRyarn workspace @gitpulse/cli testcontinues to pass (66 tests, no integration)yarn workspace @gitpulse/cli typecheckcleanBLOB_READ_WRITE_TOKEN=... GITPULSE_TEST_STORE_ID=store_ryXhp9N7MsNEbMh1 yarn workspace @gitpulse/cli test:integrationuploads → fetches 200 → lists → deletes → 404 (verified during development)Out of scope (future PRs)
processCommithook,Story.imageKeyschema field, site rendering inPRFeedItem+ commit detail page🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores