Skip to content

feat(website): add count to reference selector#5907

Open
anna-parker wants to merge 20 commits intoimprove-nextclade-metadatafrom
add_count
Open

feat(website): add count to reference selector#5907
anna-parker wants to merge 20 commits intoimprove-nextclade-metadatafrom
add_count

Conversation

@anna-parker
Copy link
Contributor

@anna-parker anna-parker commented Jan 30, 2026

resolves #5699

Screenshot

image

PR Checklist

  • 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://add-count.loculus.org

@anna-parker anna-parker changed the base branch from main to restructure-anya January 30, 2026 14:04
@claude claude bot added the website Tasks related to the web application label Jan 30, 2026
Base automatically changed from restructure-anya to main January 30, 2026 15:28
@anna-parker anna-parker force-pushed the add_count branch 3 times, most recently from b38307a to d4b3143 Compare February 2, 2026 15:10
@anna-parker anna-parker changed the base branch from main to improve-nextclade-metadata February 2, 2026 15:10
expect(new URL(page.url()).searchParams.size).toBe(0);
});

test('multi-reference mutation filter can be added and removed', async ({ page }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to move this as until there are submitted sequences no options will be shown in the references dropdown

@anna-parker anna-parker marked this pull request as ready for review February 2, 2026 15:53
@anna-parker anna-parker added the preview Triggers a deployment to argocd label Feb 2, 2026
@claude
Copy link
Contributor

claude bot commented Feb 2, 2026

Claude finished @anna-parker's task in 2m 33s —— View job


Code Review Complete

I've completed a comprehensive review of this PR. The feature successfully adds sequence counts to reference selector dropdown options, which is a valuable UX improvement.

Summary

Strengths:

  • Clean implementation that reuses existing AutoCompleteOptions infrastructure
  • Proper use of formatNumberWithDefaultLocale for internationalized number display
  • Good component extraction with SegmentReferenceSelector
  • Tests have been properly updated to mock the new data requirements
  • Integration test was appropriately relocated to where sequences exist

Issues Found:

  1. Performance - useMemo dependency issues: Incorrect dependency arrays could cause unnecessary re-renders (see inline comments on lines 136 and 162)
  2. Testing - Missing explicit count verification: While tests mock count data, none explicitly assert that counts appear in the UI

No Critical Issues:

  • No security vulnerabilities identified
  • Error handling is appropriate (shows "Failed to load options" on error)
  • Loading states are handled correctly
  • No accessibility concerns

Recommendations

  1. Fix the useMemo dependency arrays to prevent unnecessary recalculations
  2. Add a test that explicitly verifies count display and number formatting
  3. Consider stabilizing the lapisSearchParameters reference in parent components to prevent unnecessary re-renders

The core functionality is solid and the changes are well-structured. The issues noted are minor performance optimizations rather than correctness problems.

@anna-parker anna-parker force-pushed the improve-nextclade-metadata branch from 2641a12 to d189ec4 Compare February 4, 2026 10:54
@anna-parker anna-parker removed the preview Triggers a deployment to argocd label Feb 4, 2026
@anna-parker anna-parker force-pushed the improve-nextclade-metadata branch from cd7c474 to d950ab8 Compare February 6, 2026 13:48
@anna-parker
Copy link
Contributor Author

@claude figure out why the integration tests are failing - EV-A71 is not showing up as an option in integration tests (but is on previews)

@claude
Copy link
Contributor

claude bot commented Feb 6, 2026

Claude finished @anna-parker's task in 1m 45s —— View job


Integration Test Fix Complete

I've identified and fixed the issue causing the integration test failure.

Root Cause

The problem was in line 41 of singleseg-multiref-submission-flow.spec.ts:

await releasedPage.selectReference('Genotype', 'EV-A71 (1)');

The selectReference method (in search.page.ts:63) uses Playwright's selectOption({ value: option }), which selects by the value attribute of the <option> element, not the display text.

In the ReferenceSelector component (ReferenceSelector.tsx:69-76):

  • The display text shows: "EV-A71 (1)" (includes the count)
  • The value attribute contains: "EV-A71" (without the count)

The test was trying to select using the display text "EV-A71 (1)", but the actual option value is just "EV-A71".

Fix Applied

I've changed the test to use the correct value:

await releasedPage.selectReference('Genotype', 'EV-A71');

Note on Pushing

I've committed the fix locally, but I don't have push permissions to the repository. The commit is ready and staged with the message:

test(integration): fix reference selector to use value without count

The selectReference method uses selectOption({value}) which selects by
the option's value attribute, not the display text. The count is only
shown in the display text, not in the value.

You'll need to either:

  1. Give me push permissions by updating the --allowedTools configuration
  2. Pull and push the commit manually
  3. Apply the change directly (the fix is just changing line 41 from 'EV-A71 (1)' to 'EV-A71')

@anna-parker
Copy link
Contributor Author

@claude no that is not the problem, on the website when I submit 1 sequence with refence EV-A71 I see EV-A71 (1) in the options. The integration test finds no options although one segment has been submitted

@claude
Copy link
Contributor

claude bot commented Feb 6, 2026

Claude finished @anna-parker's task in 2m 17s —— View job


Root Cause Identified and Fixed

Root Cause

I found the issue! The problem is a fundamental change in how the ReferenceSelector works:

Before this PR (on main):

  • Options were hardcoded from referenceGenomesInfo.segmentReferenceGenomes[segment]
  • All options were statically available in the HTML from page load

After this PR (current code):

  • Options are fetched dynamically from the aggregated API endpoint
  • Data is only loaded when the dropdown is focused (onFocus handler at line 45-47 in ReferenceSelector.tsx)
  • Until focus occurs, the dropdown is empty

The Problem:
The selectReference method in the test used Playwright's selectOption({ value: option }), which directly sets the value without triggering focus events. Since the focus event never fired, the load() function never got called, the options never got fetched, and the dropdown remained empty.

Fix Applied

I've updated the selectReference method in search.page.ts to:

  1. Focus the dropdown first to trigger the onFocus handler that loads options
  2. Wait for options to load by checking that non-empty options exist (with 10s timeout)
  3. Then select the option as before

I've also fixed the test to use 'EV-A71' (the value) instead of 'EV-A71 (1)' (the display text with count).

Changes Made

File: integration-tests/tests/pages/search.page.ts

  • Modified selectReference method to focus dropdown and wait for options to load before selecting

File: integration-tests/tests/specs/features/singleseg-multiref-submission-flow.spec.ts

  • Changed from 'EV-A71 (1)' to 'EV-A71' (the actual option value)

Note on Pushing

I've committed the fix locally, but I don't have push permissions. The commit is ready with message:

fix(integration): wait for reference options to load before selecting

The ReferenceSelector now fetches options dynamically on focus rather than
having them statically available. The test needs to:
1. Focus the dropdown to trigger the onFocus handler that loads options
2. Wait for options to be populated
3. Then select by value (not display text)

You'll need to either:

  1. Update the GitHub Action permissions to allow pushing
  2. Pull and push the commit manually
  3. Apply the changes directly

@anna-parker anna-parker added the preview Triggers a deployment to argocd label Feb 6, 2026
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 website Tasks related to the web application

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EVs: Add number of each subtype to search selector

1 participant