Skip to content

feat: SDA-810 Adds customizability to Chromatograms#79

Open
sdee-tetra wants to merge 33 commits into
mainfrom
chromatagram-enhancements
Open

feat: SDA-810 Adds customizability to Chromatograms#79
sdee-tetra wants to merge 33 commits into
mainfrom
chromatagram-enhancements

Conversation

@sdee-tetra

@sdee-tetra sdee-tetra commented May 4, 2026

Copy link
Copy Markdown

Summary

https://tetrascience.atlassian.net/browse/SDA-810

This is what using ChromatogramChart and StackedChromatogram chart instead of using Plotly directly would look like: https://github.com/tetrascience/ts-data-app-system-suitability-testing/compare/chromatogram-encapsulation

Type of Change

  • Feature (new functionality)
  • Bug fix
  • Refactor
  • Documentation
  • Chore (build, CI, dependencies)
  • Breaking change

Checklist

  • yarn lint passes
  • yarn build passes
  • yarn test:all passes
  • Storybook stories added/updated
  • Code coverage remains the same or increased

Testing

Verification

  • Deploys to preview environment for manual verification
  • All CI/E2E checks pass

Screenshots

@vercel

vercel Bot commented May 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ts-lib-ui-kit-storybook Ready Ready Preview, Comment Jun 24, 2026 1:21am

Request Review

