Skip to content

ref(shared-views): Refactor feature flags and remove some more old components #91506

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

Merged
merged 2 commits into from
May 13, 2025

Conversation

MichaelSun48
Copy link
Member

@MichaelSun48 MichaelSun48 commented May 12, 2025

Removes some components that are no longer being used as part of the issue view sharing release.

Also refactors some feature flag checks

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 12, 2025
@@ -25,10 +25,10 @@ function IssueListFilters({query, sort, onSortChange, onSearch}: Props) {
const organization = useOrganization();

const hasIssueViews = organization.features.includes('issue-stream-custom-views');
const hasIssueViewSharing = useHasIssueViewSharing();
const prefersStackedNav = usePrefersStackedNav();
Copy link
Member Author

Choose a reason for hiding this comment

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

Here, and in severla other places, we used to check if the user had issue view sharing enabled. Since issue view sharing flag has been completely swallowed by the stacked nav flag, we can replace it with the usePrefersStackedNav() hook.

groupSearchView &&
canEditIssueView({groupSearchView, user, organization})
) {
if (groupSearchView && canEditIssueView({groupSearchView, user, organization})) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This component (PageTitle) is already gated by the stacked nav hook in a parent component (LeftNavViewsHeader)

Comment on lines -47 to -49
viewIds: newViews
.filter(view => view.id[0] !== '_' && !view.id.startsWith('default'))
.map(view => parseInt(view.id, 10)),
Copy link
Member Author

Choose a reason for hiding this comment

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

This shenanignas was cuz of the old default views and the "uncomitted" views. We can remove it now.

@MichaelSun48 MichaelSun48 requested a review from malwilley May 12, 2025 23:27
@MichaelSun48 MichaelSun48 marked this pull request as ready for review May 12, 2025 23:27
@MichaelSun48 MichaelSun48 requested a review from a team as a code owner May 12, 2025 23:27
Copy link

codecov bot commented May 13, 2025

❌ 3 Tests Failed:

Tests completed Failed Passed Skipped
10281 3 10278 9
View the top 3 failed test(s) by shortest run time
IssueListContainer issue views marks the current issue view as seen
Stack Traces | 0.025s run time
Error: expect(jest.fn()).toHaveBeenCalledTimes(expected)

Expected number of calls: 1
Received number of calls: 0
    at Object.<anonymous> (.../views/issueList/index.spec.tsx:64:37)
Sentry Application Details Editing an existing public Sentry App with a scope error handles client secret rotation
Stack Traces | 0.36s run time
TestingLibraryElementError: Unable to find an element with the text: This will be the only time your client secret is visible!. 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 Object.getElementError (.../sentry/sentry/node_modules/@.../dom/dist/config.js:37:19)
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:76:38
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:52:17
    at .../sentry/sentry/node_modules/@.../dom/dist/query-helpers.js:95:19
    at Object.<anonymous> (.../settings/organizationDeveloperSettings/sentryApplicationDetails.spec.tsx:591:42)
IssueListContainer issue views hydrates issue view query params
Stack Traces | 1.03s run time
Error: expect(received).toEqual(expected) // deep equality

- Expected  - 7
+ Received  + 1

- Object {
-   "environment": "prod",
-   "project": "1",
-   "query": "is:unresolved",
-   "sort": "date",
-   "statsPeriod": "7d",
- }
+ Object {}

Ignored nodes: comments, script, style
...
    at .../views/issueList/index.spec.tsx:77:39
    at runWithExpensiveErrorDiagnosticsDisabled (.../sentry/sentry/node_modules/@.../dom/dist/config.js:47:12)
    at checkCallback (.../sentry/sentry/node_modules/@.../dom/dist/wait-for.js:124:77)
    at checkRealTimersCallback (.../sentry/sentry/node_modules/@.../dom/dist/wait-for.js:118:16)
    at Timeout.task [as _onTimeout] (.../sentry/sentry/node_modules/jsdom/lib/jsdom/browser/Window.js:520:19)
    at listOnTimeout (node:internal/timers:594:17)
    at processTimers (node:internal/timers:529:7)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Copy link
Member

@malwilley malwilley left a comment

Choose a reason for hiding this comment

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

Looks like there are some failing tests still? Otherwise looks good

@MichaelSun48 MichaelSun48 merged commit daf22cc into master May 13, 2025
41 checks passed
@MichaelSun48 MichaelSun48 deleted the msun/sharedViews/postOldUIRefactors branch May 13, 2025 21:28
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 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.

None yet

2 participants