Consolidated debt, protocol registry, per-profile stats, and DX work#117
Merged
Conversation
Repoint the libneko clone and the sing-vmess module replace at the project's own org mirrors (identical code at the pinned commit/tag, verified byte-for-byte). Adds the same set-url/fetch guard the sibling source already uses so a cached checkout repoints cleanly. go.sum regenerated (not hand-edited).
Move the capture-free chain resolvers (resolveChain/resolveChainInternal) and the DNS domain-strategy helpers (genDomainStrategy/autoDnsDomainStrategy) to top-level private functions with explicit params. Thin nested wrappers keep the existing call sites unchanged, so this is a behavior-preserving move. The heavily entangled buildChain (captures 13+ mutable accumulators) is intentionally left nested — parameterizing it would harm clarity without benefit (Plan 028 STOP).
…hrink-only Promote TrustAllX509TrustManager from ignore to error - app code has no all-trusting X509TrustManager today, so fail-fast if one is ever introduced (a real MITM vector for a privacy-focused app). Raise RestrictedApi to warning so cross-version internal misuse is surfaced rather than hidden; leave the cosmetic/i18n ignores as intentional noise suppression. Add scripts/check-lint-baseline.sh (count-only, may-only-shrink vs the merge base) wired into the Depot and CI guard jobs, and document the baseline policy at the top of lint.xml. Guard checkouts fetch full history for the merge-base.
Move the Fragment/Activity UI-extension helpers (snackbar, startFilesForResult, needReload, needRestart, triggerFullRestart) out of ktx into a new ui/UiExt.kt so ktx no longer imports MainActivity/ThemedActivity. The layout managers in ktx/Layouts.kt now drive the FAB through a new leaf-level FabContainer interface (implemented by MainActivity) instead of casting to MainActivity directly. All call sites are in the ui package and resolve same-package; stale ktx imports and now-dead imports removed. Pure relocation - no behavior change.
… its own file Move the nested ConfigurationFragment.GroupFragment (with its ConfigurationAdapter/ ConfigurationHolder) into a top-level ConfigurationGroupFragment.kt. It is recreated by the FragmentStateAdapter via createFragment(), so the class-name change does not affect saved-state restoration. It never lexically captured the outer fragment (it reaches the parent via parentFragment as ConfigurationFragment), so the move is behavior-preserving. exportConfig visibility relaxed private -> internal for the launcher hand-off; SelectCallback reference qualified. Now-unused imports pruned from ConfigurationFragment.kt. Pure move - no behavior change.
…bean types Pure-JVM golden net (runs on the Depot unit-tests workflow) covering all 22 persistable ProxyEntity bean types: asserts serialize -> deserialize -> serialize byte-stability and full ProxyEntity type dispatch (putBean sets the right TYPE_* id and typed field; requireBean()/putByteArray round-trip to identical bytes). This is the safety net gating the protocol-registry refactor - if dispatch ever routes a type to the wrong (de)serializer the byte-arrays diverge and the test fails.
…tor registry Introduce ProtocolRegistry: each protocol is declared once as a ProtocolDescriptor (type id, Kryo deserializer, bean class, typed-field get/set, displayType, needExternal). Route putByteArray (deserialize dispatch), requireBean, displayType, needExternal and putBean (clear + assign + type id) through it, replacing the parallel when(type)/when(bean) ladders. Wire format and TYPE_* ids are unchanged: each descriptor references the same KryoConverters.*Deserialize the old ladder used and read/writes the same typed field, so serialize(requireBean()) and putByteArray(type, bytes) produce identical bytes. Bean dispatch keys on exact class (no registered bean is subclassed, so equivalent to the old is-checks). Unknown-id / unregistered-bean paths preserve the old no-op / error behavior. Guarded by ProtocolRegistryDispatchTest across all 22 bean types. singMux, toStdLink and settingIntent stay in ProxyEntity (per-encoder logic / would couple database->ui).
… v12) Add additive lifetimeRx/lifetimeTx columns to proxy_entities (default 0, schema v12 via a pure-column AutoMigration 11->12; never destructive). TrafficLooper accumulates the per-session delta into them on persist() and on selector switch, tracking a per-session flushed marker so the re-entrant persist()/switch paths never double-count; tx/rx keep their existing live/session meaning. Surface the totals read-only as a long-press tooltip on the profile row's traffic text (shown only when history exists). Completes the deferred half of the fa02117 capture spike. Note: 12.json carries the additive columns; its Room identityHash is regenerated by KSP on the CI build (cannot be computed without a build; no 11->12 migration test and runtime uses the compiled hash, so this is safe).
Stage 1: debug StrictMode ThreadPolicy (detect main-thread disk reads/writes, penaltyLog only) so remaining main-thread DAO access is observable on device. Stage 2: move the proven main-thread DB reads off the UI thread - pingTest/urlTest now read DataStore.currentGroup() inside their runOnDefaultDispatcher block instead of before it (audited the DataStore group-helper callers and every ui-package DAO site; the rest already run in suspend/dispatched scopes). Stage 3: gate allowMainThreadQueries() behind BuildConfig.ALLOW_MAIN_THREAD_DB - debug ships with it OFF (so a missed site surfaces as a catchable IllegalStateException during the device StrictMode pass), release keeps it ON until debug has run clean, after which the flag and the call are deleted. The flag is the revert switch if the device pass surfaces a site the static audit missed.
- ConfigurationFragment pingTest/urlTest: after moving DataStore.currentGroup() off the main thread, the group name is still needed by the later test.minimize callback; cache it in a var assigned inside the worker (was a compile-scope break). - TrafficLooper.flushLifetimeDelta: make it suspend and advance the flushed markers only AFTER ProfileManager.addLifetimeTraffic() completes, so a failed/incomplete DB write cannot silently drop the delta; both persist() and applySelect() await it. - check-lint-baseline.sh: drop the redundant '|| echo 0' that could emit a second line; tolerate grep -c's no-match exit under set -e via '|| true'. Declined (pre-existing, unchanged by the pure moves; not this PR's scope): moveLinear upward-range, onUpdated RecyclerView main-thread access, showShareMenu holder.entity, onViewStateRestored->onViewCreated re-run, UiExt needRestart requireContext lifetime.
📝 WalkthroughWalkthroughThis PR adds lifetime traffic tracking (schema v12, DAO/ProfileManager methods, TrafficLooper delta flushing), introduces a ProtocolRegistry replacing hardcoded protocol dispatch in ProxyEntity, extracts ConfigurationGroupFragment/FabContainer/UiExt from monolithic UI code, adds StrictMode disk I/O detection, and updates CI lint-baseline enforcement plus dependency forks. ChangesCore feature and refactor changes
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
…y scope ProxyEntity resolved isTLS/canUseSingBox via package wildcards and chainName via its own companion; the extracted registry needs explicit imports. Add fmt.v2ray.isTLS and fmt.hysteria.canUseSingBox, and reference the chain label as ProxyEntity.chainName (companion). Fixes the compileOssDebugKotlin failure.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
app/src/main/java/io/nekohasekai/sagernet/fmt/ConfigBuilder.kt (1)
184-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelegating shims add indirection but preserve call-site syntax correctly.
Each shim (
resolveChainInternal,resolveChain,genDomainStrategy,autoDnsDomainStrategy) resolves unambiguously to its top-level counterpart due to differing arity/signature, so no accidental self-recursion. Behavior-preserving as intended.Once call sites (e.g.
entity.resolveChain()at line 394,genDomainStrategy(...)at line 380,autoDnsDomainStrategy(...)at lines 1058/1071/1184) are updated to call the top-level functions directly, these four local wrappers could be removed to avoid the extra indirection layer.Also applies to: 196-196, 248-248, 289-289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/nekohasekai/sagernet/fmt/ConfigBuilder.kt` at line 184, The local extension shims are now redundant because the matching top-level functions are being called directly at the listed call sites. Remove the wrapper methods like ProxyEntity.resolveChainInternal, ProxyEntity.resolveChain, genDomainStrategy, and autoDnsDomainStrategy from ConfigBuilder once all usages point to the top-level counterparts, and verify any remaining references resolve to the intended top-level functions without the extra indirection.app/src/main/java/io/nekohasekai/sagernet/SagerNet.kt (1)
114-134: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStrictMode policy installed after several main-thread startup calls.
The
ThreadPolicyis set near the end ofonCreate(), afterJavaUtil.handleWebviewDir(this),Theme.apply(this),Theme.applyNightTheme(), andAppLocale.apply()have already executed on the main thread. Any disk I/O performed by those calls won't be caught bydetectDiskReads()/detectDiskWrites(), undermining the stated goal of surfacing main-thread disk access during startup.Consider moving the
StrictMode.setThreadPolicy(...)call to the very start of theBuildConfig.DEBUGblock (or even before it, alongsideThread.setDefaultUncaughtExceptionHandler), so it's active before any of the earlier startup work runs.♻️ Proposed reordering
+ if (BuildConfig.DEBUG) { + System.setProperty(DEBUG_PROPERTY_NAME, DEBUG_PROPERTY_VALUE_ON) + StrictMode.setThreadPolicy( + StrictMode.ThreadPolicy.Builder() + .detectDiskReads() + .detectDiskWrites() + .penaltyLog() + .build(), + ) + } + if (isMainProcess || isBgProcess) { externalAssets.mkdirs() ... } ... if (BuildConfig.DEBUG) { - System.setProperty(DEBUG_PROPERTY_NAME, DEBUG_PROPERTY_VALUE_ON) - StrictMode.setThreadPolicy( - StrictMode.ThreadPolicy.Builder() - .detectDiskReads() - .detectDiskWrites() - .penaltyLog() - .build(), - ) StrictMode.setVmPolicy( StrictMode.VmPolicy.Builder() ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/nekohasekai/sagernet/SagerNet.kt` around lines 114 - 134, The StrictMode setup in SagerNet.onCreate is being applied too late, after startup work like JavaUtil.handleWebviewDir, Theme.apply, Theme.applyNightTheme, and AppLocale.apply has already run. Move the StrictMode.setThreadPolicy call to the beginning of the BuildConfig.DEBUG initialization block (or before it with the other early startup setup) so disk reads/writes during those earlier calls are actually detected; keep the existing ThreadPolicy.Builder and VmPolicy.Builder configuration unchanged.app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt (1)
132-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSatisfy detekt without an empty override body.
onSwipedis intentionally a no-op; express it as= Unit.Proposed fix
- override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) { - } + override fun onSwiped(viewHolder: RecyclerView.ViewHolder, direction: Int) = Unit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt` around lines 132 - 133, The override of onSwiped in ConfigurationGroupFragment is an intentional no-op, but detekt flags the empty body; change this method to an expression-bodied override using Unit so the intent is explicit while keeping the same behavior.Source: Linters/SAST tools
app/src/main/java/io/nekohasekai/sagernet/ui/UiExt.kt (1)
98-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSatisfy detekt without an empty override body.
stateChangedis required by the callback interface, but this can be expressed as= Unit.Proposed fix
- override fun stateChanged(state: BaseService.State, profileName: String?, msg: String?) { - } + override fun stateChanged(state: BaseService.State, profileName: String?, msg: String?) = Unit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/io/nekohasekai/sagernet/ui/UiExt.kt` around lines 98 - 99, The stateChanged override in UiExt.kt has an empty body, which detekt flags; update the BaseService.State callback implementation to use an expression body with = Unit instead of an empty block. Keep the same stateChanged signature and preserve its role as the required callback while making the implementation explicitly no-op.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficLooper.kt`:
- Around line 112-118: The selector-switch traffic flush is launched
asynchronously in `applySelect()`, so `flushLifetimeDelta()` can race with
shutdown persistence and double-count the same delta. Make `applySelect()`
suspend and await the `runOnDefaultDispatcher` work in
`TrafficLooper.applySelect`, ensuring `ProfileManager.updateTraffic(...)` and
`flushLifetimeDelta(...)` complete before returning and the marker advances
deterministically.
In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.kt`:
- Around line 817-824: Avoid sharing the mutable groupName across dispatchers in
ConfigurationFragment’s minimize-to-notification flow, since it is written
inside runOnDefaultDispatcher and later read by the callback. Update the logic
so the name is assigned on the main thread before the callback uses it, or store
it in a thread-safe holder such as AtomicReference, and keep a fallback title
until the value is available. Apply the same fix pattern to the related test
callbacks that read the same captured name.
In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt`:
- Around line 528-589: Keep all adapter and RecyclerView interactions on the
main thread. In ConfigurationGroupFragment’s onUpdated(profile, noTraffic), move
the configurationIdList lookup and related state reads/writes into the
configurationListView.post/onMainDispatcher block instead of doing them before
the hop; likewise in onUpdated(TrafficData) and onUpdated(List<TrafficData>),
defer layoutManager.findViewByPosition and
configurationListView.getChildViewHolder until inside onMainDispatcher so they
run together on main. Also audit reloadProfiles() so any configurationList
mutation happens on the same main-thread dispatch as the view update, using the
existing configurationListView, layoutManager, and undoManager flow to keep
access consistent.
- Around line 285-287: Listener cleanup in ConfigurationGroupFragment is scoped
too late, so the old ConfigurationAdapter can still receive callbacks after the
view is detached. Move the listener removal for ProfileManager and GroupManager,
along with undoManager.flush(), from onDestroy to onDestroyView, and keep the
addListener setup in onViewCreated aligned with the adapter lifecycle.
---
Nitpick comments:
In `@app/src/main/java/io/nekohasekai/sagernet/fmt/ConfigBuilder.kt`:
- Line 184: The local extension shims are now redundant because the matching
top-level functions are being called directly at the listed call sites. Remove
the wrapper methods like ProxyEntity.resolveChainInternal,
ProxyEntity.resolveChain, genDomainStrategy, and autoDnsDomainStrategy from
ConfigBuilder once all usages point to the top-level counterparts, and verify
any remaining references resolve to the intended top-level functions without the
extra indirection.
In `@app/src/main/java/io/nekohasekai/sagernet/SagerNet.kt`:
- Around line 114-134: The StrictMode setup in SagerNet.onCreate is being
applied too late, after startup work like JavaUtil.handleWebviewDir,
Theme.apply, Theme.applyNightTheme, and AppLocale.apply has already run. Move
the StrictMode.setThreadPolicy call to the beginning of the BuildConfig.DEBUG
initialization block (or before it with the other early startup setup) so disk
reads/writes during those earlier calls are actually detected; keep the existing
ThreadPolicy.Builder and VmPolicy.Builder configuration unchanged.
In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt`:
- Around line 132-133: The override of onSwiped in ConfigurationGroupFragment is
an intentional no-op, but detekt flags the empty body; change this method to an
expression-bodied override using Unit so the intent is explicit while keeping
the same behavior.
In `@app/src/main/java/io/nekohasekai/sagernet/ui/UiExt.kt`:
- Around line 98-99: The stateChanged override in UiExt.kt has an empty body,
which detekt flags; update the BaseService.State callback implementation to use
an expression body with = Unit instead of an empty block. Keep the same
stateChanged signature and preserve its role as the required callback while
making the implementation explicitly no-op.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7342e035-1493-4c10-915a-34fcf4df300d
⛔ Files ignored due to path filters (1)
libcore/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.depot/workflows/guard.yml.github/workflows/ci.ymlapp/schemas/io.nekohasekai.sagernet.database.SagerDatabase/12.jsonapp/src/main/java/io/nekohasekai/sagernet/SagerNet.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficLooper.ktapp/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficUpdater.ktapp/src/main/java/io/nekohasekai/sagernet/database/ProfileManager.ktapp/src/main/java/io/nekohasekai/sagernet/database/ProtocolRegistry.ktapp/src/main/java/io/nekohasekai/sagernet/database/ProxyEntity.ktapp/src/main/java/io/nekohasekai/sagernet/database/SagerDatabase.ktapp/src/main/java/io/nekohasekai/sagernet/fmt/ConfigBuilder.ktapp/src/main/java/io/nekohasekai/sagernet/ktx/FabContainer.ktapp/src/main/java/io/nekohasekai/sagernet/ktx/Layouts.ktapp/src/main/java/io/nekohasekai/sagernet/ktx/Utils.ktapp/src/main/java/io/nekohasekai/sagernet/ui/BackupFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/MainActivity.ktapp/src/main/java/io/nekohasekai/sagernet/ui/UiExt.ktapp/src/main/res/values/strings.xmlapp/src/test/java/io/nekohasekai/sagernet/fmt/ProtocolRegistryDispatchTest.ktbuildScript/lib/core/get_source.shbuildSrc/src/main/kotlin/Helpers.ktlibcore/go.modlint.xmlscripts/check-lint-baseline.sh
💤 Files with no reviewable changes (2)
- app/src/main/java/io/nekohasekai/sagernet/ui/BackupFragment.kt
- app/src/main/java/io/nekohasekai/sagernet/ktx/Utils.kt
Comment on lines
+112
to
+118
| val switchedFrom = this | ||
| data.proxy?.config?.trafficMap?.get(tag)?.firstOrNull()?.let { | ||
| it.rx = rx | ||
| it.tx = tx | ||
| runOnDefaultDispatcher { | ||
| ProfileManager.updateTraffic(it.id, it.rx, it.tx) | ||
| flushLifetimeDelta(it.id, switchedFrom) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether runOnDefaultDispatcher awaits the block or launches it asynchronously.
rg -n -C4 'fun\s+runOnDefaultDispatcher|runOnDefaultDispatcher\s*=' app/src/main/java buildSrc
# Inspect call sites for assumptions around fire-and-forget vs awaited behavior.
rg -n -C3 'runOnDefaultDispatcher\s*\{' app/src/main/javaRepository: hawkff/NekoBoxForAndroid
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section of TrafficLooper.kt.
nl -ba app/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficLooper.kt | sed -n '1,260p'
# Find flush/persist state mutations in the file.
rg -n -C3 'flushLifetimeDelta|persist\(|lifetimeFlushed|applySelect|switchedFrom' app/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficLooper.ktRepository: hawkff/NekoBoxForAndroid
Length of output: 202
🏁 Script executed:
set -euo pipefail
# Show the relevant section with line numbers using portable tools.
sed -n '1,260p' app/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficLooper.kt | cat -n
# Find every occurrence of the state and helper names in this file.
rg -n -C3 'flushLifetimeDelta|persist\(|lifetimeFlushed|applySelect|switchedFrom' app/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficLooper.ktRepository: hawkff/NekoBoxForAndroid
Length of output: 15640
Await the selector-switch flush before returning
runOnDefaultDispatcher launches this work asynchronously, so flushLifetimeDelta() can still overlap with persist() on shutdown and double-count the same delta. Make applySelect() suspend and wait for the DB write and marker advance here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficLooper.kt` around
lines 112 - 118, The selector-switch traffic flush is launched asynchronously in
`applySelect()`, so `flushLifetimeDelta()` can race with shutdown persistence
and double-count the same delta. Make `applySelect()` suspend and await the
`runOnDefaultDispatcher` work in `TrafficLooper.applySelect`, ensuring
`ProfileManager.updateTraffic(...)` and `flushLifetimeDelta(...)` complete
before returning and the marker advances deterministically.
Comment on lines
+817
to
+824
| // Group display name for the minimize->notification callback. The DAO read must stay off | ||
| // the main thread, so it is fetched inside the worker below and cached here for the | ||
| // callback (which fires later, on user tap). | ||
| var groupName = "" | ||
|
|
||
| val mainJob = runOnDefaultDispatcher { | ||
| val group = DataStore.currentGroup() | ||
| groupName = group.displayName() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant fragment and nearby usages.
file="app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.kt"
echo "== line count =="
wc -l "$file"
echo
echo "== AST outline =="
ast-grep outline "$file" --view expanded | sed -n '1,220p'
echo
echo "== Relevant line ranges around 817-824, 956-960, 972-977, 1041-1045 =="
sed -n '790,860p' "$file"
echo
sed -n '930,990p' "$file"
echo
sed -n '1020,1060p' "$file"
echo
echo "== Searches for dispatch helpers and minimize callback =="
rg -n "runOnDefaultDispatcher|onMainDispatcher|minimize|notification|groupName|displayName" "$file"Repository: hawkff/NekoBoxForAndroid
Length of output: 11552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect definitions/usages of the helper functions if they are in the repo.
rg -n "fun runOnDefaultDispatcher|val runOnDefaultDispatcher|suspend fun runOnDefaultDispatcher|fun onMainDispatcher|val onMainDispatcher|suspend fun onMainDispatcher" app/src/main/java
# Find where groupName is captured/used outside the listed snippet locations.
rg -n "groupName" app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktRepository: hawkff/NekoBoxForAndroid
Length of output: 605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show only the surrounding blocks for the listed locations so we can compare
# how `groupName` is produced and consumed.
file="app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.kt"
for range in "817,824" "956,960" "972,977" "1041,1045"; do
echo "== $range =="
sed -n "${range}p" "$file"
echo
doneRepository: hawkff/NekoBoxForAndroid
Length of output: 1347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the implementations of the coroutine helpers to determine thread-hopping semantics.
rg -n "runOnDefaultDispatcher|onMainDispatcher" app/src/main/java -g '!**/build/**' -g '!**/out/**'Repository: hawkff/NekoBoxForAndroid
Length of output: 28119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrowly inspect the helper definitions and the exact callback wiring.
# This is read-only and should show whether the callback is always reached on the main thread.
rg -n -C 3 "fun runOnDefaultDispatcher|fun onMainDispatcher|groupName =|groupName\b" app/src/main/java/io/nekohasekai/sagernet | sed -n '1,220p'Repository: hawkff/NekoBoxForAndroid
Length of output: 12180
Avoid sharing groupName across dispatchers here
groupName is written on Dispatchers.Default and later read from the minimize callback, so the notification can still render [] ... or a stale name. Set it on the main thread or use an AtomicReference, and keep a fallback title until the name is ready. Same pattern in the other test callbacks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.kt` around
lines 817 - 824, Avoid sharing the mutable groupName across dispatchers in
ConfigurationFragment’s minimize-to-notification flow, since it is written
inside runOnDefaultDispatcher and later read by the callback. Update the logic
so the name is assigned on the main thread before the callback uses it, or store
it in a thread-safe holder such as AtomicReference, and keep a fallback title
until the value is available. Apply the same fix pattern to the related test
callbacks that read the same captured name.
Comment on lines
+285
to
+287
| adapter = ConfigurationAdapter() | ||
| ProfileManager.addListener(adapter!!) | ||
| GroupManager.addListener(adapter!!) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the fragment lifecycle methods and related listener usage.
file="app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt"
echo "=== Outline ==="
ast-grep outline "$file" --view expanded || true
echo
echo "=== Relevant lines around lifecycle methods ==="
nl -ba "$file" | sed -n '240,360p'
echo
echo "=== Search for listener registration/removal and undoManager usage in this file ==="
rg -n "addListener|removeListener|undoManager|onDestroyView|onDestroy" "$file"
echo
echo "=== Search for adapter/view binding references in this file ==="
rg -n "binding|adapter|RecyclerView|viewLifecycleOwner|requireView|onViewCreated" "$file"Repository: hawkff/NekoBoxForAndroid
Length of output: 1590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt"
echo "=== onViewCreated / onDestroy / nearby code ==="
python3 - <<'PY'
from pathlib import Path
path = Path("app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt")
lines = path.read_text().splitlines()
for start, end in [(279, 336), (1, 120), (150, 240)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4}: {lines[i-1]}")
PY
echo
echo "=== listener and binding references in file ==="
rg -n "addListener|removeListener|undoManager|binding\\.|configurationListView|onDestroyView|onDestroy" "$file"
echo
echo "=== any onDestroyView override in this fragment or nearby classes ==="
rg -n "override fun onDestroyView|override fun onDestroy\\(" app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt app/src/main/java/io/nekohasekai/sagernet/ui -g '*.kt'Repository: hawkff/NekoBoxForAndroid
Length of output: 17628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt"
python3 - <<'PY'
from pathlib import Path
path = Path("app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt")
lines = path.read_text().splitlines()
for start, end in [(337, 520), (520, 760), (760, 940)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4}: {lines[i-1]}")
PY
echo
echo "=== Adapter and manager-specific calls ==="
rg -n "class ConfigurationAdapter|ProfileManager\\.|GroupManager\\.|notify|reloadProfiles|commitMove|onUpdate|onRemove|onAdd|listener" "$file"Repository: hawkff/NekoBoxForAndroid
Length of output: 29933
Scope listener cleanup to onDestroyView. Because the adapter is recreated in onViewCreated, leaving the old listeners registered until onDestroy lets callbacks hit a detached view after onDestroyView. Move listener removal and undoManager.flush() there.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt`
around lines 285 - 287, Listener cleanup in ConfigurationGroupFragment is scoped
too late, so the old ConfigurationAdapter can still receive callbacks after the
view is detached. Move the listener removal for ProfileManager and GroupManager,
along with undoManager.flush(), from onDestroy to onDestroyView, and keep the
addListener setup in onViewCreated aligned with the adapter lifecycle.
Comment on lines
+528
to
+589
| override suspend fun onUpdated(profile: ProxyEntity, noTraffic: Boolean) { | ||
| if (profile.groupId != proxyGroup.id) return | ||
| val index = configurationIdList.indexOf(profile.id) | ||
| if (index < 0) return | ||
| configurationListView.post { | ||
| if (::undoManager.isInitialized) { | ||
| undoManager.flush() | ||
| } | ||
| val oldProfile = configurationList[profile.id] | ||
| configurationList[profile.id] = profile | ||
| notifyItemChanged(index) | ||
| if (noTraffic && oldProfile != null) { | ||
| runOnDefaultDispatcher { | ||
| onUpdated( | ||
| TrafficData( | ||
| id = profile.id, | ||
| rx = oldProfile.rx, | ||
| tx = oldProfile.tx, | ||
| ), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override suspend fun onUpdated(data: TrafficData) { | ||
| try { | ||
| val index = configurationIdList.indexOf(data.id) | ||
| if (index != -1) { | ||
| val holder = layoutManager.findViewByPosition(index) | ||
| ?.let { configurationListView.getChildViewHolder(it) } as ConfigurationHolder? | ||
| if (holder != null) { | ||
| onMainDispatcher { | ||
| holder.bind(holder.entity, data) | ||
| } | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Logs.w(e) | ||
| } | ||
| } | ||
|
|
||
| override suspend fun onUpdated(data: List<TrafficData>) { | ||
| try { | ||
| val positions = HashMap<Long, Int>(configurationIdList.size) | ||
| configurationIdList.forEachIndexed { index, id -> positions[id] = index } | ||
| val updates = ArrayList<Pair<ConfigurationHolder, TrafficData>>() | ||
| for (item in data) { | ||
| val index = positions[item.id] ?: continue | ||
| val holder = layoutManager.findViewByPosition(index) | ||
| ?.let { configurationListView.getChildViewHolder(it) } as ConfigurationHolder? | ||
| if (holder != null) updates.add(holder to item) | ||
| } | ||
| if (updates.isNotEmpty()) { | ||
| onMainDispatcher { | ||
| for ((holder, item) in updates) holder.bind(holder.entity, item) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Logs.w(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt'
# Show structure around the relevant region
ast-grep outline "$FILE" --view expanded || true
# Read the target lines with context
sed -n '470,700p' "$FILE"
# Find related dispatcher / adapter helpers in the same file
rg -n "onMainDispatcher|runOnDefaultDispatcher|configurationIdList|configurationList|layoutManager\.findViewByPosition|getChildViewHolder|post \{" "$FILE"
# Inspect surrounding definitions for the adapter/list state
rg -n "class .*Holder|class .*Adapter|configurationListView|layoutManager" "$FILE"Repository: hawkff/NekoBoxForAndroid
Length of output: 17172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the callback interface and dispatcher helpers
rg -n "interface .*onUpdated|suspend fun onUpdated|fun onMainDispatcher|fun runOnDefaultDispatcher" app/src/main/java
# Read the dispatcher helper implementations if present
for f in $(rg -l "fun onMainDispatcher|fun runOnDefaultDispatcher" app/src/main/java); do
echo "===== $f ====="
sed -n '1,220p' "$f"
done
# Read the interface / base class that defines the update callbacks
for f in $(rg -l "suspend fun onUpdated\\(" app/src/main/java); do
echo "===== $f ====="
sed -n '1,260p' "$f"
doneRepository: hawkff/NekoBoxForAndroid
Length of output: 41974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find callers of the update APIs to infer dispatcher/context
rg -n "ProfileManager\.(postUpdate|updateProfile|updateTraffic|createProfile|deleteProfile|updateProfileQuietly)|iterator \{" app/src/main/java
# Read the surrounding call sites that may trigger these callbacks
for f in $(rg -l "ProfileManager\.(postUpdate|updateProfile|updateTraffic|deleteProfile|updateProfileQuietly)|iterator \{" app/src/main/java); do
echo "===== $f ====="
sed -n '1,260p' "$f"
doneRepository: hawkff/NekoBoxForAndroid
Length of output: 50380
Keep adapter and RecyclerView access on the main thread. onUpdated(profile, noTraffic) reads configurationIdList before the post {} hop, onUpdated(TrafficData) / onUpdated(List<TrafficData>) call layoutManager.findViewByPosition(...) and getChildViewHolder(...) before onMainDispatcher {}, and reloadProfiles() mutates configurationList before posting back to the view. Move those lookups and mutations onto the main thread together.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt`
around lines 528 - 589, Keep all adapter and RecyclerView interactions on the
main thread. In ConfigurationGroupFragment’s onUpdated(profile, noTraffic), move
the configurationIdList lookup and related state reads/writes into the
configurationListView.post/onMainDispatcher block instead of doing them before
the hop; likewise in onUpdated(TrafficData) and onUpdated(List<TrafficData>),
defer layoutManager.findViewByPosition and
configurationListView.getChildViewHolder until inside onMainDispatcher so they
run together on main. Also audit reloadProfiles() so any configurationList
mutation happens on the same main-thread dispatch as the view update, using the
existing configurationListView, layoutManager, and undoManager flow to keep
access consistent.
spotlessApply output only: expand single-line functions, wrap the long @query, fix test import ordering, drop an unused Fragment import in Utils.kt. No logic change.
…sage) - TrafficLooper: guard the lifetime read-modify-write with a Mutex so the loop coroutine (persist) and the GlobalScope selector-switch coroutine cannot interleave the check-and-advance of lifetimeFlushed* and double-count an interval (P1). - ProxyEntity.putBean: report the offending bean class in the error instead of the stale entity type id, which was misleading for an unregistered bean (P2).
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
1 participant
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.
Consolidated debt / refactor / stats / DX work into a single PR to minimize CI review
runs. All changes are behavior-preserving unless noted; verification is on Namespace CI
(unit tests, lint, guards, APK) plus a device smoke pass before merge.
What's in here
code at the pinned commit/tag; go.sum regenerated).
TrustAllX509TrustManagertoerror(app code has none today),surface
RestrictedApiatwarning, and add a shrink-only lint-baseline guard wiredinto the guard jobs.
ktx -> uidependency cycle by moving the UI-coupledFragment/Activity helpers to a new
ui/UiExt.ktand driving the FAB from ktx through aleaf
FabContainerinterface.ktxis now a leaf w.r.t.ui.ConfigurationFragment— the paged group fragmentmoves to its own
ConfigurationGroupFragment.kt(recreated by the FragmentStateAdapter,so the class move is saved-state-safe).
ProxyEntitytype dispatch (deserialize, requireBean,displayType, needExternal, putBean) through a single
ProtocolRegistrydescriptor table.The on-disk format and stored type ids are unchanged; a new all-bean-type test asserts
serialize/deserialize/dispatch parity to guard that.
(never destructive); accumulated with a per-session flushed marker so re-entrant flushes
don't double-count; surfaced read-only as a long-press tooltip on the traffic row.
allowMainThreadQueries()— debug StrictMode threadpolicy, move the proven main-thread reads off the UI thread, and gate the allowance behind
a build flag (debug off, release on until device-clean, then deleted).
Testing
spotless, repo guards, OSS APK build.
profiles still load after the registry change; lifetime stats accumulate and survive a
restart; StrictMode pass for the main-thread-DB work.
Notes
schemas/.../12.jsoncarries the additive columns; its Room identity hash is regeneratedby the CI build.
RawUpdaterbehavior is unchanged on purpose: subscription refresh resets session andlifetime counters alike, matching existing session-counter behavior.
Greptile Summary
This is a broad consolidation PR that bundles several independent refactors, a new stats feature, and DX improvements. All changes are described as behavior-preserving except the new per-profile lifetime traffic feature.
ProtocolRegistry: replaces two dozen parallelwhen(type)/when(bean)ladders inProxyEntitywith a single descriptor table; wire format and persisted type ids are unchanged, and a new round-trip dispatch test guards the invariant.feat(stats): addslifetimeRx/lifetimeTxcolumns via an additive schema v12 auto-migration (DEFAULT 0); alifetimeMutexinTrafficLooperserializes the read-modify-write between the loop coroutine and theapplySelectGlobalScope coroutine so the previously flagged double-counting window is closed; the totals are surfaced as a long-press tooltip inConfigurationGroupFragment.refactor(ktx/ui): breaks thektx → uidependency cycle via a newFabContainerinterface; splits the 2.1 k-lineConfigurationFragmentinto a parent +ConfigurationGroupFragment; moves UI-coupled helpers toUiExt.kt; gatesallowMainThreadQueries()behindBuildConfig.ALLOW_MAIN_THREAD_DBfor a staged StrictMode migration.Confidence Score: 5/5
Safe to merge; the double-counting window in lifetime traffic accumulation is correctly closed by the new lifetimeMutex, the schema migration is purely additive, and the ProtocolRegistry refactor is guarded by a round-trip dispatch test.
The double-counting concern flagged in earlier reviews has been addressed: persist() and the applySelect GlobalScope coroutine both gate their lifetime read-modify-write behind lifetimeMutex, so whichever runs second sees the advanced flushedRx marker and no-ops. The schema v12 auto-migration adds DEFAULT 0 columns and is not destructive. The ProtocolRegistry consolidation is behaviorally equivalent to the removed when-ladders, verified by ProtocolRegistryDispatchTest. The one remaining observation (missing @volatile on item.rx/rxBase) is a JVM spec technicality that has no practical consequence on Android hardware.
TrafficLooper.kt — the lifetimeMutex logic is new and correctness-critical; the sequence diagram illustrates the two-coroutine ordering, but a second pair of eyes on the stop()/persist() interplay with in-flight GlobalScope coroutines is worthwhile before merge.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant LoopCo as LoopCoroutine participant GSCo as GlobalScopeCoroutine participant Mutex as lifetimeMutex participant DB as ProfileManager LoopCo->>GSCo: runOnDefaultDispatcher flushLifetimeDelta LoopCo->>LoopCo: continues updateAll/delay GSCo->>Mutex: withLock acquire GSCo->>GSCo: "delta = rx - rxBase - flushedRx" GSCo->>DB: addLifetimeTraffic(id, delta) GSCo->>GSCo: "flushedRx += delta" GSCo->>Mutex: release LoopCo->>LoopCo: stop called cancelAndJoin LoopCo->>LoopCo: persist iterates idMap alt GSCo already finished LoopCo->>Mutex: withLock acquire LoopCo->>LoopCo: "delta=0 flushedRx already advanced" LoopCo->>Mutex: release else GSCo still in-flight LoopCo->>Mutex: withLock blocks GSCo->>Mutex: release LoopCo->>Mutex: acquire LoopCo->>LoopCo: "delta=0 flushedRx advanced by GSCo" LoopCo->>Mutex: release end%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant LoopCo as LoopCoroutine participant GSCo as GlobalScopeCoroutine participant Mutex as lifetimeMutex participant DB as ProfileManager LoopCo->>GSCo: runOnDefaultDispatcher flushLifetimeDelta LoopCo->>LoopCo: continues updateAll/delay GSCo->>Mutex: withLock acquire GSCo->>GSCo: "delta = rx - rxBase - flushedRx" GSCo->>DB: addLifetimeTraffic(id, delta) GSCo->>GSCo: "flushedRx += delta" GSCo->>Mutex: release LoopCo->>LoopCo: stop called cancelAndJoin LoopCo->>LoopCo: persist iterates idMap alt GSCo already finished LoopCo->>Mutex: withLock acquire LoopCo->>LoopCo: "delta=0 flushedRx already advanced" LoopCo->>Mutex: release else GSCo still in-flight LoopCo->>Mutex: withLock blocks GSCo->>Mutex: release LoopCo->>Mutex: acquire LoopCo->>LoopCo: "delta=0 flushedRx advanced by GSCo" LoopCo->>Mutex: release endComments Outside Diff (1)
app/src/main/java/io/nekohasekai/sagernet/bg/proto/TrafficLooper.kt, line 109-122 (link)runOnDefaultDispatcherlaunches onGlobalScope(seeAsyncs.kt), so its coroutine is not a child of the loop'sJob. Whenstop()callsjob?.cancelAndJoin(), it waits only for the loop coroutine to finish, then immediately callspersist(). If the GlobalScope coroutine fromapplySelectis still in-flight at that point, both it andpersist()readitem.lifetimeFlushedRx == 0(the update from the GlobalScope path hasn't landed yet), compute the same positive delta, and each calladdLifetimeTraffic(id, delta, ...). Because the SQL update is additive (lifetimeRx + :rxDelta), the delta is written twice — the last selector-switch interval before disconnect gets double-counted in the lifetime total. Awaiting theJobreturned byrunOnDefaultDispatcher, or collecting those jobs in a list and joining them insidestop()beforepersist(), would close this window.Reviews (4): Last reviewed commit: "fix: address Greptile findings (lifetime..." | Re-trigger Greptile