Skip to content

Refactor RFC: Consolidate settings UI, defaults init, and featureMenu lifecycle into a deepened setup module #1347

Description

@VampireChicken12

[RFC] Refactor: Deepen Settings UI, Defaults Init, and Core Feature Setup

Problem

Three tightly coupled concerns create a maintenance trap with duplicated code, hidden side effects, and a registry blind spot:

1. Duplicated provider treessrc/pages/popup/Popup.tsx and src/pages/options/Options.tsx are near-identical (28 lines each, same NotificationsProvider → SettingsFilterProvider → QueryClientProvider → Settings nesting, same QueryClient hardcoded config). Any change to the provider chain or query config must be manually mirrored. There is nothing enforcing parity.

2. featureMenu bypasses the feature registryfeatureMenu is declared a coreFeatureKey in src/features/_registry/types.ts:21 but its lifecycle is managed by hand in src/pages/embedded/index.ts:103-115. A dedicated featureMenuOpenTypeChange message handler calls setupFeatureMenuEventListeners directly instead of routing through registry.notifyConfigChange. This means:

  • The registry's lifecycle guarantees (cleanup, ordered enable/disable, error handling) don't apply to featureMenu
  • A global window.cleanupFeatureMenuListeners pollutes the Window type
  • Anyone refactoring the registry won't know featureMenu is handled specially unless they also read embedded/index.ts

3. defaults.ts module-level IIFEsrc/defaults.ts:56-58 runs setDefaultValues() as an async IIFE on import, making imports of defaults.ts have invisible side effects with no sequencing control.

Proposed Interface

The deepened module lives at src/features/_setup/ with two public entry points:

// src/features/_setup/index.ts

/**
 * One-shot setup for the YouTube page content script.
 * Idempotent — safe to call across SPA navigations.
 * Returns an opaque handle for explicit teardown on pagehide.
 */
export function setupYouTubePage(): Promise<CleanupHandle>;

export interface CleanupHandle {
  dispose(): void;
}

/**
 * Pre-wired settings component for popup and options pages.
 * Wraps <Settings /> with all required providers in a single source of truth.
 */
export { SettingsPage } from ./components/SettingsPage;

Usage examples

embedded/index.ts — from ~181 lines to ~40:

import { setupYouTubePage } from "@/src/features/_setup";
import { browserColorLog } from "@/src/utils/logging";
import { formatError } from "@/utils/format/error";

if (window.self === window.top) {
  const init = () => setupYouTubePage().catch(err =>
    browserColorLog(`Setup failed: ${formatError(err)}`, "FgRed")
  );
  if (document.readyState === "loading") {
    document.addEventListener("DOMContentLoaded", init);
  } else {
    init();
  }
}
// error handlers remain unchanged

Popup.tsx and Options.tsx — from 28 lines to ~4 each:

import { SettingsPage } from "@/src/features/_setup";
export default function Settings() { return <SettingsPage />; }

What it hides internally

Complexity Where it moves
Provider tree duplication (NotificationsProvider, SettingsFilterProvider, QueryClientProvider + config) _setup/components/SettingsPage.tsx — single source
Defaults IIFE side effect Eliminated; setupYouTubePage calls setDefaultValues() explicitly
Guard state machine (isFirstLoad, isInitializing, isEnablingFeatures) lifecycle.ts — internal, cleaned per invocation
DOM plumbing (div#yte-message-from-youtube creation + append) lifecycle.ts — created on setup, removed on dispose
featureMenuOpenTypeChange 12-line manual handler coreFeatures.handleConfigChange()
cleanupFeatureMenuListeners global Module-level variable in coreFeatures.ts
languageChange case — disableAll + enableAll + title update coreFeatures.handleLanguageChange()
pagehide/ pageshow listener + cleanup wiring CleanupHandle.dispose()
buttonColorCache MutationObserver lifecycle.ts — created on setup, disconnected on dispose

Dependency Strategy

  • In-process: Everything is in-process. No I/O beyond browser.storage.local which is already wrapped.
  • Defaults: setDefaultValues() called explicitly as first init step — no more import-time IIFE.
  • featureMenu bridge: coreFeatures.ts imports registry, eventManager, and setupFeatureMenuEventListeners internally. Callers never see them.
  • Provider tree: SettingsPage.tsx imports @tanstack/react-query, NotificationsProvider, SettingsFilterProvider internally. Popup/Options never import them.

Testing Strategy

  • New boundary tests to write:
    • setupYouTubePage() — verify it initializes defaults, registers all features, enables them, and sets up the DOM message element
    • CleanupHandle.dispose() — verify it removes listeners, disconnects observers, tears down DOM
    • coreFeatures.handleConfigChange() — verify it calls setupFeatureMenuEventListeners with the right open type
    • coreFeatures.handleLanguageChange() — verify it triggers disableAll/enableAll cycle and updates menu title
  • Old tests to delete: N/A — no existing tests
  • Test environment: Vitest (in-process, matches Vite build), with browser.storage.local mocked

Implementation Recommendations

  • src/features/_setup/ owns: embedded page initialization, provider tree composition, core feature lifecycle bridging
  • It hides: DOM plumbing, initialization state machines, provider imports, featureMenu lifecycle details
  • It exposes: setupYouTubePage(), SettingsPage, CleanupHandle
  • Callers migrate by: replacing embedded/index.ts body with setupYouTubePage() call, replacing Popup.tsx/ Options.tsx with SettingsPage, removing the defaults IIFE
  • The featureMenu module itself (index.ts, utils.ts) stays untouched — only the calling pattern changes
  • The autoRegister.ts gets an optional initialState parameter to avoid redundant messaging

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    Status
    Todo

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions