ADFA-5067: Support deep links to open projects and files - #1651
Open
davidschachterADFA wants to merge 41 commits into
Open
ADFA-5067: Support deep links to open projects and files#1651davidschachterADFA wants to merge 41 commits into
davidschachterADFA wants to merge 41 commits into
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
Risks and follow-up:
WalkthroughAdded Android App Links support for project and file navigation. Links enter through ChangesDeep-link navigation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Android
participant DeepLinkActivity
participant MainActivity
participant EditorHandlerActivity
participant PendingDeepLinkOpen
Android->>DeepLinkActivity: Open verified project link
DeepLinkActivity->>MainActivity: Forward request when no editor is active
DeepLinkActivity->>EditorHandlerActivity: Forward request when an editor is active
MainActivity->>MainActivity: Resolve project and forward file request
EditorHandlerActivity->>EditorHandlerActivity: Resolve project and validate file target
EditorHandlerActivity->>PendingDeepLinkOpen: Queue confirmed project switch
PendingDeepLinkOpen-->>EditorHandlerActivity: Provide request during onDestroy
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
findValidProjectByName() (e9a1afb) joined projectsRoot with the attacker-controllable project name via a bare File(projectsRoot, name), regressing a safety property the O(n) findValidProjects() had for free: it only ever matches names of directories it already enumerated under projectsRoot, so it can't be pointed outside it. A deep link project name of "../../etc" (a decoded URL segment can contain slashes) would let the direct File join escape projectsRoot entirely. Resolves name through the existing resolveWithinDirectory() guard instead, matching the same protection already used for the file-path segment of a deep link. Adds regression tests: resolves a real project by name, rejects an unknown name, and rejects a dot-dot escape to a sibling directory. Found by CodeRabbit's review of the previous commit.
catch (e: Exception) around the single recentProjectDao.insert() call was broader than needed and would silently swallow an unrelated bug along with a genuine persistence failure. Room propagates android.database.SQLException (or subtypes like SQLiteConstraintException) from a failed @insert, so catching that specifically still protects the app-wide scope from a persistence hiccup while letting anything else surface. Drops the now-redundant explicit CancellationException rethrow -- it doesn't overlap with SQLException, so it already propagates on its own. Addressed from inline PR review comments.
The class doc was a one-liner ("ViewModel for main activity") that
didn't cover the LiveData main-thread requirement, the -1 sentinel for
"no screen yet", postTransition's defer-until-complete behavior, or
that the clone-request event is a buffered, single-consumer Channel
rather than persisted state. Doc-only change, no behavior change.
Addressed from inline PR review comments.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt`:
- Around line 21-29: Migrate ProjectValidationsTest from JUnit 4 to Jupiter by
replacing Rule/TemporaryFolder usage with a JUnit Jupiter `@TempDir` field and
updating each newFolder(...) call to use the temporary directory API. Add
testImplementation(libs.tests.junit.jupiter) to the app test dependencies and
configure the app unit-test task with useJUnitPlatform().
🪄 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: 5704deb9-493e-415e-bf95-ff3f00e8d78f
📒 Files selected for processing (3)
app/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.ktapp/src/main/java/com/itsaky/androidide/utils/ProjectValidations.ktapp/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/itsaky/androidide/utils/ProjectOpenBookkeeping.kt
- app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt
Comment on lines
+21
to
+29
| import org.junit.Rule | ||
| import org.junit.Test | ||
| import org.junit.rules.TemporaryFolder | ||
| import java.io.File | ||
|
|
||
| class ProjectValidationsTest { | ||
| @JvmField | ||
| @Rule | ||
| val tempFolder = TemporaryFolder() |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -eu
printf '%s\n' '--- test file ---'
cat -n app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt
printf '%s\n' '--- JUnit configuration references ---'
rg -n --hidden -S \
'useJUnitPlatform|junit-jupiter|junit\.jupiter|org\.junit\.Test|TemporaryFolder|`@TempDir`|testImplementation' \
app build.gradle.kts settings.gradle.kts gradle/libs.versions.toml 2>/dev/null || true
printf '%s\n' '--- relevant test files and build scripts ---'
git ls-files \
'app/src/test/**' \
'*build.gradle.kts' \
'gradle/libs.versions.toml' | sed -n '1,160p'Repository: appdevforall/CodeOnTheGo
Length of output: 14533
🏁 Script executed:
set -eu
printf '%s\n' '--- app test configuration ---'
sed -n '290,350p' app/build.gradle.kts
printf '%s\n' '--- testing unit module ---'
cat -n testing/unit/build.gradle.kts
printf '%s\n' '--- repository JUnit platform configuration ---'
rg -n --hidden -S \
'tasks?\.withType|Test::class|useJUnitPlatform|useJUnit|junit-vintage|junit:junit|junit-jupiter|android\.testOptions|testOptions' \
--glob '*.gradle' --glob '*.gradle.kts' --glob '*.toml' --glob '!**/build/**' .
printf '%s\n' '--- test annotation and temporary-directory usage ---'
rg -n --hidden -S \
'org\.junit\.jupiter\.api\.Test|org\.junit\.jupiter\.api\.io\.TempDir|`@TempDir`|TemporaryFolder|org\.junit\.Test' \
app/src/test testing app/build.gradle.kts testing/unit/build.gradle.kts 2>/dev/null | sed -n '1,240p'Repository: appdevforall/CodeOnTheGo
Length of output: 9409
🏁 Script executed:
set -eu
printf '%s\n' '--- app Android test options ---'
sed -n '70,110p' app/build.gradle.kts
printf '%s\n' '--- root test task configuration ---'
sed -n '70,105p' build.gradle.kts
printf '%s\n' '--- JUnit catalog entries ---'
sed -n '255,275p' gradle/libs.versions.toml
printf '%s\n' '--- Gradle plugin test configuration ---'
sed -n '20,40p' gradle-plugin/build.gradle.kts
printf '%s\n' '--- JUnit 5 usage outside the app module ---'
rg -n --hidden -S \
'org\.junit\.jupiter|useJUnitPlatform' \
--glob '*.kt' --glob '*.java' --glob '*.gradle.kts' --glob '!**/build/**' . | sed -n '1,160p'Repository: appdevforall/CodeOnTheGo
Length of output: 4807
Migrate this test to JUnit Jupiter and configure the app unit-test task for the Jupiter platform. The app currently provides JUnit 4 through testing.unit and has no Jupiter dependency or useJUnitPlatform() configuration. Add testImplementation(libs.tests.junit.jupiter), enable the Jupiter platform, replace TemporaryFolder with @TempDir, and update the newFolder(...) calls.
🤖 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/ProjectValidationsTest.kt`
around lines 21 - 29, Migrate ProjectValidationsTest from JUnit 4 to Jupiter by
replacing Rule/TemporaryFolder usage with a JUnit Jupiter `@TempDir` field and
updating each newFolder(...) call to use the temporary directory API. Add
testImplementation(libs.tests.junit.jupiter) to the app test dependencies and
configure the app unit-test task with useJUnitPlatform().
Source: Coding guidelines
jatezzz
reviewed
Aug 11, 2026
jatezzz
left a comment
jatezzz
left a comment
Collaborator
There was a problem hiding this comment.
Automated code review (medium depth). 9 findings: 1 high, 4 medium, 4 low — inline below.
The high one is a core-requirement bug: the "already in this project" fast path requires a non-null workspace, which stays null for the whole duration of a Gradle sync, so a deep link to the currently-open project wrongly prompts to close it.
Verified clean: the openFileAndSelect defensive copy, MainViewModel's dropped recentProjectDao (no callers left), the symlink handling in PathTraversal, and DeepLinkRequest's backward line/column segment matching.
🤖 Generated with Claude Code
activeProjectCloseDialog was tracked but never dismissed on destroy -- rotating the device (or any destroy) while the confirm-close dialog is showing leaked its window (WindowLeaked). Found by John Trujillo's review of PR 1651.
…rows
CodeEditorView.save() propagates an IOException from a failed disk
write uncaught. saveAllAsync's coroutine ran saveAll() with no
try/catch, so that exception skipped straight past the
withContext(Dispatchers.Main) { runAfter?.invoke() } that followed --
runAfter is the only place confirmProjectClose's confirmCloseInProgress
guard gets reset, so a disk-full or permission failure during "Save
and close" left it stuck true, permanently blocking closing that
activity instance (on top of the uncaught exception itself being a
crash risk). CancellationException is rethrown; other failures are
logged and runAfter still runs.
The other saveAllAsync caller (notifyFilesUnsaved) has the identical
gap today (invokeAfter never runs on a save failure); this fixes it
too, and now behaves the same as the success path there (proceeds
regardless of whether every file actually saved), which is no worse
than before.
Found by John Trujillo's review of PR 1651.
…cking confirmProjectClose() shared its dialog/token state between the plain manual close (back button, sidebar action) and the deep-link close-then-reopen flow. A deep link arriving while a manual close dialog was showing dismissed it and replaced it with one whose buttons run the deep-link's onClosed -- a user tapping "Close without saving" on what looked like an ordinary close ended up with an unrelated deep-linked project opened instead, or vice versa. Replaces the dismiss-and-replace strategy with reject-while-active: a single confirmCloseInProgress flag covers both the dialog being shown and its "Save and close" still writing files, and any confirmProjectClose call while it's set is dropped (with a flashError, previously silent) rather than allowed to interrupt whatever's already in flight. This also removes the need for the previous generation-token mechanism -- with only ever one flow active, there's no longer a "newer" request to distinguish from a "stale" one. Also fixes a related false-positive: the failed-save check added alongside the original guard used hasUnsavedFiles(), which stays true for files CodeEditorView.save() intentionally never writes (an ARCHIVE_EXTENSIONS extension, opened read-only) -- any such tab left "Save and close" permanently refusing to close. The new hasFilesThatFailedToSave() excludes those. Found by John Trujillo's review of PR 1651 and a fresh full re-review.
The "already in this project" fast path in EditorHandlerActivity.onNewIntent required IProjectManager.getInstance().workspace != null, but workspace stays null for the whole duration of a Gradle sync -- so a deep link to the project that's already open, tapped while its own sync is still running, fell through to the disruptive "different project" branch and prompted to close and reopen the project the user was already in. Compares projectDirPath alone, which is set as soon as a project starts opening. Also extracts the identical ~15-line try/catch(CancellationException/ SecurityException) + null-check + flashError block around findValidProjectByName, duplicated between MainActivity and EditorHandlerActivity with two different logging APIs for the same log line, into one resolveDeepLinkProject() helper. Found by John Trujillo's review of PR 1651 (the workspace bug, independently) and a fresh full re-review (the duplication).
postProjectInit() only read and cleared the PendingFileRequest intent extra when isSuccessful was true, returning before either on failure. A cold open via a file+line deep link whose initial sync fails left the extra armed indefinitely; the next unrelated *successful* sync or build-variant switch on that same activity instance would still find it and silently jump the editor back to the original deep-linked file/line, discarding whatever the user was actually working on by then. Drains the extra unconditionally on the first postProjectInit call, regardless of outcome, and only applies it if that first sync succeeded. Found by a fresh full re-review of PR 1651.
getActivity()'s WeakReference is only cleared in onDestroy(), so it stayed non-null for an EditorHandlerActivity that had already called finish() (e.g. the user picked "Close project") but hasn't been destroyed yet. DeepLinkActivity would then route a deep link tapped in that window to EditorActivityKt; since the existing instance is finishing, the framework creates a fresh instance instead of delivering via onNewIntent, whose onCreate never reads DEEP_LINK_REQUEST (only onNewIntent does) and falls back to reopening GeneralPreferences.lastOpenedProject -- the deep link was silently dropped and the wrong project opened. Filters isFinishing/isDestroyed out at the source rather than in each caller, since none of getActivity()'s three call sites (DeepLinkActivity, IDEApiFacade, EditorPanelDockableContent) can safely "trigger UI actions" on an activity that's already finishing or destroyed either. Found by John Trujillo's review of PR 1651.
…vity SINGLE_TOP alone can't dedupe MainActivity here: DeepLinkActivity is itself the top of the stack at the moment startActivity() runs (finish() comes after), so SINGLE_TOP's "is the target already at the top" check never matches -- MainActivity's own manifest declaration can't fix this either, since singleTop launch mode has the identical "must be literally on top" restriction as the Intent flag. Tapping two deep links while MainActivity is showing created two stacked MainActivity instances (each re-running startWebServer()), with Back walking through the stale one. CLEAR_TOP finds an existing MainActivity anywhere in the task and (combined with SINGLE_TOP, rather than the destroy-and-recreate CLEAR_TOP alone would do) redelivers to it via onNewIntent. EditorActivityKt is unaffected (already singleTask, always reuses its live instance). Found by John Trujillo's review of PR 1651.
… name resolveWithinDirectory's lexical check only rejects ".."/a leading separator, so a deep-link project name of "." resolved to projectsRoot itself -- if the projects directory happens to satisfy isValidProjectDirectory, the link would "open" the whole projects directory as if it were a single project. An embedded separator like "foo/bar" would similarly resolve two levels deep instead of naming a direct child. A project name is always a single path segment, so reject both up front. Found by John Trujillo's review of PR 1651.
Narrowing this to SQLException (a44feeb) assumed the usual "don't catch too broadly" guidance applies here, but this coroutine runs on ProcessLifecycleOwner's permanent, app-wide scope, which has no CoroutineExceptionHandler -- unlike the ViewModel-scoped version this replaced. Room's generated insert can throw non-SQLException types too (e.g. IllegalStateException from an already-closed database), and any of them escaping here crashes the whole process, not just fails to record one Recents entry. Given the severity of that scope, catching broadly is the correct tradeoff for this one line; CancellationException is still rethrown so cancellation isn't swallowed. Found by John Trujillo's review of PR 1651 and a fresh full re-review, independently.
The App-Links paragraph only covered the "nothing open" and "different project open" cases, omitting the "same project already open -- just navigate" branch and the whole file/line/column-opening feature (PendingFileRequest, applyDeepLinkFileRequest, resolveWithinDirectory's path-traversal guard). Found by a fresh full re-review of PR 1651.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt (1)
942-953: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the existing SLF4J logger for this failure.
Line 953 adds
Log.e(...)in a class that already useslog. Replace it withlog.error("saveAll failed", e).As per coding guidelines: “Use SLF4J LoggerFactory rather than android.util.Log.”
Proposed fix
- Log.e("EditorHandlerActivity", "saveAll failed", e) + log.error("saveAll failed", e)🤖 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 942 - 953, Replace the android.util.Log.e call in the saveAll exception handler with the existing SLF4J logger, using log.error("saveAll failed", e) while preserving the surrounding exception and cleanup flow.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.
Nitpick comments:
In
`@app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt`:
- Around line 942-953: Replace the android.util.Log.e call in the saveAll
exception handler with the existing SLF4J logger, using log.error("saveAll
failed", e) while preserving the surrounding exception and cleanup flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0659f102-2f64-4bee-854b-37289ea0db51
📒 Files selected for processing (12)
ARCHITECTURE.mdapp/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/api/ActionContextProvider.ktapp/src/main/java/com/itsaky/androidide/ui/CodeEditorView.ktapp/src/main/java/com/itsaky/androidide/utils/DeepLinkProjectResolution.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/utils/ProjectValidationsTest.ktresources/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (5)
- resources/src/main/res/values/strings.xml
- app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt
- app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt
- app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt
- app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt
…links Real functional conflict, not just textual: stage independently added project-language detection/tracking (RecentProject.language, readProjectLanguage(), RecentProjectDao.updateLanguage()) inside MainViewModel.saveProjectToRecents(), the same function this branch had already deleted as dead code after extracting its logic into the shared recordProjectOpenedBookkeeping() helper (used by both MainActivity.openProject and EditorHandlerActivity's deep-link close-then-reopen hand-off). Resolved by keeping this branch's shared-helper architecture and moving stage's language-detection/refresh logic into recordProjectOpenedBookkeeping() instead of reviving saveProjectToRecents() -- so both the manual-open and deep-link-open paths get language tracking, rather than only the manual one. Kept this branch's broader Throwable catch there (ProcessLifecycleOwner's scope has no exception handler) rather than stage's SQLException-only catch. MainActivity.kt: kept this branch's deep-link handling in onCreate/ onProject/onNewIntent, dropped stage's now-superseded inline Recents-insert block in openProject in favor of recordProjectOpenedBookkeeping(). Conflicts: app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt app/src/main/java/com/itsaky/androidide/viewmodel/MainViewModel.kt
saveAllAsync's new failure log used Log.e() in a class that already has BaseEditorActivity's protected SLF4J log field, against this repo's "use SLF4J LoggerFactory rather than android.util.Log" coding guideline. Also fixes a stale comment still referring to the guard by its old name (closeInProgress -> confirmCloseInProgress). Found by CodeRabbit's review of PR 1651.
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.
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