Improve user details UX on member page (#1889) #1934
Conversation
Summary by CodeRabbit
WalkthroughA memoized helper function was introduced in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/CardDetailsPage.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
📚 Learning: in the detailscard component (frontend/src/components/carddetailspage.tsx), there's a length check b...
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: in the detailscard component (frontend/src/components/carddetailspage.tsx), there's a safety check t...
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🔇 Additional comments (2)
frontend/src/components/CardDetailsPage.tsx (2)
158-158: LGTM! Clean integration of the helper function.The usage of
renderDetailValueis correct and cleanly replaces the previous fallback logic while adding the new interactive functionality.
153-153: LGTM! Minor formatting improvements.The spacing and indentation adjustments improve code readability and maintain consistency.
Also applies to: 160-160, 189-189, 228-228, 257-262, 266-266
|
|
@coderabbitai Any suggestions from the above chat ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@adithya-naik Great question! Looking at your SonarQube warning and the context of your implementation, I'd recommend going with Option 1: the simpler regex. Here's why: Recommended Solution: Simpler Regexconst EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[a-z]{2,}$/i;Reasons:
When to Use validator.js InsteadThe library approach would be better if you needed:
Updated ImplementationYour current usage in const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[a-z]{2,}$/i;
// ... in renderDetailValue function
if (label === "Email") {
if (EMAIL_REGEX.test(value)) {
// render mailto link
}
// render plain text
}Since you're doing basic email format checking for UX purposes (not critical validation), the simpler regex is the perfect balance of security, performance, and maintainability! 🎯 ( .. ) 🏁 Script executed: Length of output: 10663 @adithya-naik Perfect! Looking at your current implementation, I strongly recommend Option 1: the simpler regex approach. Recommended FixUpdate line 26 in // Change this:
const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/
// To this:
const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[a-z]{2,}$/iWhy This is the Best Choice🔒 Security: Fixes the ReDoS vulnerability by eliminating problematic nested quantifiers ⚡ Performance: Simple regex test is faster than library calls - perfect for your UX use case 🎯 Fit for Purpose: Your 📦 Zero Dependencies: Keeps your bundle size lean - no need to add validator.js for this single use case 🔧 Easy Maintenance: Self-contained validation logic that's easy to understand and modify When to Consider validator.jsYou'd want the library approach if you needed:
But for your current implementation, the simpler regex is the perfect solution! 🎯 The fix is a simple one-line change that addresses the security concern while maintaining your existing functionality perfectly. ( ^.^ ) |
|
https://github.com/OWASP/Nest/blob/main/CONTRIBUTING.md#code-quality-checks Also don't use comments that much -- they are needed in exceptional cases to describe reasons behind some nontrivial code. |
There was a problem hiding this comment.
@adithya-naik Please update the PR and resolve conflicts so I could review this. Thanks!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
813-813: Nit: Scope the query to avoid false positives if 'N/A' appears elsewhere.Consider narrowing the query to the details section (e.g., using within() on the details card container) to make the test more robust.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (1)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
813-813: Fallback to 'N/A' is correct and consistent with updated rendering logic.This aligns the test with the new display behavior for missing values. Good update.
| render(<CardDetailsPage {...defaultProps} details={detailsWithMissingValues} />) | ||
|
|
||
| expect(screen.getAllByText('Unknown')).toHaveLength(3) | ||
| expect(screen.getAllByText('N/A')).toHaveLength(3) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify consistency across tests for fallback changes.
Ensure no lingering expectations for "Unknown" remain in tests or UI. You can run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for 'Unknown' in tests (may indicate outdated fallback expectations)..."
rg -n --no-heading -S "Unknown" frontend/__tests__ || true
echo
echo "Searching for 'Unknown' in source (may indicate inconsistent fallback rendering)..."
rg -n --no-heading -S "Unknown" frontend/src || true
echo
echo "Optional: verify presence of tests for new link behaviors"
echo "Searching tests for mailto links..."
rg -n --no-heading -S "mailto:" frontend/__tests__ || true
echo
echo "Searching source for mailto links..."
rg -n --no-heading -S "mailto:" frontend/src || true
echo
echo "Searching tests for company '@' link rendering (heuristic)..."
rg -n --no-heading -S "@[a-zA-Z0-9_-]+" frontend/__tests__ || trueLength of output: 41269
Update fallback text from “Unknown” to “N/A” throughout the codebase
We’ve verified that while the CardDetailsPage tests have been updated to expect “N/A”, both the source and many other tests still use “Unknown” as the fallback. Those need to be aligned:
• In source files:
– frontend/src/components/LeadersList.tsx (line 16): replace <>Unknown</> with <>N/A</>.
– frontend/src/components/CardDetailsPage.tsx (lines 71 and 168): update both occurrences of 'Unknown' to 'N/A'.
• In unit tests:
Run rg -n "Unknown" frontend/__tests__ and replace all expect(...get(Text|ByText)('Unknown')…) assertions with 'N/A'. In particular:
– frontend/tests/unit/components/LeadersList.test.tsx
– frontend/tests/unit/global-error.test.tsx (if the literal “Unknown crash” is now outdated)
– Any other component/page tests that assert on “Unknown”
• Optional: add or update tests for the new mailto link behavior in CardDetailsPage (e.g. check href="mailto:…") to ensure coverage.
Once these changes are made, rerun your test suite to confirm no “Unknown” expectations remain.
🤖 Prompt for AI Agents
In frontend/__tests__/unit/components/CardDetailsPage.test.tsx at line 813 and
throughout the codebase, update all fallback text from "Unknown" to "N/A".
Specifically, replace `<>Unknown</>` with `<>N/A</>` in
frontend/src/components/LeadersList.tsx at line 16 and change both occurrences
of 'Unknown' to 'N/A' in frontend/src/components/CardDetailsPage.tsx at lines 71
and 168. Also, search all test files under frontend/__tests__ for assertions
expecting "Unknown" and update them to expect "N/A" instead, including in
LeadersList.test.tsx and global-error.test.tsx. Optionally, add or update tests
in CardDetailsPage to verify the new mailto link behavior. Finally, rerun tests
to ensure no "Unknown" expectations remain.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/pages/UserDetails.test.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/pages/UserDetails.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
frontend/__tests__/unit/pages/UserDetails.test.tsx (1)
468-468: LGTM: placeholder updated to "Unknown" for 3 missing fieldsMatches the new UX baseline and keeps tests aligned with DetailsCard’s fallback behavior.
| render(<UserDetailsPage />) | ||
| await waitFor(() => { | ||
| expect(screen.getAllByText('N/A').length).toBe(3) | ||
| expect(screen.getAllByText('Unknown').length).toBe(3) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make this assertion more resilient and verify link semantics for email/company
Counting all "Unknown" instances can become brittle as the page evolves. Prefer field-scoped assertions (email/company/location) and add tests for:
- email renders as mailto link when valid, plain text when invalid
- company starting with "@" renders as GitHub link, otherwise plain text
Example tests to add:
test('renders email as a mailto link when valid', async () => {
;(useQuery as jest.Mock).mockReturnValue({
data: { ...mockUserDetailsData, user: { ...mockUserDetailsData.user, email: '[email protected]' } },
loading: false,
error: null,
})
render(<UserDetailsPage />)
await waitFor(() => {
const emailLink = screen.getByRole('link', { name: '[email protected]' })
expect(emailLink).toHaveAttribute('href', 'mailto:[email protected]')
})
})
test('does not render email as link when invalid', async () => {
;(useQuery as jest.Mock).mockReturnValue({
data: { ...mockUserDetailsData, user: { ...mockUserDetailsData.user, email: 'invalid@' } },
loading: false,
error: null,
})
render(<UserDetailsPage />)
await waitFor(() => {
expect(screen.getByText('invalid@')).toBeInTheDocument()
expect(screen.queryByRole('link', { name: 'invalid@' })).toBeNull()
})
})
test('renders company starting with "@" as GitHub link', async () => {
;(useQuery as jest.Mock).mockReturnValue({
data: { ...mockUserDetailsData, user: { ...mockUserDetailsData.user, company: '@acme' } },
loading: false,
error: null,
})
render(<UserDetailsPage />)
await waitFor(() => {
const companyLink = screen.getByRole('link', { name: '@acme' })
expect(companyLink).toHaveAttribute('href', 'https://github.com/acme')
})
})
test('renders company without "@" as plain text', async () => {
;(useQuery as jest.Mock).mockReturnValue({
data: { ...mockUserDetailsData, user: { ...mockUserDetailsData.user, company: 'Acme Inc' } },
loading: false,
error: null,
})
render(<UserDetailsPage />)
await waitFor(() => {
const companyText = screen.getByText('Acme Inc')
expect(companyText.closest('a')).toBeNull()
})
})Quick check to see if such tests already exist:
🏁 Script executed:
#!/bin/bash
# Verify there are tests asserting mailto/company link semantics.
rg -n --hidden --glob "frontend/**/__tests__/**" \
-e 'mailto:' \
-e 'toHaveAttribute\\(.*href.*,.*mailto:' \
-e 'https://github.com/' \
-e 'toHaveAttribute\\(.*href.*,.*https://github.com/'Length of output: 427
I’ve adjusted the grep patterns to avoid the regex error and split the search for clarity. Let’s verify whether any mailto or GitHub link assertions already exist in our tests:
🏁 Script executed:
#!/bin/bash
# Search for mailto link assertions
rg -n --hidden --glob "frontend/**/__tests__/**" "mailto:"
# Search for GitHub link assertions
rg -n --hidden --glob "frontend/**/__tests__/**" "github.com/"Length of output: 13714
Let’s inspect the existing UserDetails tests around the GitHub link checks to see if any cover the company field (and confirm there are truly no mailto/email tests):
🏁 Script executed:
#!/bin/bash
sed -n '330,380p' frontend/__tests__/unit/pages/UserDetails.test.tsxLength of output: 1650
Make “Unknown” assertion field-scoped and add email/company link tests
The catch-all
expect(screen.getAllByText('Unknown').length).toBe(3)in frontend/tests/unit/pages/UserDetails.test.tsx (around line 468) is brittle and doesn’t verify email or company link behavior. Please:
- Replace it with explicit checks for each field’s “Unknown” fallback (email, company, location).
- Add new tests for link semantics:
• Email
– valid address → renders as amailto:link
– invalid address → renders as plain text (no<a>)
• Company
– starts with “@” → renders as a GitHub link (https://github.com/<org>)
– otherwise → renders plain text
Example tests to guide you:
test('renders valid email as mailto link', async () => {
(useQuery as jest.Mock).mockReturnValue({
data: { ...mockUserDetailsData, user: { ...mockUserDetailsData.user, email: '[email protected]' } },
loading: false, error: null,
});
render(<UserDetailsPage />);
await waitFor(() => {
const link = screen.getByRole('link', { name: '[email protected]' });
expect(link).toHaveAttribute('href', 'mailto:[email protected]');
});
});
test('renders company starting with "@" as GitHub link', async () => {
(useQuery as jest.Mock).mockReturnValue({
data: { ...mockUserDetailsData, user: { ...mockUserDetailsData.user, company: '@acme' } },
loading: false, error: null,
});
render(<UserDetailsPage />);
await waitFor(() => {
const link = screen.getByRole('link', { name: '@acme' });
expect(link).toHaveAttribute('href', 'https://github.com/acme');
});
});🤖 Prompt for AI Agents
In frontend/__tests__/unit/pages/UserDetails.test.tsx at line 468, replace the
broad check for 'Unknown' appearing three times with explicit assertions for
each field's fallback text—email, company, and location—to ensure clarity and
robustness. Additionally, add new tests to verify link rendering behavior: for
email, test that valid addresses render as mailto links and invalid ones as
plain text without links; for company, test that names starting with '@' render
as GitHub links and others as plain text. Use the provided example tests as a
guide to implement these specific and comprehensive checks.
|
Hi @kasya I have made the requested chnages, Please find the latest commit and if any chnages needed let me know |
|
@coderabbitai are there any mistakes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me also check the test file to ensure everything is properly tested: Now let me also check if there are any remaining issues with code style or other potential problems: @adithya-naik Looking at your latest implementation, I can see a few areas that need attention: ✅ Good News First
🔧 Issues to Address1. Excessive Comments (As noted by @arkid15r)You have quite a few comments that explain obvious code. Per the contributing guidelines, comments should only explain non-trivial logic. Consider removing these verbose comments: // Remove comments like these that explain obvious code:
// "Check if the value is a valid email"
// "Render mailto link for valid emails"
// "Check if company starts with @"2. URL Sanitization ImprovementYour 3. Potential Edge Cases
🎯 Quick Fixes Needed
The core functionality is solid, but cleaning up the comments per the maintainer's feedback would definitely help with the review process! Would you like me to point out specific comment lines that should be removed? 🏁 Script executed: Length of output: 13589 🏁 Script executed: Length of output: 14098 🏁 Script executed: Length of output: 73 🏁 Script executed: Length of output: 63 @adithya-naik Looking at your current implementation, I'm happy to report that there are no significant mistakes! 🎉 ✅ What's Working Well1. ReDoS Fix Implemented Correctly
2. Clean Code Quality
3. Solid Implementation
4. Comprehensive Test Coverage
🎯 The Implementation is ReadyYour code successfully addresses:
The implementation is clean, secure, and well-tested. Great work addressing all the feedback! The PR looks ready for final review by the maintainers. 🚀 |
There was a problem hiding this comment.
@adithya-naik I'd like to request you to come up with some more comprehensive solution with tests. We already have this code -- could you unify/extract the common logic to a separate function/module, add tests and use it as needed. I'll bump the task to level 2.
|
|
@adithya-naik hi! Are you still working on these updates? |
|
@kasya As i informed iam not good with writing test files.please do help me to commit these changes to the branch as the core functionality is done. |
@adithya-naik this may look like working solution however it's now enough for the code to be merged in some cases. We want to keep the code base clean and maintainable. This particular improvement introduces code duplication I described in my previous comment. I understand you aren't familiar with writing tests and I highly suggest to cover this gap ASAP (not in scope of this contribution of course). I'll check with @kasya on our options here but I can't merge it as is. |
|
Sure @arkid15r . Thanks for ur feedback |
📌 Summary
This PR addresses issue #1889 by enhancing the user experience of the member page. It makes the email and company fields interactive where appropriate.
✅ What's Implemented
mailto:link if valid.@.🧪 Test Scenarios
[email protected][[email protected]](mailto:[email protected])null@ServiceNow[ServiceNow](https://github.com/ServiceNow)ServiceNownull🔗 Related Issue
Closes #1889
🧠 Additional Notes
Thank you for reviewing! 🙌