Added routing support to Feed in ActivityPub#22661
Conversation
WalkthroughThe changes modify the UI navigation logic and routing for a feed feature in the application. In the Header component, the condition to show the back icon was updated to include any route containing "/feed/" in addition to the "/search" route. A new dynamic route has been added, matching the pattern "feed/:encodedId", which renders a new ContentView component. The ContentView component and its associated FeedItemDivider now manage the display of activity items and their corresponding replies by utilizing specific hooks and state management. Additionally, the Inbox component's onClick handler was updated to conditionally navigate to the new feed route when the layout is set to "feed," while retaining the original behavior in other cases. Possibly related PRs
Suggested labels
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/views/Feed/components/ContentView.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/components/layout/Header/Header.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/lib/feature-flags.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.
✨ 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 (
|
minimaluminium
force-pushed
the
ap-react-router-feed-experiment
branch
from
674efd3 to
4332bc6
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: 4
🧹 Nitpick comments (3)
apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx (3)
42-51: Consider removing commented-out code.There are several commented-out lines that appear to be placeholder functionality. Consider removing these comments or implementing the functionality if it's needed.
- // showHeader={threadParents.length > 0} - // showStats={!disableStats} type='Note' onCommentClick={() => { - // repliesRef.current?.scrollIntoView({ - // behavior: 'smooth', - // block: 'center' - // }); - // setIsFocused(true); }}
60-80: Consider removing commented-out code in reply items.Similar to the previous comment, there are commented-out lines in the reply items section that should be either implemented or removed.
actor={item.actor} allowDelete={item.object.authored} commentCount={item.object.replyCount ?? 0} - // isPending={isPendingActivity(item.id)} last={true} layout='reply' object={item.object} parentId={object.id} repostCount={item.object.repostCount ?? 0} type='Note' onClick={() => { navigate(`/feed/${encodeURIComponent(item.id)}`, { state: {activity: item} }); }} onCommentClick={() => { - // navigateForward(item.id, item.object, item.actor, true); - // setIsFocused(true); }} - // onDelete={decrementReplyCount}
11-27: Consider adding loading state handling.The component fetches thread data but doesn't handle the loading state, which could lead to a poor user experience if data loading takes time.
Consider adding a loading indicator:
const {encodedId} = useParams(); const activityId = encodedId ? decodeURIComponent(encodedId) : ''; const {data: thread, isLoading: isLoadingThread} = useThreadForUser('index', activityId); + + if (isLoadingThread) { + return ( + <Layout> + <div className='mx-auto flex h-full max-w-[620px] flex-col items-center justify-center'> + <div className='animate-pulse'>Loading content...</div> + </div> + </Layout> + ); + } + const threadPostIdx = (thread?.posts ?? []).findIndex(item => item.object.id === activityId);🧰 Tools
🪛 GitHub Actions: CI
[error] 18-18: 'isLoadingThread' is assigned a value but never used @typescript-eslint/no-unused-vars
[error] 21-21: 'threadParents' is assigned a value but never used @typescript-eslint/no-unused-vars
[error] 23-23: 'setReplyCount' is assigned a value but never used @typescript-eslint/no-unused-vars
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/admin-x-activitypub/src/components/layout/Header/Header.tsx(1 hunks)apps/admin-x-activitypub/src/routes.tsx(2 hunks)apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx(1 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx
[error] 18-18: 'isLoadingThread' is assigned a value but never used @typescript-eslint/no-unused-vars
[error] 21-21: 'threadParents' is assigned a value but never used @typescript-eslint/no-unused-vars
[error] 23-23: 'setReplyCount' is assigned a value but never used @typescript-eslint/no-unused-vars
[error] 30-30: Invalid Tailwind CSS classnames order tailwindcss/classnames-order
🔇 Additional comments (6)
apps/admin-x-activitypub/src/components/layout/Header/Header.tsx (1)
47-47: LGTM: Back button logic expanded to include feed routes.The update to include location paths containing "/feed/" in the back button condition is a good enhancement that will ensure users can easily navigate back from feed item detail views.
apps/admin-x-activitypub/src/routes.tsx (2)
1-1: LGTM: ContentView import added.The import of the ContentView component is properly added to support the new feed routing feature.
40-44: LGTM: New feed dynamic route added.The new route pattern 'feed/:encodedId' correctly implements the dynamic routing required for displaying individual feed items in the main content view.
apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx (1)
109-120: LGTM: Enhanced FeedItem click handler.The conditional navigation logic properly implements the routing enhancement for feed items, redirecting to the new feed route when in 'feed' layout while preserving the existing behavior for other layouts.
apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx (2)
1-6: LGTM: Component imports and hook requirements.All necessary imports are properly included to support the ContentView functionality.
7-9: LGTM: Simple divider component.The FeedItemDivider component is a clean, reusable solution for visually separating feed items.
apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx
Outdated
Show resolved
Hide resolved
apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx (7)
22-22: Replace useState with a regular variable.Since the
setReplyCountfunction isn't being used, consider using a regular variable instead of state. This change was previously requested and partially addressed by removingsetReplyCountfrom the destructuring, but the useState could be removed entirely.- const [replyCount] = useState(object.replyCount ?? 0); + const replyCount = object.replyCount ?? 0;
41-42: Implement or remove commented-out code.There are several sections of commented-out code throughout the component. This could confuse future developers about which functionality is intended to be implemented.
Either:
- Implement the functionality if it's required for this PR
- Remove the commented code if it's not needed
- Add TODO comments explaining why the code is commented out and when it will be implemented
Also applies to: 45-50, 74-78
29-30: The nested div structure seems unnecessarily complex.The component has multiple nested divs with similar styling properties. Consider simplifying the structure to improve readability and maintainability.
- <div className='mx-auto flex h-full max-w-[620px] flex-col'> - <div className='relative flex-1'> - <div className='grow overflow-y-auto'> + <div className='mx-auto flex h-full max-w-[620px] flex-col relative overflow-y-auto'>
32-32: Remove backticks from className if no string interpolation is used.The className property uses backticks (``) but doesn't contain any string interpolation. Using standard quotes would be clearer.
- <div className={`mx-auto px-8 pb-10 pt-5`}> + <div className='mx-auto px-8 pb-10 pt-5'>
53-83: Extract thread child rendering to a separate component.The nested mapping logic for rendering thread children adds complexity to the main component. Consider extracting this into a separate component to improve readability and maintainability.
// Example of a separate component const ThreadChildren: React.FC<{ threadChildren: any[]; // Replace 'any' with the appropriate type parentId: string; // Add other necessary props }> = ({ threadChildren, parentId, /* other props */ }) => { const navigate = useNavigate(); return ( <> {threadChildren.map((item, index) => { const showDivider = index !== threadChildren.length - 1; return ( <React.Fragment key={item.id}> <FeedItem actor={item.actor} allowDelete={item.object.authored} commentCount={item.object.replyCount ?? 0} last={true} layout='reply' object={item.object} parentId={parentId} repostCount={item.object.repostCount ?? 0} type='Note' onClick={() => { navigate(`/feed/${encodeURIComponent(item.id)}`, { state: {activity: item} }); }} /> {showDivider && <FeedItemDivider />} </React.Fragment> ); })} </> ); };
1-93: Add TypeScript type definitions for data structures.The component lacks type definitions for important data structures like
activity,object, andthread. Adding proper types would improve code safety and documentation.Consider defining interfaces for these structures:
interface ActivityActor { name: string; // Add other actor properties } interface ActivityObject { id: string; replyCount?: number; repostCount?: number; authored?: boolean; // Add other object properties } interface Activity { id: string; actor: ActivityActor; object: ActivityObject; // Add other activity properties } interface ThreadPost extends Activity { // Add thread-specific properties } interface Thread { posts: ThreadPost[]; // Add other thread properties }Then use these types throughout the component:
const activity = location.state?.activity as Activity; const {data: thread} = useThreadForUser('index', activityId) as {data: Thread | undefined};
24-25: The repliesRef is created but only used as a wrapper div.The
repliesRefis created but doesn't appear to be used for any functionality, since the scroll behavior code is commented out. Consider removing it if it's not needed, or implement the scrolling functionality.- const repliesRef = useRef<HTMLDivElement>(null); const navigate = useNavigate(); // ... - <div ref={repliesRef}> + <div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/admin-x-activitypub/src/components/layout/Header/Header.tsx(2 hunks)apps/admin-x-activitypub/src/lib/feature-flags.tsx(1 hunks)apps/admin-x-activitypub/src/views/Feed/components/ContentView.tsx(1 hunks)apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/admin-x-activitypub/src/views/Inbox/Inbox.tsx
- apps/admin-x-activitypub/src/components/layout/Header/Header.tsx
🔇 Additional comments (1)
apps/admin-x-activitypub/src/lib/feature-flags.tsx (1)
5-8: Well-implemented feature flag for new routing functionality!Adding a feature flag for 'feed-routes' is an excellent approach for introducing the new feed routing functionality. This allows for controlled rollout of the feature, enabling testing in production while maintaining the ability to toggle it off if issues arise.
The implementation correctly follows the existing pattern in the codebase, maintaining type safety as the
FeatureFlagtype automatically incorporates the new flag.
| const location = useLocation(); | ||
| const activity = location.state.activity; | ||
| const object = activity?.object; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checking for location state.
The component directly accesses location.state.activity without verifying it exists, which could cause runtime errors if the page is accessed directly without the expected state.
const location = useLocation();
- const activity = location.state.activity;
- const object = activity?.object;
+ const activity = location.state?.activity;
+ const object = activity?.object;
+
+ if (!activity || !object) {
+ // Handle missing state, e.g., redirect or show error message
+ return <div>Activity not found</div>;
+ }📝 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 location = useLocation(); | |
| const activity = location.state.activity; | |
| const object = activity?.object; | |
| const location = useLocation(); | |
| const activity = location.state?.activity; | |
| const object = activity?.object; | |
| if (!activity || !object) { | |
| // Handle missing state, e.g., redirect or show error message | |
| return <div>Activity not found</div>; | |
| } |
1 participant
no issues