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.

Sign up for GitHub

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

Jump to bottom

fix(issues): Avoid issue list cache if projects not updated #76398

Closed
wants to merge 1 commit into from
Closed

fix(issues): Avoid issue list cache if projects not updated #76398

wants to merge 1 commit into from

Conversation

scttcper
Copy link
Member

fixes an issue where we load the wrong issues cache because the PageFiltersStore does not reflect the project in the url until after componentDidMount has run.

I did try getting the pageFiltersStore to update sooner but it broke other pages.

to reproduce the issue:

  • Go to projects list
  • Click "errors: 123" under a project name and load the issues
  • Return to projects list and click another "errors: 456" link
  • See that issues list is populated with the wrong issues

fixes #76387

@scttcper
fix(issues): Avoid issue list cache if projects not updated
b26014a 
fixes an issue where we load the wrong issues from the cache because the PageFiltersStore does not reflect the project in the url.

to reproduce the issue:
- Go to projects list
- Click "errors: 123" under a project name and load the issues
- Return to projects list and click another "errors: 456" link
- See that issues list is populated with the wrong issues

fixes #76387
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Aug 19, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 19, 2024

❌ 40 Tests Failed:

Tests completed Failed Passed Skipped
7690 40 7650 0
View the top 3 failed tests by shortest run time 
IssueList render states displays the loading icon when saved searches are loading IssueList render states displays the loading icon when saved searches are loading
Stack Traces | 0.01s run time 
TypeError: Cannot read properties of undefined (reading 'location')
    at IssueListOverview.componentDidMount (.../views/issueList/overview.tsx:11682:32)
    at commitLayoutEffectOnFiber (.../react-dom/cjs/react-dom.development.js:23310:28)
    at commitLayoutMountEffects_complete (.../react-dom/cjs/react-dom.development.js:24688:9)
    at commitLayoutEffects_begin (.../react-dom/cjs/react-dom.development.js:24674:7)
    at commitLayoutEffects (.../react-dom/cjs/react-dom.development.js:24612:3)
    at commitRootImpl (.../react-dom/cjs/react-dom.development.js:26823:5)
    at commitRoot (.../react-dom/cjs/react-dom.development.js:26682:5)
    at finishConcurrentRender (.../react-dom/cjs/react-dom.development.js:25981:9)
    at performConcurrentWorkOnRoot (.../react-dom/cjs/react-dom.development.js:25809:7)
    at flushActQueue (.../react/cjs/react.development.js:2667:24)
    at act (.../react/cjs/react.development.js:2582:11)
    at .../sentry/sentry/node_modules/@.../react/dist/act-compat.js:47:25
    at renderRoot (.../sentry/sentry/node_modules/@.../react/dist/pure.js:180:26)
    at Object.render (.../sentry/sentry/node_modules/@.../react/dist/pure.js:271:10)
    at render (.../js/sentry-test/reactTestingLibrary.tsx:134:14)
    at Object.<anonymous> (.../views/issueList/overview.spec.tsx:1054:39)
    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 processTicksAndRejections (node:internal/process/task_queues:95:5)
    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)

IssueList withStores and feature flags unpins a custom query IssueList withStores and feature flags unpins a custom query
Stack Traces | 0.013s run time 
TypeError: Cannot read properties of undefined (reading 'location')
    at IssueListOverview.componentDidMount (.../views/issueList/overview.tsx:11682:32)
    at commitLayoutEffectOnFiber (.../react-dom/cjs/react-dom.development.js:23310:28)
    at commitLayoutMountEffects_complete (.../react-dom/cjs/react-dom.development.js:24688:9)
    at commitLayoutEffects_begin (.../react-dom/cjs/react-dom.development.js:24674:7)
    at commitLayoutEffects (.../react-dom/cjs/react-dom.development.js:24612:3)
    at commitRootImpl (.../react-dom/cjs/react-dom.development.js:26823:5)
    at commitRoot (.../react-dom/cjs/react-dom.development.js:26682:5)
    at finishConcurrentRender (.../react-dom/cjs/react-dom.development.js:25981:9)
    at performConcurrentWorkOnRoot (.../react-dom/cjs/react-dom.development.js:25809:7)
    at flushActQueue (.../react/cjs/react.development.js:2667:24)
    at act (.../react/cjs/react.development.js:2582:11)
    at .../sentry/sentry/node_modules/@.../react/dist/act-compat.js:47:25
    at renderRoot (.../sentry/sentry/node_modules/@.../react/dist/pure.js:180:26)
    at Object.render (.../sentry/sentry/node_modules/@.../react/dist/pure.js:271:10)
    at render (.../js/sentry-test/reactTestingLibrary.tsx:134:14)
    at Object.<anonymous> (.../views/issueList/overview.spec.tsx:603:39)
    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 processTicksAndRejections (node:internal/process/task_queues:95:5)
    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)

IssueList withStores and feature flags does not allow pagination to "previous" while on first page and resets cursors when navigating back to initial page IssueList withStores and feature flags does not allow pagination to "previous" while on first page and resets cursors when navigating back to initial page
Stack Traces | 0.014s run time 
TypeError: Cannot read properties of undefined (reading 'location')
    at IssueListOverview.componentDidMount (.../views/issueList/overview.tsx:11682:32)
    at commitLayoutEffectOnFiber (.../react-dom/cjs/react-dom.development.js:23310:28)
    at commitLayoutMountEffects_complete (.../react-dom/cjs/react-dom.development.js:24688:9)
    at commitLayoutEffects_begin (.../react-dom/cjs/react-dom.development.js:24674:7)
    at commitLayoutEffects (.../react-dom/cjs/react-dom.development.js:24612:3)
    at commitRootImpl (.../react-dom/cjs/react-dom.development.js:26823:5)
    at commitRoot (.../react-dom/cjs/react-dom.development.js:26682:5)
    at finishConcurrentRender (.../react-dom/cjs/react-dom.development.js:25981:9)
    at performConcurrentWorkOnRoot (.../react-dom/cjs/react-dom.development.js:25809:7)
    at flushActQueue (.../react/cjs/react.development.js:2667:24)
    at act (.../react/cjs/react.development.js:2582:11)
    at .../sentry/sentry/node_modules/@.../react/dist/act-compat.js:47:25
    at renderRoot (.../sentry/sentry/node_modules/@.../react/dist/pure.js:180:26)
    at Object.render (.../sentry/sentry/node_modules/@.../react/dist/pure.js:271:10)
    at render (.../js/sentry-test/reactTestingLibrary.tsx:134:14)
    at Object.<anonymous> (.../views/issueList/overview.spec.tsx:791:43)
    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 processTicksAndRejections (node:internal/process/task_queues:95:5)
    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

@malwilley
Copy link
Member

One big bug I found is that loading /issues with no query params doesn't end up loading anything! Maybe instead of skipping the early fetch we can just take the project IDs in the query params instead of the page filters store?

@getsantry
Copy link
Contributor

getsantry bot commented Sep 20, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Sep 20, 2024
@scttcper
Copy link
Member Author

this should be fixed w/ issue stream views

@scttcper scttcper closed this Sep 20, 2024
@scttcper scttcper deleted the scttcper/avoid-issue-list-cache branch September 20, 2024 23:12
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Scope: Frontend Automatically applied to PRs that change frontend components Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Project selection doesn't immediately apply to errors
2 participants
@scttcper @malwilley