Skip to content

Fix gnDownload reducer mutating previous state on DOWNLOAD_METADATA_COMPLETE#2565

Open
Valyrian-Code wants to merge 2 commits into
GeoNode:masterfrom
Valyrian-Code:fix/gndownload-state-mutation
Open

Fix gnDownload reducer mutating previous state on DOWNLOAD_METADATA_COMPLETE#2565
Valyrian-Code wants to merge 2 commits into
GeoNode:masterfrom
Valyrian-Code:fix/gndownload-state-mutation

Conversation

@Valyrian-Code

Copy link
Copy Markdown

Problem

In reducers/gnDownload, the DOWNLOAD_METADATA_COMPLETE case mutates the previous state:

const newState = { ...state };                 // shallow copy
const downloads = newState.downloads[linkType]; // same reference as state.downloads[linkType]
delete downloads[action.pk];                    // mutates the incoming state!

{ ...state } is shallow, so newState.downloads[linkType] is the same object as the incoming state.downloads[linkType]. The delete removes the key from the previous state before the immutable spread runs. Reducers must be pure — mutating prior state can break referential-equality checks, memoized selectors, and time-travel debugging.

Fix

Copy the link bucket before deleting, so the previous state is untouched:

const linkType = action?.link?.split(' ').join('');
const remaining = { ...(state.downloads[linkType] || {}) };
delete remaining[action.pk];
return { ...state, downloads: { ...state.downloads, [linkType]: remaining } };

Test

Added to reducers/__tests__/gndownload-test.js: captures the previous link bucket and asserts it is unchanged after dispatch. Fails on master (Expected {} to equal { 1: true }), passes with the fix. Output value is unchanged. Full suite green.

…OMPLETE

{ ...state } is shallow, so newState.downloads[linkType] is the same
reference as the incoming state; delete downloads[action.pk] then mutates
the previous state (reducers must be pure). Copy the link bucket before
deleting so the previous state is untouched.

Adds a test in gndownload-test.js asserting the previous state is unchanged
after dispatch; fails on master, passes with the fix.
Copilot AI review requested due to automatic review settings June 11, 2026 19:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request fixes a state mutation bug in the gndownload reducer during the DOWNLOAD_METADATA_COMPLETE action by copying the target downloads object before deleting the action's primary key. A corresponding unit test was also added to verify that the previous state is not mutated. The reviewer suggested a cleaner, more idiomatic approach using object destructuring with rest syntax to immutably omit the key instead of copying and using the delete operator.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread geonode_mapstore_client/client/js/reducers/gndownload.js Outdated
Per review: replace the copy-then-delete with object rest destructuring
({ [action.pk]: removed, ...remaining }) to omit the key without the delete
operator. Behaviour unchanged; non-mutation test still passes.
@Valyrian-Code

Copy link
Copy Markdown
Author

Done — switched to object rest destructuring (const { [action.pk]: removed, ...remaining } = ...) to omit the key immutably without delete. Lint and the non-mutation test pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants