[HOLD for payment 2024-02-07] [$500] Web - Chat - There is a cancelled request where isDarkMode=true is present

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.26.1
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #34395

Action Performed:

1. Open the network tab in Chrome console
2. Send https://staging.new.expensify.com/statements/202312
   in any 1:1 chat
3. Turn on light mode
4. Open the link

Expected Result:

There shouldn't be a cancelled request

Actual Result:

There is a cancelled request where isDarkMode=true is present

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6345667_1705518203570.bandicam_2024-01-17_17-41-00-835.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01656e05b8d4b00139
• Upwork Job ID: 1747725844186800128
• Last Price Increase: 2024-01-17
• Automatic offers:
• c3024 | Reviewer | 28117763

Issue Owner

Current Issue Owner: @muttmuure

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @tgolen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[tgolen]

Since this has no user impact, I'm removing the deploy blocker label. Please correct me if I'm wrong about the impact level.

[melvin-bot]

Triggered auto assignment to @muttmuure (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01656e05b8d4b00139

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @c3024 (External)

[aswin-s]

Proposal

Please re-state the problem that we are trying to solve in this issue.

There is a cancelled request in network panel when WalletStatementPage is loaded

What is the root cause of that problem?

On the WalletStatementPage, the statementPageUrl for WalletStatementModal is constructed as follows:

App/src/pages/wallet/WalletStatementPage.js

Line 94 in 4b7b8f2

const url = `${CONFIG.EXPENSIFY.EXPENSIFY_URL}statement.php?period=${yearMonth}${themePreference === CONST.THEME.DARK ? '&isDarkMode=true' : ''}`;

Note that the url utilises the themePreference value to determine the user's current theme.

The themePreference is derived from the useThemePreference() hook, which asynchronously reads the theme preference from Onyx via an effect.

App/src/hooks/useThemePreference.ts

Lines 12 to 21 in 4b7b8f2

	useEffect(() => {
	const theme = preferredThemeFromStorage ?? CONST.THEME.DEFAULT;
	// If the user chooses to use the device theme settings, we need to set the theme preference to the system theme
	if (theme === CONST.THEME.SYSTEM) {
	setThemePreference(systemTheme ?? CONST.THEME.FALLBACK);
	} else {
	setThemePreference(theme);
	}
	}, [preferredThemeFromStorage, systemTheme]);

Initially, themePreference is set to CONST.THEME.FALLBACK and later updated to the current theme by the effect.

App/src/hooks/useThemePreference.ts

Line 8 in 4b7b8f2

const [themePreference, setThemePreference] = useState<ThemePreferenceWithoutSystem>(CONST.THEME.FALLBACK);

When the user is on light theme, WalletStatementPage first receives the themePreference value as CONST.THEME.FALLBACK, which is dark. It is then updated to light once the useEffect runs. Consequently, the url variable changes, causing the WalletStatementModal component to re-render. As the WalletStatementModal triggers a network request upon rendering and then unmounts immediately, the request gets canceled. This is why a canceled network request is observed.

What changes do you think we should make in order to solve the problem?

The dependency on the useThemePreference hook is the fundamental issue. We should instead utilize the useTheme hook to directly read the current color scheme. Since the ThemeProvider has already loaded the theme preference, the theme.colorScheme value will remain constant when the WalletStatementPage renders, thereby preventing the canceled request.

function WalletStatementPage(props) {
    const theme = useTheme();
    
    // use theme.colorScheme to construct the url
    const url = `${CONFIG.EXPENSIFY.EXPENSIFY_URL}statement.php?period=${yearMonth}${theme.colorScheme === CONST.THEME.DARK ? '&isDarkMode=true' : ''}`;

What changes do you think we should make in order to solve the problem?

An alternative would be to not rely on the colorScheme field in the theme. Instead, we could introduce a specific currentTheme field in the ThemeContext to represent the active theme.

[kaushiktd]

Please re-state the problem that we are trying to solve in this issue.

Web - Chat - There is a cancelled request where isDarkMode=true is present

What is the root cause of that problem?

The dependency on the useThemePreference hook is identified as the fundamental issue. The hook relies on asynchronous operations, leading to initial renders with an undesired theme preference value.

What changes do you think we should make in order to solve the problem?

Optimize the useThemePreference hook using useMemo to prevent unnecessary re-renders. This change ensures that the theme preference is correctly calculated and avoids triggering network requests on each render.

const themePreference = useMemo(() => {
  const theme = preferredThemeFromStorage ?? CONST.THEME.DEFAULT;

  // If the user chooses to use the device theme settings, we need to set the theme preference to the system theme
  if (theme === CONST.THEME.SYSTEM) {
    return systemTheme ?? CONST.THEME.FALLBACK;
  } else {
    return theme;
  }
}, [preferredThemeFromStorage, systemTheme]);

return themePreference;
} 

Video:-

https://drive.google.com/file/d/15I1YNuGJ6ChQKgj1bh_2yMg-f7VZ0UFy/view?usp=sharing

[tgolen]

@c3024 Do you need some help giving these initial proposals a look or can you get to this soon?

[c3024]

I'm on it. Will complete today.

[c3024]

Thank you for your proposals.

I think the correct way to fix is fixing the useThemePreference hook. I don't see the need to have the themePreference as a state variable and set it with useEffect afterwards in the useThemePreference hook.

So, proposal here by @kaushiktd looks good to me.

🎀 👀 🎀 C+ Reviewed

[melvin-bot]

Current assignee @tgolen is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@c3024] The PR that introduced the bug has been identified. Link to the PR:
• [@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@c3024] Determine if we should create a regression test for this bug.
• [@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[kaushiktd]

@tgolen @c3024 will you send me invitation to apply on job OR it will be directly paid on Upwork?

[c3024]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@c3024] The PR that introduced the bug has been identified. Link to the PR: Theme switching: Create theme/styling hooks and context components #21667
• [@c3024] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/21667/files#r1477367303
• [@c3024] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No discussion has been started. This is a trivial bug and this is unlikely to be identified earlier
• [@c3024] Determine if we should create a regression test for this bug. No, this is not something related to user flow and also not a significant bug.
• [@c3024] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. NA

[muttmuure]

Handling

[muttmuure]

Automation seems to have hired the wrong person for reviewer and nobody for contributor

[kaushiktd]

Amm, I did not receive any payment on Upwork! Where did you pay me @muttmuure ?

[c3024]

I was paid correctly. I was the reviewer. I got a notification on Upwork asking for refund . 🤔 @muttmuure

[muttmuure]

Ah OK, so it worked correctly then

[muttmuure]

Let me cancel that refund request

[muttmuure]

OK @kaushiktd should have an offer on Upwork

[kaushiktd]

Thanks, @muttmuure

Offer is accepted.

[muttmuure]

contract extended

[muttmuure]

@c3024 maybe you just decline the refund request

[c3024]

I don't see a decline option. Maybe I will just ignore it?

[muttmuure]

Sounds fine then - let's do that

[kaushiktd]

contract extended

I am not seeing any milestone paid in my account.

[kaushiktd]

It shows me this error when I tried to accept offer.

[kaushiktd]

@muttmuure Can you please try withdrawing and creating new offer?

[kaushiktd]

@muttmuure New offer is accepted and payment milestone requested

[melvin-bot]

@tgolen, @kaushiktd, @muttmuure, @c3024 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[muttmuure]

All paid