DEVPROD-26582: Remove notifyOnNetworkStatusChange: false at global apollo client level by sophiayangDB · Pull Request #1482 · evergreen-ci/ui

sophiayangDB · 2026-03-26T16:08:12Z

Description

notifyOnNetworkStatusChange has a new default for Apollo

minnakt

Nice; but the test failures are likely real failures that need to be fixed

sentry · 2026-03-30T18:22:16Z

+    return null;
+  }
+  // if (loading || !userSettings) {


Bug: The NotificationsTab renders nothing if user settings fail to load, causing the tab content to disappear without any error or loading state.
_{Severity: MEDIUM}

Suggested Fix

Update the conditional logic in NotificationsTab to show a CardSkeleton or an error state when userSettings is null, even after the initial load has completed. This ensures the user always sees a meaningful UI state instead of a blank component.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: apps/spruce/src/pages/preferences/preferencesTabs/NotificationsTab.tsx#L19-L21 Potential issue: In the `NotificationsTab` component, if the `useUserSettings` query completes but returns `null` for `userSettings` (e.g., due to a network error or if the backend returns no settings), the component will return `null`. This causes the entire content of the notifications tab to disappear silently without displaying a loading indicator or an error message to the user. This behavior is inconsistent with the `ProfileTab`, which shows a skeleton, and is likely unintended as it provides a poor user experience during data fetch failures.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

sentry · 2026-03-30T18:22:16Z

  const awsRegions = awsRegionData?.awsRegions || [];

-  if (loading) {
+  if (awsRegionsLoading || userSettingsLoading || !userSettings) {


Bug: The ProfileTab shows a loading skeleton indefinitely if fetching user settings completes with no data, because the !userSettings check does not account for the loading state.
_{Severity: MEDIUM}

Suggested Fix

Modify the conditional logic in ProfileTab to distinguish between an initial loading state and a completed-but-empty/error state. For example, use a condition like const isInitialLoading = userSettingsLoading && userSettings === null; and only show the skeleton when isInitialLoading is true.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: apps/spruce/src/pages/preferences/preferencesTabs/ProfileTab.tsx#L15 Potential issue: In the `ProfileTab` component, the loading condition `!userSettings` causes a perpetual loading skeleton to be displayed. If the data fetch completes (`userSettingsLoading` is `false`) but the `userSettings` data is `null` (due to a query error or empty data), the `!userSettings` condition remains true. As a result, the component will show a `CardSkeleton` indefinitely, misleading the user into thinking the content is still loading when the request has already finished.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

sentry · 2026-03-30T19:08:26Z

+  const isInitialLoading = loading && userSettings === null;
+
+  if (isInitialLoading) {


Bug: When saving notification settings, if the subsequent data refetch fails, the UI silently displays stale data without any error indication, making the save appear successful.
_{Severity: HIGH}

Suggested Fix

Ensure the useUserSettings hook properly handles and exposes error states from Apollo Client's useQuery. Set notifyOnNetworkStatusChange: true for the query to ensure the loading state is updated on refetches. Implement UI to display an error message to the user when a refetch fails, preventing stale data from being shown.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: apps/spruce/src/pages/preferences/preferencesTabs/NotificationsTab.tsx#L11-L13 Potential issue: In the `NotificationsTab` component, after a user submits the form, a refetch of `UserSettings` is triggered. Due to the Apollo Client configuration (`notifyOnNetworkStatusChange` being effectively `false`), the `loading` state is not updated during this refetch. If the refetch fails due to a network error, the component will not receive an error state or a loading indicator. As a result, it will continue to display the stale data from the cache, misleading the user into believing their changes were successfully saved when they were not.

sentry · 2026-03-30T19:57:22Z

  const { notifications, slackMemberId, slackUsername } = userSettings ?? {};

-  if (loading || !userSettings) {
+  if (loading && !userSettings) {


Bug: The conditional logic for the loading skeleton at loading && !userSettings will always be false, preventing the skeleton from ever rendering during data fetching.
_{Severity: MEDIUM}

Suggested Fix

The conditional check for the loading skeleton should be simplified to if (loading). This will correctly display the skeleton while the user settings are being fetched and loading is true.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: apps/spruce/src/pages/preferences/preferencesTabs/NotificationsTab.tsx#L11 Potential issue: The `useUserSettings` hook is designed to always return a truthy value for `userSettings`, defaulting to an empty object `{}`. The conditional logic for displaying the loading skeleton was changed to `loading && !userSettings`. Because `!userSettings` will always evaluate to `false`, the entire condition is always `false`. This prevents the loading skeleton from ever being displayed, even when data is being fetched. As a result, users will see a flash of an empty form before their settings are populated, which is a degradation of the user experience.

sentry · 2026-03-30T20:26:51Z

  const { notifications, slackMemberId, slackUsername } = userSettings ?? {};

-  if (loading || !userSettings) {
+  if (loading && !userSettings) {


Bug: The condition if (loading && !userSettings) will always be false because userSettings is always a truthy object, preventing the loading skeleton from ever rendering.
_{Severity: CRITICAL}

Suggested Fix

The conditional logic should be updated to correctly show the loading state. A simple if (loading) check would be sufficient, as is done in similar components. Alternatively, reverting to the previous logic if (loading || !userSettings) would also work.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: apps/spruce/src/pages/preferences/preferencesTabs/NotificationsTab.tsx#L11 Potential issue: The `useUserSettings` hook returns `userSettings` as `user?.settings ?? {}`, which guarantees it is always at least an empty, truthy object. Because of this, the `!userSettings` part of the conditional `if (loading && !userSettings)` will always evaluate to `false`. This makes the entire condition `false`, regardless of the `loading` state. As a result, the loading skeleton will never be rendered, and users will see a broken UI with empty values while data is being fetched.

sentry · 2026-03-30T21:31:16Z

  const { notifications, slackMemberId, slackUsername } = userSettings ?? {};

-  if (loading || !userSettings) {
+  if (loading && !userSettings) {


Bug: The condition to show the loading skeleton was changed to if (loading && !userSettings). Since userSettings is always truthy, the skeleton will never render.
_{Severity: MEDIUM}

Suggested Fix

Revert the conditional logic to if (loading || !userSettings), or simplify it to if (loading) to correctly display the loading skeleton when data is being fetched. This aligns with the pattern used in other similar components.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not valid. Location: apps/spruce/src/pages/preferences/preferencesTabs/NotificationsTab.tsx#L11 Potential issue: The conditional logic for displaying the loading skeleton in the Notifications tab was changed from `if (loading || !userSettings)` to `if (loading && !userSettings)`. The `useUserSettings` hook ensures `userSettings` is always a truthy object (defaulting to `{}`), which makes the `!userSettings` part of the condition always `false`. As a result, the entire expression `loading && false` is always `false`, and the loading skeleton is never rendered. This will cause the component to attempt rendering with empty or undefined data during initial page loads or data refetches, instead of showing the expected loading state.

minnakt · 2026-03-30T21:32:29Z

+      cy.contains("button", "Delete").click();
+      cy.contains("button", "Delete").click();


Sorry I didn't pick up on your question from before. Clicking the button twice probably isn't what we intend to do; ideally, we should mimic real situations where the button would be clicked once.

Maybe it will help to assert that the delete-project-modal is visible first

Tried that no luck :(

minnakt · 2026-03-30T21:33:10Z

-        .find("button")
-        .contains("Attach")
-        .parent()
-        .click();


is there a reason why we don't attach to repo anymore? We are likely checking that the project can be deleted even if it is attached to a repo

No :( In theory once the refresh problem is actually fixed this would pass, I was being lazy

remove notifyOnNetworkStatusChange

11d3a02

sophiayangDB marked this pull request as ready for review March 26, 2026 17:53

sophiayangDB requested a review from a team as a code owner March 26, 2026 17:53

minnakt requested changes Mar 26, 2026

View reviewed changes

minnakt changed the title ~~DEVPROD-26582: Consider removing notifyOnNetworkStatusChange: false at global apollo client level~~ DEVPROD-26582: Remove notifyOnNetworkStatusChange: false at global apollo client level Mar 26, 2026

sophiayangDB added 7 commits March 26, 2026 14:56

test checks

5397fa0

tests...

45f530a

another try

42688f3

another attempt with logs

34bfe61

another attempt

682fd45

test fix

520700f

please fix?

9c6b865