feat(issue-views): add query counts back to tabs #82990
Conversation
github-actions
Bot
added
the
Scope: Frontend
❌ 10 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
MichaelSun48
force-pushed
the
msun/issueViews/addQueryCountsBack
branch
from
5469cbb to
5fd797d
Compare
MichaelSun48
force-pushed
the
msun/issueViews/addQueryCountsBack
branch
from
5fd797d to
fcf22f7
Compare
| text-overflow: ellipsis; | ||
| padding-right: 1px; | ||
| cursor: pointer; | ||
| line-height: 1.45; |
There was a problem hiding this comment.
Very minor change to align the view name better with the count:
Before:
After:
malwilley
left a comment
malwilley
left a comment
There was a problem hiding this comment.
Noting a couple things that feel a bit off:
Not sure how to reproduce consistently, but sometimes when switching projects I get an empty query count
Also searching a new query doesn't update the query count for a saved tab (maybe this is expected though?)
| ) => { | ||
| return useApiQuery<QueryCounts>(makeFetchIssueCounts(params), { | ||
| staleTime: 0, | ||
| placeholderData: keepPreviousData, |
There was a problem hiding this comment.
We want to keep previous data while new data loads so that the query count bubble doesn't change size while the new count loads. If this were not here, the previous data would be scrubbed, and the query count bubble would lose its content and shrink to its min width before growing again.
|
@malwilley both of those were unintentional bugs and are fixed now! |
|
@MichaelSun48 some failing tests! Also noticed that the loading state looks too white in dark mode:
And it may make sense to increase the stale time for the hook that fetches these counts. If you go to another page and come back it refetches everything and shows the loading state, which can be very long for some of my views |
| animate={{ | ||
| backgroundColor: queryCountFetching | ||
| ? [theme.surface400, theme.surface100, theme.surface400] | ||
| : `#00000000`, |
There was a problem hiding this comment.
I think there are some issues with just using 'transparent' here - there are some consloe warnings about not being able to interpolate the hex values with the transparent keyword, hence why we're using #000000 here
malwilley
left a comment
malwilley
left a comment
There was a problem hiding this comment.
One other small thing that I noticed is that if you click into an issue, then go back to the stream the counts badge will fade in. If we can avoid that animation when the data is already present that would be ideal
|
|
Bundle ReportChanges will decrease total bundle size by 1.3MB (-4.04%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
| @@ -30,7 +30,8 @@ export const useFetchIssueCounts = ( | |||
| options: Partial<UseApiQueryOptions<Record<string, QueryCount>>> = {} | |||
There was a problem hiding this comment.
quick question @MichaelSun48: From what I’m seeing, this API endpoint is not returning Record<string, QueryCount>, but rather Record<string, number>:
The only usage of this hook is in IssueViewQueryCount, and there, the data is stored in a count state, which is of type number.
I stumbled upon this while setting up stricter compiler options for the typescript compiler - just wanted to confirm that my observations are correct, as this code is quite new, and then I’ll update the type accordingly.
There was a problem hiding this comment.
Good catch! I took a look myself and yes, it does look like this is a discrepancy in the expected and actual return type, my bad!
| useEffect(() => { | ||
| // Only update the count once the query has finished fetching | ||
| // This preserves the previous count while the query is fetching a new one | ||
| if (queryCount && !isFetching) { | ||
| setCount( | ||
| queryCount?.[view.unsavedChanges ? view.unsavedChanges[0] : view.query] ?? 0 | ||
| ); | ||
| } else if (isError) { | ||
| setCount(0); | ||
| } | ||
| }, [queryCount, isFetching, isError, view.query, view.unsavedChanges]); |
There was a problem hiding this comment.
@MichaelSun48 Are we sure this useEffect is necessary? Since the query uses placeholderData: keepPreviousData, state gets preserved by react-query when switching between input.
Unless I’m missing something, I’m getting the same user-experience with just deriving that value:
const count = isError
? 0
: queryCount?.[view.unsavedChanges ? view.unsavedChanges[0] : view.query] ?? 0;
There was a problem hiding this comment.
Hey good point! This useEffect is to produce a minor animation difference that was specifically spec'd by our designer: when the query is updated and a new count is being fetched, the query bubble does not change in size while fetching. If you give it a spin with your suggestion, you'll notice that when the query changes, the count bubble shrinks and is set to 0 as it starts its animation, then expands again. This is most noticeable when you change from a 2-3 digit count to another 2-3 digit count.
You make a good point that this should be fixed by the placeholderData: keepPreviousData, but as you observed in the previous comment, the return type of this endpoint is a dictionary mapping a query to a count, rather than just a count. This means that in order to properly use that previous data, we also need some state to remember what the previous query was when the new query comes in.
If this endpoint would just take in a single query and return an integer, then there would be no need for the effect and state, but this endpoint already existed and I didn't feel it was worth it to create a nearly identical endpoint just for the sake of this feature.
I'd also like to avoid the state and effect (and complexity) that is required just to make this one little animation difference work, let me know if you can think of another way to achieve the same behavior!
There was a problem hiding this comment.
Thanks for explaining this. Maybe I’m missing something but the animations look identical to me? Left side of the video is my localhost with the derived code, right side is production
Screen.Recording.2025-01-15.at.11.14.59.mov
You make a good point that this should be fixed by the placeholderData: keepPreviousData, but as you observed in the previous comment, the return type of this endpoint is a dictionary mapping a query to a count, rather than just a count. This means that in order to properly use that previous data, we also need some state to remember what the previous query was when the new query comes in.
placeholerData: keepPreviousData keeps the data of the previous query key around while you’re transitioning to one that doesn’t have data yet.
So when we first have this data:
{"is:resolved issue.priority:high":97}
and I change the date filter to “Last hour”, we’ll keep having this map until it changes to:
- {"is:resolved issue.priority:high":97}
+ {"is:resolved issue.priority:high":null}there is no render in between where the count bubble could shrink and is set to 0 for the start animation. Imo, the effect does the same thing, just a bit “later” and with an additional render cycle.
There was a problem hiding this comment.
I tested it again on master instead of my local version of this branch, and it appears you're right! I can't notice any noticeable difference between your edit and what's in master, and your edit avoids the state and effect, thanks for the catch and apologies for the thrash!
I'll put up a PR to make this change and to fix the typing in your previous comment!
There was a problem hiding this comment.
@TkDodo I just realized what's going on here and why there's a discrepancy between our experiences - in the videos you showed, you're leveraging the time filter to manipulate the query count not the query. If you change the query instead, you'll see this:
Screen.Recording.2025-01-16.at.2.56.13.PM.mov
(in prod)
When the view's query changes, we are attempting to access a key that doesn't exist in the placeholder data. So what's happening is:
Start of video
query = '!is:archived'
response_data = {'!is:archived': 100}
// Count displayed: response_data[query] = 100
Immediately after query changes, before endpoint responds:
query = '!is:ongoing'
response_data = {'!is:archived': 100} // Stays the same because we keep previous data
// Count displayed: response_data[query] = undefined <- this is why it flashes to 0
After endpoint responds:
query = '!is:ongoing'
response_data = {'!is:ongoing': 100} // This stays the same because we keep previous data
// Count displayed: response_data[query] = 48
In your video, you trigger a refetch by updating the time filter and not the query, which why it appears to work correctly. So we need to keep track of the last submitted query somehow in order to prevent this.
There was a problem hiding this comment.
omg 🤦♂️ you’re totally right, you even mentioned that if the endpoint would just return an integer, we wouldn’t have that problem. Sorry for missing that.
I think we should either just revert the change to what it was before, or use the storing information from previous renders pattern to keep track of the previousCount.
const [prevCount, setPrevCount] = useState(0);
// compute the next count from the query
const count = isError
? 0
// while we are transitioning between queries, use prevCount instead
: isPlaceholderData
? prevCount
: queryCount?.[view.unsavedChanges ? view.unsavedChanges[0] : view.query] ?? 0;
if (prevCount !== count) {
setPrevCount(count);
}
isPlaceholderData is a flag you can destruct from useFetchIssueCounts and it will be true when the query has placeholderData while it’s fetching new data.
This arguably isn’t a lot less complex than the previous solution (and we’d need to check if it covers all cases), it’s just a bit more optimized as it doesn’t use an effect. I think it’s fine either way.
There was a problem hiding this comment.
I just thought of another idea: Sine we only ever fire one query to the /issue-count endpoint here, we only expect an object with exactly one entry to be returned that contains the issue count we want to display. So maybe the simplest solution would be to just always use the element of the first key we find:
const count = isError ? 0 : queryCount?.[Object.keys(queryCount)[0]!] ?? 0;
That way, the lookup shouldn’t fail even if the query changes without having to keep track of previous things. If this feels too unsafe, we could also try to only use this as a better fallback than zero:
const count = isError
? 0
: queryCount?.[view.unsavedChanges ? view.unsavedChanges[0] : view.query] ??
queryCount?.[Object.keys(queryCount)[0]!] ??
0;
There was a problem hiding this comment.
These are both great ideas! I think the increased complexity is worth getting rid of the effect and state, especially since this component is specific to the Issue Views feature and has no plans for reuse. I'm going to go ahead with the second solution since it eliminates both the state and effect.
…t, fix apiquery typing (#83515) Makes two minor, non-visual changes to the query count component: 1. Removes the use of state and effect from the query count (as per [this conversation](#82990 (comment)) in the original PR). 2. Fixes the typing of the apiQuery hook for issue counts
~Reverts the latest commit in [this PR](#83515) by adding back the use of and effect and state to achieve the desired animation behavior for issue counts.~ Implements the query count animation in a way that achieves the desired behavior without using state and effect [Relevant discussion](#82990 (comment)) This is currently in prod: https://github.com/user-attachments/assets/6c64d85c-6e39-41a5-b11e-6ceeaafa76e7 When you change the search query (not the page filters), the bubble flashes to 0 and shrinks before resizing once the response comes in.
~Pending [this PR](#82987) to be merged.~ Adds the query count back to issue view tabs, this time with a hard max of 99 (counts will not go above 99 even after clicking into the tab, unlike before).  Temp view also works: 
…t, fix apiquery typing (#83515) Makes two minor, non-visual changes to the query count component: 1. Removes the use of state and effect from the query count (as per [this conversation](#82990 (comment)) in the original PR). 2. Fixes the typing of the apiQuery hook for issue counts
~Reverts the latest commit in [this PR](#83515) by adding back the use of and effect and state to achieve the desired animation behavior for issue counts.~ Implements the query count animation in a way that achieves the desired behavior without using state and effect [Relevant discussion](#82990 (comment)) This is currently in prod: https://github.com/user-attachments/assets/6c64d85c-6e39-41a5-b11e-6ceeaafa76e7 When you change the search query (not the page filters), the bubble flashes to 0 and shrinks before resizing once the response comes in.
3 participants
Pending this PR to be merged.Adds the query count back to issue view tabs, this time with a hard max of 99 (counts will not go above 99 even after clicking into the tab, unlike before).
Temp view also works: