tech-debt(frontend): de-duplicate cross-app FE plumbing + Advanced Control/useVncControls built twice #12653

Activity

1. mrveiss

   added

   refactor

   priority: medium

   frontend

   tech-debt

   on Jul 26, 2026
2. mrveiss commented on Jul 26, 2026

   

   mrveiss

   on Jul 26, 2026

   OwnerAuthor

   More actions

   Folding in #12655 (filed before I saw this umbrella). Concrete evidence for the ApiClient cross-app fork part of this child: 3 hand-written fetch clients share ~300-380 near-identical transport lines — autobot-frontend/src/utils/ApiClient.ts (842L, ~300 transport), autobot-slm-frontend/src/utils/ApiClient.ts (419L, ~380, its docstring already defers a shared core to 'a later ADR'), and autobot-frontend/src/utils/slmClient.ts (163L, from #12614/#12644, currently HELD). Shared: rawRequest (AbortController timeout + signal-forward + FormData + auth inject + 401 hook + base-URL join), get (retry+backoff, no-retry-4xx), post/put/patch/delete, extractErrorInfo, parseJsonBody, uploadFile (XHR — identical between the two ApiClients). Canonical target: a createHttpClient(config) factory parameterized on the 5 per-app axes (baseUrl resolver, token source, 401 handler, auth-endpoint skip predicate, extra headers incl X-Organization-Id). Per this umbrella's 'FE — ADR first' order, needs the shared-package/path-alias decision (no @autobot/* workspace exists yet). Note: the intra-app SLM adapter dedup (a related but smaller cluster) is already DONE — see #12654/#12664.
3. mrveiss commented on Jul 26, 2026

   

   mrveiss

   on Jul 26, 2026

   OwnerAuthor

   More actions

   Note: #12644 (the main-frontend slmClient bridge for getSLMUrl callers) is now MERGED — it's a minimal scoped bridge that fixes a real gap (useHostInventory/useTLSCredentials were on raw fetch). It becomes the 3rd client this ADR must fold into a shared createHttpClient core. The consolidation target/count is unchanged — all 3 clients (main ApiClient, slm ApiClient, main slmClient) converge under this child.
4. mrveiss commented on Jul 29, 2026

   

   mrveiss

   on Jul 29, 2026

   OwnerAuthor

   More actions

   Measured the useVncControls half of this issue against the current tip. It is closer to done than the description suggests — and one of the two remaining copies was orphaned by my own change, which I should call out.

   copy	lines	callers
   autobot-plugins/vnc/src/composables/useVncControls.ts (canonical)	110	5 — via @autobot/vnc
   autobot-frontend/src/composables/useVncControls.ts	148	none except its own test
   autobot-slm-frontend/src/composables/useVncControls.ts	203	none of any kind

   What changed: #12931 (PR #12938) made the canonical copy accept an injected transport and migrated DesktopInterface.vue onto it. That removed the main-FE copy's last real consumer — so my change is what left it orphaned, and I did not clean it up at the time. That is the "dead code at close" rule, and it is mine to finish.

   The SLM copy is different: it has no importer of any kind, in any file type. It predates my work and appears never to have been wired at all — consistent with this issue's observation that Advanced Control was "built twice", with one build never reaching a caller.

   So the "exists once" criterion is now mostly a retirement question rather than a migration one: the canonical package already serves every real consumer (5 imports), and what remains is two files nobody calls.

   Not acting unilaterally, because it is a deletion and the repo rule is to wire in rather than delete. The honest framing is that these two have no callers and the canonical one already serves everything — but which of these you want is a real choice:

   1. Retire both — delete the two orphans and the main-FE copy's test. Smallest surface, and matches what the code is actually doing today.
   2. Redirect them — replace each with a one-line re-export of @autobot/vnc, so any straggler import (or an out-of-tree consumer) keeps working while there is one implementation. Safer, leaves two thin files.
   3. Retire the main-FE one only — it is unambiguously my orphan; leave the SLM copy pending the broader Advanced Control decision this issue also covers, since that is where its sibling duplication lives.

   My suggestion is (1) for the main-FE copy — I created that orphan and nothing references it — and (3)'s caution for the SLM one, i.e. handle it with the Advanced Control de-duplication rather than on its own, since both belong to the same never-wired SLM build.

   The remaining half of this issue (the ADR on a shared FE core, and Advanced Control existing once) is untouched and still needs the architecture call.
5. mrveiss

   added 2 commits that reference this issue on Jul 29, 2026

   fix(vnc): export VncRequest and UseVncControlsOptions from the packag…

   ...

   784362a

   refactor(vnc): make useVncControls exist once (#12653) (#12978)

   ...

   2795c98
6. mrveiss commented on Jul 29, 2026

   

   mrveiss

   on Jul 29, 2026

   OwnerAuthor

   More actions

   useVncControls now exists once — PR #12978 merged. Issue stays open for the ADR and Advanced Control.

   351 lines of forked implementation across three copies became 42 lines of re-export. The canonical @autobot/vnc serves all 5 real consumers; both app-local copies had zero callers.

   Re-exported rather than deleted, so any straggler import keeps working — the fork goes, not the callers. Same pattern used today for is_admin_role (#12948) and base_service (#12972).

   Two things worth recording:

   The main-FE copy was my orphan. #12931 migrated DesktopInterface.vue onto the shared package and left the file behind. Dead code I created; cleaned up now rather than reported.

   The shims surfaced a real gap immediately. vue-tsc rejected VncRequest and UseVncControlsOptions — #12931 added the injected-transport seam but exported only the function, not the types describing it, so a consumer could pass a transport but not name its type. Fixed at the package entry (autobot-plugins/vnc/index.ts).

   Verification: 2 tests pass asserting identity (expect(local).toBe(canonical)) rather than behaviour — two equivalent-but-distinct implementations is the fork being removed. vue-tsc clean with @autobot/vnc mapped to the worktree.

   Caveat on local typechecks here: node_modules/@autobot/vnc symlinks to the main tree, so a plain local vue-tsc compiles the unpatched package. That produced a false GREEN during #12931 and a false RED during this change. Any future work on this package needs a paths override to get a meaningful result.

   Remaining on this issue: the ADR on a shared FE core (ApiClient / ssot-config / WS singletons) and de-duplicating Advanced Control — both still need the architecture decision.
7. mrveiss commented on Jul 31, 2026

   

   mrveiss

   on Jul 31, 2026

   OwnerAuthor

   More actions

   Measured duplication (on origin/Dev_new_gui, 2026-07-31)

   All five named items measured on both sides before any change. Three of the five are not what the issue body describes.

   1. useVncControls — already converged, nothing to do

   #12931 gave the shared @autobot/vnc copy an injected transport; #12978 (commit 2795c9850) reduced both app copies to re-export shims:

   • autobot-frontend/src/composables/useVncControls.ts:13-21 → export { … } from '@autobot/vnc'
   • autobot-slm-frontend/src/composables/useVncControls.ts:13-21 → same

   Canonical implementation: autobot-plugins/vnc/src/composables/useVncControls.ts (110 lines). Sole real consumers are autobot-frontend/src/components/desktop/DesktopInterface.vue:213 and autobot-plugins/vnc/src/components/VncToolbar.vue:73, both importing from @autobot/vnc.

   2-4. ApiClient / ssot-config / WS singleton — diverged, both live, not forks

   Concept	main FE	SLM FE	How they diverge
   HTTP client	autobot-frontend/src/utils/ApiClient.ts (842L)	autobot-slm-frontend/src/utils/ApiClient.ts (419L)	main: service-discovery base URL (:191-232), auth-store token with expiry check (isRealAuthToken, :222), X-Organization-Id (:330-331), XHR upload progress (:24). SLM: getSlmApiBase(), sessionStorage['slm_access_token'] w/ localStorage fallback (:36-40), 401 → clear session + redirect /login (:149).
   Config resolver	autobot-frontend/src/config/ssot-config.ts (787L)	autobot-slm-frontend/src/config/ssot-config.ts (217L)	AutoBotConfig (:228-257) = permissions, approvals, VNC, service discovery. SLMConfig (:13-47) = Grafana/Prometheus ports, VM roles, known hosts. Shared exports: getConfig, getBackendUrl. Schemas near-disjoint.
   WS singleton	autobot-frontend/src/services/GlobalWebSocketService.ts (839L)	autobot-slm-frontend/src/composables/useSlmWebSocket.ts (415L)	Different backends and message vocabularies — SLM connects to the SLM backend /api/ws/events with SLMWebSocketMessage (:16,26).

   Both copies of each are live: main-FE ApiClient has 138 importing files, SLM ApiClient ~25. The divergence is entirely in which backend and how it authenticates. Collapsing them means a per-app strategy object for base URL, token source, 401 policy and message schema — shape converges, semantics stay forked.

   Recorded as ADR-008 (docs/adr/008-frontend-shared-code-boundary.md): the boundary is drawn at backend knowledge; shared code takes an injected transport (@autobot/vnc pattern); ApiClient/ssot-config/WS stay per-app; one client per (app, backend) pair.

   5. Advanced Control — genuinely built twice, and the obvious canonical is the dead one

   • A autobot-frontend: AdvancedControlView.vue (1103L) + useAdvancedControl.ts (338L) + AdvancedControlApiClient.ts (458L, all 17 routes incl. emergency-stop, system status/health, WS helpers). Routed at src/router/index.ts:842-853 with hideInNav: true — and nothing in the app links to it. URL-only.
   • B autobot-slm-frontend/src/views/tools/admin/AdvancedControlTool.vue: 12 of 17 routes. Routed at src/router/index.ts:476-482 and in the operator tool nav at src/views/tools/ToolsLayout.vue:26 — the copy operators actually reach.

   Two live consumer sets, so converging is a product decision, not a refactor → split to #13078 for the owner. This issue's done-condition ("Advanced Control exists once") is blocked on that.

   Delivered — PR #13077

   1. ADR-008 merged into docs/adr/, indexed in README.md and _index.md (which also picks up the previously un-indexed ADR-006/007).
   2. Mechanical part of Advanced Control: AdvancedControlTool.vue carried a private fetch helper sending only authStore.token — no autobot_access_token fallback, no 401 cleanup, no timeout — while all ~17 sibling admin tools use useAutobotApi. The endpoints, payload types and trigger/priority enums moved onto useAutobotApi (10 methods) and the tool calls them, per ADR-008 rule 3. Same 12 routes, same payloads, same error-banner wording; path params now encodeURIComponent-escaped.

   Verification: SLM type-check clean, lint 0 errors (16 pre-existing warnings, none in changed files), full suite 202→203 passing with an identical (empty) failing set via a cp-swapped baseline.

   Filed

   • decision(frontend): Advanced Control is live in both SPAs — pick the canonical one (from #12653) #13078 — decision: which Advanced Control survives (blocks closing this issue)
   • tech-debt(slm-frontend): 7 components bypass useAutobotApi with their own transport, losing the autobot_access_token fallback #13079 — 7 more SLM components carry the same transport fork against the autobot backend (OrgChartTab, ProcessMonitorTab, ConfigHistoryTab, RedisServicePanel, UserManagementSettings, CacheSettings, useSkills)

   Remaining on this issue

   Only the Advanced Control convergence, gated on #13078. The ADR is delivered and useVncControls already exists once.
8. mrveiss

   added a commit that references this issue on Jul 31, 2026

   refactor(frontend): ADR-008 shared-code boundary + de-fork SLM Advanc…

   ...

   85fdfc3
9. mrveiss commented on Aug 1, 2026

   

   mrveiss

   on Aug 1, 2026

   OwnerAuthor

   More actions

   Heads-up for whoever picks this up: step 1 of this issue is already done.

   docs/adr/008-frontend-shared-code-boundary.md — "Frontend Shared-Code Boundary Between the Two SPAs", Accepted 2026-07-31 — is the ADR this issue asks for, and it names #12653 directly.

   So the remaining work here is step 2 only: de-duplicate useVncControls and Advanced Control onto one implementation per that ADR, respecting i18n across all 11 locales and staying clear of #12420's in-flight SLM client migration.

   Noticed while auditing #12651 for the same umbrella; recording it so the ADR does not get written twice.
10. mrveiss commented on Aug 1, 2026

    

    mrveiss

    on Aug 1, 2026

    OwnerAuthor

    More actions

    ✅ Closing with proof — this is already done. Verified against the issue's own acceptance criteria rather than assumed; both halves landed in earlier work that cited this issue.

    Gate 1 — acceptance criteria, quoted verbatim

    Done when: ADR merged;

    ✅ Met. docs/adr/008-frontend-shared-code-boundary.md — "Frontend Shared-Code Boundary Between the Two SPAs", Accepted 2026-07-31, and it names #12653 directly.

    The boundary it draws: shared code is anything expressible without naming a backend, a base URL, or a token store, and it takes an injected transport — never a client of its own. Plus rule 3, "one client per (app, backend) pair… no component may open its own transport."

    useVncControls and Advanced Control exist once;

    ✅ Met, for both.

    useVncControls — one implementation in @autobot/vnc (110 lines). Both apps hold 21-line re-export shims, and autobot-frontend/src/composables/__tests__/useVncControls.test.ts asserts the local path resolves to the same object as @autobot/vnc, not a same-named twin. Both apps already declare "@autobot/vnc": "file:../autobot-plugins/vnc". This is exactly the injected-transport pattern ADR-008 rule 1 prescribes.

    Advanced Control — each app reaches it through its ADR-mandated canonical client:

    App	Backend	Canonical client (ADR-008 rule 3)	Advanced Control uses
    autobot-frontend	autobot	src/utils/ApiClient.ts	AdvancedControlApiClient → apiClient.rawRequest
    autobot-slm-frontend	autobot	src/composables/useAutobotApi.ts	useAutobotApi

    AdvancedControlApiClient.ts:204 calls apiClient.rawRequest(...) — a thin typed wrapper over the canonical singleton, not a second transport, so rule 3 holds. SLM's AdvancedControlTool.vue was migrated off its private fetch fork onto useAutobotApi, and its docstring cites #12653; that migration also fixed a real bug — the fork carried only authStore.token and so 401'd on an autobot-issued token in autobot_access_token, the one case every other admin tool survived.

    vue-tsc green.

    ✅ Met, run just now on the current origin/Dev_new_gui:

    autobot-slm-frontend : vue-tsc --noEmit -p tsconfig.json      exit=0
    autobot-frontend     : vue-tsc --noEmit -p tsconfig.app.json  exit=0
    

    What is deliberately NOT done

    Extracting the Advanced Control contract into a shared package is ADR-008's Alternative 3, recorded there as "Deferred, not rejected — revisit if a third consumer appears." Its reasoning stands: a fourth file: dependency plus lockfile edits in both apps, for ~150 lines of genuinely shared types, while the two consumers still differ in transport and auth — and it would partly reverse #12420's direction of standing the SLM contract up inside the SLM app.

    I put this to the owner (2026-08-01) after initially mis-describing SLM as still using raw fetch. Decision: close as done and leave the deferral where the ADR put it.

    The remaining per-app duplication is what ADR-008 rule 2 explicitly permits — "deliberately duplicated shape, not a fork" — not fork residue.

    Gate 2 — dangling references

    No forward-tracking references. Gate 3 — nothing partial; every AC above is met.
11. mrveiss

    closed this as completedon Aug 1, 2026
12. mrveiss

    added a parent issue on Sep 1, 2026

    • umbrella(tech-debt): converge internal forks onto canonical reusable implementations #12645
13. mrveiss

    marked this as blocking on Sep 1, 2026

    • decision(frontend): Advanced Control is live in both SPAs — pick the canonical one (from #12653) #13078
14. mrveiss

    added 2 commits that reference this issue on Sep 20, 2026

    chore(frontend): remove the two dead useVncControls shims (#14907)

    ...

    a469f69

    chore(frontend): remove the two dead useVncControls shims (#14907) (#…

    ...

    b6dbac2