Skip to content

feat(website): add tooltips for metadata field descriptions to search ui #6209

Open
tombch wants to merge 8 commits intomainfrom
search-field-tooltips
Open

feat(website): add tooltips for metadata field descriptions to search ui #6209
tombch wants to merge 8 commits intomainfrom
search-field-tooltips

Conversation

@tombch
Copy link
Copy Markdown
Collaborator

@tombch tombch commented Mar 30, 2026

Adds tooltips for metadata field definitions in search, column and download field selector modals. This could be quite a convenient place (imo) to provide additional information on processed metadata fields but open to alternate suggestions!

Screenshot

image

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://search-field-tooltips.loculus.org

@claude claude bot added website Tasks related to the web application deployment Code changes targetting the deployment infrastructure labels Mar 30, 2026
@tombch tombch marked this pull request as ready for review March 31, 2026 12:01
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 31, 2026

Claude finished @tombch's task in 1m 35s —— View job


Code Review

Overall this is a clean, well-scoped feature. The approach of attaching definitions to the FieldItem type and propagating them through the three modal entry points is consistent and minimal. A few issues to address:

Bug — YAML injection risk (inline): definition in _common-metadata.tpl is not quoted via | quote, unlike the type field on the same block. A definition containing :, #, or [ would break Helm config generation.

Bug — invalid Tailwind class (inline): text-md is not a valid Tailwind utility class; the correct class is text-base. This will be silently ignored so the tooltip heading won't render at the intended size.

Nit — redundant condition (inline): The inner {field.definition && ...} check inside the outer {field.definition && ...} block is unnecessary.

Design note — rangeDefinitionField indirection: The approach of pointing to a source field's definition via rangeDefinitionField works, but it means the definition lives on sampleCollectionDate and the range field just references it by name. A lookup failure (e.g. typo in rangeDefinitionField) silently produces no tooltip rather than an error. This is probably fine for an optional feature but worth being aware of.

Testing: No automated tests cover the consolidateGroupedFields change. Given the lookup logic added to build definitionsMap, a unit test covering the case where rangeDefinitionField resolves (and doesn't resolve) would be a nice safety net — though not a blocker for merging.

@tombch tombch added the preview Triggers a deployment to argocd label Mar 31, 2026
Comment thread kubernetes/loculus/templates/_common-metadata.tpl Outdated
Comment thread website/src/components/common/FieldSelectorModal.tsx Outdated
Comment thread website/src/components/common/FieldSelectorModal.tsx Outdated
rangeOverlapSearch:
rangeName: sampleCollectionDateRange
rangeDisplayName: Collection date
rangeDefinitionField: sampleCollectionDate
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.

Is there a reason why just adding a definition field to name: sampleCollectionDateRangeUpper and name: sampleCollectionDateRangeLower isnt enough? I cant immedietly see why we need a new field rangeDefinitionField, but I might be missing sth


{{- define "loculus.standardWebsiteMetadata" }}
- type: {{ .type | default "string" | quote }}
{{- if .definition }}
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.

ah could you make an issue detailing how we should maybe refactor the helm template to not duplicate inputFields and metadata and have the website produce inputFields from metadata and filter out input only fields when sending the metadata to the backend and silo/lapis?

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.

heyhey - think this is overall a great improvement, but I think we dont need a new rangeDefinitionField for this (I might be wrong though)

Update: ah! it would also be cool to test this on a PPX preview to see if we need to add description to some internal metadata fields still :-)

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