fix(surveys): properly clean up popover surveys on close #2595
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
adboio
force-pushed
the
11-18-fix_surveys_properly_clean_up_popover_surveys_on_close
branch
from
16f38bf to
6d91284
Compare
Contributor
Contributor
|
Size Change: -112 B (0%) Total Size: 5.06 MB
ℹ️ View Unchanged
|
adboio
requested review from
a team and
lucasheriques
and removed request for
a team
Contributor
What ChangedThe fix is in Before: if (survey.type === SurveyType.Popover) {
removeSurveyFromFocus(survey)
}After: if (isPopup) {
removeSurveyFromFocus(survey)
}This changes the condition from checking the survey's configured type to checking how the survey is actually being displayed. Why This Fix is NecessaryThe original logic had a conceptual flaw: it determined cleanup behavior based on
How This Fits the CodebaseThe fix aligns with the existing architecture where display behavior should be determined by how a survey is being shown rather than its configured type. The Potential IssuesLow Risk: The change is minimal and well-targeted. The Testing Gap: The PR doesn't include tests for this specific scenario. While the fix is straightforward, adding a test that verifies surveys can be re-opened after programmatic display and close would strengthen confidence. Backward Compatibility: This change maintains full backward compatibility - it only fixes the broken case without affecting existing working scenarios. Confidence Score: 4/5This is a well-reasoned fix that addresses a clear architectural inconsistency. The change is minimal, targeted, and aligns with existing patterns in the codebase. The only concern is the lack of accompanying tests for this specific workflow. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 file reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
Contributor
lucasheriques
left a comment
lucasheriques
left a comment
There was a problem hiding this comment.
can we add an e2e test to cover this behavior?
plus, shouldn't the usePopupVisibility be called with the isPopup? or is it already being called?
Contributor
Author
|
will add a test! yeah this change calls |
adboio
force-pushed
the
11-18-fix_surveys_properly_clean_up_popover_surveys_on_close
branch
from
6d91284 to
383d9a0
Compare
Contributor
Author
|
@lucasheriques wrapped an existing test to run for all survey types -- it was almost already testing what we want |
Contributor
Author
adboio
deleted the
11-18-fix_surveys_properly_clean_up_popover_surveys_on_close
branch
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.
Problem
when opening surveys via API
displaySurvey, they are not properly cleaned up on close, and then cannot be re-openedcloses #2586
Changes
instead of checking only survey type, check display type -- so all surveys displayed as popovers (regardless of survey type) are cleaned up as needed
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file