fix(onboarding): surface continue-to-chat failures#2394
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe onboarding context completion flow shifts error-handling responsibility from ContextPage to ContextGatheringStep. ContextPage now returns the completeAndExit promise directly without local error catching or Sentry forwarding. ContextGatheringStep wraps the completion with isCompleting loading state, error logging, Sentry capture, and error UI feedback. Tests are updated to verify the new error propagation path and loading behavior. ChangesOnboarding context completion error handling refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
graycyrus
left a comment
There was a problem hiding this comment.
Nice fix. Moving the error handling from ContextPage down into ContextGatheringStep is the right call — that's where the UI state lives, so that's where the loading spinner and error card belong.
The Promise.resolve(onNext()) wrapper handles the void | Promise<void> return type cleanly, the double-click guard via isCompleting is solid, and the Sentry capture + tags are preserved from the old page-level catch.
Tests cover the failure path well. No issues found — this is ready for approval.
|
Hey @aqilaziz — thanks for tackling #2081, the loading state on the button is a nice touch and the Sentry capture cleanup is solid work. After reviewing this more closely though, we're going to pass on showing the error card for this flow. Here's the thinking: The context-gathering pipeline is an optional background process — the user's chat is fully ready regardless of whether it succeeds or fails. Showing an error card ("Your chat is ready...") for something the user didn't explicitly ask for and doesn't need to care about adds unnecessary friction right at the moment we want onboarding to feel smooth and frictionless. The previous behavior (Sentry capture + silent continuation) was actually closer to the right UX here — if the completion RPCs fail, the best thing to do is silently move on to chat and let the team track it via Sentry. The user shouldn't be interrupted with a "something went wrong" card for a process they don't even know is running. If you'd like to iterate on this, the part worth keeping would be:
But as-is, the error card display is a UX regression for onboarding flow, so closing this one out. Appreciate the effort and the clean test coverage — hope this context helps for future contributions! |
Summary
Refs #2081
Checks
Summary by CodeRabbit
New Features
Bug Fixes
Tests