Improve user widget rendering and handle null cases #2376
Conversation
- passes user slug for user in order to have a more recognizable url - prevents possibility of `user=undefined` in iframe url
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
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
npm warn config production Use WalkthroughThe pull request introduces modifications to the Changes
Poem
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)
pages/sites/[slug]/[locale]/profile/widgets.tsx (2)
57-69: Simplify the conditional rendering structure.The null check is a good addition, but we can improve the code structure:
- Remove the unnecessary Fragment
- Simplify the nested conditions
- {user !== null && ( - <> - {user.isPrivate === false ? ( - <div className={styles.widgetsContainer}> - <iframe - src={`${process.env.WIDGET_URL}?user=${user.slug}&tenantkey=${tenantConfig.id}`} - className={styles.widgetIFrame} - /> - </div> - ) : ( - <EmbedModal {...embedModalProps} /> - )} - </> - )} + {user !== null && + (user.isPrivate ? ( + <EmbedModal {...embedModalProps} /> + ) : ( + <div className={styles.widgetsContainer}> + <iframe + src={`${process.env.WIDGET_URL}?user=${user.slug}&tenantkey=${tenantConfig.id}`} + className={styles.widgetIFrame} + /> + </div> + )) + }🧰 Tools
🪛 Biome (1.9.4)
[error] 58-69: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
57-69: Improve type safety for user object properties.Consider adding type guards to ensure safe access to user properties:
interface User { slug: string; isPrivate: boolean; // ... other properties } // Add type guard function isValidUser(user: any): user is User { return user && typeof user.slug === 'string' && typeof user.isPrivate === 'boolean'; } // Use in component {user !== null && isValidUser(user) && ( // ... rest of the code )}This ensures that required properties exist and are of the correct type before accessing them.
🧰 Tools
🪛 Biome (1.9.4)
[error] 58-69: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pages/sites/[slug]/[locale]/profile/widgets.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
pages/sites/[slug]/[locale]/profile/widgets.tsx
[error] 58-69: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
| <iframe | ||
| src={`${process.env.WIDGET_URL}?user=${user.slug}&tenantkey=${tenantConfig.id}`} | ||
| className={styles.widgetIFrame} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance iframe security and accessibility.
While using user.slug improves URL readability, the iframe implementation needs security and accessibility improvements:
<iframe
- src={`${process.env.WIDGET_URL}?user=${user.slug}&tenantkey=${tenantConfig.id}`}
+ src={`${process.env.WIDGET_URL}?user=${encodeURIComponent(user.slug)}&tenantkey=${encodeURIComponent(tenantConfig.id)}`}
className={styles.widgetIFrame}
+ title="User Widget"
+ sandbox="allow-scripts allow-same-origin"
+ referrerPolicy="no-referrer"
/>The changes:
- Encode URL parameters to prevent injection
- Add title for accessibility
- Add sandbox and referrerPolicy for security
📝 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.
| <iframe | |
| src={`${process.env.WIDGET_URL}?user=${user.slug}&tenantkey=${tenantConfig.id}`} | |
| className={styles.widgetIFrame} | |
| /> | |
| <iframe | |
| src={`${process.env.WIDGET_URL}?user=${encodeURIComponent(user.slug)}&tenantkey=${encodeURIComponent(tenantConfig.id)}`} | |
| className={styles.widgetIFrame} | |
| title="User Widget" | |
| sandbox="allow-scripts allow-same-origin" | |
| referrerPolicy="no-referrer" | |
| /> |
There was a problem hiding this comment.
These are not user content, but come from an api response.
Further sanitization is unnecessary.
There was a problem hiding this comment.
@mohitb35 I understand - since these values come from validated API responses, additional URL encoding is unnecessary. Thank you for the clarification.
✏️ Learnings added
Learnt from: mohitb35
PR: Plant-for-the-Planet-org/planet-webapp#2376
File: pages/sites/[slug]/[locale]/profile/widgets.tsx:61-64
Timestamp: 2025-01-22T04:07:44.805Z
Learning: In the Plant-for-the-Planet webapp, URL parameters `user.slug` and `tenantConfig.id` used in iframe URLs are validated API response data and don't require additional URL encoding or sanitization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Enhance the rendering logic for user widgets by passing the user slug for a more recognizable URL and preventing
user=undefinedin the iframe URL when the user is null.