[ADFA-320] - add Permissions Screen Test - #2
Merged
Merged
Conversation
romanlapa
reviewed
Jan 14, 2025
jatezzz
pushed a commit
that referenced
this pull request
Jun 22, 2026
jatezzz
pushed a commit
that referenced
this pull request
Jun 22, 2026
[ADFA-320] - add Permissions Screen Test
jatezzz
added a commit
that referenced
this pull request
Jul 17, 2026
Fan-out, adapter, and plugin-API fixes from the ProjectSearchExtension review: - Guard plugin results against null elements (mapNotNull) and wrap the collection block in a logging try/catch so a bad element no longer kills the whole fan-out silently. (#1) - buildRows buffers each section's rows and emits the header only when a group exists, skipping blank titles -- no more orphan/blank headers suppressing the empty state. (#2) - Move the fan-out to lifecycleScope so a slow plugin can't pin a destroyed Activity/ViewModel or post to a dead Activity. (#3) - Keep the search progress indicator up when the built-in search is empty but plugins are enabled, so a plugin-only match doesn't flash a terminal "no results" state. (#4) - SearchListAdapter is now a ListAdapter + DiffUtil reused across emissions; the two-phase publish diffs instead of reallocating, preserving scroll and already-run highlights. (#5) - Collapse the adapter constructors to one clicks ctor plus a thin map convenience ctor; drop the dead keys param and nullable-value scrub. (#6) - Replace the blocking .get(10s) with a non-blocking withTimeoutOrNull + await so no commonPool worker is tied up. (#7) - Trim pipe-split filter tokens at the source (endsWith(" kt") matched nothing in built-in search too); align ProjectSearchExtension KDoc with actual behavior and drop the non-ASCII em-dash. (#9, #10)
This was referenced Jul 17, 2026
jatezzz
added a commit
that referenced
this pull request
Jul 20, 2026
Fan-out, adapter, and plugin-API fixes from the ProjectSearchExtension review: - Guard plugin results against null elements (mapNotNull) and wrap the collection block in a logging try/catch so a bad element no longer kills the whole fan-out silently. (#1) - buildRows buffers each section's rows and emits the header only when a group exists, skipping blank titles -- no more orphan/blank headers suppressing the empty state. (#2) - Move the fan-out to lifecycleScope so a slow plugin can't pin a destroyed Activity/ViewModel or post to a dead Activity. (#3) - Keep the search progress indicator up when the built-in search is empty but plugins are enabled, so a plugin-only match doesn't flash a terminal "no results" state. (#4) - SearchListAdapter is now a ListAdapter + DiffUtil reused across emissions; the two-phase publish diffs instead of reallocating, preserving scroll and already-run highlights. (#5) - Collapse the adapter constructors to one clicks ctor plus a thin map convenience ctor; drop the dead keys param and nullable-value scrub. (#6) - Replace the blocking .get(10s) with a non-blocking withTimeoutOrNull + await so no commonPool worker is tied up. (#7) - Trim pipe-split filter tokens at the source (endsWith(" kt") matched nothing in built-in search too); align ProjectSearchExtension KDoc with actual behavior and drop the non-ASCII em-dash. (#9, #10)
jatezzz
added a commit
that referenced
this pull request
Jul 20, 2026
…#1528) * feat(search): add ProjectSearchExtension API for plugin result sections Fan out to enabled plugins after built-in search and render their sections. * style: apply spotless formatting to search extension files * fix(search): address review findings on plugin search extension - Bound plugin search fan-out with a 10s timeout and drop results from superseded searches via a generation counter in EditorViewModel - Guard against Java plugins completing their future with null - Flatten SearchListAdapter's nested RecyclerView into Row.Match rows so match rows recycle instead of inflating all at once - Narrow catch(Throwable) to catch(Exception) per REVIEW.md - Document plugin-api search contract types; coordinates are 0-based, end-exclusive - Marshal plugin crash recording to the UI thread and drop the unread searchResults flow. * fix(search): address review feedback on search results list - Make the group row's LinearLayout the layout root so the click listener on binding.root receives taps (the clickable child was consuming them) and drop the redundant RelativeLayout wrapper - Log highlight failures via SLF4J and keep the plain-text preview instead of overwriting the line with the matched text - Skip files with empty match lists to avoid dangling group headers - Hide the decorative file icon from accessibility services * fix(search): address code-review findings on plugin search Fan-out, adapter, and plugin-API fixes from the ProjectSearchExtension review: - Guard plugin results against null elements (mapNotNull) and wrap the collection block in a logging try/catch so a bad element no longer kills the whole fan-out silently. (#1) - buildRows buffers each section's rows and emits the header only when a group exists, skipping blank titles -- no more orphan/blank headers suppressing the empty state. (#2) - Move the fan-out to lifecycleScope so a slow plugin can't pin a destroyed Activity/ViewModel or post to a dead Activity. (#3) - Keep the search progress indicator up when the built-in search is empty but plugins are enabled, so a plugin-only match doesn't flash a terminal "no results" state. (#4) - SearchListAdapter is now a ListAdapter + DiffUtil reused across emissions; the two-phase publish diffs instead of reallocating, preserving scroll and already-run highlights. (#5) - Collapse the adapter constructors to one clicks ctor plus a thin map convenience ctor; drop the dead keys param and nullable-value scrub. (#6) - Replace the blocking .get(10s) with a non-blocking withTimeoutOrNull + await so no commonPool worker is tied up. (#7) - Trim pipe-split filter tokens at the source (endsWith(" kt") matched nothing in built-in search too); align ProjectSearchExtension KDoc with actual behavior and drop the non-ASCII em-dash. (#9, #10) * style(search): expand when branches to block form for ktlint Spotless ratchets from origin/stage; stage was reformatted after this branch diverged, so CI flagged the single-expression when branches in onUpdateWadbConnectionStatus and the multi-line FileOpResult.Success branch. Expand them to ktlint's canonical block form so the file is fully canonical and passes under any ratchet base. * fix: reported violations for spotless
fryanpan
added a commit
that referenced
this pull request
Jul 30, 2026
Bryan review, two changes: 1. Incremental dexing moves to #2, javac to 3a/3b. Adding the Affects column made the case: dexing helps ALL apps and is the largest remaining line on a Java body edit after the storage fix; javac only helps apps with Java. Strawman ordering - the effort numbers did not change. 2. sora-slow-path-gap.md is retired. Most of it duplicated the roadmap and the storage note; the three things only it had are now a "What we ruled out" section here - the two hypotheses the measurements killed, the correction to the javac design note framing, and why the analytics residual field exists. Inbound links from benchmarking, incremental-javac-design and the storage note now point at the evidence run directly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkdFG7t9oNJtiayQzUQf89
5 tasks
14 tasks
davidschachterADFA
added a commit
that referenced
this pull request
Sep 9, 2026
…elivered
Review of the first version found three problems with it, all mine.
The only test passed against the unfixed code. GradleDaemonWatcher
.shutdown() already read { scheduler.shutdownNow() } on stage -- what was
missing was anything calling it -- so a test of the watcher in isolation
pinned nothing, and deleting the new block in ToolingApiServerImpl left
the suite green. CLAUDE.md asks for the opposite and I checked it on the
sibling change and not on this one. There is now a seam
(newDaemonWatcher, defaulted, the shape GradleDaemonWatcher itself uses
for descendants and scheduler) and two tests at the caller: shutting the
server down stops the watcher, and a server that never ran a build does
not construct one just to stop it. Both fail against their own mutation.
The "after the daemons, not before" ordering did not do what it claimed.
The exit arrives through handle.onExit().thenRun { scheduler.execute
{ ... } }, and onExit completes on a process-reaper thread only once the
OS has reaped the daemon -- strictly after DefaultGradleConnector.close()
returns. There is no point in the sequence where the scheduler still
accepts work and the daemon has already been reaped, so the report is not
deliverable at shutdown under any ordering. The call goes back early,
where it ends the scanning thread soonest and no late poll can report
into a channel being torn down, and the comment says plainly that the
shutdown-time exit is not delivered.
Moving `client = null` after the wait made consequence #2 worse rather
than better: it left the client non-null for the whole daemon-stopping
window instead of none of it, so a late callback could reach a
half-torn-down channel. It goes back where stage had it.
GradleDaemonWatcher.shutdown() is now graceful then forceful -- a report
already queued still runs, a poll wedged mid-scan cannot hold the process
open. That part is a real improvement and is tested both ways.
What remains true is consequence #1, which was always the defect: an
unstopped watcher goes on scanning ProcessHandle.descendants() for up to
a minute after the server is gone.
Tests: 17 in the module, four new. Each fails against the mutation it is
named for -- removing the call, dropping the isInitialized guard,
reverting shutdown() to shutdownNow() alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j
hal-eisen-adfa
added a commit
that referenced
this pull request
Sep 11, 2026
* ADFA-5659: shut the Gradle daemon watcher down with the server GradleDaemonWatcher.shutdown() had no caller. Reported by hal-eisen-adfa on PR #1812, where the code is in the base commit rather than the diff -- it came in with ADFA-5514 via #1798, so it is live on stage. - the watcher's thread outlived server shutdown, and an in-flight poll chain went on scanning ProcessHandle.descendants() for up to a minute - onExit().thenRun { client?.onGradleDaemonExited(pid) } could fire into a client whose RPC channel was being torn down; shutdown() sets client to null, so that was a race rather than a guaranteed no-op - shutdown() was dead code, which made onBuildStarted's note about the scheduler rejecting work after shutdown describe an unreachable state Two details that are easy to get wrong, both found in review of the version of this fix that rides ADFA-5589: It is called after DefaultGradleConnector.close(), not before. Stopping the daemons is what produces the exit, and the exit is reported through scheduler.execute { ... } -- a scheduler already shut down rejects it and merely logs, so the client never hears that the daemon it is plotting has gone. For the same reason the client is cleared after the wait rather than before it; best effort even then, since the client's own channel is going away at the same time. It goes through the lazy delegate rather than the property, or a server that never ran a build constructs a watcher, and its scheduler, purely to shut it down again. This is carried out of PR #1813, which is a draft while the review queue drains, so the fix does not wait on it. Tests: 9 in GradleDaemonWatcherTest, one new -- shutdown reaches scheduler.shutdownNow(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j * ADFA-5659: pin the fix at the caller, and stop claiming the exit is delivered Review of the first version found three problems with it, all mine. The only test passed against the unfixed code. GradleDaemonWatcher .shutdown() already read { scheduler.shutdownNow() } on stage -- what was missing was anything calling it -- so a test of the watcher in isolation pinned nothing, and deleting the new block in ToolingApiServerImpl left the suite green. CLAUDE.md asks for the opposite and I checked it on the sibling change and not on this one. There is now a seam (newDaemonWatcher, defaulted, the shape GradleDaemonWatcher itself uses for descendants and scheduler) and two tests at the caller: shutting the server down stops the watcher, and a server that never ran a build does not construct one just to stop it. Both fail against their own mutation. The "after the daemons, not before" ordering did not do what it claimed. The exit arrives through handle.onExit().thenRun { scheduler.execute { ... } }, and onExit completes on a process-reaper thread only once the OS has reaped the daemon -- strictly after DefaultGradleConnector.close() returns. There is no point in the sequence where the scheduler still accepts work and the daemon has already been reaped, so the report is not deliverable at shutdown under any ordering. The call goes back early, where it ends the scanning thread soonest and no late poll can report into a channel being torn down, and the comment says plainly that the shutdown-time exit is not delivered. Moving `client = null` after the wait made consequence #2 worse rather than better: it left the client non-null for the whole daemon-stopping window instead of none of it, so a late callback could reach a half-torn-down channel. It goes back where stage had it. GradleDaemonWatcher.shutdown() is now graceful then forceful -- a report already queued still runs, a poll wedged mid-scan cannot hold the process open. That part is a real improvement and is tested both ways. What remains true is consequence #1, which was always the defect: an unstopped watcher goes on scanning ProcessHandle.descendants() for up to a minute after the server is gone. Tests: 17 in the module, four new. Each fails against the mutation it is named for -- removing the call, dropping the isInitialized guard, reverting shutdown() to shutdownNow() alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j * ADFA-5659: serialise building the daemon watcher against stopping it `shutdown()` asked `lazyDaemonWatcher.isInitialized()` while `runBuild` touched `daemonWatcher`, with nothing between them. Both bodies run on the common pool, so a build submitted just before a shutdown can construct the watcher, and its scheduler, after shutdown has already looked and found none -- the leak this PR's shutdown call exists to prevent, arriving by the one route that check cannot see. One lock now covers the construction and a shutdown flag. Shutdown first means the build skips building a watcher at all; build first means shutdown sees it and stops it. The lock spans only the construction: `shutdown()` and `onBuildStarted()` both run outside it, so neither blocks the other, and a watcher stopped in between hits the scheduler rejection `onBuildStarted` already guards. Severity, since the reported finding overstated it: the scheduler's thread is a daemon thread and `Main` ends with `exitProcess(0)`, so a leaked watcher costs extra `descendants()` scans during teardown rather than holding the JVM open. The same is true of the defect this PR started from. Tests, both proved against the unfixed code: - a build that starts after shutdown does not build a watcher -> without the flag: expected 0 but was 1 - shutdown waits for a watcher a concurrent build is building -> without the lock: shutdown completed while the watcher was still being constructed The second asserts a bounded negative -- shutdown did not conclude within a second, against an unlocked shutdown that returns in milliseconds -- and its comment says so. Found by CodeRabbit on #1816. * ADFA-5659: restore the interrupt only for an actual interrupt `shutdown()` wrapped `awaitTermination` in `runCatching`, which catches `Throwable`, and re-interrupted the calling thread for anything it caught. Only an `InterruptedException` says anything about that thread's cancellation state; for any other failure the flag is set on a thread that was never cancelled. That matters because of where the flag lands. `shutdown()` runs as a teardown step on a `ForkJoinPool.commonPool` worker, and its caller's next act is `connectionCloseFuture.get()` in `ToolingApiServerImpl.shutdown()`. A set flag makes that `get()` throw immediately, so `connector.disconnect()` is never waited on, and the worker carries a stale interrupt into whatever the pool runs next. The catch stays `Throwable`: narrowing it would let a non-interrupt propagate out of `shutdown()` and skip `scheduler.shutdownNow()`, leaving the scheduler merely graceful. A failure still counts as "not drained", so the forceful stop still runs -- pinned by the first test below. This addresses the reported "any Throwable sets the flag" half only. A genuine interrupt still sets it and still aborts the connection-close wait; that half is left as reported, deliberately. Tests, both against the unfixed code: - a wait that fails for another reason does not mark the thread interrupted -> without the narrowing: interrupted() expected to be false - an interrupted wait still marks the thread interrupted -> pins that the narrowing did not overshoot Reported by @jatezzz on #1816. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Hal Eisen <haleisen@appdevforall.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the Kaspresso test for the Permissons screen.
https://appdevforall.atlassian.net/browse/ADFA-320