Skip to content

shapefileId is decoded with encodeString, so it and selectedFeatures diverge when sf is absent #2076

Description

@wantaekchoi

Before contributing to this project, please review Contribution.md.

Description

In static/src/js/util/url/url.js, the urlDefs entry for shapefileId uses encodeString as its decoder:

shapefileId: {
  shortKey: 'sf',
  encode: encodeString,
  decode: encodeString   // every other entry uses its matching decodeX here
},

Every other entry pairs encode: encodeX with decode: decodeX. tagKey, two entries below, is the closest comparison: same encoder, correct decoder (decodeString).

export const decodeString = (string) => string
export const encodeString = (string) => string || ''

So a URL with no sf decodes shapefileId to '', while every other absent parameter decodes to undefined. That matters because updateStore merges decoded values over the store with lodash merge, which skips undefined sources but overwrites with '' (comment at line 93: "Use merge on the queries to correctly use the initial state as a fallback for undefined decoded values"). shapefileId's sibling selectedFeatures decodes via decodeArray, which returns undefined when absent and falls back correctly. shapefileId never does, so it diverges from selectedFeatures after any navigation without sf:

const storeShapefile = { shapefileId: '12345', selectedFeatures: ['a'] }
const { shapefile: decoded } = decodeUrlParams('?p=C100000-EDSC')
merge({}, storeShapefile, decoded)
// { shapefileId: '', selectedFeatures: ['a'] }

This is reachable through ordinary navigation, not just browser back/forward: HistoryContainer calls changePath on a POP history action, and PortalLinkContainer calls it from any in-app link with updatePath set (used by SecondaryToolbar, SearchSidebarHeader, and others).

Expected behavior

shapefileId and selectedFeatures should follow the same rule when sf is absent. Decoding with decodeString, like tagKey and every other string parameter, gives that. If clearing on navigation is actually intended, then selectedFeatures is the field that's wrong instead. Either way, the two should agree.

Additional context

Introduced in EDSC-2392: Fix shapefile subsetting (Sep 30 2019), which switched the entry from the integer encoders to the string ones and appears to have copied the line above instead of renaming it. The existing test fixtures (url.mocks.js, changePath.test.ts) record the current '' as expected, which is why nothing caught this.

Acceptance Criteria

After a navigation to a URL with no sf, shapefile.shapefileId and shapefile.selectedFeatures are either both retained or both cleared.

I have a fix and test ready to open as a PR once this has a ticket number, per the commit convention in CONTRIBUTING.md.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions