Skip to content

IGVF-3459 Add file download to pseudobulk-set pages#1046

Merged
forresttanaka merged 1 commit into
devfrom
IGVF-3459-pseudobulk-dpwnload
May 8, 2026
Merged

IGVF-3459 Add file download to pseudobulk-set pages#1046
forresttanaka merged 1 commit into
devfrom
IGVF-3459-pseudobulk-dpwnload

Conversation

@forresttanaka
Copy link
Copy Markdown
Collaborator

Code Review — IGVF-3459-pseudobulk-dpwnload

Reviewed by GitHub Copilot (Claude Sonnet 4.6)


Summary of Changes

This branch converts pages/pseudobulk-sets/[id].js from JavaScript to TypeScript and adds a full pseudobulk set detail page. It also introduces several type-safety and refactoring improvements across the lib layer that benefit multiple pages.


Files Reviewed

File Change
pages/pseudobulk-sets/[id].tsx New — JS→TS conversion, full detail page
lib/file-sets.ts New — extracted file-set request helpers
lib/next-js.ts New — shared PageProps interface
lib/database-object.ts Enhanced type guards, pathsFromDatabaseObjects
lib/common-requests.ts Stronger return types, generic requestSupersedes<T>
lib/files.ts getFilesFileSets, requestFilesQualityMetrics, removed ad-hoc type guards
lib/errors.ts Generic errorObjectToProps<T>
lib/search-results.ts New isSearchResults type guard
lib/facets.ts Use isSearchResults instead of cast
lib/types.ts isEmbeddedArray fix, LinkTo<T> type
globals.d.ts LinkTo<T> on union fields, SearchResults decoupled from DatabaseObject, status required, submitter_comment moved to DatabaseObject
lib/data-grid.ts CellContentProps fields made required
lib/ontology-terms.ts profiles accepts null
components/add.tsx void on unhandled promises
components/file-graph/file-set-modal.tsx Typed grid config, corrected input_file_for guard
components/institutional-certificate-table.tsx Typed grid config, removed redundant casts
components/search/list-renderer/index.tsx Typed reduce accumulators
pages/assays.tsx isMatrixResultsObject guard, cleaner variable naming
pages/computational-tools.tsx Use isSearchResults, remove cast
pages/users/[uuid].tsx Minor type cleanup
lib/__tests__/… Tests added/updated throughout

Observations

  • lib/file-sets.ts is a clean extraction that eliminates copy-pasted patterns across page files.
  • pathsFromDatabaseObjects handles both the embedded-object and path-string cases uniformly, which simplifies all call sites.
  • errorObjectToProps<T> being generic removes as unknown as casts that were scattered across getServerSideProps implementations.
  • isDatabaseObjectArray returning false for empty arrays is consistent with isDatabaseObjectArrayOfType and isEmbeddedArray, but callers should be aware this is a deliberate convention.
  • isSearchResults checking @type.length === 1 is stricter than other type guards in the codebase (which use .includes()). This is fine as long as the backend never adds extra entries to @type on search responses.
  • Making status required on DatabaseObject is correct for well-formed objects but may require attention in test fixtures that omit it — the updated tests handle this.

Conclusion

The branch is in good shape. The overall direction — replacing ad-hoc casts and inline type guards with shared, well-tested utilities — makes the codebase meaningfully safer and easier to maintain. Ready to merge.

@forresttanaka
Copy link
Copy Markdown
Collaborator Author

Code Review: IGVF-3459 Add file download to pseudobulk-set pages

  • Model: GPT-5.3-Codex
  • Branch: IGVF-3459-pseudobulk-dpwnload
  • Compared against: dev...HEAD
  • Date: 2026-05-07

Findings

Low

  1. Global type-guard semantics changed for empty arrays in lib/types.ts.
  • isEmbeddedArray now returns false for empty arrays (value.length > 0 required).
  • This can alter behavior at call sites that previously treated an empty embedded array as valid embedded data.
  • Existing tests cover non-empty and invalid cases, but there is no explicit regression test documenting intended behavior for the empty-array case after this change.

Not a Finding (Intent Clarified)

  • Input file sets on pseudobulk pages are still rendered.
  • Simplified related-set data for pseudobulk input file sets (control/auxiliary/measurement meta arrays) is intentional because those linked types cannot appear for pseudobulk sets.

Testing Notes

  • This review is static (diff-based).
  • I did not execute the full test suite in this review pass.

Conclusion

  • The branch successfully implements pseudobulk file-download related changes and improves type safety across several modules.
  • No blocking issues remain from input file-set simplification because that behavior is intentionally constrained for pseudobulk sets.
  • Remaining review risk is limited to the global empty-array semantics change in isEmbeddedArray.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds/updates pseudobulk-set detail functionality (including file download actions) while converting the pseudobulk-set page to TypeScript and introducing shared request/type-guard utilities across the lib layer to reduce casting and improve safety.

