ADFA-3472: Guard BuildOutputFragment access when detached#1415
ADFA-3472: Guard BuildOutputFragment access when detached#1415fryanpan wants to merge 3 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 27 minutes and 24 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesDetached Fragment Crash Fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Sentry APPDEVFORALL-N4. Skip activityViewModels access when the fragment is detached; guard the caller with isAdded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reproduces the Sentry IllegalStateException ('not attached to an activity')
thrown when clearOutput()/getShareableContent() force the activityViewModels
lazy delegate (-> requireActivity()) on a detached fragment. RED on the
pre-fix baseline (4d9b100), GREEN with the isAdded/activity guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
64ac5df to
e6941b4
Compare
Jira Ticket: https://appdevforall.atlassian.net/browse/ADFA-3472
Sentry Issue: https://appdevforall-inc-9p.sentry.io/issues/APPDEVFORALL-N4
Question
Looks like this file is part of AndroidIDE -- do we make changes here or upstream?
Reproduction Details
BuildOutputFragmentholdsbuildOutputViewModelviaby activityViewModels(). WhenclearOutput()/getShareableContent()run on a detached fragment, forcing that lazy delegate callsrequireActivity(), which throwsIllegalStateException: … not attached to an activity.Stack Trace
User Steps
User steps leading up to crash, based on Sentry breadcrumbs:
BuildOutputFragmentgoes through save-state → stopped → view destroyed → detached, and an incoming build-output update then callsclearOutput()on the now-detached fragment → crash.Was able to reproduce in a unit test?
Yes.
BuildOutputFragmentDetachedTest(:app, Robolectric) constructs a never-attached fragment (isAdded == false) and calls each method. Baseline: both FAIL withIllegalStateExceptionatrequireActivityvia theactivityViewModelsdelegate; branch: both pass.What Was Fixed
if (!isAdded || activity == null) returnguard inclearOutput()/getShareableContent()(returns "" for the latter) before touching the activity-scoped view model.Testing
:app:testV8DebugUnitTest→ 2/2 green (red on baseline). Local CodeRabbit review: no findings. Reviewer note (local): theactivity == nullclause is redundant with!isAdded, andEditorBottomSheet'stakeIf { it.isAdded }is now belt-and-suspenders — harmless.viewLifecycleOwneris NOT the right alternative here (the precondition isisAdded, which it doesn't capture).Fixes APPDEVFORALL-N4