-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat: Add generic modal for requesting access to private gaming repository #96291
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
feat: Add generic modal for requesting access to private gaming repository #96291
Conversation
511f9b9 to
9a120e9
Compare
| }) | ||
| ); | ||
| setIsSubmitting(false); | ||
| closeModal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Feedback Formatting and Submission State Issues
The PrivateGamingSdkAccessModal has two issues:
- The feedback message sent via
captureFeedbackinconsistently formats gaming platform names: single platforms use raw values (e.g., 'playstation'), while multiple platforms use display labels (e.g., 'PlayStation'), resulting in inconsistent capitalization. - If the
captureFeedbackcall fails, theisSubmittingstate is not reset and the modal remains open, leaving the UI in a perpetual submitting state.
Co-authored-by: Bruno Garcia <[email protected]>
… into priscila/feat/add-private-gaming-sdk-access-modal
| }) | ||
| ); | ||
| setIsSubmitting(false); | ||
| closeModal(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Feedback Submission Timing and Formatting Issues
The captureFeedback function is asynchronous but not awaited, leading to premature isSubmitting state reset, early success message display, and modal closure before feedback is sent. Additionally, the feedback message inconsistently formats gaming platform names, using raw values for single selections and human-readable labels for multiple selections.
| openModal(deps => <Modal {...deps} {...options} />); | ||
| } | ||
|
|
||
| export async function openPrivateGamingSdkAccessModal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is unused, so I’d expect knip to complain here (which it doesn’t). I’ll look into that, but can we make the function used somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| const isFormValid = !!githubProfile.trim() && gamingPlatforms.length > 0; | ||
|
|
||
| const handleSubmit = useCallback(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please not wrap every function we create in useCallback 🙏 . There is absolutely no need here, because:
- it’s only passed to the click handler of a
Button, which doesn’t care about referential stability. - it is dependent on
onSubmit, which is a prop, which is always an inline function, so theuseCallbackhas to run every time anyways.
All it does in this instance is add more code and complexity and mental overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey thanks! it is not always super clear when we should use useCallback, and I noticed we've been applying it by default whenever we define a function in our codebase. This one for example was copied and pasted from here.
I was wondering...do we have a linter or any tooling that could help us identify when useCallback is actually necessary? That would be super helpful!
| sdkName, | ||
| }) | ||
| ); | ||
| setIsSubmitting(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m unsure how this should work. This handleSubmit function is a synchronous callback, so it first calls setIsSubmitting(true) followed by setIsSubmitting(false), so I don’t see where the true state would ever be rendered 🤔 .
This is part of the reason why merging something that has no usage is a bit counter productive imo because we can’t know how it works / what it does.
I’m guessing onSubmit is going to maybe trigger a mutation on the call-side? Would it be better to trigger that mutation from inside here, which would also give us mutation state like isPending and callbacks to add the success messages properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - it would be great to use isPending and show a success message only if everything went well. But we're using captureFeedback, which doesn't wait for the result. It just tries to send the feedback and returns immediately. You can read more about it here.
This modal will be removed in a future phase, but for now, we’re keeping it simple by reusing what we already have.
8588c04 to
b9c2a51
Compare
Note
The idea is to replace PlayStationSdkAccessModal after this is merged
closes https://linear.app/getsentry/issue/TET-921/collect-github-handle
Screen.Recording.2025-07-24.at.10.08.56.mov