Skip to content

ref(shared-views): Use queryclient cache instead of state, rewire with new endpoints#89441

Merged
malwilley merged 11 commits into
masterfrom
msun/sharedViews/refUseQueryClientCache
Apr 15, 2025
Merged

ref(shared-views): Use queryclient cache instead of state, rewire with new endpoints#89441
malwilley merged 11 commits into
masterfrom
msun/sharedViews/refUseQueryClientCache

Conversation

@MichaelSun48

@MichaelSun48 MichaelSun48 commented Apr 11, 2025

Copy link
Copy Markdown
Contributor

This PR makes a couple of major refactors to the new Issue Views Navigation component family in preparation for the launch of shared views. The same refactors have not been applied to the legacy issue views component since it will be completely deleted with the release of issue view sharing.

Here's a high level overview of the changes:

  • QueryClient cache is now the single source of truth for starred views state: Previously, I used a react useState that consumed the views from the QueryClient cache, which was essentially a middleman that provided 0 value.
  • All entrypoints to PUT /group-search-views/ have been rewired: All issue view mutations in the new nav have been rewired to use the new CRUD endpoints created.
  • Unsaved Changes no longer persist across navigation: Clicking away from your view with unsaved changes will now discard those changes automatically. This saves us from a lot of state management and complexity.

Unfortunately, these refactors were a bit tricky to implement by themselves without destroying or degrading functionality (this has active users), hence why the size of this PR is quite large. I have tried to comprehensively annotate my changes in the diff, happy to elaborate more on any the changes.

Some bonus style changes:

  • Each component's mutation logic is significantly more localized. Previously, the root issue views component passed down a single bulk-mutate function to all of its children (because its debounced and updates react state). Now, each component self-contains its own mutation logic, rather than consuming it from its parent. This means significantly less prop drilling, and significantly more balanced components (in terms of LOC/component)

@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 11, 2025
Comment on lines +10 to +13
import type {IssueViewParams} from 'sentry/views/issueList/issueViews/issueViews';
import {IssueSortOptions} from 'sentry/views/issueList/utils';

export function createIssueViewFromUrl({
query,
}: {
query: Query;
}): Pick<
GroupSearchView,
'query' | 'querySort' | 'projects' | 'environments' | 'timeFilters'
> {
export function createIssueViewFromUrl({query}: {query: Query}): IssueViewParams {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Minor refactor:

Pick<GroupSearchView, 'query' | 'querySort' | 'projects' | 'environments' | 'timeFilters'>

Is completely congruent with the existing IssueViewParams type.

Comment on lines +27 to +33
changedParams: {
query: !isEqual(currentViewData.query, viewFromUrl.query),
querySort: !isEqual(currentViewData.querySort, viewFromUrl.querySort),
projects: !isEqual(currentViewData.projects, viewFromUrl.projects),
environments: !isEqual(currentViewData.environments, viewFromUrl.environments),
timeFilters: !isEqual(currentViewData.timeFilters, viewFromUrl.timeFilters),
},

@MichaelSun48 MichaelSun48 Apr 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is used here to support the unsavedChangesIndicator tooltip

@codecov

codecov Bot commented Apr 11, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #89441      +/-   ##
==========================================
+ Coverage   86.65%   86.66%   +0.01%     
==========================================
  Files       10159    10159              
  Lines      573773   573629     -144     
  Branches    22555    22529      -26     
==========================================
- Hits       497175   497138      -37     
+ Misses      76164    76057     -107     
  Partials      434      434              

Comment on lines -19 to -21
onDeleteView: () => void;
onDuplicateView: () => void;
onUpdateView: (view: IssueView) => void;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The ellipsis menu can now make these event handlers within itself, rather than relying on the root issue views component passing down the debounce update function.

Comment on lines +43 to +74
const {mutate: updateIssueView} = useUpdateGroupSearchView({
onSuccess: () => {
trackAnalytics('issue_views.saved_changes', {
leftNav: true,
organization: organization.slug,
});
},
});

trackAnalytics('issue_views.saved_changes', {
leftNav: true,
organization: organization.slug,
});
};
const {mutate: deleteIssueView} = useDeleteGroupSearchView({
onSuccess: () => {
trackAnalytics('issue_views.deleted_view', {
leftNav: true,
organization: organization.slug,
});
},
onError: () => {
addErrorMessage(t('Failed to delete view'));
},
});

const handleDiscardChanges = () => {
const updatedView = {
...view,
unsavedChanges: undefined,
};
onUpdateView(updatedView);
navigate(constructViewLink(baseUrl, updatedView));
const {mutate: createIssueView} = useCreateGroupSearchView({
onSuccess: data => {
navigate(
normalizeUrl(`/organizations/${organization.slug}/issues/views/${data.id}/`)
);
trackAnalytics('issue_views.duplicated_view', {
leftNav: true,
organization,
});
},
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Completely remove the Usage of PUT /group-search-views in favor of new purposeful endpoints:

  • Duplicate: POST /group-search-views/
  • Delete: DELETE /group-search-views/:viewId

);
};

// TODO(msun): Once nuqs supports native array query params, we can use that here and replace this absurd function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This outrageous function has been replace by the useIssueViewUnsavedChanges();, which is a far cleaner way of handling the same behavior.

}
}, [loadedViews, views, setViews]);

