-
Notifications
You must be signed in to change notification settings - Fork 178
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
refactor(app): improve top level query chaining #16179
Conversation
738becd
to
1c4126c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple inline things; interested in your thoughts.
runRecord == null || | ||
runStatus == null || | ||
runActions == null || | ||
isFetching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't using isFetching
mean that we renav away after every notification? I think we just want the data being nullish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! There's no navigation action taking place if isFetching
is null. This isFetching
check currently exists in chore_release8.0.0
. I talked about it a bit in #16084.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved per in person convo. Returning null
doesn't do a redirect.
) { | ||
return null | ||
} | ||
// grabbing run id off of the run query to have all routing info come from one source of truth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels a little gross since we're fetching the run by exact id, hard to think of a situation where they'd be different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, yeah, I see what you mean here, and yeah, I agree. I guess this comes down to whether we want to keep isFetching
, which we probably do per in person convo
} | ||
// grabbing run id off of the run query to have all routing info come from one source of truth | ||
const runId = runRecord.data.id | ||
const hasRunStarted = runActions?.some( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you know, i know this has been the approach for a while, but it does feel a little gross and I wonder if it would be better to do one or more of
- check whether
startedAt
is non-null - have the run data model present a different status for "done" and "not started yet"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with #1, good idea.
For #2, I'd have to think about it a bit (and I'm starting to mull over the idea of run statuses in general with the ProtocolRunHeader
refactor). If this was just the "not started yet" case, I'd say this is a different question, but the concept of a done status (or terminal status as we more commonly call it in the app) would go a long way...the issue being that while a "not started yet" status is directly tied to one status only, a done status is tied to "failed", "completed", and "cancelled" statuses.
import { CURRENT_RUN_POLL } from './constants' | ||
import { useCurrentRunRoute } from './hooks' | ||
|
||
export function TopLevelRedirects(): JSX.Element | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we call this ODDTopLevelRedirects
? It will only ever be used there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be the change we want to see in the world.
ce30d89
to
a43aea5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... yes...
Closes EXEC-695
Overview
#16084 highlighted some of the existing issues we had after the recent React Router migration. While that PR fixed some problems, the intention was to prioritize keeping the bug radius small for the upcoming 8.0 release, which meant some changes were not implemented optimally.
Now that we have plenty of time before the 8.1 release, let's dog food networking changes for as long as possible, starting with this one: currently,
TopLevelRedirects
chains react queries, taking thecurrentRunId
fromuseCurrentRunId
and feeds it directly intouseNotifyQuery
. WhenevercurrentRunId
isnull
, weGET /runs/null
, which is a network request that we should avoid.After discussion, the most Reactive solution is to isolate the hooks into their own components, and conditionally render a component with the chained hook only if the hook(s) farther up the chain are not
null
. In other words, keep the concept of query chaining, but just do it in components.This PR adds some light refactoring and testing, too.
Current Behavior
Fixed Behavior
Test Plan and Hands on Testing
/runs/null
occurred.Changelog
run/null
route.Risk assessment
low in theory, but routing changes are always a bit fun