Sync develop → master (post-v0.17.0)#467
Merged
Merged
Conversation
Delete src/docker/test/ directory (python/ and angular/ subdirs). These Dockerfiles are not referenced by CI, Makefile, or any script. The python Dockerfile references a non-existent base image (seedsync/run/python/devenv). Same cleanup as f7d654f which removed the e2e test directory. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Switch from python:3.12-slim-bookworm (Debian) to python:3.13-alpine so the test image matches the production Alpine base. Replace apt-get with apk, groupadd/useradd with addgroup/adduser. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…erfiles Remove stale Docker test infrastructure
Merge the python-deps and runtime stages into a single python:3.13-alpine stage, eliminating the separate alpine:3.21 base and the COPY --from=python-deps step. - Upgrade Python 3.12 → 3.13 - Remove 6 dead stdlib cleanup entries (distutils, lib2to3, audioop, ossaudiodev, nis, _crypt — all removed from CPython) - Replace pip self-uninstall with direct rm -rf of pip/setuptools - Update CI python-version from 3.12 to 3.13 across all jobs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewrite test-image Dockerfile as Alpine with Python 3.13
…lpine Collapse Dockerfile to 2 stages with Python 3.13-alpine
* Use RUN --mount for uv to avoid 49 MB ghost layer Mount the uv binary from ghcr.io/astral-sh/uv:0.11 during RUN instead of COPY-ing it into a layer. The COPY+delete pattern left a 49.4 MB layer in the image even though uv was removed afterward. The mount approach keeps uv out of all committed layers entirely. Image size: 114 MB → 64 MB. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Restore 3-stage build and use RUN --mount for uv The 2-stage collapse in PR 3 caused a size regression (45 MB → 114 MB) because stdlib cleanup in a RUN layer can't reclaim space from the base image's Python layer. Restore the intermediate python-deps stage so COPY --from only brings stripped files into the final alpine image. Combined with RUN --mount for uv (avoids 49 MB ghost layer), the final image is 48 MB — smaller than the original 45 MB thanks to the Alpine 3.23 upgrade and Python 3.13 stdlib removals. Image size: 114 MB → 48 MB. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Strip additional unused Python artifacts from runtime image Remove C headers (/usr/local/include, 2.4 MB), build config (config-3.13-*, 284 KB), REPL enhancements (_pyrepl, 240 KB), decimal fallback (_pydecimal.py, 224 KB), test extension modules (xx*.so, 204 KB), and ~100 unused encoding codepages (~1.2 MB). Keeps only UTF-8, ASCII, Latin-1, UTF-16/32, charmap, IDNA, and unicode escape encodings — all that LC_ALL=C.UTF-8 needs. Image size: 48 MB → 44 MB. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove apk and unused SSH binaries from runtime image Strip the package manager (apk, libapk, /etc/apk, /lib/apk) and unused openssh binaries (ssh-keyscan, ssh-add, ssh-agent, ssh-pkcs11-helper, ssh-copy-id) plus utility binaries (scanelf, c_rehash, iconv, getconf, ssl_client, findssl.sh) from the final image. The app only needs ssh, scp, and sftp. Prevents installing packages inside a running container and reduces attack surface. Image size: 44 MB → 42.5 MB. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove ssh moduli, exclude ruff cache from build context Remove /etc/ssh/moduli (568 KB) — only needed by sshd (server), not the ssh client. Add .ruff_cache to .dockerignore to exclude 1.3 MB of linter cache from COPY src/python/. Image size: 42.5 MB → 41.5 MB. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Exclude dev artifacts from runtime image Add .pytest_cache, pyrightconfig.json to .dockerignore. Remove pyproject.toml and uv.lock from /app/python/ after COPY (they're needed in the python-deps stage but not at runtime). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Exclude typings/ stubs from runtime image The typings/ directory contains .pyi type stubs used only by Pyright during development. Not needed at runtime. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove missed .so modules, venv, and libapk from runtime Add to python-deps cleanup: _tkinter.so (tkinter dir was removed but not the .so), _xxtestfuzz.so (test module), _lsprof.so (profiler), _interp*.so (sub-interpreters), and venv/ directory. Add /usr/lib/libapk.so* to runtime cleanup (missed in apk removal). Image size: 41.3 MB → 40.9 MB. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t-reload (#433) * Add requiresRestart flag to differentiate config restart notifications Only show the "restart required" notification for settings that truly need a restart (server, staging, extract, scan intervals, autoqueue core, logging, web port/API key). Settings like notifications, validation, exclude patterns, verbose logging, LFTP tuning, and auto_delete_remote take effect immediately and no longer trigger the restart banner. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Flush configs to disk immediately after REST mutations (#427 Phase 1) Add file paths to Context and pass them through WebAppBuilder to each REST handler. After every successful mutation (config set, path pair CRUD, integration CRUD, auto-queue add/remove), the handler now calls to_file() to persist immediately instead of waiting for the 30-second timer. The periodic flush remains as a safety net for ControllerPersist. Test infrastructure updated to create a temp directory for flush paths and clean up in tearDown. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add LFTP hot-reload: reapply tuning settings without restart (#427 Phase 3) Add threading.Event reconfigure flag to Controller with a public request_lftp_reconfigure() method. In process(), when the flag is set, clear it and re-call _configure_lftp() on all pair contexts. ConfigHandler gets an optional on_lftp_config_change callback and a _LFTP_TUNING_KEYS set (14 keys covering parallelism, connections, rate limits, timeouts, etc.). After a successful set_property on an lftp tuning key, the callback fires to signal reconfiguration. WebAppBuilder wires controller.request_lftp_reconfigure into ConfigHandler. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Use (section, key) tuples for LFTP tuning keys; add verbose and xfer_verify Change _LFTP_TUNING_KEYS from a flat set of key names to a frozenset of (section, key) tuples so the lookup is section-aware. Add ("general", "verbose") and ("validate", "xfer_verify") which also feed into configure_lftp via the Controller. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix Pyright errors and add validate.algorithm to LFTP hot-reload keys - Assert context paths are non-None in WebAppBuilder to satisfy Pyright str vs str|None type check - Add ("validate", "algorithm") to _LFTP_TUNING_KEYS since it affects xfer:verify-command in configure_lftp() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Replace assert with explicit RuntimeError for context path checks Asserts are stripped with python -O. Explicit RuntimeError with descriptive messages is more robust and Pyright can narrow the types. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Add E2E tests for file actions and error states - file-actions.spec.ts: 11 tests covering file row selection, action button visibility, disabled states, delete confirmation double-click pattern, selection switching, and bulk action bar presence - error-states.spec.ts: 5 tests covering no-enabled-pairs warning notification, notification text/level, restart notification on config change, and re-enabling a pair clears the warning Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix E2E test failures and address CodeRabbit review comments - error-states: Extract disableAllPairs/restorePairs helpers that always ensure a disabled pair exists (fixes all 4 failures -- the backend only sets noEnabledPairs when pairs exist but none are enabled) - error-states: Assert res.ok on every API setup/teardown call - error-states: Move temp pair cleanup into finally via restorePairs helper - error-states: Scope restart notification locator to #header .alert - settings: Remove separate clear() before fill() to avoid Angular signal re-render race; add blur() and toBeEnabled wait; increase poll timeout - settings.page: Scope getRestartNotification() to #header .alert - file-actions: Assert .actions container is hidden after deselection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Skip noEnabledPairs E2E tests when server is not fully configured The noEnabledPairs notification requires status.server.up === true, which only happens when the server has valid config (remote address, paths, etc.). CI containers start with incomplete config so the controller never evaluates pair state. Skip these tests when the server reports as not up, matching the existing skip pattern used in dashboard.spec.ts for file-dependent tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Add Python integration tests to CI Add a new CI job that runs the web handler integration tests (tests/integration/test_web). These use webtest.TestApp with mocks and don't require external dependencies (lftp, SSH). The heavier integration tests (test_lftp, test_controller) that require real lftp/SSH are excluded — they need the Docker test container which is a separate concern. Gate the publish job on this new job passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix pre-existing integration test failures exposed by CI - test_auto_queue: use resp.text/resp.json instead of resp.html (handlers now return text/plain and application/json, not text/html) - test_stream_*: increase Timer delays from 0.5s to 2.0s to prevent race conditions where the stop timer fires before update timers on slow CI runners Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover all four security subsystems in web/security.py: - CSRF protection: safe method exemption, loopback bypass, origin/referer matching, port normalization, proxy header detection - Rate limiting: per-IP tracking, limit enforcement, stale sweep, SSE exemption, disable flag, X-Forwarded-For extraction - Security headers: nosniff, X-Frame-Options, CSP, Referrer-Policy - API key auth: enabled/disabled states, exempt paths, SSE query param, header validation Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover four previously untested modules: - pair_context: PairContext init state, validate_config (missing fields, extract path logic), configure_lftp (mandatory/optional settings, xfer_verify) - active_scanner: empty scan, set_active_files, queue draining, missing file debug logging, unexpected error warning logging - local_scanner: missing path handling, successful scan, ScannerError, temp file suffix - delete_process: local file/dir deletion, path traversal blocking, remote SSH command construction, tilde path escaping Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Add web app job and context unit tests (14 tests) Cover two previously untested modules: - web_app_job: setup creates server and starts thread, execute calls process, cleanup stops server and joins thread, never-initialized server logs warning, request logging middleware logs method/path/status - context: create_child_context returns copy with child logger, print_to_log redacts sensitive fields, handles present/absent path pairs, Args.as_dict serializes all fields as strings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix assertLogs/assertRaises nesting in error logging test assertLogs must be the outer context so it can validate captured log records on normal exit. With assertRaises as the outer context, the RuntimeError propagated through assertLogs preventing its validation from running — making the log assertion a no-op. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Expand handler integration tests (+15 tests) Add integration tests to three existing handler test files: - test_server (+5): response body text, 200 status, state transition False->True, restart idempotency, state persistence - test_status (+5): full response structure (server + controller sections), null scan times initially, no_enabled_pairs reflects actual state - test_stream_log (+5): QueueLogHandler attach/remove, multiple handlers independent operation, CachedQueueLogHandler delivers history, cache=0 sends no historical records Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix ruff format: add missing blank line between classes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add missing status assertions and fix stream_log docstring - test_status: assert latest_remote_scan_failed and latest_remote_scan_error are present in controller response (both are emitted by the serializer but were not checked) - test_stream_log: fix docstring that incorrectly described QueueLogHandler connection behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Test NameFilterCriteria (null/empty match-all, case-insensitive substring, space-dot fuzzy matching), StatusFilterCriteria (null match-all, specific status filtering), AndFilterCriteria (both-match/either-fails), and service integration (option changes trigger setFilterCriteria, redundant values suppressed). Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- integrations.page.ts: Page object scoped to .integrations container with helpers for form filling, instance rows, edit/delete/test buttons - integrations.spec.ts: 10 tests covering empty state, add form with kind pre-selection, validation error on empty name, create/edit/delete flows with API verification, double-click delete confirmation, test connection, enable/disable toggle, and cancel without creating Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AutoQueueService: 14 tests covering connect/disconnect pattern fetch, empty/whitespace/duplicate pattern rejection, successful add with double-encoding, server rejection preserving state, remove filtering, and graceful degradation on GET failure. PathPairsService: 11 tests covering connect/disconnect pair fetch, create POST with append, 409 conflict rethrow, update PUT with replace, remove DELETE with filter, and error handling. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…448) FileOptionsComponent: 8 tests covering status filter enablement based on file list contents, reactive updates, and delegation to ViewFileOptionsService. IntegrationsComponent: 12 tests covering add/edit/cancel forms, empty name validation, service create delegation, double-click delete confirmation with 3-second timeout reset, ngOnDestroy timer cleanup, and error handling (null response, 409 conflict). OptionComponent: 9 tests covering debounce pipe logic (1000ms delay, rapid changes emit only final value, distinctUntilChanged), password REDACTED_SENTINEL suppression, and effectiveChoices computed signal. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HeaderComponent: 12 tests covering server-down DANGER notification, recovery hiding, waiting-for-scan INFO, remote-scan-failure WARNING, no-enabled-pairs WARNING, text replacement, duplicate suppression, rule coexistence, and cleanup on recovery. VersionCheckService: 8 tests covering newer/same/older version detection, v-prefix stripping, network error graceful degradation, malformed JSON handling, and URL inclusion in notification text. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Cache npm deps via setup-node cache option - Use npm ci instead of npm install for deterministic installs - Cache Playwright browser binaries (~150 MB) in ~/.cache/ms-playwright - On cache hit, only install system deps (apt packages) since browsers are already cached - Cache key includes package-lock.json hash so it invalidates on Playwright version bumps Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Playwright caching added in #452 references src/e2e-playwright/package-lock.json, but the root .gitignore was filtering all package-lock.json files except the angular and website ones. With no lockfile in the repo, actions/setup-node failed at "Some specified paths were not resolved, unable to cache dependencies" and the npm ci step would have failed too. Add an allow-list entry for src/e2e-playwright and check in the generated lockfile. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add static badges showing test counts: 828 Python (697 unit + 131 integration), 412 Angular, 95 E2E. Update Python badge from 3.12 to 3.13 to match the Docker image upgrade. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Expand E2E settings tests with 8 new tests Add coverage for previously untested settings sections: - Staging Directory: path field save, use staging checkbox toggle - Archive Extraction: extract in downloads directory toggle - Integrity Check: verify checkbox + algorithm select presence, algorithm save - Connections: max parallel downloads field save - Notifications: Discord/Telegram webhook field presence - Logging: log level dropdown persistence after page reload Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix E2E: use apiSetConfig for cleanup instead of page interaction The hash algorithm select test's finally block used UI interaction (selectOption) to restore the original value, but the page/browser may already be torn down by that point. Switch to apiSetConfig which makes a direct HTTP call and doesn't depend on the page being open. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix E2E: enable integrity check before testing algorithm select The Hash Algorithm select is disabled when xfer_verify is false (the default). Enable it via API before testing, and restore the original state in the finally block. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix E2E: set xfer_verify before navigation, use waitForStream Set the xfer_verify config via API BEFORE navigating to the settings page, so the page loads with the select already enabled. Use waitForStream to ensure SSE config delivery before interacting. Increase toBeEnabled timeout to 10s for CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix E2E: wait for config value before asserting select enabled The algorithm select is disabled when its value is null/undefined (option.component.html). Wait for the SSE config delivery to populate the select value before asserting it's enabled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Updates the Features list with file integrity validation, Discord/Telegram/webhook notifications, Sonarr/Radarr integration, staging directory, and API key auth. Moves the hard-link Recommended Workflow up to follow Quick Start, adds Configuration subsections for the new settings, and lists /staging in the volumes table. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Fix flaky Hash Algorithm select wait condition
The Integrity Check select test waited with not.toHaveValue("") for the
config to arrive via SSE, but Angular renders selects bound to a null
model with a non-empty "? null:null ?" placeholder option. That caused
the assertion to pass while the model was still null, after which
toBeEnabled would deterministically fail because value() === null still
disables the select.
Wait on a real algorithm value (md5|sha1|sha256) instead. The
toBeEnabled check is left in place as an explicit confirmation of the
gate the comment describes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Wait for SSE-delivered value before asserting persisted log level
After reloading the settings page, the test asserted on the Log Level
select's value with only the default 5 s expect timeout. Recent CI runs
showed this test finishing at ~4.2 s, leaving little headroom — and the
margin shrinks once Playwright workers go up.
settings.goto() only waits for Angular's sidebar render (before SSE
config delivery), so toBeEnabled is the right gate: the disable
condition is `value() === null || value() === undefined`, so an enabled
select implies the model has received the SSE value. This mirrors the
pattern used in the Hash Algorithm test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Use validate.enabled to unlock Hash Algorithm select in test
The original setup set validate.xfer_verify = True under the assumption
that this enabled the algorithm select. It does not. The disable rule
lives in buildValidateContext() in settings-page.component.ts and is
gated on config.validate.enabled (post-download validation), not on
xfer_verify (inline transfer verification). So the select stayed
disabled regardless of the SSE-delivered model value, and toBeEnabled
deterministically failed.
Switch the setup to toggle validate.enabled, and reorder the assertions
so toBeEnabled (the slower wait, since it depends on both SSE delivery
AND the dynamic context rebuild) gates the toHaveValue check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Run Build and Test on develop pushes and bump Playwright workers to 5
Two CI throughput improvements rolled up.
1. Drop the develop-push skip on Build and Test (amd64). With it in place
the job never ran on develop merges, so the actions/cache step never
populated a develop-scoped Playwright browser cache. PRs targeting
develop fall back to looking at their own branch (empty for fresh
PRs) and at develop (also empty), and pay the full browser+system-deps
install cost every run. Letting the job run on develop pushes warms
the cache once per merge so subsequent PRs hit it.
2. Set workers: 5 in playwright.config.ts. Default on a 4-vCPU runner is
~2 workers, which the user observed as "two tests at a time". Bumping
to 5 lets five spec files run in parallel. fullyParallel stays false,
so within a file tests still run serially.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Update playwright.config.ts
* Move pair-disable test to path-pairs.spec.ts to avoid cross-file race
settings.spec.ts:176 created and deleted a temporary path pair to
verify the Server Directory field becomes disabled. With workers > 2
that test ran in parallel with path-pairs.spec.ts, whose beforeEach
and afterEach delete every existing pair to start from a clean slate.
The result was a 404 on cleanup ("Failed to delete temp pair … 404")
and intermittently the assertion itself, depending on which spec's
hook fired first.
Move the test into path-pairs.spec.ts so it shares the same per-file
serialized cleanup. No other spec creates pairs from outside its own
describe block, which removes the cross-file pair race entirely.
integrations.spec.ts has the same nuclear-cleanup pattern but no other
spec creates integrations, so it isn't affected today — flagging it
as a pattern to watch as we add more cross-cutting tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Drop Playwright workers from 10 to 4 to match runner CPU count
ubuntu-latest has 4 vCPUs. With workers: 10 we ran 10 chromium
processes plus the SeedSync container on those 4 cores, ~2.5x CPU
oversubscription. The suite ran slower overall — context-switching
overhead and contention on the shared backend dominated any
parallelism gain. With fullyParallel: false the per-file serialization
also caps useful parallelism at the spec count, and the longest spec
(settings) sets the floor regardless of worker count.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Drop Playwright workers from 4 to 3
Leave one of the runner's 4 vCPUs free for the SeedSync Docker
container and runner overhead instead of saturating all four with
chromium worker processes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Run E2E in official Playwright container; drop workers to 2
Replace the host-installed Playwright pipeline with a single docker run
of mcr.microsoft.com/playwright:v<lockfile-version>-noble. That image
ships chromium plus all system deps prebaked, so we eliminate:
- Set up Node.js (cache: npm)
- Install Playwright dependencies (npm ci on host)
- Cache Playwright browsers (actions/cache)
- Install Playwright browsers (npx playwright install)
- Install Playwright system deps (apt install fonts/xfonts)
The version pin is read from package-lock.json with jq so a future
@playwright/test bump auto-bumps the image tag — the chromium binary
in the image must match the @playwright/test API expectations.
Container-to-SUT networking uses --network host so localhost:8800
still resolves to the seedsync test-container started in the previous
step.
Also drop workers from 3 to 2: with the Playwright image now running
as another process group on the same 4-vCPU runner, the previous
"leave one core for runner overhead" math becomes "leave two cores
for runner + Playwright container coordination."
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Bump Playwright workers back to 4 for the new container baseline
Trying 4 (one per vCPU) now that the system-deps install is gone and
chromium runs inside the official Playwright image instead of on the
host. If we see contention or test races we can drop again.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Test setUp methods across the suite added a fresh StreamHandler to a shared logger (mostly the root logger) and never removed it. Because loggers are singletons, by test N the logger held N handlers and each log line printed N times. Test correctness wasn't affected — no assertion depends on handler count — only output noise. Use unittest's addCleanup hook to remove the handler after each test. addCleanup runs even if setUp partially fails after the registration, so the cleanup is robust against initialization errors. Touches 10 setUp methods across 8 files (TestAppProcess / TestAppOneShotProcess and TestDeleteLocalProcess / TestDeleteRemoteProcess each have their own setUp). Closes #450 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bumps [@docusaurus/preset-classic](https://github.com/facebook/docusaurus/tree/HEAD/packages/docusaurus-preset-classic) from 3.10.0 to 3.10.1. - [Release notes](https://github.com/facebook/docusaurus/releases) - [Changelog](https://github.com/facebook/docusaurus/blob/main/CHANGELOG.md) - [Commits](https://github.com/facebook/docusaurus/commits/v3.10.1/packages/docusaurus-preset-classic) --- updated-dependencies: - dependency-name: "@docusaurus/preset-classic" dependency-version: 3.10.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@docusaurus/faster](https://github.com/facebook/docusaurus/tree/HEAD/packages/docusaurus-faster) from 3.10.0 to 3.10.1. - [Release notes](https://github.com/facebook/docusaurus/releases) - [Changelog](https://github.com/facebook/docusaurus/blob/main/CHANGELOG.md) - [Commits](https://github.com/facebook/docusaurus/commits/v3.10.1/packages/docusaurus-faster) --- updated-dependencies: - dependency-name: "@docusaurus/faster" dependency-version: 3.10.1 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…463) Bumps the angular group in /src/angular with 10 updates: | Package | From | To | | --- | --- | --- | | [@angular/cdk](https://github.com/angular/components) | `21.2.8` | `21.2.9` | | [@angular/common](https://github.com/angular/angular/tree/HEAD/packages/common) | `21.2.10` | `21.2.11` | | [@angular/compiler](https://github.com/angular/angular/tree/HEAD/packages/compiler) | `21.2.10` | `21.2.11` | | [@angular/core](https://github.com/angular/angular/tree/HEAD/packages/core) | `21.2.10` | `21.2.11` | | [@angular/forms](https://github.com/angular/angular/tree/HEAD/packages/forms) | `21.2.10` | `21.2.11` | | [@angular/platform-browser](https://github.com/angular/angular/tree/HEAD/packages/platform-browser) | `21.2.10` | `21.2.11` | | [@angular/router](https://github.com/angular/angular/tree/HEAD/packages/router) | `21.2.10` | `21.2.11` | | [@angular/build](https://github.com/angular/angular-cli) | `21.2.8` | `21.2.9` | | [@angular/cli](https://github.com/angular/angular-cli) | `21.2.8` | `21.2.9` | | [@angular/compiler-cli](https://github.com/angular/angular/tree/HEAD/packages/compiler-cli) | `21.2.10` | `21.2.11` | Updates `@angular/cdk` from 21.2.8 to 21.2.9 - [Release notes](https://github.com/angular/components/releases) - [Changelog](https://github.com/angular/components/blob/main/CHANGELOG.md) - [Commits](angular/components@v21.2.8...v21.2.9) Updates `@angular/common` from 21.2.10 to 21.2.11 - [Release notes](https://github.com/angular/angular/releases) - [Changelog](https://github.com/angular/angular/blob/main/CHANGELOG.md) - [Commits](https://github.com/angular/angular/commits/v21.2.11/packages/common) Updates `@angular/compiler` from 21.2.10 to 21.2.11 - [Release notes](https://github.com/angular/angular/releases) - [Changelog](https://github.com/angular/angular/blob/main/CHANGELOG.md) - [Commits](https://github.com/angular/angular/commits/v21.2.11/packages/compiler) Updates `@angular/core` from 21.2.10 to 21.2.11 - [Release notes](https://github.com/angular/angular/releases) - [Changelog](https://github.com/angular/angular/blob/main/CHANGELOG.md) - [Commits](https://github.com/angular/angular/commits/v21.2.11/packages/core) Updates `@angular/forms` from 21.2.10 to 21.2.11 - [Release notes](https://github.com/angular/angular/releases) - [Changelog](https://github.com/angular/angular/blob/main/CHANGELOG.md) - [Commits](https://github.com/angular/angular/commits/v21.2.11/packages/forms) Updates `@angular/platform-browser` from 21.2.10 to 21.2.11 - [Release notes](https://github.com/angular/angular/releases) - [Changelog](https://github.com/angular/angular/blob/main/CHANGELOG.md) - [Commits](https://github.com/angular/angular/commits/v21.2.11/packages/platform-browser) Updates `@angular/router` from 21.2.10 to 21.2.11 - [Release notes](https://github.com/angular/angular/releases) - [Changelog](https://github.com/angular/angular/blob/main/CHANGELOG.md) - [Commits](https://github.com/angular/angular/commits/v21.2.11/packages/router) Updates `@angular/build` from 21.2.8 to 21.2.9 - [Release notes](https://github.com/angular/angular-cli/releases) - [Changelog](https://github.com/angular/angular-cli/blob/main/CHANGELOG.md) - [Commits](angular/angular-cli@v21.2.8...v21.2.9) Updates `@angular/cli` from 21.2.8 to 21.2.9 - [Release notes](https://github.com/angular/angular-cli/releases) - [Changelog](https://github.com/angular/angular-cli/blob/main/CHANGELOG.md) - [Commits](angular/angular-cli@v21.2.8...v21.2.9) Updates `@angular/compiler-cli` from 21.2.10 to 21.2.11 - [Release notes](https://github.com/angular/angular/releases) - [Changelog](https://github.com/angular/angular/blob/main/CHANGELOG.md) - [Commits](https://github.com/angular/angular/commits/v21.2.11/packages/compiler-cli) --- updated-dependencies: - dependency-name: "@angular/cdk" dependency-version: 21.2.9 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/common" dependency-version: 21.2.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/compiler" dependency-version: 21.2.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/core" dependency-version: 21.2.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/forms" dependency-version: 21.2.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/platform-browser" dependency-version: 21.2.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/router" dependency-version: 21.2.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/build" dependency-version: 21.2.9 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/cli" dependency-version: 21.2.9 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: angular - dependency-name: "@angular/compiler-cli" dependency-version: 21.2.11 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: angular ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [typescript-eslint](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/typescript-eslint) from 8.59.0 to 8.59.1. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/typescript-eslint/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v8.59.1/packages/typescript-eslint) --- updated-dependencies: - dependency-name: typescript-eslint dependency-version: 8.59.1 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [jsdom](https://github.com/jsdom/jsdom) from 29.0.2 to 29.1.1. - [Release notes](https://github.com/jsdom/jsdom/releases) - [Commits](jsdom/jsdom@v29.0.2...v29.1.1) --- updated-dependencies: - dependency-name: jsdom dependency-version: 29.1.1 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@docusaurus/module-type-aliases](https://github.com/facebook/docusaurus/tree/HEAD/packages/docusaurus-module-type-aliases) from 3.10.0 to 3.10.1. - [Release notes](https://github.com/facebook/docusaurus/releases) - [Changelog](https://github.com/facebook/docusaurus/blob/main/CHANGELOG.md) - [Commits](https://github.com/facebook/docusaurus/commits/v3.10.1/packages/docusaurus-module-type-aliases) --- updated-dependencies: - dependency-name: "@docusaurus/module-type-aliases" dependency-version: 3.10.1 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) Bumps [eslint](https://github.com/eslint/eslint) from 10.2.1 to 10.3.0. - [Release notes](https://github.com/eslint/eslint/releases) - [Commits](eslint/eslint@v10.2.1...v10.3.0) --- updated-dependencies: - dependency-name: eslint dependency-version: 10.3.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@docusaurus/types](https://github.com/facebook/docusaurus/tree/HEAD/packages/docusaurus-types) from 3.10.0 to 3.10.1. - [Release notes](https://github.com/facebook/docusaurus/releases) - [Changelog](https://github.com/facebook/docusaurus/blob/main/CHANGELOG.md) - [Commits](https://github.com/facebook/docusaurus/commits/v3.10.1/packages/docusaurus-types) --- updated-dependencies: - dependency-name: "@docusaurus/types" dependency-version: 3.10.1 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…462) Bumps [@docusaurus/core](https://github.com/facebook/docusaurus/tree/HEAD/packages/docusaurus) from 3.10.0 to 3.10.1. - [Release notes](https://github.com/facebook/docusaurus/releases) - [Changelog](https://github.com/facebook/docusaurus/blob/main/CHANGELOG.md) - [Commits](https://github.com/facebook/docusaurus/commits/v3.10.1/packages/docusaurus) --- updated-dependencies: - dependency-name: "@docusaurus/core" dependency-version: 3.10.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR upgrades Python CI from 3.12 to 3.13, implements persistent configuration with hot-reload for LFTP settings via Context/handler path parameters and Controller signaling, removes legacy Docker test images, migrates E2E tests to Docker-based Playwright execution, expands Angular/Python test coverage with unit/integration tests, adds ChangesConfiguration Persistence & Runtime LFTP Reconfiguration
Testing Infrastructure, Coverage & CI Modernization
Dependency Updates & Documentation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/angular/package.json (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVersion field must be bumped before merge.
The
versionfield is still0.17.0. The pre-merge checklist in this PR explicitly requires a version bump here, and the coding guidelines require this field to reflect the release version (used on the About page).🔖 Proposed fix
- "version": "0.17.0", + "version": "0.18.0",(Adjust the target version to whatever this release is being tagged as.)
As per coding guidelines: "Update the version in
src/angular/package.jsonto match the release version for display on the About 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 `@src/angular/package.json` at line 3, The package.json "version" field is still set to "0.17.0"; update that "version" property in package.json to the correct release version (the value this PR will be tagged with) so the About page shows the proper release number—replace "0.17.0" with the new release string (e.g., "0.18.0" or whatever the target release is).
🤖 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 `@src/angular/src/app/pages/settings/option.component.spec.ts`:
- Around line 15-16: Export the existing private debounce constant from
option.component.ts as a top-level exported const named DEBOUNCE_TIME_MS
(alongside existing exports like OptionType and OptionValue), then update this
spec to import DEBOUNCE_TIME_MS instead of redefining it; ensure the component
uses the exported const internally (replace the private constant usage in the
component with the exported symbol) so the test and component share a single
source of truth.
- Around line 90-125: Tests that call vi.useFakeTimers() inline can leak fake
timers if an assertion fails; add a suite-level cleanup by adding afterEach(()
=> vi.useRealTimers()) to this spec so real timers are always restored, and
remove the per-test vi.useRealTimers() calls inside the three it blocks that
currently call vi.useFakeTimers(); keep the existing vi.useFakeTimers() calls in
the tests ('should suppress REDACTED_SENTINEL for password type', 'should pass
real password value through to Subject', 'should not suppress REDACTED_SENTINEL
for non-password types') and leave component.onChange and OptionType usage
unchanged.
In `@src/angular/src/app/services/settings/path-pairs.service.spec.ts`:
- Around line 197-212: Rename the spec to reflect actual behavior: change the
test name from 'should preserve existing list when refresh fails' to something
like 'should clear pairs when refresh fails (catchError emits empty array)';
update any inline comment to remove the contradictory wording and reference the
implemented behavior in PathPairsService (the refresh() call and its
catchError(of([])) path that leads to pairsSubject.next([]) and the pairs$
observable); ensure the assertion still checks expect(result).toEqual([]) and
that the test continues to subscribe to service.pairs$ to capture the emitted
empty array.
In `@src/angular/src/app/services/utils/version-check.service.spec.ts`:
- Around line 66-77: The test in the spec uses a hardcoded release string;
update the “should not show notification when release is same version” test to
import packageJson and pass the actual app version to makeGithubResponse (use
'v' + packageJson.version) when stubbing mockRestService.sendRequest so the test
continues to validate "same version" behavior; locate the test around the
makeGithubResponse call and the
createService()/notificationService.notifications$ usage and swap the hardcoded
'v0.17.0' for the dynamic 'v'+packageJson.version import.
In `@src/docker/build/docker-image/Dockerfile`:
- Line 17: The FROM line uses the floating tag "python:3.13-alpine" which can
pull a different Alpine minor than the runtime; update the Dockerfile's builder
stage reference (the FROM instruction currently using python:3.13-alpine) to the
pinned image "python:3.13-alpine3.23" so the Python builder and the pinned
alpine:3.23 runtime share the same Alpine minor version.
In `@src/docker/build/test-image/Dockerfile`:
- Around line 6-9: The Dockerfile RUN line uses the floating image tag
ghcr.io/astral-sh/uv:0.11 which can change with future 0.11.x patches; update
that reference in the RUN
--mount=from=ghcr.io/astral-sh/uv:0.11,source=/uv,target=/tmp/uv to a pinned
patch release (e.g., ghcr.io/astral-sh/uv:0.11.8) or, for maximum
reproducibility, replace it with the image digest (SHA256) form so the mounted
/tmp/uv content is deterministic during builds.
In `@src/e2e-playwright/tests/integrations.spec.ts`:
- Around line 8-17: The setup currently continues when the list or delete API
calls fail; change test.beforeEach (and the similar beforeAll/other hook) to
fail fast by checking response.ok after the GET to "/server/integrations" and
after each DELETE to "/server/integrations/${inst.id}" and throwing an Error
(including status/code and body or statusText) when a response is not ok so the
test stops immediately instead of proceeding with stale state; locate the logic
in test.beforeEach, the apiFetch calls, and the loop over inst to add these
checks.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 222-225: Replace the race-prone clear()+fill() sequence with the
stable pattern: await field.toBeEnabled(); await field.fill(testValue); await
field.blur(); i.e., remove calls to field.clear() and instead ensure the input
is enabled then fill and blur to trigger save; apply this change to both places
where field.clear(); field.fill(...) is used (the new occurrence and the other
use around the same test in settings.spec.ts).
In `@src/python/tests/integration/test_web/test_handler/test_status.py`:
- Around line 15-55: Replace instances of json.loads(str(resp.html)) with
resp.json in the tests (e.g., in test_full_response_structure,
test_remote_scan_time_null_initially, test_local_scan_time_null_initially,
test_no_enabled_pairs_reflects_state, test_no_enabled_pairs_true_when_set) so
each GET /server/status call uses resp.json to parse the response; after
updating all occurrences, remove the now-unused module-level import json if it
has no other uses.
In `@src/python/tests/unittests/test_common/test_context.py`:
- Around line 13-15: The helper _make_context() is leaking StreamHandler
instances by calling logging.getLogger("test_context") then creating and adding
handler; remove the creation/attachment of handler (the three lines creating
handler and logger.addHandler(handler)) so the function does not modify the
shared logger, or if you must attach, register cleanup with logger.removeHandler
via self.addCleanup(logger.removeHandler, handler); also remove the now-unused
import sys; reference _make_context(), logging.getLogger("test_context"),
handler, and TestCreateChildContext when locating the change.
In `@src/python/tests/unittests/test_controller/test_scan/test_active_scanner.py`:
- Around line 26-30: Tests create ActiveScanner instances (e.g., scanner =
ActiveScanner("/local")) but only call scanner.close() at the end, so failures
can leak resources; immediately register cleanup after construction using the
test framework's cleanup mechanism (call self.addCleanup(scanner.close) right
after each ActiveScanner(...) creation) so close() runs even if assertions
fail—apply this change to every test in this file that constructs ActiveScanner
(instances referenced by symbol scanner in the tests at the shown locations).
In `@src/python/tests/unittests/test_web/test_web_app_job.py`:
- Around line 14-24: The _make_context method attaches a StreamHandler to the
shared logger "test_web_app_job" and never removes it, leaking handlers across
tests; modify _make_context to register cleanup that removes the handler after
each test (e.g., call self.addCleanup(logger.removeHandler, handler) or
self.addCleanup(lambda: logger.removeHandler(handler)) immediately after
logger.addHandler(handler)), or alternatively use logger.handlers manipulation
to ensure the handler is removed in tearDown; reference the _make_context
function, the logger variable and handler so the handler is removed after each
test run.
In `@src/python/web/handler/auto_queue.py`:
- Around line 47-49: The code mutates state via
self.__auto_queue_persist.add_pattern/remove_pattern and then writes with
self.__auto_queue_persist.to_file(self.__persist_path) but does not catch IO
errors; wrap the persistence call in a try/except that catches Exception (or
specific IO errors), log the exception (including the error object) and return a
controlled HTTP 5xx response (e.g., HTTPResponse(status=500, body="failed to
persist auto-queue")) instead of letting the exception bubble; apply this change
around the places that call add_pattern + to_file and remove_pattern + to_file
so both mutating endpoints handle persistence failures gracefully.
In `@src/python/web/handler/config.py`:
- Around line 76-79: Do not mutate self.__config before persistence; instead
create a copy of the section/config (referencing inner_config and
self.__config), apply inner_config.set_property(key, value) on the copy, attempt
to write the updated config to disk via
self.__config.to_file(self.__config_path) (or write to a temp file and
atomically replace), and only if the write succeeds replace self.__config with
the updated config and then call self.__on_lftp_config_change() if present;
ensure exceptions during to_file prevent in-memory changes and avoid calling the
LFTP callback.
In `@src/python/web/handler/integrations.py`:
- Around line 113-117: The code detaches the integration via
self.__path_pairs_config.detach_arr_target(instance_id) but then writes
self.__config.to_file before persisting self.__path_pairs_config.to_file,
risking dangling references if the process crashes; change the save order so you
call self.__path_pairs_config.to_file(self.__path_pairs_path) immediately after
detach_arr_target (persisting the detached path-pairs), then update/write the
integrations config via self.__config.to_file(self.__integrations_path) and
return the HTTPResponse; keep the same methods
(__path_pairs_config.detach_arr_target, __path_pairs_config.to_file, and
__config.to_file) and paths (__path_pairs_path, __integrations_path).
In `@src/python/web/handler/path_pairs.py`:
- Around line 117-118: The persistence calls using
self.__config.to_file(self.__path_pairs_path) (invoked in the
create/update/delete handlers that return
HTTPResponse(body=json.dumps(pair.to_dict()), status=201, ...) and similar
returns) must be wrapped in try/except to catch failures and return a
deterministic 5xx response; modify the handlers that call self.__config.to_file
(and the code paths that currently return status 201 or 204) to try to write,
and on exception log the error and return HTTPResponse(status=500,
body=json.dumps({"error":"failed to persist path pairs"}),
headers={"Content-Type":"application/json"}) so that persistence failures do not
surface as unhandled request errors.
---
Outside diff comments:
In `@src/angular/package.json`:
- Line 3: The package.json "version" field is still set to "0.17.0"; update that
"version" property in package.json to the correct release version (the value
this PR will be tagged with) so the About page shows the proper release
number—replace "0.17.0" with the new release string (e.g., "0.18.0" or whatever
the target release is).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76f0d793-ceb4-424d-b10f-063ed4ceca7d
⛔ Files ignored due to path filters (3)
src/angular/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsonsrc/e2e-playwright/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsonwebsite/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (60)
.github/workflows/ci.yml.gitignoreREADME.mdsrc/angular/package.jsonsrc/angular/src/app/pages/files/file-options.component.spec.tssrc/angular/src/app/pages/main/header.component.spec.tssrc/angular/src/app/pages/settings/integrations.component.spec.tssrc/angular/src/app/pages/settings/option.component.spec.tssrc/angular/src/app/pages/settings/options-list.tssrc/angular/src/app/pages/settings/settings-page.component.htmlsrc/angular/src/app/pages/settings/settings-page.component.tssrc/angular/src/app/services/autoqueue/autoqueue.service.spec.tssrc/angular/src/app/services/files/view-file-filter.service.spec.tssrc/angular/src/app/services/settings/path-pairs.service.spec.tssrc/angular/src/app/services/utils/version-check.service.spec.tssrc/docker/build/docker-image/Dockerfilesrc/docker/build/docker-image/Dockerfile.dockerignoresrc/docker/build/test-image/Dockerfilesrc/docker/test/angular/Dockerfilesrc/docker/test/angular/compose.ymlsrc/docker/test/python/Dockerfilesrc/docker/test/python/compose.ymlsrc/docker/test/python/entrypoint.shsrc/e2e-playwright/playwright.config.tssrc/e2e-playwright/tests/error-states.spec.tssrc/e2e-playwright/tests/file-actions.spec.tssrc/e2e-playwright/tests/integrations.spec.tssrc/e2e-playwright/tests/pages/integrations.page.tssrc/e2e-playwright/tests/pages/settings.page.tssrc/e2e-playwright/tests/path-pairs.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/python/common/context.pysrc/python/controller/controller.pysrc/python/seedsync.pysrc/python/tests/integration/test_controller/test_controller.pysrc/python/tests/integration/test_lftp/test_lftp.pysrc/python/tests/integration/test_web/test_handler/test_auto_queue.pysrc/python/tests/integration/test_web/test_handler/test_server.pysrc/python/tests/integration/test_web/test_handler/test_status.pysrc/python/tests/integration/test_web/test_handler/test_stream_log.pysrc/python/tests/integration/test_web/test_handler/test_stream_model.pysrc/python/tests/integration/test_web/test_handler/test_stream_status.pysrc/python/tests/integration/test_web/test_web_app.pysrc/python/tests/unittests/test_common/test_app_process.pysrc/python/tests/unittests/test_common/test_context.pysrc/python/tests/unittests/test_controller/test_delete/__init__.pysrc/python/tests/unittests/test_controller/test_delete/test_delete_process.pysrc/python/tests/unittests/test_controller/test_pair_context.pysrc/python/tests/unittests/test_controller/test_scan/test_active_scanner.pysrc/python/tests/unittests/test_controller/test_scan/test_local_scanner.pysrc/python/tests/unittests/test_lftp/test_lftp.pysrc/python/tests/unittests/test_ssh/test_sshcp.pysrc/python/tests/unittests/test_web/test_security.pysrc/python/tests/unittests/test_web/test_web_app_job.pysrc/python/web/handler/auto_queue.pysrc/python/web/handler/config.pysrc/python/web/handler/integrations.pysrc/python/web/handler/path_pairs.pysrc/python/web/web_app_builder.pywebsite/package.json
💤 Files with no reviewable changes (5)
- src/docker/test/python/entrypoint.sh
- src/docker/test/python/compose.yml
- src/docker/test/angular/Dockerfile
- src/docker/test/python/Dockerfile
- src/docker/test/angular/compose.yml
This was referenced May 5, 2026
* Address CodeRabbit test-quality findings on develop All test-side hardening only — no production code changes. The bigger production findings (auto_queue/path_pairs/config/integrations error handling and ordering) are deferred for individual PRs with proper test coverage rather than bundled here. - option.component.spec.ts: add suite-level afterEach(useRealTimers) so a failing assertion inside vi.useFakeTimers() can't leak fake timers into the next test. Removed the per-test useRealTimers() calls that the new afterEach makes redundant. - path-pairs.service.spec.ts: rename "preserve existing list when refresh fails" to "clear pairs when refresh fails (catchError emits empty array)" — the test body always asserted [] so the old name was contradictory. - version-check.service.spec.ts: import packageJson and use 'v'+packageJson.version in the same-version test instead of hardcoding 'v0.17.0'. Future version bumps no longer break this test. - integrations.spec.ts: fail fast on API errors in beforeEach and afterEach (GET + each DELETE). Stops the test with a clear error instead of silently proceeding against unknown state. - settings.spec.ts: drop redundant field.clear() calls (3 sites). fill() already clears before typing; calling clear() separately races with Angular's signal-driven re-render. Pattern matches the existing fix in "text field change saves to backend". - test_context.py / test_web_app_job.py: stop attaching a StreamHandler in _make_context. assertLogs() in the tests installs its own capture handler, so the addHandler call only leaked across tests (loggers are singletons). Drops the now-unused import sys too. - test_active_scanner.py: register self.addCleanup(scanner.close) right after each ActiveScanner construction so multiprocessing resources always get closed even if assertions raise. Trailing scanner.close() calls left in place — Queue.close() is idempotent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Bump version to 0.18.0 and add CHANGELOG entry Minor bump for the develop → master sync. New user-facing functionality (LFTP hot-reload + differentiated restart notifications) and a non-trivial operational improvement (image size from 114 MB to 64 MB) warrant the minor over a patch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@src/e2e-playwright/tests/integrations.spec.ts`:
- Around line 349-353: The test silently swallows API errors by turning a failed
response into an empty array — change the logic around apiFetch so the test
fails if the GET to "/server/integrations" is not successful: check res.ok (the
response from apiFetch) and if false throw or assert fail with a clear message
before attempting to parse JSON and inspect instances; update the local symbols
referenced (res, instances, and the call to apiFetch("/server/integrations")) so
the test does not proceed when the HTTP request failed.
- Around line 132-142: The POST fixture setup calls using apiFetch (e.g.,
apiFetch("/server/integrations", { method: "POST", ... })) currently ignore the
HTTP response; capture the response (const res = await apiFetch(...)) and assert
it succeeded before continuing — for example check res.ok or res.status is
200/201 and optionally verify the returned JSON contains the created
resource/id; if the assertion fails, surface a clear error (throw or test
expect) so the test stops with "fixture creation failed" rather than later UI
failures; apply this pattern to every POST-based fixture setup that uses
apiFetch in the spec.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 472-498: The test calls select.selectOption(newValue) on the Log
Level select before SSE-delivered model may be ready; add an explicit guard by
awaiting expect(select).toBeEnabled({ timeout: 10_000 }) (same pattern used for
the Hash Algorithm test) immediately after obtaining select from
settings.getSelect("Log Level") and before calling
select.selectOption(newValue), keeping the existing finally block that restores
originalLevel.
In `@src/python/tests/unittests/test_common/test_context.py`:
- Around line 71-77: Replace the direct private attribute assignment to
PathPairsConfig._pairs with the public property setter to respect encapsulation
and thread-safety: locate where PathPairsConfig (ppc) is created and a PathPair
instance is appended (the lines using PathPairsConfig and PathPair and the
assignment ppc._pairs = [pair]) and change it to use the pairs property
(ppc.pairs) so the class's locking logic is used instead of touching the private
_pairs field directly.
In `@src/python/tests/unittests/test_controller/test_scan/test_active_scanner.py`:
- Around line 26-31: Tests register scanner.close via self.addCleanup but also
call scanner.close() explicitly at the end, causing double-close of
ActiveScanner (potentially non-idempotent). Remove the redundant explicit
scanner.close() calls from the test methods (e.g., in test_scan and the other
occurrences) and rely solely on self.addCleanup(scanner.close) so
ActiveScanner.close() is invoked once by the test framework; search for
ActiveScanner instantiations and lines with both self.addCleanup(scanner.close)
and a trailing scanner.close() and delete the explicit trailing close calls.
In `@src/python/tests/unittests/test_web/test_web_app_job.py`:
- Around line 105-118: Remove the dead captured_status dictionary and simplify
the start_response closure since captured_status is never asserted; update the
test in test_web_app_job.py by deleting captured_status and changing the
start_response function used with middleware(environ, start_response) to a no-op
(e.g., an empty closure that only accepts status, headers, *args) or add an
assertion that explicitly checks captured_status["status"]; reference the test's
captured_status and start_response to locate the change and ensure the rest of
the assertions against log_ctx.output and middleware result remain unchanged.
- Around line 114-118: The test is brittle because it inspects log_ctx.output[0]
directly; change the three assertions to use an any(...) scan over
log_ctx.output so we check that some log line contains "GET", "/test/path", and
"200" together. Replace the three self.assertIn(...) calls that reference
log_output = log_ctx.output[0] with a single assertion like
self.assertTrue(any("GET" in o and "/test/path" in o and "200" in o for o in
log_ctx.output)) (or equivalent three any(...) checks) to mirror the safer
pattern used elsewhere and reference the same log_ctx.output sequence.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cf4b05b-2256-4dac-ba22-5f7ffbd98a0c
📒 Files selected for processing (10)
CHANGELOG.mdsrc/angular/package.jsonsrc/angular/src/app/pages/settings/option.component.spec.tssrc/angular/src/app/services/settings/path-pairs.service.spec.tssrc/angular/src/app/services/utils/version-check.service.spec.tssrc/e2e-playwright/tests/integrations.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/python/tests/unittests/test_common/test_context.pysrc/python/tests/unittests/test_controller/test_scan/test_active_scanner.pysrc/python/tests/unittests/test_web/test_web_app_job.py
* Address second round of CodeRabbit test-quality findings All 7 findings from this round are valid; all fixed. Test-only — no production code changes (the production findings tracked in #469 are still unaddressed in this PR by design). - integrations.spec.ts: GET on the "should not exist" assertion now fails fast instead of returning [] when the API errors. Five POST-fixture sites now check `res.ok` via a small `expectFixtureOk` helper so a 4xx/5xx surfaces as "fixture creation failed" instead of cascading into a confusing UI failure. - settings.spec.ts: Log Level select-change test waits for the select to be enabled before calling selectOption — same SSE-delivery race the Hash Algorithm test had. - test_context.py: use the public PathPairsConfig.pairs property setter (which acquires the internal lock) instead of touching ._pairs. - test_active_scanner.py: drop the redundant trailing scanner.close() calls now that addCleanup handles the close path. Six sites — one cleanup mechanism instead of two. - test_web_app_job.py: drop the unused captured_status dict from the middleware logging test, and replace `log_ctx.output[0]` indexing with an `any(...)` scan over all captured lines so other loggers emitting first don't break the test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ruff format: collapse assertTrue onto a single line Round-2 commit wrapped the new any(...) assertion across three lines; ruff format prefers it on one. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@CHANGELOG.md`:
- Around line 30-46: The "Tests" section in CHANGELOG.md is non-conforming;
either move the entire bulleted block into the "Added" section of the new
top-version entry (renaming it to e.g. "Added: expanded unit and E2E test
coverage" and keeping the individual bullets) or remove it if tests are
considered internal, and ensure the top-level changelog entry follows the
project's semver/date and allowed sections (Changed, Added, Fixed, Removed,
Security); update the heading and section names accordingly so there is no
standalone "Tests" section.
- Around line 5-24: Reorder the topmost release entry in CHANGELOG.md so the
section headings follow the required order: move the "### Changed" block to
appear before the "### Added" block (ensuring the sequence is Changed, Added,
Fixed, Removed, Security as applicable); update the release header to include
the new semver version and YYYY-MM-DD date if missing and verify headings "###
Changed", "### Added", "### Fixed" remain intact and unchanged otherwise.
In `@src/angular/src/app/services/settings/path-pairs.service.spec.ts`:
- Around line 31-34: The snapshot() helper creates a lasting subscription to
service.pairs$ and never tears it down; change it to consume a single emission
and unsubscribe immediately (e.g., use an RxJS one-shot approach such as piping
service.pairs$ through take(1) or use firstValueFrom, or explicitly store the
Subscription and call unsubscribe) so snapshot() returns the latest PathPair[]
without accumulating live subscribers; update the snapshot() function
accordingly.
In `@src/angular/src/app/services/utils/version-check.service.spec.ts`:
- Around line 151-163: The test dereferences notifications[0] without first
asserting any notification was emitted; update the spec (around the
notificationService.notifications$.subscribe and createService() call) to assert
that notifications has at least one item before indexing (for example add
expect(notifications.length).toBeGreaterThan(0) or
expect(notifications).toHaveLength(1)) so the failure is clear and safe when
checking notifications[0].text; keep references to
notificationService.notifications$, notifications variable, and createService()
when adding the assertion.
In `@src/e2e-playwright/tests/integrations.spec.ts`:
- Around line 255-267: The poll callback used with expect.poll (the async
function calling apiFetch("/server/integrations") and returning
instances.find(...) for the Delete Me check) returns undefined when the fetch
response is non-OK, which falsely satisfies .toBeUndefined(); change the
callback so that on non-ok responses it does NOT return undefined — either throw
an error or return a non-undefined sentinel (e.g. `error:${res.status}`) so the
poll keeps retrying until the integration is actually absent; update the same
anonymous async callback used with expect.poll and keep the final assertion as
.toBeUndefined().
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Line 185: The assertion await expect(field).toBeEnabled() omits an explicit
timeout and can race with SSE-delivered config; update these guards to use the
same explicit timeout used elsewhere in this spec for SSE-gated inputs (e.g.,
await expect(field).toBeEnabled({ timeout: <sameTimeoutValue> })), applying the
change for the occurrences that currently lack it (the asserts using field at
the three noted locations) so they match the established SSE-wait pattern.
In `@src/python/tests/unittests/test_web/test_web_app_job.py`:
- Around line 116-136: The test test_logs_even_on_app_error currently only
checks that something was logged; update it to assert the log content like the
happy-path test: use the same assertLogs("test_request_logging_error",
level="DEBUG") context to capture the log output from the
_RequestLoggingMiddleware invocation and then assert the captured log contains
the HTTP method "POST", the path "/fail", and the status "500" (e.g., via
self.assertIn checks on the logged messages) so the middleware's error branch is
verified for the expected fields.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c8ad347-4ea0-43f4-834b-525d4d99f07a
📒 Files selected for processing (10)
CHANGELOG.mdsrc/angular/package.jsonsrc/angular/src/app/pages/settings/option.component.spec.tssrc/angular/src/app/services/settings/path-pairs.service.spec.tssrc/angular/src/app/services/utils/version-check.service.spec.tssrc/e2e-playwright/tests/integrations.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/python/tests/unittests/test_common/test_context.pysrc/python/tests/unittests/test_controller/test_scan/test_active_scanner.pysrc/python/tests/unittests/test_web/test_web_app_job.py
All 6 findings verified valid; all fixed. Test/changelog only.
- CHANGELOG.md: Reorder 0.18.0 sections to Changed → Added → Fixed
per CLAUDE.md's documented section order. Fold the "Tests" block
into Added as a single sub-bulleted item — test additions are
additive content, and the standalone heading wasn't a recognized
section name.
- path-pairs.service.spec.ts: snapshot() now pipes through take(1)
before subscribing so the helper doesn't accumulate live
subscribers on service.pairs$ across test runs.
- version-check.service.spec.ts: Assert notifications.toHaveLength(1)
before indexing notifications[0].text. A 0-length array would
otherwise throw a confusing "cannot read property 'text' of
undefined".
- integrations.spec.ts: poll callback for the "Delete Me" check now
throws on a non-OK response instead of returning undefined.
Returning undefined was falsely satisfying toBeUndefined() and
masking real API failures.
- settings.spec.ts: Three SSE-gated toBeEnabled() calls now use an
explicit { timeout: 5000 } matching the established text-field
pattern at line 43, instead of relying on the implicit default.
- test_web_app_job.py: error-branch test now asserts the log line
contains POST, /fail, and 500 — mirrors the happy-path assertion
added in round 2 so the middleware's exception path is also
verified for the expected fields.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The runtime stage is `FROM alpine:3.23` and copies /usr/local from the python-deps builder verbatim. Leaving the builder on the floating `python:3.13-alpine` tag risks compiling Python + site-packages C extensions against a newer Alpine's musl while running on alpine:3.23 — a real ABI-mismatch hazard, not just a reproducibility nit. Tag verified to exist on Docker Hub via `docker manifest inspect`. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/e2e-playwright/tests/settings.spec.ts (1)
114-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit enabled wait before changing
Log Format.At Line 117,
selectOption("json")is called without a readiness guard. This is the same SSE timing class that previously flaked for other selects in this file. Please gate it withawait expect(select).toBeEnabled({ timeout: 10_000 })before the firstselectOption.#!/bin/bash # Verify this select interaction currently lacks a readiness guard. rg -n -C3 'getSelect\("Log Format"\)|selectOption\("json"\)|toBeEnabled\(\{ timeout: 10_000 \}\)' src/e2e-playwright/tests/settings.spec.ts🤖 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 `@src/e2e-playwright/tests/settings.spec.ts` around lines 114 - 117, The select for "Log Format" is being changed before ensuring it is enabled; add a readiness guard by awaiting expect(select).toBeEnabled({ timeout: 10_000 }) immediately before the call to select.selectOption("json") so the getSelect("Log Format") interaction waits for the control to be enabled; target the variable select and the selectOption("json") call when inserting this check.
🤖 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 `@src/docker/build/docker-image/Dockerfile`:
- Around line 24-32: The Dockerfile copies uv.lock but then calls the uv pip
interface which ignores the lockfile (using "uv pip install ... -r
/tmp/pyproject.toml"); change the build step to use a lock-aware flow: either
run "uv sync --locked --no-dev --system" to install the locked environment into
system Python, or run "uv export --frozen -o /tmp/requirements.txt" and then
"pip install --system --no-cache -r /tmp/requirements.txt"; ensure the uv.lock
and /tmp/uv.lock copy remains and remove the temporary requirements file and
uv.lock after install to preserve the original cleanup behavior.
In `@src/e2e-playwright/tests/integrations.spec.ts`:
- Around line 12-14: The shared IntegrationsPage instance and global cleanup in
the describe block cause races under workers: 4; either make the suite run
serially by adding test.describe.configure({ mode: "serial" }) at the top of the
"Integrations CRUD" describe, or make each test create and track a unique
integration name (e.g., append a UUID in beforeEach) and change the afterEach
cleanup to delete only that test's integration instead of wiping all
integrations; update references in the describe block, beforeEach, afterEach and
any uses of integrations to use the per-test name or the serial configuration
accordingly.
- Around line 116-133: In the three poll callbacks used in the "fill and save",
"edit", and "toggle" tests replace the current silent `return undefined` when
`res.ok` is false with throwing an Error (include response.status and a snippet
of response.text or statusText) so the poll surfaces the real API failure
instead of timing out; mimic the delete-test poll behavior by throwing on non-OK
responses in the anonymous async callbacks passed to `.poll(...)` that call
`apiFetch("/server/integrations")`, `apiFetch(...)` in the edit test, and the
one in the toggle test so failing backend calls produce clear error messages.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 441-451: The test currently only checks visibility for the secret
notification inputs (discordField and telegramTokenField); change each assertion
to verify the input type is masked by adding an assertion that the locator has
attribute type="password" (e.g. replace or augment await
expect(discordField).toBeVisible() and await
expect(telegramTokenField).toBeVisible() with await
expect(discordField).toHaveAttribute("type","password") and await
expect(telegramTokenField).toHaveAttribute("type","password"), ensuring the test
fails if those fields are not rendered as password inputs.
In `@src/python/tests/unittests/test_controller/test_scan/test_active_scanner.py`:
- Around line 80-84: The test test_missing_file_logged_at_debug currently only
checks for the message text and not the log level; update it to assert the
missing-file message is logged specifically at DEBUG by inspecting
log_ctx.output entries (formatted as "LEVEL:logger:message") for an entry that
starts with "DEBUG:ActiveScanner:" and contains "does not exist" (or split each
entry and assert the first token is "DEBUG" and the logger token is
"ActiveScanner" with the message containing the missing-file text) so the
assertion enforces DEBUG-level logging from ActiveScanner.
---
Duplicate comments:
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 114-117: The select for "Log Format" is being changed before
ensuring it is enabled; add a readiness guard by awaiting
expect(select).toBeEnabled({ timeout: 10_000 }) immediately before the call to
select.selectOption("json") so the getSelect("Log Format") interaction waits for
the control to be enabled; target the variable select and the
selectOption("json") call when inserting this check.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6da0f70-f200-410e-9fd8-5931f5df3542
📒 Files selected for processing (11)
CHANGELOG.mdsrc/angular/package.jsonsrc/angular/src/app/pages/settings/option.component.spec.tssrc/angular/src/app/services/settings/path-pairs.service.spec.tssrc/angular/src/app/services/utils/version-check.service.spec.tssrc/docker/build/docker-image/Dockerfilesrc/e2e-playwright/tests/integrations.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/python/tests/unittests/test_common/test_context.pysrc/python/tests/unittests/test_controller/test_scan/test_active_scanner.pysrc/python/tests/unittests/test_web/test_web_app_job.py
Five of six findings actioned; one skipped. Test + Dockerfile only.
- Dockerfile: `uv pip install -r pyproject.toml` was ignoring uv.lock,
so transitive dep versions could drift between identical rebuilds.
Now run `uv export --frozen --no-dev` to materialize the lockfile
into a requirements.txt and install from that. Cleanup includes
the temp requirements file.
- integrations.spec.ts: Three poll callbacks ("fill and save",
"edit", "toggle") returned undefined on a non-OK response, which
would silently mask backend failures and either time out or pass
on stale data. Now throw with status + statusText, matching the
pattern already used in the "Delete Me" poll.
- settings.spec.ts (Notifications): Discord and Telegram token
fields previously only asserted visibility. Added explicit
toHaveAttribute("type", "password") so a regression flipping the
input back to type="text" is caught.
- settings.spec.ts (Log Format): Added the same SSE-readiness guard
used for the other selects — toBeEnabled({ timeout: 10_000 })
before selectOption("json").
- test_active_scanner.py: test_missing_file_logged_at_debug now
asserts the "DEBUG:ActiveScanner:" prefix on the captured log
output, not just the message text. assertLogs(level="DEBUG")
captures records >= DEBUG, so a plain message check could pass
even if the log was emitted at WARNING/ERROR.
Skipped: workers:4 race claim on integrations.spec.ts. With
fullyParallel:false, tests within a file run serially, and a grep
confirmed no other spec file touches /server/integrations. The
"global cleanup races at workers:4" premise isn't current state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/angular/src/app/pages/settings/option.component.spec.ts (1)
15-16: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRe-declared
DEBOUNCE_TIME_MSstill drifts from the component's private constant.The component's private
DEBOUNCE_TIME_MS = 1000is mirrored here. If the component's value changes, this suite continues to pass while exercising the wrong period. Export the constant fromoption.component.tsand import it here so both stay in sync.🤖 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 `@src/angular/src/app/pages/settings/option.component.spec.ts` around lines 15 - 16, The test re-declares DEBOUNCE_TIME_MS, which can drift from the component's private constant; export the constant from option.component.ts (e.g. export const DEBOUNCE_TIME_MS) and update option.component.spec.ts to import that exported DEBOUNCE_TIME_MS instead of declaring its own, then remove the duplicate declaration so the spec uses the component's single source of truth (referencing OptionComponent and DEBOUNCE_TIME_MS).
🤖 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 `@src/angular/src/app/pages/settings/option.component.spec.ts`:
- Around line 96-125: Remove the unnecessary vi.useFakeTimers() setup calls from
the three tests that exercise component.onChange for OptionType.Password and
OptionType.Text; these tests call newValue.next(...) synchronously, so drop the
vi.useFakeTimers() invocations at the start of the "should suppress
REDACTED_SENTINEL for password type", "should pass real password value through
to Subject", and "should not suppress REDACTED_SENTINEL for non-password types"
specs and keep the existing spies on (component as unknown as { newValue:
Subject<OptionValue> }).newValue and the assertions that check nextSpy behavior
for REDACTED_SENTINEL and real-password.
In `@src/angular/src/app/services/settings/path-pairs.service.spec.ts`:
- Around line 212-214: The test creates a lingering subscription by calling
service.pairs$.subscribe(...) without unsubscribing; change that assertion to
consume only one emission (e.g., use RxJS take(1) on service.pairs$ or call the
existing snapshot() helper if available) so the subscriber is torn down
automatically; update the expectation to read from service.pairs$.pipe(take(1))
(or from service.snapshot()) and assert the resulting value equals [] to avoid
leaving a live subscriber.
In `@src/docker/build/docker-image/Dockerfile`:
- Around line 29-32: Replace the inline shell cd usage in the Dockerfile RUN
that calls /tmp/uv (the RUN line invoking "cd /tmp && /tmp/uv export ... &&
/tmp/uv pip install ...") by setting a WORKDIR /tmp before that RUN (or
otherwise running /uv with absolute paths), i.e., add a preceding WORKDIR /tmp
and remove the "cd /tmp &&" so the commands using "uv export" and "uv pip
install" run from /tmp and avoid the Hadolint DL3003 / Trivy DS-0013 lint.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 358-392: The test mutates validate.enabled before the try block
causing the finally cleanup to be skipped if setup throws; move the
apiSetConfig("validate", "enabled", "True") call inside the try that begins
before interacting with SettingsPage so that the corresponding finally always
runs and restores validate.enabled via apiSetConfig("validate", "enabled",
"False") and apiSetConfig("validate", "algorithm", String(originalAlgorithm)).
Ensure the sequence around SettingsPage(page).goto(), waitForStream(page),
settings.getSelect("Hash Algorithm"), and the expect checks remains inside the
same try so any thrown error triggers the finally cleanup.
In `@src/python/tests/unittests/test_controller/test_scan/test_active_scanner.py`:
- Around line 101-105: Update the test_unexpected_error_logged_at_warning to
assert the WARNING prefix explicitly: when calling scanner.scan() inside
assertLogs("ActiveScanner", level="WARNING"), check that log_ctx.output contains
an entry that starts with "WARNING:ActiveScanner:" and includes "Unexpected scan
error" (similar to how test_missing_file_logged_at_debug asserts the DEBUG
prefix). Locate the test by name (test_unexpected_error_logged_at_warning) and
modify the final assertion to verify the "WARNING:ActiveScanner:" prefix plus
the message instead of only checking the message text.
In `@src/python/tests/unittests/test_web/test_web_app_job.py`:
- Around line 10-64: The TestWebAppJobSetup class mixes setup, execute, and
cleanup lifecycle tests; split them into separate test classes named to match
the lifecycle (e.g., TestWebAppJobSetup, TestWebAppJobExecute,
TestWebAppJobCleanup) so each only contains the corresponding test method (move
test_setup_creates_server_and_starts_thread into TestWebAppJobSetup,
test_execute_calls_process into TestWebAppJobExecute, and
test_cleanup_stops_server_and_joins_thread into TestWebAppJobCleanup); keep or
refactor the shared _make_context helper into a small base TestCase (or
duplicate it) so each class can call it, and ensure the patch decorators and
references to WebAppJob, setup, execute, cleanup, Thread, and MyWSGIRefServer
remain unchanged.
---
Duplicate comments:
In `@src/angular/src/app/pages/settings/option.component.spec.ts`:
- Around line 15-16: The test re-declares DEBOUNCE_TIME_MS, which can drift from
the component's private constant; export the constant from option.component.ts
(e.g. export const DEBOUNCE_TIME_MS) and update option.component.spec.ts to
import that exported DEBOUNCE_TIME_MS instead of declaring its own, then remove
the duplicate declaration so the spec uses the component's single source of
truth (referencing OptionComponent and DEBOUNCE_TIME_MS).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65d462d0-b8f7-4f4d-83df-64813594c354
📒 Files selected for processing (11)
CHANGELOG.mdsrc/angular/package.jsonsrc/angular/src/app/pages/settings/option.component.spec.tssrc/angular/src/app/services/settings/path-pairs.service.spec.tssrc/angular/src/app/services/utils/version-check.service.spec.tssrc/docker/build/docker-image/Dockerfilesrc/e2e-playwright/tests/integrations.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/python/tests/unittests/test_common/test_context.pysrc/python/tests/unittests/test_controller/test_scan/test_active_scanner.pysrc/python/tests/unittests/test_web/test_web_app_job.py
All 7 findings actioned. Test/Dockerfile/single-line component edit only.
- option.component.spec.ts (3 onChange tests): drop unnecessary
vi.useFakeTimers() — onChange pushes to the Subject synchronously
before any debounceTime delay, so the spy on next() observes it
immediately. Fake timers were a leftover from when these tests
inspected the debounced output instead of the Subject.
- option.component.ts / spec: export DEBOUNCE_TIME_MS as a top-level
const. Spec now imports it instead of redeclaring 1000 — single
source of truth, no drift if the debounce window ever changes.
- path-pairs.service.spec.ts (last assertion in catchError test):
reuse the snapshot() helper (which already pipes through take(1))
instead of an ad-hoc subscribe that left a live subscriber.
- Dockerfile: replace `cd /tmp && ...` with `WORKDIR /tmp` to silence
Hadolint DL3003 / Trivy DS-0013. The python-deps stage's working
directory after this RUN doesn't affect the runtime stage (which
only copies /usr/local).
- settings.spec.ts (Hash Algorithm test): move the
`apiSetConfig("validate", "enabled", "True")` setup inside the try
block so the finally cleanup always runs, even if the apiSetConfig
itself or any subsequent step (goto, getSelect, expect) throws.
- test_active_scanner.py (test_unexpected_error_logged_at_warning):
pin the level explicitly via the "WARNING:ActiveScanner:" prefix,
mirroring the DEBUG check in test_missing_file_logged_at_debug.
assertLogs(level="WARNING") captures records >= WARNING, so the
prior message-only check could also pass on ERROR/CRITICAL.
- test_web_app_job.py: split the misnamed TestWebAppJobSetup class
into TestWebAppJobSetup, TestWebAppJobExecute, and
TestWebAppJobCleanup, each containing only the lifecycle method
it tests. Shared _make_context() lifted into _WebAppJobBase so
the helper isn't duplicated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 `@src/angular/src/app/services/utils/version-check.service.spec.ts`:
- Around line 95-107: The test "should strip v prefix before comparing versions"
currently duplicates the "should show notification when a newer release is
available" scenario; update it to verify stripping by exercising both prefixed
and non‑prefixed responses and asserting identical behavior: call
mockRestService.sendRequest to return makeGithubResponse('v99.0.0') for one run
and makeGithubResponse('99.0.0') for the other, subscribe to
notificationService.notifications$ and compare the resulting notifications
arrays are equal (or both length === 1), and ensure you reset the test module
between runs using TestBed.resetTestingModule() before re-configuring and
calling createService() so you don't get a cached singleton; keep using
createService(), mockRestService.sendRequest, makeGithubResponse and
notificationService.notifications$ to locate the code to change.
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 469-521: The test captures apiGet("/server/config/get") and
constructs originalLevel/newValue and calls expect(select).toBeEnabled() outside
the try block which is fine functionally but inconsistent with the Hash
Algorithm test; to tighten setup and ensure consistency, move the retrieval of
originalLevel, computation of newValue and the await
expect(select).toBeEnabled({ timeout: 10_000 }) into the try block that contains
select.selectOption(newValue) (referencing SettingsPage, getSelect("Log Level"),
select.selectOption, and the try/finally cleanup) so all state-read/setup that
precedes mutation lives inside the try and the finally still restores
originalLevel via select.selectOption(String(originalLevel)) and the apiGet
polls.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2287afff-1b2c-442b-96c0-ca256b387e31
📒 Files selected for processing (12)
CHANGELOG.mdsrc/angular/package.jsonsrc/angular/src/app/pages/settings/option.component.spec.tssrc/angular/src/app/pages/settings/option.component.tssrc/angular/src/app/services/settings/path-pairs.service.spec.tssrc/angular/src/app/services/utils/version-check.service.spec.tssrc/docker/build/docker-image/Dockerfilesrc/e2e-playwright/tests/integrations.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/python/tests/unittests/test_common/test_context.pysrc/python/tests/unittests/test_controller/test_scan/test_active_scanner.pysrc/python/tests/unittests/test_web/test_web_app_job.py
- version-check.service.spec.ts: the "should strip v prefix" test was a structural duplicate of "should show notification when a newer release is available" — both ran "v99.0.0" and asserted length === 1, exercising no v-prefix logic. Rewrote it to run the same scenario twice (with "v99.0.0" and with "99.0.0"), reset TestBed between runs, and assert the resulting notifications are equivalent. Now actually verifies the stripping behavior the test name implies. - settings.spec.ts (Log Level test): move toBeEnabled and the newValue computation inside the try block so any throw before the mutation still hits the finally cleanup. Mirrors the structure of the Hash Algorithm test from round 5. originalLevel and select stay at function scope (finally references originalLevel). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/angular/src/app/pages/settings/option.component.spec.ts`:
- Around line 34-72: The tests hardcode 1000ms in vi.advanceTimersByTime and in
one test title; replace all literal 1000 usages in the spec with the imported
DEBOUNCE_TIME_MS and update the test description to reference DEBOUNCE_TIME_MS
(or a non-magic wording) so the specs use the same debounce constant as the
component; search for vi.advanceTimersByTime(1000) and the string '1000ms' in
this file and change them to use DEBOUNCE_TIME_MS and adjust the test name
accordingly to avoid brittle magic numbers.
In `@src/angular/src/app/services/settings/path-pairs.service.spec.ts`:
- Around line 160-171: Add a second spec for update() that mirrors the non-409
coverage of create(): after emitting connectedSubject.next(true) and seeding the
list via httpMock.expectOne('/server/pathpairs').flush([makePair()]), call
service.update(makePair({ name: 'other' })) and stub the PUT/patch request to
'/server/pathpairs/pair-1' to return a non-409 error (e.g., flush('error', {
status: 500, statusText: 'Server Error' })), then assert the subscriber receives
null (same expectation create() uses) to ensure update() falls back to null on
non-409 errors; reference service.update, connectedSubject, httpMock.expectOne,
and makePair() when adding the test.
In `@src/angular/src/app/services/utils/version-check.service.spec.ts`:
- Around line 51-65: The inline comment that hardcodes the old current version
is stale; update or remove it so it no longer references "Current version is
0.17.0". In the test around mockRestService.sendRequest /
makeGithubResponse('v99.0.0') and the notification assertions
(notificationService.notifications$ subscription and createService()), replace
the comment with a neutral note like "99.0.0 chosen because it is always newer
than the current version" or remove it entirely to avoid future confusion when
the package version bumps.
In `@src/docker/build/docker-image/Dockerfile`:
- Around line 110-116: The Dockerfile currently deletes apk tooling and DB in
the rm -rf line (removing /sbin/apk, /etc/apk, /var/cache/apk, /lib/apk, etc.),
which breaks downstream vulnerability scanners and prevents users from running
apk in derived images; either stop removing the apk binaries/DB in that rm -rf
command so tools like Trivy/Grype can enumerate installed Alpine packages and
downstream RUN apk add works, or if you must keep the smaller image, add a clear
note in the README/CHANGELOG security section for ghcr.io/nitrobass24/seedsync
explaining that the final image strips apk and that consumers must scan the
runtime base layers via image-history/SBOM (and mention affected packages like
lftp, openssh-client, ca-certificates, libstdc++).
In `@src/e2e-playwright/tests/settings.spec.ts`:
- Around line 214-522: Several describe blocks repeat the same setup lines
"const settings = new SettingsPage(page); await settings.goto();" in each test;
extract this by adding a beforeEach inside each sibling describe (e.g., inside
the "Settings — Staging Directory", "Archive Extraction", "Integrity Check",
"Connections", "Notifications", and "Logging" describe blocks) that constructs
SettingsPage(page) and calls settings.goto(), then expose settings to tests via
a shared variable or by extending the test fixture (e.g., add a fixture that
returns settings: SettingsPage). Update tests to use the injected/shared
settings reference and remove the duplicated constructor/goto calls.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2def33b-25e0-42ca-8863-b90e854e9236
📒 Files selected for processing (12)
CHANGELOG.mdsrc/angular/package.jsonsrc/angular/src/app/pages/settings/option.component.spec.tssrc/angular/src/app/pages/settings/option.component.tssrc/angular/src/app/services/settings/path-pairs.service.spec.tssrc/angular/src/app/services/utils/version-check.service.spec.tssrc/docker/build/docker-image/Dockerfilesrc/e2e-playwright/tests/integrations.spec.tssrc/e2e-playwright/tests/settings.spec.tssrc/python/tests/unittests/test_common/test_context.pysrc/python/tests/unittests/test_controller/test_scan/test_active_scanner.pysrc/python/tests/unittests/test_web/test_web_app_job.py
Comment on lines
+110
to
+116
| && rm -rf /sbin/apk /etc/apk /usr/share/apk /lib/apk /var/cache/apk \ | ||
| /lib/libapk.so* /usr/lib/libapk.so* /usr/bin/scanelf \ | ||
| /usr/bin/ssh-keyscan /usr/bin/ssh-add /usr/bin/ssh-agent \ | ||
| /usr/bin/ssh-pkcs11-helper /usr/bin/ssh-copy-id \ | ||
| /usr/bin/findssl.sh /usr/bin/c_rehash \ | ||
| /usr/bin/iconv /usr/bin/getconf /usr/bin/ssl_client \ | ||
| /etc/ssh/moduli |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Removing apk and the apk DB blocks downstream CVE scanning and image patching.
Stripping /sbin/apk, /etc/apk, /lib/apk, and /var/cache/apk is a real size win, but it also:
- Prevents Trivy / Grype / Docker Scout from enumerating installed Alpine packages by querying the apk database — vulnerability scans of
ghcr.io/nitrobass24/seedsyncwill under-report OS-level CVEs in lftp, openssh-client, ca-certificates, libstdc++, etc. - Removes the ability for downstream users to layer
RUN apk add ...on this image (e.g., for debugging or custom builds) without first reinstalling apk.
Both are deliberate trade-offs for the documented 114→64 MB win, but worth calling out in the README/CHANGELOG security section so users running scanners against the published image know they need to scan the runtime base layers via image-history/SBOM rather than the final image alone.
🤖 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 `@src/docker/build/docker-image/Dockerfile` around lines 110 - 116, The
Dockerfile currently deletes apk tooling and DB in the rm -rf line (removing
/sbin/apk, /etc/apk, /var/cache/apk, /lib/apk, etc.), which breaks downstream
vulnerability scanners and prevents users from running apk in derived images;
either stop removing the apk binaries/DB in that rm -rf command so tools like
Trivy/Grype can enumerate installed Alpine packages and downstream RUN apk add
works, or if you must keep the smaller image, add a clear note in the
README/CHANGELOG security section for ghcr.io/nitrobass24/seedsync explaining
that the final image strips apk and that consumers must scan the runtime base
layers via image-history/SBOM (and mention affected packages like lftp,
openssh-client, ca-certificates, libstdc++).
All 5 findings actioned. Test + CHANGELOG only. - option.component.spec.ts: replace 4 `vi.advanceTimersByTime(1000)` literals with the imported DEBOUNCE_TIME_MS constant. Update the one test name that hardcoded "1000ms" to the more durable "the full debounce window". The 200ms literals stay — those are intentionally sub-debounce, not the debounce window. - path-pairs.service.spec.ts: add the missing non-409 fallback test for update(). The service does `return of(null)` on any non-409 error from PUT; create() already had the matching test, so the coverage gap was just on update(). - version-check.service.spec.ts: drop the stale "Current version is 0.17.0" comment. Replaced with a version-agnostic note about why 99.0.0 was chosen (always newer than current package.json). - CHANGELOG.md (0.18.0): add a Security note documenting that the runtime image strips apk and the apk DB. In-image scanners can't enumerate Alpine packages from a running container; consumers scanning the published image should use SBOM/image-history tools. Reviewer accepted documentation as an alternative to keeping apk in the image (which would re-balloon the runtime). - settings.spec.ts: extract the repeated `const settings = new SettingsPage(page); await settings.goto();` to a per-describe beforeEach in 6 sibling describes (Staging Directory, Archive Extraction, Integrity Check, Connections, Notifications, Logging). Mirrors the existing pattern in the top-level "Settings Page" describe (line 5). Removes 8 inline constructions; tests destructure only the fixtures they actually need now. Hash Algorithm test keeps an explicit `await settings.goto()` inside its try block — beforeEach loads the page before validate.enabled is applied, and the test needs the reload + waitForStream so buildValidateContext() observes the new state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Brings 35 commits from
developtomaster. No new user-facing features beyond what shipped in v0.17.0 — this batch is dominated by test coverage, CI rework, image slimming, and dependency bumps.Highlights
User-facing
RUN --mountforuvcut the runtime image from 114 MB to 64 MB (Use RUN --mount for uv to cut image size from 114 MB to 64 MB #437); Dockerfile collapsed to 2 stages on Python 3.13-alpine (Collapse Dockerfile to 2 stages with Python 3.13-alpine #436)Test coverage expansion
CI / infrastructure
Bug fixes
validate.enabled(the real disable gate) — fixes flake from PR 11: E2E — Settings Coverage Expansion #442 (Fix flaky Hash Algorithm select wait condition #455)addCleanup(Test logger handlers leak across test runs #450, Stop leaking StreamHandlers across test runs (#450) #457)Dependency bumps
Full commit log
35 commits — see the Commits tab. Notable PR numbers above.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests
Documentation