feat(admin): Invalidate Cache #3528
Conversation
|
""" WalkthroughThis change introduces a new cache invalidation feature for administrators. It adds backend support for batch deletion of cache keys, including new asynchronous Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds cache invalidation functionality to the admin console, allowing administrators to clear system caches through a new UI panel and corresponding backend endpoints.
Key points:
- New
/api/v1/admin/invalidate-cacheendpoint lacks authentication middleware present in other admin routes, potentially allowing unauthorized cache invalidation - Redis SCAN operation in
keystore.tsuses a fixed COUNT of 1000 which may need tuning for production loads - The
deleteItemsimplementation inmemory.tsuses RE2 for safe regex handling but should validate pattern input - CachingPanel UI component lacks proper loading state handling during cache invalidation operations
- Documentation is missing to explain how customers will discover and properly use this new cache invalidation feature
16 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx
Outdated
Show resolved
Hide resolved
![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 (3)
backend/src/keystore/memory.ts (1)
24-34: Well-implemented pattern-based deletion methodThe
deleteItemsmethod effectively implements pattern-based deletion by:
- Converting wildcard patterns to proper regex
- Scanning all keys for matches
- Efficiently deleting matching keys
- Returning an accurate count of deleted items
Consider adding basic error handling to catch and log any unexpected issues that might occur during pattern matching or deletion:
deleteItems: async (pattern) => { - const regex = new RE2(pattern.replace(/\*/g, ".*")); - let deletedCount = 0; - for (const key of Object.keys(store)) { - if (regex.test(key)) { - delete store[key]; - deletedCount += 1; - } - } - return deletedCount; + try { + const regex = new RE2(pattern.replace(/\*/g, ".*")); + let deletedCount = 0; + for (const key of Object.keys(store)) { + if (regex.test(key)) { + delete store[key]; + deletedCount += 1; + } + } + return deletedCount; + } catch (error) { + console.error("Error in deleteItems:", error); + return 0; // Return 0 deleted items in case of error + } },backend/src/services/super-admin/super-admin-service.ts (1)
574-576: Consider adding error handling to the cache invalidation function.The
invalidateCacheimplementation is simple and functional, but lacks error handling. If the keyStore operation fails, the error will propagate up without any specific context about the cache invalidation operation.Consider adding try/catch to handle potential errors with more context:
const invalidateCache = async (type: CacheType) => { - if (type === CacheType.ALL || type === CacheType.SECRETS) await keyStore.deleteItems("secret-manager:*"); + try { + if (type === CacheType.ALL || type === CacheType.SECRETS) await keyStore.deleteItems("secret-manager:*"); + } catch (error) { + throw new Error(`Failed to invalidate ${type} cache: ${error.message}`); + } };frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx (1)
10-42: Cache invalidation component has good structure but could use improved error handling.The component has a clean structure with proper state management and API integration. The
handleInvalidateCacheSubmitfunction correctly:
- Shows loading state during API call
- Displays appropriate success/error notifications
- Closes the modal after completion
However, the error handling could be more specific about what went wrong.
Consider enhancing the error notification to provide more context:
} catch (err) { console.error(err); createNotification({ - text: `Failed to purge ${type} cache`, + text: `Failed to purge ${type} cache: ${err instanceof Error ? err.message : 'Unknown error'}`, type: "error" }); + setIsLoading(false); }Also, don't forget to set
isLoadingto false in the catch block to ensure the button doesn't remain in the loading state after an error occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/e2e-test/mocks/keystore.ts(2 hunks)backend/src/keystore/keystore.ts(3 hunks)backend/src/keystore/memory.ts(2 hunks)backend/src/server/config/rateLimiter.ts(1 hunks)backend/src/server/routes/v1/admin-router.ts(2 hunks)backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts(1 hunks)backend/src/services/super-admin/super-admin-service.ts(4 hunks)backend/src/services/super-admin/super-admin-types.ts(1 hunks)backend/src/services/telemetry/telemetry-types.ts(3 hunks)frontend/src/hooks/api/admin/index.ts(1 hunks)frontend/src/hooks/api/admin/mutation.ts(2 hunks)frontend/src/hooks/api/admin/queries.ts(1 hunks)frontend/src/hooks/api/admin/types.ts(1 hunks)frontend/src/layouts/ProjectLayout/ProjectLayout.tsx(1 hunks)frontend/src/pages/admin/OverviewPage/OverviewPage.tsx(4 hunks)frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
frontend/src/pages/admin/OverviewPage/OverviewPage.tsx (1)
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx (1)
CachingPanel(10-100)
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx (3)
frontend/src/hooks/api/admin/index.ts (1)
useInvalidateCache(6-6)frontend/src/hooks/api/admin/mutation.ts (1)
useInvalidateCache(152-162)frontend/src/components/v2/DeleteActionModal/DeleteActionModal.tsx (1)
DeleteActionModal(25-118)
frontend/src/hooks/api/admin/mutation.ts (3)
frontend/src/hooks/api/admin/index.ts (1)
useInvalidateCache(6-6)frontend/src/hooks/api/admin/types.ts (1)
TInvalidateCacheDTO(83-85)frontend/src/hooks/api/admin/queries.ts (1)
adminQueryKeys(20-28)
backend/src/services/super-admin/super-admin-service.ts (1)
backend/src/keystore/keystore.ts (1)
TKeyStoreFactory(15-15)
backend/src/server/routes/v1/admin-router.ts (1)
backend/src/server/config/rateLimiter.ts (1)
invalidateCacheLimit(104-109)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check TS and Lint
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Run integration test
- GitHub Check: Check API Changes
🔇 Additional comments (31)
frontend/src/layouts/ProjectLayout/ProjectLayout.tsx (1)
18-18: Import order change does not affect functionality
TheuseProjectPermissionhook has been reordered within the@app/contextimports; this is a non-functional, stylistic adjustment.backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts (1)
10-10: Trivial import reordering — no action required
TheapplyJitterimport has been moved up to join the other internal@app/libimports. This is purely organizational and aligns with the existing grouping conventions.backend/src/services/super-admin/super-admin-types.ts (1)
48-51: Well-designed enum for cache invalidation typesThe
CacheTypeenum is a good addition that clearly defines the scope of cache invalidation operations. Having distinct values for invalidating all caches (ALL) versus just secrets-related caches (SECRETS) provides good flexibility for admins.frontend/src/hooks/api/admin/index.ts (1)
6-6: Export looks goodThe addition of
useInvalidateCacheto the exports properly exposes the new cache invalidation hook for use in admin components.frontend/src/hooks/api/admin/queries.ts (1)
26-27: Query key addition follows consistent patternThe new
getInvalidateCachequery key function follows the established pattern and naming convention of other query keys in this file. This will allow proper cache management when invalidation occurs.backend/src/keystore/memory.ts (1)
1-2: Good choice of regex libraryUsing RE2 is a strong choice for pattern matching as it provides linear-time matching performance, which is important when potentially scanning a large number of cache keys.
frontend/src/hooks/api/admin/types.ts (2)
78-81: Clean enum definition for cache types.The
CacheTypeenum is well-defined with clear values for distinguishing between invalidating all caches versus only secrets-related caches. This granularity allows administrators to target specific parts of the cache system when needed.
83-85: Good DTO type definition.The
TInvalidateCacheDTOtype creates a clear contract for the cache invalidation API payload, making the interface between frontend and backend explicit.backend/src/server/config/rateLimiter.ts (1)
104-109: Appropriate rate limiting for cache invalidation.Setting a strict rate limit of 1 request per minute is appropriate for cache invalidation operations, which are typically infrequent administrative actions that could impact system performance if executed too frequently.
backend/e2e-test/mocks/keystore.ts (2)
3-3: Import added for pattern matching support.RE2 is a good choice for implementing pattern-based key deletion in the mock keystore, providing consistent behavior with the production implementation.
22-32: Well-implemented pattern-based deletion for mock keystore.The implementation correctly:
- Converts wildcard patterns to proper regex patterns
- Tests each key against the pattern
- Tracks and returns the count of deleted items
This matches the expected behavior of the Redis-backed implementation, ensuring consistent testing.
backend/src/server/routes/v1/admin-router.ts (3)
7-8: Import dependencies for the new cache invalidation route.Good addition of the necessary imports for rate limiting and telemetry functionality.
14-14: Import CacheType enum for request validation.Reusing the
CacheTypeenum from the super-admin types ensures consistency between different parts of the application.
557-565: Good telemetry implementation for cache invalidation.The code correctly sends a telemetry event after cache invalidation, which is important for tracking admin operations and debugging purposes.
frontend/src/pages/admin/OverviewPage/OverviewPage.tsx (4)
34-34: Import successfully added for CachingPanel component.The import statement is correctly placed alongside other component imports from the same directory.
46-47: Enum values properly extended.The TabSections enum is correctly extended with the Kmip and Caching values, maintaining the established naming pattern. This provides the foundation for the new caching tab in the admin interface.
169-169: Tab element properly added to the TabList.The Tab element for the Caching section is correctly positioned at the end of the tab list, with the proper value reference to TabSections.Caching.
414-416: TabPanel component successfully integrated.The TabPanel for the CachingPanel is properly implemented with the correct value reference, following the established pattern in this file. The component is correctly rendered without unnecessary props.
frontend/src/hooks/api/admin/mutation.ts (2)
12-12: Import added correctly for the DTO type.The TInvalidateCacheDTO import has been properly added to the imports list, maintaining alphabetical order.
152-162: Cache invalidation mutation hook implementation looks good.The
useInvalidateCachehook follows the established pattern for API mutations in this codebase:
- Uses React Query's useMutation with proper typing
- Makes a POST request to the correct admin endpoint
- Invalidates the appropriate query cache on success
This enables the UI to trigger cache invalidation and react appropriately to the operation's outcome.
backend/src/services/super-admin/super-admin-service.ts (3)
29-29: CacheType import correctly added.The import for the CacheType enum is properly added to the existing import group from the same file.
49-49: KeyStore dependency type correctly extended.The keyStore dependency now includes the
deleteItemsmethod in its type definition, ensuring type safety when using this method in the service.
592-592: Service method properly exported in the returned interface.The new
invalidateCachemethod is correctly added to the service interface, making it available to consumers of this service.backend/src/services/telemetry/telemetry-types.ts (3)
24-25: New telemetry event type added correctly.The new
InvalidateCacheevent type is properly added to the PostHogEventTypes enum, following the established pattern of using descriptive string values for events.
207-212: Telemetry event type definition follows established patterns.The new
TInvalidateCacheEventtype is correctly structured, with an event field referencing the enum value and a properties object containing the optional userAgent field, consistent with other telemetry event types.
232-232: Event properly added to union type.The new event type is correctly added to the TPostHogEvent union type, ensuring it can be properly processed by the telemetry system.
backend/src/keystore/keystore.ts (2)
60-60: Good choice for batch size constant.Defining
DELETION_BATCH_SIZEas a named constant makes the implementation more maintainable and easier to tune for performance optimization.
138-138: Successfully exposed the new deleteItems method.The new
deleteItemsmethod is properly exposed in the interface, maintaining consistency with the existing API style.frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx (3)
43-66: Good UI implementation with appropriate permission checks.The UI for the Secrets Cache invalidation is well designed with:
- Clear explanatory text describing the cache purpose
- Proper permission checks to disable the button for non-admin users
- Loading state handling to prevent multiple submissions
This implementation follows good security practices by restricting administrative actions to users with the appropriate role.
67-89: Future-proofing with commented code for all cache types.The commented section for invalidating all cache types is a good way to prepare for future functionality without releasing incomplete features. When this functionality is needed, it will be simple to uncomment and enable.
90-98: Good use of confirmation modal for destructive actions.Using the
DeleteActionModalcomponent for confirmation before invalidating the cache is an excellent practice for potentially destructive operations. The requirement to type "confirm" prevents accidental triggering of cache invalidation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/e2e-test/mocks/keystore.ts (1)
40-40: Consider a more efficient jitter calculation.Same suggestion as in the memory.ts implementation for consistency.
-await delayMs(Math.max(0, delay + Math.floor((Math.random() * 2 - 1) * jitter))); +await delayMs(Math.max(0, delay + Math.floor(Math.random() * jitter * 2) - jitter));backend/src/keystore/keystore.ts (1)
107-107: Consider a more efficient jitter calculation.Same suggestion as in the other implementations for consistency.
-await delayMs(Math.max(0, delay + Math.floor((Math.random() * 2 - 1) * jitter))); +await delayMs(Math.max(0, delay + Math.floor(Math.random() * jitter * 2) - jitter));
🧹 Nitpick comments (2)
backend/src/keystore/memory.ts (1)
41-41: Consider a more efficient jitter calculation.The current implementation:
delay + Math.floor((Math.random() * 2 - 1) * jitter)works correctly for applying jitter, but could be simplified.-await delayMs(Math.max(0, delay + Math.floor((Math.random() * 2 - 1) * jitter))); +await delayMs(Math.max(0, delay + Math.floor(Math.random() * jitter * 2) - jitter));backend/src/keystore/keystore.ts (1)
86-112: Consider adding a basic memory usage safeguard.When scanning large Redis databases with many keys, the array returned by SCAN could potentially be very large and consume significant memory.
const deleteItems = async ({ pattern, batchSize = 500, delay = 1500, jitter = 200 }: TDeleteItems) => { let cursor = "0"; let totalDeleted = 0; + const MAX_KEYS_PER_SCAN = 1000; // Explicit constant for clarity do { // Await in loop is needed so that Redis is not overwhelmed // eslint-disable-next-line no-await-in-loop - const [nextCursor, keys] = await redis.scan(cursor, "MATCH", pattern, "COUNT", 1000); // Count should be 1000 - 5000 for prod loads + const [nextCursor, keys] = await redis.scan(cursor, "MATCH", pattern, "COUNT", MAX_KEYS_PER_SCAN); cursor = nextCursor; + // Safety check in case we get an unexpectedly large number of keys + if (keys.length > MAX_KEYS_PER_SCAN * 10) { + console.warn(`Unusually large key count (${keys.length}) returned from Redis SCAN. Pattern: ${pattern}`); + } + for (let i = 0; i < keys.length; i += batchSize) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/e2e-test/mocks/keystore.ts(2 hunks)backend/src/keystore/keystore.ts(4 hunks)backend/src/keystore/memory.ts(2 hunks)backend/src/lib/delay/index.ts(1 hunks)backend/src/server/routes/v1/admin-router.ts(2 hunks)backend/src/services/super-admin/super-admin-service.ts(4 hunks)backend/src/services/telemetry/telemetry-types.ts(3 hunks)frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx
- backend/src/server/routes/v1/admin-router.ts
- backend/src/services/telemetry/telemetry-types.ts
- backend/src/services/super-admin/super-admin-service.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/e2e-test/mocks/keystore.ts (1)
backend/src/lib/delay/index.ts (1)
delay(1-4)
backend/src/keystore/memory.ts (1)
backend/src/lib/delay/index.ts (1)
delay(1-4)
backend/src/keystore/keystore.ts (1)
backend/src/lib/delay/index.ts (1)
delay(1-4)
🔇 Additional comments (5)
backend/src/lib/delay/index.ts (1)
1-4: Clean utility implementation for asynchronous delays.This utility function is well-implemented - it provides a simple, type-safe way to create asynchronous delays that will be used in the batch processing of the cache invalidation feature.
backend/src/keystore/memory.ts (1)
25-45: Implementation correctly handles pattern-based key deletion with batching.The
deleteItemsmethod is well implemented with proper regex escaping to prevent injection vulnerabilities. I like the batch processing approach with controlled delays to prevent system overload.backend/e2e-test/mocks/keystore.ts (1)
24-44: Consistent implementation with other key store variants.The mock implementation correctly mirrors the functionality in the memory and Redis key stores, ensuring consistent behavior across different environments. The regex pattern handling is properly secured against injection.
backend/src/keystore/keystore.ts (2)
52-57: Good type definition for deleteItems parameters.The
TDeleteItemstype properly captures the parameters needed for the cache invalidation operation with appropriate optional flags.
86-112: Efficient implementation of Redis key pattern deletion.The implementation uses Redis best practices:
- SCAN instead of KEYS to avoid blocking Redis
- Batched processing with controlled delays
- UNLINK for non-blocking deletion
- Pipeline for efficient command execution
While I note that error handling is deferred to higher levels as mentioned in previous comments, the core implementation is solid.
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx (2)
44-71: Improve error handling in the cache invalidation submission.The function correctly handles the basic flow, but the error handling could be improved to provide more specific error messages.
Consider capturing and displaying more specific error information:
try { await invalidateCache({ type }); // ...rest of the success flow } catch (err) { console.error(err); createNotification({ - text: `Failed to invalidate ${type} cache`, + text: `Failed to invalidate ${type} cache: ${err instanceof Error ? err.message : 'Unknown error'}`, type: "error" }); }
73-97: Polling mechanism looks robust but could use extraction.The polling implementation correctly handles cleaning up intervals and timeouts, but the logic is complex and might be clearer if extracted to a custom hook.
Consider extracting this polling logic to a reusable hook like
usePollingto simplify the component and make the functionality more reusable.backend/src/services/super-admin/invalidate-cache-queue.ts (6)
15-27: Consider adding retry mechanism for queue operations.The
startInvalidatefunction queues the job but doesn't handle potential queue failures. Since cache invalidation is likely an important operation, consider adding retry logic or at least error handling.export const invalidateCacheQueueFactory = ({ queueService, keyStore }: TInvalidateCacheQueueFactoryDep) => { const startInvalidate = async (dto: { data: { type: CacheType; }; }) => { - await queueService.queue(QueueName.InvalidateCache, QueueJobs.InvalidateCache, dto, { - removeOnComplete: true, - removeOnFail: true, - jobId: "invalidate-cache" - }); + try { + await queueService.queue(QueueName.InvalidateCache, QueueJobs.InvalidateCache, dto, { + removeOnComplete: true, + removeOnFail: true, + jobId: "invalidate-cache" + }); + } catch (error) { + logger.error(error, "Failed to enqueue cache invalidation job"); + throw new Error("Failed to start cache invalidation"); + } };
34-34: Extract magic numbers to named constants.The 30-minute expiry time (1800 seconds) is hardcoded. Consider extracting this to a named constant at the top of the file for better maintainability.
+ const CACHE_INVALIDATION_EXPIRY_SECONDS = 1800; // 30 minutes + export const invalidateCacheQueueFactory = ({ queueService, keyStore }: TInvalidateCacheQueueFactoryDep) => { // ...existing code... - await keyStore.setItemWithExpiry("invalidating-cache", 1800, "true"); // 30 minutes max (in case the job somehow silently fails) + await keyStore.setItemWithExpiry("invalidating-cache", CACHE_INVALIDATION_EXPIRY_SECONDS, "true"); // max time in case the job somehow silently fails
28-44: Add progress reporting mechanism for long-running operations.For large caches, invalidation might take a long time. Consider enhancing the implementation to provide progress updates, especially since you already have an "invalidating-cache" flag.
You could store additional metadata like start time, progress percentage, or the total number of keys being processed to give administrators better visibility into the operation's status.
queueService.start(QueueName.InvalidateCache, async (job) => { try { const { data: { type } } = job.data; - await keyStore.setItemWithExpiry("invalidating-cache", 1800, "true"); // 30 minutes max (in case the job somehow silently fails) + // Store more detailed status information + const status = { + inProgress: true, + startedAt: new Date().toISOString(), + type + }; + await keyStore.setItemWithExpiry("invalidating-cache", 1800, JSON.stringify(status)); if (type === CacheType.ALL || type === CacheType.SECRETS) await keyStore.deleteItems({ pattern: "secret-manager:*" }); await keyStore.deleteItem("invalidating-cache"); } catch (err) { logger.error(err, "Failed to invalidate cache"); await keyStore.deleteItem("invalidating-cache"); } });
36-37: Extract cache patterns to constants for better maintainability.The pattern "secret-manager:*" is hardcoded in the implementation. Consider extracting this to a named constant or a configuration object that maps cache types to patterns.
+ const CACHE_PATTERNS = { + [CacheType.SECRETS]: "secret-manager:*", + // Add more patterns for other cache types as needed + }; + export const invalidateCacheQueueFactory = ({ queueService, keyStore }: TInvalidateCacheQueueFactoryDep) => { // ...existing code... if (type === CacheType.ALL || type === CacheType.SECRETS) - await keyStore.deleteItems({ pattern: "secret-manager:*" }); + await keyStore.deleteItems({ pattern: CACHE_PATTERNS[CacheType.SECRETS] });
36-37: Consider implementing more granular cache type handling.The current implementation has a simple condition for
CacheType.ALL || CacheType.SECRETS. As the system grows, you might need to handle more cache types with different patterns. Consider a more extensible approach.A more scalable approach would be to define a mapping of cache types to the patterns they should invalidate, then process each pattern based on the requested type:
const CACHE_TYPE_PATTERNS: Record<CacheType, string[]> = { [CacheType.ALL]: ["secret-manager:*", /* other patterns */], [CacheType.SECRETS]: ["secret-manager:*"], // Add more cache types as needed }; // Then in your queue processor: const patternsToInvalidate = CACHE_TYPE_PATTERNS[type] || []; for (const pattern of patternsToInvalidate) { await keyStore.deleteItems({ pattern }); }This would make it easier to add new cache types and their associated patterns in the future.
1-50: Add JSDoc documentation to the module and exported functions.This module implements an important feature but lacks documentation. Consider adding JSDoc comments to explain the purpose, behavior, and usage of the module and its methods.
+ /** + * Factory for creating a cache invalidation queue service. + * This service handles asynchronous cache invalidation operations via a queue. + * + * @param {TInvalidateCacheQueueFactoryDep} dependencies - Required dependencies + * @returns {TInvalidateCacheQueueFactory} Cache invalidation queue service + */ export const invalidateCacheQueueFactory = ({ queueService, keyStore }: TInvalidateCacheQueueFactoryDep) => { + /** + * Starts the cache invalidation process by enqueueing a job. + * Only one job with ID "invalidate-cache" can exist at a time. + * + * @param {Object} dto - Data transfer object + * @param {Object} dto.data - Job data + * @param {CacheType} dto.data.type - Type of cache to invalidate + * @returns {Promise<void>} + */ const startInvalidate = async (dto: { data: { type: CacheType; }; }) => { // ... }; // ... return { startInvalidate }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
backend/e2e-test/mocks/keystore.ts(2 hunks)backend/src/@types/fastify.d.ts(1 hunks)backend/src/db/migrations/20250502201443_invalidate-cache-status-superadmin.ts(1 hunks)backend/src/db/schemas/projects.ts(1 hunks)backend/src/db/schemas/super-admin.ts(1 hunks)backend/src/keystore/keystore.ts(5 hunks)backend/src/keystore/memory.ts(2 hunks)backend/src/queue/queue-service.ts(4 hunks)backend/src/server/routes/index.ts(3 hunks)backend/src/server/routes/v1/admin-router.ts(2 hunks)backend/src/server/routes/v1/index.ts(1 hunks)backend/src/services/super-admin/invalidate-cache-queue.ts(1 hunks)backend/src/services/super-admin/super-admin-service.ts(6 hunks)backend/src/services/telemetry/telemetry-types.ts(3 hunks)frontend/src/hooks/api/admin/index.ts(1 hunks)frontend/src/hooks/api/admin/mutation.ts(2 hunks)frontend/src/hooks/api/admin/queries.ts(3 hunks)frontend/src/hooks/api/admin/types.ts(2 hunks)frontend/src/pages/admin/OverviewPage/OverviewPage.tsx(4 hunks)frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- backend/src/@types/fastify.d.ts
- backend/src/server/routes/v1/index.ts
- backend/src/db/schemas/super-admin.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/src/hooks/api/admin/index.ts
- frontend/src/hooks/api/admin/mutation.ts
- backend/e2e-test/mocks/keystore.ts
- frontend/src/hooks/api/admin/types.ts
- backend/src/server/routes/v1/admin-router.ts
- frontend/src/pages/admin/OverviewPage/OverviewPage.tsx
- backend/src/keystore/memory.ts
- backend/src/services/super-admin/super-admin-service.ts
- backend/src/keystore/keystore.ts
- frontend/src/hooks/api/admin/queries.ts
- backend/src/services/telemetry/telemetry-types.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx (4)
frontend/src/hooks/api/admin/index.ts (1)
useInvalidateCache(6-6)frontend/src/hooks/api/admin/mutation.ts (1)
useInvalidateCache(131-141)frontend/src/hooks/api/admin/queries.ts (1)
useGetInvalidatingCacheStatus(125-136)frontend/src/components/v2/DeleteActionModal/DeleteActionModal.tsx (1)
DeleteActionModal(25-118)
backend/src/server/routes/index.ts (1)
backend/src/services/super-admin/invalidate-cache-queue.ts (1)
invalidateCacheQueueFactory(15-49)
backend/src/services/super-admin/invalidate-cache-queue.ts (2)
backend/src/queue/queue-service.ts (1)
TQueueServiceFactory(250-250)backend/src/keystore/keystore.ts (1)
TKeyStoreFactory(17-17)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
- GitHub Check: Check Frontend Type and Lint check
- GitHub Check: Check API Changes
🔇 Additional comments (15)
backend/src/db/schemas/projects.ts (1)
30-30:❓ Verification inconclusive
Security concern: Default delete protection has been disabled
The default value for
hasDeleteProtectionhas been changed fromtruetofalse. This means that new projects will no longer have delete protection enabled by default, which could increase the risk of accidental project deletion.While this change may provide more flexibility for administrators, it represents a significant change to the default security posture.
- Is this change intentional and aligned with the cache invalidation feature being implemented?
- Given the file header states "DO NOT EDIT", was this change made by updating the database schema and regenerating this file?
- How will existing projects be affected by this change?
🏁 Script executed:
#!/bin/bash # Check if this schema file is referenced in the cache invalidation implementation echo "Checking for references to projects schema in cache invalidation code..." rg -l "ProjectsSchema|hasDeleteProtection" --type ts | grep -i cache # Check for any migration files that might be related to this change echo -e "\nChecking for related migration files..." fd -e sql -e js -e ts "migration|schema" | grep -i "project\|delete\|protection" | sort -r | head -n 10Length of output: 426
Verify Default Delete Protection Change
It looks like the default for
hasDeleteProtectionwas flipped fromtruetofalsewithout any supporting migrations or references in the cache-invalidation code. Please verify:
- This change is intentional and tied to the new cache-invalidation feature.
- The
backend/src/db/schemas/projects.tsfile was regenerated from your DB schema (not manually edited).- You have a migration strategy for existing projects (how they’ll inherit or override delete protection).
File: backend/src/db/schemas/projects.ts (line 30)
- hasDeleteProtection: z.boolean().default(true).nullable().optional() + hasDeleteProtection: z.boolean().default(false).nullable().optional()backend/src/server/routes/index.ts (3)
244-244: Good import organization.This new import for
invalidateCacheQueueFactoryis properly placed within the existing import structure.
614-617: Factory instantiation looks good.The invalidateCacheQueue factory is properly instantiated with the required dependencies (keyStore and queueService).
730-731: Proper dependency injection.The invalidateCacheQueue is correctly added as a dependency to the superAdminService, maintaining the existing pattern for service composition.
backend/src/db/migrations/20250502201443_invalidate-cache-status-superadmin.ts (1)
1-21: Well-structured migration for cache invalidation status.The migration follows best practices with proper up/down functions and defensive checks before applying/reverting changes. The boolean column with a default value of false is appropriate for tracking invalidation status.
backend/src/queue/queue-service.ts (4)
28-28: Good import addition.The CacheType import is properly added to support the new InvalidateCache queue type.
53-54: Queue name addition follows convention.The addition of InvalidateCache to the QueueName enum maintains the existing pattern.
86-87: Queue job definition follows convention.The addition of InvalidateCache to the QueueJobs enum maintains the existing pattern.
240-247: Well-defined queue job type.The queue job type for InvalidateCache is properly defined with appropriate payload structure, matching the pattern of other queue types.
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx (5)
13-26: Good component initialization with proper hooks.The component correctly uses hooks for API calls, permissions, and UI state management. The initial state setting is appropriate, and I appreciate that you've included reference handling for the polling and timeout mechanisms.
99-115: Clever approach to ignore initial state.Using a ref to ignore the initial effect execution is a good approach. The cleanup on unmount is properly handled.
119-149: UI for secrets cache section looks good.The UI clearly displays the current state with appropriate badge for invalidation in progress. The disabling of the button based on permission and state is well implemented.
151-172: Good preparation for future cache types.The commented-out code for "All Cache" indicates forward thinking while keeping the current UI focused on implemented functionality.
174-182: Confirm modal implementation is solid.Using a DeleteActionModal for confirmation is appropriate for this destructive action. The title and parameters are well configured.
backend/src/services/super-admin/invalidate-cache-queue.ts (1)
1-13: Good job on the clean dependency injection setup.The module clearly defines its dependencies and return types, making it testable and maintainable.
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/admin/OverviewPage/components/CachingPanel.tsx
Outdated
Show resolved
Hide resolved
Description 📣
Added
deleteItemsto keyStore, added an admin endpoint + service func, and added some UI to the server admin console.Type ✨
Summary by CodeRabbit