From 88688c1c769783a7984543efabff569e1c179c29 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 18:59:35 -0700 Subject: [PATCH 01/16] ADFA-5472: Add "create link" to the editor file-tab menu ADFA-5067 taught the app to open a deep link; this is the other half -- making one. The file-tab drop-down gains a "Create link" item that copies a URL naming the current project, file, line and column to the clipboard and confirms with a flashbar. DeepLinkRequest.buildUrl() is the inverse of parse(), and lives beside it so the two cannot drift. Each of its rejections mirrors a specific check on the read side: a project name carrying a separator can never name a direct child of the projects root, a line or column of zero is what zeroBasedOrInvalid() reports back as invalid, an empty path component would emit the "//" parse() refuses, and the length ceiling is measured against the decoded path because that is what parse() measures. Two details are load-bearing rather than incidental: Path components are appended one at a time. Uri.Builder.appendPath encodes its argument as a single segment and so percent-encodes '/' along with everything else, and handing it a whole relative path emits ".../file/src%2Fmain%2FMain.kt" -- which parse() reads back correctly, so nothing fails, it just becomes a URL no human can read. Encoding is not optional in the other direction either: '#' and '?' are legal in a Linux filename and would otherwise truncate the path into a fragment or a query. Line and column are always written, never omitted when the cursor sits at 1:1. A real trailing keyword/value pair is what keeps a file path whose own last segments look like "line"/"column" out of peelTrailingKeyword's reach -- the ambiguity parse() documents as unresolvable stays confined to hand-authored links. Paired tests pin both halves of that, so making line/column conditional fails loudly. The action hides itself when the open project is not a direct child of the projects root -- opened from the file picker, Recents, or a clone destination. No URL resolves to such a project on any device, so the alternative is offering an affordance that yields a dead link. It reuses the reader's own containment rule, isDeepLinkTargetOfOpenProject, rather than restating it. Tests: 15 new round-trip cases, including a space in a project name, an NFD-decomposed accented name, '#' and '?' in filenames, and a guard that the emitted URL contains no "%2F" -- the whole-path mistake still round-trips, so parse() alone would not catch it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../actions/file/CreateLinkAction.kt | 137 +++++++++++++++ .../androidide/models/DeepLinkRequest.kt | 97 ++++++++++- .../androidide/utils/EditorActivityActions.kt | 2 + .../androidide/models/DeepLinkBuildUrlTest.kt | 162 ++++++++++++++++++ resources/src/main/res/values/strings.xml | 3 + 5 files changed, 400 insertions(+), 1 deletion(-) create mode 100644 app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt create mode 100644 app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt new file mode 100644 index 0000000000..8f03b680a1 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -0,0 +1,137 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.actions.file + +import android.content.Context +import androidx.core.content.ContextCompat +import com.itsaky.androidide.R +import com.itsaky.androidide.actions.ActionData +import com.itsaky.androidide.actions.markInvisible +import com.itsaky.androidide.activities.editor.EditorHandlerActivity +import com.itsaky.androidide.activities.projectsRoot +import com.itsaky.androidide.models.DeepLinkRequest +import com.itsaky.androidide.projects.IProjectManager +import com.itsaky.androidide.utils.copyToClipboard +import com.itsaky.androidide.utils.flashError +import com.itsaky.androidide.utils.flashSuccess +import com.itsaky.androidide.utils.isDeepLinkTargetOfOpenProject +import java.io.File + +/** + * Copies a deep link to the current file -- and to the cursor's line and column within it -- to the + * clipboard, so it can be pasted somewhere another person (or the same person on another device) can + * tap it. The read side of the same link is + * [DeepLinkActivity][com.itsaky.androidide.activities.DeepLinkActivity]. + * + * @author David Schachter + */ +class CreateLinkAction( + context: Context, + override val order: Int, +) : FileTabAction() { + override val id: String = ID + + companion object { + const val ID = "ide.editor.fileTab.createLink" + + /** Shown in the clipboard preview on Android 13+, so it names the app rather than the action. */ + private const val CLIP_LABEL = "Code on the Go link" + } + + init { + label = context.getString(R.string.action_create_link) + icon = ContextCompat.getDrawable(context, R.drawable.ic_copy) + } + + override fun prepare(data: ActionData) { + super.prepare(data) + + if (!visible) { + return + } + + val activity = + data.getActivity() + ?: run { + markInvisible() + return + } + + // Hidden rather than shown-and-failing: the states that produce no link are properties of how + // the project was opened, not transient ones the user could correct by tapping again. + if (linkForCurrentFile(activity) == null) { + markInvisible() + } + } + + override fun EditorHandlerActivity.doAction(data: ActionData): Boolean { + // Recomputed rather than cached from prepare(): the menu can outlive the state it was built + // from (the tab can be closed, the project re-synced) and a stale link is worse than none. + // prepare() hides this action for every state that is stably unlinkable, so getting here with + // nothing to copy means something moved underneath the open menu -- rare, but a tap that does + // nothing at all reads as a broken button, so say so. + val url = + linkForCurrentFile(this) ?: run { + flashError(R.string.msg_deeplink_cannot_create) + return false + } + + copyToClipboard(url, label = CLIP_LABEL) + flashSuccess(R.string.msg_deeplink_copied) + return true + } + + /** + * The deep link for the file in [activity]'s currently selected tab, or `null` if that file has no + * link that would resolve anywhere. + */ + private fun linkForCurrentFile(activity: EditorHandlerActivity): String? { + val editorView = activity.getCurrentEditor() ?: return null + val file = editorView.file ?: return null + + val projectPath = IProjectManager.getInstance().projectDirPath + if (projectPath.isBlank()) { + return null + } + val projectDir = File(projectPath) + + // A deep link can only ever name /, but a project can be opened from + // anywhere -- the file picker, Recents, a clone destination. For one of those there is no URL + // that resolves on this device, let alone on the recipient's, so there is nothing honest to + // put on the clipboard. Reusing the reader's own containment rule keeps the two from drifting. + if (!isDeepLinkTargetOfOpenProject(projectPath, projectDir.name, projectsRoot())) { + return null + } + + // relativeToOrNull walks up with ".." when the file lies outside the project rather than + // failing, so containment has to be checked on the result and not just on the call succeeding. + val relativePath = file.relativeToOrNull(projectDir)?.invariantSeparatorsPath ?: return null + if (relativePath.isEmpty() || relativePath == ".." || relativePath.startsWith("../")) { + return null + } + + // The editor is zero-based at both ends; the URL scheme is one-based. See buildUrl's docs. + val cursor = editorView.editor?.cursor + return DeepLinkRequest.buildUrl( + projectName = projectDir.name, + filePath = relativePath, + line = cursor?.let { it.leftLine + 1 }, + column = cursor?.let { it.leftColumn + 1 }, + ) + } +} diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index 926afbe085..04a20efb0e 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -56,9 +56,17 @@ data class DeepLinkRequest( private const val SCHEME = "https" + /** + * The host [buildUrl] writes. + * + * Both entries in [HOSTS] resolve identically, so this is only a choice about which one a + * generated link shows the person who receives it; the bare domain is the shorter. + */ + private const val CANONICAL_HOST = "appdevforall.org" + // Both hosts serve an identical, verified assetlinks.json (see AndroidManifest.xml's matching // pair of elements on DeepLinkActivity's intent-filter) -- kept in sync with that list. - private val HOSTS = setOf("www.appdevforall.org", "appdevforall.org") + private val HOSTS = setOf("www.$CANONICAL_HOST", CANONICAL_HOST) private const val PATH_PREFIX = "/device/open/project/" /** @@ -225,6 +233,93 @@ data class DeepLinkRequest( return DeepLinkRequest(projectName = projectName, fileRequest = fileRequest) } + + /** + * The inverse of [parse]: the canonical URL naming [projectName], optionally the + * project-relative [filePath] inside it, and optionally a [line] and [column] inside that file. + * + * [line] and [column] are ONE-BASED, matching the URL scheme rather than the editor. The + * editor's own cursor is zero-based and + * [EditorHandlerActivity][com.itsaky.androidide.activities.editor.EditorHandlerActivity] + * subtracts one again when it reads a link back, so a caller holding a cursor passes + * `cursor.leftLine + 1`. + * + * Returns `null` rather than a URL that [parse] would reject or read back as something other + * than what was asked for. Each rejection below mirrors a specific check in [parse] or in + * [lookupValidProjectByName][com.itsaky.androidide.utils.lookupValidProjectByName]: there is + * nothing to be gained by handing someone a link this same app refuses to open. + */ + fun buildUrl( + projectName: String, + filePath: String? = null, + line: Int? = null, + column: Int? = null, + ): String? { + // A project name is a single path segment naming a direct child of the projects root, so a + // name carrying a separator can never resolve -- lookupValidProjectByName rejects it before + // it ever touches the disk. + if (projectName.isEmpty() || projectName.contains('/') || projectName.contains('\\')) { + return null + } + + // parse() keeps line/column only when there is a file for them to apply to, and silently + // drops them otherwise. Refusing beats emitting a link that quietly loses them. + if (filePath == null && (line != null || column != null)) { + return null + } + + // Zero and negative are exactly what zeroBasedOrInvalid() reports back to the user as an + // invalid line/column, so they must not be written down in the first place. + if ((line != null && line <= 0) || (column != null && column <= 0)) { + return null + } + + val builder = Uri.Builder().scheme(SCHEME).authority(CANONICAL_HOST) + + // Derived from PATH_PREFIX rather than spelled out a second time, so a change to the prefix + // parse() requires cannot leave the writer emitting the old one. + PATH_PREFIX.trim('/').split('/').forEach { builder.appendPath(it) } + builder.appendPath(projectName) + + if (filePath != null) { + val segments = filePath.split('/') + // An empty component would put "//" in the path, which parse() rejects outright. + if (segments.any(String::isEmpty)) { + return null + } + + builder.appendPath(SEGMENT_FILE) + + // One appendPath call PER COMPONENT. Uri.Builder.appendPath encodes its argument as a + // single segment, which means it percent-encodes '/' along with everything outside + // [A-Za-z0-9_-!.~'()*] -- so handing it the whole relative path in one call emits + // ".../file/src%2Fmain%2FMain.kt". parse() does read that back correctly (getPathSegments + // splits the ENCODED path, so an encoded slash never becomes a separator), but it is + // neither the shape this class documents nor a URL a human can read. Encoding is not + // optional either way: '#' and '?' are legal in a Linux filename and would otherwise + // truncate the path into a fragment or a query. + segments.forEach { builder.appendPath(it) } + + // Always written as a full keyword/value pair, even though parse() tolerates a bare + // trailing keyword. A real pair is also what keeps a file path whose own trailing + // segments look like "line"/"column" out of peelTrailingKeyword's reach -- see parse()'s + // notes on the shapes it cannot resolve. Hand-authored links stay exposed to that; + // links from here do not. + line?.let { builder.appendPath(SEGMENT_LINE).appendPath(it.toString()) } + column?.let { builder.appendPath(SEGMENT_COLUMN).appendPath(it.toString()) } + } + + val uri = builder.build() + + // parse() measures the DECODED path against this ceiling and Uri.getPath() is decoded, so + // this is the same number it will see -- percent-expansion in the emitted string does not + // count against it. + if ((uri.path?.length ?: 0) > MAX_LINK_PATH_LENGTH) { + return null + } + + return uri.toString() + } } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/EditorActivityActions.kt b/app/src/main/java/com/itsaky/androidide/utils/EditorActivityActions.kt index 64895fd1bd..d1866e0417 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/EditorActivityActions.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/EditorActivityActions.kt @@ -44,6 +44,7 @@ import com.itsaky.androidide.actions.etc.LaunchAppAction import com.itsaky.androidide.actions.file.CloseAllFilesAction import com.itsaky.androidide.actions.file.CloseFileAction import com.itsaky.androidide.actions.file.CloseOtherFilesAction +import com.itsaky.androidide.actions.file.CreateLinkAction import com.itsaky.androidide.actions.file.FormatCodeAction import com.itsaky.androidide.actions.file.InstallFileAction import com.itsaky.androidide.actions.file.SaveFileAction @@ -117,6 +118,7 @@ class EditorActivityActions { registry.registerAction(CloseOtherFilesAction(context, order++)) registry.registerAction(CloseAllFilesAction(context, order++)) registry.registerAction(InstallFileAction(context, order++)) + registry.registerAction(CreateLinkAction(context, order++)) // file tree actions registry.registerAction(CopyPathAction(context, ORDER_COPY_PATH)) diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt new file mode 100644 index 0000000000..81007a431f --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt @@ -0,0 +1,162 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.models + +import android.net.Uri +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * [DeepLinkRequest.buildUrl] -- the write side of the deep-link scheme, checked mostly by + * round-tripping through [DeepLinkRequest.parse], which is the only consumer that matters. + */ +@RunWith(RobolectricTestRunner::class) +class DeepLinkBuildUrlTest { + private fun roundTrip(url: String?) = DeepLinkRequest.parse(Uri.parse(url!!)) + + @Test + fun `project only`() { + val url = DeepLinkRequest.buildUrl(projectName = "MyApp") + assertThat(url).isEqualTo("https://appdevforall.org/device/open/project/MyApp") + assertThat(roundTrip(url)).isEqualTo(DeepLinkRequest(projectName = "MyApp")) + } + + @Test + fun `file with line and column`() { + val url = DeepLinkRequest.buildUrl("MyApp", "src/main/Main.kt", line = 7, column = 3) + assertThat(url) + .isEqualTo("https://appdevforall.org/device/open/project/MyApp/file/src/main/Main.kt/line/7/column/3") + assertThat(roundTrip(url)) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "src/main/Main.kt", lineRaw = "7", columnRaw = "3"), + ), + ) + } + + @Test + fun `path separators stay separators, they are not percent-encoded away`() { + // Uri.Builder.appendPath encodes '/' inside a single segment, so the whole relative path must + // be appended one component at a time. Getting this wrong still round-trips -- it just emits + // an unreadable ".../file/src%2Fmain%2FMain.kt" -- so assert the emitted shape, not just parse. + val url = DeepLinkRequest.buildUrl("MyApp", "src/main/Main.kt") + assertThat(url).doesNotContain("%2F") + assertThat(url).endsWith("/file/src/main/Main.kt") + } + + @Test + fun `space in project name is encoded and survives the round trip`() { + val url = DeepLinkRequest.buildUrl("My App", "Main.kt", line = 1, column = 1) + assertThat(url).contains("/project/My%20App/") + assertThat(roundTrip(url)?.projectName).isEqualTo("My App") + } + + @Test + fun `hash in a filename is encoded rather than truncating the path into a fragment`() { + val url = DeepLinkRequest.buildUrl("MyApp", "notes/Draft#1.md", line = 4, column = 2) + assertThat(url).contains("Draft%231.md") + assertThat(roundTrip(url)?.fileRequest?.filePath).isEqualTo("notes/Draft#1.md") + } + + @Test + fun `question mark in a filename is encoded rather than starting a query`() { + val url = DeepLinkRequest.buildUrl("MyApp", "Why?.txt") + assertThat(url).contains("Why%3F.txt") + assertThat(roundTrip(url)?.fileRequest?.filePath).isEqualTo("Why?.txt") + } + + @Test + fun `decomposed accented name survives the round trip byte for byte`() { + // NFD: "Cafe" + COMBINING ACUTE ACCENT, the form a directory cloned from macOS carries. + val nfd = "Cafe\u0301" + val url = DeepLinkRequest.buildUrl(nfd, "Main.kt") + assertThat(url).contains("Cafe%CC%81") + assertThat(roundTrip(url)?.projectName).isEqualTo(nfd) + } + + @Test + fun `a directory named line does not swallow the file path when line and column are written`() { + // The shape parse() documents as unresolvable, defused: because a real trailing line/column + // pair always follows, peelTrailingKeyword consumes that pair and never reaches the "line" + // directory in the path itself. + val url = DeepLinkRequest.buildUrl("MyApp", "src/line/5", line = 7, column = 3) + assertThat(roundTrip(url)) + .isEqualTo( + DeepLinkRequest( + projectName = "MyApp", + fileRequest = PendingFileRequest(filePath = "src/line/5", lineRaw = "7", columnRaw = "3"), + ), + ) + } + + @Test + fun `the same path without a line is misread -- the limitation buildUrl's callers must avoid`() { + // Documents why CreateLinkAction always passes a line and a column rather than omitting them + // when the cursor sits at 1:1. Nothing here is a bug in buildUrl; it is parse()'s positional + // peeling, and this test exists so a future change to either side notices. + val url = DeepLinkRequest.buildUrl("MyApp", "src/line/5") + assertThat(roundTrip(url)?.fileRequest) + .isEqualTo(PendingFileRequest(filePath = "src", lineRaw = "5", columnRaw = null)) + } + + @Test + fun `column without a line round-trips`() { + val url = DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = null, column = 3) + assertThat(roundTrip(url)?.fileRequest) + .isEqualTo(PendingFileRequest(filePath = "Main.kt", lineRaw = null, columnRaw = "3")) + } + + @Test + fun `rejects a project name that cannot name a direct child of the projects root`() { + assertThat(DeepLinkRequest.buildUrl("")).isNull() + assertThat(DeepLinkRequest.buildUrl("nested/MyApp")).isNull() + assertThat(DeepLinkRequest.buildUrl("nested\\MyApp")).isNull() + } + + @Test + fun `rejects an empty path component, which would emit a rejected double slash`() { + assertThat(DeepLinkRequest.buildUrl("MyApp", "src//Main.kt")).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "/Main.kt")).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "")).isNull() + } + + @Test + fun `rejects a line or column that parse would report back as invalid`() { + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 0)).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = -1)).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 1, column = 0)).isNull() + } + + @Test + fun `rejects a line with no file to apply it to`() { + assertThat(DeepLinkRequest.buildUrl("MyApp", filePath = null, line = 7)).isNull() + } + + @Test + fun `rejects a path over the length parse enforces`() { + val tooLong = "a".repeat(600) + assertThat(DeepLinkRequest.buildUrl("MyApp", tooLong)).isNull() + + // And is not simply refusing everything long: a path just inside the ceiling still builds. + val insideCeiling = "a".repeat(400) + assertThat(DeepLinkRequest.buildUrl("MyApp", insideCeiling)).isNotNull() + } +} diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 9d24ea5d36..d30cb64ec3 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -146,6 +146,8 @@ \"%s\" is not a valid column number. (no value given) Could not scan projects for this link. + Link copied to the clipboard + Could not create a link for this file. A project close is already in progress. Try again in a moment. Create new project Open a saved project @@ -281,6 +283,7 @@ Install Close others Close this + Create link All files saved Failed to save files Destination: %s/ From 68d0b13bbaaf63585dd89b4e2960c8eae3656283 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 19:28:03 -0700 Subject: [PATCH 02/16] ADFA-5472: Address review findings Five fixes from /code-review and CodeRabbit, which each caught things the other missed. buildUrl now verifies its own output. The per-check guards mirror the rejections in parse(), but parse() also peels line/column POSITIONALLY, and no enumeration of guards catches every path whose own trailing segments happen to look like that metadata. Parsing the built URI back and comparing it against the ARGUMENTS that built it -- not against a re-derivation, so it cannot pass by agreeing with itself -- turns that whole class into a null return for the cost of one parse. That subsumes what was previously only documented: buildUrl("MyApp", "src/line/5") with no line of its own read back as file "src" at line 5. The test that recorded that misread as expected behavior now requires rejection, and a new test asserts the contract directly over a table of inputs. CreateLinkAction now requires a cursor rather than treating one as optional. With editorView.editor null -- true until the view is inflated -- it emitted a coordinate-free link, which is exactly the ambiguous shape the commit that added it claimed could never be generated. The claim is now enforced instead of asserted. Dot segments are rejected on both halves of the URL. isProjectCandidateDir() refuses any name starting with '.', so a hidden directory is never a project, and that one check also covers "." and "..". Those two matter beyond being unopenable: a browser or messenger that normalizes dot segments rewrites the path in transit. File paths reject "." and ".." components for the same reason, while still allowing hidden files -- .gitignore is perfectly linkable. The project containment check is memoised per project path and wrapped in allowThreadDiskReads. isDeepLinkTargetOfOpenProject canonicalises both sides, which is two filesystem calls, and prepare() runs on the UI thread every time a file-tab menu opens -- enough for StrictMode's detectAll() to report a DiskReadViolation on each one, and enough to jank the popup on a slow external volume. The answer only changes when a different project is opened. Verified on a Pixel 6 Pro: the generated URL is byte-identical before and after, including both %20 in the project name "My great app". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../actions/file/CreateLinkAction.kt | 44 +++++++++++-- .../androidide/models/DeepLinkRequest.kt | 39 +++++++++-- .../androidide/models/DeepLinkBuildUrlTest.kt | 64 +++++++++++++++++-- 3 files changed, 132 insertions(+), 15 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index 8f03b680a1..ffc0970eec 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -26,6 +26,7 @@ import com.itsaky.androidide.activities.editor.EditorHandlerActivity import com.itsaky.androidide.activities.projectsRoot import com.itsaky.androidide.models.DeepLinkRequest import com.itsaky.androidide.projects.IProjectManager +import com.itsaky.androidide.utils.allowThreadDiskReads import com.itsaky.androidide.utils.copyToClipboard import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess @@ -51,6 +52,34 @@ class CreateLinkAction( /** Shown in the clipboard preview on Android 13+, so it names the app rather than the action. */ private const val CLIP_LABEL = "Code on the Go link" + + /** + * Memoised answer to "is the open project one a link can name", keyed by its path. + * + * [isDeepLinkTargetOfOpenProject] canonicalises both sides, which is two filesystem calls, and + * `prepare()` runs on the UI thread every single time a file-tab menu is opened -- enough for + * StrictMode (which this app arms with `detectAll()` in debug builds) to report a + * DiskReadViolation on each one, and enough to jank the popup on a slow external volume. The + * answer only changes when a different project is opened, so it is computed once per project + * instead of once per menu. + */ + @Volatile + private var linkableProject: Pair? = null + + private fun isLinkableProject(projectPath: String): Boolean { + linkableProject?.let { (cachedPath, cached) -> + if (cachedPath == projectPath) return cached + } + + // The one unavoidable read, taken once per project. Exempted rather than moved off-thread + // because prepare() has to answer synchronously to decide whether to show the item at all. + val linkable = + allowThreadDiskReads("Canonicalising the open project once, to decide if it can be linked") { + isDeepLinkTargetOfOpenProject(projectPath, File(projectPath).name, projectsRoot()) + } + linkableProject = projectPath to linkable + return linkable + } } init { @@ -114,7 +143,7 @@ class CreateLinkAction( // anywhere -- the file picker, Recents, a clone destination. For one of those there is no URL // that resolves on this device, let alone on the recipient's, so there is nothing honest to // put on the clipboard. Reusing the reader's own containment rule keeps the two from drifting. - if (!isDeepLinkTargetOfOpenProject(projectPath, projectDir.name, projectsRoot())) { + if (!isLinkableProject(projectPath)) { return null } @@ -125,13 +154,20 @@ class CreateLinkAction( return null } + // A cursor is required, not optional. Without one the link would carry no line or column, and + // a coordinate-free link is the one shape parse() can misread: a file path whose own trailing + // segments look like "line"/"column" gets peeled apart as metadata. buildUrl now rejects that + // case outright, so treating a missing cursor as "no link" is what keeps this action from + // silently producing nothing at the moment of the tap. The editor is null only before its view + // is inflated, and prepare() re-runs on every menu open. + val cursor = editorView.editor?.cursor ?: return null + // The editor is zero-based at both ends; the URL scheme is one-based. See buildUrl's docs. - val cursor = editorView.editor?.cursor return DeepLinkRequest.buildUrl( projectName = projectDir.name, filePath = relativePath, - line = cursor?.let { it.leftLine + 1 }, - column = cursor?.let { it.leftColumn + 1 }, + line = cursor.leftLine + 1, + column = cursor.leftColumn + 1, ) } } diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index 04a20efb0e..bb1e21fcaa 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -257,8 +257,17 @@ data class DeepLinkRequest( ): String? { // A project name is a single path segment naming a direct child of the projects root, so a // name carrying a separator can never resolve -- lookupValidProjectByName rejects it before - // it ever touches the disk. - if (projectName.isEmpty() || projectName.contains('/') || projectName.contains('\\')) { + // it ever touches the disk. A leading dot is rejected for the same reason one level down: + // isProjectCandidateDir() refuses any name starting with '.', so a hidden directory is + // never a project. That one check also covers "." and "..", which matter more than merely + // being unopenable -- a browser or messenger that normalizes dot segments rewrites + // "/device/open/project/../file/x" into an entirely different path before the app ever + // sees it. + if (projectName.isEmpty() || + projectName.startsWith('.') || + projectName.contains('/') || + projectName.contains('\\') + ) { return null } @@ -283,8 +292,12 @@ data class DeepLinkRequest( if (filePath != null) { val segments = filePath.split('/') - // An empty component would put "//" in the path, which parse() rejects outright. - if (segments.any(String::isEmpty)) { + // An empty component would put "//" in the path, which parse() rejects outright. A "." + // or ".." component is refused for the reason given above: resolveWithinDirectory treats + // it as traversal and rejects the link, and any URL-normalizing intermediary silently + // rewrites it into a different path on the way. Other dot-prefixed names are fine -- + // unlike a project directory, a hidden FILE (.gitignore) is perfectly linkable. + if (segments.any { it.isEmpty() || it == "." || it == ".." }) { return null } @@ -318,6 +331,24 @@ data class DeepLinkRequest( return null } + // The contract, enforced rather than reasoned about: never hand back a URL that this same + // app reads as something other than what was asked for. The guards above each mirror one + // known rejection in parse(), but parse() also peels line/column POSITIONALLY, and no + // enumeration of guards catches every path whose own trailing segments happen to look like + // that metadata (".../file/src/line/5" with no line of its own reads back as file "src" at + // line 5). Comparing the parse of what was just built against the arguments that built it + // turns that whole class into a null return, and costs one parse of a string already in + // hand. Compared against the ARGUMENTS, not against a re-derivation, so this cannot pass + // by agreeing with itself. + val parsed = parse(uri) ?: return null + if (parsed.projectName != projectName || + parsed.fileRequest?.filePath != filePath || + parsed.fileRequest?.lineRaw != line?.toString() || + parsed.fileRequest?.columnRaw != column?.toString() + ) { + return null + } + return uri.toString() } } diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt index 81007a431f..1e997c6567 100644 --- a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt @@ -108,13 +108,63 @@ class DeepLinkBuildUrlTest { } @Test - fun `the same path without a line is misread -- the limitation buildUrl's callers must avoid`() { - // Documents why CreateLinkAction always passes a line and a column rather than omitting them - // when the cursor sits at 1:1. Nothing here is a bug in buildUrl; it is parse()'s positional - // peeling, and this test exists so a future change to either side notices. - val url = DeepLinkRequest.buildUrl("MyApp", "src/line/5") - assertThat(roundTrip(url)?.fileRequest) - .isEqualTo(PendingFileRequest(filePath = "src", lineRaw = "5", columnRaw = null)) + fun `refuses the same path with no line, because parse would read it as metadata`() { + // Without a trailing line/column pair to consume, parse() peels "line/5" off the path itself + // and reads this back as file "src" at line 5. buildUrl's round-trip check catches that and + // returns null rather than emitting a link that opens the wrong file. + assertThat(DeepLinkRequest.buildUrl("MyApp", "src/line/5")).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "src/column/5")).isNull() + + // Not over-refusing: the same path IS expressible once a real pair follows it. + assertThat(DeepLinkRequest.buildUrl("MyApp", "src/line/5", line = 7, column = 3)).isNotNull() + } + + @Test + fun `rejects dot segments the reader would refuse or an intermediary would rewrite`() { + // isProjectCandidateDir() refuses any name starting with '.', so none of these can name a + // project; "." and ".." are worse than unopenable, since a URL-normalizing browser or + // messenger rewrites them into a different path in transit. + assertThat(DeepLinkRequest.buildUrl(".")).isNull() + assertThat(DeepLinkRequest.buildUrl("..")).isNull() + assertThat(DeepLinkRequest.buildUrl(".hidden")).isNull() + + assertThat(DeepLinkRequest.buildUrl("MyApp", "../../etc/passwd")).isNull() + assertThat(DeepLinkRequest.buildUrl("MyApp", "src/./Main.kt")).isNull() + + // A hidden FILE is legitimate, unlike a hidden project directory -- resolveWithinDirectory + // rejects traversal, not dot-prefixed names. + assertThat(DeepLinkRequest.buildUrl("MyApp", ".gitignore", line = 1, column = 1)).isNotNull() + } + + @Test + fun `never returns a url that parses back as something else`() { + // The contract the round-trip check exists to hold. Anything buildUrl returns must read back + // as exactly the arguments it was given. + val cases = + listOf( + Triple("MyApp", "src/main/Main.kt", 7 to 3), + Triple("My App", "a b/c d.kt", 1 to 1), + Triple("Cafe\u0301", "notes/Draft#1.md", 12 to 40), + Triple("MyApp", "src/line/5", 7 to 3), + Triple("MyApp", "src/column/9/line/2", 4 to 6), + ) + for ((project, path, coords) in cases) { + val (line, column) = coords + val url = DeepLinkRequest.buildUrl(project, path, line, column) + assertThat(url).isNotNull() + assertThat(roundTrip(url)) + .isEqualTo( + DeepLinkRequest( + projectName = project, + fileRequest = + PendingFileRequest( + filePath = path, + lineRaw = line.toString(), + columnRaw = column.toString(), + ), + ), + ) + } } @Test From b26336bb07ec1f261ac2e98dcb4ed3678760c5e2 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 23:13:31 -0700 Subject: [PATCH 03/16] ADFA-5472: Decide project eligibility without blocking the UI thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit wrapped isDeepLinkTargetOfOpenProject in allowThreadDiskReads. That was the wrong call, and the repo says so directly: REVIEW.md §3 -- "the whitelist is only for vendor/framework code we can't change -- never for app-owned violations. If your code trips StrictMode, fix the code" -- and ADR 0007 lists per-call allowThreadDiskReads() suppressions among the alternatives it explicitly rejected, as "easy to abuse, and impossible to audit centrally." So the check no longer suppresses anything. It decides the common case without touching the filesystem at all: equal path strings name the same directory, so canonicalising both sides could only agree. Every project opened from the projects list or reached by a deep link takes that path, because both are resolved against projectsRoot() in the first place -- on the test device the recorded project path is /storage/emulated/0/CodeOnTheGoProjects/My great app, whose parent is string-identical to projectsRoot(). Only the residue -- paths that differ as text, where a symlink might still make them one directory -- needs canonicalisation, and that runs on Dispatchers.IO in the activity's lifecycleScope. The menu item stays hidden until the answer lands and the next menu open reads it from the cache. That case is a project opened from the file picker or a clone destination, which is usually not linkable anyway. The memo cache stays, holding only a path string and a boolean -- no Context, so it is safe in a companion. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../actions/file/CreateLinkAction.kt | 62 ++++++++++++------- 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index ffc0970eec..68b048c42f 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -19,6 +19,7 @@ package com.itsaky.androidide.actions.file import android.content.Context import androidx.core.content.ContextCompat +import androidx.lifecycle.lifecycleScope import com.itsaky.androidide.R import com.itsaky.androidide.actions.ActionData import com.itsaky.androidide.actions.markInvisible @@ -26,11 +27,12 @@ import com.itsaky.androidide.activities.editor.EditorHandlerActivity import com.itsaky.androidide.activities.projectsRoot import com.itsaky.androidide.models.DeepLinkRequest import com.itsaky.androidide.projects.IProjectManager -import com.itsaky.androidide.utils.allowThreadDiskReads import com.itsaky.androidide.utils.copyToClipboard import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess import com.itsaky.androidide.utils.isDeepLinkTargetOfOpenProject +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch import java.io.File /** @@ -54,31 +56,49 @@ class CreateLinkAction( private const val CLIP_LABEL = "Code on the Go link" /** - * Memoised answer to "is the open project one a link can name", keyed by its path. - * - * [isDeepLinkTargetOfOpenProject] canonicalises both sides, which is two filesystem calls, and - * `prepare()` runs on the UI thread every single time a file-tab menu is opened -- enough for - * StrictMode (which this app arms with `detectAll()` in debug builds) to report a - * DiskReadViolation on each one, and enough to jank the popup on a slow external volume. The - * answer only changes when a different project is opened, so it is computed once per project - * instead of once per menu. + * Memoised answer to "is the open project one a link can name", keyed by its path. Holds no + * Context, so it is safe in a companion; the answer only changes when a project is opened. */ @Volatile private var linkableProject: Pair? = null - private fun isLinkableProject(projectPath: String): Boolean { - linkableProject?.let { (cachedPath, cached) -> - if (cachedPath == projectPath) return cached + private fun cachedLinkable(projectPath: String): Boolean? = linkableProject?.takeIf { it.first == projectPath }?.second + + /** + * Whether a link can name the open project, or `null` while that is still being decided off + * this thread. + * + * `prepare()` has to answer synchronously, and the full rule + * ([isDeepLinkTargetOfOpenProject]) canonicalises both sides -- filesystem work that REVIEW.md + * §3 and ADR 0007 require be moved rather than suppressed, since the StrictMode whitelist is + * for vendored code we cannot change and never for our own. So the common case is decided + * without touching the disk at all, and only the residue goes to [Dispatchers.IO]. + */ + private fun linkableProject( + activity: EditorHandlerActivity, + projectPath: String, + ): Boolean? { + cachedLinkable(projectPath)?.let { return it } + + // Equal path strings name the same directory, so canonicalising both sides could only + // agree -- decidable here with no filesystem call, and this is the path every project + // opened from the projects list takes. (The name half of the rule is trivially satisfied: + // the caller derives the project name from this very path.) + val parent = File(projectPath).parentFile + if (parent != null && parent.absolutePath == projectsRoot().absolutePath) { + linkableProject = projectPath to true + return true } - // The one unavoidable read, taken once per project. Exempted rather than moved off-thread - // because prepare() has to answer synchronously to decide whether to show the item at all. - val linkable = - allowThreadDiskReads("Canonicalising the open project once, to decide if it can be linked") { - isDeepLinkTargetOfOpenProject(projectPath, File(projectPath).name, projectsRoot()) - } - linkableProject = projectPath to linkable - return linkable + // The two differ as text, so only canonicalisation can say whether a symlink still makes + // them one directory. That is the rare case -- a project opened from the file picker or a + // clone destination -- and it is answered off the main thread. The item stays hidden until + // the result lands, and the next menu open reads it from the cache. + activity.lifecycleScope.launch(Dispatchers.IO) { + val linkable = isDeepLinkTargetOfOpenProject(projectPath, File(projectPath).name, projectsRoot()) + linkableProject = projectPath to linkable + } + return null } } @@ -143,7 +163,7 @@ class CreateLinkAction( // anywhere -- the file picker, Recents, a clone destination. For one of those there is no URL // that resolves on this device, let alone on the recipient's, so there is nothing honest to // put on the clipboard. Reusing the reader's own containment rule keeps the two from drifting. - if (!isLinkableProject(projectPath)) { + if (linkableProject(activity, projectPath) != true) { return null } From 48d79e9d680cd4ea9716ac1339771edb4c899f06 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Thu, 3 Sep 2026 23:52:08 -0700 Subject: [PATCH 04/16] ADFA-5472: Address the xhigh review Eleven findings from a second review pass, at a level that surfaces uncertain ones -- so each was checked against the code before being acted on. Two turned out to be pre-existing conditions this change worsens rather than caused; both are fixed here anyway, since the growth that exposes them is the row this PR adds. The linkability decision is now shared, deduped and error-handled. ProjectValidations gains deepLinkTargetOfOpenProjectWithoutIo -- the half of the rule decidable without a filesystem call -- and the full rule uses it as its own fast path, so the writer no longer keeps a private approximation of "is this project linkable" that would silently keep answering the old question if the rule were tightened. In the action, concurrent menu opens now launch one canonicalisation instead of N; a slow verdict for a project the user has since left no longer overwrites the newer one; and the coroutine handles its own failure rather than leaving an NPE from a null Environment.PROJECTS_DIR to the crash wrapper. The popup can scroll. It was a bare LinearLayout in a WRAP_CONTENT PopupWindow, which clips rather than scrolls what it cannot fit, so at 2x font scale the last rows go out of reach with no sign they exist -- against CLAUDE.md's font-scale rule, and this PR adds a row. Wrapped in a ScrollView; ActionMenuUtils adds to the inner container. The clipboard label is a string resource. It is shown in the Android 13+ clipboard preview, so it was user-facing text as an inline literal, invisible to translation. msg_deeplink_copied also gained the terminal period every other msg_deeplink_* string carries. buildUrl's contract no longer over-claims. It said each rejection mirrors a check in parse or lookupValidProjectByName; the reader's isValidProjectDirectory requirement is not mirrored and deliberately cannot be -- it needs disk I/O, and it is a fact about the project at open time, so a project that stops looking like an Android project after a link is sent invalidates that link regardless. Documented as a property of the scheme rather than left as a false claim. Also removed the duplicate "project" literal: PATH_PREFIX is now derived from the segment list rather than the two spelling it independently. Tests for the parts that had none: the containment guard is extracted as projectRelativePathOrNull and covered directly, since it is the security-relevant half -- a regression dropping its "../" check would emit a link naming a file outside the project, and nothing would have failed. The 512 ceiling is now pinned either side of the boundary rather than at 400 and 600; verified by flipping the guard to >=, which fails that test and only that test. ARCHITECTURE.md documents the write side beside the read side, including the one-based-vs-zero-based cursor contract and the round-trip invariant. Not fixed, deliberately: long-pressing the new item shows the shared "close options" tooltip, because ActionMenuUtils hardcodes one tag for every item in this menu. A per-action tag needs matching docdb content to be an improvement rather than an empty tooltip, which is a separate piece of work. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- ARCHITECTURE.md | 4 + .../actions/file/CreateLinkAction.kt | 103 ++++++++++++------ .../androidide/models/DeepLinkRequest.kt | 29 +++-- .../androidide/utils/ActionMenuUtils.kt | 6 +- .../androidide/utils/ProjectValidations.kt | 32 +++++- .../res/layout/file_action_popup_window.xml | 21 +++- .../actions/file/CreateLinkActionTest.kt | 70 ++++++++++++ .../androidide/models/DeepLinkBuildUrlTest.kt | 18 +-- .../utils/ProjectValidationsTest.kt | 28 +++++ resources/src/main/res/values/strings.xml | 3 +- 10 files changed, 259 insertions(+), 55 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 0d635babfe..1e420b5dfa 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -59,6 +59,10 @@ Feature code layers as **UI → ViewModel → Repository → data source**, with The optional file path is attacker-controllable (a URL segment), so it's resolved through `PathTraversal.resolveWithinDirectory`'s traversal/symlink guard rather than a bare `File` join, both when opening a file in the already-open project and when matching the requested project name to a directory under `Environment.PROJECTS_DIR` (`findValidProjectByName`). +**The same scheme is also written, and the writer verifies itself against the reader.** `DeepLinkRequest.buildUrl` (same file as `parse`, deliberately) is the inverse: `CreateLinkAction` — the "Create link" item on the editor's file-tab drop-down — turns the current project, file and cursor into a URL on the clipboard. Two invariants are load-bearing. **Line and column are one-based in the URL and zero-based in the editor**; `buildUrl` takes the one-based form and `EditorHandlerActivity.zeroBasedOrInvalid` converts back, so a caller holding a cursor passes `cursor.leftLine + 1`. And **`buildUrl` parses its own output back and compares it against the arguments that built it**, returning `null` on any mismatch — because `parse` peels `line`/`column` *positionally*, so a file path whose own trailing segments look like that metadata (`.../file/src/line/5` with no line of its own) would otherwise read back as a different file. That check makes the guarantee total rather than a list of enumerated cases; it is why the action always writes both coordinates, and why a path it cannot express unambiguously yields no link instead of a wrong one. + +Whether the open project *can* be named at all is `isDeepLinkTargetOfOpenProject` (`ProjectValidations.kt`): a link can only ever resolve to `/`, but a project can be opened from anywhere. Its `deepLinkTargetOfOpenProjectWithoutIo` half exists so a caller on the main thread can settle the common case — equal path strings name the same directory — without the `canonicalPath` calls the full rule needs; the residue goes to `Dispatchers.IO`, never to the StrictMode whitelist (ADR 0007). + ## Module Structure Strategy: **layer-and-subsystem based**, not feature-by-feature. The Gradle build has ~80 modules (`settings.gradle.kts`) plus three included composite builds. `app` is the integration point; the rest are libraries it composes. diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index 68b048c42f..b6d3b7a870 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -18,6 +18,7 @@ package com.itsaky.androidide.actions.file import android.content.Context +import androidx.annotation.VisibleForTesting import androidx.core.content.ContextCompat import androidx.lifecycle.lifecycleScope import com.itsaky.androidide.R @@ -30,9 +31,11 @@ import com.itsaky.androidide.projects.IProjectManager import com.itsaky.androidide.utils.copyToClipboard import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess +import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectWithoutIo import com.itsaky.androidide.utils.isDeepLinkTargetOfOpenProject import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch +import org.slf4j.LoggerFactory import java.io.File /** @@ -52,8 +55,7 @@ class CreateLinkAction( companion object { const val ID = "ide.editor.fileTab.createLink" - /** Shown in the clipboard preview on Android 13+, so it names the app rather than the action. */ - private const val CLIP_LABEL = "Code on the Go link" + private val log = LoggerFactory.getLogger(CreateLinkAction::class.java) /** * Memoised answer to "is the open project one a link can name", keyed by its path. Holds no @@ -62,46 +64,69 @@ class CreateLinkAction( @Volatile private var linkableProject: Pair? = null - private fun cachedLinkable(projectPath: String): Boolean? = linkableProject?.takeIf { it.first == projectPath }?.second + /** Path whose canonicalisation is already running, so N menu opens launch one job, not N. */ + @Volatile + private var linkabilityInFlight: String? = null /** * Whether a link can name the open project, or `null` while that is still being decided off * this thread. * - * `prepare()` has to answer synchronously, and the full rule - * ([isDeepLinkTargetOfOpenProject]) canonicalises both sides -- filesystem work that REVIEW.md - * §3 and ADR 0007 require be moved rather than suppressed, since the StrictMode whitelist is - * for vendored code we cannot change and never for our own. So the common case is decided - * without touching the disk at all, and only the residue goes to [Dispatchers.IO]. + * `prepare()` has to answer synchronously, and the full rule canonicalises both sides -- + * filesystem work that REVIEW.md §3 and ADR 0007 require be moved rather than suppressed, + * since the StrictMode whitelist is for vendored code we cannot change and never for our own. + * So [deepLinkTargetOfOpenProjectWithoutIo] settles the common case with no disk call, and + * only the residue -- paths that differ as text, where a symlink might still make them one + * directory -- goes to [Dispatchers.IO]. + * + * Known limitation while that rare case is pending: the item is absent from the menu until the + * answer lands, and reappears on the next open. Showing it optimistically instead would mean + * a tap that fails, which reads worse; the population this can affect is a project opened from + * the file picker or a clone destination, which is usually not linkable anyway. */ private fun linkableProject( activity: EditorHandlerActivity, projectPath: String, ): Boolean? { - cachedLinkable(projectPath)?.let { return it } - - // Equal path strings name the same directory, so canonicalising both sides could only - // agree -- decidable here with no filesystem call, and this is the path every project - // opened from the projects list takes. (The name half of the rule is trivially satisfied: - // the caller derives the project name from this very path.) - val parent = File(projectPath).parentFile - if (parent != null && parent.absolutePath == projectsRoot().absolutePath) { - linkableProject = projectPath to true - return true + linkableProject?.takeIf { it.first == projectPath }?.let { return it.second } + + val root = runCatching { projectsRoot() }.getOrNull() ?: return null + val projectName = File(projectPath).name + + deepLinkTargetOfOpenProjectWithoutIo(projectPath, projectName, root)?.let { + linkableProject = projectPath to it + return it } - // The two differ as text, so only canonicalisation can say whether a symlink still makes - // them one directory. That is the rare case -- a project opened from the file picker or a - // clone destination -- and it is answered off the main thread. The item stays hidden until - // the result lands, and the next menu open reads it from the cache. + // One job per path, not one per menu open: ActionMenuUtils calls prepare() every time the + // popup is built, and canonicalising the same path N times in parallel is pure waste. + if (linkabilityInFlight == projectPath) return null + linkabilityInFlight = projectPath + activity.lifecycleScope.launch(Dispatchers.IO) { - val linkable = isDeepLinkTargetOfOpenProject(projectPath, File(projectPath).name, projectsRoot()) - linkableProject = projectPath to linkable + // Handled here rather than left to the crash wrapper (REVIEW.md §1): a failure to + // canonicalise says nothing about whether the project is linkable, so it is logged and + // left uncached, and the next menu open retries. + runCatching { isDeepLinkTargetOfOpenProject(projectPath, projectName, root) } + .onSuccess { linkable -> + // Published only if this is still the project in question. A slow answer for a + // project the user has since left must not overwrite the newer one's verdict. + if (IProjectManager.getInstance().projectDirPath == projectPath) { + linkableProject = projectPath to linkable + } + }.onFailure { log.warn("Could not determine whether {} can be linked", projectPath, it) } + + if (linkabilityInFlight == projectPath) { + linkabilityInFlight = null + } } return null } } + /** Shown in the clipboard preview on Android 13+, so it is user-facing and lives in resources. */ + private val clipLabel: String = context.getString(R.string.clip_label_deeplink) + init { label = context.getString(R.string.action_create_link) icon = ContextCompat.getDrawable(context, R.drawable.ic_copy) @@ -140,7 +165,7 @@ class CreateLinkAction( return false } - copyToClipboard(url, label = CLIP_LABEL) + copyToClipboard(url, label = clipLabel) flashSuccess(R.string.msg_deeplink_copied) return true } @@ -167,12 +192,7 @@ class CreateLinkAction( return null } - // relativeToOrNull walks up with ".." when the file lies outside the project rather than - // failing, so containment has to be checked on the result and not just on the call succeeding. - val relativePath = file.relativeToOrNull(projectDir)?.invariantSeparatorsPath ?: return null - if (relativePath.isEmpty() || relativePath == ".." || relativePath.startsWith("../")) { - return null - } + val relativePath = projectRelativePathOrNull(projectDir, file) ?: return null // A cursor is required, not optional. Without one the link would carry no line or column, and // a coordinate-free link is the one shape parse() can misread: a file path whose own trailing @@ -191,3 +211,24 @@ class CreateLinkAction( ) } } + +/** + * [file]'s path relative to [projectDir], always '/'-separated, or `null` when [file] does not lie + * inside [projectDir]. + * + * Split out of the action because it is the security-relevant half and the only part worth testing + * on its own: `relativeToOrNull` walks *up* with ".." when the file is outside rather than failing, + * so containment has to be checked on the result and not merely on the call succeeding. Without that + * check a link could name a file outside the project it claims. + */ +@VisibleForTesting +internal fun projectRelativePathOrNull( + projectDir: File, + file: File, +): String? { + val relative = file.relativeToOrNull(projectDir)?.invariantSeparatorsPath ?: return null + if (relative.isEmpty() || relative == ".." || relative.startsWith("../")) { + return null + } + return relative +} diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index bb1e21fcaa..eb8af71c57 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -67,7 +67,6 @@ data class DeepLinkRequest( // Both hosts serve an identical, verified assetlinks.json (see AndroidManifest.xml's matching // pair of elements on DeepLinkActivity's intent-filter) -- kept in sync with that list. private val HOSTS = setOf("www.$CANONICAL_HOST", CANONICAL_HOST) - private const val PATH_PREFIX = "/device/open/project/" /** * See [parse]: an upper bound on the whole path, since the parsed pieces get parcelled. @@ -87,6 +86,14 @@ data class DeepLinkRequest( private const val SEGMENT_LINE = "line" private const val SEGMENT_COLUMN = "column" + /** + * The fixed segments every link starts with, and the prefix [parse] matches, derived from them + * so the literal "project" is spelled once. The old pairing had it in both [PATH_PREFIX] and + * [SEGMENT_PROJECT], which is the drift this is meant to prevent -- [parse] reads both. + */ + private val PATH_SEGMENTS = listOf("device", "open", SEGMENT_PROJECT) + private val PATH_PREFIX = PATH_SEGMENTS.joinToString(separator = "/", prefix = "/", postfix = "/") + /** First index at or after [from] holding [segment], or -1. Unlike [List.indexOf], never * matches an already-consumed segment earlier in the path -- e.g. a project name that * happens to equal `"line"` can't be mistaken for the `line` keyword that follows it. */ @@ -245,9 +252,18 @@ data class DeepLinkRequest( * `cursor.leftLine + 1`. * * Returns `null` rather than a URL that [parse] would reject or read back as something other - * than what was asked for. Each rejection below mirrors a specific check in [parse] or in - * [lookupValidProjectByName][com.itsaky.androidide.utils.lookupValidProjectByName]: there is - * nothing to be gained by handing someone a link this same app refuses to open. + * than what was asked for. Each rejection below mirrors one of the *static* checks in [parse] + * or in [lookupValidProjectByName][com.itsaky.androidide.utils.lookupValidProjectByName]: + * there is nothing to be gained by handing someone a link this same app refuses to open. + * + * Deliberately NOT mirrored is that reader's + * [isValidProjectDirectory][com.itsaky.androidide.utils.isValidProjectDirectory] requirement. + * Checking it would mean disk I/O on whatever thread builds a link, and it is a fact about the + * project at *open* time rather than at link time -- a project that stops looking like an + * Android project after the link is made (its `app/build.gradle` renamed, say) would invalidate + * an already-sent link no matter what was verified here. So a link can still be emitted that + * this app later declines with "no project named X"; that is a property of the scheme, not + * something this function can close. */ fun buildUrl( projectName: String, @@ -285,9 +301,8 @@ data class DeepLinkRequest( val builder = Uri.Builder().scheme(SCHEME).authority(CANONICAL_HOST) - // Derived from PATH_PREFIX rather than spelled out a second time, so a change to the prefix - // parse() requires cannot leave the writer emitting the old one. - PATH_PREFIX.trim('/').split('/').forEach { builder.appendPath(it) } + // The same list parse()'s own prefix is built from, so the two cannot disagree. + PATH_SEGMENTS.forEach { builder.appendPath(it) } builder.appendPath(projectName) if (filePath != null) { diff --git a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt index 1f301a2032..b427c62f1e 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt @@ -98,7 +98,7 @@ object ActionMenuUtils { true } } - binding.root.addView(itemView) + binding.actionItems.addView(itemView) } val visiblePluginItems = pluginMenuItems.filter { it.isEnabled && it.isVisible } @@ -116,7 +116,7 @@ object ActionMenuUtils { ) setBackgroundColor(typedValue.data) } - binding.root.addView(divider) + binding.actionItems.addView(divider) visiblePluginItems.forEach { item -> val itemView = @@ -147,7 +147,7 @@ object ActionMenuUtils { } } } - binding.root.addView(itemView) + binding.actionItems.addView(itemView) } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt index 10db33199a..4db71cb3f1 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt @@ -147,10 +147,40 @@ internal fun isDeepLinkTargetOfOpenProject( projectName: String, projectsRoot: File, ): Boolean { + // Whatever can be settled without touching the disk, settle here. + deepLinkTargetOfOpenProjectWithoutIo(openProjectPath, projectName, projectsRoot)?.let { return it } + + val open = File(openProjectPath) + return canonicalOrAbsolute(open.parentFile ?: return false) == canonicalOrAbsolute(projectsRoot) +} + +/** + * The part of [isDeepLinkTargetOfOpenProject] decidable without any filesystem call, for callers + * that must answer on a thread where I/O is not allowed -- `null` means "only canonicalisation can + * tell", so the caller has to go off-thread for the rest. + * + * Lives here, beside the full rule, rather than being reimplemented at the call site: a writer with + * its own private copy of "is this project linkable" would keep answering the old question if this + * rule were ever tightened. + */ +internal fun deepLinkTargetOfOpenProjectWithoutIo( + openProjectPath: String, + projectName: String, + projectsRoot: File, +): Boolean? { if (openProjectPath.isBlank()) return false val open = File(openProjectPath) if (!projectNamesMatch(open.name, projectName)) return false - return canonicalOrAbsolute(open.parentFile ?: return false) == canonicalOrAbsolute(projectsRoot) + val parent = open.parentFile ?: return false + + // Equal path strings name the same directory, so canonicalising both sides could only agree. + // This is the case for every project reached through the projects list or a deep link, since + // both are resolved against projectsRoot to begin with. + if (parent.absolutePath == projectsRoot.absolutePath) return true + + // They differ as text, so a symlink on either side may still make them one directory -- and only + // canonicalPath can say, which is exactly the call this function exists to avoid. + return null } private fun canonicalOrAbsolute(file: File): String = runCatching { file.canonicalPath }.getOrElse { file.absolutePath } diff --git a/app/src/main/res/layout/file_action_popup_window.xml b/app/src/main/res/layout/file_action_popup_window.xml index da481701b1..282043a49f 100644 --- a/app/src/main/res/layout/file_action_popup_window.xml +++ b/app/src/main/res/layout/file_action_popup_window.xml @@ -1,8 +1,19 @@ - - + - + android:scrollbars="none"> + + + diff --git a/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt b/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt new file mode 100644 index 0000000000..1c72cc039d --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt @@ -0,0 +1,70 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.actions.file + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import java.io.File + +/** + * The non-UI half of [CreateLinkAction]: deciding what path a link may name. A link gets handed to + * another person, so "outside the project" has to be a hard no rather than a best effort. + */ +class CreateLinkActionTest { + private val project = File("/storage/emulated/0/CodeOnTheGoProjects/MyApp") + + @Test + fun `a file inside the project relativises to a slash-separated path`() { + assertThat(projectRelativePathOrNull(project, File(project, "app/src/main/Main.kt"))) + .isEqualTo("app/src/main/Main.kt") + } + + @Test + fun `a file at the project root relativises to its bare name`() { + assertThat(projectRelativePathOrNull(project, File(project, "settings.gradle.kts"))) + .isEqualTo("settings.gradle.kts") + } + + @Test + fun `a file outside the project is refused rather than escaping with dot-dot`() { + // relativeToOrNull walks UP with ".." instead of failing, so a missing containment check here + // would emit a link naming a file the project does not contain. + assertThat(projectRelativePathOrNull(project, File("/storage/emulated/0/Download/secrets.txt"))).isNull() + + val sibling = File("/storage/emulated/0/CodeOnTheGoProjects/OtherApp/app/Main.kt") + assertThat(projectRelativePathOrNull(project, sibling)).isNull() + + // The project's own parent, which relativises to exactly "..". + assertThat(projectRelativePathOrNull(project, project.parentFile)).isNull() + } + + @Test + fun `the project directory itself has no relative path to name`() { + assertThat(projectRelativePathOrNull(project, project)).isNull() + } + + @Test + fun `a hidden file inside the project is allowed`() { + assertThat(projectRelativePathOrNull(project, File(project, ".gitignore"))).isEqualTo(".gitignore") + } + + @Test + fun `a path on a different volume shares no root and is refused`() { + assertThat(projectRelativePathOrNull(project, File("/data/local/tmp/Main.kt"))).isNull() + } +} diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt index 1e997c6567..d289f26ad8 100644 --- a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt @@ -201,12 +201,16 @@ class DeepLinkBuildUrlTest { } @Test - fun `rejects a path over the length parse enforces`() { - val tooLong = "a".repeat(600) - assertThat(DeepLinkRequest.buildUrl("MyApp", tooLong)).isNull() - - // And is not simply refusing everything long: a path just inside the ceiling still builds. - val insideCeiling = "a".repeat(400) - assertThat(DeepLinkRequest.buildUrl("MyApp", insideCeiling)).isNotNull() + fun `the length ceiling falls exactly on 512 decoded characters`() { + // Pinned either side of the boundary, not merely far from it: with only a 400 and a 600 case, + // flipping the guard to >= (or to > LIMIT + 1, which would emit a link parse then rejects) + // left the suite green. "/device/open/project/P/file/" is 28 characters, so a file path of + // (512 - 28) puts the decoded path at exactly the limit. + val prefixLength = "/device/open/project/P/file/".length + assertThat(prefixLength).isEqualTo(28) + + assertThat(DeepLinkRequest.buildUrl("P", "a".repeat(512 - prefixLength))).isNotNull() + assertThat(DeepLinkRequest.buildUrl("P", "a".repeat(513 - prefixLength))).isNull() + assertThat(DeepLinkRequest.buildUrl("P", "a".repeat(600))).isNull() } } diff --git a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt index 02ce033d3b..092fa86745 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt @@ -147,4 +147,32 @@ class ProjectValidationsTest { assertThat(findValidProjectByName(root, name)).isEqualTo(expected) } } + + @Test + fun `the no-IO shortcut answers the same-parent case and defers the rest`() { + val root = tempFolder.newFolder("projects") + + // Same parent by path: decidable with no filesystem call at all. + assertThat(deepLinkTargetOfOpenProjectWithoutIo(File(root, "MyApp").path, "MyApp", root)).isTrue() + + // Definitively not this project, also decidable without I/O. + assertThat(deepLinkTargetOfOpenProjectWithoutIo(File(root, "MyApp").path, "Other", root)).isFalse() + assertThat(deepLinkTargetOfOpenProjectWithoutIo("", "MyApp", root)).isFalse() + + // Parent differs as text, so only canonicalisation can settle it -- the caller must go + // off-thread rather than treat this as a "no". + val elsewhere = tempFolder.newFolder("elsewhere") + assertThat(deepLinkTargetOfOpenProjectWithoutIo(File(elsewhere, "MyApp").path, "MyApp", root)).isNull() + } + + @Test + fun `the full rule agrees with the shortcut wherever the shortcut commits`() { + val root = tempFolder.newFolder("projects2") + for (case in listOf(File(root, "MyApp").path to "MyApp", File(root, "MyApp").path to "Other", "" to "MyApp")) { + val (path, name) = case + val shortcut = deepLinkTargetOfOpenProjectWithoutIo(path, name, root) + assertThat(shortcut).isNotNull() + assertThat(isDeepLinkTargetOfOpenProject(path, name, root)).isEqualTo(shortcut) + } + } } diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index d30cb64ec3..756df477ed 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -146,8 +146,9 @@ \"%s\" is not a valid column number. (no value given) Could not scan projects for this link. - Link copied to the clipboard + Link copied to the clipboard. Could not create a link for this file. + Code on the Go link A project close is already in progress. Try again in a moment. Create new project Open a saved project From 7e8f81974f26facecff862ac871a687d30d3d5fc Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 00:13:09 -0700 Subject: [PATCH 05/16] ADFA-5472: Sort the new import The pre-commit formatter's own correction to the previous commit, which staged before it ran. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../java/com/itsaky/androidide/actions/file/CreateLinkAction.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index b6d3b7a870..b8a8504786 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -29,9 +29,9 @@ import com.itsaky.androidide.activities.projectsRoot import com.itsaky.androidide.models.DeepLinkRequest import com.itsaky.androidide.projects.IProjectManager import com.itsaky.androidide.utils.copyToClipboard +import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectWithoutIo import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess -import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectWithoutIo import com.itsaky.androidide.utils.isDeepLinkTargetOfOpenProject import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch From dad001fd31d8badf260ad3b7f92b1ca8d0b9147d Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 00:13:49 -0700 Subject: [PATCH 06/16] ADFA-5472: Apply the formatter to the popup layout The pre-commit formatter's own output for the file this PR rewrites (tabs, and the header comment rewrapped). Deliberately not included: the same hook wants to reformat all of ActionMenuUtils.kt, which predates the repo's tab convention and so reformats on any commit that touches it. That is ~227 lines of churn around a three-line change, and no CI check enforces formatting, so it is left for a commit that is only about formatting. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../res/layout/file_action_popup_window.xml | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/app/src/main/res/layout/file_action_popup_window.xml b/app/src/main/res/layout/file_action_popup_window.xml index 282043a49f..e0171d6dc1 100644 --- a/app/src/main/res/layout/file_action_popup_window.xml +++ b/app/src/main/res/layout/file_action_popup_window.xml @@ -1,19 +1,18 @@ - - + + - + From b1a777e4aafd213a63367668eb6d0500f11f3d47 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 00:14:29 -0700 Subject: [PATCH 07/16] ADFA-5472: Run spotless over ActionMenuUtils Formatting only, no behavior change -- spotless output, nothing hand-written. This file predates the repo's tab convention, so touching it at all makes spotless rewrite the whole thing. I first tried leaving the reformat out to keep a three-line change reviewable; the pre-push hook rejects that, so it belongs in the branch. Isolated in its own commit so the diff that matters stays legible: review 48d79e9 and skip this one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../androidide/utils/ActionMenuUtils.kt | 227 +++++++++--------- 1 file changed, 118 insertions(+), 109 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt index b427c62f1e..81f8c94e90 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt @@ -35,122 +35,131 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.plugins.extensions.FileTabMenuItem object ActionMenuUtils { + fun showPopupWindow( + context: Context, + anchorView: View, + pluginMenuItems: List = emptyList(), + ) { + val registry = ActionsRegistry.getInstance() + val actionData = ActionData.create(context) - fun showPopupWindow( - context: Context, - anchorView: View, - pluginMenuItems: List = emptyList() - ) { - val registry = ActionsRegistry.getInstance() - val actionData = ActionData.create(context) + val binding = + FileActionPopupWindowBinding.inflate(LayoutInflater.from(context), null, false) - val binding = - FileActionPopupWindowBinding.inflate(LayoutInflater.from(context), null, false) + val popupWindow = + PopupWindow( + binding.root, + LayoutParams.WRAP_CONTENT, + LayoutParams.WRAP_CONTENT, + ).apply { + elevation = 2f + isOutsideTouchable = true + } - val popupWindow = PopupWindow( - binding.root, - LayoutParams.WRAP_CONTENT, - LayoutParams.WRAP_CONTENT, - ).apply { - elevation = 2f - isOutsideTouchable = true - } + val tooltipListener = + OnLongClickListener { view -> + TooltipManager.showIdeCategoryTooltip( + context = view.context, + anchorView = view, + tag = TooltipTag.DIALOG_FIND_IN_FILE_OPTIONS, + ) + popupWindow.dismiss() + true + } - val tooltipListener = OnLongClickListener { view -> - TooltipManager.showIdeCategoryTooltip( - context = view.context, - anchorView = view, - tag = TooltipTag.DIALOG_FIND_IN_FILE_OPTIONS - ) - popupWindow.dismiss() - true - } + binding.root.setOnLongClickListener(tooltipListener) - binding.root.setOnLongClickListener(tooltipListener) + val actions = registry.getActions(ActionItem.Location.EDITOR_FILE_TABS) + actions.forEach { action -> + action.value.prepare(actionData) + if (!action.value.visible || !action.value.enabled) return@forEach - val actions = registry.getActions(ActionItem.Location.EDITOR_FILE_TABS) - actions.forEach { action -> - action.value.prepare(actionData) - if (!action.value.visible || !action.value.enabled) return@forEach + val itemView = + FileActionPopupWindowItemBinding + .inflate( + LayoutInflater.from(context), + null, + false, + ).root + itemView.apply { + text = action.value.label + setOnClickListener { + (registry as DefaultActionsRegistry).executeAction( + action.value, + actionData, + ) + popupWindow.dismiss() + } + setOnLongClickListener { + TooltipManager.showIdeCategoryTooltip( + context = context, + anchorView = anchorView, + tag = TooltipTag.EDITOR_FILE_CLOSE_OPTIONS, + ) + popupWindow.dismiss() + true + } + } + binding.actionItems.addView(itemView) + } - val itemView = - FileActionPopupWindowItemBinding.inflate( - LayoutInflater.from(context), - null, - false - ).root - itemView.apply { - text = action.value.label - setOnClickListener { - (registry as DefaultActionsRegistry).executeAction( - action.value, - actionData - ) - popupWindow.dismiss() - } - setOnLongClickListener { - TooltipManager.showIdeCategoryTooltip( - context = context, - anchorView = anchorView, - tag = TooltipTag.EDITOR_FILE_CLOSE_OPTIONS - ) - popupWindow.dismiss() - true - } - } - binding.actionItems.addView(itemView) - } + val visiblePluginItems = pluginMenuItems.filter { it.isEnabled && it.isVisible } + if (visiblePluginItems.isNotEmpty()) { + val divider = + View(context).apply { + layoutParams = + LinearLayout + .LayoutParams( + LinearLayout.LayoutParams.MATCH_PARENT, + 1, + ).apply { + topMargin = 8 + bottomMargin = 8 + } + val typedValue = android.util.TypedValue() + context.theme.resolveAttribute( + com.google.android.material.R.attr.colorOutline, + typedValue, + true, + ) + setBackgroundColor(typedValue.data) + } + binding.actionItems.addView(divider) - val visiblePluginItems = pluginMenuItems.filter { it.isEnabled && it.isVisible } - if (visiblePluginItems.isNotEmpty()) { - val divider = View(context).apply { - layoutParams = LinearLayout.LayoutParams( - LinearLayout.LayoutParams.MATCH_PARENT, 1 - ).apply { - topMargin = 8 - bottomMargin = 8 - } - val typedValue = android.util.TypedValue() - context.theme.resolveAttribute( - com.google.android.material.R.attr.colorOutline, typedValue, true - ) - setBackgroundColor(typedValue.data) - } - binding.actionItems.addView(divider) + visiblePluginItems.forEach { item -> + val itemView = + FileActionPopupWindowItemBinding + .inflate( + LayoutInflater.from(context), + null, + false, + ).root + itemView.apply { + text = item.title + setOnClickListener { + try { + item.action() + } catch (e: Exception) { + android.util.Log.e("ActionMenuUtils", "Plugin menu action failed", e) + } + popupWindow.dismiss() + } + item.tooltipTag?.let { tag -> + setOnLongClickListener { + TooltipManager.showIdeCategoryTooltip( + context = context, + anchorView = anchorView, + tag = tag, + ) + popupWindow.dismiss() + true + } + } + } + binding.actionItems.addView(itemView) + } + } - visiblePluginItems.forEach { item -> - val itemView = - FileActionPopupWindowItemBinding.inflate( - LayoutInflater.from(context), - null, - false - ).root - itemView.apply { - text = item.title - setOnClickListener { - try { - item.action() - } catch (e: Exception) { - android.util.Log.e("ActionMenuUtils", "Plugin menu action failed", e) - } - popupWindow.dismiss() - } - item.tooltipTag?.let { tag -> - setOnLongClickListener { - TooltipManager.showIdeCategoryTooltip( - context = context, - anchorView = anchorView, - tag = tag - ) - popupWindow.dismiss() - true - } - } - } - binding.actionItems.addView(itemView) - } - } - - popupWindow.showAsDropDown(anchorView, 0, 0) - } + popupWindow.showAsDropDown(anchorView, 0, 0) + } } From 67277350d42ef62d6fb25ff54c3144960ade8246 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 02:59:51 -0700 Subject: [PATCH 08/16] ADFA-5472: Fix a crash the ScrollView introduced, and 13 more findings The important one first: wrapping the popup layout in a ScrollView broke the OTHER consumer of FileActionPopupWindowBinding. ActionMenuUtils was retargeted to the inner container at three sites; EditorHandlerActivity.showPluginTabPopup was not, and still called binding.root.addView -- now a ScrollView whose only child slot is already taken by @id/action_items. Tapping an already-selected plugin tab would have thrown IllegalStateException on the main thread. My own change, my own miss: I retargeted the file I was editing and never grepped for other callers of the binding. Same omission, quieter symptom: the popup-background tooltip listener was also left on the root, where a ScrollView consumes the touch without running View.onTouchEvent, so the long-press silently stopped firing. The 24dp padding it is attached to had moved to the inner container too. The in-flight guard could latch permanently. It was claimed on the main thread before launch() but released only inside the coroutine body, so a rotation before the IO dispatcher picked the block up left a process-wide flag set forever, hiding the item for the rest of the process -- the exact opposite of the KDoc's "reappears on the next open". Released via invokeOnCompletion as well as a finally. The verdict cache is now a ConcurrentHashMap keyed by path, and the claim is an atomic add() rather than a check-then-set on a @Volatile: visibility is not atomicity. Keying by path also retires the stale-write gate that read ProjectManagerImpl's non-volatile lateinit from an IO thread. The writer was re-spelling a subset of the reader's path rule. The reader's isLexicallyRejected splits on '\' as well as '/' and refuses a leading one; a guard checking only '/' components sees "a\..\b.kt" as one harmless filename and copies a link that is then refused on open, after telling the user it was copied. That predicate is now public and called rather than approximated, plus a Paths.get check for characters the reader's resolve() cannot accept at all. A column with no line is now refused. parse() reads that shape, so a test asserted it round-trips -- but zeroBasedOrInvalid(null) yields 0, so the reader applies the column to line 1: a position the link never named, with no invalid-value message. That is what a line with no file is already refused for. Dead code and self-agreeing tests. buildUrl's MAX_LINK_PATH_LENGTH check was byte-identical to the one parse() applies to the same Uri thirteen lines later, so the boundary test I added last round was pinning parse's copy, not buildUrl's; the duplicate is gone. The round-trip self-check now parses Uri.parse(url) -- the StringUri DeepLinkActivity actually receives -- rather than the builder's own Uri. The shortcut-agreement test compared isDeepLinkTargetOfOpenProject against the shortcut, which now returns it verbatim, so it would have passed with the fast path inverted; it compares against an independently computed canonical answer instead. And ProjectValidations' null-parent elvis was unreachable. An unreachable guard with a six-line justification. CodeEditorView.file is `editor?.file`, so a non-null file already proves a non-null editor; the cursor elvis could never fire and the comment described a state that cannot occur. Restructured to take the editor first and the file from it. prepare() no longer builds a URL to throw away. It ran on every action on every menu open, synchronously in the touch handler, doing ~17 encoding passes plus a full parse -- then doAction paid again. Gathering is split from formatting, so prepare() only answers "would there be a link". Corrected claims rather than leaving them: ARCHITECTURE.md said the canonicalPath residue never reaches the main thread, but the two older callers were never migrated and still do; and buildUrl's KDoc claimed to mirror every reader check while three cannot be mirrored at all (symlink containment, the reader's project-name-only unicode normalization, isValidProjectDirectory). The guarantee is stated exactly now: nothing it returns will be MISREAD, which is narrower than "the reader will accept it". Also: scrollbar and fading edge restored to the popup, since the layout's own comment names invisibility as half the defect; and two non-ASCII section signs replaced per CLAUDE.md. The review refuted one of its own candidates, worth recording: an embedded ".." in projectRelativePathOrNull is not reachable, because Kotlin's toRelativeStringOrNull normalizes both component lists first. Still deferred: the shared "close options" tooltip tag. A per-action tag needs matching docdb content or it shows an empty tooltip. Not verified on device: the phone locked itself with a secure keyguard before this round. The plugin-tab crash fix in particular wants a real tap on a docked plugin tab. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- ARCHITECTURE.md | 4 +- .../actions/file/CreateLinkAction.kt | 141 +++++++++++------- .../editor/EditorHandlerActivity.kt | 4 +- .../androidide/models/DeepLinkRequest.kt | 90 +++++++---- .../androidide/utils/ActionMenuUtils.kt | 2 +- .../androidide/utils/ProjectValidations.kt | 6 +- .../res/layout/file_action_popup_window.xml | 7 +- .../actions/file/CreateLinkActionTest.kt | 5 +- .../androidide/models/DeepLinkBuildUrlTest.kt | 27 +++- .../utils/ProjectValidationsTest.kt | 22 ++- .../itsaky/androidide/utils/PathTraversal.kt | 6 +- 11 files changed, 205 insertions(+), 109 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1e420b5dfa..a36d3d9d26 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -59,9 +59,9 @@ Feature code layers as **UI → ViewModel → Repository → data source**, with The optional file path is attacker-controllable (a URL segment), so it's resolved through `PathTraversal.resolveWithinDirectory`'s traversal/symlink guard rather than a bare `File` join, both when opening a file in the already-open project and when matching the requested project name to a directory under `Environment.PROJECTS_DIR` (`findValidProjectByName`). -**The same scheme is also written, and the writer verifies itself against the reader.** `DeepLinkRequest.buildUrl` (same file as `parse`, deliberately) is the inverse: `CreateLinkAction` — the "Create link" item on the editor's file-tab drop-down — turns the current project, file and cursor into a URL on the clipboard. Two invariants are load-bearing. **Line and column are one-based in the URL and zero-based in the editor**; `buildUrl` takes the one-based form and `EditorHandlerActivity.zeroBasedOrInvalid` converts back, so a caller holding a cursor passes `cursor.leftLine + 1`. And **`buildUrl` parses its own output back and compares it against the arguments that built it**, returning `null` on any mismatch — because `parse` peels `line`/`column` *positionally*, so a file path whose own trailing segments look like that metadata (`.../file/src/line/5` with no line of its own) would otherwise read back as a different file. That check makes the guarantee total rather than a list of enumerated cases; it is why the action always writes both coordinates, and why a path it cannot express unambiguously yields no link instead of a wrong one. +**The same scheme is also written, and the writer verifies itself against the reader.** `DeepLinkRequest.buildUrl` (same file as `parse`, deliberately) is the inverse: `CreateLinkAction` — the "Create link" item on the editor's file-tab drop-down — turns the current project, file and cursor into a URL on the clipboard. Two invariants are load-bearing. **Line and column are one-based in the URL and zero-based in the editor**; `buildUrl` takes the one-based form and `EditorHandlerActivity.zeroBasedOrInvalid` converts back, so a caller holding a cursor passes `cursor.leftLine + 1`. And **`buildUrl` parses its own output back and compares it against the arguments that built it**, returning `null` on any mismatch — because `parse` peels `line`/`column` *positionally*, so a file path whose own trailing segments look like that metadata (`.../file/src/line/5` with no line of its own) would otherwise read back as a different file. That check is what makes “will not be misread” a guarantee rather than a list of enumerated cases — distinct from, and narrower than, “the reader will accept it”, which no writer-side check can promise (see `buildUrl`'s own KDoc on symlinked file paths, unicode normalization and `isValidProjectDirectory`); it is why the action always writes both coordinates, and why a path it cannot express unambiguously yields no link instead of a wrong one. -Whether the open project *can* be named at all is `isDeepLinkTargetOfOpenProject` (`ProjectValidations.kt`): a link can only ever resolve to `/`, but a project can be opened from anywhere. Its `deepLinkTargetOfOpenProjectWithoutIo` half exists so a caller on the main thread can settle the common case — equal path strings name the same directory — without the `canonicalPath` calls the full rule needs; the residue goes to `Dispatchers.IO`, never to the StrictMode whitelist (ADR 0007). +Whether the open project *can* be named at all is `isDeepLinkTargetOfOpenProject` (`ProjectValidations.kt`): a link can only ever resolve to `/`, but a project can be opened from anywhere. Its `deepLinkTargetOfOpenProjectWithoutIo` half exists so a caller on the main thread can settle the common case — equal path strings name the same directory — without the `canonicalPath` calls the full rule needs. `CreateLinkAction` sends its residue to `Dispatchers.IO` rather than to the StrictMode whitelist (ADR 0007); the two older callers (`EditorHandlerActivity.onNewIntent`, `BaseEditorActivity.onCreate`) still fall through to `canonicalPath` on the main thread and have not been migrated, so the shortcut narrows that exposure rather than removing it. ## Module Structure diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index b8a8504786..1ac49fa7f1 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -37,6 +37,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import org.slf4j.LoggerFactory import java.io.File +import java.util.concurrent.ConcurrentHashMap /** * Copies a deep link to the current file -- and to the cursor's line and column within it -- to the @@ -58,68 +59,68 @@ class CreateLinkAction( private val log = LoggerFactory.getLogger(CreateLinkAction::class.java) /** - * Memoised answer to "is the open project one a link can name", keyed by its path. Holds no - * Context, so it is safe in a companion; the answer only changes when a project is opened. + * Verdicts for "can a link name this project", keyed by project path. Keyed rather than a + * single slot so a slow answer for a project the user has left cannot be mistaken for the + * current one -- there is nothing to invalidate and no stale-write window to guard. Holds only + * strings and booleans, so it is safe in a companion. */ - @Volatile - private var linkableProject: Pair? = null + private val linkableProjects = ConcurrentHashMap() - /** Path whose canonicalisation is already running, so N menu opens launch one job, not N. */ - @Volatile - private var linkabilityInFlight: String? = null + /** Paths whose canonicalisation is already running, so N menu opens launch one job, not N. */ + private val canonicalisationsInFlight: MutableSet = ConcurrentHashMap.newKeySet() /** * Whether a link can name the open project, or `null` while that is still being decided off * this thread. * * `prepare()` has to answer synchronously, and the full rule canonicalises both sides -- - * filesystem work that REVIEW.md §3 and ADR 0007 require be moved rather than suppressed, - * since the StrictMode whitelist is for vendored code we cannot change and never for our own. - * So [deepLinkTargetOfOpenProjectWithoutIo] settles the common case with no disk call, and - * only the residue -- paths that differ as text, where a symlink might still make them one - * directory -- goes to [Dispatchers.IO]. + * filesystem work that REVIEW.md section 3 and ADR 0007 require be moved rather than + * suppressed, since the StrictMode whitelist is for vendored code we cannot change and never + * for our own. So [deepLinkTargetOfOpenProjectWithoutIo] settles the common case with no disk + * call, and only the residue -- paths differing as text, where a symlink might still make them + * one directory -- goes to [Dispatchers.IO]. * - * Known limitation while that rare case is pending: the item is absent from the menu until the - * answer lands, and reappears on the next open. Showing it optimistically instead would mean - * a tap that fails, which reads worse; the population this can affect is a project opened from - * the file picker or a clone destination, which is usually not linkable anyway. + * While that rare case is pending the item is absent and reappears on the next menu open. + * Showing it optimistically would mean a tap that fails, which reads worse. */ private fun linkableProject( activity: EditorHandlerActivity, projectPath: String, ): Boolean? { - linkableProject?.takeIf { it.first == projectPath }?.let { return it.second } + linkableProjects[projectPath]?.let { return it } val root = runCatching { projectsRoot() }.getOrNull() ?: return null val projectName = File(projectPath).name deepLinkTargetOfOpenProjectWithoutIo(projectPath, projectName, root)?.let { - linkableProject = projectPath to it + linkableProjects[projectPath] = it return it } - // One job per path, not one per menu open: ActionMenuUtils calls prepare() every time the - // popup is built, and canonicalising the same path N times in parallel is pure waste. - if (linkabilityInFlight == projectPath) return null - linkabilityInFlight = projectPath - - activity.lifecycleScope.launch(Dispatchers.IO) { - // Handled here rather than left to the crash wrapper (REVIEW.md §1): a failure to - // canonicalise says nothing about whether the project is linkable, so it is logged and - // left uncached, and the next menu open retries. - runCatching { isDeepLinkTargetOfOpenProject(projectPath, projectName, root) } - .onSuccess { linkable -> - // Published only if this is still the project in question. A slow answer for a - // project the user has since left must not overwrite the newer one's verdict. - if (IProjectManager.getInstance().projectDirPath == projectPath) { - linkableProject = projectPath to linkable - } - }.onFailure { log.warn("Could not determine whether {} can be linked", projectPath, it) } - - if (linkabilityInFlight == projectPath) { - linkabilityInFlight = null + // add() is the atomic claim: @Volatile would give visibility without atomicity, so a + // check-then-set here could still let two menu opens launch the same canonicalisation. + if (!canonicalisationsInFlight.add(projectPath)) return null + + val job = + activity.lifecycleScope.launch(Dispatchers.IO) { + try { + // Handled here rather than left to the crash wrapper (REVIEW.md section 1): a + // failure to canonicalise says nothing about whether the project is linkable, + // so it is logged and left uncached, and the next menu open retries. + runCatching { isDeepLinkTargetOfOpenProject(projectPath, projectName, root) } + .onSuccess { linkableProjects[projectPath] = it } + .onFailure { log.warn("Could not determine whether {} can be linked", projectPath, it) } + } finally { + canonicalisationsInFlight.remove(projectPath) + } } - } + + // Released on completion, not only in the body's finally: lifecycleScope is cancelled at + // ON_DESTROY, so a rotation before the IO dispatcher picks the block up means the body -- + // and its finally -- never runs at all. A claim released only in there would latch for the + // rest of the process and hide the item permanently, the opposite of "reappears on the + // next menu open". + job.invokeOnCompletion { canonicalisationsInFlight.remove(projectPath) } return null } } @@ -148,7 +149,12 @@ class CreateLinkAction( // Hidden rather than shown-and-failing: the states that produce no link are properties of how // the project was opened, not transient ones the user could correct by tapping again. - if (linkForCurrentFile(activity) == null) { + // + // Only the cheap predicates run here. prepare() is called synchronously, from the touch + // handler, for every action in this menu on every open, and building the URL just to compare + // it against null cost ~17 percent-encoding passes plus a full parse of the result -- all + // discarded, then paid again on the tap. + if (linkTarget(activity) == null) { markInvisible() } } @@ -160,7 +166,7 @@ class CreateLinkAction( // nothing to copy means something moved underneath the open menu -- rare, but a tap that does // nothing at all reads as a broken button, so say so. val url = - linkForCurrentFile(this) ?: run { + linkTarget(this)?.toUrl() ?: run { flashError(R.string.msg_deeplink_cannot_create) return false } @@ -171,12 +177,37 @@ class CreateLinkAction( } /** - * The deep link for the file in [activity]'s currently selected tab, or `null` if that file has no - * link that would resolve anywhere. + * Everything a link needs, gathered without building one. Splitting the gathering from the + * formatting is what lets `prepare()` answer "would there be a link?" cheaply while `doAction()` + * pays for the URL exactly once. + */ + private class LinkTarget( + val projectName: String, + val relativePath: String, + val line: Int, + val column: Int, + ) { + fun toUrl(): String? = + DeepLinkRequest.buildUrl( + projectName = projectName, + filePath = relativePath, + line = line, + column = column, + ) + } + + /** + * The link target for the file in [activity]'s currently selected tab, or `null` if that file has + * no link that would resolve anywhere. */ - private fun linkForCurrentFile(activity: EditorHandlerActivity): String? { + private fun linkTarget(activity: EditorHandlerActivity): LinkTarget? { val editorView = activity.getCurrentEditor() ?: return null - val file = editorView.file ?: return null + + // The editor, then the file from it. CodeEditorView.file is itself `editor?.file`, so asking + // for the file first and then null-checking the editor separately would be asking the same + // question twice and dressing the second as a safeguard. + val editor = editorView.editor ?: return null + val file = editor.file ?: return null val projectPath = IProjectManager.getInstance().projectDirPath if (projectPath.isBlank()) { @@ -187,29 +218,25 @@ class CreateLinkAction( // A deep link can only ever name /, but a project can be opened from // anywhere -- the file picker, Recents, a clone destination. For one of those there is no URL // that resolves on this device, let alone on the recipient's, so there is nothing honest to - // put on the clipboard. Reusing the reader's own containment rule keeps the two from drifting. + // put on the clipboard. if (linkableProject(activity, projectPath) != true) { return null } val relativePath = projectRelativePathOrNull(projectDir, file) ?: return null - // A cursor is required, not optional. Without one the link would carry no line or column, and - // a coordinate-free link is the one shape parse() can misread: a file path whose own trailing - // segments look like "line"/"column" gets peeled apart as metadata. buildUrl now rejects that - // case outright, so treating a missing cursor as "no link" is what keeps this action from - // silently producing nothing at the moment of the tap. The editor is null only before its view - // is inflated, and prepare() re-runs on every menu open. - val cursor = editorView.editor?.cursor ?: return null - - // The editor is zero-based at both ends; the URL scheme is one-based. See buildUrl's docs. - return DeepLinkRequest.buildUrl( + // The editor is zero-based at both ends; the URL scheme is one-based. Both coordinates are + // always written, never omitted at 1:1 -- see buildUrl, which refuses the coordinate-free and + // column-only shapes precisely because the reader mis-handles them. + val cursor = editor.cursor + return LinkTarget( projectName = projectDir.name, - filePath = relativePath, + relativePath = relativePath, line = cursor.leftLine + 1, column = cursor.leftColumn + 1, ) } + } /** diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt index c8aa83ab93..e721f608c7 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt @@ -2209,7 +2209,7 @@ open class EditorHandlerActivity : } } - binding.root.addView(closeItem) + binding.actionItems.addView(closeItem) val undockItem = FileActionPopupWindowItemBinding @@ -2238,7 +2238,7 @@ open class EditorHandlerActivity : popupWindow.dismiss() } } - binding.root.addView(undockItem) + binding.actionItems.addView(undockItem) popupWindow.showAsDropDown(anchorView, 0, 0) } diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index eb8af71c57..4e37ff1cb5 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -18,8 +18,10 @@ package com.itsaky.androidide.models import android.net.Uri +import com.itsaky.androidide.utils.ContainedPathResolver import android.os.Parcelable import kotlinx.parcelize.Parcelize +import java.nio.file.Paths /** * A request to open a file at an optional line/column, carried as part of a [DeepLinkRequest] or a @@ -256,14 +258,21 @@ data class DeepLinkRequest( * or in [lookupValidProjectByName][com.itsaky.androidide.utils.lookupValidProjectByName]: * there is nothing to be gained by handing someone a link this same app refuses to open. * - * Deliberately NOT mirrored is that reader's - * [isValidProjectDirectory][com.itsaky.androidide.utils.isValidProjectDirectory] requirement. - * Checking it would mean disk I/O on whatever thread builds a link, and it is a fact about the - * project at *open* time rather than at link time -- a project that stops looking like an - * Android project after the link is made (its `app/build.gradle` renamed, say) would invalidate - * an already-sent link no matter what was verified here. So a link can still be emitted that - * this app later declines with "no project named X"; that is a property of the scheme, not - * something this function can close. + * Three of the reader's checks are deliberately NOT mirrored, because none of them can be + * answered without a filesystem call or a fact that outlives the link: + * [ContainedPathResolver][com.itsaky.androidide.utils.ContainedPathResolver]'s real-path + * containment (a file reached through a symlink inside the project relativises to a clean path + * here and resolves outside the project there), the reader's NFC/NFD candidate matching -- + * which it applies to the project NAME only, so an intermediary that normalizes the URL breaks + * the FILE half of a link even though the project half survives -- and that reader's + * [isValidProjectDirectory][com.itsaky.androidide.utils.isValidProjectDirectory] requirement, + * which is a fact about the project at *open* time -- one that stops looking like an Android + * project after the link is sent (its `app/build.gradle` renamed, say) invalidates an + * already-sent link no matter what was verified here. + * + * So a link can still be emitted that this app later declines. The guarantee this function + * does make is narrower and worth stating exactly: nothing it returns will be *misread* -- + * read back as naming a different project, file, line or column than the caller asked for. */ fun buildUrl( projectName: String, @@ -293,6 +302,14 @@ data class DeepLinkRequest( return null } + // And a column with no line is refused for exactly the same reason: zeroBasedOrInvalid(null) + // yields 0, so the reader would silently apply the column to line 1 -- a position the link + // never named, with no invalid-value message. parse() can still READ that shape (a + // hand-authored link), it is just not one worth writing. + if (column != null && line == null) { + return null + } + // Zero and negative are exactly what zeroBasedOrInvalid() reports back to the user as an // invalid line/column, so they must not be written down in the first place. if ((line != null && line <= 0) || (column != null && column <= 0)) { @@ -306,13 +323,29 @@ data class DeepLinkRequest( builder.appendPath(projectName) if (filePath != null) { + // The reader's own lexical rule, called rather than re-spelled: it splits on '\\' as + // well as '/' and refuses a leading one, which a guard looking only at '/' components + // misses -- a file legitimately named "a\\..\\b.kt" is one harmless-looking component + // here and a traversal there, so the link would copy with a success message and then be + // refused on open. + if (ContainedPathResolver.isLexicallyRejected(filePath)) { + return null + } + val segments = filePath.split('/') - // An empty component would put "//" in the path, which parse() rejects outright. A "." - // or ".." component is refused for the reason given above: resolveWithinDirectory treats - // it as traversal and rejects the link, and any URL-normalizing intermediary silently - // rewrites it into a different path on the way. Other dot-prefixed names are fine -- - // unlike a project directory, a hidden FILE (.gitignore) is perfectly linkable. - if (segments.any { it.isEmpty() || it == "." || it == ".." }) { + // Refused locally on top of that rule: an empty component would put "//" in the path, + // which parse() rejects outright, and a "." component normalizes away to the base + // directory, which resolveWithinDirectory refuses as "not a path inside". Other + // dot-prefixed names are fine -- unlike a project directory, a hidden FILE + // (.gitignore) is perfectly linkable. + if (segments.any { it.isEmpty() || it == "." }) { + return null + } + + // A character the reader's base.resolve() cannot accept -- a NUL, say -- percent-encodes + // and round-trips through parse() cleanly, then dies there with InvalidPathException. + // Refuse it on the same terms, with no filesystem call. + if (runCatching { Paths.get(filePath) }.isFailure) { return null } @@ -337,25 +370,16 @@ data class DeepLinkRequest( column?.let { builder.appendPath(SEGMENT_COLUMN).appendPath(it.toString()) } } - val uri = builder.build() - - // parse() measures the DECODED path against this ceiling and Uri.getPath() is decoded, so - // this is the same number it will see -- percent-expansion in the emitted string does not - // count against it. - if ((uri.path?.length ?: 0) > MAX_LINK_PATH_LENGTH) { - return null - } + val url = builder.build().toString() - // The contract, enforced rather than reasoned about: never hand back a URL that this same - // app reads as something other than what was asked for. The guards above each mirror one - // known rejection in parse(), but parse() also peels line/column POSITIONALLY, and no - // enumeration of guards catches every path whose own trailing segments happen to look like - // that metadata (".../file/src/line/5" with no line of its own reads back as file "src" at - // line 5). Comparing the parse of what was just built against the arguments that built it - // turns that whole class into a null return, and costs one parse of a string already in - // hand. Compared against the ARGUMENTS, not against a re-derivation, so this cannot pass - // by agreeing with itself. - val parsed = parse(uri) ?: return null + // Re-parsed from the STRING rather than checked against the builder's own Uri: that string + // is what goes on the clipboard and comes back through Uri.parse in DeepLinkActivity, and + // a builder-built Uri is a different implementation of the same interface. Verifying the + // object the reader never sees would leave the difference untested. + // + // There is deliberately no separate length check here: parse() applies + // MAX_LINK_PATH_LENGTH to this very path, so the round-trip below already enforces it. + val parsed = parse(Uri.parse(url)) ?: return null if (parsed.projectName != projectName || parsed.fileRequest?.filePath != filePath || parsed.fileRequest?.lineRaw != line?.toString() || @@ -364,7 +388,7 @@ data class DeepLinkRequest( return null } - return uri.toString() + return url } } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt index 81f8c94e90..d95c55b263 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt @@ -67,7 +67,7 @@ object ActionMenuUtils { true } - binding.root.setOnLongClickListener(tooltipListener) + binding.actionItems.setOnLongClickListener(tooltipListener) val actions = registry.getActions(ActionItem.Location.EDITOR_FILE_TABS) actions.forEach { action -> diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt index 4db71cb3f1..77008e2ce0 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt @@ -150,8 +150,10 @@ internal fun isDeepLinkTargetOfOpenProject( // Whatever can be settled without touching the disk, settle here. deepLinkTargetOfOpenProjectWithoutIo(openProjectPath, projectName, projectsRoot)?.let { return it } - val open = File(openProjectPath) - return canonicalOrAbsolute(open.parentFile ?: return false) == canonicalOrAbsolute(projectsRoot) + // Reached only when the shortcut deferred, which it does only after establishing a non-null + // parent and a matching name -- so this re-derives the parent rather than re-checking the rest. + val parent = File(openProjectPath).parentFile ?: return false + return canonicalOrAbsolute(parent) == canonicalOrAbsolute(projectsRoot) } /** diff --git a/app/src/main/res/layout/file_action_popup_window.xml b/app/src/main/res/layout/file_action_popup_window.xml index e0171d6dc1..c4ffc3e3c4 100644 --- a/app/src/main/res/layout/file_action_popup_window.xml +++ b/app/src/main/res/layout/file_action_popup_window.xml @@ -1,13 +1,16 @@ + (CLAUDE.md: every screen must survive 2x font scale). The scrollbar and fading edge are the other + half of that: a clipped list with neither looks pixel-identical to a complete one. --> + android:fadeScrollbars="false" + android:requiresFadingEdge="vertical" + android:scrollbars="vertical"> Date: Fri, 4 Sep 2026 03:00:52 -0700 Subject: [PATCH 09/16] ADFA-5472: Spotless Formatting only: an import reordered, a stray blank line, a comment rewrapped. Spotless output, nothing hand-written. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../com/itsaky/androidide/actions/file/CreateLinkAction.kt | 1 - .../main/java/com/itsaky/androidide/models/DeepLinkRequest.kt | 2 +- app/src/main/res/layout/file_action_popup_window.xml | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index 1ac49fa7f1..ee1d19d932 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -236,7 +236,6 @@ class CreateLinkAction( column = cursor.leftColumn + 1, ) } - } /** diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index 4e37ff1cb5..a5b0dfb97b 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -18,8 +18,8 @@ package com.itsaky.androidide.models import android.net.Uri -import com.itsaky.androidide.utils.ContainedPathResolver import android.os.Parcelable +import com.itsaky.androidide.utils.ContainedPathResolver import kotlinx.parcelize.Parcelize import java.nio.file.Paths diff --git a/app/src/main/res/layout/file_action_popup_window.xml b/app/src/main/res/layout/file_action_popup_window.xml index c4ffc3e3c4..64df3d1457 100644 --- a/app/src/main/res/layout/file_action_popup_window.xml +++ b/app/src/main/res/layout/file_action_popup_window.xml @@ -1,8 +1,8 @@ + (CLAUDE.md: every screen must survive 2x font scale). The scrollbar and fading edge are the other half of that: a clipped + list with neither looks pixel-identical to a complete one. --> Date: Fri, 4 Sep 2026 03:36:03 -0700 Subject: [PATCH 10/16] ADFA-5472: Keep a project path out of the log, and share the name rule Two findings from the review of the last push. The warning logged the open project's path. That path is user-controlled and names private directories, and the repo's logging rule is "no secrets/PII". The message says what failed and keeps the throwable; nothing is lost, because there is only ever one open project to be talking about. The cheap eligibility path had its own idea of a valid project name -- which is to say, none. A direct child named ".foo" satisfies the containment check (that one compares parents, not names) but buildUrl refuses it, so linkTarget() returned a target, prepare() offered the item and the tap then failed. That is the third instance of the same shape in this branch: a caller re-deciding, or failing to decide, something the authority already decides. So the rule moved rather than being copied -- DeepLinkRequest.isLinkableProjectName is now the single definition, and buildUrl calls it too. A test pins the two together over both accepting and rejecting names, so a future divergence fails rather than reaching a user as an offer that cannot be honoured. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../actions/file/CreateLinkAction.kt | 9 ++++- .../androidide/models/DeepLinkRequest.kt | 36 ++++++++++++------- .../androidide/models/DeepLinkBuildUrlTest.kt | 15 ++++++++ 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index ee1d19d932..0d03208963 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -109,7 +109,7 @@ class CreateLinkAction( // so it is logged and left uncached, and the next menu open retries. runCatching { isDeepLinkTargetOfOpenProject(projectPath, projectName, root) } .onSuccess { linkableProjects[projectPath] = it } - .onFailure { log.warn("Could not determine whether {} can be linked", projectPath, it) } + .onFailure { log.warn("Could not determine whether the open project can be linked", it) } } finally { canonicalisationsInFlight.remove(projectPath) } @@ -223,6 +223,13 @@ class CreateLinkAction( return null } + // buildUrl's own rule, called rather than restated. A direct child named ".foo" satisfies the + // containment check above -- that one compares parents, not names -- but is not a project a + // link can name, so without this the item was offered and the tap then failed. + if (!DeepLinkRequest.isLinkableProjectName(projectDir.name)) { + return null + } + val relativePath = projectRelativePathOrNull(projectDir, file) ?: return null // The editor is zero-based at both ends; the URL scheme is one-based. Both coordinates are diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index a5b0dfb97b..a5532ddb5a 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -243,6 +243,28 @@ data class DeepLinkRequest( return DeepLinkRequest(projectName = projectName, fileRequest = fileRequest) } + /** + * Whether a link can name [projectName] at all. + * + * A project name is a single path segment naming a direct child of the projects root, so a + * name carrying a separator can never resolve -- lookupValidProjectByName rejects it before it + * ever touches the disk. A leading dot is rejected for the same reason one level down: + * isProjectCandidateDir() refuses any name starting with '.', so a hidden directory is never a + * project. That one check also covers "." and "..", which matter more than merely being + * unopenable -- a browser or messenger that normalizes dot segments rewrites + * "/device/open/project/../file/x" into an entirely different path before the app ever sees it. + * + * Public because [buildUrl] is not the only caller that needs it: a UI deciding whether to + * *offer* to make a link has to reach the same verdict, and cheaply. Its own copy of these + * four conditions is exactly the drift that put an offer in front of the user for a project + * this function refuses. + */ + fun isLinkableProjectName(projectName: String): Boolean = + projectName.isNotEmpty() && + !projectName.startsWith('.') && + !projectName.contains('/') && + !projectName.contains('\\') + /** * The inverse of [parse]: the canonical URL naming [projectName], optionally the * project-relative [filePath] inside it, and optionally a [line] and [column] inside that file. @@ -280,19 +302,7 @@ data class DeepLinkRequest( line: Int? = null, column: Int? = null, ): String? { - // A project name is a single path segment naming a direct child of the projects root, so a - // name carrying a separator can never resolve -- lookupValidProjectByName rejects it before - // it ever touches the disk. A leading dot is rejected for the same reason one level down: - // isProjectCandidateDir() refuses any name starting with '.', so a hidden directory is - // never a project. That one check also covers "." and "..", which matter more than merely - // being unopenable -- a browser or messenger that normalizes dot segments rewrites - // "/device/open/project/../file/x" into an entirely different path before the app ever - // sees it. - if (projectName.isEmpty() || - projectName.startsWith('.') || - projectName.contains('/') || - projectName.contains('\\') - ) { + if (!isLinkableProjectName(projectName)) { return null } diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt index 94e6c90359..db3826e808 100644 --- a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt @@ -119,6 +119,21 @@ class DeepLinkBuildUrlTest { assertThat(DeepLinkRequest.buildUrl("MyApp", "src/line/5", line = 7, column = 3)).isNotNull() } + @Test + fun `the shared project-name rule is exactly what buildUrl enforces`() { + // One rule, two callers: buildUrl and the UI deciding whether to offer a link. Pinned together + // because a private copy in the UI is what previously offered a link for a ".foo" project and + // then failed on the tap. + for (name in listOf("MyApp", "My App", "Cafe\u0301", "we ird-name_1")) { + assertThat(DeepLinkRequest.isLinkableProjectName(name)).isTrue() + assertThat(DeepLinkRequest.buildUrl(name)).isNotNull() + } + for (name in listOf("", ".", "..", ".hidden", "nested/MyApp", "nested\\MyApp")) { + assertThat(DeepLinkRequest.isLinkableProjectName(name)).isFalse() + assertThat(DeepLinkRequest.buildUrl(name)).isNull() + } + } + @Test fun `rejects dot segments the reader would refuse or an intermediary would rewrite`() { // isProjectCandidateDir() refuses any name starting with '.', so none of these can name a From 52edb9371709608deded4a308a55babb72a19fb5 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 04:03:14 -0700 Subject: [PATCH 11/16] ADFA-5472: Address the second xhigh review Nine findings. The first is a regression I introduced fixing the last review, which is worth saying plainly. prepare() offered links it could not build. Last round's perf finding was that prepare() built a whole URL only to null-check it, then doAction built it again -- so I took buildUrl out of prepare(). That reintroduced shown-and-failing for every state buildUrl alone rejects: a file named "a\..\b.kt", a path whose decoded URL exceeds 512. Both are permanent properties of the file, so the tap failed every time, and my own comment still claimed prepare() hides "every state that is stably unlinkable". The two reviews were pulling in opposite directions; memoising satisfies both. prepare() builds and keeps the result, the tap reuses it -- the cursor cannot have moved in between -- so the URL is built once per menu open rather than twice or never. The in-flight claim was released twice. The body's finally and invokeOnCompletion both cleared it, so job A completing could release a claim job B had already re-taken, launching duplicate canonicalisation -- the exact waste the claim exists to prevent. invokeOnCompletion alone covers both cases that matter, including the cancelled-before-dispatch one it was added for. A transient IO failure latched "not linkable" for the process. canonicalOrAbsolute converts a failed canonicalPath into absolutePath, so isDeepLinkTargetOfOpenProject cannot report "could not tell" -- it returns false, indistinguishable from a real mismatch, and the action cached it. An EACCES right after a permission change, or an EIO on a flaky volume, therefore hid the item permanently. deepLinkTargetOfOpenProjectOrNull keeps the third state, mirroring ProjectNameLookup.Unverifiable, and only a real answer is cached. That also makes the log line honest: it is a backstop for an unforeseen throw, not the retry path it claimed to be. The side effect ran before the free checks. linkTarget() dispatched the canonicalisation job before the two pure rejections -- isLinkableProjectName and projectRelativePathOrNull -- that would have made its result moot. Reordered, so a project opened from Download or a tab outside the project launches no filesystem work at all. Two comments asserted things that are not true. The reader does not "die with InvalidPathException" on a NUL in a path -- lexicalResolve catches it and returns null, so the link is rejected cleanly; the guard is still right, its stated reason was not. And the PATH_SEGMENTS refactor claimed "project" is now spelled once, while AndroidManifest.xml spells the whole prefix twice -- and the manifest, not the Kotlin, decides whether a link reaches the app at all. DeepLinkManifestPrefixTest now fails on that drift; verified by adding a segment to PATH_SEGMENTS, which turns every generated link into a browser open with the rest of the suite still green. The "independent" comparison test was not independent enough. It used == where the code uses projectNamesMatch, and no case differed in normal form, so dropping NFD tolerance passed. Verified by making exactly that change, which now fails. Also: no KDoc link from common up into app (the module dependency only goes the other way, so it could never resolve), and clip_label_deeplink is translatable="false" like its sibling product-name strings. Not fixed, with a concrete reason this time: the shared "close options" tooltip. documentation.db is read-only and prebuilt, its schema owned by the separate OfflineDocumentationTools project, and docs/documentation-database.md warns that authoring against a stale local copy has already nearly overwritten curated content once. A new tag without content shows an empty tooltip, so this needs that pipeline, not this PR. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../actions/file/CreateLinkAction.kt | 90 +++++++++++++------ .../androidide/models/DeepLinkRequest.kt | 16 +++- .../androidide/utils/ProjectValidations.kt | 24 +++++ .../androidide/models/DeepLinkBuildUrlTest.kt | 5 +- .../models/DeepLinkManifestPrefixTest.kt | 62 +++++++++++++ .../utils/ProjectValidationsTest.kt | 20 ++++- .../itsaky/androidide/utils/PathTraversal.kt | 7 +- resources/src/main/res/values/strings.xml | 2 +- 8 files changed, 185 insertions(+), 41 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index 0d03208963..849d4ff853 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -32,7 +32,7 @@ import com.itsaky.androidide.utils.copyToClipboard import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectWithoutIo import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess -import com.itsaky.androidide.utils.isDeepLinkTargetOfOpenProject +import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectOrNull import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import org.slf4j.LoggerFactory @@ -103,23 +103,28 @@ class CreateLinkAction( val job = activity.lifecycleScope.launch(Dispatchers.IO) { - try { - // Handled here rather than left to the crash wrapper (REVIEW.md section 1): a - // failure to canonicalise says nothing about whether the project is linkable, - // so it is logged and left uncached, and the next menu open retries. - runCatching { isDeepLinkTargetOfOpenProject(projectPath, projectName, root) } - .onSuccess { linkableProjects[projectPath] = it } + // Cached only when the filesystem actually answered. deepLinkTargetOfOpenProjectOrNull + // returns null for "could not tell", which a momentary EACCES or EIO produces -- and + // caching that as "not linkable" would hide the item for the rest of the process over + // a condition that has since cleared. + val verdict = + runCatching { deepLinkTargetOfOpenProjectOrNull(projectPath, projectName, root) } + // A backstop, not the retry path: that function reports failure by returning + // null rather than throwing. Present so an unforeseen throw cannot reach the + // crash wrapper from a launch with no handler (REVIEW.md section 1). .onFailure { log.warn("Could not determine whether the open project can be linked", it) } - } finally { - canonicalisationsInFlight.remove(projectPath) + .getOrNull() + + if (verdict != null) { + linkableProjects[projectPath] = verdict } } - // Released on completion, not only in the body's finally: lifecycleScope is cancelled at - // ON_DESTROY, so a rotation before the IO dispatcher picks the block up means the body -- - // and its finally -- never runs at all. A claim released only in there would latch for the - // rest of the process and hide the item permanently, the opposite of "reappears on the - // next menu open". + // Released here and nowhere else. invokeOnCompletion fires for both outcomes that matter -- + // the block ran, and the block never ran because lifecycleScope was cancelled at ON_DESTROY + // before the dispatcher picked it up. A release inside the body as well would let job A's + // completion clear a claim job B had already re-taken, which is the duplicate work the + // claim exists to prevent. job.invokeOnCompletion { canonicalisationsInFlight.remove(projectPath) } return null } @@ -154,7 +159,7 @@ class CreateLinkAction( // handler, for every action in this menu on every open, and building the URL just to compare // it against null cost ~17 percent-encoding passes plus a full parse of the result -- all // discarded, then paid again on the tap. - if (linkTarget(activity) == null) { + if (urlFor(activity) == null) { markInvisible() } } @@ -166,7 +171,7 @@ class CreateLinkAction( // nothing to copy means something moved underneath the open menu -- rare, but a tap that does // nothing at all reads as a broken button, so say so. val url = - linkTarget(this)?.toUrl() ?: run { + urlFor(this) ?: run { flashError(R.string.msg_deeplink_cannot_create) return false } @@ -177,11 +182,34 @@ class CreateLinkAction( } /** - * Everything a link needs, gathered without building one. Splitting the gathering from the - * formatting is what lets `prepare()` answer "would there be a link?" cheaply while `doAction()` - * pays for the URL exactly once. + * The most recently built URL, with the target it was built from. + * + * `prepare()` has to know whether a URL can be built at all, not merely whether the pieces exist: + * [DeepLinkRequest.buildUrl] refuses paths the reader would reject, and those are permanent + * properties of the file, so an item shown without asking it fails on every tap forever. But + * building the URL and discarding it made `prepare()` -- which runs for every action on every + * menu open, in the touch handler -- pay for a full encode-and-reparse it threw away, then pay + * again on the tap. + * + * Keeping the last result settles both: `prepare()` builds, and the tap that follows reuses it, + * because the cursor cannot have moved in between. Read and written only from the main thread + * (`requiresUIThread`), so it needs no synchronisation. + */ + private var lastBuilt: Pair? = null + + /** The URL for the current tab, built at most once per menu open, or `null` if there is none. */ + private fun urlFor(activity: EditorHandlerActivity): String? { + val target = linkTarget(activity) ?: return null + lastBuilt?.let { (built, url) -> if (built == target) return url } + val url = target.toUrl() ?: return null + lastBuilt = target to url + return url + } + + /** + * Everything a link needs, gathered without building one. */ - private class LinkTarget( + private data class LinkTarget( val projectName: String, val relativePath: String, val line: Int, @@ -215,6 +243,19 @@ class CreateLinkAction( } val projectDir = File(projectPath) + // The two free rejections first. Both are pure and settle the answer outright, so asking them + // before linkableProject -- which dispatches a canonicalisation as a side effect -- avoids + // launching filesystem work whose result could not change the verdict. + // + // buildUrl's own rule, called rather than restated: a direct child named ".foo" satisfies the + // containment check below (that one compares parents, not names) but is not a project a link + // can name. + if (!DeepLinkRequest.isLinkableProjectName(projectDir.name)) { + return null + } + + val relativePath = projectRelativePathOrNull(projectDir, file) ?: return null + // A deep link can only ever name /, but a project can be opened from // anywhere -- the file picker, Recents, a clone destination. For one of those there is no URL // that resolves on this device, let alone on the recipient's, so there is nothing honest to @@ -223,15 +264,6 @@ class CreateLinkAction( return null } - // buildUrl's own rule, called rather than restated. A direct child named ".foo" satisfies the - // containment check above -- that one compares parents, not names -- but is not a project a - // link can name, so without this the item was offered and the tap then failed. - if (!DeepLinkRequest.isLinkableProjectName(projectDir.name)) { - return null - } - - val relativePath = projectRelativePathOrNull(projectDir, file) ?: return null - // The editor is zero-based at both ends; the URL scheme is one-based. Both coordinates are // always written, never omitted at 1:1 -- see buildUrl, which refuses the coordinate-free and // column-only shapes precisely because the reader mis-handles them. diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index a5532ddb5a..9e2b1dbaaa 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -90,8 +90,14 @@ data class DeepLinkRequest( /** * The fixed segments every link starts with, and the prefix [parse] matches, derived from them - * so the literal "project" is spelled once. The old pairing had it in both [PATH_PREFIX] and - * [SEGMENT_PROJECT], which is the drift this is meant to prevent -- [parse] reads both. + * so the literal "project" is spelled once *here*. The old pairing had it in both [PATH_PREFIX] + * and [SEGMENT_PROJECT], which is the drift this is meant to prevent -- [parse] reads both. + * + * It is spelled once more outside Kotlin, and that copy is the one that decides whether a link + * ever reaches the app at all: `AndroidManifest.xml`'s two `android:pathPrefix` attributes on + * DeepLinkActivity's intent-filter. Changing this list without changing those leaves every + * generated link opening in a browser, with the whole unit suite still green -- so + * `DeepLinkManifestPrefixTest` fails on exactly that. */ private val PATH_SEGMENTS = listOf("device", "open", SEGMENT_PROJECT) private val PATH_PREFIX = PATH_SEGMENTS.joinToString(separator = "/", prefix = "/", postfix = "/") @@ -353,8 +359,10 @@ data class DeepLinkRequest( } // A character the reader's base.resolve() cannot accept -- a NUL, say -- percent-encodes - // and round-trips through parse() cleanly, then dies there with InvalidPathException. - // Refuse it on the same terms, with no filesystem call. + // and round-trips through parse() cleanly, and is then REJECTED there: lexicalResolve + // catches the InvalidPathException and returns null, so the link opens nothing and the + // user is told the file was not found. Refuse it here instead, with no filesystem call, + // rather than copying a link that cannot work. if (runCatching { Paths.get(filePath) }.isFailure) { return null } diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt index 77008e2ce0..979dced7f6 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt @@ -156,6 +156,30 @@ internal fun isDeepLinkTargetOfOpenProject( return canonicalOrAbsolute(parent) == canonicalOrAbsolute(projectsRoot) } +/** + * [isDeepLinkTargetOfOpenProject] with "could not tell" kept apart from "no", for a caller that + * remembers the answer. + * + * The Boolean version cannot express the difference: [canonicalOrAbsolute] turns a failed + * `canonicalPath` into the plain `absolutePath` and the comparison then yields `false`, which is + * indistinguishable from a genuine mismatch. A caller that caches that `false` latches "not + * linkable" for the rest of the process over what may have been a momentary EACCES after a + * permission change, or an EIO on a flaky external volume. Same reasoning, and same shape, as + * [ProjectNameLookup.Unverifiable]. + */ +internal fun deepLinkTargetOfOpenProjectOrNull( + openProjectPath: String, + projectName: String, + projectsRoot: File, +): Boolean? { + deepLinkTargetOfOpenProjectWithoutIo(openProjectPath, projectName, projectsRoot)?.let { return it } + + val parent = File(openProjectPath).parentFile ?: return false + val parentPath = runCatching { parent.canonicalPath }.getOrNull() ?: return null + val rootPath = runCatching { projectsRoot.canonicalPath }.getOrNull() ?: return null + return parentPath == rootPath +} + /** * The part of [isDeepLinkTargetOfOpenProject] decidable without any filesystem call, for callers * that must answer on a thread where I/O is not allowed -- `null` means "only canonicalisation can diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt index db3826e808..a08c8c197d 100644 --- a/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkBuildUrlTest.kt @@ -199,8 +199,9 @@ class DeepLinkBuildUrlTest { assertThat(DeepLinkRequest.buildUrl("MyApp", "\\Main.kt", line = 1, column = 1)).isNull() assertThat(DeepLinkRequest.buildUrl("MyApp", "/Main.kt", line = 1, column = 1)).isNull() - // A character the reader's base.resolve() cannot accept: encodes and round-trips fine, dies - // there with InvalidPathException. + // A character the reader's base.resolve() cannot accept: encodes and round-trips fine here, + // and is rejected there (lexicalResolve catches InvalidPathException), so the link opens + // nothing. assertThat(DeepLinkRequest.buildUrl("MyApp", "a\u0000b.kt", line = 1, column = 1)).isNull() // A backslash that is NOT traversal stays linkable -- the guard mirrors the reader, it does diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt new file mode 100644 index 0000000000..edbd695dbc --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt @@ -0,0 +1,62 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.models + +import android.net.Uri +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.io.File + +/** + * The deep-link path prefix is spelled in two places that cannot see each other: `DeepLinkRequest`, + * which builds and parses URLs, and `AndroidManifest.xml`, whose `android:pathPrefix` decides + * whether Android delivers the link to this app at all. + * + * Nothing else catches a disagreement. Change the Kotlin side alone and `buildUrl` and `parse` still + * agree with each other, every other test stays green, and every generated link quietly opens in a + * browser instead of the editor. + */ +@RunWith(RobolectricTestRunner::class) +class DeepLinkManifestPrefixTest { + @Test + fun `every manifest pathPrefix matches the prefix buildUrl emits`() { + val manifest = File("src/main/AndroidManifest.xml") + assertThat(manifest.exists()).isTrue() + + val declared = + Regex("""android:pathPrefix\s*=\s*"([^"]*)"""") + .findAll(manifest.readText()) + .map { it.groupValues[1] } + .toList() + + // If the intent-filter stops declaring a prefix, this test must fail rather than vacuously + // pass over an empty list -- there are two elements today, one per verified host. + assertThat(declared).isNotEmpty() + + // Taken from a URL the builder actually produces, so this asserts against the emitted shape + // rather than against a second copy of the constant. + val built = DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 1, column = 1) + val path = Uri.parse(built!!).path!! + + for (prefix in declared) { + assertThat(path).startsWith(prefix) + } + } +} diff --git a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt index 7a43f67e83..0ba593a36a 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt @@ -178,12 +178,28 @@ class ProjectValidationsTest { ): Boolean { if (path.isBlank()) return false val open = File(path) - if (open.name != name) return false + // Normalised on both sides, matching projectNamesMatch. Comparing with plain == would + // leave the rule's NFD tolerance unpinned: dropping it from the shortcut would still pass. + if (Normalizer.normalize(open.name, Normalizer.Form.NFC) != Normalizer.normalize(name, Normalizer.Form.NFC)) { + return false + } val parent = open.parentFile ?: return false return parent.canonicalPath == root.canonicalPath } - for ((path, name) in listOf(File(root, "MyApp").path to "MyApp", File(root, "MyApp").path to "Other", "" to "MyApp")) { + val nfd = "Cafe\u0301" + val nfc = Normalizer.normalize(nfd, Normalizer.Form.NFC) + val cases = + listOf( + File(root, "MyApp").path to "MyApp", + File(root, "MyApp").path to "Other", + "" to "MyApp", + // A project on disk in decomposed form, named in the link in composed form -- the + // macOS-clone case lookupValidProjectByName tries both forms for. + File(root, nfd).path to nfc, + File(root, nfc).path to nfd, + ) + for ((path, name) in cases) { val shortcut = deepLinkTargetOfOpenProjectWithoutIo(path, name, root) assertThat(shortcut).isNotNull() assertThat(shortcut).isEqualTo(independentlyLinkable(path, name)) diff --git a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt index 71a0776d0e..1c25e364b9 100644 --- a/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt +++ b/common/src/main/java/com/itsaky/androidide/utils/PathTraversal.kt @@ -251,9 +251,10 @@ class ContainedPathResolver( * symlink) can apply the same reject first: an entry that fails here is a bad archive * however the filesystem looks, never fallback material. * - * Public so a *writer* of these paths can refuse what this reader would -- see - * [DeepLinkRequest.buildUrl][com.itsaky.androidide.models.DeepLinkRequest.Companion.buildUrl], - * which would otherwise re-spell a subset of this rule and drift from it. + * Public so a *writer* of these paths can refuse what this reader would -- the deep-link URL + * builder in the app module is one, and would otherwise re-spell a subset of this rule and + * drift from it. Named in prose rather than linked: `common` does not depend on `app`, and + * must not, so a KDoc link that way would never resolve. */ fun isLexicallyRejected(relativePath: String): Boolean = // Split on both separators: '\' is not a path separator on Android, but a caller handing diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 756df477ed..7bec8cfd8e 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -148,7 +148,7 @@ Could not scan projects for this link. Link copied to the clipboard. Could not create a link for this file. - Code on the Go link + Code on the Go link A project close is already in progress. Try again in a moment. Create new project Open a saved project From 60a4ad7cff6535d23258fdf08edb095e6fdfeec4 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 04:03:44 -0700 Subject: [PATCH 12/16] ADFA-5472: Spotless Import ordering. I ran spotlessApply before making a later import change, so it did not cover it -- formatting only. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../java/com/itsaky/androidide/actions/file/CreateLinkAction.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index 849d4ff853..7c7f9e33d2 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -29,10 +29,10 @@ import com.itsaky.androidide.activities.projectsRoot import com.itsaky.androidide.models.DeepLinkRequest import com.itsaky.androidide.projects.IProjectManager import com.itsaky.androidide.utils.copyToClipboard +import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectOrNull import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectWithoutIo import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess -import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectOrNull import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import org.slf4j.LoggerFactory From a372497792c49c31015b9af0799f4cd725e8ded6 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 05:50:22 -0700 Subject: [PATCH 13/16] ADFA-5472: Address the third xhigh review Nine of fifteen findings. The six left are argued below rather than skipped quietly. Structured cancellation. The IO block used runCatching, which catches Throwable -- so a CancellationException raised while the activity is going away was logged as a failure and the job completed normally, breaking cancellation, and an OutOfMemoryError went the same way. Latent today because the body has no suspension point, and exactly the kind of latent that stops being latent the moment someone adds one. Now a narrow catch that rethrows CancellationException (REVIEW.md section 1). The action uses the scope built for it. EditorActivityAction already provides actionScope and cancels it in destroy(); the code was threading an Activity through purely to reach lifecycleScope. And the verdict caches are cleared in destroy() -- a cached answer describes the filesystem as it was, so a project directory renamed mid-process left it answering the old question. "There is nothing to invalidate" was only true if the disk never changes. Three functions became two. isDeepLinkTargetOfOpenProject now delegates to the nullable variant, so the canonicalisation step is spelled once. That is not a behaviour change: the old canonicalOrAbsolute fallback compared plain absolute paths, and equal absolute paths are already settled true by the no-IO shortcut -- so reaching the fallback meant they differed and the answer was false either way. The clipboard label was untranslatable in the wrong half. Last round I marked the whole string translatable="false" on the strength of its product-name siblings; the noun is not a product name. Now "%1$s link" with app_name substituted, so "Code on the Go" stays fixed and "link" reaches translators. Tests for the parts that had none, and two false premises removed from the ones that did. deepLinkTargetOfOpenProjectOrNull's two reasons for existing are now pinned: a symlinked parent that differs as text resolves true, and an uncanonicalisable path reports null rather than false -- verified by flipping that return to false, which fails that test and only that test. The manifest drift guard no longer regexes the whole file (an unrelated App Link anywhere in it would have failed and pointed the reader at DeepLinkRequest) and no longer assumes one working directory. Also: the dead null-activity branch in prepare() is gone (super already markInvisible()s), the lastBuilt KDoc no longer credits requiresUIThread for a main-thread guarantee that flag does not give, and the plugin divider's margins are dp instead of raw pixels. Deliberately not done: - The shared "close options" tooltip, a fourth time, now with the reason measured rather than asserted: retrieveTooltipTag defaults to a tooltipTag property the close actions do not set, so routing the shared listener through it would break THEIR tooltips, and documentation.db is read-only and externally owned so a new tag would show empty content. This needs a follow-up with the docdb pipeline. - android.util.Log.e and the bare catch in the plugin-item handler. Pre-existing, plugin-facing error semantics, unrelated to this feature. The divider margins next to them are fixed because that is a two-line mechanical change with no behavioural risk; changing how plugin failures are reported is not. - Lexical-only containment of the file inside the project. Mirroring the reader's real-path check needs canonicalisation on the path just moved off-thread, and the premise -- a project path and a file path reaching the same directory through different aliases -- is not demonstrated, since file paths are derived from the project directory. - Cursor coordinates taken from an unsaved buffer can name a line that does not exist in the file the recipient opens. Real, and a product decision: saving on the user's behalf, warning, or omitting coordinates are all defensible and none is mine to pick. Raised for the ticket. - Bounding the popup to the space below its anchor. The ScrollView only starts scrolling once content exceeds the display, not the room under the tab strip, because both call sites pass WRAP_CONTENT height. The fix is a measurement change I cannot verify -- the test device is locked -- and shipping an unverifiable layout measurement is worse than a documented partial fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../actions/file/CreateLinkAction.kt | 161 ++++++++++-------- .../androidide/utils/ActionMenuUtils.kt | 5 +- .../androidide/utils/ProjectValidations.kt | 20 +-- .../models/DeepLinkManifestPrefixTest.kt | 23 ++- .../utils/ProjectValidationsTest.kt | 31 ++++ resources/src/main/res/values/strings.xml | 2 +- 6 files changed, 155 insertions(+), 87 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index 7c7f9e33d2..f094437e7e 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -20,7 +20,6 @@ package com.itsaky.androidide.actions.file import android.content.Context import androidx.annotation.VisibleForTesting import androidx.core.content.ContextCompat -import androidx.lifecycle.lifecycleScope import com.itsaky.androidide.R import com.itsaky.androidide.actions.ActionData import com.itsaky.androidide.actions.markInvisible @@ -33,6 +32,7 @@ import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectOrNull import com.itsaky.androidide.utils.deepLinkTargetOfOpenProjectWithoutIo import com.itsaky.androidide.utils.flashError import com.itsaky.androidide.utils.flashSuccess +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import org.slf4j.LoggerFactory @@ -69,75 +69,100 @@ class CreateLinkAction( /** Paths whose canonicalisation is already running, so N menu opens launch one job, not N. */ private val canonicalisationsInFlight: MutableSet = ConcurrentHashMap.newKeySet() - /** - * Whether a link can name the open project, or `null` while that is still being decided off - * this thread. - * - * `prepare()` has to answer synchronously, and the full rule canonicalises both sides -- - * filesystem work that REVIEW.md section 3 and ADR 0007 require be moved rather than - * suppressed, since the StrictMode whitelist is for vendored code we cannot change and never - * for our own. So [deepLinkTargetOfOpenProjectWithoutIo] settles the common case with no disk - * call, and only the residue -- paths differing as text, where a symlink might still make them - * one directory -- goes to [Dispatchers.IO]. - * - * While that rare case is pending the item is absent and reappears on the next menu open. - * Showing it optimistically would mean a tap that fails, which reads worse. - */ - private fun linkableProject( - activity: EditorHandlerActivity, - projectPath: String, - ): Boolean? { - linkableProjects[projectPath]?.let { return it } - - val root = runCatching { projectsRoot() }.getOrNull() ?: return null - val projectName = File(projectPath).name - - deepLinkTargetOfOpenProjectWithoutIo(projectPath, projectName, root)?.let { - linkableProjects[projectPath] = it - return it - } + } + + /** + * Whether a link can name the open project, or `null` while that is still being decided off this + * thread. + * + * `prepare()` has to answer synchronously, and the full rule canonicalises both sides -- + * filesystem work that REVIEW.md section 3 and ADR 0007 require be moved rather than suppressed, + * since the StrictMode whitelist is for vendored code we cannot change and never for our own. So + * [deepLinkTargetOfOpenProjectWithoutIo] settles the common case with no disk call, and only the + * residue -- paths differing as text, where a symlink might still make them one directory -- goes + * off-thread. + * + * While that rare case is pending the item is absent and reappears on the next menu open. Showing + * it optimistically would mean a tap that fails, which reads worse. + */ + private fun linkableProject(projectPath: String): Boolean? { + linkableProjects[projectPath]?.let { return it } + + val root = runCatching { projectsRoot() }.getOrNull() ?: return null + val projectName = File(projectPath).name + + deepLinkTargetOfOpenProjectWithoutIo(projectPath, projectName, root)?.let { + linkableProjects[projectPath] = it + return it + } - // add() is the atomic claim: @Volatile would give visibility without atomicity, so a - // check-then-set here could still let two menu opens launch the same canonicalisation. - if (!canonicalisationsInFlight.add(projectPath)) return null - - val job = - activity.lifecycleScope.launch(Dispatchers.IO) { - // Cached only when the filesystem actually answered. deepLinkTargetOfOpenProjectOrNull - // returns null for "could not tell", which a momentary EACCES or EIO produces -- and - // caching that as "not linkable" would hide the item for the rest of the process over - // a condition that has since cleared. - val verdict = - runCatching { deepLinkTargetOfOpenProjectOrNull(projectPath, projectName, root) } - // A backstop, not the retry path: that function reports failure by returning - // null rather than throwing. Present so an unforeseen throw cannot reach the - // crash wrapper from a launch with no handler (REVIEW.md section 1). - .onFailure { log.warn("Could not determine whether the open project can be linked", it) } - .getOrNull() - - if (verdict != null) { - linkableProjects[projectPath] = verdict + // add() is the atomic claim: @Volatile would give visibility without atomicity, so a + // check-then-set here could still let two menu opens launch the same canonicalisation. + if (!canonicalisationsInFlight.add(projectPath)) return null + + // actionScope, not the activity's lifecycleScope: the base class builds this scope for exactly + // this kind of background work and cancels it in destroy(), so the action no longer has to be + // handed an Activity just to reach a scope. + val job = + actionScope.launch(Dispatchers.IO) { + // Cached only when the filesystem actually answered. deepLinkTargetOfOpenProjectOrNull + // returns null for "could not tell", which a momentary EACCES or EIO produces -- and + // caching that as "not linkable" would hide the item for the rest of the process over a + // condition that has since cleared. + val verdict = + try { + deepLinkTargetOfOpenProjectOrNull(projectPath, projectName, root) + } catch (e: CancellationException) { + // Rethrown, never logged as a failure: swallowing it would complete this job + // normally and break structured cancellation (REVIEW.md section 1). runCatching + // here would catch it, and every Error too. + throw e + } catch (e: Exception) { + // A backstop, not the retry path: that function reports failure by returning null + // rather than throwing. Present so an unforeseen throw cannot reach the crash + // wrapper from a launch with no handler. + log.warn("Could not determine whether the open project can be linked", e) + null } + + if (verdict != null) { + linkableProjects[projectPath] = verdict } + } - // Released here and nowhere else. invokeOnCompletion fires for both outcomes that matter -- - // the block ran, and the block never ran because lifecycleScope was cancelled at ON_DESTROY - // before the dispatcher picked it up. A release inside the body as well would let job A's - // completion clear a claim job B had already re-taken, which is the duplicate work the - // claim exists to prevent. - job.invokeOnCompletion { canonicalisationsInFlight.remove(projectPath) } - return null - } + // Released here and nowhere else. invokeOnCompletion fires for both outcomes that matter -- + // the block ran, and the block never ran because the scope was cancelled before the dispatcher + // picked it up. A release inside the body as well would let job A's completion clear a claim + // job B had already re-taken, which is the duplicate work the claim exists to prevent. + job.invokeOnCompletion { canonicalisationsInFlight.remove(projectPath) } + return null } - /** Shown in the clipboard preview on Android 13+, so it is user-facing and lives in resources. */ - private val clipLabel: String = context.getString(R.string.clip_label_deeplink) + /** + * Shown in the clipboard preview on Android 13+, so it is user-facing and lives in resources. + * + * Composed from the app name rather than spelled out, because only half of it is translatable: + * "Code on the Go" is a product name and must not be localised, while the noun after it should + * be. Marking the whole string untranslatable, as an earlier round did, kept the noun out of + * translation too. + */ + private val clipLabel: String = context.getString(R.string.clip_label_deeplink, context.getString(R.string.app_name)) init { label = context.getString(R.string.action_create_link) icon = ContextCompat.getDrawable(context, R.drawable.ic_copy) } + override fun destroy() { + super.destroy() + // A verdict describes the filesystem as it was. Renaming the project directory, or swapping + // for a symlink, while the process lives would leave a cached answer to the old + // question -- so the answers do not outlive the activity that asked. REVIEW.md section 2 flags + // companion-held state for exactly this. + linkableProjects.clear() + canonicalisationsInFlight.clear() + } + override fun prepare(data: ActionData) { super.prepare(data) @@ -145,12 +170,9 @@ class CreateLinkAction( return } - val activity = - data.getActivity() - ?: run { - markInvisible() - return - } + // No null branch: FileTabAction.prepare() (via super, above) already calls markInvisible() when + // there is no activity, and the !visible check just above returns in exactly that case. + val activity = data.getActivity() ?: return // Hidden rather than shown-and-failing: the states that produce no link are properties of how // the project was opened, not transient ones the user could correct by tapping again. @@ -192,8 +214,13 @@ class CreateLinkAction( * again on the tap. * * Keeping the last result settles both: `prepare()` builds, and the tap that follows reuses it, - * because the cursor cannot have moved in between. Read and written only from the main thread - * (`requiresUIThread`), so it needs no synchronisation. + * because the cursor cannot have moved in between. That pairing is the whole job -- one build per + * menu open instead of two. It deliberately does NOT survive the cursor moving between two menu + * opens, because the URL genuinely differs then and one build is the minimum either way. + * + * Read and written only from the main thread, because `ActionMenuUtils.showPopupWindow` prepares + * these actions synchronously from the touch handler that opens the menu -- not because of + * `requiresUIThread`, which governs `execAction` alone. */ private var lastBuilt: Pair? = null @@ -260,7 +287,7 @@ class CreateLinkAction( // anywhere -- the file picker, Recents, a clone destination. For one of those there is no URL // that resolves on this device, let alone on the recipient's, so there is nothing honest to // put on the clipboard. - if (linkableProject(activity, projectPath) != true) { + if (linkableProject(projectPath) != true) { return null } diff --git a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt index d95c55b263..6decb3d8cd 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt @@ -113,8 +113,9 @@ object ActionMenuUtils { LinearLayout.LayoutParams.MATCH_PARENT, 1, ).apply { - topMargin = 8 - bottomMargin = 8 + // dp, not raw pixels: a literal 8 is 8dp on a 1x device and 2.7dp on a 3x one. + topMargin = context.dpToPx(8f) + bottomMargin = context.dpToPx(8f) } val typedValue = android.util.TypedValue() context.theme.resolveAttribute( diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt index 979dced7f6..6a8e609f8a 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt @@ -146,15 +146,13 @@ internal fun isDeepLinkTargetOfOpenProject( openProjectPath: String, projectName: String, projectsRoot: File, -): Boolean { - // Whatever can be settled without touching the disk, settle here. - deepLinkTargetOfOpenProjectWithoutIo(openProjectPath, projectName, projectsRoot)?.let { return it } - - // Reached only when the shortcut deferred, which it does only after establishing a non-null - // parent and a matching name -- so this re-derives the parent rather than re-checking the rest. - val parent = File(openProjectPath).parentFile ?: return false - return canonicalOrAbsolute(parent) == canonicalOrAbsolute(projectsRoot) -} +): Boolean = + // "Could not tell" collapses to "no" for callers that only want a Boolean. That is not a + // behaviour change from the older canonicalOrAbsolute fallback: that fallback compared the plain + // absolutePath when canonicalisation failed, and equal absolute paths are already settled true by + // the no-IO shortcut -- so reaching it at all meant they differed, and the answer was false + // either way. Expressing it this way keeps the canonicalisation spelled once. + deepLinkTargetOfOpenProjectOrNull(openProjectPath, projectName, projectsRoot) ?: false /** * [isDeepLinkTargetOfOpenProject] with "could not tell" kept apart from "no", for a caller that @@ -174,6 +172,8 @@ internal fun deepLinkTargetOfOpenProjectOrNull( ): Boolean? { deepLinkTargetOfOpenProjectWithoutIo(openProjectPath, projectName, projectsRoot)?.let { return it } + // Non-null by construction: the shortcut returns false, not null, when there is no parent, so + // deferring to here already established one. val parent = File(openProjectPath).parentFile ?: return false val parentPath = runCatching { parent.canonicalPath }.getOrNull() ?: return null val rootPath = runCatching { projectsRoot.canonicalPath }.getOrNull() ?: return null @@ -209,8 +209,6 @@ internal fun deepLinkTargetOfOpenProjectWithoutIo( return null } -private fun canonicalOrAbsolute(file: File): String = runCatching { file.canonicalPath }.getOrElse { file.absolutePath } - /** Determines if the directory contains a valid Android project structure. */ fun isValidProjectDirectory(selectedDir: File): Boolean { if (isPluginProject(selectedDir)) { diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt index edbd695dbc..4981420fb0 100644 --- a/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt @@ -36,14 +36,25 @@ import java.io.File @RunWith(RobolectricTestRunner::class) class DeepLinkManifestPrefixTest { @Test - fun `every manifest pathPrefix matches the prefix buildUrl emits`() { - val manifest = File("src/main/AndroidManifest.xml") - assertThat(manifest.exists()).isTrue() + fun `every deep-link pathPrefix in the manifest matches the prefix buildUrl emits`() { + // Located by trying both roots rather than assuming one: the Gradle test task runs with the + // module directory as the working directory, an IDE run configuration often uses the repo + // root, and a wrong guess would fail as "manifest missing" rather than as real drift. + val manifest = + listOf("src/main/AndroidManifest.xml", "app/src/main/AndroidManifest.xml") + .map(::File) + .firstOrNull { it.isFile } + assertThat(manifest).isNotNull() + // Scoped to elements that name a deep-link host, NOT every pathPrefix in the file. An + // unrelated App Link added elsewhere in the manifest is not drift in this scheme, and failing + // on it would point the reader at DeepLinkRequest for someone else's change. val declared = - Regex("""android:pathPrefix\s*=\s*"([^"]*)"""") - .findAll(manifest.readText()) - .map { it.groupValues[1] } + Regex("""]*>""", RegexOption.DOT_MATCHES_ALL) + .findAll(manifest!!.readText()) + .map { it.value } + .filter { it.contains("appdevforall.org") } + .mapNotNull { Regex("""android:pathPrefix\s*=\s*"([^"]*)"""").find(it)?.groupValues?.get(1) } .toList() // If the intent-filter stops declaring a prefix, this test must fail rather than vacuously diff --git a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt index 0ba593a36a..dceb37d74d 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt @@ -205,4 +205,35 @@ class ProjectValidationsTest { assertThat(shortcut).isEqualTo(independentlyLinkable(path, name)) } } + + @Test + fun `the nullable variant resolves a symlinked parent that differs as text`() { + val root = tempFolder.newFolder("real-projects") + val project = File(root, "MyApp") + assertThat(project.mkdirs()).isTrue() + + // An alias to the same directory, the shape /sdcard -> /storage/self/primary has on a device. + val alias = File(tempFolder.root, "alias") + java.nio.file.Files.createSymbolicLink(alias.toPath(), root.toPath()) + + val viaAlias = File(alias, "MyApp").path + // The no-IO shortcut cannot settle this -- the parents differ as text. + assertThat(deepLinkTargetOfOpenProjectWithoutIo(viaAlias, "MyApp", root)).isNull() + // Canonicalising does, and that is the branch the nullable variant exists for. + assertThat(deepLinkTargetOfOpenProjectOrNull(viaAlias, "MyApp", root)).isTrue() + } + + @Test + fun `the nullable variant reports null, not false, when a path cannot be canonicalised`() { + val root = tempFolder.newFolder("projects3") + + // A NUL makes getCanonicalPath throw IOException. "Could not tell" must stay distinct from + // "no": a caller that caches false here latches "not linkable" for the whole process over a + // condition that may be momentary. + val unresolvable = File("/nonexistent\u0000dir/MyApp").path + assertThat(deepLinkTargetOfOpenProjectOrNull(unresolvable, "MyApp", root)).isNull() + + // And the Boolean form collapses that to false for callers that only want a Boolean. + assertThat(isDeepLinkTargetOfOpenProject(unresolvable, "MyApp", root)).isFalse() + } } diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 7bec8cfd8e..67cb4df9b2 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -148,7 +148,7 @@ Could not scan projects for this link. Link copied to the clipboard. Could not create a link for this file. - Code on the Go link + %1$s link A project close is already in progress. Try again in a moment. Create new project Open a saved project From 29b00320c9cdea86e67fc755c90ce02152017d61 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 05:51:07 -0700 Subject: [PATCH 14/16] ADFA-5472: Spotless Formatting only: a line wrap and a blank line. I ran spotlessApply before committing this time, but a Gradle daemon lock made that invocation fail without my checking its result -- so the hook still had work to do. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../com/itsaky/androidide/actions/file/CreateLinkAction.kt | 1 - .../java/com/itsaky/androidide/utils/ProjectValidationsTest.kt | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index f094437e7e..3f48dc999a 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -68,7 +68,6 @@ class CreateLinkAction( /** Paths whose canonicalisation is already running, so N menu opens launch one job, not N. */ private val canonicalisationsInFlight: MutableSet = ConcurrentHashMap.newKeySet() - } /** diff --git a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt index dceb37d74d..bd003a54cf 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/ProjectValidationsTest.kt @@ -214,7 +214,8 @@ class ProjectValidationsTest { // An alias to the same directory, the shape /sdcard -> /storage/self/primary has on a device. val alias = File(tempFolder.root, "alias") - java.nio.file.Files.createSymbolicLink(alias.toPath(), root.toPath()) + java.nio.file.Files + .createSymbolicLink(alias.toPath(), root.toPath()) val viaAlias = File(alias, "MyApp").path // The no-IO shortcut cannot settle this -- the parents differ as text. From d45ec86bec9c421ec6eb4f736aa6f8a668dee0e0 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 06:34:10 -0700 Subject: [PATCH 15/16] ADFA-5472: Revert two fixes from the last round that made things worse Eleven of twelve findings. Two of them are mine from the previous round, and both were wrong in the same way: I took a reviewer's suggestion at face value without checking the surrounding facts. destroy() must not clear the caches. I added that clearing last round on the grounds that it "costs nothing and removes the whole class of staleness". It costs plenty: EditorActivityLifecyclerObserver.onPause calls EditorActivityActions.clearActions(), which calls destroy() on every action in this location, and onResume registers fresh ones. So destroy() runs on every backgrounding, not at teardown. The clearing therefore threw the verdict cache away on each background/foreground cycle -- so the item went missing from the first menu open after every resume -- and clearing the in-flight set alongside it let a still-running job's completion release a claim the next cycle's job had already taken, which is exactly the duplicate work the claim exists to prevent. Reverted, with the pause/resume fact recorded so the suggestion is not retried. isDeepLinkTargetOfOpenProject keeps its per-side fallback. I collapsed it to `deepLinkTargetOfOpenProjectOrNull(...) ?: false` last round and asserted in the commit message that this "is not a behaviour change ... the answer was false either way". It is a behaviour change: the old form compares per side, so when exactly one side's canonicalPath fails it still compares that side's absolutePath against the other's canonical path, and those can match. At this function's two callers a false means a same-project deep link is treated as a project switch -- the project is closed and reopened instead of the file being shown. The two functions now differ deliberately: lenient for callers that must answer now, strict for the one caller that remembers the answer. That also makes the KDoc true again, including its [canonicalOrAbsolute] link. The rest: The popup is bounded to the room below its anchor. WRAP_CONTENT reports height -2 to showAsDropDown, whose fit check is then trivially satisfied, so the content was measured against the whole display and the ScrollView believed it fit. capHeightToSpaceBelow measures and caps against getMaxAvailableHeight, at both call sites. It leaves the popup alone when the content already fits, so a bad measurement degrades to the previous behaviour rather than a zero-height window. The manifest guard now pins host and scheme, not just the path. An intent-filter matches on all three, so changing CANONICAL_HOST or SCHEME breaks delivery exactly as changing the prefix does -- and the path-only assertion stayed green through both. The one-based cursor conversion is named and tested. It is called an invariant twice in ARCHITECTURE.md and had no test at any level; dropping a `+ 1` fails silently, because buildUrl then refuses the link and the item merely disappears. oneBasedCursorPosition is now covered directly, plus a test showing the raw zero-based values produce no link at all -- which is why the conversion is load-bearing rather than cosmetic. That test needed a real android.net.Uri, so CreateLinkActionTest gains the Robolectric runner its siblings already use. projectsRoot() failing is reported as false, not null. null means "ask again shortly" and there is nothing to wait for, so the item was hidden forever while the code promised a retry; the catch is also narrow now, because runCatching there swallowed Error while a comment twenty lines below explicitly rejected runCatching for that reason. Corrected comments rather than leaving them: onNewIntent's claim that the same-project check is "disk-free" (it falls through to two canonicalPath calls on the main thread), prepare()'s hide-silently rationale (which did not cover the length ceiling, a property of the FILE -- so the item comes and goes as the user switches between a deep and a shallow tab), and a note that the shared rule's name half is trivially satisfied at this call site because the name is derived from the path being compared. Deferred: the shared "close options" tooltip is now ADFA-5477, with the two blockers recorded there. Also worth recording: every `:app:spotlessApply` in this branch's history silently did nothing -- there is no such task, only a root-level `spotlessApply`, and the "task not found" failure looked like the daemon noise I had been seeing. That is why the hook kept finding work. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../actions/file/CreateLinkAction.kt | 72 +++++++++++++------ .../editor/EditorHandlerActivity.kt | 12 +++- .../androidide/utils/ActionMenuUtils.kt | 28 ++++++++ .../androidide/utils/ProjectValidations.kt | 27 +++++-- .../actions/file/CreateLinkActionTest.kt | 21 ++++++ .../models/DeepLinkManifestPrefixTest.kt | 23 ++++-- 6 files changed, 150 insertions(+), 33 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index 3f48dc999a..81ee288873 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -61,8 +61,19 @@ class CreateLinkAction( /** * Verdicts for "can a link name this project", keyed by project path. Keyed rather than a * single slot so a slow answer for a project the user has left cannot be mistaken for the - * current one -- there is nothing to invalidate and no stale-write window to guard. Holds only - * strings and booleans, so it is safe in a companion. + * current one. Holds only strings and booleans, so it is safe in a companion. + * + * Deliberately NOT cleared in `destroy()`, which was tried and is wrong: + * `EditorActivityLifecyclerObserver.onPause` calls `EditorActivityActions.clearActions()`, + * which destroys every action in this location, and `onResume` registers fresh ones. So + * `destroy()` runs on every backgrounding, not at teardown -- clearing here threw the cache + * away on each background/foreground cycle, and clearing the in-flight set alongside it let a + * still-running job's completion release a claim the next cycle's job had already taken. + * + * The cost of keeping them is a verdict that outlives the fact it describes: rename the + * project directory mid-process and a cached answer is stale. That surfaces as one link that + * fails to open, or one item hidden that need not be -- both recoverable, and both cheaper + * than losing the answer every time the user switches apps. */ private val linkableProjects = ConcurrentHashMap() @@ -87,7 +98,19 @@ class CreateLinkAction( private fun linkableProject(projectPath: String): Boolean? { linkableProjects[projectPath]?.let { return it } - val root = runCatching { projectsRoot() }.getOrNull() ?: return null + // false, not null: null means "ask again shortly", and there is nothing to wait for if the + // projects root cannot be derived at all. Narrow, because runCatching here would swallow Error + // too -- the same reason the IO block below does not use it. + val root = + try { + projectsRoot() + } catch (e: Exception) { + log.warn("Cannot locate the projects directory, so nothing can be linked", e) + return false + } + + // The name half of the shared rule is trivially satisfied here -- the name is derived from the + // very path being compared -- so this call is really asking the parent question alone. val projectName = File(projectPath).name deepLinkTargetOfOpenProjectWithoutIo(projectPath, projectName, root)?.let { @@ -152,16 +175,6 @@ class CreateLinkAction( icon = ContextCompat.getDrawable(context, R.drawable.ic_copy) } - override fun destroy() { - super.destroy() - // A verdict describes the filesystem as it was. Renaming the project directory, or swapping - // for a symlink, while the process lives would leave a cached answer to the old - // question -- so the answers do not outlive the activity that asked. REVIEW.md section 2 flags - // companion-held state for exactly this. - linkableProjects.clear() - canonicalisationsInFlight.clear() - } - override fun prepare(data: ActionData) { super.prepare(data) @@ -173,8 +186,13 @@ class CreateLinkAction( // there is no activity, and the !visible check just above returns in exactly that case. val activity = data.getActivity() ?: return - // Hidden rather than shown-and-failing: the states that produce no link are properties of how - // the project was opened, not transient ones the user could correct by tapping again. + // Hidden rather than shown-and-failing. Most states that produce no link are properties of how + // the project was opened, which a second tap could not correct. One is a property of the FILE: + // a path long enough to push the decoded URL past parse()'s 512-character ceiling. That one + // makes the item come and go as the user switches between a deep and a shallow tab, with no + // explanation offered, because the tap that would explain is never available. Surfacing a + // reason means buildUrl reporting WHY it refused rather than just null -- worth doing, and + // deliberately not bundled into this change. // // Only the cheap predicates run here. prepare() is called synchronously, from the touch // handler, for every action in this menu on every open, and building the URL just to compare @@ -290,15 +308,15 @@ class CreateLinkAction( return null } - // The editor is zero-based at both ends; the URL scheme is one-based. Both coordinates are - // always written, never omitted at 1:1 -- see buildUrl, which refuses the coordinate-free and - // column-only shapes precisely because the reader mis-handles them. + // Both coordinates are always written, never omitted at 1:1 -- see buildUrl, which refuses the + // coordinate-free and column-only shapes precisely because the reader mis-handles them. val cursor = editor.cursor + val (line, column) = oneBasedCursorPosition(cursor.leftLine, cursor.leftColumn) return LinkTarget( projectName = projectDir.name, relativePath = relativePath, - line = cursor.leftLine + 1, - column = cursor.leftColumn + 1, + line = line, + column = column, ) } } @@ -323,3 +341,17 @@ internal fun projectRelativePathOrNull( } return relative } + +/** + * The one-based line and column a link carries, from the editor's zero-based cursor. + * + * Two characters of arithmetic, named and tested because dropping either `+ 1` fails silently: + * buildUrl refuses a line or column below 1, so a cursor at the very start of a file simply makes + * the menu item vanish -- indistinguishable from the ordinary hidden-when-unlinkable state -- and a + * cursor anywhere else yields a link that quietly points one line too high. + */ +@VisibleForTesting +internal fun oneBasedCursorPosition( + zeroBasedLine: Int, + zeroBasedColumn: Int, +): Pair = (zeroBasedLine + 1) to (zeroBasedColumn + 1) diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt index e721f608c7..c2e6933792 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt @@ -111,6 +111,7 @@ import com.itsaky.androidide.tasks.executeAsync import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import com.itsaky.androidide.ui.ARCHIVE_EXTENSIONS import com.itsaky.androidide.ui.CodeEditorView +import com.itsaky.androidide.utils.ActionMenuUtils import com.itsaky.androidide.utils.DeepLinkProjectLookup import com.itsaky.androidide.utils.DialogUtils.newMaterialDialogBuilder import com.itsaky.androidide.utils.DialogUtils.showConfirmationDialog @@ -2240,6 +2241,8 @@ open class EditorHandlerActivity : } binding.actionItems.addView(undockItem) + // Same cap as the file-tab popup: this popup shares that layout, so it shares the problem. + with(ActionMenuUtils) { popupWindow.capHeightToSpaceBelow(anchorView, binding.root) } popupWindow.showAsDropDown(anchorView, 0, 0) } @@ -2670,8 +2673,13 @@ open class EditorHandlerActivity : // branch reads the carried-forward extra itself -- they only apply whatever fileRequest THIS // intent carries, which is often none. Comparing the deep link's project name against the // currently-loading project's directory name (mirroring BaseEditorActivity.onCreate's own - // deepLinkTargetsAnotherProject check) is a synchronous, disk-free way to tell same from - // different without waiting on the deep-link path's own async resolve. + // deepLinkTargetsAnotherProject check) is a synchronous way to tell same from + // different without waiting on the deep-link path's own async resolve. It is not free, though: + // this comment used to claim "disk-free", and isDeepLinkTargetOfOpenProject falls through to + // two File.canonicalPath calls whenever the paths differ as text. Deep-link arrival is a rare, + // user-initiated event so the stall is bounded, but it is a main-thread read on this path and + // on BaseEditorActivity.onCreate's -- see deepLinkTargetOfOpenProjectWithoutIo for the half + // that needs no disk, which CreateLinkAction uses because it is called on every menu open. // EditorIntentExtras.EXTRA_PREVIOUS_PROJECT_PATH, when present, is what IProjectManager.projectDirPath held before // MainActivity.openProject's bookkeeping call overwrote it to the NEW path -- by the time this // intent arrives here, the global itself already reads as the new path regardless of whether diff --git a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt index 6decb3d8cd..9b56fc3bc3 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt @@ -35,6 +35,33 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.plugins.extensions.FileTabMenuItem object ActionMenuUtils { + /** + * Caps [this] at the room actually available below [anchorView], measuring [content] first. + * + * A PopupWindow built WRAP_CONTENT reports height -2 to `showAsDropDown`, and its fit check + * (`height <= spaceBelow`) is therefore trivially satisfied -- so no resize is ever negotiated and + * the content gets measured against the whole display instead of the space under the tab strip. + * The ScrollView then believes it fits and does not scroll, and the window is left to run off the + * bottom or be shoved up over the tabs and toolbar. At 2x font scale with the undock item and + * plugin contributions, this menu can reach that size. + * + * Left alone when the content already fits, so the failure mode of a bad measurement is the + * previous behaviour rather than a zero-height popup. + */ + internal fun PopupWindow.capHeightToSpaceBelow( + anchorView: View, + content: View, + ) { + val available = getMaxAvailableHeight(anchorView) + if (available <= 0) return + + val unspecified = View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED) + content.measure(unspecified, unspecified) + if (content.measuredHeight > available) { + height = available + } + } + fun showPopupWindow( context: Context, anchorView: View, @@ -161,6 +188,7 @@ object ActionMenuUtils { } } + popupWindow.capHeightToSpaceBelow(anchorView, binding.root) popupWindow.showAsDropDown(anchorView, 0, 0) } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt index 6a8e609f8a..eebbae8644 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ProjectValidations.kt @@ -146,13 +146,24 @@ internal fun isDeepLinkTargetOfOpenProject( openProjectPath: String, projectName: String, projectsRoot: File, -): Boolean = - // "Could not tell" collapses to "no" for callers that only want a Boolean. That is not a - // behaviour change from the older canonicalOrAbsolute fallback: that fallback compared the plain - // absolutePath when canonicalisation failed, and equal absolute paths are already settled true by - // the no-IO shortcut -- so reaching it at all meant they differed, and the answer was false - // either way. Expressing it this way keeps the canonicalisation spelled once. - deepLinkTargetOfOpenProjectOrNull(openProjectPath, projectName, projectsRoot) ?: false +): Boolean { + deepLinkTargetOfOpenProjectWithoutIo(openProjectPath, projectName, projectsRoot)?.let { return it } + + // Deliberately NOT `deepLinkTargetOfOpenProjectOrNull(...) ?: false`, which was tried on the + // grounds that it could not change behaviour. It can: this compares per side, so when exactly one + // side's canonicalPath fails it still compares that side's absolutePath against the other's + // canonical path -- and those can match, e.g. a projectsRoot given as /sdcard/CodeOnTheGoProjects + // against an open project under /storage/emulated/0/CodeOnTheGoProjects whose parent has just + // become unreadable. The nullable variant returns null on the first failure, which collapses to + // false, and at this function's two callers a false means a same-project deep link is treated as + // a project switch: the project is closed and reopened instead of the file simply being shown. + // + // So the two functions differ on purpose. This one stays lenient for callers that must answer + // now; the nullable one is strict for a caller that will REMEMBER the answer, where a wrong false + // outlives the condition that caused it. + val parent = File(openProjectPath).parentFile ?: return false + return canonicalOrAbsolute(parent) == canonicalOrAbsolute(projectsRoot) +} /** * [isDeepLinkTargetOfOpenProject] with "could not tell" kept apart from "no", for a caller that @@ -209,6 +220,8 @@ internal fun deepLinkTargetOfOpenProjectWithoutIo( return null } +private fun canonicalOrAbsolute(file: File): String = runCatching { file.canonicalPath }.getOrElse { file.absolutePath } + /** Determines if the directory contains a valid Android project structure. */ fun isValidProjectDirectory(selectedDir: File): Boolean { if (isPluginProject(selectedDir)) { diff --git a/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt b/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt index be5804e2e8..757819bf13 100644 --- a/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt +++ b/app/src/test/java/com/itsaky/androidide/actions/file/CreateLinkActionTest.kt @@ -18,13 +18,17 @@ package com.itsaky.androidide.actions.file import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.models.DeepLinkRequest import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner import java.io.File /** * The non-UI half of [CreateLinkAction]: deciding what path a link may name. A link gets handed to * another person, so "outside the project" has to be a hard no rather than a best effort. */ +@RunWith(RobolectricTestRunner::class) class CreateLinkActionTest { private val project = File("/storage/emulated/0/CodeOnTheGoProjects/MyApp") @@ -70,4 +74,21 @@ class CreateLinkActionTest { // which is the branch worth covering anyway. assertThat(projectRelativePathOrNull(project, File("/data/local/tmp/Main.kt"))).isNull() } + + @Test + fun `the cursor is converted from zero-based to one-based`() { + assertThat(oneBasedCursorPosition(0, 0)).isEqualTo(1 to 1) + assertThat(oneBasedCursorPosition(9, 4)).isEqualTo(10 to 5) + } + + @Test + fun `a cursor at the very start of a file still yields a link`() { + // Why the conversion is load-bearing rather than cosmetic: buildUrl refuses a line or column + // below 1, so passing the raw zero-based cursor produces no link at all -- and the action + // hides the menu item, which looks exactly like the ordinary hidden-when-unlinkable state. + val (line, column) = oneBasedCursorPosition(0, 0) + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line, column)) + .endsWith("/file/Main.kt/line/1/column/1") + assertThat(DeepLinkRequest.buildUrl("MyApp", "Main.kt", 0, 0)).isNull() + } } diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt index 4981420fb0..07411f60df 100644 --- a/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt @@ -49,13 +49,13 @@ class DeepLinkManifestPrefixTest { // Scoped to elements that name a deep-link host, NOT every pathPrefix in the file. An // unrelated App Link added elsewhere in the manifest is not drift in this scheme, and failing // on it would point the reader at DeepLinkRequest for someone else's change. - val declared = + val elements = Regex("""]*>""", RegexOption.DOT_MATCHES_ALL) .findAll(manifest!!.readText()) .map { it.value } .filter { it.contains("appdevforall.org") } - .mapNotNull { Regex("""android:pathPrefix\s*=\s*"([^"]*)"""").find(it)?.groupValues?.get(1) } .toList() + val declared = elements.mapNotNull { attribute(it, "android:pathPrefix") } // If the intent-filter stops declaring a prefix, this test must fail rather than vacuously // pass over an empty list -- there are two elements today, one per verified host. @@ -64,10 +64,25 @@ class DeepLinkManifestPrefixTest { // Taken from a URL the builder actually produces, so this asserts against the emitted shape // rather than against a second copy of the constant. val built = DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 1, column = 1) - val path = Uri.parse(built!!).path!! + val emitted = Uri.parse(built!!) for (prefix in declared) { - assertThat(path).startsWith(prefix) + assertThat(emitted.path).startsWith(prefix) } + + // Host and scheme too, not only the path. An intent-filter matches on all three, so changing + // CANONICAL_HOST or SCHEME without the manifest breaks delivery exactly as changing the prefix + // does -- and a path-only assertion stays green through both. + val hosts = elements.mapNotNull { attribute(it, "android:host") }.toSet() + val schemes = elements.mapNotNull { attribute(it, "android:scheme") }.toSet() + assertThat(hosts).isNotEmpty() + assertThat(schemes).isNotEmpty() + assertThat(hosts).contains(emitted.host) + assertThat(schemes).contains(emitted.scheme) } + + private fun attribute( + element: String, + name: String, + ): String? = Regex("""$name\s*=\s*"([^"]*)"""").find(element)?.groupValues?.get(1) } From a7df5f0003ec9002e17b47f5a811087da02af15a Mon Sep 17 00:00:00 2001 From: David Schachter Date: Fri, 4 Sep 2026 07:53:53 -0700 Subject: [PATCH 16/16] ADFA-5472: Withdraw the popup height cap, and eight other fixes The cap I added last round is gone. It was wrong twice over, and I could not have found either by reading my own code: PopupWindow.getMaxAvailableHeight returns Math.max(distanceToBottom, distanceToTop), not the space below -- verified in AOSP android-36 PopupWindow.java:2010. So for an anchor in the lower half of the frame the cap set the height to the space ABOVE, showAsDropDown's fit check then failed, and the popup flipped to sit above the anchor at that height: drawn over the tab strip and toolbar, which is precisely the outcome the helper's own KDoc named as the thing to avoid. The name, the KDoc and the comment I added to EditorHandlerActivity all asserted something the platform contradicts. It also measured with an UNSPECIFIED width spec, so a wrapping label reported its unwrapped single-line height and the cap was skipped in the case that needs it most -- a long plugin title at 2x font scale. Both are fixable. Neither is fixable blind, and this is the third unverifiable change I have made to this popup's sizing. The ScrollView and its scrollbar stay, because they are inert until content exceeds the display and they do not move the window; the sizing work -- a correct distance-to-bottom cap, isFocusable for key/D-pad reachability once the fold engages, and extracting the two hand-rolled copies of this popup into one factory -- belongs behind a device check at 1x and 2x. I would rather hand over a documented gap than a measurement nobody has watched. The rest: urlFor now remembers a refusal, not just a URL. A file buildUrl can never express -- a path past the 512-character ceiling -- repeated the whole encode-and-reparse on every menu open and discarded it, which is exactly the cost this cache was added to remove. linkableProjects is pruned to the open project on every lookup. One entry instead of one per project the session ever touched, and it also retires the staleness the last round's KDoc conceded: renaming the project directory changes the key, so the old verdict is dropped rather than left answering a question about a directory that no longer exists. The divider's height is dp. Last round converted its margins and walked past the height literal in the same constructor -- 1px is 0.33dp on a 3x device, so the separator was effectively invisible while its margins were correct. "Close Tab" is a string resource. It sat hardcoded in English two lines from the diff's own edits, next to a getString'd "Undock" in the same two-item popup. buildUrl rejects percent-encoded dot segments. A directory literally named "%2e%2e" round-trips cleanly, then gets rewritten in transit by anything that decodes once and collapses dot segments -- the same hazard the literal "."/".." rejection exists for. The manifest test stops carrying its own copies. It hard-coded "appdevforall.org" to select the elements -- a third copy of CANONICAL_HOST, in the test written to stop that constant being duplicated, so renaming the domain correctly everywhere would have failed it for a change with no drift. It now selects by the host the builder emits, asserts scheme as well as path, and finds the manifest through FileProvider.projectRoot() and the repo's .androidide_root sentinel instead of guessing two relative paths. That sentinel is reachable after all -- app's testImplementation(projects.testing.unit) exposes api(projects.shared) -- which an earlier review had told me it was not. Also removed: the unreachable null-parent branch in deepLinkTargetOfOpenProjectOrNull, whose own comment said it could not be reached. Still open: ADFA-5477 for the tooltip, and the popup sizing gap above. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gw89A3KWsPtvgPYtXYLBwr --- .../actions/file/CreateLinkAction.kt | 26 ++++++-- .../editor/EditorHandlerActivity.kt | 7 +-- .../androidide/models/DeepLinkRequest.kt | 7 ++- .../androidide/utils/ActionMenuUtils.kt | 32 +--------- .../models/DeepLinkManifestPrefixTest.kt | 60 ++++++++----------- resources/src/main/res/values/strings.xml | 1 + 6 files changed, 56 insertions(+), 77 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt index 81ee288873..af06fb8cd0 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/CreateLinkAction.kt @@ -70,10 +70,10 @@ class CreateLinkAction( * away on each background/foreground cycle, and clearing the in-flight set alongside it let a * still-running job's completion release a claim the next cycle's job had already taken. * - * The cost of keeping them is a verdict that outlives the fact it describes: rename the - * project directory mid-process and a cached answer is stale. That surfaces as one link that - * fails to open, or one item hidden that need not be -- both recoverable, and both cheaper - * than losing the answer every time the user switches apps. + * Bounded instead by pruning to the open project on every lookup -- see [linkableProject]. + * Exactly one project is open at a time, so this holds one entry rather than one per project + * the session ever touched, and a path that stops being the open one is dropped rather than + * left answering a question about a directory that may since have been renamed. */ private val linkableProjects = ConcurrentHashMap() @@ -96,6 +96,15 @@ class CreateLinkAction( * it optimistically would mean a tap that fails, which reads worse. */ private fun linkableProject(projectPath: String): Boolean? { + // Only the open project's verdict is worth keeping, and exactly one project is open at a time. + // Pruning here bounds the map to a single entry rather than one per project the session ever + // touched, and it is also what retires a stale answer: renaming the project directory changes + // this key, so the old verdict is dropped instead of left answering a question about a + // directory that no longer exists. + if (linkableProjects.keys.any { it != projectPath }) { + linkableProjects.keys.retainAll(setOf(projectPath)) + } + linkableProjects[projectPath]?.let { return it } // false, not null: null means "ask again shortly", and there is nothing to wait for if the @@ -239,13 +248,18 @@ class CreateLinkAction( * these actions synchronously from the touch handler that opens the menu -- not because of * `requiresUIThread`, which governs `execAction` alone. */ - private var lastBuilt: Pair? = null + private var lastBuilt: Pair? = null /** The URL for the current tab, built at most once per menu open, or `null` if there is none. */ private fun urlFor(activity: EditorHandlerActivity): String? { val target = linkTarget(activity) ?: return null lastBuilt?.let { (built, url) -> if (built == target) return url } - val url = target.toUrl() ?: return null + + // The refusal is remembered too, not just the URL. A file buildUrl will never express -- a + // path over the 512-character ceiling, say -- is a permanent state, so leaving it uncached + // meant repeating the whole encode-and-reparse on every menu open and throwing it away, which + // is the cost this cache exists to remove. + val url = target.toUrl() lastBuilt = target to url return url } diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt index c2e6933792..85dd5754bf 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/EditorHandlerActivity.kt @@ -111,7 +111,6 @@ import com.itsaky.androidide.tasks.executeAsync import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult import com.itsaky.androidide.ui.ARCHIVE_EXTENSIONS import com.itsaky.androidide.ui.CodeEditorView -import com.itsaky.androidide.utils.ActionMenuUtils import com.itsaky.androidide.utils.DeepLinkProjectLookup import com.itsaky.androidide.utils.DialogUtils.newMaterialDialogBuilder import com.itsaky.androidide.utils.DialogUtils.showConfirmationDialog @@ -2200,7 +2199,9 @@ open class EditorHandlerActivity : ).root closeItem.apply { - text = "Close Tab" + // A string resource, like the "Undock" item eleven lines below: this label sat + // untranslated next to a translated one in the same two-item popup. + text = getString(string.action_close_tab) setOnClickListener { val position = tab.position if (isPluginTab(position)) { @@ -2241,8 +2242,6 @@ open class EditorHandlerActivity : } binding.actionItems.addView(undockItem) - // Same cap as the file-tab popup: this popup shares that layout, so it shares the problem. - with(ActionMenuUtils) { popupWindow.capHeightToSpaceBelow(anchorView, binding.root) } popupWindow.showAsDropDown(anchorView, 0, 0) } diff --git a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt index 9e2b1dbaaa..53d312cf20 100644 --- a/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt +++ b/app/src/main/java/com/itsaky/androidide/models/DeepLinkRequest.kt @@ -354,7 +354,12 @@ data class DeepLinkRequest( // directory, which resolveWithinDirectory refuses as "not a path inside". Other // dot-prefixed names are fine -- unlike a project directory, a hidden FILE // (.gitignore) is perfectly linkable. - if (segments.any { it.isEmpty() || it == "." }) { + // The decoded form is checked too: a directory literally named "%2e%2e" survives + // encoding as "%252e%252e" and round-trips cleanly here, but an intermediary that + // decodes once and then collapses dot segments rewrites the path in transit -- the same + // hazard the literal "."/".." rejection exists for. It fails closed either way, but a + // dead link is still the defect that check was written to prevent. + if (segments.any { it.isEmpty() || it == "." || Uri.decode(it) == "." || Uri.decode(it) == ".." }) { return null } diff --git a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt index 9b56fc3bc3..b68232c40b 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/ActionMenuUtils.kt @@ -35,33 +35,6 @@ import com.itsaky.androidide.idetooltips.TooltipTag import com.itsaky.androidide.plugins.extensions.FileTabMenuItem object ActionMenuUtils { - /** - * Caps [this] at the room actually available below [anchorView], measuring [content] first. - * - * A PopupWindow built WRAP_CONTENT reports height -2 to `showAsDropDown`, and its fit check - * (`height <= spaceBelow`) is therefore trivially satisfied -- so no resize is ever negotiated and - * the content gets measured against the whole display instead of the space under the tab strip. - * The ScrollView then believes it fits and does not scroll, and the window is left to run off the - * bottom or be shoved up over the tabs and toolbar. At 2x font scale with the undock item and - * plugin contributions, this menu can reach that size. - * - * Left alone when the content already fits, so the failure mode of a bad measurement is the - * previous behaviour rather than a zero-height popup. - */ - internal fun PopupWindow.capHeightToSpaceBelow( - anchorView: View, - content: View, - ) { - val available = getMaxAvailableHeight(anchorView) - if (available <= 0) return - - val unspecified = View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED) - content.measure(unspecified, unspecified) - if (content.measuredHeight > available) { - height = available - } - } - fun showPopupWindow( context: Context, anchorView: View, @@ -138,9 +111,9 @@ object ActionMenuUtils { LinearLayout .LayoutParams( LinearLayout.LayoutParams.MATCH_PARENT, - 1, + context.dpToPx(1f), ).apply { - // dp, not raw pixels: a literal 8 is 8dp on a 1x device and 2.7dp on a 3x one. + // dp, not raw pixels -- height included: a literal 1 is 0.33dp on a 3x device. topMargin = context.dpToPx(8f) bottomMargin = context.dpToPx(8f) } @@ -188,7 +161,6 @@ object ActionMenuUtils { } } - popupWindow.capHeightToSpaceBelow(anchorView, binding.root) popupWindow.showAsDropDown(anchorView, 0, 0) } } diff --git a/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt index 07411f60df..c05d49a9af 100644 --- a/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt +++ b/app/src/test/java/com/itsaky/androidide/models/DeepLinkManifestPrefixTest.kt @@ -19,10 +19,10 @@ package com.itsaky.androidide.models import android.net.Uri import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.utils.FileProvider import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner -import java.io.File /** * The deep-link path prefix is spelled in two places that cannot see each other: `DeepLinkRequest`, @@ -36,49 +36,37 @@ import java.io.File @RunWith(RobolectricTestRunner::class) class DeepLinkManifestPrefixTest { @Test - fun `every deep-link pathPrefix in the manifest matches the prefix buildUrl emits`() { - // Located by trying both roots rather than assuming one: the Gradle test task runs with the - // module directory as the working directory, an IDE run configuration often uses the repo - // root, and a wrong guess would fail as "manifest missing" rather than as real drift. - val manifest = - listOf("src/main/AndroidManifest.xml", "app/src/main/AndroidManifest.xml") - .map(::File) - .firstOrNull { it.isFile } - assertThat(manifest).isNotNull() + fun `the manifest's deep-link filter matches the scheme, host and path buildUrl emits`() { + // Located from the repo's own root sentinel rather than by guessing relative paths: the + // previous two-guess list failed as "manifest missing" under any working directory that was + // neither the module nor the repo root, which is the misdiagnosis it was meant to avoid. + val manifest = FileProvider.projectRoot().resolve("app/src/main/AndroidManifest.xml").toFile() + assertThat(manifest.isFile).isTrue() - // Scoped to elements that name a deep-link host, NOT every pathPrefix in the file. An - // unrelated App Link added elsewhere in the manifest is not drift in this scheme, and failing - // on it would point the reader at DeepLinkRequest for someone else's change. + // Taken from a URL the builder actually produces, so every assertion below is against the + // emitted shape rather than a second copy of a constant. + val emitted = Uri.parse(DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 1, column = 1)!!) + + // The elements are selected by the host the builder emits, not by a hard-coded domain. A + // literal here would be a third copy of CANONICAL_HOST -- and renaming the domain correctly + // in both Kotlin and the manifest would then fail this test for a change with no drift at all. val elements = Regex("""]*>""", RegexOption.DOT_MATCHES_ALL) - .findAll(manifest!!.readText()) + .findAll(manifest.readText()) .map { it.value } - .filter { it.contains("appdevforall.org") } + .filter { attribute(it, "android:host") == emitted.host } .toList() - val declared = elements.mapNotNull { attribute(it, "android:pathPrefix") } - - // If the intent-filter stops declaring a prefix, this test must fail rather than vacuously - // pass over an empty list -- there are two elements today, one per verified host. - assertThat(declared).isNotEmpty() - // Taken from a URL the builder actually produces, so this asserts against the emitted shape - // rather than against a second copy of the constant. - val built = DeepLinkRequest.buildUrl("MyApp", "Main.kt", line = 1, column = 1) - val emitted = Uri.parse(built!!) + // Not a vacuous pass: if no element names the emitted host, delivery is broken and that + // is the drift this test exists to catch. + assertThat(elements).isNotEmpty() - for (prefix in declared) { - assertThat(emitted.path).startsWith(prefix) + // All three, because an intent-filter matches on all three. A path-only assertion stayed green + // through a changed CANONICAL_HOST or SCHEME, which breaks delivery just as surely. + for (element in elements) { + assertThat(emitted.path).startsWith(attribute(element, "android:pathPrefix")) + assertThat(attribute(element, "android:scheme")).isEqualTo(emitted.scheme) } - - // Host and scheme too, not only the path. An intent-filter matches on all three, so changing - // CANONICAL_HOST or SCHEME without the manifest breaks delivery exactly as changing the prefix - // does -- and a path-only assertion stays green through both. - val hosts = elements.mapNotNull { attribute(it, "android:host") }.toSet() - val schemes = elements.mapNotNull { attribute(it, "android:scheme") }.toSet() - assertThat(hosts).isNotEmpty() - assertThat(schemes).isNotEmpty() - assertThat(hosts).contains(emitted.host) - assertThat(schemes).contains(emitted.scheme) } private fun attribute( diff --git a/resources/src/main/res/values/strings.xml b/resources/src/main/res/values/strings.xml index 67cb4df9b2..f3a8ed20e7 100644 --- a/resources/src/main/res/values/strings.xml +++ b/resources/src/main/res/values/strings.xml @@ -284,6 +284,7 @@ Install Close others Close this + Close tab Create link All files saved Failed to save files