ADFA-4128 (11/11): app + bench — wiring Quick Build into the IDE and the benchmark harness - #1723
ADFA-4128 (11/11): app + bench — wiring Quick Build into the IDE and the benchmark harness#1723fryanpan wants to merge 51 commits into
Conversation
5b48f90 to
c69d8ef
Compare
c69d8ef to
5a3d5eb
Compare
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
5a3d5eb to
ac3ab4e
Compare
ac3ab4e to
a45a359
Compare
|
@coderabbitai review |
|
a45a359 to
7c72105
Compare
3e8d9d2 to
6254159
Compare
jatezzz
left a comment
There was a problem hiding this comment.
@fryanpan — review of the Quick Build app wiring. Seven findings; two are worth fixing before merge and are left as inline comments:
ProjectHandlerActivity.kt:549— thereArmInstallsafety net does not cover the clobber dialog, so a rotation while it is up loses the install silently.QuickBuildStatusBar.kt:151— a landed build is re-announced on every re-subscribe and, withonlyIfOwned = false, stomps project-init / plugin-install status.
The remaining five are low: the session teardown on a transient APK parse failure, the null-activity path that builds stale content, the ellipsized actionable status copy, and three unused imports that should fail spotlessCheck.
One more (low), which could not be left inline because the file is not in this diff:
app/src/main/java/com/itsaky/androidide/actions/BaseBuildAction.kt:43 — a missed sibling of this PR's raw-vs-user-visible split.
This PR moved every UI decider over to isUserVisibleBuildInProgress — AbstractCancellableRunAction, ProjectHandlerActivity.onResume, the progress bar, BuildVariantsFragment — but BaseBuildAction.prepare still reads the raw flag:
enabled = buildService?.let { !it.isBuildInProgress } == trueWith the experiments flag on, Quick Build's eager prebuild now runs on every project open, so RunTasksAction (and any other direct BaseBuildAction) is silently greyed out for its whole duration with no explanation — unlike QuickBuildAction, which relabels, or QuickRunAction, which flashes msg_build_slot_busy. Note that AbstractCancellableRunAction.prepare unconditionally re-sets enabled = true, which is why its new slot-busy flash is reachable and these are not.
Checked and cleared: all new string/drawable/menu resources resolve on the head branch; ProjectManagerImpl.generateSources() returns Boolean, so GenerateSourcesDeferral's refusal-retry contract holds and a throw is treated as a refusal rather than cancelling the scope; InternalBuildBracket releases strictly after isBuildInProgress clears, so there is no window where isUserVisibleBuildInProgress reads true for an internal build; QuickBuildOutputNarrator confines its mutable state to one Dispatchers.Main.immediate scope and compares sink identity correctly; QuickBuildReloadTimingMetric.asBundle() is 25 params worst case, within the Firebase cap; ThermalSafeStrategy copies GradleDaemonConfig, so the new non-defaulted daemonIdleTimeoutMs is safe and 2h fits in Int; and InstallationResultHandler.onResult returning null is handled as "do nothing" by its only caller.
| import com.itsaky.androidide.models.FileExtension | ||
| import com.itsaky.androidide.models.OpenedFile | ||
| import com.itsaky.androidide.models.OpenedFilesCache | ||
| import com.itsaky.androidide.models.Position |
There was a problem hiding this comment.
@fryanpan low — unused import.
Position appears only on this import line; the identifier is never used in the file. Same standard:no-unused-imports / Spotless-ratchet issue as the two EventBus imports added to ProjectHandlerActivity.kt.
There was a problem hiding this comment.
NITPICK: still open at head - Position occurs exactly once in the file, on this import line.
Worth knowing why the tooling misses it: the file contains 47 occurrences of the substring Position (tabPosition, fromPosition, ensurePositionVisible, ...), and ktlint's unused-import detection is substring-based, so the identifier reads as used. spotlessApply will not remove this one for you.
There was a problem hiding this comment.
Not deleted, and the reason is worth knowing. It was unused at the head you reviewed. The stack has since been rebased onto stage, and stage's deep-link work (#1651) added a Position(line, column) call in this same file — so the import is live again and deleting it breaks the build. Leaving it.
There was a problem hiding this comment.
Re-checked at the current tip: Position(line, column) is still called in this file from the deep-link path, so the import stays. Nothing further from my side.
6254159 to
7e90fff
Compare
7e90fff to
1f82366
Compare
The review thread asked for the failed-start bar line to be ownership-gated, so a start failure in one project cannot stomp the next project's line. The gate is the wrong lever - the ownership flag is an activity field, so gating would also drop the line after an activity recreation, where it must come back. What actually separates the two cases is the flag's lifetime, and the reducer already ends the failed-start story on a teardown: from Idle in the SessionRestartRequested arm, and from any other state in the top-level teardown guard. Closing a project sends exactly that event. Nothing was missing in production code, only the test that says so. Adds two tests, one per arm. Both were checked against a mutant: removing the Idle arm fails the first with "expected: Clear / but was: Show(...)", and having the top-level guard keep its state instead of resting at Idle fails the second. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017o3nPrBbGi2XYkMUGavG2A
… a fixed 100dp At font scale 2.0 on the A56 the two longest status lines wrapped to two lines and clipped at both the top edge of the collapsed sheet and the swipe hint below them, and the hint truncated with no ellipsis in every state. Swiping up does not help: onSlide scales the header to zero, so a line that does not fit is lost. The collapsed height is now the larger of the dimen and what the status block measures, so ordinary text keeps the familiar height and larger text gets the room it needs. The block is measured against an unbounded height on its own, because the header's height is set explicitly for the slide and so cannot wrap. The re-measure runs after a status change and only applies while the sheet is collapsed; mid-slide the height belongs to onSlide, which reads the same value. The swipe hint gets ellipsize=end with maxLines=1. It is the one disposable string here - it names a gesture rather than a remedy. No JVM test is possible for this: it is view measurement, and the app module has no Robolectric surface for the bottom sheet. The check that means anything is the manual-QA font-scale block, at 1.0 and 2.0 on a device, which this pass did not run. The doc is updated to say the fix itself is unverified there. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017o3nPrBbGi2XYkMUGavG2A
Two defects hid the hint. 978ff9b measured the build-status block against an UNSPECIFIED height spec, which ConstraintLayout does not support: it answered 146 px for a block whose own children reached 262 px, and that stale figure became the block's laid-out height, clipping the hint away. Measure against a bounded AT_MOST spec instead, and ask for a real layout pass afterwards, since the manual measure ran outside one. Second, and older than that commit: the sheet carries the status bar's height as top padding for its expanded state, so a 100dp header did not fit in a 100dp peek and its bottom 108 px fell below the window. The peek now covers that chrome as well as the header. Measured on the A56 at font scale 1.0, 2.0 and 3.0 in the ready, live-reloaded and BUILD FAILED states: the hint renders at 36, 77 and 115 px, and no content bound passes y=2205, where the navigation bar starts. At 3.0 the status block measures above the dimen floor, so the header and peek grow with it - the growth path 978ff9b intended, which the bad measure had made unreachable. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017o3nPrBbGi2XYkMUGavG2A
Drops the peek-height half of aae4bc5 by decision: the sheet's collapsed peek is the header height alone again, so the sheet sits where it did before the hint fix. The measure fix (AT_MOST instead of UNSPECIFIED) stays, so the status block is laid out at its real height; the hint can still clip where the sheet's status-bar padding pushes the header bottom below the window. The QA doc says so and asks the tester to record hint visibility. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017o3nPrBbGi2XYkMUGavG2A
refreshCollapsedHeight called header.requestLayout() before its unchanged-height early-out, and setStatus posts it after every write - including every Gradle progress line, for every user, with the Experiments flag off. That was an off-pass measure plus a forced layout per task event during a standard build. The request now sits below the early-out, where it runs only when the measured height differs; the text change itself is already laid out by setText. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
resourceXmlSaved was modified && isXml && isAndroidResource(), and ProjectManagerImpl.isAndroidResource matches only a module's resource directories. AndroidManifest.xml sits outside all of them, so a manifest-only save no longer refreshed the generated Manifest class or the merged manifest - for every user, flag off - where the old xmlSaved gate did. The flag fold now sets it for AndroidManifest.xml by name, without paying for the resource lookup. Both call-site comments and the SaveResult doc drop the "known trade" wording, and manual-qa.md gets T23 for the manifest walk. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
quick_build_switch_unknown_app_message said "Code On The Go" while quick_build_reload_crashed four lines below, app_name and every other user-facing string in this file say "Code on the Go" (44 occurrences; the capitalised form survives only in three legacy strings). Both feed Crowdin, so the mismatch would have shipped into every locale. The new string follows the file, not the docs' spelling. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
…ncluded ownsQuickBuildStatus was cleared only in setStatus, but the debugger writes its "Debugger starting" / "started" / "starting failed" lines through doSetStatus directly (BaseEditorActivity). With Quick Build still owning the bar, a passive refresh then overwrote the debugger's line and a session end blanked it. The clear moves into a doSetStatus override, so ownership tracks whoever writes the bar rather than whoever writes it through one overload. setStatus is now a plain forward. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
…ds the slot fireAutostartStandardBuild stamped standardBuildStarted and then called runQuickBuild, which returns without publishing a state when a build is already in progress. The stamp then stayed armed until the user's next build reached a terminal state, and standardBuildEnded read that build as the autostarted one: install suppressed, no message. The stamp now runs in runQuickBuild's beforeBuild, which only executes after the slot was claimed. Debug source set and bench flag only; no release path changes. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
…kspace A bench open for project B while project A was initialized set ProjectManagerImpl.projectPath = B and started the single-top editor; its onNewIntent (run before EditorHandlerActivity's switch logic) claimed the latch against the already-moved projectPath and tapped Quick Build with A's module model still in the workspace. Two guards: the editor claims only when the intent's project is the one its workspace was synced for, and the bench trampoline refuses a different-project open while one is initialized - the close-and-reopen path needs a dialog an unattended run cannot answer, and the harness force-stops between projects anyway. Debug source set and bench flag only. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
… read it everywhere else observeStates warmed the Koin graph on Dispatchers.IO, but the main-thread callers (the toolbar action, the bench autostart, the stagger) resolved the same singleton themselves through GlobalContext, so whichever ran first paid the graph build on the main thread and the warm-up was ordering-dependent. QuickBuildGraphWarmUp owns the one resolve: warmUp() builds off-main, sessionManagerOrNull is a plain read that never resolves, and the two callers that must not miss the manager await() it. Review thread: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
Every provision, prebuild and rebaseline re-extracted the 62 MB daemon zip after deleting daemon/. A rebaseline runs while the compile daemon is alive and loading jars from that directory lazily, so the wipe could turn an unopened jar into a NoClassDefFoundError inside the daemon. The extraction is now skipped when a stamp keyed on the installed APK's versionCode and lastUpdateTime matches and the daemon jar is present; the stamp is written last so a crash mid-extract re-stages. An APK update force-stops the app and its children, so the one path that does wipe never runs under a daemon. Review thread: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
The sink's branches for a completion with no matching start, a success without a start, the outcome and route metric names, an invalidation reason and a failed proxy rebuild had no test. Each case now pins the emitted field values. Review thread: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
…s in the app GenerationTracker is now opened through its suspend GenerationTracker.open, so the provisioner's baseline allocator becomes a suspend lambda; its one call site already runs inside the suspend proxy app build. A build that landed with warnings now lists them in the Build Output under the "reloaded to generation" line, indented like a failed build's errors, from the diagnostics the orchestration branch carries on QuickBuildStatus.UpToDate. A warning the Gradle build would print no longer disappears because the quick build succeeded. Review threads: #1719 (comment) #1719 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
…e hint survives 2x fonts The sheet carries the status bar's height as top padding, but its collapsed peek is the header height alone (2748c9a), so the bottom paddingTop + divider px of the header (48 on the A06, 108 on the A56) hang below the window. The status block filled the whole header and its spread chain put the hint last: at font scale 1.0 the chain's slack matched the hidden strip and the hint ended at the window edge; at 2.0 the taller text shrank the gaps and the hint's lower 18 px went under the navigation bar (device pass 0904, row C23). The header now carries that strip as bottom padding, so the block is laid out in the part that is on screen, and the collapsed height adds the strip back only when the block measures more than the floor leaves visible. The block is included with match_parent so it fills the header's content box, and its minHeight goes: the floor lives in code. The padding is re-applied on the sheet's layout changes, since the status-bar inset can land after setOffsetAnchor first padded the header (and that call no longer accumulates paddingBottom on a second anchor). Measured on the A06 (300dpi, font scale 1.0 and 2.0, idle and through a Gradle sync): the header stays [1370,1510]; the hint renders in full at [1457,1482] and [1448,1499], above the navigation bar at 1510. The sheet's on-screen height is unchanged. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
The QuickBuildArtifactStager class doc said a rebaseline provisions while the compile daemon is running and that the stamp keeps that safe. The rebaseline shuts the daemon down before its Gradle build, and no path stages under a live daemon, so the doc now says what the stamp does: skip a 62 MB re-extraction unless the APK changed, and keep the one wipe path behind an APK update that force-stopped the app. The AutostartBuild.NONE arm loses its explicit Unit. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2
…and-back The hand-back marks a live session's baseline untrusted after every finished Gradle build, and the resource-save generateSources build the deferral parks is one of them - so the session paid a full recompile for a build that regenerates nothing it compiles against (the daemon's classpath is the payload jars the proxy app build diverted at provisioning, not the intermediates R.jar). The deferral now remembers a dispatched build and the hand-back claims that build's completion once; the next build to finish after a dispatch is the deferral's, since the tooling server runs one build at a time and generateSources refuses while another is in progress. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
…child on show The measured status height went onto header_container, a ViewFlipper, so a three-line status line at 2x font scale inflated the symbol-input and install-progress rows too. collapsedHeight now counts the status block only while the flipper shows it, and showChild re-applies the height so the extra rows leave with the status and come back with it. onSoftInputChanged routes through showChild for the same reason. Not device-verified: a JVM test covers the height rule; the keyboard-up and install-progress flips still need a device at font scale 1.0 and 2.0. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
…tall resolves NeededForUnknownAppId never equals Needed(id), so a Run whose applicationId had not resolved at tap time asked at the tap and again at install once the APK named the package. The tap-time answer already consented to replacing whatever sits under this app's id; the install resolving that id is the same answer with the name filled in, so it is no longer asked twice. The other direction (a named tap, an unresolved install) still asks. Review: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
Both tap-time clobber gates ran the callback-shaped dialog from inside a suspend function on lifecycleScope, so their cleanup ran before the user answered; they now await the dialog like the install path does, which also takes the dialog down when the activity goes. The callback form had no callers left and is gone. The accessor KDoc names onPause and onResume, where restartSession, the narrator reset and onHostForegrounded actually live. Answers: #1723 (comment) #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
onFileSaved moved out of the per-editor write into the three save entry points, so a save-all posts one event instead of N identical ones. The dropdown KDoc says what the Help item does without a documentation row: it opens the no-tooltip fallback. Answers: #1723 (comment) #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
beforeBuild skipped the save-all for a destroyed activity and let the build go ahead on stale disk content; it now throws, which BuildViewModel's catch turns into an Error state. Answers: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
The analytics call ran before the BUILDING cancel branch, so a stop was tracked as a use. Answers: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
Three dots on a three-character budget, with the test asserting the same. Answers: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
RuntimeLog.d(String, Throwable) instead of string concatenation, matching the warn two lines below. Answers: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
The Gradle provisioner fills the proxyAppUid the rebuild outcome now carries, from the install verdict. Answers: #1720 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
T24 in Block B covers the refusal window: the msg_build_slot_busy flash and no second build. Answers: #1723 (comment) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01Y6rFuAJin4QFFwwzUTFZ1K
runTasks (stage #1803) claims the build slot but never resets clobberAnswerAtTap, so a Run whose clobber check was already answered, followed by a Run Tasks install of the same app, could skip the confirmation that install owes. Reset it when Run Tasks claims the slot; Run Tasks never asks, so the install must. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_0153YfwDnqn7ktHcVgXSNNe8
itsaky-adfa
left a comment
There was a problem hiding this comment.
Round 5, high effort, verified against head e476d20 - the tip of the stack, so every finding
here is at the tip by definition.
REQUEST_CHANGES, on two IMPORTANT findings, both about behaviour outside the Quick Build flag:
ProjectManagerImpl.onFileCreatedstill callsgenerateSources()directly rather than
through the new deferral, so adding a resource file from the file tree runs an undeferred
Gradle build that nothing claims. The hand-back then marks a live session's baseline
untrusted and the next save recompiles everything. Its delete and rename siblings are
already inert, becauseisAndroidResourcechecks existence - this is the one entry point
that needs routing.- The collapsed bottom-sheet peek now tracks the status line's row count, so at a 2x font
scale it changes repeatedly through every standard Gradle build. Bound it or debounce it
while a build is narrating.
Counts: 0 CRITICAL, 2 IMPORTANT, 9 MINOR, 2 NITPICK. Thirteen findings, none dropped; eleven
are inline here and two are replies on their existing threads.
CLAUDE.md's Jira section is what governs the verdict: a review with no outstanding critical,
high or medium findings is what moves the ticket to QA, so ADFA-4128 should stay in Code review
until the two above are closed. The nine MINOR and two NITPICK findings do not block.
Prior rounds first, because that is the strong part of this one. 34 of the 41 threads carry a
finding from a human reviewer. I re-derived each against the tree at head rather than reading
the replies: 28 are genuinely fixed, 3 are accepted deferrals or a correct won't-fix (the GPL
header reflow / ADFA-5464, the bench activity's main-thread runBlocking / ADFA-5475, the
activity extraction / ADFA-5505 - that file is now 2067 lines, up from the 2003 I flagged),
1 was correctly refuted (the Position import is live again from the deep-link path), and 1 is
addressed by the alternative I offered rather than by a gate (the 62 MB release payload, now
measured in the body). That is a good round of fixes: the awaited clobber dialogs, the graph
warm-up, the manifest save, the ViewFlipper height, the stager stamp and the deferral's own
hand-back all check out where they are claimed to.
One prior finding is still substantively open, replied to in its own thread: the StrictMode
and LeakCanary runs happened and the numbers are convincing, but the PR body still contains
neither word, and REVIEW.md's evidence ledger asks for those two lines in the PR rather than
in a review thread.
Three of this round's findings are siblings of fixes that landed. Worth naming as a pattern
rather than as three separate points: the generateSources narrowing routed the two UI save
paths and the plugin path through the deferral but not ProjectManagerImpl's own file-created
subscriber; the dropdown fix gated the Quick Build row but not the destructive Restart session
row beside it; and the feature-used event moved below the stop branch but still sits above the
save-failure and declined-clobber returns. The sibling sweep CLAUDE.md asks for is what would
have caught all three.
Two MINOR findings are worth reading before the rest. The GradleBuildTuner one is about an
agreement rather than only about code: the body presents the Metaspace floor and the tiered
daemon idle timeouts as "two changes here deliberately ship to all users, with the Experiments
flag off (approved)", and the new class KDoc says the tuner is "deliberately NOT gated behind
FeatureFlags.isExperimentsEnabled" - but its only call site is inside that gate
(GradleBuildService.kt:488), and the else branch sends no jvmArgs at all. The gate is
pre-stack and untouched here, so nothing regresses; what is new is the KDoc and the claim, and
the approval on record was given on that basis. Correcting both is cheap. Lifting the tuner out
of the gate is not - that would be a real all-users change to every Gradle build and would want
its own device pass. The other is that this PR still has no font-scale line in its body, which
CLAUDE.md and REVIEW.md section 8 both ask for on a changed screen; the A56 runs are recorded
in review threads, which is not where QA reads.
On the 62 MB release delta: the measurement answers what I asked for, and I am not re-raising
it. But it is a decision for the release owner rather than for this thread, so please get that
sign-off explicitly before this merges - a 62.06 MB increase for a runtime-preference feature
is hard to walk back after a release.
Two calibration notes, so the confidence here is legible. QB-11's consequence is inferred: that
the narrator reset clears an empty queue is confirmed, but whether the post-unbind lines reach
the next project's pane depends on Main-queue ordering I did not measure. QB-13 has no
demonstrated throw - the finding is that the guard cannot fire, not that it would have. And
QB-2's height claim is deliberately qualitative: the mechanism and the 1.0-vs-2.0 straddle are
confirmed, the per-row pixel figures are not derivable from these sources and were cut.
Findings without a diff anchor: none. All eleven inline anchors are on the RIGHT side and were
validated against the PR diff before posting.
| * session's way. | ||
| */ | ||
| fun onExternalGradleBuildFinished() { | ||
| if (GenerateSourcesDeferral.finishedBuildWasOwn()) { |
There was a problem hiding this comment.
IMPORTANT: creating a resource file bypasses the deferral, so the hand-back downgrades the next Quick Build to a full recompile - the entry point this sweep missed.
The defect is at ProjectManagerImpl.generateSourcesIfNecessary (subprojects/projects/.../ProjectManagerImpl.kt:404), outside this diff: onFileCreated (:446) calls generateSources(builder) directly, never GenerateSourcesDeferral.notifyResourceSaved(). Input: with a live session, add res/layout/foo.xml from the file tree (NewFileAction.kt:493 posts the event). Result: an undeferred Gradle build lands mid-pipeline, finishedBuildWasOwn() is false because nothing armed the claim, so onStandardRunCompleted() -> RefreshBaseline -> onBaselineUntrusted() - and the next save recompiles everything.
Route it through GenerateSourcesDeferral.notifyResourceSaved(). The delete and rename subscribers beside it are already inert, since isAndroidResource checks existence.
| measuredStatusHeight = measured | ||
| // The measure above ran outside a layout pass, so ask for a real one to replace it. | ||
| header.requestLayout() | ||
| applyCollapsedHeaderHeight() |
There was a problem hiding this comment.
IMPORTANT: at a 2x font scale the collapsed sheet's peek height changes repeatedly through every standard Gradle build, for all users with the Experiments flag off.
setStatus posts refreshCollapsedHeight() (:728) on every write, and its hottest caller is EditorBuildEventListener.onProgressEvent (:150), one call per TaskStartEvent. With maxLines now 3, a longer task name wraps to a different row count, measured != measuredStatusHeight, and this line pushes a new peekHeight plus header height. At scale 1.0 the status block stays under the editor_sheet_collapsed_height floor whatever it wraps to, so nothing moves; at 2.0 the row count crosses that floor, so the peek jumps as Gradle alternates between "Task :app:compileV8DebugKotlin" and "Resolve dependencies of :app:v8DebugRuntimeClasspath".
Only grow the peek, or debounce it, while a build is narrating.
| } | ||
|
|
||
| R.id.action_quick_build_restart_session -> { | ||
| quickBuildSessionManager()?.restartSessionAndReprovision() |
There was a problem hiding this comment.
MINOR: the dropdown's "Restart session" row is still ungated, so it tears a warm session down for a reprovision that cannot run.
The Quick Build row above now reads quickBuild.label/quickBuild.enabled (:846-851), which closed the earlier finding on this menu. This row got no gate. While a standard Gradle build owns the slot - the exact state prepare() greys the button for - a tap here dispatches SessionRestartAndReprovisionRequested, which tears down the session and the daemon, then provision() reaches runProxyAppBuild's isBuildInProgress check (GradleQuickBuildProvisioner.kt:402) and returns SlotBusy -> quick_build_slot_busy. The user loses a warm session and gets "wait and try again".
Hide or disable this row while QuickBuildAction.isBlockedByStandardBuild().
| block: suspend () -> T, | ||
| ): T = | ||
| internalBuild.hold { | ||
| internalBuildProgress = progressListener |
There was a problem hiding this comment.
MINOR: the progress listener is a single field while the bracket around it is depth-counted, so a nested internal build silently blinds the outer one.
InternalBuildBracket uses a counter specifically so nesting is safe ("A counter rather than a boolean, so a nested internal build cannot leave this stuck on"), but internalBuildProgress is not counted: an inner withInternalBuild overwrites the field and its finally (:198) sets it to null, so every remaining output line of the still-running OUTER build goes nowhere - and takeInternalBuildOutput() is the only place Gradle's reason exists for a suppressed build.
Unreachable today: git grep withInternalBuild finds one caller, GradleQuickBuildProvisioner.kt:425, reached only from runProxyAppBuild behind its own isBuildInProgress pre-check. Save and restore the previous value.
| android:layout_height="wrap_content" | ||
| android:ellipsize="end" | ||
| android:gravity="center" | ||
| android:maxLines="3" |
There was a problem hiding this comment.
MINOR: the PR body carries no font-scale statement, which CLAUDE.md and REVIEW.md section 8 both require in the PR for a changed screen.
Squarely in scope: maxLines 1 -> 3 here, a collapsed header now measured rather than fixed, three new AlertDialogs, a new PopupMenu and a new toolbar button. CLAUDE.md says verify at 1.0 and 2.0 and "say in the PR that you did"; REVIEW.md section 8 adds that "silence is not" a valid opt-out. gh pr view 1723 --json body has zero occurrences of "font", "scale", "1.0" or "2x". The evidence exists - your status-bar and ViewFlipper replies record A56 runs at 1.0/2.0/3.0 - but QA reads the body, not the threads.
Add the one line, naming both scales and what you checked.
| if (!result.xmlSaved) { | ||
| result.xmlSaved = modified && isXml | ||
| accumulateSaveFlags(result, fileName, modified) { | ||
| frag.file?.let { file -> |
There was a problem hiding this comment.
MINOR: this lambda re-reads the editor's file off the main thread instead of using savedFile, so a disposed view silently loses the save's generateSources run.
savedFile was captured on Main at :1530 and is what the line right above (fileTimestamps[savedFile.absolutePath]) uses; this is the one editor-state read in the function not marshalled to Main. CodeEditorView.file is get() = editor?.file and editor is get() = _binding?.editor, so once the binding is released frag.file is null, the lambda yields false, resourceXmlSaved stays false, and a layout save leaves the Java LSP with stale R.*.
Narrow today: the disposal your catch (IllegalStateException) at :1538 documents returns at :1545 before reaching here, so it needs a release between save() returning true and this line. Pass savedFile.
| // The narrator is a process-wide singleton and its queue is per-project narration. | ||
| // Held lines belong to the project being closed, so without this they flush into the | ||
| // NEXT project's Build Output as that project's progress. | ||
| quickBuildOutputNarrator()?.reset() |
There was a problem hiding this comment.
MINOR: the reset runs before the narration it exists to discard, so it clears an empty queue.
The comment pairs this with restartSession() on the line above, but that only dispatches onto the session thread - the teardown's status lines do not exist yet. Meanwhile the sink is still bound, since unbind is on the lifecycle observer's onDestroy (:399-405), so anything narrated between here and then goes to appendBuildOutput, which drops it (isFinishing is true, :1246). The lines that arrive after onDestroy unbinds then queue with nothing left to clear them, and flush into the next project's Build Output - which is what this reset claims to prevent.
Reset after the unbind, or have the session signal teardown-complete and reset on that.
| // re-arms mid-dialog respectively, and a re-arm mid-dialog loops: the re-armed | ||
| // AwaitingInstall shows a second dialog behind the first. | ||
| val confirmed = | ||
| when (val decision = installTimeClobberConfirmation(answerAtTap, now)) { |
There was a problem hiding this comment.
MINOR: an unparseable APK turns an ordinary Run into a destructive "Replace the app installed for this project?" dialog, and a decline silently drops the install.
With the flag on and no proxy app ever created: the tap resolves the id, finds the slot empty, and carries NotNeeded. The install then fails to parse the APK, so apkApplicationId is null (:576, :647-651) and now is NeededForUnknownAppId. installTimeClobberConfirmation only treats an atTap of NeededForUnknownAppId as covering a later answer, so NotNeeded -> unknown falls to else -> now and the unknown-occupant dialog is shown after a successful build. Decline sets dispatched = true (:617), so nothing installs, nothing re-arms, and no message is shown.
An atTap of NotNeeded should not be widened by a parse failure on our own APK.
| // Awaited, not read: a fire that skipped because the warm-up was still running | ||
| // would drop the prebuild for this sync. Same scope as the stagger, so closing | ||
| // the project drops a fire still waiting on the warm-up. | ||
| editorActivityScope.launch { |
There was a problem hiding this comment.
MINOR: this re-launch puts the risky work outside the try/catch written to protect it.
QuickBuildPrebuildStagger wraps fire() in a catch whose comment explains the stakes: "The scope is the editor activity's, which carries a plain Job and no CoroutineExceptionHandler, so a throw here takes the IDE down half a minute after a project opens". But this fire body only calls editorActivityScope.launch, which returns a Job immediately, so the catch sees nothing and the await()/onProjectSynced work runs as a fresh child of editorActivityScope - CoroutineScope(Dispatchers.Default), plain Job, no handler (BaseEditorActivity.kt:208). A throw there cancels the scope, killing the editor's other launch sites, and reaches the default uncaught handler.
Move the try/catch inside this launch.
| GeneralPreferences.lastOpenedProject = project.path | ||
| val editor = | ||
| Intent(this, EditorActivityKt::class.java).apply { | ||
| putExtra("PROJECT_PATH", project.path) |
There was a problem hiding this comment.
NITPICK: this is the one intent-extra call site that writes the literal instead of the constant, and it is the one the new autostart guard depends on.
EditorIntentExtras's own KDoc says why the object exists: "Spelled out here rather than repeated as string literals at each of their ~13 call sites ... A typo in one of those reads back as a missing extra -- silently, at runtime". ProjectHandlerActivity.onNewIntent (:715) reads EditorIntentExtras.EXTRA_PROJECT_PATH for the same-project check added last round, so a typo here would make target null, skip the guard, and re-open the bug you filed - with no compile error and no test failure. The values do match today (EXTRA_PROJECT_PATH = "PROJECT_PATH"), so nothing is broken.
Use EditorIntentExtras.EXTRA_PROJECT_PATH.
Part 11/11 of the stacked split of #1669 (requested by Akash). Base: feature/ADFA-4128-qb-10-gradle-plugin. Stack overview + review mechanics: PR 1 (#1713). Terms are defined in quickbuild/README.md (lands in PR 1).
Puts Quick Build in front of the user: a button next to Run, and enough narration to tell what it is doing and when it has finished. It also adds a harness to make it easier to run standard Gradle build and Quick Build benchmarks, and to gather key metrics about the stages of the build process.
flowchart TB subgraph appc["<b>This PR: inside app/ — wiring and bench</b>"] act["QuickBuildAction<br/>registered only when<br/>FeatureFlags.isExperimentsEnabled<br/><i>QuickBuildAction.kt</i>"] --> mgr["QuickBuildManager<br/>session lifecycle, provisioning,<br/>stop-tap cancellation"] mgr --> narr["QuickBuildOutputNarrator<br/>attached to the session manager;<br/>queues while no pane is bound<br/><i>QuickBuildOutputNarrator.kt</i>"] mgr --> sb["status bar collector<br/>lifecycle-scoped: state, not history<br/><i>QuickBuildStatusBar.kt</i>"] koin["QuickBuildModule (Koin)<br/>binds every core port;<br/>assetsLiveReloadable read once<br/>at the Android edge<br/><i>QuickBuildModule.kt</i>"] tr["bench trampoline activity<br/>debug-source-set manifest only<br/><i>QuickBuildBenchActivity.kt</i>"] --> mgr mgr --> hooks["QuickBuildBenchHooks<br/>inert release twin<br/><i>debug/QuickBuildBenchHooks.kt</i>"] hooks --> rec["event + metrics recorders"] rec --> log["bench-events.jsonl<br/><i>BenchEventsFile.kt</i>"] hooks --> e2e["MODE_STANDARD<br/>autostarts the standard Run and<br/>stops at the build result<br/><i>QuickBuildBenchAutostart.kt</i>"] end adb["adb shell am start<br/>gated on android.permission.DUMP"] --> tr mgr --> core[":quickbuild:core session manager (PRs 5-8)"] narr --> pane["Build Output pane (existing)"] sb --> bar["bottom status bar (existing)"] mgr -- "provisioning + rebuild builds" --> gbs["GradleBuildService (existing)"] classDef thisPrBox fill:#dbeafe,stroke:#93c5fd,color:#1e3a5f classDef inPr fill:#ffffff,stroke:#64748b,color:#000 class appc thisPrBox class act,mgr,narr,sb,koin,tr,hooks,rec,log,e2e inPrWhat to review
QuickBuildAction.kt— owns the session tap: start, stop-tap cancel, grey-out. Line-by-line.Gradle tuning — Metaspace 192→384 MB + daemon idle timeouts (30 min balanced / 2 h high-perf); the only changes to non-QB behavior
QuickBuildOutputNarrator.kt,QuickBuildStatusBar.kt— queued narration; lifecycle-scoped status showing state, not history.QuickBuildModule.kt— binds every core port; reads assetsLiveReloadable at the Android edge.GenerateSourcesDeferral.kt— defers resource-XML generateSources until Quick Build goes idle.John's items C15, C16, C17, C22, C23 folded in as fixes.
Rollback: without the flag there is no UI entry point.
Followup, not fixed: R8 emits kotlin.Metadata warning noise.
Release APK: the Quick Build daemon zip (61,963,732 bytes) and runtime AAR (95,371 bytes) are staged into every variant's assets, stored uncompressed, so the release APK grows by 62.06 MB (59.2 MiB). Not gated to debug because
FeatureFlags.isExperimentsEnabledis a runtime preference available in release; once the flag graduates the payload is the feature's cost, until the work to merge the Quick Build tools with the ones CoGo already ships is done.Dialogs and the toolbar dropdown are Views, not composables, because they live in the existing View-based editor toolbar ADR 0009 keeps as-is, and the Help item routes into the 3-tier help that has no Compose entry point until ADFA-4381.
QuickBuildBenchAutostart.kt— MODE_STANDARD autostarts the standard Run and stops at the build result, so it measures the build only. Line-by-line.The standard arm's install dialog is suppressed, because an unattended run cannot answer it. Quick Build's arm measures build, deploy and reload, so the two are not like for like from in-app numbers alone.
QuickBuildBenchActivity.kt,QuickBuildBenchHooks.kt— DUMP-gated trampoline; inert release twin.BenchQuickBuildMetricsSink.kt—relaunchOkis always recorded;toRunningMillisrides only on a relaunch that reconnected, so it is absent rather than a measured zero.How this PR Was Tested
Automated tests (see coverage details below)
Manual QA — walked the
manual-qa.mdtest plan on the A56 [measured on a56]Benchmark — measured on real devices, both arms: a warm code edit reaches the running app with about a 5x median speedup over a standard build + deploy (A56 4.35x, A06 6.53x, 5.12x combined, from the pass accepted on 2026-08-11). The weaker the phone, the bigger the win.
How the two arms were made comparable, since they are not like for like from in-app numbers: the in-app standard arm (
MODE_STANDARD) suppresses the install dialog and stops at the build result, so it measures the build only. The benchmark harness adds install and launch from outside the process — it drives the install dialog and takes the span from the save to the app'sDisplayedon the device clock — and the published basis subtracts the harness's own time from that span. Quick Build's arm measures build, deploy and reload in-app. There is noMODE_STANDARD_E2E; earlier drafts of this description named one that was never in the tree.Still open — the rebaseline relaunch path is not yet device-verified, and neither is the API 28/29 resource-swap success path.
Coverage (JaCoCo, measured at 3da6482 on 2026-09-05, single run of the full
:appunit-test suite: 100 suites, 797 tests, 0 failures). Measured against the classes the tests load (Sentry's ASM-transformed output), so no execution data goes unmatched; the earlier table was read against the untransformed classes and under-counted every row that has tests. The rows at or near 0% are Android-bound or UI classes that do not run on the JVM; they are device-tested.A lot of this was UI code and wasn't covered very well by automated tests.
actions/buildQuickBuildActionpresentation and save order have JVM testsactions/fileactivities/editorSaveResultFlagsandQuickBuildClobberConfirmationread 100%analytics/quickbuildappApplicationclasses, Android-bounddifragments/sidebarhandlersquickbuildGradleQuickBuildProvisioner(23%) and the bench Activity carry most of the gapservices/builderServiceat 0% over 408 lines; tuner and strategies 73–100%uiEditorBottomSheet)utilsviewmodelReview fixes (2026-08-22)
A review-fixes commit addresses the code-review findings. Two changes here deliberately ship to all users, with the Experiments flag off (approved):
One candidate followup from review (orchestrator forcing a full-changed compile after a failed dex/deploy) was re-checked and refuted at this tip: the forced flag re-arms and a forced no-op already performs the full rebuild. The daemon-side recovery lever stays in as defense in depth.
🤖 Generated with Claude Code
https://claude.ai/code/session_01XkGof8cLt23LkxZ8MKzin2