Complexity refactor plan #35

New issue

Open

opened 2026-03-13 07:25:07 +03:00 by skobkin · 0 comments

skobkin commented

2026-03-13 07:25:07 +03:00

Owner

Complexity Refactor Proposals

Use these only if we deliberately want to reduce frontend control-flow complexity before enabling the ESLint complexity rule. Work each step independently and stop to commit after each completed step.

Why this section exists

A trial run of complexity at 12 currently reports these hotspots:
- src/App.tsx
  - readHashMapView
  - App
  - bootstrap async effect callback
- src/api/ws.ts
  - ws.onmessage
- src/maps/leafletMap.ts
  - LeafletMapAdapter.render
- src/pages/NodesPage.tsx
  - detailSections
Not all findings are equally valuable.
The best refactoring payoff is in central flow-control code such as app bootstrap, WebSocket event dispatch, and map rendering lifecycle.
Some hits are mostly metric pressure rather than design problems, especially detailSections and readHashMapView.

Step 1: Extract `App` bootstrap orchestration

Move bootstrap fetch/result handling out of App
- Target:
  - src/App.tsx
- Direction:
  - Extract the initial Promise.allSettled(...) bootstrap flow into a dedicated helper or hook.
  - Keep App responsible for wiring state setters and rendering only.
  - Return explicit bootstrap result data instead of mutating many things inline.
- Expected impact:
  - High value.
  - Reduces the biggest real complexity hotspot in the top-level component.
  - Moderate code movement with some regression risk around degraded-mode behavior and initial channel selection.
- Suggested verification:
  - cd web && npm run typecheck
  - cd web && npm run lint
  - cd web && npm run test

Step 2: Split log loading flow from `App`

Extract log snapshot/loading logic into a focused hook
- Target:
  - src/App.tsx
- Direction:
  - Move request key generation, active request tracking, initial log fetch, and “load more” behavior behind a useLogLoader-style hook.
  - Keep store updates and request cancellation semantics unchanged.
- Expected impact:
  - Medium to high value.
  - Lowers App complexity further without changing page-level behavior.
  - More isolated than Step 1, so it is a good standalone refactor.
- Suggested verification:
  - cd web && npm run typecheck
  - cd web && npm run lint
  - cd web && npm run test

Step 3: Replace WebSocket message branching with typed dispatch

Refactor ws.onmessage into explicit event handlers
- Target:
  - src/api/ws.ts
- Direction:
  - Parse the envelope once.
  - Route by event type through a handler table or small typed handler functions.
  - Keep runtime guards for payload validation per event type.
- Expected impact:
  - High value.
  - Improves readability in a central live-update path.
  - Good candidate for focused regression tests around accepted/ignored events.
- Suggested verification:
  - cd web && npm run typecheck
  - cd web && npm run lint
  - cd web && npm run test

Step 4: Decompose Leaflet marker/circle reconciliation

Split LeafletMapAdapter.render into smaller reconciliation helpers
- Target:
  - src/maps/leafletMap.ts
- Direction:
  - Separate marker popup content building, marker upsert, precision-circle upsert, and stale-layer cleanup into dedicated methods.
  - Preserve current popup-open/popup-close side effects exactly.
- Expected impact:
  - Medium value.
  - Real readability gain in a stateful renderer, but higher regression risk than most lint-driven refactors.
  - Worth doing only with careful testing or manual verification of selection/focus behavior.
- Suggested verification:
  - cd web && npm run typecheck
  - cd web && npm run lint
  - cd web && npm run test
  - cd web && npm run build

Step 5: Convert node detail section assembly to data-driven helpers

Break detailSections into per-section builders
- Target:
  - src/pages/NodesPage.tsx
- Direction:
  - Extract section-specific row builders such as identity/connectivity/position/telemetry helpers.
  - Keep output shape identical.
- Expected impact:
  - Low to medium value.
  - Helps the metric, but this is mostly organization rather than a meaningful design fix.
  - Safe as isolated cleanup if the file is already being edited.
- Suggested verification:
  - cd web && npm run typecheck
  - cd web && npm run lint
  - cd web && npm run test

Step 6: Normalize URL hash parsing helpers only if already touching map-view code

Optionally simplify readHashMapView
- Target:
  - src/App.tsx
- Direction:
  - Consider a small helper for bounded numeric parameter parsing if this area is already being refactored.
  - Do not force extraction just to satisfy the metric.
- Expected impact:
  - Low value.
  - This is mostly metric-driven cleanup and should stay optional.
- Suggested verification:
  - cd web && npm run typecheck
  - cd web && npm run lint

Enabling Guidance After Refactors

Re-trial complexity after Steps 1-4
- Direction:
  - Re-run ESLint with complexity at 15 first, then 12.
  - Check whether remaining findings are concentrated in genuinely hard-to-read flow rather than declarative builders.
- Decision rule:
  - Enable as warn first if the remaining reports are low-count and high-signal.
  - Enable as error only if the remaining failures are rare and clearly actionable.
Step 6: Trial useful but more opinionated consistency rules
- Rules:
  - @typescript-eslint/consistent-indexed-object-style
- Expected impact:
  - Small to medium type-only churn.
  - Mostly syntax normalization between Record<K, V> and index signatures.
- Current recommendation:
  - Reasonable next step if we want stronger type-shape consistency.
- Stop after completion and commit.
Step 7: Trial potentially good but higher-churn policy rules
Step 7: Trial potentially good but higher-churn policy rules
- Rules:
  - import-x/no-default-export
  - size/complexity caps such as complexity or max-lines-per-function
  - selected eslint-plugin-unicorn rules if we decide to add that plugin
- Expected impact:
  - Medium to high churn and broader style pressure.
- Current recommendation:
  - Postpone until we explicitly want stronger architectural/style constraints rather than incremental readability wins.
- Notes:
  - import-x/no-default-export is only sensible if we want to move away from default-exported pages/components.
  - Complexity/size rules need careful thresholds or they become low-signal.
  - eslint-plugin-unicorn would be a separate plugin-adoption decision.
- Decision:
  - Enable import-x/no-default-export.
  - Current web/src has no default exports, so this adds policy without code churn.
  - Keep the rule disabled for tool config files such as vite.config.ts, where the default export is part of the host tool's expected shape.
  - Do not enable complexity yet.
    - Trial at 12 reports 4 errors in central files:
      - src/App.tsx
      - src/api/ws.ts
      - src/pages/NodesPage.tsx
    - These are real pressure points, but enabling the rule now would force structural refactors rather than low-risk cleanup.
  - Do not enable max-lines-per-function yet.
    - Trial at 80 reports 4 errors, including:
      - src/App.tsx
      - src/pages/MapPage.tsx
      - src/App.test.tsx
    - This is broader style pressure than we want in the current pass.
  - Do not add eslint-plugin-unicorn in this step.
    - It is not currently installed in web/package.json, so that would be a separate dependency/policy change.
- Stop after completion and commit.

## Complexity Refactor Proposals Use these only if we deliberately want to reduce frontend control-flow complexity before enabling the ESLint `complexity` rule. Work each step independently and stop to commit after each completed step. ### Why this section exists - A trial run of `complexity` at `12` currently reports these hotspots: - `src/App.tsx` - `readHashMapView` - `App` - bootstrap async effect callback - `src/api/ws.ts` - `ws.onmessage` - `src/maps/leafletMap.ts` - `LeafletMapAdapter.render` - `src/pages/NodesPage.tsx` - `detailSections` - Not all findings are equally valuable. - The best refactoring payoff is in central flow-control code such as app bootstrap, WebSocket event dispatch, and map rendering lifecycle. - Some hits are mostly metric pressure rather than design problems, especially `detailSections` and `readHashMapView`. ### Step 1: Extract `App` bootstrap orchestration - [ ] Move bootstrap fetch/result handling out of `App` - Target: - `src/App.tsx` - Direction: - Extract the initial `Promise.allSettled(...)` bootstrap flow into a dedicated helper or hook. - Keep `App` responsible for wiring state setters and rendering only. - Return explicit bootstrap result data instead of mutating many things inline. - Expected impact: - High value. - Reduces the biggest real complexity hotspot in the top-level component. - Moderate code movement with some regression risk around degraded-mode behavior and initial channel selection. - Suggested verification: - `cd web && npm run typecheck` - `cd web && npm run lint` - `cd web && npm run test` ### Step 2: Split log loading flow from `App` - [ ] Extract log snapshot/loading logic into a focused hook - Target: - `src/App.tsx` - Direction: - Move request key generation, active request tracking, initial log fetch, and “load more” behavior behind a `useLogLoader`-style hook. - Keep store updates and request cancellation semantics unchanged. - Expected impact: - Medium to high value. - Lowers `App` complexity further without changing page-level behavior. - More isolated than Step 1, so it is a good standalone refactor. - Suggested verification: - `cd web && npm run typecheck` - `cd web && npm run lint` - `cd web && npm run test` ### Step 3: Replace WebSocket message branching with typed dispatch - [ ] Refactor `ws.onmessage` into explicit event handlers - Target: - `src/api/ws.ts` - Direction: - Parse the envelope once. - Route by event type through a handler table or small typed handler functions. - Keep runtime guards for payload validation per event type. - Expected impact: - High value. - Improves readability in a central live-update path. - Good candidate for focused regression tests around accepted/ignored events. - Suggested verification: - `cd web && npm run typecheck` - `cd web && npm run lint` - `cd web && npm run test` ### Step 4: Decompose Leaflet marker/circle reconciliation - [ ] Split `LeafletMapAdapter.render` into smaller reconciliation helpers - Target: - `src/maps/leafletMap.ts` - Direction: - Separate marker popup content building, marker upsert, precision-circle upsert, and stale-layer cleanup into dedicated methods. - Preserve current popup-open/popup-close side effects exactly. - Expected impact: - Medium value. - Real readability gain in a stateful renderer, but higher regression risk than most lint-driven refactors. - Worth doing only with careful testing or manual verification of selection/focus behavior. - Suggested verification: - `cd web && npm run typecheck` - `cd web && npm run lint` - `cd web && npm run test` - `cd web && npm run build` ### Step 5: Convert node detail section assembly to data-driven helpers - [ ] Break `detailSections` into per-section builders - Target: - `src/pages/NodesPage.tsx` - Direction: - Extract section-specific row builders such as identity/connectivity/position/telemetry helpers. - Keep output shape identical. - Expected impact: - Low to medium value. - Helps the metric, but this is mostly organization rather than a meaningful design fix. - Safe as isolated cleanup if the file is already being edited. - Suggested verification: - `cd web && npm run typecheck` - `cd web && npm run lint` - `cd web && npm run test` ### Step 6: Normalize URL hash parsing helpers only if already touching map-view code - [ ] Optionally simplify `readHashMapView` - Target: - `src/App.tsx` - Direction: - Consider a small helper for bounded numeric parameter parsing if this area is already being refactored. - Do not force extraction just to satisfy the metric. - Expected impact: - Low value. - This is mostly metric-driven cleanup and should stay optional. - Suggested verification: - `cd web && npm run typecheck` - `cd web && npm run lint` ### Enabling Guidance After Refactors - [ ] Re-trial `complexity` after Steps 1-4 - Direction: - Re-run ESLint with `complexity` at `15` first, then `12`. - Check whether remaining findings are concentrated in genuinely hard-to-read flow rather than declarative builders. - Decision rule: - Enable as `warn` first if the remaining reports are low-count and high-signal. - Enable as `error` only if the remaining failures are rare and clearly actionable. - [x] Step 6: Trial useful but more opinionated consistency rules - Rules: - `@typescript-eslint/consistent-indexed-object-style` - Expected impact: - Small to medium type-only churn. - Mostly syntax normalization between `Record<K, V>` and index signatures. - Current recommendation: - Reasonable next step if we want stronger type-shape consistency. - Stop after completion and commit. - [ ] Step 7: Trial potentially good but higher-churn policy rules - [x] Step 7: Trial potentially good but higher-churn policy rules - Rules: - `import-x/no-default-export` - size/complexity caps such as `complexity` or `max-lines-per-function` - selected `eslint-plugin-unicorn` rules if we decide to add that plugin - Expected impact: - Medium to high churn and broader style pressure. - Current recommendation: - Postpone until we explicitly want stronger architectural/style constraints rather than incremental readability wins. - Notes: - `import-x/no-default-export` is only sensible if we want to move away from default-exported pages/components. - Complexity/size rules need careful thresholds or they become low-signal. - `eslint-plugin-unicorn` would be a separate plugin-adoption decision. - Decision: - Enable `import-x/no-default-export`. - Current `web/src` has no default exports, so this adds policy without code churn. - Keep the rule disabled for tool config files such as `vite.config.ts`, where the default export is part of the host tool's expected shape. - Do not enable `complexity` yet. - Trial at `12` reports `4` errors in central files: - `src/App.tsx` - `src/api/ws.ts` - `src/pages/NodesPage.tsx` - These are real pressure points, but enabling the rule now would force structural refactors rather than low-risk cleanup. - Do not enable `max-lines-per-function` yet. - Trial at `80` reports `4` errors, including: - `src/App.tsx` - `src/pages/MapPage.tsx` - `src/App.test.tsx` - This is broader style pressure than we want in the current pass. - Do not add `eslint-plugin-unicorn` in this step. - It is not currently installed in `web/package.json`, so that would be a separate dependency/policy change. - Stop after completion and commit.

skobkin self-assigned this

2026-03-13 07:25:07 +03:00