Changes:

  • Replaces the pseudobulk set detail page (pages/pseudobulk-sets/[id]) with a TypeScript implementation and server-side data fetching for associated objects (files, samples, donors, publications, etc.).
  • Adds shared helpers for file-set–related requests and strengthens several shared request utilities/type guards.
  • Updates global typings and expands Jest coverage for the new helpers/guards.

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
pages/pseudobulk-sets/[id].tsx New TS pseudobulk-set detail page with server-side data loading and download UI integration
pages/pseudobulk-sets/[id].js Removes the prior JS implementation of the pseudobulk-set page
pages/users/[uuid].tsx Tightens SSR typing and adds a params not-found guard
pages/computational-tools.tsx Uses isSearchResults guard and updates SSR error handling path
pages/assays.tsx Adds a matrix-response guard and tweaks table header sizing
components/add.tsx Adds void for unhandled promises; adjusts types
components/file-graph/file-set-modal.tsx Strengthens guards for mixed embedded/link fields; improves grid typing
components/institutional-certificate-table.tsx Types the sortable grid configuration and simplifies render signatures
components/search/list-renderer/index.tsx Adds explicit accumulator typings to reduce calls
lib/file-sets.ts New helper module for requesting associated file-set linked objects
lib/files.ts Adds helpers for file-set retrieval and for requesting reference/quality-metric objects
lib/common-requests.ts Strengthens return types and generics for requests (files, quality metrics, supersedes, etc.)
lib/database-object.ts Enhances DB object guards and adds pathsFromDatabaseObjects
lib/errors.ts Makes errorObjectToProps generic for SSR typing
lib/facets.ts Uses isSearchResults instead of casting for facet retrieval
lib/search-results.ts Adds isSearchResults type guard
lib/types.ts Adjusts isEmbeddedArray behavior to require non-empty arrays
lib/ontology-terms.ts Allows profiles to be null in preferred assay title mapping
lib/next-js.ts New shared PageProps shape for SSR-returned page props
lib/data-grid.ts Makes CellContentProps.id and .source required in typings
globals.d.ts Refactors LinkTo<T> usage, strengthens DatabaseObject, and adjusts SearchResults definition
lib/tests/search-results.test.ts Adds tests for isSearchResults
lib/tests/files.test.ts Adds tests for new file helpers and updates fixtures
lib/tests/file-sets.test.ts New tests for lib/file-sets request helpers
lib/tests/facets.test.ts Updates facet tests to satisfy the new isSearchResults guard
lib/tests/database-objects.test.ts New tests for database-object guards and pathsFromDatabaseObjects
lib/tests/common-requests.test.ts Updates fixtures/expectations for typed request changes

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

Comment thread pages/assays.tsx
Comment thread pages/computational-tools.tsx
Comment thread lib/search-results.ts
Comment thread lib/files.ts Outdated
Comment thread pages/pseudobulk-sets/[id].tsx
Comment thread pages/assays.tsx
* Convert pseudobulk sets page to Typescript.
* Move the new functions to retrieve objects related to file sets to a new `lib/file-sets.ts` file. This will have to be merged with the same file in another branch.
* Allow database-object helpers and file-set request utilities to accept arrays of paths as well as embedded objects, including embedded arrays without @type values. This updates pseudobulk set data loading to use the shared path extraction logic and adds donor links to the file-set type definition.
* Add Jest coverage for file set association helpers, including associated file sets, donors, publications, and samples. The tests verify successful bulk fetches, deduped related objects, empty relationship handling, and non-database object inputs.
* Update the Jest tests for lib/common-requests.ts.
* Add tests for database object type guards, array validation, type-specific checks, and path extraction behavior. Update the donor object type so documents can be omitted when absent from donor responses.
* Get full Jest coverage for lib/files.ts.
* Fix build errors from the latest Typescript changes. Update Jest tests with these new changes.
* Relax the search-results guard to accept any @type array and simplify file graph child-file handling now that native file paths are already validated. Also clean up assay comments and remove a stale commented assignment.
* Correct a comment.
@forresttanaka forresttanaka force-pushed the IGVF-3459-pseudobulk-dpwnload branch from 904521c to 2d55ba3 Compare May 8, 2026 00:00
@forresttanaka forresttanaka merged commit cd1261b into dev May 8, 2026
6 checks passed
@forresttanaka forresttanaka deleted the IGVF-3459-pseudobulk-dpwnload branch May 8, 2026 17:55
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.

2 participants