@sdee-tetra sdee-tetra temporarily deployed to artifactory-prod May 4, 2026 13:56 — with GitHub Actions Inactive
@sdee-tetra sdee-tetra temporarily deployed to artifactory-prod May 5, 2026 01:57 — with GitHub Actions Inactive
@sdee-tetra sdee-tetra changed the title Adds range annotations to ChromatogramChart Adds customizability to Chromatograms May 6, 2026
@sdee-tetra sdee-tetra changed the title Adds customizability to Chromatograms feat: SDA-810 Adds customizability to Chromatograms May 6, 2026
sdee-tetra and others added 3 commits May 12, 2026 09:38
Add \`color?: string\` to PeakAnnotation. When set it overrides the
series-derived color for the annotation label, arrow, border, and
boundary markers. Series-color remains the default when omitted, so
existing consumers are unaffected.

Motivated by the SST data app's pass/fail (green/red/grey) peak
coloring, which today is implemented as a custom Plotly overlay.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add PeakAnnotation.regionOverlay (with optional regionOverlayWidth) to
paint a thickened colored segment along the underlying trace between
the peak's startX and endX. Honors peak.color from the per-peak color
override; falls back to the series color.

This is the first-class equivalent of the ad-hoc Plotly overlay used
by the SST runner's ChromatogramPanel, which marks integrated regions
green/red/grey by pass/fail.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add stackingOrder ("first-on-bottom" | "first-on-top", default
"first-on-bottom") to control which end of the stack series[0] lands
on. Annotations and numeric-yAnchor range annotations follow the
chosen direction so they stay pinned to their trace.

The default preserves existing behavior. The new "first-on-top"
direction matches the convention used by the SST Injection-Viewer
panel and lets consumers keep annotations in source order.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@blee-tetrascience blee-tetrascience left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes for Zephyr metadata and lint policy. The diff adds concrete Zephyr IDs, and I do not see a sync action commit on this branch. Please clear the added IDs to empty strings and let the sync action fill them.

// fields don't change — consumers should memoize selectionAppearance if needed).
const resolvedAppearance = useMemo(
() => resolveSelectionAppearance(selectionAppearance),
// eslint-disable-next-line react-hooks/exhaustive-deps

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Repo guidance says not to add lint disable comments. Please make the memo dependency shape lint clean, for example by normalizing the selected appearance fields before the memo or by depending on the full object and requiring callers to memoize it.

- ChromatogramChart: normalize selection-appearance fields into
  primitives before the resolvedAppearance memo so its dependency
  array is exhaustive-deps clean. Removes the
  react-hooks/exhaustive-deps eslint-disable. Behavior unchanged.
- Clear the Zephyr testCaseIds added by this PR to "" so the
  sync-storybook-zephyr action assigns real IDs. The Stacked IDs
  (SW-T1120/1121/1122/1124) were colliding with TdpSearchServer
  stories on main.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@54321jenn-ts

Copy link
Copy Markdown
Contributor

@blee-tetrascience addressed both points in 1fdacfb:

Zephyr metadata — cleared the concrete IDs this PR adds to "" so the sync action can fill them:

  • ChromatogramChart.stories.tsx: UserDefinedPeaks (SW-T1114), PeakHoverAndSelection (SW-T1118), InlineAnnotationStyle (SW-T1119)

  • StackedChromatogramChart.stories.tsx: SW-T1120 / SW-T1121 / SW-T1122 / SW-T1124

    Heads-up: those four Stacked IDs were already in use by TdpSearch/TdpSearchServer.stories.tsx on main, so clearing them also removes a real collision. I kept SW-T1108 / SW-T1109 / SW-T1111 on SingleTrace / MultipleTraces / PeakDetection since those are unchanged and already map to the same-named, sync-assigned stories on main. Next step is applying the zephyr_sync label so the action backfills the cleared IDs, then confirming no duplicate non-empty IDs before merge.

Lint policy — removed the react-hooks/exhaustive-deps disable on the resolvedAppearance memo. The selection-appearance fields are now normalized into primitives before the memo and the dependency array lists exactly those primitives, so it's exhaustive-deps clean with no disable comment. Behavior is unchanged (resolveSelectionAppearance still applies the same ?? defaults).

@54321jenn-ts

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Copilot AI commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved — I merged origin/main into this branch and fixed the merge conflicts in commit 426ae3c.

When series is empty the component skipped Plotly init and left an
uninitialized container, which read as a broken/blank chart. Now it
renders the EmptyState (no-data variant) sized and centered to the
chart box, so empty/loading data shows a clear "No chromatogram data"
placeholder. The EmptySeries play test now asserts the placeholder is
visible (the prior step name claimed to check the title but never did).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 95.01% (🎯 83%)
⬆️ +0.27%
21143 / 22253
🟢 Statements 95.01% (🎯 83%)
⬆️ +0.27%
21143 / 22253
🟢 Functions 93.76% (🎯 74%)
⬆️ +0.18%
933 / 995
🟢 Branches 89.2% (🎯 81%)
⬆️ +0.46%
3934 / 4410
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/components/charts/ChromatogramChart/ChromatogramChart.tsx 98.16%
⬆️ +0.63%
81.25%
⬆️ +4.59%
100%
🟰 ±0%
98.16%
⬆️ +0.63%
310-314
src/components/charts/ChromatogramChart/annotations.ts 100%
🟰 ±0%
100%
⬆️ +3.13%
100%
🟰 ±0%
100%
🟰 ±0%
src/components/charts/ChromatogramChart/boundaryMarkers.ts 100%
⬆️ +4.00%
100%
⬆️ +33.34%
100%
🟰 ±0%
100%
⬆️ +4.00%
src/components/charts/ChromatogramChart/constants.ts 100%
🟰 ±0%
100%
🟰 ±0%
100%
🟰 ±0%
100%
🟰 ±0%
src/components/charts/ChromatogramChart/plotBuilder.ts 100% 97.91% 100% 100%
src/components/charts/ChromatogramChart/regionOverlays.ts 100% 100% 100% 100%
src/components/charts/ChromatogramChart/types.ts 100%
🟰 ±0%
100%
🟰 ±0%
100%
🟰 ±0%
100%
🟰 ±0%
src/components/charts/StackedChromatogramChart/StackedChromatogramChart.tsx 100% 100% 100% 100%
src/components/charts/StackedChromatogramChart/transforms.ts 100% 100% 100% 100%
src/components/charts/StackedChromatogramChart/types.ts 100% 100% 100% 100%
Generated in workflow #838 for commit e1982a1 by the Vitest Coverage Report Action

@54321jenn-ts

54321jenn-ts commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@blee-tetrascience both review items are resolved — re-requesting review.

Zephyr metadata — the zephyr_sync action ran (commit d13c6a1) and assigned real IDs to the cleared stories (SW-T5420–SW-T5430). Verified no duplicate non-empty IDs across the branch — the earlier Stacked IDs were colliding with TdpSearchServer.stories.tsx, which is now fixed. Kept SW-T1108 / SW-T1109 / SW-T1111 since those map unchanged to the same-named, already-synced stories on main. The label was auto-removed by the workflow.

Lint policy — the react-hooks/exhaustive-deps disable on the resolvedAppearance memo is gone; selection-appearance fields are normalized into primitives before the memo and the dependency array lists exactly those, so it's clean with no disable comment.

Also folded in a small follow-up while in here: empty series now renders the EmptyState (no-data) placeholder instead of a blank/uninitialized Plotly container, so a loading/empty chart no longer looks broken.

Screenshot 2026-06-23 at 8 21 56 PM

yarn lint / yarn typecheck / chart tests all pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants