Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions issues/epic-7-tech-debt/ISSUE-E7-01.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# ISSUE-E7-01 β€” Consolidate Duplicate Scale Utilities

## Objective
Remove the duplicated scale-computation logic that exists across two separate feature modules and establish a single authoritative implementation.

## Background
Two distinct utility files compute the same diatonic pitch-class set:

| File | Export | Return type |
|---|---|---|
| `client/src/features/chromatic-circle/utils/scaleUtils.ts` | `getDiatonicIndices(root, mode)` | `Set<number>` |
| `client/src/features/scale/utils/scaleUtils.ts` | `getScaleNotes(rootIndex, scaleType)` | `number[]` |

Both accept a root pitch class (0–11) and a scale/mode identifier and return the same set of diatonic notes. Having two implementations means any bug fix or new mode must be applied in two places, and callers in different features may silently diverge.

The `scale` feature module is the natural owner of scale-computation logic. `chromatic-circle` should depend on it, not maintain a parallel copy.

## Files To Edit

- `client/src/features/scale/utils/scaleUtils.ts` β€” if needed, expose a `Set<number>`-typed variant so callers in `chromatic-circle` do not need to convert.
- `client/src/features/chromatic-circle/utils/scaleUtils.ts` β€” replace the implementation body with a re-export or thin wrapper that delegates to the `scale` feature utility.
- Any file that currently imports from `chromatic-circle/utils/scaleUtils` β€” update to import from the canonical location once the API is unified.

## Files To Add
None.

## Acceptance Criteria
- [ ] Scale computation logic exists in exactly one place (`client/src/features/scale/utils/scaleUtils.ts`).
- [ ] `client/src/features/chromatic-circle/utils/scaleUtils.ts` either re-exports from `scale` or is deleted (no duplicate algorithm body).
- [ ] All existing callers compile without TypeScript errors.
- [ ] All existing unit tests pass.
- [ ] `npm run lint` passes with `--max-warnings=0`.
- [ ] `npm run build` succeeds with no TypeScript errors.

## Verification Commands
```bash
cd client
npm run lint
npm run build
npm test
```
52 changes: 52 additions & 0 deletions issues/epic-7-tech-debt/ISSUE-E7-02.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# ISSUE-E7-02 β€” Extract Repeated Chord-Note Retrieval into a Shared Utility

## Objective
Replace a copy-pasted conditional that retrieves pitch classes from a `Chord` value with a single reusable utility function, removing the duplication from at least five files.

## Background
The following pattern appears verbatim (or near-verbatim) in multiple files across the codebase:

```ts
isCustomChord(chord) ? chord.customNotes : getChordNoteIndices(chord.root, chord.type)
```

**Known locations (non-exhaustive):**
- `client/src/features/current-chord/components/CurrentChordPanel.tsx`
- `client/src/features/progression-sidebar/components/ChordTile.tsx`
- `client/src/features/progression-sidebar/components/ProgressionSidebar.tsx`
- `client/src/features/midi-export/utils/midiBuilder.ts`
- `client/src/features/progression-sidebar/utils/pairMetrics.ts`

Each duplicate is a maintenance hazard: introducing a new chord variant (e.g., quartal chords) requires hunting down and updating every occurrence. A single function eliminates that risk.

## Proposed API
```ts
// client/src/features/chord/utils/getChordPitchClasses.ts
export function getChordPitchClasses(chord: Chord): number[]
```

The function should:
1. Check `isCustomChord(chord)` and return `chord.customNotes` if true.
2. Otherwise call `getChordNoteIndices(chord.root, chord.type)` and return the result.

## Files To Add
- `client/src/features/chord/utils/getChordPitchClasses.ts` β€” new utility function.

## Files To Edit
- `client/src/features/chord/utils/index.ts` β€” re-export `getChordPitchClasses`.
- All files listed in **Known locations** above β€” replace inline conditionals with a call to `getChordPitchClasses(chord)`.

## Acceptance Criteria
- [ ] `getChordPitchClasses(chord: Chord): number[]` exists in `client/src/features/chord/utils/`.
- [ ] No file outside `getChordPitchClasses.ts` contains the pattern `isCustomChord(chord) ? chord.customNotes`.
- [ ] All existing tests pass.
- [ ] `npm run lint` passes with `--max-warnings=0`.
- [ ] `npm run build` succeeds with no TypeScript errors.

## Verification Commands
```bash
cd client
npm run lint
npm run build
npm test
```
72 changes: 72 additions & 0 deletions issues/epic-7-tech-debt/ISSUE-E7-03.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# ISSUE-E7-03 β€” Refactor `useChordState` into Focused Single-Concern Hooks

## Objective
Break the monolithic `useChordState` hook (309 lines, 7 `useState` calls, 5 `useCallback` functions, multiple unrelated concerns) into smaller, independently testable hooks.

## Background
`client/src/features/chromatic-circle/hooks/useChordState.ts` currently manages all of the following in a single function:

| Concern | State/callbacks |
|---|---|
| Pointer drag detection | `dragStart`, `currentDrag`, `dragHasMoved` |
| Named chord selection | `selectedChord`, `selectChord` |
| Custom chord construction | `customNotes`, `toggleCustomNote`, `clearCustomNotes`, `isCustomMode` |
| Primitive chord shape | derived from `selectedChord.type` |
| Screen-reader announcements | `announcement` string, setter |

The combined size makes the hook impossible to unit-test (too many internal dependencies) and difficult to reason about (unrelated state changes may trigger re-renders together).

## Proposed Split

### `useDragState` (new)
```ts
// Owns: dragStart, currentDrag, dragHasMoved
// Exported from: chromatic-circle/hooks/useDragState.ts
```

### `useChordSelection` (new)
```ts
// Owns: selectedChord, selectChord callback
// Exported from: chromatic-circle/hooks/useChordSelection.ts
```

### `useCustomChordState` (new)
```ts
// Owns: customNotes, toggleCustomNote, clearCustomNotes, isCustomMode
// Exported from: chromatic-circle/hooks/useCustomChordState.ts
```

### `useChordState` (thin orchestrator β€” keep same public interface)
```ts
// Composes the three new hooks + announcement state
// Public surface unchanged β€” callers require no edits
// Target: ≀150 lines
```

## Files To Add
- `client/src/features/chromatic-circle/hooks/useDragState.ts`
- `client/src/features/chromatic-circle/hooks/useChordSelection.ts`
- `client/src/features/chromatic-circle/hooks/useCustomChordState.ts`

## Files To Edit
- `client/src/features/chromatic-circle/hooks/useChordState.ts` β€” gutted to orchestrator; delegates to new hooks.
- `client/src/features/chromatic-circle/hooks/index.ts` β€” export new hooks if not already done.

## Files Not To Edit
- Any component that calls `useChordState` β€” the public interface must remain identical.

## Acceptance Criteria
- [ ] Three new focused hooks exist, each ≀150 lines and with a single declared concern.
- [ ] `useChordState.ts` is ≀150 lines and only composes the three hooks.
- [ ] No caller of `useChordState` requires any change.
- [ ] All existing tests pass.
- [ ] `npm run lint` passes with `--max-warnings=0`.
- [ ] `npm run build` succeeds with no TypeScript errors.

## Verification Commands
```bash
cd client
npm run lint
npm run build
npm test
```
39 changes: 39 additions & 0 deletions issues/epic-7-tech-debt/ISSUE-E7-04.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# ISSUE-E7-04 β€” Remove Dead Code in `shared/`

## Objective
Delete the unused `ChromaticCircle` component from `client/src/shared/` and clean up the otherwise-empty placeholder directory.

## Background
`client/src/shared/ChromaticCircle.tsx` is a copy of the main chromatic-circle component that is not imported anywhere in the application. It was likely created as an intermediate step during an earlier refactor and was never cleaned up.

The `shared/` directory contains only this file plus `.gitkeep` placeholders:

```
client/src/shared/
β”œβ”€β”€ ChromaticCircle.tsx ← unused duplicate
β”œβ”€β”€ components/.gitkeep
β”œβ”€β”€ hooks/.gitkeep
β”œβ”€β”€ types/.gitkeep
└── utils/.gitkeep
```

Having an unreferenced component that mirrors the real one creates confusion about which is canonical and may mislead future contributors into editing the wrong file.

## Files To Delete
- `client/src/shared/ChromaticCircle.tsx`

## Files To Edit
None. The `.gitkeep` files may remain as placeholders for future shared utilities; if the project convention is to remove empty directories, they can be removed too β€” that decision is left to the implementer.

## Acceptance Criteria
- [ ] `client/src/shared/ChromaticCircle.tsx` no longer exists in the repository.
- [ ] No other file imports from `client/src/shared/ChromaticCircle`.
- [ ] `npm run lint` passes with `--max-warnings=0`.
- [ ] `npm run build` succeeds with no TypeScript errors.

## Verification Commands
```bash
cd client
npm run lint
npm run build
```
36 changes: 36 additions & 0 deletions issues/epic-7-tech-debt/ISSUE-E7-05.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# ISSUE-E7-05 β€” Separate Auto-Generated API Types from Hand-Written Client Wrappers

## Objective
Ensure that `client/src/api/client/index.ts` contains only hand-written code so that running `npm run generate:api` can never overwrite manual endpoint wrappers.

## Background
`client/src/api/client/index.ts` currently mixes two categories of code:
1. **Auto-generated type bindings** β€” should live exclusively in `client/src/api/generated/index.ts` (marked "do not edit").
2. **Hand-written endpoint wrappers** β€” `getHealth()`, `getScaleFromRoot()`, and the `createClient` configuration. These are manually maintained.

If a developer runs `npm run generate:api`, the generated file is overwritten. If the generation script ever targets `client/src/api/client/index.ts` instead of (or in addition to) the generated file, hand-written logic would be silently lost.

Additionally:
- `getScaleFromRoot()` hard-codes the scale type to `"Major"` with no parameter, making the function inflexible.
- The `VITE_API_BASE_URL` fallback hard-codes `http://localhost:5110`, which should be documented as a dev-only default.

## Files To Edit
- `client/src/api/client/index.ts` β€” remove any type re-declarations that duplicate `generated/index.ts`; ensure all type imports come from `../generated`; document the `VITE_API_BASE_URL` fallback; give `getScaleFromRoot` a `scaleType` parameter with a sensible default.

## Files Not To Edit
- `client/src/api/generated/index.ts` β€” auto-generated; never edit manually.

## Acceptance Criteria
- [ ] `client/src/api/client/index.ts` imports all API types exclusively from `../generated`.
- [ ] No type definition in `client/src/api/client/index.ts` duplicates one from `generated/index.ts`.
- [ ] `getScaleFromRoot` accepts an optional `scaleType` parameter (default `"Major"` to preserve existing behaviour).
- [ ] `npm run generate:api` (when backend is running) does not touch `client/src/api/client/index.ts`.
- [ ] `npm run lint` passes with `--max-warnings=0`.
- [ ] `npm run build` succeeds with no TypeScript errors.

## Verification Commands
```bash
cd client
npm run lint
npm run build
```
68 changes: 68 additions & 0 deletions issues/epic-7-tech-debt/ISSUE-E7-06.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# ISSUE-E7-06 β€” Replace Magic Numbers with Named Constants

## Objective
Replace all unattributed numeric and color literals in component and utility files with named constants that document their purpose.

## Background
Several components and utilities contain literal values whose meaning is not self-evident. These make the code harder to understand and create silent maintenance hazards (e.g., changing one instance without updating the others).

**Identified magic numbers:**

| Location | Value | Meaning |
|---|---|---|
| `client/src/app/App.tsx` line ~8 | `1200` | Default chord duration in milliseconds |
| `client/src/features/current-chord/components/CurrentChordPanel.tsx` line ~123 | `1500` | Duration of "Copied!" feedback badge in milliseconds |
| `client/src/features/chromatic-circle/components/NoteNode.tsx` line ~22 | `"#10b981"` | Hard-coded emerald highlight color |
| `client/src/features/chord-intervals/components/IntervalLabel.tsx` line ~16 | `"#1F2937"` | Hard-coded dark text color |
| `client/src/features/color-language/utils/chordColorUtils.ts` lines ~23–24 | `0x11`, `0x18`, `0x27` | RGB byte values of the dark text color used in contrast checks |
| `client/src/features/color-language/utils/chordColorUtils.ts` line ~33 | `0.04045`, `12.92`, `0.055`, `1.055`, `2.4` | sRGB linearisation constants (gamma correction) |
| `client/src/shared/ChromaticCircle.tsx` line ~7 | `300` | SVG viewBox size (already a constant in the main component β€” see E7-04) |

## Proposed Constants

```ts
// App.tsx or a new client/src/app/constants.ts
export const DEFAULT_CHORD_DURATION_MS = 1200;

// CurrentChordPanel or shared/constants
export const COPY_FEEDBACK_DURATION_MS = 1500;

// chordColorUtils.ts (inline, documented)
const DARK_TEXT_R = 0x11;
const DARK_TEXT_G = 0x18;
const DARK_TEXT_B = 0x27;

// chordColorUtils.ts (inline, documented)
// sRGB IEC 61966-2-1 linearisation coefficients
const SRGB_LINEAR_THRESHOLD = 0.04045;
const SRGB_LINEAR_DIVISOR = 12.92;
const SRGB_GAMMA_OFFSET = 0.055;
const SRGB_GAMMA_DIVISOR = 1.055;
const SRGB_GAMMA_EXPONENT = 2.4;
```

Inline color literals (`"#10b981"`, `"#1F2937"`) should either reference the existing `chordQualityColors` constants or be moved to `visualConstants.ts`.

## Files To Edit
- `client/src/app/App.tsx` β€” extract `1200` to a named constant.
- `client/src/features/current-chord/components/CurrentChordPanel.tsx` β€” extract `1500` to a named constant.
- `client/src/features/color-language/utils/chordColorUtils.ts` β€” name RGB bytes and sRGB constants.
- `client/src/features/chromatic-circle/components/NoteNode.tsx` β€” replace `"#10b981"` with a constant or CSS variable.
- `client/src/features/chord-intervals/components/IntervalLabel.tsx` β€” replace `"#1F2937"` with a constant.

## Files To Add
None required; constants may be co-located with the files that use them.

## Acceptance Criteria
- [ ] None of the literal values listed in the table above appear unattributed in source files.
- [ ] Each constant has a name that conveys its purpose.
- [ ] Colour constants in `chordColorUtils.ts` have inline comments citing their standard (sRGB IEC 61966-2-1).
- [ ] `npm run lint` passes with `--max-warnings=0`.
- [ ] `npm run build` succeeds with no TypeScript errors.

## Verification Commands
```bash
cd client
npm run lint
npm run build
```
50 changes: 50 additions & 0 deletions issues/epic-7-tech-debt/ISSUE-E7-07.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# ISSUE-E7-07 β€” Add React Error Boundary for Graceful Crash Recovery

## Objective
Wrap the application's component tree in a React Error Boundary so that unhandled runtime errors produce a visible, recoverable error UI instead of a blank white screen.

## Background
The app currently has no `ErrorBoundary` component. Any unhandled exception thrown during rendering, or in a `useEffect` / lifecycle method, silently replaces the entire viewport with nothing. Users have no indication of what went wrong and no path to recovery.

React Error Boundaries catch rendering and lifecycle errors from any descendant component and render a fallback UI. They do not catch errors inside event handlers or asynchronous code (those need separate try/catch), but they protect against the most common "white screen of death" scenarios.

## Proposed Design

### `AppErrorBoundary` component
```tsx
// client/src/app/components/AppErrorBoundary.tsx
// Class component β€” required by React Error Boundary API
```

The fallback UI should:
- Display a brief, user-friendly error message (e.g., "Something went wrong.")
- Provide a "Reload" button that calls `window.location.reload()`.
- Optionally render the error message in development mode (`import.meta.env.DEV`).

### Integration in `App.tsx`
```tsx
<AppErrorBoundary>
{/* existing app tree */}
</AppErrorBoundary>
```

## Files To Add
- `client/src/app/components/AppErrorBoundary.tsx`

## Files To Edit
- `client/src/app/App.tsx` β€” wrap the JSX tree with `<AppErrorBoundary>`.

## Acceptance Criteria
- [ ] `AppErrorBoundary` is a React class component implementing `componentDidCatch` and `getDerivedStateFromError`.
- [ ] The fallback UI is visible and includes a reload affordance.
- [ ] In development mode (`import.meta.env.DEV`), the error detail is also rendered.
- [ ] The entire app tree in `App.tsx` is wrapped by `AppErrorBoundary`.
- [ ] `npm run lint` passes with `--max-warnings=0`.
- [ ] `npm run build` succeeds with no TypeScript errors.

## Verification Commands
```bash
cd client
npm run lint
npm run build
```
Loading