fix(bookmarks): resolve category validation and duplication when moving folders

[syed-tp]

What:

• Translate Question, Post, and ForumPost types to their backend categories.
• Pass bookmarkType instead of empty type from BookmarksScreen.
• Update folder counts and retain metadata (title/chapter) on move/update.
• Access providers via a stable ProviderContainer inside the bottom sheet.

Why:

• Prevents 400 Bad Request API errors for non-lesson bookmarked content.
• Stops empty category validation errors on cached bookmarks.
• Prevents moved bookmarks from duplicating and title from flickering to "Unknown".
• Fixes the "Cannot use ref after the widget was disposed" crash after dismissing the sheet.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[claude]

Claude finished @syed-tp's task in 3m 47s —— View job

---

Todo List

• Read AGENTS.md and core docs for repo rules
• Check openspec/ for relevant ADRs
• Fetch full PR diff
• Review OpenSpec compliance
• Review SDK boundary / import rules
• Review design system & widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation pattern
• Review Riverpod provider pattern
• Review data layer architecture
• Review generated files
• Review architecture & file structure
• Review offline data consistency
• Post final structured review

PR Review Summary

✅ What's good

• Category-mapping fix (_mapToBackendCategory) correctly lower-cases the switch's case labels ('embedcontent' etc.) — the old code compared a lower-cased input against a mixed-case label, so it could never match. Good catch.
• ProviderScope.containerOf(context) captured before widget.onClose() in bookmark_folders_sheet.dart:59 is the correct, standard fix for the "used ref after disposed" Riverpod crash.
• Deferring the success toast until after the awaited provider call resolves (bookmark_folders_sheet.dart:88-94) correctly fixes the "success toast then error toast" UX bug.
• OpenSpec docs (openspec/changes/fix-bookmark-move-category/*) were added even though this is arguably a bug fix (exempt) — good practice, and appreciated.
• Widget/design-system/accessibility/import-boundary rules are all respected — no Material/Cupertino widgets, tokens read via Design.of(context), existing AppSemantics usage untouched.

🔴 CRITICAL — Must fix before merge

1. The core "duplicate bookmark on move" bug is not actually fixed. _toggleBookmark in packages/core/lib/widgets/bookmark_folders_sheet.dart:55-105 only acts on the tapped row's own existingBookmarks — it either adds to the tapped folder or removes from the tapped folder. It never removes the bookmark from the previous folder when a new folder is selected. BookmarkRepository.addBookmark (bookmark_repository.dart:313-434) does insertOnConflictUpdate keyed on newBookmark.id, i.e. the ID returned by the backend POST /bookmarks call. Both HttpDataSource.createBookmark (http_data_source.dart:1220-1238, plain _dio.post) and MockDataSource.createBookmark (mock_data_source.dart:1803-1857, which only reuses an existing row when lessonId and folderId both match) create a new bookmark record per lesson+folder pair rather than moving an existing one. So selecting a new folder for an already-bookmarked lesson creates a second local+remote row instead of replacing the old one — the exact duplication bug the PR claims to fix.
   This directly contradicts the PR's own OpenSpec docs: design.md "Decision 5: Clean up old bookmarks in other folders on move" and specs/bookmark-folders/spec.md scenario "Move folder deletes old bookmark" both describe querying bookmarksForLessonProvider, diffing against the new folder, and calling removeBookmarkProvider for stale entries before addBookmarkProvider — none of that logic is present in the shipped _toggleBookmark.
   The folder-count bookkeeping added in addBookmark (decrementing oldFolderId, incrementing newBookmark.folderId) also silently assumes the old local row gets replaced in place, which won't happen if the new row has a different ID — this can further desync bookmarksCount from the actual list contents.
   Same shared widget is also used from packages/courses/lib/screens/lesson_detail_orchestrator.dart:219, so the bug isn't limited to BookmarksScreen.
   Fix this →

🟡 WARNING — Should fix

1. No new/updated tests accompany a fix for a data-corruption-adjacent bug (duplicate bookmarks, folder count drift). tasks.md says "Run tests… to ensure no regressions" but doesn't add a regression test for the move/duplicate scenario or the category-mapping fix. Given the bug in the CRITICAL item above, a test would likely have caught it.
2. BookmarkRepository.addBookmark's existing-metadata lookup (bookmark_repository.dart:329-333) selects bookmarkItemsTable by lessonId only with limit(1) and no ordering — if a lesson has bookmarks in multiple folders simultaneously, this can pull metadata from an arbitrary row. Low impact today since title/chapterName should be identical across folders for the same lesson, but worth a comment or explicit orderBy if that assumption ever changes.

🔵 SUGGESTION — Nice to have

1. _CreateFolderDialogState._saveFolder (bookmark_folders_sheet.dart:419-466) still uses ref.read(...) directly rather than the same ProviderScope.containerOf pattern used in _toggleBookmark. It's currently safe because widget.onClose() is only called after the awaited call and a mounted check, but for consistency with the newly-documented pattern (and defense against future refactors) it could use the same stable-container approach.

Verdict

CHANGES REQUESTED

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 3m 13s —— View job

---

Todo List

• Read AGENTS.md and core docs for repo rules
• Check openspec/ for relevant ADRs
• Fetch full PR diff
• Review OpenSpec compliance
• Review SDK boundary / import rules
• Review design system & widget usage
• Review accessibility semantics
• Review motion & animation
• Review platform neutrality
• Review navigation pattern
• Review Riverpod provider pattern
• Review data layer architecture
• Review generated files
• Review architecture & file structure
• Review offline data consistency
• Post final structured review

---

PR Review Summary

This re-reviews the latest commits (def8aa1, f747e60, 1f19908), which addressed the earlier CRITICAL finding (duplicate bookmark on move) by adding a dedicated BookmarkRepository.moveBookmark / moveBookmarkProvider. That fix works for the primary "select a different folder in the sheet" flow, but the same duplication bug is still reachable through a second path, and the new move logic has a partial-failure gap.

✅ What's good

• _mapToBackendCategory now lower-cases category before the switch (bookmark_repository.dart:677) and adds question/post/forumpost mappings — confirmed by the new bookmark_repository_test.dart case-insensitivity test.
• The core "move" path (BookmarkFoldersSheet._toggleBookmark) now correctly detects an existing bookmark in another folder via bookmarksForLessonProvider and routes through the new moveBookmarkProvider instead of blindly calling addBookmarkProvider — this is a real fix for the duplication bug in the primary flow.
• ProviderScope.containerOf(context) is now used consistently in both _toggleBookmark and _CreateFolderDialogState._saveFolder (bookmark_folders_sheet.dart:60, :457), resolving the earlier SUGGESTION about inconsistent ref/container usage.
• Success toast is deferred until after the awaited provider call resolves, and ApiException.message is now surfaced for rename/create failures with proper localization added consistently across all 4 locale files (app_en/ar/ml/ta.arb).
• .g.dart diff is consistent with the @riverpod source changes (new title/chapterName/moveBookmark params) — no hand-editing.

🔴 CRITICAL — Must fix before merge

1. The "create new folder and auto-bookmark" path still causes remote bookmark duplication. _CreateFolderDialogState._saveFolder (packages/core/lib/widgets/bookmark_folders_sheet.dart:476-485) calls addBookmarkProvider directly (never moveBookmarkProvider) when auto-selecting a newly created folder for the current lesson. This dialog is reachable from BookmarkFoldersSheet's "create new folder" action (onCreateFolderRequest, line 357/359) for a lesson that is already bookmarked in another folder. BookmarkRepository.addBookmark only calls _dataSource.createBookmark (never deleteBookmark) — it does delete the stale local row (lines ~352-373 of the diff) but leaves the old bookmark on the backend. Because fetchBookmarks does insertOrReplace from the server response on every refresh (bookmark_repository.dart:213-237), the next pull-to-refresh or pagination reload will re-insert that orphaned remote duplicate into the local cache too — silently undoing the local-only cleanup and reproducing the exact bug this PR claims to fix, just via a different entry point.
   Fix this →

🟡 WARNING — Should fix

1. moveBookmark's delete+create are not safe against partial failure. BookmarkRepository.moveBookmark (bookmark_repository.dart:468-473) fires _dataSource.deleteBookmark(oldBookmarkId) and _dataSource.createBookmark(...) together via Future.wait, which uses eagerError: true by default. If deleteBookmark succeeds but createBookmark fails (or times out), the lesson loses its bookmark entirely (old deleted, new never created) even though the UI shows a generic "failed to update" error — the user has no bookmark and no indication it was actually removed. If createBookmark succeeds but deleteBookmark fails, the backend ends up with two bookmarks for the lesson again. Consider sequencing these (create the new bookmark first, confirm success, then delete the old one) so a failure leaves the pre-move state recoverable rather than silently destructive.
2. OpenSpec design.md doesn't match the shipped implementation. openspec/changes/fix-bookmark-move-category/design.md "Decision 5" describes deleting stale bookmarks via a loop of removeBookmarkProvider calls before addBookmarkProvider, but the actual code introduces a distinct moveBookmark/moveBookmarkProvider (parallel delete+create in the repository, not a sequential remove-then-add in the widget). Worth updating the doc to match, since the design doc is the source of truth other contributors will read for this feature.
3. No test coverage for the new moveBookmark repository method or the CreateFolderDialog duplication path from CRITICAL feat(ui): implement LMS core primitives #1 — bookmark_repository_test.dart only covers addBookmark (category mapping + metadata preservation). Given this is explicitly a data-corruption/duplication fix, a regression test asserting deleteBookmark(oldId) + createBookmark(newFolder) are both invoked, and that the old local row is removed, would have caught CRITICAL feat(ui): implement LMS core primitives #1.
4. otherBookmarks.first (bookmark_folders_sheet.dart:92) only moves the first "other folder" bookmark it finds. If a lesson somehow ends up bookmarked in more than one other folder (e.g. from a pre-existing duplicate, or the CRITICAL feat(ui): implement LMS core primitives #1 gap), only one gets cleaned up remotely per move; the rest are only removed from the local cache (via the otherLocalBookmarks cleanup in moveBookmark/addBookmark) and will resurface on the next server sync, same root cause as CRITICAL feat(ui): implement LMS core primitives #1.

🔵 SUGGESTION — Nice to have

1. addBookmark and moveBookmark in bookmark_repository.dart duplicate a large block of logic (existing-metadata lookup, lesson-table fallback, "delete other local duplicates" loop, folder count bookkeeping, BookmarkDto reconstruction). Extracting the shared parts into a private helper would reduce the risk of the two methods drifting out of sync (as already happened slightly — e.g. bookmarkType fallback source differs subtly between them).

Verdict

CHANGES REQUESTED

[syed-tp]

@claude review

[claude]

Claude finished @syed-tp's task in 1m 56s —— View job

---

Todo List

• Read AGENTS.md and core docs for repo rules
• Check openspec/ for relevant ADRs
• Fetch full PR diff (focus on latest commit 8a87549)
• Re-review previously flagged CRITICAL/WARNING items
• Review new "multiple IDs" move logic for correctness
• Post final structured review

---

PR Review Summary

This re-reviews the latest commit (8a87549) against the two prior review rounds. All previously reported CRITICAL issues are now resolved.

✅ What's good

• CRITICAL feat(ui): implement LMS core primitives #1 (create-folder duplication) — fixed. _CreateFolderDialogState._saveFolder (bookmark_folders_sheet.dart:476-511) now checks bookmarksForLessonProvider and routes through moveBookmarkProvider when the lesson already has bookmarks elsewhere, mirroring _toggleBookmark. Both entry points into the shared sheet (bookmarks_screen.dart, review_answer_detail_screen.dart, lesson_detail_orchestrator.dart) go through this same widget, so the fix covers all callers.
• WARNING (partial-failure gap) — fixed. BookmarkRepository.moveBookmark (bookmark_repository.dart:468-483) now sequences the calls correctly: create the new bookmark first, then delete old ones, instead of the previous Future.wait race. A create failure now leaves the pre-move state intact.
• WARNING (only first "other" bookmark cleaned up) — fixed. Both _toggleBookmark and _saveFolder now collect all otherBookmarks and pass the full oldBookmarkIds list into moveBookmarkProvider/moveBookmark, and _atomicLocalWrite deletes each one (plus a defensive sweep for any remaining duplicates) inside a single DB transaction.
• WARNING (design doc drift) — fixed. design.md Decision 5 and the OpenSpec scenario "Move folder deletes old bookmark" now accurately describe the shipped moveBookmark/moveBookmarkProvider flow (create-then-delete, single local transaction) rather than the old sequential remove/add description.
• WARNING (no test coverage) — addressed. The new bookmark_repository_test.dart adds a dedicated moveBookmark test that seeds two old bookmarks, verifies createBookmark → deleteBookmark(111) → deleteBookmark(222) ordering via verifyInOrder, and asserts final local DB state and folder-count bookkeeping (2 → 0, 0 → 1).
• bookmark_provider.g.dart diff (new moveBookmarkProvider family) is fully consistent with the @riverpod source — no hand-editing.
• Localization: bookmarkMovedToFolder and errorFailedToRenameFolder are added consistently across all 4 locale ARB files and their generated app_localizations_*.dart counterparts.

🟡 WARNING — Should fix

1. Old-bookmark deletion failures are silently swallowed. In BookmarkRepository.moveBookmark (bookmark_repository.dart:475-481), each _dataSource.deleteBookmark(oldId) call is wrapped in its own try/catch that only does debugPrint on failure — the move is reported as successful to the caller even if a stale backend bookmark couldn't be deleted. Since fetchBookmarks does insertOrReplace from the server on every refresh, that orphaned remote bookmark will resurface as a duplicate on the next sync/pull-to-refresh — the same root cause as the original bug, just narrowed to the case where the delete call itself fails (network blip, stale ID, etc.) rather than happening on every move. Consider surfacing this partial failure (e.g. a distinct toast/log/telemetry signal) so it's not entirely invisible.

🔵 SUGGESTION — Nice to have

1. addBookmark and moveBookmark in bookmark_repository.dart still duplicate a fair amount of logic (existing-metadata lookup, lessonsTable fallback, BookmarkDto reconstruction). Not a blocker, but extracting the shared metadata-resolution block into a private helper would reduce future drift risk between the two methods.

Verdict

APPROVED

Note: I don't have flutter/dart available in this environment, so I wasn't able to run flutter analyze or the test suite directly — verification here is based on static review of the diff and the new bookmark_repository_test.dart coverage. If you'd like automated verification, please add the appropriate tool permissions.