Code Review Timeline #46132
Code Review Timeline #46132
Conversation
maureensturgeon
changed the base branch from
staging
to
maureen/LP-2301-code-review-component
maureensturgeon
force-pushed
the
maureen/LP-2312-code-review-timeline
branch
from
ca85aed
to
fcd4b42
Compare
| refresh(); | ||
| }, [refresh]); | ||
|
|
||
| const refresh = useCallback(async () => { |
There was a problem hiding this comment.
This is like a modern react/js tutorial for me 👏
| isVersionExpired: PropTypes.bool | ||
| }); | ||
|
|
||
| const commentShape = PropTypes.shape({ |
There was a problem hiding this comment.
Nit: maybe reviewCommentShape since commits also have comments?
| const [commitsData, setCommitsData] = useState([]); | ||
| const [hasOpenCodeReview, setHasOpenCodeReview] = useState(null); | ||
|
|
||
| const dataApi = useMemo( |
There was a problem hiding this comment.
This interaction between dataApi, refresh and the useEffect seems a bit complicated.
I think useMemo is intended as an optimization -- React is allowed to throw away the memoized value and recalculate if it wants to. Unfortunately, I think that would lead to a refresh here which seems undesirable. I think my suggestion here is to not have the data api be instanced. Just use it to organize some exports and pass all of the needed parameters to getCodeReviews and getCommits.
I'm not sure about this, but it might also be helpful to have the data api expose a single method (hook?) that gets both code reviews and commits and combines that data (the logic that's currently in CodeReviewTimeline). One way to think about this is that the fact this data is coming from two separate APIs is abstracted from all of the UX components. This might then simplify refresh enough that we don't need to define it separately and can call the data api directly from the useEffect.
Finally, just throwing it out there for the future, I wonder if useFetch would help further simplify some of the logic.
(Sorry, this ended up being a lot. Also happy to chat in person.)
There was a problem hiding this comment.
One of the benefits of having an instance is storing the csrf token, which could be stored on the component instead. I do see your point about having some of the combining logic extracted. I'll play around with it. Things actually change a bit in the next PR where I decide to store the open code review separately on the state so that I can more easily mutate it when changes are made: #46150
There was a problem hiding this comment.
Sounds good, up to you whether you want to continue to iterate on this PR or if you want to merge this and come back to it later.
There was a problem hiding this comment.
I'm looking at potential refactors on the next branch, so I'm going to merge this one and keep looking at it.
| useEffect(() => { | ||
| scrollToBottom(); | ||
| }, [reviewData, commitsData]); |
There was a problem hiding this comment.
Does this typically only happen once (after the data arrives)? (I could imagine this being disorienting if it happens more often.)
There was a problem hiding this comment.
Yeah the intent is that this just happens when the data loads. This will probably be easier to tweak when things are more wired up and I'm able to walk through the different flows
| mergedData.sort((a, b) => new Date(a.createdAt) - new Date(b.createdAt)); | ||
|
|
||
| return ( | ||
| <div style={styles.wrapper}> |
| commit: 'commit' | ||
| }; | ||
|
|
||
| const CodeReviewTimeline = props => { |
There was a problem hiding this comment.
If this is the "primary" component, it'd be great to have a header comment that gives a short description of how it and its children components work together.
…de-review-timeline
Renders the timeline dynamically from the review and commit data in a new CodeReviewTimeline component.
Links
Testing story
Tested locally and added unit tests
PR Checklist: