Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(app): Fix failedCommand caching issues #16874

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Nov 18, 2024

Closes RABR-671, RQA-3213, and RQA-3595

Overview

Fixes rendering issues resulting from state derived from the failedCommand, supplied from useCurrentlyRecoveringFrom, a waterfall request hook.

3a67269 - No "functional" changes here. Error Recovery once in a blue moon persisting past the point that is should is almost certainly a result of race conditions that occur given multiple state updates happening in sequence. Let's just confine things to useEffects properly and simplify the logic a bit.

a0e0330 - Since it sounds like we view the error recovery splash flickering as a bug, we should gate error recovery behind insurances that the waterfall requests are completed. The normal React Query pattern for doing this is utilizing the isFetching status, so we need to amalgamate our requests' isFetching statuses together and make sure we only gate the condition behind the first time we enter Error Recovery (since it's possible to continually fetch during Error Recovery if polling conditions occur).

8f953a4 - This is just an optimization. We actually can avoid a couple network requests during Error Recovery to get stepCounts, since we already have that data.

Note: I intentionally did not gate the "At step: ..." copy updating from preventing Error Recovery to render, since this is a command scanning operation, and it could take a while. If we want prevent this updating after the splash screen renders, we should tackle this server side IMO.

Test Plan and Hands on Testing

  • Ran various Error Recovery protocols, ensuring that there is no "flickering" when Error Recovery begins. If a second, different error populates the splash page, it does not cause a screen flicker, too.
  • Ensured that doing things like cancelling the run during Error Recovery persist the Error Recovery modal as expected.

Changelog

  • Fixed Error Recovery "flickering" with the incorrect error type at initial render.

Risk assessment

low - We're well covered by existing tests, and the new isFetching logic is pretty innocuous.

@mjhuff mjhuff requested review from sfoster1, TamarZanzouri, smb2268 and a team November 18, 2024 21:10
@mjhuff mjhuff requested a review from a team as a code owner November 18, 2024 21:10
const isValidERStatus = (status: RunStatus | null): boolean => {
return (
status !== null &&
VALID_ER_RUN_STATUSES.includes(status) &&
Copy link
Member

Choose a reason for hiding this comment

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

nit can we maybe phrase this

status !== null &&
(status === AWAITING_RECOVERY) || VALID_ER_RUN_STATUSES.includes(status) && hasSeenErrorRecovery

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks good, just a clarifying comment - I think that logic works better if it's the sentence "If we're in AWAITING_RECOVERY, or we're in one of these states after AWAITING_RECOVERY" rather than "if we're in one of these states and either we've seen the state AWAITING_RECOVERY or we are currently in AWAITING_RECOVERY" - spelled out like that it feels redundant even though it's formally not

@mjhuff mjhuff merged commit c0f95de into chore_release-8.2.0 Nov 19, 2024
30 checks passed
@mjhuff mjhuff deleted the app_fix-recovery-cache-issues branch November 19, 2024 01:09
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