feat: integrate core service with SSO and GraphQL proxy updates #1875
Conversation
WalkthroughAdds SSO authentication endpoints and GraphQL proxying, implements an Unraid Core service (rc.unraid) with install/uninstall integration, switches SSO token validation to an HTTP endpoint, centralizes CSRF handling, and updates frontend OAuth callback routing and tests. Changes
Sequence DiagramssequenceDiagram
actor User
participant Browser as Browser/Web App
participant NGINX as NGINX Proxy
participant Core as Unraid Core (rc.unraid)
participant OIDC as OIDC Provider
participant API as Unraid API / OIDC Callback
User->>Browser: Click "SSO Login"
Browser->>Browser: set state=loading, generate state token
Browser->>NGINX: GET /auth/sso (initiate SSO)
NGINX->>Core: proxy /auth/sso -> unraid-core.sock
Core-->>NGINX: redirect to OIDC provider
NGINX-->>Browser: redirect to OIDC provider
Browser->>OIDC: Authenticate (external)
OIDC-->>Browser: Redirect callback (code & state) -> NGINX
Browser->>NGINX: GET /callback?code&state
NGINX->>API: proxy /graphql/api/auth/oidc or forward to internal OIDC endpoint
API->>API: validate code/state, establish session
API-->>Browser: redirect to /login (or app) with session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1875 +/- ##
==========================================
+ Coverage 46.40% 46.46% +0.05%
==========================================
Files 954 954
Lines 59791 59836 +45
Branches 5538 5551 +13
==========================================
+ Hits 27749 27804 +55
+ Misses 31923 31913 -10
Partials 119 119 ☔ 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}
pujitm
force-pushed
the
feat/substitute-graphql
branch
from
230e0cd to
861c49d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@plugin/plugins/dynamix.unraid.net.plg`:
- Around line 333-346: Replace the broad glob "unraid-*" used when setting
core_pkg_installed with a version-specific pattern or filter to avoid matching
unrelated packages: update the ls/grep invocation that assigns
core_pkg_installed to only match names like "unraid-" followed by a digit (e.g.,
use "unraid-[0-9]*" or pipe through grep -E '^unraid-[0-9]') so
core_pkg_installed and core_pkg_basename resolve to the actual core package name
and removepkg --terse is invoked only for that package.
In `@plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid`:
- Around line 67-79: The rollback() function currently does rm -rf
/usr/local/unraid then mv /usr/local/unraid.prev /usr/local/unraid which is not
atomic and can leave the system without an installation if mv fails; change to a
safer three-step swap: rename the current dir to a temp backup (e.g., mv
/usr/local/unraid /usr/local/unraid.tmp), then mv /usr/local/unraid.prev to
/usr/local/unraid, and only after verifying that succeeds remove the temp
backup; ensure each mv is checked for failure and on any error attempt to
restore from the temp backup and return a non-zero exit code so rollback is
atomic and recoverable, updating the rollback() function and its error handling
accordingly.
🧹 Nitpick comments (5)
web/src/components/sso/useSsoAuth.ts (1)
91-93: Guard placement has no effect - consider moving to function start.This guard at the end of the try block is a no-op: if a token or error exists, the function already returned at lines 76 or 88. If neither exists, there's no code after this guard anyway.
If the intent is to restrict OAuth callback processing to the login page only, move this guard to the beginning of
handleOAuthCallback:♻️ Suggested refactor
const handleOAuthCallback = async () => { + if (window.location.pathname !== '/login') { + return; + } + try { // First check hash parameters (for token and error - keeps them out of server logs) const hashParams = new URLSearchParams(window.location.hash.slice(1)); ... - if (window.location.pathname !== '/login') { - return; - } } catch (err) {plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid (2)
25-26: Minor: Simplify command substitution.Line 26 uses an unnecessary
catpipe. Consider simplifying:export SECRET_KEY_BASE=$(cat "$CONFIG_DIR/secret_key_base") -export RELEASE_COOKIE=$(cat "$CONFIG_DIR/secret_key_base" | head -c 20) +export RELEASE_COOKIE=$(head -c 20 "$CONFIG_DIR/secret_key_base")
81-88: Unknown commands should exit with non-zero status.When an invalid command is passed, the script shows usage but exits with status 0 (implicit). Consider exiting with non-zero for unknown commands:
case "${1:-}" in start) start ;; stop) stop ;; restart) restart ;; status) status ;; rollback) rollback ;; - *) echo "Usage: $0 {start|stop|restart|status|rollback}" ;; + *) echo "Usage: $0 {start|stop|restart|status|rollback}"; exit 1 ;; esacplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (1)
219-219: Trailing whitespace.Line 219 has trailing whitespace after
fi. Consider removing it for consistency.-fi +fiapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts (1)
98-105: Clarify why OS version check is disabled.The change to pass
{ checkOsVersion: false }removes version gating for this modification. Consider adding a brief comment explaining why this modification should apply to all versions, given the class docstring mentions "< Unraid 7.2.0".async shouldApply(): Promise<ShouldApplyWithReason> { + // Apply to all versions as core socket routing is needed regardless of OS version const { shouldApply, reason } = await super.shouldApply({ checkOsVersion: false });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/rc.nginx.modified.snapshotapi/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patchapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.tsplugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/etc/rc.d/rc.unraidplugin/source/dynamix.unraid.net/etc/rc.d/rc6.d/K30unraid-coreplugin/source/dynamix.unraid.net/install/doinst.shplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.shweb/__test__/components/SsoButton.test.tsweb/src/components/sso/useSsoAuth.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- web/test/components/SsoButton.test.ts
- plugin/source/dynamix.unraid.net/install/doinst.sh
- plugin/source/dynamix.unraid.net/etc/rc.d/rc6.d/K30unraid-core
- api/src/unraid-api/unraid-file-modifier/modifications/test/snapshots/rc.nginx.modified.snapshot
🧰 Additional context used
📓 Path-based instructions (7)
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraidweb/src/components/sso/useSsoAuth.tsplugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.shapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.tsapi/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with.jsextensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks
Files:
web/src/components/sso/useSsoAuth.tsapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
web/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Always run
pnpm codegenfor GraphQL code generation in the web directory
Files:
web/src/components/sso/useSsoAuth.ts
web/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure Vue reactivity imports are added to store files (computed, ref, watchEffect)
Files:
web/src/components/sso/useSsoAuth.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use theanytype. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start
Files:
web/src/components/sso/useSsoAuth.tsapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
api/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS repo located at
api/src/unraid-api/instead of the legacy code
Files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.tsapi/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)
Files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Learnt from: mdatelle
Repo: unraid/api PR: 1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraidplugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-01-27T14:31:42.305Z
Learnt from: elibosley
Repo: unraid/api PR: 1063
File: web/components/SsoButton.ce.vue:5-8
Timestamp: 2025-01-27T14:31:42.305Z
Learning: In the Unraid API web components, SSO-related props are intentionally provided in both camelCase (`ssoEnabled`) and lowercase (`ssoenabled`) variants to support interchangeable usage across different contexts (e.g., HTML attributes vs Vue props).
Applied to files:
web/src/components/sso/useSsoAuth.ts
📚 Learning: 2025-09-04T18:42:53.531Z
Learnt from: pujitm
Repo: unraid/api PR: 1658
File: plugin/plugins/dynamix.unraid.net.plg:73-79
Timestamp: 2025-09-04T18:42:53.531Z
Learning: In the dynamix.unraid.net plugin, versions 6.12.1-6.12.14 and 6.12.15 prereleases are intentionally allowed to install with warnings (rather than immediate cleanup) to provide users with a grace period and notice before functionality is completely removed. This is a deliberate UX decision to avoid immediately breaking existing setups while encouraging upgrades.
Applied to files:
plugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-01-29T00:59:26.633Z
Learnt from: zackspear
Repo: unraid/api PR: 1079
File: web/scripts/deploy-dev.sh:51-54
Timestamp: 2025-01-29T00:59:26.633Z
Learning: For the Unraid web components deployment process, JS file validation isn't required in auth-request.php updates since the files come from the controlled build pipeline where we are the source.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-09-04T15:26:34.416Z
Learnt from: elibosley
Repo: unraid/api PR: 1657
File: web/scripts/deploy-dev.sh:37-41
Timestamp: 2025-09-04T15:26:34.416Z
Learning: In web/scripts/deploy-dev.sh, the command `rm -rf /usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/*` intentionally removes all contents of the unraid-components directory before deploying standalone components. This broader cleanup is desired behavior according to the maintainer elibosley.
Applied to files:
plugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-05-07T16:07:47.236Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh:107-113
Timestamp: 2025-05-07T16:07:47.236Z
Learning: The Unraid API is designed to handle missing configuration files gracefully with smart internal fallbacks rather than requiring installation scripts to create default configurations.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-05-08T19:28:54.365Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh:19-24
Timestamp: 2025-05-08T19:28:54.365Z
Learning: The directory `/usr/local/emhttp/plugins/dynamix.my.servers` is a valid directory that exists as part of the Unraid API plugin installation and should be included in verification checks.
Applied to files:
plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-01-29T16:35:43.699Z
Learnt from: elibosley
Repo: unraid/api PR: 1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.699Z
Learning: In the Unraid API, FileModification implementations (apply/rollback methods) don't need to implement their own error handling as it's handled by the UnraidFileModifierService caller.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
📚 Learning: 2025-01-29T16:35:43.699Z
Learnt from: elibosley
Repo: unraid/api PR: 1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.699Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations, including detailed error logging with stack traces and modification IDs. Individual FileModification implementations should focus on their core functionality without duplicating error handling.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
📚 Learning: 2025-01-29T16:36:04.777Z
Learnt from: elibosley
Repo: unraid/api PR: 1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: In the Unraid API, FileModification implementations (like LogRotateModification) don't need to handle errors internally as error handling is managed at the UnraidFileModifierService level.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
📚 Learning: 2025-01-29T16:36:04.777Z
Learnt from: elibosley
Repo: unraid/api PR: 1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations. It includes try/catch blocks, detailed error logging, and safe rollback mechanisms. Individual FileModification implementations (like LogRotateModification) should allow errors to propagate to this service layer rather than handling them internally.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
📚 Learning: 2025-02-03T18:57:53.577Z
Learnt from: elibosley
Repo: unraid/api PR: 1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts:80-80
Timestamp: 2025-02-03T18:57:53.577Z
Learning: In the unraid/api codebase, patch files generated for file modifications always use the .patch extension, regardless of the source file type being modified.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.tsapi/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
📚 Learning: 2025-11-24T17:51:37.915Z
Learnt from: CR
Repo: unraid/api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:51:37.915Z
Learning: Applies to api/**/* : Prefer adding new files to the NestJS repo located at `api/src/unraid-api/` instead of the legacy code
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
📚 Learning: 2025-02-03T17:21:26.738Z
Learnt from: elibosley
Repo: unraid/api PR: 1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-02-03T17:21:26.738Z
Learning: The project uses patches to override existing Unraid pages rather than modifying them directly.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test API
🔇 Additional comments (11)
web/src/components/sso/useSsoAuth.ts (1)
52-56: LGTM!Clean simplification with proper URL encoding of the provider ID to prevent injection issues.
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid (1)
1-10: LGTM!Clear header, appropriate variable definitions with sensible defaults for paths. Using
${UNRAID_LOG_PATH:-...}for configurable logging is good practice.plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (2)
43-50: LGTM!The additions of
/etc/rc.d/rc.unraidto critical files and/usr/local/unraidto critical directories are consistent with the new Unraid Core service being introduced in this PR.
164-170: LGTM!The new shutdown script check for
K30unraid-corefollows the established pattern used forK10flash-backupandK20unraid-api. The ordering (K30) ensures the core service stops after the API service (K20).api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch (2)
25-40: LGTM - Well-structured remote access check.The
check_remote_access()function properly validates multiple conditions before enabling remote access:
- Plugin enablement via
api_utils.sh- Config file and
jqavailability- Both
wanaccess=trueAND valid usernameThe defensive checks (file existence, command availability, null handling) are appropriate.
73-86: Good fix for array iteration.Quoting the array expansion (
"${!NET_FQDN[@]}"instead of${!NET_FQDN[@]}) correctly prevents word splitting on interface names that might contain spaces or special characters.api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts (1)
71-74: LGTM!The proxy_pass target change aligns with routing
/graphqlrequests through the new Unraid Core service socket.plugin/plugins/dynamix.unraid.net.plg (4)
12-15: LGTM!Core package entities are properly defined with SHA256 checksum for integrity verification.
59-63: LGTM!Core package download block follows the same pattern as the main API package with proper URL and SHA256 verification.
555-584: Core package installation follows good patterns.The installation logic properly:
- Cleans up old package versions
- Stops the service before upgrade
- Uses
upgradepkg --install-new --reinstall- Sets executable permission on rc.unraid
- Exits with error if package file is missing
One note: Line 559's cleanup pattern
unraid-*.txzcould also be made more specific similar to the uninstall pattern concern.
661-667: LGTM!Core service startup after API startup is appropriate. The warning message when
rc.unraidis not found provides good visibility for debugging.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
Outdated
Show resolved
Hide resolved
| # Stop and remove Unraid Core package | ||
| if [ -x "/etc/rc.d/rc.unraid" ]; then | ||
| echo "Stopping Unraid Core..." | ||
| /etc/rc.d/rc.unraid stop || echo "Warning: Failed to stop Unraid Core" | ||
| fi | ||
| core_pkg_installed=$(ls -1 /var/log/packages/unraid-* 2>/dev/null | head -1) | ||
| if [ -n "$core_pkg_installed" ]; then | ||
| core_pkg_basename=$(basename "$core_pkg_installed") | ||
| echo "Removing core package: $core_pkg_basename" | ||
| removepkg --terse "$core_pkg_basename" | ||
| else | ||
| echo "No Unraid Core package found" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n plugin/plugins/dynamix.unraid.net.plg | sed -n '333,346p'Repository: unraid/api
Length of output: 722
🏁 Script executed:
# Check for package naming patterns in the codebase
rg "unraid-" plugin/plugins/dynamix.unraid.net.plg | head -20Repository: unraid/api
Length of output: 1437
🏁 Script executed:
# Look for references to package removals or installations to understand naming conventions
rg "removepkg|installpkg|txz" plugin/plugins/dynamix.unraid.net.plg | head -30Repository: unraid/api
Length of output: 1699
Use a more specific pattern for core package matching.
The glob pattern unraid-* on line 339 could match unrelated packages like unraid-api if they're installed in /var/log/packages/. Since the core package follows a version-based naming convention (e.g., unraid-0.1.0-2026.01.14.1), use:
Suggested fix
- core_pkg_installed=$(ls -1 /var/log/packages/unraid-* 2>/dev/null | head -1)
+ core_pkg_installed=$(ls -1 /var/log/packages/unraid-[0-9]* 2>/dev/null | head -1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Stop and remove Unraid Core package | |
| if [ -x "/etc/rc.d/rc.unraid" ]; then | |
| echo "Stopping Unraid Core..." | |
| /etc/rc.d/rc.unraid stop || echo "Warning: Failed to stop Unraid Core" | |
| fi | |
| core_pkg_installed=$(ls -1 /var/log/packages/unraid-* 2>/dev/null | head -1) | |
| if [ -n "$core_pkg_installed" ]; then | |
| core_pkg_basename=$(basename "$core_pkg_installed") | |
| echo "Removing core package: $core_pkg_basename" | |
| removepkg --terse "$core_pkg_basename" | |
| else | |
| echo "No Unraid Core package found" | |
| fi | |
| # Stop and remove Unraid Core package | |
| if [ -x "/etc/rc.d/rc.unraid" ]; then | |
| echo "Stopping Unraid Core..." | |
| /etc/rc.d/rc.unraid stop || echo "Warning: Failed to stop Unraid Core" | |
| fi | |
| core_pkg_installed=$(ls -1 /var/log/packages/unraid-[0-9]* 2>/dev/null | head -1) | |
| if [ -n "$core_pkg_installed" ]; then | |
| core_pkg_basename=$(basename "$core_pkg_installed") | |
| echo "Removing core package: $core_pkg_basename" | |
| removepkg --terse "$core_pkg_basename" | |
| else | |
| echo "No Unraid Core package found" | |
| fi |
🤖 Prompt for AI Agents
In `@plugin/plugins/dynamix.unraid.net.plg` around lines 333 - 346, Replace the
broad glob "unraid-*" used when setting core_pkg_installed with a
version-specific pattern or filter to avoid matching unrelated packages: update
the ls/grep invocation that assigns core_pkg_installed to only match names like
"unraid-" followed by a digit (e.g., use "unraid-[0-9]*" or pipe through grep -E
'^unraid-[0-9]') so core_pkg_installed and core_pkg_basename resolve to the
actual core package name and removepkg --terse is invoked only for that package.
pujitm
force-pushed
the
feat/substitute-graphql
branch
from
861c49d to
2a31900
Compare
There was a problem hiding this comment.
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 (2)
web/src/components/sso/useSsoAuth.ts (2)
11-23: Return type doesn't reflect nullable fields.The function's return type declares all fields as non-null, but the implementation can return null values when DOM queries fail. This creates a type safety gap where callers rely on the return type but receive null values at runtime.
Suggested fix
- const getInputFields = (): { - form: HTMLFormElement; - passwordField: HTMLInputElement; - usernameField: HTMLInputElement; - } => { + const getInputFields = (): { + form: HTMLFormElement | null; + passwordField: HTMLInputElement | null; + usernameField: HTMLInputElement | null; + } => {
58-93: Move the pathname check to the start of the function.The login path check on line 91 only executes when no token or error is present. If a user navigates to a non-login page with a token in the URL (e.g.,
/dashboard#token=xyz), the code will still attempt to process the callback—changing state, attempting form manipulation, and cleaning up the URL—before the form lookup fails silently.Moving the check to the start avoids unnecessary processing and potential side effects on non-login pages.
Suggested fix
const handleOAuthCallback = async () => { try { + if (window.location.pathname !== '/login') { + return; + } + // First check hash parameters (for token and error - keeps them out of server logs) const hashParams = new URLSearchParams(window.location.hash.slice(1)); const hashToken = hashParams.get('token'); const hashError = hashParams.get('error'); // Then check query parameters (for error/token fallback) const search = new URLSearchParams(window.location.search); // Check for error in hash (preferred) or query params (fallback) const errorParam = hashError || search.get('error') || ''; if (errorParam) { currentState.value = 'error'; error.value = errorParam; // Clean up the URL (both hash and query params) window.history.replaceState({}, document.title, window.location.pathname); return; } // Handle OAuth callback if we have a token in hash (from OIDC redirect) const token = hashToken || search.get('token'); // Check hash first, query as fallback if (token) { currentState.value = 'loading'; disableFormOnSubmit(); enterCallbackTokenIntoField(token); // Clean up the URL (both hash and query params) window.history.replaceState({}, document.title, window.location.pathname); return; } - - if (window.location.pathname !== '/login') { - return; - } } catch (err) {
♻️ Duplicate comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
339-346: Use a more specific pattern for core package matching.The glob pattern
unraid-*on line 339 could match unrelated packages likeunraid-api-*in/var/log/packages/. Since the core package follows a version-based naming convention (e.g.,unraid-0.1.0-2026.01.14.1), use a more specific pattern.Suggested fix
- core_pkg_installed=$(ls -1 /var/log/packages/unraid-* 2>/dev/null | head -1) + core_pkg_installed=$(ls -1 /var/log/packages/unraid-[0-9]* 2>/dev/null | head -1)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/rc.nginx.modified.snapshotapi/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patchapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.tsplugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/etc/rc.d/rc.unraidplugin/source/dynamix.unraid.net/etc/rc.d/rc6.d/K30unraid-coreplugin/source/dynamix.unraid.net/install/doinst.shplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.shweb/__test__/components/SsoButton.test.tsweb/src/components/sso/useSsoAuth.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- web/test/components/SsoButton.test.ts
- plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid
- api/src/unraid-api/unraid-file-modifier/modifications/test/snapshots/rc.nginx.modified.snapshot
- plugin/source/dynamix.unraid.net/install/doinst.sh
- api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
- api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
plugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/etc/rc.d/rc6.d/K30unraid-coreweb/src/components/sso/useSsoAuth.tsplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with.jsextensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks
Files:
web/src/components/sso/useSsoAuth.ts
web/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Always run
pnpm codegenfor GraphQL code generation in the web directory
Files:
web/src/components/sso/useSsoAuth.ts
web/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure Vue reactivity imports are added to store files (computed, ref, watchEffect)
Files:
web/src/components/sso/useSsoAuth.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use theanytype. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start
Files:
web/src/components/sso/useSsoAuth.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Learnt from: mdatelle
Repo: unraid/api PR: 1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
📚 Learning: 2025-09-04T18:42:53.531Z
Learnt from: pujitm
Repo: unraid/api PR: 1658
File: plugin/plugins/dynamix.unraid.net.plg:73-79
Timestamp: 2025-09-04T18:42:53.531Z
Learning: In the dynamix.unraid.net plugin, versions 6.12.1-6.12.14 and 6.12.15 prereleases are intentionally allowed to install with warnings (rather than immediate cleanup) to provide users with a grace period and notice before functionality is completely removed. This is a deliberate UX decision to avoid immediately breaking existing setups while encouraging upgrades.
Applied to files:
plugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/etc/rc.d/rc6.d/K30unraid-coreplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-09-04T15:26:34.416Z
Learnt from: elibosley
Repo: unraid/api PR: 1657
File: web/scripts/deploy-dev.sh:37-41
Timestamp: 2025-09-04T15:26:34.416Z
Learning: In web/scripts/deploy-dev.sh, the command `rm -rf /usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/*` intentionally removes all contents of the unraid-components directory before deploying standalone components. This broader cleanup is desired behavior according to the maintainer elibosley.
Applied to files:
plugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/etc/rc.d/rc6.d/K30unraid-coreplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-02-04T17:21:39.710Z
Learnt from: mdatelle
Repo: unraid/api PR: 1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-05-08T19:28:54.365Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh:19-24
Timestamp: 2025-05-08T19:28:54.365Z
Learning: The directory `/usr/local/emhttp/plugins/dynamix.my.servers` is a valid directory that exists as part of the Unraid API plugin installation and should be included in verification checks.
Applied to files:
plugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
plugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/etc/rc.d/rc6.d/K30unraid-coreplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-05-08T19:31:52.417Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh:11-16
Timestamp: 2025-05-08T19:31:52.417Z
Learning: The `dynamix.my.servers` namespace is still valid and should not be changed to `dynamix.unraid.net` in file paths, as both namespaces coexist in the codebase.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
plugin/plugins/dynamix.unraid.net.plgplugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh
📚 Learning: 2025-02-05T21:10:48.136Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-01-29T00:59:26.633Z
Learnt from: zackspear
Repo: unraid/api PR: 1079
File: web/scripts/deploy-dev.sh:51-54
Timestamp: 2025-01-29T00:59:26.633Z
Learning: For the Unraid web components deployment process, JS file validation isn't required in auth-request.php updates since the files come from the controlled build pipeline where we are the source.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-05-07T16:07:47.236Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh:107-113
Timestamp: 2025-05-07T16:07:47.236Z
Learning: The Unraid API is designed to handle missing configuration files gracefully with smart internal fallbacks rather than requiring installation scripts to create default configurations.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-01-27T14:31:42.305Z
Learnt from: elibosley
Repo: unraid/api PR: 1063
File: web/components/SsoButton.ce.vue:5-8
Timestamp: 2025-01-27T14:31:42.305Z
Learning: In the Unraid API web components, SSO-related props are intentionally provided in both camelCase (`ssoEnabled`) and lowercase (`ssoenabled`) variants to support interchangeable usage across different contexts (e.g., HTML attributes vs Vue props).
Applied to files:
web/src/components/sso/useSsoAuth.ts
🔇 Additional comments (12)
web/src/components/sso/useSsoAuth.ts (4)
1-9: LGTM!Clean imports and well-typed state initialization.
25-50: LGTM!Form manipulation logic is straightforward. The null guards properly protect against missing DOM elements.
52-56: LGTM!Clean simplification. Using
encodeURIComponenton theproviderIdis good practice for preventing path injection.
102-111: LGTM!Clean lifecycle hook usage and well-scoped public API.
plugin/source/dynamix.unraid.net/etc/rc.d/rc6.d/K30unraid-core (1)
1-7: LGTM!The shutdown script follows the established pattern and correctly orders the core service shutdown (K30) after the API (K20). The executable check provides a safe guard when
rc.unraidisn't present.plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh (2)
45-50: LGTM!The additions to
CRITICAL_FILESandCRITICAL_DIRSare consistent with the new core service infrastructure.
164-170: LGTM!The shutdown script verification for
K30unraid-corefollows the established pattern and integrates correctly with the error tracking.plugin/plugins/dynamix.unraid.net.plg (5)
12-15: LGTM!The core package entity definitions follow the established pattern for the API package and include proper SHA256 integrity verification.
59-63: LGTM!The download block correctly references the core package entities and maintains SHA256 integrity verification.
432-434: LGTM!The core package variables follow the established pattern for the API package.
661-667: LGTM!The startup sequence correctly starts the core service after the API, with appropriate error handling and user feedback.
555-584: Core installation block looks correct.The installation flow properly:
- Cleans up old core package files using glob
unraid-*.txz- Stops the service before upgrade to avoid races
- Installs the package
- Fails fast if package is missing (consistent with project patterns)
The glob is safe since core packages use
unraid-*.txznaming while API packages usedynamix.unraid.net-*.txznaming, so cleanup won't accidentally remove API packages.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pujitm
force-pushed
the
feat/substitute-graphql
branch
from
9182cff to
f73919a
Compare
There was a problem hiding this comment.
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)
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts (1)
32-40: Add idempotency check for MYSERVERS replacement to match pattern used elsewhere in file.The MYSERVERS replacement (lines 36-40) lacks the idempotency protection used by other major patches in this file (lines 42, 76, 84, 102). If the exact string doesn't match—due to whitespace variations or file differences—the replace silently succeeds without actually replacing, leaving
API_UTILSandCONNECT_CONFIGundefined while subsequent patches likecheck_remote_access()still execute.Either:
- Wrap lines 36-40 with
if (!newContent.includes('CONNECT_CONFIG='))check- Add back the MYSERVERS validation (lines 32-34) to fail fast on unexpected content
- Check for pre-existence of the replacement before attempting it
♻️ Duplicate comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
339-346: Use a more specific pattern for core package matching.The glob pattern
unraid-*on line 339 could potentially match unrelated packages. Since the core package follows the naming conventionunraid-X.Y.Z-YYYY.MM.DD.N(starting with a digit afterunraid-), a more specific pattern would be safer.Suggested fix
- core_pkg_installed=$(ls -1 /var/log/packages/unraid-* 2>/dev/null | head -1) + core_pkg_installed=$(ls -1 /var/log/packages/unraid-[0-9]* 2>/dev/null | head -1)
🧹 Nitpick comments (2)
plugin/plugins/dynamix.unraid.net.plg (1)
559-564: Use a more specific pattern for cleanup loop.Similar to the removal logic, the glob pattern
unraid-*.txzcould match unrelated packages. Use a version-specific pattern for consistency and safety.Suggested fix
- for txz_file in /boot/config/plugins/dynamix.my.servers/unraid-*.txz; do + for txz_file in /boot/config/plugins/dynamix.my.servers/unraid-[0-9]*.txz; doweb/src/helpers/create-apollo-client.ts (1)
46-51: Good centralization of CSRF token handling.The extraction of
csrfTokenwith a fallback value ensures consistent CSRF token usage across both HTTP headers and WebSocket query parameters. This aligns with the sandbox behavior where CSRF validation should fail silently with a fallback.Consider adding a type declaration for
globalThis.csrf_tokento improve type safety:declare global { var csrf_token: string | undefined; }This could be added to the existing
Windowinterface declaration block or in a separate global types file.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/rc.nginx.modified.snapshotapi/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patchapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.tsplugin/plugins/dynamix.unraid.net.plgweb/src/components/sso/useSsoAuth.tsweb/src/helpers/create-apollo-client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/sso/useSsoAuth.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with.jsextensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks
Files:
web/src/helpers/create-apollo-client.tsapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
web/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Always run
pnpm codegenfor GraphQL code generation in the web directory
Files:
web/src/helpers/create-apollo-client.ts
web/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure Vue reactivity imports are added to store files (computed, ref, watchEffect)
Files:
web/src/helpers/create-apollo-client.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use theanytype. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start
Files:
web/src/helpers/create-apollo-client.tsapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
web/src/helpers/create-apollo-client.tsplugin/plugins/dynamix.unraid.net.plgapi/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patchapi/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/rc.nginx.modified.snapshotapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
api/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS repo located at
api/src/unraid-api/instead of the legacy code
Files:
api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patchapi/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/rc.nginx.modified.snapshotapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)
Files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
🧠 Learnings (20)
📓 Common learnings
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Learnt from: elibosley
Repo: unraid/api PR: 1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:24-27
Timestamp: 2025-01-31T22:01:22.708Z
Learning: The Unraid UI uses a modern notification system with a custom `unraid-toaster` component replacing the legacy jGrowl notifications. The system is backed by a comprehensive GraphQL API with real-time subscription support for notification updates.
Learnt from: elibosley
Repo: unraid/api PR: 1063
File: web/components/SsoButton.ce.vue:5-8
Timestamp: 2025-01-27T14:31:42.305Z
Learning: In the Unraid API web components, SSO-related props are intentionally provided in both camelCase (`ssoEnabled`) and lowercase (`ssoenabled`) variants to support interchangeable usage across different contexts (e.g., HTML attributes vs Vue props).
Learnt from: mdatelle
Repo: unraid/api PR: 1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
Learnt from: mdatelle
Repo: unraid/api PR: 1219
File: api/src/unraid-api/main.ts:32-55
Timestamp: 2025-03-07T17:35:50.406Z
Learning: Helmet security configuration in the Unraid API is intentionally relaxed (with disabled CSP, CORS policies, and HSTS) to maintain compatibility with existing Unraid plugins. Stricter security settings might break current plugin functionality.
Learnt from: elibosley
Repo: unraid/api PR: 1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations. It includes try/catch blocks, detailed error logging, and safe rollback mechanisms. Individual FileModification implementations (like LogRotateModification) should allow errors to propagate to this service layer rather than handling them internally.
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh:107-113
Timestamp: 2025-05-07T16:07:47.236Z
Learning: The Unraid API is designed to handle missing configuration files gracefully with smart internal fallbacks rather than requiring installation scripts to create default configurations.
Learnt from: elibosley
Repo: unraid/api PR: 1211
File: unraid-ui/src/components/form/number/NumberFieldInput.vue:1-21
Timestamp: 2025-03-13T16:17:21.897Z
Learning: The unraid-ui is a UI library being published externally, where wrapping third-party components (like those from reka-ui) is an intentional design choice to maintain a consistent interface, standardize styling, and control the exposed API.
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Learnt from: elibosley
Repo: unraid/api PR: 1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.699Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations, including detailed error logging with stack traces and modification IDs. Individual FileModification implementations should focus on their core functionality without duplicating error handling.
Learnt from: mdatelle
Repo: unraid/api PR: 1219
File: api/src/unraid-api/main.ts:57-63
Timestamp: 2025-03-07T17:36:52.790Z
Learning: The CORS configuration in the unraid API is intentionally set to allow all origins (`origin: true`). This was a deliberate architectural decision when removing the previous custom CORS implementation and implementing helmet security headers instead.
📚 Learning: 2025-01-15T21:34:00.006Z
Learnt from: pujitm
Repo: unraid/api PR: 1047
File: api/src/unraid-api/graph/sandbox-plugin.ts:57-57
Timestamp: 2025-01-15T21:34:00.006Z
Learning: In the GraphQL sandbox (api/src/unraid-api/graph/sandbox-plugin.ts), CSRF token validation should fail silently with a fallback value to maintain sandbox accessibility, as it's a development tool where strict security measures aren't required.
Applied to files:
web/src/helpers/create-apollo-client.ts
📚 Learning: 2025-03-10T17:24:06.914Z
Learnt from: mdatelle
Repo: unraid/api PR: 1219
File: api/src/unraid-api/auth/cookie.strategy.ts:19-20
Timestamp: 2025-03-10T17:24:06.914Z
Learning: In the auth system, CSRF token validation and cookie validation have been unified in the `validateCookiesCasbin()` method in the AuthService class, which takes the entire FastifyRequest object and performs both validations sequentially.
Applied to files:
web/src/helpers/create-apollo-client.ts
📚 Learning: 2025-09-04T18:42:53.531Z
Learnt from: pujitm
Repo: unraid/api PR: 1658
File: plugin/plugins/dynamix.unraid.net.plg:73-79
Timestamp: 2025-09-04T18:42:53.531Z
Learning: In the dynamix.unraid.net plugin, versions 6.12.1-6.12.14 and 6.12.15 prereleases are intentionally allowed to install with warnings (rather than immediate cleanup) to provide users with a grace period and notice before functionality is completely removed. This is a deliberate UX decision to avoid immediately breaking existing setups while encouraging upgrades.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-05-08T19:28:54.365Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh:19-24
Timestamp: 2025-05-08T19:28:54.365Z
Learning: The directory `/usr/local/emhttp/plugins/dynamix.my.servers` is a valid directory that exists as part of the Unraid API plugin installation and should be included in verification checks.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-02-04T17:21:39.710Z
Learnt from: mdatelle
Repo: unraid/api PR: 1106
File: unraid-ui/src/components/index.ts:2-2
Timestamp: 2025-02-04T17:21:39.710Z
Learning: The unraid-ui package is undergoing a major refactoring process, and breaking changes are expected during this transition period.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-09-04T15:26:34.416Z
Learnt from: elibosley
Repo: unraid/api PR: 1657
File: web/scripts/deploy-dev.sh:37-41
Timestamp: 2025-09-04T15:26:34.416Z
Learning: In web/scripts/deploy-dev.sh, the command `rm -rf /usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/*` intentionally removes all contents of the unraid-components directory before deploying standalone components. This broader cleanup is desired behavior according to the maintainer elibosley.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-06-11T14:14:30.348Z
Learnt from: pujitm
Repo: unraid/api PR: 1415
File: plugin/plugins/dynamix.unraid.net.plg:234-236
Timestamp: 2025-06-11T14:14:30.348Z
Learning: For the Unraid Connect plugin, the script `/etc/rc.d/rc.unraid-api` is bundled with the plugin package itself, so its presence on the target system is guaranteed during installation.
Applied to files:
plugin/plugins/dynamix.unraid.net.plgapi/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
📚 Learning: 2025-05-08T19:31:52.417Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/verify_install.sh:11-16
Timestamp: 2025-05-08T19:31:52.417Z
Learning: The `dynamix.my.servers` namespace is still valid and should not be changed to `dynamix.unraid.net` in file paths, as both namespaces coexist in the codebase.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-02-05T21:10:48.136Z
Learnt from: elibosley
Repo: unraid/api PR: 1120
File: plugin/plugins/dynamix.unraid.net.plg:35-38
Timestamp: 2025-02-05T21:10:48.136Z
Learning: When providing error handling guidance for Unraid plugins, direct users to use the web GUI (Plugins > Installed Plugins) rather than suggesting command-line actions.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-03-27T13:34:53.438Z
Learnt from: pujitm
Repo: unraid/api PR: 1252
File: api/src/environment.ts:56-56
Timestamp: 2025-03-27T13:34:53.438Z
Learning: For critical components in the Unraid API, such as retrieving version information from package.json, failing fast (allowing crashes) is preferred over graceful degradation with fallback values.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-01-29T00:59:26.633Z
Learnt from: zackspear
Repo: unraid/api PR: 1079
File: web/scripts/deploy-dev.sh:51-54
Timestamp: 2025-01-29T00:59:26.633Z
Learning: For the Unraid web components deployment process, JS file validation isn't required in auth-request.php updates since the files come from the controlled build pipeline where we are the source.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-05-07T16:07:47.236Z
Learnt from: elibosley
Repo: unraid/api PR: 1381
File: plugin/source/dynamix.unraid.net/usr/local/share/dynamix.unraid.net/install/scripts/setup_api.sh:107-113
Timestamp: 2025-05-07T16:07:47.236Z
Learning: The Unraid API is designed to handle missing configuration files gracefully with smart internal fallbacks rather than requiring installation scripts to create default configurations.
Applied to files:
plugin/plugins/dynamix.unraid.net.plg
📚 Learning: 2025-02-03T18:57:53.577Z
Learnt from: elibosley
Repo: unraid/api PR: 1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/generic-modification.spec.ts:80-80
Timestamp: 2025-02-03T18:57:53.577Z
Learning: In the unraid/api codebase, patch files generated for file modifications always use the .patch extension, regardless of the source file type being modified.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
📚 Learning: 2025-02-03T17:21:26.738Z
Learnt from: elibosley
Repo: unraid/api PR: 1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-02-03T17:21:26.738Z
Learning: The project uses patches to override existing Unraid pages rather than modifying them directly.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
📚 Learning: 2025-03-07T17:35:50.406Z
Learnt from: mdatelle
Repo: unraid/api PR: 1219
File: api/src/unraid-api/main.ts:32-55
Timestamp: 2025-03-07T17:35:50.406Z
Learning: Helmet security configuration in the Unraid API is intentionally relaxed (with disabled CSP, CORS policies, and HSTS) to maintain compatibility with existing Unraid plugins. Stricter security settings might break current plugin functionality.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch
📚 Learning: 2025-03-12T13:48:14.850Z
Learnt from: pujitm
Repo: unraid/api PR: 1211
File: web/composables/gql/gql.ts:17-18
Timestamp: 2025-03-12T13:48:14.850Z
Learning: In the Unraid API project, the duplicate GraphQL query and mutation strings in gql.ts files are intentionally generated by GraphQL CodeGen tool and are necessary for the type system to function properly.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/rc.nginx.modified.snapshotapi/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
📚 Learning: 2025-09-02T19:35:14.427Z
Learnt from: elibosley
Repo: unraid/api PR: 1639
File: web/components/standalone-mount.ts:38-54
Timestamp: 2025-09-02T19:35:14.427Z
Learning: In the unraid/api project, exposing Apollo client and GraphQL utilities (apolloClient, gql, graphqlParse) on the global window object is intentional to allow developers to interact with GraphQL from the browser console without needing to be inside Vue components. Developer experience is prioritized over minimizing global exposure in this context.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/rc.nginx.modified.snapshot
📚 Learning: 2025-01-29T16:35:43.699Z
Learnt from: elibosley
Repo: unraid/api PR: 1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:39-41
Timestamp: 2025-01-29T16:35:43.699Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations, including detailed error logging with stack traces and modification IDs. Individual FileModification implementations should focus on their core functionality without duplicating error handling.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
📚 Learning: 2025-01-29T16:36:04.777Z
Learnt from: elibosley
Repo: unraid/api PR: 1082
File: api/src/unraid-api/unraid-file-modifier/modifications/log-rotate.modification.ts:33-37
Timestamp: 2025-01-29T16:36:04.777Z
Learning: The UnraidFileModifierService in the Unraid API provides comprehensive error handling for all FileModification implementations. It includes try/catch blocks, detailed error logging, and safe rollback mechanisms. Individual FileModification implementations (like LogRotateModification) should allow errors to propagate to this service layer rather than handling them internally.
Applied to files:
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts
🔇 Additional comments (11)
plugin/plugins/dynamix.unraid.net.plg (5)
12-15: LGTM!The entity definitions for the Unraid Core package follow a consistent pattern with the existing API package definitions, including proper SHA256 verification and versioned naming.
59-63: LGTM!The download definition for the Unraid Core package mirrors the existing API package download pattern with proper SHA256 integrity verification.
432-434: LGTM!Variable definitions follow the established naming convention and mirror the API package variables above.
573-584: Consider cleanup on core installation failure.If the core package installation fails (lines 573-576) or the core package file is missing (lines 582-583), the script exits with an error. However, at this point the API package has already been installed successfully. This could leave the system in a partially installed state.
Consider whether the installation should be atomic (rolling back the API package on core failure) or if this partial state is acceptable for troubleshooting purposes.
661-667: LGTM!The conditional startup of the Unraid Core service with a warning fallback is appropriate. The service startup order (API first, then Core) aligns with the dependency structure.
api/src/unraid-api/unraid-file-modifier/modifications/patches/rc-nginx.patch (3)
19-40: Well-structured remote access check function.The
check_remote_access()function properly:
- Verifies the API utils script exists before sourcing
- Checks plugin enablement via the utility script
- Parses JSON config with jq, handling missing commands gracefully
- Validates both
wanaccessandusernamefields correctly (including the"null"string check for jq -r output)
69-96: GraphQL routing split correctly handles the migration.The routing changes appropriately:
/graphql/api→unraid-api.sockpreserves access to the legacy API/graphql→unraid-core.sockroutes to the new core service- WebSocket upgrade detection with rewrite to
/graphql/socketfollows the expected patternThe
if ($http_upgrade = "websocket")usage inside a location block is acceptable here since it only performs a simple rewrite withbreak, avoiding the typical pitfalls of nginx'sifdirective.
110-125: Correct fix for bash array iteration.Quoting the array expansion
"${!NET_FQDN[@]}"prevents word splitting on keys containing special characters.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/rc.nginx.modified.snapshot (1)
386-454: Snapshot correctly captures the new proxy configurations.The test snapshot properly reflects:
- SSO endpoint at
/auth/sso(lines 386-395)- GraphQL API endpoint at
/graphql/api(lines 431-439)- Updated
/graphqllocation with websocket upgrade handling (lines 440-454)This provides good regression coverage for the nginx configuration modifications.
api/src/unraid-api/unraid-file-modifier/modifications/rc-nginx.modification.ts (2)
76-90: Good use of idempotent injection pattern.The guards (
!newContent.includes('location /auth/sso')and!newContent.includes('location /graphql/api')) ensure the blocks are only injected once, making the modification safe to re-run.
114-121: Clarify OS version handling.The class docstring states this patch is for "< Unraid 7.2.0", but
checkOsVersion: falsemeans it will apply on all versions. If Unraid 7.2.0+ already includes these changes natively (via the backported webgui PR), the idempotent checks should prevent double-injection, but the patch will still run unnecessarily.Is
checkOsVersion: falseintentional to ensure the patch always applies for safety, or should this be restored to check the OS version and skip on 7.2.0+?
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pujitm
changed the title
pujitm
force-pushed
the
feat/substitute-graphql
branch
from
c3b1b86 to
d77b2b7
Compare
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/src/unraid-api/unraid-file-modifier/modifications/patches/sso.patch (1)
20-24: Unescaped dots in JWT regex allow invalid token formats.The regex
/^[A-Za-z0-9-_]+.[A-Za-z0-9-_]+.[A-Za-z0-9-_]+$/uses.which matches any character, not just literal dots. This would accept tokens likeabc1def2ghias valid.🐛 Proposed fix - escape the dots
- if (!preg_match('/^[A-Za-z0-9-_]+.[A-Za-z0-9-_]+.[A-Za-z0-9-_]+$/', $password)) { + if (!preg_match('/^[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+$/', $password)) {api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/.login.php.modified.snapshot.php (1)
16-16: Snapshot contains the same regex bug - unescaped dots.This snapshot should be regenerated after fixing the regex in the source. The test will correctly fail until both are aligned.
🤖 Fix all issues with AI agents
In
`@api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/.login.php.modified.snapshot.php`:
- Around line 45-47: The snapshot shows preg_replace using an empty replacement
which loses the captured status code; update the source where $code is set (the
preg_replace call that reads $http_response_header[0]) to use the captured group
('$1') as the replacement so the HTTP status code is extracted correctly, then
regenerate the .login.php.modified.snapshot.php snapshot to reflect the
corrected output.
In `@api/src/unraid-api/unraid-file-modifier/modifications/patches/sso.patch`:
- Around line 50-52: The HTTP status extraction is wrong because preg_replace
with an empty replacement removes the match and yields an empty string; update
the logic that sets $code (which currently references $http_response_header[0]
and uses preg_replace('/^HTTP\/[0-9.]+\s+(\d+).*/', '', ...)) to instead capture
the group — either use preg_replace with '$1' as the replacement or use
preg_match to extract the first capture and cast that to int — so $code becomes
the numeric HTTP status code.
In `@plugin/plugins/dynamix.unraid.net.plg`:
- Around line 559-596: The cleanup loop uses a broad hardcoded glob; change it
to derive the directory from CORE_PKG_FILE and iterate that directory only so
you don't accidentally match files elsewhere. Replace the for loop over
/boot/config/plugins/dynamix.my.servers/unraid-*.txz with a loop using
core_dir="$(dirname "$CORE_PKG_FILE")" and for txz_file in
"$core_dir"/unraid-*.txz; keep the existing if [ -f "$txz_file" ] && [
"$txz_file" != "${CORE_PKG_FILE}" ] check and rm -f to remove stale packages.
♻️ Duplicate comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
339-346: Use a more specific pattern for core package matching.The glob pattern
unraid-*could match unrelated packages. Since the core package follows a version-based naming convention (e.g.,unraid-0.1.0-2026.01.20.1), use a more specific pattern.🔧 Suggested fix
- core_pkg_installed=$(ls -1 /var/log/packages/unraid-* 2>/dev/null | head -1) + core_pkg_installed=$(ls -1 /var/log/packages/unraid-[0-9]* 2>/dev/null | head -1)
🧹 Nitpick comments (4)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid (2)
19-23: Secret key generation looks good, but consider stricter permissions on creation.The 64-byte base64-encoded key from
/dev/urandomis cryptographically sound. However, there's a brief window between file creation andchmod 600where the file may have default permissions (typically 644).♻️ Suggested improvement using umask
# Generate secret_key_base if not exists if [ ! -f "$CONFIG_DIR/secret_key_base" ]; then + (umask 077 && head -c 64 /dev/urandom | base64 | tr -d '\n' > "$CONFIG_DIR/secret_key_base") - head -c 64 /dev/urandom | base64 | tr -d '\n' > "$CONFIG_DIR/secret_key_base" - chmod 600 "$CONFIG_DIR/secret_key_base" fi
44-49: Consider verifying the daemon started successfully.The
daemoncommand returns immediately; a failed start may not be detected. A brief delay followed by a PID check would provide better feedback.♻️ Optional improvement
start() { echo -n "Starting Unraid... " [ -S "$SOCKET_PATH" ] && rm -f "$SOCKET_PATH" "$RELEASE_BIN" daemon + sleep 1 + if "$RELEASE_BIN" pid >/dev/null 2>&1; then + echo "done" + else + echo "failed" + return 1 + fi - echo "done" }web/__test__/components/SsoButton.test.ts (1)
425-469: Consider usingafterEachortry/finallyfor cleanup to ensure restoration on test failure.If an assertion fails before line 468,
URLSearchParamsremains stubbed, potentially breaking subsequent tests.♻️ Suggested improvement
it('handles unexpected callback errors', async () => { + const originalURLSearchParams = globalThis.URLSearchParams; const mockProviders = [ { id: 'unraid-net', name: 'Unraid.net', buttonText: 'Log In With Unraid.net', }, ]; mockUseQuery.mockReturnValue({ result: { value: { publicOidcProviders: mockProviders } }, refetch: vi.fn().mockResolvedValue({ data: { publicOidcProviders: mockProviders } }), }); - const originalURLSearchParams = globalThis.URLSearchParams; - vi.stubGlobal( - 'URLSearchParams', - vi.fn(() => { - throw new Error('boom'); - }) - ); - - mockLocation.search = ''; - mockLocation.hash = ''; - mockLocation.pathname = '/login'; - - const wrapper = mount(SsoButtons, { - global: { - plugins: [createTestI18n()], - stubs: { - SsoProviderButton: SsoProviderButtonStub, - Button: { template: '<button><slot /></button>' }, - }, - }, - }); - - await flushPromises(); - - const errorElement = wrapper.find('p.text-red-500'); - expect(errorElement.exists()).toBe(true); - expect(errorElement.text()).toBe('Error fetching token'); - expect(mockForm.style.display).toBe('block'); - - vi.stubGlobal('URLSearchParams', originalURLSearchParams); + try { + vi.stubGlobal( + 'URLSearchParams', + vi.fn(() => { + throw new Error('boom'); + }) + ); + + mockLocation.search = ''; + mockLocation.hash = ''; + mockLocation.pathname = '/login'; + + const wrapper = mount(SsoButtons, { + global: { + plugins: [createTestI18n()], + stubs: { + SsoProviderButton: SsoProviderButtonStub, + Button: { template: '<button><slot /></button>' }, + }, + }, + }); + + await flushPromises(); + + const errorElement = wrapper.find('p.text-red-500'); + expect(errorElement.exists()).toBe(true); + expect(errorElement.text()).toBe('Error fetching token'); + expect(mockForm.style.display).toBe('block'); + } finally { + vi.stubGlobal('URLSearchParams', originalURLSearchParams); + } });api/src/unraid-api/unraid-file-modifier/modifications/patches/sso.patch (1)
65-73:json_decodedoesn't throw exceptions withoutJSON_THROW_ON_ERRORflag.The try/catch is effectively dead code since
json_decodereturnsnullon error rather than throwing. The current logic still works becauseisset($decoded['valid'])handles the null case, but the catch block creates a false sense of error handling.♻️ Suggested clarification
Either remove the try/catch (since the null case is handled) or add the flag:
try { - $decoded = json_decode($response, true); + $decoded = json_decode($response, true, 512, JSON_THROW_ON_ERROR); if (isset($decoded['valid']) && $decoded['valid'] === true) { return true; } - } catch (Exception $e) { + } catch (JsonException $e) { my_logger("SSO Login Attempt Exception: " . $e->getMessage()); return false; }
| if (isset($http_response_header[0])) { | ||
| $code = (int) preg_replace('/^HTTP\/[0-9.]+\s+(\d+).*/', '', $http_response_header[0]); | ||
| } |
There was a problem hiding this comment.
Snapshot contains incorrect preg_replace replacement string.
Same issue as the patch file - uses '' instead of '$1'. Regenerate this snapshot after fixing the source.
🤖 Prompt for AI Agents
In
`@api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/.login.php.modified.snapshot.php`
around lines 45 - 47, The snapshot shows preg_replace using an empty replacement
which loses the captured status code; update the source where $code is set (the
preg_replace call that reads $http_response_header[0]) to use the captured group
('$1') as the replacement so the HTTP status code is extracted correctly, then
regenerate the .login.php.modified.snapshot.php snapshot to reflect the
corrected output.
| + if (isset($http_response_header[0])) { | ||
| + $code = (int) preg_replace('/^HTTP\/[0-9.]+\s+(\d+).*/', '', $http_response_header[0]); | ||
| + } |
There was a problem hiding this comment.
HTTP status code extraction returns empty string instead of the status code.
The preg_replace uses '' as the replacement, which removes the entire match instead of extracting the captured group. This will always set $code to 0.
🐛 Proposed fix
if (isset($http_response_header[0])) {
- $code = (int) preg_replace('/^HTTP\/[0-9.]+\s+(\d+).*/', '', $http_response_header[0]);
+ $code = (int) preg_replace('/^HTTP\/[0-9.]+\s+(\d+).*/', '$1', $http_response_header[0]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + if (isset($http_response_header[0])) { | |
| + $code = (int) preg_replace('/^HTTP\/[0-9.]+\s+(\d+).*/', '', $http_response_header[0]); | |
| + } | |
| if (isset($http_response_header[0])) { | |
| $code = (int) preg_replace('/^HTTP\/[0-9.]+\s+(\d+).*/', '$1', $http_response_header[0]); | |
| } |
🤖 Prompt for AI Agents
In `@api/src/unraid-api/unraid-file-modifier/modifications/patches/sso.patch`
around lines 50 - 52, The HTTP status extraction is wrong because preg_replace
with an empty replacement removes the match and yields an empty string; update
the logic that sets $code (which currently references $http_response_header[0]
and uses preg_replace('/^HTTP\/[0-9.]+\s+(\d+).*/', '', ...)) to instead capture
the group — either use preg_replace with '$1' as the replacement or use
preg_match to extract the first capture and cast that to int — so $code becomes
the numeric HTTP status code.
| # Install Unraid Core package | ||
| if [ -f "$CORE_PKG_FILE" ]; then | ||
| echo "Installing Unraid Core package..." | ||
| # Clean up any old core package txz files if they don't match our current version | ||
| for txz_file in /boot/config/plugins/dynamix.my.servers/unraid-*.txz; do | ||
| if [ -f "$txz_file" ] && [ "$txz_file" != "${CORE_PKG_FILE}" ]; then | ||
| echo "Removing old core package file: $txz_file" | ||
| rm -f "$txz_file" | ||
| fi | ||
| done | ||
| # Stop the core service before mutating /usr/local/unraid | ||
| if [ -x "/etc/rc.d/rc.unraid" ]; then | ||
| echo "Stopping Unraid Core service before upgrade..." | ||
| /etc/rc.d/rc.unraid stop || echo "Warning: Failed to stop Unraid Core service" | ||
| fi | ||
| upgradepkg --install-new --reinstall "${CORE_PKG_FILE}" | ||
| if [ $? -ne 0 ]; then | ||
| echo "⚠️ Core package installation failed" | ||
| if [ "$API_PKG_INSTALLED" -eq 1 ]; then | ||
| echo "⚠️ Unraid API package was installed; leaving it in place for troubleshooting." | ||
| echo " Re-run install or uninstall via Plugins > Installed Plugins to rollback." | ||
| fi | ||
| exit 1 | ||
| fi | ||
| if [ -f "/etc/rc.d/rc.unraid" ]; then | ||
| chmod +x /etc/rc.d/rc.unraid | ||
| fi | ||
| else | ||
| echo "⚠️ Core package file not found: $CORE_PKG_FILE" | ||
| if [ "$API_PKG_INSTALLED" -eq 1 ]; then | ||
| echo "⚠️ Unraid API package was installed; leaving it in place for troubleshooting." | ||
| echo " Re-run install or uninstall via Plugins > Installed Plugins to rollback." | ||
| fi | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Core installation logic is sound, but apply the same glob pattern fix.
The installation flow correctly stops the service before upgrade and handles failures gracefully with user-friendly guidance. However, the cleanup glob at line 563 has the same broad pattern issue.
🔧 Suggested fix for cleanup glob
- for txz_file in /boot/config/plugins/dynamix.my.servers/unraid-*.txz; do
+ for txz_file in /boot/config/plugins/dynamix.my.servers/unraid-[0-9]*.txz; doBased on learnings, the error handling correctly directs users to use the web GUI (Plugins > Installed Plugins) for recovery.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Install Unraid Core package | |
| if [ -f "$CORE_PKG_FILE" ]; then | |
| echo "Installing Unraid Core package..." | |
| # Clean up any old core package txz files if they don't match our current version | |
| for txz_file in /boot/config/plugins/dynamix.my.servers/unraid-*.txz; do | |
| if [ -f "$txz_file" ] && [ "$txz_file" != "${CORE_PKG_FILE}" ]; then | |
| echo "Removing old core package file: $txz_file" | |
| rm -f "$txz_file" | |
| fi | |
| done | |
| # Stop the core service before mutating /usr/local/unraid | |
| if [ -x "/etc/rc.d/rc.unraid" ]; then | |
| echo "Stopping Unraid Core service before upgrade..." | |
| /etc/rc.d/rc.unraid stop || echo "Warning: Failed to stop Unraid Core service" | |
| fi | |
| upgradepkg --install-new --reinstall "${CORE_PKG_FILE}" | |
| if [ $? -ne 0 ]; then | |
| echo "⚠️ Core package installation failed" | |
| if [ "$API_PKG_INSTALLED" -eq 1 ]; then | |
| echo "⚠️ Unraid API package was installed; leaving it in place for troubleshooting." | |
| echo " Re-run install or uninstall via Plugins > Installed Plugins to rollback." | |
| fi | |
| exit 1 | |
| fi | |
| if [ -f "/etc/rc.d/rc.unraid" ]; then | |
| chmod +x /etc/rc.d/rc.unraid | |
| fi | |
| else | |
| echo "⚠️ Core package file not found: $CORE_PKG_FILE" | |
| if [ "$API_PKG_INSTALLED" -eq 1 ]; then | |
| echo "⚠️ Unraid API package was installed; leaving it in place for troubleshooting." | |
| echo " Re-run install or uninstall via Plugins > Installed Plugins to rollback." | |
| fi | |
| exit 1 | |
| fi | |
| # Install Unraid Core package | |
| if [ -f "$CORE_PKG_FILE" ]; then | |
| echo "Installing Unraid Core package..." | |
| # Clean up any old core package txz files if they don't match our current version | |
| for txz_file in /boot/config/plugins/dynamix.my.servers/unraid-[0-9]*.txz; do | |
| if [ -f "$txz_file" ] && [ "$txz_file" != "${CORE_PKG_FILE}" ]; then | |
| echo "Removing old core package file: $txz_file" | |
| rm -f "$txz_file" | |
| fi | |
| done | |
| # Stop the core service before mutating /usr/local/unraid | |
| if [ -x "/etc/rc.d/rc.unraid" ]; then | |
| echo "Stopping Unraid Core service before upgrade..." | |
| /etc/rc.d/rc.unraid stop || echo "Warning: Failed to stop Unraid Core service" | |
| fi | |
| upgradepkg --install-new --reinstall "${CORE_PKG_FILE}" | |
| if [ $? -ne 0 ]; then | |
| echo "⚠️ Core package installation failed" | |
| if [ "$API_PKG_INSTALLED" -eq 1 ]; then | |
| echo "⚠️ Unraid API package was installed; leaving it in place for troubleshooting." | |
| echo " Re-run install or uninstall via Plugins > Installed Plugins to rollback." | |
| fi | |
| exit 1 | |
| fi | |
| if [ -f "/etc/rc.d/rc.unraid" ]; then | |
| chmod +x /etc/rc.d/rc.unraid | |
| fi | |
| else | |
| echo "⚠️ Core package file not found: $CORE_PKG_FILE" | |
| if [ "$API_PKG_INSTALLED" -eq 1 ]; then | |
| echo "⚠️ Unraid API package was installed; leaving it in place for troubleshooting." | |
| echo " Re-run install or uninstall via Plugins > Installed Plugins to rollback." | |
| fi | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In `@plugin/plugins/dynamix.unraid.net.plg` around lines 559 - 596, The cleanup
loop uses a broad hardcoded glob; change it to derive the directory from
CORE_PKG_FILE and iterate that directory only so you don't accidentally match
files elsewhere. Replace the for loop over
/boot/config/plugins/dynamix.my.servers/unraid-*.txz with a loop using
core_dir="$(dirname "$CORE_PKG_FILE")" and for txz_file in
"$core_dir"/unraid-*.txz; keep the existing if [ -f "$txz_file" ] && [
"$txz_file" != "${CORE_PKG_FILE}" ] check and rm -f to remove stale packages.
![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.
Reviewed commit: 8fdb128e23
ℹ️ 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".
| async shouldApply(): Promise<ShouldApplyWithReason> { | ||
| const { shouldApply, reason } = await super.shouldApply(); | ||
| const { shouldApply, reason } = await super.shouldApply({ checkOsVersion: false }); | ||
| return { |
There was a problem hiding this comment.
By forcing shouldApply to ignore the OS version, this modification now runs on systems where rc.nginx already contains all of these changes (e.g., Unraid 7.2+). In that case every replace in generatePatch is a no-op and createPatchWithDiff returns a patch with zero hunks, which FileModification.apply() treats as invalid and throws. The result is a hard failure during install/upgrade on up‑to‑date systems. Consider keeping the version gate or explicitly short‑circuiting when newContent === fileContent before returning a patch.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.