Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(custom-views): Add custom views to issue stream behind feature flag #75883

Merged
merged 14 commits into from
Aug 16, 2024

Conversation

MichaelSun48
Copy link
Member

@MichaelSun48 MichaelSun48 commented Aug 8, 2024

This PR releases the custom views feature into the issue stream under a feature flag. Flagged users will also see the "Custom Search" button has been removed from the issue stream in favor of custom views. The bulk of this new component exists in <CustomViewsIssueListHeader/>, whose parent component is in overview.tsx.

Several changes have also been made to the base draggable tab components to iron out any compatibility issues that did not appear during sandbox development.

Things that are still broken that I am actively working on:

  • Tab Renaming is both ugly and broken (can't type in spaces) (fix(custom-views): editableTabTitle Improvements #76247)
  • Tab overflow logic does not exist at all
  • QueryCounts do not work for any tabs with queries that do not match queries of previous default tabs
  • Some queries that are functionally the same but off by a space register as different, thus triggering the "unsaved changes" state.
  • Tabs do not animate in when added, nor do they animate out when deleted
  • No loading or failure state for tabs request
  • Stories is busted as a result of all of the changes made to base tabs components (deleted stories, this component is not, and was not meant to be reusable)

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Bundle Report

Changes will increase total bundle size by 658.22kB ⬆️

Bundle name Size Change
app-webpack-bundle-array-push 29.51MB 658.22kB ⬆️

Copy link

codecov bot commented Aug 8, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
7668 1 7667 0
View the top 1 failed tests by shortest run time
trace view keyboard navigation roving updates the element in the drawer�trace view keyboard navigation roving updates the element in the drawer
Stack Traces | 3s run time
Error: Unable to find an element with the text: /transaction-op-0/i. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

Ignored nodes: comments, script, style
...
at waitForWrapper (.../sentry/sentry/node_modules/@.../dom/dist/wait-for.js:163:27)
at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:86:33
at keyboardNavigationTestSetup (.../performance/newTraceDetails/trace.spec.tsx:261:52)
at Object.<anonymous> (.../performance/newTraceDetails/trace.spec.tsx:706:17)
at Promise.then.completed (.../sentry/sentry/node_modules/jest-circus/build/utils.js:298:28)
at new Promise (<anonymous>)
at callAsyncCircusFn (.../sentry/sentry/node_modules/jest-circus/build/utils.js:231:10)
at _callCircusTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:316:40)
at runNextTicks (node:internal/process/task_queues:60:5)
at listOnTimeout (node:internal/timers:540:9)
at processTimers (node:internal/timers:514:7)
at _runTest (.../sentry/sentry/node_modules/jest-circus/build/run.js:252:3)
at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:126:9)
at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
at _runTestsForDescribeBlock (.../sentry/sentry/node_modules/jest-circus/build/run.js:121:9)
at run (.../sentry/sentry/node_modules/jest-circus/build/run.js:71:3)
at runAndTransformResultsToJestFormat (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:122:21)
at jestAdapter (.../sentry/sentry/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:79:19)
at runTestInternal (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:367:16)
at runTest (.../sentry/sentry/node_modules/jest-runner/build/runTest.js:444:34)
at Object.worker (.../sentry/sentry/node_modules/jest-runner/build/testWorker.js:106:12)

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

Copy link
Member Author

Choose a reason for hiding this comment

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

You can safely ignore this stories file - the storybook is somewhat broken as a result of some of the refactoring I've done to the draggable tabs components.

@@ -5,7 +5,8 @@ import {
type Tab,
} from 'sentry/views/issueList/groupSearchViewTabs/draggableTabBar';

