fix(workflow): resolve stale greeting and race condition in startup prompt

[jeremyeder]

Summary

Addresses Critical and Major review feedback from code review of #780:

• Critical — Stale greeting persists when switching workflows: setWorkflowGreeting was only called when startupPrompt was truthy, so switching from a workflow with a greeting to one without left the old greeting displayed. Now clears unconditionally with setWorkflowGreeting(workflow.startupPrompt || null).

• Major — Race condition drops greeting silently: The useEffect that activates queued workflows referenced ootbWorkflows but didn't include it in the dependency array. If the OOTB workflows API response hadn't resolved when the session reached "Running" phase, fullWorkflow would be undefined and startupPrompt silently lost. Now gates on ootbWorkflows.length > 0 and includes ootbWorkflows in the dependency array.

Test plan

• Select a workflow with a startupPrompt — greeting appears immediately
• Switch to a workflow without a startupPrompt — previous greeting is cleared
• Select a workflow before session reaches "Running" (queued activation) — greeting still appears after session starts
• Verify no duplicate workflow activations from the added dependency

Generated with Claude Code

[github-actions]

Claude Code Review

Summary

PR #846 delivers two targeted bug fixes to the workflow management system: (1) a stale greeting that persisted when switching from a workflow with a startupPrompt to one without, and (2) a race condition where a queued workflow's startupPrompt was silently dropped if the OOTB workflows list hadn't loaded by the time the session reached "Running" phase. The fixes are surgical and correct. A Vitest unit test suite is added as regression coverage, which is a valuable addition to the codebase.

Issues by Severity

Blocker Issues

None

Critical Issues

None

Major Issues

1. ootbWorkflows.length > 0 uses array length as a proxy for "data loaded" state

• File: components/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsx:338
• Problem: The guard ootbWorkflows.length > 0 is intended to mean "OOTB workflows have finished loading," but it conflates "loaded with results" and "still loading." If the API legitimately returns an empty array (no workflows configured), the condition will never be satisfied and a queued workflow will silently never activate — even after the loading completes. This uses array length as a loading-state proxy rather than the actual loading state from the data-fetching layer.
• Standard violated: react-query-usage.md — prefer explicit loading/error states from React Query (isLoading, isPending) over inferring state from data shape.
• Suggested fix: If ootbWorkflows comes from a React Query hook, pass the isSuccess flag (or a boolean like ootbWorkflowsLoaded) alongside the data and gate on that instead:

  // e.g. if ootbWorkflows comes from: const { data: ootbWorkflows = [], isSuccess: ootbWorkflowsLoaded } = useOotbWorkflows(...)
  if (phase === "Running" && queuedWorkflow && !queuedWorkflow.activatedAt && ootbWorkflowsLoaded) {

  This correctly distinguishes the empty-but-loaded case from the still-loading case.

Minor Issues

1. afterEach used without an explicit import in the test file

• File: components/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/__tests__/use-workflow-management.test.ts (line ~62)
• Problem: describe, it, expect, beforeEach, and vi are all explicitly imported from "vitest", but afterEach is used bare, relying on globals: true in vitest.config.ts. This inconsistency could confuse contributors unfamiliar with the globals setting.
• Suggested fix: Add afterEach to the import:

  import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";

2. Redundant setupFiles: [] in vitest config

• File: components/frontend/vitest.config.ts:7
• Problem: setupFiles: [] is the default and adds no value; it is just noise.
• Suggested fix: Remove the line entirely.

3. makeWorkflow helper return type is widened by Record<string, unknown> spread

• File: components/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/__tests__/use-workflow-management.test.ts:33
• Problem: Spreading overrides: Record<string, unknown> into the return object widens the inferred return type to include an index signature, which can suppress TypeScript errors when the result is passed to activateWorkflow. If the real workflow type is available, using Partial<WorkflowConfig> would be more precise.
• Suggested fix:

  // Import actual type if available
  function makeWorkflow(overrides: Partial<WorkflowConfig> = {}): WorkflowConfig {
    return { id: "wf-test", ... , ...overrides } as WorkflowConfig;
  }

Positive Highlights

• Surgical, correct one-line fix — setWorkflowGreeting(workflow.startupPrompt || null) is exactly the right idiom: always write the state unconditionally, which both sets a new greeting and clears a stale one. No unnecessary complexity introduced.
• Excellent regression test design — The test suite is thoughtfully organized. Each test case maps directly to a specific failure mode (stale greeting, empty-string prompt, API failure) and the inline comment explaining the pre-patch bug behavior is a great practice that makes future bisecting much easier.
• Proper dependency array fix — Adding ootbWorkflows to the useEffect deps array correctly resolves the exhaustive-deps concern and ensures the effect re-evaluates when the workflow list loads. The eslint-disable comment is still valid for the workflowManagement.activateWorkflow reference that intentionally isn't included.
• Test infrastructure investment — Setting up Vitest + jsdom + @testing-library/react as dev dependencies for hook-level unit tests is a meaningful addition that will enable faster, more targeted testing of hook logic without requiring E2E infrastructure.

Recommendations

1. (Major — before merge) Replace ootbWorkflows.length > 0 with a proper loading-state boolean from the React Query hook. This closes the silent-failure edge case for the empty-list scenario and sets a better precedent for loading-state handling.
2. (Minor) Add afterEach to the explicit vitest imports for consistency.
3. (Minor) Remove setupFiles: [] from vitest.config.ts.
4. (Minor) Tighten the makeWorkflow helper type to use the real workflow type instead of Record<string, unknown>.

---

🤖 Reviewed by Claude Code using the Amber review system

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.