Reader improvements in ActivityPub #22264
Conversation
|
Warning Rate limit exceeded@minimaluminium has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes refactor the UI components related to popovers and loading states. In the Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/shade/src/components/ui/skeleton.tsx (1)
25-31: Consider simplifying the width calculation logic.The current implementation could be simplified while maintaining the same functionality.
Apply this diff to simplify the width calculation:
- const steps = Math.floor((maxWidth - minWidth) / 5); - const randomStep = Math.floor(Math.random() * (steps + 1)); - const randomWidth = minWidth + (randomStep * 5); + const randomWidth = minWidth + (Math.floor(Math.random() * ((maxWidth - minWidth) / 5 + 1)) * 5);Also, consider adding a fallback for environments where
crypto.randomUUID()is not available:- uniqueKeys.push(`skeleton-${crypto.randomUUID()}`); + uniqueKeys.push(`skeleton-${crypto.randomUUID?.() ?? Math.random().toString(36).slice(2)}`);apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (1)
742-742: Remove incomplete comment.There's an incomplete comment that should be removed.
- {/* {modalSize === MODAL_SIZE_LG && object.type === 'Article' && <Popover position='end' trigger={ <Button className='transition-color flex h-10 w-10 items-center justify-center rounded-full bg-white hover:bg-gray-100 dark:bg-black dark:hover:bg-gray-950' icon='typography' size='sm' unstyled onClick={() => {}}/> */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(3 hunks)apps/shade/package.json(1 hunks)apps/shade/src/components/ui/popover.stories.tsx(1 hunks)apps/shade/src/components/ui/popover.tsx(1 hunks)apps/shade/src/components/ui/skeleton.tsx(1 hunks)apps/shade/src/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/shade/src/components/ui/popover.stories.tsx
[error] 17-17: 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)
🔇 Additional comments (9)
apps/shade/src/components/ui/popover.stories.tsx (1)
1-34: LGTM! Well-structured Storybook story.The story effectively demonstrates the Popover component's functionality with a clear example.
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: 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)
apps/shade/src/components/ui/popover.tsx (1)
1-30: LGTM! Well-implemented Popover components.The implementation:
- Properly forwards refs
- Uses TypeScript for type safety
- Follows composable component patterns
- Includes appropriate animations and styling
apps/shade/src/components/ui/skeleton.tsx (1)
4-10: LGTM! Well-defined props interface.The new props provide good flexibility for customizing skeleton widths.
apps/shade/src/index.ts (1)
11-11: LGTM! Export follows established patterns.The popover export is correctly placed in the UI components section and maintains alphabetical order.
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (4)
8-8: LGTM: Clean import organization.The imports are well-organized, with a clear separation between the new Shade components and existing design system components.
Also applies to: 11-11
286-294: LGTM: Enhanced loading state implementation.The new loading state implementation using multiple
Skeletoncomponents with varying heights and randomization provides a more polished and natural-looking placeholder UI while content loads.
845-845: LGTM: Improved table of contents display logic.The condition change to
tocItems.length > 1is more appropriate, as displaying a table of contents for a single heading would be redundant.
743-839: LGTM: Well-structured popover implementation.The new popover implementation using Shade components is well-organized with:
- Proper separation of trigger and content components
- Comprehensive typography controls
- Appropriate accessibility attributes
apps/shade/package.json (1)
73-73:✅ Verification successful
Verify the updated package version.
The version update to
^1.1.6allows for compatible updates. Let's verify if this is the latest stable version and check for any breaking changes or security issues.
🌐 Web query:
What is the latest version of @radix-ui/react-popover and are there any known issues with version 1.1.6?💡 Result:
The latest stable version of
@radix-ui/react-popoveris 1.1.6, published on February 5, 2025[1][5]. A release candidate (1.1.7-rc.6) exists in development but isn't officially released[6].Known issues with v1.1.6:
- Nested popovers/dialogs: When using Popover inside Dialog components, some users report unexpected closing behavior[8][10]. This appears related to event propagation conflicts between overlays[10].
- Scroll interaction conflicts: Integration with scroll-smoothing libraries like GSAP ScrollSmoother can cause positioning issues when popover triggers are below the fold[4].
- Version mismatch warnings: Some dependency trees might show warnings about newer Radix Primitives versions (like 3.x) while using this package[3][7].
These issues appear environment-specific and may require workarounds like:
<Popover.Root closeOnInteractOutside={false}> {/* Content */} </Popover.Root>For scroll-related issues, adjusting
positioningprops or using CSS containment might help[4][9]. The Radix team has addressed similar component conflicts in recent theme updates (v3.2+)[3][7].Citations:
- 1: https://www.npmjs.com/package/@radix-ui/react-popover
- 2: https://codesandbox.io/examples/package/@radix-ui/react-popover
- 3: https://github.com/radix-ui/themes/blob/main/packages/radix-ui-themes/changelog.md
- 4: https://gsap.com/community/forums/topic/42658-scrollsmoother-issue-with-radixshadcn-popover-component/
- 5: https://www.npmjs.com/package/@radix-ui/react-popover?activeTab=dependencies
- 6: https://github.com/radix-ui/primitives/blob/main/packages/react/popover/package.json
- 7: https://www.radix-ui.com/themes/docs/overview/releases
- 8: Popover opened inside a Dialog closes immediately under certain circumstances radix-ui/primitives#2348
- 9: https://www.radix-ui.com/primitives/docs/overview/releases
- 10: [Popover] [Dialog] When nesting
PopoverinDialog, clicks in thePopoverare closing it radix-ui/primitives#2121
Latest Version Confirmation: @radix-ui/react-popover 1.1.6
I verified that the version specified in
apps/shade/package.jsonis indeed the latest stable release as of February 2025. The update to^1.1.6introduces compatible updates without breaking changes. Please note that while there are some known issues (such as nested popovers/dialogs slightly mishandling event propagation and occasional scroll positioning challenges with certain scroll-smoothing libraries), these remain environment-specific and do not imply any security risks.
| export const Default: Story = { | ||
| args: { | ||
| children: [ | ||
| <div style={{height: 400}}> |
There was a problem hiding this comment.
Add a key prop to the div element in the children array.
React requires a unique key for elements in arrays to optimize rendering and reconciliation.
Apply this diff:
- <div style={{height: 400}}>
+ <div key="popover-demo" style={{height: 400}}>📝 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.
| <div style={{height: 400}}> | |
| <div key="popover-demo" style={{height: 400}}> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: 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)
minimaluminium
force-pushed
the
ap-reader-improvements-AP-751
branch
from
3129e3d to
f192456
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/shade/src/components/ui/skeleton.tsx (1)
25-31: Consider making the width calculation more flexible.The current implementation uses a fixed step size of 5, which might be too coarse for some use cases. Consider making the step size configurable or using a more precise calculation.
Apply this diff to make the width calculation more flexible:
- const steps = Math.floor((maxWidth - minWidth) / 5); - const randomStep = Math.floor(Math.random() * (steps + 1)); - const randomWidth = minWidth + (randomStep * 5); + const range = maxWidth - minWidth; + const randomWidth = minWidth + Math.floor(Math.random() * range);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx(3 hunks)apps/shade/package.json(1 hunks)apps/shade/src/components/ui/popover.stories.tsx(1 hunks)apps/shade/src/components/ui/popover.tsx(1 hunks)apps/shade/src/components/ui/skeleton.tsx(1 hunks)apps/shade/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/shade/src/index.ts
- apps/shade/package.json
- apps/shade/src/components/ui/popover.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/shade/src/components/ui/popover.stories.tsx
[error] 17-17: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (5)
apps/shade/src/components/ui/popover.stories.tsx (1)
17-17: Add a key prop to the div element in the children array.React requires a unique key for elements in arrays to optimize rendering and reconciliation.
Apply this diff:
- <div style={{height: 400}}> + <div key="popover-demo" style={{height: 400}}>🧰 Tools
🪛 Biome (1.9.4)
[error] 17-17: 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)
apps/shade/src/components/ui/skeleton.tsx (1)
7-9: LGTM! Well-typed interface properties.The new optional properties are properly typed and provide good flexibility for customizing the skeleton appearance.
apps/admin-x-activitypub/src/components/feed/ArticleModal.tsx (3)
742-839: LGTM! Improved Popover implementation.The new implementation using separate Popover, PopoverTrigger, and PopoverContent components improves modularity and separation of concerns.
286-294: LGTM! Enhanced loading state with Skeleton components.The loading state now provides better visual feedback using multiple Skeleton components with varying heights and randomized widths.
845-845:✅ Verification successful
Verify the table of contents condition change.
The condition for displaying the table of contents has been changed from
tocItems.length > 0totocItems.length > 1. Please confirm if this change is intentional and if it aligns with the expected behavior.Run the following script to check if there are any other occurrences of this condition in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Search for table of contents length conditions. # Test: Search for similar conditions. Expect: Only occurrences of the new condition. rg -A 5 'tocItems\.length'Length of output: 1437
TOC Display Condition Verified
The search confirms that the only occurrence applying the new threshold (
tocItems.length > 1) is in the modal-rendering logic for Articles. Note that the earlier check (if (!iframeElement?.contentDocument || !tocItems.length) { … }) remains unchanged and serves a different purpose by verifying the existence of TOC items. This implies the change was likely intentional—displaying the TOC only when there’s more than one item (presumably to avoid showing a TOC with a single entry).Please confirm that this behavior aligns with your design expectations.
ref AP-751
randomizeproperty to Skeleton component for easier random placeholders