describe('DraggableTabBar', () => {
// biome-ignore lint/suspicious/noSkippedTests: <explanation>
describe.skip('DraggableTabBar', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

There have been a couple of changes to DraggableTabBar to get custom views to a point where it can be released internally. These changes have also wrecked these tests, so in the interest of getting this feature out to internal as fast as possible, I've skipped them for now and have added the task of getting them working again to the list of things to do while this PR is getting reviewed or after custom views is released internally.

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [queryCounts]);

const onAddView = () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

The state management of these tabs is tangled between CustomViewsHeader and DraggableTabBar right now, though I'd ideally like to move the tab state management to DraggableTabBar in the future and just have CustomViewsHeader worry about the debouncing and which queries/sort to navigate to. This is easy enough to do for onDelete, for example, but onAddView and onDiscard require some more information about the query being displayed, so I have left the handlers for this in CustomViewsHeader for now.

I'd like to generalize DraggableTabBar as much as possible in general, but in the interest of getting custom views out for internal testing as fast as possible, I'll leave this up to future me to do in future PRs, or while this PR is getting reviewed.

@MichaelSun48 MichaelSun48 marked this pull request as ready for review August 9, 2024 22:02
@MichaelSun48 MichaelSun48 requested a review from a team as a code owner August 9, 2024 22:02
static/app/views/issueList/customViewsHeader.tsx Outdated Show resolved Hide resolved
static/app/views/issueList/customViewsHeader.tsx Outdated Show resolved Hide resolved
static/app/views/issueList/customViewsHeader.tsx Outdated Show resolved Hide resolved
}

const [draggableTabs, setDraggableTabs] = useState<Tab[]>(viewsToTabs);
const [selectedTabKey, setSelectedTabKey] = useState<string>(initialTabKey);
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we can get the selected tab key from the URL? We want to avoid having two sources of truth if possible

static/app/views/issueList/customViewsHeader.tsx Outdated Show resolved Hide resolved
static/app/views/issueList/customViewsHeader.tsx Outdated Show resolved Hide resolved
static/app/utils/withSavedSearches.tsx Outdated Show resolved Hide resolved
static/app/views/issueList/customViewsHeader.tsx Outdated Show resolved Hide resolved
static/app/views/issueList/customViewsHeader.tsx Outdated Show resolved Hide resolved
return draggableTabs[0].key;
};

// TODO: infer selected tab key state from URL params
Copy link
Member

@malwilley malwilley Aug 15, 2024

Choose a reason for hiding this comment

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

Can still do this in a followup, but I forgot to provide the number 1 reason for using the URL params to drive this is that you need to support using browser navigation (back/forward). Right now if you switch tabs and click the back button, you will end up with unsaved changes instead of switching to the previous tab.

});
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Member

Choose a reason for hiding this comment

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

When you take a second crack at this, we want to remove as many of these as possible since it usually leads to bugs

@MichaelSun48 MichaelSun48 merged commit 28a4dd2 into master Aug 16, 2024
41 checks passed
@MichaelSun48 MichaelSun48 deleted the msun/addCustomViewsToIssueStream branch August 16, 2024 16:38
Copy link

sentry-io bot commented Aug 16, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: Cannot read properties of undefined (reading '0'): /issues /issues View Issue

Did you find this useful? React with a 👍 or 👎

@MichaelSun48 MichaelSun48 added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Aug 16, 2024
@getsentry-bot
Copy link
Contributor

PR reverted: 3a79696

getsentry-bot added a commit that referenced this pull request Aug 16, 2024
…eature flag (#75883)"

This reverts commit 28a4dd2.

Co-authored-by: MichaelSun48 <55160142+MichaelSun48@users.noreply.github.com>
MichaelSun48 added a commit that referenced this pull request Aug 16, 2024
…ount for default tabs) (#76327)

Same PR as #75883, except this includes a fix to a bug that caused the
issue stream to crash. This bug was caused by the frontend trying to
access the `id` of a tab object, but forgetting to account for the fact
that default tabs do not have ids, leading to a `Cannot read properties
of undefined` error.
@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2024
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 Trigger: Revert Add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants