feat(website): add count to reference selector#5907
feat(website): add count to reference selector#5907anna-parker wants to merge 39 commits intomainfrom
Conversation
a87b261 to
7a54fe6
Compare
b38307a to
d4b3143
Compare
| expect(new URL(page.url()).searchParams.size).toBe(0); | ||
| }); | ||
|
|
||
| test('multi-reference mutation filter can be added and removed', async ({ page }) => { |
There was a problem hiding this comment.
I had to move this as until there are submitted sequences no options will be shown in the references dropdown
|
Claude finished @anna-parker's task in 2m 33s —— View job Code Review CompleteI'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. SummaryStrengths:
Issues Found:
No Critical Issues:
Recommendations
The core functionality is solid and the changes are well-structured. The issues noted are minor performance optimizations rather than correctness problems. |
2641a12 to
d189ec4
Compare
7d66310 to
a431ec1
Compare
a431ec1 to
4f315f5
Compare
cd7c474 to
d950ab8
Compare
4f315f5 to
c2976e7
Compare
|
@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 finished @anna-parker's task in 1m 45s —— View job Integration Test Fix CompleteI've identified and fixed the issue causing the integration test failure. Root CauseThe problem was in line 41 of await releasedPage.selectReference('Genotype', 'EV-A71 (1)');The In the ReferenceSelector component (ReferenceSelector.tsx:69-76):
The test was trying to select using the display text Fix AppliedI've changed the test to use the correct value: await releasedPage.selectReference('Genotype', 'EV-A71');Note on PushingI've committed the fix locally, but I don't have push permissions to the repository. The commit is ready and staged with the message: You'll need to either:
|
|
@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 finished @anna-parker's task in 2m 17s —— View job Root Cause Identified and FixedRoot CauseI found the issue! The problem is a fundamental change in how the ReferenceSelector works: Before this PR (on main):
After this PR (current code):
The Problem: Fix AppliedI've updated the
I've also fixed the test to use Changes MadeFile:
File:
Note on PushingI've committed the fix locally, but I don't have push permissions. The commit is ready with message: You'll need to either:
|
cac6268 to
97235ea
Compare
| await expect(page.getByRole('option', { name: 'A.1 (2)' })).toBeVisible(); | ||
| await expect(page.getByRole('option', { name: 'A.2 (2)' })).toBeVisible(); | ||
| await expect(page.getByRole('option', { name: 'A.1.1 (1)' })).toBeVisible(); | ||
| await expect(page.getByRole('option', { name: 'A(3)' })).toBeVisible(); |
There was a problem hiding this comment.
this is weird in the unit tests there has always been no space why was there a space here? ... website/src/components/SearchPage/fields/LineageField.spec.tsx
There was a problem hiding this comment.
(having a quick look at a few things on this now)
There was a problem hiding this comment.
this is so odd... then how did the unit tests pass before? I removed the space to get them to pass
There was a problem hiding this comment.
@theosanderson these are the tests that were failing from me until I removed the space: https://github.com/loculus-project/loculus/blob/main/website/src/components/SearchPage/fields/LineageField.spec.tsx#L129 - happy to add the space back though I do think it looks better
| return String(v); | ||
| }; | ||
|
|
||
| const defaultRenderOptionLabel = (opt: Option) => { |
There was a problem hiding this comment.
I guess before the count and the label were in 2 separate spans? could this be the difference with the space?


resolves #5699
Screenshot
PR Checklist
🚀 Preview: https://add-count.loculus.org