Skip to content

Conversation

@anna-parker
Copy link
Contributor

@anna-parker anna-parker commented Feb 5, 2026

resolves #5947

Screenshot

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?) Submitted to all 3 dummy organisms and confirmed submission works and that I can only view fasta fiels from details pages where consensus sequences are enabled

🚀 Preview: https://disableconsensussequences.loculus.org

@anna-parker anna-parker added the preview Triggers a deployment to argocd label Feb 5, 2026
@anna-parker anna-parker force-pushed the disableConsensusSequences branch from a8628eb to f6abd9d Compare February 5, 2026 12:32
@claude claude bot added the deployment Code changes targetting the deployment infrastructure label Feb 5, 2026
@claude
Copy link
Contributor

claude bot commented Feb 5, 2026

This PR may be related to: #5947 (InsdcAccessionFull missing error when running Loculus without consensus sequences)

@theosanderson
Copy link
Member

(we should merge this)

@theosanderson
Copy link
Member

same for unaligned organism potentially?

theosanderson
theosanderson previously approved these changes Feb 5, 2026
@anna-parker
Copy link
Contributor Author

@theosanderson sorry I didnt see you approved - I now fixed the fasta downloads. I also changed this for the dummy-organism now (I hope this wont break the prepro pipeline - lets see!)

@anna-parker anna-parker changed the title test fix(website, config): update docs for organism with disableConsensusSequence flag and dont allow fasta downloads Feb 5, 2026
@theosanderson
Copy link
Member

nice, yeah, lgtm. If the unaligned organism doesn't work we can just roll that back for now.

@theosanderson
Copy link
Member

(or fix it as you are :) )

@anna-parker anna-parker marked this pull request as ready for review February 5, 2026 13:22
@claude
Copy link
Contributor

claude bot commented Feb 5, 2026

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


Code Review Complete ✅

I've reviewed this PR that updates documentation and prevents FASTA downloads for organisms without consensus sequences.

Overall Assessment

The implementation correctly addresses the issue by conditionally hiding FASTA download options based on whether reference genomes are configured. The approach is sound, but there are some areas for improvement regarding performance, testing, and code clarity.

Key Findings

Testing Coverage ⚠️

  • No automated tests: The PR checklist indicates tests are not implemented
  • The changes affect user-facing download functionality in two components
  • Manual testing was performed, but automated tests would prevent regressions
  • Consider adding Playwright tests to verify:
    • FASTA download button is hidden for organisms without consensus sequences
    • FASTA download button is visible for organisms with consensus sequences
    • TSV download remains available in both cases

Code Quality Issues

  1. Performance concern in SequencesDataTableTitle.astro (see inline comment at line 21)

    • Async findOrganismAndData() call during render can block SSR
    • This data might already be available from parent component
    • Consider passing referenceGenomesInfo as a prop instead
  2. Optional parameter handling (see inline comments at lines 128 and 180 in SeqPreviewModal.tsx)

    • allowFastaDownload should have explicit default value
    • Current implementation works but could be more defensive

Documentation ✅

  • Tutorial updated appropriately to reflect empty referenceGenomes: []
  • Clear comment added: # We are not performing alignment
  • Config changes align with documentation

Security ✅

  • No security concerns identified
  • Changes only affect UI visibility, not backend authorization

Configuration Changes ✅

  • Properly removes reference genome config for organisms with disableConsensusSequences flag
  • Consistent across both dummy-organism and not-aligned-organism

Recommendations

  1. Add automated tests - High priority to prevent regressions
  2. Optimize Astro component - Pass data as props rather than fetching during render
  3. Add default parameter values - Make optional parameter behavior explicit

CI Status

All tests passing except integration tests still running. No failures detected.


@theosanderson theosanderson self-requested a review February 5, 2026 13:27
@theosanderson theosanderson dismissed their stale review February 5, 2026 13:27

probably good to look over final ver

Copy link
Member

@theosanderson theosanderson left a comment

Choose a reason for hiding this comment

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

lgtm

@anna-parker anna-parker merged commit a1c4390 into main Feb 5, 2026
43 checks passed
@anna-parker anna-parker deleted the disableConsensusSequences branch February 5, 2026 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

InsdcAccessionFull missing error when running Loculus without consensus sequences

2 participants