Skip to content

fix: data refreshing on first render is restricted to avoid multiple api calls and optimised the handling of search query params - #1231

Merged
yoganandaness merged 7 commits into
developfrom
SWAP-4986-user-office-proposal-popup-does-not-show-up
Nov 3, 2025
Merged

fix: data refreshing on first render is restricted to avoid multiple api calls and optimised the handling of search query params#1231
yoganandaness merged 7 commits into
developfrom
SWAP-4986-user-office-proposal-popup-does-not-show-up

Conversation

@yoganandaness

Copy link
Copy Markdown
Contributor

Description

The Proposal table have few usability issues

  1. Sorting the column resets the page size and page number
  2. Page size is not persisting in query params, so refreshing the page forgets the page size
  3. Proposal Modal on refreshing does not work sometimes, as the page size and page number is lost
  4. Avoid fetching multiple times

The above have been fixed by ensuring that the refreshing of table happens only on filter changes and not on first rendering. Also the setting of search params have been optimised in such a way that it does not resets the other ones

Motivation and Context

Improve the usability of the the Proposal Table and ensuing persistence of the query params, so that the page refresh shows the same result

How Has This Been Tested

Manually

Fixes

Changes

In Proposal Table component, the search params handling and the refreshing data restriction has been done

Depends on

Tests included/Docs Updated?

  • I have added tests to cover my changes.
  • All relevant doc has been updated

…api calls and optimised the handling of search query params
@yoganandaness
yoganandaness requested a review from a team as a code owner October 20, 2025 11:18
@yoganandaness
yoganandaness requested review from TCMeldrum, deepaksftc and janosbabik and removed request for a team October 20, 2025 11:18

@janosbabik janosbabik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @yoganandaness !
It looks good, just a small request from my side.

<ReduceCapacityIcon data-cy="bulk-reassign-reviews" />
);

const [isFirstRender, setIsFirstRender] = useState(true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if we really need to track it, can we use useRef instead of useState?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure

let isMounted = true;

if (isMounted) {
if (isMounted && !isFirstRender) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the original implementation was not correct, using isMounted here doesn’t make sense, it’s being used incorrectly.

We can simplify it like this:

useEffect(() => {
  if (isFirstRender.current) {
    isFirstRender.current = false;
    return;
  }
  
  refreshTableData();
}, [proposalFilter]);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice point. Yes, we dont need the isMounted

@yoganandaness
yoganandaness enabled auto-merge (squash) October 31, 2025 13:15
@yoganandaness
yoganandaness merged commit bbea11f into develop Nov 3, 2025
22 checks passed
@yoganandaness
yoganandaness deleted the SWAP-4986-user-office-proposal-popup-does-not-show-up branch November 3, 2025 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants