-
Notifications
You must be signed in to change notification settings - Fork 11
refactor: remove unnecessary network stack reload in ConnectSettingsService #1751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
refactor: remove unnecessary network stack reload in ConnectSettingsService #1751
Conversation
WalkthroughA blank line and a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
}, | ||
}); | ||
|
||
await this.networkService.reloadNetworkStack(); | ||
return true; |
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.
Regresses network reload when remote access update fails
After updateRemoteAccess
updates the config it now immediately returns, relying on DynamicRemoteAccessService.enableDynamicRemoteAccess
to emit events that reload nginx/DNS. That service swallows exceptions and may return early without emitting the ENABLE_WAN_ACCESS
/DISABLE_WAN_ACCESS
events (e.g. UpnpRemoteAccessService.stop()
throws before the event is emitted). Prior to this change the method unconditionally awaited networkService.reloadNetworkStack()
, ensuring the new connect.config.wanaccess
value was applied even when the dynamic service failed. With the fallback removed, disabling WAN access can silently leave the previous nginx configuration active until another component triggers a reload, keeping WAN access exposed longer than intended.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1751 +/- ##
=======================================
Coverage 52.64% 52.64%
=======================================
Files 866 866
Lines 49335 49335
Branches 4940 4940
=======================================
Hits 25972 25972
Misses 23290 23290
Partials 73 73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This plugin has been deployed to Cloudflare R2 and is available for testing.
|
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (1)
209-248
: Restore network stack reload call
enableDynamicRemoteAccess and its services don’t trigger any network reload, so removingnetworkService.reloadNetworkStack()
breaks the update flow. Re-add the reload inupdateRemoteAccess
or implement it withinDynamicRemoteAccessService
to restore the previous behavior.
🧹 Nitpick comments (1)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts (1)
44-44
: Remove unusedNetworkService
injection
TheNetworkService
import and constructor parameter inConnectSettingsService
are unused. Remove both to clean up dead code.-import { NetworkService } from '../network/network.service.js'; … - private readonly networkService: NetworkService
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}
: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
packages/unraid-api-plugin-connect/src/unraid-connect/connect-settings.service.ts
Summary by CodeRabbit
Release Notes