From 1e29d42edf156450dd05c00bc33a8dd0be5fb8ab Mon Sep 17 00:00:00 2001 From: Michael Sun <55160142+MichaelSun48@users.noreply.github.com> Date: Fri, 27 Sep 2024 15:46:03 -0700 Subject: [PATCH] fix(issue-views): Fix uncommitted new views being saved (#78188) Fixes a bug where views that were newly created, but were not progressed past the new view flow, were being saved to the backend. https://github.com/user-attachments/assets/dde66230-7af9-41e6-8c2b-40ccde7b4fa2 _Notice how even after triggering an action that saves the views to the backend (renaming the first tab), the newly created view still has the new view flow, indicating it has not been saved._ Achieves this by adding an `isCommitted` property to each tab. The frontend only send views where `isCommitted=false` to be saved. **Logic details:** * All views are initially marked with `isCommitted=true` * Only views created via the `+ Add View` button will have is `isCommitted=false` * Views created via duplcating, or by saving a temp view have `isCommitted=false` * `isCommitted` for a view can be switched from `false` to `true`... * If the view is renamed * If a recommended search or saved search is clicked * If the query is changed manually There are some interesting questions to be considered regarding the last bullet point: * Should we toggle `isCommitted` to true if the query is changed manually or if a recommended/saved search is chosen from a new view? Maybe this would give the user a chance to look at the results first before hitting `Save Changes`, which would then toggle `isCommitted` to true. * Should we toggle `isCommitted` to true if the view is renamed? This cause the new view flow to disappear after renaming, which probably isn't ideal. On the other hand, it would be strange if I renamed a tab, forgot to add a query, then came back and found the tab to be gone. --- .../app/views/issueList/customViewsHeader.tsx | 25 ++++++++++++------- .../draggableTabBar.spec.tsx | 3 +++ .../groupSearchViewTabs/draggableTabBar.tsx | 21 ++++++++++++---- 3 files changed, 35 insertions(+), 14 deletions(-) diff --git a/static/app/views/issueList/customViewsHeader.tsx b/static/app/views/issueList/customViewsHeader.tsx index 67bca434a0acc3..7fdfa231570f81 100644 --- a/static/app/views/issueList/customViewsHeader.tsx +++ b/static/app/views/issueList/customViewsHeader.tsx @@ -144,6 +144,7 @@ function CustomViewsIssueListHeaderTabsContent({ label: name, query: viewQuery, querySort: viewQuerySort, + isCommitted: true, }; } ); @@ -177,6 +178,7 @@ function CustomViewsIssueListHeaderTabsContent({ label: t('Unsaved'), query: query, querySort: sort ?? IssueSortOptions.DATE, + isCommitted: true, } : undefined ); @@ -189,14 +191,18 @@ function CustomViewsIssueListHeaderTabsContent({ if (newTabs) { updateViews({ orgSlug: organization.slug, - groupSearchViews: newTabs.map(tab => ({ - // Do not send over an ID if it's a temporary or default tab so that - // the backend will save these and generate permanent Ids for them - ...(tab.id[0] !== '_' && !tab.id.startsWith('default') ? {id: tab.id} : {}), - name: tab.label, - query: tab.query, - querySort: tab.querySort, - })), + groupSearchViews: newTabs + .filter(tab => tab.isCommitted) + .map(tab => ({ + // Do not send over an ID if it's a temporary or default tab so that + // the backend will save these and generate permanent Ids for them + ...(tab.id[0] !== '_' && !tab.id.startsWith('default') + ? {id: tab.id} + : {}), + name: tab.label, + query: tab.query, + querySort: tab.querySort, + })), }); } }, 500), @@ -329,7 +335,7 @@ function CustomViewsIssueListHeaderTabsContent({ useEffect(() => { if (viewId?.startsWith('_')) { - if (draggableTabs.find(tab => tab.id === viewId)?.label.endsWith('(Copy)')) { + if (draggableTabs.find(tab => tab.id === viewId)?.isCommitted) { return; } // If the user types in query manually while the new view flow is showing, @@ -342,6 +348,7 @@ function CustomViewsIssueListHeaderTabsContent({ ? { ...tab, unsavedChanges: [query, sort ?? IssueSortOptions.DATE], + isCommitted: true, } : tab ); diff --git a/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.spec.tsx b/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.spec.tsx index c1f98635533c51..2a1ad884a3ec5f 100644 --- a/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.spec.tsx +++ b/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.spec.tsx @@ -33,6 +33,7 @@ describe.skip('DraggableTabBar', () => { query: 'priority:high', querySort: IssueSortOptions.DATE, unsavedChanges: ['priority:low', IssueSortOptions.DATE], + isCommitted: true, }, { id: '2', @@ -40,6 +41,7 @@ describe.skip('DraggableTabBar', () => { label: 'For Review', query: 'is:unassigned', querySort: IssueSortOptions.DATE, + isCommitted: true, }, { id: '3', @@ -47,6 +49,7 @@ describe.skip('DraggableTabBar', () => { label: 'Regressed', query: 'is:regressed', querySort: IssueSortOptions.DATE, + isCommitted: true, }, ]; diff --git a/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.tsx b/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.tsx index 94b7b3398ae538..2b9425b41d3efb 100644 --- a/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.tsx +++ b/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.tsx @@ -27,6 +27,12 @@ import {NewTabContext, type NewView} from 'sentry/views/issueList/utils/newTabCo export interface Tab { id: string; + /** + * False for tabs that were added view the "Add View" button, but + * have not been edited in any way. Only tabs with isCommitted=true + * will be saved to the backend. + */ + isCommitted: boolean; key: string; label: string; query: string; @@ -122,7 +128,7 @@ export function DraggableTabBar({ const {setNewViewActive, setOnNewViewsSaved} = useContext(NewTabContext); const handleOnReorder = (newOrder: Node[]) => { - const newTabs = newOrder + const newTabs: Tab[] = newOrder .map(node => { const foundTab = tabs.find(tab => tab.key === node.key); return foundTab?.key === node.key ? foundTab : null; @@ -138,7 +144,7 @@ export function DraggableTabBar({ const handleOnSaveChanges = () => { const originalTab = tabs.find(tab => tab.key === tabListState?.selectedKey); if (originalTab) { - const newTabs = tabs.map(tab => { + const newTabs: Tab[] = tabs.map(tab => { return tab.key === tabListState?.selectedKey && tab.unsavedChanges ? { ...tab, @@ -186,7 +192,7 @@ export function DraggableTabBar({ const renamedTab = tabs.find(tb => tb.key === tabKey); if (renamedTab && newLabel !== renamedTab.label) { const newTabs = tabs.map(tab => - tab.key === renamedTab.key ? {...tab, label: newLabel} : tab + tab.key === renamedTab.key ? {...tab, label: newLabel, isCommitted: true} : tab ); setTabs(newTabs); onTabRenamed?.(newTabs, newLabel); @@ -201,13 +207,14 @@ export function DraggableTabBar({ if (idx !== -1) { const tempId = generateTempViewId(); const duplicatedTab = tabs[idx]; - const newTabs = [ + const newTabs: Tab[] = [ ...tabs.slice(0, idx + 1), { ...duplicatedTab, id: tempId, key: tempId, label: `${duplicatedTab.label} (Copy)`, + isCommitted: true, }, ...tabs.slice(idx + 1), ]; @@ -248,6 +255,7 @@ export function DraggableTabBar({ label: 'New View', query: tempTab.query, querySort: tempTab.querySort, + isCommitted: true, }; const newTabs = [...tabs, newTab]; navigate( @@ -287,7 +295,7 @@ export function DraggableTabBar({ const tempId = generateTempViewId(); const currentTab = tabs.find(tab => tab.key === tabListState?.selectedKey); if (currentTab) { - const newTabs = [ + const newTabs: Tab[] = [ ...tabs, { id: tempId, @@ -295,6 +303,7 @@ export function DraggableTabBar({ label: 'New View', query: '', querySort: IssueSortOptions.DATE, + isCommitted: false, }, ]; navigate({ @@ -333,6 +342,7 @@ export function DraggableTabBar({ unsavedChanges: view.saveQueryToView ? undefined : [view.query, IssueSortOptions.DATE], + isCommitted: true, }; return viewToTab; }); @@ -344,6 +354,7 @@ export function DraggableTabBar({ query: saveQueryToView ? query : '', querySort: IssueSortOptions.DATE, unsavedChanges: saveQueryToView ? undefined : [query, IssueSortOptions.DATE], + isCommitted: true, }; } return tab;