Replace feedback button with FeatureOS widget#2568
Merged
Conversation
elibosley
force-pushed
the
feat/replace-feedback-button-with-widget
branch
from
241626e to
9a3cd8b
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces deployment automation infrastructure, refactors authorization logic with canonicalized path validation, implements asynchronous widget initialization for the feedback system, and adds comprehensive test coverage for authentication and configuration initialization flows across five new/modified files. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FeedbackButton as FeedbackButton Handler
participant WidgetScript as FeatureOS Widget Script
participant WidgetInstance as Widget Instance
participant UI as Page UI
User->>FeedbackButton: Click feedback button
FeedbackButton->>FeedbackButton: Check if script loaded
alt Script not yet loaded
FeedbackButton->>WidgetScript: Fetch and cache script
activate WidgetScript
WidgetScript-->>FeedbackButton: Script ready
deactivate WidgetScript
end
FeedbackButton->>FeedbackButton: Build widget options (prerelease, SSO token)
FeedbackButton->>WidgetInstance: Initialize widget instance
activate WidgetInstance
WidgetInstance-->>FeedbackButton: Instance created
deactivate WidgetInstance
FeedbackButton->>UI: Update loading state (complete)
FeedbackButton->>WidgetInstance: Open widget dialog
WidgetInstance->>User: Display feedback form
alt Load/Init fails
FeedbackButton->>UI: Log error & show alert
FeedbackButton->>UI: Clear loading state
end
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🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/auth_request_canonical_path_test.php (1)
92-118: Please add a regression case for the new external asset mapping.This file exercises
getCanonicalRequestUri(), but the PR also changedisAllowedPublicAssetRequest()for/webGui/images/case-model.png→/boot/config/plugins/dynamix/case-model.png. A focused test for that branch would keep the custom case-image fix covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth_request_canonical_path_test.php` around lines 92 - 118, Add a regression test that exercises the new external asset mapping by invoking isAllowedPublicAssetRequest()/getCanonicalRequestUri for the specific mapped path: set $_SERVER['REQUEST_URI'] to '/webGui/images/case-model.png' and assert that the function returns the mapped canonical path '/boot/config/plugins/dynamix/case-model.png' (and also test the negative case if appropriate). Locate the test near existing canonical-path cases in tests/auth_request_canonical_path_test.php and add assertions that call getCanonicalRequestUri($canonicalDocroot) and/or isAllowedPublicAssetRequest(...) referencing those exact URIs to ensure the custom case-image mapping remains covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.codex/coderabbit-fixes-wip.md:
- Around line 5-10: The .codex/coderabbit-fixes-wip.md entry contains metadata
from a different PR (branch fix/persist-rclone-configs-7.2, PR 2550) that was
accidentally committed; remove this entire .codex/coderabbit-fixes-wip.md file
or delete the pinned block (the lines listing Repo, Branch, PR, PR URL,
Generated at) so the unrelated bookkeeping is not merged into the current
changeset, and commit the cleaned file (or delete the file) so only intentional
artifacts remain.
In `@deploy_to_unraid.sh`:
- Around line 106-108: The remote backup currently runs fire-and-forget; change
the ssh command so you first check existence of TARGET_PATH on TARGET_HOST, then
perform cp via positional parameters (e.g., use 'sh -s -- "$TARGET_PATH"
"$BACKUP_PATH"' or similar) and capture the cp exit status, exiting the local
script if the remote cp returns non-zero; update the commands that build
BACKUP_PATH and BACKUP_PARENT so you run: ssh "$TARGET_HOST" "if [ -f \"$1\" ];
then cp \"$1\" \"$2\" || exit 1; fi" (passing TARGET_PATH and BACKUP_PATH as
positional args) and ensure the local script aborts when ssh returns non-zero.
- Around line 74-76: The current BACKUP_DIR uses TARGET_EMHTTP which places
backups under the served docroot; change BACKUP_DIR to point outside
TARGET_EMHTTP (e.g., introduce TARGET_BACKUP_ROOT or default to a non-served
location like /var/backups or /mnt/user/backups) and use that when creating the
directory and SSH mkdir command (update
BACKUP_DIR="$TARGET_BACKUP_ROOT/backups/$(date +%Y%m%d_%H%M%S)" and the ssh
"mkdir -p '$BACKUP_DIR'" invocation); ensure TARGET_BACKUP_ROOT is configurable
and falls back to a safe non-docroot path so backups are never written under
TARGET_EMHTTP.
In `@emhttp/plugins/dynamix/FeedbackButton.page`:
- Around line 37-54: The retry hangs because when a script load fails the failed
<script data-featureos-feedback-widget> element stays in the DOM and future
attempts reuse it; update the failure path so that when the load errors you
remove the stale script element before clearing
featureOsFeedbackWidgetScriptPromise (look for the
existingScript/querySelector('script[data-featureos-feedback-widget]') logic and
the .catch block) — remove the DOM node (existingScript.remove() or
parent.removeChild(existingScript)) on error, then clear
featureOsFeedbackWidgetScriptPromise and rethrow so subsequent attempts create a
fresh <script> and attach new load/error listeners.
---
Nitpick comments:
In `@tests/auth_request_canonical_path_test.php`:
- Around line 92-118: Add a regression test that exercises the new external
asset mapping by invoking isAllowedPublicAssetRequest()/getCanonicalRequestUri
for the specific mapped path: set $_SERVER['REQUEST_URI'] to
'/webGui/images/case-model.png' and assert that the function returns the mapped
canonical path '/boot/config/plugins/dynamix/case-model.png' (and also test the
negative case if appropriate). Locate the test near existing canonical-path
cases in tests/auth_request_canonical_path_test.php and add assertions that call
getCanonicalRequestUri($canonicalDocroot) and/or
isAllowedPublicAssetRequest(...) referencing those exact URIs to ensure the
custom case-image mapping remains covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: caf7b59e-8593-475a-8b08-65f789bd07e4
📒 Files selected for processing (6)
.codex/coderabbit-fixes-wip.mddeploy_to_unraid.shemhttp/auth-request.phpemhttp/plugins/dynamix/FeedbackButton.pagetests/auth_request_canonical_path_test.phptests/rclone_config_init_test.sh
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/FeedbackButton.page (1)
117-123: Redundant loading state reset.Line 119 calls
featureOsFeedbackSetLoading(false)butfeatureOsFeedbackWidget()already clears the loading state in itsfinallyblock (line 106). This is harmless but redundant.♻️ Suggested cleanup
.catch(function(error) { console.error('Unable to load FeatureOS feedback widget.', error); - featureOsFeedbackSetLoading(false); if (typeof swal === 'function') { swal('<?=addslashes(_('Feedback'))?>', '<?=addslashes(_('Unable to load feedback widget.'))?>', 'error'); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/FeedbackButton.page` around lines 117 - 123, The catch block in the featureOsFeedbackWidget flow redundantly calls featureOsFeedbackSetLoading(false) even though featureOsFeedbackWidget already clears loading in its finally block; remove the featureOsFeedbackSetLoading(false) invocation from the catch handler so the finally block is the single place that resets loading (keep the console.error and swal error UI logic intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@emhttp/plugins/dynamix/FeedbackButton.page`:
- Around line 117-123: The catch block in the featureOsFeedbackWidget flow
redundantly calls featureOsFeedbackSetLoading(false) even though
featureOsFeedbackWidget already clears loading in its finally block; remove the
featureOsFeedbackSetLoading(false) invocation from the catch handler so the
finally block is the single place that resets loading (keep the console.error
and swal error UI logic intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a2144193-c626-4ecb-9319-81a58974c9eb
📒 Files selected for processing (1)
emhttp/plugins/dynamix/FeedbackButton.page
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/FeedbackButton.page (1)
36-54:⚠️ Potential issue | 🟠 MajorRemove the failed widget
<script>before allowing retries.This still hangs after the first load failure. Lines 51-53 clear
featureOsFeedbackWidgetScriptPromise, but the failedscript[data-featureos-feedback-widget]remains in the DOM, so the next attempt reattaches listeners to a dead node and never resolves.🔁 Suggested fix
}).catch(function(error) { + var staleScript = document.querySelector('script[data-featureos-feedback-widget]'); + if (staleScript && !window.HellonextWidget) staleScript.remove(); featureOsFeedbackWidgetScriptPromise = null; throw error; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/FeedbackButton.page` around lines 36 - 54, The promise catch handler should remove the failed script element from the DOM before clearing featureOsFeedbackWidgetScriptPromise so retries can re-insert a fresh <script>; update the catch block for featureOsFeedbackWidgetScriptPromise to locate document.querySelector('script[data-featureos-feedback-widget]') (or reuse existingScript if in scope), remove that element from its parent if present, then set featureOsFeedbackWidgetScriptPromise = null and rethrow the error so subsequent attempts can create a new script node and attach fresh listeners.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@emhttp/plugins/dynamix/FeedbackButton.page`:
- Around line 36-54: The promise catch handler should remove the failed script
element from the DOM before clearing featureOsFeedbackWidgetScriptPromise so
retries can re-insert a fresh <script>; update the catch block for
featureOsFeedbackWidgetScriptPromise to locate
document.querySelector('script[data-featureos-feedback-widget]') (or reuse
existingScript if in scope), remove that element from its parent if present,
then set featureOsFeedbackWidgetScriptPromise = null and rethrow the error so
subsequent attempts can create a new script node and attach fresh listeners.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 588b00c4-1038-4218-a56a-5f5c2e6e4a59
📒 Files selected for processing (1)
emhttp/plugins/dynamix/FeedbackButton.page
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/FeedbackButton.page (1)
47-64:⚠️ Potential issue | 🟠 MajorStale script element causes retry to hang indefinitely.
The previous review flagged this: when script load fails, the catch block clears the promise but leaves the failed
<script>element in the DOM. On retry, lines 47-51 attach listeners to this dead element where load/error already fired, so the promise never settles.Additionally, there's a race condition: if an existing script element has already completed loading or erroring before listeners are attached (lines 49-50), the promise will never resolve.
,
🔧 Proposed fix
}).catch(function(error) { featureOsFeedbackWidgetScriptPromise = null; + var staleScript = document.querySelector('script[data-featureos-feedback-widget]'); + if (staleScript && !window.HellonextWidget) staleScript.remove(); throw error; });For the race condition with existing scripts, consider also checking script ready state:
var existingScript = document.querySelector('script[data-featureos-feedback-widget]'); if (existingScript) { + // If script errored previously, remove and create fresh + if (!window.HellonextWidget) { + existingScript.remove(); + } else { - existingScript.addEventListener('load', resolve, {once: true}); - existingScript.addEventListener('error', reject, {once: true}); - return; + resolve(); + return; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix/FeedbackButton.page` around lines 47 - 64, The promise can hang because a stale <script> element remains in the DOM and/or its load/error already fired before listeners attach; change the existingScript handling so that if existingScript is found you first check its ready state (existingScript.readyState === 'loaded' || existingScript.readyState === 'complete' or existingScript.complete) and immediately call resolve, otherwise attach load/error listeners as before; additionally, in the catch block where featureOsFeedbackWidgetScriptPromise is cleared, remove the failed script element from the DOM (query and call remove()) so retries create a fresh <script> element.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@emhttp/plugins/dynamix/FeedbackButton.page`:
- Around line 47-64: The promise can hang because a stale <script> element
remains in the DOM and/or its load/error already fired before listeners attach;
change the existingScript handling so that if existingScript is found you first
check its ready state (existingScript.readyState === 'loaded' ||
existingScript.readyState === 'complete' or existingScript.complete) and
immediately call resolve, otherwise attach load/error listeners as before;
additionally, in the catch block where featureOsFeedbackWidgetScriptPromise is
cleared, remove the failed script element from the DOM (query and call remove())
so retries create a fresh <script> element.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 954336a0-7f8b-475c-b39c-3ebaa08b2948
📒 Files selected for processing (1)
emhttp/plugins/dynamix/FeedbackButton.page
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
deploy_to_unraid.sh (1)
43-47: Appending\nto a shell variable and later usingecho -eis fragile.
echo -ebehavior varies across shells and systems. Consider using an array or newline-separated approach withprintf:♻️ Suggested alternative using printf
ADDITIONAL_FILES="" if [ $# -gt 1 ]; then shift # Remove the target host from arguments for FILE in "$@"; do if [ -f "$FILE" ]; then - ADDITIONAL_FILES="$ADDITIONAL_FILES$FILE\n" + ADDITIONAL_FILES="${ADDITIONAL_FILES}${FILE} +" fi done fiOr use
printf '%s\n' "$FILE"patterns with process substitution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy_to_unraid.sh` around lines 43 - 47, The loop appends literal "\n" into ADDITIONAL_FILES and later relies on echo -e which is fragile; change the implementation to either collect files into an array (e.g., use ADDITIONAL_FILES_ARRAY+=( "$FILE" ) inside the for FILE in "$@" loop and later iterate or join with printf) or build a newline-separated string using printf (e.g., printf '%s\n' "$FILE" >> /tmp/somefile or into a variable via process substitution) so you stop using echo -e and the ADDITIONAL_FILES variable; update all places that read ADDITIONAL_FILES to consume the array or newline-separated output via printf or read -r loop.emhttp/auth-request.php (1)
53-66: Consider adding explicit type handling forrealpathfailure in external asset check.When
realpath()returnsfalse(file doesn't exist), the comparison on line 65 will safely fail sincefalse !== string. However, the intent would be clearer with an explicit check:♻️ Optional clarity improvement
$allowedExternalTargets = getAllowedExternalPublicAssetTargets(); - return isset($allowedExternalTargets[$requestUri]) && - $realRequestPath === $allowedExternalTargets[$requestUri]; + return is_string($realRequestPath) && + isset($allowedExternalTargets[$requestUri]) && + $realRequestPath === $allowedExternalTargets[$requestUri]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/auth-request.php` around lines 53 - 66, The function isAllowedPublicAssetRequest currently relies on implicit behavior when realpath() returns false; make the intent explicit by checking the realpath result before comparing to allowed external targets: in isAllowedPublicAssetRequest, after computing $realRequestPath = realpath(...), ensure you only perform the external-target equality check when $realRequestPath is a string (e.g., is_string($realRequestPath) or $realRequestPath !== false) and return false otherwise; update the final return to use that explicit guard when comparing $realRequestPath to getAllowedExternalPublicAssetTargets()[$requestUri].tests/auth_request_canonical_path_test.php (1)
45-67: Fragile helper extraction relies on source code markers.The
loadAuthRequestHelpersfunction searches for literal strings'function isPathInDocroot'and'// Base whitelist of files'to extract the helper block. If the source file is refactored (e.g., comments changed, functions reordered), this test will silently break.Consider either:
- Extracting helpers into a separate includable file
- Using a more robust extraction method (e.g., PHP tokenizer)
- Adding a structured marker comment like
//@test-helpers-start/ `// `@test-helpers-end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth_request_canonical_path_test.php` around lines 45 - 67, The helper extraction in loadAuthRequestHelpers currently brittlely searches for literal strings ('function isPathInDocroot' and '// Base whitelist of files') to eval a block and can break if the source is refactored; change this by either (A) moving the helper functions (isPathInDocroot, getCanonicalRequestUri, etc.) into a dedicated includable test helpers file and require/include that file from the test, or (B) implement a robust extraction in loadAuthRequestHelpers using the PHP tokenizer (token_get_all) to locate and extract the helper function tokens, or (C) add explicit structured markers in the source like // `@test-helpers-start` and // `@test-helpers-end` and search for those markers instead; update loadAuthRequestHelpers accordingly so it reliably loads getCanonicalRequestUri without fragile string searches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy_to_unraid.sh`:
- Around line 74-77: The remote mkdir command that creates BACKUP_DIR via ssh to
TARGET_HOST lacks error handling; after the ssh "mkdir -p '$BACKUP_DIR'" call
check its exit status ($? or use set -e) and if it failed log a clear error
including TARGET_HOST and BACKUP_DIR and exit non‑zero to avoid continuing the
deployment; update the section around BACKUP_DIR/ssh to validate the ssh/mkdir
result and fail-fast with an explanatory message.
In `@emhttp/plugins/dynamix/FeedbackButton.page`:
- Around line 92-96: The widget may be opened twice via onInitialized and the
.then() handler; to fix, ensure only one path performs open: either clear the
featureOsFeedbackOpenRequested flag immediately after calling
featureOsFeedbackWidgetInstance.open() in onInitialized (set
featureOsFeedbackOpenRequested = false) or add a guard in both the .then()
handler and onInitialized that checks featureOsFeedbackWidgetInstance state
(e.g., only call open() if featureOsFeedbackWidgetInstance exists and is not
already open/initializing) so open() runs exactly once; update references in
onInitialized, featureOsFeedbackOpenRequested, featureOsFeedbackWidgetInstance,
and the featureOsFeedbackWidget().then(...) handler accordingly.
- Around line 46-52: The Promise created in featureOsFeedbackWidgetScriptPromise
can hang if an existingScript has already fired load/error before listeners
attach; update the existingScript handling in the
featureOsFeedbackWidgetScriptPromise logic to first detect whether the script
has already completed (e.g. check existingScript.readyState === 'loaded' ||
existingScript.readyState === 'complete' where available, or a dedicated
data-attribute set when you create the script like
data-featureos-feedback-widget-loaded) and immediately resolve or reject
accordingly, and only attach load/error listeners
(existingScript.addEventListener('load', ...) / 'error') when the script is
still pending.
---
Nitpick comments:
In `@deploy_to_unraid.sh`:
- Around line 43-47: The loop appends literal "\n" into ADDITIONAL_FILES and
later relies on echo -e which is fragile; change the implementation to either
collect files into an array (e.g., use ADDITIONAL_FILES_ARRAY+=( "$FILE" )
inside the for FILE in "$@" loop and later iterate or join with printf) or build
a newline-separated string using printf (e.g., printf '%s\n' "$FILE" >>
/tmp/somefile or into a variable via process substitution) so you stop using
echo -e and the ADDITIONAL_FILES variable; update all places that read
ADDITIONAL_FILES to consume the array or newline-separated output via printf or
read -r loop.
In `@emhttp/auth-request.php`:
- Around line 53-66: The function isAllowedPublicAssetRequest currently relies
on implicit behavior when realpath() returns false; make the intent explicit by
checking the realpath result before comparing to allowed external targets: in
isAllowedPublicAssetRequest, after computing $realRequestPath = realpath(...),
ensure you only perform the external-target equality check when $realRequestPath
is a string (e.g., is_string($realRequestPath) or $realRequestPath !== false)
and return false otherwise; update the final return to use that explicit guard
when comparing $realRequestPath to
getAllowedExternalPublicAssetTargets()[$requestUri].
In `@tests/auth_request_canonical_path_test.php`:
- Around line 45-67: The helper extraction in loadAuthRequestHelpers currently
brittlely searches for literal strings ('function isPathInDocroot' and '// Base
whitelist of files') to eval a block and can break if the source is refactored;
change this by either (A) moving the helper functions (isPathInDocroot,
getCanonicalRequestUri, etc.) into a dedicated includable test helpers file and
require/include that file from the test, or (B) implement a robust extraction in
loadAuthRequestHelpers using the PHP tokenizer (token_get_all) to locate and
extract the helper function tokens, or (C) add explicit structured markers in
the source like // `@test-helpers-start` and // `@test-helpers-end` and search for
those markers instead; update loadAuthRequestHelpers accordingly so it reliably
loads getCanonicalRequestUri without fragile string searches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7976e18c-6bc9-4c80-ae7c-81039820adca
📒 Files selected for processing (4)
deploy_to_unraid.shemhttp/auth-request.phpemhttp/plugins/dynamix/FeedbackButton.pagetests/auth_request_canonical_path_test.php
deploy_to_unraid.sh
Outdated
Comment on lines
+74
to
+77
| # Create backup directory on target | ||
| BACKUP_DIR="$TARGET_BACKUP_ROOT/backups/$(date +%Y%m%d_%H%M%S)" | ||
| echo "📦 Creating backup directory on target..." | ||
| ssh -n "$TARGET_HOST" "mkdir -p '$BACKUP_DIR'" |
Contributor
There was a problem hiding this comment.
Missing error check for remote backup directory creation.
If the SSH connection fails or mkdir fails, the script continues and may produce confusing errors later during file backup/deployment.
🛡️ Suggested fix
# Create backup directory on target
BACKUP_DIR="$TARGET_BACKUP_ROOT/backups/$(date +%Y%m%d_%H%M%S)"
echo "📦 Creating backup directory on target..."
-ssh -n "$TARGET_HOST" "mkdir -p '$BACKUP_DIR'"
+if ! ssh -n "$TARGET_HOST" "mkdir -p '$BACKUP_DIR'"; then
+ echo "❌ Failed to create backup directory on target"
+ exit 1
+fi📝 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.
Suggested change
| # Create backup directory on target | |
| BACKUP_DIR="$TARGET_BACKUP_ROOT/backups/$(date +%Y%m%d_%H%M%S)" | |
| echo "📦 Creating backup directory on target..." | |
| ssh -n "$TARGET_HOST" "mkdir -p '$BACKUP_DIR'" | |
| # Create backup directory on target | |
| BACKUP_DIR="$TARGET_BACKUP_ROOT/backups/$(date +%Y%m%d_%H%M%S)" | |
| echo "📦 Creating backup directory on target..." | |
| if ! ssh -n "$TARGET_HOST" "mkdir -p '$BACKUP_DIR'"; then | |
| echo "❌ Failed to create backup directory on target" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy_to_unraid.sh` around lines 74 - 77, The remote mkdir command that
creates BACKUP_DIR via ssh to TARGET_HOST lacks error handling; after the ssh
"mkdir -p '$BACKUP_DIR'" call check its exit status ($? or use set -e) and if it
failed log a clear error including TARGET_HOST and BACKUP_DIR and exit non‑zero
to avoid continuing the deployment; update the section around BACKUP_DIR/ssh to
validate the ssh/mkdir result and fail-fast with an explanatory message.
🧹 PR Test Plugin Cleaned UpThe test plugin and associated files for this PR have been removed from the preview environment. 🤖 This comment is automatically generated when a PR is closed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests