From 5eac201d88689d878119916670a4670b0968b7eb Mon Sep 17 00:00:00 2001 From: "bert.hausmans" Date: Mon, 4 May 2026 22:06:20 +0200 Subject: [PATCH] =?UTF-8?q?docs(refactor):=20audit=20axios=E2=86=94store?= =?UTF-8?q?=20coupling=20for=20decoupling=20work?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase A of TECH-AXIOS-STORE-COUPLING. Read-only inventory of the four lib/axios.ts → stores/ touchpoints (lines 3, 5, 63, 75 carry per-line boundary disables), plugin load-ordering analysis, test coverage matrix, consumer audit (30 importers, all using the `apiClient` named export), and Vite mixed-import warning confirmation. Surfaces four open questions for Phase B sign-off: Q1 callback-injection vs event-bus → recommends callback-injection Q2 location of `Deps` type → recommends inside lib/axios.ts Q3 test scope for this session → recommends defer to backlog Q4 plugin filename → recommends 3.axios-bindings.ts No code changed. No BACKLOG.md edit. Awaiting Bert's Phase B sign-off before implementing Phase C. Co-Authored-By: Claude --- dev-docs/TECH-AXIOS-STORE-COUPLING-AUDIT.md | 367 ++++++++++++++++++++ 1 file changed, 367 insertions(+) create mode 100644 dev-docs/TECH-AXIOS-STORE-COUPLING-AUDIT.md diff --git a/dev-docs/TECH-AXIOS-STORE-COUPLING-AUDIT.md b/dev-docs/TECH-AXIOS-STORE-COUPLING-AUDIT.md new file mode 100644 index 00000000..b0d05050 --- /dev/null +++ b/dev-docs/TECH-AXIOS-STORE-COUPLING-AUDIT.md @@ -0,0 +1,367 @@ +# TECH-AXIOS-STORE-COUPLING — Phase A audit + +**Date:** 2026-05-04 +**Branch:** `refactor/axios-store-coupling` +**Scope:** read-only audit. No code changed in this phase. +**Goal:** make `apps/app/src/lib/axios.ts` a pure HTTP module by routing +its four `stores/` references through a callback seam wired up from a +new `plugins/` plugin. Phase A inventories what exists today; Phase B +asks Bert to resolve four open questions; Phase C implements. + +--- + +## A.1 — Touchpoint inventory in `apps/app/src/lib/axios.ts` + +The file is 109 lines on `main` at `831f36e`. The four boundary-rule +disables are at lines **3, 5, 63, 75** (single-line drift versus the +BACKLOG note of "regels 3-4, 61, 72" — caused by minor formatting +changes since 1c, semantically identical). + +### Touchpoint 1 — `useOrganisationStore` (static) + +| Field | Value | +| --- | --- | +| Disable line | 3 | +| Import line | 4 (`import { useOrganisationStore } from '@/stores/useOrganisationStore'`) | +| Invocation line | 20 (`const orgStore = useOrganisationStore()`) | +| Interceptor | request | +| Access | read state — `orgStore.activeOrganisationId` (string \| null) | +| Semantic purpose | inject the active organisation ULID into the `X-Organisation-Id` request header so the backend can scope the query to the active tenant | +| Shape needed in `Deps` | `getActiveOrgId: () => string \| null` | + +### Touchpoint 2 — `useNotificationStore` (static) + +| Field | Value | +| --- | --- | +| Disable line | 5 | +| Import line | 6 (`import { useNotificationStore } from '@/stores/useNotificationStore'`) | +| Invocation line | 59 (`const notificationStore = useNotificationStore()`) | +| Interceptor | response (error path) | +| Access | calls `.show(message, level)` action | +| Semantic purpose | toast a message via `v-snackbar` for 403 / 404 / 422 / 503 / 5xx / network errors | +| Shape needed in `Deps` | `notify: (message: string, level: 'error' \| 'warning') => void` (the only two levels used in the current code) | + +### Touchpoint 3 — `useImpersonationStore` (dynamic) + +| Field | Value | +| --- | --- | +| Disable line | 63 | +| Import line | 64 (`const { useImpersonationStore } = await import('@/stores/useImpersonationStore')`) | +| Invocation line | 65 (`const impersonationStore = useImpersonationStore()`) | +| Interceptor | response (error path), under `status === 403 && error.response?.data?.impersonation_ended` | +| Access | calls `.clearState()` then `window.location.href = '/platform'` | +| Semantic purpose | when the backend revokes an impersonation session mid-flight (403 with `impersonation_ended`), drop the local impersonation state and full-page-redirect back to `/platform` so the original admin session restores | +| Shape needed in `Deps` | `onImpersonationRevoked: () => void` (the redirect is the side-effect of clearing state; whether the callback owns the redirect or just the clear is a Phase B Q2-adjacent question — see notes) | + +### Touchpoint 4 — `useAuthStore` (dynamic) + +| Field | Value | +| --- | --- | +| Disable line | 75 | +| Import line | 76 (`const { useAuthStore } = await import('@/stores/useAuthStore')`) | +| Invocation line | 77 (`const authStore = useAuthStore()`) | +| Interceptor | response (error path), under `status === 401` | +| Access | reads `.isInitialized`; if true, calls `.handleUnauthorized()` | +| Semantic purpose | on 401, if the auth store has finished its initial `/auth/me` probe, clear local auth state and redirect to `/login`. The `isInitialized` guard exists because the initial `/auth/me` call itself returns 401 for unauthenticated visitors and that is *not* a logout-trigger | +| Shape needed in `Deps` | `onAuthFail: () => void` — the callback closure should encapsulate both the `isInitialized` check and the `handleUnauthorized()` call (see Phase B Q2). Alternatively, two callbacks (`isAuthInitialized: () => boolean` + `onAuthFail: () => void`) but that's a leakier seam. | + +### Non-store behavior in `axios.ts` (preserve, do not refactor) + +- **Lines 1–2:** `axios` import + named-type imports. +- **Lines 8–16:** `apiClient` factory — `axios.create({ baseURL, withCredentials, headers, timeout: 30000 })`. Pure config, no store deps. +- **Lines 27–37 (request interceptor):** reads `sessionStorage.getItem('crewli_impersonation')`, parses JSON, sets `X-Impersonate-User` header. **This is *not* a boundary violation** — it touches `sessionStorage` directly, not a store. It is conceptually parallel to touchpoint 1 but already store-free, so it stays as-is in Phase C unless Bert prefers a parallel callback (`getImpersonationTargetId: () => string | null`) for symmetry. Recommendation: leave alone in Phase C — the seam should only encapsulate the store-coupled paths. Refactoring this is scope creep. +- **Line 40, 50, 56:** `console.log/error` under `import.meta.env.DEV` — pure logging. +- **Lines 73, 75–80, 82, 85, 88–95, 97, 99–101:** the rest of the response error mapping — same status-code branching, all toast-via-`notify` once touchpoint 2 is callback-ified. +- **Line 108:** `export { apiClient }` — single named export. **Critical:** 30 importers across `apps/app/src/` consume `import { apiClient } from '@/lib/axios'`. The named export must keep working post-refactor. Phase C only adds a *new* `registerInterceptors` export; nothing is renamed or removed. + +### Singleton vs. factory + +`apiClient` is a top-level singleton (created at module-eval time). The +two interceptor registrations execute as a side-effect of the first +import. Phase C must split this: the singleton stays, but interceptor +registration moves to an explicit `registerInterceptors(client, deps)` +call invoked from the new bindings plugin. Until that call runs, the +client is interceptor-less — fine, because the bindings plugin runs in +the plugin-init phase before any composable or component issues an +HTTP call (Pinia + the bindings plugin both initialize during +`registerPlugins(app)` in `main.ts`, before `app.mount`). + +--- + +## A.2 — Plugins ordering and proposed filename + +### How plugins load + +`main.ts` calls `registerPlugins(app)` from +`@core/utils/plugins.ts`. The helper is a Vuexy convention that uses +`import.meta.glob(['../../plugins/*.{ts,js}', '../../plugins/*/index.{ts,js}'], { eager: true })`, +collects the keys, **alphabetically sorts the path strings**, and calls +each module's `default(app)` in that order. There is no manual +registration in `main.ts` — drop a file in `src/plugins/` and it gets +picked up. + +### Current alphabetical load order + +| Sort key (path string) | File | Role | Pinia ready after this? | +| --- | --- | --- | --- | +| `../../plugins/1.router/index.ts` | `1.router/index.ts` | `app.use(router)` + guards | no | +| `../../plugins/2.pinia.ts` | `2.pinia.ts` | `createPinia()` + `app.use(store)` | **yes (this is where it happens)** | +| `../../plugins/iconify/index.ts` | `iconify/index.ts` | iconify registration | yes | +| `../../plugins/layouts.ts` | `layouts.ts` | `createLayouts()` | yes | +| `../../plugins/vuetify/index.ts` | `vuetify/index.ts` | Vuetify registration | yes | +| `../../plugins/webfontloader.ts` | `webfontloader.ts` | webfont preload | yes | + +Ordering invariant: anything that needs Pinia stores must sort +**after** `2.pinia.ts`. The numeric prefix is the established convention +for explicit ordering (router=1, pinia=2). Anything without a numeric +prefix sorts after the numeric ones because digits sort before letters +in standard lexicographic comparison. + +### Proposed filename + +**`apps/app/src/plugins/3.axios-bindings.ts`** + +Rationale: +- `3.` makes the post-Pinia ordering explicit and human-readable + alongside the existing `1.router` / `2.pinia` numeric prefixes. + Future readers reading top-to-bottom in a directory listing see + the load sequence at a glance. +- A single `.ts` file (not a directory + `index.ts`) — the wiring is + ~30 lines of closure plumbing, no internal modules to split. +- Name `axios-bindings` matches the BACKLOG entry's wording. "Bindings" + reads as "bind axios interceptors to store callbacks" — the file's + job is exactly that wiring. + +If Bert prefers a non-numeric name (e.g. just `axios-bindings.ts`, +sorting after the numeric files), that works too — Pinia is still +already up. Numeric-prefix is recommended for parity with the existing +router/pinia ordering convention. **Open as Phase B Q4.** + +--- + +## A.3 — Test coverage matrix for the four acceptance scenarios + +`apps/app` runs Vitest against 49 tests across 10 files (sanity smoke +in `tests/`, plus `__tests__/` colocated under `layouts/`, +`composables/api/`, and `components/form-failures/`). + +| Acceptance scenario | Existing test? | Notes | +| --- | --- | --- | +| 1. `X-Organisation-Id` header injection | **No** | No test exercises the request interceptor. | +| 2. 401 → `handleUnauthorized()` flow | **No** | `useAuthStore` has no colocated tests; no interceptor test. | +| 3. 403 + `impersonation_ended` revocation | **No** | `useImpersonationStore` has no colocated tests; no interceptor test. | +| 4. 4xx/5xx error toast via `useNotificationStore` | **No** | `useNotificationStore` has no colocated tests; no interceptor test. | + +### How `@/lib/axios` is used in existing tests + +All 6 existing axios-touching test files use `vi.mock('@/lib/axios', () => ({ apiClient: { ... } }))` to swap the entire module out. None of them load real interceptors. List: + +- `src/composables/api/__tests__/useFormFailures.spec.ts` +- `src/components/form-failures/__tests__/FormFailuresTable.spec.ts` +- `src/components/form-failures/__tests__/DismissFailureDialog.spec.ts` +- `src/components/form-failures/__tests__/FormFailureDetail.spec.ts` +- `src/components/form-failures/__tests__/RetryFailureDialog.spec.ts` +- `src/components/form-failures/__tests__/ResolveFailureDialog.spec.ts` + +These tests will keep passing post-refactor regardless of how +`registerInterceptors` looks — they replace the module wholesale. + +### Stores tested directly? + +No — `apps/app/src/stores/__tests__/` does not exist. None of the four +stores referenced from `axios.ts` has a unit test. The Vitest harness +(`tests/sanity.spec.ts`) was just landed in WS-6 sessie 3b +(TECH-APP-VITEST closure); the colocated `__tests__/` folders only +cover layouts, form-failures components, and `useFormFailures`. + +### Recommendation for Phase B Q3 + +Default to **option C from the brief: add as follow-up backlog entry +`TECH-AXIOS-INTERCEPTOR-TESTS`**, do not add tests in this session. + +Reasoning: +- All 4 acceptance scenarios are untested today, so the refactor is + not regressing existing coverage. +- Building an interceptor-test fixture that loads the real + `apiClient`, registers `registerInterceptors` with mocked deps, + and asserts on `axios-mock-adapter` (or `nock`-style) intercepted + requests is genuinely >2h of infrastructure work in a project + that has only had Vitest for a few sessies. The brief's default + scope rule applies. +- The 6 module-mocked tests give us a regression net for "the + `apiClient` named export still works": if any `.get`/`.post` chain + breaks, those component tests fall over. Combined with manual + smoke-testing the four flows in dev, that's adequate for this + session. + +If Bert overrules this and wants tests in-session, the smallest +viable path is option B targeted on **just touchpoint 1** (`X-Organisation-Id` +header injection) — it's the cheapest to test (request-time, no +mocked window navigation) and the most boring to verify by hand, +making it the highest-value test-per-effort. Touchpoints 2–4 +involve `window.location.href` mutations that are awkward in +Vitest+happy-dom. + +--- + +## A.4 — Consumer audit for `@/lib/axios` + +```bash +grep -rln "from '@/lib/axios'" apps/app/src/ | wc -l # 30 +grep -rln "vi.mock('@/lib/axios'" apps/app/src/ | wc -l # 6 +grep -rln "import('@/lib/axios')" apps/app/src/ | wc -l # 1 (AccountTab.vue, dynamic) +``` + +### Group 1 — composables/api (22 files) + +All 22 files in `src/composables/api/use*.ts` import `{ apiClient }` and +use `.get` / `.post` / `.patch` / `.put` / `.delete`. Vanilla axios usage. + +### Group 2 — stores (2 files) + +`src/stores/useAuthStore.ts` and `src/stores/useImpersonationStore.ts` +both import `{ apiClient }`. (Other stores don't talk to axios.) +**Important:** these are the two stores that are ALSO referenced from +`axios.ts` via dynamic import — that's the cycle the Vite warnings +flag. Removing those dynamic imports from `axios.ts` resolves the +cycle automatically. + +### Group 3 — components / pages (6 files) + +- `components/platform/ImpersonateDialog.vue` +- `components/account-settings/AccountTab.vue` (dynamic import) +- `components/common/ImageUploadField.vue` +- `pages/login.vue` +- `pages/forgot-password.vue` +- `pages/reset-password.vue` +- `pages/verify-email-change.vue` + +These violate the project rule "never import axios directly in a component (use a composable)" — that's a separate cleanup +not in this session's scope. Listing for completeness. + +### Group 4 — tests that mock the module (6 files) + +Listed in A.3. They use `vi.mock` and replace `apiClient` with a stub; +they don't care about the interceptor wiring. + +### Breaking-change surface + +**Zero.** Every consumer reads `{ apiClient }` from +`@/lib/axios`. Phase C only: +- Adds a new export `registerInterceptors`. +- Adds a new export `Deps` type (or hosts it in `@/types/` per Phase B Q2). + +Existing imports keep working unchanged. No fanout. + +--- + +## A.5 — Vite mixed-import warning confirmation + +Ran `pnpm build` from `apps/app/` on `main`@`831f36e`. Both warnings +reproduce as the BACKLOG describes. The relevant excerpt: + +``` +[plugin vite:reporter] +(!) /…/apps/app/src/stores/useAuthStore.ts is dynamically imported by + /…/apps/app/src/lib/axios.ts but also statically imported by + /…/apps/app/src/App.vue, /…/AccountTab.vue, /…/SecurityTab.vue, + /…/EventMetricCards.vue, /…/EventTabsNav.vue, + /…/OrganisationSwitcher.vue, … (≈30 callers total), + dynamic import will not move module into another chunk. + +[plugin vite:reporter] +(!) /…/apps/app/src/stores/useImpersonationStore.ts is dynamically + imported by /…/apps/app/src/lib/axios.ts but also statically + imported by /…/apps/app/src/App.vue, /…/ImpersonateDialog.vue, + /…/ImpersonationBanner.vue, /…/DefaultLayoutWithVerticalNav.vue, + dynamic import will not move module into another chunk. +``` + +Build still succeeds (10.96s, 49 tests pass). The warnings are +diagnostic only, but they are an explicit acceptance criterion for +Phase C: removing the two `await import('@/stores/...')` calls in +`axios.ts` should drop both warnings to zero. If either warning +persists post-refactor, it means another file in the codebase is +also doing `await import('@/stores/useAuthStore')` or +`'.../useImpersonationStore'`. Quick `grep -rn "await import.*useAuthStore\|await import.*useImpersonationStore" apps/app/src/` +during Phase C will confirm; expectation is that `axios.ts` is the +only offender. + +--- + +## A.6 — Open questions for Bert (Phase B sign-off) + +### Q1. Approach 1 (callback-injection) or Approach 2 (event-bus)? + +**Recommendation: Approach 1 (callback-injection).** + +The four touchpoints map cleanly to four callbacks (or three + +one read), all single-subscriber, all synchronous in their +domain (the heavy work is the callee's, not the seam's). No +case where multiple subscribers would want to react to the same +event. No case where the subscriber needs to register/unregister +dynamically at runtime. An event bus would add a second +indirection, a stringly-typed event name surface, and break +TypeScript's ability to enforce that every event has exactly the +deps it needs. + +Approach 2 would only beat Approach 1 if we wanted the bindings +plugin to subscribe to events emitted from places other than +`axios.ts`. We don't. + +### Q2. Where should the typed `Deps` interface live? + +**Recommendation: inside `lib/axios.ts`, exported.** + +Rationale: +- The `Deps` shape is the *contract of `lib/axios.ts`*. It belongs + to the module that consumes it. The bindings plugin imports it + to know what to provide; that's a normal `import type` flow and + doesn't break the boundaries matrix (`plugins → lib` is allowed). +- Hoisting it to `src/types/` would be more "architecturally pure" + but adds a hop with no payoff: nothing else in the codebase + consumes this contract, and `types/` is for shared + domain/value-object types, not for module-internal interfaces. +- The architectural test that matters is "does `lib/axios.ts` + import from `@/stores/...`?" — that's what the boundaries rule + enforces. Where the type lives doesn't affect that. + +If Bert prefers the `types/`-hoist for consistency, no objection; +the diff cost is one extra file. State preference in Phase B. + +### Q3. Test scope for this session. + +**Recommendation: option C — add follow-up backlog entry +`TECH-AXIOS-INTERCEPTOR-TESTS`, no new tests in this session.** + +Reasoning in A.3. All four scenarios are untested today; the refactor +preserves behavior 1:1; the 6 existing module-mock tests are an +adequate "named-export still works" net; building real +interceptor-fixture infrastructure is >2h. + +If Bert prefers option B (add tests for touchpoint 1 only — the +`X-Organisation-Id` header injection — as the cheapest meaningful +test), that's a reasonable middle ground; estimate ~1h. + +### Q4. Confirm plugin filename and ordering. + +**Recommendation: `apps/app/src/plugins/3.axios-bindings.ts`.** + +Numeric prefix `3.` for parity with `1.router/` and `2.pinia.ts`, +making the post-Pinia load order explicit. Alternative: drop the +prefix (`axios-bindings.ts`) — it would still sort after the +numeric files because digits precede letters lexicographically. +Numeric is preferred for readability. + +--- + +## A.7 — Deliverable + +This file is `dev-docs/TECH-AXIOS-STORE-COUPLING-AUDIT.md`. Phase A +ends here. **No code edits, no `BACKLOG.md` change, no plugin file, +no `axios.ts` change yet.** Awaiting Phase B sign-off on Q1–Q4. + +Branch: `refactor/axios-store-coupling`. + +Phase A commit message: +`docs(refactor): audit axios↔store coupling for decoupling work`.