(extension) allow deleting title mapping [DA-412]#333
Conversation
Mr-Quin
commented
Mar 21, 2026
|
Task linked: DA-412 Allow deleting title mapping |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for managing title mappings within the extension by introducing robust deletion capabilities. Users can now easily remove individual title mappings or select multiple mappings for bulk deletion, streamlining the management process. This is achieved through new UI components for multiselection and underlying service and RPC method additions for efficient data handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
danmaku-anywhere-web | 9e50fb7 | Commit Preview URL Branch Preview URL |
Mar 21 2026, 04:59 AM |
There was a problem hiding this comment.
Code Review
This pull request adds the functionality to delete title mappings, both individually and in bulk. The changes span from the backend service and RPC layer to the frontend components. New reusable components like MultiselectChip and SelectionBottomBar have been created, and DraggableList has been enhanced to support multiselect. The implementation is solid. I have a few suggestions to improve the user experience of the "select all" feature and to clean up the translation keys for better maintainability. I also found a missing translation key in the Chinese localization file which could lead to incorrect text being displayed.
packages/danmaku-anywhere/src/common/localization/locales/zh/translation.json
Show resolved
Hide resolved
| const handleSelectAll = () => { | ||
| if (selectedIds.length > 0) { | ||
| setSelectedIds([]) | ||
| } else { | ||
| setSelectedIds(mappings.map((m) => m.key)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The "select all" functionality can be made more intuitive. The current implementation deselects all items if any are selected. A more standard UX is:
- If some items are selected, the checkbox is in an
indeterminatestate. Clicking it selects all. - If all items are selected, the checkbox is
checked. Clicking it deselects all. - If no items are selected, it's unchecked. Clicking it selects all.
To implement this, you can adjust handleSelectAll and also update the Checkbox component on line 123 to use the checked and indeterminate props correctly, like so:
<Checkbox
checked={mappings.length > 0 && selectedIds.length === mappings.length}
indeterminate={selectedIds.length > 0 && selectedIds.length < mappings.length}
onChange={handleSelectAll}
size="small"
edge="start"
/>| const handleSelectAll = () => { | |
| if (selectedIds.length > 0) { | |
| setSelectedIds([]) | |
| } else { | |
| setSelectedIds(mappings.map((m) => m.key)) | |
| } | |
| } | |
| const handleSelectAll = () => { | |
| if (mappings.length > 0 && selectedIds.length === mappings.length) { | |
| setSelectedIds([]) | |
| } else { | |
| setSelectedIds(mappings.map((m) => m.key)) | |
| } | |
| } |
| const handleDeleteOne = (key: string) => { | ||
| dialog.delete({ | ||
| title: t('common.delete', 'Delete'), | ||
| content: t( | ||
| 'titleMapping.deleteConfirmOne', | ||
| 'Are you sure you want to delete this title mapping?' | ||
| ), | ||
| onConfirm: async () => { | ||
| await mutations.delete.mutateAsync(key) | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
To improve maintainability and reduce the number of translation strings, you can reuse the existing pluralized translation key titleMapping.deleteConfirm for single item deletion confirmation, instead of using a separate titleMapping.deleteConfirmOne key. You can do this by passing { count: 1 } to the t function. After this change, the deleteConfirmOne key can be removed from the translation files.
| const handleDeleteOne = (key: string) => { | |
| dialog.delete({ | |
| title: t('common.delete', 'Delete'), | |
| content: t( | |
| 'titleMapping.deleteConfirmOne', | |
| 'Are you sure you want to delete this title mapping?' | |
| ), | |
| onConfirm: async () => { | |
| await mutations.delete.mutateAsync(key) | |
| }, | |
| }) | |
| } | |
| const handleDeleteOne = (key: string) => { | |
| dialog.delete({ | |
| title: t('common.delete', 'Delete'), | |
| content: t( | |
| 'titleMapping.deleteConfirm', | |
| 'Are you sure you want to delete {{count}} title mapping(s)?', | |
| { count: 1 } | |
| ), | |
| onConfirm: async () => { | |
| await mutations.delete.mutateAsync(key) | |
| }, | |
| }) | |
| } |
packages/danmaku-anywhere/src/common/localization/locales/en/translation.json
Show resolved
Hide resolved
packages/danmaku-anywhere/src/common/localization/locales/zh/translation.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds UI and backend support for deleting title mappings in the browser extension, including single-item delete actions and multiselect bulk deletion.
Changes:
- Add bulk-delete RPC (
seasonMapDeleteMany) and background persistence support to remove multiple mappings at once. - Update the Title Mapping UI to support multiselect, single delete actions, and a bottom action bar for bulk deletion.
- Refactor multiselect UI into reusable components and broaden the player content-script match pattern.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/danmaku-anywhere/src/content/player/PlayerScript.service.ts | Uses getRandomUUID() for safer UUID generation in broader contexts. |
| packages/danmaku-anywhere/src/common/seasonMap/queries/useAllSeasonMap.ts | Adds deleteMany mutation calling the new RPC endpoint. |
| packages/danmaku-anywhere/src/common/rpcClient/background/types.ts | Extends background RPC contract with seasonMapDeleteMany. |
| packages/danmaku-anywhere/src/common/localization/locales/en/translation.json | Adds delete confirmation strings for single and plural deletion cases. |
| packages/danmaku-anywhere/src/common/localization/locales/zh/translation.json | Adds delete confirmation strings for single and plural deletion cases. |
| packages/danmaku-anywhere/src/common/components/TitleMapping/TitleMappingPageCore.tsx | Introduces multiselect state, delete confirmation dialogs, and bulk delete UX. |
| packages/danmaku-anywhere/src/common/components/TitleMapping/TitleMappingList.tsx | Adds per-row delete action and passes multiselect props into DraggableList. |
| packages/danmaku-anywhere/src/common/components/SelectionBottomBar.tsx | New reusable bottom bar for multiselect actions (cancel + selection count + actions). |
| packages/danmaku-anywhere/src/common/components/MultiselectChip.tsx | New reusable “Multiselect” toggle chip used by multiple pages. |
| packages/danmaku-anywhere/src/common/components/DraggableList.tsx | Adds multiselect rendering/selection toggling to the generic list component. |
| packages/danmaku-anywhere/src/common/components/DanmakuSelector/components/MountPageToolbar.tsx | Reuses MultiselectChip instead of inlining the chip UI. |
| packages/danmaku-anywhere/src/background/services/persistence/TitleMappingService.ts | Adds removeMany() to delete multiple mappings in one Dexie query. |
| packages/danmaku-anywhere/src/background/rpc/RpcManager.ts | Wires new RPC method to TitleMappingService.removeMany. |
| packages/danmaku-anywhere/manifest.ts | Changes player content-script match patterns to <all_urls>. |
| package.json | Adds convenience scripts for extension development. |
Comments suppressed due to low confidence (1)
packages/danmaku-anywhere/manifest.ts:60
- Switching the player content script match patterns from
https://*/*+http://*/*to<all_urls>is a significant scope expansion (e.g., includesfile://andftp://). If the intent is only to support additional specific schemes (likefile://), consider explicitly listing only the required schemes/patterns to minimize permission/surface area, and verify the declaredhost_permissionsremain aligned with thematchespatterns.
{
matches: ['<all_urls>'],
js: ['src/content/player/index.ts'],
run_at: 'document_start',
all_frames: true,
// isolated world by default
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Checkbox | ||
| checked={selectedIds.length > 0} | ||
| onChange={handleSelectAll} | ||
| size="small" | ||
| edge="start" | ||
| /> |
There was a problem hiding this comment.
The “select all” checkbox is currently driven by selectedIds.length > 0 and handleSelectAll clears the selection whenever any items are selected. This makes it impossible to go from a partial selection to “all selected”, and the checkbox can’t represent a partial (indeterminate) state. Consider setting checked only when selectedIds.length === mappings.length (and mappings.length > 0), using indeterminate when 0 < selectedIds.length < mappings.length, and updating handleSelectAll to select all unless everything is already selected.
| const handleSelectAll = () => { | ||
| if (selectedIds.length > 0) { | ||
| setSelectedIds([]) | ||
| } else { | ||
| setSelectedIds(mappings.map((m) => m.key)) | ||
| } |
There was a problem hiding this comment.
handleSelectAll currently toggles based on selectedIds.length > 0, which means if the user has a partial selection it will clear instead of selecting all. Consider toggling based on whether all items are already selected (e.g., compare against mappings.length) so the control behaves like a true “select all / clear all”.
packages/danmaku-anywhere/src/common/components/TitleMapping/TitleMappingList.tsx
Outdated
Show resolved
Hide resolved
53b9836 to
911ec1f
Compare
911ec1f to
9e50fb7
Compare