Browse Source

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.
Michael Sun 5 months ago
parent
commit
1e29d42edf

+ 16 - 9
static/app/views/issueList/customViewsHeader.tsx

@@ -144,6 +144,7 @@ function CustomViewsIssueListHeaderTabsContent({
         label: name,
         label: name,
         query: viewQuery,
         query: viewQuery,
         querySort: viewQuerySort,
         querySort: viewQuerySort,
+        isCommitted: true,
       };
       };
     }
     }
   );
   );
@@ -177,6 +178,7 @@ function CustomViewsIssueListHeaderTabsContent({
           label: t('Unsaved'),
           label: t('Unsaved'),
           query: query,
           query: query,
           querySort: sort ?? IssueSortOptions.DATE,
           querySort: sort ?? IssueSortOptions.DATE,
+          isCommitted: true,
         }
         }
       : undefined
       : undefined
   );
   );
@@ -189,14 +191,18 @@ function CustomViewsIssueListHeaderTabsContent({
         if (newTabs) {
         if (newTabs) {
           updateViews({
           updateViews({
             orgSlug: organization.slug,
             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),
       }, 500),
@@ -329,7 +335,7 @@ function CustomViewsIssueListHeaderTabsContent({
 
 
   useEffect(() => {
   useEffect(() => {
     if (viewId?.startsWith('_')) {
     if (viewId?.startsWith('_')) {
-      if (draggableTabs.find(tab => tab.id === viewId)?.label.endsWith('(Copy)')) {
+      if (draggableTabs.find(tab => tab.id === viewId)?.isCommitted) {
         return;
         return;
       }
       }
       // If the user types in query manually while the new view flow is showing,
       // If the user types in query manually while the new view flow is showing,
@@ -342,6 +348,7 @@ function CustomViewsIssueListHeaderTabsContent({
             ? {
             ? {
                 ...tab,
                 ...tab,
                 unsavedChanges: [query, sort ?? IssueSortOptions.DATE],
                 unsavedChanges: [query, sort ?? IssueSortOptions.DATE],
+                isCommitted: true,
               }
               }
             : tab
             : tab
         );
         );

+ 3 - 0
static/app/views/issueList/groupSearchViewTabs/draggableTabBar.spec.tsx

@@ -33,6 +33,7 @@ describe.skip('DraggableTabBar', () => {
       query: 'priority:high',
       query: 'priority:high',
       querySort: IssueSortOptions.DATE,
       querySort: IssueSortOptions.DATE,
       unsavedChanges: ['priority:low', IssueSortOptions.DATE],
       unsavedChanges: ['priority:low', IssueSortOptions.DATE],
+      isCommitted: true,
     },
     },
     {
     {
       id: '2',
       id: '2',
@@ -40,6 +41,7 @@ describe.skip('DraggableTabBar', () => {
       label: 'For Review',
       label: 'For Review',
       query: 'is:unassigned',
       query: 'is:unassigned',
       querySort: IssueSortOptions.DATE,
       querySort: IssueSortOptions.DATE,
+      isCommitted: true,
     },
     },
     {
     {
       id: '3',
       id: '3',
@@ -47,6 +49,7 @@ describe.skip('DraggableTabBar', () => {
       label: 'Regressed',
       label: 'Regressed',
       query: 'is:regressed',
       query: 'is:regressed',
       querySort: IssueSortOptions.DATE,
       querySort: IssueSortOptions.DATE,
+      isCommitted: true,
     },
     },
   ];
   ];
 
 

+ 16 - 5
static/app/views/issueList/groupSearchViewTabs/draggableTabBar.tsx

@@ -27,6 +27,12 @@ import {NewTabContext, type NewView} from 'sentry/views/issueList/utils/newTabCo
 
 
 export interface Tab {
 export interface Tab {
   id: string;
   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;
   key: string;
   label: string;
   label: string;
   query: string;
   query: string;
@@ -122,7 +128,7 @@ export function DraggableTabBar({
   const {setNewViewActive, setOnNewViewsSaved} = useContext(NewTabContext);
   const {setNewViewActive, setOnNewViewsSaved} = useContext(NewTabContext);
 
 
   const handleOnReorder = (newOrder: Node<DraggableTabListItemProps>[]) => {
   const handleOnReorder = (newOrder: Node<DraggableTabListItemProps>[]) => {
-    const newTabs = newOrder
+    const newTabs: Tab[] = newOrder
       .map(node => {
       .map(node => {
         const foundTab = tabs.find(tab => tab.key === node.key);
         const foundTab = tabs.find(tab => tab.key === node.key);
         return foundTab?.key === node.key ? foundTab : null;
         return foundTab?.key === node.key ? foundTab : null;
@@ -138,7 +144,7 @@ export function DraggableTabBar({
   const handleOnSaveChanges = () => {
   const handleOnSaveChanges = () => {
     const originalTab = tabs.find(tab => tab.key === tabListState?.selectedKey);
     const originalTab = tabs.find(tab => tab.key === tabListState?.selectedKey);
     if (originalTab) {
     if (originalTab) {
-      const newTabs = tabs.map(tab => {
+      const newTabs: Tab[] = tabs.map(tab => {
         return tab.key === tabListState?.selectedKey && tab.unsavedChanges
         return tab.key === tabListState?.selectedKey && tab.unsavedChanges
           ? {
           ? {
               ...tab,
               ...tab,
@@ -186,7 +192,7 @@ export function DraggableTabBar({
     const renamedTab = tabs.find(tb => tb.key === tabKey);
     const renamedTab = tabs.find(tb => tb.key === tabKey);
     if (renamedTab && newLabel !== renamedTab.label) {
     if (renamedTab && newLabel !== renamedTab.label) {
       const newTabs = tabs.map(tab =>
       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);
       setTabs(newTabs);
       onTabRenamed?.(newTabs, newLabel);
       onTabRenamed?.(newTabs, newLabel);
@@ -201,13 +207,14 @@ export function DraggableTabBar({
     if (idx !== -1) {
     if (idx !== -1) {
       const tempId = generateTempViewId();
       const tempId = generateTempViewId();
       const duplicatedTab = tabs[idx];
       const duplicatedTab = tabs[idx];
-      const newTabs = [
+      const newTabs: Tab[] = [
         ...tabs.slice(0, idx + 1),
         ...tabs.slice(0, idx + 1),
         {
         {
           ...duplicatedTab,
           ...duplicatedTab,
           id: tempId,
           id: tempId,
           key: tempId,
           key: tempId,
           label: `${duplicatedTab.label} (Copy)`,
           label: `${duplicatedTab.label} (Copy)`,
+          isCommitted: true,
         },
         },
         ...tabs.slice(idx + 1),
         ...tabs.slice(idx + 1),
       ];
       ];
@@ -248,6 +255,7 @@ export function DraggableTabBar({
         label: 'New View',
         label: 'New View',
         query: tempTab.query,
         query: tempTab.query,
         querySort: tempTab.querySort,
         querySort: tempTab.querySort,
+        isCommitted: true,
       };
       };
       const newTabs = [...tabs, newTab];
       const newTabs = [...tabs, newTab];
       navigate(
       navigate(
@@ -287,7 +295,7 @@ export function DraggableTabBar({
     const tempId = generateTempViewId();
     const tempId = generateTempViewId();
     const currentTab = tabs.find(tab => tab.key === tabListState?.selectedKey);
     const currentTab = tabs.find(tab => tab.key === tabListState?.selectedKey);
     if (currentTab) {
     if (currentTab) {
-      const newTabs = [
+      const newTabs: Tab[] = [
         ...tabs,
         ...tabs,
         {
         {
           id: tempId,
           id: tempId,
@@ -295,6 +303,7 @@ export function DraggableTabBar({
           label: 'New View',
           label: 'New View',
           query: '',
           query: '',
           querySort: IssueSortOptions.DATE,
           querySort: IssueSortOptions.DATE,
+          isCommitted: false,
         },
         },
       ];
       ];
       navigate({
       navigate({
@@ -333,6 +342,7 @@ export function DraggableTabBar({
           unsavedChanges: view.saveQueryToView
           unsavedChanges: view.saveQueryToView
             ? undefined
             ? undefined
             : [view.query, IssueSortOptions.DATE],
             : [view.query, IssueSortOptions.DATE],
+          isCommitted: true,
         };
         };
         return viewToTab;
         return viewToTab;
       });
       });
@@ -344,6 +354,7 @@ export function DraggableTabBar({
             query: saveQueryToView ? query : '',
             query: saveQueryToView ? query : '',
             querySort: IssueSortOptions.DATE,
             querySort: IssueSortOptions.DATE,
             unsavedChanges: saveQueryToView ? undefined : [query, IssueSortOptions.DATE],
             unsavedChanges: saveQueryToView ? undefined : [query, IssueSortOptions.DATE],
+            isCommitted: true,
           };
           };
         }
         }
         return tab;
         return tab;