ADFA-5067: Support deep links to open projects and files - #1651
Merged
Conversation
…ookkeeping helper New, self-contained plumbing for deep-link support (no behavioral wiring yet): - DeepLinkRequest/PendingFileRequest/DeepLinkOpenRequest models and the URL parser for https://www.appdevforall.org/device/open/project/{name}[/file/{f}[/line/{n}[/column/{n}]]]. - PendingDeepLinkOpen, an in-memory handoff for the close-then-reopen continuation. - resolveWithinDirectory, a path-traversal guard for the attacker-controllable {filename} segment, mirroring the existing zip-slip pattern in AssetsInstallationHelper.extractZipToDir. Also guards against InvalidPathException from an embedded NUL byte (a %00 in the URL decodes to a literal NUL character, which java.nio.file.Path.resolve() throws on if uncaught). - recordProjectOpenedBookkeeping, extracted from MainActivity.openProject so a deep-link-triggered project switch gets the same Recents/analytics bookkeeping. - New error strings for the above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DeepLinkActivity is a UI-less trampoline holding the only <intent-filter> for https://www.appdevforall.org/device/open/project/... links. It parses the incoming URI, checks whether a project is already loaded (IProjectManager.getInstance().workspace), and routes to MainActivity (nothing open) or the live, singleTask EditorActivityKt (one is, reused via onNewIntent), then finishes itself immediately. Kept as a plain Activity (matching the existing SplashActivity precedent), not BaseIDEActivity, since it never calls setContentView and has no theming needs of its own -- this avoids a visible flash of MainActivity's real UI in the common case where the actual destination is the already-running editor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wires DeepLinkRequest handling into MainActivity's onCreate/onNewIntent: resolves the project name via findValidProjects, flashes an error if it doesn't exist, and otherwise opens it directly via openProject (bypassing GeneralPreferences.confirmProjectOpen -- an explicit link tap is itself a specific request to open project X, so re-confirming it is redundant friction). openProject gains an optional pendingFileRequest param that rides along in the EditorActivityKt intent extras for file/line/column navigation once the project finishes loading; all existing call sites are unaffected since it defaults to null. Also reindents a pre-existing over-length line in startWebServer() that the Spotless ratchet now covers as a side effect of touching this file (no behavior change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dlerActivity This is the activity that owns both the confirm-close dialog and the open editor tabs, so it makes the same-project/different-project decision itself rather than MainActivity: - onNewIntent resolves the project name and compares it against IProjectManager's current workspace/projectDirPath. Same project already open -> no-op project-wise, just navigate to the requested file. Different project open -> reuse the existing, unmodified confirmProjectClose() dialog. - confirmProjectClose/performCloseAllFiles gain an optional trailing onClosed callback (default null, so both existing call sites -- back-press and the sidebar "Close Project" action -- are byte-for-byte unchanged in behavior). onClosed only records the pending request (PendingDeepLinkOpen); it does not call startActivity synchronously, because doing so immediately after finish() risks the framework redelivering the new PROJECT_PATH to the dying singleTask instance via onNewIntent instead of spawning a fresh one. Instead onDestroy() drains it once the instance is guaranteed torn down. - applyDeepLinkFileRequest resolves the file/line/column request through resolveWithinDirectory (path-traversal guard) and reuses the existing openFileAndSelect/validateRange clamping -- no new clamping logic needed. - postProjectInit consumes a pending file request once a freshly opened project (cold open, or the tail of a close-then-reopen) finishes loading. Also fixes a pre-existing race in openFileAndSelect, found while testing the above on-device: EditorFeatures.validateRange mutates its Position arguments in place, and a freshly-created CodeEditorView's own async content-load pipeline calls validateRange/setSelection on that *same* Range instance separately from this function's own call. If this function's postInLifecycle callback ran first -- while the document was still the just-constructed empty one line -- it permanently clamped the shared Position down to (0,0) before the real content ever loaded, so opening a file that wasn't already in a tab at a specific line silently landed the cursor at line 1 instead. Fixed with a defensive copy so this function can no longer corrupt the shared instance regardless of which side runs first. This is existing, general-purpose API, not deep-link-specific -- no other caller happened to combine "brand-new tab" with a non-origin selection before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rification Placed at the top level so it mirrors the real eventual absolute path (https://www.appdevforall.org/.well-known/assetlinks.json) exactly, meaning relocating it to the actual website later is a literal file copy, not a rename. sha256_cert_fingerprints is left as a TODO placeholder -- the real value belongs to whoever controls the release signing key / Play Console and can't be filled in from source. Until that's live, autoVerify will fail Digital Asset Links verification and Android may show a disambiguation chooser instead of auto-opening the app; expected per the ticket's own framing ("we will move it to the website later"). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
WalkthroughAdded verified HTTPS App Links for project and file navigation. The change adds deep-link parsing, project resolution, editor handoff, lifecycle-safe state handling, recent-project bookkeeping, save-result propagation, and traversal-safe filesystem validation. ChangesDeep-link navigation
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to Deep-link project switching can be silently dropped during activity teardown, and file navigation can receive corrupted selection state; an additional process-death edge case may replay an older link. These are concrete correctness issues in user-facing deep-link flows, so the PR is not merge-ready until they are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Android
participant DeepLinkActivity
participant MainActivity
participant EditorHandlerActivity
participant RecentProjectRepository
Android->>DeepLinkActivity: Open verified HTTPS project link
DeepLinkActivity->>MainActivity: Forward parsed request
MainActivity->>RecentProjectRepository: Persist project-open bookkeeping
MainActivity->>EditorHandlerActivity: Open project and pending file
EditorHandlerActivity-->>Android: Display project file at requested position
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth new test files use raw JUnit assertions instead of Truth. The repository convention requires Google Truth assertions in new tests. The shared root cause is the
org.junit.Assertimport in each file.
app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt#L21-L22: replaceassertEquals/assertNullwithassertThat(...).isEqualTo(...)andassertThat(...).isNull(), and keepRobolectricTestRunner.app/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt#L20-L21: replaceassertEquals/assertNullwith the equivalent Truth assertions.
As per coding guidelines: "Use JUnit Jupiter, Truth, MockK for new tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt` around lines 21 - 22, Replace raw JUnit assertions with Google Truth assertions in DeepLinkRequestTest.kt (lines 21-22) and PathTraversalTest.kt (lines 20-21), importing Truth’s assertThat and converting assertEquals/assertNull to isEqualTo/isNull; retain RobolectricTestRunner in DeepLinkRequestTest.Source: Coding guidelines
app/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt (2)
50-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the rejected path or drop the unused binding.
detekt reports
SwallowedExceptionat line 54. The coding guidelines require that handled notable failures are logged rather than dropped. Add an SLF4J debug log, or rename the parameter to_if the rejection is intentionally silent.As per coding guidelines: "Do not swallow exceptions silently; log handled notable failures and report them through the established observability mechanism when appropriate."♻️ Proposed fix
+private val log = LoggerFactory.getLogger("PathTraversal") + fun resolveWithinDirectory( baseDir: File, relativePath: String, ): File? { if (relativePath.contains("..") || relativePath.startsWith("/") || relativePath.startsWith("\\")) { return null } return try { val base = baseDir.toPath().toAbsolutePath().normalize() val resolved = base.resolve(relativePath).normalize() if (!resolved.startsWith(base)) null else resolved.toFile() } catch (e: InvalidPathException) { + log.debug("Rejected unrepresentable deep-link path", e) null } }Add the import:
import org.slf4j.LoggerFactory🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt` around lines 50 - 56, Update the InvalidPathException handling in the path-resolution function to satisfy SwallowedException: either log the rejected path at debug level using the project’s established SLF4J logger, or rename the unused exception binding to “_” when silent rejection is intentional. Keep the existing null return behavior.Sources: Coding guidelines, Linters/SAST tools
51-53: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueNote the symlink gap in the containment check.
normalize()resolves the path lexically only. A symlink inside the project directory that points outside still passesstartsWith(base). If the threat model includes symlinks in a cloned or imported project, usetoRealPath()for existing files and compare the real paths. If symlinks are out of scope, state that in the KDoc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt` around lines 51 - 53, Update the path containment logic around baseDir and relativePath to close the symlink gap: for existing paths, resolve both the base directory and candidate through toRealPath() before comparing containment, while preserving appropriate handling for nonexistent targets. If symlinks are intentionally out of scope instead, document that limitation in the function’s KDoc.app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider telling the user when the link cannot be parsed.
If
parsereturnsnull, the activity finishes with no feedback. The user taps a link and sees nothing. A toast or a route toMainActivitywould make the failure visible. The strings file already contains deep-link error messages for the other failure modes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt` around lines 41 - 45, The null-request branch in DeepLinkActivity should provide user-visible feedback before finishing, using the existing deep-link error string from the strings resource. Update the request parsing failure path around DeepLinkRequest.parse to show an appropriate toast or equivalent message, then preserve the existing finish-and-return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.well-known/assetlinks.json:
- Around line 7-9: Replace TODO_REPLACE_WITH_RELEASE_SIGNING_SHA256_FINGERPRINT
in the sha256_cert_fingerprints configuration with the actual release
certificate SHA-256 fingerprint, then publish assetlinks.json at the required
.well-known URL with Content-Type application/json before enabling App Links.
In `@app/src/main/AndroidManifest.xml`:
- Around line 99-114: Reformat the complete AndroidManifest.xml with Spotless
using the Eclipse WTP formatter, converting XML indentation to tabs and line
endings to LF throughout the file, including the DeepLinkActivity intent-filter
block.
In `@app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt`:
- Around line 485-495: Handle SecurityException within the lifecycleScope
coroutine in MainActivity.kt lines 485-495 around handleDeepLinkRequest, and
apply the same change in EditorHandlerActivity.kt lines 1872-1895: rethrow
CancellationException, log other scan failures, and switch to the main thread to
show a user-visible error instead of allowing the coroutine to fail silently.
In `@app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt`:
- Around line 91-92: The parse logic in DeepLinkRequest.parse must locate line
and column keywords only after the file marker, rather than searching the full
segment list, so project or directory names matching keywords are not
misinterpreted; update the forward-only lookup in DeepLinkRequest.kt lines 91-92
while preserving valid deep-link parsing. Add regression cases in
DeepLinkRequestTest.kt lines 76-84 for /project/line/file/Main.kt,
/project/MyApp/file/line/Main.kt, and /project/file/file/Main.kt, asserting
lineRaw remains null and filePath excludes the project name.
In `@app/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.kt`:
- Around line 52-63: Update the coroutine launched in ProjectOpenBookkeeping
around RecentProjectRoomDatabase.getDatabase and recentProjectDao().insert to
catch recoverable Room/database exceptions locally, log them with SLF4J, and
preserve the in-memory project-open state when persistence fails. Ensure
CancellationException is rethrown rather than swallowed, while retaining the
existing project creation and insertion flow for successful operations.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt`:
- Around line 41-45: The null-request branch in DeepLinkActivity should provide
user-visible feedback before finishing, using the existing deep-link error
string from the strings resource. Update the request parsing failure path around
DeepLinkRequest.parse to show an appropriate toast or equivalent message, then
preserve the existing finish-and-return behavior.
In `@app/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt`:
- Around line 50-56: Update the InvalidPathException handling in the
path-resolution function to satisfy SwallowedException: either log the rejected
path at debug level using the project’s established SLF4J logger, or rename the
unused exception binding to “_” when silent rejection is intentional. Keep the
existing null return behavior.
- Around line 51-53: Update the path containment logic around baseDir and
relativePath to close the symlink gap: for existing paths, resolve both the base
directory and candidate through toRealPath() before comparing containment, while
preserving appropriate handling for nonexistent targets. If symlinks are
intentionally out of scope instead, document that limitation in the function’s
KDoc.
In `@app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt`:
- Around line 21-22: Replace raw JUnit assertions with Google Truth assertions
in DeepLinkRequestTest.kt (lines 21-22) and PathTraversalTest.kt (lines 20-21),
importing Truth’s assertThat and converting assertEquals/assertNull to
isEqualTo/isNull; retain RobolectricTestRunner in DeepLinkRequestTest.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f467961-aaec-4187-bbb1-dd4404cc9d29
📒 Files selected for processing (14)
.well-known/README.md.well-known/assetlinks.jsonARCHITECTURE.mdapp/src/main/AndroidManifest.xmlapp/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.ktapp/src/main/java/com/itsaky/androidide/activities/MainActivity.ktapp/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/deeplink/PendingDeepLinkOpen.ktapp/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.ktapp/src/main/java/com/itsaky/androidide/utils/PathTraversal.ktapp/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.ktapp/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.ktapp/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.ktresources/src/main/res/values/strings.xml
Route on ActionContextProvider.getActivity() (tracks the live EditorHandlerActivity instance) instead of IProjectManager's workspace, which stays null for the whole duration of a Gradle sync even while EditorActivityKt is already open -- a link tapped mid-sync was mis-routed to MainActivity instead of the running editor. Found in code review of PR 1651.
Only handle a deep-link request when savedInstanceState == null, and clear the DeepLinkRequest extra afterward, matching postProjectInit's existing "don't reapply on a later config-change recreate" guard. Without this, a font-scale/dark-mode/locale change or a process-death restore re-triggered handleDeepLinkRequest and redundantly relaunched EditorActivityKt. Found in code review of PR 1651.
…p link confirmProjectClose() now dismisses any dialog it previously showed before showing a new one. Without this, two deep links for different projects arriving in quick succession (onNewIntent can fire repeatedly on the singleTask editor activity) could stack two confirm-close dialogs; confirming either one overwrote the single PendingDeepLinkOpen.value, silently dropping whichever project the user actually confirmed opening. Found in code review of PR 1651.
Replace repeated whole-list segments.indexOf(keyword) lookups with a cursor-based forward scan (indexOfFrom). indexOf always returns the first occurrence in the entire path, so a project name that happened to equal "line"/"file"/"column" was mistaken for that keyword later in the path, corrupting the file/line/column split. The cursor-based scan only matches occurrences at or after the previously consumed segment, so an already-consumed segment can never be re-matched. Adds a regression test for a project literally named "line". Found in code review of PR 1651.
The existing guard only normalized the path lexically, so a symlink physically present inside the project directory (e.g. from a git clone, which supports symlinks) pointing outside it was never detected -- the OS would follow it at actual file-open time. Add a third layer mirroring AssetsInstallationHelper.extractZipToDir's zip-slip guard: resolve the nearest existing ancestor of the requested path to its real, on-disk path via toRealPath() and re-verify containment. Skipped when the base directory itself doesn't exist, since there's nothing on disk to symlink-escape through. Adds a regression test with a real symlink pointing outside the base directory, and a companion test that a plain file inside a real base directory still resolves. Found in code review of PR 1651.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt (1)
58-68: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle an unavailable route target locally.
If
startActivitythrowsActivityNotFoundException, log non-sensitive route metadata through SLF4J and callfinish()infinally. Otherwise, the exception skipsfinish()and reaches the global crash handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt` around lines 58 - 68, Update the startActivity flow in DeepLinkActivity to catch ActivityNotFoundException, log only non-sensitive route metadata through SLF4J, and ensure finish() executes in a finally block. Preserve the existing intent construction and successful launch behavior while preventing unavailable targets from reaching the global crash handler.Sources: Coding guidelines, Learnings
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (4)
1852-1852: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftKeep unsaved buffers open when saving fails.
The callback at Line 1852 closes the project after
saveAllAsync.saveAllAsyncalways invokes its callback at Lines 933-939, and afrag.save()failure can return normally. The deep-link handoff can therefore close editors with unsaved changes.Expose a real all-files-saved result, or check
hasUnsavedFiles()beforeperformCloseAllFiles. Keep the confirmation open and report the failure when any buffer remains modified. Do not usesaveAll'sgradleSavedBoolean as the overall save result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt` at line 1852, The save-completion flow around saveAllAsync must not close editors when any buffer remains unsaved. Track or derive a true all-files-saved result from the save operations, explicitly excluding saveAll’s gradleSaved Boolean, and only call performCloseAllFiles when hasUnsavedFiles() is false; otherwise keep the confirmation open and report the save failure.
1932-1934: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject directories before opening deep-link targets.
resolveWithinDirectoryreturns contained directories, andFile.exists()accepts them. Requirefile.isFilebeforeopenFileAndSelect; otherwiseCodeEditorViewentersfile.readContent(...)with a directory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt` around lines 1932 - 1934, Update the deep-link target validation around resolveWithinDirectory in EditorHandlerActivity to require file.isFile instead of only file.exists(). Preserve the existing not-found error path, and ensure directories are rejected before openFileAndSelect is invoked.
361-366: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
ActivityNotFoundExceptionaround theEditorActivityKtlaunch. Keep the pending request untilstartActivitysucceeds, and record project-open bookkeeping only after success. Log and report launch failures through the established observability path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt` around lines 361 - 366, Wrap the EditorActivityKt launch in the existing error-handling flow for ActivityNotFoundException, keeping pending until startActivity completes successfully. Move project-open bookkeeping and pending-request cleanup after the successful launch, and use the established logging and reporting path to record and surface launch failures.Sources: Coding guidelines, Learnings
1881-1883: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle project-discovery failures locally.
listFiles()?.orEmpty()handles null results, butFilechecks can throwSecurityException. Catch and report this failure, rethrowCancellationException, and show a dedicated deep-link error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt` around lines 1881 - 1883, Update the project-discovery coroutine around findValidProjects in EditorHandlerActivity so File-related SecurityException failures are caught locally and reported, while CancellationException is rethrown unchanged. On discovery failure, show the dedicated deep-link error instead of continuing to the normal project-opening flow.Source: Coding guidelines
🧹 Nitpick comments (1)
app/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt (1)
22-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse framework-compatible test runners and Truth assertions.
- Keep
DeepLinkRequestTeston JUnit 4 withRobolectricTestRunner; Robolectric 4.11.1 does not support Jupiter. Replaceorg.junit.Assertcalls with Truth assertions.- Migrate
PathTraversalTestto Jupiter and@TempDironly after configuring the app to run Jupiter alongside existing JUnit 4 tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt` around lines 22 - 34, Configure the app test setup to run Jupiter alongside existing JUnit 4 tests, then migrate PathTraversalTest from JUnit 4 TemporaryFolder to Jupiter with `@TempDir`. Keep DeepLinkRequestTest on JUnit 4 with RobolectricTestRunner, and replace its org.junit.Assert calls with Truth assertions; apply the changes in app/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt (lines 22-34) and app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt (lines 86-99).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt`:
- Around line 1818-1827: Serialize deep-link handling in the flow around
confirmProjectClose and its onNewIntent callers: track the latest request using
a generation or job so stale project lookups cannot replace newer dialogs, and
add close-in-progress state to prevent another request from starting while
save-and-close is active. Ignore or queue incoming requests until the current
close callback completes, ensuring performCloseAllFiles runs only once and the
latest valid request is handled.
In `@app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt`:
- Around line 86-99: Update the deep-link parser used by parse so line and
column markers are identified unambiguously rather than treating the first
matching segment after file as metadata, preserving reserved keywords within
file paths. Define the position parsing contract, apply it to the file-path
extraction logic, and add regression tests covering both line and column
segments embedded in file paths.
---
Outside diff comments:
In `@app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt`:
- Around line 58-68: Update the startActivity flow in DeepLinkActivity to catch
ActivityNotFoundException, log only non-sensitive route metadata through SLF4J,
and ensure finish() executes in a finally block. Preserve the existing intent
construction and successful launch behavior while preventing unavailable targets
from reaching the global crash handler.
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt`:
- Line 1852: The save-completion flow around saveAllAsync must not close editors
when any buffer remains unsaved. Track or derive a true all-files-saved result
from the save operations, explicitly excluding saveAll’s gradleSaved Boolean,
and only call performCloseAllFiles when hasUnsavedFiles() is false; otherwise
keep the confirmation open and report the save failure.
- Around line 1932-1934: Update the deep-link target validation around
resolveWithinDirectory in EditorHandlerActivity to require file.isFile instead
of only file.exists(). Preserve the existing not-found error path, and ensure
directories are rejected before openFileAndSelect is invoked.
- Around line 361-366: Wrap the EditorActivityKt launch in the existing
error-handling flow for ActivityNotFoundException, keeping pending until
startActivity completes successfully. Move project-open bookkeeping and
pending-request cleanup after the successful launch, and use the established
logging and reporting path to record and surface launch failures.
- Around line 1881-1883: Update the project-discovery coroutine around
findValidProjects in EditorHandlerActivity so File-related SecurityException
failures are caught locally and reported, while CancellationException is
rethrown unchanged. On discovery failure, show the dedicated deep-link error
instead of continuing to the normal project-opening flow.
---
Nitpick comments:
In `@app/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt`:
- Around line 22-34: Configure the app test setup to run Jupiter alongside
existing JUnit 4 tests, then migrate PathTraversalTest from JUnit 4
TemporaryFolder to Jupiter with `@TempDir`. Keep DeepLinkRequestTest on JUnit 4
with RobolectricTestRunner, and replace its org.junit.Assert calls with Truth
assertions; apply the changes in
app/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt (lines 22-34)
and app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt (lines
86-99).
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c28bc8d6-72f2-4c4b-a82a-d3a92f91607d
📒 Files selected for processing (7)
app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.ktapp/src/main/java/com/itsaky/androidide/activities/MainActivity.ktapp/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.ktapp/src/main/java/com/itsaky/androidide/utils/PathTraversal.ktapp/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.ktapp/src/test/java/com/itsaky/androidide/utils/PathTraversalTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt
- app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt
The doc still described the routing check as IProjectManager.getInstance().workspace, which the prior commit in this branch replaced with ActionContextProvider.getActivity() (see "Fix deep-link routing race in DeepLinkActivity").
recordProjectOpenedBookkeeping() called RecentProjectRoomDatabase.getDatabase(context, scope) directly instead of the RecentProjectDao already wired into Koin's coreModule (the same instance MainViewModel/RecentProjectsViewModel inject) -- a second, DI-bypassing acquisition path for the same singleton database, against ADR 0001/0006's "persistence is provided through Koin". recordProjectOpenedBookkeeping() now takes a RecentProjectDao parameter; both call sites (MainActivity, EditorHandlerActivity) inject it the same way they already inject analyticsManager. Found in architecture review of PR 1651.
DeepLinkActivity silently finished on an unparseable URI with no feedback to the user. Uses a Toast rather than the existing flashError helper -- this activity finishes immediately after, tearing down its window before a view-based Flashbar could ever render. Also adds msg_deeplink_scan_failed, used by the next commit. Addressed from inline PR review comments.
findValidProjects() can throw SecurityException (e.g. a storage permission revoked mid-session) inside the IO coroutine launched by MainActivity.handleDeepLinkRequest and EditorHandlerActivity.onNewIntent. Uncaught, that would crash the coroutine's scope instead of just failing this one deep link. CancellationException is rethrown; other failures are logged and reported to the user on the main thread. Addressed from inline PR review comments.
recordProjectOpenedBookkeeping()'s recentProjectDao.insert() ran with no error handling on ProcessLifecycleOwner's app-wide scope -- a transient Room/SQLite failure would crash the whole process instead of just failing to record one Recents entry. CancellationException is rethrown; other failures are logged. The in-memory project-open state (ProjectManagerImpl.projectPath, GeneralPreferences.lastOpenedProject) is set synchronously before the coroutine launches, so it's unaffected either way. Addressed from inline PR review comments.
resolveWithinDirectory()'s InvalidPathException/IOException catches intentionally discard the exception (the caller only needs null-or-not for attacker-controllable input) -- name the bindings "_" rather than "e" to make that explicit instead of reading as an accidentally swallowed exception. Addressed from inline PR review comments.
Two more cases for the indexOfFrom cursor-scan fix (045aa00): a project named "line" with no line suffix, and a project named "file". Both already passed before this commit -- this only adds coverage. A third proposed case, a project's file *path* itself starting with a segment literally named "line" (e.g. .../file/line/Main.kt), is not addressable by any segment-based fix: with no delimiter between the optional line/column suffix and the preceding filename, "the file path happens to start with 'line'" and "there's a real line/{n} suffix" are the same shape at the segment level. Not tested here -- a real fix would need a schema change (e.g. line/column as query parameters). Addressed from inline PR review comments.
Three related fixes in EditorHandlerActivity, all in the deep-link close-then-reopen path: - confirmProjectClose(): a generation token now guards the "Save and close" async callback. saveAllAsync completes asynchronously, so an older deep-link request's callback could still fire (contentOrNull stays non-null until onStop()/onDestroy(), well after finish()) after a newer request's dialog was already answered, overwriting PendingDeepLinkOpen.value with the superseded project. Only the request owning the current token is allowed to act. - Same callback no longer closes files unconditionally after "Save and close": saveAll()'s return value is gradleSaved (whether a build file changed), not "everything saved successfully". Now checks hasUnsavedFiles() and reports a failure instead of silently discarding unsaved changes on a failed write. - applyDeepLinkFileRequest(): require file.isFile, not just file.exists() -- a deep link resolving to an existing directory was passed straight to openFileAndSelect(). Addressed from inline PR review comments.
The previous fix (045aa00) searched for the line/column keywords forward from just after `file`, which still mismatched a file path that legitimately contains "line" or "column" as an early segment (e.g. a directory named "line") when a real trailing line/{n} suffix also follows it -- the forward search would still latch onto the first, coincidental occurrence. line/column are trailing modifiers, so match them from the end of the path backward instead: check for "column" immediately before the last segment, then "line" in whatever remains. This correctly keeps an early, coincidental "line"/"column" segment as part of the filename as long as a real trailing pair follows it. The one shape still unresolvable: a file path whose entire content is just the keyword plus one segment, with nothing else following (e.g. `file/line/Main.kt` alone) -- indistinguishable from a real line suffix with no delimiter in this URL scheme; documented as a known limitation with a locked-in test rather than silently misbehaving. Addressed from inline PR review comments.
…file Adds regression tests for the end-anchored line/column matching (df705c9): a file path segment literally named "line" or "column" is now preserved when a real trailing line/column suffix follows it, plus a test locking in the one remaining unresolvable shape (documented in the previous commit) so a future change doesn't alter it silently. Also converts this file's assertions from raw JUnit to Google Truth, per ARCHITECTURE.md's testing guidelines -- Truth is already available to :app's test source set transitively via testing:unit, so this is a same-file, no-build-config-change cleanup. Addressed from inline PR review comments.
…ight The generation-token fix (a451470) stops a stale "Save and close" completion from overwriting PendingDeepLinkOpen, but doesn't stop a second request from doing real damage while the first is still running: saveAllAsync iterates and mutates editorViewModel's file/editor state on a background coroutine, and "Close without saving" calls performCloseAllFiles synchronously on the main thread against that same state -- a second deep link answered with "Close without saving" while an earlier one's save is still in flight would race that save. confirmProjectClose() now drops a new request outright while closeInProgress is true (set for the duration of the async save), rather than showing a dialog whose buttons could trigger a concurrent mutation. This also protects the ordinary manual "close project" path against racing a deep-link-triggered save. Addressed from inline PR review comments.
…eepLinkOpen
Two small cleanups deferred from the original code review:
- MainViewModel.saveProjectToRecents() has had zero callers since the
deep-link work replaced it with recordProjectOpenedBookkeeping() --
delete it along with the now-unused RecentProjectDao constructor
parameter it existed only to serve.
- PendingDeepLinkOpen was a hand-rolled Kotlin `object` singleton,
against ADR 0006 ("no hand-rolled singletons -- prefer Koin"). Now a
Koin-provided `single`, injected into EditorHandlerActivity the same
way as analyticsManager/recentProjectDao. Same one-process-wide
instance either way; this just keeps it substitutable in tests and
out of the pattern the ADR asks new code to avoid.
AppModule.kt's diff also reformats the whole file to tabs -- it wasn't
previously tab-indented, and editing it at all pulls the whole file
under the Spotless ratchet (file-level, not line-level).
Addressed from deferred code-review findings.
…anning all
MainActivity.handleDeepLinkRequest and EditorHandlerActivity.onNewIntent
both did findValidProjects(PROJECTS_DIR).find { it.name == name } --
duplicated across both call sites, and findValidProjects itself
validates every project under PROJECTS_DIR just to find one by a
known name.
Adds findValidProjectByName(), the O(1) counterpart to
findValidProjects() for a caller that already knows the exact name,
and uses it at both call sites -- deduplicating the expression and
skipping the full-directory scan.
Addressed from deferred code-review findings.
applyDeepLinkFileRequest() had two copy-pasted 8-line blocks for line/column parsing, differing only in the target var, the error string resource, and which PendingFileRequest field was read. Collapsed into one zeroBasedOrFlashError() helper. Also folds in a stray PendingDeepLinkOpen.value -> pendingDeepLinkOpen rename left over from 9741df7's Koin conversion. Addressed from deferred code-review findings.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd KDoc for
MainViewModel.Document its screen-state contract, LiveData threading expectations, and clone-request event behavior.
As per coding guidelines, "Public classes, functions, and non-obvious logic must have KDoc or Javadoc documenting contracts, rationale, threading, nullability, side effects, or units."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt` at line 37, Add KDoc to the public MainViewModel class documenting its screen-state contract, LiveData threading expectations, and clone-request event behavior, including relevant nullability and side effects where applicable.Source: Coding guidelines
app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse JUnit Jupiter for this new Robolectric test class.
@RunWith(RobolectricTestRunner::class)runs this class through JUnit 4. Migrate the test to the project's JUnit Jupiter and Robolectric integration.As per coding guidelines, "Use JUnit Jupiter, Truth, MockK for new tests, Mockito-Kotlin where legacy conventions require it, and Robolectric for framework-dependent JVM tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt` around lines 26 - 28, Migrate DeepLinkRequestTest from JUnit 4 to JUnit Jupiter while preserving its Robolectric execution through the project’s Jupiter/Robolectric integration. Remove the RunWith-based JUnit 4 setup and use the appropriate Jupiter-compatible annotation or configuration already established in the test suite; keep the parse helper and test behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.kt`:
- Around line 68-76: In the Recents insert handling around
recentProjectDao.insert, replace the broad Exception catch with
android.database.SQLException or the narrowest applicable SQLite exception,
while preserving the existing CancellationException rethrow and warning log
behavior.
In `@app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt`:
- Around line 33-35: Update the project-candidate validation around
isProjectCandidateDir and isValidProjectDirectory to canonicalize both
projectsRoot and the candidate path, then accept the candidate only when its
canonical parent is exactly the canonical root, preventing traversal and symlink
escapes. Preserve the existing project-directory validation and add regression
tests covering .. traversal and symlinked paths outside the configured root.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt`:
- Line 37: Add KDoc to the public MainViewModel class documenting its
screen-state contract, LiveData threading expectations, and clone-request event
behavior, including relevant nullability and side effects where applicable.
In `@app/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.kt`:
- Around line 26-28: Migrate DeepLinkRequestTest from JUnit 4 to JUnit Jupiter
while preserving its Robolectric execution through the project’s
Jupiter/Robolectric integration. Remove the RunWith-based JUnit 4 setup and use
the appropriate Jupiter-compatible annotation or configuration already
established in the test suite; keep the parse helper and test behavior
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1342e9da-8f2b-420f-bb5a-36a795af02d6
📒 Files selected for processing (12)
app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.ktapp/src/main/java/com/itsaky/androidide/activities/MainActivity.ktapp/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.ktapp/src/main/java/com/itsaky/androidide/deeplink/PendingDeepLinkOpen.ktapp/src/main/java/com/itsaky/androidide/di/AppModule.ktapp/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.ktapp/src/main/java/com/itsaky/androidide/utils/PathTraversal.ktapp/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.ktapp/src/main/java/com/itsaky/androidide/utils/ProjectValidations.ktapp/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.ktapp/src/test/java/com/itsaky/androidide/models/DeepLinkRequestTest.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/java/com/itsaky/androidide/deeplink/PendingDeepLinkOpen.kt
- app/src/main/java/com/itsaky/androidide/activities/DeepLinkActivity.kt
- resources/src/main/res/values/strings.xml
- app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt
- app/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt
- app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt
- app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt
…links # Conflicts: # app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt
…links Resolves the collision with ADFA-5257 (#1736), which centralized the path-containment check this branch had carried as its own copy: - AssetsInstallationHelper.kt, ZipUtils.kt, ZipUtilsTest.kt: took stage's versions outright. Each conflict was this branch's "keep the three copies in sync" doc or its interim guard, which #1736 superseded with the shared ContainedPathResolver (a strictly stronger check, and stage's ZipUtilsTest contains both tests this branch had added). - Deleted app/.../utils/PathTraversal.kt and its PathTraversalTest: #1736 landed resolveWithinDirectory in common -- same package, name, and signature, documented there as awaiting this PR -- so the app-module copy would otherwise make every call site ambiguous. Deep-link call sites (EditorHandlerActivity, ProjectValidations) resolve to common's unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
The recurring shape is partial application: the right thing done at one site and not at its siblings. - postDestroy() lacked the didCompleteLiveOnCreate guard that preDestroy() has in three places, so an instance whose onCreate took the deep-link bail still ran Lookup.unregisterAll() and cleared three registries process-wide, out from under a live sibling. It matters more here than in preDestroy, because everything in it is process-wide rather than per-instance. - SetupState handled the Environment.init() race for PREFIX and ANDROID_HOME but not PROJECTS_DIR, which is passed to a non-null Kotlin parameter. Added projectsRoot() alongside the other two; both call sites use it. - The git sheet gated on areFilesModified() while this branch introduced hasFilesThatFailedToSave() for exactly that check. The cached flag counts read-only archive tabs no save ever writes, so with a .zip open every commit and pull was refused with no way to clear it. Promoted to IEditorHandler as hasUnsavedWritableFiles(). - One save-failure site used the String overload for its indefinite duration and explained why; the third still used the ~1s toast. State that was never rolled back: - MainActivity.openProject writes projectPath, lastOpenedProject, Recents and analytics before the confirm dialog is answered. Declining left the app pointing at the refused project: deep-link file navigation resolved inside it and the next cold start reopened it. restoreIntentToStayingProject now rolls both globals back, and uses a snapshot of the staying project rather than the global openProject already overwrote. - The deep-link resolve's not-found path stranded the captured file request, so the staying project's pending navigation was lost and every later switch skipped its own capture. - Bookkeeping ran twice per plain switch. DeepLinkOpenRequest carries whether it already ran. Smaller: an empty filePath (`.../file/line/5`) became a file request for the empty string and surfaced as `File "" was not found`; a failed resolve or a declined dialog left the request unconsumed so it re-fired on every recreate; DeepLinkActivity's setup-incomplete branch omitted the FLAG_ACTIVITY_NEW_TASK its sibling two blocks below has. Sweeps found more than the review reported. Range.pointRange aliased one mutable Position as both ends, so EditorFeatures.validateRange's two clamps moved each other -- fixed at the factory, which fixes every caller, and EditorProviderImpl had the same open-coded Range(pos, pos). The missing GPL header was on two new files. The "PROJECT_PATH" literals lived in a fourth file beyond the three reported; all 13 now use EditorIntentExtras. Verified on an Android 13 arm64 device (SM-N986U), not only by reading: - The re-fire fix is proven by revert check. Unfixed, `No project named "NoSuchProject" was found.` returns after a font-scale recreate and on every one after; fixed, it does not. - Two findings did NOT reproduce, and the fixes are kept only as cheap hardening. The PROJECTS_DIR race never fired: the unfixed build survived 6 of 6 cold-start deep links. And the background-activity-start hazard cannot occur on this path at all -- dumpsys shows the editor is never its task root (MainActivity is always at Hist #0 beneath it, and EditorActivityKt is not exported), so finishing it never empties the task. A recovery path written for that case was removed again rather than left defending an unreachable state; the reasoning is recorded where it was. - Regression-checked end to end: onboarding runs clean from a deep link on an unconfigured install, and a Beepy -> Aegis1 switch confirms and opens with no dropped start and no fatal exceptions. Both screens render correctly at font scale 2.0. Two tests changed. DeepLinkRequestTest locked in the empty-filePath behaviour that was itself the defect; it now pins the corrected outcome, plus the two shapes the review named. SameProjectDeepLinkMidSyncTest reflects on switchToProject, whose arity changed, and was additionally crashing on "KoinApplication has not been started" -- it now starts Koin. That test still fails, and did so before this change: with _binding assigned, contentOrNull is null, so switchToProject never reaches the same-project branch the test exists to pin. Left failing rather than weakened into passing. Not addressed: XMLFormatterDocumentNew.java open-codes the same Range aliasing, but belongs to the XML formatter and is untouched by this branch.
Nothing here is behavioural. Editing one line of this file for ADFA-5067 (the
pointRange aliasing fix in the preceding commit) enrolled it in the
ratchetFrom = origin/stage ratchet, which reformats every differing file in
full, so ktlint rewrote all 191 lines of it.
What it did: added trailing commas, split parameter lists one-per-line, dropped
the blank line after class and companion-object openings, and converted five
single-return block bodies to expression bodies (pointRange(line, column),
containsLine, containsColumn, isSmallerThan, toString). Plus a missing newline
at end of file.
Verified inert: stripping all whitespace and commas from both versions leaves
two token streams differing only by those five `{ return x }` -> `= x`
conversions, which Kotlin treats identically.
Kept out of the behavioural commit so that commit's diff is the ten lines it
actually changes rather than a 191-line whitespace wall.
jatezzz
reviewed
Sep 1, 2026
jatezzz
reviewed
Sep 1, 2026
jatezzz
reviewed
Sep 1, 2026
jatezzz
reviewed
Sep 1, 2026
jatezzz
reviewed
Sep 1, 2026
jatezzz
reviewed
Sep 1, 2026
jatezzz
reviewed
Sep 1, 2026
Two medium:
- Consumption of the deep-link extras on the editor side now survives
process death: intent.removeExtra only mutates this process's Intent,
while a recreate is handed the parceled copy with the extras intact,
re-firing the navigation with no user action. BaseEditorActivity now
keeps consumedDeepLinkRequests/consumedFileRequests (persisted via
onSaveInstanceState, mirroring MainActivity's existing marker) and
gates onCreate's DeepLinkRequest read and postProjectInit's
PendingFileRequest read on them. ConsumedDeepLinkRequests is
generified to ConsumedRequests<T> so both extra types share the
bookkeeping, with remove() so a deliberate re-arm of an equal-by-value
request is not skipped as already consumed.
- onDestroy's pendingCloseCallback drain is now additionally gated on
closeDialogAnswered, set only by the confirm-close dialog's two
confirm buttons: isFinishing alone is also true when the task is
swiped out of Recents while the dialog is still up, and the armed
callback then performed a project switch the user never confirmed.
Five low:
- MainActivity records the deep-link request each open/confirm dialog
was actually raised for (threaded through handleOpenProject/
askProjectOpenPermission), not latestDeepLinkRequest at answer time,
which a second link can have overwritten.
- switchToProject's close-in-progress branch no longer drains a
still-valid older pending request for the staying project; it drops
only the new request's own copy of the extra.
- ProjectHandlerActivity.initializeProject's two init-failure early
returns (no build service, tooling server down) now drain the pending
file request, matching postProjectInit's regardless-of-outcome drain
they bypass.
- DeepLinkRequest.parse rejects paths with an empty segment ("//"),
which Uri.pathSegments silently drops and which made
".../project//file/Main.kt" parse as a project named "file".
- The isFinishing/isDestroyed liveness filter is scoped to a new
ActionContextProvider.getLiveActivity() used by DeepLinkActivity's
routing; getActivity() keeps its pre-existing semantics for the
floating editor panel and IDEApiFacade.runApp.
Conflict in EditorHandlerActivity: stage's ADFA-4501 replaced performCloseAllFiles(manualFinish, onClosed) with a parameterless version plus closeProject(saveFloatingFiles), which defers finish() until the floating tabs are closed. Kept this branch's confirm-close flow (pendingCloseCallback and the deep-link switch handoff) and adopted stage's shape by threading onClosed through closeProject.
Builds on c371122, which covered the process-death consumption markers, the closeDialogAnswered gate, the per-dialog request threading, the close-in-progress drain and the empty-segment parse. These are the ones left, plus two places the previous round got wrong. Corrections to my own earlier work: - The Range aliasing fix guarded the wrong call. openFileAndSelect copies the selection for its own postInLifecycle validate, but passed the ORIGINAL to openFile, whose CodeEditorView constructor pipeline calls validate() and validateRange() on it -- both clamping start/end in place on the caller's object. IDEEditor.showDocument routes an LSP ShowDocumentParams Range through IDELanguageClientImpl.openFileAndSelect, so this mutated the language server's own Location. Copy at that call too. - Relatedly, pointRange's comment claimed a bug that does not exist: for a POINT range the two ends hold equal values and the column clamp depends only on the already-clamped line, so aliasing them is idempotent today. The copies stay -- that is a property of the current clamp, not of the type -- but the comment now says so instead of asserting corruption that cannot happen. - GitBottomSheetFragment was half-migrated: the post-save check moved to hasUnsavedWritableFiles() while the check that RAISES the prompt kept the cached areFilesModified(). With an archive tab open that prompted "save before the git action?" on every commit, pull and push with nothing dirty, offering a save that could not clear it. Both gates now ask the same question. - drainPendingDeepLinkOpen in onDestroy was the sibling the didCompleteLiveOnCreate sweep missed, and the one that matters most: pendingDeepLinkOpen is a Koin single, more widely shared than any registry the three guarded hooks protect. An instance that took onCreate's deepLinkTargetsAnotherProject bail could drain a handoff a different, still-live instance had armed. The rest: - performPendingDeepLinkOpen omitted EXTRA_PREVIOUS_PROJECT_PATH while calling recordProjectOpenedBookkeeping, which overwrites the global the receiver falls back to. A delivery landing on onNewIntent computed previousProjectPath == newProjectPath, so a confirmed switch silently no-opped with the global naming one project and the editor showing another. - The "Save and close" completion treated isDestroyed as teardown. It is also true for a config-change recreate, where a successor for the same project is already coming up -- and this continuation now survives one at all because the PR moved saveAllAsync onto the process-wide appScope. Running the callback there armed a switch on behalf of a replaced instance and draining it fired startActivity while the successor was on screen. Config recreate is now its own branch, left for the successor to own. - deepLinkTargetsAnotherProject and isProjectSwitchIntent compared directory leaf names, but a deep link can only resolve to <projectsRoot>/<name> while projects open from anywhere (file picker, Recents, clone destination). With an unrelated MyApp open from Download/work, the link's file path resolved against the OPEN project; for two clones of a repo the path exists in both, so the wrong file opened silently. Both sites now go through isDeepLinkTargetOfOpenProject, which requires the parent to be the projects root, canonicalised. - MainActivity.onNewIntent had no consumed-requests check, the gate onCreate applies. BaseEditorActivity re-forwards a request here on any project mismatch, so a declined request came back through that door and re-showed its dialog. - The resolve-failure restore ran unguarded: with nothing captured, restoreIntentToStayingProject's restore == null arm deletes a carried-forward request belonging to the staying project. It also lacked the supersession check its two siblings carry. Both added. - DeepLinkRequest.parse now bounds the path length. DeepLinkActivity is exported, the parsed strings are parcelled into ConsumedRequests and written to MainActivity's saved-instance Bundle, so unbounded names crossed the ~1 MB Binder budget and crashed the activity on every rotation until the task was cleared. - DeepLinkActivity gains taskAffinity="". excludeFromRecents applies to a task via its ROOT activity, and this trampoline shared the app's affinity, so a cold start rooted the real task here and left the whole IDE session with no Recents card. SameProjectDeepLinkMidSyncTest passes, and for the first time tests what it names. It was never reaching its branch: view binding generates `content` as a public Java FIELD, and mockk stubs methods rather than fields, so a relaxed mock left it null, contentOrNull returned null and switchToProject took the binding-torn-down path. The field is now set by reflection. Its Koin setup also joins an already-started context instead of calling startKoin unconditionally, which threw KoinApplicationAlreadyStartedException in the full suite while passing in isolation. :app:testV8DebugUnitTest --rerun-tasks: 420 tests, 0 failures.
The sentinel corruption is the one that mattered: Range.NONE is a process-wide @JvmField whose two ends are the single Position.NONE instance, and EditorFeatures.validateRange assigns line/column straight onto whatever objects it is handed. This class hands the sentinels out itself as `?: Range.NONE` / `?: Position.NONE` defaults, so clamping one rewrote Position.NONE from (-1, -1) to real coordinates for the rest of the process -- and since Position has structural equals, every later "nothing found" check against it (GoToDefinition, FindUsages, OrganizeImports, CodeFormatProvider) silently stopped matching. The previous round defended three call sites and left the sentinel itself, which is the thing every one of those paths funnels into. Guarded at the mutators instead: validateRange, Range.validate and Position.zeroIfNegative all no-op on the sentinels. Not by handing out fresh instances -- JavaCompilerService and CodeFormatProvider compare with reference `==`, so that would have broken those checks silently. The other three of my own: - The Range defensive copy reached two of the three entry points IEditorHandler advertises. openFileAndGetIndex still passed the caller's own object to CodeEditorView, whose pipeline clamps it in place -- and dereferenced a declared-nullable parameter with `!!`, so any caller honouring that nullability got a KotlinNullPointerException. - EXTRA_PREVIOUS_PROJECT_PATH did not work on the path it was added for. It read the live global, which MainActivity.openProject has already overwritten on the plain-switch path, so previous == new and the receiver still saw a same-project no-op. The staying path now travels on DeepLinkOpenRequest, which is where switchToProject already had the correct value. - findValidProjects(Environment.PROJECTS_DIR) was an unswept sibling of the NPE projectsRoot() exists to prevent, in the same file as a projectsRoot() call, and the one such site not wrapped in try/catch. RecentProjectsFragment's identical call is guarded and is left alone. - The onNewIntent consumed gate I added last round was itself a regression: it dedups by value and DeepLinkRequest carries no nonce, so deliberately re-tapping a URL was silently dropped -- permanently, for a link naming a project that did not exist when first tapped. The gate now applies only to BaseEditorActivity's programmatic re-forward, which is marked as such. The rest: - ConsumedRequests evicted the first-inserted entry, which is by construction the request on the task's launch Intent -- the one entry the class exists to remember, since that Intent is what Android replays after process death. Losing it force-reopened its project over whatever the user was doing. The launch entry is pinned, eviction takes the second-oldest, and a re-add now refreshes position so "oldest" tracks use. Its test asserted the old behaviour and is updated, with a second case for the refresh. - restoreIntentToStayingProject's capture guard existed at one of three call sites; the other two are reachable with no capture, because onNewIntent and switchToProject decide "is this a switch?" with predicates that disagree whenever a path reaches the same directory by a different string. Moved into the function, which is what keeps the three from drifting again. - onGradleBuildServiceConnected's build-in-progress return was a third un-swept early return that never reaches postProjectInit's drain, so a deep-link file request opened during a running build stayed armed and fired on some later unrelated sync. - A superseded confirm-open dialog was closed with dismiss(), which fires no listener, so its request was never recorded and a later recreate re-raised it. The dismiss in onDestroy deliberately still does not record: a config-change successor should re-raise a dialog nobody answered. - MAX_LINK_PATH_LENGTH was not a bound. 32 entries x 4096 chars x UTF-16 across three saved sets is ~768 KB against a ~1 MB Binder limit; 512 makes it ~96 KB, still far above any nameable project. - AppModule's single<CoroutineScope> was unqualified and shared between the save scope and the Room database, so a second unqualified one would silently retarget the save. Named now. - SetupState documents that answering true bypasses Splash's storage and x86 checks and Onboarding's device checks entirely, so a check added there is not applied to links unless it is added here too. Known and deliberately not fixed here, because each wants a design decision rather than another point fix: - saveAllAsync on the app scope retains the activity, its binding and every editor buffer for the save's duration. The narrow fix is to move only the ~200-byte pendingDeepLinkOpen arming off the activity and leave the save on lifecycleScope -- a change to this method's contract with its callers. Documented at the call. - Three findings cluster on config-change recreate while the confirm-close dialog is up: the editor consumes the request before it is answered, the successor can commit a switch nobody confirmed via the already-mutated global, and closeProject arms the handoff before a finish() the recreate cancels. They share one cause -- per-instance switch state that does not survive a recreate -- and want the state machine looked at as a whole. :app:testV8DebugUnitTest --rerun-tasks: 421 tests, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvLHuVNpCQLQhcnNiCSbxw
ConsumedRequests protected the request on the task's launch Intent -- the one Android replays verbatim after process death -- by having the eviction loop skip slot 0. But add() unconditionally removed and re-appended every request it saw so that "oldest" tracked use rather than first sighting, which slid the launch entry off slot 0 as soon as it was re-added while anything else was in the set. Its protection then covered whichever unrelated request had taken its place, and a sender firing links in a loop -- DeepLinkActivity is exported, which is the threat model this cap exists for -- could evict the launch entry and force its project open over whatever the user was doing on the next process-death recreate. Hold the pin in a field instead. add() skips the recency re-insert for the pinned entry, evictExcess() chooses the oldest victim that is not pinned, and remove() releases the pin so the next add() re-establishes it. restore() also re-applies MAX_REMEMBERED, which it did not before: it addAll()'d whatever it was handed, so an over-long list came back oversized and went straight into the next onSaveInstanceState, defeating the Bundle bound the cap is for. The three new pin tests each need a second entry present before the launch entry is re-added. Written without one they passed against the unfixed code -- with the set holding nothing else, remove-then-append puts the entry straight back on slot 0 and the positional pin still covered it. Claude-Session: https://claude.ai/code/session_01LuyR4ajssmKw1o1UQsjJbP
Four defects that all trace back to the same shift: saveAllAsync moved to the process-wide appScope so its continuation outlives the activity, while the close/switch decision state stayed per-instance and the hand-off was only armed at onDestroy. The continuation reliably survived into a world where nothing could read what it had decided. EditorActivityKt does not declare uiMode, locale, density, keyboard or navigation in configChanges, so an automatic dark-mode switch recreates it mid-dialog and reaches all of this. Fail-open unsaved-file guard. getEditorForFile() resolves through contentOrNull and returns null for EVERY file once the binding is gone, so hasFilesThatFailedToSave() reported "nothing failed" for a whole set of genuinely modified buffers. Callers use it to decide whether it is safe to close, discard or commit -- GitBottomSheetFragment gates commit/pull/push on it -- so an unknowable answer must read as unsafe. Falls back to the retained ViewModel's areFilesModified, which is only ever recomputed while the binding is alive and so holds the last state actually observed. The ViewModel-backed check this replaced failed closed; this one had inverted that. Hand-off stranded by a cancelled finish(). closeProject() armed the process-wide PendingDeepLinkOpen and then deferred its finish() into a lifecycleScope coroutine that ON_DESTROY cancels. A destroy that beat the finish() left isFinishing false, so onDestroy skipped its drain and a confirmed switch sat in a Koin single until it fired against some later, unrelated project close. Replace the isFinishing/didCompleteLiveOnCreate pair -- two different questions, both wrong at the edges -- with ownership: arm(owner) / drainArmedBy(owner), so an instance performs its own hand-off and only its own. didCompleteLiveOnCreate was standing in for "did I arm this?", which is now asked directly. Switch committed mid-save. closeDialogAnswered went true the instant "Save and close" was tapped, so a finish() arriving while files were still being written let onDestroy invoke the close callback and skip the !saveSucceeded || hasFilesThatFailedToSave() abort entirely -- abandoning the user's edits with no message. Add closeCommitted, true only once the close is actually being performed, and decide the save outcome first, before any teardown branching. That check is only answerable during teardown because of the guard fix above. Switch lost on a config-change recreate. The !isFinishing && isDestroyed branch returned and left the close callback "for the successor instance", but nothing handed it over: pendingCloseCallback is per-instance, the hand-off was never armed, and onNewIntent had already recorded the request consumed and stripped it from the intent, with the consumed mark persisted. The successor could not learn a switch had been confirmed and a re-tap was gated out as value-equal, so the switch was lost permanently and silently. Arm and drain in place instead; the successor may show the old project briefly before the new one replaces it, which beats dropping a switch the user confirmed. Un-confirmed project persisted across a recreate. onDestroy dismissed the confirm-close dialog, and dismiss() dispatches neither the negative button nor the OnCancelListener, so a dialog still on screen at destroy time died with its decline handling never run -- leaving the intent and the process-wide bookkeeping pointing at a switch the user never confirmed. cancel() would dispatch it, but cancelOrDecline() can recursively show a fresh dialog for a superseding request, which must not happen from onDestroy; declineInFlightProjectClose() is its rollback half. That alone was not enough. onSaveInstanceState runs BEFORE onDestroy and wrote KEY_PROJECT_PATH from the live IProjectManager global, which MainActivity's bookkeeping has already moved to the INCOMING project before the intent is even delivered -- so the Bundle named the project the user had not agreed to open, and the successor loaded it against a retained ViewModel still holding the previous project's tabs. The saved path now comes from an open projectPathForInstanceState, which EditorHandlerActivity overrides with the staying-project snapshot it already keeps for the decline path. Claude-Session: https://claude.ai/code/session_01LuyR4ajssmKw1o1UQsjJbP
…resolve Two ways a deep link could go silently dead forever, both in the consumption bookkeeping. A transient filesystem failure was recorded as "no such project". resolveWithinDirectory maps everything that is not Contained to null, so ContainedPathResolver's Resolution.Unverifiable -- an IOException the resolver models explicitly as "not an escape, refused because unproven" -- arrived at findValidProjectByName indistinguishable from a genuine miss. An EACCES right after a storage-permission change, or an EIO on a flaky SD/FUSE mount, therefore told the user "No project named X was found" about a project that plainly exists, and MainActivity then recorded the request consumed on the stated reasoning that the project does not exist -- so the identical URL was a silent no-op on every later delivery. The SecurityException path had the same problem. Add lookupValidProjectByName, returning Found/NotFound/Unverifiable, and have resolveDeepLinkProject return the matching tri-state instead of File?. Only a definitive NotFound is recorded consumed. Unverifiable now reports the scan-failed message rather than "no project named X": telling someone a project they can see in the projects list does not exist is worse than admitting the lookup failed. An Unverifiable from one Unicode normal form does not mask a Found from another -- it is remembered and only returned if no candidate form resolves. findValidProjectByName stays, reduced to a null-or-directory view for the callers that cannot act on the difference. A fresh tap was mistaken for a programmatic re-delivery. The re-forward gate dropped any request already in consumedDeepLinkRequests. It exists to stop a bounce loop -- MainActivity opens a project, the editor decides the link names a different one and bounces it back, and the dialog goes straight back up -- but it could not tell that loop from a genuinely new tap that happens to be re-forwarded, and DeepLinkRequest carries no nonce, so a repeat tap is equal by value to the earlier one. Tapping a link for a project that does not exist yet, creating it, then tapping again was dropped with no dialog, no error and no log, on that and every subsequent tap -- which is the flow the feature exists for. Track the failed-resolve consumptions separately in unresolvedDeepLinkRequests and exempt them from the gate. A request that never resolved never reached the editor, so no bounce can originate from it and the loop cannot come back. The set is persisted alongside consumedDeepLinkRequests, or the same sequence across a process death lands in the identical hole, and a request is dropped from it as soon as it is retried, so a later success stops the exemption. The Unverifiable branch has no automated coverage: provoking a real EACCES/EIO from the filesystem mid-call is not something a JVM unit test can do reliably. The new tests pin the two outcomes that are reachable, plus that findValidProjectByName still agrees with the lookup it now delegates to. Claude-Session: https://claude.ai/code/session_01LuyR4ajssmKw1o1UQsjJbP
Nothing on the device recorded that a deep link had happened. Every bug this feature shipped with looked identical from the outside -- a link that silently did nothing -- and there was no way to tell one from another, or to notice one at all. Adds a DeepLinkMetric through the existing IAnalyticsManager rather than touching Firebase directly: the manager already wraps FirebaseAnalytics behind a consent gate, and trackBuildRun/trackBuildCompleted already show the Metric pattern, so trackDeepLink is a default interface method and AnalyticsManager itself needs no new code. One event name for every outcome, so the funnel is a single event filtered by `outcome` rather than a set of events that have to be summed. Emitted once as RECEIVED when a link is accepted, and again with whatever terminal outcome it reaches, so a link that is accepted and then goes nowhere shows up as a gap between the two rather than as silence. `depth` records how far down project/file/line/column the link actually reached. PROJECT_NOT_FOUND and PROJECT_UNVERIFIABLE are deliberately separate, matching the split the resolve code now makes: the first means published links naming projects people do not have, the second means storage trouble on the device. The project name is hashed, never sent -- it is the user's content, and a link can carry a file path too. Matches trackProjectOpened's project_hash, which also makes the two joinable without either carrying the name off the device. A link with no project omits the key rather than logging zero, so it cannot be mistaken for a project whose name happens to hash to zero. Also guards trackMetric. Reaching the lazy `analytics` initializes FirebaseAnalytics, which throws outright when the default FirebaseApp was never initialized in this process -- DeepLinkSetupGateTest went red on exactly that the moment the first metric call was added to DeepLinkActivity. That activity is exported and logs before it does anything else, so an uninitialized Firebase would have turned every incoming link into a crash. Measuring a feature must not be able to break it, so the failure is swallowed and logged at the one choke point instead of each call site guarding for itself; this covers the existing build metrics too, which had the same latent exposure. Claude-Session: https://claude.ai/code/session_01LuyR4ajssmKw1o1UQsjJbP
Only MainActivity's resolve failure was instrumented, so a link that arrived while an editor was already live emitted RECEIVED and then nothing. That is the exact shape of the silent drop-off the paired events were added to expose -- a missing instrument masquerading as the bug it was meant to find. Found on the emulator: a warm-start link naming a nonexistent project logged received and no outcome. Claude-Session: https://claude.ai/code/session_01LuyR4ajssmKw1o1UQsjJbP
hal-eisen-adfa
approved these changes
Sep 2, 2026
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
5 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.
Summary
https://www.appdevforall.org/device/open/project/{name}[/file/{f}[/line/{n}[/column/{n}]]]: opens/focuses a project and, optionally, a file at a specific cursor position, per ADFA-5067.DeepLinkActivityis a UI-less trampoline holding the soleintent-filter, routing toMainActivity(nothing open) or the liveEditorHandlerActivity(something is — same-project no-op, different-project confirm-close-then-reopen via anonDestroy()-deferred handoff to avoid asingleTaskre-delivery race).EditorFeatures.validateRange) and adds a path-traversal guard (resolveWithinDirectory) for the attacker-controllable{filename}segment, mirroring the existing zip-slip pattern inAssetsInstallationHelper.EditorHandlerActivity.openFileAndSelectwhile testing on-device: opening a not-yet-open file at a specific line silently landed the cursor at line 1, because a mutableRange/Positionwas shared and clamped-to-zero by one caller before the file's own async content-load pipeline got to use it. Not deep-link-specific — this feature was just the first caller to combine "brand-new tab" with a non-origin selection..well-known/assetlinks.json(placeholder signing fingerprint — needs release engineering to fill in before App Links actually auto-verify).Filed separately (out of scope here): ADFA-5086, an unrelated pre-existing unguarded
InvalidPathExceptioncrash risk inplugin-manager'sIdeCommandServiceImpl, found while auditing the codebase for the same NUL-byte bug pattern.Commit-by-commit is intentional — see individual commit messages for the reasoning behind each piece (especially the
onDestroy()-deferred handoff and theopenFileAndSelectfix).Test plan
:app:compileV8DebugKotlincleanDeepLinkRequestTest(URL parsing, all optional-segment combinations),PathTraversalTest(literal.., encoded-slash shape, leading//\, embedded NUL byte, multi-segment paths)spotlessApplycleanadb shell am start -a android.intent.action.VIEW -d "<url>"):../../../data/data/.../shared_prefs/...) → rejected, no escape, no crash.well-known/assetlinks.json(blocked on release engineering / Play Console access — tracked as a follow-up, not blocking this PR per the ticket's own framing)🤖 Generated with Claude Code