diff --git a/docs/coding-agent/lessons.md b/docs/coding-agent/lessons.md index e88ab74..34de8e4 100644 --- a/docs/coding-agent/lessons.md +++ b/docs/coding-agent/lessons.md @@ -258,3 +258,58 @@ Prevention: Evidence: - Reviewer Task_6 result NEEDS_REVISION with concrete conflict findings and file references. + +## 2026-02-25 — Auto-run research tasks unless implementation is required [tags: workflow, scope, communication] + +Context: +- Plan: docs/coding-agent/plans/active/exercise-intensity-dropdown-investigation-plan.md +- Task/Wave: pre-Task_1 execution +- Roles involved: Orchestrator + +Symptom: +- Orchestrator paused for explicit approval after plan creation even though the next step was research-only. +- User clarified that implementation tasks should be surfaced, but research-only tasks may proceed without explicit approval. + +Root cause: +- Approval gating default was applied too broadly and did not distinguish research-only execution from implementation execution. + +Fix applied: +- Adopted a split approval policy for this repository conversation: proceed automatically for research-only tasks; explicitly notify user before implementation tasks. + +Prevention: +- Repo rule candidate: + - audience: orchestrator + - proposed rule: After plan creation, do not block on explicit approval for research-only next steps unless the user requested a hard approval gate; always announce before implementation dispatch. +- Dispatch/plan guardrail (optional): + - During pre-dispatch check, label each upcoming task as research-only or implementation and apply approval requirements accordingly. + +Evidence: +- User instruction on 2026-02-25 explicitly defining approval expectations for research vs implementation tasks. + +## 2026-02-25 — Prefer non-E2E tests unless E2E adds unique value [tags: validation, testing, scope] + +Context: +- Plan: docs/coding-agent/plans/completed/exercise-intensity-dropdown-investigation-plan.md +- Task/Wave: Task_4 / validation strategy +- Roles involved: Orchestrator + +Symptom: +- Test recommendations included E2E as a routine option for a regression that could be covered by non-E2E tests. +- User clarified that E2E should be reserved for cases where it is uniquely necessary or clearly superior. + +Root cause: +- Default validation suggestion pattern over-emphasized broad flow coverage and did not prioritize the lowest-effective test layer first. + +Fix applied: +- Switched this task to non-E2E-first validation (`npm run check`, `npm run test:unit`) and added focused unit regression coverage for intensity state sync behavior. + +Prevention: +- Repo rule candidate: + - audience: orchestrator + - proposed rule: Propose E2E only after stating why unit/integration-level validation is insufficient for the specific risk being covered. +- Dispatch/plan guardrail (optional): + - For validation planning, document test-layer choice as: unit/integration first, E2E only for residual cross-layer risk. + +Evidence: +- User correction on 2026-02-25 explicitly setting E2E usage expectations. +- This implementation passed with non-E2E validations and targeted unit regression coverage. diff --git a/docs/coding-agent/plans/completed/exercise-intensity-dropdown-investigation-plan.md b/docs/coding-agent/plans/completed/exercise-intensity-dropdown-investigation-plan.md new file mode 100644 index 0000000..0945e44 --- /dev/null +++ b/docs/coding-agent/plans/completed/exercise-intensity-dropdown-investigation-plan.md @@ -0,0 +1,113 @@ +# Exercise Intensity Dropdown Investigation Plan + +- status: completed +- owner: Orchestrator +- created: 2026-02-25 +- updated: 2026-02-25 + +## Objective +Identify why selecting `light` or `hard` exercise intensity fails in create/edit entry flows, then provide a robust fix proposal that addresses code-level, runtime, and data-contract causes. + +## Scope +In scope: +- `sleep-ui` create/edit flow for exercise intensity select controls +- related API/domain serialization/deserialization for intensity values +- runtime/environment conditions that could make only specific options non-selectable + +Out of scope: +- implementing behavior changes before plan approval +- unrelated UI redesign + +## Task Waves +- Wave 1 (sequential): Task_1 +- Wave 2 (sequential): Task_2 +- Wave 3 (sequential): Task_3 +- Wave 4 (sequential): Task_4 + +## Tasks + +### Task_1 +- type: research +- owns: + - sleep-ui/src/** + - sleep-api/src/** + - sleep-ui/tests/** + - docs/coding-agent/plans/active/exercise-intensity-dropdown-investigation-plan.md +- depends_on: [] +- acceptance: + - Reproduce or logically confirm failure path for selecting `light` and `hard`. + - Enumerate plausible root causes across UI state handling, option binding, enum/string mapping, and API contract handling. + - Verify whether create and edit flows share code or diverge in a way that explains selective option failure. + - Capture evidence (code paths and, if needed, local runtime observations). +- validation: + - required: true + owner: orchestrator + kind: review + detail: Ensure investigation includes both code-path and runtime-condition analysis (not only static grep). + +### Task_2 +- type: review +- owns: + - docs/coding-agent/plans/active/exercise-intensity-dropdown-investigation-plan.md +- depends_on: + - Task_1 +- acceptance: + - Provide a prioritized fix proposal with root-cause confidence level. + - Include at least one immediate fix and one hardening/guardrail recommendation (e.g., test coverage or schema validation). + - State validation steps required to verify the fix after implementation. +- validation: + - required: true + owner: orchestrator + kind: review + detail: Proposal addresses create + edit flows and includes post-fix verification commands aligned with validation mapping. + +### Task_3 +- type: impl +- owns: + - sleep-ui/src/lib/components/Input.svelte + - sleep-ui/src/lib/components/SleepForm.svelte + - sleep-ui/src/lib/utils/** +- depends_on: + - Task_2 +- acceptance: + - Remove intensity selection dependence on wrapper-level `change` event delivery. + - Ensure create/edit flows preserve user-selected intensity and still accept late `initialIntensity` updates when untouched. + - Keep behavior scoped to intensity handling with minimal unrelated diff. +- validation: + - required: true + owner: worker + kind: command + detail: cd sleep-ui && npm run check + +### Task_4 +- type: test +- owns: + - sleep-ui/tests/unit/** +- depends_on: + - Task_3 +- acceptance: + - Add regression coverage for intensity sync/dirty-state behavior using non-E2E tests. + - Validate wrapper event forwarding behavior where practical without introducing heavy new test infrastructure. + - Keep E2E as optional/targeted only if a gap cannot be covered otherwise. +- validation: + - required: true + owner: worker + kind: command + detail: cd sleep-ui && npm run test:unit + +## Validation Plan +- No code edits are made before user approval. +- Investigation evidence will include file-level references and (if needed) local reproduction checks. +- If UI behavior is runtime-sensitive, include explicit environment factors and mitigation. +- Implementation validations prioritize non-E2E coverage (`npm run check`, `npm run test:unit`). +- E2E is optional and only used for residual risk not effectively covered by unit-level tests. + +## Progress Log +- 2026-02-25: Drafted plan for investigation/proposal workflow. Awaiting user approval. +- 2026-02-25: User approved proceeding directly to robust implementation; added implementation and unit-regression waves. +- 2026-02-25: Implemented robust fix in `Input` + `SleepForm` and added unit regression coverage for intensity synchronization state transitions. +- 2026-02-25: Validation executed: `cd sleep-ui && npm run check` (pass), `npm run test:unit` (pass after cwd-safe rerun). + +## Decision Log +- 2026-02-25: Treated request as non-trivial due to non-obvious bug spanning UI + possible backend/runtime mapping concerns. +- 2026-02-25: Applied non-E2E-first validation strategy; deferred E2E because regression was fully covered by targeted unit tests and type checks. diff --git a/sleep-ui/src/lib/components/Input.svelte b/sleep-ui/src/lib/components/Input.svelte index 5a72dbb..22e4578 100644 --- a/sleep-ui/src/lib/components/Input.svelte +++ b/sleep-ui/src/lib/components/Input.svelte @@ -1,4 +1,6 @@ {#if as === 'textarea'} - {:else if as === 'select'} - {:else} - + {/if} diff --git a/sleep-ui/src/lib/components/SleepForm.svelte b/sleep-ui/src/lib/components/SleepForm.svelte index 8688dd7..19c4a98 100644 --- a/sleep-ui/src/lib/components/SleepForm.svelte +++ b/sleep-ui/src/lib/components/SleepForm.svelte @@ -23,6 +23,7 @@ import { upsertRecent, setIntensity } from '$lib/stores/sleep'; import { pushToast } from '$lib/stores/toast'; import { computeDurationMin, formatDurationMin } from '$lib/utils/sleep'; + import { syncIntensityState } from '$lib/utils/intensity'; /** * Props @@ -55,6 +56,7 @@ let notes = initialNotes; let intensityDirty = false; + let previousInitialIntensity: 'none' | 'light' | 'hard' = initialIntensity; let loading = false; let errorMsg: string | null = null; @@ -74,8 +76,16 @@ ); $: warningMessage = getDurationWarningMessage(durationBounds, formatDurationMin); - $: if (!intensityDirty && intensity !== initialIntensity) { - intensity = initialIntensity; + $: { + const next = syncIntensityState({ + intensity, + initialIntensity, + previousInitialIntensity, + dirty: intensityDirty + }); + intensity = next.intensity; + previousInitialIntensity = next.previousInitialIntensity; + intensityDirty = next.dirty; } function today(): string { @@ -308,7 +318,6 @@ as="select" className={inputClass} bind:value={intensity} - on:change={() => (intensityDirty = true)} > diff --git a/sleep-ui/src/lib/utils/intensity.ts b/sleep-ui/src/lib/utils/intensity.ts new file mode 100644 index 0000000..0e3d7fa --- /dev/null +++ b/sleep-ui/src/lib/utils/intensity.ts @@ -0,0 +1,31 @@ +export type ExerciseIntensity = 'none' | 'light' | 'hard'; + +export type IntensityStateInput = { + intensity: ExerciseIntensity; + initialIntensity: ExerciseIntensity; + previousInitialIntensity: ExerciseIntensity; + dirty: boolean; +}; + +export type IntensityStateOutput = { + intensity: ExerciseIntensity; + previousInitialIntensity: ExerciseIntensity; + dirty: boolean; +}; + +export function syncIntensityState(input: IntensityStateInput): IntensityStateOutput { + let { intensity, initialIntensity, previousInitialIntensity, dirty } = input; + + if (initialIntensity !== previousInitialIntensity) { + if (!dirty) { + intensity = initialIntensity; + } + previousInitialIntensity = initialIntensity; + } + + if (!dirty && intensity !== initialIntensity) { + dirty = true; + } + + return { intensity, previousInitialIntensity, dirty }; +} diff --git a/sleep-ui/tests/unit/intensity.spec.ts b/sleep-ui/tests/unit/intensity.spec.ts new file mode 100644 index 0000000..5822f87 --- /dev/null +++ b/sleep-ui/tests/unit/intensity.spec.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from 'vitest'; +import { syncIntensityState, type ExerciseIntensity } from '../../src/lib/utils/intensity'; + +describe('syncIntensityState', () => { + it('keeps intensity unchanged when initial value is unchanged and not dirty', () => { + const result = syncIntensityState({ + intensity: 'none', + initialIntensity: 'none', + previousInitialIntensity: 'none', + dirty: false + }); + + expect(result).toEqual({ + intensity: 'none', + previousInitialIntensity: 'none', + dirty: false + }); + }); + + it('marks state dirty when user selection diverges from initial value', () => { + const result = syncIntensityState({ + intensity: 'hard', + initialIntensity: 'none', + previousInitialIntensity: 'none', + dirty: false + }); + + expect(result).toEqual({ + intensity: 'hard', + previousInitialIntensity: 'none', + dirty: true + }); + }); + + it('syncs to a late initialIntensity update when untouched', () => { + const result = syncIntensityState({ + intensity: 'none', + initialIntensity: 'light', + previousInitialIntensity: 'none', + dirty: false + }); + + expect(result).toEqual({ + intensity: 'light', + previousInitialIntensity: 'light', + dirty: false + }); + }); + + it('does not overwrite user selection after dirty state, even if initial changes later', () => { + const result = syncIntensityState({ + intensity: 'hard', + initialIntensity: 'light', + previousInitialIntensity: 'none', + dirty: true + }); + + expect(result).toEqual({ + intensity: 'hard', + previousInitialIntensity: 'light', + dirty: true + }); + }); + + it('supports all enum values without narrowing issues', () => { + const values: ExerciseIntensity[] = ['none', 'light', 'hard']; + + for (const value of values) { + const result = syncIntensityState({ + intensity: value, + initialIntensity: value, + previousInitialIntensity: value, + dirty: false + }); + expect(result.intensity).toBe(value); + } + }); +});