Skip to content

refactor(angular): deduplicate services/components — misc shrinks #676

Description

@nitrobass24

Context

From a repo-wide over-engineering audit (ponytail-audit). Frontend duplication and boilerplate — same logic, fewer lines. No behavior changes. Estimated cut: ~350 lines.

Findings

  1. models/config.ts:111-200 (~55): eight per-section DEFAULT_* consts re-spread into DEFAULT_CONFIG; the section consts are referenced only by the dead barrel file and one spec. Collapse to a single DEFAULT_CONFIG object literal.
  2. IntegrationsService vs PathPairsService (services/settings/, ~50): the same CRUD-over-HTTP service twice (BehaviorSubject + connected$ refresh + create/update/remove with identical tap + catchError(409 rethrow) bodies), differing only in BASE_URL and item type. Extract one generic collection-service base (generic over item type + URL), two thin instantiations. Keep the CLAUDE.md mutating-service contract (mutations in the returned pipeline via tap; IntegrationsService is the documented reference implementation — update CLAUDE.md's reference if the file moves).
  3. Filter criteria class hierarchy (services/files/view-file-filter.service.ts:8-48,82-92, ~45): ViewFileFilterCriteria interface + AndFilterCriteria/StatusFilterCriteria/NameFilterCriteria classes + 4-branch buildFilterCriteria. The sibling ViewFileComparator is already a plain function type — do the same: type ViewFilePredicate = (f: ViewFile) => boolean; AndFilterCriteria becomes &&.
  4. settings-page (pages/settings/settings-page.component.ts:55-66,96,162-191, ~38): 4 static buildXContext wrappers each calling applyDisableRules with 2 of 3 flags hardcoded false → one call site with active flags; 12 readonly OPTIONS_CONTEXT_X = OPTIONS_CONTEXT_X template aliases → one readonly OPTIONS map; OVERRIDE_NOTE field "retained for the unit spec" → fix the spec to import the source constant.
  5. Double-click-confirm state machine copy-pasted verbatim into three components (pages/files/file.component.ts:199-227, pages/files/bulk-action-bar.component.ts:66-100, pages/settings/path-pairs.component.ts:254-276, ~35): confirmingDelete/confirmResetTimer/setConfirming/resetConfirmState/clearConfirmTimer, 3000ms. Extract one shared helper class or directive. Remember the OnPush + setTimeout rule: cdr.markForCheck() inside the timeout callback.
  6. testDiscord/testTelegram byte-identical but for URL segment; same catchError message-extraction appears a third time in IntegrationsService.test; settings-page mirrors with testingDiscord/testingTelegram/discordResult/telegramResult + two identical handlers (services/settings/notifications.service.ts, settings-page.component.ts:83-86,243-267, ~33). Parameterize by channel.
  7. LoggerService (services/utils/logger.service.ts, ~30): level is never written anywhere, so all four getters unconditionally return the bound console method; local LogLevel enum has no external consumer. Reduce to four console.* bindings (keep the injectable so specs can stub).
  8. logs-page hand-rolled trackBy (pages/logs/logs-page.component.ts:103-110,204-233, ~30): two WeakMaps + monotonic counters encoding "track by object identity", which @for/*cdkVirtualFor do natively. Drop.
  9. routes.ts:49-76 (~28): ROUTES hand-duplicates ROUTE_INFOS (same 5 paths/components). Derive: ROUTE_INFOS.map(({path, component}) => ({path, component})) + redirect.
  10. mapState 26-line switch (services/files/view-file-capabilities.ts:90-119, ~24): ModelFileState and ViewFileStatus share byte-identical string values for all 12 shared members; only DEFAULT differs and the default: arm is unreachable. One ternary + cast. Also (~6): isExtractable and isLocallyDeletable are computed from the byte-identical expression (view-file-capabilities.ts:73-74,147-148, models/view-file.ts:22-23) — one flag; file.component.isExtractable() already ANDs isArchive at the call site.
  11. STATE_LOOKUP/LEVEL_LOOKUP (models/model-file.ts:38-51, models/log-record.ts:31-38, ~22): tables mapping an enum's own uppercase keys onto its members → Enum[k as keyof typeof Enum] ?? DEFAULT.
  12. Small ones (~40 total): ServerStatusService repeats a 12-line initial-status literal twice → disconnectedStatus(msg) factory (server-status.service.ts:15-27,56-68); ArrInstanceCreate → Omit<ArrInstance, 'id'> (models/arr-instance.ts:14-20); duplicate {success, message} interface declared in both integrations.service.ts:14-17 and notifications.service.ts:6-9 → one shared type; constant-returning NotificationRule.level functions → plain field (pages/main/header.component.ts:13-18,40-70); single-call-site URL builder lambdas → inline template strings (autoqueue.service.ts:13-15, config.service.ts:31-32); Localization class of nested static classes → plain const object (models/localization.ts); LogService.fetchHistory 4 sequential if HttpParams builds → Object.entries loop, and drop the before param nothing passes (services/logs/log.service.ts:9-14,62-69).

Constraints

  • Zero user-visible behavior change.
  • Specs updated alongside; no action/feature loses coverage.
  • npx ng lint and npx ng test pass (Node ≥22.22.3).

Acceptance

  • Each item applied or noted as intentionally skipped (with reason)
  • Lint + unit tests + build pass

Workflow: branch refactor/angular-shrinks off develop, PR to develop. Items are independent — split if the diff gets big. Note item 1 conflicts textually with the barrel-file deletion issue; land whichever first and rebase.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    javascriptPull requests that update javascript coderefactorCode refactoring and cleanup

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions