-
-
Notifications
You must be signed in to change notification settings - Fork 365
[reverse proxy] Use getApiBase() to get GraphQL Endpoint for System Information about Network #1398
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
Conversation
📝 WalkthroughWalkthroughMultiple frontend files were updated to replace hardcoded URL construction using protocol, host, and port variables with a centralized Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
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)
front/systeminfoNetwork.php (1)
34-101: Eliminate duplicate code blocks.The same code block (External IP retrieval and network variable assignments) is repeated three times identically. This violates the DRY principle and makes maintenance difficult.
♻️ Proposed fix to remove duplication
Remove the duplicate blocks at lines 59-76 and 84-101. The first occurrence (lines 34-51) is sufficient since these variables are used in the HTML output that follows.
-// ---------------------------------------------------- -// Network Hardware Stats -// ---------------------------------------------------- - - -// External IP -$externalIp = getExternalIp(); - -// Server Name -$network_NAME = gethostname() ?: lang('Systeminfo_Network_Server_Name_String'); - -// HTTPS Check -$network_HTTPS = isset($_SERVER['HTTPS']) ? 'Yes (HTTPS)' : lang('Systeminfo_Network_Secure_Connection_String'); - -// Query String -$network_QueryString = !empty($_SERVER['QUERY_STRING']) - ? $_SERVER['QUERY_STRING'] - : lang('Systeminfo_Network_Server_Query_String'); - -// Referer -$network_referer = !empty($_SERVER['HTTP_REFERER']) - ? $_SERVER['HTTP_REFERER'] - : lang('Systeminfo_Network_HTTP_Referer_String'); - - - -// ---------------------------------------------------- -// Network Stats (General) -// ---------------------------------------------------- - -// External IP -$externalIp = getExternalIp(); - -// Server Name -$network_NAME = gethostname() ?: lang('Systeminfo_Network_Server_Name_String'); - -// HTTPS Check -$network_HTTPS = isset($_SERVER['HTTPS']) ? 'Yes (HTTPS)' : lang('Systeminfo_Network_Secure_Connection_String'); - -// Query String -$network_QueryString = !empty($_SERVER['QUERY_STRING']) - ? $_SERVER['QUERY_STRING'] - : lang('Systeminfo_Network_Server_Query_String'); - -// Referer -$network_referer = !empty($_SERVER['HTTP_REFERER']) - ? $_SERVER['HTTP_REFERER'] - : lang('Systeminfo_Network_HTTP_Referer_String'); -
🧹 Nitpick comments (1)
front/deviceDetailsTools.php (1)
221-222: Remove redundant apiBase declaration.The
apiBaseconstant is already declared globally at line 214. Re-declaring it here is unnecessary and creates a shadowed variable.♻️ Proposed fix
- // Build base URL dynamically - const apiBase = getApiBase(); - $.ajax({The same issue exists in
deleteDeviceEvents()at line 586 andresetDeviceProps()at line 638. Consider removing those redundant declarations as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
front/appEventsCore.phpfront/deviceDetailsTools.phpfront/events.phpfront/systeminfoNetwork.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,php,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use environment variables (
NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding/data/dbor relative paths. Use/datafor persistent config/db,/tmpfor runtime logs/api/nginx state
Files:
front/appEventsCore.phpfront/events.phpfront/systeminfoNetwork.phpfront/deviceDetailsTools.php
🔇 Additional comments (5)
front/systeminfoNetwork.php (1)
339-344: LGTM: Centralized API base URL construction.The change correctly uses
getApiBase()to construct the endpoint URL, replacing the previous hardcoded approach. This improves maintainability and consistency across the application.front/appEventsCore.php (1)
27-28: LGTM: Centralized GraphQL endpoint construction.The change correctly replaces manual protocol/host/port assembly with the centralized
getApiBase()function, improving consistency across the codebase.front/events.php (2)
163-166: LGTM: Centralized API base for event totals endpoint.The change correctly uses
getApiBase()to construct the endpoint URL, aligning with the PR's objective to standardize API base URL handling.
209-211: LGTM: Centralized API base for session events endpoint.The change correctly uses
getApiBase()to construct the endpoint URL, maintaining consistency with the other updated functions in this file.front/deviceDetailsTools.php (1)
213-215: LGTM: Global API configuration.The global
apiBaseandapiTokendeclarations provide convenient access across multiple tool functions. This centralizes the API configuration effectively.
Use
getApiBase()to get GraphQL Endpoint forfront/systeminfoNetwork.php.Note that you might want to check the Variable Names and see if
baseUrlorapiBaseis preferrable to you (it might not be fully consistent with the rest of the Code).Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.