tests(issue-views): Add tests and a couple of refactors #78539
Conversation
github-actions
bot
added
the
Scope: Frontend
| return; | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [navigate, organization.slug, query, sort, viewId, tabListState]); | ||
| }, []); |
There was a problem hiding this comment.
I have refactored this useEffect to only run on first render. Previously, it was responsible for two things:
- Making sure that the query params align with the tab that's selected
- Making sure that the unsaved changes are being registered properly every time a new query is loaded
This led to a lot of double work being done: the tabs component already handles updating the url query properly when the user switches between views, so the navigates in this useEffect were unnecessary and redundant. Now, this useEffect is only responsible for making sure the query params align with the tab that's selected when the component initially loads, thus allowing the tabs component to handle navigation thereon after.
The new useEffect below handles updating the unsaved changes when the search query changes.
There was a problem hiding this comment.
Removing dependencies from the array is often a source of problems, so you want to be careful here. I think this may have introduced a bug where if you click to a different view tab, then click "Issues" in the sidebar you redirect to /issues, but your tab does not change (and has the temp tab visual). You want to be careful of any values that might change after first render
There was a problem hiding this comment.
@malwilley good catch, I can reproduce this and it's 100% caused by the removal of these dependencies.
I'm curious if you think removing these dependencies is the right option at all. My thought process was that it was a way too overloaded - it was handling both setting the query params on mount, and on most tab actions like switching. I figured this led to some conflicting behavior with the tabListState that react-aria' provides, like double navigating to the same tab every time we switched (though that's still occurring in this branch). I can't tell if I'm being overly nervous with how much the scope of this useEffect has grown. 🤔
There was a problem hiding this comment.
Ideally we'd have the useEffect run on every dep change (and remove that eslint ignore line). To decrease the scope, you might be able to separate it into multiple different useEffect's. That is, if that's possible at all.
There was a problem hiding this comment.
I updated the useEffect to include all dependencies and ensured it does not infinitely loop, and I removed the second useEffect. If it does infinitely render, would this be caught in tests somehow?
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #78539 +/- ##
==========================================
+ Coverage 78.11% 78.19% +0.08%
==========================================
Files 7082 7093 +11
Lines 312248 312442 +194
Branches 51010 51018 +8
==========================================
+ Hits 243910 244317 +407
+ Misses 61981 61782 -199
+ Partials 6357 6343 -14 |
MichaelSun48
force-pushed
the
msun/issueViews/updateTests1
branch
from
ca32611
to
7855806
Compare
MichaelSun48
force-pushed
the
msun/issueViews/updateTests1
branch
from
7855806
to
8fd3f08
Compare
MichaelSun48
force-pushed
the
msun/issueViews/updateTests1
branch
from
8fd3f08
to
3579cf3
Compare
Bundle ReportChanges will increase total bundle size by 114.99kB (0.37%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
| return; | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [navigate, organization.slug, query, sort, viewId, tabListState]); | ||
| }, []); |
There was a problem hiding this comment.
Removing dependencies from the array is often a source of problems, so you want to be careful here. I think this may have introduced a bug where if you click to a different view tab, then click "Issues" in the sidebar you redirect to /issues, but your tab does not change (and has the temp tab visual). You want to be careful of any values that might change after first render
|
PR reverted: dfcdadb |
…)" This reverts commit ec7c343. Co-authored-by: MichaelSun48 <55160142+MichaelSun48@users.noreply.github.com>
Same exact PR as #78539 but rebased on master. Reverted that one because of potential CI failures. After testing locally, it does not appear to be an issue.
This PR adds the first round of tests for issue views, and it makes a couple of important refactors to the custom views component. Some of these are bug fixes that were uncovered by testing, others are other unrelated refactors. I will do my best to document these changes via comments.
I have also left some of the test sections blank with the intention of writing them up later in the interest of keeping this PR of reasonable length. There are significantly more tests to be written, and I think there are only benefits to writing them in stages.