Migrate tags list to React (part 1) #24707
Conversation
WalkthroughAdds a tags API module (types, infinite browse query, single-tag query, delete mutation) and loosens Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
jonatansberg
force-pushed
the
tags-migration
branch
from
94bce40 to
3fcf3cb
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24707 +/- ##
==========================================
- Coverage 71.45% 71.45% -0.01%
==========================================
Files 1530 1530
Lines 114957 114960 +3
Branches 13840 13841 +1
==========================================
+ Hits 82146 82148 +2
Misses 31809 31809
- Partials 1002 1003 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jonatansberg
force-pushed
the
tags-migration
branch
from
77e4d1c to
a01d594
Compare
jonatansberg
force-pushed
the
tags-migration
branch
from
a01d594 to
b08dc0d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/posts/src/views/Tags/components/TagsList.tsx (3)
111-116: Encode the slug in the posts filter link.Safer for any future slug formats or special chars.
Apply this diff:
- href={`#/posts?tag=${item.slug}`} + href={`#/posts?tag=${encodeURIComponent(item.slug)}`}
110-121: Pluralization for “post(s)”.Minor UX polish: use singular “post” for count 1.
Example tweak:
const postsCount = item.count?.posts ?? 0; const postsLabel = `${formatNumber(postsCount)} ${postsCount === 1 ? 'post' : 'posts'}`; ... {postsCount > 0 ? ( <a ...>{postsLabel}</a> ) : ( <span className="text-muted-foreground">0 posts</span> )}
28-36: Placeholder row spans only one cell; consider colSpan for large screens.On
lgwhere table semantics apply, a singletdin atrmay cause layout jumps. SettingcolSpan={4}on the placeholder cell will align with the 4-column header and reduce jitter.Example:
- <TableCell className="relative z-10 h-24 animate-pulse"> + <TableCell className="relative z-10 h-24 animate-pulse" colSpan={4}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/posts/src/views/Tags/components/TagsHeader.tsx(1 hunks)apps/posts/src/views/Tags/components/TagsList.tsx(1 hunks)ghost/core/core/shared/labs.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/posts/src/views/Tags/components/TagsHeader.tsx
- ghost/core/core/shared/labs.js
🧰 Additional context used
🧬 Code graph analysis (1)
apps/posts/src/views/Tags/components/TagsList.tsx (2)
apps/admin-x-framework/src/api/tags.ts (1)
Tag(9-34)apps/posts/src/views/Tags/components/VirtualTable/useInfiniteVirtualScroll.tsx (1)
useInfiniteVirtualScroll(5-70)
🪛 Biome (2.1.2)
apps/posts/src/views/Tags/components/TagsList.tsx
[error] 85-85: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 89-92: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: E2E tests
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
🔇 Additional comments (2)
apps/posts/src/views/Tags/components/TagsList.tsx (2)
93-103: Row-wide clickable overlay can swallow interactions; verify layering and pointer events.The title link uses
before:absolute before:inset-0 before:z-10which effectively makes the entire row clickable. That can:
- Intercept clicks intended for other controls (e.g., the pencil button).
- Create confusing focus/hover behavior.
Please verify with keyboard and pointer:
- Tab to the pencil control and activate it.
- Click the pencil control; ensure it’s not blocked by the overlay.
- Confirm that clicking empty space in the row opens the tag as intended.
If it’s blocking, either:
- Move the overlay to only cover the first cell (make the anchor
relativeand avoid anchoring to the row), or- Keep the row-wide overlay but ensure other interactive elements are layered above it (e.g.,
z-20) and, if possible, setbefore:pointer-events-noneon the overlay so it doesn’t intercept clicks.Also applies to: 110-118, 124-133
40-52: Overall structure looks solid.Good use of virtualization boundaries, responsive table header, and Shade components. The mapping of
itemsto visible rows and use ofspaceBefore/spaceAfteris clear and maintainable.Also applies to: 63-77
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/posts/src/views/Tags/components/TagsList.tsx (4)
53-65: Ref type now matches the actual element. LGTM.Thanks for fixing the earlier mismatch between
HTMLTableElementand<div>. The currentuseRef<HTMLDivElement>(null)aligns with the<div ref={parentRef}>.
80-92: Fix missing React keys and avoid passing key via spread (Biome error).Explicitly set a stable key on each mapped element and pass ref/data-index explicitly. Don’t pass key via spread. This also clears Biome’s errors on Lines 85 and 89–92.
Apply this diff:
- {visibleItems.map(({virtualItem, item, props}) => { - const shouldRenderPlaceholder = - virtualItem.index > items.length - 1; - - if (shouldRenderPlaceholder) { - return <PlaceholderRow {...props} />; - } - - return ( - <TableRow - {...props} - className="relative grid w-full grid-cols-[1fr_5rem] items-center gap-x-4 p-2 md:grid-cols-[1fr_auto_5rem] lg:table-row lg:p-0" - > + {visibleItems.map(({virtualItem, item, props}) => { + const {key: _key, ref, ['data-index']: dataIndex, ...rowProps} = props; + const shouldRenderPlaceholder = + virtualItem.index > items.length - 1; + + if (shouldRenderPlaceholder) { + return ( + <PlaceholderRow + key={virtualItem.key} + ref={ref} + data-index={virtualItem.index} + {...rowProps} + /> + ); + } + + return ( + <TableRow + key={virtualItem.key} + ref={ref} + data-index={virtualItem.index} + {...rowProps} + className="relative grid w-full grid-cols-[1fr_5rem] items-center gap-x-4 p-2 md:grid-cols-[1fr_auto_5rem] lg:table-row lg:p-0" + >
95-95: Prevent row-wide overlay from occluding other interactive controls.The title link uses a ::before absolute overlay positioned against the row (because the row is relative). This can intercept clicks on the posts link and the edit button.
Constrain the overlay to the link’s own box and disable its pointer events:
- <a - className="block truncate pb-1 text-lg font-medium before:absolute before:inset-0 before:z-10" + <a + className="relative block truncate pb-1 text-lg font-medium before:absolute before:inset-0 before:z-10 before:pointer-events-none" href={`#/tags/${item.slug}`} >
124-133: Make the action button accessible and ensure it’s clickable above the overlay.The button is aria-hidden and unfocusable (tabIndex -1); also it may sit under the ::before overlay. Provide an accessible name and raise stacking.
Apply this diff:
- <TableCell className="col-start-2 col-end-2 row-start-1 row-end-3 p-0 md:col-start-3 md:col-end-3 lg:table-cell lg:p-4"> + <TableCell className="relative z-20 col-start-2 col-end-2 row-start-1 row-end-3 p-0 md:col-start-3 md:col-end-3 lg:table-cell lg:p-4"> <Button - aria-hidden="true" - className="w-12" - size="icon" - tabIndex={-1} - variant="outline" + aria-label={`Edit ${item.name}`} + className="w-12" + size="icon" + variant="outline" > <LucideIcon.Pencil /> </Button> </TableCell>
🧹 Nitpick comments (4)
apps/posts/src/views/Tags/components/TagsList.tsx (4)
96-96: Encode slug in URL.Safer if slugs contain special characters.
- href={`#/tags/${item.slug}`} + href={`#/tags/${encodeURIComponent(item.slug)}`}
113-114: Encode slug in filtered posts URL.Prevents malformed URLs with non-alphanumeric slugs.
- href={`#/posts?tag=${item.slug}`} + href={`#/posts?tag=${encodeURIComponent(item.slug)}`}
16-20: Use Shade’s TableRow/TableCell for spacer rows for consistency.Keeps styling/theming consistent and avoids mixing raw tags with design-system components.
-const SpacerRow = ({height}: { height: number }) => ( - <tr className="flex lg:table-row"> - <td className="flex lg:table-cell" style={{height}} /> - </tr> -); +const SpacerRow = ({height}: {height: number}) => ( + <TableRow className="flex lg:table-row"> + <TableCell className="flex lg:table-cell" style={{height}} /> + </TableCell> +);
22-31: Tighten PlaceholderRow typing for better DX.Type its props from TableRow to catch invalid attrs at compile time. Also matches the ref target.
-import {forwardRef, useRef} from 'react'; +import {forwardRef, useRef, type ComponentPropsWithoutRef} from 'react'; @@ -// TODO: Remove forwardRef once we have upgraded to React 19 -const PlaceholderRow = forwardRef<HTMLTableRowElement>(function PlaceholderRow( - props, - ref -) { +// TODO: Revisit once on React 19 if ref handling changes +const PlaceholderRow = forwardRef< + HTMLTableRowElement, + ComponentPropsWithoutRef<typeof TableRow> +>(function PlaceholderRow(props, ref) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/posts/src/views/Tags/components/TagsList.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/posts/src/views/Tags/components/TagsList.tsx (2)
apps/admin-x-framework/src/api/tags.ts (1)
Tag(9-34)apps/posts/src/views/Tags/components/VirtualTable/useInfiniteVirtualScroll.tsx (1)
useInfiniteVirtualScroll(5-70)
🪛 Biome (2.1.2)
apps/posts/src/views/Tags/components/TagsList.tsx
[error] 85-85: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 89-92: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
There was a problem hiding this comment.
Let's try to stick to Lucide Icons wherever it's possible. In this case https://lucide.dev/icons/tag works fine.
There was a problem hiding this comment.
Seems like there is a tags icon as well The new version of that is very similar to the old one. We are on an older version of Lucide tho that has a different design for the same icon.
There was a problem hiding this comment.
This is what the current (i.e. the old Lucide) tags icon looks like.
There was a problem hiding this comment.
@jonatansberg In this context, let's use the EmptyIndicator component from Shade
There was a problem hiding this comment.
I'l make the switch! I must have missed that when looking through the existing components. I'll take the lack of comments re the icon as a silent approval unless you say something else :)
There was a problem hiding this comment.
@peterzimon Actually the EmptyIndicator isn't a perfect fit. It requires a title, a description, actions and an icon. Here there is no description.
I could update the EmptyState to make the description optional, or we could add one, or we keep it as it is. What would you prefer? I don't have a strong preference.
There was a problem hiding this comment.
Let's make the description optional 👍
The old implementation would loop around and fetch page 1 if there are no more pages.
jonatansberg
force-pushed
the
tags-migration
branch
from
de10ec6 to
4761c79
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/posts/src/views/PostAnalytics/Newsletter/Newsletter.tsx (1)
146-156: Use trimmedUrl when updating to prevent saving whitespace-broken links
trimmedUrlis validated buteditedUrl(untrimmed) is sent toeditLinks, allowing stray spaces to be saved.Apply:
- editLinks({ - originalUrl: link.link.originalTo, - editedUrl: editedUrl, - postId: postId - }, { + editLinks({ + originalUrl: link.link.originalTo, + editedUrl: trimmedUrl, + postId: postId + }, {
♻️ Duplicate comments (2)
apps/posts/src/views/Tags/components/TagsList.tsx (2)
80-87: Add explicit React keys and don’t pass key via spread (fixes Biome error).Provide
key,ref, anddata-indexexplicitly for both PlaceholderRow and TableRow; avoid spreadingkey.- {visibleItems.map(({virtualItem, item, props}) => { + {visibleItems.map(({virtualItem, item, props}) => { const shouldRenderPlaceholder = virtualItem.index > items.length - 1; if (shouldRenderPlaceholder) { - return <PlaceholderRow {...props} />; + return ( + <PlaceholderRow + key={virtualItem.key} + ref={props.ref} + data-index={virtualItem.index} + /> + ); } - return ( - <TableRow - {...props} - className="relative grid w-full grid-cols-[1fr_5rem] items-center gap-x-4 p-2 md:grid-cols-[1fr_auto_5rem] lg:table-row lg:p-0" - > + const postsCount = item?.count?.posts ?? 0; + return ( + <TableRow + key={virtualItem.key} + ref={props.ref} + data-index={virtualItem.index} + className="relative grid w-full grid-cols-[1fr_5rem] items-center gap-x-4 p-2 md:grid-cols-[1fr_auto_5rem] lg:table-row lg:p-0" + >Also applies to: 88-92
93-103: Fix row-wide overlay occluding actions; restore button accessibility.Make the title link’s overlay relative to the link only; raise action cell z-index and remove
aria-hidden/tabIndex={-1}.- <TableCell className="static col-start-1 col-end-1 row-start-1 row-end-1 flex min-w-0 flex-col p-0 lg:table-cell lg:w-1/2 lg:p-4 xl:w-3/5"> + <TableCell className="static col-start-1 col-end-1 row-start-1 row-end-1 flex min-w-0 flex-col p-0 lg:table-cell lg:w-1/2 lg:p-4 xl:w-3/5"> <a - className="block truncate pb-1 text-lg font-medium before:absolute before:inset-0 before:z-10" - href={`#/tags/${item.slug}`} + className="relative block truncate pb-1 text-lg font-medium before:absolute before:inset-0 before:z-10" + href={`#/tags/${encodeURIComponent(item.slug)}`} > {item.name} </a>- <TableCell className="col-start-2 col-end-2 row-start-1 row-end-3 p-0 md:col-start-3 md:col-end-3 lg:table-cell lg:p-4"> + <TableCell className="relative z-20 col-start-2 col-end-2 row-start-1 row-end-3 p-0 md:col-start-3 md:col-end-3 lg:table-cell lg:p-4"> <Button - aria-hidden="true" - className="w-12" - size="icon" - tabIndex={-1} - variant="outline" + aria-label={`Edit ${item.name}`} + className="w-12" + size="icon" + variant="outline" > <LucideIcon.Pencil /> </Button> </TableCell>Also applies to: 124-133
🧹 Nitpick comments (3)
apps/posts/src/views/PostAnalytics/Newsletter/Newsletter.tsx (1)
363-363: Remove stray “$” from classNameTypo adds an invalid utility class.
- <div className={`$ mx-auto grid grid-cols-1 items-center justify-center gap-4 transition-all md:gap-0 ${chartHeaderClass === 'grid-cols-2' && 'md:grid-cols-2'} ${chartHeaderClass === 'grid-cols-3' && 'md:grid-cols-3'}`}> + <div className={`mx-auto grid grid-cols-1 items-center justify-center gap-4 transition-all md:gap-0 ${chartHeaderClass === 'grid-cols-2' && 'md:grid-cols-2'} ${chartHeaderClass === 'grid-cols-3' && 'md:grid-cols-3'}`}>apps/posts/src/views/Tags/components/TagsList.tsx (2)
109-121: Pluralize post count and encode the tag query.Use singular “post” for 1 and ensure slug is URL-safe.
- <TableCell className="col-start-1 col-end-1 row-start-3 row-end-3 flex p-0 md:col-start-2 md:col-end-2 md:row-start-1 md:row-end-3 lg:table-cell lg:p-4"> - {item.count?.posts ? ( - <a - className="relative z-10 -m-4 inline-block p-4 hover:underline" - href={`#/posts?tag=${item.slug}`} - > - {formatNumber(item.count?.posts)}{' '} - posts - </a> - ) : ( - <span className="text-muted-foreground"> - 0 posts - </span> - )} - </TableCell> + <TableCell className="col-start-1 col-end-1 row-start-3 row-end-3 flex p-0 md:col-start-2 md:col-end-2 md:row-start-1 md:row-end-3 lg:table-cell lg:p-4"> + {postsCount > 0 ? ( + <a + className="relative z-20 -m-4 inline-block p-4 hover:underline" + href={`#/posts?tag=${encodeURIComponent(item.slug)}`} + aria-label={`View ${formatNumber(postsCount)} ${postsCount === 1 ? 'post' : 'posts'} with tag ${item.name}`} + > + {formatNumber(postsCount)} {postsCount === 1 ? 'post' : 'posts'} + </a> + ) : ( + <span className="text-muted-foreground">0 posts</span> + )} + </TableCell>
16-20: Optional: use TableRow/TableCell for spacer for consistency.Keeps semantics consistent with Shade components.
-const SpacerRow = ({height}: { height: number }) => ( - <tr className="flex lg:table-row"> - <td className="flex lg:table-cell" style={{height}} /> - </tr> -); +const SpacerRow = ({height}: { height: number }) => ( + <TableRow className="flex lg:table-row"> + <TableCell className="flex lg:table-cell" style={{height}} /> + </TableRow> +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
ghost/core/test/e2e-api/admin/__snapshots__/config.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (23)
apps/admin-x-framework/src/api/tags.ts(1 hunks)apps/admin-x-framework/src/utils/api/hooks.ts(1 hunks)apps/admin-x-settings/src/components/settings/advanced/labs/PrivateFeatures.tsx(1 hunks)apps/posts/package.json(1 hunks)apps/posts/src/routes.tsx(3 hunks)apps/posts/src/views/PostAnalytics/Newsletter/Newsletter.tsx(1 hunks)apps/posts/src/views/PostAnalytics/Overview/Overview.tsx(2 hunks)apps/posts/src/views/PostAnalytics/Overview/components/NewsletterOverview.tsx(2 hunks)apps/posts/src/views/PostAnalytics/Overview/components/WebOverview.tsx(1 hunks)apps/posts/src/views/PostAnalytics/Web/Web.tsx(1 hunks)apps/posts/src/views/PostAnalytics/components/PostAnalyticsHeader.tsx(1 hunks)apps/posts/src/views/PostAnalytics/components/Sidebar.tsx(1 hunks)apps/posts/src/views/Tags/Tags.tsx(1 hunks)apps/posts/src/views/Tags/components/TagsContent.tsx(1 hunks)apps/posts/src/views/Tags/components/TagsHeader.tsx(1 hunks)apps/posts/src/views/Tags/components/TagsLayout.tsx(1 hunks)apps/posts/src/views/Tags/components/TagsList.tsx(1 hunks)apps/posts/src/views/Tags/components/VirtualTable/getScrollParent.tsx(1 hunks)apps/posts/src/views/Tags/components/VirtualTable/useInfiniteVirtualScroll.tsx(1 hunks)ghost/admin/app/routes/tags.js(1 hunks)ghost/admin/app/services/feature.js(1 hunks)ghost/admin/app/templates/tags-x.hbs(1 hunks)ghost/core/core/shared/labs.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- apps/posts/src/views/PostAnalytics/Web/Web.tsx
- apps/posts/src/views/Tags/components/TagsContent.tsx
- apps/posts/src/views/Tags/components/VirtualTable/getScrollParent.tsx
- ghost/admin/app/routes/tags.js
- apps/posts/src/views/Tags/components/TagsHeader.tsx
- apps/posts/src/views/PostAnalytics/Overview/components/WebOverview.tsx
- apps/posts/src/views/Tags/Tags.tsx
- apps/posts/src/views/Tags/components/TagsLayout.tsx
- apps/admin-x-settings/src/components/settings/advanced/labs/PrivateFeatures.tsx
- ghost/admin/app/services/feature.js
- apps/posts/src/views/Tags/components/VirtualTable/useInfiniteVirtualScroll.tsx
- apps/posts/src/views/PostAnalytics/Overview/components/NewsletterOverview.tsx
- apps/admin-x-framework/src/api/tags.ts
- ghost/admin/app/templates/tags-x.hbs
- apps/posts/src/views/PostAnalytics/components/PostAnalyticsHeader.tsx
- apps/posts/src/routes.tsx
- ghost/core/core/shared/labs.js
- apps/posts/src/views/PostAnalytics/Overview/Overview.tsx
- apps/posts/package.json
- apps/admin-x-framework/src/utils/api/hooks.ts
- apps/posts/src/views/PostAnalytics/components/Sidebar.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/posts/src/views/Tags/components/TagsList.tsx (2)
apps/admin-x-framework/src/api/tags.ts (1)
Tag(9-34)apps/posts/src/views/Tags/components/VirtualTable/useInfiniteVirtualScroll.tsx (1)
useInfiniteVirtualScroll(5-70)
🪛 Biome (2.1.2)
apps/posts/src/views/Tags/components/TagsList.tsx
[error] 85-85: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 89-92: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
🔇 Additional comments (2)
apps/posts/src/views/PostAnalytics/Newsletter/Newsletter.tsx (1)
83-88: Update lingering old analytics route in PostAnalyticsHeaderThe search results reveal an outdated cross-app navigation still pointing to
/analytics/in the breadcrumb:• apps/posts/src/views/PostAnalytics/components/PostAnalyticsHeader.tsx:91
- onClick={() => navigate('/analytics/', {crossApp: true})} + onClick={() => navigate('/posts/analytics/', {crossApp: true})}Please update this link to use the new
/posts/analytics/prefix so it routes correctly and avoids 404s.Likely an incorrect or invalid review comment.
apps/posts/src/views/Tags/components/TagsList.tsx (1)
53-61: Ref type now matches the DOM element. LGTM.
useRef<HTMLDivElement>(null)aligns with the<div ref={parentRef}>and the hook’sRefObject<HTMLElement>.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/posts/src/views/Tags/components/TagsList.tsx (3)
53-61: Ref type fix looks good.Switch to HTMLDivElement aligns with the actual container element and the hook’s HTMLElement constraint. LGTM.
123-133: Make the action button accessible and clickable; lift above overlay.Remove aria-hidden/tabIndex, add an accessible name, and raise stacking to avoid the overlay blocking clicks.
- <TableCell className="col-start-2 col-end-2 row-start-1 row-end-3 p-0 md:col-start-3 md:col-end-3 lg:table-cell lg:p-4"> + <TableCell className="relative z-20 col-start-2 col-end-2 row-start-1 row-end-3 p-0 md:col-start-3 md:col-end-3 lg:table-cell lg:p-4"> <Button - aria-hidden="true" - className="w-12" + aria-label={`Edit ${item.name}`} + className="w-12" size="icon" - tabIndex={-1} variant="outline" > <LucideIcon.Pencil /> </Button> </TableCell>
80-92: Make keys, ref, and data-index explicit (fixes Biome errors).Don’t pass key via spread; set key/ref/data-index explicitly for both PlaceholderRow and TableRow.
@@ - if (shouldRenderPlaceholder) { - return <PlaceholderRow {...props} />; - } + if (shouldRenderPlaceholder) { + return ( + <PlaceholderRow + key={virtualItem.key} + ref={props.ref} + data-index={virtualItem.index} + /> + ); + } @@ - <TableRow - {...props} - className="relative grid w-full grid-cols-[1fr_5rem] items-center gap-x-4 p-2 md:grid-cols-[1fr_auto_5rem] lg:table-row lg:p-0" - > + <TableRow + key={virtualItem.key} + ref={props.ref} + data-index={virtualItem.index} + className="relative grid w-full grid-cols-[1fr_5rem] items-center gap-x-4 p-2 md:grid-cols-[1fr_auto_5rem] lg:table-row lg:p-0" + >
🧹 Nitpick comments (1)
apps/posts/src/views/Tags/components/TagsList.tsx (1)
96-114: URL-encode slugs in hrefs.Avoid issues with special characters in slugs.
- href={`#/tags/${item.slug}`} + href={`#/tags/${encodeURIComponent(item.slug)}`} @@ - href={`#/posts?tag=${item.slug}`} + href={`#/posts?tag=${encodeURIComponent(item.slug)}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/posts/src/views/Tags/components/TagsList.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/posts/src/views/Tags/components/TagsList.tsx (2)
apps/admin-x-framework/src/api/tags.ts (1)
Tag(9-34)apps/posts/src/views/Tags/components/VirtualTable/useInfiniteVirtualScroll.tsx (1)
useInfiniteVirtualScroll(5-70)
🪛 Biome (2.1.2)
apps/posts/src/views/Tags/components/TagsList.tsx
[error] 85-85: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 89-92: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Comments-UI tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Signup-form tests
- GitHub Check: ActivityPub tests
- GitHub Check: E2E tests
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Lint
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (1)
apps/posts/src/views/Tags/components/TagsList.tsx (1)
109-121: Pluralization LGTM.Inline singular/plural handling for “post(s)” matches the design guidance discussed.
This is the first in a series of PRs to migrate the various tag related screens from Ember to React. The new tags list is available behind the
tagsXfeature flag.Once the new Playwright test setup is merged I'll follow up with a separate PR for tests so that we can go ahead and remove the feature flag for the list view.
The new tags list screen uses the Shade design system components and should be more or less a 1:1 port of the old Ember version. There are some minor cosmetic differences and an improved responsive layout.