fix: ensure loading spinner stops when marketplace app installation fails#38524
fix: ensure loading spinner stops when marketplace app installation fails#38524dodaa08 wants to merge 45 commits into
Conversation
🦋 Changeset detectedLatest commit: 3398bac The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded centralized error handling to the AppStatus component's confirmAction method using a try-catch-finally pattern with handleAPIError. Expanded test suite from minimal coverage to comprehensive error-handling scenarios covering install, update, and purchase action failures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38524 +/- ##
===========================================
+ Coverage 70.70% 70.76% +0.06%
===========================================
Files 3192 3192
Lines 113064 113061 -3
Branches 20466 20496 +30
===========================================
+ Hits 79941 80008 +67
+ Misses 31080 31009 -71
- Partials 2043 2044 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
aleksandernsilva
left a comment
aleksandernsilva
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Could you also implement some unit tests covering this behavior?
Yeah sure, let me try |
dougfabris
added
valid
…l on error handling to gracefully cancel the operation
Hi, I've added unit tests to cover this behavior. I also made some changes to error handling in another file to gracefully handle errors and clear the loading state. Please review and let me know your thoughts. Also for the tests I added it for 2-3 places so I used a global wrapper which can be used by all the places for mook app setup. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx`:
- Around line 98-104: The error tests for install/update/purchase use a
try/catch pattern that can pass vacuously; add expect.assertions(1) at the start
of each of those tests to ensure the catch-block assertion runs — update the
three tests that call onSuccessCallback!('install'|'update'|'purchase') inside
act(...) to include expect.assertions(1) before invoking onSuccessCallback so
the expect(err).toBe(...) in the catch is enforced.
🧹 Nitpick comments (2)
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx (2)
56-73: Smoke test with no explicit assertions.This test only verifies the component doesn't crash on render and click. Consider adding at least one explicit assertion (e.g., button is visible) to make the test's intent clearer and prevent it from silently passing if the component changes.
86-90: TheonSuccessCallbackcapture relies onuseAppInstallationHandlerbeing called during render.The
onSuccessCallback!non-null assertion on lines 100, 134, and 169 will throw a confusing runtime error if the hook mock isn't invoked before the callback is used (e.g., due to a conditional render path change). This is acceptable in tests but worth noting —expect.assertions()(from the previous comment) would also help catch this scenario more gracefully.Also applies to: 120-124, 155-159
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsxapps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.tsxapps/meteor/client/views/marketplace/hooks/useAppInstallationHandler.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/marketplace/hooks/useAppInstallationHandler.tsxapps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
🧠 Learnings (13)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build (coverage)
🔇 Additional comments (4)
apps/meteor/client/views/marketplace/hooks/useAppInstallationHandler.tsx (2)
83-86: LGTM — dismissing on API error correctly resets UI state.The
onDismiss()call afterhandleAPIErrorensures the loading spinner stops and the modal closes when the external URL purchase flow fails.
117-120: Consistent error-handling pattern for the request flow.Same approach as the purchase path — looks good.
apps/meteor/client/views/marketplace/AppDetailsPage/tabs/AppStatus/AppStatus.spec.tsx (2)
1-14: LGTM — mock setup is clean and follows standard patterns.
15-54: Well-structured test scaffolding with reusable wrapper factory.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Hi, I've added them but this change feels a little big and I need some feedback on weather the tests seems fine, let me know once you review, thanks. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/client/views/marketplace/hooks/useAppInstallationHandler.tsx">
<violation number="1">
P2: Error handling no longer calls `onDismiss`, so failures from `buildExternalUrl` / `buildExternalAppRequest` leave loading state/modal open (callers rely on `onDismiss` to reset loading).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Thanks @aleksandernsilva for all the suggestions, Ran locally |
|
@aleksandernsilva I tried a few times but the tests are failing 
I think they were fine yesterday, but after a few merge they doesn't seem to pass. |
dodaa08
force-pushed
the
fix/Marketplace-Loading
branch
from
286bdce to
e13ff25
Compare
|
Hi @aleksandernsilva , the checks are passing now, let me know if anything else is needed here, once you review. |
Hi @aleksandernsilva , I waiting for your response. |
3 participants
Proposed changes (including videos or screenshots)
Fixes a UI issue where the Marketplace Install button spinner never stops when an app installation fails.
The loading state is now properly reset on failure so the spinner stops and the UI does not remain stuck.
Screencast.From.2026-02-06.12-41-30.mp4
Issue(s)
Fixes #38523
Also Fixes #35518
A similar issue where spinner doesn't stop.
After:
Screencast.From.2026-02-06.13-12-12.mp4
Steps to test or reproduce
Run server locally
Further comments
The fix uses a try-finally pattern which is the standard approach for ensuring cleanup code runs regardless of success/failure. We intentionally don't use try-catch because we want errors to propagate to the existing error handling infrastructure (React Query's
onError
→
handleAPIError()
→ toast notification).
This is a minimal, focused fix that only addresses the loading state management without changing any business logic or error handling behavior.
COMM-130
Summary by CodeRabbit
Bug Fixes
Tests