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

ref(issue-views): Overhaul issue views state and logic to a new context #82429

Merged
merged 7 commits into from
Dec 20, 2024

Conversation

MichaelSun48
Copy link
Member

@MichaelSun48 MichaelSun48 commented Dec 19, 2024

This PR makes a major code refactor to the Issue Views family of components. No functionality should be broken or otherwise altered.

The purpose of this refactor was to move lots of reused code and duplicated state into one unified context, IssueViews.tsx. This allows the displayed components to be much much cleaner and easier to understand while making it easier to add new functionality in the future. The two primary things it does are:

  1. Creates a new context, IssueViews, that extends the old Tabs context. This new context now contains the views and temporary tabs state
  2. Delegates almost all tab alteration logic to a useReducer within the IssueViews context

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Dec 19, 2024
@MichaelSun48 MichaelSun48 force-pushed the msun/issueViews/moveLogicToContext branch from 0ca20a8 to 71c94e0 Compare December 19, 2024 22:32
@MichaelSun48 MichaelSun48 marked this pull request as ready for review December 19, 2024 22:32
@MichaelSun48 MichaelSun48 requested a review from a team as a code owner December 19, 2024 22:32
import {NewTabContext, type NewView} from 'sentry/views/issueList/utils/newTabContext';

export interface Tab {
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to IssueView and moved to IssueViews.tsx

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.

It's great to see all the state logic in one place! Good refactor!

);
}

export function IssueViews<T extends string | number>({
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract this to a new file? We don't usually include components in util files

Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename this to issueViewsContext to be more descriptive? Might also be better to put everything together in an issueViews folder or something so that this isn't floating off in the utils folder for the whole issue list

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropped this in /groupsearchviewtabs/ for now, but I do want to save bigger renamings and restructures for a separate PR since there are a bunch of other places where it could be improved.

static/app/views/issueList/utils/issueViews.tsx Outdated Show resolved Hide resolved
}

export const IssueViewsContext = createContext<IssueViewsContextType>({
rootProps: {orientation: 'horizontal'},
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary to be in the context?

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 is one of react-aria's tab context variables that I included since this context is essentially just a wrapper on that one. I copied the behavior from our base tabs component here.

static/app/views/issueList/utils/issueViews.tsx Outdated Show resolved Hide resolved
static/app/views/issueList/utils/issueViews.tsx Outdated Show resolved Hide resolved
static/app/views/issueList/utils/issueViews.tsx Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 20, 2024

Bundle Report

Changes will increase total bundle size by 39.07kB (0.12%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
app-webpack-bundle-array-push 32.15MB 39.07kB (0.12%) ⬆️

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.

Good work!

@MichaelSun48 MichaelSun48 merged commit e2ca54d into master Dec 20, 2024
42 checks passed
@MichaelSun48 MichaelSun48 deleted the msun/issueViews/moveLogicToContext branch December 20, 2024 21:34
MichaelSun48 added a commit that referenced this pull request Dec 20, 2024
I deleted some of the analytics calls in [this refactoring
change](#82429) and forgot to
add them back. This PR adds them back.

I found what the old analytics keys were
[here](https://github.com/getsentry/sentry/blob/7cb3a89c7a0737ea5c53ab0a1e85b03c7d8dd4ee/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.tsx)
MichaelSun48 added a commit that referenced this pull request Dec 27, 2024
)

This PR refactors the issue views family of components by moving lots of
the navigation logic into the IssueViews context created in [this
PR](#82429).

Many of the tab actions that were folded into the IssueViews context
require changes to the query parameters, which is why it makes sense to
combine these two actions in the reducer function. For example:
* Duplicating a tab requires the newly created tab to be selected
(change viewId parameter)
* Discarding a tab's changes requires the query/sort to be reset to the
original query/sort (change query/sort param
Copy link

sentry-io bot commented Dec 27, 2024

Suspect Issues

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

  • ‼️ SecurityError: The operation is insecure. /issues View Issue
  • ‼️ SecurityError: Attempt to use history.replaceState() more than 100 times per 10 seconds /issues View Issue
  • ‼️ SecurityError: The operation is insecure. /issues View Issue
  • ‼️ SecurityError: Attempt to use history.replaceState() more than 100 times per 10 seconds /issues View Issue
  • ‼️ SecurityError: The operation is insecure. /issues View Issue

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

andrewshie-sentry pushed a commit that referenced this pull request Jan 2, 2025
…xt (#82429)

This PR makes a major code refactor to the Issue Views family of
components. No functionality should be broken or otherwise altered.

The purpose of this refactor was to move lots of reused code and
duplicated state into one unified context, `IssueViews.tsx`. This allows
the displayed components to be much much cleaner and easier to
understand while making it easier to add new functionality in the
future. The two primary things it does are:

1. **Creates a new context, `IssueViews`, that extends the old `Tabs`
context. This new context now contains the views and temporary tabs
state**
2. **Delegates almost all tab alteration logic to a `useReducer` within
the `IssueViews` context**
andrewshie-sentry pushed a commit that referenced this pull request Jan 2, 2025
I deleted some of the analytics calls in [this refactoring
change](#82429) and forgot to
add them back. This PR adds them back.

I found what the old analytics keys were
[here](https://github.com/getsentry/sentry/blob/7cb3a89c7a0737ea5c53ab0a1e85b03c7d8dd4ee/static/app/views/issueList/groupSearchViewTabs/draggableTabBar.tsx)
andrewshie-sentry pushed a commit that referenced this pull request Jan 2, 2025
)

This PR refactors the issue views family of components by moving lots of
the navigation logic into the IssueViews context created in [this
PR](#82429).

Many of the tab actions that were folded into the IssueViews context
require changes to the query parameters, which is why it makes sense to
combine these two actions in the reducer function. For example:
* Duplicating a tab requires the newly created tab to be selected
(change viewId parameter)
* Discarding a tab's changes requires the query/sort to be reset to the
original query/sort (change query/sort param
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 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.

3 participants