docs: retro for the WebServer stall work (ADFA-5172/5175/5176) #1691
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,12 +8,23 @@ | |
| - Before pushing a follow-up commit to a community PR, check `gh pr view <n> --json headRepositoryOwner` — the PR head is usually on the contributor's **fork**, so a same-named push to `origin` doesn't touch the PR and just creates a confusing dead branch that has to be deleted. | ||
|
|
||
| ## Android / Kotlin | ||
| - A config data class whose **default** values call framework APIs (e.g. `ServerConfig`'s paths default to `Environment.getExternalStorageDirectory()`) makes itself unconstructable in a JVM unit test — `RuntimeException: Method ... not mocked`, thrown from the constructor before your test body runs. Any new test has to pass *every* such parameter explicitly, which is easy to miss when copying a config from a test that already does. Prefer lazily-resolved paths in new config types. | ||
|
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- docs/process/learnings.md ---'
cat -n docs/process/learnings.md
printf '%s\n' '--- framework API references and mocking guidance ---'
rg -n -C 4 'Environment|getExternalStorageDirectory|mockk|MockK|ServerConfig|default values|JVM unit test' --glob '!build' --glob '!node_modules' .
printf '%s\n' '--- repository files relevant to test configuration ---'
git ls-files | rg '(^|/)(build.gradle(\.kts)?|gradle/libs.versions.toml|.*Test.*|.*test.*)$' | head -200Repository: appdevforall/CodeOnTheGo Length of output: 50381 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- learnings excerpt ---'
sed -n '1,35p' docs/process/learnings.md
printf '%s\n' '--- ServerConfig definitions and call sites ---'
rg -n -C 8 'data class ServerConfig|class ServerConfig|ServerConfig\(' app --glob '*.kt'
printf '%s\n' '--- Environment and MockK guidance ---'
rg -n -C 8 'object Environment|fun init|mockkStatic\(Environment|mockkObject\(Environment|mock.*Environment|Environment\.' docs app/src/test app/src/main --glob '*.md' --glob '*.kt' --glob '*.java' | head -300Repository: appdevforall/CodeOnTheGo Length of output: 50382 Limit the JVM-test claim to tests that do not mock framework APIs. A JVM unit test that does not mock or provide these framework APIs must pass every framework-backed parameter explicitly. 🤖 Prompt for AI Agents |
||
| - `Handler.removeCallbacks(Runnable)` only removes callbacks posted by that *exact* `Handler` instance, not just the same `Looper` — `Handler(Looper.getMainLooper()).removeCallbacks(x)` won't cancel something posted via a *different* `Handler` bound to the same looper. Any post/cancel pair needs to share one `Handler` instance (see `TaskExecutor.mainThreadHandler`, added when replacing blankj's `ThreadUtils.getMainHandler()`). | ||
|
|
||
| ## Serving content to a WebView | ||
| - A WebView can be handed content **in-process** through `WebViewClient.shouldInterceptRequest`, returning a `WebResourceResponse` built from a stream — no socket, no port, no handshake. It intercepts *whatever URL the WebView loads*, so an existing `http://localhost:PORT/...` URL space needs **no rewriting**: strings.xml entries, link builders and even a published plugin-API contract keep working while the transport underneath changes (ADFA-5176 turned 31 TCP connections per documentation page into 0 this way). | ||
| - A WebView does **not** decode an intercepted response, so hand back decompressed bytes and don't bother with `Content-Encoding`. Give `WebResourceResponse` the bare MIME type with the charset as its own argument, and pass `null` for binary types — claiming a charset on an image makes the WebView try to decode it as text. | ||
| - `shouldInterceptRequest` never sees a POST body, and `WebResourceResponse` can't answer a range request with 206. Neither mattered for documentation (the WebView asked for a whole 407 KB PDF), but a range-dependent viewer would need the socket path. | ||
| - Android's WebView cannot render a PDF at all: pointing one at a `application/pdf` URL shows a blank page, identically over HTTP or in-process. | ||
|
Comment on lines
+15
to
+18
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 4 \
'shouldInterceptRequest|WebResourceRequest|WebResourceResponse|Range|Content-Range|206|application/pdf' \
--glob '*.kt' --glob '*.java' --glob '*.md' .Repository: appdevforall/CodeOnTheGo Length of output: 50381 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- target document ---'
cat -n docs/process/learnings.md | sed -n '1,40p'
echo '--- focused repository matches ---'
rg -n -C 5 \
'shouldInterceptRequest|WebResourceRequest|WebResourceResponse|Content-Range|application/pdf|Range' \
--glob '!composite-builds/**' \
--glob '!**/build/**' \
--glob '!**/generated/**' \
--glob '*.kt' --glob '*.java' --glob '*.md' . | head -n 500Repository: appdevforall/CodeOnTheGo Length of output: 50380 🌐 Web query:
💡 Result: Regarding your queries on Android WebView development: 1. POST Body Access: The official Android WebView API does not provide a mechanism to access the POST request body within shouldInterceptRequest [1][2][3]. The WebResourceRequest interface only exposes metadata such as the URL, method, and headers [4][5]. Developers requiring access to POST bodies typically use workarounds, such as injecting JavaScript into the page to intercept AJAX/Fetch calls or utilizing custom network stacks [1][2][3]. 2. WebResourceResponse and 206 Partial Content: The WebResourceResponse class allows you to return custom response data, status codes, and headers [6][7]. While you can technically set a status code of 206 (Partial Content), the WebView's ability to correctly handle and resume content based on that status depends on the internal implementation of the underlying network stack [6]. Using this for manual implementation of range requests requires careful management of the response headers and the provided InputStream [6][7]. 3. PDF Rendering: Android WebView does not natively render PDF files [8]. If a URL points to a PDF, the WebView will typically attempt to download it or fail to display it. To handle PDFs, you must intercept the request (e.g., via shouldInterceptRequest or shouldOverrideUrlLoading), detect the content type, and then decide whether to handle the file using an external PDF viewer intent or an integrated third-party PDF rendering library [8]. Top results: [1][2][6][8] Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- exact API usages outside vendored sources ---'
rg -n \
'shouldInterceptRequest|WebResourceRequest|WebResourceResponse|Content-Range|application/pdf' \
--glob '!composite-builds/**' \
--glob '!**/build/**' \
--glob '!**/generated/**' \
. | grep -vE '(^|/)(IEditor|EditorFeatures|TreeSitter|SearchResult|Range|DocumentEvents|TextProcessor|SimpleMacroProcessor)\.' || true
echo '--- all files containing WebView interception symbols ---'
rg -l \
'shouldInterceptRequest|WebResourceRequest|WebResourceResponse' \
--glob '!composite-builds/**' \
--glob '!**/build/**' \
--glob '!**/generated/**' \
. || true
echo '--- all project references to documentation PDF handling ---'
rg -n -C 8 \
'application/pdf|\.pdf|Content-Range|Range' \
app feature-* core-* --glob '*.kt' --glob '*.java' --glob '*.md' 2>/dev/null | head -n 400 || trueRepository: appdevforall/CodeOnTheGo Length of output: 45478 🌐 Web query:
💡 Result: No, the shouldInterceptRequest method in the Android WebView does not intercept requests for blob: URLs. According to the official Android Developers documentation, the shouldInterceptRequest callback is not invoked for blob: URLs, javascript: URLs, or assets accessed via file:///android_asset/ and file:///android_res/ URLs [1]. The method is intended for other URL schemes such as http(s):, data:, and file: [1]. Citations: 🌐 Web query:
💡 Result: The methods setStatusCodeAndReasonPhrase and setResponseHeaders are part of the Android WebResourceResponse API, which is used to manage HTTP responses for web resources loaded within an Android WebView [1]. The setStatusCodeAndReasonPhrase method is used to configure the HTTP status code and its associated reason phrase for the response [1]. When using this method, the status code must fall within the range of 100 to 299 or 400 to 599 [1]. Specifying a status code in the 3xx range (redirects) is not supported by this API [1]. The setResponseHeaders method is used to define the HTTP headers for the resource response, accepting a map of header names to their respective values [1]. Both of these methods were introduced in Android API level 21 [1]. Citations: Correct the WebView API limitations in the documentation.
🤖 Prompt for AI AgentsSources: Coding guidelines, MCP tools |
||
|
|
||
| ## Android system SQLite | ||
| - Don't assume the JSON1 extension. On a Samsung Android 13 device, `JSON_OBJECT`/`JSON_GROUP_ARRAY` fail at runtime with `no such function: JSON_OBJECT` even though the same query runs fine against the same database file under a desktop sqlite3. Any query using JSON functions needs either a fallback or a documented minimum, and a JSON-based endpoint can be dead on real hardware while passing every desktop test. | ||
|
|
||
| ## Reverse-engineering a library before porting it | ||
| - When writing a same-name drop-in for a third-party utility (to remove the dependency without changing call-site behavior), don't guess its semantics from memory/docs — extract the AAR's `classes.jar` and run `javap -c` against the actual bytecode to confirm exact chaining/wrapping behavior, especially for fluent/reflection-style APIs where a subtle mismatch (e.g., wrapping a field's *declared* type vs. its *runtime* class) changes behavior at existing call sites. | ||
|
|
||
| ## MockK | ||
| - To unit-test code that touches WebView plumbing without Robolectric: `mockkStatic(android.os.Environment::class)` for `getExternalStorageDirectory()`, and a plain `mockk<Uri>` stubbing only `host`/`port`/`path`. Keep the framework *construction* out of the unit under test — a `WebResourceResponse` constructor throws `Stub!` in a JVM test, so split the decision (which content answers this request) from the wrapping, and test the decision. | ||
| - Migrating a mocked call from a Java static method (`mockkStatic(SomeClass::class)`) to a Kotlin top-level extension function requires `mockkStatic("com.package.FileNameKt")` (the compiled JVM facade class name) instead — `mockkStatic(ExtensionReceiver::class)` doesn't work for extension functions. | ||
|
|
||
| ## Measuring a real before/after delta | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the mechanism-removal wording.
“the mechanism can go” is informal, and “stop generating them” is ambiguous for examples such as
bytes per call. State explicitly that the mechanism, or the work it creates, should be removed.Proposed wording
🧰 Tools
🪛 LanguageTool
[style] ~43-~43: The wording of this phrase can be improved.
Context: ...usually to stop generating them, not to make each one better. Check what the platform already offers...
(MAKE_STYLE_BETTER)
🤖 Prompt for AI Agents
Source: Linters/SAST tools