Add bookmark soft delete with deletedAt#64
Add bookmark soft delete with deletedAt#64katsumi-axis wants to merge 3 commits intoviperrcrypto:mainfrom
Conversation
…e and create corresponding index
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: Bookmark Soft Delete
Overall: Solid implementation of soft delete pattern. The deletedAt nullable timestamp approach is idiomatic and the comprehensive query updates across all API routes show good attention to detail.
What looks good
- Consistent use of
deletedAt: nullfilter across all Prisma queries - New
getActiveBookmarkCountMap()helper properly filters via join table - Migration includes index on
deletedAtfor query performance - UI handles deletion with optimistic update and proper loading state
- DELETE endpoint returns appropriate 404/200 responses
Issues to address
1. Missing foreign key cleanup consideration (medium risk)
The PR adds soft delete to bookmarks but doesn't address related data in BookmarkCategory join table. While the count queries correctly filter via the relation, you may want to consider:
- Should deleting a bookmark also cascade-soft-delete its category relationships?
- Or should
BookmarkCategoryentries remain (they won't show in counts due to the join filter, but they accumulate orphaned rows)
2. UI race condition on category page
The category page fetches data server-side then allows deletion. If a bookmark is deleted, the total count decrements but the pagination might be stale if the user navigates pages. Consider refetching or using the existing SWR pattern.
3. No test coverage
A change touching 20+ files and modifying core data access patterns should have at least:
- Unit tests for
getActiveBookmarkCountMap() - Integration tests that verify deleted bookmarks don't appear in search/AI routes
- Test for the DELETE endpoint (404 case, already deleted case, success case)
Minor
- The migration filename suggests manual timestamp — fine, but verify it doesn't conflict with existing migrations
- Consider adding a
deletedAtfilter to any analytics/aggregation queries that might be in other files not shown in the diff
Security note
Good: The delete endpoint checks if bookmark exists and already deleted before proceeding. No auth middleware shown in diff — assume that's handled upstream or in layout.
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: Bookmark Soft Delete
Overall: Solid implementation of soft delete pattern. The deletedAt nullable timestamp approach is idiomatic and the comprehensive query updates across all API routes show good attention to detail.
What looks good
- Consistent use of
deletedAt: nullfilter across all Prisma queries - New
getActiveBookmarkCountMap()helper properly filters via join table - Migration includes index on
deletedAtfor query performance - UI handles deletion with optimistic update and proper loading state
- DELETE endpoint returns appropriate 404/200 responses
Issues to address
1. Missing foreign key cleanup consideration (medium risk)
The PR adds soft delete to bookmarks but doesn't address related data in BookmarkCategory join table. While the count queries correctly filter via the relation, you may want to consider:
- Should deleting a bookmark also cascade-soft-delete its category relationships?
- Or should
BookmarkCategoryentries remain (they won't show in counts due to the join filter, but they accumulate orphaned rows)
2. UI race condition on category page
The category page fetches data server-side then allows deletion. If a bookmark is deleted, the total count decrements but the pagination might be stale if the user navigates pages. Consider refetching or using the existing SWR pattern.
3. No test coverage
A change touching 20+ files and modifying core data access patterns should have at least:
- Unit tests for
getActiveBookmarkCountMap() - Integration tests that verify deleted bookmarks don't appear in search/AI routes
- Test for the DELETE endpoint (404 case, already deleted case, success case)
Minor
- The migration filename suggests manual timestamp — fine, but verify it doesn't conflict with existing migrations
- Consider adding a
deletedAtfilter to any analytics/aggregation queries that might be in other files not shown in the diff
Security note
Good: The delete endpoint checks if bookmark exists and already deleted before proceeding. No auth middleware shown in diff — assume that's handled upstream or in layout.
Summary
This PR adds bookmark soft delete support using
deletedAtso imported data can be removed from the app without physically deleting related records.What Changed
Bookmark.deletedAtand a new per-bookmarkDELETE /api/bookmarks/[id]routeWhy
The app previously only had an all-data destructive delete path. This change adds a safer, more reversible deletion model while keeping the existing structure mostly intact and minimizing changes to current query patterns.
Impact
DELETE /api/bookmarksbehavior is unchangedtweetIdValidation
npx prisma generatenpx prisma db pushgetActiveBookmarkCountMap()executes successfullyKnown Issue
npx tsc --noEmitis currently blocked by an existing unrelated nullability error inapp/api/import/x-oauth/fetch/route.ts.