fix(billing): add panel fallback + robust matching to precondition modal routing (FE-878)#12854
Conversation
…type The queue paywall arrives as exception type PAYMENT_REQUIRED; matching it by type (not just the message string) keeps classification stable if the backend wording changes.
… open Precondition routing dropped subscription/sign-in/credit errors from the panel and opened a modal, but the subscription modal no-ops when subscription mode is disabled (non-cloud or config off), leaving the user with nothing on screen. Add a pure canRoutePreconditionToModal gate (no auth/dialog coupling) shared by the dialog opener, the queue catch, and the execution store: suppress the panel only when a modal can actually take over, otherwise keep the error visible. Also normalize string-shaped /prompt response errors so the queue paywall routes regardless of payload shape.
dosubot
Bot
added
the
size:M
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🎨 Storybook:
|
🎭 Playwright: ✅ 1668 passed, 0 failed · 3 flaky📊 Browser Reports
|
dante01yoon
left a comment
dante01yoon
left a comment
There was a problem hiding this comment.
Reviewed at 8632d1a. The panel-fallback design — gate modal routing on canRoutePreconditionToModal, keep the error in the panel when the modal can't open — is sound and applied consistently across all three surfaces (dialog opener, queue catch, execution store). canRoutePreconditionToModal is correctly pure, and the PAYMENT_REQUIRED-by-type rule sits after the credit rules in runtimeErrorMatcher, so it won't misroute insufficient-credits.
One blocking regression in the queue catch — details inline.
issue:(1, blocking)resolvePromptResponsePrecondition(error.response.error)throws aTypeErroron a/prompterror body that has no top-levelerrorkey (e.g. the 403 middleware{"message":"..."}shape the same catch block handles a few lines below). The pre-refactor code guarded this withtypeof error.response.error === 'object'; the refactor dropped that guard.
Note: typecheck / unit / Playwright checks had not reported at review time, and no test covers the undefined-error queue path, so this regression is currently uncaught by CI.
…or field
The /prompt body is unvalidated JSON, so error.response.error can be undefined
(e.g. the 403 middleware { message } shape) despite the typed union. Guard the
object branch in resolvePromptResponsePrecondition so it returns undefined
instead of throwing, letting the queue catch fall through to its 403 handler.
Add unit tests for the undefined and no-error-field cases.
dante01yoon
added
the
needs-backport
|
I recommend you merge this PR once parent got merged |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## jaewon/fe-878-bug-subscription-required-error-message-feels-choppyabrupt #12854 +/- ##
============================================================================================================
- Coverage 62.14% 61.67% -0.47%
============================================================================================================
Files 1458 1459 +1
Lines 75108 75337 +229
Branches 21167 19641 -1526
============================================================================================================
- Hits 46675 46466 -209
- Misses 28089 28517 +428
- Partials 344 354 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 98 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@MaanilVerma can you check CI failing |
@dante01yoon Yes, I saw that! Those were random errors Now all CI's have passed |
…eels-choppyabrupt' into fix/fe-878-precondition-fallback-and-string-error
dante01yoon
deleted the branch
Comfy-Org:jaewon/fe-878-bug-subscription-required-error-message-feels-choppyabrupt
|
Closing as the changes were folded into the parent #12785 and merged to main, so this stacked PR is now redundant (#12840 closed by that merge too). Thanks @MaanilVerma! |
2 participants
Summary
Builds on top of #12785 precondition→modal routing: adds an error-panel fallback so a gated-off modal never leaves the user with a blank screen, plus more robust paywall matching.
Changes
canRoutePreconditionToModalgate (shared by the dialog opener, queue catch, and execution store) suppresses the error panel only when a modal can actually open; otherwise the error stays in the panel. Fixes the blank-screen case when the subscription modal no-ops (non-cloud or__CONFIG__.subscription_requiredoff).PAYMENT_REQUIREDby exception type, not just the message string, so classification survives backend wording changes./promptresponse errors (resolvePromptResponsePrecondition) so the queue paywall routes regardless of payload shape.executionStore → dialog → authcoupling.Review Focus
The fallback rule: matched precondition + modal opens → panel suppressed; matched + modal can't open → panel kept (never blank); unmatched → panel as before.
canRoutePreconditionToModalis pure (readsisCloud+__CONFIG__, no auth/dialog imports) and is the single source of truth used byuseAccountPreconditionDialog.open, the queue catch, andexecutionStore.subscriptionPaywallError.spec.tsupdated: in the OSS test build (non-cloud) the paywall correctly falls back to the panel.Fixes Subscription paywall shows raw "Subscription required to queue workflows" with no upgrade action #12840