Skip to content

feat: bulk delete documents in nova app#777

Open
MaheshtheDev wants to merge 4 commits intomainfrom
03-10-feat_bulk_delete_documents_in_nova_app
Open

feat: bulk delete documents in nova app#777
MaheshtheDev wants to merge 4 commits intomainfrom
03-10-feat_bulk_delete_documents_in_nova_app

Conversation

@MaheshtheDev
Copy link
Member

@MaheshtheDev MaheshtheDev commented Mar 10, 2026

image.png

@MaheshtheDev MaheshtheDev marked this pull request as ready for review March 10, 2026 05:36
@graphite-app graphite-app bot requested a review from Dhravya March 10, 2026 05:36
@cloudflare-workers-and-pages
Copy link

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
❌ Deployment failed
View logs
supermemory-app 022fab2 Mar 10 2026, 05:36 AM

Copy link
Member Author


How to use the Graphite Merge Queue

Add the label Main to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 10, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
supermemory-app 71c7400 Commit Preview URL Mar 10 2026, 07:52 AM

Comment on lines +187 to +197
{ documentIds: ids },
{
onSuccess: () => {
setSelectedDocumentIds(new Set())
setIsSelectionMode(false)
if (selectedDocument && ids.includes(selectedDocument.id ?? "")) {
setDocId(null)
}
},
},
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Performing a bulk delete clears any unsaved text in the quick note draft input due to an onSuccess callback firing unintentionally.
Severity: MEDIUM

Suggested Fix

Decouple the onClose logic from the bulk delete mutation's onSuccess callback. The onSuccess for bulkDeleteMutation should only handle logic related to the deletion itself, such as clearing selection state. The resetDraft() function should not be called as a side effect of a successful bulk deletion.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: apps/web/app/(app)/page.tsx#L186-L197

Potential issue: When a user performs a bulk delete operation, two separate `onSuccess`
callbacks are triggered. One is defined at the `useMutation` level, which calls an
`onClose` handler that in turn calls `resetDraft()`. This occurs even if the user has
unsaved text in the quick note input field. The `resetDraft()` function then clears this
unsaved text without warning, leading to data loss for the user. This bug is triggered
specifically when a user has an unsaved quick note draft and successfully completes a
bulk delete action.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Member

@Prasanna721 Prasanna721 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you fix the build issues?

Copy link
Member

Prasanna721 commented Mar 10, 2026

can select & delete button can be at the right of dismiss?

Copy link
Member Author

can select & delete button can be at the right of dismiss?

​what do you mean @Prasanna721 i didn't get it

@claude
Copy link

claude bot commented Mar 10, 2026

Claude finished @MaheshtheDev's task in 58s —— View job


Review Complete

  • Gather context from PR and existing review comments
  • Review the PR diff for bugs, security issues, and logic errors
  • Check if previous review issues have been addressed
  • Submit review findings

Overview: Adds bulk delete functionality for documents in the nova app with selection mode UI.

Previously flagged issues - now resolved:

  1. Bug (fixed): The onClose?.() call that was unintentionally clearing the quick note draft has been removed from bulkDeleteMutation.onSuccess (use-document-mutations.ts:546-552)

  2. Bug (fixed): The pluralization issue ("memoryies") has been corrected to properly display "memory" or "memories" (memories-grid.tsx:462)

Issues found: None - this looks good to ship.

Score: 9/10

Good implementation with selection mode UI, optimistic updates, proper error handling with rollback, and a confirmation dialog. All previously identified bugs have been addressed. The Cloudflare deployment failure mentioned in comments appears to be infrastructure-related rather than a code issue in this PR.

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview: Adds bulk delete functionality for documents in the nova app with selection mode UI.

Issues found:

  1. Bug (confirmed from Sentry): bulkDeleteMutation.onSuccess calls onClose?.() which resets the quick note draft unintentionally. Users with unsaved text in the quick note input will lose their draft when performing bulk delete. See inline comment for the fix.

Regarding build issues: The TypeScript errors in the type-check output are pre-existing in @supermemory/tools (test files) and unrelated to this PR's changes. The Cloudflare deployment failure appears to be infrastructure-related rather than a code issue in this PR.

Score: 8/10

Good implementation overall - the selection mode UI, optimistic updates, and confirmation dialog are well done. Just needs the one bug fix before merging.

)
queryClient.invalidateQueries({ queryKey: ["documents-with-memories"] })
onClose?.()
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug confirmed: The Sentry AI report is correct. Calling onClose?.() here will unintentionally clear the quick note draft when bulk delete succeeds.

The onClose callback is designed for note/link/file mutations where the user saves content and the modal closes (resetting the draft makes sense there). But for bulk delete, this side effect is unexpected - users deleting documents shouldn't lose their unsaved quick note text.

Fix: Remove onClose?.() from this onSuccess callback. The inline onSuccess in handleBulkDelete already handles clearing selection state appropriately.

Suggested change
},
onSuccess: (_data, variables) => {
analytics.documentsBulkDeleted({ count: variables.documentIds.length })
toast.success(
`${variables.documentIds.length} document${variables.documentIds.length === 1 ? "" : "s"} deleted`,
)
queryClient.invalidateQueries({ queryKey: ["documents-with-memories"] })
},

Comment on lines +461 to +464
This will permanently delete {selectedDocumentIds.size} memory
{selectedDocumentIds.size === 1 ? "" : "ies"}. This action cannot
be undone.
</AlertDialogDescription>

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants