From 3f9cea9ecaf8fdf6e44b574c42ff68fe7ea3a3e9 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Sun, 15 Feb 2026 22:20:30 +0100 Subject: [PATCH] refactor: address all code review findings for roadmap XState refactor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix all CodeRabbit review comments and Biome warnings: - Replace non-null assertions with explicit null checks in roadmap-store.ts (lines 548, 553, 559, 565) - Replace non-null assertion with proper type guards in test (line 396) - Use @shared/state-machines path alias instead of relative imports - Prune stale feature actors in setRoadmap when roadmap changes - Add lastActivityAt timestamp tracking to generation machine context - Update deriveGenerationStatus to use persisted lastActivityAt from context - Fix misleading test title "ignore MARK_DONE" → "self-transition" - Add test verifying progress preservation on GENERATION_ERROR - Remove progress reset in setError action to preserve progress on errors - Add compile-time assertions to ensure state name arrays stay in sync with XState machines All tests pass (77/77). TypeScript compilation successful. Zero Biome warnings in modified files. Co-Authored-By: Claude Opus 4.6 --- .../src/renderer/stores/roadmap-store.ts | 34 +++++++++++++------ .../__tests__/roadmap-feature-machine.test.ts | 2 +- .../roadmap-generation-machine.test.ts | 17 +++++++++- .../roadmap-generation-machine.ts | 8 ++++- .../state-machines/roadmap-state-utils.ts | 9 +++++ 5 files changed, 57 insertions(+), 13 deletions(-) diff --git a/apps/frontend/src/renderer/stores/roadmap-store.ts b/apps/frontend/src/renderer/stores/roadmap-store.ts index 84054fa2..829e78cb 100644 --- a/apps/frontend/src/renderer/stores/roadmap-store.ts +++ b/apps/frontend/src/renderer/stores/roadmap-store.ts @@ -17,7 +17,7 @@ import { mapFeatureStateToStatus, type RoadmapGenerationEvent, type RoadmapFeatureEvent -} from '../../shared/state-machines'; +} from '@shared/state-machines'; // --------------------------------------------------------------------------- // Module-level XState actor singletons @@ -176,9 +176,7 @@ function deriveGenerationStatus(actor: Actor): message: ctx.message ?? '', error: ctx.error, startedAt: ctx.startedAt ? new Date(ctx.startedAt) : undefined, - lastActivityAt: phase !== 'idle' && phase !== 'complete' && phase !== 'error' - ? new Date() - : undefined + lastActivityAt: ctx.lastActivityAt ? new Date(ctx.lastActivityAt) : undefined }; } @@ -191,8 +189,20 @@ export const useRoadmapStore = create((set) => ({ // Actions setRoadmap: (roadmap) => { - featureActors.forEach((actor) => actor.stop()); - featureActors.clear(); + // Prune stale actors: stop and remove actors for features not in the new roadmap + if (roadmap) { + const newFeatureIds = new Set(roadmap.features.map((f) => f.id)); + for (const [featureId, actor] of featureActors.entries()) { + if (!newFeatureIds.has(featureId)) { + actor.stop(); + featureActors.delete(featureId); + } + } + } else { + // No roadmap → cleanup all actors + featureActors.forEach((actor) => actor.stop()); + featureActors.clear(); + } return set({ roadmap }); }, @@ -545,24 +555,28 @@ async function reconcileLinkedFeatures(projectId: string, roadmap: Roadmap): Pro let hasChanges = false; for (const feature of featuresNeedingReconciliation) { - const task = taskMap.get(feature.linkedSpecId!); + // Safe: linkedSpecId is guaranteed to exist by the filter on line 531 + const linkedSpecId = feature.linkedSpecId; + if (!linkedSpecId) continue; + + const task = taskMap.get(linkedSpecId); if (!task) { // Task no longer exists → mark as done with deleted outcome if (feature.status !== 'done' || feature.taskOutcome !== 'deleted') { - store.markFeatureDoneBySpecId(feature.linkedSpecId!, 'deleted'); + store.markFeatureDoneBySpecId(linkedSpecId, 'deleted'); hasChanges = true; } } else if (task.status === 'done' || task.status === 'pr_created') { // Task is completed → mark feature as done if (feature.status !== 'done' || !feature.taskOutcome) { - store.markFeatureDoneBySpecId(feature.linkedSpecId!, 'completed'); + store.markFeatureDoneBySpecId(linkedSpecId, 'completed'); hasChanges = true; } } else if (task.metadata?.archivedAt) { // Task is archived → mark feature as done with archived outcome if (feature.status !== 'done' || feature.taskOutcome !== 'archived') { - store.markFeatureDoneBySpecId(feature.linkedSpecId!, 'archived'); + store.markFeatureDoneBySpecId(linkedSpecId, 'archived'); hasChanges = true; } } diff --git a/apps/frontend/src/shared/state-machines/__tests__/roadmap-feature-machine.test.ts b/apps/frontend/src/shared/state-machines/__tests__/roadmap-feature-machine.test.ts index 0b3ba887..d3814f61 100644 --- a/apps/frontend/src/shared/state-machines/__tests__/roadmap-feature-machine.test.ts +++ b/apps/frontend/src/shared/state-machines/__tests__/roadmap-feature-machine.test.ts @@ -283,7 +283,7 @@ describe('roadmapFeatureMachine', () => { expect(snapshot.value).toBe('in_progress'); }); - it('should ignore MARK_DONE when already in done', () => { + it('should remain in done on MARK_DONE self-transition', () => { const snapshot = runEvents([{ type: 'MARK_DONE' }, { type: 'MARK_DONE' }]); expect(snapshot.value).toBe('done'); }); diff --git a/apps/frontend/src/shared/state-machines/__tests__/roadmap-generation-machine.test.ts b/apps/frontend/src/shared/state-machines/__tests__/roadmap-generation-machine.test.ts index 8518fe6e..28012519 100644 --- a/apps/frontend/src/shared/state-machines/__tests__/roadmap-generation-machine.test.ts +++ b/apps/frontend/src/shared/state-machines/__tests__/roadmap-generation-machine.test.ts @@ -168,6 +168,17 @@ describe('roadmapGenerationMachine', () => { expect(snapshot.value).toBe('error'); expect(snapshot.context.error).toBe('Generation failed'); }); + + it('should preserve progress when transitioning to error', () => { + const snapshot = runEvents([ + { type: 'START_GENERATION' }, + { type: 'PROGRESS_UPDATE', progress: 60, message: 'Processing...' }, + { type: 'GENERATION_ERROR', error: 'Something went wrong' }, + ]); + expect(snapshot.value).toBe('error'); + expect(snapshot.context.progress).toBe(60); + expect(snapshot.context.error).toBe('Something went wrong'); + }); }); describe('stop flow: STOP from analyzing/discovering/generating → idle', () => { @@ -393,7 +404,11 @@ describe('roadmapGenerationMachine', () => { actor.send({ type: 'START_GENERATION' }); const secondStartedAt = actor.getSnapshot().context.startedAt; - expect(secondStartedAt).toBeGreaterThan(firstStartedAt!); + expect(firstStartedAt).toBeDefined(); + expect(secondStartedAt).toBeDefined(); + if (firstStartedAt && secondStartedAt) { + expect(secondStartedAt).toBeGreaterThan(firstStartedAt); + } actor.stop(); }); }); diff --git a/apps/frontend/src/shared/state-machines/roadmap-generation-machine.ts b/apps/frontend/src/shared/state-machines/roadmap-generation-machine.ts index 4e351566..606a577e 100644 --- a/apps/frontend/src/shared/state-machines/roadmap-generation-machine.ts +++ b/apps/frontend/src/shared/state-machines/roadmap-generation-machine.ts @@ -6,6 +6,7 @@ export interface RoadmapGenerationContext { error?: string; startedAt?: number; completedAt?: number; + lastActivityAt?: number; } export type RoadmapGenerationEvent = @@ -32,6 +33,7 @@ export const roadmapGenerationMachine = createMachine( error: undefined, startedAt: undefined, completedAt: undefined, + lastActivityAt: undefined, }, states: { idle: { @@ -83,21 +85,24 @@ export const roadmapGenerationMachine = createMachine( error: () => undefined, startedAt: () => Date.now(), completedAt: () => undefined, + lastActivityAt: () => Date.now(), }), updateProgress: assign({ progress: ({ event }) => event.type === 'PROGRESS_UPDATE' ? Math.min(100, Math.max(0, event.progress)) : 0, message: ({ event }) => event.type === 'PROGRESS_UPDATE' ? event.message : undefined, + lastActivityAt: () => Date.now(), }), setCompleted: assign({ progress: () => 100, completedAt: () => Date.now(), + lastActivityAt: () => Date.now(), }), setError: assign({ error: ({ event }) => event.type === 'GENERATION_ERROR' ? event.error : undefined, - progress: () => 0, + lastActivityAt: () => Date.now(), }), resetContext: assign({ progress: () => 0, @@ -105,6 +110,7 @@ export const roadmapGenerationMachine = createMachine( error: () => undefined, startedAt: () => undefined, completedAt: () => undefined, + lastActivityAt: () => undefined, }), }, } diff --git a/apps/frontend/src/shared/state-machines/roadmap-state-utils.ts b/apps/frontend/src/shared/state-machines/roadmap-state-utils.ts index 2e3678f1..e02751f7 100644 --- a/apps/frontend/src/shared/state-machines/roadmap-state-utils.ts +++ b/apps/frontend/src/shared/state-machines/roadmap-state-utils.ts @@ -5,7 +5,10 @@ * derived from the roadmap machine definitions. Used by roadmap-store * and roadmap hooks to avoid duplicate constants. */ +import type { StateValueFrom } from 'xstate'; import type { RoadmapGenerationStatus, RoadmapFeatureStatus } from '../types/roadmap'; +import { roadmapGenerationMachine } from './roadmap-generation-machine'; +import { roadmapFeatureMachine } from './roadmap-feature-machine'; /** * All XState generation state names. @@ -19,6 +22,9 @@ export const GENERATION_STATE_NAMES = [ export type GenerationStateName = typeof GENERATION_STATE_NAMES[number]; +// Compile-time assertion: ensure GENERATION_STATE_NAMES stays in sync with roadmapGenerationMachine +const _genCheck: readonly StateValueFrom[] = GENERATION_STATE_NAMES; + /** * All XState feature state names. * @@ -31,6 +37,9 @@ export const FEATURE_STATE_NAMES = [ export type FeatureStateName = typeof FEATURE_STATE_NAMES[number]; +// Compile-time assertion: ensure FEATURE_STATE_NAMES stays in sync with roadmapFeatureMachine +const _featCheck: readonly StateValueFrom[] = FEATURE_STATE_NAMES; + /** * Generation states where the machine has settled — the generation is * complete or has errored. Stale progress events should NOT overwrite