fix: update myservers config references to connect config references #1810
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a centralized ConnectConfig source and refactors Connect.page to derive and normalize remote-access settings, add client helpers for port parsing and building connect inputs, and extend submission to support legacy Dispatcher and optional Apollo GraphQL flows with improved error handling and legacy fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI (Connect.page)
participant PHP as Connect.page (PHP)
participant Config as ConnectConfig
participant Legacy as Legacy myServersFlashCfg
participant Dispatcher
participant Apollo as Apollo GraphQL
UI->>PHP: Load page
PHP->>Config: ConnectConfig::getConfig()
alt Config returned
Config-->>PHP: Normalized config
else Fallback to legacy
PHP->>Legacy: read myServersFlashCfg['remote']
Legacy-->>PHP: Legacy values
PHP->>PHP: Derive normalized fields (wanAccess, upnp, port)
end
rect rgb(230,245,255)
Note over UI,PHP: Client init uses `wanAccessOrg`, `remoteWanPortRaw`
UI->>UI: parsePort(), buildConnectSettingsInput()
end
UI->>UI: Submit form → collect mutations
UI->>Dispatcher: POST legacy endpoint (if used)
alt Apollo available
UI->>Apollo: send GraphQL mutations (Promise)
end
par Apply remote settings
Dispatcher-->UI: response
Apollo-->UI: response
and
UI-->UI: Promise.all → finalize success/error
end
rect rgb(255,235,235)
Note over UI: On error restore button state and show message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Files:
🔇 Additional comments (5)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| $connectConfig = ConnectConfig::getConfig(); | ||
| $legacyRemoteCfg = $serverState->myServersFlashCfg['remote'] ?? []; | ||
|
|
||
| $remoteDynamicRemoteAccessType = $connectConfig['dynamicRemoteAccessType'] ?? ($legacyRemoteCfg['dynamicRemoteAccessType'] ?? null); | ||
| $remoteWanAccessRaw = $connectConfig['wanaccess'] ?? ($legacyRemoteCfg['wanaccess'] ?? null); |
There was a problem hiding this comment.
Lines 24-28 now populate the page from ConnectConfig::getConfig(), which reads configs/connect.json, but registerServer still posts updates to /boot/config/plugins/dynamix.my.servers/myservers.cfg (see line 147) even though the connect plugin’s persister migrates to and reads from connect.json. User edits to remote access or WAN port will therefore be saved only to the legacy file while the page reload continues to read the untouched connect.json, leaving the settings unchanged in practice.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1810 +/- ##
=======================================
Coverage 52.04% 52.04%
=======================================
Files 874 874
Lines 50372 50372
Branches 5017 5017
=======================================
Hits 26214 26214
Misses 24083 24083
Partials 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page (2)
92-92: Consider usingjson_encode()for JavaScript string output.While the current values are well-controlled ('yes', 'no', or ''), using
json_encode()would be more defensive against future changes that might introduce special characters.-const wanAccessOrg = "<?=$wanAccessOriginal?>"; +const wanAccessOrg = <?=json_encode($wanAccessOriginal)?>;
57-57: Minor: Redundant null coalescing.The
?? nullis unnecessary since$remoteDynamicRemoteAccessTypecan already be null from line 27. However, this is harmless and documents the expected fallback clearly.-$dynamicRemoteAccessType = $remoteDynamicRemoteAccessType ?? null; +$dynamicRemoteAccessType = $remoteDynamicRemoteAccessType;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page
🔇 Additional comments (4)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page (4)
24-30: LGTM! Clean configuration migration pattern.The nested null coalescing operator provides a proper fallback chain from new config to legacy config to null default. This ensures backward compatibility during migration.
32-40: LGTM! Robust input normalization.The use of
filter_varwithFILTER_VALIDATE_BOOLEANandFILTER_NULL_ON_FAILUREcorrectly handles various boolean representations ("yes", "no", "true", "false", "1", "0") while safely defaulting tofalsefor invalid inputs. The numeric port validation is also appropriately defensive.
77-82: LGTM! Proper type normalization for JavaScript interop.The conditional chain correctly handles all possible types: booleans are converted to 'yes'/'no' strings, existing strings are preserved, and other types (including null) default to empty string.
220-220: LGTM! Correct variable substitution.The error message properly uses the new
$remoteWanPortvariable which is guaranteed to be an integer, matching the%uformat specifier.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
Just don't ever change the filename of the .plg without consulting me first. |
myservers.cfgno longer gets written to or read (except for migration purposes), so it'd be better to read from the new values instead of continuing to use the old ones @elibosley @Squidly271 .unless i'm missing something! see #1805
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.