Repository navigation
Maintenance: complete workflow, config, and profile-list improvements - #131
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request adds OpenLRTC sidecar integration, emulator-based migration testing, service stop/reload coordination, registry-driven protocol metadata, cached configuration assembly, batch profile deletion, lifecycle-safe profile updates, parser coverage, and build validation changes. ChangesBuild and CI integration
Service lifecycle coordination
Protocol and configuration model
Profile UI lifecycle and incremental updates
Parsing and runtime support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![greptile-apps[bot]](/s/com/githubusercontent/avatars/G.https/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](/s/com/githubusercontent/avatars/G.https/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
app/src/main/java/io/nekohasekai/sagernet/ktx/Logs.kt (1)
9-9: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider marking
sink@Volatilefor cross-thread visibility.
sinkis a plain mutablevarread fromd/i/w/ecalls that may execute on background/plugin threads, while it's expected to be swapped out (e.g. in tests) from another thread. Without@Volatile, a writer thread's update tosinkisn't guaranteed to be visible to reader threads on all JVM/ART memory models.♻️ Proposed fix
- internal var sink: (String) -> Unit = { Libcore.nekoLogPrintln(it) } + `@Volatile` + internal var sink: (String) -> Unit = { Libcore.nekoLogPrintln(it) }🤖 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/ktx/Logs.kt` at line 9, Mark the mutable `sink` property in `Logs.kt` with `@Volatile` so updates made by one thread are immediately visible to background and plugin threads that read it..github/workflows/ci.yml (1)
55-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding the instrumented job's runtime.
Neither the job nor the
Run migration test on emulatorstep setstimeout-minutes. Android emulator runs are known to occasionally hang (boot loops, hungadb/QEMU threads) well past normal completion, and a stuck job here would only fall back to GitHub's default 360-minute job timeout, burning runner minutes on a Namespace runner.⏱️ Suggested fix
instrumented: name: Instrumented (Room migrations) runs-on: namespace-profile-nekoyay + timeout-minutes: 20 needs: - libcore🤖 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 @.github/workflows/ci.yml around lines 55 - 97, The instrumented workflow can run indefinitely when the Android emulator or migration test hangs. Add an appropriate timeout-minutes value to the instrumented job, and optionally a shorter timeout to the Run migration test on emulator step, ensuring the overall runtime is bounded.libcore/build.sh (1)
51-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare function-local variables with
local.
dir,expected,label, andactualleak into the script's global scope since they aren't declaredlocalinsidecheck_sibling_pin. Low risk in this short-lived script, but worth tightening for hygiene/future edits.🧹 Suggested fix
check_sibling_pin() { - dir="$1" - expected="$2" - label="$3" + local dir="$1" + local expected="$2" + local label="$3" [ -z "$dir" ] && return - actual="$(git -C "$dir" rev-parse HEAD 2>/dev/null || echo unknown)" + local actual="$(git -C "$dir" rev-parse HEAD 2>/dev/null || echo unknown)"🤖 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 `@libcore/build.sh` around lines 51 - 62, Declare dir, expected, label, and actual as function-local variables using local at the start of check_sibling_pin, while preserving the existing argument assignments and validation behavior.app/src/main/java/moe/matsuri/nb4a/Protocols.kt (2)
20-35: 🚀 Performance & Scalability | 🔵 TrivialFull-bean equality is more expensive than the prior endpoint hash.
Per the inline comment, non-
ConfigBeanequality now serializes the whole bean (minus name) on everyhashCode()/equals()call, versus the old lightweight address+port+type hash. This is a deliberate tradeoff for stricter dedup (confirmed byDeduplicationTest), but for large subscription imports this adds serialization cost per comparison.🤖 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/moe/matsuri/nb4a/Protocols.kt` around lines 20 - 35, The Deduplication equality implementation now performs expensive full-bean serialization for non-ConfigBean values. Preserve the stricter equality required by DeduplicationTest while avoiding repeated serialization during large imports, for example by caching a stable comparison key or hash in Deduplication and reusing it from hashCode() and equals(); update relevant tests as needed.
15-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
typeis unused inDeduplication
Deduplication.typeis no longer read byhashCode()orequals(). If nothing else consumes it, consider dropping it or documenting why it is retained.🤖 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/moe/matsuri/nb4a/Protocols.kt` around lines 15 - 36, Remove the unused type property from Deduplication and update all construction sites and references accordingly; if it is required for an external contract, document its purpose and include it consistently in equality and hashing.app/src/main/java/io/nekohasekai/sagernet/fmt/ConfigBuilder.kt (1)
702-708: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd golden-test coverage for the TLS-fragment detour path.
The
DataStore.enableTLSFragmentdetection was reworked from map-lookup to JSON-tree traversal (SingBoxOptions.toJsonTree(...).get("tls")...), butConfigBuilderGoldenTest.kthas no test exercising this branch (e.g. a TLS-enabled outbound with fragment enabled assertingdetour == TAG_FRAGMENT). Given this is exactly the kind of logic most likely to silently regress on a map-to-JsonElement rewrite, a small golden test would catch future breakage cheaply.🤖 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` around lines 702 - 708, Add a golden test in ConfigBuilderGoldenTest covering a TLS-enabled outbound when TLS fragmentation is enabled, asserting that the generated outbound detour equals TAG_FRAGMENT. Configure the test through the existing DataStore.enableTLSFragment and outbound setup patterns, and use the current golden-test assertion conventions to exercise the JSON-tree TLS detection branch.
🤖 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/database/ProxyEntity.kt`:
- Around line 213-216: Update ProxyEntity.toStdLink to honor the compact
parameter by passing it through the standard-link conversion or applying the
appropriate compact-link transformation before returning. Ensure both registered
protocol converters and the bean.toUniversalLink fallback produce compact output
when compact is true.
In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt`:
- Around line 782-815: In onUpdated, avoid calling undoManager?.flush() for
every profile update because normal selection changes can dismiss pending undo
actions. Only flush when the updated profile conflicts with an active pending
removal, using the undo manager’s pending-removal state and the updated profile
identity; leave unrelated updates without flushing. Keep onAdd behavior
unchanged unless the same conflict handling is required there.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 55-97: The instrumented workflow can run indefinitely when the
Android emulator or migration test hangs. Add an appropriate timeout-minutes
value to the instrumented job, and optionally a shorter timeout to the Run
migration test on emulator step, ensuring the overall runtime is bounded.
In `@app/src/main/java/io/nekohasekai/sagernet/fmt/ConfigBuilder.kt`:
- Around line 702-708: Add a golden test in ConfigBuilderGoldenTest covering a
TLS-enabled outbound when TLS fragmentation is enabled, asserting that the
generated outbound detour equals TAG_FRAGMENT. Configure the test through the
existing DataStore.enableTLSFragment and outbound setup patterns, and use the
current golden-test assertion conventions to exercise the JSON-tree TLS
detection branch.
In `@app/src/main/java/io/nekohasekai/sagernet/ktx/Logs.kt`:
- Line 9: Mark the mutable `sink` property in `Logs.kt` with `@Volatile` so
updates made by one thread are immediately visible to background and plugin
threads that read it.
In `@app/src/main/java/moe/matsuri/nb4a/Protocols.kt`:
- Around line 20-35: The Deduplication equality implementation now performs
expensive full-bean serialization for non-ConfigBean values. Preserve the
stricter equality required by DeduplicationTest while avoiding repeated
serialization during large imports, for example by caching a stable comparison
key or hash in Deduplication and reusing it from hashCode() and equals(); update
relevant tests as needed.
- Around line 15-36: Remove the unused type property from Deduplication and
update all construction sites and references accordingly; if it is required for
an external contract, document its purpose and include it consistently in
equality and hashing.
In `@libcore/build.sh`:
- Around line 51-62: Declare dir, expected, label, and actual as function-local
variables using local at the start of check_sibling_pin, while preserving the
existing argument assignments and validation behavior.
🪄 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: 1bcdead9-7257-4009-8238-0b5cf8818f26
📒 Files selected for processing (36)
.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/preview.yml.github/workflows/release.ymlapp/build.gradle.ktsapp/src/main/java/io/nekohasekai/sagernet/bg/BaseService.ktapp/src/main/java/io/nekohasekai/sagernet/bg/Executable.ktapp/src/main/java/io/nekohasekai/sagernet/bg/ServiceStopGate.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/fmt/ConfigBuilder.ktapp/src/main/java/io/nekohasekai/sagernet/ktx/Logs.ktapp/src/main/java/io/nekohasekai/sagernet/ktx/Utils.ktapp/src/main/java/io/nekohasekai/sagernet/plugin/PluginManager.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/ProfileDiffCallback.ktapp/src/main/java/moe/matsuri/nb4a/Protocols.ktapp/src/main/java/moe/matsuri/nb4a/SingBoxOptions.javaapp/src/main/java/moe/matsuri/nb4a/utils/Util.ktapp/src/test/java/io/nekohasekai/sagernet/bg/ServiceStopGateTest.ktapp/src/test/java/io/nekohasekai/sagernet/database/ProfileManagerBatchDeleteTest.ktapp/src/test/java/io/nekohasekai/sagernet/fmt/ConfigBuilderGoldenTest.ktapp/src/test/java/io/nekohasekai/sagernet/fmt/ConfigBuilderTestEnv.ktapp/src/test/java/io/nekohasekai/sagernet/fmt/ProtocolRegistryDispatchTest.ktapp/src/test/java/io/nekohasekai/sagernet/group/RawUpdaterParseTest.ktapp/src/test/java/io/nekohasekai/sagernet/ui/ProfileDiffCallbackTest.ktapp/src/test/java/moe/matsuri/nb4a/DeduplicationTest.ktapp/src/test/resources/subscriptions/clash-basic.yamlapp/src/test/resources/subscriptions/clash-malformed.yamlapp/src/test/resources/subscriptions/clash-unknown-tag.yamlapp/src/test/resources/subscriptions/singbox-outbounds.jsonapp/src/test/resources/subscriptions/wireguard.conflibcore/build.shscripts/verify.sh
💤 Files with no reviewable changes (1)
- app/src/main/java/io/nekohasekai/sagernet/bg/Executable.kt
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.kt (1)
389-406: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRefresh the group pager after batch delete
ProfileManager.deleteProfiles()skips listener callbacks, so this path can leave the emptyungroupedtab visible after deleting its last proxy. Emit a group-level reload here, or route the empty-group cleanup through the same listener path as single deletions.🤖 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 389 - 406, The successful batch-delete path in deleteProfilesFromGroup must trigger a group-level reload because ProfileManager.deleteProfiles does not emit listener callbacks. After ProfileManager.deleteProfiles(profiles) completes, call GroupManager.postReload(groupId) before updating the adapter, while preserving the existing failure handling.
🤖 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/moe/matsuri/nb4a/Protocols.kt`:
- Line 20: Update the comparisonHash initialization in Protocols to handle
nullable ConfigBean.config safely, matching the null-safe behavior used by
equals: use the config hash when present and 0 when null, while preserving the
existing bean.hashCode() path for non-ConfigBean values.
---
Outside diff comments:
In `@app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.kt`:
- Around line 389-406: The successful batch-delete path in
deleteProfilesFromGroup must trigger a group-level reload because
ProfileManager.deleteProfiles does not emit listener callbacks. After
ProfileManager.deleteProfiles(profiles) completes, call
GroupManager.postReload(groupId) before updating the adapter, while preserving
the existing failure handling.
🪄 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: 442e85f0-8e62-4fc5-9f58-8cf967dde4d3
📒 Files selected for processing (9)
.github/workflows/ci.ymlapp/src/main/java/io/nekohasekai/sagernet/group/RawUpdater.ktapp/src/main/java/io/nekohasekai/sagernet/ktx/Logs.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationFragment.ktapp/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.ktapp/src/main/java/moe/matsuri/nb4a/Protocols.ktapp/src/test/java/io/nekohasekai/sagernet/fmt/ConfigBuilderGoldenTest.ktapp/src/test/java/moe/matsuri/nb4a/DeduplicationTest.ktlibcore/build.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/workflows/ci.yml
- app/src/main/java/io/nekohasekai/sagernet/ktx/Logs.kt
- app/src/test/java/moe/matsuri/nb4a/DeduplicationTest.kt
- libcore/build.sh
- app/src/test/java/io/nekohasekai/sagernet/fmt/ConfigBuilderGoldenTest.kt
- app/src/main/java/io/nekohasekai/sagernet/ui/ConfigurationGroupFragment.kt
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.
Summary
resolveChainlookups, remove redundant JSON round trips, and centralize mandatory protocol dispatch.Verification
Notes