EDSC-4662: As a user, I would like to set reprojection and know which parameters are still valid#2055
Merged
Merged
Conversation
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR refactors how Harmony access-method capabilities are computed and applied. It introduces a ChangesHarmony Capabilities Infrastructure and Refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Bundle Size ComparisonFull build detailsThe full bundle is smaller than main by -104.62 kB. 🎉 The index.js is smaller than main by -2.97 kB. 🎉 Run
|
mandyparson
requested review from
eudoroolivares2016,
macrouch,
stephenkilbourn and
trevorlang
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/EDSC-4638 #2055 +/- ##
====================================================
Coverage ? 93.54%
====================================================
Files ? 752
Lines ? 17483
Branches ? 4887
====================================================
Hits ? 16355
Misses ? 1017
Partials ? 111 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
static/src/js/components/AccessMethod/AccessMethod.jsx (1)
118-140:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix useEffect dependency array to prevent stale closure bugs.
The effect uses
isTemporalSubsettingSelected,isRecurring,onUpdateAccessMethod,metadata.conceptId, andselectedAccessMethod, but only lists[projectCollection]in the dependency array. This can cause the effect to use stale values if those variables change withoutprojectCollectionchanging.Additionally, the auto-disable logic for recurring temporal subsetting should probably be in a separate
useEffectthat specifically watchesisRecurringandisTemporalSubsettingSelected, rather than being coupled to granule list rebuilding.🔧 Recommended fix
Split into two separate effects:
useEffect(() => { if (addedGranuleIds.length > 0) { granulesToDisplay = addedGranuleIds } else { granulesToDisplay = granulesAllIds } // Build the list of granules const granuleListObj = granulesToDisplay.map((id) => granulesMetadata[id]) setGranuleList(granuleListObj) + }, [projectCollection]) + + // Disable temporal subsetting if the user has a recurring date selected + useEffect(() => { - // Disable temporal subsetting if the user has a recurring date selected if (isTemporalSubsettingSelected && isRecurring) { onUpdateAccessMethod({ collectionId: metadata.conceptId, method: { [selectedAccessMethod]: { enableTemporalSubsetting: false } } }) } - }, [projectCollection]) + }, [isTemporalSubsettingSelected, isRecurring, onUpdateAccessMethod, metadata.conceptId, selectedAccessMethod])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/components/AccessMethod/AccessMethod.jsx` around lines 118 - 140, The current useEffect in AccessMethod.jsx that rebuilds granuleList also references isTemporalSubsettingSelected, isRecurring, onUpdateAccessMethod, metadata.conceptId, and selectedAccessMethod but only lists [projectCollection], causing stale closures; split into two effects: keep the granule-list building effect (useEffect that maps addedGranuleIds/granulesAllIds using granulesMetadata and calls setGranuleList) with dependencies [projectCollection, addedGranuleIds, granulesAllIds, granulesMetadata, setGranuleList], and create a separate effect for the auto-disable logic that watches [isTemporalSubsettingSelected, isRecurring, onUpdateAccessMethod, metadata.conceptId, selectedAccessMethod] and calls onUpdateAccessMethod({ collectionId: metadata.conceptId, method: { [selectedAccessMethod]: { enableTemporalSubsetting: false } } }) only when isTemporalSubsettingSelected && isRecurring to avoid stale values and unintended calls.static/src/js/util/accessMethods/computeKeywordMappings.js (1)
10-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against undefined variable IDs in keyword mapping computation.
The
determineVariableIdhelper returnsstring | undefined, but line 24 pushesvariableIddirectly intocalculatedMappings[leafNode]without checking forundefined. If a variable has neither a validconceptIdnorhref(or has empty strings for both), the resulting keyword mapping children would contain{ id: undefined }, violating theKeywordMappingtype contract that expectsid: string.This creates an inconsistency with
getVariables.ts(line 38 in this PR), which explicitly guards against undefined IDs:if (!variableId) return. For consistency and type safety, skip variables without valid IDs here as well.🛡️ Proposed fix to skip variables without valid IDs
items.forEach((variable) => { const variableId = determineVariableId(variable) + + // Skip variables that don't have a valid ID + if (!variableId) return const { scienceKeywords = [] } = variable🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/accessMethods/computeKeywordMappings.js` around lines 10 - 26, The loop uses determineVariableId(variable) which can return undefined, but variableId is pushed into calculatedMappings[leafNode] unguarded; update the logic in the items.forEach block to skip variables without a valid ID (e.g., if (!variableId) return/continue) before referencing variableId or pushing into calculatedMappings[leafNode], ensuring calculatedMappings only receives string IDs and not undefined.
🧹 Nitpick comments (5)
static/src/js/components/AccessMethod/AccessMethod.jsx (1)
427-434: 💤 Low valueUpdate or remove outdated comment.
The comment says "We filter the formats based on whether their mimeType exists in the ousFormatMapping" but the code no longer performs any such filtering—it simply maps all
supportedOutputFormatsto<option>elements.📝 Suggested clarification
if (isOpendap) { - // We filter the formats based on whether their mimeType exists in the ousFormatMapping, - // then map the result to <option> elements. + // Map each supported format to an <option> element. supportedOutputFormatOptions = supportedOutputFormats .map((format) => (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/components/AccessMethod/AccessMethod.jsx` around lines 427 - 434, The comment above the supportedOutputFormatOptions generation is outdated: it claims filtering by ousFormatMapping but the code simply maps supportedOutputFormats to <option> elements; update or remove the comment to accurately describe current behavior (e.g., "Map supportedOutputFormats to option elements for each format" or remove the sentence) and keep references to supportedOutputFormatOptions, supportedOutputFormats and ousFormatMapping in AccessMethod.jsx only if relevant.static/src/js/util/request/__tests__/harmonyCapabilitiesRequest.test.js (1)
3-13: ⚡ Quick winAdd a unit test for
search()GET/query behavior.This file only validates constructor defaults. Please add a test asserting
search({ collectionId, version })calls GET with query params (not POST body), since that’s the core contract change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/request/__tests__/harmonyCapabilitiesRequest.test.js` around lines 3 - 13, Add a unit test for HarmonyCapabilitiesRequest#search that verifies it performs an HTTP GET with query parameters (not a POST body): instantiate HarmonyCapabilitiesRequest (e.g., new HarmonyCapabilitiesRequest(token, 'prod')), stub or spy the underlying HTTP call used by search (fetch/axios/getClient) and call request.search({ collectionId, version }), then assert the spy was invoked with method 'GET' and the request URL contains the query string ?collectionId=<...>&version=<...> (and no request body was sent). Ensure the test references HarmonyCapabilitiesRequest and its search method so it explicitly checks GET/query behavior.static/src/js/util/request/harmonyCapabilitiesDocumentRequest.ts (1)
4-4: ⚡ Quick winUse a type-only import for
HarmonyRequestParams(line 4).
HarmonyRequestParamsis only used in the type position ofsearch(params: HarmonyRequestParams)—switching toimport type { HarmonyRequestParams } ...avoids implying a runtime import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/request/harmonyCapabilitiesDocumentRequest.ts` at line 4, Change the import of HarmonyRequestParams to a type-only import to avoid implying a runtime dependency: replace the current import statement that brings in HarmonyRequestParams with an "import type { HarmonyRequestParams } ..." and ensure the only usage is the type annotation in the search(params: HarmonyRequestParams) function so no runtime import is retained.static/src/js/types/sharedTypes.ts (1)
10-10: ⚡ Quick winUse a type-only import for
HarmonyScienceKeyword.
static/src/js/types/sharedTypes.tsimportsHarmonyScienceKeywordas a value (line 10) but it’s only used in type positions (scienceKeywords: HarmonyScienceKeyword[], line 578). Switch toimport type { HarmonyScienceKeyword } ...to make intent explicit and align with existingimport typeusage in this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/types/sharedTypes.ts` at line 10, Change the value import of HarmonyScienceKeyword to a type-only import: replace the current import of HarmonyScienceKeyword with an "import type { HarmonyScienceKeyword } ..." so the symbol is only brought in for type-checking (it's used in type positions like scienceKeywords: HarmonyScienceKeyword[]). This makes intent explicit and aligns with existing import type usage; update the import line that currently reads "import { HarmonyScienceKeyword } ..." to use "import type".static/src/js/util/accessMethods/__tests__/getVariables.test.js (1)
11-53: 💤 Low valueConsider consolidating duplicate describe blocks for clarity.
Two top-level
describe('getVariables', ...)blocks exist at lines 11 and 34 with identical names. While this works, it's confusing for maintainability. Consider either:
- Combining into a single describe block with nested describes for 'OPeNDAP variables' and 'Harmony variables', or
- Renaming to
describe('getVariables (OPeNDAP)', ...)anddescribe('getVariables (Harmony)', ...)♻️ Suggested consolidation
describe('getVariables', () => { - describe('when variables exist', () => { - test('correctly formats variables from graphql', () => { + describe('OPeNDAP variables', () => { + test('correctly formats variables from graphql', () => { const { keywordMappings, variables } = getVariables(opendapVariablesResponse) expect(keywordMappings).toEqual(mockOpendapKeywordMappings) expect(variables).toEqual(mockOpendapVariables) }) - }) - - describe('when no variables exist', () => { - test('correctly formats variables from graphql', () => { + + test('correctly formats when no variables exist', () => { const { keywordMappings, variables } = getVariables({ count: 0, items: null }) expect(keywordMappings).toEqual([]) expect(variables).toEqual({}) }) }) -}) - -describe('getVariables', () => { - describe('when variables exist', () => { - test('correctly formats variables from the capabilities document', () => { + + describe('Harmony variables', () => { + test('correctly formats variables from the capabilities document', () => { // Harmony variables come as an array, not an object with `items` const { keywordMappings, variables } = getVariables(harmonyVariablesResponse.items) expect(keywordMappings).toEqual(mockHarmonyKeywordMappings) expect(variables).toEqual(mockHarmonyVariables) }) - }) - - describe('when no variables exist', () => { - test('correctly returns empty data structures', () => { + + test('correctly returns empty data structures when no variables exist', () => { const { keywordMappings, variables } = getVariables([]) expect(keywordMappings).toEqual([]) expect(variables).toEqual({}) }) }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/accessMethods/__tests__/getVariables.test.js` around lines 11 - 53, The tests include two separate top-level describe blocks both named "getVariables" which is confusing; consolidate by either merging them into one describe('getVariables', ...) containing nested describe blocks (e.g., describe('OPeNDAP variables', ...) and describe('Harmony variables', ...) that wrap the existing tests for opendapVariablesResponse and harmonyVariablesResponse respectively) or rename the two top-level describes to unique names (e.g., describe('getVariables (OPeNDAP)') and describe('getVariables (Harmony)')) so each group is clearly identified; locate the blocks that reference getVariables, opendapVariablesResponse, harmonyVariablesResponse, mockOpendapVariables, mockHarmonyVariables, mockOpendapKeywordMappings and mockHarmonyKeywordMappings and apply the chosen consolidation/renaming consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/src/js/components/AccessMethod/__tests__/__mocks__/mocks.ts`:
- Line 26: In the mocks file update the misspelled projection code by changing
the mocked object property crs values from 'EPGS:4313' to the correct
'EPSG:4313' so projection comparisons succeed; locate the crs properties in the
mock objects inside
static/src/js/components/AccessMethod/__tests__/__mocks__/mocks.ts and replace
every occurrence (currently 'EPGS:4313') with 'EPSG:4313'.
In `@static/src/js/components/AccessMethod/AccessMethod.jsx`:
- Line 278: Remove the debug console.log in the AccessMethod component by
deleting the statement that logs "🚀 ~ AccessMethod.jsx:330 ~ AccessMethod ~
accessMethods:" (inside the AccessMethod function/component where accessMethods
is referenced) and ensure no other stray console.debug/console.log calls remain
in that file; keep behavior unchanged otherwise and run linting/tests to confirm
no leftover debug prints.
In `@static/src/js/util/__tests__/determineVariableId.test.ts`:
- Around line 5-43: Add a test to determineVariableId that covers the edge case
where a variable object contains both a UmmSVariable.conceptId and a
HarmonyVariable.href; create a combined fixture (or two objects) with conceptId
set to e.g. "V789-CMR" and href containing a different ID, call
determineVariableId on the object, and assert that the returned value equals the
conceptId (verifying precedence of conceptId over href); reference the
determineVariableId function and the UmmSVariable/HarmonyVariable shapes when
adding the test to static/src/js/util/__tests__/determineVariableId.test.ts.
In `@static/src/js/util/determineVariableId.ts`:
- Around line 18-25: The logic in determineVariableId sets variableId from
variable.conceptId and then always re-checks href, causing href to overwrite
conceptId when both exist; change the second check to an else if so conceptId
takes precedence (i.e., keep the existing if ('conceptId' in variable &&
variable.conceptId) { variableId = variable.conceptId } and make the href branch
else if ('href' in variable && variable.href) { variableId =
variable.href.split('/').pop() }) to enforce the documented fallback behavior.
In `@static/src/js/util/getDerivedHarmonyState/getDerivedHarmonyState.ts`:
- Around line 241-246: The spatial subsetting supported flag is only using
summaryBbox, so change the logic that sets spatialSubset.supported to consider
either bbox OR shape being supported (e.g., use summaryBbox || summaryShape)
when computing summary.subsetting.spatial; update the same pattern where it
appears (also at the other occurrence around line 374). Ensure you reference the
summaryBbox and summaryShape variables and set
summary.subsetting.spatial.supported accordingly while leaving temporal/variable
logic (summaryTemporal/summaryVariable) unchanged.
In `@static/src/js/zustand/slices/createProjectSlice.ts`:
- Around line 280-316: The current map callback returns buildPromise(null) on a
Harmony 401, which only exits the individual callback and doesn't short-circuit
Promise.all; change that so the outer Promise.all rejects immediately by
re-throwing the 401 error (or returning Promise.reject(error)) instead of
returning buildPromise(null). Locate the filteredIds.map async callback in
createProjectSlice.ts (the block that constructs
HarmonyCapabilitiesDocumentRequest and calls .search), and replace the `return
buildPromise(null)` inside the `if (error instanceof AxiosError &&
error.response?.status === 401)` branch with `throw error` (or `return
Promise.reject(error)`) so Promise.all rejects and the outer flow stops; keep
the existing zustandState.errors.handleError call intact.
- Around line 779-787: The code destructures selectedAccessMethod and
accessMethods from collection without first ensuring collection exists, so if
the collection was removed the destructure will throw; update the setter to
first check that const collection = byId[collectionId] is truthy (or bail out if
undefined) before doing const { selectedAccessMethod, accessMethods } =
collection — e.g., guard collection (and keep the existing early return for
missing selectedAccessMethod/accessMethods) so the updateAccessMethod branch /
set((state) => { ... }) no-ops safely when the collection is absent.
- Around line 795-850: The current update manually copies selection flags into
harmonyMethod after computing getDerivedHarmonyState, which leaves variables,
hierarchyMappings, and keywordMappings stale; instead, rebuild the entire
Harmony method object using the new derivedHarmonyState (i.e. call the existing
buildHarmony function or the routine that constructs a fresh harmonyMethod)
using updatedSelections and the new derivedHarmonyState, then replace
harmonyMethod with that rebuilt object so variables, hierarchyMappings,
keywordMappings and all availability flags come from the canonical build rather
than piecemeal assignment; ensure you preserve any necessary identity/refs
expected elsewhere (or update callers) and remove the manual field-by-field
syncing that sets enableTemporalSubsetting, enableSpatialSubsetting,
selectedVariables, outputFormatAvailability, etc.
In `@tests/e2e/history/history.spec.js`:
- Around line 89-93: The current page.route matcher uses an exact query-string
and is brittle; update the mock in the tests to match the /capabilities path
more robustly by using a looser route pattern (e.g., '**/capabilities**') and
then inspect the actual request URL inside the handler via route.request().url()
or new URL(...). Check searchParams for collectionId === conceptId and version
=== '2', call route.fulfill({ json: { services: [] } }) when it matches,
otherwise call route.continue() so other queries are not incorrectly mocked.
In `@tests/e2e/page_titles/page_titles.spec.js`:
- Around line 166-170: The route currently matches a full glob including query
params which is fragile; change the page.route registration for the capabilities
endpoint to match the base path (e.g., '**/capabilities') and inside the route
handler inspect route.request().url (or construct a URL object) to check
searchParams for collectionId and version, then call route.fulfill({ json: {
services: [] } }) only when those params match; update the handler around the
existing page.route callback and keep using route.fulfill to respond.
---
Outside diff comments:
In `@static/src/js/components/AccessMethod/AccessMethod.jsx`:
- Around line 118-140: The current useEffect in AccessMethod.jsx that rebuilds
granuleList also references isTemporalSubsettingSelected, isRecurring,
onUpdateAccessMethod, metadata.conceptId, and selectedAccessMethod but only
lists [projectCollection], causing stale closures; split into two effects: keep
the granule-list building effect (useEffect that maps
addedGranuleIds/granulesAllIds using granulesMetadata and calls setGranuleList)
with dependencies [projectCollection, addedGranuleIds, granulesAllIds,
granulesMetadata, setGranuleList], and create a separate effect for the
auto-disable logic that watches [isTemporalSubsettingSelected, isRecurring,
onUpdateAccessMethod, metadata.conceptId, selectedAccessMethod] and calls
onUpdateAccessMethod({ collectionId: metadata.conceptId, method: {
[selectedAccessMethod]: { enableTemporalSubsetting: false } } }) only when
isTemporalSubsettingSelected && isRecurring to avoid stale values and unintended
calls.
In `@static/src/js/util/accessMethods/computeKeywordMappings.js`:
- Around line 10-26: The loop uses determineVariableId(variable) which can
return undefined, but variableId is pushed into calculatedMappings[leafNode]
unguarded; update the logic in the items.forEach block to skip variables without
a valid ID (e.g., if (!variableId) return/continue) before referencing
variableId or pushing into calculatedMappings[leafNode], ensuring
calculatedMappings only receives string IDs and not undefined.
---
Nitpick comments:
In `@static/src/js/components/AccessMethod/AccessMethod.jsx`:
- Around line 427-434: The comment above the supportedOutputFormatOptions
generation is outdated: it claims filtering by ousFormatMapping but the code
simply maps supportedOutputFormats to <option> elements; update or remove the
comment to accurately describe current behavior (e.g., "Map
supportedOutputFormats to option elements for each format" or remove the
sentence) and keep references to supportedOutputFormatOptions,
supportedOutputFormats and ousFormatMapping in AccessMethod.jsx only if
relevant.
In `@static/src/js/types/sharedTypes.ts`:
- Line 10: Change the value import of HarmonyScienceKeyword to a type-only
import: replace the current import of HarmonyScienceKeyword with an "import type
{ HarmonyScienceKeyword } ..." so the symbol is only brought in for
type-checking (it's used in type positions like scienceKeywords:
HarmonyScienceKeyword[]). This makes intent explicit and aligns with existing
import type usage; update the import line that currently reads "import {
HarmonyScienceKeyword } ..." to use "import type".
In `@static/src/js/util/accessMethods/__tests__/getVariables.test.js`:
- Around line 11-53: The tests include two separate top-level describe blocks
both named "getVariables" which is confusing; consolidate by either merging them
into one describe('getVariables', ...) containing nested describe blocks (e.g.,
describe('OPeNDAP variables', ...) and describe('Harmony variables', ...) that
wrap the existing tests for opendapVariablesResponse and
harmonyVariablesResponse respectively) or rename the two top-level describes to
unique names (e.g., describe('getVariables (OPeNDAP)') and
describe('getVariables (Harmony)')) so each group is clearly identified; locate
the blocks that reference getVariables, opendapVariablesResponse,
harmonyVariablesResponse, mockOpendapVariables, mockHarmonyVariables,
mockOpendapKeywordMappings and mockHarmonyKeywordMappings and apply the chosen
consolidation/renaming consistently.
In `@static/src/js/util/request/__tests__/harmonyCapabilitiesRequest.test.js`:
- Around line 3-13: Add a unit test for HarmonyCapabilitiesRequest#search that
verifies it performs an HTTP GET with query parameters (not a POST body):
instantiate HarmonyCapabilitiesRequest (e.g., new
HarmonyCapabilitiesRequest(token, 'prod')), stub or spy the underlying HTTP call
used by search (fetch/axios/getClient) and call request.search({ collectionId,
version }), then assert the spy was invoked with method 'GET' and the request
URL contains the query string ?collectionId=<...>&version=<...> (and no request
body was sent). Ensure the test references HarmonyCapabilitiesRequest and its
search method so it explicitly checks GET/query behavior.
In `@static/src/js/util/request/harmonyCapabilitiesDocumentRequest.ts`:
- Line 4: Change the import of HarmonyRequestParams to a type-only import to
avoid implying a runtime dependency: replace the current import statement that
brings in HarmonyRequestParams with an "import type { HarmonyRequestParams }
..." and ensure the only usage is the type annotation in the search(params:
HarmonyRequestParams) function so no runtime import is retained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2bec23f-1c65-4823-9009-5fa4cfd148ce
📒 Files selected for processing (57)
serverless/src/graphQl/resolvers/__tests__/retrieval.test.jssharedUtils/outputFormatMaps.jsstatic.config.jsonstatic/src/js/components/AccessMethod/AccessMethod.jsxstatic/src/js/components/AccessMethod/__tests__/AccessMethod.test.jsxstatic/src/js/components/AccessMethod/__tests__/EchoForm.test.tsxstatic/src/js/components/AccessMethod/__tests__/__mocks__/mocks.tsstatic/src/js/components/AccessMethod/__tests__/mocks.tsstatic/src/js/components/OrderStatus/__tests__/OrderStatusCollection.test.tsxstatic/src/js/components/ProjectPanels/ProjectPanels.jsxstatic/src/js/components/ProjectPanels/__tests__/ProjectPanels.test.jsxstatic/src/js/hooks/__tests__/useCreateRetrieval.test.tsxstatic/src/js/hooks/useCreateRetrieval.tsstatic/src/js/types/sharedTypes.tsstatic/src/js/util/__tests__/determineVariableId.test.tsstatic/src/js/util/__tests__/retrievals.test.jsstatic/src/js/util/accessMethods/__tests__/buildAccessMethods.test.jsstatic/src/js/util/accessMethods/__tests__/buildAccessMethods.test.tsstatic/src/js/util/accessMethods/__tests__/computeKeywordMappings.test.jsstatic/src/js/util/accessMethods/__tests__/defaultConcatenation.test.jsstatic/src/js/util/accessMethods/__tests__/getVariables.test.jsstatic/src/js/util/accessMethods/__tests__/insertSavedAccessConfig.test.jsstatic/src/js/util/accessMethods/__tests__/mocks.jsstatic/src/js/util/accessMethods/__tests__/supportsBoundingBoxSubsetting.test.jsstatic/src/js/util/accessMethods/__tests__/supportsConcatenation.test.jsstatic/src/js/util/accessMethods/__tests__/supportsShapefileSubsetting.test.jsstatic/src/js/util/accessMethods/__tests__/supportsTemporalSubsetting.test.jsstatic/src/js/util/accessMethods/buildAccessMethods.jsstatic/src/js/util/accessMethods/buildAccessMethods.tsstatic/src/js/util/accessMethods/buildAccessMethods/__tests__/buildHarmony.test.jsstatic/src/js/util/accessMethods/buildAccessMethods/__tests__/buildHarmony.test.tsstatic/src/js/util/accessMethods/buildAccessMethods/__tests__/buildOpendap.test.jsstatic/src/js/util/accessMethods/buildAccessMethods/buildHarmony.jsstatic/src/js/util/accessMethods/buildAccessMethods/buildHarmony.tsstatic/src/js/util/accessMethods/buildAccessMethods/buildOpendap.jsstatic/src/js/util/accessMethods/computeHierarchyMappings.jsstatic/src/js/util/accessMethods/computeKeywordMappings.jsstatic/src/js/util/accessMethods/defaultConcatenation.jsstatic/src/js/util/accessMethods/getVariables.jsstatic/src/js/util/accessMethods/getVariables.tsstatic/src/js/util/accessMethods/supportsBoundingBoxSubsetting.jsstatic/src/js/util/accessMethods/supportsConcatenation.jsstatic/src/js/util/accessMethods/supportsShapefileSubsetting.jsstatic/src/js/util/accessMethods/supportsTemporalSubsetting.jsstatic/src/js/util/determineVariableId.tsstatic/src/js/util/getDerivedHarmonyState/__tests__/__mocks__/mocks.tsstatic/src/js/util/getDerivedHarmonyState/__tests__/getDerivedHarmonyState.test.tsstatic/src/js/util/getDerivedHarmonyState/getDerivedHarmonyState.tsstatic/src/js/util/request/__tests__/harmonyCapabilitiesRequest.test.jsstatic/src/js/util/request/harmonyCapabilitiesDocumentRequest.tsstatic/src/js/util/request/request.tsstatic/src/js/util/retrievals.jsstatic/src/js/zustand/slices/__tests__/createProjectSlice.test.tsstatic/src/js/zustand/slices/createProjectSlice.tsstatic/src/js/zustand/types.tstests/e2e/history/history.spec.jstests/e2e/page_titles/page_titles.spec.js
💤 Files with no reviewable changes (19)
- static/src/js/util/accessMethods/tests/defaultConcatenation.test.js
- static/src/js/util/accessMethods/buildAccessMethods/tests/buildHarmony.test.js
- static/src/js/util/accessMethods/defaultConcatenation.js
- static/src/js/util/accessMethods/supportsBoundingBoxSubsetting.js
- static/src/js/util/accessMethods/supportsShapefileSubsetting.js
- static/src/js/util/accessMethods/supportsTemporalSubsetting.js
- static/src/js/util/accessMethods/tests/supportsBoundingBoxSubsetting.test.js
- static/src/js/components/OrderStatus/tests/OrderStatusCollection.test.tsx
- static/src/js/util/accessMethods/buildAccessMethods/buildHarmony.js
- static/src/js/components/AccessMethod/tests/mocks.ts
- static/src/js/util/accessMethods/tests/supportsTemporalSubsetting.test.js
- static/src/js/util/accessMethods/supportsConcatenation.js
- static/src/js/util/accessMethods/buildAccessMethods.js
- static/src/js/util/accessMethods/getVariables.js
- static/src/js/util/accessMethods/tests/supportsShapefileSubsetting.test.js
- serverless/src/graphQl/resolvers/tests/retrieval.test.js
- static/src/js/util/retrievals.js
- sharedUtils/outputFormatMaps.js
- static/src/js/util/accessMethods/tests/supportsConcatenation.test.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
static/src/js/components/AccessMethod/AccessMethod.jsx (1)
118-140:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix useEffect dependency array to prevent stale closure bugs.
The effect uses
isTemporalSubsettingSelected,isRecurring,onUpdateAccessMethod,metadata.conceptId, andselectedAccessMethod, but only lists[projectCollection]in the dependency array. This can cause the effect to use stale values if those variables change withoutprojectCollectionchanging.Additionally, the auto-disable logic for recurring temporal subsetting should probably be in a separate
useEffectthat specifically watchesisRecurringandisTemporalSubsettingSelected, rather than being coupled to granule list rebuilding.🔧 Recommended fix
Split into two separate effects:
useEffect(() => { if (addedGranuleIds.length > 0) { granulesToDisplay = addedGranuleIds } else { granulesToDisplay = granulesAllIds } // Build the list of granules const granuleListObj = granulesToDisplay.map((id) => granulesMetadata[id]) setGranuleList(granuleListObj) + }, [projectCollection]) + + // Disable temporal subsetting if the user has a recurring date selected + useEffect(() => { - // Disable temporal subsetting if the user has a recurring date selected if (isTemporalSubsettingSelected && isRecurring) { onUpdateAccessMethod({ collectionId: metadata.conceptId, method: { [selectedAccessMethod]: { enableTemporalSubsetting: false } } }) } - }, [projectCollection]) + }, [isTemporalSubsettingSelected, isRecurring, onUpdateAccessMethod, metadata.conceptId, selectedAccessMethod])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/components/AccessMethod/AccessMethod.jsx` around lines 118 - 140, The current useEffect in AccessMethod.jsx that rebuilds granuleList also references isTemporalSubsettingSelected, isRecurring, onUpdateAccessMethod, metadata.conceptId, and selectedAccessMethod but only lists [projectCollection], causing stale closures; split into two effects: keep the granule-list building effect (useEffect that maps addedGranuleIds/granulesAllIds using granulesMetadata and calls setGranuleList) with dependencies [projectCollection, addedGranuleIds, granulesAllIds, granulesMetadata, setGranuleList], and create a separate effect for the auto-disable logic that watches [isTemporalSubsettingSelected, isRecurring, onUpdateAccessMethod, metadata.conceptId, selectedAccessMethod] and calls onUpdateAccessMethod({ collectionId: metadata.conceptId, method: { [selectedAccessMethod]: { enableTemporalSubsetting: false } } }) only when isTemporalSubsettingSelected && isRecurring to avoid stale values and unintended calls.static/src/js/util/accessMethods/computeKeywordMappings.js (1)
10-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against undefined variable IDs in keyword mapping computation.
The
determineVariableIdhelper returnsstring | undefined, but line 24 pushesvariableIddirectly intocalculatedMappings[leafNode]without checking forundefined. If a variable has neither a validconceptIdnorhref(or has empty strings for both), the resulting keyword mapping children would contain{ id: undefined }, violating theKeywordMappingtype contract that expectsid: string.This creates an inconsistency with
getVariables.ts(line 38 in this PR), which explicitly guards against undefined IDs:if (!variableId) return. For consistency and type safety, skip variables without valid IDs here as well.🛡️ Proposed fix to skip variables without valid IDs
items.forEach((variable) => { const variableId = determineVariableId(variable) + + // Skip variables that don't have a valid ID + if (!variableId) return const { scienceKeywords = [] } = variable🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/accessMethods/computeKeywordMappings.js` around lines 10 - 26, The loop uses determineVariableId(variable) which can return undefined, but variableId is pushed into calculatedMappings[leafNode] unguarded; update the logic in the items.forEach block to skip variables without a valid ID (e.g., if (!variableId) return/continue) before referencing variableId or pushing into calculatedMappings[leafNode], ensuring calculatedMappings only receives string IDs and not undefined.
🧹 Nitpick comments (5)
static/src/js/components/AccessMethod/AccessMethod.jsx (1)
427-434: 💤 Low valueUpdate or remove outdated comment.
The comment says "We filter the formats based on whether their mimeType exists in the ousFormatMapping" but the code no longer performs any such filtering—it simply maps all
supportedOutputFormatsto<option>elements.📝 Suggested clarification
if (isOpendap) { - // We filter the formats based on whether their mimeType exists in the ousFormatMapping, - // then map the result to <option> elements. + // Map each supported format to an <option> element. supportedOutputFormatOptions = supportedOutputFormats .map((format) => (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/components/AccessMethod/AccessMethod.jsx` around lines 427 - 434, The comment above the supportedOutputFormatOptions generation is outdated: it claims filtering by ousFormatMapping but the code simply maps supportedOutputFormats to <option> elements; update or remove the comment to accurately describe current behavior (e.g., "Map supportedOutputFormats to option elements for each format" or remove the sentence) and keep references to supportedOutputFormatOptions, supportedOutputFormats and ousFormatMapping in AccessMethod.jsx only if relevant.static/src/js/util/request/__tests__/harmonyCapabilitiesRequest.test.js (1)
3-13: ⚡ Quick winAdd a unit test for
search()GET/query behavior.This file only validates constructor defaults. Please add a test asserting
search({ collectionId, version })calls GET with query params (not POST body), since that’s the core contract change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/request/__tests__/harmonyCapabilitiesRequest.test.js` around lines 3 - 13, Add a unit test for HarmonyCapabilitiesRequest#search that verifies it performs an HTTP GET with query parameters (not a POST body): instantiate HarmonyCapabilitiesRequest (e.g., new HarmonyCapabilitiesRequest(token, 'prod')), stub or spy the underlying HTTP call used by search (fetch/axios/getClient) and call request.search({ collectionId, version }), then assert the spy was invoked with method 'GET' and the request URL contains the query string ?collectionId=<...>&version=<...> (and no request body was sent). Ensure the test references HarmonyCapabilitiesRequest and its search method so it explicitly checks GET/query behavior.static/src/js/util/request/harmonyCapabilitiesDocumentRequest.ts (1)
4-4: ⚡ Quick winUse a type-only import for
HarmonyRequestParams(line 4).
HarmonyRequestParamsis only used in the type position ofsearch(params: HarmonyRequestParams)—switching toimport type { HarmonyRequestParams } ...avoids implying a runtime import.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/request/harmonyCapabilitiesDocumentRequest.ts` at line 4, Change the import of HarmonyRequestParams to a type-only import to avoid implying a runtime dependency: replace the current import statement that brings in HarmonyRequestParams with an "import type { HarmonyRequestParams } ..." and ensure the only usage is the type annotation in the search(params: HarmonyRequestParams) function so no runtime import is retained.static/src/js/types/sharedTypes.ts (1)
10-10: ⚡ Quick winUse a type-only import for
HarmonyScienceKeyword.
static/src/js/types/sharedTypes.tsimportsHarmonyScienceKeywordas a value (line 10) but it’s only used in type positions (scienceKeywords: HarmonyScienceKeyword[], line 578). Switch toimport type { HarmonyScienceKeyword } ...to make intent explicit and align with existingimport typeusage in this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/types/sharedTypes.ts` at line 10, Change the value import of HarmonyScienceKeyword to a type-only import: replace the current import of HarmonyScienceKeyword with an "import type { HarmonyScienceKeyword } ..." so the symbol is only brought in for type-checking (it's used in type positions like scienceKeywords: HarmonyScienceKeyword[]). This makes intent explicit and aligns with existing import type usage; update the import line that currently reads "import { HarmonyScienceKeyword } ..." to use "import type".static/src/js/util/accessMethods/__tests__/getVariables.test.js (1)
11-53: 💤 Low valueConsider consolidating duplicate describe blocks for clarity.
Two top-level
describe('getVariables', ...)blocks exist at lines 11 and 34 with identical names. While this works, it's confusing for maintainability. Consider either:
- Combining into a single describe block with nested describes for 'OPeNDAP variables' and 'Harmony variables', or
- Renaming to
describe('getVariables (OPeNDAP)', ...)anddescribe('getVariables (Harmony)', ...)♻️ Suggested consolidation
describe('getVariables', () => { - describe('when variables exist', () => { - test('correctly formats variables from graphql', () => { + describe('OPeNDAP variables', () => { + test('correctly formats variables from graphql', () => { const { keywordMappings, variables } = getVariables(opendapVariablesResponse) expect(keywordMappings).toEqual(mockOpendapKeywordMappings) expect(variables).toEqual(mockOpendapVariables) }) - }) - - describe('when no variables exist', () => { - test('correctly formats variables from graphql', () => { + + test('correctly formats when no variables exist', () => { const { keywordMappings, variables } = getVariables({ count: 0, items: null }) expect(keywordMappings).toEqual([]) expect(variables).toEqual({}) }) }) -}) - -describe('getVariables', () => { - describe('when variables exist', () => { - test('correctly formats variables from the capabilities document', () => { + + describe('Harmony variables', () => { + test('correctly formats variables from the capabilities document', () => { // Harmony variables come as an array, not an object with `items` const { keywordMappings, variables } = getVariables(harmonyVariablesResponse.items) expect(keywordMappings).toEqual(mockHarmonyKeywordMappings) expect(variables).toEqual(mockHarmonyVariables) }) - }) - - describe('when no variables exist', () => { - test('correctly returns empty data structures', () => { + + test('correctly returns empty data structures when no variables exist', () => { const { keywordMappings, variables } = getVariables([]) expect(keywordMappings).toEqual([]) expect(variables).toEqual({}) }) }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/accessMethods/__tests__/getVariables.test.js` around lines 11 - 53, The tests include two separate top-level describe blocks both named "getVariables" which is confusing; consolidate by either merging them into one describe('getVariables', ...) containing nested describe blocks (e.g., describe('OPeNDAP variables', ...) and describe('Harmony variables', ...) that wrap the existing tests for opendapVariablesResponse and harmonyVariablesResponse respectively) or rename the two top-level describes to unique names (e.g., describe('getVariables (OPeNDAP)') and describe('getVariables (Harmony)')) so each group is clearly identified; locate the blocks that reference getVariables, opendapVariablesResponse, harmonyVariablesResponse, mockOpendapVariables, mockHarmonyVariables, mockOpendapKeywordMappings and mockHarmonyKeywordMappings and apply the chosen consolidation/renaming consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/src/js/components/AccessMethod/__tests__/__mocks__/mocks.ts`:
- Line 26: In the mocks file update the misspelled projection code by changing
the mocked object property crs values from 'EPGS:4313' to the correct
'EPSG:4313' so projection comparisons succeed; locate the crs properties in the
mock objects inside
static/src/js/components/AccessMethod/__tests__/__mocks__/mocks.ts and replace
every occurrence (currently 'EPGS:4313') with 'EPSG:4313'.
In `@static/src/js/components/AccessMethod/AccessMethod.jsx`:
- Line 278: Remove the debug console.log in the AccessMethod component by
deleting the statement that logs "🚀 ~ AccessMethod.jsx:330 ~ AccessMethod ~
accessMethods:" (inside the AccessMethod function/component where accessMethods
is referenced) and ensure no other stray console.debug/console.log calls remain
in that file; keep behavior unchanged otherwise and run linting/tests to confirm
no leftover debug prints.
In `@static/src/js/util/__tests__/determineVariableId.test.ts`:
- Around line 5-43: Add a test to determineVariableId that covers the edge case
where a variable object contains both a UmmSVariable.conceptId and a
HarmonyVariable.href; create a combined fixture (or two objects) with conceptId
set to e.g. "V789-CMR" and href containing a different ID, call
determineVariableId on the object, and assert that the returned value equals the
conceptId (verifying precedence of conceptId over href); reference the
determineVariableId function and the UmmSVariable/HarmonyVariable shapes when
adding the test to static/src/js/util/__tests__/determineVariableId.test.ts.
In `@static/src/js/util/determineVariableId.ts`:
- Around line 18-25: The logic in determineVariableId sets variableId from
variable.conceptId and then always re-checks href, causing href to overwrite
conceptId when both exist; change the second check to an else if so conceptId
takes precedence (i.e., keep the existing if ('conceptId' in variable &&
variable.conceptId) { variableId = variable.conceptId } and make the href branch
else if ('href' in variable && variable.href) { variableId =
variable.href.split('/').pop() }) to enforce the documented fallback behavior.
In `@static/src/js/util/getDerivedHarmonyState/getDerivedHarmonyState.ts`:
- Around line 241-246: The spatial subsetting supported flag is only using
summaryBbox, so change the logic that sets spatialSubset.supported to consider
either bbox OR shape being supported (e.g., use summaryBbox || summaryShape)
when computing summary.subsetting.spatial; update the same pattern where it
appears (also at the other occurrence around line 374). Ensure you reference the
summaryBbox and summaryShape variables and set
summary.subsetting.spatial.supported accordingly while leaving temporal/variable
logic (summaryTemporal/summaryVariable) unchanged.
In `@static/src/js/zustand/slices/createProjectSlice.ts`:
- Around line 280-316: The current map callback returns buildPromise(null) on a
Harmony 401, which only exits the individual callback and doesn't short-circuit
Promise.all; change that so the outer Promise.all rejects immediately by
re-throwing the 401 error (or returning Promise.reject(error)) instead of
returning buildPromise(null). Locate the filteredIds.map async callback in
createProjectSlice.ts (the block that constructs
HarmonyCapabilitiesDocumentRequest and calls .search), and replace the `return
buildPromise(null)` inside the `if (error instanceof AxiosError &&
error.response?.status === 401)` branch with `throw error` (or `return
Promise.reject(error)`) so Promise.all rejects and the outer flow stops; keep
the existing zustandState.errors.handleError call intact.
- Around line 779-787: The code destructures selectedAccessMethod and
accessMethods from collection without first ensuring collection exists, so if
the collection was removed the destructure will throw; update the setter to
first check that const collection = byId[collectionId] is truthy (or bail out if
undefined) before doing const { selectedAccessMethod, accessMethods } =
collection — e.g., guard collection (and keep the existing early return for
missing selectedAccessMethod/accessMethods) so the updateAccessMethod branch /
set((state) => { ... }) no-ops safely when the collection is absent.
- Around line 795-850: The current update manually copies selection flags into
harmonyMethod after computing getDerivedHarmonyState, which leaves variables,
hierarchyMappings, and keywordMappings stale; instead, rebuild the entire
Harmony method object using the new derivedHarmonyState (i.e. call the existing
buildHarmony function or the routine that constructs a fresh harmonyMethod)
using updatedSelections and the new derivedHarmonyState, then replace
harmonyMethod with that rebuilt object so variables, hierarchyMappings,
keywordMappings and all availability flags come from the canonical build rather
than piecemeal assignment; ensure you preserve any necessary identity/refs
expected elsewhere (or update callers) and remove the manual field-by-field
syncing that sets enableTemporalSubsetting, enableSpatialSubsetting,
selectedVariables, outputFormatAvailability, etc.
In `@tests/e2e/history/history.spec.js`:
- Around line 89-93: The current page.route matcher uses an exact query-string
and is brittle; update the mock in the tests to match the /capabilities path
more robustly by using a looser route pattern (e.g., '**/capabilities**') and
then inspect the actual request URL inside the handler via route.request().url()
or new URL(...). Check searchParams for collectionId === conceptId and version
=== '2', call route.fulfill({ json: { services: [] } }) when it matches,
otherwise call route.continue() so other queries are not incorrectly mocked.
In `@tests/e2e/page_titles/page_titles.spec.js`:
- Around line 166-170: The route currently matches a full glob including query
params which is fragile; change the page.route registration for the capabilities
endpoint to match the base path (e.g., '**/capabilities') and inside the route
handler inspect route.request().url (or construct a URL object) to check
searchParams for collectionId and version, then call route.fulfill({ json: {
services: [] } }) only when those params match; update the handler around the
existing page.route callback and keep using route.fulfill to respond.
---
Outside diff comments:
In `@static/src/js/components/AccessMethod/AccessMethod.jsx`:
- Around line 118-140: The current useEffect in AccessMethod.jsx that rebuilds
granuleList also references isTemporalSubsettingSelected, isRecurring,
onUpdateAccessMethod, metadata.conceptId, and selectedAccessMethod but only
lists [projectCollection], causing stale closures; split into two effects: keep
the granule-list building effect (useEffect that maps
addedGranuleIds/granulesAllIds using granulesMetadata and calls setGranuleList)
with dependencies [projectCollection, addedGranuleIds, granulesAllIds,
granulesMetadata, setGranuleList], and create a separate effect for the
auto-disable logic that watches [isTemporalSubsettingSelected, isRecurring,
onUpdateAccessMethod, metadata.conceptId, selectedAccessMethod] and calls
onUpdateAccessMethod({ collectionId: metadata.conceptId, method: {
[selectedAccessMethod]: { enableTemporalSubsetting: false } } }) only when
isTemporalSubsettingSelected && isRecurring to avoid stale values and unintended
calls.
In `@static/src/js/util/accessMethods/computeKeywordMappings.js`:
- Around line 10-26: The loop uses determineVariableId(variable) which can
return undefined, but variableId is pushed into calculatedMappings[leafNode]
unguarded; update the logic in the items.forEach block to skip variables without
a valid ID (e.g., if (!variableId) return/continue) before referencing
variableId or pushing into calculatedMappings[leafNode], ensuring
calculatedMappings only receives string IDs and not undefined.
---
Nitpick comments:
In `@static/src/js/components/AccessMethod/AccessMethod.jsx`:
- Around line 427-434: The comment above the supportedOutputFormatOptions
generation is outdated: it claims filtering by ousFormatMapping but the code
simply maps supportedOutputFormats to <option> elements; update or remove the
comment to accurately describe current behavior (e.g., "Map
supportedOutputFormats to option elements for each format" or remove the
sentence) and keep references to supportedOutputFormatOptions,
supportedOutputFormats and ousFormatMapping in AccessMethod.jsx only if
relevant.
In `@static/src/js/types/sharedTypes.ts`:
- Line 10: Change the value import of HarmonyScienceKeyword to a type-only
import: replace the current import of HarmonyScienceKeyword with an "import type
{ HarmonyScienceKeyword } ..." so the symbol is only brought in for
type-checking (it's used in type positions like scienceKeywords:
HarmonyScienceKeyword[]). This makes intent explicit and aligns with existing
import type usage; update the import line that currently reads "import {
HarmonyScienceKeyword } ..." to use "import type".
In `@static/src/js/util/accessMethods/__tests__/getVariables.test.js`:
- Around line 11-53: The tests include two separate top-level describe blocks
both named "getVariables" which is confusing; consolidate by either merging them
into one describe('getVariables', ...) containing nested describe blocks (e.g.,
describe('OPeNDAP variables', ...) and describe('Harmony variables', ...) that
wrap the existing tests for opendapVariablesResponse and
harmonyVariablesResponse respectively) or rename the two top-level describes to
unique names (e.g., describe('getVariables (OPeNDAP)') and
describe('getVariables (Harmony)')) so each group is clearly identified; locate
the blocks that reference getVariables, opendapVariablesResponse,
harmonyVariablesResponse, mockOpendapVariables, mockHarmonyVariables,
mockOpendapKeywordMappings and mockHarmonyKeywordMappings and apply the chosen
consolidation/renaming consistently.
In `@static/src/js/util/request/__tests__/harmonyCapabilitiesRequest.test.js`:
- Around line 3-13: Add a unit test for HarmonyCapabilitiesRequest#search that
verifies it performs an HTTP GET with query parameters (not a POST body):
instantiate HarmonyCapabilitiesRequest (e.g., new
HarmonyCapabilitiesRequest(token, 'prod')), stub or spy the underlying HTTP call
used by search (fetch/axios/getClient) and call request.search({ collectionId,
version }), then assert the spy was invoked with method 'GET' and the request
URL contains the query string ?collectionId=<...>&version=<...> (and no request
body was sent). Ensure the test references HarmonyCapabilitiesRequest and its
search method so it explicitly checks GET/query behavior.
In `@static/src/js/util/request/harmonyCapabilitiesDocumentRequest.ts`:
- Line 4: Change the import of HarmonyRequestParams to a type-only import to
avoid implying a runtime dependency: replace the current import statement that
brings in HarmonyRequestParams with an "import type { HarmonyRequestParams }
..." and ensure the only usage is the type annotation in the search(params:
HarmonyRequestParams) function so no runtime import is retained.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2bec23f-1c65-4823-9009-5fa4cfd148ce
📒 Files selected for processing (57)
serverless/src/graphQl/resolvers/__tests__/retrieval.test.jssharedUtils/outputFormatMaps.jsstatic.config.jsonstatic/src/js/components/AccessMethod/AccessMethod.jsxstatic/src/js/components/AccessMethod/__tests__/AccessMethod.test.jsxstatic/src/js/components/AccessMethod/__tests__/EchoForm.test.tsxstatic/src/js/components/AccessMethod/__tests__/__mocks__/mocks.tsstatic/src/js/components/AccessMethod/__tests__/mocks.tsstatic/src/js/components/OrderStatus/__tests__/OrderStatusCollection.test.tsxstatic/src/js/components/ProjectPanels/ProjectPanels.jsxstatic/src/js/components/ProjectPanels/__tests__/ProjectPanels.test.jsxstatic/src/js/hooks/__tests__/useCreateRetrieval.test.tsxstatic/src/js/hooks/useCreateRetrieval.tsstatic/src/js/types/sharedTypes.tsstatic/src/js/util/__tests__/determineVariableId.test.tsstatic/src/js/util/__tests__/retrievals.test.jsstatic/src/js/util/accessMethods/__tests__/buildAccessMethods.test.jsstatic/src/js/util/accessMethods/__tests__/buildAccessMethods.test.tsstatic/src/js/util/accessMethods/__tests__/computeKeywordMappings.test.jsstatic/src/js/util/accessMethods/__tests__/defaultConcatenation.test.jsstatic/src/js/util/accessMethods/__tests__/getVariables.test.jsstatic/src/js/util/accessMethods/__tests__/insertSavedAccessConfig.test.jsstatic/src/js/util/accessMethods/__tests__/mocks.jsstatic/src/js/util/accessMethods/__tests__/supportsBoundingBoxSubsetting.test.jsstatic/src/js/util/accessMethods/__tests__/supportsConcatenation.test.jsstatic/src/js/util/accessMethods/__tests__/supportsShapefileSubsetting.test.jsstatic/src/js/util/accessMethods/__tests__/supportsTemporalSubsetting.test.jsstatic/src/js/util/accessMethods/buildAccessMethods.jsstatic/src/js/util/accessMethods/buildAccessMethods.tsstatic/src/js/util/accessMethods/buildAccessMethods/__tests__/buildHarmony.test.jsstatic/src/js/util/accessMethods/buildAccessMethods/__tests__/buildHarmony.test.tsstatic/src/js/util/accessMethods/buildAccessMethods/__tests__/buildOpendap.test.jsstatic/src/js/util/accessMethods/buildAccessMethods/buildHarmony.jsstatic/src/js/util/accessMethods/buildAccessMethods/buildHarmony.tsstatic/src/js/util/accessMethods/buildAccessMethods/buildOpendap.jsstatic/src/js/util/accessMethods/computeHierarchyMappings.jsstatic/src/js/util/accessMethods/computeKeywordMappings.jsstatic/src/js/util/accessMethods/defaultConcatenation.jsstatic/src/js/util/accessMethods/getVariables.jsstatic/src/js/util/accessMethods/getVariables.tsstatic/src/js/util/accessMethods/supportsBoundingBoxSubsetting.jsstatic/src/js/util/accessMethods/supportsConcatenation.jsstatic/src/js/util/accessMethods/supportsShapefileSubsetting.jsstatic/src/js/util/accessMethods/supportsTemporalSubsetting.jsstatic/src/js/util/determineVariableId.tsstatic/src/js/util/getDerivedHarmonyState/__tests__/__mocks__/mocks.tsstatic/src/js/util/getDerivedHarmonyState/__tests__/getDerivedHarmonyState.test.tsstatic/src/js/util/getDerivedHarmonyState/getDerivedHarmonyState.tsstatic/src/js/util/request/__tests__/harmonyCapabilitiesRequest.test.jsstatic/src/js/util/request/harmonyCapabilitiesDocumentRequest.tsstatic/src/js/util/request/request.tsstatic/src/js/util/retrievals.jsstatic/src/js/zustand/slices/__tests__/createProjectSlice.test.tsstatic/src/js/zustand/slices/createProjectSlice.tsstatic/src/js/zustand/types.tstests/e2e/history/history.spec.jstests/e2e/page_titles/page_titles.spec.js
💤 Files with no reviewable changes (19)
- static/src/js/util/accessMethods/tests/defaultConcatenation.test.js
- static/src/js/util/accessMethods/buildAccessMethods/tests/buildHarmony.test.js
- static/src/js/util/accessMethods/defaultConcatenation.js
- static/src/js/util/accessMethods/supportsBoundingBoxSubsetting.js
- static/src/js/util/accessMethods/supportsShapefileSubsetting.js
- static/src/js/util/accessMethods/supportsTemporalSubsetting.js
- static/src/js/util/accessMethods/tests/supportsBoundingBoxSubsetting.test.js
- static/src/js/components/OrderStatus/tests/OrderStatusCollection.test.tsx
- static/src/js/util/accessMethods/buildAccessMethods/buildHarmony.js
- static/src/js/components/AccessMethod/tests/mocks.ts
- static/src/js/util/accessMethods/tests/supportsTemporalSubsetting.test.js
- static/src/js/util/accessMethods/supportsConcatenation.js
- static/src/js/util/accessMethods/buildAccessMethods.js
- static/src/js/util/accessMethods/getVariables.js
- static/src/js/util/accessMethods/tests/supportsShapefileSubsetting.test.js
- serverless/src/graphQl/resolvers/tests/retrieval.test.js
- static/src/js/util/retrievals.js
- sharedUtils/outputFormatMaps.js
- static/src/js/util/accessMethods/tests/supportsConcatenation.test.js
🛑 Comments failed to post (10)
static/src/js/components/AccessMethod/__tests__/__mocks__/mocks.ts (1)
26-26:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix typo in EPSG projection code.
The projection CRS code is misspelled as
EPGS:4313in multiple places; it should beEPSG:4313(note the 'P'). This typo could cause projection matching failures if the code performs strict string comparisons.🔧 Proposed fix
{ name: 'Geographic', - crs: 'EPGS:4313' + crs: 'EPSG:4313' }Apply the same correction at lines 54, 106, and 173.
Also applies to: 54-54, 106-106, 173-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/components/AccessMethod/__tests__/__mocks__/mocks.ts` at line 26, In the mocks file update the misspelled projection code by changing the mocked object property crs values from 'EPGS:4313' to the correct 'EPSG:4313' so projection comparisons succeed; locate the crs properties in the mock objects inside static/src/js/components/AccessMethod/__tests__/__mocks__/mocks.ts and replace every occurrence (currently 'EPGS:4313') with 'EPSG:4313'.static/src/js/components/AccessMethod/AccessMethod.jsx (1)
278-278:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove debug console.log before merging.
This debug logging statement should be removed from production code.
🧹 Proposed fix
- console.log('🚀 ~ AccessMethod.jsx:330 ~ AccessMethod ~ accessMethods:', accessMethods)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/components/AccessMethod/AccessMethod.jsx` at line 278, Remove the debug console.log in the AccessMethod component by deleting the statement that logs "🚀 ~ AccessMethod.jsx:330 ~ AccessMethod ~ accessMethods:" (inside the AccessMethod function/component where accessMethods is referenced) and ensure no other stray console.debug/console.log calls remain in that file; keep behavior unchanged otherwise and run linting/tests to confirm no leftover debug prints.static/src/js/util/__tests__/determineVariableId.test.ts (1)
5-43: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test case for variables with both
conceptIdandhref.The test suite is missing coverage for the edge case where a variable has both
conceptIdandhrefpresent. This test would verify the documented precedence behavior (conceptId should win) and would have caught the logic error flagged in the implementation.🧪 Proposed test case to add
test('returns undefined when no variable information is blank from harmony', () => { const harmonyVariable: HarmonyVariable = { name: '', href: '', scienceKeywords: [], longName: '', units: '' } const result = determineVariableId(harmonyVariable) expect(result).toEqual(undefined) }) + + test('prefers conceptId when both conceptId and href are present', () => { + // Construct a hybrid object with both fields (edge case) + const hybridVariable = { + conceptId: 'V123-CMR', + name: 'Test Variable', + href: 'https://cmr.example.com/search/concepts/V456-HARMONY', + scienceKeywords: [], + definition: '', + instanceInformation: null, + longName: '', + nativeId: '', + units: '' + } + const result = determineVariableId(hybridVariable as any) + expect(result).toEqual('V123-CMR') + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/__tests__/determineVariableId.test.ts` around lines 5 - 43, Add a test to determineVariableId that covers the edge case where a variable object contains both a UmmSVariable.conceptId and a HarmonyVariable.href; create a combined fixture (or two objects) with conceptId set to e.g. "V789-CMR" and href containing a different ID, call determineVariableId on the object, and assert that the returned value equals the conceptId (verifying precedence of conceptId over href); reference the determineVariableId function and the UmmSVariable/HarmonyVariable shapes when adding the test to static/src/js/util/__tests__/determineVariableId.test.ts.static/src/js/util/determineVariableId.ts (1)
18-25:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical logic error:
hrefoverwritesconceptIdinstead of falling back.The implementation uses two independent
ifstatements, so when a variable has bothconceptIdandhref, the function will:
- Set
variableId = variable.conceptId(line 19)- Then overwrite it with
variable.href.split('/').pop()(line 24)This contradicts the documented behavior on line 8: "Prefers the conceptId, but will fall back to parsing the href for Harmony variables."
🐛 Proposed fix: use `else if` to enforce conceptId precedence
// Use a type guard to check for 'conceptId'. // The `variable.conceptId` check ensures we don't return an empty string. if ('conceptId' in variable && variable.conceptId) { variableId = variable.conceptId - } - - // Use a type guard to check for 'href'. This is the fallback for Harmony variables. - if ('href' in variable && variable.href) { + } else if ('href' in variable && variable.href) { + // Fallback to href for Harmony variables when conceptId is absent variableId = variable.href.split('/').pop() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/determineVariableId.ts` around lines 18 - 25, The logic in determineVariableId sets variableId from variable.conceptId and then always re-checks href, causing href to overwrite conceptId when both exist; change the second check to an else if so conceptId takes precedence (i.e., keep the existing if ('conceptId' in variable && variable.conceptId) { variableId = variable.conceptId } and make the href branch else if ('href' in variable && variable.href) { variableId = variable.href.split('/').pop() }) to enforce the documented fallback behavior.static/src/js/util/getDerivedHarmonyState/getDerivedHarmonyState.ts (1)
241-246:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompute spatial support from
bbox OR shape, notbboxalone.
spatialSubset.supportedcurrently mirrors onlysummaryBbox. If shape is supported while bbox is not, spatial subsetting is incorrectly reported as unsupported.💡 Suggested fix
summary: { subsetting: { bbox: summaryBbox = false, + shape: summaryShape = false, temporal: summaryTemporal = false, variable: summaryVariable = false }, @@ spatialSubset: { - supported: summaryBbox, + supported: summaryBbox || summaryShape, disabled: disabledCapabilities.spatialSubset, shapeDisabled: disabledCapabilities.shape, bboxDisabled: disabledCapabilities.bbox },Also applies to: 374-374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/util/getDerivedHarmonyState/getDerivedHarmonyState.ts` around lines 241 - 246, The spatial subsetting supported flag is only using summaryBbox, so change the logic that sets spatialSubset.supported to consider either bbox OR shape being supported (e.g., use summaryBbox || summaryShape) when computing summary.subsetting.spatial; update the same pattern where it appears (also at the other occurrence around line 374). Ensure you reference the summaryBbox and summaryShape variables and set summary.subsetting.spatial.supported accordingly while leaving temporal/variable logic (summaryTemporal/summaryVariable) unchanged.static/src/js/zustand/slices/createProjectSlice.ts (3)
280-316:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop the outer flow on the first Harmony 401.
return buildPromise(null)only exits themapcallback. Because these requests are launched underPromise.all, one expired token still lets the other Harmony calls and the GraphQL fetch continue. That can fire multiple auth redirects and build a nested/login?...state=/login?...URL instead of a single clean redirect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/zustand/slices/createProjectSlice.ts` around lines 280 - 316, The current map callback returns buildPromise(null) on a Harmony 401, which only exits the individual callback and doesn't short-circuit Promise.all; change that so the outer Promise.all rejects immediately by re-throwing the 401 error (or returning Promise.reject(error)) instead of returning buildPromise(null). Locate the filteredIds.map async callback in createProjectSlice.ts (the block that constructs HarmonyCapabilitiesDocumentRequest and calls .search), and replace the `return buildPromise(null)` inside the `if (error instanceof AxiosError && error.response?.status === 401)` branch with `throw error` (or `return Promise.reject(error)`) so Promise.all rejects and the outer flow stops; keep the existing zustandState.errors.handleError call intact.
779-787:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
collectionbefore destructuring it.If a Harmony update arrives after the collection is removed or before it exists,
const { selectedAccessMethod, accessMethods } = collectionthrows before your bailout runs. The genericupdateAccessMethodpath used to no-op safely for a missing collection; this new branch loses that protection.Suggested fix
set((state) => { const { collections } = state.project const { byId } = collections const collection = byId[collectionId] - const { selectedAccessMethod, accessMethods } = collection - - // If missing selectedAccessMethod or accessMethods, do no execute - if (!selectedAccessMethod || !accessMethods) return + if (!collection?.selectedAccessMethod || !collection.accessMethods) return + + const { selectedAccessMethod, accessMethods } = collection🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/zustand/slices/createProjectSlice.ts` around lines 779 - 787, The code destructures selectedAccessMethod and accessMethods from collection without first ensuring collection exists, so if the collection was removed the destructure will throw; update the setter to first check that const collection = byId[collectionId] is truthy (or bail out if undefined) before doing const { selectedAccessMethod, accessMethods } = collection — e.g., guard collection (and keep the existing early return for missing selectedAccessMethod/accessMethods) so the updateAccessMethod branch / set((state) => { ... }) no-ops safely when the collection is absent.
795-850:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild the Harmony method instead of manually syncing a subset.
After recomputing
derivedHarmonyState, this only copies flags and selected values back into store state.variables,hierarchyMappings, andkeywordMappingsstay from the old build even thoughbuildHarmonyderives them from the new Harmony state. A selection change that narrows the active service set can therefore leave stale Harmony variable options in the store/UI.Suggested direction
+import { buildHarmony } from '../../util/accessMethods/buildAccessMethods/buildHarmony' ... - // Recalculate the derived state - harmonyMethod.derivedHarmonyState = getDerivedHarmonyState( - updatedSelections, - harmonyMethod.harmonyCapabilitiesDocument - ) - - const { capabilities } = harmonyMethod.derivedHarmonyState - - if (!capabilities) return - - const { - temporalSubset, - spatialSubset, - outputFormats, - variableSubset, - concatenate, - reproject - } = capabilities - - // Use the derived harmony state to set what is enabled or disabled - harmonyMethod.enableTemporalSubsetting = updatedSelections.temporalSubset || false - ... - harmonyMethod.isConcatenationDisabled = concatenate.disabled + const rebuiltHarmonyMethod = buildHarmony( + harmonyMethod.harmonyCapabilitiesDocument, + updatedSelections + ) + + if (!rebuiltHarmonyMethod) return + + accessMethods[selectedAccessMethod] = rebuiltHarmonyMethod🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/src/js/zustand/slices/createProjectSlice.ts` around lines 795 - 850, The current update manually copies selection flags into harmonyMethod after computing getDerivedHarmonyState, which leaves variables, hierarchyMappings, and keywordMappings stale; instead, rebuild the entire Harmony method object using the new derivedHarmonyState (i.e. call the existing buildHarmony function or the routine that constructs a fresh harmonyMethod) using updatedSelections and the new derivedHarmonyState, then replace harmonyMethod with that rebuilt object so variables, hierarchyMappings, keywordMappings and all availability flags come from the canonical build rather than piecemeal assignment; ensure you preserve any necessary identity/refs expected elsewhere (or update callers) and remove the manual field-by-field syncing that sets enableTemporalSubsetting, enableSpatialSubsetting, selectedVariables, outputFormatAvailability, etc.tests/e2e/history/history.spec.js (1)
89-93:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCapabilities route mock is brittle due to exact query-string matching.
This matcher can fail if query params are reordered or additional params are appended, causing flaky e2e behavior.
💡 Suggested fix
- await page.route(`**/capabilities?collectionId=${conceptId}&version=2`, async (route) => { - await route.fulfill({ - json: { services: [] } - }) - }) + await page.route('**/capabilities?*', async (route) => { + const url = new URL(route.request().url()) + + if ( + url.searchParams.get('collectionId') === conceptId + && url.searchParams.get('version') === '2' + ) { + await route.fulfill({ json: { services: [] } }) + return + } + + await route.continue() + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/history/history.spec.js` around lines 89 - 93, The current page.route matcher uses an exact query-string and is brittle; update the mock in the tests to match the /capabilities path more robustly by using a looser route pattern (e.g., '**/capabilities**') and then inspect the actual request URL inside the handler via route.request().url() or new URL(...). Check searchParams for collectionId === conceptId and version === '2', call route.fulfill({ json: { services: [] } }) when it matches, otherwise call route.continue() so other queries are not incorrectly mocked.tests/e2e/page_titles/page_titles.spec.js (1)
166-170:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse parameter-based matching for capabilities route to avoid flaky misses.
The exact URL glob is fragile to query-string ordering and optional params.
💡 Suggested fix
- await page.route('**/capabilities?collectionId=C1443528505-LAADS&version=2', async (route) => { - await route.fulfill({ - json: { services: [] } - }) - }) + await page.route('**/capabilities?*', async (route) => { + const url = new URL(route.request().url()) + + if ( + url.searchParams.get('collectionId') === 'C1443528505-LAADS' + && url.searchParams.get('version') === '2' + ) { + await route.fulfill({ json: { services: [] } }) + return + } + + await route.continue() + })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.await page.route('**/capabilities?*', async (route) => { const url = new URL(route.request().url()) if ( url.searchParams.get('collectionId') === 'C1443528505-LAADS' && url.searchParams.get('version') === '2' ) { await route.fulfill({ json: { services: [] } }) return } await route.continue() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/page_titles/page_titles.spec.js` around lines 166 - 170, The route currently matches a full glob including query params which is fragile; change the page.route registration for the capabilities endpoint to match the base path (e.g., '**/capabilities') and inside the route handler inspect route.request().url (or construct a URL object) to check searchParams for collectionId and version, then call route.fulfill({ json: { services: [] } }) only when those params match; update the handler around the existing page.route callback and keep using route.fulfill to respond.
mandyparson
force-pushed
the
feature/EDSC-4638
branch
from
ef7f4d4 to
4cd834d
Compare
Collaborator
|
How does renaming "Selected Area:" to "Spatial Constraint:" sound? then similar for "Selected Range:" renamed to "Temporal Constraint:". |
Collaborator
|
Will: Is there a use case where the "Default Format" would disable one of the other options right away or are all options always available on initial load? Mandy: Default Format means that no conversion was chosen. It will not disable any other options. |
| reproject: { | ||
| /** Flag to indicate if reprojection is supported by the collection */ | ||
| supported: boolean | ||
| supported: SupportedProjection[] |
Contributor
There was a problem hiding this comment.
A TODO here might be good or a comment that we do this because we don't currently support interpolation I added EDSC-4686 as a placeholder for that effort for now
Contributor
Author
There was a problem hiding this comment.
I added a comment about not supporting Interpolation Methods at this time. I don't want to add a TODO here as we will most likely export this as a library before any work is done for Interpolation Methods.
william-valencia
approved these changes
Jun 16, 2026
macrouch
reviewed
Jun 17, 2026
Contributor
There was a problem hiding this comment.
"None" feels like the same problem we fixed with the output format. I don't think the data can have a projection of "none", it will always have something, so maybe this should be "Default Projection"?
| }) | ||
|
|
||
| describe('when supportsSpatialSubsetting is set to true', () => { | ||
| describe('when enableSpatialSubsetting is set to false', () => { |
Contributor
There was a problem hiding this comment.
There is no describe block for "when enableSpatialSubsetting is set to true"
Comment on lines
+774
to
+777
| }: { | ||
| collectionId: string, | ||
| newSelections: UserSelections | ||
| } |
Contributor
There was a problem hiding this comment.
This type shouldn't be defined here, all of the zustand function types are defined in zustand/types.ts
macrouch
reviewed
Jun 23, 2026
Comment on lines
+771
to
+776
| updateHarmonySelection: ( | ||
| { | ||
| collectionId, | ||
| newMethod | ||
| }: UpdateHarmonySelectionParams | ||
| ) => { |
Contributor
There was a problem hiding this comment.
Suggested change
| updateHarmonySelection: ( | |
| { | |
| collectionId, | |
| newMethod | |
| }: UpdateHarmonySelectionParams | |
| ) => { | |
| updateHarmonySelection: ({ collectionId, newMethod }) => { |
That param object is already defined as UpdateHarmonySelectionParams in zustand/types.ts
Contributor
Author
There was a problem hiding this comment.
I see. Because line 902 in zustand/types explicitly says updateHarmonySelection: ({ collectionId, newMethod }: UpdateHarmonySelectionParams) => void, I don't have to say it again here.
macrouch
approved these changes
Jun 23, 2026
mandyparson
added a commit
that referenced
this pull request
Jun 24, 2026
… parameters are still valid (#2055) * EDSC-4662: saving progress * EDSC-4662: Finalizing changes for customization forms * EDSC-4662: Performing self-review and Code Rabbit Comments * EDSC-4662: PR feedback * EDSC-4662: Changing name * EDSC-4662: PR Feedback * EDSC-4662: Fixing variables * EDSC-4662: fixing update params * EDSC-4662: Changing 'None' to 'Default Projection' * EDSC-4662: Changed to 'No Reprojection'
mandyparson
added a commit
that referenced
this pull request
Jun 26, 2026
… parameters are still valid (#2055) * EDSC-4662: saving progress * EDSC-4662: Finalizing changes for customization forms * EDSC-4662: Performing self-review and Code Rabbit Comments * EDSC-4662: PR feedback * EDSC-4662: Changing name * EDSC-4662: PR Feedback * EDSC-4662: Fixing variables * EDSC-4662: fixing update params * EDSC-4662: Changing 'None' to 'Default Projection' * EDSC-4662: Changed to 'No Reprojection'
mandyparson
added a commit
that referenced
this pull request
Jul 1, 2026
… parameters are still valid (#2055) * EDSC-4662: saving progress * EDSC-4662: Finalizing changes for customization forms * EDSC-4662: Performing self-review and Code Rabbit Comments * EDSC-4662: PR feedback * EDSC-4662: Changing name * EDSC-4662: PR Feedback * EDSC-4662: Fixing variables * EDSC-4662: fixing update params * EDSC-4662: Changing 'None' to 'Default Projection' * EDSC-4662: Changed to 'No Reprojection'
mandyparson
added a commit
that referenced
this pull request
Jul 1, 2026
… parameters are still valid (#2055) * EDSC-4662: saving progress * EDSC-4662: Finalizing changes for customization forms * EDSC-4662: Performing self-review and Code Rabbit Comments * EDSC-4662: PR feedback * EDSC-4662: Changing name * EDSC-4662: PR Feedback * EDSC-4662: Fixing variables * EDSC-4662: fixing update params * EDSC-4662: Changing 'None' to 'Default Projection' * EDSC-4662: Changed to 'No Reprojection'
mandyparson
added a commit
that referenced
this pull request
Jul 2, 2026
… parameters are still valid (#2055) * EDSC-4662: saving progress * EDSC-4662: Finalizing changes for customization forms * EDSC-4662: Performing self-review and Code Rabbit Comments * EDSC-4662: PR feedback * EDSC-4662: Changing name * EDSC-4662: PR Feedback * EDSC-4662: Fixing variables * EDSC-4662: fixing update params * EDSC-4662: Changing 'None' to 'Default Projection' * EDSC-4662: Changed to 'No Reprojection'
mandyparson
added a commit
that referenced
this pull request
Jul 7, 2026
… parameters are still valid (#2055) * EDSC-4662: saving progress * EDSC-4662: Finalizing changes for customization forms * EDSC-4662: Performing self-review and Code Rabbit Comments * EDSC-4662: PR feedback * EDSC-4662: Changing name * EDSC-4662: PR Feedback * EDSC-4662: Fixing variables * EDSC-4662: fixing update params * EDSC-4662: Changing 'None' to 'Default Projection' * EDSC-4662: Changed to 'No Reprojection'
mandyparson
added a commit
that referenced
this pull request
Jul 7, 2026
* EDSC-4642: As a user, I would like to set temporal, spatial, or output format capabilities and know which parameters are still valid (#2042) * EDSC-4642: Initial Commit * EDSC-4642: Editing static.config * EDSC-4642: Edits to static.config * EDSC-4642: Updating based on community feedback * EDSC-4642: Fixing tests for all but AccessMethod * EDSC-4642: Fixing tests * EDSC-4642: Adding version to url params * EDSC-4642: Structural changes to store * EDSC-4642: A bit of clean up * EDSC-4642: Cleaning up as per feedback * EDSC-4642: PR feedback * EDSC-4661: As a user, I would like to set variables and concatenation and know which parameters are still valid (#2051) * EDSC-4661: Adding variables and concatenation * EDSC-4661: Fixing bug found with multiple harmony cap docs * EDSC-4661: Addressing PR feedback * EDSC-4661: PR Feedback and cleaning up tests * EDSC-4661: Lint errors * EDSC-4661: Fixing tests * EDSC-4661: Fixing minor env variable * EDSC-4661: Adding test and comments * EDSC-4662: As a user, I would like to set reprojection and know which parameters are still valid (#2055) * EDSC-4662: saving progress * EDSC-4662: Finalizing changes for customization forms * EDSC-4662: Performing self-review and Code Rabbit Comments * EDSC-4662: PR feedback * EDSC-4662: Changing name * EDSC-4662: PR Feedback * EDSC-4662: Fixing variables * EDSC-4662: fixing update params * EDSC-4662: Changing 'None' to 'Default Projection' * EDSC-4662: Changed to 'No Reprojection' * EDSC-4683: Adding e2e playwright tests (#2062) * EDSC-4638: Writing playwright tests * EDSC-4683: Fixing page_titles.spec * EDSC-4683:PR Feedback * EDSC-4683: Line fix * EDSC-4683-testing * EDSC-4638-testing: removing alert for all but Harmony * EDSC-4638: Fixing that test * EDSC-4638: Fixing test coverage * EDSC-4638: Addressing feedback * EDSC-4638: lint * EDSC-4638: Removing another 'no-implicit-any' line
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
What is the feature?
In addition to the AC for this ticket, this PR aims to also ensure that all other elements of this element are working as expected. The functionality of other access methods should have stayed the same while changes to Harmony were made.
What is the Solution?
A lot of refactoring.
What areas of the application does this impact?
AccessMethods, createProjectSlice, buildHarmony, getDerivedHarmonyState and associated utilities. Many of which I was able to cut after these changes had been made.
Please note that some of these files and associated tests have been converted to typescript for better performance and readability.
Testing
Reproduction steps
Harmony Documentation so you can find a service to work with: https://harmony.earthdata.nasa.gov/docs
Harmony link to capabilities document used to create any HarmonyAccessMethod: https://harmony.earthdata.nasa.gov/capabilities?collectionId=&version=3
Some collections I've been using:
C3685896287-LARC_CLOUD: Has interpolation methods | sds/swath-projector
https://harmony.earthdata.nasa.gov/capabilities?collectionId=C3685896287-LARC_CLOUD&version=3
C2949811996-POCLOUD: Has supportedProjections | HyBIG >> Mocked with json object below
https://harmony.earthdata.nasa.gov/capabilities?collectionId=C2949811996-POCLOUD&version=3
C2723754847-GES_DISC: No Concatenation and Variables without Science Keywords
C1453188197-GES_DISC: Has variables with scienceKeywords
C2205121315-POCLOUD: Has concatenation
C4054955340-GES_DISC - General
https://harmony.earthdata.nasa.gov/capabilities?collectionId=C4054955340-GES_DISC&version=3
C3685668680-LARC_CLOUD - Selecting PNG disables spatial and temporal (also has reprojection)
https://harmony.earthdata.nasa.gov/capabilities?collectionId=C3685668680-LARC_CLOUD&version=3
C3300834719-OB_CLOUD - No services but has type Harmony
https://harmony.earthdata.nasa.gov/capabilities?collectionId=C3300834719-OB_CLOUD&version=3
C1623882456-LPDAAC_ECS: ESI
C2799438271-POCLOUD: SWODLR
UAT C1224234386-E2E_18_4 - Opendap
For testing, select any collection for your project (preferably multiple) and start messing around with the form. See if anything breaks or doesn't work as it should.
Attachments
Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor