Skip to content

Conversation

@maverbiest
Copy link
Contributor

@maverbiest maverbiest commented Dec 19, 2025

resolves #

This PR will introduce changes to alert users during the submitting group creation process when they are trying to create a group with a name that is already in use. See Stage 1 of #4980 for details.

New features

  • Updated /groups endpoint. This endpoint still returns all groups when a simple GET request is made, but it now also accepts an optional request parameter name (i.e., /groups?name=myGroupName). If this parameter is provided, the list of groups is filtered to return only groups where the name is a case-insensitive perfect match to the request parameter.
  • Website uses the updated endpoint to check for existing groups with the same name as the group being created. This happens in both the group creation and the group edit form.
  • If one or more existing groups exist with the name: open a modal to alert the user and let them know that they can:
    1. Choose another group name
    2. Contact one of the existing groups using contact information shown in the modal
    3. Go ahead with group creation anyway

What I tested manually

  • Group creation/edit works as before when the new group's name does not already exist
  • If the name does exist, an alert comes up containing contact info of the existing groups
  • Clicking the 'create additional group' button disables the button, so spam clicking it does not result in multiple duplicate groups being created (was the case initially)

Screenshot

The alert (table of existing groups is scrollable if there are enough existing groups):
Screenshot 2026-01-14 at 13 51 29

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: https://similar-groups-alert.loculus.org

@maverbiest maverbiest force-pushed the similar-groups-alert branch from 789a72a to cd3facf Compare January 7, 2026 13:03
@maverbiest maverbiest marked this pull request as ready for review January 8, 2026 10:33
@claude
Copy link
Contributor

claude bot commented Jan 8, 2026

Pull Request Review: Group Name Duplication Warning

This PR successfully implements Stage 1 of #4980, adding user-friendly warnings when creating groups with duplicate names. The implementation is well-structured with good test coverage. Below is my detailed review:


✅ Strengths

Backend Implementation

  • Clean API design: The optional name query parameter on /groups endpoint is well-designed and maintains backward compatibility
  • Case-insensitive matching: Properly implemented using lowerCase() in backend/src/main/kotlin/org/loculus/backend/service/groupmanagement/GroupManagementDatabaseService.kt:156
  • Code refactoring: Good use of the existing toGroup() helper method to reduce duplication (backend/src/main/kotlin/org/loculus/backend/service/groupmanagement/GroupManagementDatabaseService.kt:160)

Frontend Implementation

  • UX consideration: Modal provides clear options and contact information for existing groups
  • Anti-spam protection: Good implementation of isSubmitting guard in website/src/components/Group/GroupForm.tsx:81 to prevent duplicate submissions
  • Separation of concerns: New ExistingGroupsModal component is well-isolated and reusable
  • Error handling: Proper error handling in the API call chain

Testing

  • Backend test: New test case properly validates case-insensitive filtering (backend/src/test/kotlin/org/loculus/backend/controller/groupmanagement/GroupManagementControllerTest.kt:240-252)
  • Frontend test: Good addition of test for modal appearance (website/src/components/Group/GroupForm.spec.tsx:87-104)

🔍 Issues & Concerns

1. Security: Email Exposure ⚠️

Location: backend/src/main/kotlin/org/loculus/backend/service/groupmanagement/GroupManagementDatabaseService.kt:160

The getGroups() method calls toGroup() without the redactEmail parameter, exposing contact emails to unauthenticated users. Other methods in the same file use redactEmail = true when appropriate (line 45).

Impact: This could enable email scraping and potential spam/phishing attacks against group contacts.

Recommendation:

return entities.map { it.toGroup(redactEmail = !authenticated) }

Or if the endpoint should be public, at least document this security decision.

2. Race Condition in Group Creation

Location: website/src/components/Group/GroupForm.tsx:108-114

There's a TOCTOU (Time-of-Check-Time-of-Use) race condition:

  1. User checks if group name exists (line 108)
  2. User sees "no duplicates" and proceeds
  3. Another user creates a group with the same name
  4. First user submits and creates a duplicate anyway

Impact: The warning can be bypassed unintentionally, reducing its effectiveness.

Recommendation: Consider adding backend validation that returns a warning flag when duplicates are detected, even if creation succeeds. The frontend could then show a success message mentioning other groups exist.

3. Playwright Test Compatibility 🎭

Location: website/src/components/Group/ExistingGroupsModal.tsx:65

According to the project's CLAUDE.md guidelines:

When adding or modifying interactive components in the website, ensure they are disabled until React hydration completes to prevent race conditions in Playwright tests

The Button component is correctly imported from the common directory (line 5), which is good. However, please verify that BaseDialog (line 27) is also properly wrapped or doesn't have hydration issues.

4. Minor: Empty Div

Location: website/src/components/Group/ExistingGroupsModal.tsx:28

<div className='min-w-[1000px]'></div>

This empty div seems to be a width hack. Consider applying min-w-[1000px] directly to the parent container or a semantic element instead.

5. API Design: Filtering Logic

Location: backend/src/main/kotlin/org/loculus/backend/service/groupmanagement/GroupManagementDatabaseService.kt:154-156

The comment says "just case-insensitive perfect matches for now" suggesting this might expand. Consider:

  • Is substring matching planned? If so, the current API design supports it
  • Should there be pagination? A search for common words might return many results
  • Should there be a limit on results returned?

🧪 Testing Suggestions

  1. Add backend test for email redaction when querying groups
  2. Add integration test for the complete flow: form submission → duplicate detection → modal display → bypass creation
  3. Add edge case test for:
    • Very long group names in the modal (UI overflow)
    • Many duplicate groups (scrolling behavior)
    • Special characters in group names (SQL injection check)

📝 Minor Code Quality Notes

  1. TypeScript: Good use of discriminated unions for success/error results
  2. Naming: submitFromModal and submitFromForm are clear and self-documenting
  3. State management: Clean separation of currentGroup, existingGroups, and isSubmitting states

🎯 Recommendations Priority

  1. HIGH: Fix email exposure security issue
  2. MEDIUM: Document/address the race condition possibility
  3. LOW: Clean up the empty div width hack
  4. LOW: Add pagination/limits if duplicate group names are expected to be common

Overall, this is a solid implementation with good UX considerations. The main concerns are the security issue with email exposure and the inherent race condition in client-side validation. The PR is close to being ready to merge once the high-priority security issue is addressed.

Great work on the anti-spam protection and comprehensive testing! 🚀

@chaoran-chen chaoran-chen added the preview Triggers a deployment to argocd label Jan 8, 2026
@theosanderson theosanderson marked this pull request as draft January 10, 2026 16:01
@theosanderson theosanderson marked this pull request as ready for review January 10, 2026 16:01
@claude

This comment was marked as duplicate.

@theosanderson
Copy link
Member

theosanderson commented Jan 10, 2026

(draftifying and re-reviewing is just because I want to see what the new claude review does - to test the CI not about this PR!)

@anna-parker
Copy link
Contributor

I think I found a little bug, when I try to edit a group to a name that already exists clicking the create group button causes an internal server error:
image

@theosanderson
Copy link
Member

I think I found a little bug, when I try to edit a group to a name that already exists clicking the create group button causes an internal server error:

Not able to reproduce here so it may be quite specific in some way

(nit: we should post this stuff as comments on files then it's easier to keep threads together)

Copy link
Contributor

@anna-parker anna-parker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small non blocking suggestions :-)

@maverbiest maverbiest merged commit d6408b8 into main Jan 14, 2026
45 checks passed
@maverbiest maverbiest deleted the similar-groups-alert branch January 14, 2026 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants