Storage location redesign#60
Open
chrismin13 wants to merge 27 commits into
Open
Conversation
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:
📝 WalkthroughWalkthroughThis PR introduces a new ChangesStorage and Backup Safety Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 11
🧹 Nitpick comments (2)
scripts/validate_storage_source.py (1)
7-17: ⚡ Quick winDocument the import-path bootstrap intent.
_add_app_to_path()encodes deployment assumptions (repo_root/scriptsand/opt/SimpleSaferServer) that are easy to forget; add a short comment/docstring explaining why these candidates are ordered this way and when this is needed.As per coding guidelines, "Always include comments about things that might be forgotten in a few months or that might not immediately seem obvious on a first read."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate_storage_source.py` around lines 7 - 17, The `_add_app_to_path()` function contains hardcoded deployment paths that lack documentation about their purpose and ordering. Add a docstring or comment block above the function that explains why these specific candidates are checked in order (first the repository structure with script_path.parents[1], then the production deployment path at /opt/SimpleSaferServer), and clarify when this bootstrap mechanism is needed to ensure the simple_safer_server module can be imported. This documentation will help future maintainers understand the deployment assumptions baked into this path manipulation logic.Source: Coding guidelines
tests/test_backup_cloud_script.py (1)
28-83: ⚡ Quick winDocument the harness contract for injected executables/env vars.
A short comment/docstring explaining why
PATH,SSS_PYTHON_BIN, and script stubs are wired this way will prevent accidental breakage during future test edits.As per coding guidelines,
**/*.{js,ts,tsx,jsx,py,java,go,rs,rb,php,cpp,c,h,cs}: “Always include comments about things that might be forgotten in a few months or that might not immediately seem obvious on a first read”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_backup_cloud_script.py` around lines 28 - 83, The _run_script function in tests/test_backup_cloud_script.py is setting up a test harness with injected executables and environment variables (PATH, SSS_PYTHON_BIN, SSS_VALIDATE_STORAGE_SCRIPT, SSS_LOG_ALERT_SCRIPT) and fake command stubs (fake-python, msmtp, rclone, journalctl), but lacks documentation explaining the contract of this setup. Add a comprehensive docstring at the beginning of the _run_script function that explains why these environment variables are being injected, what each fake executable stub represents, how the PATH override enables the test to control which binaries are called, and how this prevents the script from executing real external commands. This will prevent future test modifications from accidentally breaking the harness without understanding its purpose.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cloud_backup.md`:
- Line 54: The documentation in docs/cloud_backup.md and docs/setup.md does not
accurately describe the full storage readiness validation. Update the
existing-folder storage description in docs/cloud_backup.md (around line 54) to
explicitly mention that the app compares the recorded mount identity when a
folder is accessed, and can reject the backup if the folder appears under a
different mount source than when it was originally configured. Similarly, update
the Cloud Backup step description in docs/setup.md (lines 133-134) to reflect
this same contract regarding mount identity validation for existing-folder
storage, ensuring both documents consistently describe that storage readiness
checks include verifying mount identity, not just folder existence and
read-write access.
In `@docs/storage.md`:
- Around line 70-79: The documentation currently lists all validation checks as
if they apply to every cloud backup, but the mounted filesystem UUID check only
occurs in prepared_drive mode. Restructure the checklist in the storage.md file
to clearly distinguish between checks that apply to both operating modes and
those specific to prepared_drive mode only. Specifically, separate the bullet
point about the mounted filesystem UUID validation (the last item in the current
list) into a dedicated subsection or group that indicates it only applies when
using a prepared drive, while the other checks (marker existence, marker ID
matching, read/write operations, and test file handling) apply in both
prepared_drive and existing_folder modes.
In `@index.html`:
- Line 164: The Storage link in the list item is missing the security attribute
required for safe external navigation. Add rel="noopener noreferrer" to the
anchor tag that links to the GitHub storage.md documentation. This attribute
prevents the destination page from accessing the window.opener object, which is
a security best practice for all target="_blank" links pointing to external
resources.
In `@simple_safer_server/routes/storage.py`:
- Around line 406-411: The `storage_status` call is not wrapped in exception
handling, which will cause the page render to fail with a 500 error if the
function raises an exception. Wrap the `storage_status` call in a try/except
block similar to the pattern used at line 225-235 in the same file, catching any
exceptions and providing a fallback status dict with appropriate default values
(such as marking the status as unavailable or error). This ensures graceful
degradation if the status cannot be retrieved rather than crashing the entire
page render.
- Around line 226-231: In the storage_service.py file (lines 53-57), the call to
storage_status is passing incorrect arguments. The second positional argument
should be self._system_utils instead of self._command_adapter, and the call
should also include the command_runner parameter. Update the storage_status
function call in storage_service.py to match the correct invocation pattern
shown in routes/storage.py (lines 226-231), ensuring that config_manager is
passed first, system_utils as the second parameter, and command_runner is passed
as a named parameter.
In `@simple_safer_server/services/storage_location.py`:
- Around line 119-123: The _normalize_storage_path function calls .strip() on
the path parameter without first validating that path is actually a string. If a
non-string value (such as a number or boolean) is passed in, this will raise an
AttributeError instead of returning a clear validation error. Add a type check
at the beginning of the _normalize_storage_path function before calling .strip()
to ensure path is a string, and raise a StorageLocationError with a descriptive
message if it is not a string type.
- Around line 39-57: The UNSAFE_STORAGE_PATHS set includes ROOT_PATH (which
represents the root directory /), causing all absolute paths to be rejected
during validation since every absolute path has / as an ancestor. Remove
ROOT_PATH from the UNSAFE_STORAGE_PATHS set definition at line 40 to fix this
issue. Additionally, review the validation logic at lines 144-146 where parent
path checking occurs to ensure it properly handles absolute paths after removing
ROOT_PATH from the unsafe set, and adjust if necessary to avoid over-blocking
legitimate paths.
- Around line 179-188: The _read_storage_marker function at lines 179-188 and
similar functions at lines 191-207 can raise filesystem exceptions (like
OSError, PermissionError, etc.) beyond the currently handled FileNotFoundError
and JSONDecodeError, which are not caught by storage_status() at lines 420-430
that only catches StorageLocationError. This allows raw filesystem exceptions to
escape and surface as 500 errors instead of controlled error responses. Wrap the
marker.read_text() call in _read_storage_marker and any similar filesystem
operations in the functions at lines 191-207 with a broader exception handler
that catches all exceptions (not just the specific ones being handled) and
converts them to StorageLocationError with appropriate context. This ensures
storage_status() can reliably handle all failures from these functions.
In `@simple_safer_server/services/system_utils.py`:
- Around line 491-503: The loop currently disables only the timer units (.timer)
for services in disabled_services, but leaves the paired service units
(.service) enabled, which allows them to run on boot. In the same loop where you
disable the .timer unit, add another self.run_command call to also disable the
corresponding .service unit (e.g., '{service_name}.service') immediately after
disabling the .timer unit to ensure the service cannot run on boot.
In `@static/css/styles.css`:
- Around line 1231-1233: The .drive-setup-status rule defined at line 1231 in
the storage section is being overridden by a duplicate .drive-setup-status rule
that appears later at line 2322+. Fix this by either scoping the new rule at
line 1231 with a parent selector (for example, change .drive-setup-status to
.storage-drive-scan .drive-setup-status) to apply it only within the storage
context, or alternatively, consolidate both .drive-setup-status definitions by
removing the duplicate at line 2322+ and merging all the necessary properties
into a single authoritative .drive-setup-status block.
In `@static/js/storage.js`:
- Around line 49-50: The save and repair flows call window.AsyncButtonState
unconditionally, which causes hard JavaScript failures if that global is
unavailable due to load-order issues or partial template inclusion. Guard the
window.AsyncButtonState.start(saveExistingBtn) call in the save flow and the
corresponding call in the repair flow by checking if window.AsyncButtonState
exists before invoking it, following the pattern already used by other handlers
elsewhere in the file.
---
Nitpick comments:
In `@scripts/validate_storage_source.py`:
- Around line 7-17: The `_add_app_to_path()` function contains hardcoded
deployment paths that lack documentation about their purpose and ordering. Add a
docstring or comment block above the function that explains why these specific
candidates are checked in order (first the repository structure with
script_path.parents[1], then the production deployment path at
/opt/SimpleSaferServer), and clarify when this bootstrap mechanism is needed to
ensure the simple_safer_server module can be imported. This documentation will
help future maintainers understand the deployment assumptions baked into this
path manipulation logic.
In `@tests/test_backup_cloud_script.py`:
- Around line 28-83: The _run_script function in
tests/test_backup_cloud_script.py is setting up a test harness with injected
executables and environment variables (PATH, SSS_PYTHON_BIN,
SSS_VALIDATE_STORAGE_SCRIPT, SSS_LOG_ALERT_SCRIPT) and fake command stubs
(fake-python, msmtp, rclone, journalctl), but lacks documentation explaining the
contract of this setup. Add a comprehensive docstring at the beginning of the
_run_script function that explains why these environment variables are being
injected, what each fake executable stub represents, how the PATH override
enables the test to control which binaries are called, and how this prevents the
script from executing real external commands. This will prevent future test
modifications from accidentally breaking the harness without understanding its
purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef862ee9-36bd-49e1-b777-b37747af7ccf
📒 Files selected for processing (40)
docs/architecture.mddocs/cloud_backup.mddocs/dashboard.mddocs/drive_health.mddocs/fake_mode.mddocs/manual_install.mddocs/network_file_sharing.mddocs/setup.mddocs/storage.mdindex.htmlscripts/backup_cloud.shscripts/validate_storage_source.pysimple_safer_server/routes/drive_health.pysimple_safer_server/routes/setup_wizard.pysimple_safer_server/routes/storage.pysimple_safer_server/routes/tasks.pysimple_safer_server/services/cloud_backup_service.pysimple_safer_server/services/config_manager.pysimple_safer_server/services/drive_health.pysimple_safer_server/services/storage_location.pysimple_safer_server/services/storage_service.pysimple_safer_server/services/system_utils.pysimple_safer_server/services/task_service.pystatic/css/styles.cssstatic/js/storage.jstemplates/base.htmltemplates/dashboard.htmltemplates/drive_health.htmltemplates/setup.htmltemplates/storage.htmltests/test_backup_cloud_script.pytests/test_cloud_backup_service.pytests/test_drive_health.pytests/test_setup_wizard.pytests/test_storage_location.pytests/test_storage_routes.pytests/test_storage_service.pytests/test_system_utils.pytests/test_task_service.pyuninstall.sh
| - the test file written by the app can be read back | ||
| - the test file can be deleted | ||
|
|
||
| For a drive prepared by SimpleSaferServer, the check also confirms that the app-managed mount point is mounted and that the mounted filesystem UUID matches the configured drive UUID. For an existing folder, the app does not mount it, but it records where that folder was mounted when it was selected. If the folder later appears under a different mount source, the backup fails until an administrator checks it. |
Contributor
There was a problem hiding this comment.
Storage readiness is stricter than the docs currently say.
The implementation doesn't stop at marker/read-write probing: prepared-drive mode also checks the mounted UUID, and existing-folder storage compares its recorded mount identity so a remounted path can be rejected even when the folder still exists.
docs/cloud_backup.md#L54-L54: update the existing-folder paragraph to mention the full identity check.docs/setup.md#L133-L134: update the Cloud Backup step to match the same contract.
📍 Affects 2 files
docs/cloud_backup.md#L54-L54(this comment)docs/setup.md#L133-L134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cloud_backup.md` at line 54, The documentation in docs/cloud_backup.md
and docs/setup.md does not accurately describe the full storage readiness
validation. Update the existing-folder storage description in
docs/cloud_backup.md (around line 54) to explicitly mention that the app
compares the recorded mount identity when a folder is accessed, and can reject
the backup if the folder appears under a different mount source than when it was
originally configured. Similarly, update the Cloud Backup step description in
docs/setup.md (lines 133-134) to reflect this same contract regarding mount
identity validation for existing-folder storage, ensuring both documents
consistently describe that storage readiness checks include verifying mount
identity, not just folder existence and read-write access.
| <li><i class="fa-solid fa-gear"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/setup.md" target="_blank">Setup Guide</a></li> | ||
| <li><i class="fa-solid fa-right-to-bracket"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/login.md" target="_blank">Login & User Management</a></li> | ||
| <li><i class="fa-solid fa-gauge-high"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/dashboard.md" target="_blank">Dashboard</a></li> | ||
| <li><i class="fa-solid fa-folder-tree"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/storage.md" target="_blank">Storage</a></li> |
Contributor
There was a problem hiding this comment.
Add rel="noopener noreferrer" to the new docs link.
This new _blank link can expose window.opener to the destination tab. Please add the noopener/noreferrer attributes so the Storage entry follows the safer external-link pattern.
♻️ Suggested fix
- <li><i class="fa-solid fa-folder-tree"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/storage.md" target="_blank">Storage</a></li>
+ <li><i class="fa-solid fa-folder-tree"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/storage.md" target="_blank" rel="noopener noreferrer">Storage</a></li>📝 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
| <li><i class="fa-solid fa-folder-tree"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/storage.md" target="_blank">Storage</a></li> | |
| <li><i class="fa-solid fa-folder-tree"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/storage.md" target="_blank" rel="noopener noreferrer">Storage</a></li> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@index.html` at line 164, The Storage link in the list item is missing the
security attribute required for safe external navigation. Add rel="noopener
noreferrer" to the anchor tag that links to the GitHub storage.md documentation.
This attribute prevents the destination page from accessing the window.opener
object, which is a security best practice for all target="_blank" links pointing
to external resources.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/js/storage_change_drive.js`:
- Around line 50-56: The showError function stores error details in a single
global variable currentErrorDetails on line 54, which causes data loss when
multiple feedback sections have errors simultaneously. Instead of using a global
currentErrorDetails variable, scope the error details to each feedback section
by storing the details directly on the feedback object itself. Update the
showError function to store details as a property of the feedback object (for
example, feedback.currentErrorDetails), and then update the code at lines
276-283 that reads currentErrorDetails to read from the corresponding feedback
object property instead. This ensures each feedback section maintains its own
error details independently.
In `@tests/test_backup_drive_setup.py`:
- Around line 513-519: The tests for format_backup_drive at lines 513-519 and
541-551 patch os.path.realpath and os.stat but fail to patch os.path.exists and
os.access, making the tests dependent on the host filesystem state. Add patches
for os.path.exists and os.access (with appropriate return values like True for
exists and os.R_OK|os.W_OK for access) to both test locations so that all
filesystem gate checks are mocked, ensuring the tests validate the actual
format_backup_drive behavior rather than failing based on host filesystem
conditions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2482132-eb23-43ac-b78c-70b103746d52
📒 Files selected for processing (13)
docs/setup.mddocs/storage.mdsimple_safer_server/adapters/backup_drive_commands.pysimple_safer_server/routes/storage.pysimple_safer_server/services/backup_drive_setup.pystatic/css/styles.cssstatic/js/storage.jsstatic/js/storage_change_drive.jstemplates/base.htmltemplates/storage.htmltemplates/storage_change_drive.htmltests/test_backup_drive_setup.pytests/test_storage_routes.py
💤 Files with no reviewable changes (1)
- static/js/storage.js
✅ Files skipped from review due to trivial changes (1)
- docs/storage.md
🚧 Files skipped from review as they are similar to previous changes (2)
- templates/base.html
- docs/setup.md
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/storage_change_drive.html (1)
141-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to the icon-only modal close button.
The top-right close control is icon-only, so it needs an explicit accessible label (and explicit button type) for assistive tech consistency.
Suggested patch
- <button class="modal-close" data-modal-close><i class="fas fa-xmark"></i></button> + <button type="button" class="modal-close" data-modal-close aria-label="Close error details dialog"> + <i class="fas fa-xmark"></i> + </button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates/storage_change_drive.html` at line 141, The modal-close button element is icon-only and lacks accessibility features needed for assistive technologies. Add a type="button" attribute to explicitly declare the button type, and add an aria-label attribute with an appropriate accessible name such as "Close modal" or "Close" to provide a text label for screen readers and other assistive tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@templates/storage_change_drive.html`:
- Line 141: The modal-close button element is icon-only and lacks accessibility
features needed for assistive technologies. Add a type="button" attribute to
explicitly declare the button type, and add an aria-label attribute with an
appropriate accessible name such as "Close modal" or "Close" to provide a text
label for screen readers and other assistive tools.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbd50bd8-0088-4a45-92e3-929f03c9aee9
📒 Files selected for processing (12)
AGENTS.mddocs/setup.mddocs/storage.mdsimple_safer_server/routes/storage.pystatic/css/styles.cssstatic/js/storage.jsstatic/js/storage_existing_folder.jstemplates/base.htmltemplates/storage.htmltemplates/storage_change_drive.htmltemplates/storage_existing_folder.htmltests/test_storage_routes.py
💤 Files with no reviewable changes (2)
- AGENTS.md
- static/js/storage.js
✅ Files skipped from review due to trivial changes (1)
- docs/storage.md
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/base.html
- tests/test_storage_routes.py
- simple_safer_server/routes/storage.py
- docs/setup.md
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
templates/storage_change_drive.html (1)
141-141:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to the icon-only modal close button.
The close button is icon-only and currently has no accessible label, so assistive tech won’t announce its purpose.
Suggested fix
- <button class="modal-close" data-modal-close><i class="fas fa-xmark"></i></button> + <button class="modal-close" data-modal-close aria-label="Close error details dialog"> + <i class="fas fa-xmark"></i> + </button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates/storage_change_drive.html` at line 141, The modal close button element with class "modal-close" is icon-only and lacks an accessible name, preventing assistive technologies from announcing its purpose. Add an aria-label attribute to the button element with an appropriate value such as "Close modal" or "Close" to provide an accessible name that screen readers will announce to users.Source: Coding guidelines
simple_safer_server/services/drive_health.py (3)
502-509:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat placeholder serials as missing before choosing the drive-state key.
hdsentinel_drive_key()currently prefers any truthy serial. If HDSentinel reports placeholders such as-,?, orunknownfor multiple drives, they all persist under the sameserial:*key and overwrite each other. Normalize those placeholders toNoneso the code falls back to the device path.Suggested fix
+def _clean_hdsentinel_identity(value): + if value is None: + return None + cleaned = str(value).strip() + if not cleaned or cleaned in {"-", "?"} or cleaned.lower() in {"unknown", "n/a", "none"}: + return None + return cleaned + + def hdsentinel_drive_key(snapshot): - serial = (snapshot or {}).get("serial") + serial = _clean_hdsentinel_identity((snapshot or {}).get("serial")) if serial: return f"serial:{serial}" - device = (snapshot or {}).get("device") + device = _clean_hdsentinel_identity((snapshot or {}).get("device")) if device: return f"device:{device}" return None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@simple_safer_server/services/drive_health.py` around lines 502 - 509, The hdsentinel_drive_key() function needs to filter out placeholder serial values before using them as a drive key. After retrieving the serial from the snapshot, check if it is a placeholder value such as "-", "?", or "unknown". If the serial matches any of these placeholders, treat it as falsy (do not return it as the key) and allow the function to fall through to check the device path instead. This ensures that multiple drives with the same placeholder serial will not overwrite each other, and will instead be identified by their device paths.
484-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the per-drive map when falling back to single-snapshot state.
save_hdsentinel_state()now overwrites the same state file thatsave_hdsentinel_drive_state()uses, but writes onlylast_snapshot. If the all-drive-solidpath is temporarily unavailable and the monitor falls back tocollect_hdsentinel_snapshot(), the existingdrivesmap is erased, so the next successful per-drive run loses previous health baselines and can miss alerts.Suggested fix
def save_hdsentinel_state(snapshot, runtime=None): runtime = runtime or get_runtime() path = get_hdsentinel_state_path(runtime) - _write_json_atomically(path, {"last_snapshot": snapshot}) + payload = {"last_snapshot": snapshot} + if path.exists(): + try: + existing = json.loads(path.read_text()) + if isinstance(existing.get("drives"), dict): + payload["drives"] = existing["drives"] + except Exception as exc: + LOGGER.warning("Failed to preserve HDSentinel drive state: %s", exc) + _write_json_atomically(path, payload)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@simple_safer_server/services/drive_health.py` around lines 484 - 499, The save_hdsentinel_state function overwrites the entire state file with only a last_snapshot value, erasing any existing drives map that was previously saved by save_hdsentinel_drive_state. To fix this, modify save_hdsentinel_state to read the existing state file first (if it exists), preserve the drives map from that state, and then write back both the preserved drives map and the new last_snapshot value. This ensures that when the monitor falls back to collecting a single snapshot, the per-drive health baselines are retained for the next per-drive collection cycle.
662-664:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLet all-drive HDSentinel timeouts fall back instead of aborting the monitor.
collect_hdsentinel_snapshot()handlesTimeoutExpired, but the new all-drivecollect_hdsentinel_drive_list()path does not. A sleeping USB drive can now raise beforerun_hdsentinel_health_monitor()reaches the single-drive fallback path.Suggested fix
- result = _run_hdsentinel_command(binary_path, ["-solid"]) + try: + result = _run_hdsentinel_command(binary_path, ["-solid"]) + except TimeoutExpired: + LOGGER.warning(DRIVE_HEALTH_TIMEOUT_MESSAGE) + return [] if result.returncode != 0: return []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@simple_safer_server/services/drive_health.py` around lines 662 - 664, The _run_hdsentinel_command call with the ["-solid"] argument in the collect_hdsentinel_drive_list function does not handle TimeoutExpired exceptions, which can be raised when a sleeping USB drive times out. Wrap this _run_hdsentinel_command call in a try-except block to catch TimeoutExpired exceptions and return an empty list, matching the exception handling pattern already used in collect_hdsentinel_snapshot(), so that timeouts fall back gracefully instead of aborting the monitor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@simple_safer_server/services/drive_health.py`:
- Around line 502-509: The hdsentinel_drive_key() function needs to filter out
placeholder serial values before using them as a drive key. After retrieving the
serial from the snapshot, check if it is a placeholder value such as "-", "?",
or "unknown". If the serial matches any of these placeholders, treat it as falsy
(do not return it as the key) and allow the function to fall through to check
the device path instead. This ensures that multiple drives with the same
placeholder serial will not overwrite each other, and will instead be identified
by their device paths.
- Around line 484-499: The save_hdsentinel_state function overwrites the entire
state file with only a last_snapshot value, erasing any existing drives map that
was previously saved by save_hdsentinel_drive_state. To fix this, modify
save_hdsentinel_state to read the existing state file first (if it exists),
preserve the drives map from that state, and then write back both the preserved
drives map and the new last_snapshot value. This ensures that when the monitor
falls back to collecting a single snapshot, the per-drive health baselines are
retained for the next per-drive collection cycle.
- Around line 662-664: The _run_hdsentinel_command call with the ["-solid"]
argument in the collect_hdsentinel_drive_list function does not handle
TimeoutExpired exceptions, which can be raised when a sleeping USB drive times
out. Wrap this _run_hdsentinel_command call in a try-except block to catch
TimeoutExpired exceptions and return an empty list, matching the exception
handling pattern already used in collect_hdsentinel_snapshot(), so that timeouts
fall back gracefully instead of aborting the monitor.
In `@templates/storage_change_drive.html`:
- Line 141: The modal close button element with class "modal-close" is icon-only
and lacks an accessible name, preventing assistive technologies from announcing
its purpose. Add an aria-label attribute to the button element with an
appropriate value such as "Close modal" or "Close" to provide an accessible name
that screen readers will announce to users.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bba0ba9-5ad6-4fa6-b7df-f05c90f87523
📒 Files selected for processing (27)
AGENTS.mddocs/cloud_backup.mddocs/dashboard.mddocs/drive_health.mddocs/fake_mode.mddocs/setup.mddocs/storage.mdsimple_safer_server/routes/drive_health.pysimple_safer_server/routes/setup_wizard.pysimple_safer_server/routes/storage.pysimple_safer_server/services/backup_drive_setup.pysimple_safer_server/services/config_manager.pysimple_safer_server/services/drive_health.pysimple_safer_server/services/storage_location.pysimple_safer_server/services/storage_service.pysimple_safer_server/services/system_utils.pystatic/css/styles.cssstatic/js/storage_change_drive.jstemplates/drive_health.htmltemplates/setup.htmltemplates/storage.htmltemplates/storage_change_drive.htmltemplates/storage_existing_folder.htmltests/test_drive_health.pytests/test_storage_location.pytests/test_storage_routes.pytests/test_storage_service.py
✅ Files skipped from review due to trivial changes (2)
- docs/dashboard.md
- docs/storage.md
🚧 Files skipped from review as they are similar to previous changes (18)
- docs/fake_mode.md
- AGENTS.md
- simple_safer_server/routes/drive_health.py
- tests/test_storage_service.py
- docs/cloud_backup.md
- templates/storage_existing_folder.html
- templates/storage.html
- static/js/storage_change_drive.js
- docs/drive_health.md
- tests/test_storage_location.py
- simple_safer_server/routes/setup_wizard.py
- templates/drive_health.html
- simple_safer_server/services/system_utils.py
- simple_safer_server/services/backup_drive_setup.py
- templates/setup.html
- docs/setup.md
- simple_safer_server/services/storage_location.py
- simple_safer_server/routes/storage.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/test_drive_health.py (1)
330-349: 💤 Low valueConsider adding a comment explaining the shared-file preservation behavior.
This test verifies that
save_hdsentinel_statepreserves existing per-drive state when updating the snapshot. This shared-file design (where per-drive monitor and fallback single-drive check use the same state file) might not be immediately obvious to future maintainers. As per coding guidelines, consider adding a brief comment explaining what specific behavior this test protects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_drive_health.py` around lines 330 - 349, The test method test_save_hdsentinel_state_preserves_drive_state lacks a comment explaining the shared-file preservation behavior it validates. Add a brief comment at the beginning of the test method explaining that this test verifies the shared-file design where both the per-drive monitor (save_hdsentinel_drive_state) and fallback single-drive check (save_hdsentinel_state) use the same state file, and that updates to the snapshot should not lose the previously saved per-drive state. This clarification will help future maintainers understand why this particular interaction between the two save functions is being tested.Source: Coding guidelines
templates/storage.html (1)
39-53: 💤 Low valueHTMLHint duplicate ID warnings are false positives.
The static analyzer flags
storageCurrentStatusLineandstorageCurrentStatusNoteas duplicate IDs, but this is a false positive. The Jinja2 conditionals are mutually exclusive, so only one branch renders at runtime, ensuring unique IDs in the final DOM.The pattern appears intentional: reusing the same IDs across branches allows
storage.jsto update these elements regardless of which state initially rendered.Optionally, consider adding a brief HTML comment before line 39 explaining the status state machine to help future maintainers quickly understand the four possible states (unchecked+ok, unchecked+fail, checked+ok, checked+fail).
Optional clarifying comment
<div class="storage-field-label">Status</div> + <!-- Status has 4 states based on (checked, ok): unchecked+ok=Configured, unchecked+fail=Not mounted, checked+ok=Passed, checked+fail=Failed. + IDs are reused across mutually-exclusive branches so storage.js can update them regardless of initial state. --> {% if not storage_status.checked %}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@templates/storage.html` around lines 39 - 53, Add a brief HTML comment before the storage status block starting at line 39 in templates/storage.html that explains the Jinja2 state machine logic. The comment should document the four possible states that can render: unchecked+ok, unchecked+fail, checked+ok, and checked+fail. This clarifies that the identical IDs storageCurrentStatusLine and storageCurrentStatusNote are intentionally reused across mutually exclusive conditional branches, where only one branch renders at runtime, ensuring unique IDs in the final DOM while allowing storage.js to reliably target these elements regardless of which state was initially rendered.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@simple_safer_server/routes/storage.py`:
- Around line 268-276: In the exception handler for the psutil.disk_usage()
call, replace the broad `except Exception:` clause with `except OSError:` since
psutil.disk_usage() only raises OSError for system-level issues such as path not
found, device not ready, or access denied. This narrower exception specification
prevents masking unrelated errors and aligns with Ruff's linting guidance.
In `@simple_safer_server/routes/tasks.py`:
- Around line 53-57: The `passive_storage_status()` function reports existing
folders as healthy without validating disk readability via
`psutil.disk_usage()`. Update the logic within `passive_storage_status()` to
mirror the disk readability checks performed in the `/api/storage/status`
endpoint so that unreadable or missing existing folders are not incorrectly
marked as available. Ensure that if disk usage cannot be retrieved for an
existing folder path, the status reflects an error state rather than returning
`ok=True` with empty error information.
In `@static/js/mega_folder_picker.js`:
- Around line 43-45: The activeLoadId variable is currently declared as a local
variable within the openMegaFolderPicker() function invocation, which means each
time the picker is opened, a new activeLoadId is created. This allows stale
requests from a previous invocation to still execute if they settle after the
modal is reopened, since they reference their own activeLoadId closure value.
Move activeLoadId from being a local variable to being stored as a property on
the modal element itself (persisting across reopenings), and update all
references to activeLoadId throughout the picker logic (in the load validation
checks and request callback handlers around lines 86-118 and 156-160) to read
from and compare against this persisted modal property instead of the local
closure variable.
- Around line 136-141: The folder row is set up with button role and tabindex
attributes making it keyboard accessible, but the click event listener on the
folder item only handles mouse activation. Add a keyboard event listener to the
same item element that triggers the same loadDirs navigation when the Enter or
Space keys are pressed, checking for keyCode values 13 (Enter) or 32 (Space) in
the event handler to ensure proper keyboard accessibility for the folder
navigation.
---
Nitpick comments:
In `@templates/storage.html`:
- Around line 39-53: Add a brief HTML comment before the storage status block
starting at line 39 in templates/storage.html that explains the Jinja2 state
machine logic. The comment should document the four possible states that can
render: unchecked+ok, unchecked+fail, checked+ok, and checked+fail. This
clarifies that the identical IDs storageCurrentStatusLine and
storageCurrentStatusNote are intentionally reused across mutually exclusive
conditional branches, where only one branch renders at runtime, ensuring unique
IDs in the final DOM while allowing storage.js to reliably target these elements
regardless of which state was initially rendered.
In `@tests/test_drive_health.py`:
- Around line 330-349: The test method
test_save_hdsentinel_state_preserves_drive_state lacks a comment explaining the
shared-file preservation behavior it validates. Add a brief comment at the
beginning of the test method explaining that this test verifies the shared-file
design where both the per-drive monitor (save_hdsentinel_drive_state) and
fallback single-drive check (save_hdsentinel_state) use the same state file, and
that updates to the snapshot should not lose the previously saved per-drive
state. This clarification will help future maintainers understand why this
particular interaction between the two save functions is being tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbcc6929-635c-4db9-9e0f-6abb931826be
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
docs/architecture.mddocs/cloud_backup.mddocs/dashboard.mddocs/development.mddocs/railway.mddocs/setup.mddocs/storage.mdnixpacks.tomlsimple_safer_server/app_factory.pysimple_safer_server/routes/setup_wizard.pysimple_safer_server/routes/smb.pysimple_safer_server/routes/storage.pysimple_safer_server/routes/tasks.pysimple_safer_server/services/drive_health.pysimple_safer_server/services/filesystem_browser.pysimple_safer_server/services/storage_location.pysimple_safer_server/services/storage_service.pysimple_safer_server/services/system_utils.pystatic/css/styles.cssstatic/js/mega_folder_picker.jsstatic/js/storage.jsstatic/js/storage_existing_folder.jstemplates/network_file_sharing.htmltemplates/setup.htmltemplates/storage.htmltemplates/storage_existing_folder.htmltests/test_app_factory_routes.pytests/test_drive_health.pytests/test_filesystem_browser.pytests/test_setup_wizard.pytests/test_smb_routes.pytests/test_storage_location.pytests/test_storage_routes.pytests/test_storage_service.pytests/test_system_utils.py
💤 Files with no reviewable changes (1)
- docs/development.md
✅ Files skipped from review due to trivial changes (3)
- docs/dashboard.md
- docs/architecture.md
- docs/storage.md
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/test_system_utils.py
- simple_safer_server/services/storage_service.py
- templates/storage_existing_folder.html
- docs/cloud_backup.md
- simple_safer_server/services/system_utils.py
- simple_safer_server/routes/setup_wizard.py
- static/css/styles.css
- simple_safer_server/services/storage_location.py
- docs/setup.md
- simple_safer_server/services/drive_health.py
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
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 by CodeRabbit
cloud_enabled(true/false) and is blocked when storage can’t be verified