fix(quick-start): Fix animation reordering issue #88075
Merged
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
github-actions
bot
added
the
Scope: Frontend
❌ 20 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
priscilawebdev
commented
Mar 27, 2025
Comment on lines
-14
to
-38
| describe('updateOnboardingTask', function () { | ||
| it('Adds the task to the organization when task does not exists', async function () { | ||
| const detailedOrg = OrganizationFixture(); | ||
|
|
||
| // User is not passed into the update request | ||
| const testTask = { | ||
| task: OnboardingTaskKey.FIRST_PROJECT, | ||
| status: 'complete', | ||
| } as const; | ||
|
|
||
| const mockUpdate = MockApiClient.addMockResponse({ | ||
| url: `/organizations/${detailedOrg.slug}/onboarding-tasks/`, | ||
| method: 'POST', | ||
| body: testTask, | ||
| }); | ||
|
|
||
| updateOnboardingTask(api, detailedOrg, testTask); | ||
| await tick(); | ||
|
|
||
| expect(mockUpdate).toHaveBeenCalled(); | ||
|
|
||
| expect(OrganizationStore.onUpdate).toHaveBeenCalledWith({ | ||
| onboardingTasks: [{...testTask, user}], | ||
| }); | ||
| }); |
Member
Author
There was a problem hiding this comment.
I don't see why we would want to do that, and I believe it is related to the sandbox. However, since the sandbox now uses its own function, we can remove this extra code. Can you please confirm this, @obostjancic ?
Comment on lines
+15
to
24
| describe('useUpdateOnboardingTasks', function () { | ||
| it('Updates existing onboarding tasks', async function () { | ||
| const organization = OrganizationFixture({ | ||
| onboardingTasks: [ | ||
| { | ||
| task: OnboardingTaskKey.FIRST_EVENT, | ||
| status: 'pending', | ||
| }, | ||
| ], | ||
| }); |
Member
Author
There was a problem hiding this comment.
updated this test, so it works with a hook
| { | ||
| task: OnboardingTaskKey.FIRST_PROJECT, | ||
| status: 'complete', | ||
| user: UserFixture(), |
Member
Author
There was a problem hiding this comment.
we no longer display a user face in the quick start. This can be removed
|
|
||
| jest.unmock('sentry/utils/recreateRoute'); | ||
| jest.mock('sentry/actionCreators/indicator'); | ||
| jest.mock('sentry/actionCreators/onboardingTasks'); |
Member
Author
There was a problem hiding this comment.
This is probably here by mistake.
| completionSeen?: string | boolean; | ||
| data?: {[key: string]: string}; | ||
| dateCompleted?: string; | ||
| user?: AvatarUser | null; |
Member
Author
There was a problem hiding this comment.
this is not longer used in the quick start
| ); | ||
| expect(metric.startSpan).toHaveBeenCalledWith({name: 'saveAlertRule'}); | ||
|
|
||
| expect(mockUpdateOnboardingTasks).toHaveBeenCalledTimes(1); |
Member
Author
There was a problem hiding this comment.
this make sure that when an issue alert is created and the demo mode is not active, we mark the alert rule task as complete
Comment on lines
+180
to
+182
| if (isDemoModeActive()) { | ||
| return; | ||
| } |
Member
Author
There was a problem hiding this comment.
we missed this here...so users on sandbox is marking this rule as complete atm
| expect(metric.startSpan).toHaveBeenCalledWith({name: 'saveAlertRule'}); | ||
| }); | ||
|
|
||
| it('updates the alert onboarding task', async function () { |
Member
Author
There was a problem hiding this comment.
we are covering this in the file static/app/views/alerts/create.spec.tsx
| /** | ||
| * A callback triggered when the rule is saved successfully | ||
| */ | ||
| onSaveSuccess?: () => void; |
Member
Author
There was a problem hiding this comment.
has to add this prop because the component is still class based :(
…' of github.com:getsentry/sentry into priscila/ref/quick-start/fix-animation-reordering-issue
priscilawebdev
force-pushed
the
priscila/ref/quick-start/fix-animation-reordering-issue
branch
from
397ce31 to
688264b
Compare
obostjancic
approved these changes
Mar 27, 2025
Member
obostjancic
left a comment
obostjancic
left a comment
There was a problem hiding this comment.
LGTM, lets just check it out in the demo mode before we merge!
Comment on lines
-14
to
-38
| describe('updateOnboardingTask', function () { | ||
| it('Adds the task to the organization when task does not exists', async function () { | ||
| const detailedOrg = OrganizationFixture(); | ||
|
|
||
| // User is not passed into the update request | ||
| const testTask = { | ||
| task: OnboardingTaskKey.FIRST_PROJECT, | ||
| status: 'complete', | ||
| } as const; | ||
|
|
||
| const mockUpdate = MockApiClient.addMockResponse({ | ||
| url: `/organizations/${detailedOrg.slug}/onboarding-tasks/`, | ||
| method: 'POST', | ||
| body: testTask, | ||
| }); | ||
|
|
||
| updateOnboardingTask(api, detailedOrg, testTask); | ||
| await tick(); | ||
|
|
||
| expect(mockUpdate).toHaveBeenCalled(); | ||
|
|
||
| expect(OrganizationStore.onUpdate).toHaveBeenCalledWith({ | ||
| onboardingTasks: [{...testTask, user}], | ||
| }); | ||
| }); |
Comment on lines
310
to
315
| if (isDemoModeActive()) { | ||
| updateDemoWalkthroughTask({ | ||
| task: task.task, | ||
| status: 'skipped', | ||
| completionSeen: true, | ||
| }); |
Member
There was a problem hiding this comment.
I'd do an early return here for demo mode since I don't think we'll add a skip task feature there anytime soon.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the quick-start animation reordering issue by ensuring all promises resolve before updating the state and updates the onboarding tasks flow by switching from the old updateOnboardingTask function to a React Query mutation hook. Additionally, the PR removes outdated code and test mocks, modernizing several components and related tests concerning onboarding tasks and demo mode.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| static/app/views/alerts/rules/issue/index.tsx | Removed updateOnboardingTask import and added the onSaveSuccess callback in the IssueRuleEditor. |
| static/app/views/alerts/rules/issue/index.spec.tsx | Updated tests by removing references and mocks for updateOnboardingTask. |
| static/app/views/alerts/create.tsx | Integrated useUpdateOnboardingTasks and updated the onSaveSuccess handler with demo mode check. |
| static/app/views/alerts/create.spec.tsx | Updated test mocks to use the new update hook and verified onboarding task mutation. |
| static/app/types/onboarding.tsx | Removed the AvatarUser type from OnboardingTaskStatus. |
| static/app/components/sidebar/onboardingStatus.spec.tsx | Removed the static user property from onboarding task objects in tests. |
| static/app/components/onboardingWizard/content.tsx | Refactored onboarding wizard workflows: replaced updateOnboardingTask with useUpdateOnboardingTasks and updated demo mode handling via updateDemoWalkthroughTask. |
| static/app/components/nav/primary/onboarding.spec.tsx | Removed obsolete user field when verifying onboarding status in tests. |
| static/app/components/externalIssues/ticketRuleModal.spec.tsx | Removed the mocking for updateOnboardingTask. |
| static/app/actionCreators/onboardingTasks.tsx | Replaced updateOnboardingTask with the new custom useUpdateOnboardingTasks hook leveraging React Query mutation. |
| static/app/actionCreators/onboardingTasks.spec.tsx | Updated tests to validate the new hook’s behavior and removed legacy tests. |
| static/app/actionCreators/guides.tsx | Updated the demo walkthrough task completion to use updateDemoWalkthroughTask. |
Comments suppressed due to low confidence (1)
static/app/components/onboardingWizard/content.tsx:437
- In demo mode, updateDemoWalkthroughTask is called later with the task object but without an explicit 'status' field (unlike other usages where 'status' is provided). Confirm if this discrepancy is intentional or if a status should be added for consistency.
.map(task => ({...task, completionSeen: true}))
Member
Author
I checked whether the changes break when demo mode is active, and everything seems fine. However, I did come across a few unrelated bugs, which I’ve shared internally. |
priscilawebdev
deleted the
priscila/ref/quick-start/fix-animation-reordering-issue
branch
andrewshie-sentry
pushed a commit
that referenced
this pull request
Mar 31, 2025
- Fixes the weird reordering animation in Quick Start by using Promise.all to ensure all promises are resolved before updating the Organization Store. - Updates the action function to use the React Query mutation hook. - When the demo is active, the updateDemoWalkthroughTask function can now be used directly, eliminating the need for the updateOnboardingTask function - so, it got removed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR does the following:
And yes, it does a bit more than just that:
Before
Screen.Recording.2025-03-26.at.14.14.56.mov
After
Screen.Recording.2025-03-27.at.14.18.14.mov