feat: multiple path pairs (#122)#149
Merged
nitrobass24 merged 5 commits intofeat/v0.13.0from Mar 5, 2026
Merged
Conversation
Replace paste.httpserver with Python's wsgiref.simple_server and paste.translogger with a lightweight _RequestLoggingMiddleware. Remove paste from runtime dependencies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add PathPairsConfig for managing multiple remote-to-local directory mappings with migration from legacy single-path config. Wire through the full stack: Python backend (Model, ModelBuilder, Controller, REST API, SSE serialization) and Angular frontend (ModelFile, ViewFile, PathPairsService with CRUD operations). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds multi-path-pair support: new PathPair model and services in Angular, PathPairsConfig and HTTP handlers in Python, composite-key (pair_id:name) propagation across models, persistence/migration of path_pairs.json, and updates to command routing and server wiring. Changes
sequenceDiagram
participant UI as Angular UI
participant AS as PathPairsService
participant S as Server (/server/pathpairs)
participant CFG as PathPairsConfig
participant DISK as Disk
UI->>AS: createPair(payload)
AS->>S: POST /server/pathpairs (payload)
S->>CFG: add_pair(new Pair)
CFG->>DISK: persist path_pairs.json
CFG-->>S: return created Pair (with id)
S-->>AS: 201 Created (Pair JSON)
AS->>AS: update BehaviorSubject pairs$
AS-->>UI: observable emits new pair
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/python/web/web_app_job.py (1)
105-107:⚠️ Potential issue | 🟠 MajorCall
shutdown()beforeserver_close()to properly exit the server loop.The
serve_forever()call on line 103 blocks indefinitely untilshutdown()is explicitly called. Currently,stop()only callsserver_close(), which closes the listening socket but does not signal the server loop to exit. This leaves the server thread blocked onserve_forever(), causing thejoin()on line 49 to hang indefinitely during cleanup.🔧 Suggested fix
def stop(self): self.logger.debug("Stopping web server") - self.server.server_close() + if self.server is not None: + self.server.shutdown() + self.server.server_close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/web/web_app_job.py` around lines 105 - 107, The stop() method currently only calls server.server_close() which won't break the serve_forever() loop; modify stop() to first call server.shutdown() to signal the server loop to exit (so the thread can finish and join() won't hang), then call server.server_close() and keep the existing logger.debug call; adjust references to self.server.shutdown() and self.server.server_close() inside the stop() method where serve_forever() is run.src/python/controller/model_builder.py (1)
238-238:⚠️ Potential issue | 🟠 MajorPropagate
pair_idwhen creating childModelFilenodes.Children are created without
pair_id, so nested files lose pair affinity and can break pair-scoped identity/commands in tree operations.🔧 Proposed fix
- _child_model_file = ModelFile(_child_name, _is_dir) + _child_model_file = ModelFile(_child_name, _is_dir, pair_id=self.__pair_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/controller/model_builder.py` at line 238, When instantiating child ModelFile nodes you must propagate the parent's pair_id so children retain pair affinity; change the creation of _child_model_file = ModelFile(_child_name, _is_dir) to include the parent's pair_id (e.g., pass pair_id from the current/parent ModelFile variable such as _model_file.pair_id or self.pair_id) so nested files preserve pair-scoped identity and commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/angular/src/app/services/files/view-file.service.spec.ts`:
- Line 14: The spec adds pair_id but the test helper still emits map entries
keyed only by name, so it never verifies same-name files across different pairs;
update the test to include a case with identical name but different pair_id and
adjust the helper that emits map entries (the helper in
view-file.service.spec.ts that currently keys by name) to include pair_id in the
key (e.g., use a composite key like `${pair_id}:${name}` or make the map nested
by pair_id then name) so the assertion at the map check validates pair-aware
behavior.
In `@src/angular/src/app/services/files/view-file.service.ts`:
- Line 214: The forEach callback calling this.indices.set(viewFileKey(value),
index) returns the Map (triggering a linter warning); replace the forEach with a
simple for...of loop to avoid returning a value. Locate the two occurrences
where newViewFiles.forEach(...) is used (the one that sets this.indices using
viewFileKey and index, and the similar pattern around line 286) and rewrite them
as: iterate newViewFiles with for (const [index, value] of
newViewFiles.entries()) { this.indices.set(viewFileKey(value), index); } so no
implicit return value is produced.
In `@src/python/common/path_pairs_config.py`:
- Around line 103-113: from_str currently accepts arbitrary JSON shapes, can
raise raw KeyError/TypeError, and appends PathPair objects bypassing uniqueness
checks; update PathPairsConfig.from_str to validate that the top-level
"path_pairs" is a list and each item is a dict, wrap PathPair.from_dict calls in
try/except and convert KeyError/TypeError/ValueError into PersistError with a
clear message, and instead of appending to _pairs directly call
self.add_pair(...) (or perform an explicit duplicate check on pair_id returned
by PathPair.from_dict) so duplicate pair_id values are rejected consistently;
reference PathPairsConfig.from_str, PathPair.from_dict, add_pair, PersistError
in your changes.
In `@src/python/controller/controller.py`:
- Around line 60-63: Controller.__init__ saves pair_id but the command execution
path still resolves targets using only filename, causing cross-pair ambiguity;
update the file/target lookup at the command execution site (the place that
currently calls the file-resolution with only filename) to pass self.pair_id as
an additional parameter, and update the called lookup function(s) (e.g., any
get_target_by_filename / resolve_target_by_filename) to accept and use pair_id
when querying so lookups are scoped to the correct pair; ensure any call sites
that now require the extra argument are updated accordingly.
In `@src/python/controller/model_builder.py`:
- Around line 24-26: ModelBuilder's __init__ sets self.__pair_id but callers
instantiate ModelBuilder() without forwarding the current pair id, leaving
__pair_id None; update every call site that constructs ModelBuilder (e.g., the
Controller code that calls ModelBuilder()) to pass the active pair id (e.g.,
pass self.pair_id or the local pair_id/context variable) so
ModelBuilder(pair_id=...) is used, and ensure any factory/helper that creates
ModelBuilder also forwards the pair id.
In `@src/python/seedsync.py`:
- Around line 385-387: Replace the hardcoded dummy string with the class
sentinel: use Seedsync.__CONFIG_DUMMY_VALUE instead of "<replace me>" when
building remote_path and local_path (currently assigned to local variable
dummy); change the expressions that set remote_path = config.lftp.remote_path if
config.lftp.remote_path != dummy else "" and local_path = config.lftp.local_path
if config.lftp.local_path != dummy else "" to compare against
Seedsync.__CONFIG_DUMMY_VALUE so the code references the single source of truth
for the sentinel.
In `@src/python/web/handler/path_pairs.py`:
- Around line 30-41: The handler currently assumes json.loads(request.body...)
returns a mapping and calls data.get(...), which will raise AttributeError for
non-object JSON (list/string/number); update the parsing blocks around the
PathPair creation (the first block that constructs PathPair with
name/remote_path/local_path/enabled/auto_queue and the similar block later that
also builds a PathPair) to validate that the parsed value is a dict/mapping,
returning HTTP 400 "Invalid JSON" if not, before accessing .get(); keep existing
JSONDecodeError/UnicodeDecodeError handling intact.
In `@src/python/web/web_app_job.py`:
- Around line 99-103: Replace the single-threaded WSGI server created by
make_server with a threaded server to prevent the SSE long-lived loop in
__web_stream from blocking other requests: define a ThreadingWSGIServer
(subclassing socketserver.ThreadingMixIn and wsgiref.simple_server.WSGIServer)
and pass it as the server_class argument to make_server where
_RequestLoggingMiddleware is wrapped and self.server is assigned (currently
calling make_server(self.host, self.port, handler,
handler_class=_QuietHandler)); this ensures each request (including SSE
connections) runs in its own thread and avoids head-of-line blocking.
---
Outside diff comments:
In `@src/python/controller/model_builder.py`:
- Line 238: When instantiating child ModelFile nodes you must propagate the
parent's pair_id so children retain pair affinity; change the creation of
_child_model_file = ModelFile(_child_name, _is_dir) to include the parent's
pair_id (e.g., pass pair_id from the current/parent ModelFile variable such as
_model_file.pair_id or self.pair_id) so nested files preserve pair-scoped
identity and commands.
In `@src/python/web/web_app_job.py`:
- Around line 105-107: The stop() method currently only calls
server.server_close() which won't break the serve_forever() loop; modify stop()
to first call server.shutdown() to signal the server loop to exit (so the thread
can finish and join() won't hang), then call server.server_close() and keep the
existing logger.debug call; adjust references to self.server.shutdown() and
self.server.server_close() inside the stop() method where serve_forever() is
run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 966d5dc8-5c52-4a65-8ec9-d6a0213179c2
📒 Files selected for processing (28)
src/angular/src/app/models/index.tssrc/angular/src/app/models/model-file.spec.tssrc/angular/src/app/models/model-file.tssrc/angular/src/app/models/path-pair.tssrc/angular/src/app/models/view-file.tssrc/angular/src/app/pages/files/file.component.spec.tssrc/angular/src/app/services/files/model-file.service.tssrc/angular/src/app/services/files/view-file.service.spec.tssrc/angular/src/app/services/files/view-file.service.tssrc/angular/src/app/services/index.tssrc/angular/src/app/services/settings/path-pairs.service.tssrc/python/common/__init__.pysrc/python/common/context.pysrc/python/common/path_pairs_config.pysrc/python/controller/controller.pysrc/python/controller/model_builder.pysrc/python/model/diff.pysrc/python/model/file.pysrc/python/model/model.pysrc/python/pyproject.tomlsrc/python/requirements.txtsrc/python/seedsync.pysrc/python/web/handler/controller.pysrc/python/web/handler/path_pairs.pysrc/python/web/serialize/serialize_model.pysrc/python/web/web_app.pysrc/python/web/web_app_builder.pysrc/python/web/web_app_job.py
💤 Files with no reviewable changes (2)
- src/python/requirements.txt
- src/python/pyproject.toml
| overrides: Partial<ModelFile> & { name: string }, | ||
| ): ModelFile { | ||
| return { | ||
| pair_id: null, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a duplicate-name cross-pair regression test.
Line 14 adds pair_id, but the helper still emits map entries keyed only by name (Line 87), so this spec never validates same-name files across different pairs. Add one case with identical name + different pair_id to confirm pair-aware behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/angular/src/app/services/files/view-file.service.spec.ts` at line 14, The
spec adds pair_id but the test helper still emits map entries keyed only by
name, so it never verifies same-name files across different pairs; update the
test to include a case with identical name but different pair_id and adjust the
helper that emits map entries (the helper in view-file.service.spec.ts that
currently keys by name) to include pair_id in the key (e.g., use a composite key
like `${pair_id}:${name}` or make the map nested by pair_id then name) so the
assertion at the map check validates pair-aware behavior.
| this.files = newViewFiles; | ||
| this.indices.clear(); | ||
| newViewFiles.forEach((value, index) => this.indices.set(value.name, index)); | ||
| newViewFiles.forEach((value, index) => this.indices.set(viewFileKey(value), index)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use for...of instead of forEach to avoid implicit return value.
Map.set() returns the Map, causing the callback to return a value. While functionally harmless, this triggers a linter warning and for...of is clearer.
This same pattern appears on line 286.
♻️ Suggested refactor
- newViewFiles.forEach((value, index) => this.indices.set(viewFileKey(value), index));
+ for (let index = 0; index < newViewFiles.length; index++) {
+ this.indices.set(viewFileKey(newViewFiles[index]), index);
+ }And similarly for line 286:
- newViewFiles.forEach((value, index) => this.indices.set(viewFileKey(value), index));
+ for (let index = 0; index < newViewFiles.length; index++) {
+ this.indices.set(viewFileKey(newViewFiles[index]), index);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| newViewFiles.forEach((value, index) => this.indices.set(viewFileKey(value), index)); | |
| for (let index = 0; index < newViewFiles.length; index++) { | |
| this.indices.set(viewFileKey(newViewFiles[index]), index); | |
| } |
🧰 Tools
🪛 Biome (2.4.4)
[error] 214-214: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/angular/src/app/services/files/view-file.service.ts` at line 214, The
forEach callback calling this.indices.set(viewFileKey(value), index) returns the
Map (triggering a linter warning); replace the forEach with a simple for...of
loop to avoid returning a value. Locate the two occurrences where
newViewFiles.forEach(...) is used (the one that sets this.indices using
viewFileKey and index, and the similar pattern around line 286) and rewrite them
as: iterate newViewFiles with for (const [index, value] of
newViewFiles.entries()) { this.indices.set(viewFileKey(value), index); } so no
implicit return value is produced.
Comment on lines
+60
to
+63
| def __init__(self, action: Action, filename: str, pair_id: str = None): | ||
| self.action = action | ||
| self.filename = filename | ||
| self.pair_id = pair_id |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Controller command lookup ==="
rg -n -C2 'command\.pair_id|get_file\(command\.filename\)|Model\.make_key' src/python/controller/controller.py
echo "=== Model key/get API ==="
rg -n -C3 'def make_key|def file_key|def get_file' src/python/model/model.py
echo "=== Command construction sites ==="
rg -n -C2 'Controller\.Command\(' src/python/web/handler/controller.pyRepository: nitrobass24/seedsync
Length of output: 3079
pair_id is captured but not used for command target resolution.
Line 60–63 stores pair_id, but command execution at Line 604 resolves files by filename only. In multi-pair scenarios, same-name files can be acted on incorrectly.
Fix the lookup to pass pair_id:
🔧 Proposed fix
- file = self.__model.get_file(command.filename)
+ file = self.__model.get_file(command.filename, command.pair_id)🧰 Tools
🪛 Ruff (0.15.2)
[warning] 60-60: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
[warning] 60-60: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/controller/controller.py` around lines 60 - 63,
Controller.__init__ saves pair_id but the command execution path still resolves
targets using only filename, causing cross-pair ambiguity; update the
file/target lookup at the command execution site (the place that currently calls
the file-resolution with only filename) to pass self.pair_id as an additional
parameter, and update the called lookup function(s) (e.g., any
get_target_by_filename / resolve_target_by_filename) to accept and use pair_id
when querying so lookups are scoped to the correct pair; ensure any call sites
that now require the extra argument are updated accordingly.
Comment on lines
+24
to
+26
| def __init__(self, pair_id: str = None): | ||
| self.logger = logging.getLogger("ModelBuilder") | ||
| self.__pair_id = pair_id |
There was a problem hiding this comment.
pair_id constructor path is not fully wired, so propagation can remain inactive.
ModelBuilder now supports pair_id, but current instantiation still uses ModelBuilder() without passing it (see src/python/controller/controller.py Lines 101-104). That leaves self.__pair_id as None, defeating pair-aware model identity.
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 24-24: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
[warning] 24-24: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/controller/model_builder.py` around lines 24 - 26, ModelBuilder's
__init__ sets self.__pair_id but callers instantiate ModelBuilder() without
forwarding the current pair id, leaving __pair_id None; update every call site
that constructs ModelBuilder (e.g., the Controller code that calls
ModelBuilder()) to pass the active pair id (e.g., pass self.pair_id or the local
pair_id/context variable) so ModelBuilder(pair_id=...) is used, and ensure any
factory/helper that creates ModelBuilder also forwards the pair id.
- Use __CONFIG_DUMMY_VALUE sentinel instead of hardcoded string in migration - Add ThreadingWSGIServer so SSE connections don't block other requests - Call shutdown() before server_close() for clean server stop - Validate JSON body is dict in path pairs REST handlers - Harden PathPairsConfig.from_str with type checks and add_pair() uniqueness - Propagate pair_id to child ModelFiles in ModelBuilder Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/python/controller/model_builder.py (1)
24-26: 🧹 Nitpick | 🔵 TrivialMake
pair_idexplicitly optional and annotate__init__return type.Current annotation uses
str = None; switch to explicit optional typing and add-> Nonefor lint/type consistency.Proposed patch
- def __init__(self, pair_id: str = None): + def __init__(self, pair_id: Optional[str] = None) -> None: self.logger = logging.getLogger("ModelBuilder") self.__pair_id = pair_id#!/bin/bash # Verify the constructor signature still uses implicit Optional / missing return type rg -nP 'def __init__\(self,\s*pair_id:\s*str\s*=\s*None\)' src/python/controller/model_builder.py rg -nP 'def __init__\(self,\s*pair_id:\s*Optional\[str\]\s*=\s*None\)\s*->\s*None:' src/python/controller/model_builder.pyBased on learnings: "Maintain consistency by using typing.Dict, typing.List, and typing.Set across Python code. Do not suggest converting to built-in generic types (dict, list, set) on a per-file basis."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/controller/model_builder.py` around lines 24 - 26, Update the ModelBuilder constructor signature to use explicit optional typing and an explicit return annotation: change def __init__(self, pair_id: str = None): to def __init__(self, pair_id: Optional[str] = None) -> None: (import Optional from typing if not already) and keep assigning self.__pair_id = pair_id; ensure references to ModelBuilder.__init__, pair_id, and self.__pair_id remain unchanged.src/python/common/path_pairs_config.py (1)
115-120:⚠️ Potential issue | 🟠 MajorFail fast on malformed entries during
from_str()instead of silently dropping them.Lines 115-120 currently skip malformed/duplicate entries with a warning. That can silently erase mappings on the next save cycle. Raise
PersistErrorso load/backup recovery paths handle corruption explicitly.🧩 Proposed fix
- for pair_dict in pairs_list: - try: - config.add_pair(PathPair.from_dict(pair_dict)) - except (KeyError, TypeError, ValueError) as e: - _logger.warning("Skipping malformed path pair: %s", e) + for i, pair_dict in enumerate(pairs_list): + if not isinstance(pair_dict, dict): + raise PersistError("Invalid path pair at index {}: expected object".format(i)) + try: + config.add_pair(PathPair.from_dict(pair_dict)) + except (KeyError, TypeError, ValueError) as e: + raise PersistError("Invalid path pair at index {}: {}".format(i, str(e))) from e return config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/common/path_pairs_config.py` around lines 115 - 120, from_str() currently swallows malformed PathPair entries by catching KeyError/TypeError/ValueError in the loop over pairs_list and logging via _logger.warning; instead, stop silently dropping them and raise PersistError so callers can handle corruption explicitly. Change the except block around PathPair.from_dict(...) / config.add_pair(...) to raise a PersistError (include the original exception as context or in the message) rather than calling _logger.warning, preserving the error details and ensuring duplicate/malformed entries surface to load/backup recovery logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/common/path_pairs_config.py`:
- Around line 70-100: PathPairsConfig currently accesses and mutates shared list
self._pairs without synchronization; introduce a threading.RLock (e.g.,
self._lock) and wrap all accesses/mutations to _pairs in the lock (use with
self._lock:) in the pairs getter/setter and in get_pair, add_pair, update_pair,
remove_pair, and any to_str/serialization methods referenced around lines
122-127 so reads and writes cannot interleave; ensure add_pair/update_pair still
raise the same ValueError conditions but performed while holding the lock to
avoid race conditions.
In `@src/python/web/handler/path_pairs.py`:
- Around line 37-43: The code constructs a PathPair from unvalidated JSON fields
(data.get("name"/"remote_path"/"local_path"/"enabled"/"auto_queue")), which
allows non-string or non-boolean values to be persisted; validate each field
before creating PathPair: ensure name, remote_path, and local_path are strings
(or coerce via str() only if safe) and ensure enabled and auto_queue are
booleans (reject with a 400/BadRequest or normalize using explicit isinstance
checks and a clear conversion path); update the handler that builds PathPair to
perform these checks and return an error response on invalid types instead of
passing raw values into PathPair.
- Around line 51-72: In __handle_update, a race can cause
self.__config.update_pair(updated) to raise ValueError if the pair was deleted
after existing = self.__config.get_pair(pair_id); catch ValueError around the
update_pair call and return HTTPResponse(body="Path pair not found", status=404)
instead of letting it propagate, while leaving other exceptions unchanged so
they still surface as errors.
In `@src/python/web/web_app_job.py`:
- Around line 112-115: The stop() method currently dereferences self.server
unconditionally; change it to first check that self.server exists and is not
None (e.g., use getattr(self, "server", None) or hasattr(self, "server") and a
truthy check) before calling shutdown() and server_close(), and log a
debug/warning if the server was never initialized (this prevents an
AttributeError when run() failed during server creation and preserves the
original exception flow).
---
Duplicate comments:
In `@src/python/common/path_pairs_config.py`:
- Around line 115-120: from_str() currently swallows malformed PathPair entries
by catching KeyError/TypeError/ValueError in the loop over pairs_list and
logging via _logger.warning; instead, stop silently dropping them and raise
PersistError so callers can handle corruption explicitly. Change the except
block around PathPair.from_dict(...) / config.add_pair(...) to raise a
PersistError (include the original exception as context or in the message)
rather than calling _logger.warning, preserving the error details and ensuring
duplicate/malformed entries surface to load/backup recovery logic.
In `@src/python/controller/model_builder.py`:
- Around line 24-26: Update the ModelBuilder constructor signature to use
explicit optional typing and an explicit return annotation: change def
__init__(self, pair_id: str = None): to def __init__(self, pair_id:
Optional[str] = None) -> None: (import Optional from typing if not already) and
keep assigning self.__pair_id = pair_id; ensure references to
ModelBuilder.__init__, pair_id, and self.__pair_id remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 051e21dc-2c64-4744-bb6a-a793b4c2a790
📒 Files selected for processing (5)
src/python/common/path_pairs_config.pysrc/python/controller/model_builder.pysrc/python/seedsync.pysrc/python/web/handler/path_pairs.pysrc/python/web/web_app_job.py
Comment on lines
112
to
115
| def stop(self): | ||
| self.logger.debug("Stopping web server") | ||
| self.server.shutdown() | ||
| self.server.server_close() |
There was a problem hiding this comment.
Guard stop() when server initialization fails.
Line 114 dereferences self.server unconditionally. If server creation/bind fails in run(), cleanup can raise AttributeError and hide the original failure.
🔧 Proposed fix
def stop(self):
self.logger.debug("Stopping web server")
+ if self.server is None:
+ return
self.server.shutdown()
self.server.server_close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/web/web_app_job.py` around lines 112 - 115, The stop() method
currently dereferences self.server unconditionally; change it to first check
that self.server exists and is not None (e.g., use getattr(self, "server", None)
or hasattr(self, "server") and a truthy check) before calling shutdown() and
server_close(), and log a debug/warning if the server was never initialized
(this prevents an AttributeError when run() failed during server creation and
preserves the original exception flow).
- Add threading.RLock to PathPairsConfig for thread-safe access - Raise PersistError on malformed pairs in from_str() instead of swallowing - Validate field types (str/bool) in path pairs REST handlers - Catch ValueError race in __handle_update when pair deleted concurrently - Guard MyWSGIRefServer.stop() against None server - Fix ModelBuilder.__init__ typing: pair_id: Optional[str] = None Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 5, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/python/controller/model_builder.py (1)
24-26:⚠️ Potential issue | 🟠 MajorMake
pair_idnon-optional to prevent silent deactivation.Line 24 allows
pair_id=None, andsrc/python/controller/controller.pyLine 101 still constructsModelBuilder()without passing it. That makes Line 201 and Line 238 propagatepair_id=None, so pair-aware identity stays inactive.🔧 Proposed fix
- def __init__(self, pair_id: Optional[str] = None) -> None: + def __init__(self, pair_id: str) -> None: self.logger = logging.getLogger("ModelBuilder") self.__pair_id = pair_id# src/python/controller/controller.py self.__model_builder = ModelBuilder(pair_id=<active_pair_id>)As per coding guidelines, follow existing patterns for controller state management (persist → model_builder → model).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/python/controller/model_builder.py` around lines 24 - 26, The ModelBuilder constructor currently accepts pair_id: Optional[str] which allows silent deactivation; change ModelBuilder.__init__ to require pair_id: str (make it non-optional) and propagate that change by updating the instantiation in controller (replace self.__model_builder = ModelBuilder() with passing the active pair id, e.g. the controller's active_pair_id/get_active_pair method) so that pair-aware identity remains active; update any call sites of ModelBuilder() to supply the pair id and follow the controller state flow (persist → model_builder → model) when wiring the value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/python/common/path_pairs_config.py`:
- Around line 106-108: The remove_pair method currently silently does nothing if
the given pair_id is absent; change remove_pair (in class containing _pairs and
_lock) to detect whether any element was removed and raise ValueError("Path pair
not found") when none were removed so callers can distinguish
concurrent-delete/missing cases; then update the caller in path_pairs handler
(the code that calls remove_pair) to catch ValueError and return a 404 HTTP
response (e.g., HTTPResponse with "Path pair not found") instead of treating the
call as success.
- Around line 43-51: PathPair.from_dict currently trusts input dict types; add
explicit type checks for keys used there (pair_id/id, name, remote_path,
local_path, enabled, auto_queue) inside PathPair.from_dict: ensure id, name,
remote_path, local_path are str (use d["id"] / d.get("name", "") etc.) and that
enabled and auto_queue are bool (use d.get(..., True)), and if any value is the
wrong type raise a clear ValueError/TypeError mentioning the field and actual
value; keep behavior of using defaults for missing optional fields but validate
their types when present so invalid configs are rejected before constructing the
PathPair.
In `@src/python/web/web_app_job.py`:
- Around line 113-115: stop() can return early if called before run() sets
self.server, allowing the server thread to still call serve_forever() and
causing join to hang; add a shutdown coordination flag/condition: introduce a
boolean like self._shutdown_requested (or an Event) that cleanup()/stop() sets
unconditionally, then modify stop() to not only check self.server but also set
the flag and, if self.server is None, wait/signall that run() should shutdown
once it creates/assigns self.server; in run() after assigning self.server call
server.shutdown() immediately if self._shutdown_requested is set (or notify the
condition) so the server will not enter a permanent serve_forever(); ensure the
join() call then reliably completes.
- Around line 71-87: The __call__ middleware logs duration in a finally that
runs before the WSGI iterable is consumed (so streaming responses are logged
prematurely); fix by detecting when self.app(environ, _start_response) returns
an iterable and wrapping it in a generator wrapper that yields from the original
iterable and only executes the logging (using self.logger.log and self.level)
after iteration completes or on error, and also delegates .close() if present
and propagates exceptions; keep the existing _start_response behavior and ensure
you do not fully materialize the response body when wrapping.
---
Duplicate comments:
In `@src/python/controller/model_builder.py`:
- Around line 24-26: The ModelBuilder constructor currently accepts pair_id:
Optional[str] which allows silent deactivation; change ModelBuilder.__init__ to
require pair_id: str (make it non-optional) and propagate that change by
updating the instantiation in controller (replace self.__model_builder =
ModelBuilder() with passing the active pair id, e.g. the controller's
active_pair_id/get_active_pair method) so that pair-aware identity remains
active; update any call sites of ModelBuilder() to supply the pair id and follow
the controller state flow (persist → model_builder → model) when wiring the
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2433607b-4519-4554-8110-5550c4b7d4f9
📒 Files selected for processing (4)
src/python/common/path_pairs_config.pysrc/python/controller/model_builder.pysrc/python/web/handler/path_pairs.pysrc/python/web/web_app_job.py
Comment on lines
+106
to
+108
| def remove_pair(self, pair_id: str): | ||
| with self._lock: | ||
| self._pairs = [p for p in self._pairs if p.id != pair_id] |
There was a problem hiding this comment.
remove_pair should signal not-found to avoid masked concurrent-delete misses.
Line 106–108 silently succeeds when nothing is removed. Downstream, src/python/web/handler/path_pairs.py Line 100–105 can return success even if another request already deleted the pair.
🔧 Proposed fix
def remove_pair(self, pair_id: str):
with self._lock:
- self._pairs = [p for p in self._pairs if p.id != pair_id]
+ original_len = len(self._pairs)
+ self._pairs = [p for p in self._pairs if p.id != pair_id]
+ if len(self._pairs) == original_len:
+ raise ValueError("PathPair with id '{}' not found".format(pair_id))# src/python/web/handler/path_pairs.py
try:
self.__config.remove_pair(pair_id)
except ValueError:
return HTTPResponse(body="Path pair not found", status=404)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/common/path_pairs_config.py` around lines 106 - 108, The
remove_pair method currently silently does nothing if the given pair_id is
absent; change remove_pair (in class containing _pairs and _lock) to detect
whether any element was removed and raise ValueError("Path pair not found") when
none were removed so callers can distinguish concurrent-delete/missing cases;
then update the caller in path_pairs handler (the code that calls remove_pair)
to catch ValueError and return a 404 HTTP response (e.g., HTTPResponse with
"Path pair not found") instead of treating the call as success.
Comment on lines
+71
to
+87
| def __call__(self, environ, start_response): | ||
| method = environ.get("REQUEST_METHOD", "") | ||
| path = environ.get("PATH_INFO", "") | ||
| start = time.monotonic() | ||
| status_code = None | ||
|
|
||
| def _start_response(status, headers, *args): | ||
| nonlocal status_code | ||
| status_code = status.split(" ", 1)[0] | ||
| return start_response(status, headers, *args) | ||
|
|
||
| try: | ||
| return self.app(environ, _start_response) | ||
| finally: | ||
| duration_ms = (time.monotonic() - start) * 1000 | ||
| self.logger.log(self.level, "%s %s %s %.1fms", | ||
| method, path, status_code or "-", duration_ms) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the exact current state of the __call__ method
rg -n -A25 'def __call__' src/python/web/web_app_job.pyRepository: nitrobass24/seedsync
Length of output: 1060
🏁 Script executed:
#!/bin/bash
# Check the full _RequestLoggingMiddleware class
rg -n -B5 -A40 'class _RequestLoggingMiddleware' src/python/web/web_app_job.pyRepository: nitrobass24/seedsync
Length of output: 1764
🏁 Script executed:
#!/bin/bash
# Check if there are any streaming/SSE responses in the codebase
rg -n 'text/event-stream|StreamingResponse|yield' src/python/web/ --type pyRepository: nitrobass24/seedsync
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Verify Bottle framework usage and how app iterables are handled
rg -n 'from bottle|import bottle' src/python/web/ --type pyRepository: nitrobass24/seedsync
Length of output: 759
Streaming responses log duration before transmission completes.
The middleware returns the iterable directly in the try block, so the finally block executes immediately after creating the response object, not after the WSGI server consumes it. This causes streamed responses (like the SSE endpoint at web_app.py:142-156) to log incomplete timing and potentially incomplete status information.
🔧 Proposed fix
class _RequestLoggingMiddleware:
def __call__(self, environ, start_response):
method = environ.get("REQUEST_METHOD", "")
path = environ.get("PATH_INFO", "")
start = time.monotonic()
status_code = None
def _start_response(status, headers, *args):
nonlocal status_code
status_code = status.split(" ", 1)[0]
return start_response(status, headers, *args)
- try:
- return self.app(environ, _start_response)
- finally:
- duration_ms = (time.monotonic() - start) * 1000
- self.logger.log(self.level, "%s %s %s %.1fms",
- method, path, status_code or "-", duration_ms)
+ app_iter = self.app(environ, _start_response)
+
+ def _iter():
+ try:
+ for chunk in app_iter:
+ yield chunk
+ finally:
+ close = getattr(app_iter, "close", None)
+ if callable(close):
+ close()
+ duration_ms = (time.monotonic() - start) * 1000
+ self.logger.log(self.level, "%s %s %s %.1fms",
+ method, path, status_code or "-", duration_ms)
+
+ return _iter()🧰 Tools
🪛 Ruff (0.15.2)
[warning] 71-71: Missing return type annotation for special method __call__
(ANN204)
[warning] 77-77: Missing return type annotation for private function _start_response
(ANN202)
[warning] 77-77: Missing type annotation for *args
(ANN002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/web/web_app_job.py` around lines 71 - 87, The __call__ middleware
logs duration in a finally that runs before the WSGI iterable is consumed (so
streaming responses are logged prematurely); fix by detecting when
self.app(environ, _start_response) returns an iterable and wrapping it in a
generator wrapper that yields from the original iterable and only executes the
logging (using self.logger.log and self.level) after iteration completes or on
error, and also delegates .close() if present and propagates exceptions; keep
the existing _start_response behavior and ensure you do not fully materialize
the response body when wrapping.
Comment on lines
+113
to
+115
| if self.server is None: | ||
| self.logger.warning("Web server was never initialized; skipping shutdown") | ||
| return |
There was a problem hiding this comment.
stop() can miss shutdown during startup race and hang join.
If cleanup() calls stop() before run() assigns self.server, this returns early. The server thread can still enter serve_forever(), and Line 50 (join) may block indefinitely.
🔧 Proposed fix
-from threading import Thread
+from threading import Thread, Event
@@
class MyWSGIRefServer(bottle.ServerAdapter):
@@
def __init__(self, logger: logging.Logger, *args, **kwargs):
super().__init__(*args, **kwargs)
self.logger = logger
self.server = None
+ self._shutdown_requested = Event()
@@
def run(self, handler):
self.logger.debug("Starting web server")
handler = _RequestLoggingMiddleware(handler, logger=self.logger,
level=logging.DEBUG)
self.server = make_server(self.host, self.port, handler,
server_class=_ThreadingWSGIServer,
handler_class=_QuietHandler)
+ if self._shutdown_requested.is_set():
+ self.server.server_close()
+ return
self.server.serve_forever()
@@
def stop(self):
+ self._shutdown_requested.set()
if self.server is None:
self.logger.warning("Web server was never initialized; skipping shutdown")
return
self.logger.debug("Stopping web server")
self.server.shutdown()
self.server.server_close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/python/web/web_app_job.py` around lines 113 - 115, stop() can return
early if called before run() sets self.server, allowing the server thread to
still call serve_forever() and causing join to hang; add a shutdown coordination
flag/condition: introduce a boolean like self._shutdown_requested (or an Event)
that cleanup()/stop() sets unconditionally, then modify stop() to not only check
self.server but also set the flag and, if self.server is None, wait/signall that
run() should shutdown once it creates/assigns self.server; in run() after
assigning self.server call server.shutdown() immediately if
self._shutdown_requested is set (or notify the condition) so the server will not
enter a permanent serve_forever(); ensure the join() call then reliably
completes.
- remove_pair now raises ValueError when pair_id not found - Delete handler catches ValueError instead of pre-checking with get_pair - PathPair.from_dict validates types of all fields (str/bool checks) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 5, 2026
4 tasks
3 tasks
This was referenced Mar 18, 2026
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.
Summary
path_pairs.jsonconfig with CRUD operations and migration from legacy single remote_path/local_pathpair_idon ModelFile, composite keys in Model, pair-aware ModelBuilder, pair_id on Controller.Command, REST CRUD at/server/pathpairs, SSE serialization includes pair_idWhat's included
PathPairsConfigclass with JSON persistence,PathPairdataclass, legacy migrationpath_pairs_configwired into Context and seedsync.py startup/persistpair_idfield on ModelFile, composite key (pair_id:name) support in Modelpair_idparameter, stamps it on created ModelFilespair_idon Command (passed via?pair_id=query param from frontend)/server/pathpairswith PathPairsHandlerpair_idincluded in model serializationNot yet included (follow-up PRs)
Test plan
/server/pathpairsendpoints🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Refactor
Closes #122
Closes #23