refactor: convention alignment round 2 — canonical defaults, helpers, dead exports, tests (#1956)
* refactor: convention alignment round 2 — canonical defaults, helpers, dead exports, tests
Follow-up to #1950/#1952/#1953/#1954. Post-audit factorization; behavior-preserving.
Canonical defaults promoted to Constants.ts:
- UNIT_DIVIDER_CENTI/DECI (100/10); adopt UNIT_DIVIDER_KILO at 3 kW/kWh sites
- DEFAULT_RECONNECT_JITTER_PERCENT, SOC_MAXIMUM_PERCENT
- DEFAULT_AUTH_CACHE_MAX_ABSOLUTE_LIFETIME_MS
- DEFAULT_REMOTE_AUTH_CACHE_TTL_SECONDS
- MS_PER_DAY, SECONDS_PER_DAY (align with existing MS_PER_HOUR)
Canonical defaults promoted to OCPP20Constants.ts (OCPP-variable-read fallbacks):
- DEFAULT_RETRY_BACKOFF_{WAIT_MINIMUM,RANDOM_RANGE}_SECONDS
- DEFAULT_RETRY_BACKOFF_REPEAT_TIMES
- DEFAULT_CERT_SIGNING_WAIT_MINIMUM_SECONDS
Module-scope constants:
- WS_DEFLATE_{CONCURRENCY_LIMIT,SERVER_MAX_WINDOW_BITS,ZLIB_*} in UIWebSocketServer
- STATISTICS_PERCENTILE in PerformanceStatistics
- AUTH_CACHE_LIFETIME_{MIN,MAX}_SECONDS, AUTH_CACHE_MIN_ENTRIES,
AUTH_TIMEOUT_{MIN,MAX}_SECONDS in ConfigValidator
- JITTER_PERCENT local to src/worker/ (module standalone; no cross-dep)
- SIGINT/SIGTERM_EXIT_CODE, MISSING_VALUE_PLACEHOLDER,
TEMPLATE_NAME_SUFFIX, TRUNCATED_HASH_ID_LENGTH in ui/cli output
- SKIN_ERROR_RELOAD_COUNT_KEY in ui/web core
Helper adoption:
- millisecondsToSeconds (date-fns) at AbstractUIServer / InMemoryAuthCache / test
- isNotEmptyString in TemplateValidation
- WebSocketReadyState enum (ui-common) at CLI wsIcon/renderers and web stationStatus
- OCPP16ChargePointStatus / OCPP20ConnectorStatusEnumType enum keys instead
of string literals in CLI STATUS_ABBREVIATIONS and web CONNECTOR_STATUS_VARIANT
- nonEmptyStringOrUndefined shared helper (ui/web/src/shared/utils) adopted
by 3 composables + skin error extractor
- Extracted OCPPRequestService.logRequestHandlerError() consumed by OCPP16/20
RequestService (byte-identical catch shell)
Barrel/module hygiene:
- Close barrel gap: meter-values/index.ts exposes disposeCoherentSessionRuntime
(JSDoc already advertised it)
- UIServerAccessPolicy imports UI_SERVER_ACCESS_POLICY_DEFAULTS via utils/index
barrel (drop deep import)
- ui/web skins route 8 imports through @/shared/utils/index barrel
- UIServiceWorkerBroadcastChannel type-only imports AbstractUIService via
ui-server/index barrel (safe by type erasure; comment documents the constraint)
Dead barrel exports pruned (0 external consumers):
- src/utils: FieldError, applyConfigurationMigration, coerceConfigurationVersion,
queueMicrotaskErrorThrowing (tests updated to import from source)
- charging-station/meter-values: ChargingCurvePoint
- ui-server/mcp: MCPToolSchema
- ocpp/auth: AuthStats, CacheStats, CertificateAuthProvider, CertificateInfo
- ocpp/2.0/__testable__: SendMessageFn, TestableRequestServiceOptions/Result
re-exports; TestableOCPP20IncomingRequestService downgraded to module-private;
TestableOCPP20VariableManager type re-export dropped
Anchor-comment reasons added on 3 eslint-disable directives (Utils /
StatisticUtils / UIServerFactory).
Test constants:
- Heartbeat interval sites use Constants.DEFAULT_HEARTBEAT_INTERVAL_MS /
secondsToMilliseconds
- New tests/utils/TestNetworkConstants.ts: TEST_SUPERVISION_URL derived from
Constants.DEFAULT_UI_SERVER_{HOST,PORT}; adopted at MessageChannelUtils.test
+ StorageTestHelpers + ConfigurationFixtures
- Rate-limit / mock-timers-tick 60000 literals replaced by minutesToMilliseconds(1)
in UIServerSecurity / OCPP20CertSigningRetryManager / SignedMeterValues tests
Deliberately not applied (documented rationale):
- src/worker/WorkerAbstract isNotEmptyString adoption: src/worker/ is a
standalone module with zero cross-module dependencies; retracted rather
than adding an import to ../utils
- AbstractUIService → broadcast-channel/index value-import via barrel:
empirically amplifies circular dependencies 62 → 71 because the barrel also
re-exports ChargingStationWorkerBroadcastChannel whose transitive graph
closes a runtime cycle back through ui-server/index; deep import kept
Quality gates:
- pnpm typecheck: clean (root + ui-common + cli + web)
- pnpm lint: 0 errors (1 pre-existing cspell warning unchanged)
- pnpm circular-deps: 62 (baseline unchanged)
- pnpm test: 2955/2961 pass (6 skipped pre-existing; 0 fail)
* [autofix.ci] apply automated fixes
* refactor: address self-review findings — DRY constant refs, helper adoption, naming
Fixes 9 findings surfaced by fan-out self-review of commit
a25f4ba9.
Behavior-preserving; no public API change.
Findings addressed:
- H1 (DRY, Constants.ts) — `DEFAULT_AUTH_CACHE_MAX_ABSOLUTE_LIFETIME_MS` and
`MS_PER_DAY` both held the literal `86_400_000` (same value, same commit).
Class-static forward-reference is illegal in TypeScript (TS2729:
"Property 'B' is used before its initialization"), so the shared literal is
hoisted to module-scope `DAY_IN_MS` / `DAY_IN_SECONDS` helpers before the
class and referenced from both derived defaults and canonical time-unit
constants. Single source of truth restored.
- H2 (DRY, ConfigValidator.ts) — `AUTH_CACHE_LIFETIME_MAX_SECONDS = 86_400`
duplicated `Constants.SECONDS_PER_DAY = 86_400`. Fixed cross-file by
importing `Constants` (already reachable via `../../../../utils/index.js`)
and referencing `Constants.SECONDS_PER_DAY`.
- M1 (helper adoption, ui/cli format.ts:102) — `fuzzyTime` still used
`diff / 1000` while the same PR adopted `millisecondsToSeconds` from
`date-fns` at 4 other sites. Replaced by `millisecondsToSeconds(diff)`
for consistency.
- L1 (naming coherence, ConfigValidator.ts) — Renamed
`AUTH_CACHE_LIFETIME_{MIN,MAX}_SECONDS` to
`AUTH_CACHE_TTL_{MIN,MAX}_SECONDS` to align with the pre-existing
`DEFAULT_AUTH_CACHE_TTL_SECONDS` naming family. Two words for one concept
eliminated within the auth module.
- L2 (naming coherence, ui/cli output) — Renamed cli
`MISSING_VALUE_PLACEHOLDER` to `EMPTY_VALUE_PLACEHOLDER` to align with
ui/web's pre-existing `EMPTY_VALUE_PLACEHOLDER`. Values remain
medium-specific (`'–'` cli / `'Ø'` web), only the semantic role name is
shared. 6 sites updated.
- L3 (stranded export, format.ts) — `TRUNCATED_HASH_ID_LENGTH` had zero
external consumers (only the in-file default arg on `truncateId`).
Dropped `export` keyword.
- L4 (stranded export, format.ts) — `TEMPLATE_NAME_SUFFIX` had zero external
consumers (only the in-file `stripTemplateSuffix` helper). Dropped
`export` keyword; the helper itself keeps `export` (2 external consumers).
- L5 (documented duplication, WorkerUtils.ts) — JSDoc on `JITTER_PERCENT`
now explicitly cross-references `Constants.DEFAULT_RECONNECT_JITTER_PERCENT`
and documents the maintenance invariant. The two constants must stay
synchronized manually because `src/worker/` is a standalone module (no
cross-module value imports).
- I1 (anchor accuracy, StatisticUtils.ts:52) — The
`no-unnecessary-condition` disable reason is reworded from a plausible-
but-imprecise "JSON.parse violation" framing to the actual root cause:
`baseIndex+1` can equal `sortedDataSet.length`, and TypeScript indexes
as `number` (no `noUncheckedIndexedAccess`), so the runtime `!= null`
guard is genuinely needed for out-of-bounds protection.
Empirical retraction verifications:
- B1-5 (cycle amplification 62 → 71) — Re-verified by applying only the
retracted change on top of `
daad8a9d` and running `pnpm circular-deps`:
62 baseline, 71 after B1-5, 62 after revert. Retraction rationale holds.
- A1-17 (`src/worker/` standalone) — Re-verified by `grep -rE
"^import.*\.\./" src/worker/`: zero cross-module value imports; the new
L5 JSDoc documents the exception rationale in-source.
Quality gates:
- pnpm typecheck: clean (root + ui-common + cli + web)
- pnpm lint: 0 errors (1 pre-existing cspell warning unchanged)
- pnpm circular-deps: 62 (baseline unchanged)
- pnpm test: 2955/2961 pass (6 skipped pre-existing; 0 fail)
* docs: fix JSDoc accuracy from review v3 findings (G1, A1, A2)
Doc-only follow-up to commit
6ba8503c. Behavior unchanged.
- G1 (JITTER doc accuracy) — Reworded both `Constants.DEFAULT_RECONNECT_JITTER_PERCENT`
and worker-local `JITTER_PERCENT` JSDoc from the empirically false claim
"symmetric ±20 %" to "bounds the jitter within ±20 %". The `randomizeDelay`
algorithm couples sign and magnitude through a single uniform draw, so with
`random ∈ [0, 1)` and `sign = random < 0.5 ? -1 : +1` the output range is
`(0.9·delay, delay] ∪ [1.1·delay, 1.2·delay)` — asymmetric, with a gap at
`[delay, 1.1·delay)`. The new wording accurately caps the magnitude without
overclaiming symmetry; the WorkerUtils docstring adds an explicit distribution
note pointing to `randomizeDelay`. Algorithm itself pre-dates the PR and is
behavior-preserved.
- A1 (InMemoryAuthCache @param defaults) — Three JSDoc `@param default:` fields
were pointing at raw literals (3600,
86400000, 60000) instead of the promoted
Constants members. Aligned with the pattern already used for `maxEntries`
(which cites `Constants.DEFAULT_AUTH_CACHE_MAX_ENTRIES`): now all six
`@param default:` values reference the canonical `Constants.*` symbol.
- A2 (DEFAULT_REMOTE_AUTH_CACHE_TTL_SECONDS wording) — JSDoc used the phrase
"distinct from local `_TTL_SECONDS = 3600`" where `_TTL_SECONDS` is not an
actual symbol. Replaced by explicit sibling reference
`DEFAULT_AUTH_CACHE_TTL_SECONDS (3600, local cache default)`.
Quality gates:
- pnpm typecheck: clean (root + ui-common + cli + web)
- pnpm lint: 0 errors (1 pre-existing cspell warning unchanged)
- pnpm circular-deps: 62 (baseline unchanged)
* fix: web-test regression from A2-5 + review v4 findings + documented quality gates
Restores web test suite to full green after documented quality gates surfaced
regressions the prior review rounds missed. Behavior-preserving vs the intent
of round-1 refactor.
Regressions fixed (QG-1/QG-2/QG-3 — blockers surfaced by `pnpm --filter web
test:coverage`):
- `stationStatus.ts` — round-1 finding A2-5 replaced `switch (status?.toLowerCase())`
by a Record keyed by `OCPP16ChargePointStatus.*` / `OCPP20ConnectorStatusEnumType.*`
enum values. That change (a) dropped the case-insensitive contract asserted by
`stationStatus.test.ts:74`, and (b) introduced a module-scope runtime access
to `ui-common` enum values, which fails at module init when the containing
test suite mocks `ui-common` (2 file-level FAILs in `useAddStationsForm.test.ts`
and `useStartTxForm.test.ts`).
Design chosen (Option F in review v4 report): keep the Record structure but
key it by lowercase string literals and normalize input via `.toLowerCase()`
on lookup. Retains the refactor benefit (Record > switch for O(1) lookup,
clearer add/remove semantics), restores case-insensitive input, removes the
module-scope enum dependency, and preserves behaviour for the 9 status values
the switch covered. Enum imports dropped since no other value site remains.
Web test suite: 31 files / 532 tests pass (previously 3 files failed / 1
test failed).
Review v4 low-severity findings:
- v4-A1 `Constants.ts:77` — JSDoc "bounds within ±20 %" for
`DEFAULT_RECONNECT_JITTER_PERCENT` implied a bidirectional range, but algebra
of the sole consumer `computeExponentialBackOffDelay` (`jitter = delay ×
jitterPercent × secureRandom()` with `secureRandom() ∈ [0, 1)`) yields
`jitter ∈ [0, +20 %)` strictly non-negative. Reworded to describe the
additive-uniform-draw semantic and cross-reference the two distinct consumer
distributions.
- v4-A2 `WorkerUtils.ts:12` — "shared jitter policy" overclaimed behavioural
parity between the two consumers (positive-only vs asymmetric with a
probability-zero gap). Softened to "shared jitter magnitude cap".
- v4-B1 `ConfigurationFixtures.ts:67` — `uiServer.options` still hardcoded
`host: 'localhost', port: 8080` after the sibling `supervisionUrls` field on
the same object literal was migrated to `TEST_SUPERVISION_URL`. Extended
`tests/utils/TestNetworkConstants.ts` to also export `TEST_UI_SERVER_HOST`
and `TEST_UI_SERVER_PORT` (re-exports of `Constants.DEFAULT_UI_SERVER_HOST` /
`Constants.DEFAULT_UI_SERVER_PORT`).
- v4-B4 `ConfigurationMigrations.test.ts:130,141` — string literals
`'ws://localhost:8080'` in a file the PR already touched (barrel-to-source
import migration) but where the URL literals were skipped in round 2.
Migrated to `TEST_SUPERVISION_URL`.
Lint policy alignment (user directive "aucun warning"):
- `cspell.config.yaml` — added `subclassing` (used in
`src/charging-station/ui-server/index.ts:10`, pre-existing since #1954). Root
lint now reports 0 errors AND 0 warnings.
Deferred (documented in review v4 report, out of this PR's scope):
- v4-B2 `ConfigValidator.ts` MIN/MAX bounds harmonization (already tracked as M2)
- v4-B3 asymmetry between `1.6/__testable__/index.ts` (exports
`TestableOCPP16IncomingRequestService`) and `2.0/__testable__/index.ts`
(interface demoted to non-exported in round 2). Requires either restoring
the 2.0 export or migrating 1.6 consumers to `ReturnType<typeof …>` —
larger touch, kept for a dedicated follow-up.
- Pre-existing gap/asymmetry in `randomizeDelay`'s distribution
(`(−10 %, 0] ∪ [+10 %, +20 %)` — magnitude and sign both derive from a single
uniform draw). Documented in-source, not fixed by this refactor.
Quality gates (documented per `.serena/memories/task_completion_checklist`):
- Root: `pnpm format` / `typecheck` / `lint` (0 err, 0 warn) / `build` / `test`
(2955 pass / 6 skipped / 0 fail)
- CLI: `pnpm format` / `typecheck` / `lint` / `build` / `test` (152/152 pass)
- Web: `pnpm format` / `typecheck` / `lint` / `build` / `test:coverage`
(532/532 pass, 31 test files)
- Circular deps: 62 (baseline unchanged)
* docs: tighten prose in 5 comments — state-describing, not narrative
Removes historical/imperative framing ("kept as", "hoisted to", "if a future
edit converts...", "must not access... because") in favour of concise
statements of what is.
- `Constants.ts:9` module-scope helper: 3→2 lines
- `Constants.ts:80` DEFAULT_RECONNECT_JITTER_PERCENT JSDoc: 6→4 lines
- `WorkerUtils.ts:1` JITTER_PERCENT JSDoc: 9→5 lines
- `UIServiceWorkerBroadcastChannel.ts:1` type-only barrier: 5→3 lines
- `stationStatus.ts:64` CONNECTOR_STATUS_VARIANT: reworded, same length
Net: -9 lines. All invariants preserved (TS2729, cross-module boundary,
runtime-cycle prevention, no-module-scope-enum). Behavior unchanged.
Quality gates:
- Root: format / typecheck / lint (0 err, 0 warn) / build / test (2955 pass)
- CLI: 5/5 clean (152/152 pass)
- Web: 5/5 clean (532/532 pass, test:coverage)
* fix: address review v5 findings — imperative comment, dead exports, module-scope enum
Applies the 3 actionable findings from review round 5. Behavior-preserving.
- v5-A1 `SkinLoadError.vue:28-32` — Reworded `resetToDefault` JSDoc from 4-line
imperative `NOTE: ...should clear the reload-count sessionStorage key...`
to 1-line state-describing `Counter is reset by useSkin.switchSkin on
successful load.` Missed by the round-4 comment tightening sweep.
- v5-B1 `TestNetworkConstants.ts` / `ConfigurationFixtures.ts` — Retired
`TEST_UI_SERVER_HOST` / `TEST_UI_SERVER_PORT` exports (1 external consumer
each, both at the same `ConfigurationFixtures.ts:71` expression — criterion 10
requires ≥2 consumers). Inlined `Constants.DEFAULT_UI_SERVER_HOST` /
`Constants.DEFAULT_UI_SERVER_PORT` at the single call site.
`TEST_SUPERVISION_URL` remains exported (5 consumers).
- v5-B2 `ui/web/src/shared/utils/stationStatus.ts` — Removed the module-scope
value import `import { WebSocketReadyState } from 'ui-common'` and switched
`getWebSocketStateVariant` to 4 module-local numeric constants
`WS_STATE_{CONNECTING,OPEN,CLOSING,CLOSED} = 0/1/2/3`. Eliminates the same
ui-common runtime module-scope dependency that the round-4 fix removed for
`CONNECTOR_STATUS_VARIANT`. No behavior change (values match WHATWG WebSocket
readyState spec).
Quality gates:
- Root: 5/5 clean, 2942/2948 pass
- CLI: 5/5 clean, 152/152 pass
- Web: 5/5 clean, 532/532 pass (test:coverage)
- Circular deps: 62 (baseline)
* docs: drop narrative comments in stationStatus.ts
Removes two comment blocks that narrated rationale ("mirror of...",
"module-local to keep...", "test suites mock...") instead of describing
what is. The `WS_STATE_*` constants and lowercase Record keys are
self-documenting.
- `stationStatus.ts:62-66` — 4-line rationale block removed; `cspell:ignore`
directive retained
- `stationStatus.ts:83-85` — 3-line rationale block removed
Quality gates: root/web 5/5 clean, 532/532 pass.
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>