const replaceWithPersistentViewIds = useCallback(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Get rid of this absurd logic to hotswap the viewId in the url params to support optimistic rendering.

Now, hitting duplicate will result in a second before the view appears. This is a slightly worse experience, but we are going to remove this as an option entirely pretty soon anyways.

sectionRef={sectionRef}
baseUrl={baseUrl}
/>
<IssueViewNavItems sectionRef={sectionRef} baseUrl={baseUrl} />

@MichaelSun48 MichaelSun48 Apr 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The IssueViewNavItems component no longer needs to consumes views from the nav — it instead consumes it from the APIQueryClient cache.

@MichaelSun48 MichaelSun48 force-pushed the msun/sharedViews/refUseQueryClientCache branch from ff0b4fe to 0c45ef6 Compare April 11, 2025 23:13
Comment on lines -229 to -234
onChange={value => {
onUpdateView({...view, label: value});
trackAnalytics('issue_views.renamed_view', {
leftNav: true,
organization: organization.slug,
});

@MichaelSun48 MichaelSun48 Apr 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The IssueViewEditableTitle handles this mutation within itself now

@MichaelSun48 MichaelSun48 marked this pull request as ready for review April 11, 2025 23:43
@MichaelSun48 MichaelSun48 requested a review from a team as a code owner April 11, 2025 23:43
Comment thread static/app/views/issueList/mutations/useDeleteGroupSearchView.tsx Outdated
@MichaelSun48 MichaelSun48 requested a review from malwilley April 14, 2025 18:31
Co-authored-by: Scott Cooper <scttcper@gmail.com>
Comment thread static/app/views/issueList/issueViews/issueViewsList/issueViewsList.tsx Outdated
Comment thread static/app/views/issueList/mutations/useDeleteGroupSearchView.tsx Outdated
makeFetchStarredGroupSearchViewsKey({orgSlug: organization.slug}),
data => data?.filter(view => view.id !== variables.id)
);
} else {

@malwilley malwilley Apr 14, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@MichaelSun48 actually the invalidation that I thought was happening is actually not working correctly because it is pointing at the old query and not the /starred/ query.

This is the line that needs to be updated:

queryKey: makeFetchGroupSearchViewsKey({orgSlug: organization.slug}),

You can see in the vercel deployment that unstarring from the all views page doesn't recover correctly because of this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤦 my bad! so many caches to deal with 😵‍💫

@malwilley malwilley merged commit 0cf29e0 into master Apr 15, 2025
@malwilley malwilley deleted the msun/sharedViews/refUseQueryClientCache branch April 15, 2025 17:19
andrewshie-sentry pushed a commit that referenced this pull request Apr 22, 2025
…h new endpoints (#89441)

This PR makes a couple of major refactors to the new Issue Views
Navigation component family in preparation for the launch of shared
views. The same refactors have **not** been applied to the legacy issue
views component since it will be completely deleted with the release of
issue view sharing.

Here's a high level overview of the changes: 
* **QueryClient cache is now the single source of truth for starred
views state:** Previously, I used a react useState that consumed the
views from the QueryClient cache, which was essentially a middleman that
provided 0 value.
* **All entrypoints to `PUT` `/group-search-views/` have been rewired:**
All issue view mutations in the new nav have been rewired to use the new
CRUD endpoints created.
* **Unsaved Changes no longer persist across navigation:** Clicking away
from your view with unsaved changes will now discard those changes
automatically. This saves us from a lot of state management and
complexity.
@github-actions github-actions Bot locked and limited conversation to collaborators May 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants