Set the repository image branch to the repository's default branch and add a delete button to delete built images#559
Open
kartikye wants to merge 3 commits intoColeMurray:mainfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds the ability to delete pre-built stored repository images across the entire stack: new database methods for retrieving and removing stored images, a control plane DELETE endpoint that orchestrates Modal provider image deletion followed by database cleanup, a web API layer that forwards requests to the control plane, and UI controls with deletion state tracking and error handling. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant WebAPI as Web API<br/>/repo-images/:owner/:name
participant CP as Control Plane<br/>/repo-images/:owner/:name
participant DB as Database<br/>(D1)
participant Modal as Modal API
Client->>WebAPI: DELETE /api/repo-images/:owner/:name
WebAPI->>WebAPI: Check supportsRepoImages()
WebAPI->>WebAPI: Verify authenticated session
WebAPI->>CP: DELETE /repo-images/:owner/:name
CP->>DB: getStoredImagesForRepo(owner, name)
DB-->>CP: Return list of stored images
par Delete Provider Images
loop For each stored image
CP->>Modal: DELETE provider_image_id
Modal-->>CP: { deleted: true/false }
end
end
CP->>CP: Verify all deletions succeeded
CP->>DB: deleteStoredImagesForRepo(owner, name)
DB-->>CP: Return deletion count
CP-->>WebAPI: { ok: true, deleted: count }
WebAPI-->>Client: { ok: true, deleted: count }
Client->>Client: Mutate SWR cache
Client->>Client: Update UI
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 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/control-plane/src/db/repo-images.test.ts (1)
630-698: Mismatched describe label.The first test inside
describe("deleteStoredImagesForRepo", ...)actually exercisesgetStoredImagesForRepo(returns stored rows without building entries). Consider splitting into two describe blocks, one per method, for clearer test reporting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/db/repo-images.test.ts` around lines 630 - 698, The first test under describe("deleteStoredImagesForRepo", ...) is actually testing getStoredImagesForRepo; update the test organization so each method has its own describe: move or recreate the "returns stored rows without building entries" it-block under a separate describe("getStoredImagesForRepo", ...) and keep the other two it-blocks under describe("deleteStoredImagesForRepo", ...), ensuring the test names and expectations remain unchanged; reference the functions getStoredImagesForRepo and deleteStoredImagesForRepo to locate the correct tests in repo-images.test.ts.packages/control-plane/test/integration/repo-images.test.ts (1)
500-528: Consider also asserting the building row's id/state isn't consumed.The current assertion (
status[0].status === "building") is fine, but explicitly checkingstatus[0].id === "img-building"would harden the test against accidental deletion of the wrong row if the filter regressed (e.g., ifstatuswere ever stored case-insensitively or with whitespace).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/test/integration/repo-images.test.ts` around lines 500 - 528, Add an explicit assertion that the remaining build row is the expected one by checking its id: after calling store.getStatus("acme", "repo") and the existing expect(status[0].status).toBe("building"), also assert expect(status[0].id).toBe("img-building") in the "DELETE /repo-images/:owner/:name deletes stored images only" test so the test fails if the wrong row is kept; locate the assertions around the variable status in this test to insert the new check.packages/control-plane/src/routes/repo-images.ts (1)
345-348: Throwing on!result.deleteddiscards which image failed.The current message (
Failed to delete N provider images) doesn't include the offendingprovider_image_id(s) — important for ops debugging. Include the ids in the error message and/or in a structuredlogger.errorbefore throwing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/routes/repo-images.ts` around lines 345 - 348, The current check that throws when undeleted.length > 0 loses which images failed; update the block that builds undeleted from results (the results array and undeleted constant) to collect the failing provider_image_id values (e.g., undeleted.map(r => r.provider_image_id)), log them via logger.error with context, and include the list of ids in the thrown Error message (e.g., `Failed to delete N provider images: [id1,id2]`) so operators can see which provider_image_id(s) failed.packages/web/src/components/settings/images-settings.tsx (2)
207-215: Add a confirmation step before destructive deletion.The delete button immediately fires a
DELETEto the control plane on click. Since this removes provider images and stored DB rows for the repo (irreversible from the UI), consider a confirmation prompt (e.g.,window.confirmor an existing dialog component) to prevent accidental clicks.♻️ Minimal example using
window.confirm<Button variant="destructive" size="icon" - onClick={() => handleDelete(repo.owner, repo.name)} + onClick={() => { + if ( + window.confirm( + `Delete stored pre-built images for ${repo.owner}/${repo.name}? This cannot be undone.` + ) + ) { + handleDelete(repo.owner, repo.name); + } + }} disabled={isDeleting || !image || image.status === "building"} title="Delete stored images" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/settings/images-settings.tsx` around lines 207 - 215, The delete Button currently calls handleDelete immediately; add a confirmation step so destructive deletion isn't triggered by accident: update the onClick handler for the Button (the JSX using TrashIcon, isDeleting, image.status) to first prompt the user (e.g., window.confirm or your existing dialog component) and only call handleDelete(repo.owner, repo.name) if the user confirms; ensure the disabled logic (isDeleting or no image or image.status === "building") remains unchanged and that any confirmation implementation short-circuits without calling handleDelete when cancelled.
122-148: Defensive parsing of error body.
await res.json()will throw if the control-plane wrapper returned a non-JSON 5xx (e.g., a generic gateway error from the platform). The throw is caught by the outercatch, which will then surface "Failed to delete stored images" instead of any meaningful upstream error. Optional:await res.json().catch(() => ({}))to reach the fallback message in a single path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/settings/images-settings.tsx` around lines 122 - 148, The handleDelete function currently calls await res.json() which can throw on non-JSON 5xx responses; wrap the JSON parse so parsing failures are handled and you still surface any upstream error message—e.g., replace direct await res.json() with a safe parse (res.json().catch(() => ({})) or a try/catch around parsing) and then use errBody.error || "Failed to delete stored images" when calling setError; keep the existing flow of mutate(REPO_IMAGES_KEY) on success and the deletingRepos cleanup in the finally block.packages/control-plane/src/routes/repo-images.test.ts (2)
104-126: Tighten assertions: call count and ordering.The test name says "deletes provider images before removing stored records" but doesn't actually assert either of those properties:
- It doesn't assert that
deleteProviderImagewas called exactly once (theprovider_image_id: ""entry should be filtered out byBoolean(...)).- It doesn't assert the ordering between
deleteProviderImageanddeleteStoredImagesForRepo.expect(mockModalClient.deleteProviderImage).toHaveBeenCalledTimes(1); const deleteOrder = mockModalClient.deleteProviderImage.mock.invocationCallOrder[0]; const dbDeleteOrder = mockRepoImageStore.deleteStoredImagesForRepo.mock.invocationCallOrder[0]; expect(deleteOrder).toBeLessThan(dbDeleteOrder);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/routes/repo-images.test.ts` around lines 104 - 126, Add stricter assertions to the test so it verifies the provider-image delete is called exactly once and that it happens before the DB delete: after invoking handler, assert mockModalClient.deleteProviderImage was calledTimes(1) (since one entry has an empty provider_image_id and should be filtered), then compare invocationCallOrder between mockModalClient.deleteProviderImage and mockRepoImageStore.deleteStoredImagesForRepo to assert deleteProviderImage was invoked before deleteStoredImagesForRepo; keep references to mockRepoImageStore.getStoredImagesForRepo, mockModalClient.deleteProviderImage, and mockRepoImageStore.deleteStoredImagesForRepo so the checks point to the correct mocks.
48-56:createEnvcast hides missingEnvfields.The
as Envcast bypasses theEnvinterface check. If new requiredEnvproperties are added later, these tests will silently keep compiling while exercising an incomplete environment. Consider building a typed helper that extends aPartial<Env>with the explicit fields needed, or at least a comment noting fields are intentionally omitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/routes/repo-images.test.ts` around lines 48 - 56, The test helper createEnv currently uses a blunt "as Env" cast which masks missing properties; change it to build a typed Partial<Env> with only the explicit required fields and then explicitly convert to Env, or use TypeScript's "satisfies" if available. For example, declare const partial: Partial<Env> = { DB: {} as D1Database, SANDBOX_PROVIDER: "modal", MODAL_API_SECRET: "secret", MODAL_WORKSPACE: "workspace", WORKER_URL: "https://worker.test" }; then return partial as Env (or return partial satisfies Env) and add a short comment that omitted fields are intentional so future additions to Env will error if not handled. This targets the createEnv function and the Env type usage so tests fail if new required Env properties are added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/control-plane/src/db/repo-images.ts`:
- Around line 29-52: getStoredImagesForRepo and deleteStoredImagesForRepo can
race: a row that was "building" when fetched may become "ready" before the
generic DELETE (status != 'building'), causing provider images to be orphaned;
fix by changing deleteStoredImagesForRepo to accept the explicit ids returned by
getStoredImagesForRepo (or accept an id[] param), normalize ids if needed, and
perform a parameterized DELETE using "WHERE id IN (?,...,?)" so you delete
exactly the rows gathered earlier (update any callers such as
handleDeleteRepoImages to pass the collected ids instead of relying on the
status filter).
In `@packages/control-plane/src/routes/repo-images.ts`:
- Around line 334-349: Replace Promise.all with Promise.allSettled when calling
client.deleteProviderImage for providerImageIds so you can inspect per-call
outcomes; collect the providerImageIds that fulfilled (results.status ===
"fulfilled") vs rejected (status === "rejected", possibly ModalApiError), log
each outcome with its provider_image_id, then call deleteStoredImagesForRepo (or
the existing DB cleanup path) only for the successfully-deleted provider ids to
avoid removing rows for failed deletions; finally, if any deletions failed,
surface a partial-failure (207-style) or throw after logging, including the list
of failed provider IDs in the error/log so operators can reconcile.
---
Nitpick comments:
In `@packages/control-plane/src/db/repo-images.test.ts`:
- Around line 630-698: The first test under
describe("deleteStoredImagesForRepo", ...) is actually testing
getStoredImagesForRepo; update the test organization so each method has its own
describe: move or recreate the "returns stored rows without building entries"
it-block under a separate describe("getStoredImagesForRepo", ...) and keep the
other two it-blocks under describe("deleteStoredImagesForRepo", ...), ensuring
the test names and expectations remain unchanged; reference the functions
getStoredImagesForRepo and deleteStoredImagesForRepo to locate the correct tests
in repo-images.test.ts.
In `@packages/control-plane/src/routes/repo-images.test.ts`:
- Around line 104-126: Add stricter assertions to the test so it verifies the
provider-image delete is called exactly once and that it happens before the DB
delete: after invoking handler, assert mockModalClient.deleteProviderImage was
calledTimes(1) (since one entry has an empty provider_image_id and should be
filtered), then compare invocationCallOrder between
mockModalClient.deleteProviderImage and
mockRepoImageStore.deleteStoredImagesForRepo to assert deleteProviderImage was
invoked before deleteStoredImagesForRepo; keep references to
mockRepoImageStore.getStoredImagesForRepo, mockModalClient.deleteProviderImage,
and mockRepoImageStore.deleteStoredImagesForRepo so the checks point to the
correct mocks.
- Around line 48-56: The test helper createEnv currently uses a blunt "as Env"
cast which masks missing properties; change it to build a typed Partial<Env>
with only the explicit required fields and then explicitly convert to Env, or
use TypeScript's "satisfies" if available. For example, declare const partial:
Partial<Env> = { DB: {} as D1Database, SANDBOX_PROVIDER: "modal",
MODAL_API_SECRET: "secret", MODAL_WORKSPACE: "workspace", WORKER_URL:
"https://worker.test" }; then return partial as Env (or return partial satisfies
Env) and add a short comment that omitted fields are intentional so future
additions to Env will error if not handled. This targets the createEnv function
and the Env type usage so tests fail if new required Env properties are added.
In `@packages/control-plane/src/routes/repo-images.ts`:
- Around line 345-348: The current check that throws when undeleted.length > 0
loses which images failed; update the block that builds undeleted from results
(the results array and undeleted constant) to collect the failing
provider_image_id values (e.g., undeleted.map(r => r.provider_image_id)), log
them via logger.error with context, and include the list of ids in the thrown
Error message (e.g., `Failed to delete N provider images: [id1,id2]`) so
operators can see which provider_image_id(s) failed.
In `@packages/control-plane/test/integration/repo-images.test.ts`:
- Around line 500-528: Add an explicit assertion that the remaining build row is
the expected one by checking its id: after calling store.getStatus("acme",
"repo") and the existing expect(status[0].status).toBe("building"), also assert
expect(status[0].id).toBe("img-building") in the "DELETE
/repo-images/:owner/:name deletes stored images only" test so the test fails if
the wrong row is kept; locate the assertions around the variable status in this
test to insert the new check.
In `@packages/web/src/components/settings/images-settings.tsx`:
- Around line 207-215: The delete Button currently calls handleDelete
immediately; add a confirmation step so destructive deletion isn't triggered by
accident: update the onClick handler for the Button (the JSX using TrashIcon,
isDeleting, image.status) to first prompt the user (e.g., window.confirm or your
existing dialog component) and only call handleDelete(repo.owner, repo.name) if
the user confirms; ensure the disabled logic (isDeleting or no image or
image.status === "building") remains unchanged and that any confirmation
implementation short-circuits without calling handleDelete when cancelled.
- Around line 122-148: The handleDelete function currently calls await
res.json() which can throw on non-JSON 5xx responses; wrap the JSON parse so
parsing failures are handled and you still surface any upstream error
message—e.g., replace direct await res.json() with a safe parse
(res.json().catch(() => ({})) or a try/catch around parsing) and then use
errBody.error || "Failed to delete stored images" when calling setError; keep
the existing flow of mutate(REPO_IMAGES_KEY) on success and the deletingRepos
cleanup in the finally block.
🪄 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
Run ID: adb807db-0d37-4dcd-8939-f26ab40aef79
📒 Files selected for processing (8)
packages/control-plane/src/db/repo-images.test.tspackages/control-plane/src/db/repo-images.tspackages/control-plane/src/routes/repo-images.test.tspackages/control-plane/src/routes/repo-images.tspackages/control-plane/test/integration/repo-images.test.tspackages/web/src/app/api/repo-images/[owner]/[name]/route.tspackages/web/src/components/settings/images-settings.tsxpackages/web/src/components/ui/icons.tsx
Comment on lines
+29
to
+52
| async getStoredImagesForRepo(repoOwner: string, repoName: string): Promise<StoredRepoImage[]> { | ||
| const normalizedOwner = repoOwner.toLowerCase(); | ||
| const normalizedName = repoName.toLowerCase(); | ||
| const storedImages = await this.db | ||
| .prepare( | ||
| `SELECT id, provider_image_id FROM repo_images | ||
| WHERE repo_owner = ? AND repo_name = ? AND status != 'building'` | ||
| ) | ||
| .bind(normalizedOwner, normalizedName) | ||
| .all<StoredRepoImage>(); | ||
|
|
||
| return storedImages.results || []; | ||
| } | ||
|
|
||
| async deleteStoredImagesForRepo(repoOwner: string, repoName: string): Promise<number> { | ||
| const result = await this.db | ||
| .prepare( | ||
| "DELETE FROM repo_images WHERE repo_owner = ? AND repo_name = ? AND status != 'building'" | ||
| ) | ||
| .bind(repoOwner.toLowerCase(), repoName.toLowerCase()) | ||
| .run(); | ||
|
|
||
| return result.meta?.changes ?? 0; | ||
| } |
There was a problem hiding this comment.
Possible TOCTOU between stored-image lookup and deletion.
handleDeleteRepoImages in packages/control-plane/src/routes/repo-images.ts calls getStoredImagesForRepo to gather provider image ids, then later calls deleteStoredImagesForRepo to remove DB rows by the same status != 'building' filter. If a building row transitions to ready between those two calls, deleteStoredImagesForRepo will remove a DB row whose provider_image_id was never sent to Modal — orphaning the provider image.
Consider deleting by the explicit ids collected in getStoredImagesForRepo (e.g., a DELETE ... WHERE id IN (?, ?, ...)) so the two operations stay consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/control-plane/src/db/repo-images.ts` around lines 29 - 52,
getStoredImagesForRepo and deleteStoredImagesForRepo can race: a row that was
"building" when fetched may become "ready" before the generic DELETE (status !=
'building'), causing provider images to be orphaned; fix by changing
deleteStoredImagesForRepo to accept the explicit ids returned by
getStoredImagesForRepo (or accept an id[] param), normalize ids if needed, and
perform a parameterized DELETE using "WHERE id IN (?,...,?)" so you delete
exactly the rows gathered earlier (update any callers such as
handleDeleteRepoImages to pass the collected ids instead of relying on the
status filter).
Comment on lines
+334
to
+349
| if (providerImageIds.length > 0) { | ||
| const client = createModalClient(env.MODAL_API_SECRET, env.MODAL_WORKSPACE); | ||
| const results = await Promise.all( | ||
| providerImageIds.map((providerImageId) => | ||
| client.deleteProviderImage( | ||
| { providerImageId }, | ||
| { trace_id: ctx.trace_id, request_id: ctx.request_id } | ||
| ) | ||
| ) | ||
| ); | ||
|
|
||
| const undeleted = results.filter((result) => !result.deleted); | ||
| if (undeleted.length > 0) { | ||
| throw new Error(`Failed to delete ${undeleted.length} provider images`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Partial-failure handling on provider image deletion.
Promise.all rejects on the first throw, but the other in-flight deleteProviderImage calls still complete. If, say, image A deletes successfully on Modal but image B's call rejects with ModalApiError, the handler returns 500 and skips deleteStoredImagesForRepo — leaving the DB row for image A pointing at a provider image that no longer exists in Modal. On the next triggerBuild → markReady, the replaced-image path will try to delete a missing provider image (best-effort, OK), but getLatestReady may also surface a stale row in the meantime depending on status.
Consider Promise.allSettled so you can:
- record successful Modal deletions and only delete those DB rows by id, or
- attempt the DB cleanup unconditionally for provider ids that succeeded, and surface a 207-style partial response with the failed ids.
At minimum, log per-image outcomes (success/failure with provider_image_id) before throwing so operators can reconcile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/control-plane/src/routes/repo-images.ts` around lines 334 - 349,
Replace Promise.all with Promise.allSettled when calling
client.deleteProviderImage for providerImageIds so you can inspect per-call
outcomes; collect the providerImageIds that fulfilled (results.status ===
"fulfilled") vs rejected (status === "rejected", possibly ModalApiError), log
each outcome with its provider_image_id, then call deleteStoredImagesForRepo (or
the existing DB cleanup path) only for the successfully-deleted provider ids to
avoid removing rows for failed deletions; finally, if any deletions failed,
surface a partial-failure (207-style) or throw after logging, including the list
of failed provider IDs in the error/log so operators can reconcile.
kartikye
changed the title
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
mainWhy
Repo image handling was too rigid for repositories whose default branch is not
main, and there was no direct way toclear stored images from the UI. This change makes build triggering branch-aware and adds an explicit cleanup action
for stored images.
Impact
Users can now:
Happy to remove the delete button but if needed.
Summary by CodeRabbit
New Features
Bug Fixes