IGVF-3450 Reorder analysis-set list-view card items

[forresttanaka]

Code Review — IGVF-3450-analysisset-list-supp

Files reviewed:

• components/search/list-renderer/analysis-set.js → analysis-set.tsx
• components/search/__tests__/list-renderer.test.js
• globals.d.ts
• lib/workflow.ts

---

analysis-set.tsx

Conversion from JS + PropTypes to TypeScript

The file is migrated cleanly. PropTypes declarations are replaced with an inline TypeScript parameter type for the component, and the AccessoryData type is defined locally. No issues.

Bug fix: non-embedded lab

The original code accessed analysisSet.lab.title unconditionally. Since lab can be either a LabObject or a plain string link depending on whether the API embedded the object, this would throw at runtime when lab is a string. The fix is correct:

const lab = isEmbedded(analysisSet.lab) ? analysisSet.lab : null;
// ...
<span key="lab">{lab?.title}</span>

Bug fix: non-embedded files

The original .map((file) => file.content_type) would fail if files contains plain path strings rather than embedded objects. Adding isEmbeddedArray(analysisSet.files) as a guard before the map is the right approach, and returns an empty array gracefully when files are not embedded.

Sort comparator

Changing .sort() to .sort((a, b) => a.localeCompare(b)) is a good improvement. The default Array.prototype.sort uses locale-dependent behavior in some environments; using localeCompare makes ordering explicit and consistent.

Supplement section reordering

Samples now appears before Files in the supplement area. This matches a more natural reading order (what the data represents → what files contain it) and is a reasonable UX improvement.

isSupplementsVisible and render conditions are now consistent

isSupplementsVisible uses analysisSet.sample_summary (truthy check) and the Samples render condition also uses analysisSet.sample_summary && (truthy check). Both are consistent. An empty string would be treated as absent, which is the correct behaviour for a display field.

No TypeScript errors

No compiler errors in this file.

---

globals.d.ts

Three additions to FileSetObject:

• data_use_limitation_summaries?: string[] — was already being consumed in analysis-set.tsx by <DataUseLimitationSummaries>; this correctly documents the type.
• file_set_type: string (now required) — the component accesses this unconditionally (.split(" ")[0]), so promoting it from optional to required is accurate and tightens type safety.
• sample_summary?: string — correctly optional, matching API semantics.

All three changes are appropriate.

---

lib/workflow.ts

uniform_pipeline is changed from boolean | undefined to a required boolean. The property is accessed in .some((workflow) => workflow.uniform_pipeline) without any null guard, so marking it required is more accurate. No callers break.

---

list-renderer.test.js

A new test case is added to cover the non-embedded-lab scenario: lab is provided as a path string rather than an embedded object. The test verifies that:

1. The meta element is still rendered.
2. The lab title is not displayed (since it cannot be read from a string link).

This directly covers the bug that was fixed and is a clear, minimal test. No issues.

---

Summary

The changes are well-scoped and correct. The two runtime crashes fixed (unguarded lab.title access and unguarded files.map) are meaningful bug fixes. The TypeScript migration is clean, the type additions to globals.d.ts and lib/workflow.ts accurately reflect reality, and the new test covers the primary bug scenario. No issues found.

[forresttanaka]

Code Review Summary: IGVF-3450-analysisset-list-supp

Model

GPT-5.3-Codex

Scope Reviewed

• Compared branch against dev using fresh diff
• Re-read all changed files in the branch
• Reviewed both implementation and tests
• Ran targeted test suites for modified areas

Findings

No blocking issues were found in the current branch state.

What Was Verified

• AnalysisSet list renderer migration from JS to TS in components/search/list-renderer/analysis-set.tsx
• Guard handling for non-embedded lab and non-embedded files
• Added/updated typing for FileSetObject and WorkflowObject
• Test fixture updates required by stricter type definitions
• Coverage for new non-embedded lab case in list renderer tests

Execution Results

• lib/__tests__/files.test.ts: passed
• components/__tests__/workflow.test.tsx: passed
• lib/__tests__/workflow.test.ts: passed
• components/file-graph/__tests__/types.test.ts: passed
• lib/__tests__/ontology-terms.test.ts: passed
• lib/__tests__/batch-download.test.ts: passed
• components/search/__tests__/list-renderer.test.js: passed

Residual Risk

• Full workspace typecheck/build was not fully validated via the configured task because the fallback tsc binary is unavailable in this environment.

Final Conclusion

The branch changes are consistent, test-backed, and ready for merge from a code review perspective, with no defects identified in the reviewed scope.

[Copilot]

Pull request overview

Migrates the AnalysisSet search list renderer to TypeScript while improving robustness around partially embedded API responses, and updates shared typings/tests to reflect required fields used by renderers.

Changes:

• Convert AnalysisSet list renderer to TSX, reorder supplement sections, and guard against non-embedded lab / files.
• Tighten shared types (FileSetObject.file_set_type and WorkflowObject.uniform_pipeline) and update affected tests/fixtures accordingly.
• Extend FileSetObject typing with data_use_limitation_summaries and sample_summary.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
lib/workflow.ts	Make uniform_pipeline required on WorkflowObject.
lib/tests/workflow.test.ts	Update workflow mocks to include uniform_pipeline.
lib/tests/ontology-terms.test.ts	Update FileSetObject mocks for required file_set_type.
lib/tests/files.test.ts	Update FileSetObject mock for required file_set_type and adjust SampleObject type import.
lib/tests/batch-download.test.ts	Update FileSetObject mocks for required file_set_type.
globals.d.ts	Add data_use_limitation_summaries/sample_summary; make file_set_type required.
components/search/list-renderer/analysis-set.tsx	TS migration + guards for non-embedded lab/files; reorder Samples/Files supplements.
components/search/tests/list-renderer.test.js	Add coverage for non-embedded lab rendering behavior.
components/file-graph/tests/types.test.ts	Update mocks for required file_set_type and updated color map shape.
components/tests/workflow.test.tsx	Update workflow mocks to include uniform_pipeline.

Comments suppressed due to low confidence (1)

components/search/list-renderer/analysis-set.tsx:39

• accessoryData is given a default value (= null), which implies callers may omit the prop, but the props type currently makes it required (accessoryData: AccessoryData | null). This can cause TypeScript errors for TS/TSX call sites that render <AnalysisSet item={...} />. Make accessoryData optional (e.g., accessoryData?: AccessoryData | null) to align the type with runtime behavior and other list renderers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.