Fixes #1747 Add Unused Tags Management to Cleanups Dashboard#2224
Fixes #1747 Add Unused Tags Management to Cleanups Dashboard#2224sepej-osu wants to merge 2 commits intokarakeep-app:mainfrom
Conversation
WalkthroughReplaces the previous inline cleanups page with a lightweight page that renders a new client-side Cleanups component. Adds two new cleanup subcomponents (DuplicateTags, UnusedTags), renames TagDuplicationDetection → DuplicateTags, updates TagPill to support optional counts, and adds one i18n key. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
sepej-osu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/web/components/dashboard/cleanups/DuplicateTags.tsx (1)
208-307: Card-based DuplicateTags UI is solid; consider i18n + immutability tweaksThe new Card layout, badge, and collapsible table integrate well and keep the UX clear. Two small follow-ups to consider:
- The header and helper copy (
"Duplicate Tags","Merge similar tags to keep your collection organized","You have X suggestions for tag merging.","Hide All"/"Show All") are hardcoded strings. Since you already havecleanups.cleanupsandcleanups.duplicate_tags.titlein the locale file, it would be cleaner to useuseTranslationhere as well so this section is fully localizable.- In the
useEffect,allTags.tags.sort(...)mutates the array returned fromapi.tags.list.useQuery. To avoid side effects on the cached query data, consider cloning first, e.g.const sortedTags = [...(allTags?.tags ?? [])].sort(...).apps/web/components/dashboard/cleanups/Cleanups.tsx (1)
6-16: Cleanups wrapper is fine; consider localizing the titleThe layout composition looks good. To stay consistent with the rest of the app, you might want to:
- Use
useTranslationandt("cleanups.cleanups")instead of the hardcoded"Cleanups"string.- Optionally switch the
<span className="text-2xl">to an actual heading element (e.g.<h1>) for better semantics and a11y.apps/web/app/dashboard/cleanups/page.tsx (1)
1-4: Page wrapper is good; async can be droppedRouting the page through the shared
Cleanupscomponent keeps things tidy. SinceCleanupsPagedoesn’tawaitanything, you can drop theasynckeyword to reduce mental overhead, but it’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/app/dashboard/cleanups/page.tsx(1 hunks)apps/web/components/dashboard/cleanups/Cleanups.tsx(1 hunks)apps/web/components/dashboard/cleanups/DuplicateTags.tsx(4 hunks)apps/web/components/dashboard/cleanups/UnusedTags.tsx(1 hunks)apps/web/components/dashboard/tags/AllTagsView.tsx(3 hunks)apps/web/components/dashboard/tags/TagPill.tsx(2 hunks)apps/web/lib/i18n/locales/en/translation.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
apps/web/components/dashboard/cleanups/Cleanups.tsxapps/web/components/dashboard/tags/TagPill.tsxapps/web/components/dashboard/cleanups/UnusedTags.tsxapps/web/app/dashboard/cleanups/page.tsxapps/web/components/dashboard/cleanups/DuplicateTags.tsxapps/web/components/dashboard/tags/AllTagsView.tsx
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
apps/web/components/dashboard/cleanups/Cleanups.tsxapps/web/lib/i18n/locales/en/translation.jsonapps/web/components/dashboard/tags/TagPill.tsxapps/web/components/dashboard/cleanups/UnusedTags.tsxapps/web/app/dashboard/cleanups/page.tsxapps/web/components/dashboard/cleanups/DuplicateTags.tsxapps/web/components/dashboard/tags/AllTagsView.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
apps/web/components/dashboard/cleanups/Cleanups.tsxapps/web/components/dashboard/tags/TagPill.tsxapps/web/components/dashboard/cleanups/UnusedTags.tsxapps/web/app/dashboard/cleanups/page.tsxapps/web/components/dashboard/cleanups/DuplicateTags.tsxapps/web/components/dashboard/tags/AllTagsView.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/components/dashboard/cleanups/Cleanups.tsxapps/web/components/dashboard/tags/TagPill.tsxapps/web/components/dashboard/cleanups/UnusedTags.tsxapps/web/app/dashboard/cleanups/page.tsxapps/web/components/dashboard/cleanups/DuplicateTags.tsxapps/web/components/dashboard/tags/AllTagsView.tsx
🧬 Code graph analysis (5)
apps/web/components/dashboard/tags/TagPill.tsx (1)
apps/web/components/ui/separator.tsx (1)
Separator(30-30)
apps/web/components/dashboard/cleanups/UnusedTags.tsx (7)
packages/shared-react/hooks/tags.ts (2)
useDeleteUnusedTags(106-118)usePaginatedSearchTags(7-18)apps/web/components/ui/use-toast.ts (1)
toast(188-188)apps/web/components/ui/action-confirming-dialog.tsx (1)
ActionConfirmingDialog(15-55)packages/shared/types/tags.ts (1)
ZTagBasic(42-42)apps/web/components/dashboard/tags/DeleteTagConfirmationDialog.tsx (1)
DeleteTagConfirmationDialog(8-58)apps/web/components/dashboard/tags/TagPill.tsx (1)
TagPill(12-128)apps/web/components/ui/card.tsx (5)
Card(80-80)CardHeader(81-81)CardTitle(83-83)CardDescription(84-84)CardContent(85-85)
apps/web/app/dashboard/cleanups/page.tsx (1)
apps/web/components/dashboard/cleanups/Cleanups.tsx (1)
Cleanups(6-18)
apps/web/components/dashboard/cleanups/DuplicateTags.tsx (3)
apps/web/components/ui/card.tsx (5)
Card(80-80)CardHeader(81-81)CardTitle(83-83)CardDescription(84-84)CardContent(85-85)apps/web/components/ui/collapsible.tsx (3)
Collapsible(50-50)CollapsibleTrigger(51-51)CollapsibleContent(52-52)apps/web/components/ui/table.tsx (5)
Table(108-108)TableHeader(109-109)TableRow(113-113)TableHead(112-112)TableBody(110-110)
apps/web/components/dashboard/tags/AllTagsView.tsx (1)
apps/web/components/dashboard/cleanups/UnusedTags.tsx (1)
UnusedTags(72-176)
🔇 Additional comments (3)
apps/web/lib/i18n/locales/en/translation.json (1)
605-629: New delete-all unused tags key looks consistent
tags.delete_all_unused_tags_buttonfits the existing naming pattern and matches the button usage inUnusedTags; no issues from an i18n perspective.apps/web/components/dashboard/tags/TagPill.tsx (1)
12-26: TagPill showCount prop is backward compatible and clearThe new
showCountprop (defaulting totrue) cleanly gates the separator and count without impacting existing callers. This is a good, minimal API extension.Also applies to: 91-96
apps/web/components/dashboard/tags/AllTagsView.tsx (1)
32-36: UnusedTags in AllTagsView ignores search/sort; confirm if that’s intentionalWith the refactor, the top-level
searchQueryandsortByare applied only to human and AI tags; theUnusedTagscomponent uses its ownusePaginatedSearchTagswithnameContains: ""and fixedsortBy: "usage". That means the search box and sort dropdown no longer affect the “Unused tags” section in this view.If that’s not intended, consider threading
searchQueryandsortByintoUnusedTagsas props and using them in its internal query, so all three sections react consistently to the filters.Also applies to: 101-117, 335-335
| function DeleteAllUnusedTags({ numUnusedTags }: { numUnusedTags: number }) { | ||
| const { t } = useTranslation(); | ||
| const { mutate, isPending } = useDeleteUnusedTags({ | ||
| onSuccess: () => { | ||
| toast({ | ||
| description: `Deleted all ${numUnusedTags} unused tags`, | ||
| }); | ||
| }, | ||
| onError: () => { | ||
| toast({ | ||
| description: "Something went wrong", | ||
| variant: "destructive", | ||
| }); | ||
| }, | ||
| }); | ||
| return ( | ||
| <ActionConfirmingDialog | ||
| title={t("tags.delete_all_unused_tags")} | ||
| description={`Are you sure you want to delete the ${numUnusedTags} unused tags?`} | ||
| actionButton={() => ( | ||
| <ActionButton | ||
| variant="destructive" | ||
| loading={isPending} | ||
| onClick={() => mutate()} | ||
| > | ||
| <Trash2 className="mr-2 size-4" /> | ||
| {t("tags.delete_all_unused_tags_button")} | ||
| </ActionButton> | ||
| )} | ||
| > | ||
| <Button variant="destructive" disabled={numUnusedTags == 0}> | ||
| <Trash2 className="mr-2 size-4" /> | ||
| {t("tags.delete_all_unused_tags")} | ||
| </Button> | ||
| </ActionConfirmingDialog> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Delete-all dialog should close on success and text may understate what’s deleted
The delete-all behavior works functionally, but two details are worth tightening:
- The confirmation dialog never closes automatically after a successful mutation because
setDialogOpen(false)isn’t called. This differs fromDeleteTagConfirmationDialogand forces users to hit “Close” manually. You can mirror the other pattern by wiringsetDialogOpeninto the mutation:
- <ActionConfirmingDialog
+ <ActionConfirmingDialog
title={t("tags.delete_all_unused_tags")}
description={`Are you sure you want to delete the ${numUnusedTags} unused tags?`}
- actionButton={() => (
+ actionButton={(setDialogOpen) => (
<ActionButton
variant="destructive"
loading={isPending}
- onClick={() => mutate()}
+ onClick={() =>
+ mutate(undefined, {
+ onSuccess: () => setDialogOpen(false),
+ })
+ }
>
<Trash2 className="mr-2 size-4" />
{t("tags.delete_all_unused_tags_button")}
</ActionButton>
)}
>numUnusedTagsis the count of tags currently loaded in the list, butuseDeleteUnusedTagslikely deletes all unused tags on the backend. The confirmation text and success toast (“Deleted all X unused tags”) may therefore under-report what’s actually being deleted if there are more pages. Consider either:- deriving the true deleted count from the mutation response, or
- rewording the copy to something like “Delete all unused tags?” without echoing the possibly partial client-side count.
| export function UnusedTags({ | ||
| showAsCard = true, | ||
| showCount = true, | ||
| }: UnusedTagsProps) { | ||
| const { t } = useTranslation(); | ||
| const [selectedTag, setSelectedTag] = useState<ZTagBasic | null>(null); | ||
| const isDialogOpen = !!selectedTag; | ||
|
|
||
| const { | ||
| data: unusedTagsData, | ||
| isLoading: isUnusedTagsLoading, | ||
| hasNextPage: hasNextPageUnusedTags, | ||
| fetchNextPage: fetchNextPageUnusedTags, | ||
| isFetchingNextPage: isFetchingNextPageUnusedTags, | ||
| } = usePaginatedSearchTags({ | ||
| nameContains: "", | ||
| sortBy: "usage", | ||
| attachedBy: "none", | ||
| limit: 50, | ||
| }); | ||
|
|
||
| const unusedTags = unusedTagsData?.tags ?? []; | ||
| const numUnusedTags = unusedTags.length; | ||
|
|
||
| const handleOpenDialog = (tag: ZTagBasic) => { | ||
| setSelectedTag(tag); | ||
| }; | ||
|
|
||
| const content = ( | ||
| <> | ||
| {selectedTag && ( | ||
| <DeleteTagConfirmationDialog | ||
| tag={selectedTag} | ||
| open={isDialogOpen} | ||
| setOpen={(o) => { | ||
| if (!o) { | ||
| setSelectedTag(null); | ||
| } | ||
| }} | ||
| /> | ||
| )} | ||
| {isUnusedTagsLoading && unusedTags.length === 0 ? ( | ||
| <div className="flex justify-center py-8"> | ||
| <LoadingSpinner /> | ||
| </div> | ||
| ) : ( | ||
| <> | ||
| {unusedTags.length > 0 && ( | ||
| <div className="flex flex-wrap gap-3"> | ||
| {unusedTags.map((tag) => ( | ||
| <TagPill | ||
| key={tag.id} | ||
| id={tag.id} | ||
| name={tag.name} | ||
| count={showCount ? tag.numBookmarks : 0} | ||
| isDraggable={false} | ||
| onOpenDialog={handleOpenDialog} | ||
| showCount={false} | ||
| /> | ||
| ))} | ||
| </div> | ||
| )} | ||
| {unusedTags.length === 0 && ( | ||
| <p className="py-4 text-center text-sm text-muted-foreground"> | ||
| {t("tags.no_unused_tags")} | ||
| </p> | ||
| )} | ||
| {hasNextPageUnusedTags && ( | ||
| <div className="mt-4"> | ||
| <ActionButton | ||
| variant="secondary" | ||
| onClick={() => fetchNextPageUnusedTags()} | ||
| loading={isFetchingNextPageUnusedTags} | ||
| ignoreDemoMode | ||
| > | ||
| {t("actions.load_more")} | ||
| </ActionButton> | ||
| </div> | ||
| )} | ||
| {numUnusedTags > 0 && ( | ||
| <div className="mt-4"> | ||
| <DeleteAllUnusedTags numUnusedTags={numUnusedTags} /> | ||
| </div> | ||
| )} | ||
| </> | ||
| )} | ||
| </> | ||
| ); | ||
| if (!showAsCard) { | ||
| return content; | ||
| } | ||
|
|
||
| return ( | ||
| <Card> | ||
| <CardHeader> | ||
| <CardTitle className="flex items-center gap-2"> | ||
| <span>{t("tags.unused_tags")}</span> | ||
| <Badge variant="secondary">{numUnusedTags}</Badge> | ||
| </CardTitle> | ||
| <CardDescription>{t("tags.unused_tags_info")}</CardDescription> | ||
| </CardHeader> | ||
| <CardContent className="flex flex-col gap-4">{content}</CardContent> | ||
| </Card> | ||
| ); |
There was a problem hiding this comment.
UnusedTags showCount prop doesn’t currently change the UI
UnusedTagsProps exposes showCount, but inside the TagPill mapping you have:
<TagPill
key={tag.id}
id={tag.id}
name={tag.name}
count={showCount ? tag.numBookmarks : 0}
isDraggable={false}
onOpenDialog={handleOpenDialog}
showCount={false}
/>Because showCount is always forced to false when passed to TagPill, the count value is never actually rendered. From a consumer’s perspective, toggling showCount on UnusedTags has no visible effect.
If you want UnusedTags to truly control whether per-pill counts are shown, a cleaner wiring would be:
- <TagPill
- key={tag.id}
- id={tag.id}
- name={tag.name}
- count={showCount ? tag.numBookmarks : 0}
- isDraggable={false}
- onOpenDialog={handleOpenDialog}
- showCount={false}
- />
+ <TagPill
+ key={tag.id}
+ id={tag.id}
+ name={tag.name}
+ count={tag.numBookmarks}
+ isDraggable={false}
+ onOpenDialog={handleOpenDialog}
+ showCount={showCount}
+ />If the product decision is to never show per-pill counts for unused tags, you could instead drop the showCount prop from UnusedTags altogether and hardcode showCount={false} to avoid a misleading API.
🤖 Prompt for AI Agents
In apps/web/components/dashboard/cleanups/UnusedTags.tsx around lines 72 to 175,
the component accepts a showCount prop but always passes showCount={false} into
TagPill so the per-tag count never renders; change the TagPill usage to forward
the component's showCount (i.e., showCount={showCount}) so the count prop is
honored, or if the decision is to never display counts remove the showCount prop
from UnusedTags and hardcode TagPill to showCount={false} and update the
component signature and usages accordingly.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/components/dashboard/cleanups/UnusedTags.tsx (2)
44-57: Dialog should close automatically on successful deletion.The
actionButtoncallback receivessetDialogOpenas a parameter (perActionConfirmingDialog's signature), but it's not being used here. After a successful mutation, users must manually close the dialog.<ActionConfirmingDialog title={t("tags.delete_all_unused_tags")} description={`Are you sure you want to delete the ${numUnusedTags} unused tags?`} - actionButton={() => ( + actionButton={(setDialogOpen) => ( <ActionButton variant="destructive" loading={isPending} - onClick={() => mutate()} + onClick={() => + mutate(undefined, { + onSuccess: () => setDialogOpen(false), + }) + } > <Trash2 className="mr-2 size-4" /> {t("tags.delete_all_unused_tags_button")} </ActionButton> )} >
130-138:showCountprop is not forwarded toTagPill.The
UnusedTagscomponent accepts ashowCountprop (defaulting totrue), butTagPillalways receivesshowCount={false}. This makes the prop ineffective—toggling it has no visible effect.Either forward the prop consistently:
<TagPill key={tag.id} id={tag.id} name={tag.name} - count={showCount ? tag.numBookmarks : 0} + count={tag.numBookmarks} isDraggable={false} onOpenDialog={handleOpenDialog} - showCount={false} + showCount={showCount} />Or remove the unused
showCountprop fromUnusedTagsPropsif the design intent is to never show counts for unused tags.
🧹 Nitpick comments (1)
apps/web/components/dashboard/cleanups/UnusedTags.tsx (1)
93-94: Consider displaying total count instead of loaded count.
numUnusedTagsreflects only the currently loaded tags (up to 50 per page), not the total unused tags on the server. The badge and delete-all confirmation may show a lower number than what actually exists if pagination is involved. Consider fetching or deriving a total count from the API response if available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/dashboard/cleanups/page.tsx(1 hunks)apps/web/components/dashboard/cleanups/Cleanups.tsx(1 hunks)apps/web/components/dashboard/cleanups/UnusedTags.tsx(1 hunks)apps/web/components/dashboard/tags/AllTagsView.tsx(3 hunks)apps/web/lib/i18n/locales/en/translation.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/components/dashboard/cleanups/Cleanups.tsx
- apps/web/app/dashboard/cleanups/page.tsx
- apps/web/lib/i18n/locales/en/translation.json
- apps/web/components/dashboard/tags/AllTagsView.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript for type safety in all source files
Files:
apps/web/components/dashboard/cleanups/UnusedTags.tsx
**/*.{ts,tsx,js,jsx,json,css,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier according to project standards
Files:
apps/web/components/dashboard/cleanups/UnusedTags.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Lint code using oxlint and fix issues with
pnpm lint:fix
Files:
apps/web/components/dashboard/cleanups/UnusedTags.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Use Tailwind CSS for styling in the web application
Use Next.js for building the main web application
Files:
apps/web/components/dashboard/cleanups/UnusedTags.tsx
🧬 Code graph analysis (1)
apps/web/components/dashboard/cleanups/UnusedTags.tsx (6)
packages/shared-react/hooks/tags.ts (2)
useDeleteUnusedTags(106-118)usePaginatedSearchTags(7-18)apps/web/components/ui/action-confirming-dialog.tsx (1)
ActionConfirmingDialog(15-55)packages/shared/types/tags.ts (1)
ZTagBasic(42-42)apps/web/components/ui/card.tsx (5)
Card(80-80)CardHeader(81-81)CardTitle(83-83)CardDescription(84-84)CardContent(85-85)apps/web/components/dashboard/tags/DeleteTagConfirmationDialog.tsx (1)
DeleteTagConfirmationDialog(8-58)apps/web/components/dashboard/tags/TagPill.tsx (1)
TagPill(12-128)
🔇 Additional comments (4)
apps/web/components/dashboard/cleanups/UnusedTags.tsx (4)
1-28: LGTM!Imports are well-organized with proper type-only import for
ZTagBasicand appropriate separation of UI components, hooks, and local modules.
110-120: LGTM!The controlled dialog pattern with
selectedTagstate is clean. SettingselectedTagtonullwhen the dialog closes properly resets the state.
121-146: LGTM!Loading and empty states are properly handled. The spinner shows only during initial load (when no data exists yet), and the empty message uses proper i18n.
147-163: LGTM!Pagination is properly implemented with loading state handling. The delete-all button is correctly hidden when there are no unused tags.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| const allTags = [...allHumanTags, ...allAiTags, ...allEmptyTags]; | ||
| const allTags = [...allHumanTags, ...allAiTags]; | ||
| setVisibleTagIds(allTags.map((tag) => tag.id) ?? []); | ||
| return () => { | ||
| setVisibleTagIds([]); |
There was a problem hiding this comment.
The bulk-edit store now only registers human and AI tags (setVisibleTagIds is populated from those arrays), while unused tags are rendered separately via UnusedTags with plain TagPill rows. When bulk edit is enabled or “Select all” is pressed on the All Tags page, unused tags are no longer selectable or included in bulk delete/merge operations—a regression from the previous implementation where unused tags participated in bulk actions.
Useful? React with 👍 / 👎.
|
Thanks a lot for the PR, looks good to me. I did some minor changes, but before merging, we also need to address codex's comment as bulk editing no longer works for unused tags. |
2 participants
Fixes #1747
(Tag | 0)