Optimistically added notes and replies to the feed and relevant threads #22471
Conversation
WalkthroughThe pull request makes several updates across the ActivityPub codebase. In the API, the Possibly related PRs
Suggested labels
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/hooks/use-activity-pub-queries.tsOops! 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/admin-x-activitypub/src/components/feed/FeedItemMenu.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/admin-x-activitypub/src/api/activitypub.tsOops! 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.
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/admin-x-activitypub/src/views/Profile/Profile.tsx (1)
160-160: Good implementation of isPending in the PostsTabThe isPending prop correctly uses the imported utility function to conditionally display pending state for optimistic updates in the feed.
However, I notice that the
LikesTabcomponent (lines 185-237) doesn't implement the sameisPendingproperty on itsFeedItemcomponents. Consider adding it there as well for consistent behavior across all tabs.apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (1)
296-296: Minor style inconsistency in conditional expressionThe expression
isPending === falsecould be simplified to!isPendingfor consistency with the coding style used elsewhere in the file.-{!isLoading ? renderTimestamp(object, isPending === false) : <Skeleton className='w-4' />} +{!isLoading ? renderTimestamp(object, !isPending) : <Skeleton className='w-4' />}apps/admin-x-activitypub/src/components/global/APAvatar.tsx (1)
93-93: Inconsistent handling of click events between avatar typesThere's an inconsistency in how click events are disabled. The first condition checks both
size === 'lg' || disabledwhile the second only checksdisabled. This suggests that large avatars shouldn't be clickable, but the logic isn't applied consistently.Consider applying the same condition to both click handlers:
-onClick={size === 'lg' || disabled ? undefined : handleClick} +onClick={size === 'lg' || disabled ? undefined : handleClick} -onClick={disabled ? undefined : handleClick} +onClick={size === 'lg' || disabled ? undefined : handleClick}Or if large avatars without images should be clickable:
-onClick={disabled ? undefined : handleClick} +onClick={disabled ? undefined : size === 'lg' ? undefined : handleClick}Also applies to: 108-108
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx (1)
25-34: Streamlined post handling for optimistic updatesThe code has been simplified to directly mutate with the content and unconditionally update the UI. This supports the optimistic update pattern but removes explicit error handling.
Consider adding a fallback error handling mechanism to notify users when their optimistic post eventually fails.
noteMutation.mutate(trimmedContent); updateRoute('feed'); modal.remove(); + +// Consider adding error handling with toast notification +// noteMutation.mutate(trimmedContent, { +// onError: () => { +// toast.error('Failed to create post. Please try again.'); +// } +// });apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts (3)
613-641: Consider edge scenarios with empty pages.
prependActivityToPaginatedCollectioncorrectly prepends the new activity to the first page, but if there are zero pages, nothing is done. This might be acceptable, though verifying that an empty page state never requires further handling is prudent.
643-676: Check insertion logic whenafteris undefined.
The logic inserts the activity after index0by default, which may or may not be desired. Consider clarifying this behavior or giving users the option to insert at the head of the list when noafteris provided.
877-925: Minor spelling fix suggested for error message.
Typo in “Actvitiy” at line 904. Otherwise, the optimistic note logic is excellent.- throw new Error('Actvitiy returned from API has no id'); + throw new Error('Activity returned from API has no id');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6b30c7a and 9d26a2ef75f5879b88b8d95ef3c366b3bf157a9f.
📒 Files selected for processing (13)
apps/admin-x-activitypub/src/api/activitypub.ts(2 hunks)apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(5 hunks)apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(8 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx(3 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx(5 hunks)apps/admin-x-activitypub/src/components/global/APAvatar.tsx(3 hunks)apps/admin-x-activitypub/src/components/global/APReplyBox.tsx(6 hunks)apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts(4 hunks)apps/admin-x-activitypub/src/utils/pending-activity.ts(1 hunks)apps/admin-x-activitypub/src/utils/render-timestamp.tsx(1 hunks)apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx(2 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(2 hunks)apps/admin-x-activitypub/src/views/Profile/Profile.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Lint
🔇 Additional comments (47)
apps/admin-x-activitypub/src/views/Profile/Profile.tsx (1)
26-26: Good addition of the isPendingActivity utilityThe import of this utility function will help identify pending activities and enhance the user experience with optimistic updates.
apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx (2)
13-13: Good addition of isPendingActivity utilityThis import allows the Inbox view to properly handle optimistic updates for activities.
97-97: Well-implemented isPending propThe isPending property is correctly implemented in the FeedItem component, consistent with the implementation in Profile.tsx. This enables optimistic UI updates for feed items in the inbox view.
apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx (3)
24-24: Good addition of disabled prop to interfaceAdding the optional disabled property to the FeedItemMenuProps interface enhances the component's flexibility.
33-33: Appropriate default value for disabled propSetting the default value to false is a good practice, ensuring backward compatibility with existing usage of the component.
49-49: Well-implemented disabled state in PopoverTriggerThe disabled prop is correctly passed to the PopoverTrigger component, allowing interaction controls when items are in a pending state.
apps/admin-x-activitypub/src/api/activitypub.ts (2)
1-2: Good addition of ActorProperties type importAdding the ActorProperties type import improves type safety in the codebase.
321-321: Improved type safety in getUser methodExplicitly casting the return value to ActorProperties enhances type safety and improves development experience by providing better type information for consumers of this API.
apps/admin-x-activitypub/src/utils/render-timestamp.tsx (3)
15-18: LGTM: Function signature updated to support conditional link renderingThe function now accepts a new parameter
asLinkwhich allows the component to conditionally render the timestamp as either a link or a plain text span. This helps support optimistic updates by allowing timestamps for pending activities to be non-interactive.
20-30: Clear conditional rendering implementationThe implementation cleanly separates the rendering logic based on the
asLinkparameter, maintaining consistent styling between both rendering options.
32-36: Original rendering logic preserved as fallbackThe original span element rendering is now used as the fallback when
asLinkis false, maintaining backward compatibility.apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx (3)
13-13: LGTM: Clean interface extensionAdding the optional
disabledproperty to the interface properly types the new functionality.
24-24: Sensible default value for disabled propSetting the default value to
falseensures backward compatibility with existing code that doesn't pass this prop.
75-75: Consistent application of disabled state to all buttonsThe disabled prop is correctly applied to all three buttons in the component, ensuring consistent behavior across all interactive elements.
Also applies to: 93-93, 109-109
apps/admin-x-activitypub/src/components/feed/FeedItem.tsx (6)
183-183: LGTM: Added isPending property to support optimistic updatesThe new optional
isPendingproperty allows the component to visually indicate pending state for optimistic updates, aligning with the PR objective.
203-203: Sensible default value for isPending propSetting the default value to
falseensures backward compatibility with existing code that doesn't pass this prop.
231-233: Early return for click handler when pendingPreventing click actions while in a pending state is a good UX approach, as it avoids potential confusion or duplicate submissions.
271-271: Clear visual indication of pending stateThe CSS adjustments (cursor and opacity) provide appropriate visual feedback to users when an item is in a pending state.
281-281: Consistent propagation of disabled state to child componentsThe
isPendingstate is consistently passed down to child components as adisabledprop, ensuring the entire component tree reflects the pending state.Also applies to: 302-302, 340-340, 454-454
422-422: Consistent usage of renderTimestamp throughout the componentThe updated
renderTimestampfunction with theasLinkparameter is consistently used across different layouts, maintaining uniform behavior.apps/admin-x-activitypub/src/components/global/APAvatar.tsx (3)
23-23: LGTM: Added disabled property to support pending statesThe new optional
disabledproperty allows the avatar to be non-interactive when an activity is in a pending state.
26-26: Proper function signature update with default valueThe component function signature is properly updated to include the new parameter with a sensible default value.
28-28: Appropriate visual adjustments for disabled stateThe CSS class logic correctly removes hover effects and cursor pointer when in a disabled state, providing appropriate visual feedback.
apps/admin-x-activitypub/src/utils/pending-activity.ts (2)
1-7: Well-structured utility for generating pending activity IDsThe implementation uses crypto.randomUUID() to create unique identifiers with a clear prefix, making pending activities easily identifiable throughout the system.
9-11: Clean implementation of pending activity checkSimple and effective function to determine if an activity ID belongs to a pending activity.
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx (2)
14-14: User parameter added to note mutationThe mutation now includes the user object, enabling optimistic updates by allowing the creation of pending activities with user information.
20-20: Simplified disabled state logicThe button is now disabled only when content is empty or user is undefined, which is appropriate for the optimistic update pattern.
apps/admin-x-activitypub/src/components/global/APReplyBox.tsx (6)
7-9: Updated imports for optimistic updatesThe imports now correctly include ActorProperties to properly type the user data used in optimistic updates.
20-22: Improved callback API with separate success and error handlersReplacing the single
onNewReplywith separateonReplyandonReplyErrorcallbacks provides better separation of concerns and enables proper optimistic update handling.
59-59: User data added to reply mutationIncluding the user object in the mutation enables the creation of pending activities with the correct user information displayed.
70-86: Streamlined reply handling for optimistic updatesThe function now checks for both content and user presence, properly handles errors by calling the error callback, and has simplified success handling that supports the optimistic update pattern.
109-109: Updated button disabled state logicThe disabled state now correctly depends on both text content and user presence, which is appropriate for the optimistic update flow.
146-146: Simplified button implementationThe loading state has been removed from the button since the optimistic update pattern makes it unnecessary to block the UI during the mutation.
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (4)
23-23: Added import for pending activity checkThe isPendingActivity utility is now properly imported and used to identify pending activities in the UI.
447-453: Clean implementation of reply count managementThe functions for incrementing and decrementing reply count provide a clear, modular approach that works well with the optimistic update pattern. The default parameter of 1 is a nice touch for common use cases.
940-942: Updated reply box props for optimistic updatesThe component now uses the new callback API with separate
onReplyandonReplyErrorhandlers that increment and decrement the reply count appropriately.
958-958: Added pending state to feed itemsThe isPending prop is now calculated using the isPendingActivity utility and passed to the FeedItem component, allowing the UI to properly reflect pending activities.
apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts (10)
12-12: No concerns about the new imports.
The import ofActorPropertiesis consistent with its usage below.
21-21: Import usage looks good.
generatePendingActivityandgeneratePendingActivityIddovetail well with subsequent optimistic updates.
23-23: Toast import is appropriate.
Great choice for showing user feedback on errors.
597-611: Implementation detail looks correct.
useQueryis straightforward, andrefetchOnMount: 'always'ensures the thread data is refreshed whenever the component mounts.
678-709: Pagination update mechanism looks solid.
updateActivityInPaginatedCollectionsuccessfully updates an item in nested paginated results. No issues spotted.
711-735: Non-paginated collection updates are well-handled.
updateActivityInCollectionfollows the same pattern as the paginated helper, ensuring correct item updates by ID.
737-762: Removal from paginated collections is handled correctly.
removeActivityFromPaginatedCollectionproperly filters out the specified ID across all pages.
764-782: Consistent removal logic.
removeActivityFromCollectionfilters items by ID in a simpler, non-paginated structure.
784-804: Overwriting theidproperty is intentional.
prepareNewActivitysetsid = activity.object.id, ensuring consistent references while enabling correct post-action lookups. Verify all call sites expect this.
806-875: Optimistic reply mutation is well-crafted.
The approach adds a pending reply, increments relevant reply counts, replaces the pending item on success, and handles errors gracefully.
9d26a2e to
76b8c17
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (1)
958-958: Use of isPending prop.
TheisPending={isPendingActivity(item.id)}logic is straightforward. Consider if items could trigger re-renders rapidly, in which case a memoized or batched approach might be beneficial for performance.apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts (2)
21-21: Pending activity utilities import.
These helpers appear essential for optimistic updates. Ensure any additional complexities (like unique ID collisions) are handled.
806-875: Comprehensive reply mutation.
The optimistic insertion of a pending reply, with rollback on error, is robust.
- Suggest verifying that the parent’s reply count also updates in the thread if relevant.
- Current approach ensures the feed is always updated, but might skip partial edge cases if the parent item doesn’t exist in feed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 9d26a2ef75f5879b88b8d95ef3c366b3bf157a9f and 76b8c1797537302b5e3508770c0947acd7718af6.
📒 Files selected for processing (14)
apps/admin-x-activitypub/src/api/activitypub.ts(2 hunks)apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(5 hunks)apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(8 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx(3 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx(5 hunks)apps/admin-x-activitypub/src/components/global/APAvatar.tsx(3 hunks)apps/admin-x-activitypub/src/components/global/APReplyBox.tsx(6 hunks)apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts(4 hunks)apps/admin-x-activitypub/src/utils/pending-activity.ts(1 hunks)apps/admin-x-activitypub/src/utils/render-timestamp.tsx(1 hunks)apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx(2 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(2 hunks)apps/admin-x-activitypub/src/views/Profile/Profile.tsx(2 hunks)apps/admin-x-activitypub/test/unit/utils/pending-activity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx
- apps/admin-x-activitypub/src/views/Profile/Profile.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx
- apps/admin-x-activitypub/src/utils/render-timestamp.tsx
- apps/admin-x-activitypub/src/components/global/APAvatar.tsx
- apps/admin-x-activitypub/src/api/activitypub.ts
- apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
🔇 Additional comments (26)
apps/admin-x-activitypub/test/unit/utils/pending-activity.ts (1)
1-83: Well-structured test suite for pending activity utilitiesThis is a comprehensive test suite that thoroughly verifies the functionality of all exported pending activity utilities. The tests validate both the format of generated IDs, the identification logic for pending activities, and the complete structure of generated pending activity objects.
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx (3)
14-14: User parameter added to useNoteMutationForUserThe note mutation now requires the user object to support optimistic updates.
20-20: Updated disabled state logicThe button disabled state now depends on user existence rather than mutation loading state, which aligns with the optimistic updates approach.
25-27: Additional validation for user existenceGood addition of user validation before proceeding with the post operation.
apps/admin-x-activitypub/src/utils/pending-activity.ts (2)
3-11: Well-designed pending activity identification utilitiesThe prefix constant and utility functions for generating and identifying pending activities are cleanly implemented.
13-71: Comprehensive pending activity generation with proper typingThe
generatePendingActivityfunction creates a properly structured Activity object with all required fields. The code includes helpful comments explaining which properties are necessary for type compliance but not functionally used in the application.apps/admin-x-activitypub/src/components/global/APReplyBox.tsx (5)
7-8: Updated imports to support new functionalityAdded ActorProperties import and Button component from design system.
20-22: API change: Simplified callback props for reply handlingThe callback props have been updated from a single
onNewReplythat receives an activity to separateonReplyandonReplyErrorcallbacks without parameters. This change supports the optimistic update approach.
59-59: User parameter added to useReplyMutationForUserThe reply mutation now requires the user object to support optimistic updates.
109-109: Updated button disabled state logicThe button disabled state now depends on user existence rather than mutation loading state, which aligns with the optimistic updates approach.
146-146: Button component replaces previous implementationUsing the Button component from the design system improves consistency.
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (4)
384-384: Validate 'index' versus dynamic handle usage.
Currently,useThreadForUser('index', activityId)might be an intentional choice for a single system handle, but verify if'index'is indeed the correct or final handle for all users’ threads.
447-448: Increment functionality looks good.
TheincrementReplyCountprovides a flexible approach by letting you specify astep. The default of 1 is straightforward and, so far, no issues are seen.
940-941: Clear success and error handlers for replies.
Splitting reply count increment/decrement betweenonReplyandonReplyErroris a clean approach and aligns well with optimistic UI patterns.
23-23:❓ Verification inconclusive
Confirm usage of pending-activity import.
It appears thatisPendingActivityis imported for conditional UI logic. Ensure it's only called when simple checks suffice and consider memoization if it's being called repeatedly in the render loop.Would you like to verify performance in larger threads by profiling the calls to
isPendingActivity?
🏁 Script executed:
#!/bin/bash # This script searches the codebase for all calls of isPendingActivity to check usage frequency and context rg "isPendingActivity(" -A 3Length of output: 117
Manual Verification Required: Confirm
isPendingActivityUsageIt appears that
isPendingActivityis imported for conditional UI logic. The initial verification command produced a regex error due to an unescaped parenthesis. To accurately assess how frequently this function is being called (and to check if it’s invoked repeatedly within render loops, which might warrant memoization), please run the following corrected shell script:#!/bin/bash # Corrected search for calls to isPendingActivity with properly escaped parentheses rg "isPendingActivity\\(" -A 3Verify that the usage of
isPendingActivityaligns with performance expectations. If you find that the function is called excessively in performance-critical contexts, consider applying memoization or refining the conditional logic accordingly.apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts (11)
12-12: Newly imported Activity and ActorProperties.
This import is aligned with improved type safety for the updated function signatures. No immediate issues.
23-23: Toast notifications add clarity on errors.
TheshowToastimport is beneficial for user feedback. Be mindful of avoiding toast spam if errors occur in quick succession.
384-384: Expanded signature for useReplyMutationForUser.
The optionalactorPropsis an excellent addition for advanced usage. Verify that calls include the needed actor data in all relevant scenarios.
597-598: Refactored useThreadForUser hook.
UsinguseQuerydirectly simplifies state management. Looks clean and consistent with standard React Query usage.
613-641: Prepending paginated collections.
prependActivityToPaginatedCollectionis a neat pattern for optimistic UIs.
- Confirm that the first page is always the “top” of your list.
- If multiple pages exist and the item belongs in later pages, it could appear in the wrong place.
678-709: Updating activities in paginated collections.
updateActivityInPaginatedCollectionis well-structured. If concurrency is possible, ensure the callback approach merges changes from multiple concurrent updates.
711-735: Responsibly updating single-page collections.
Matches the logic from the paginated function but simpler for the single collection. No major concerns.
737-762: Removing an activity from paginated lists.
The approach is consistent. Verify no other references to the removed item exist in subsequent pages if one page fails to locate the item.
764-782: Removing activities from single-collection data.
Straightforward filter-based approach that should be sufficient.
784-804: prepareNewActivity fosters clarity.
The eagerness to set flags likeauthoredandurlis good for consistent user flow. This function is well designed for reusability.
877-925: Optimistic notes creation with fallback.
useNoteMutationForUsereffectively follows the same pattern with feed/outbox synchronization. Overall this is an excellent approach for an optimistic UI.
ea48a6f to
05fcfbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx (1)
29-33:⚠️ Potential issueSimplified mutation handling needs error case handling
The mutation call has been simplified, but there's no error handling. If the mutation fails, the user will still be redirected to the feed and the modal will be removed without any error notification.
Consider adding error handling to provide feedback to the user in case of failure:
- noteMutation.mutate(trimmedContent); - - updateRoute('feed'); - - modal.remove(); + noteMutation.mutate(trimmedContent, { + onSuccess: () => { + updateRoute('feed'); + modal.remove(); + }, + onError: () => { + // Show error notification or handle error appropriately + } + });
🧹 Nitpick comments (2)
apps/admin-x-activitypub/src/utils/render-timestamp.tsx (1)
20-30: Consider validating object.url before using it in the anchor href.While the new conditional rendering works correctly, there's no validation to ensure
object.urlexists before using it as the href attribute. This could potentially lead to broken links if the URL is undefined.if (asLink) { + // Only render as link if URL exists + if (!object.url) { + return ( + <span className='whitespace-nowrap text-gray-700'> + {relativeTimestamp} + </span> + ); + } return ( <a className='whitespace-nowrap text-gray-700 hover:underline' href={object.url} title={timestamp} > {relativeTimestamp} </a> ); }apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts (1)
877-925: Well-implemented note mutation with optimistic updates.Similar to the reply mutation, this function has been enhanced to:
- Accept actor properties for pending activities
- Add pending notes to feeds immediately
- Replace pending notes on success
- Clean up and show appropriate error messages on failure
There's a small typo in the error message (line 904): "Actvitiy" instead of "Activity".
- throw new Error('Actvitiy returned from API has no id'); + throw new Error('Activity returned from API has no id');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ea48a6ffa79ccb9985f800b47bac1df082baadb1 and 05fcfbb7292403fffba01b9cd7ba1ecab969c439.
📒 Files selected for processing (15)
apps/admin-x-activitypub/src/api/activitypub.ts(2 hunks)apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(6 hunks)apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(8 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx(3 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx(5 hunks)apps/admin-x-activitypub/src/components/global/APAvatar.tsx(3 hunks)apps/admin-x-activitypub/src/components/global/APReplyBox.tsx(6 hunks)apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts(4 hunks)apps/admin-x-activitypub/src/utils/content-handlers.ts(1 hunks)apps/admin-x-activitypub/src/utils/pending-activity.ts(1 hunks)apps/admin-x-activitypub/src/utils/render-timestamp.tsx(1 hunks)apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx(2 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(3 hunks)apps/admin-x-activitypub/src/views/Profile/Profile.tsx(2 hunks)apps/admin-x-activitypub/test/unit/utils/pending-activity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/admin-x-activitypub/src/views/Profile/Profile.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx
- apps/admin-x-activitypub/src/utils/content-handlers.ts
- apps/admin-x-activitypub/test/unit/utils/pending-activity.ts
- apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
- apps/admin-x-activitypub/src/components/global/APAvatar.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx
- apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx
- apps/admin-x-activitypub/src/api/activitypub.ts
🔇 Additional comments (31)
apps/admin-x-activitypub/src/utils/render-timestamp.tsx (2)
15-15: Function signature has been enhanced to support conditional link rendering.The addition of the
asLinkparameter with a default value oftrueallows the function to conditionally render the timestamp as a clickable link or plain text, which aligns well with the PR objective of supporting optimistic updates.
32-36: Style consistency between link and non-link versions.The styling is now consistent between the two rendering options, with the only difference being the hover effect on the link version, which is appropriate.
apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx (3)
14-14: User-aware mutation now enabledThe mutation hook now includes the user object as a parameter, enabling optimistic updates by associating the pending activity with the current user's identity.
20-20: Button disabled state improved for user contextChanged the disabled condition to check for user existence rather than mutation loading state, which aligns with the optimistic update pattern.
25-27: Enhanced validation prior to mutationAdded user existence check before proceeding with the note mutation, ensuring proper context for creating the note.
apps/admin-x-activitypub/src/utils/pending-activity.ts (3)
3-7: Well-structured pending activity identifier implementationGood implementation of a prefix-based system for identifying pending activities with a consistent naming convention and random UUID generation.
9-11: Simple but effective pending activity detectionThe function efficiently checks if an activity ID corresponds to a pending activity by verifying if it starts with the specified prefix.
13-71: Pending Activity Generation: Add Test CoverageThe implementation for
generatePendingActivitycorrectly constructs a fully structured Activity object with all required type-compliant fields. However, our search for tests returned no matching test files.Please add a test case—preferably in a file such as
apps/admin-x-activitypub/src/utils/pending-activity.test.ts—to verify that:
- The generated Activity object matches the expected format.
- The activity is properly recognized by the
isPendingActivityfunction.#!/bin/bash # Check for any existing test files related to pending activity echo "Searching for tests related to pending activity..." fd "pending.*test" --type f # Check for existing test file patterns in the project echo -e "\nExisting test file patterns in the project:" fd ".*test\.(ts|tsx)" --type f | head -n 5apps/admin-x-activitypub/src/components/global/APReplyBox.tsx (8)
7-8: Improved imports organizationConsolidated the ActivityPub type imports and properly ordered the design system component import.
20-21: Enhanced reply lifecycle management with separate callbacksReplaced the single
onNewReplycallback with separateonReplyandonReplyErrorcallbacks, providing more granular control over the optimistic update lifecycle.
59-59: User-aware reply mutation enables optimistic updatesThe mutation hook now includes the user object as a parameter, enabling optimistic updates by associating the pending reply with the current user's identity.
70-72: Improved validation before submitting replyEnhanced the check to verify both text content and user existence before proceeding with the reply submission.
74-81: Error handling improved in reply mutationAdded proper error callback that triggers the
onReplyErrorfunction when the mutation fails, allowing for appropriate error handling.
83-85: Optimistic update pattern implementationThe code now immediately updates the UI by resetting the text value and calling
onReplybefore the server responds, implementing the optimistic update pattern correctly.
109-109: Button disabled state improved for user contextChanged the disabled condition to check for user existence rather than mutation loading state, which aligns with the optimistic update pattern.
146-146: Simplified button implementationThe button now has a cleaner implementation focused on the disabled state without the loading indicator, consistent with the optimistic update approach.
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (5)
23-23: Added pending activity utility importImported the
isPendingActivityfunction to enable pending state detection for feed items.
443-445: Implemented incremental reply count managementCreated a dedicated function to increment the reply count, providing a clear abstraction for handling optimistic updates to the reply count.
447-449: Added reply count decrement for error handlingImplemented a function to decrement the reply count, which is essential for rolling back optimistic updates when operations fail.
936-938: Enhanced reply box with optimistic update callbacksUpdated the APReplyBox component to use the new callback pattern with
onReplyandonReplyError, connected to the reply count management functions.
954-954: Added pending state to feed itemsImplemented the
isPendingprop for FeedItem components using theisPendingActivityutility, enabling visual differentiation of pending activities in the UI.apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts (10)
12-12: Appropriate imports added for optimistic updates.The new imports support the optimistic update functionality by providing the necessary types, utilities for generating pending activities, and toast notifications for error feedback.
Also applies to: 21-21, 23-23
597-611: Clean simplification of useThreadForUser.The function has been simplified to directly return the useQuery result, removing unnecessary state management. This is a cleaner approach and reduces code complexity while maintaining the same functionality.
613-641: Well-implemented collection utility for prepending activities.This utility function adds a new activity to the beginning of a paginated collection, which is perfect for displaying new content at the top of feeds. It correctly handles the pagination structure and only updates the first page.
643-676: Effective implementation for adding activities to a thread.This utility properly splices a new activity after a specific reference item. The function includes appropriate null checks and handles the case where the reference item isn't found.
678-709: Clean utility for updating activities in paginated collections.The function implements a flexible update pattern using a callback, allowing for complex transformations while maintaining proper type safety. The pagination structure is correctly preserved.
711-735: Useful utility for updating activities in non-paginated collections.This parallels the paginated version but for simpler collection structures. The consistent pattern between these utility functions makes the codebase more maintainable.
737-762: Well-implemented removal utility for paginated collections.The function correctly filters out activities by ID while preserving the pagination structure. It includes proper null checking to prevent errors.
764-782: Clean utility for removing activities from non-paginated collections.Similar to its paginated counterpart, this function maintains a consistent pattern while being tailored to the simpler collection structure.
784-804: Helpful activity preparation utility with good documentation.This utility transforms raw API responses into the expected format for the UI, with clear comments explaining the purpose of each transformation. Setting default values for properties like
replyCountsupports the optimistic update pattern.
806-875: Improved reply mutation with optimistic updates.The updated function now:
- Accepts optional actor properties to support pending activity generation
- Implements optimistic updates by adding a pending activity to the thread
- Increments reply counts instantly for better UX
- Properly handles success by replacing pending activities
- Includes error handling with toast notifications and reverting optimistic changes
This implementation aligns perfectly with the PR objective of optimistic updates for replies.
05fcfbb to
338e826
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/admin-x-activitypub/src/components/global/APReplyBox.tsx (1)
74-86:⚠️ Potential issueEnsure reply success actions are executed only after successful mutation.
The current implementation executes success actions (clearing text and calling onReply) outside the mutation callbacks, which means they'll execute regardless of whether the mutation succeeds or not.
Consider moving the success actions into the onSuccess callback:
replyMutation.mutate({ inReplyTo: object.id, content: textValue }, { + onSuccess() { + setTextValue(''); + onReply?.(); + }, onError() { onReplyError?.(); } }); - - setTextValue(''); - - onReply?.();
🧹 Nitpick comments (1)
apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts (1)
904-904: Fix the typo in error message.There's a typo in the error message: "Actvitiy" should be "Activity".
- throw new Error('Actvitiy returned from API has no id'); + throw new Error('Activity returned from API has no id');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 05fcfbb7292403fffba01b9cd7ba1ecab969c439 and 338e826f09c08f9678550fe1e53a63b312a09e02.
📒 Files selected for processing (16)
apps/admin-x-activitypub/package.json(1 hunks)apps/admin-x-activitypub/src/api/activitypub.ts(2 hunks)apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(6 hunks)apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(8 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx(3 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx(5 hunks)apps/admin-x-activitypub/src/components/global/APAvatar.tsx(3 hunks)apps/admin-x-activitypub/src/components/global/APReplyBox.tsx(6 hunks)apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts(4 hunks)apps/admin-x-activitypub/src/utils/content-handlers.ts(1 hunks)apps/admin-x-activitypub/src/utils/pending-activity.ts(1 hunks)apps/admin-x-activitypub/src/utils/render-timestamp.tsx(1 hunks)apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx(2 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(3 hunks)apps/admin-x-activitypub/src/views/Profile/Profile.tsx(2 hunks)apps/admin-x-activitypub/test/unit/utils/pending-activity.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/admin-x-activitypub/package.json
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/admin-x-activitypub/src/components/global/APAvatar.tsx
- apps/admin-x-activitypub/src/views/Profile/Profile.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx
- apps/admin-x-activitypub/src/utils/content-handlers.ts
- apps/admin-x-activitypub/src/api/activitypub.ts
- apps/admin-x-activitypub/test/unit/utils/pending-activity.ts
- apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
- apps/admin-x-activitypub/src/utils/pending-activity.ts
- apps/admin-x-activitypub/src/utils/render-timestamp.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
🔇 Additional comments (20)
apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx (4)
13-13: Good addition of the pending activity state checker.The import of
isPendingActivityis a good addition that enables optimistic UI updates to provide immediate feedback to users.
24-25: Simplified query hook usage.Removing the
updateInboxActivityandupdateFeedActivityvariables simplifies the component by relying on the query functionality alone, which is sufficient for this use case.
96-96: Proper integration of pending state detection.Adding the
isPendingprop to theFeedItemcomponent based on the result ofisPendingActivity(activity.id)enables visual feedback to users when an activity is in a pending state, improving the user experience by making the optimistic updates visible.
101-102: Streamlined event handlers.The simplified
onClickandonCommentClickhandlers without theupdateActivityparameter make the code cleaner and more focused on their primary responsibilities.apps/admin-x-activitypub/src/components/global/APReplyBox.tsx (5)
20-21: Improved reply callback structure.Replacing the single
onNewReplycallback with separateonReplyandonReplyErrorcallbacks provides clearer separation of concerns and better error handling.
59-59: Enhanced mutation with required user context.Including the
userin the reply mutation is essential for creating proper attribution for replies and supports the optimistic updates feature.
70-72: Robust user existence check.Adding the user existence check prevents attempts to create replies without the necessary user context, avoiding potential errors.
109-109: Improved button disabled state logic.The updated disabled state logic that checks for both text value and user presence ensures that replies are only submitted when all required data is available, preventing potential errors.
146-146: Simplified button rendering.Removing the loading state from the button is consistent with the optimistic updates approach, as the pending state is now handled at a higher level with the
isPendingproperty on activities.apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (4)
23-23: Good addition of pending activity detection.Importing the
isPendingActivityutility properly supports the optimistic updates feature.
443-449: Good separation of reply count management.Implementing distinct functions for incrementing and decrementing reply counts improves code readability and maintainability, making the intent clearer than a consolidated function with multiple responsibilities.
936-938: Updated reply box to use new callback pattern.The update from
onNewReplyto separateonReplyandonReplyErrorcallbacks properly integrates with the new reply handling mechanism.
954-954: Proper pending state handling for thread replies.The addition of the
isPendingprop to theFeedItemfor thread children ensures consistent visual feedback for pending replies across the application.apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts (7)
21-23: Good imports for optimistic updates.Adding imports for pending activity generation and toast notifications properly supports the optimistic updates feature and error handling.
597-611: Simplified thread query handling.The simplification of the
useThreadForUserfunction to directly return the query result reduces unnecessary code while maintaining functionality.
613-641: Excellent modularization of collection manipulation functions.The implementation of various utility functions for manipulating activities in collections greatly improves code organization, maintainability, and reusability. The functions provide a clear API for adding, updating, and removing activities from different collection types.
Also applies to: 643-676, 678-709, 711-735, 737-762, 764-782, 784-804
806-806: Enhanced mutation functions with actor properties.Updating
useReplyMutationForUseranduseNoteMutationForUserto acceptactorPropsenables proper attribution and display of pending activities, which is crucial for the optimistic updates feature.Also applies to: 877-877
816-840: Robust optimistic updates for replies.The implementation of optimistic updates for replies is thorough, correctly handling the temporary ID, generating a pending activity, adding it to the thread, and updating reply counts in the feed.
850-873: Improved error handling for reply mutations.The error handling for reply mutations now properly cleans up the optimistically added activities and provides user feedback through toast notifications, creating a better user experience when errors occur.
889-901: Good optimistic updates and error handling for notes.The implementation of optimistic updates and error handling for notes follows a similar pattern to replies, ensuring consistency in the codebase and providing a good user experience.
Also applies to: 912-923
refs https://linear.app/ghost/issue/AP-881 Optimistically added notes and replies to the feed and relevant threads when posting a note or reply
338e826 to
c04cf5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/admin-x-activitypub/test/unit/utils/pending-activity.ts (2)
24-83: Thorough test for generatePendingActivity with complete actor properties.The test is comprehensive, creating a complete mock actor and validating all expected properties in the resulting pending activity. Good use of type casting when accessing the
attributedToproperties.Consider adding tests for edge cases, such as handling minimal actor objects or potential error conditions.
// You might want to add an additional test case for minimal required actor properties: it('should generate a pending activity with minimal actor properties', function () { const minimalActor: Partial<ActorProperties> = { id: 'https://example.com/actor', url: 'https://example.com/actor', name: 'Example Actor', preferredUsername: 'example', icon: { url: 'https://example.com/icon.png' }, type: 'Person' }; const id = generatePendingActivityId(); const content = 'Test content'; const pendingActivity = generatePendingActivity(minimalActor as ActorProperties, id, content); // Assert essential properties are correctly set expect(pendingActivity.id).toBe(id); expect(pendingActivity.object.content).toBe(content); expect(pendingActivity.actor.preferredUsername).toEqual(minimalActor.preferredUsername); });
85-94: Clear test for formatPendingActivityContent.The test effectively verifies that newline characters are correctly replaced with HTML line breaks.
Consider adding tests for edge cases such as empty strings or content with multiple consecutive newlines.
// Additional test cases you might consider adding: it('should handle empty string', function () { expect(formatPendingActivityContent('')).toBe(''); }); it('should handle multiple consecutive newlines', function () { expect(formatPendingActivityContent('line1\n\nline3')).toBe('line1<br /><br />line3'); });apps/admin-x-activitypub/src/components/global/APReplyBox.tsx (1)
69-86: Improved reply submission flow with validationThe reply submission flow now properly checks for both
textValueanduserexistence before proceeding. The success and error paths have been separated appropriately, addressing the previous review concern.However, the success actions (clearing text and calling onReply) are still outside the mutation callbacks. This means they'll execute immediately after the request is sent but before it completes.
Consider moving success actions inside the onSuccess callback for proper sequence:
replyMutation.mutate({ inReplyTo: object.id, content: textValue }, { + onSuccess() { + setTextValue(''); + onReply?.(); + }, onError() { onReplyError?.(); } }); -setTextValue(''); - -onReply?.();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 338e826f09c08f9678550fe1e53a63b312a09e02 and c04cf5f.
📒 Files selected for processing (16)
apps/admin-x-activitypub/package.json(1 hunks)apps/admin-x-activitypub/src/api/activitypub.ts(2 hunks)apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(6 hunks)apps/admin-x-activitypub/src/components/feed/FeedItem.tsx(8 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx(3 hunks)apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx(5 hunks)apps/admin-x-activitypub/src/components/global/APAvatar.tsx(3 hunks)apps/admin-x-activitypub/src/components/global/APReplyBox.tsx(6 hunks)apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts(4 hunks)apps/admin-x-activitypub/src/utils/content-handlers.ts(1 hunks)apps/admin-x-activitypub/src/utils/pending-activity.ts(1 hunks)apps/admin-x-activitypub/src/utils/render-timestamp.tsx(1 hunks)apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx(2 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(3 hunks)apps/admin-x-activitypub/src/views/Profile/Profile.tsx(2 hunks)apps/admin-x-activitypub/test/unit/utils/pending-activity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/admin-x-activitypub/package.json
- apps/admin-x-activitypub/src/components/feed/FeedItemMenu.tsx
- apps/admin-x-activitypub/src/views/Profile/Profile.tsx
- apps/admin-x-activitypub/src/utils/render-timestamp.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItemStats.tsx
- apps/admin-x-activitypub/src/components/feed/FeedItem.tsx
- apps/admin-x-activitypub/src/utils/content-handlers.ts
- apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx
- apps/admin-x-activitypub/src/api/activitypub.ts
- apps/admin-x-activitypub/src/components/global/APAvatar.tsx
- apps/admin-x-activitypub/src/views/Feed/components/NewPostModal.tsx
- apps/admin-x-activitypub/src/hooks/use-activity-pub-queries.ts
🧰 Additional context used
🧠 Learnings (2)
apps/admin-x-activitypub/test/unit/utils/pending-activity.ts (2)
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T10:03:20.701Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T10:03:20.701Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
apps/admin-x-activitypub/src/utils/pending-activity.ts (2)
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T10:03:20.701Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T10:03:20.701Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
🔇 Additional comments (19)
apps/admin-x-activitypub/test/unit/utils/pending-activity.ts (4)
1-9: Proper imports with clear type safety.The imports are well-organized and include the necessary type definitions, particularly
ActorPropertieswhich ensures type safety for the tests. Good practice importing all the utilities being tested from the source directory.
11-15: Well-structured test for generatePendingActivityId.The test effectively validates that the generated ID follows the expected format with a prefix followed by a UUID v4. The regex pattern is accurate for validation.
17-22: Comprehensive testing of isPendingActivity.Excellent implementation testing both positive and negative cases for the isPendingActivity function. This ensures the function correctly identifies pending activities based on their ID prefix.
1-94: Well-structured and comprehensive test suite.Overall, this test suite is well-organized with clear separation between different function tests. Each test focuses on specific functionality and verifies the expected behavior thoroughly. The structure follows best practices for unit testing.
The tests align with the retrieved learnings, confirming that the pending activity utilities are indeed thoroughly tested in this file.
apps/admin-x-activitypub/src/utils/pending-activity.ts (4)
1-7: Well implemented utility for generating pending activity IDsThe implementation of
generatePendingActivityIdusing thecrypto.randomUUID()function is a good approach for creating unique identifiers. The constantPENDING_ACTIVITY_PREFIXmakes the code more maintainable by centralizing the prefix value.
9-11: Clean and efficient implementation for checking pending activitiesThe
isPendingActivityfunction provides a simple and efficient way to check if an activity is pending by verifying the prefix. This approach is consistent with the generation method.
13-71: Well-structured pending activity generator with comprehensive type handlingThe
generatePendingActivityfunction properly creates a complete Activity object with all the required properties. I appreciate the thorough comments explaining which properties are required for type compliance but not functionally used in the application.As noted in the learning feedback, this function is covered by tests in
apps/admin-x-activitypub/test/unit/utils/pending-activity.ts.
73-75: Simple and effective content formatting utilityThe
formatPendingActivityContentfunction provides a straightforward way to format content by replacing newlines with HTML line breaks, which is appropriate for displaying plain text in HTML contexts.apps/admin-x-activitypub/src/components/global/APReplyBox.tsx (6)
7-8: Improved imports with more explicit typesUpdating the imports to directly use
ActorPropertiesfrom the framework API improves type safety throughout the component.
20-21: Better callback separation for reply handlingReplacing the single
onNewReplywith separateonReplyandonReplyErrorcallbacks allows for more granular control over success and error states, supporting the optimistic UI updates strategy.
51-52: Updated prop destructuring to match interface changesThe component props are properly updated to match the interface changes, maintaining consistency.
59-59: Improved dependency handling in mutation hookThe mutation now depends on the user data, which is a necessary improvement for proper reply functionality.
109-109: Improved button state logicThe button's disabled state now depends on both
textValueanduser, which is more accurate and prevents submission attempts when the necessary data is missing.
146-146: Simplified button with relevant propsThe button has been simplified by removing the loading state, which is appropriate since the optimistic UI approach means we don't need to show loading indicators for this interaction.
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (5)
10-10: Improved imports with additional types and utilitiesThe updates to the imports, including the
ActorPropertiestype and theisPendingActivityutility, support the new optimistic UI update functionality.Also applies to: 23-23
443-445: Well-implemented reply count increment functionThe
incrementReplyCountfunction provides a clean way to increase the reply count, accepting an optional step parameter with a sensible default value of 1.
447-449: Good symmetry with decrement functionThe
decrementReplyCountfunction mirrors the increment function, maintaining consistency in the API. This is useful for handling error cases or deletions.
936-937: Updated reply box callbacks for optimistic updatesThe
APReplyBoxnow receives the appropriate callbacks to handle optimistic updates, withincrementReplyCountfor successful replies anddecrementReplyCountfor error cases.
954-954: Added pending state indicator for optimistic UIThe
isPendingprop is now passed to theFeedItemcomponent, using theisPendingActivityutility to determine if an activity is in a pending state. This is a key part of the optimistic UI update mechanism.
refs https://linear.app/ghost/issue/AP-881
Optimistically added notes and replies to the feed and relevant threads when posting a note or reply