Fix: race conditions for deletions #2753
Conversation
Console (appwrite/console)Project ID: Tip Realtime gives you live updates for users, storage, functions, and databases |
WalkthroughThe PR adds a new Submit enum member Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks✅ Passed checks (3 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.svelte (1)
40-52: Store mutation inside the batch callback could cause false failures.If
subscribers[subscriberId]is undefined (e.g., due to stale state), accessingtargeton line 49 will throw, causingbatchDeleteto mark this item as failed even though the API deletion succeeded. Consider moving the store cleanup outside the callback or adding a guard.🔎 Proposed fix
async function handleDelete(batchDelete: DeleteOperation): Promise<DeleteOperationState> { const result = await batchDelete(async (subscriberId) => { await sdk .forProject(page.params.region, page.params.project) .messaging.deleteSubscriber({ topicId: page.params.topic, subscriberId }); - - const { target } = subscribers[subscriberId]; - const { [target.$id]: _, ...rest } = $targetsById; - $targetsById = rest; }); + // Clean up targetsById for successfully deleted subscribers + for (const subscriberId of result.deleted) { + const subscriber = subscribers[subscriberId]; + if (subscriber?.target?.$id) { + const { [subscriber.target.$id]: _, ...rest } = $targetsById; + $targetsById = rest; + } + } + try {
🧹 Nitpick comments (7)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.svelte (1)
34-53: Consider simplifying error handling.The
tryblock wraps only analytics calls that checkresult.errorconditionally. UnlesstrackErrorortrackEventcan throw exceptions, thistry-catchis unnecessary. You can simplify to:async function deleteLogs(batchDelete: DeleteOperation): Promise<DeleteOperationState> { const result = await batchDelete((logId) => sdk.forProject(page.params.region, page.params.project).sites.deleteLog({ siteId: page.params.site, logId }) ); - try { - if (result.error) { - trackError(result.error, Submit.LogDelete); - } else { - trackEvent(Submit.LogDelete, { total: result.deleted.length }); - } - } finally { - await invalidate(Dependencies.EXECUTIONS); - } + if (result.error) { + trackError(result.error, Submit.LogDelete); + } else { + trackEvent(Submit.LogDelete, { total: result.deleted.length }); + } + await invalidate(Dependencies.EXECUTIONS); return result; }That said, the current implementation is correct and ensures cleanup via
finally.src/routes/(console)/project-[region]-[project]/auth/user-[user]/targets/table.svelte (1)
27-45: Consider simplifying error handling.Same observation as in other files: the
tryblock is unnecessary iftrackErrorandtrackEventdon't throw. Consider:async function handleDelete(batchDelete: DeleteOperation): Promise<DeleteOperationState> { const result = await batchDelete((id) => sdk .forProject(page.params.region, page.params.project) .users.deleteTarget({ userId: page.params.user, targetId: id }) ); - try { - if (result.error) { - trackError(result.error, Submit.UserTargetDelete); - } else { - trackEvent(Submit.UserTargetDelete, { total: result.deleted.length }); - } - } finally { - await invalidate(Dependencies.USER_TARGETS); - } + if (result.error) { + trackError(result.error, Submit.UserTargetDelete); + } else { + trackEvent(Submit.UserTargetDelete, { total: result.deleted.length }); + } + await invalidate(Dependencies.USER_TARGETS); return result; }src/routes/(console)/project-[region]-[project]/storage/bucket-[bucket]/+page.svelte (1)
69-88: Consider simplifying error handling.The
try-finallyblock can be simplified since only conditional checks are performed:async function handleBulkDelete(batchDelete: DeleteOperation): Promise<DeleteOperationState> { const result = await batchDelete((fileId) => sdk.forProject(page.params.region, page.params.project).storage.deleteFile({ bucketId: page.params.bucket, fileId }) ); - try { - if (result.error) { - trackError(result.error, Submit.FileDelete); - } else { - trackEvent(Submit.FileDelete, { total: result.deleted.length }); - } - } finally { - await invalidate(Dependencies.FILES); - } + if (result.error) { + trackError(result.error, Submit.FileDelete); + } else { + trackEvent(result.FileDelete, { total: result.deleted.length }); + } + await invalidate(Dependencies.FILES); return result; }src/routes/(console)/project-[region]-[project]/auth/user-[user]/memberships/+page.svelte (1)
27-52: Consider simplifying error handling.The
try-finallywrapping analytics can be simplified:- try { - if (result.error) { - trackError(result.error, Submit.MembershipUpdate); - } else { - trackEvent(Submit.MembershipUpdate, { total: result.deleted.length }); - } - } finally { - await invalidate(Dependencies.MEMBERSHIPS); - } + if (result.error) { + trackError(result.error, Submit.MembershipUpdate); + } else { + trackEvent(Submit.MembershipUpdate, { total: result.deleted.length }); + } + await invalidate(Dependencies.MEMBERSHIPS);src/routes/(console)/project-[region]-[project]/auth/user-[user]/identities/table.svelte (1)
29-47: Consider simplifying error handling.The
try-finallyblock can be simplified:async function handleDelete(batchDelete: DeleteOperation): Promise<DeleteOperationState> { const result = await batchDelete((id) => sdk .forProject(page.params.region, page.params.project) .users.deleteIdentity({ identityId: id }) ); - try { - if (result.error) { - trackError(result.error, Submit.UserIdentityDelete); - } else { - trackEvent(Submit.UserIdentityDelete, { total: result.deleted.length }); - } - } finally { - await invalidate(Dependencies.USER_IDENTITIES); - } + if (result.error) { + trackError(result.error, Submit.UserIdentityDelete); + } else { + trackEvent(Submit.UserIdentityDelete, { total: result.deleted.length }); + } + await invalidate(Dependencies.USER_IDENTITIES); return result; }src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte (1)
45-67: Consider simplifying the try/finally block.The
try/finallywrapping analytics calls (lines 53-63) seems unnecessary sincetrackErrorandtrackEventare synchronous fire-and-forget calls that shouldn't throw. The invalidation could be called directly after the conditional tracking.That said, the batch deletion logic itself is correct and properly handles error tracking via
result.errorand success tracking viaresult.deleted.length.🔎 Proposed simplification
const result = await batchDelete((deploymentId) => sdk.forProject(page.params.region, page.params.project).sites.deleteDeployment({ siteId: page.params.site, deploymentId }) ); - try { - if (result.error) { - trackError(result.error, Submit.DeploymentDelete); - } else { - trackEvent(Submit.DeploymentDelete, { total: result.deleted.length }); - } - } finally { - await Promise.all([ - invalidate(Dependencies.DEPLOYMENTS), - invalidate(Dependencies.SITE) - ]); + if (result.error) { + trackError(result.error, Submit.DeploymentDelete); + } else { + trackEvent(Submit.DeploymentDelete, { total: result.deleted.length }); } + + await Promise.all([ + invalidate(Dependencies.DEPLOYMENTS), + invalidate(Dependencies.SITE) + ]); return result;src/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/table.svelte (1)
141-149: Consider adding thetotalcount to the success event for consistency.Other batch delete handlers in this PR (e.g., executions, platforms, messaging) include
{ total: result.deleted.length }in the success tracking event. This one omits it.🔎 Proposed fix
if (result.error) { trackError(result.error, Submit.DatabaseBackupDelete); } else { - trackEvent(Submit.DatabaseBackupDelete); + trackEvent(Submit.DatabaseBackupDelete, { total: result.deleted.length }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/lib/actions/analytics.tssrc/lib/components/multiSelectTable.sveltesrc/routes/(console)/project-[region]-[project]/auth/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/members/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/identities/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/memberships/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/targets/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/+page.sveltesrc/routes/(console)/project-[region]-[project]/messaging/providers/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topics/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.sveltesrc/routes/(console)/project-[region]-[project]/overview/(components)/table.sveltesrc/routes/(console)/project-[region]-[project]/overview/platforms/+page.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.sveltesrc/routes/(console)/project-[region]-[project]/storage/bucket-[bucket]/+page.svelte
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,svelte}: Import reusable modules from the src/lib directory using the $lib alias
Use minimal comments in code; reserve comments for TODOs or complex logic explanations
Use $lib, $routes, and $themes aliases instead of relative paths for module imports
Files:
src/routes/(console)/project-[region]-[project]/messaging/topics/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/+page.sveltesrc/routes/(console)/project-[region]-[project]/storage/bucket-[bucket]/+page.sveltesrc/lib/actions/analytics.tssrc/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/targets/table.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/+page.sveltesrc/routes/(console)/project-[region]-[project]/messaging/providers/table.sveltesrc/lib/components/multiSelectTable.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/identities/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.sveltesrc/routes/(console)/project-[region]-[project]/overview/platforms/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/members/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/memberships/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/+page.sveltesrc/routes/(console)/project-[region]-[project]/overview/(components)/table.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/table.svelte
src/routes/**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use SvelteKit file conventions: +page.svelte for components, +page.ts for data loaders, +layout.svelte for wrappers, +error.svelte for error handling, and dynamic route params in square brackets like [param]
Files:
src/routes/(console)/project-[region]-[project]/messaging/topics/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/+page.sveltesrc/routes/(console)/project-[region]-[project]/storage/bucket-[bucket]/+page.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/targets/table.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/+page.sveltesrc/routes/(console)/project-[region]-[project]/messaging/providers/table.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/identities/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.sveltesrc/routes/(console)/project-[region]-[project]/overview/platforms/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/members/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/memberships/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/+page.sveltesrc/routes/(console)/project-[region]-[project]/overview/(components)/table.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/table.svelte
**/*.{ts,tsx,js,jsx,svelte,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation, single quotes, 100 character line width, and no trailing commas per Prettier configuration
Files:
src/routes/(console)/project-[region]-[project]/messaging/topics/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/+page.sveltesrc/routes/(console)/project-[region]-[project]/storage/bucket-[bucket]/+page.sveltesrc/lib/actions/analytics.tssrc/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/targets/table.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/+page.sveltesrc/routes/(console)/project-[region]-[project]/messaging/providers/table.sveltesrc/lib/components/multiSelectTable.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/identities/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.sveltesrc/routes/(console)/project-[region]-[project]/overview/platforms/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/members/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/memberships/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/+page.sveltesrc/routes/(console)/project-[region]-[project]/overview/(components)/table.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/table.svelte
**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use Svelte 5 + SvelteKit 2 syntax with TypeScript for component development
Files:
src/routes/(console)/project-[region]-[project]/messaging/topics/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/+page.sveltesrc/routes/(console)/project-[region]-[project]/storage/bucket-[bucket]/+page.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/targets/table.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/+page.sveltesrc/routes/(console)/project-[region]-[project]/messaging/providers/table.sveltesrc/lib/components/multiSelectTable.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/identities/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.sveltesrc/routes/(console)/project-[region]-[project]/overview/platforms/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/members/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/memberships/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/+page.sveltesrc/routes/(console)/project-[region]-[project]/overview/(components)/table.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/table.svelte
src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Configure dynamic routes using SvelteKit convention with [param] syntax in route directory names
Files:
src/routes/(console)/project-[region]-[project]/messaging/topics/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/+page.sveltesrc/routes/(console)/project-[region]-[project]/storage/bucket-[bucket]/+page.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/targets/table.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/+page.sveltesrc/routes/(console)/project-[region]-[project]/messaging/providers/table.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/identities/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.sveltesrc/routes/(console)/project-[region]-[project]/overview/platforms/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/members/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/user-[user]/memberships/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/+page.sveltesrc/routes/(console)/project-[region]-[project]/overview/(components)/table.sveltesrc/routes/(console)/project-[region]-[project]/functions/function-[function]/table.sveltesrc/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/table.svelte
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Define types inline or in .d.ts files, avoid creating separate .types.ts files
Use TypeScript in non-strict mode; any type is tolerated in this project
Files:
src/lib/actions/analytics.ts
src/lib/components/**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for component file names and place them in src/lib/components/[feature]/ directory structure
Files:
src/lib/components/multiSelectTable.svelte
🧠 Learnings (7)
📚 Learning: 2025-11-25T03:15:27.539Z
Learnt from: CR
Repo: appwrite/console PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:15:27.539Z
Learning: Applies to src/routes/**/*.svelte : Use SvelteKit file conventions: +page.svelte for components, +page.ts for data loaders, +layout.svelte for wrappers, +error.svelte for error handling, and dynamic route params in square brackets like [param]
Applied to files:
src/routes/(console)/project-[region]-[project]/auth/+page.sveltesrc/routes/(console)/project-[region]-[project]/sites/site-[site]/logs/table.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/+page.svelte
📚 Learning: 2025-10-07T14:16:31.893Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2413
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(entity)/helpers/analytics.ts:17-18
Timestamp: 2025-10-07T14:16:31.893Z
Learning: In the console codebase analytics helpers at `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(entity)/helpers/analytics.ts`, the dynamically constructed enum lookups (like `Submit[enumKey as keyof typeof Submit]`) are designed to return `undefined` safely when a terminology entry doesn't exist, because the corresponding views won't be rendered in those scenarios (e.g., DocumentsDB columns/attributes have no view), so the analytics code paths won't be reached.
Applied to files:
src/lib/actions/analytics.ts
📚 Learning: 2025-10-13T05:13:54.542Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2413
File: src/routes/(console)/project-[region]-[project]/databases/table.svelte:33-39
Timestamp: 2025-10-13T05:13:54.542Z
Learning: In Svelte 5, `import { page } from '$app/state'` provides a reactive state proxy that can be accessed directly (e.g., `page.params`), unlike the older `import { page } from '$app/stores'` which returns a readable store requiring the `$page` syntax for auto-subscription in components.
Applied to files:
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table.sveltesrc/routes/(console)/project-[region]-[project]/messaging/+page.sveltesrc/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.sveltesrc/routes/(console)/project-[region]-[project]/overview/platforms/+page.svelte
📚 Learning: 2025-10-05T09:41:40.439Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2398
File: src/routes/(console)/verify-email/+page.svelte:48-51
Timestamp: 2025-10-05T09:41:40.439Z
Learning: In SvelteKit 5, `page` imported from `$app/state` is a reactive state object (using runes), not a store. It should be accessed as `page.data` without the `$` prefix, unlike the store-based `$page` from `$app/stores` in earlier versions.
Applied to files:
src/routes/(console)/project-[region]-[project]/messaging/+page.sveltesrc/routes/(console)/project-[region]-[project]/overview/platforms/+page.svelte
📚 Learning: 2025-10-13T05:16:07.656Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2413
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/header.svelte:54-58
Timestamp: 2025-10-13T05:16:07.656Z
Learning: In SvelteKit apps, shared layout components (like headers) that use `$derived(page.data.*)` should use optional chaining when accessing properties that may not be present on all routes. During page transitions, reactive statements can briefly evaluate with different page.data structures, so optional chaining prevents runtime errors when navigating between routes with different data shapes (e.g., between `/databases` and `/databases/database-[database]`).
Applied to files:
src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.svelte
📚 Learning: 2025-11-19T11:22:42.553Z
Learnt from: atharvadeosthale
Repo: appwrite/console PR: 2512
File: src/routes/(console)/project-[region]-[project]/overview/platforms/llmBanner.svelte:51-83
Timestamp: 2025-11-19T11:22:42.553Z
Learning: In src/routes/(console)/project-[region]-[project]/overview/platforms/llmBanner.svelte, the Cursor integration URL format `https://cursor.com/link/prompt` with the `text` query parameter is correct and functional.
Applied to files:
src/routes/(console)/project-[region]-[project]/overview/platforms/+page.svelte
📚 Learning: 2025-10-26T10:20:29.792Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2509
File: src/routes/(console)/project-[region]-[project]/auth/teams/+page.svelte:47-61
Timestamp: 2025-10-26T10:20:29.792Z
Learning: When deleting teams in the Appwrite Console codebase, only `Dependencies.TEAMS` needs to be invalidated. Additional invalidations for `Dependencies.TEAM` (detail route) and `Dependencies.MEMBERSHIPS` are not necessary because the deleted teams and their memberships no longer exist.
Applied to files:
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/members/+page.sveltesrc/routes/(console)/project-[region]-[project]/auth/teams/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (22)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte (1)
2-7: LGTM!Import changes correctly bring in the
DeleteOperationtype along with the existing exports, following the $lib alias convention.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table.svelte (2)
6-11: LGTM!Import changes correctly add the
DeleteOperationtype while maintaining the existing component and type imports.
28-48: Batch deletion pattern correctly implemented.The refactored
onDeletefunction properly uses thebatchDeletecallback pattern. Error and success tracking are handled appropriately based on the batch result. Thefinallyblock ensuresDependencies.TABLESis invalidated andsubNavigationis updated regardless of the outcome.src/lib/components/multiSelectTable.svelte (4)
2-10: LGTM!The new type definitions are well-structured:
DeleteOperationStatewith optionalerroranddeletedarray covers both success and partial failure scenariosDeleteOperationsignature properly accepts a deletion callback and optional batch size
84-125: Well-designed batch deletion implementation.The
batchDeletefunction correctly:
- Uses
Promise.allSettledto handle partial failures gracefully- Tracks successfully deleted items separately from errors
- Captures only the first error (reasonable for user-facing messaging)
- Supports optional batching for large deletion sets
One minor observation: the batching condition at line 112 (
batchSize && batchSize < ids.length) means batching only kicks in when there are more items than the batch size. This is correct behavior.
127-146: Solid state management in consumeDeleteOperation.The function properly:
- Invokes the consumer's
onDeletewith the batch helper- Filters out successfully deleted items from
selectedRows(line 137)- Handles partial failures by keeping undeleted items selected
- Returns a boolean to indicate complete success vs partial/failure
199-205: Modal state management is correct.The modal correctly:
- Waits for the delete operation to complete
- Only closes on full success (
allDeleted)- Re-enables the modal buttons after the operation finishes
However, there's a potential UX issue: if partial deletion occurs,
disableModalis set tofalsebut the modal stays open with an error. This seems intentional to let users retry or cancel.src/routes/(console)/project-[region]-[project]/overview/(components)/table.svelte (1)
70-73: Signature adaptation is correct, but consider leveraging batchDelete.The handler correctly adapts to the new two-parameter signature by ignoring the
batchDeletefunction. Since this component uses a customDeleteBatchmodal (line 138) and setsconfirmDeletion={false}, the built-in batch deletion is bypassed.This works, but note that the custom
DeleteBatchcomponent won't benefit from the race condition fixes unless it also uses similar batching logic internally.src/lib/actions/analytics.ts (1)
280-280: LGTM!The new
DatabaseBackupDeleteanalytics event follows the established naming convention and is properly placed in theSubmitenum.src/routes/(console)/project-[region]-[project]/auth/teams/+page.svelte (2)
14-16: LGTM!Import correctly adds
DeleteOperationtype alongside existing component imports.
48-64: Batch deletion correctly implemented.The
handleDeletefunction properly uses the batch pattern and correctly invalidates onlyDependencies.TEAMSafter deletion. Based on learnings, additional invalidations forDependencies.TEAMorDependencies.MEMBERSHIPSare unnecessary since deleted teams and their memberships no longer exist.src/routes/(console)/project-[region]-[project]/auth/+page.svelte (2)
9-19: LGTM!Import changes correctly add
DeleteOperationtype while maintaining alphabetical ordering of the component imports.
64-80: Batch deletion pattern correctly applied.The
handleDeletefunction follows the same pattern as other files in this PR:
- Uses
batchDeletecallback for deletions- Tracks errors via
trackErroror success viatrackEvent- Invalidates
Dependencies.USERSin thefinallyblock- Returns the batch result for the component to handle
The implementation is consistent and correct.
src/routes/(console)/project-[region]-[project]/messaging/topics/table.svelte (1)
28-46: LGTM!The batch deletion implementation is consistent with the pattern used across the codebase. Error handling and dependency invalidation are correctly implemented.
src/routes/(console)/project-[region]-[project]/messaging/providers/table.svelte (1)
28-46: LGTM!The batch deletion implementation follows the established pattern. Error handling via
result.errorand success tracking viaresult.deleted.lengthare correctly implemented.src/routes/(console)/project-[region]-[project]/messaging/+page.svelte (1)
103-121: LGTM!The batch-based deletion is correctly implemented with appropriate error handling and dependency invalidation.
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte (1)
60-79: LGTM!The batch deletion pattern is consistently applied. The implementation correctly handles errors and tracks analytics based on the batch operation result.
src/routes/(console)/project-[region]-[project]/auth/teams/team-[team]/members/+page.svelte (1)
33-52: Missing retry logic contradicts PR objective.The
batchDeleteimplementation inMultiSelectionTablehandles concurrent deletions well usingPromise.allSettled(), allowing all deletions to execute in parallel with partial success support. However, the code has no retry mechanism, which contradicts the PR objective stating "handling retries smartly" to fix race conditions.Currently, each deletion is attempted only once. If a deletion fails due to transient errors or race conditions (e.g., concurrent access issues), there is no automatic retry. The
batchSizeparameter handles batching for large volumes, but not recovery from failures.Implement retry logic (with exponential backoff or similar strategy) in the
batchDeletefunction to align with the stated PR objective, especially for handling potential race conditions in team membership deletion scenarios.⛔ Skipped due to learnings
Learnt from: ItzNotABug Repo: appwrite/console PR: 2509 File: src/routes/(console)/project-[region]-[project]/auth/teams/+page.svelte:47-61 Timestamp: 2025-10-26T10:20:29.792Z Learning: When deleting teams in the Appwrite Console codebase, only `Dependencies.TEAMS` needs to be invalidated. Additional invalidations for `Dependencies.TEAM` (detail route) and `Dependencies.MEMBERSHIPS` are not necessary because the deleted teams and their memberships no longer exist.src/routes/(console)/project-[region]-[project]/functions/function-[function]/executions/table.svelte (1)
38-57: LGTM!The refactored
deleteExecutionsfunction correctly implements the batch-based deletion pattern. The error handling withresult.errorand success tracking withresult.deleted.lengthis consistent with the PR's approach across other components.src/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/table.svelte (1)
114-134: LGTM!The
deleteSingleBackupfunction properly handles the single-item deletion flow triggered from the action menu, with appropriate notifications and state cleanup.src/routes/(console)/project-[region]-[project]/overview/platforms/+page.svelte (1)
135-156: LGTM!The
handlePlatformDeletefunction correctly implements the batch-based deletion pattern with proper error tracking, success analytics, and dependency invalidation.src/routes/(console)/project-[region]-[project]/messaging/topics/topic-[topic]/table.svelte (1)
54-64: LGTM!The error tracking and success analytics follow the established pattern, and dependency invalidation is correctly placed in the finally block.
src/routes/(console)/project-[region]-[project]/auth/user-[user]/memberships/+page.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/multiSelectTable.svelte (1)
199-210: Wrap consumeDeleteOperation in try-finally to prevent stuck UI state.If
consumeDeleteOperation()throws an unexpected error (e.g., implementation bug inonDelete), line 209 never executes, leavingdisableModal = truepermanently. The modal becomes unresponsive and the user must reload the page.🔎 Proposed fix: Use try-finally to ensure state cleanup
onSubmit={async () => { disableModal = true; onDeleteError = null; - - const allDeleted = await consumeDeleteOperation(); - - if (allDeleted) { - showConfirmDeletion = false; + + try { + const allDeleted = await consumeDeleteOperation(); + + if (allDeleted) { + showConfirmDeletion = false; + } + } catch (err) { + onDeleteError = err instanceof Error ? err.message : String(err); + } finally { + disableModal = false; } - - disableModal = false; }}>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/multiSelectTable.svelte
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,svelte}: Import reusable modules from the src/lib directory using the $lib alias
Use minimal comments in code; reserve comments for TODOs or complex logic explanations
Use $lib, $routes, and $themes aliases instead of relative paths for module imports
Files:
src/lib/components/multiSelectTable.svelte
src/lib/components/**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for component file names and place them in src/lib/components/[feature]/ directory structure
Files:
src/lib/components/multiSelectTable.svelte
**/*.{ts,tsx,js,jsx,svelte,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation, single quotes, 100 character line width, and no trailing commas per Prettier configuration
Files:
src/lib/components/multiSelectTable.svelte
**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use Svelte 5 + SvelteKit 2 syntax with TypeScript for component development
Files:
src/lib/components/multiSelectTable.svelte
🧠 Learnings (1)
📚 Learning: 2025-11-25T03:15:27.539Z
Learnt from: CR
Repo: appwrite/console PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:15:27.539Z
Learning: Applies to **/*.svelte : Use Svelte 5 + SvelteKit 2 syntax with TypeScript for component development
Applied to files:
src/lib/components/multiSelectTable.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (2)
src/lib/components/multiSelectTable.svelte (2)
2-10: LGTM: Well-designed types for partial deletion tracking.The type definitions clearly support partial success scenarios, which aligns with the PR's goal of handling race conditions in deletions. The
DeleteOperationtype provides a clean API for batch operations.
131-150: LGTM: Partial success handling is correct.The function properly tracks deleted items and handles partial failures, allowing users to retry only the failed deletions. The filtering logic at line 141 ensures
selectedRowsaccurately reflects items that still need deletion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/components/multiSelectTable.svelte (2)
96-99: Consider throwing an error for invalid batchSize instead of silent correction.The validation prevents the infinite loop flagged in previous reviews by ensuring
batchSize >= 1. However, silently correcting invalid inputs (negative, zero, or decimal values) may hide bugs in calling code. Consider throwing a descriptive error instead:if (batchSize !== undefined && (batchSize <= 0 || !Number.isInteger(batchSize))) { throw new Error('batchSize must be a positive integer'); }This fails fast and makes debugging easier.
148-150: Error message could be more informative.The error message at line 149 provides the first error's message but doesn't indicate how many items failed or which specific items. Consider enhancing the message:
onDeleteError = `Failed to delete ${selectedRows.length - deletedCount} of ${selectedRows.length} ${getPluralResource()}. Error: ${state.error.message}`;This gives users a clearer picture of the failure scope.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/multiSelectTable.svelte
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,svelte}: Import reusable modules from the src/lib directory using the $lib alias
Use minimal comments in code; reserve comments for TODOs or complex logic explanations
Use $lib, $routes, and $themes aliases instead of relative paths for module imports
Files:
src/lib/components/multiSelectTable.svelte
src/lib/components/**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for component file names and place them in src/lib/components/[feature]/ directory structure
Files:
src/lib/components/multiSelectTable.svelte
**/*.{ts,tsx,js,jsx,svelte,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation, single quotes, 100 character line width, and no trailing commas per Prettier configuration
Files:
src/lib/components/multiSelectTable.svelte
**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use Svelte 5 + SvelteKit 2 syntax with TypeScript for component development
Files:
src/lib/components/multiSelectTable.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (7)
src/lib/components/multiSelectTable.svelte (7)
2-10: LGTM! Well-structured types for batch deletion.The new
DeleteOperationStateandDeleteOperationtypes clearly express the batch deletion contract. The union withvoidallows for no-op scenarios, and the use ofExcludeensures the return type is always defined.
51-58: Breaking API change enables batch deletion control.The updated
onDeletesignature provides both abatchDeletehelper and rawselectedRows, giving consumers flexibility to use the batched helper or implement custom logic. The inline comment clearly explains the use case for accessingselectedRowsdirectly.
67-77: LGTM! Parameter change supports accurate notifications for partial deletions.Accepting
countas a parameter allows the notification to reflect the actual number of items deleted, which is crucial when some deletions may fail in a batch.
121-128: Batch processing logic is correct.The conditional batching at line 121 correctly handles both batched and non-batched scenarios. The loop at line 122 safely increments by the validated
batchSize, and thesliceoperation at line 123 correctly extracts each batch.
136-155: Orchestration logic is well-structured.The
consumeDeleteOperationfunction correctly:
- Wraps
batchDeletewith the currentselectedRowscontext- Updates
selectedRowsby filtering deleted IDs (line 146)- Handles the void case (no-op)
- Returns a boolean to control modal visibility
The curry pattern at lines 137-139 elegantly closes over
selectedRowswhile giving the callback control overdeleteFnandbatchSize.
185-191: LGTM! Delete button correctly uses the new flow.The button properly routes through
consumeDeleteOperationwhenconfirmDeletionis false, ensuring consistent batch deletion behavior regardless of the confirmation setting.
204-215: Confirmation flow correctly handles partial failures.The modal submission logic properly:
- Disables the modal during processing (line 205)
- Calls
consumeDeleteOperationand captures the result (line 208)- Keeps the modal open if deletions fail (lines 210-212), allowing users to see the error
- Re-enables the modal after processing (line 214)
This provides good UX for handling partial deletion failures.
What does this PR do?
Handles retries smartly!
Test Plan
Manual.
Related PRs and Issues
N/A.
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.