Review fixes for proxy robustness, connection UX and performance - #33
Merged
Conversation
APK artifacts
Built from commit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This review fixes failures in proxy connection isolation, Android trust prompts, cancellation, configuration validation, and build portability.
Findings and fixes
Connection UX review
Performance review
These changes remove repeated work visible in the source; no FPS, battery or launch-time improvement is claimed without device profiling. First-screen configuration loading and the ordered per-edit persistence queue remain profiling targets; write coalescing needs separate ordering/retry validation.
Go crash, memory and error review
native_fuzzFastlane lane for a bounded 20-second, two-worker campaign; its seeds run in the normal native tests.Native race tests pass, and the fuzz campaign executed 244,214 inputs without a crash. Android/JNI builds, JVM checks and lint passed during this review; the final SSH close guard was then rechecked with native race tests. This is bounded validation, not proof that OOM is impossible. Cleanup Close errors remain best-effort rather than masking the primary failure or forcing a panic.
JVM crash and memory review
bundle exec fastlane android android_checkspasses: 96 JVM tests (10 new), lint, debug APK and verified unsigned release APK. Tests cover deep/wide JSON, size limits, escaped string delimiters, lenient-syntax rejection, storage failure/recovery, fatal-error propagation and JNI callback Object contracts. No real heap-exhaustion or device lifecycle stress run was performed.Remaining memory risks: the ordered per-edit configuration queue retains snapshots without a bound, and repeated imports can grow the total stored profile set beyond a single import limit. Addressing these requires an explicit persistence/backpressure policy and compatibility handling for existing configurations; this change does not claim to eliminate every OOM scenario.
JVM/native boundary review
bundle exec fastlane android testpasses: native race tests, 104 JVM tests, lint, debug APK and verified unsigned release APK. Regressions cover borrowed-FD survival, duplicate CLOEXEC, overlapping Start/Stop, global dialer release, protection failures/range checks, stale callbacks and reporter recovery. Actual JNI execution, GC timing and rapid lifecycle transitions on an Android device remain untested locally.Full-diff regression review
Re-reviewed the complete PR against main, including interactions between the earlier review fixes. Corrected these regressions and gaps:
Final local validation:
bundle exec fastlane android testpasses native race tests, 104 JVM tests, lint, debug APK and verified unsigned release APK.bundle exec fastlane android python_checkspasses 31 tests and Black/isort. Added regression coverage for failover trust identity, newly approved pins versus edited endpoints, stalled SSH worker recovery and deterministic closed-socket errors.git diff --checkpasses. Android device lifecycle/visual smoke testing remains outstanding; no device execution is claimed.Compose interaction tests on the JVM
Latest local validation:
bundle exec fastlane android android_checkspasses 140 JVM tests, including all 36 Robolectric Compose tests, lint, debug APK and verified unsigned release APK. This covers UI interactions and persistence; screenshot comparisons and physical-device lifecycle checks remain outside this suite.Test quality review
docs/reviews/test-quality.md. Main remaining gaps are service lifecycle/recovery, activity recreation during transfers, real storage/key failures, UI configuration variants and the native HTTP/2 failure matrix. Intent-recording UI tests do not cover actual VPN lifecycle execution.Latest validation: 149 JVM tests, lint, debug and unsigned release builds pass via
android_checks; native race tests and all 31 Python tests/Black/isort pass through Fastlane. No line-coverage percentage or device execution is claimed.Full CI reruns including skipped checks
The launcher now labels its full rerun option explicitly as including skipped checks. When Change scope executes on GitHub run attempt 2 or later, it enables Android/Compose, native and Python suites without diff/history filtering. Initial PR runs retain normal change filtering; pushes to main always run all suites. Failed-only reruns retain prior scope unless Change scope itself must rerun. This also works with GitHub's Re-run all jobs button; old workflow runs retain their original definition.
Add regression tests for forced scope/output/summary without Git or history lookup, initial docs-only filtering, and full reruns of successful runs with skipped jobs. All 34 Python tests and Black/isort pass via Fastlane; workflow YAML and
git diff --checkpass. Both Fastlane references and project memory document the behavior.Complete CI coverage on main
Every push to main enables Android/Compose UI, Go and Python checks without consulting change history or the diff, including documentation-only updates. Initial PR runs retain selective checks. Pin the README badge and its link to main push runs, and update both Fastlane guides and project memory.
A regression verifies that the initial push attempt enables every suite even with identical base/head commits and without Git/history lookup. All 35 Python tests and Black/isort pass through Fastlane;
git diff --checkpasses. This behavior takes effect in main when the PR is merged.Documentation consistency review
Validation: local Markdown link targets exist; all nine public Fastlane lanes appear in each of the three command references; toolchain versions and diagnostic-provider lists match source; published v0.0.12 asset names match download links;
git diff --checkpasses. Documentation-only changes do not require rerunning application tests. Store text/policy changes are repository updates, not publication to Play Console.Focused privacy policy review
Revise the policy around data categories, purpose, recipients and deletion. Remove unnecessary cipher/TLS details and link to README for the diagnostic endpoint inventory. Add local installed-app/routing processing, source-IP exposure during bootstrap DNS, voluntary report contents, clipboard/share handoff and support email handling. State the developer-confirmed retention practice: support correspondence and attachments are kept until the reported issue is fixed, then deleted.
Record source evidence and Google Play references in
docs/reviews/privacy-policy.md. The policy is now linked at the bottom of Settings. Play Console/Data safety and in-app VPN disclosure still need separate verification. These documentation changes do not claim store compliance or modify consent UI. Source correspondence, local links andgit diff --checkwere checked; no application code changed.Privacy policy access in Settings
Add a full-width text link at the bottom of Settings, after support actions, with English/Russian labels. It opens the public policy on main in a browser and shows the existing localized error if opening fails. Update the privacy review to mark the missing in-app link as addressed.
Review scope and validation
Final source review of head
3b3b406against main4acca42found no additional confirmed regressions in the PR diff. Local validation passed: native race tests, 149 JVM tests (36 Compose interactions), lint, debug/unsigned release builds, 35 Python tests and Black/isort. No source changes were needed. Device lifecycle/network-handover coverage and unbounded configuration queue/profile-set growth remain the previously documented limitations; this review does not establish device-level certification.Reviewed native HTTPS/SSH/Jump connection paths, bootstrap and DoH handling, native startup, Android VPN lifecycle, intent exposure, configuration import/persistence and editor paths, logging, and CI/launcher/release tooling. This is a source review with deterministic checks, not certification of all device/network scenarios.
bundle exec fastlane android test. The latestbundle exec fastlane android android_checkspasses 149 JVM tests, lint, debug APK and verified unsigned release APK.bundle exec fastlane android python_checks: 31 tests and pinned Black/isort checks pass.git diff --checkpass. Regression tests exercise session isolation, HTTP cancellation, IP literals, validation, log I/O failure and JDK selection. A manifest contract keeps the trust-review activity private.The SSH error distinction follows the OpenChannelError contract.