Skip to content

Commit

Permalink
fix(issue-views): Fix uncommitted new views being saved (#78188)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
MichaelSun48 committed Sep 27, 2024
1 parent 09cb65f commit 1e29d42
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 14 deletions.
25 changes: 16 additions & 9 deletions static/app/views/issueList/customViewsHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ function CustomViewsIssueListHeaderTabsContent({
label: name,
query: viewQuery,
querySort: viewQuerySort,
isCommitted: true,
};
}
);
Expand Down Expand Up @@ -177,6 +178,7 @@ function CustomViewsIssueListHeaderTabsContent({
label: t('Unsaved'),
query: query,
querySort: sort ?? IssueSortOptions.DATE,
isCommitted: true,
}
: undefined
);
Expand All @@ -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),
Expand Down Expand Up @@ -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,
Expand All @@ -342,6 +348,7 @@ function CustomViewsIssueListHeaderTabsContent({
? {
...tab,
unsavedChanges: [query, sort ?? IssueSortOptions.DATE],
isCommitted: true,
}
: tab
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,23 @@ describe.skip('DraggableTabBar', () => {
query: 'priority:high',
querySort: IssueSortOptions.DATE,
unsavedChanges: ['priority:low', IssueSortOptions.DATE],
isCommitted: true,
},
{
id: '2',
key: '2',
label: 'For Review',
query: 'is:unassigned',
querySort: IssueSortOptions.DATE,
isCommitted: true,
},
{
id: '3',
key: '3',
label: 'Regressed',
query: 'is:regressed',
querySort: IssueSortOptions.DATE,
isCommitted: true,
},
];

Expand Down
21 changes: 16 additions & 5 deletions static/app/views/issueList/groupSearchViewTabs/draggableTabBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -122,7 +128,7 @@ export function DraggableTabBar({
const {setNewViewActive, setOnNewViewsSaved} = useContext(NewTabContext);

const handleOnReorder = (newOrder: Node<DraggableTabListItemProps>[]) => {
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;
Expand All @@ -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,
Expand Down Expand Up @@ -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);
Expand All @@ -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),
];
Expand Down Expand Up @@ -248,6 +255,7 @@ export function DraggableTabBar({
label: 'New View',
query: tempTab.query,
querySort: tempTab.querySort,
isCommitted: true,
};
const newTabs = [...tabs, newTab];
navigate(
Expand Down Expand Up @@ -287,14 +295,15 @@ 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,
key: tempId,
label: 'New View',
query: '',
querySort: IssueSortOptions.DATE,
isCommitted: false,
},
];
navigate({
Expand Down Expand Up @@ -333,6 +342,7 @@ export function DraggableTabBar({
unsavedChanges: view.saveQueryToView
? undefined
: [view.query, IssueSortOptions.DATE],
isCommitted: true,
};
return viewToTab;
});
Expand All @@ -344,6 +354,7 @@ export function DraggableTabBar({
query: saveQueryToView ? query : '',
querySort: IssueSortOptions.DATE,
unsavedChanges: saveQueryToView ? undefined : [query, IssueSortOptions.DATE],
isCommitted: true,
};
}
return tab;
Expand Down

0 comments on commit 1e29d42

Please sign in to comment.