Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Cursor Agent can help with this pull request. Just |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a bulk-delete flow to the Cache Admin route: introduces a delete-all-matching intent, helper to compute matching cache keys, loader changes to compute matching counts, server-side bulk deletion across SQLite and LRU caches with primary-instance validation, and a UI button + confirmation to trigger the bulk delete. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as UI Component
participant Server as Server Action
participant SQLite as SQLite Cache
participant LRU as LRU Cache
User->>UI: Click "Delete All Matching"
UI->>UI: Show confirmation prompts
User->>UI: Confirm
UI->>Server: POST (intent=delete-all-matching, query, limit)
Server->>Server: Validate instance & params, compute matching keys
Server->>SQLite: Delete matching keys
SQLite-->>Server: Deletion result
Server->>LRU: Delete matching keys
LRU-->>Server: Deletion result
Server-->>UI: Return success and updated counts
UI->>User: Display result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (1)
app/routes/cache.admin.tsx (1)
122-123: UsematchingCacheValuesCountconsistently in the results badge.The count is recomputed inline at Line 155 even though it’s already derived at Lines 122-123. Reusing the variable avoids drift.
Small cleanup
<div className="absolute top-0 right-2 flex h-full w-14 items-center justify-between text-lg font-medium text-slate-500"> <span title="Total results shown"> - {data.cacheKeys.sqlite.length + data.cacheKeys.lru.length} + {matchingCacheValuesCount} </span> </div>Also applies to: 155-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/cache.admin.tsx` around lines 122 - 123, The code computes the total count twice; reuse the existing matchingCacheValuesCount variable (which equals data.cacheKeys.sqlite.length + data.cacheKeys.lru.length) wherever the inline sum is re-calculated (e.g., the results badge rendering) instead of recomputing data.cacheKeys.sqlite.length + data.cacheKeys.lru.length inline; update all occurrences (including the second instance noted) to reference matchingCacheValuesCount so the badge and any other UI elements stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/routes/cache.admin.tsx`:
- Around line 40-51: Sanitize and clamp the incoming limit inside
getMatchingCacheKeys: coerce to an integer (Math.floor), treat NaN or values <=
0 as a minimum of 1, and cap at a sensible MAX_LIMIT (e.g., 1000) to prevent
unbounded or invalid operations; then pass the sanitized value to
searchCacheKeys and getAllCacheKeys. Add a local MAX_LIMIT constant (or
file-level constant) and use something like sanitizedLimit = Math.min(MAX_LIMIT,
Math.max(1, Math.floor(limit) || 1)) before calling searchCacheKeys(query,
sanitizedLimit) or getAllCacheKeys(sanitizedLimit).
- Around line 73-77: Bulk deletes currently call cache.delete for each key and
can trigger fire-and-forget cross-replica writes; to prevent returning success
before replica operations complete, add a server-side guard to require the
primary instance for bulk delete: after resolving currentInstance via
getInstanceInfo (the existing currentInstance) and after
ensureInstance(instance) confirm that instance === currentInstance and if not
return an error/invariantResponse rejecting bulk delete; update the route
handling the bulk-delete loop (where cache.delete is iterated) to perform this
check and only proceed with the loop when the requested instance equals
currentInstance.
---
Nitpick comments:
In `@app/routes/cache.admin.tsx`:
- Around line 122-123: The code computes the total count twice; reuse the
existing matchingCacheValuesCount variable (which equals
data.cacheKeys.sqlite.length + data.cacheKeys.lru.length) wherever the inline
sum is re-calculated (e.g., the results badge rendering) instead of recomputing
data.cacheKeys.sqlite.length + data.cacheKeys.lru.length inline; update all
occurrences (including the second instance noted) to reference
matchingCacheValuesCount so the badge and any other UI elements stay consistent.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/routes/cache.admin.tsx (2)
38-59: Sanitization helpers look correct — past issue fully addressed.
sanitizeCacheKeysLimitcorrectly handles all degenerate inputs (NaN,0, negative, non-integer, and above-max).getMatchingCacheKeysconsistently applies the sanitized limit for both lookup paths. The previous review concern is resolved here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/cache.admin.tsx` around lines 38 - 59, sanitizeCacheKeysLimit and getMatchingCacheKeys are correct and the previous sanitization bug is resolved; no code changes needed—keep sanitizeCacheKeysLimit (handles non-finite, zero, negative, non-integer, and above-max values) and ensure getMatchingCacheKeys continues to call sanitizeCacheKeysLimit before invoking searchCacheKeys or getAllCacheKeys.
88-92: Primary-instance guard correctly addresses the fire-and-forget bulk-delete concern.Because
currentIsPrimaryis asserted before the deletion loop, allcache.deletecalls take the primary branch (preparedDelete.run) with no cross-replica fire-and-forget risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/cache.admin.tsx` around lines 88 - 92, The primary-instance guard is correctly placed: ensure getInstanceInfo() is called and invariantResponse(currentIsPrimary, `Bulk delete must run on the primary instance (${primaryInstance})`) remains before any cache.delete calls so that cache.delete uses the primary branch (preparedDelete.run) and avoids cross-replica fire-and-forget behavior; confirm the code paths using preparedDelete.run and cache.delete are unchanged and no deletion loop executes before this check.
🧹 Nitpick comments (1)
app/routes/cache.admin.tsx (1)
81-88: RedundantgetInstanceInfo()call — merge into one.
getInstanceInfo()is awaited on Line 81 and again on Line 88. All needed fields can be destructured from a single call.♻️ Proposed refactor
- const { currentInstance } = await getInstanceInfo() const instance = formData.get('instance') ?? currentInstance invariantResponse(typeof instance === 'string', 'instance must be a string') await ensureInstance(instance) const intent = formData.get('intent') if (intent === deleteAllMatchingIntent) { - const { currentIsPrimary, primaryInstance } = await getInstanceInfo() + const { currentInstance, currentIsPrimary, primaryInstance } = await getInstanceInfo() + const instance = formData.get('instance') ?? currentInstance + invariantResponse(typeof instance === 'string', 'instance must be a string') + await ensureInstance(instance) + + const intent = formData.get('intent') + if (intent === deleteAllMatchingIntent) { invariantResponse( currentIsPrimary, `Bulk delete must run on the primary instance (${primaryInstance})`, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/routes/cache.admin.tsx` around lines 81 - 88, The code calls getInstanceInfo() twice; merge into a single call and destructure all needed fields (currentInstance, currentIsPrimary, primaryInstance) once at the top, then use those variables for the instance default and the intent check; remove the second await of getInstanceInfo() and ensure invariantResponse/ensureInstance still use the single destructured currentInstance and instance variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/routes/cache.admin.tsx`:
- Around line 38-59: sanitizeCacheKeysLimit and getMatchingCacheKeys are correct
and the previous sanitization bug is resolved; no code changes needed—keep
sanitizeCacheKeysLimit (handles non-finite, zero, negative, non-integer, and
above-max values) and ensure getMatchingCacheKeys continues to call
sanitizeCacheKeysLimit before invoking searchCacheKeys or getAllCacheKeys.
- Around line 88-92: The primary-instance guard is correctly placed: ensure
getInstanceInfo() is called and invariantResponse(currentIsPrimary, `Bulk delete
must run on the primary instance (${primaryInstance})`) remains before any
cache.delete calls so that cache.delete uses the primary branch
(preparedDelete.run) and avoids cross-replica fire-and-forget behavior; confirm
the code paths using preparedDelete.run and cache.delete are unchanged and no
deletion loop executes before this check.
---
Nitpick comments:
In `@app/routes/cache.admin.tsx`:
- Around line 81-88: The code calls getInstanceInfo() twice; merge into a single
call and destructure all needed fields (currentInstance, currentIsPrimary,
primaryInstance) once at the top, then use those variables for the instance
default and the intent check; remove the second await of getInstanceInfo() and
ensure invariantResponse/ensureInstance still use the single destructured
currentInstance and instance variable.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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
2 participants
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.
Add a "Delete all matching cache values" button to
/cache/adminto enable bulk deletion of filtered cache entries.Note
Medium Risk
Adds a destructive bulk-delete path over caches; mistakes in filtering/limits or primary-instance enforcement could lead to unintended cache invalidation across the system.
Overview
Adds a bulk deletion action to
/cache/adminthat removes all currently filtered cache keys from both the SQLite-backed cache and in-memory LRU cache.Introduces input hardening around the
limitparam (sanitized and capped at10_000) and enforces that bulk deletes can only run on the primary LiteFS instance. The UI now shows a conditional “delete all N matching cache values” button with a double-confirmation flow and in-progress status.Written by Cursor Bugbot for commit 869f83a. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements