Added handling for cases where the user in a PR review is missing#998
Open
avasconcelos114 wants to merge 1 commit intomasterfrom
Open
Added handling for cases where the user in a PR review is missing#998avasconcelos114 wants to merge 1 commit intomasterfrom
avasconcelos114 wants to merge 1 commit intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes introduce optional user fields in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
There is an edge case where if an account that previously reviewed the PR is deleted, the list of PRs in the sidebar will fail to load as it attempts to read the user attribute for the review.
This PR combats this by both filtering the PR reviews that don't have an user attached to them, as well as adding some defensive coding on the webapp to also prevent this from affecting the rendering of the sidebar contents
QA Notes
Reproducing this naturally is somewhat difficult given that you need to have given a review to a PR with a temporary account that is then deleted. The easiest way to confirm that the fix is working would be to use a browser extension to mock the response of
/plugins/github/api/v1/prsdetailsand remove theuserobject from one of the reviews.Once that is done just connect your account and open the "Your Open Pull Requests" sidebar from the LHS menu buttons
Change Impact: 🟡 Medium
Reasoning: These changes span both backend and frontend to handle the edge case of missing review authors, and include a type definition change to optionality. While the changes are isolated to review handling and are purely defensive, the scope across layers and the modification to the data model warrant medium-level scrutiny.
Regression Risk: Low. The changes add null-checking guards before accessing potentially undefined properties and filter invalid reviews server-side before transmission. No existing behavioral logic is modified; these are additive defensive measures that prevent crashes in an edge case (deleted/missing user accounts in PR reviews) without altering normal operation.
QA Recommendation: Light manual QA is recommended. Primary verification: confirm the "Your Open Pull Requests" sidebar loads successfully when a PR contains a review from a deleted/missing user account. The PR provides browser dev tools instructions to mock the edge case. Risk of skipping QA is medium—while changes are defensive and low-regression, the sidebar is user-facing and the edge case being fixed is real. A quick smoke test of sidebar loading with typical PRs is sufficient for low-risk release.