crank/__REVIEW_FRONT.md

# Frontend Refactoring Plan

## Purpose

This document defines the frontend refactoring track for the Alpine.js UI. It is based on the current implementation, the observed code smells, and the explicit review findings.

The goal is not to replace Alpine.js. The stack choice is valid for the current product size. The goal is to make the UI:

- safe against obvious XSS classes;
- internally consistent;
- easier to maintain;
- easier to test;
- easier to ship and profile.

This document is implementation-oriented and split into concrete tracks, slices, file ownership, and acceptance criteria.

## Scope

The frontend refactoring scope covers:

1. XSS hardening and safe DOM rendering;
2. shell/nav/auth unification;
3. wizard modularization;
4. module-pattern unification;
5. removal of inline event handler patterns;
6. lightweight build pipeline introduction;
7. CSS class cleanup instead of imperative inline styles;
8. low-risk performance improvements;
9. frontend observability and testability improvements.

## Non-Goals

This plan does not assume:

- migration to React/Vue/Svelte;
- a full SPA rewrite;
- a design-system rewrite;
- replacing Alpine.js as the core UI model.

The target is a cleaner Alpine-compatible codebase, not a framework migration.

## Current State Summary

### Valid architectural choices

- Alpine.js is an appropriate choice for this admin UI size.
- `api.js` is already a strong shared transport layer.
- `ui-feedback.js` is simple and correct.
- the visual language and token system are already above average for an admin panel.
- EN/RU i18n support is already broad and worth preserving.

### Primary issues

1. unsafe `innerHTML` and string-built DOM with API data;
2. mixed UI architecture:
   - Alpine on some pages;
   - imperative DOM on others;
3. `wizard.js` is a monolith;
4. duplicated shell logic between `auth.js` and `nav.js`;
5. inconsistent module patterns;
6. inline `onclick="..."` string injection in some renderers;
7. no bundling/build pipeline;
8. direct `element.style` mutation where CSS classes should be used;
9. avoidable repeated filtering/computation in Alpine render paths.

## Guiding Rules

- Keep Alpine.js.
- Centralize common frontend primitives instead of re-implementing them page by page.
- Prefer DOM node construction and `textContent` over string-built HTML.
- Restrict `innerHTML` to trusted, static, translation-reviewed content only.
- Keep shared shell state in one place.
- Break the wizard by concern, not by arbitrary line count alone.
- Introduce the lightest build pipeline that solves the real problems.

## Track A: XSS Hardening

### Problem

Several files still render data from API responses through `innerHTML` and string concatenation without guaranteed escaping.

Known examples from review:

- `apps/ui/js/secrets.js`
- `apps/ui/js/workspace.js`
- error rendering paths where backend-provided strings are interpolated into HTML

This is a real frontend safety issue even in an authenticated admin panel.

### Goal

No untrusted API data is inserted through raw `innerHTML`.

### Target Design

Introduce a small shared DOM/safety utility module:

```text
apps/ui/js/utils/
  dom.js
  escape.js
```

### `utils/escape.js`

Responsibilities:

- `escapeHtml(value)`
- `escapeAttr(value)` if attribute-safe escaping is still needed during transition

Rule:

- this file becomes the only source of escape helpers;
- page scripts stop defining local copies such as the current `escapeHtml()` inside `wizard.js`.

### `utils/dom.js`

Responsibilities:

- helper builders for repetitive patterns:
  - `textNode(value)`
  - `el(tag, className?, text?)`
  - `clear(node)`
  - `appendChildren(node, children)`
- helper renderers for empty/error banners when they currently rely on string HTML

### Required Migrations

#### Slice A1: shared escape helpers

Files:

- `apps/ui/js/wizard.js`
- `apps/ui/js/secrets.js`
- `apps/ui/js/workspace.js`
- any other file using local escape helpers or raw string interpolation into HTML

Actions:

- move escape helpers into shared `utils/escape.js`;
- remove duplicate local definitions.

#### Slice A2: high-risk string renderers

Priority files:

- `apps/ui/js/secrets.js`
- `apps/ui/js/workspace.js`
- `apps/ui/js/api-keys.js` if any dynamic HTML remains
- `apps/ui/js/logs.js` / `usage.js` review for dynamic interpolation patterns

Actions:

- replace `innerHTML` row rendering with node creation;
- use `textContent` for names, labels, and API-provided error strings;
- use explicit element creation for buttons and actions.

#### Slice A3: error rendering contract

Actions:

- never assign backend error text to `innerHTML`;
- render API errors through:
  - `textContent`
  - safe toast body
  - pre-defined empty/error state templates

Explicit case to fix:

- `apps/ui/js/secrets.js`
  - current pattern builds `profilesList.innerHTML` using `state.error`
  - `state.error` is populated from caught API/fetch error text
  - that makes server-provided message text an unsafe HTML source
  - this renderer must be rewritten to build DOM nodes and place the message through `textContent`

### Acceptance Criteria

- no API-provided field is rendered via raw `innerHTML`;
- `escapeHtml()` exists once, in a shared utility file;
- XSS-sensitive renderers use `textContent` or safe DOM builders.

## Track B: Shell/Auth/Nav Unification

### Problem

`auth.js` and `nav.js` both:

- read `localStorage.getItem('crank_user')`;
- derive role/workspace labels;
- update `.nav-avatar`, `.user-dropdown-name`, `.user-dropdown-role`.

This creates duplicate state and drift.

### Goal

One module owns shell identity state and shell rendering.

### Target Design

Introduce:

```text
apps/ui/js/shell/
  identity.js
  nav.js
  routes.js
```

### Ownership

#### `shell/identity.js`

Responsibilities:

- session cache;
- local mirror persistence;
- deriving initials;
- deriving role label;
- deriving current workspace label;
- rendering shell identity DOM;
- broadcasting session change events.

Functions to absorb from current files:

- from `auth.js`:
  - `roleLabel`
  - `currentWorkspaceLabel`
  - `clearUserMirror`
  - `primaryMembership`
  - `initials`
  - `persistUserMirror`
  - `mirroredUser`
  - `renderShellIdentity`
  - `replaceSession`
- from `nav.js`:
  - `currentUser`
  - `fillUserInfo`

#### `shell/nav.js`

Responsibilities:

- dropdown open/close behavior;
- mobile nav behavior;
- binding click handlers for:
  - logout
  - profile
  - settings

It must not own identity derivation anymore.

#### `auth.js`

Responsibilities after refactor:

- login/logout/session fetch;
- route guarding;
- delegating shell identity updates to `shell/identity.js`.

### Slice Plan

##### Slice B1: move identity rendering

- create `shell/identity.js`;
- move identity ownership there;
- update `auth.js` and `nav.js` to consume it.

##### Slice B2: remove duplicate storage constants

- `STORAGE_KEY` exists once;
- `roleLabel()` exists once.

##### Slice B3: event-driven shell refresh

- shell reacts to `crank:sessionchange`;
- individual page scripts stop updating shell identity directly.

### Acceptance Criteria

- only one module owns mirrored user state;
- `auth.js` and `nav.js` no longer duplicate shell rendering logic;
- shell identity remains correct after login/logout/workspace switch.

## Track C: Wizard Modularization

### Problem

`apps/ui/js/wizard.js` is a monolith of roughly 3k lines, holding:

- global state;
- step navigation;
- 5 protocol flows;
- file uploads;
- mapping;
- test-run handling;
- import/export;
- UI state transitions.

This is the frontend equivalent of the `postgres.rs` monolith.

### Goal

Split wizard code by stable responsibility while keeping the current HTML-driven workflow.

### Target Layout

```text
apps/ui/js/wizard/
  index.js
  state.js
  navigation.js
  protocol-capabilities.js
  upstream.js
  auth-selector.js
  mapping.js
  test-run.js
  protocol-rest.js
  protocol-graphql.js
  protocol-grpc.js
  protocol-websocket.js
  protocol-soap.js
  shared.js
```

### Ownership by Module

#### `wizard/state.js`

Responsibilities:

- encapsulate all mutable wizard state;
- replace top-level globals with a single state object and accessors.

State currently to move:

- `currentStep`
- `wizardProtocol`
- `wizardMode`
- `wizardEditId`
- `wizardWorkspaceId`
- `wizardCurrentOperation`
- `wizardCurrentVersion`
- upload buffers
- protocol capability cache
- test-run preview state

Target API:

- `createWizardState()`
- `getWizardState()`
- `resetWizardState()`

#### `wizard/navigation.js`

Responsibilities:

- dynamic step loading;
- `goToStep`
- `loadStep`
- `step3PanelId`
- sidebar/progress rendering;
- continue/back button states.

Important cleanup:

- replace inline style mutation for disabled buttons with CSS classes.

#### `wizard/protocol-capabilities.js`

Responsibilities:

- defaults and fetched capability model;
- `defaultProtocolCapabilities()`
- `currentProtocolCapabilities()`
- `loadProtocolCapabilities()`

#### `wizard/upstream.js`

Responsibilities:

- upstream dropdown rendering;
- upstream auth badge rendering;
- upstream create/edit interactions;
- upstream selection state.

#### `wizard/auth-selector.js`

Responsibilities:

- secret/auth profile selection;
- quick-create secret/profile flows;
- auth form validation and serialization.

#### `wizard/mapping.js`

Responsibilities:

- input/output mapping;
- preview helpers;
- field inference helpers;
- path defaulting.

#### `wizard/test-run.js`

Responsibilities:

- test-run request build;
- test-run submission;
- mode-aware result rendering for:
  - unary
  - window
  - session
  - async job

#### `wizard/protocol-*.js`

Responsibilities:

- only protocol-specific behavior.

Examples:

`protocol-graphql.js`

- selected GraphQL type;
- query/mutation defaults;
- operation name extraction;
- top-level field extraction.

`protocol-grpc.js`

- descriptor upload;
- service/method rendering;
- reflection-related flow.

`protocol-websocket.js`

- subscribe/unsubscribe templates;
- heartbeat/reconnect form state.

`protocol-soap.js`

- WSDL/XSD upload;
- service catalog inspection;
- binding apply.

### Migration Order

##### Slice C1: state + navigation split

- extract state and navigation first;
- preserve current behavior.

##### Slice C2: protocol capability + shared helpers

- move generic helper logic out of main file.

##### Slice C3: GraphQL/gRPC/REST protocol extraction

- highest-traffic protocol flows first.

##### Slice C4: WebSocket/SOAP protocol extraction

- move less common but newer protocol flows.

##### Slice C5: auth selector + test-run split

- finalize with secret/auth and result handling modules.

##### Slice C6: thin entrypoint

- `wizard/index.js` becomes the only page bootstrap file.

### Acceptance Criteria

- no frontend file comparable in maintenance risk to current `wizard.js`;
- wizard state is centralized, not scattered across globals;
- protocol-specific logic is isolated by file.

## Track D: Module Pattern Unification

### Problem

Current JS modules use mixed patterns:

- IIFE modules with encapsulation;
- top-level globals in page scripts;
- direct `window` leakage.

### Goal

Move all first-party UI code to one consistent browser-friendly module pattern.

### Recommended Target

Use build-generated ES module bundles, while keeping runtime API globals explicit where needed.

During transition:

- keep page entrypoints small;
- expose only intentionally public page bootstrap functions on `window`;
- no free-floating utility functions in global scope.

### Rules

- utility modules must never write to `window` unless they are explicitly public;
- page entrypoints may attach one namespace if needed:
  - `window.CrankWizard`
  - `window.CrankShell`
- generic helpers must stay module-local.

### Acceptance Criteria

- no stray utility functions like `escapeHtml()` living in global scope;
- global exposure is explicit and minimal.

## Track E: Event Handling Cleanup

### Problem

Some renderers still inject handlers through HTML strings such as:

- `onclick="switchWorkspace('...')"`

This is brittle and unsafe if values contain quotes.

### Goal

All dynamic UI actions use bound event listeners, not injected inline JS.

### Required Migrations

Priority files:

- `apps/ui/js/workspace.js`
- any renderer that currently injects action handlers via HTML strings

Approach:

- build nodes;
- attach `addEventListener`;
- carry identifiers through closures or `data-*` attributes.

### Acceptance Criteria

- no dynamic `onclick="..."` string generation remains in first-party code.

## Track F: Build Pipeline

### Problem

The current UI ships many discrete JS files directly, without bundling or minification. Vendor files are committed in-repo and loaded directly.

This is acceptable for early stages but no longer ideal for:

- cache invalidation;
- dependency graph hygiene;
- payload count;
- module splitting;
- future lint/test integration.

### Goal

Introduce a lightweight build pipeline without turning the UI into a heavyweight SPA toolchain.

### Recommended Tooling

Use `esbuild`.

Reasoning:

- minimal configuration;
- fast;
- good enough for bundling this codebase;
- easy Docker integration.

### Target Layout

```text
apps/ui/
  src/
    js/
      ...
  public/
    ...
  dist/
```

This does not require a full source tree rewrite immediately. A transitional layout is acceptable:

- keep current HTML layout;
- bundle JS entrypoints into `dist/js/*.js`;
- keep CSS mostly as-is initially.

### Required Outputs

- one bundle per page entrypoint where needed:
  - `catalog`
  - `agents`
  - `api-keys`
  - `logs`
  - `usage`
  - `settings`
  - `workspace-setup`
  - `wizard`
  - `secrets`
- shared chunk for common utilities

### Docker Changes

UI Docker build should:

1. install npm deps;
2. run build;
3. serve bundled assets via nginx.

### Vendor Policy

Move from committed vendor runtime files toward npm-managed dependencies where possible:

- `alpinejs`
- `js-yaml`

If some vendored asset remains, it must be intentional and documented.

