Conversation
|
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/Profile.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. WalkthroughThe pull request introduces enhancements to the ActivityPub API and related components, focusing on user account management and profile functionality. It adds new types and interfaces for accounts, modifies data fetching mechanisms, and removes unused methods to streamline the codebase. The changes aim to provide a more structured and efficient approach to handling user account information, including followers, following, and profile details. Changes
Sequence DiagramsequenceDiagram
participant User
participant ProfileComponent
participant ActivityPubAPI
participant AccountHooks
User->>ProfileComponent: View Profile
ProfileComponent->>AccountHooks: useAccountForUser(handle)
AccountHooks->>ActivityPubAPI: getAccount()
ActivityPubAPI-->>AccountHooks: Return Account Details
AccountHooks-->>ProfileComponent: Provide Account Data
ProfileComponent->>AccountHooks: useAccountFollowsForUser(handle, type)
AccountHooks->>ActivityPubAPI: getAccountFollows(type)
ActivityPubAPI-->>AccountHooks: Return Follows List
AccountHooks-->>ProfileComponent: Provide Follows Data
ProfileComponent->>User: Display Profile Information
Possibly related PRs
Poem
🪧 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 (
|
mike182uk
changed the title
admin-x-activitypub
…ivitypub refs [AP-647](https://linear.app/ghost/issue/AP-648/refactor-profile-tab-to-use-account-and-follows) Updated the profile tab in admin-x-activitypub to use dedicated account endpoints. This is to remove coupling between the UI and activitypub endpoints in preparation for the upcoming changes around storing accounts and follows in the database
mike182uk
force-pushed
the
mike-ap-648-refactor-profile-tab-to-use-account-and-follows
branch
from
374610c to
797e003
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: 0
🔭 Outside diff range comments (1)
apps/admin-x-activitypub/src/components/global/APAvatar.tsx (1)
Line range hint
71-77: Remove unsafe type casting and update utility function.The component uses unsafe type casting to
ActorPropertiesin both theonClickhandler andtitlecalculation. This could lead to runtime errors if the expected properties fromActorPropertiesare missing.Consider updating the
getUsernameutility to work with the new author type structure:
- Update the utility function:
// Before function getUsername(author: ActorProperties): string // After function getUsername(author: { name: string; /* add required properties */ }): string
- Remove the type casting:
-getUsername(author as ActorProperties) +getUsername(author)
🧹 Nitpick comments (4)
apps/admin-x-activitypub/src/components/global/APAvatar.tsx (1)
Line range hint
1-116: Consider completing the type migration.The changes are part of a larger effort to update account-related types, as mentioned in the PR objectives. To maintain consistency and type safety:
- Complete the migration away from
ActorProperties- Update all related utility functions to work with the new types
- Document the new type structure for other developers
This will help prepare for the future modifications related to accounts storage mentioned in the PR objectives.
apps/admin-x-activitypub/src/components/Profile.tsx (3)
384-384: Add a uniquekeyprop to elements in listsThe element inside the
customFields.mapfunction lacks a uniquekeyprop, which is necessary for React to properly track elements in a list.Apply this diff to add the
keyprop:- {customFields.map(customField => ( + {customFields.map(customField => ( + <span key={customField.name} className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'> <span className={`text-xs font-semibold`}>{customField.name}</span> <span dangerouslySetInnerHTML={{__html: customField.value}} className='ap-profile-content truncate'/> </span> ))}🧰 Tools
🪛 Biome (1.9.4)
[error] 384-384: 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)
318-323: Avoid using non-null assertions to prevent potential runtime errorsUsing the non-null assertion operator
!onaccountcan lead to runtime errors ifaccountis null or undefined. Consider checking thataccountis defined before accessing its properties.Apply this diff to safely access
account.customFields:- const customFields = Object.keys(account?.customFields || {}).map((key) => { - return { - name: key, - value: account!.customFields[key] - }; - }) || []; + const customFields = account?.customFields + ? Object.keys(account.customFields).map((key) => ({ + name: key, + value: account.customFields[key] + })) + : [];
200-201: Remove redundantkeyprop from child elementHaving a
keyprop on both theReact.Fragmentand its childActivityItemis unnecessary. Thekeyprop should be applied to the outermost element in the list. You can remove the redundantkeyprop fromActivityItem.Apply this diff:
<React.Fragment key={account.id}> <ActivityItem - key={account.id} onClick={() => handleAccountClick(account.handle)} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 73f8bcf and 374610ce8874b47cdbf292ba39b9e3efed75d654.
📒 Files selected for processing (4)
apps/admin-x-activitypub/src/api/activitypub.ts(2 hunks)apps/admin-x-activitypub/src/components/Profile.tsx(12 hunks)apps/admin-x-activitypub/src/components/global/APAvatar.tsx(1 hunks)apps/admin-x-activitypub/src/hooks/useActivityPubQueries.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/admin-x-activitypub/src/components/Profile.tsx
[error] 380-380: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 384-384: 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] 386-386: 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/global/APAvatar.tsx (2)
Line range hint
25-28: Well-implemented state management and null safety!The component correctly handles the
authorprop with proper optional chaining and state synchronization.
12-17: Inconsistent type usage detected.While the
authorprop type has been updated to a more specific structure, the component still casts it back toActorPropertiesin theonClickhandler. This suggests an incomplete migration from theActorPropertiestype.Let's verify the usage of
ActorPropertiesin the codebase:apps/admin-x-activitypub/src/components/Profile.tsx (2)
380-380:⚠️ Potential issueAvoid using dangerouslySetInnerHTML to prevent XSS vulnerabilities
Using
dangerouslySetInnerHTMLcan expose users to XSS attacks. Please ensure thataccount.biois properly sanitized before rendering. Alternatively, consider rendering the bio as plain text or using a safe HTML rendering library.🧰 Tools
🪛 Biome (1.9.4)
[error] 380-380: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
386-386:⚠️ Potential issueAvoid using dangerouslySetInnerHTML to prevent XSS vulnerabilities
Using
dangerouslySetInnerHTMLcan expose users to XSS attacks. Please ensure thatcustomField.valueis properly sanitized before rendering. Consider rendering the content as plain text or using a safe HTML rendering library.🧰 Tools
🪛 Biome (1.9.4)
[error] 386-386: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
apps/admin-x-activitypub/src/api/activitypub.ts (1)
409-449: LGTM!The new methods and types introduced align with the enhancements made to the account management features. Error handling is properly implemented.
apps/admin-x-activitypub/src/hooks/useActivityPubQueries.ts (1)
532-556: LGTM!The new hooks
useAccountForUseranduseAccountFollowsForUserare well implemented and integrate properly with the existing codebase.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/admin-x-activitypub/src/components/Profile.tsx (2)
44-52: Add explicit error handling for unknown data structures.The data flattening logic should handle unknown data structures explicitly.
const items = (data?.pages.flatMap((page) => { if ('data' in page) { return page.data; } else if ('accounts' in page) { return page.accounts as TData[]; } - return []; + console.warn('Unknown data structure in page:', page); + return []; }) ?? []);
Line range hint
294-314: Consider using default values for counters.Using
|| 0could lead to issues with falsy values. Consider using nullish coalescing.-counter: account?.likedCount || 0 +counter: account?.likedCount ?? 0 -counter: account?.followingCount || 0 +counter: account?.followingCount ?? 0 -counter: account?.followerCount || 0 +counter: account?.followerCount ?? 0apps/admin-x-activitypub/src/hooks/useActivityPubQueries.ts (1)
533-542: Add error handling to account query.Consider adding error handling to provide better user feedback.
export function useAccountForUser(handle: string) { return useQuery({ queryKey: [`account:${handle}`], async queryFn() { const siteUrl = await getSiteUrl(); const api = createActivityPubAPI(handle, siteUrl); - return api.getAccount(); + try { + return await api.getAccount(); + } catch (error) { + console.error('Failed to fetch account:', error); + throw error; + } } }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 374610ce8874b47cdbf292ba39b9e3efed75d654 and 797e003.
📒 Files selected for processing (4)
apps/admin-x-activitypub/src/api/activitypub.ts(2 hunks)apps/admin-x-activitypub/src/components/Profile.tsx(12 hunks)apps/admin-x-activitypub/src/components/global/APAvatar.tsx(1 hunks)apps/admin-x-activitypub/src/hooks/useActivityPubQueries.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/admin-x-activitypub/src/components/global/APAvatar.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/admin-x-activitypub/src/components/Profile.tsx
[error] 380-380: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 384-384: 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] 386-386: 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 (4)
apps/admin-x-activitypub/src/api/activitypub.ts (3)
46-72: Well-structured account types and interfaces!The new account-related types are well-designed:
Accountinterface provides a comprehensive structure for user profilesMinimalAccounttype efficiently uses Pick to optimize data transfer- Clear separation between full and minimal account information
410-412: LGTM! Consistent URL getter implementation.The
accountApiUrlgetter follows the established pattern for API endpoint construction.
420-449: LGTM! Robust implementation of account follows retrieval.The method includes:
- Proper pagination support
- Thorough null checks
- Array type validation
- Consistent error handling
apps/admin-x-activitypub/src/hooks/useActivityPubQueries.ts (1)
544-556: LGTM! Well-implemented infinite query hook.The implementation follows React Query best practices:
- Proper compound query key
- Correct pagination handling
- Type-safe implementation
| async getAccount(): Promise<GetAccountResponse> { | ||
| const json = await this.fetchJSON(this.accountApiUrl); | ||
|
|
||
| return json as GetAccountResponse; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null response handling.
The method should handle null responses consistently with other methods in the class.
async getAccount(): Promise<GetAccountResponse> {
const json = await this.fetchJSON(this.accountApiUrl);
+ if (json === null) {
+ throw new Error('Failed to fetch account');
+ }
return json as GetAccountResponse;
}📝 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.
| async getAccount(): Promise<GetAccountResponse> { | |
| const json = await this.fetchJSON(this.accountApiUrl); | |
| return json as GetAccountResponse; | |
| } | |
| async getAccount(): Promise<GetAccountResponse> { | |
| const json = await this.fetchJSON(this.accountApiUrl); | |
| if (json === null) { | |
| throw new Error('Failed to fetch account'); | |
| } | |
| return json as GetAccountResponse; | |
| } |
| dangerouslySetInnerHTML={{__html: account?.bio ?? ''}} | ||
| className='ap-profile-content mt-3 text-[1.5rem] [&>p]:mb-3' | ||
| /> | ||
| {attachments.map(attachment => ( | ||
| {customFields.map(customField => ( | ||
| <span className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'> | ||
| <span className={`text-xs font-semibold`}>{attachment.name}</span> | ||
| <span dangerouslySetInnerHTML={{__html: attachment.value}} className='ap-profile-content truncate'/> | ||
| <span className={`text-xs font-semibold`}>{customField.name}</span> | ||
| <span dangerouslySetInnerHTML={{__html: customField.value}} className='ap-profile-content truncate'/> |
There was a problem hiding this comment.
Sanitize HTML content before rendering.
The use of dangerouslySetInnerHTML without sanitization poses a security risk for XSS attacks.
Consider using a sanitization library like DOMPurify:
+import DOMPurify from 'dompurify';
-dangerouslySetInnerHTML={{__html: account?.bio ?? ''}}
+dangerouslySetInnerHTML={{__html: DOMPurify.sanitize(account?.bio ?? '')}}
-dangerouslySetInnerHTML={{__html: customField.value}}
+dangerouslySetInnerHTML={{__html: DOMPurify.sanitize(customField.value)}}📝 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.
| dangerouslySetInnerHTML={{__html: account?.bio ?? ''}} | |
| className='ap-profile-content mt-3 text-[1.5rem] [&>p]:mb-3' | |
| /> | |
| {attachments.map(attachment => ( | |
| {customFields.map(customField => ( | |
| <span className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'> | |
| <span className={`text-xs font-semibold`}>{attachment.name}</span> | |
| <span dangerouslySetInnerHTML={{__html: attachment.value}} className='ap-profile-content truncate'/> | |
| <span className={`text-xs font-semibold`}>{customField.name}</span> | |
| <span dangerouslySetInnerHTML={{__html: customField.value}} className='ap-profile-content truncate'/> | |
| dangerouslySetInnerHTML={{__html: DOMPurify.sanitize(account?.bio ?? '')}} | |
| className='ap-profile-content mt-3 text-[1.5rem] [&>p]:mb-3' | |
| /> | |
| {customFields.map(customField => ( | |
| <span className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'> | |
| <span className={`text-xs font-semibold`}>{customField.name}</span> | |
| <span dangerouslySetInnerHTML={{__html: DOMPurify.sanitize(customField.value)}} className='ap-profile-content truncate'/> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 380-380: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 384-384: 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] 386-386: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
| {customFields.map(customField => ( | ||
| <span className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'> | ||
| <span className={`text-xs font-semibold`}>{attachment.name}</span> | ||
| <span dangerouslySetInnerHTML={{__html: attachment.value}} className='ap-profile-content truncate'/> | ||
| <span className={`text-xs font-semibold`}>{customField.name}</span> | ||
| <span dangerouslySetInnerHTML={{__html: customField.value}} className='ap-profile-content truncate'/> | ||
| </span> |
There was a problem hiding this comment.
Add key prop to custom field spans.
React requires a unique key for elements in an iterable.
-<span className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'>
+<span key={customField.name} className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'>📝 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.
| {customFields.map(customField => ( | |
| <span className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'> | |
| <span className={`text-xs font-semibold`}>{attachment.name}</span> | |
| <span dangerouslySetInnerHTML={{__html: attachment.value}} className='ap-profile-content truncate'/> | |
| <span className={`text-xs font-semibold`}>{customField.name}</span> | |
| <span dangerouslySetInnerHTML={{__html: customField.value}} className='ap-profile-content truncate'/> | |
| </span> | |
| {customFields.map(customField => ( | |
| <span key={customField.name} className='mt-3 line-clamp-1 flex flex-col text-[1.5rem]'> | |
| <span className={`text-xs font-semibold`}>{customField.name}</span> | |
| <span dangerouslySetInnerHTML={{__html: customField.value}} className='ap-profile-content truncate'/> | |
| </span> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 384-384: 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] 386-386: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/admin-x-activitypub/src/components/Profile.tsx (1)
380-386:⚠️ Potential issueSanitize HTML content before rendering.
The use of
dangerouslySetInnerHTMLwithout sanitization poses a security risk for XSS attacks.As mentioned in the previous review, consider using DOMPurify:
+import DOMPurify from 'dompurify'; -dangerouslySetInnerHTML={{__html: account?.bio ?? ''}} +dangerouslySetInnerHTML={{__html: DOMPurify.sanitize(account?.bio ?? '')}} -dangerouslySetInnerHTML={{__html: customField.value}} +dangerouslySetInnerHTML={{__html: DOMPurify.sanitize(customField.value)}}🧰 Tools
🪛 Biome (1.9.4)
[error] 380-380: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 386-386: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🧹 Nitpick comments (3)
apps/admin-x-activitypub/src/components/Profile.tsx (3)
27-27: Consider enhancing error handling in data flattening.While the type union and data structure handling is good, the empty array fallback might hide potential errors.
Consider this improvement:
- return []; + console.warn('Unexpected data structure in page:', page); + return [];Also applies to: 44-52
179-183: Consider memoizing the click handler.The handler function could be memoized using
useCallbackto prevent unnecessary re-renders in child components.-const handleAccountClick = (handle: string) => { +const handleAccountClick = useCallback((handle: string) => { NiceModal.show(ViewProfileModal, { profile: handle }); -}; +}, []);
Line range hint
186-216: Consider extracting shared code into a reusable component.The
FollowingTabandFollowersTabcomponents share significant code structure. Consider creating a shared component to reduce duplication.Example refactor:
interface AccountListProps { accounts: MinimalAccount[]; onAccountClick: (handle: string) => void; } const AccountList: React.FC<AccountListProps> = ({accounts, onAccountClick}) => ( <List> {accounts.map((account, index) => ( <React.Fragment key={account.id}> <ActivityItem key={account.id} onClick={() => onAccountClick(account.handle)} > <APAvatar author={{ icon: { url: account.avatarUrl }, name: account.name }} /> <div> <div className='text-grey-600'> <span className='mr-1 font-bold text-black'>{account.name}</span> <div className='text-sm'>{account.handle}</div> </div> </div> </ActivityItem> {index < accounts.length - 1 && <Separator />} </React.Fragment> ))} </List> );Also applies to: 227-257
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/admin-x-activitypub/package.json(1 hunks)apps/admin-x-activitypub/src/components/Profile.tsx(12 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/admin-x-activitypub/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
apps/admin-x-activitypub/src/components/Profile.tsx
[error] 380-380: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 386-386: 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 (1)
apps/admin-x-activitypub/src/components/Profile.tsx (1)
8-15: LGTM! Import and type changes align with PR objectives.The new imports and type definitions properly support the transition to dedicated account endpoints, improving the separation of concerns.
mike182uk
deleted the
mike-ap-648-refactor-profile-tab-to-use-account-and-follows
branch
1 participant
refs AP-647
Updated the profile tab in
admin-x-activitypubto use dedicated account endpoints. This is to remove coupling between the UI and the ActivityPub endpoints in preparation for the upcoming changes around storingaccountsandfollowsin the databaseSummary by CodeRabbit
New Features
Improvements
Changes
Version Update