Added delete action and dialog to ActivityPub feed item #22356
Conversation
WalkthroughThe pull request refactors the menu handling within the Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/admin-x-activitypub/src/components/feed/FeedItem.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/admin-x-activitypub".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/admin-x-activitypub/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/shade/src/components/ui/alert-dialog.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/shade".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/shade/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. apps/shade/src/components/ui/popover.tsxOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/shade".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/shade/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🪛 Biome (1.9.4)apps/admin-x-activitypub/src/components/feed/FeedItem.tsx[error] 418-418: Avoid passing content using the dangerouslySetInnerHTML prop. Setting content using code can expose users to cross-site scripting (XSS) attacks (lint/security/noDangerouslySetInnerHtml) 🔇 Additional comments (6)
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx (1)
70-72: Consider revising the confirmation message tone.The confirmation message ends with a casual "We warned you, k?" which may not be appropriate for all users or contexts. Consider using a more professional tone for the alert dialog.
- You're about to delete this post. This is permanent! We warned you, k? + You're about to delete this post. This action is permanent and cannot be undone.apps/shade/src/components/ui/alert-dialog.tsx (1)
13-26: Well-implemented overlay with GPU acceleration.The overlay component:
- Correctly uses forwardRef for proper ref handling
- Implements nice visual effects with backdrop blur
- Uses GPU acceleration with
transform-gpufor better performance- Has appropriate animations for state transitions
However, there's a small inconsistency in the blur settings:
- 'fixed inset-0 z-50 bg-black/30 backdrop-blur-none transform-gpu data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=open]:backdrop-blur-[3px]', + 'fixed inset-0 z-50 bg-black/30 transform-gpu data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:backdrop-blur-none data-[state=open]:backdrop-blur-[3px]',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(8 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx(1 hunks)apps/shade/package.json(2 hunks)apps/shade/src/components/ui/alert-dialog.tsx(1 hunks)apps/shade/src/components/ui/dialog.tsx(1 hunks)apps/shade/src/components/ui/popover.tsx(2 hunks)apps/shade/src/index.ts(1 hunks)apps/shade/styles.css(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/shade/src/components/ui/dialog.tsx
- apps/shade/styles.css
🧰 Additional context used
🪛 Biome (1.9.4)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
[error] 424-424: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (26)
apps/shade/src/index.ts (1)
2-2: LGTM: Successfully exported the new AlertDialog component.The new export line allows the AlertDialog component to be used throughout the project, which is necessary for implementing the confirmation dialog mentioned in the PR objectives.
apps/shade/package.json (2)
68-68: LGTM: Added Radix UI AlertDialog dependency.This dependency is correctly added to support the new AlertDialog component for the delete confirmation functionality.
77-77: LGTM: Updated @radix-ui/react-slot dependency.The version update from
1.1.1to^1.1.2ensures compatibility with the other Radix UI components being used.apps/shade/src/components/ui/popover.tsx (2)
12-12: LGTM: Added PopoverClose component.The PopoverClose component from Radix UI is correctly defined, which will enable dismissing popovers programmatically.
31-31: LGTM: Updated exports to include PopoverClose.Properly updated the export statement to include the new PopoverClose component, making it available for consumption by other components.
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (8)
1-1: LGTM: Imported the new FeedItemMenu component.This aligns with the PR objectives of abstracting menu behavior into a separate component.
4-4: LGTM: Renamed Button import to ButtonX.The import was updated to reflect the component rename mentioned in the PR summary.
241-242: LGTM: Updated to use ButtonX component.Correctly uses the renamed ButtonX component in the UserMenuTrigger.
285-289: LGTM: Implemented FeedItemMenu in feed layout.Successfully replaced the previously hardcoded menu implementation with the new FeedItemMenu component, passing appropriate props for the feed layout.
377-381: LGTM: Implemented FeedItemMenu in modal layout.Successfully incorporated the FeedItemMenu component in the modal layout view.
414-418: LGTM: Implemented FeedItemMenu in reply layout.Successfully incorporated the FeedItemMenu component in the reply layout view.
426-426: LGTM: Updated Button to ButtonX in render output.Consistently used the renamed ButtonX component throughout the component.
509-513: LGTM: Implemented FeedItemMenu in inbox layout.Successfully incorporated the FeedItemMenu component in the inbox layout view.
apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx (6)
1-17: Good component organization with comprehensive imports.The imports are well-organized, importing all necessary components from the
@tryghost/shadeUI library, including the new AlertDialog components.
19-23: Clean interface definition with appropriate props.The
FeedItemMenuPropsinterface is well-defined with the necessary properties:
trigger: Allows for flexible UI triggering elementonCopyLinkandonDelete: Clear callback functions for the actionsThis is a good example of component API design with clear separation of concerns.
30-38: Good event handling with stopPropagation.Both event handlers correctly prevent event bubbling by calling
e.stopPropagation()before executing their respective callbacks, which prevents unintended side effects when these actions are triggered.
43-45: Event propagation is properly handled in the trigger.Good use of
stopPropagationto prevent the click event from affecting parent elements when the menu is triggered.
46-66: Well-structured popover menu with clear actions.The popover content is well organized with:
- Proper alignment using
align='end'- Clear action buttons with appropriate styling
- Properly nested components to handle both the popover and alert dialog interactions
74-84: Well-implemented alert dialog footer with appropriate button styling.The footer properly implements:
- Cancel and Delete buttons with correct positioning
- Appropriate event handling for the Delete button
- Destructive styling for the Delete button using buttonVariants
- Event propagation control
apps/shade/src/components/ui/alert-dialog.tsx (7)
1-6: Good imports and dependency setup.The imports are well organized, correctly importing from Radix UI for the alert dialog primitives and local utility functions.
7-11: Clean re-export of Radix primitives.Exporting the primitives directly makes the implementation cleaner and more maintainable.
48-74: Good layout components for the dialog.The header and footer components:
- Have clear, semantic names
- Use appropriate flex layouts that adapt to different screen sizes
- Allow for customization via className prop
- Handle responsive design (column on mobile, row on larger screens for footer)
76-99: Well-implemented Title and Description components.Both components:
- Correctly use forwardRef
- Have appropriate default styling
- Pass through additional props
- Set display names for better debugging
101-127: Action buttons with appropriate styling.The Action and Cancel buttons:
- Use appropriate button variants (primary and outline)
- Handle responsiveness with mobile-specific margins
- Allow for className overrides
- Correctly use forwardRef for proper ref handling
129-141: Comprehensive exports for the dialog components.All components are properly exported, making them available for use throughout the application.
28-45:✅ Verification successful
Well-structured dialog content with animations.
The AlertDialogContent component:
- Has proper positioning (centered on screen)
- Includes responsive styling with SM breakpoint
- Uses appropriate animations for transitions
- Properly stops event propagation on the overlay
However, consider examining the need for the extra
shadediv:
🏁 Script executed:
#!/bin/bash # Check if the 'shade' class is used elsewhere or defined in the codebase echo "Checking for the shade class definition..." rg -A 2 -B 2 "\.shade\s*{" --type css --type scss --type less echo "Checking for other uses of the shade className..." rg "className=.*shade" --type tsx --type jsxLength of output: 428
The previous commands ran into regex errors and unrecognized file type issues. I've updated the script to escape special characters properly and use glob patterns for TSX/JSX files. Please run these new commands and verify whether the extra
<div className="shade">is needed based on its definition or usage in your codebase.Review Note: The component is otherwise well-structured (proper centering, responsive styling, animations, and event handling). Confirm the necessity of the extra
shadediv once you have the output from the above script.
🏁 Script executed:
#!/bin/bash echo "Re-checking for the 'shade' class definition in CSS/SCSS/LESS files..." rg --regexp '\\.shade\\s*{' --type css --type scss --type less echo "Re-checking for the usage of 'shade' in React files (TSX/JSX) using glob patterns..." rg 'className=.*shade' --glob '*.tsx' --glob '*.jsx'Length of output: 1153
AlertDialogContent Component: Confirming Consistency in the 'shade' Wrapper
The component is well-structured—with proper centering, responsive styling, smooth animations, and effective event handling. Our investigation shows that the extra
<div className="shade">wrapper is used consistently across the codebase (e.g., inapps/admin-x-activitypub/src/App.tsx, and multiple instances in theapps/shade/src/components/uifolder such as inalert-dialog.tsx,dialog.tsx,dropdown-menu.tsx, andtooltip.tsx). This indicates that its inclusion is likely intentional to maintain consistent styling or layout behavior.Next Steps:
- Please confirm that the
.shadeclass has the appropriate style definitions in your global or modular CSS. If the styling isn’t defined or applied as intended, consider revisiting its necessity or consolidating the pattern.- If the styles are defined and the extra wrapper meets design requirements, no changes are needed.
| trigger={UserMenuTrigger} | ||
| onCopyLink={handleCopyLink} | ||
| onDelete={handleDelete} | ||
| /> | ||
| </div> | ||
| <div className={`relative z-10 col-start-2 col-end-3 w-full gap-4`}> | ||
| <div className='flex flex-col'> | ||
| {(object.type === 'Article') && renderFeedAttachment(object, layout)} | ||
| {object.name && <Heading className='my-1 text-pretty leading-tight' level={5} data-test-activity-heading>{object.name}</Heading>} | ||
| {(object.preview && object.type === 'Article') ? <div className='line-clamp-3 leading-tight'>{object.preview.content}</div> : <div dangerouslySetInnerHTML={({__html: object.content ?? ''})} className='ap-note-content text-pretty tracking-[-0.006em] text-gray-900 dark:text-gray-600'></div>} |
There was a problem hiding this comment.
Address XSS vulnerability with dangerouslySetInnerHTML.
Using dangerouslySetInnerHTML directly with content from external sources (like ActivityPub objects) exposes users to cross-site scripting (XSS) attacks.
Consider implementing content sanitization before rendering:
-<div dangerouslySetInnerHTML={({__html: object.content ?? ''})} className='ap-note-content text-pretty tracking-[-0.006em] text-gray-900 dark:text-gray-600'></div>
+<div dangerouslySetInnerHTML={({__html: sanitizeHtml(object.content ?? '')})} className='ap-note-content text-pretty tracking-[-0.006em] text-gray-900 dark:text-gray-600'></div>You'll need to import a sanitization library like DOMPurify:
import DOMPurify from 'dompurify';
// Define a sanitization function
const sanitizeHtml = (html) => {
return DOMPurify.sanitize(html, {
ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a', 'p', 'br'],
ALLOWED_ATTR: ['href', 'target', 'rel']
});
};Apply this same pattern to all other instances of dangerouslySetInnerHTML in the component.
🧰 Tools
🪛 Biome (1.9.4)
[error] 424-424: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
| const handleDelete = () => { | ||
|
|
||
| menuItems.push({ | ||
| id: 'copy-link', | ||
| label: 'Copy link to post', | ||
| onClick: handleCopyLink | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Implement the delete handler function.
The handleDelete function is currently empty. To fulfill the PR objective of adding delete functionality to feed items, this function needs to be implemented.
const handleDelete = () => {
+ // API call to delete the feed item
+ // Example implementation:
+ // deleteActivityPubObject(object.id)
+ // .then(() => {
+ // showToast({
+ // title: 'Item deleted successfully',
+ // type: 'success'
+ // });
+ // })
+ // .catch((error) => {
+ // showToast({
+ // title: 'Failed to delete item',
+ // description: error.message,
+ // type: 'error'
+ // });
+ // });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDelete = () => { | |
| menuItems.push({ | |
| id: 'copy-link', | |
| label: 'Copy link to post', | |
| onClick: handleCopyLink | |
| }); | |
| }; | |
| const handleDelete = () => { | |
| // API call to delete the feed item | |
| // Example implementation: | |
| // deleteActivityPubObject(object.id) | |
| // .then(() => { | |
| // showToast({ | |
| // title: 'Item deleted successfully', | |
| // type: 'success' | |
| // }); | |
| // }) | |
| // .catch((error) => { | |
| // showToast({ | |
| // title: 'Failed to delete item', | |
| // description: error.message, | |
| // type: 'error' | |
| // }); | |
| // }); | |
| }; |
| @@ -250,8 +238,8 @@ const FeedItem: React.FC<FeedItemProps> = ({actor, object, layout, type, comment | |||
| // }); | |||
|
|
|||
| const UserMenuTrigger = ( | |||
| <Button | |||
| className={`transition-color relative z-[9998] flex h-[34px] w-[34px] items-center justify-center ${layout === 'feed' ? 'rounded-full' : 'rounded-md'} bg-white hover:bg-gray-100 dark:bg-black dark:hover:bg-gray-950 ${(layout === 'feed' || layout === 'modal') && 'ml-auto'}`} | |||
| <ButtonX | |||
There was a problem hiding this comment.
We should replace and use the Button component from Shade instead of the old system. What's the reason to keep the old one here?
| } from '@tryghost/shade'; | ||
|
|
||
| interface FeedItemMenuProps { | ||
| trigger: React.ReactNode; |
There was a problem hiding this comment.
I really like this pattern of passing the trigger to the dialog! What do you think about instead of passing on a ReactNode component as a prop, so the menu could be used like this:
<FeedItemMenu onCopyLink={...} onDelete={...}>
<Button>Open menu</Button>
</FeedItemMenu>
Does this make sense to you?
minimaluminium
force-pushed
the
ap-feed-item-menu-AP-819
branch
from
fae5eb3 to
dad596d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (1)
228-230:⚠️ Potential issueImplement the delete handler function.
The
handleDeletefunction is currently empty. To fulfill the PR objective of adding delete functionality to feed items, this function needs to be implemented.Consider implementing the delete functionality similar to:
const handleDelete = () => { + // API call to delete the feed item + if (object?.id) { + // Call your delete API here, e.g.: + // deleteActivityPubObject(object.id) + // .then(() => { + // showToast({ + // title: 'Item deleted successfully', + // type: 'success' + // }); + // }) + // .catch((error) => { + // showToast({ + // title: 'Failed to delete item', + // description: error.message, + // type: 'error' + // }); + // }); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(8 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx(1 hunks)apps/shade/package.json(2 hunks)apps/shade/src/components/ui/alert-dialog.tsx(1 hunks)apps/shade/src/components/ui/dialog.tsx(1 hunks)apps/shade/src/components/ui/popover.tsx(2 hunks)apps/shade/src/index.ts(1 hunks)apps/shade/styles.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/shade/src/components/ui/dialog.tsx
- apps/shade/src/components/ui/popover.tsx
- apps/shade/styles.css
- apps/shade/src/index.ts
- apps/shade/package.json
- apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx
- apps/shade/src/components/ui/alert-dialog.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
[error] 424-424: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (6)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (6)
1-1: Good refactoring approach with the new FeedItemMenu component!The extraction of menu functionality into a dedicated
FeedItemMenucomponent is a good architectural decision that improves modularity and code organization.
285-289: Good implementation of the FeedItemMenu component!The refactoring to use a dedicated menu component improves code organization and reusability. The menu component receives the trigger element and callback functions, which is a clean approach.
304-306:⚠️ Potential issueAddress XSS vulnerability with dangerouslySetInnerHTML.
Using
dangerouslySetInnerHTMLdirectly with content from external sources (like ActivityPub objects) exposes users to cross-site scripting (XSS) attacks.Consider implementing content sanitization before rendering:
-<div dangerouslySetInnerHTML={{ - __html: object.content ?? '' -}} ref={contentRef} /> +<div dangerouslySetInnerHTML={{ + __html: sanitizeHtml(object.content ?? '') +}} ref={contentRef} />You'll need to import a sanitization library like DOMPurify:
import DOMPurify from 'dompurify'; // Define a sanitization function const sanitizeHtml = (html) => { return DOMPurify.sanitize(html, { ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a', 'p', 'br'], ALLOWED_ATTR: ['href', 'target', 'rel'] }); };Apply this same pattern to all other instances of
dangerouslySetInnerHTMLin the component (lines 365, 424, 482, 489).🧰 Tools
🪛 Biome (1.9.4)
[error] 304-304: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
377-381: Good implementation of the FeedItemMenu in modal layout!Consistent implementation of the menu component across different layouts improves maintainability.
414-418: Good implementation of the FeedItemMenu in reply layout!Consistent implementation of the menu component across different layouts improves maintainability.
509-513: Good implementation of the FeedItemMenu in inbox layout!Consistent implementation of the menu component across different layouts improves maintainability.
| import React, {useEffect, useRef, useState} from 'react'; | ||
| import {ActorProperties, ObjectProperties} from '@tryghost/admin-x-framework/api/activitypub'; | ||
| import {Button, Heading, Icon, Menu, MenuItem, showToast} from '@tryghost/admin-x-design-system'; | ||
| import {Button as ButtonX, Heading, Icon, showToast} from '@tryghost/admin-x-design-system'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename from Button to ButtonX needs clarification.
The renamed import suggests a transition period or potential confusion with component naming. Per the previous reviewer comment, consider using the Button component from Shade directly instead of using this renamed approach.
🏁 Script executed:
#!/bin/bash
# Check if there's a Button component in the Shade library that should be used instead
find ./apps/shade/src -type f -name "*.tsx" | xargs grep -l "export.*Button"Length of output: 120
Clarify the Button Source and Renaming Approach
The current import aliases the design system’s Button as ButtonX, but our verification confirms that a Button component exists directly in the Shade library (see apps/shade/src/components/ui/button.tsx). Please clarify if the alias is intended for a transitional period or if we should update the code to import the Button component from Shade directly.
| </div> | ||
| <div className={`relative z-10 col-start-2 col-end-3 w-full gap-4`}> | ||
| <div className='flex flex-col'> | ||
| {(object.type === 'Article') && renderFeedAttachment(object, layout)} | ||
| {object.name && <Heading className='my-1 text-pretty leading-tight' level={5} data-test-activity-heading>{object.name}</Heading>} | ||
| {(object.preview && object.type === 'Article') ? <div className='line-clamp-3 leading-tight'>{object.preview.content}</div> : <div dangerouslySetInnerHTML={({__html: object.content ?? ''})} className='ap-note-content text-pretty tracking-[-0.006em] text-gray-900 dark:text-gray-600'></div>} | ||
| {(object.type === 'Note') && renderFeedAttachment(object, layout)} | ||
| {(object.type === 'Article') && <Button | ||
| {(object.type === 'Article') && <ButtonX |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Button component renamed but not replaced.
The PR description indicates that the Menu component was replaced with Popover from Shade, but the Button component wasn't fully replaced with its Shade equivalent as suggested in the previous review.
🏁 Script executed:
#!/bin/bash
# Check if there's a ButtonX component defined in the design system
find ./apps -type f -name "*.tsx" | xargs grep -l "export.*ButtonX"Length of output: 69
Action Required: Replace legacy ButtonX with the new Shade button component
The usage of ButtonX at line 426 in apps/admin-x-activitypub/src/components/feed/FeedItem.tsx remains unchanged. Our earlier verification confirms that there is no export for ButtonX in the design system, which aligns with previous concerns. Please update this instance to use the appropriate Shade button component as indicated in the PR description (similar to the Menu component’s replacement with Shade’s Popover).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (1)
420-426: 🛠️ Refactor suggestionReplace ButtonX with Shade Button component
Per previous reviewer comments and your other changes, you should replace the remaining instance of ButtonX with the Button component from Shade.
-{(object.type === 'Article') && <ButtonX - className={`mt-3 self-start text-gray-900 transition-all hover:opacity-60`} - color='grey' - fullWidth={true} - id='read-more' - label='Read more' - size='md' -/>} +{(object.type === 'Article') && <Button + className="mt-3 self-start text-gray-900 transition-all hover:opacity-60" + fullWidth + id="read-more" + size="md" + variant="secondary" +> + Read more +</Button>}
♻️ Duplicate comments (3)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (3)
228-230:⚠️ Potential issueImplement the delete handler function
The
handleDeletefunction is currently empty. To fulfill the PR objective of adding delete functionality to ActivityPub feed items, this function needs to be implemented.const handleDelete = () => { + // API call to delete the feed item + // Example implementation: + // deleteActivityPubObject(object.id) + // .then(() => { + // showToast({ + // title: 'Item deleted successfully', + // type: 'success' + // }); + // }) + // .catch((error) => { + // showToast({ + // title: 'Failed to delete item', + // description: error.message, + // type: 'error' + // }); + // }); };
418-418:⚠️ Potential issueAddress XSS vulnerability with dangerouslySetInnerHTML
Using
dangerouslySetInnerHTMLdirectly with content from external sources (like ActivityPub objects) exposes users to cross-site scripting (XSS) attacks.Consider implementing content sanitization before rendering:
-<div dangerouslySetInnerHTML={({__html: object.content ?? ''})} className='ap-note-content text-pretty tracking-[-0.006em] text-gray-900 dark:text-gray-600'></div> +<div dangerouslySetInnerHTML={({__html: sanitizeHtml(object.content ?? '')})} className='ap-note-content text-pretty tracking-[-0.006em] text-gray-900 dark:text-gray-600'></div>You'll need to import a sanitization library like DOMPurify:
import DOMPurify from 'dompurify'; // Define a sanitization function const sanitizeHtml = (html) => { return DOMPurify.sanitize(html, { ALLOWED_TAGS: ['b', 'i', 'em', 'strong', 'a', 'p', 'br'], ALLOWED_ATTR: ['href', 'target', 'rel'] }); };Apply this same pattern to all other instances of
dangerouslySetInnerHTMLin the component.🧰 Tools
🪛 Biome (1.9.4)
[error] 418-418: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
4-5: 🛠️ Refactor suggestionButton import inconsistency needs to be resolved
You've imported
Buttonfrom@tryghost/shadebut also keptButton as ButtonXfrom the design system. Per previous reviewer comments, you should fully migrate to using the Shade button component throughout the file.import {Button, LucideIcon, Skeleton} from '@tryghost/shade'; -import {Button as ButtonX, Heading, Icon, showToast} from '@tryghost/admin-x-design-system'; +import {Heading, Icon, showToast} from '@tryghost/admin-x-design-system';
🧹 Nitpick comments (2)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (2)
228-238: Verify if the comment block should be removedThere's a commented block of code explaining menu item creation for delete functionality. Since you've now implemented this functionality in the FeedItemMenu component, consider if this comment should be removed.
- // TODO: If this is your own Note/Article, you should be able to delete it - // menuItems.push({ - // id: 'delete', - // label: 'Delete', - // destructive: true, - // onClick: handleDelete - // });
259-261: Potential UI improvement for interactive elementsYou've adjusted the flex layout here. For better accessibility, consider adding a hover state for interactive elements within this container to provide visual feedback to users.
-<div className='flex min-w-0 items-center gap-3'> +<div className='flex min-w-0 items-center gap-3 group'> <APAvatar author={author} /> -<div className='flex min-w-0 grow flex-col gap-0.5'> +<div className='flex min-w-0 grow flex-col gap-0.5 group-hover:opacity-90 transition-opacity'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
[error] 418-418: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (6)
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (6)
1-1: Good addition of the FeedItemMenu componentThe component import supports the PR objective of adding delete functionality to ActivityPub feed items.
241-243: Good use of Shade Button component for the menu triggerThe menu trigger now uses the Shade Button component with appropriate styling and icon, which is consistent with the PR objective of replacing components with ones from Shade.
279-283: Well-structured implementation of FeedItemMenuThe FeedItemMenu component nicely encapsulates the menu functionality, receiving the trigger element and callback functions as props. This is a cleaner approach than the previous implementation with menuItems array.
371-375: Consistent implementation of FeedItemMenu across different layoutsThe FeedItemMenu is consistently implemented in the modal layout, which is good for maintainability.
408-412: Consistent implementation in reply layoutThe FeedItemMenu is correctly implemented in the reply layout as well.
503-507: Consistent implementation in inbox layoutThe FeedItemMenu is correctly implemented in the inbox layout as well, maintaining consistency across all UI variations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx (4)
20-24: Consider implementing the children pattern as suggested in previous feedback.Based on the previous review comment from peterzimon, consider using React children instead of a trigger prop. This would follow a more common React pattern and make your component usage more intuitive.
You could restructure the interface and component like this:
interface FeedItemMenuProps { - trigger: React.ReactNode; onCopyLink: () => void; onDelete: () => void; + children: React.ReactNode; } -const FeedItemMenu: React.FC<FeedItemMenuProps> = ({ - trigger, - onCopyLink, - onDelete -}) => { +const FeedItemMenu: React.FC<FeedItemMenuProps> = ({ + children, + onCopyLink, + onDelete +}) => {And then in the JSX:
<PopoverTrigger asChild onClick={e => e.stopPropagation()}> - {trigger} + {children} </PopoverTrigger>This would allow more intuitive usage:
<FeedItemMenu onCopyLink={...} onDelete={...}> <Button>Open menu</Button> </FeedItemMenu>
73-78: Consider revising the confirmation message tone.The current message "You're about to delete this post. This is permanent! We warned you, k?" has a casual tone with "k?" that might not match the professional nature of an admin interface.
<AlertDialogTitle>Are you sure you want to delete this post?</AlertDialogTitle> <AlertDialogDescription> - You're about to delete this post. This is permanent! We warned you, k? + This action cannot be undone. The post will be permanently removed from your ActivityPub feed. </AlertDialogDescription>
33-41: Add error handling for callback operations.The component currently doesn't handle any potential errors that might occur during the copy link or delete operations.
Consider adding try/catch blocks to handle potential errors gracefully:
const handleCopyLinkClick = (e: React.MouseEvent<HTMLElement>) => { e.stopPropagation(); - onCopyLink(); + try { + onCopyLink(); + } catch (error) { + console.error('Failed to copy link:', error); + // Optionally show an error notification to the user + } }; const handleDeleteClick = (e: React.MouseEvent<HTMLElement>) => { e.stopPropagation(); - onDelete(); + try { + onDelete(); + } catch (error) { + console.error('Failed to delete post:', error); + // Optionally show an error notification to the user + } };
1-17: Consider grouping related imports for better organization.The import statement is quite lengthy. Consider grouping the imports by their related functionality for better readability.
import { + // Alert Dialog components AlertDialog, AlertDialogAction, AlertDialogCancel, AlertDialogContent, AlertDialogDescription, AlertDialogFooter, AlertDialogHeader, AlertDialogTitle, AlertDialogTrigger, + // Button components Button, + // Popover components Popover, PopoverClose, PopoverContent, PopoverTrigger, buttonVariants } from '@tryghost/shade';Or alternatively, structure them by semantic groups:
import { + // UI Components AlertDialog, Button, Popover, + // Subcomponents AlertDialogAction, AlertDialogCancel, AlertDialogContent, AlertDialogDescription, AlertDialogFooter, AlertDialogHeader, AlertDialogTitle, AlertDialogTrigger, PopoverClose, PopoverContent, PopoverTrigger, + // Utilities buttonVariants } from '@tryghost/shade';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx(1 hunks)
🔇 Additional comments (4)
apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx (4)
46-48: LGTM! Smart implementation to prevent event bubbling.The
stopPropagation()call here is important to prevent the click event from triggering actions on parent elements.
56-68: Well-implemented feature flag for the delete button.Good practice using a feature flag to control the availability of the delete functionality. This allows for gradual rollout and A/B testing.
60-65: Excellent use of color semantics for the destructive action.The delete button styling appropriately uses red for the text and hover state, clearly indicating it's a destructive action.
83-88: Good implementation of a destructive variant for the confirm button.Using the
buttonVariantsutility with the destructive variant is a good practice for styling confirmation buttons for destructive actions.
minimaluminium
force-pushed
the
ap-feed-item-menu-AP-819
branch
from
6da42a9 to
139a638
Compare
ref AP-817, AP-818, AP-819