### Acceptance Criteria

- first-party JS is bundled and minified;
- cache invalidation is deterministic;
- page HTML references built bundles, not a long list of raw scripts.

## Track G: CSS State Cleanup

### Problem

Some UI states are expressed by directly mutating inline styles in JS.

Example class of issue:

- button disabled visuals set by `element.style.opacity`, `style.cursor`, etc.

### Goal

Move stateful visuals into CSS classes.

### Target Approach

Introduce semantic classes:

- `.is-disabled`
- `.is-hidden`
- `.is-loading`
- `.is-active`
- `.is-selected`

JS only toggles classes and attributes.

### Priority File

- `apps/ui/js/wizard.js`

### Acceptance Criteria

- no UI state requires imperative style strings when a reusable class can express it.

## Track H: Performance Polish

### Problem

Some Alpine getters recompute filters repeatedly.

Known example:

- operations tab counts in `catalog.js` recompute filtered lists multiple times per render.

### Goal

Reduce avoidable repeated work without introducing premature complexity.

### Strategy

#### `catalog.js`

Replace repeated tab filtering with a cached derived snapshot, for example:

- one getter that computes filtered operations once;
- one derived count map per tab;
- `tabCount(tabId)` reads from that cached map.

#### General Rule

Only optimize:

- repeated O(n) list filters inside render loops;
- repeated expensive formatting inside x-for;
- repeated DOM queries in hot paths.

Do not over-engineer with memoization everywhere.

### Acceptance Criteria

- obvious repeated list filtering in Alpine render paths is removed;
- rendered behavior remains identical.

## Track I: Template and i18n Safety Cleanup

### Problem

There are small but risky patterns around naming collisions and template ergonomics.

Known example:

- `x-for="t in tabDefs"` where `t` also exists globally as the translation function.

### Goal

Avoid template variable names that collide with global utility names.

### Required Rule

Within Alpine templates:

- do not use `t`, `tf`, or other globally meaningful helper names as loop variables.

Use:

- `tab`
- `item`
- `entry`
- `profile`
- `workspace`

instead.

### Immediate Fixes

- rename `x-for="t in tabDefs"` to `x-for="tab in tabDefs"` in operations UI;
- review similar short variable names across templates.

### Acceptance Criteria

- no Alpine template variable shadows translation helpers or globally meaningful names.

## Track J: Frontend Observability and Testability

### Problem

As the UI grows, debugging and regression confidence depend on:

- shared state boundaries;
- consistent render helpers;
- stable e2e selectors;
- predictable logs and request tracing.

### Goal

Make the UI easier to verify and debug.

### Work Items

#### Shared test selectors

Add stable `data-testid` or equivalent hooks in critical areas:

- wizard protocol cards
- secret create/rotate flows
- auth selector
- shell identity

#### Console discipline

- no stray debug logging in production code;
- user-facing failures flow through toasts or error states only.

#### Request ID surfacing

When backend tracing lands, expose request IDs in developer-facing error surfaces where appropriate:

- test-run result panel;
- logs detail drawer;
- adapter failure toasts where safe.

### Acceptance Criteria

- critical UI flows have stable automation hooks;
- frontend failures are easier to trace to backend request IDs once that backend work lands.

## Delivery Order

Recommended order:

1. `feat/frontend-refactoring-plan`
2. `feat/frontend-xss-hardening`
3. `feat/frontend-shell-unification`
4. `feat/frontend-template-safety-cleanup`
5. `feat/frontend-wizard-modularization`
6. `feat/frontend-build-pipeline`
7. `feat/frontend-css-state-cleanup`
8. `feat/frontend-performance-polish`
9. `feat/frontend-observability-and-testability`

## Per-Slice Verification

Each slice must run the smallest realistic verification set for the affected area.

Baseline:

- `node --check` for touched JS files
- UI Docker build
- relevant Playwright specs where available

Additional rules:

- XSS hardening slices must include targeted regression checks for renderer behavior;
- shell unification slices must include login/logout/workspace switch smoke;
- wizard modularization slices must preserve existing wizard e2e coverage;
- build pipeline slices must validate production asset serving in Docker.

## Success Definition

The frontend refactoring track is complete when:

- untrusted API data is no longer rendered through unsafe HTML paths;
- shell identity logic has one owner;
- the wizard no longer lives in a single giant file;
- page scripts follow a consistent module pattern;
- inline event handler injection is removed;
- the UI ships through a lightweight build pipeline;
- common UI states are driven by CSS classes instead of imperative style strings;
- obvious repeated render-time computations are cleaned up.

At that point, the frontend remains Alpine-based but becomes significantly safer and easier to evolve.