Vibe branch#73
Open
chrismin13 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThree features are added: (1) admin-configurable rclone include/exclude filter patterns for cloud backup, stored in private files and passed via Changesrclone filter patterns for cloud backup
Setup self-backup service, CLI, and API
Task automatic-runs schedule toggle
Sequence Diagram(s)sequenceDiagram
participant Admin
participant CloudBackupUI
participant CloudBackupService
participant rclone_filters
participant RcloneAdapter
Admin->>CloudBackupUI: enter include/exclude patterns, save
CloudBackupUI->>CloudBackupService: POST /api/cloud_backup/config {rclone_include_patterns, rclone_exclude_patterns}
CloudBackupService->>rclone_filters: write_rclone_pattern_texts(runtime, include, exclude)
rclone_filters-->>CloudBackupService: patterns persisted (0600)
note over rclone_filters,RcloneAdapter: At backup runtime
rclone_filters->>rclone_filters: write_temp_rclone_filter_file(runtime)
rclone_filters-->>RcloneAdapter: temp filter file path
RcloneAdapter->>RcloneAdapter: build_sync_command(..., filter_from=path)
RcloneAdapter-->>RcloneAdapter: rclone sync ... --filter-from <path>
sequenceDiagram
participant SetupWizard
participant SetupSelfBackupService
participant Filesystem
participant TarArchive
SetupWizard->>SetupSelfBackupService: create_backup(dest, keep)
SetupSelfBackupService->>Filesystem: validate mount point
SetupSelfBackupService->>TarArchive: write manifest.json + files/
SetupSelfBackupService->>Filesystem: chmod 0600, prune old archives
SetupSelfBackupService-->>SetupWizard: {archive, file_count}
SetupWizard->>SetupSelfBackupService: restore_backup(archive, force_setup_incomplete=True)
SetupSelfBackupService->>TarArchive: extract manifest, validate version
SetupSelfBackupService->>SetupSelfBackupService: _force_setup_incomplete(config_text)
SetupSelfBackupService->>Filesystem: _atomic_copy_bytes for each allowed file
SetupSelfBackupService-->>SetupWizard: {archive, restored_files}
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 14
🧹 Nitpick comments (1)
static/js/scripts.js (1)
158-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the on/off semantics at the endpoint call site.
A short comment here would help prevent future confusion between this direct toggle flow and the temporary disable flow.
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.Suggested diff
async function setScheduleEnabled(enabled) { if (!scheduleToggle) return; + // Direct dashboard/detail toggle: false maps to a permanent timer disable. + // Temporary pauses are handled through the schedule-management flow. scheduleToggle.disabled = true; try { const response = await window.ApiClient.fetchJson( `/task/${encodeURIComponent(taskName)}/schedule-enabled`,🤖 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 `@static/js/scripts.js` around lines 158 - 168, The setScheduleEnabled function lacks documentation about the toggle's on/off semantics and how this direct toggle flow differs from temporary disable behavior. Add a clear comment above the window.ApiClient.fetchJson call that explains what this endpoint does (direct persistent toggle of schedule enabled/disabled state) and specifically clarifies the distinction from any temporary disable flow to prevent future confusion and align with the coding guidelines requiring comments for non-obvious functionality.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 `@index.html`:
- Line 162: The external link to the SimpleSaferServer repository with
target="_blank" is missing the rel attribute for security purposes. Locate the
anchor tag that links to the GitHub setup_self_backup.md documentation and add
the rel="noopener noreferrer" attribute to the opening anchor tag alongside the
existing target="_blank" attribute. This prevents the new tab from gaining
access to the window object of the original page, strengthening tab isolation.
In `@scripts/setup_self_backup.py`:
- Around line 10-16: Add inline comments to the _add_app_to_path function to
explain why sys.path is being patched at runtime (document the runtime path
injection strategy). Additionally, add a brief comment near the implicit default
subcommand logic (around the area mentioned at lines 75-76) to clarify why the
'create' subcommand is used as the default when no subcommand is specified. Both
comments should be concise and explain the reasoning behind these design
decisions to prevent future misinterpretation.
In `@simple_safer_server/routes/tasks.py`:
- Around line 255-257: The issue is that when request.get_json(silent=True)
returns a non-dictionary JSON value (such as a list or string), the assignment
on line 255 leaves data as that non-dict value, and then calling
data.get("enabled") on line 256 raises an AttributeError because lists and
strings don't have a .get() method. Before accessing data.get("enabled"), add a
type check to ensure data is actually a dictionary, and return a 400 validation
error if it's not a dict. This will prevent the AttributeError from bubbling up
as a 500 error and properly handle invalid JSON payloads.
In `@simple_safer_server/services/cloud_backup_service.py`:
- Around line 97-102: The issue is in the write_rclone_pattern_texts function
call where using data.get() with an empty string as the default causes
unintended clearing of pattern fields during partial updates. When only
"rclone_include_patterns" or "rclone_exclude_patterns" is provided, the missing
parameter defaults to an empty string, erasing the previously saved value. To
fix this, first retrieve the existing rclone pattern values from self._runtime
before calling write_rclone_pattern_texts, then use those existing values as
defaults instead of empty strings, so that only the explicitly provided patterns
are updated while preserving the other field.
In `@simple_safer_server/services/rclone_filters.py`:
- Around line 55-67: The current implementation writes the include patterns file
before validating the exclude patterns, creating a risk of partial persistence
where the include file is updated but the exclude file update fails, leaving an
inconsistent state. To fix this, validate and normalize both patterns before
writing any files by calling normalize_rclone_pattern_text on both
include_patterns and exclude_patterns at the start of the function, storing the
results in variables, then use those pre-validated results in the subsequent
atomic_write_text calls for both include_path and exclude_path. This ensures
that if either pattern validation fails, no files are modified.
In `@simple_safer_server/services/setup_self_backup.py`:
- Around line 32-57: Add a docstring or comment block to the _atomic_copy_bytes
function explaining its purpose and the durability/safety guarantees it
provides. The comment should document why specific operations like fsync, atomic
rename via os.replace, and the mode setting are important to prevent accidental
weakening of the write guarantees in future edits. This should be placed at the
start of the function before the code begins.
- Around line 125-170: The create_backup method currently allows raw exceptions
from tarfile operations, JSON serialization via atomic_json_bytes, and file I/O
to propagate directly instead of wrapping them consistently in
SetupSelfBackupError. Wrap the entire archive creation block (starting with
tarfile.open and including all operations on the archive, manifest creation, and
file additions) and the atomic_json_bytes call in a try-except block that
catches all exceptions and re-raises them as SetupSelfBackupError with a
descriptive message. Apply the same consistent error wrapping pattern to the
restore_backup method (lines 222-263) and any other methods that perform archive
or JSON operations to ensure the service maintains its error contract and only
raises SetupSelfBackupError.
In `@simple_safer_server/services/system_utils.py`:
- Around line 318-322: In the setup_self_backup.service unit configuration
within the system_utils.py file, remove the line containing
Wants=check_health.service while keeping the After=check_health.service
directive. This prevents systemd from automatically triggering the health check
service whenever the backup service runs. The After directive will maintain the
proper startup ordering without causing unintended activation of the health
check.
In `@static/js/cloud_backup.js`:
- Around line 345-346: The short-circuit evaluation of the && operator in the
filtersValid assignment prevents the second validateRclonePatternText call from
executing when the first validation fails, leaving stale validity styling on the
second textarea. Refactor the validation logic in the filtersValid assignment
(around line 345-346) and the similar code at lines 418-419 to call
validateRclonePatternText independently for both rcloneIncludePatterns and
rcloneExcludePatterns first, storing their results in separate variables, then
combine those results together. This ensures both validation functions execute
regardless of the first result and both textareas receive proper styling
updates.
In `@templates/404.html`:
- Line 18: The `network_file_sharing` route reference in the template uses a
non-namespaced `url_for()` call while other routes like `task_routes.dashboard`
use the blueprint pattern with namespacing. To fix this, either move the
`network_file_sharing` route registration from being directly on the app in
`app_factory.py` to a dedicated blueprint module (similar to how `task_routes`
is structured), and update the `url_for()` call to use the blueprint namespace,
or add a comment explaining why this route intentionally deviates from the
blueprint pattern used by other routes in the template.
- Around line 37-38: The 404.html template uses the variable `requested_path` on
line 38, but the `handle_not_found` error handler in app_factory.py does not
pass this variable when rendering the template. Update the `handle_not_found`
function (decorated with `@app.errorhandler`(404)) to explicitly pass
`requested_path=request.path` as a parameter to the render_template call so the
template has access to the requested path value.
- Around line 1-42: The 404 template in 404.html uses layout-specific CSS
classes (.not-found-page, .not-found-panel, .not-found-main, .not-found-code,
.not-found-copy, .not-found-actions, .not-found-path) that lack corresponding
CSS definitions in static/css/styles.css. Add CSS styling for these classes to
provide proper spacing, alignment, and visual hierarchy. Ensure the styling
follows the Bunker design system aesthetic, including frosted-glass card effects
for the .not-found-panel, appropriate sizing for the .not-found-code element,
and proper layout for the .not-found-actions section with its navigation
buttons.
In `@templates/dashboard.html`:
- Around line 420-423: The keydown handler on the row element is missing the
interactive-descendant guard that exists in the click handler. When users press
Space or Enter on interactive elements like the toggle checkbox, the event
bubbles up and triggers unwanted navigation. Add the same guard check to the
row's keydown event listener using event.target.closest() to detect interactive
elements (a, button, input, label, select, textarea), returning early if found,
before the logic that handles Enter or Space key presses and navigates to the
href.
In `@tests/test_task_schedule_ui_rendering.py`:
- Around line 187-189: The assertion on lines 187-189 is checking for the
absence of DDNS Update in only the portion of the page before the first
occurrence of class="task-schedule-toggle", which creates an under-scoped check
that could miss the DDNS toggle if it appears later in the table. Modify the
assertion to check the entire page content for the absence of
data-task-name="DDNS Update" without restricting the search to only the HTML
before the first toggle element, ensuring the assertion covers all rendered
toggles in the page.
---
Nitpick comments:
In `@static/js/scripts.js`:
- Around line 158-168: The setScheduleEnabled function lacks documentation about
the toggle's on/off semantics and how this direct toggle flow differs from
temporary disable behavior. Add a clear comment above the
window.ApiClient.fetchJson call that explains what this endpoint does (direct
persistent toggle of schedule enabled/disabled state) and specifically clarifies
the distinction from any temporary disable flow to prevent future confusion and
align with the coding guidelines requiring comments for non-obvious
functionality.
🪄 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: bc1836b4-57cd-49dd-8e57-34f5c4f9880d
📒 Files selected for processing (34)
docs/cloud_backup.mddocs/dashboard.mddocs/setup.mddocs/setup_self_backup.mddocs/task_detail.mdindex.htmlscripts/backup_cloud.shscripts/setup_self_backup.pysimple_safer_server/adapters/rclone.pysimple_safer_server/routes/setup_wizard.pysimple_safer_server/routes/tasks.pysimple_safer_server/services/cloud_backup_service.pysimple_safer_server/services/rclone_filters.pysimple_safer_server/services/setup_self_backup.pysimple_safer_server/services/system_utils.pysimple_safer_server/services/task_service.pystatic/css/styles.cssstatic/js/cloud_backup.jsstatic/js/scripts.jstemplates/404.htmltemplates/cloud_backup.htmltemplates/dashboard.htmltemplates/task_detail.htmltests/test_cloud_backup_service.pytests/test_rclone_adapter.pytests/test_rclone_filters.pytests/test_setup_self_backup.pytests/test_setup_wizard.pytests/test_system_utils.pytests/test_task_routes.pytests/test_task_schedule_ui_rendering.pytests/test_task_service.pytests/test_uninstall.pyuninstall.sh
| <li><i class="fa-solid fa-flask"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/fake_mode.md" target="_blank">Fake Mode</a></li> | ||
| <li><i class="fa-solid fa-train-subway"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/railway.md" target="_blank">Railway Deployment Notes</a></li> | ||
| <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-box-archive"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/setup_self_backup.md" target="_blank">Setup Self-Backup</a></li> |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add rel attributes to the external docs link opened in a new tab.
Line 162 uses target="_blank" without rel="noopener noreferrer", which weakens tab isolation.
Suggested fix
- <li><i class="fa-solid fa-box-archive"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/setup_self_backup.md" target="_blank">Setup Self-Backup</a></li>
+ <li><i class="fa-solid fa-box-archive"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/setup_self_backup.md" target="_blank" rel="noopener noreferrer">Setup Self-Backup</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-box-archive"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/setup_self_backup.md" target="_blank">Setup Self-Backup</a></li> | |
| <li><i class="fa-solid fa-box-archive"></i> <a href="https://github.com/chrismin13/SimpleSaferServer/blob/main/docs/setup_self_backup.md" target="_blank" rel="noopener noreferrer">Setup Self-Backup</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 162, The external link to the SimpleSaferServer
repository with target="_blank" is missing the rel attribute for security
purposes. Locate the anchor tag that links to the GitHub setup_self_backup.md
documentation and add the rel="noopener noreferrer" attribute to the opening
anchor tag alongside the existing target="_blank" attribute. This prevents the
new tab from gaining access to the window object of the original page,
strengthening tab isolation.
Comment on lines
+10
to
+16
| def _add_app_to_path() -> None: | ||
| script_path = Path(__file__).resolve() | ||
| candidates = [script_path.parents[1], Path("/opt/SimpleSaferServer")] | ||
| for candidate in candidates: | ||
| if (candidate / "simple_safer_server").is_dir(): | ||
| sys.path.insert(0, str(candidate)) | ||
| return |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the path-injection fallback and implicit default command.
Please add brief inline comments explaining (1) why sys.path is patched at runtime and (2) why no subcommand defaults to create, since both are easy to misinterpret later.
✍️ Suggested comment additions
def _add_app_to_path() -> None:
+ # This script may run from installed locations where project root is not
+ # already on PYTHONPATH (for example under /opt).
script_path = Path(__file__).resolve()
candidates = [script_path.parents[1], Path("/opt/SimpleSaferServer")]
@@
def main() -> int:
@@
- command = args.command or "create"
+ # Timer/non-interactive invocations omit a subcommand, so default to create.
+ command = args.command or "create"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.
Also applies to: 75-76
🤖 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/setup_self_backup.py` around lines 10 - 16, Add inline comments to
the _add_app_to_path function to explain why sys.path is being patched at
runtime (document the runtime path injection strategy). Additionally, add a
brief comment near the implicit default subcommand logic (around the area
mentioned at lines 75-76) to clarify why the 'create' subcommand is used as the
default when no subcommand is specified. Both comments should be concise and
explain the reasoning behind these design decisions to prevent future
misinterpretation.
Source: Coding guidelines
Comment on lines
+255
to
+257
| data = request.get_json(silent=True) or request.form | ||
| enabled = data.get("enabled") | ||
| if not isinstance(enabled, bool): |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard non-object JSON payloads before calling .get.
Line 255 can produce a non-dict JSON value (e.g., [] or "x"), and Line 256 then raises AttributeError outside the exception handler, returning a 500 instead of a validation 400.
💡 Suggested fix
- data = request.get_json(silent=True) or request.form
+ data = request.get_json(silent=True)
+ if data is None:
+ data = request.form
+ elif not isinstance(data, dict):
+ return json_problem(
+ ValidationProblem(
+ "Request body must be a JSON object with enabled=true/false.",
+ slug="task-schedule-toggle-validation-error",
+ )
+ )
enabled = data.get("enabled")📝 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
| data = request.get_json(silent=True) or request.form | |
| enabled = data.get("enabled") | |
| if not isinstance(enabled, bool): | |
| data = request.get_json(silent=True) | |
| if data is None: | |
| data = request.form | |
| elif not isinstance(data, dict): | |
| return json_problem( | |
| ValidationProblem( | |
| "Request body must be a JSON object with enabled=true/false.", | |
| slug="task-schedule-toggle-validation-error", | |
| ) | |
| ) | |
| enabled = data.get("enabled") | |
| if not isinstance(enabled, bool): |
🤖 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/routes/tasks.py` around lines 255 - 257, The issue is
that when request.get_json(silent=True) returns a non-dictionary JSON value
(such as a list or string), the assignment on line 255 leaves data as that
non-dict value, and then calling data.get("enabled") on line 256 raises an
AttributeError because lists and strings don't have a .get() method. Before
accessing data.get("enabled"), add a type check to ensure data is actually a
dictionary, and return a 400 validation error if it's not a dict. This will
prevent the AttributeError from bubbling up as a 500 error and properly handle
invalid JSON payloads.
Comment on lines
+97
to
+102
| if "rclone_include_patterns" in data or "rclone_exclude_patterns" in data: | ||
| write_rclone_pattern_texts( | ||
| self._runtime, | ||
| include_patterns=data.get("rclone_include_patterns", ""), | ||
| exclude_patterns=data.get("rclone_exclude_patterns", ""), | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid clearing the other filter field on partial updates.
When only one key is provided, the missing key falls back to "", which unintentionally erases previously saved patterns for that side.
💡 Proposed fix
if "rclone_include_patterns" in data or "rclone_exclude_patterns" in data:
+ current_patterns = read_rclone_pattern_texts(self._runtime)
write_rclone_pattern_texts(
self._runtime,
- include_patterns=data.get("rclone_include_patterns", ""),
- exclude_patterns=data.get("rclone_exclude_patterns", ""),
+ include_patterns=data.get(
+ "rclone_include_patterns",
+ current_patterns["rclone_include_patterns"],
+ ),
+ exclude_patterns=data.get(
+ "rclone_exclude_patterns",
+ current_patterns["rclone_exclude_patterns"],
+ ),
)🤖 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/cloud_backup_service.py` around lines 97 - 102,
The issue is in the write_rclone_pattern_texts function call where using
data.get() with an empty string as the default causes unintended clearing of
pattern fields during partial updates. When only "rclone_include_patterns" or
"rclone_exclude_patterns" is provided, the missing parameter defaults to an
empty string, erasing the previously saved value. To fix this, first retrieve
the existing rclone pattern values from self._runtime before calling
write_rclone_pattern_texts, then use those existing values as defaults instead
of empty strings, so that only the explicitly provided patterns are updated
while preserving the other field.
Comment on lines
+55
to
+67
| include_path, exclude_path = rclone_pattern_paths(runtime) | ||
| runtime.config_dir.mkdir(parents=True, exist_ok=True) | ||
| runtime.config_dir.chmod(0o700) | ||
| atomic_write_text( | ||
| include_path, | ||
| normalize_rclone_pattern_text(include_patterns), | ||
| mode=0o600, | ||
| ) | ||
| atomic_write_text( | ||
| exclude_path, | ||
| normalize_rclone_pattern_text(exclude_patterns), | ||
| mode=0o600, | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent partial persistence when one pattern field fails validation.
write_rclone_pattern_texts can persist the include file before exclude validation fails, leaving a mixed old/new state while the request returns an error.
💡 Proposed fix
def write_rclone_pattern_texts(
runtime: Any,
*,
include_patterns: Any,
exclude_patterns: Any,
) -> None:
include_path, exclude_path = rclone_pattern_paths(runtime)
runtime.config_dir.mkdir(parents=True, exist_ok=True)
runtime.config_dir.chmod(0o700)
+ normalized_include = normalize_rclone_pattern_text(include_patterns)
+ normalized_exclude = normalize_rclone_pattern_text(exclude_patterns)
atomic_write_text(
include_path,
- normalize_rclone_pattern_text(include_patterns),
+ normalized_include,
mode=0o600,
)
atomic_write_text(
exclude_path,
- normalize_rclone_pattern_text(exclude_patterns),
+ normalized_exclude,
mode=0o600,
)📝 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
| include_path, exclude_path = rclone_pattern_paths(runtime) | |
| runtime.config_dir.mkdir(parents=True, exist_ok=True) | |
| runtime.config_dir.chmod(0o700) | |
| atomic_write_text( | |
| include_path, | |
| normalize_rclone_pattern_text(include_patterns), | |
| mode=0o600, | |
| ) | |
| atomic_write_text( | |
| exclude_path, | |
| normalize_rclone_pattern_text(exclude_patterns), | |
| mode=0o600, | |
| ) | |
| include_path, exclude_path = rclone_pattern_paths(runtime) | |
| runtime.config_dir.mkdir(parents=True, exist_ok=True) | |
| runtime.config_dir.chmod(0o700) | |
| normalized_include = normalize_rclone_pattern_text(include_patterns) | |
| normalized_exclude = normalize_rclone_pattern_text(exclude_patterns) | |
| atomic_write_text( | |
| include_path, | |
| normalized_include, | |
| mode=0o600, | |
| ) | |
| atomic_write_text( | |
| exclude_path, | |
| normalized_exclude, | |
| mode=0o600, | |
| ) |
🤖 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/rclone_filters.py` around lines 55 - 67, The
current implementation writes the include patterns file before validating the
exclude patterns, creating a risk of partial persistence where the include file
is updated but the exclude file update fails, leaving an inconsistent state. To
fix this, validate and normalize both patterns before writing any files by
calling normalize_rclone_pattern_text on both include_patterns and
exclude_patterns at the start of the function, storing the results in variables,
then use those pre-validated results in the subsequent atomic_write_text calls
for both include_path and exclude_path. This ensures that if either pattern
validation fails, no files are modified.
Comment on lines
+1
to
+42
| {% extends "base.html" %} | ||
|
|
||
| {% block title %}{{ browser_title('Page not found') }}{% endblock %} | ||
| {% block header %}Page not found{% endblock %} | ||
|
|
||
| {% block content %} | ||
| <section class="not-found-page" aria-labelledby="not-found-title"> | ||
| <div class="not-found-panel"> | ||
| <div class="not-found-main"> | ||
| <div class="not-found-code" aria-hidden="true">404</div> | ||
| <div class="not-found-copy"> | ||
| <h2 id="not-found-title">This page is not here</h2> | ||
| <p>This page does not exist, or the link is out of date.</p> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="not-found-actions" aria-label="Useful places to go next"> | ||
| <a href="{{ url_for('task_routes.dashboard') }}" class="btn btn-primary"> | ||
| <i class="fas fa-house"></i> Go to Overview | ||
| </a> | ||
| <a href="{{ url_for('network_file_sharing') }}" class="btn btn-secondary"> | ||
| <i class="fas fa-share-nodes"></i> File Sharing | ||
| </a> | ||
| <a href="{{ url_for('drive_health_routes.drives') }}" class="btn btn-secondary"> | ||
| <i class="fas fa-hard-drive"></i> Drive Health | ||
| </a> | ||
| <a href="{{ url_for('cloud_backup_routes.cloud_backup_page') }}" class="btn btn-secondary"> | ||
| <i class="fas fa-cloud-arrow-up"></i> Cloud Backup | ||
| </a> | ||
| <a href="{{ url_for('alerts_routes.alerts_page') }}" class="btn btn-secondary"> | ||
| <i class="fas fa-bell"></i> Alerts | ||
| </a> | ||
| </div> | ||
|
|
||
| <div class="not-found-path"> | ||
| <span>Requested path</span> | ||
| {# The path is escaped by Jinja, so odd URLs can be shown without becoming HTML. #} | ||
| <code>{{ requested_path }}</code> | ||
| </div> | ||
| </div> | ||
| </section> | ||
| {% endblock %} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if 404 template is referenced in index.html and verify CSS class definitions
# Search for button class usage elsewhere to verify consistency
rg -l 'btn btn-primary|btn btn-secondary' --type=html
# Check if index.html mentions or links to error/404 documentation
grep -n '404\|error\|not found' index.htmlRepository: chrismin13/SimpleSaferServer
Length of output: 442
🏁 Script executed:
# Find CSS files and check for button styling definitions
fd '\.(css|scss)$' --type f | head -20Repository: chrismin13/SimpleSaferServer
Length of output: 144
🏁 Script executed:
# Check base.html to understand the layout structure and Bunker style
head -50 templates/base.htmlRepository: chrismin13/SimpleSaferServer
Length of output: 2620
🏁 Script executed:
# Look for any documentation or style guide files mentioning "Bunker" or button styling
rg -i 'bunker|design\s*system|style\s*guide' --type=md --type=html --type=txtRepository: chrismin13/SimpleSaferServer
Length of output: 2419
🏁 Script executed:
# Compare 404.html with other special pages to verify design consistency
ls -la templates/ | grep -E '(error|404|403|500)'Repository: chrismin13/SimpleSaferServer
Length of output: 132
🏁 Script executed:
# Check the internal UI patterns documentation to understand Bunker style
cat docs/internal_ui_patterns.mdRepository: chrismin13/SimpleSaferServer
Length of output: 3987
🏁 Script executed:
# Check the main styles.css to understand button styling and spacing patterns
cat static/css/styles.css | head -200Repository: chrismin13/SimpleSaferServer
Length of output: 5879
🏁 Script executed:
# Look for any section or panel styling patterns to verify 404 template layout
rg -A 5 '\.not-found-' static/css/styles.css || echo "Pattern not found, checking for similar styling"Repository: chrismin13/SimpleSaferServer
Length of output: 120
🏁 Script executed:
# Check if there's a main index.html file that should link to 404 or documentation
find . -name 'index.html' -type fRepository: chrismin13/SimpleSaferServer
Length of output: 85
🏁 Script executed:
# Check for .not-found styles in the CSS files
rg 'not-found' static/css/Repository: chrismin13/SimpleSaferServer
Length of output: 54
🏁 Script executed:
# Check the actual index.html file
cat index.htmlRepository: chrismin13/SimpleSaferServer
Length of output: 12108
🏁 Script executed:
# Also check for button styling definitions to verify btn classes follow Bunker aesthetic
rg -A 8 '\.btn\b' static/css/styles.css | head -50Repository: chrismin13/SimpleSaferServer
Length of output: 1239
🏁 Script executed:
# Check git history for 404 template to see if CSS was removed
git log --oneline templates/404.html 2>/dev/null | head -5Repository: chrismin13/SimpleSaferServer
Length of output: 121
🏁 Script executed:
# Check if there are any other error pages (403, 500, etc.)
find templates -name '*[34]*.html' -o -name '*error*' -o -name '*exception*'Repository: chrismin13/SimpleSaferServer
Length of output: 91
🏁 Script executed:
# Verify the full styles.css to see if there's more content below what was shown
wc -l static/css/styles.css
tail -100 static/css/styles.css | head -50Repository: chrismin13/SimpleSaferServer
Length of output: 1316
Add CSS styling for the 404 template layout.
The template uses layout classes (.not-found-page, .not-found-panel, .not-found-main, .not-found-code, .not-found-copy, .not-found-actions, .not-found-path) that have no corresponding CSS definitions in static/css/styles.css. The button classes (.btn, .btn-primary, .btn-secondary) correctly follow the Bunker design system, but the page layout itself is unstyled and will not render with proper spacing, frosted-glass card styling, or the Bunker aesthetic required by your design guidelines.
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🤖 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/404.html` around lines 1 - 42, The 404 template in 404.html uses
layout-specific CSS classes (.not-found-page, .not-found-panel, .not-found-main,
.not-found-code, .not-found-copy, .not-found-actions, .not-found-path) that lack
corresponding CSS definitions in static/css/styles.css. Add CSS styling for
these classes to provide proper spacing, alignment, and visual hierarchy. Ensure
the styling follows the Bunker design system aesthetic, including frosted-glass
card effects for the .not-found-panel, appropriate sizing for the
.not-found-code element, and proper layout for the .not-found-actions section
with its navigation buttons.
Source: Coding guidelines
| </div> | ||
|
|
||
| <div class="not-found-actions" aria-label="Useful places to go next"> | ||
| <a href="{{ url_for('task_routes.dashboard') }}" class="btn btn-primary"> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all route references exist and check for naming consistency
# Search for route definitions and their namespaces
rg -B 2 'def (dashboard|network_file_sharing|drives|cloud_backup_page|alerts_page)' --type=pyRepository: chrismin13/SimpleSaferServer
Length of output: 1247
🏁 Script executed:
cat -n templates/404.html | head -35Repository: chrismin13/SimpleSaferServer
Length of output: 1736
🏁 Script executed:
# Check blueprint registration to understand the correct namespace names
rg -B 1 -A 1 'blueprint|register_blueprint' --type=py | head -50Repository: chrismin13/SimpleSaferServer
Length of output: 2050
Align network_file_sharing route with blueprint pattern.
Line 21 uses non-namespaced url_for('network_file_sharing') while lines 18, 24, 27, 30 use blueprinted routes (e.g., url_for('task_routes.dashboard')). The network_file_sharing route is currently registered directly on the app in app_factory.py rather than as part of a blueprint. Move it to a dedicated blueprint module to match the existing pattern, or document why it is intentionally different.
🤖 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/404.html` at line 18, The `network_file_sharing` route reference in
the template uses a non-namespaced `url_for()` call while other routes like
`task_routes.dashboard` use the blueprint pattern with namespacing. To fix this,
either move the `network_file_sharing` route registration from being directly on
the app in `app_factory.py` to a dedicated blueprint module (similar to how
`task_routes` is structured), and update the `url_for()` call to use the
blueprint namespace, or add a comment explaining why this route intentionally
deviates from the blueprint pattern used by other routes in the template.
Comment on lines
+37
to
+38
| {# The path is escaped by Jinja, so odd URLs can be shown without becoming HTML. #} | ||
| <code>{{ requested_path }}</code> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that requested_path is passed to 404 template by checking error handler implementation
# Search for 404 handler implementation and any explicit context passing
rg -A 10 'handle_not_found|errorhandler.*404' --type=py -B 2Repository: chrismin13/SimpleSaferServer
Length of output: 869
🏁 Script executed:
# Search for context processors that might provide requested_path
rg -i 'context_processor|requested_path' --type=py -B 2 -A 2
# Check the actual 404.html to confirm its usage
cat -n templates/404.html
# Check if there's a custom error template configuration
rg -i 'error.*template|errorhandler.*render' --type=py -B 2 -A 5Repository: chrismin13/SimpleSaferServer
Length of output: 2318
🏁 Script executed:
# Get the full context_processor function to see what it provides
rg -A 30 'def inject_template_context' --type=py
# Also check if requested_path is provided anywhere else in the codebase
rg 'requested_path' --type=py -B 2 -A 2Repository: chrismin13/SimpleSaferServer
Length of output: 2437
🏁 Script executed:
# Check the full 404 error handler to see the exact implementation
rg -B 5 -A 15 'def handle_not_found' --type=py
# Check if there's any custom error template configuration in the app factory
rg 'app.errorhandler|render_template.*error|register_error_handler' --type=py -B 2 -A 8Repository: chrismin13/SimpleSaferServer
Length of output: 3541
Add explicit requested_path to the 404 error handler context.
Line 38 uses {{ requested_path }}, but the error handler at app_factory.py returns the bare error object without passing this variable. Update the 404 handler to explicitly render the template with the requested path:
`@app.errorhandler`(404)
def handle_not_found(error):
if request.path.startswith("/api/"):
return json_problem(NotFoundProblem("Not found."))
return render_template('404.html', requested_path=request.path)🤖 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/404.html` around lines 37 - 38, The 404.html template uses the
variable `requested_path` on line 38, but the `handle_not_found` error handler
in app_factory.py does not pass this variable when rendering the template.
Update the `handle_not_found` function (decorated with `@app.errorhandler`(404))
to explicitly pass `requested_path=request.path` as a parameter to the
render_template call so the template has access to the requested path value.
Comment on lines
+420
to
+423
| row.addEventListener('click', (event) => { | ||
| if (event.target.closest('a, button, input, label, select, textarea')) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect row interaction handlers around the changed block
sed -n '400,470p' templates/dashboard.html
# Locate keydown handlers in this template
rg -n -C3 "addEventListener\\((\"|')keydown" templates/dashboard.htmlRepository: chrismin13/SimpleSaferServer
Length of output: 2788
Add interactive-descendant guard to the row keydown handler.
The keydown handler lacks the guard present in the click handler. When a user presses Space or Enter on the toggle checkbox, the keydown event bubbles to the row and triggers navigation, bypassing the toggle's click stopPropagation. Add the same interactive-target check:
row.addEventListener('keydown', (event) => {
if (event.target.closest('a, button, input, label, select, textarea')) {
return;
}
if (event.key === 'Enter' || event.key === ' ') {
event.preventDefault();
window.location.href = row.dataset.href;
}
});🤖 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/dashboard.html` around lines 420 - 423, The keydown handler on the
row element is missing the interactive-descendant guard that exists in the click
handler. When users press Space or Enter on interactive elements like the toggle
checkbox, the event bubbles up and triggers unwanted navigation. Add the same
guard check to the row's keydown event listener using event.target.closest() to
detect interactive elements (a, button, input, label, select, textarea),
returning early if found, before the logic that handles Enter or Space key
presses and navigates to the href.
Comment on lines
+187
to
+189
| assert ( | ||
| 'data-task-name="DDNS Update"' not in page.split('class="task-schedule-toggle"')[0] | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The DDNS toggle absence assertion is under-scoped.
Line 188 inspects only the HTML before the first toggle, so it can miss an unsupported toggle rendered later in the table.
💡 Suggested fix
- assert (
- 'data-task-name="DDNS Update"' not in page.split('class="task-schedule-toggle"')[0]
- )
+ assert 'class="task-schedule-toggle" data-task-name="DDNS Update"' not in page🤖 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_task_schedule_ui_rendering.py` around lines 187 - 189, The
assertion on lines 187-189 is checking for the absence of DDNS Update in only
the portion of the page before the first occurrence of
class="task-schedule-toggle", which creates an under-scoped check that could
miss the DDNS toggle if it appears later in the table. Modify the assertion to
check the entire page content for the absence of data-task-name="DDNS Update"
without restricting the search to only the HTML before the first toggle element,
ensuring the assertion covers all rendered toggles in the page.
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
Release Notes
New Features
Documentation