Refactor bucket from audit (#531): parser decomposition, ViewFileService split, controller/config decoupling#543
Open
nitrobass24 wants to merge 6 commits into
Open
Refactor bucket from audit (#531): parser decomposition, ViewFileService split, controller/config decoupling#543nitrobass24 wants to merge 6 commits into
nitrobass24 wants to merge 6 commits into
Conversation
Hoist the 17 per-call-compiled regexes in __parse_jobs to module-level re.compile() constants (compiled once at import, not per parse() call) and split the ~410-line, complexity-48 method into focused helpers driven by a thin ordered dispatch loop: - _parse_pget_header_block, _parse_mirror_header, _parse_mirror_fl_header, _parse_filename_chunk, _build_chunk_transfer_state (header/data parsing) - _skip_mirror_noise, _skip_chunk_header, _skip_chmod_block, _skip_noise_line (match-and-ignore leaf helpers) - _is_valid_first_line, _is_orphan_progress_line (guard helpers) - _dispatch_job_line + __parse_jobs (thin loop) Strictly behavior-preserving: every compiled pattern is byte-identical to the original (verified), load-bearing branch order and lookahead/pop semantics on the shared lines list are unchanged, and the pget-vs-filename name-comparison asymmetry is preserved. The shared regex fragment strings move to module scope (mangling-safe) with the class attributes aliasing them, so _size_to_bytes, _eta_to_seconds, and the out-of-scope __parse_queue are unaffected. Every function now measures cyclomatic complexity <= 12 (max is 10); the # noqa: C901 on __parse_jobs is removed (now 5). __parse_queue is left as-is. The existing 66 tests pass unchanged (no assertions modified); adds 34 focused helper tests including a compile-once identity regression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…leService (#524) Split the ViewFileService god service incrementally and behavior-preservingly: - Extract the status->capability state machine into a pure, separately-tested module view-file-capabilities.ts. The four deletion/extraction allow-lists that were duplicated copy-paste arrays are now named constants (single source of truth): QUEUEABLE_STATUSES, STOPPABLE_STATUSES, LOCAL_ACTION_STATUSES (shared verbatim by isExtractable + isLocallyDeletable), REMOTELY_DELETABLE_STATUSES, VALIDATABLE_STATUSES. Pure mapState() + deriveCapabilities() reproduce the exact switch (incl. DEFAULT->STOPPED-when-both-sizes>0 and unknown->DEFAULT) and the exact capability predicates, keyed off RAW nullable sizes for isValidatable / validateTooltip. createViewFile now calls them and spreads the result; its signature, percentDownloaded math, pairName resolution, and ViewFile shape are byte-for-byte identical. - Extract ViewFileSelectionService owning checkedSet, lastCheckedKey, and the checked$ stream. It is input-driven (callers pass display-order key lists) with no back-reference to ViewFileService, so the dependency stays acyclic (view-file-sort.service already injects ViewFileService). ViewFileService keeps toggleCheck/shiftCheck/checkAll/uncheckAll/checked$ as thin delegations; updateCheckedState still re-spreads only flipped rows (issue #521 identity); buildViewFromModelFiles uses selection.isChecked + selection.pruneRemoved. ViewFileService's public API and exports are unchanged, so file-list.component, file-options.component, view-file-sort.service and app.config need no edits. Existing view-file.service.spec.ts passes with zero assertion changes; added view-file-capabilities.spec.ts (34) and view-file-selection.service.spec.ts (16). DEFERRED: the ViewFileCommandService split (createAction/queue/.../bulk*). That slice straddles diffing-owned prevModelFiles and selection-owned check state; a clean extraction needs a ModelFile-resolver + checked-snapshot threaded in or a back-reference, both larger seams than a strictly behavior-preserving pass warrants here. Recommend as a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ons (#525) Address the four strictly behavior-preserving smells in #525: (a) Move Command/Action/ICallback, CommandProcessWrapper, and MAX_CONCURRENT_COMMAND_PROCESSES into controller/commands.py. Controller re-exports them as class attributes (identity preserved), and CommandPipeline imports them directly, removing the TYPE_CHECKING + lazy 'from .controller import Controller' imports and the controller_cls plumbing threaded through dispatch. (b) Extract sync_persist into a PersistSync collaborator (controller/persist_sync.py) injected into both CommandPipeline and ModelUpdater at construction, removing the placeholder lambda and post-hoc re-patch in Controller.__init__. ModelUpdater keeps a thin sync_persist_to_all_builders() delegating to PersistSync.sync(). (c) Move conditional-disable metadata (disabledWhen/overrideNote) onto the option definitions in options-list.ts; the component applies one generic applyDisableRules transform. Static wrappers retained so existing specs pass unchanged. (d) Type config access: ConfigValuePath is derived from the Config model, IOption.valuePath is typed against it, and the 'as unknown as' casts in config.service.ts and settings-page.component.ts are removed via a typed getConfigValue accessor and config section index signatures. (e) Documented the Angular mutating-service result/error contract in CLAUDE.md and confirmed IntegrationsService conforms; the ConfigService/AutoQueueService tap migration is explicitly deferred (not behavior-preserving against unchanged specs). New focused tests: test_commands.py, test_persist_sync.py, options-list.spec.ts, settings-page.disable-rules.spec.ts. All safety-net suites pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #523 decomposition added helpers without parameter type annotations; CI's strict pyright (Python Type Check) requires them. Annotate the regex-match params (re.Match[str]) and add an assert where the caller guarantees a non-None match. No behavior change; 100 parser tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…525 review) Review found #525's AC#4 "removed the casts" by adding index signatures (Config extends Record<...>, sections extends ConfigSection), which collapses keyof to string and lets typos compile — a type-safety regression, the opposite of the AC. Remove the index signatures so Config/sections keep their specific keys and ConfigValuePath becomes a real typo-checked discriminated union; keep a single localized record cast at the genuinely-dynamic access sites (ConfigService.set, getConfigValue). This immediately surfaced a latent bug the index signature was masking: the options list references ['lftp','remote_python_path'] (a real backend key, config.py:268) but the Lftp model never declared it. Added the field + default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 57 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (22)
✨ 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 |
The #525 edit left command_pipeline.py not ruff-format clean (CI's Python Lint runs ruff format --check). Format-only; tests unchanged. Co-Authored-By: Claude Opus 4.8 (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.
Fixes the 🏗️ Architecture / refactor bucket of tracking issue #531 (3 issues) — the final audit bucket. All changes are strictly behavior-preserving: existing test assertions pass unchanged; new focused tests cover each extracted unit.
Note
Intentional multi-issue grouping (per the maintainer's request). #524 and #525 are large architectural refactors done as contained, behavior-preserving subsets, with the deeper splits tracked as follow-ups (#541, #542) — a smaller correct refactor beats a risky big one.
How this was built
A multi-agent workflow (investigate → sequential implement → adversarial review) with the verify phase explicitly checking behavior preservation and that no existing test was weakened to make the refactor pass. I then ran the full Python + Angular suites and pyright/tsc myself and fixed the gaps the agents'
ruff/ng testruns missed (below).Issues
__parse_jobs(complexity 48) into focused_parse_*/_skip_*helpers driven by a thin dispatch loop, and hoisted the ~17 per-call-compiled regexes to module constants. Every function ≤ 12 (__parse_jobsnow ~5;# noqa: C901removed). Byte-identical parse output.ViewFileSelectionService(selection/check/shift/range) and a testedview-file-capabilities.ts(one source of truth for status→capability).ViewFileServiceslimmed accordingly.Command/Action/ICallback/CommandProcessWrapper+ the concurrency cap to a sharedcommands.py(removed the lazy import +controller_clsplumbing); extractedsync_persistinto an injectedPersistSync; moved option disable-rule metadata onto the option definitions; made config access genuinely typo-safe.Review findings → resolution (the parts I fixed after the agents)
re.Match[str], etc.). The agents had runruff(which doesn't type-check), not pyright.as unknown ascasts by adding index signatures (Config extends Record<…>), which collapseskeyoftostringand lets typos compile — the opposite of the goal. I removed the index signatures soConfigValuePathis a real typo-checked union, with a single localized cast at the genuinely-dynamic sites. This surfaced a latent bug: the options list referenceslftp.remote_python_path(a real backend key) that theLftpmodel never declared — now added.Deferred → follow-ups
ViewFileCommandService(the command-dispatch slice of Refactor: split ViewFileService god service & de-duplicate capability state machine #524; it straddles diffing + selection state).ConfigService.set/AutoQueueServiceto the documentedtap-based mutating-service contract (needs its specs updated to subscribe; not behavior-preserving against the current unchanged specs). The contract itself is now documented inCLAUDE.md, withIntegrationsServiceas the reference.Test plan
test_scan_file_with_latin_chars, is a pre-existing macOS-onlyErrno 92that passes on CI's Linux runner).ruff check+C901(≤12) clean; pyright (strict) clean acrosscontroller/,lftp/,web/.ng lint --max-warnings 0+tscclean.Closes #523. Closes #524. Closes #525.
🤖 Generated with Claude Code