Skip to content

fix!: file format filter dropped from curl manifest request (#543)#544

Merged
NoopDog merged 6 commits into
mainfrom
fran/543-file-manifest
Jun 19, 2025
Merged

fix!: file format filter dropped from curl manifest request (#543)#544
NoopDog merged 6 commits into
mainfrom
fran/543-file-manifest

Conversation

@frano-m

@frano-m frano-m commented Jun 19, 2025

Copy link
Copy Markdown
Contributor

Closes #543.

This pull request introduces enhancements to the file manifest functionality, focusing on improving file count management and refining filter behavior for requests. The changes include adding support for a new speciesFacetName parameter across multiple components, implementing a new hook (useFileManifestFileCount) to manage file counts, and updating the file manifest state and its associated utilities to handle file count logic more effectively.

Enhancements to File Manifest Functionality

Support for speciesFacetName Parameter:

  • Added speciesFacetName to the DownloadCurlCommandProps, ExportToTerraProps, and ManifestDownloadProps interfaces, along with corresponding updates to the useFileManifest calls in downloadCurlCommand.tsx, exportToTerra.tsx, and manifestDownload.tsx to include this parameter. [1] [2] [3] [4] [5] [6]

File Count Management:

  • Introduced the useFileManifestFileCount hook to fetch and dispatch file count data based on filters and excluded facet names. This hook ensures that file count is updated dynamically and is integrated into the useFileManifest hook. [1] [2]
  • Updated the FileManifestState type to include a fileCount property and added a new action (UpdateFileCount) in the file manifest reducer to handle file count updates. [1] [2] [3] [4] [5] [6] [7] [8]

Refinements to Filter Behavior:

  • Replaced the areAllFormFilterTermsSelected utility with areAllFilesSelected, which compares the file count to the summary file count to determine whether all files are selected. Updated the buildRequestFilters function to use this logic for generating request filters. [1] [2]
  • Adjusted related tests to reflect the new file count-based logic for filter behavior. [1] [2]

These changes collectively improve the handling of file manifest data, particularly in scenarios involving large datasets and complex filtering requirements.

Copilot AI left a comment

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.

Pull Request Overview

This PR enhances file manifest functionality by adding support for a new speciesFacetName parameter, introducing a useFileManifestFileCount hook to manage file counts dynamically, and refining filter logic to use file-count comparisons.

  • Support speciesFacetName across manifest hooks and export components
  • Add useFileManifestFileCount hook, update state and reducer to track fileCount
  • Replace term-based selection checks with areAllFilesSelected in buildRequestFilters

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/useRequestManifest.test.ts Add test case for undefined fileCount
tests/buildRequestFilters.test.ts Rename tests to reflect file-count-based filter logic
src/providers/fileManifestState/constants.ts Initialize fileCount in default state
src/providers/fileManifestState.tsx Add fileCount to state and UpdateFileCount action
src/mocks/useRequestFileManifest.mocks.ts Update mock state with fileCount and summary
src/hooks/useRequestManifest/utils.ts Implement areAllFilesSelected and update buildRequestFilters
src/hooks/useFileManifest/useFileManifestFileCount.ts Create hook to fetch and dispatch file count
src/hooks/useFileManifest/useFileManifest.ts Pass speciesFacetName to useFileManifestFileCount
src/components/Export/components/ManifestDownload/manifestDownload.tsx Add speciesFacetName prop
src/components/Export/components/ExportToTerra/exportToTerra.tsx Add speciesFacetName prop
src/components/Export/components/DownloadCurlCommand/downloadCurlCommand.tsx Add speciesFacetName prop
Comments suppressed due to low confidence (3)

src/components/Export/components/ManifestDownload/manifestDownload.tsx:28

  • The speciesFacetName prop is marked required but was previously optional. Consider making it optional (speciesFacetName?: string) to avoid breaking existing consumers and align with fileSummaryFacetName.
  speciesFacetName: string;

src/components/Export/components/ExportToTerra/exportToTerra.tsx:27

  • The speciesFacetName prop is marked required but was previously optional. Consider making it optional (speciesFacetName?: string) to avoid breaking existing consumers and align with fileSummaryFacetName.
  speciesFacetName: string;

src/components/Export/components/DownloadCurlCommand/downloadCurlCommand.tsx:33

  • The speciesFacetName prop is marked required but was previously optional. Consider making it optional (speciesFacetName?: string) to avoid breaking existing consumers and align with fileSummaryFacetName.
  speciesFacetName: string;

Comment thread src/hooks/useRequestManifest/utils.ts Outdated
const { fileCount, summary } = state;

// Return false if file count or summary file count is undefined.
if (!fileCount || !summary?.fileCount) return false;

Copilot AI Jun 19, 2025

Copy link

Choose a reason for hiding this comment

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

Avoid using a falsy check for fileCount or summary.fileCount, as a count of zero will be treated as undefined. Instead, explicitly check for undefined (e.g., fileCount === undefined || summary?.fileCount === undefined).

Suggested change
if (!fileCount || !summary?.fileCount) return false;
if (fileCount === undefined || summary?.fileCount === undefined) return false;

Copilot uses AI. Check for mistakes.
);

useEffect(() => {
if (!fileCount) return;

Copilot AI Jun 19, 2025

Copy link

Choose a reason for hiding this comment

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

Using if (!fileCount) will skip dispatch when fileCount is zero. Use fileCount === undefined to only skip on missing values.

Suggested change
if (!fileCount) return;
if (fileCount === undefined) return;

Copilot uses AI. Check for mistakes.
Comment on lines +21 to +27
const { fileCount, summary } = state;

// Return false if file count or summary file count is undefined.
if (!fileCount || !summary?.fileCount) return false;

// Return true if file count equals summary file count.
return fileCount === summary.fileCount;

Copilot AI Jun 19, 2025

Copy link

Choose a reason for hiding this comment

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

areAllFilesSelected references state.summary, but the state type defines fileSummary. This mismatch means summary is always undefined. Update code (or state) so the correct property (fileSummary) is used or stored.

Suggested change
const { fileCount, summary } = state;
// Return false if file count or summary file count is undefined.
if (!fileCount || !summary?.fileCount) return false;
// Return true if file count equals summary file count.
return fileCount === summary.fileCount;
const { fileCount, fileSummary } = state;
// Return false if file count or fileSummary file count is undefined.
if (!fileCount || !fileSummary?.fileCount) return false;
// Return true if file count equals fileSummary file count.
return fileCount === fileSummary.fileCount;

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

@NoopDog NoopDog 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.

Thanks @frano-m !

@NoopDog NoopDog merged commit 3169f5c into main Jun 19, 2025
2 checks passed
@frano-m frano-m deleted the fran/543-file-manifest branch June 19, 2025 22:02
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.

File format filter dropped from cURL manifest request

3 participants