Skip to content

feat(website): Metadata documentation for generated fields#6259

Open
tombch wants to merge 24 commits intomainfrom
generated-field-docs
Open

feat(website): Metadata documentation for generated fields#6259
tombch wants to merge 24 commits intomainfrom
generated-field-docs

Conversation

@tombch
Copy link
Copy Markdown
Collaborator

@tombch tombch commented Apr 10, 2026

This PR does the following:

  • Adds definitions for common metadata fields in ‎kubernetes/loculus/templates/_common-metadata.tpl.
  • Adds the basic files/config for navigating to and viewing metadata documentation in Loculus (without needing e.g. a Pathoplexus overlay).
  • Updates the documentation structure with tabs for input and generated fields, ability to link to a specific field on the page in the URL, and a search bar.

resolves #

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?)

🚀 Preview: https://generated-field-docs.loculus.org

@claude claude bot added website Tasks related to the web application deployment Code changes targetting the deployment infrastructure labels Apr 10, 2026
@tombch tombch added the preview Triggers a deployment to argocd label Apr 10, 2026
@tombch tombch marked this pull request as ready for review April 17, 2026 08:52
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 17, 2026

Claude finished @tombch's task in 3m 4s —— View job


Code Review

  • Gather context and understand the PR
  • Review changed files
  • Post review feedback

Good addition overall — the tab structure, search bar, and URL-addressable field links are all solid UX improvements. A few issues worth addressing before merge:

Bug (inline comment on line 61): fieldLinkId.split('-')[1] breaks for any field name that contains a hyphen (e.g. Nextclade fields like coverage-main). The header and field name are joined with - but not escaped, so split('-') produces more than two parts. Use indexOf('-') to split only on the first occurrence.

SSR safety (inline comment on line 45): Reading window.location.search in the lazy useState initializer will throw on the server during Astro SSR if this component is ever rendered with client:load or client:idle. The OrganismMetadataTableSelector already handles this correctly by reading window.location inside useEffect — recommend the same pattern here.

Performance (inline comments on lines 53 and 134–138):

  • inputFieldNames is a new Set(...) recreated on every render; wrap in useMemo([organism]).
  • inputFieldTypes is rebuilt inside each iteration of the header-groups .map(), so it runs N times (once per group). Move it outside the loop.

Minor (inline comment on config.ts line 71): The InputField.definition comment ("for submitters") is now slightly misleading since definition has been added to Metadata too.

No tests — the checklist item is unchecked, which is fine for a docs/UI feature, but worth noting that the hash-parsing logic in particular would benefit from a unit test given the edge cases.

Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
Comment thread website/src/components/OrganismMetadataTable/OrganismMetadataTable.tsx Outdated
Comment thread website/src/types/config.ts
Copy link
Copy Markdown
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.

Already looks great here and on the PPX as well - it might be good to already add the descriptions for the INSDC fields here as well

import type { TopNavigationItems } from './navigationItems.ts';

export const extraStaticTopNavigationItems = [];
export const extraStaticTopNavigationItems: TopNavigationItems = [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess as this will now lead all instances running loculus to have a docs link with Fill this in with documentation for your loculus instance. maybe we dont want to add a link to the docs by default to the top? @theosanderson do you have thoughts?

}> = ({ organisms }) => {
const [selectedOrganismKey, setSelectedOrganismKey] = useState<string>('');

// Set the initial selected organism based on the URL parameter
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not a react expert but my understanding is we added popstate to get ensure that the state of the page and the URL are kept in sync when a user would navigate forwards and backwards, could this now get out of sync?

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 website Tasks related to the web application

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants