Code Smell / Tech-Debt Tracking

[Predixx]

Code Smell / Tech-Debt Tracking

Tracking issue for maintainability findings from a structured code-smell audit of the extension source (extension/src, ~35k LOC, 272 files), 2026-06-02.

Context / scope:

• knip runs clean (no dead code, no unused exports) and ESLint is wired, so this issue deliberately excludes anything tooling already catches. It covers design/maintainability smells only.
• The codebase is healthy overall (good DI seams, careful concurrency/lifecycle handling). Most items below are "grew too large" / copy-paste debt, not bugs. The exceptions are the four Real risks at the top.
• Items are grouped by severity. Severity is a triage hint, not a promise that everything is urgent. Check off as addressed; split into dedicated issues/PRs where useful.
• Update 2026-06-08 (re-verified): the source tree was reorganized since this audit. Everything now lives under src/extension/ and src/webview/, so the paths below are stale by directory (findings re-verified against current code). All four Real risks remain fixed. No maintainability item is fully resolved yet, but two have a sub-part addressed (see the inline ⚠️ notes below): parseRecordedData (duplicate-schema) and extensionCommands (WS-status state machine). The show*Message count is now ~165 (was 144).

---

🔴 Real risks (latent correctness, do first)

• Fragile git status --porcelain parsing — services/workspace/workspaceFileChecker.ts:216. line.slice(3).trim() blindly slices; renames (R old -> new) and quoted/spaced paths get mangled and silently dropped from the file set sent to Iris. → git status --porcelain=v1 -z + NUL-split, handle rename form. ✅ Fixed in fix(workspace): parse git status with -z to handle renames and spaced paths #258.
• No timeout/abort on network fetches — api/artemisApi.ts:71 (also :298, :392). makeRequest/authenticate/logoutFromServer have no AbortController/timeout (git execs do); a stalled server hangs login/chat indefinitely. → thread an AbortSignal with default timeout. ✅ Fixed in fix(api): add request timeouts to Artemis API calls #259. Follow-up fix(auth): keep stored credentials on transient startup failures #261 (don't clear stored credentials on a transient startup timeout). Deferred (LOW): body-read timeout — fetchWithTimeout only covers connect+headers; a full-body timeout needs an API-layer wrapper refactor.
• async dispose() on a synchronous vscode.Disposable — services/telemetry/recording/sessionRecorder.ts:491 (also storageWriter.ts:255). Returns a Promise but the interface is void → fire-and-forget disposal; the documented awaited-shutdown durability guarantee only holds on the one hand-wired deactivate() path. → sync dispose() that schedules teardown + explicit await drain()/shutdown(). ✅ Fixed in refactor(recording): harden recorder teardown contract, fix write-lane TOCTOU #260 (renamed dispose()→shutdown() and dropped implements vscode.Disposable, so the awaitable teardown can't be structurally mistaken for a sync disposable).
• TOCTOU across await, masked by ! — services/telemetry/recording/storageWriter.ts:190 (also :211, :383). Guard on _snapshotsDir, then _snapshotsDir! inside a queued lambda; endSession/abort can null it in between → runtime crash instead of a typed branch. → capture path into a local const before enqueuing. ✅ Fixed in refactor(recording): harden recorder teardown contract, fix write-lane TOCTOU #260 (:190/:211 captured into locals; :383 was already safe — its guard runs inside the lambda).

🟠 God objects (oversized, mixed responsibilities)

• provider/chatWebviewProvider.ts (966 LOC) — largest file in the repo. Extract the command dispatcher (_handleCommand:513) + ~10 _handle* methods into a ChatWebviewMessageHandler (mirroring the sidebar's WebViewMessageHandler); leave the provider as a thin coordinator.
• api/artemisApi.ts (587 LOC) — one class owns auth + submission/result + Iris + problem-statement endpoints. Split into AuthApi/ProgrammingApi/IrisApi over a shared makeRequest core.
• services/telemetry/telemetryManager.ts (536 LOC) — orchestrates 13 sub-services, 8 listeners, config loading, debug lifecycle, and decision dispatch. Extract event-listener wiring and the config/debug lifecycle into collaborators.
• activation/extensionCommands.ts (707 LOC) + extension.ts activate() (~235 LOC) — command file contains a ~110-line WS-status state machine; activate() has a load-bearing but implicit construction order. Move the WS-status logic to its own module; split activate() into phase helpers. ⚠️ Partial (2026-06-08): the WS-status state machine is now extracted into pure helpers (collectWebSocketStatus/buildStatusReport/decideStatusActions/buildStatusHeadline/handleStatusAction). The file is still 707 LOC and activate() (~236 LOC) is unchanged.
• views/IrisChat/IrisChatView.tsx (646 LOC) — 12-case message reducer + send/retry/feedback + 5 layers of nested render-gating ternaries (:561-598). Extract a useIrisChatMessages hook + a pure deriveChatUiState selector.
• views/ExerciseDetail/ExerciseDetailView.tsx (623 LOC) — ~250-line derivation engine in the render body that reruns on every WebSocket re-render. Extract a useExerciseDetailViewModel selector; move time-remaining into existing utils.
• services/iris/chat/chatSessionService.ts (692 LOC) — availability + concurrency + settings probing + formatting + create/switch/reset/reload, with repeated isCurrentContext guard blocks. Extract availability classification + a shared guard helper.
• services/telemetry/recording/parseRecordedData.ts (786 LOC) — 36 hand-written validators, each event schema declared twice (here + types.ts) with redundant as casts. Replace with a schema lib (zod/valibot) or a small field-spec DSL. ⚠️ Partial (2026-06-08): the "declared twice" half is resolved. Dispatch is now a single EVENT_PARSERS table (satisfies Record<RecordedEvent['type'], EventParser>; a missing parser fails compilation, and the runtime type set is derived from its keys, per Schema source of truth: deduplicate event-type lists between parser and validator #215). The 40 hand-written validators (was 36) remain.

🟠 Duplication

• withErrorBoundary for command handlers — controller/commands/*. try { … } catch { logger.X; showErrorMessage } is copy-pasted across dozens of handlers (144 show*Message calls in scope) and already drifts. → a single withErrorBoundary(label, fn) decorator.
• useViewInit + <ViewScaffold> — ~13 webview views each re-wire init listener, reload/retry/back, getState/setState persistence, and loading/error/empty scaffolding; they already diverge (local isLoaded vs store isLoading; only some handle ViewInitError).
• Merge showNotificationEQ/showProactiveHelpEQ — services/telemetry/interventionService.ts:157 vs :201. Near-identical incl. a copy-pasted accept/dismiss block. → shared _showModalIntervention + _handleResult.
• Derive triplicated intervention schemas — intervention action/level/triggerType/dismissReason spelled out three times (types.ts + parser isOneOf + recorder API). Adding an enum value silently drops events in replay if one copy is missed. → export const-asserted arrays and derive unions + guards. Note (2026-06-08): blockedReason is now only 2× (the recorder references the derived InterventionEvent['blockedReason'] type). action/level/triggerType/dismissReason remain 3×.
• Misc duplication (bundle) — record-emission gating copied across ~18 listeners (recording/observation/observationRegistry.ts:103); repo-match block 4× (workspace/workspaceDetectionService.ts:216); Git-extension API access reimplemented twice with opposite type rigor (workspaceFileChecker.ts:127 vs fileMonitorService.ts:65); logout sequence 2× (extensionCommands.ts:27 vs authCommands.ts:59); chat-retry payload rebuilt (artemisApi.ts:478).

🟡 Scattered constants / magic numbers

• Centralize struggle-detection thresholds — spread across interventionService.ts:42 (0.45/0.80), decision/interventionDecisionEngine.ts:27 (0.15/0.35/0.60), eventPipeline/compileEquivalentEmitter.ts:210, despite types.ts being the advertised config home. Also: LOOKAHEAD_WINDOW_MS=500 (replay) must stay in lock-step with the live emitter's 500ms timer but nothing links them → silent desync risk.
• Centralize own-extension command IDs + config keys — 'iris.chatView.focus', getConfiguration('artemis'), etc. hard-coded inline despite VSCODE_CONFIG constants existing; 'artemis.trustedDomains' global-state key duplicated (utilityCommands.ts + extensionCommands.ts:522).
• Source EQ colors from shared constants — views/StruggleDetection/StruggleDetectionView.tsx hard-codes EQ color hex values that duplicate the decision-engine threshold semantics; dozens of inline style={{}} bypass the CSS-module convention used elsewhere.

🟡 Type-safety holes

• Remove decision.level casts + tighten API unions — activation/sessionRecorderWiring.ts:92-118 casts decision.level 5×; shared/types/apiResponses.ts:67/78/99 type state/type/initializationState as bare string instead of literal unions, forcing runtime narrowing in every view.
• Drop redundant typeof x === 'function' checks — controller/commands/navigationCommands.ts:90/95, irisCommands.ts:68/92 guard methods that the IChatWebviewProvider interface declares as required; dead defensive branches. A plain null-check suffices.

🟢 Low / hygiene

• Log hygiene — decorative emoji in structured logs (~21× in controller scope, pervasive in Iris services) and JSON.stringify(payload) on info-level hot paths (iris/chat/chatMessageService.ts). → demote step tracing to debug/trace, drop emoji from log lines.
• Low bundle — index used in React keys despite stable id (CourseListView.tsx:270/325; Dashboard tracks expanded state by array index); internal "Block F/AB/K" provenance comments in recording/types.ts reference an unresolvable plan; errorCount diverges between the two replay snapshot builders (replay/snapshotReconstructor.ts:51 raw count vs :78 family count) — currently not an active bug since EQ uses only hasErrors/errorFamilies, but a latent trap if errorCount is ever consumed.