Refactor bucket from audit (#531): parser decomposition, ViewFileService split, controller/config decoupling

CLAUDE.md

@@ -276,6 +276,17 @@ When you see these, open an issue for the extraction rather than working around
 - **Mechanical checks** (Python): ruff's `C901` (cyclomatic complexity) with `max-complexity = 12`. Enforced in CI; existing outliers are annotated with `# noqa: C901`. Test files (`tests/**`) are excluded via `per-file-ignores` in `pyproject.toml`.
 - **Ratchet pattern**: when a bound exists but the codebase has outliers, set the threshold just above the worst and drop it over time (same pattern we use for `--max-warnings` in `ng lint`).
 
+### Angular mutating-service contract
+
+Services that mutate a `BehaviorSubject` from a REST/HTTP call should follow **one** documented contract so callers can reason about side effects:
+
+- **Update the subject inside the returned observable's pipeline (`tap`/`map`), not via a second internal `.subscribe()`.** A second internal subscribe fires the side effect independently of the caller, runs the request a second time when the source is cold, and makes "when does the store update?" caller-independent and surprising. Folding the mutation into the returned pipeline means the store updates exactly when (and as many times as) the caller subscribes.
+- **Return a typed result and recover errors with `catchError`** so the caller always gets a value (or a typed failure) rather than an error notification. Map HTTP responses to a small typed result/`WebReaction`.
+
+`IntegrationsService` (`src/angular/src/app/services/settings/integrations.service.ts`) is the reference implementation: `create`/`update`/`remove`/`test` use `tap`/`map` to update `instancesSubject` and `catchError` to map failures to a typed result, with no second subscribe.
+
+**Known deviation (tracked):** `ConfigService.set` and `AutoQueueService.add/remove` still use the legacy `const obs = sendRequest(url); obs.subscribe({next: ...mutate...}); return obs;` pattern (a second internal subscribe). Migrating them to `tap` is **deferred**: their unit specs call `set()`/`add()`/`remove()` *without* subscribing and assert the `BehaviorSubject` mutated, which the current caller-independent subscribe makes pass. Because `RestService.sendRequest` uses `shareReplay(1)`, switching to `tap` defers the mutation to caller subscription and is **not** behavior-preserving against those unchanged tests. The migration must ship in its own PR that also updates those specs to subscribe.
+
 ## GitHub Repository
 
 - **Repo**: github.com/nitrobass24/seedsync

src/angular/src/app/models/config.ts

@@ -3,6 +3,17 @@
  * Note: Naming convention matches that used in the JSON.
  */
 
+/** The value any single config field may hold. */
+export type ConfigValue = string | number | boolean | null;
+
+/**
+ * A config section read as a string-keyed bag of ConfigValues. Sections do NOT
+ * `extends` this (that would collapse `keyof Section` to `string` and defeat the
+ * typo-catching ConfigValuePath in options-list.ts); it is only used as an
+ * explicit, localized cast target at the few dynamic section/option access sites.
+ */
+export type ConfigSection = Record<string, ConfigValue>;
+
 export interface General {
   log_level: string | null;
   verbose: boolean | null;
@@ -17,6 +28,7 @@ export interface Lftp {
   remote_path: string | null;
   local_path: string | null;
   remote_path_to_scan_script: string | null;
+  remote_python_path: string | null;
   use_ssh_key: boolean | null;
   num_max_parallel_downloads: number | null;
   num_max_parallel_files_per_download: number | null;
@@ -107,6 +119,7 @@ export const DEFAULT_LFTP: Lftp = {
   remote_path: null,
   local_path: null,
   remote_path_to_scan_script: null,
+  remote_python_path: null,
   use_ssh_key: null,
   num_max_parallel_downloads: null,
   num_max_parallel_files_per_download: null,

src/angular/src/app/pages/settings/options-list.spec.ts

@@ -0,0 +1,27 @@
+import { describe, it, expect } from 'vitest';
+import { getConfigValue } from './options-list';
+import { Config, DEFAULT_CONFIG } from '../../models/config';
+
+describe('getConfigValue', () => {
+  const config: Config = {
+    ...DEFAULT_CONFIG,
+    lftp: { ...DEFAULT_CONFIG.lftp, remote_path: '/remote', remote_port: 22 },
+    web: { ...DEFAULT_CONFIG.web, api_key: 'secret' },
+  };
+
+  it('reads a string field by typed path', () => {
+    expect(getConfigValue(config, ['lftp', 'remote_path'])).toBe('/remote');
+  });
+
+  it('reads a numeric field by typed path', () => {
+    expect(getConfigValue(config, ['lftp', 'remote_port'])).toBe(22);
+  });
+
+  it('reads a field from another section', () => {
+    expect(getConfigValue(config, ['web', 'api_key'])).toBe('secret');
+  });
+
+  it('returns null for a field whose value is null', () => {
+    expect(getConfigValue(config, ['lftp', 'local_path'])).toBeNull();
+  });
+});

src/angular/src/app/pages/settings/options-list.ts

@@ -1,13 +1,48 @@
-import { OptionType } from './option.component';
+import { Config, ConfigSection } from '../../models/config';
+import { OptionType, OptionValue } from './option.component';
+
+/**
+ * A [section, option] path into the Config model. The union over Config's
+ * sections keeps each tuple's second element constrained to keys of that
+ * section, so a typo or a renamed field is a compile error rather than a
+ * silently-broken magic string.
+ */
+export type ConfigValuePath = {
+  [S in keyof Config]: [S, keyof Config[S]];
+}[keyof Config];
+
+/**
+ * Conditions under which an option is rendered disabled. The component maps
+ * each flag to a runtime predicate; co-locating the condition with the option
+ * definition keeps the disable rules next to the options they affect.
+ */
+export type OptionDisabledWhen = 'pairsEnabled' | 'validateDisabled';
 
 export interface IOption {
   type: OptionType;
   label: string;
-  valuePath: [string, string];
+  valuePath: ConfigValuePath;
   description: string | null;
   disabled?: boolean;
   choices?: string[];
   requiresRestart?: boolean;
+  /** When set, the option is disabled while the named condition holds. */
+  disabledWhen?: OptionDisabledWhen;
+  /** Description shown in place of the default while disabledWhen is active. */
+  overrideNote?: string;
+}
+
+/** Note shown on options that Path Pairs overrides once any pair is enabled. */
+export const OVERRIDE_NOTE = 'Overridden by Path Pairs when any pair is enabled';
+
+/** Read a config value by its typed [section, option] path. */
+export function getConfigValue(config: Config, path: ConfigValuePath): OptionValue {
+  const [section, option] = path;
+  // path is a typo-checked ConfigValuePath; the read itself is dynamic, so the
+  // string-indexed section access is localized here through ConfigSection.
+  const sectionObj = config[section] as unknown as ConfigSection;
+  if (!sectionObj) return null;
+  return sectionObj[option as string] ?? null;
 }
 
 export interface IOptionsContext {
@@ -54,13 +89,17 @@ export const OPTIONS_CONTEXT_SERVER: IOptionsContext = {
       valuePath: ['lftp', 'remote_path'],
       description: 'Path to your files on the remote server',
       requiresRestart: true,
+      disabledWhen: 'pairsEnabled',
+      overrideNote: OVERRIDE_NOTE,
     },
     {
       type: OptionType.Text,
       label: 'Local Directory',
       valuePath: ['lftp', 'local_path'],
       description: 'Downloaded files are placed here',
       requiresRestart: true,
+      disabledWhen: 'pairsEnabled',
+      overrideNote: OVERRIDE_NOTE,
     },
     {
       type: OptionType.Text,
@@ -215,6 +254,8 @@ export const OPTIONS_CONTEXT_AUTOQUEUE: IOptionsContext = {
       valuePath: ['autoqueue', 'enabled'],
       description: null,
       requiresRestart: true,
+      disabledWhen: 'pairsEnabled',
+      overrideNote: OVERRIDE_NOTE,
     },
     {
       type: OptionType.Checkbox,
@@ -441,13 +482,15 @@ export const OPTIONS_CONTEXT_VALIDATE: IOptionsContext = {
       label: 'Auto-validate after download',
       valuePath: ['validate', 'auto_validate'],
       description: 'Automatically validate files when download completes. Requires post-download validation above.',
+      disabledWhen: 'validateDisabled',
     },
     {
       type: OptionType.Select,
       label: 'Hash Algorithm',
       valuePath: ['validate', 'algorithm'],
       description: 'Checksum algorithm used for both inline transfer verification and post-download validation',
       choices: ['md5', 'sha1', 'sha256'],
+      disabledWhen: 'validateDisabled',
     },
   ],
 };

src/angular/src/app/pages/settings/settings-page.component.ts

@@ -18,7 +18,10 @@ import { OptionComponent, OptionValue } from './option.component';
 import { PathPairsComponent } from './path-pairs.component';
 import { IntegrationsComponent } from './integrations.component';
 import {
+  ConfigValuePath,
   IOptionsContext,
+  OVERRIDE_NOTE,
+  getConfigValue,
   OPTIONS_CONTEXT_SERVER,
   OPTIONS_CONTEXT_DISCOVERY,
   OPTIONS_CONTEXT_CONNECTIONS,
@@ -86,7 +89,9 @@ export class SettingsPageComponent implements OnInit {
   );
   private badValueNotifs = new Map<string, Notification>();
 
-  private static readonly OVERRIDE_NOTE = 'Overridden by Path Pairs when any pair is enabled';
+  // Retained for the unit spec, which reads this via a statics cast. The
+  // canonical note now lives next to the option definitions in options-list.ts.
+  private static readonly OVERRIDE_NOTE = OVERRIDE_NOTE;
 
   ngOnInit(): void {
     this.connectedService.connected$.pipe(
@@ -121,47 +126,54 @@ export class SettingsPageComponent implements OnInit {
     });
   }
 
-  private static buildServerContext(hasEnabledPairs: boolean): IOptionsContext {
+  /**
+   * Apply the per-option disable rules carried by each option definition
+   * (disabledWhen/overrideNote). One generic transform replaces the former
+   * per-context builders: an option is disabled when its disabledWhen flag is
+   * currently active, and its description is swapped for overrideNote when one
+   * is provided.
+   */
+  private static applyDisableRules(
+    context: IOptionsContext,
+    active: { pairsEnabled: boolean; validateDisabled: boolean },
+  ): IOptionsContext {
     return {
-      ...OPTIONS_CONTEXT_SERVER,
-      options: OPTIONS_CONTEXT_SERVER.options.map((option) => {
-        if (hasEnabledPairs && (option.valuePath[1] === 'remote_path' || option.valuePath[1] === 'local_path')) {
-          return { ...option, description: SettingsPageComponent.OVERRIDE_NOTE, disabled: true };
+      ...context,
+      options: context.options.map((option) => {
+        if (option.disabledWhen && active[option.disabledWhen]) {
+          return option.overrideNote !== undefined
+            ? { ...option, disabled: true, description: option.overrideNote }
+            : { ...option, disabled: true };
         }
         return option;
       }),
     };
   }
 
+  private static buildServerContext(hasEnabledPairs: boolean): IOptionsContext {
+    return SettingsPageComponent.applyDisableRules(OPTIONS_CONTEXT_SERVER, {
+      pairsEnabled: hasEnabledPairs,
+      validateDisabled: false,
+    });
+  }
+
   private static buildAutoqueueContext(hasEnabledPairs: boolean): IOptionsContext {
-    return {
-      ...OPTIONS_CONTEXT_AUTOQUEUE,
-      options: OPTIONS_CONTEXT_AUTOQUEUE.options.map((option) => {
-        if (hasEnabledPairs && option.valuePath[1] === 'enabled') {
-          return { ...option, description: SettingsPageComponent.OVERRIDE_NOTE, disabled: true };
-        }
-        return option;
-      }),
-    };
+    return SettingsPageComponent.applyDisableRules(OPTIONS_CONTEXT_AUTOQUEUE, {
+      pairsEnabled: hasEnabledPairs,
+      validateDisabled: false,
+    });
   }
 
   private static buildValidateContext(validateEnabled: boolean): IOptionsContext {
-    return {
-      ...OPTIONS_CONTEXT_VALIDATE,
-      options: OPTIONS_CONTEXT_VALIDATE.options.map((option) => {
-        if (!validateEnabled && (option.valuePath[1] === 'auto_validate' || option.valuePath[1] === 'algorithm')) {
-          return { ...option, disabled: true };
-        }
-        return option;
-      }),
-    };
+    return SettingsPageComponent.applyDisableRules(OPTIONS_CONTEXT_VALIDATE, {
+      pairsEnabled: false,
+      validateDisabled: !validateEnabled,
+    });
   }
 
-  getOptionValue(config: Config | null, valuePath: [string, string]): OptionValue {
+  getOptionValue(config: Config | null, valuePath: ConfigValuePath): OptionValue {
     if (!config) return null;
-    const section = (config as unknown as Record<string, Record<string, OptionValue> | undefined>)[valuePath[0]];
-    if (!section) return null;
-    return section[valuePath[1]] ?? null;
+    return getConfigValue(config, valuePath);
   }
 
   onSetConfig(section: string, option: string, value: OptionValue, requiresRestart?: boolean): void {

src/angular/src/app/pages/settings/settings-page.disable-rules.spec.ts

@@ -0,0 +1,55 @@
+import '@angular/compiler';
+import { describe, it, expect } from 'vitest';
+import { SettingsPageComponent } from './settings-page.component';
+import { IOptionsContext } from './options-list';
+
+// buildValidateContext is a private static reached the same way the existing
+// settings-page spec reaches buildServerContext/buildAutoqueueContext. This
+// focused spec covers the validate ('validateDisabled') branch of the generic
+// applyDisableRules transform, which the existing spec does not exercise.
+interface ValidateStatics {
+  buildValidateContext(validateEnabled: boolean): IOptionsContext;
+}
+const statics = SettingsPageComponent as unknown as ValidateStatics;
+const buildValidateContext = (enabled: boolean): IOptionsContext => statics.buildValidateContext(enabled);
+
+describe('SettingsPageComponent.buildValidateContext', () => {
+  it('disables auto_validate and algorithm when validation is disabled', () => {
+    const ctx = buildValidateContext(false);
+    const autoValidate = ctx.options.find((o) => o.valuePath[1] === 'auto_validate')!;
+    const algorithm = ctx.options.find((o) => o.valuePath[1] === 'algorithm')!;
+
+    expect(autoValidate.disabled).toBe(true);
+    expect(algorithm.disabled).toBe(true);
+  });
+
+  it('keeps the original description on disabled validate options (no override note)', () => {
+    const ctx = buildValidateContext(false);
+    const autoValidate = ctx.options.find((o) => o.valuePath[1] === 'auto_validate')!;
+
+    // validateDisabled options carry no overrideNote, so the description is unchanged.
+    expect(autoValidate.description).toBe(
+      'Automatically validate files when download completes. Requires post-download validation above.',
+    );
+  });
+
+  it('does not disable auto_validate and algorithm when validation is enabled', () => {
+    const ctx = buildValidateContext(true);
+    const autoValidate = ctx.options.find((o) => o.valuePath[1] === 'auto_validate')!;
+    const algorithm = ctx.options.find((o) => o.valuePath[1] === 'algorithm')!;
+
+    expect(autoValidate.disabled).toBeFalsy();
+    expect(algorithm.disabled).toBeFalsy();
+  });
+
+  it('never disables the other validate options', () => {
+    const ctx = buildValidateContext(false);
+    const others = ctx.options.filter(
+      (o) => o.valuePath[1] !== 'auto_validate' && o.valuePath[1] !== 'algorithm',
+    );
+
+    for (const option of others) {
+      expect(option.disabled).toBeFalsy();
+    }
+  });
+});