feat(website): Cumulative graph of submission on groups page

[theosanderson]

feat(website): Add cumulative submissions chart to group page

Adds a line chart showing cumulative submissions over time on the group page, with each organism as its own series.

Changes

• Added new config option metadataItemForCumulativeGroupGraph to control which metadata field is used for the x-axis (e.g. earliestReleaseDate)
• Default is null (chart hidden), set to earliestReleaseDate for preview server
• Created CumulativeSubmissionsChart component in its own file
• Chart only displays when config value is set

Files changed

Kubernetes config:

• values.schema.json - Added schema for new config option
• values.yaml - Default value (null)
• values_preview_server.yaml - Set to earliestReleaseDate
• templates/_common-metadata.tpl - Pass config to website

Website:

• src/types/config.ts - Added type for new config field
• src/components/User/CumulativeSubmissionsChart.tsx - New chart component
• src/components/User/GroupPage.tsx - Fetch time series data and render chart
• src/pages/group/[groupId]/index.astro - Pass config prop
• Test files updated with new required prop

🚀 Preview: https://dates-graph.loculus.org

[anna-parker]

I guess we could just notshow the graph is earliestReleaseDate is not configured?

[claude]

Claude finished @theosanderson's task in 1m 49s —— View job

---

Code Review Complete

I've reviewed the PR and provided inline comments on specific issues. Here's my overall assessment:

Summary

This PR adds a cumulative submissions chart to the group page, controlled by a new config option metadataItemForCumulativeGroupGraph. The implementation is generally solid and well-structured, but there are several areas that need attention.

Key Findings

Architecture & Design:

• ✅ Good separation of concerns with a dedicated CumulativeSubmissionsChart component
• ✅ Proper configuration-driven approach using Kubernetes values
• ✅ The chart is appropriately hidden when the config is null

Code Quality Issues:

• ⚠️ Chart.js registration: Using useEffect for global registration is an anti-pattern (see inline comment)
• ⚠️ Unsafe non-null assertion: TypeScript type safety is compromised in the query function
• ⚠️ Silent error handling: API failures are swallowed without logging or user feedback
• ℹ️ Color palette limitation: Only 8 colors available, which may cause confusion with many organisms

Testing:

• ✅ Tests properly updated with the new required prop
• ⚠️ No tests for the new CumulativeSubmissionsChart component itself
• ⚠️ No tests for time series data fetching logic

Documentation:

• ✅ PR description is detailed and explains the changes well
• ℹ️ Config schema description could be more explicit about field requirements

Recommendations

High Priority:

1. Fix the Chart.js registration pattern (move to module level)
2. Add proper null handling for the metadata field query
3. Add error logging for failed API requests

Medium Priority:
4. Add unit tests for the chart component
5. Consider testing the cumulative calculation logic
6. Enhance the schema description for the config option

Low Priority:
7. Document or address the 8-color limitation
8. Add explanatory comments for the data filtering logic

Security & Performance

• ✅ No security concerns identified
• ✅ Proper use of Promise.all for parallel fetching
• ✅ Chart only renders when data is available
• ✅ Loading states are properly handled

Response to anna-parker's comment

Regarding the suggestion to hide the graph when earliestReleaseDate is not configured: The current implementation already handles this well! When metadataItemForCumulativeGroupGraph is null, the chart section doesn't render at all (line 238-247 in GroupPage.tsx). This is better than checking for a specific field name because it:

• Makes the feature more flexible (works with any date field)
• Is explicit about enabling/disabling the feature
• Follows the principle of least surprise

The PR looks good overall and is ready for merge after addressing the high-priority items.

[claude]

The Chart.js registration should not be done in a component using useEffect. This violates React best practices and can cause issues:

1. Chart.js plugins are global and should be registered once at module level
2. Using state (isRegistered) to conditionally render based on registration is unnecessary
3. Multiple component instances would try to register the same plugins multiple times

Recommendation: Move registration to module level:

// At the top of the file, outside the component
ChartJS.register(CategoryScale, LinearScale, PointElement, LineElement, Title, Tooltip, Legend);

Then remove the isRegistered state and the useEffect entirely (lines 36-41 and 85-87).

[claude]

This filter removes organisms with zero cumulative submissions, but only checks the final data point. Consider these edge cases:

1. An organism with submissions that were all revoked would still show as a line at 0
2. The check > 0 could be !== 0 for consistency (though functionally equivalent)

Recommendation: Add a comment explaining the intent:

// Only show organisms that have at least one submission
datasets: datasets.filter((ds) => ds.data[ds.data.length - 1] > 0),

[claude]

The query will execute even when metadataItemForCumulativeGroupGraph is null, which could cause issues:

1. The non-null assertion (metadataItemForCumulativeGroupGraph!) on line 76 is unsafe - TypeScript will assume it's non-null but it could still be null
2. If someone changes the enabled condition, the query function could be called with null

Recommendation: Make the queryFn handle the null case explicitly:

queryFn: () => {
    if (metadataItemForCumulativeGroupGraph === null) {
        return Promise.resolve({});
    }
    return fetchTimeSeriesData(groupId, clientConfig, organisms, metadataItemForCumulativeGroupGraph);
},

[claude]

Silent error handling with an empty array could mask real issues. When the API request fails, users won't know if:

1. The group genuinely has no data
2. The API is down
3. There's a configuration error

Recommendation: Consider logging errors or showing an error state:

} catch (error) {
    console.error(`Failed to fetch time series data for ${key}:`, error);
    data[key] = [];
}

Same issue exists in fetchSequenceCounts at line 318.

[claude]

The description could be more specific about the expected metadata field characteristics:

1. Should mention that the field should be a date field (though technically any field could work)
2. Could clarify what happens when set to null vs an invalid field name
3. The relationship to earliestReleaseDate configuration could be mentioned

Recommendation: Enhance the description:

"description": "The metadata field used to plot cumulative submissions over time on the group page (typically 'earliestReleaseDate' or another date field). Set to null to disable the chart. If the specified field does not exist for an organism, the chart will show no data for that organism."