feat: complete missing UI for implemented store features — clip tags, groove pool, track presets#1617
feat: complete missing UI for implemented store features — clip tags, groove pool, track presets#1617
Conversation
… groove pool, track presets Three features had full store/service implementations but no user-facing UI. This commit adds interactive components for all three: 1. Clip Tag Management: interactive add/remove tags with autocomplete in ClipInspectorPanel (9 new tests) 2. Groove Templates Panel: browse, rename, delete grooves in PianoRoll toolbar (8 new tests) 3. Track Preset Manager: save, browse, apply, delete track presets as a floating panel (9 new tests) Also adds command palette entries for groove deletion and track preset manager. Test results: 7046 passed (+26 new), 0 failures, 0 TS errors, build succeeds. https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
Right-clicking a MIDI clip now shows "Extract Groove..." which captures the clip's timing/velocity pattern into the project's groove pool. Uses default 1/16 grid with 4-beat analysis length. 3 new tests, 7049 total passing, 0 TS errors. https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
There was a problem hiding this comment.
Pull request overview
This PR exposes previously store-only features in the UI by adding interactive clip tag editing, a groove pool panel (with extract-from-clip entry point), and a track preset manager panel, plus command palette integration and unit tests.
Changes:
- Add interactive clip tag add/remove UI with autocomplete in
ClipInspectorPanel. - Add groove extraction to the clip context menu and a new Piano Roll “Groove” panel for browsing/renaming/deleting groove templates.
- Add a new floating Track Preset Manager panel, toggleable from the command palette, with tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/store/uiStore.ts | Adds UI state + setter to control Track Preset Manager visibility. |
| src/services/commandPalette.ts | Registers commands for toggling Track Preset Manager and deleting groove templates. |
| src/components/tracks/TrackPresetManager.tsx | New UI for saving, listing, applying, and deleting track presets. |
| src/components/tracks/tests/TrackPresetManager.test.tsx | Unit tests covering Track Preset Manager behaviors. |
| src/components/timeline/ClipInspectorPanel.tsx | Adds editable tag chips + autocomplete tag input to the clip inspector. |
| src/components/timeline/tests/ClipInspectorPanel.test.tsx | Unit tests for the new clip tag management UI. |
| src/components/timeline/ClipContextMenuContainer.tsx | Wires a new “Extract Groove...” action for MIDI clips. |
| src/components/timeline/ClipContextMenu.tsx | Adds the “Extract Groove...” context menu item. |
| src/components/timeline/tests/extractGrooveContextMenu.test.tsx | Tests visibility and invocation of “Extract Groove...” menu item. |
| src/components/pianoroll/PianoRoll.tsx | Adds a toolbar toggle and renders the groove templates panel. |
| src/components/pianoroll/GrooveTemplatesPanel.tsx | New groove pool panel UI with rename/delete and strength slider. |
| src/components/pianoroll/tests/GrooveTemplatesPanel.test.tsx | Unit tests for groove pool panel list/rename/delete/slider UI. |
| src/components/layout/TrackPresetManagerPanel.tsx | New floating wrapper panel for TrackPresetManager. |
| src/components/layout/AppShell.tsx | Mounts TrackPresetManagerPanel into the app shell. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onChange={(e) => { setValue(e.target.value); setShowSuggestions(true); }} | ||
| onKeyDown={handleKeyDown} | ||
| onBlur={() => setTimeout(() => setShowSuggestions(false), 150)} | ||
| onFocus={() => { if (value.trim()) setShowSuggestions(true); }} |
There was a problem hiding this comment.
onBlur uses a bare setTimeout to hide suggestions. If the inspector closes/unmounts within the 150ms window, this can trigger a state update on an unmounted component (React warning) and can also race with re-focus. Consider storing the timer id in a ref and clearing it in an effect cleanup, or avoid the timeout entirely by preventing the input blur when clicking a suggestion (e.g., handle suggestion onMouseDown / pointerdown to keep focus, then close synchronously).
| const [strength, setStrength] = useState(100); | ||
|
|
||
| return ( | ||
| <div className="flex flex-col h-full"> | ||
| {/* Header */} | ||
| <div className="px-2 py-1.5 border-b border-zinc-700/50"> | ||
| <div className="text-[10px] font-semibold text-zinc-400 uppercase tracking-wider"> | ||
| Groove Pool | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Strength slider */} | ||
| <div className="px-2 py-1.5 border-b border-zinc-700/30 flex items-center gap-2"> | ||
| <label className="text-[9px] text-zinc-500" htmlFor="groove-strength"> | ||
| Strength | ||
| </label> | ||
| <input | ||
| id="groove-strength" | ||
| type="range" | ||
| min={0} | ||
| max={100} | ||
| value={strength} | ||
| onChange={(e) => setStrength(Number(e.target.value))} | ||
| className="flex-1 h-1 accent-zinc-400" | ||
| aria-label="Groove strength" | ||
| /> | ||
| <span className="text-[9px] text-zinc-400 w-7 text-right font-mono"> | ||
| {strength}% | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
The “Strength” slider is currently only stored in local component state and isn’t used to affect any groove application behavior. This makes the control effectively cosmetic and the header comment (“adjust the application strength”) misleading. Either wire this value into whatever code path applies grooves (e.g., store it in UI state / pass to applyGrooveToClip options when applying), or remove/disable the slider until it has an effect.
| onExtractGroove={isMidiClip ? () => { | ||
| onClose(); | ||
| const name = `Groove from ${track.displayName || track.trackName}`; | ||
| extractGrooveFromClip(clip.id, name, { gridBeats: 0.25, lengthBeats: 4 }); | ||
| } : undefined} |
There was a problem hiding this comment.
extractGrooveFromClip is invoked with hard-coded extraction options (gridBeats: 0.25, lengthBeats: 4). These are non-obvious magic numbers and may not match the current piano-roll grid or the clip’s musical length. Consider deriving them from the current piano roll grid setting / clip length (or centralizing them as named constants) so the extracted groove better reflects the user’s context.
| it('shows save preset form with track selector', () => { | ||
| setupProject([]); | ||
| render(<TrackPresetManager />); | ||
| expect(screen.getByPlaceholderText(/preset name/i)).toBeTruthy(); | ||
| expect(screen.getByRole('button', { name: /save preset/i })).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
This test name says the save form renders “with track selector”, but setupProject() only creates a single track and the component only renders the selector when tracks.length > 1. Either adjust the fixture to include 2+ tracks (and assert the <select> is present) or rename the test to match what’s actually being verified.
|
|
||
| // Default to first track | ||
| const effectiveTrackId = selectedTrackId || tracks[0]?.id || ''; | ||
|
|
||
| const handleSave = useCallback(() => { | ||
| const trimmed = name.trim(); | ||
| if (!trimmed || !effectiveTrackId) return; | ||
| saveTrackPreset(effectiveTrackId, trimmed); | ||
| setName(''); | ||
| }, [name, effectiveTrackId, saveTrackPreset]); | ||
|
|
||
| return ( | ||
| <div className="flex items-center gap-1.5 px-2 py-1.5"> | ||
| {tracks.length > 1 && ( | ||
| <select | ||
| value={effectiveTrackId} | ||
| onChange={(e) => setSelectedTrackId(e.target.value)} | ||
| className="text-[10px] bg-zinc-800 border border-zinc-700/50 rounded px-1 py-0.5 text-zinc-300 focus:outline-none focus:border-zinc-500" | ||
| > | ||
| {tracks.map((t) => ( | ||
| <option key={t.id} value={t.id}> | ||
| {t.displayName || t.trackName} | ||
| </option> | ||
| ))} | ||
| </select> | ||
| )} | ||
| <input | ||
| type="text" | ||
| value={name} | ||
| onChange={(e) => setName(e.target.value)} | ||
| onKeyDown={(e) => { if (e.key === 'Enter') handleSave(); }} | ||
| placeholder="Preset name..." | ||
| className="flex-1 px-1.5 py-0.5 text-[10px] bg-zinc-800/50 border border-zinc-700/50 rounded text-zinc-300 placeholder:text-zinc-600 focus:outline-none focus:border-zinc-500/70" | ||
| /> | ||
| <button | ||
| type="button" | ||
| onClick={handleSave} | ||
| className="text-[9px] px-2 py-0.5 rounded bg-zinc-700/50 text-zinc-300 hover:bg-zinc-600/50 transition-colors" | ||
| aria-label="Save preset" | ||
| > | ||
| Save | ||
| </button> |
There was a problem hiding this comment.
saveTrackPreset in the project store throws if the track id is not found. Here effectiveTrackId prefers selectedTrackId even if that id is no longer present in tracks (e.g., user selected a track then that track was deleted). Clicking “Save” in that state would call saveTrackPreset and throw, potentially crashing the panel. Consider validating that effectiveTrackId exists in tracks (otherwise fall back to tracks[0].id) and/or wrapping the save call in a try/catch with user-visible feedback.
| // Default to first track | |
| const effectiveTrackId = selectedTrackId || tracks[0]?.id || ''; | |
| const handleSave = useCallback(() => { | |
| const trimmed = name.trim(); | |
| if (!trimmed || !effectiveTrackId) return; | |
| saveTrackPreset(effectiveTrackId, trimmed); | |
| setName(''); | |
| }, [name, effectiveTrackId, saveTrackPreset]); | |
| return ( | |
| <div className="flex items-center gap-1.5 px-2 py-1.5"> | |
| {tracks.length > 1 && ( | |
| <select | |
| value={effectiveTrackId} | |
| onChange={(e) => setSelectedTrackId(e.target.value)} | |
| className="text-[10px] bg-zinc-800 border border-zinc-700/50 rounded px-1 py-0.5 text-zinc-300 focus:outline-none focus:border-zinc-500" | |
| > | |
| {tracks.map((t) => ( | |
| <option key={t.id} value={t.id}> | |
| {t.displayName || t.trackName} | |
| </option> | |
| ))} | |
| </select> | |
| )} | |
| <input | |
| type="text" | |
| value={name} | |
| onChange={(e) => setName(e.target.value)} | |
| onKeyDown={(e) => { if (e.key === 'Enter') handleSave(); }} | |
| placeholder="Preset name..." | |
| className="flex-1 px-1.5 py-0.5 text-[10px] bg-zinc-800/50 border border-zinc-700/50 rounded text-zinc-300 placeholder:text-zinc-600 focus:outline-none focus:border-zinc-500/70" | |
| /> | |
| <button | |
| type="button" | |
| onClick={handleSave} | |
| className="text-[9px] px-2 py-0.5 rounded bg-zinc-700/50 text-zinc-300 hover:bg-zinc-600/50 transition-colors" | |
| aria-label="Save preset" | |
| > | |
| Save | |
| </button> | |
| const [saveError, setSaveError] = useState(''); | |
| const hasSelectedTrack = tracks.some((track) => track.id === selectedTrackId); | |
| // Prefer the selected track only while it still exists; otherwise fall back | |
| // to the first available track. | |
| const effectiveTrackId = (hasSelectedTrack ? selectedTrackId : tracks[0]?.id) || ''; | |
| const handleSave = useCallback(() => { | |
| const trimmed = name.trim(); | |
| if (!trimmed || !effectiveTrackId) return; | |
| setSaveError(''); | |
| try { | |
| saveTrackPreset(effectiveTrackId, trimmed); | |
| setName(''); | |
| } catch (error) { | |
| setSaveError(error instanceof Error ? error.message : 'Failed to save preset.'); | |
| } | |
| }, [name, effectiveTrackId, saveTrackPreset]); | |
| return ( | |
| <div className="px-2 py-1.5"> | |
| <div className="flex items-center gap-1.5"> | |
| {tracks.length > 1 && ( | |
| <select | |
| value={effectiveTrackId} | |
| onChange={(e) => { | |
| setSelectedTrackId(e.target.value); | |
| setSaveError(''); | |
| }} | |
| className="text-[10px] bg-zinc-800 border border-zinc-700/50 rounded px-1 py-0.5 text-zinc-300 focus:outline-none focus:border-zinc-500" | |
| > | |
| {tracks.map((t) => ( | |
| <option key={t.id} value={t.id}> | |
| {t.displayName || t.trackName} | |
| </option> | |
| ))} | |
| </select> | |
| )} | |
| <input | |
| type="text" | |
| value={name} | |
| onChange={(e) => setName(e.target.value)} | |
| onKeyDown={(e) => { if (e.key === 'Enter') handleSave(); }} | |
| placeholder="Preset name..." | |
| className="flex-1 px-1.5 py-0.5 text-[10px] bg-zinc-800/50 border border-zinc-700/50 rounded text-zinc-300 placeholder:text-zinc-600 focus:outline-none focus:border-zinc-500/70" | |
| /> | |
| <button | |
| type="button" | |
| onClick={handleSave} | |
| className="text-[9px] px-2 py-0.5 rounded bg-zinc-700/50 text-zinc-300 hover:bg-zinc-600/50 transition-colors" | |
| aria-label="Save preset" | |
| > | |
| Save | |
| </button> | |
| </div> | |
| {saveError && ( | |
| <div className="mt-1 text-[9px] text-red-400" role="alert"> | |
| {saveError} | |
| </div> | |
| )} |
1. Extract Groove: replace magic numbers with named constants and derive grid/length from clip's MIDI data and project BPM 2. Track selector test: use 2 tracks so <select> actually renders 3. SavePresetForm: validate stale track ID, add try/catch with error display 4. Groove strength: wire slider to uiStore so applyGrooveToClip can read it 5. Tag autocomplete: replace setTimeout onBlur with onMouseDown preventDefault to prevent state updates on unmounted components https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| setShowUndoHistoryPanel: (v) => set({ showUndoHistoryPanel: v }), | ||
| setShowTrackPresetManager: (v) => set({ showTrackPresetManager: v }), | ||
| setGrooveStrength: (v) => set({ grooveStrength: v }), |
There was a problem hiding this comment.
setGrooveStrength stores the value as-is. Since groove strength is documented/used as a 0–100 percentage (e.g., ApplyGrooveOptions.strength), this setter should clamp to that range to prevent out-of-bounds values from persisted state, tests, or future callers.
| setGrooveStrength: (v) => set({ grooveStrength: v }), | |
| setGrooveStrength: (v) => set({ grooveStrength: Math.max(0, Math.min(100, v)) }), |
| commandPaletteQuery: '', | ||
| recentCommandIds: [], | ||
| showUndoHistoryPanel: false, | ||
| showTrackPresetManager: false, | ||
| grooveStrength: 100, | ||
| historyFocusScope: 'arrangement', |
There was a problem hiding this comment.
New UI state (showTrackPresetManager, grooveStrength) is added to the store defaults, but the zustand persist.partialize config does not include these fields. As a result, the track preset manager visibility and groove strength will always reset on reload unlike other panel open/size preferences; consider explicitly persisting (or intentionally excluding) them for consistent UX.
| return ( | ||
| <div | ||
| className="fixed right-4 top-14 w-[280px] max-w-[calc(100vw-2rem)] max-h-[400px] overflow-hidden rounded-xl border border-white/10 bg-[#141426]/95 shadow-2xl backdrop-blur flex flex-col" | ||
| style={{ zIndex: Z.commandPalette }} |
There was a problem hiding this comment.
This panel uses zIndex: Z.commandPalette (160), which will cause it to render above modal dialogs (Z.modal = 80) and potentially block interactions. Use an appropriate layer such as Z.panel/Z.dropdown so modals remain topmost.
| style={{ zIndex: Z.commandPalette }} | |
| style={{ zIndex: Z.panel }} |
| /** Default grid size for groove extraction (16th note = 0.25 beats). */ | ||
| const DEFAULT_GROOVE_GRID_BEATS = 0.25; | ||
| /** Default groove analysis length in beats (1 bar of 4/4). */ | ||
| const DEFAULT_GROOVE_LENGTH_BEATS = 4; | ||
|
|
There was a problem hiding this comment.
DEFAULT_GROOVE_LENGTH_BEATS is hard-coded to 4 ("1 bar of 4/4"), but projects can have different time signatures. Consider deriving the default bar length from the current project time signature (and timeSignatureMap when present) so groove extraction analyzes one bar consistently in 3/4, 6/8, etc.
| <button | ||
| type="button" | ||
| onClick={onDelete} | ||
| className="text-zinc-600 hover:text-red-400 transition-colors opacity-0 group-hover:opacity-100 text-[11px]" | ||
| aria-label="Delete groove template" | ||
| > | ||
| × | ||
| </button> |
There was a problem hiding this comment.
The delete button is hidden with opacity-0 group-hover:opacity-100, which makes it effectively invisible for keyboard users when tab-focused. Add a focus style (e.g., focus:opacity-100 / group-focus-within:opacity-100) so the control is discoverable without hover.
| <button | ||
| type="button" | ||
| onClick={onApply} | ||
| className="text-[9px] px-1.5 py-0.5 rounded bg-zinc-700/40 text-zinc-300 hover:bg-zinc-600/50 transition-colors opacity-0 group-hover:opacity-100" | ||
| aria-label="Apply preset" | ||
| > | ||
| Apply | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={onDelete} | ||
| className="text-zinc-600 hover:text-red-400 transition-colors opacity-0 group-hover:opacity-100 text-[11px]" | ||
| aria-label="Delete preset" | ||
| > |
There was a problem hiding this comment.
The Apply/Delete buttons are hidden with opacity-0 group-hover:opacity-100, which makes them invisible when reached via keyboard navigation. Add focus-visible/focus styles (e.g., focus:opacity-100 / group-focus-within:opacity-100) so keyboard and assistive-tech users can discover and use these actions.
1. Clamp grooveStrength to 0-100 range in setter 2. TrackPresetManagerPanel: use Z.panel instead of Z.commandPalette 3. Derive groove length from project time signature (not hardcoded 4/4) 4. Add focus:opacity-100 for keyboard accessibility on groove delete button 5. Add focus:opacity-100 for keyboard accessibility on preset apply/delete 6. Persist grooveStrength to localStorage (showTrackPresetManager stays transient, consistent with showUndoHistoryPanel) https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const timeSig = project?.timeSignature ?? 4; | ||
| const clipBeats = clip.duration * (bpm / 60); | ||
| const oneBar = timeSig; // beats per bar (e.g. 3 for 3/4, 4 for 4/4) |
There was a problem hiding this comment.
onExtractGroove computes clipBeats and oneBar assuming constant tempo and that beats-per-bar equals project.timeSignature (numerator). This is incorrect when timeSignatureDenominator !== 4 (e.g. 6/8) and will also be wrong if a tempo map/time-signature map is in use. Consider using the existing tempo/time-signature helpers (e.g. timeToBeat + getTimeSignatureBarLength) to derive bar length / clip length in beats, or at least factor in timeSignatureDenominator when computing oneBar/lengthBeats.
| const timeSig = project?.timeSignature ?? 4; | |
| const clipBeats = clip.duration * (bpm / 60); | |
| const oneBar = timeSig; // beats per bar (e.g. 3 for 3/4, 4 for 4/4) | |
| const timeSigNumerator = project?.timeSignature ?? 4; | |
| const timeSigDenominator = (project as { timeSignatureDenominator?: number } | null)?.timeSignatureDenominator ?? 4; | |
| const clipBeats = clip.duration * (bpm / 60); | |
| const oneBar = timeSigNumerator * (4 / timeSigDenominator); // quarter-note beats per bar (e.g. 3 for 6/8, 4 for 4/4) |
6/8 time → 3 quarter-note beats per bar (not 6). Formula: oneBar = numerator * (4 / denominator) https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className="px-2 py-1.5 border-b border-zinc-700/30 flex items-center gap-2"> | ||
| <label className="text-[9px] text-zinc-500" htmlFor="groove-strength"> | ||
| Strength | ||
| </label> | ||
| <input | ||
| id="groove-strength" | ||
| type="range" | ||
| min={0} | ||
| max={100} | ||
| value={strength} | ||
| onChange={(e) => setStrength(Number(e.target.value))} | ||
| className="flex-1 h-1 accent-zinc-400" | ||
| aria-label="Groove strength" | ||
| /> | ||
| <span className="text-[9px] text-zinc-400 w-7 text-right font-mono"> | ||
| {strength}% | ||
| </span> |
There was a problem hiding this comment.
The groove strength slider updates UI state (grooveStrength) but that value is not used anywhere to affect groove application (e.g., there are no references passing it into applyGrooveToClip(..., { strength })). As-is, the control is user-visible but has no functional impact. Either wire grooveStrength into the code path that applies grooves (so changes affect ApplyGrooveOptions.strength), or remove/disable the slider until an apply UI exists to avoid misleading behavior.
| <div className="px-2 py-1.5 border-b border-zinc-700/30 flex items-center gap-2"> | |
| <label className="text-[9px] text-zinc-500" htmlFor="groove-strength"> | |
| Strength | |
| </label> | |
| <input | |
| id="groove-strength" | |
| type="range" | |
| min={0} | |
| max={100} | |
| value={strength} | |
| onChange={(e) => setStrength(Number(e.target.value))} | |
| className="flex-1 h-1 accent-zinc-400" | |
| aria-label="Groove strength" | |
| /> | |
| <span className="text-[9px] text-zinc-400 w-7 text-right font-mono"> | |
| {strength}% | |
| </span> | |
| <div className="px-2 py-1.5 border-b border-zinc-700/30"> | |
| <div className="flex items-center gap-2"> | |
| <label className="text-[9px] text-zinc-500" htmlFor="groove-strength"> | |
| Strength | |
| </label> | |
| <input | |
| id="groove-strength" | |
| type="range" | |
| min={0} | |
| max={100} | |
| value={strength} | |
| disabled | |
| className="flex-1 h-1 accent-zinc-400 opacity-50 cursor-not-allowed" | |
| aria-label="Groove strength unavailable in this panel" | |
| /> | |
| <span className="text-[9px] text-zinc-400 w-7 text-right font-mono"> | |
| {strength}% | |
| </span> | |
| </div> | |
| <div className="mt-1 text-[9px] text-zinc-500"> | |
| Strength adjustment will be available when applying a groove. | |
| </div> |
| const timeSigDenominator = project?.timeSignatureDenominator ?? 4; | ||
| const clipBeats = clip.duration * (bpm / 60); | ||
| const oneBar = timeSigNumerator * (4 / timeSigDenominator); // quarter-note beats per bar (e.g. 3 for 6/8, 4 for 4/4) | ||
| const lengthBeats = clipBeats >= 1 ? Math.round(clipBeats) : (oneBar || FALLBACK_GROOVE_LENGTH_BEATS); |
There was a problem hiding this comment.
lengthBeats is derived with Math.round(clipBeats), which can produce a value smaller than the actual clip length in beats (e.g., 4.4 → 4). Because groove extraction wraps note positions with note.startBeat % lengthBeats (see extractGroove), using a shorter lengthBeats will wrap later notes into earlier slots and skew the extracted groove. Consider using Math.ceil(clipBeats) (and/or snapping to whole bars) to ensure lengthBeats is never less than the clip’s beat length.
| const lengthBeats = clipBeats >= 1 ? Math.round(clipBeats) : (oneBar || FALLBACK_GROOVE_LENGTH_BEATS); | |
| const lengthBeats = clipBeats >= 1 ? Math.ceil(clipBeats) : (oneBar || FALLBACK_GROOVE_LENGTH_BEATS); |
…eats 1. Add "Apply" button to each groove row that calls applyGrooveToClip with the strength slider value and selected notes (or all notes) 2. Use Math.ceil instead of Math.round for clipBeats to prevent note wrapping when clip length isn't a whole number of beats 3. Add 2 new tests for Apply button rendering and store call https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function collectProjectTags(project: import('../../types/project').Project | null): string[] { | ||
| if (!project) return []; | ||
| const tagSet = new Set<string>(); | ||
| for (const track of project.tracks) { |
There was a problem hiding this comment.
Using an inline import('../../types/project').Project type makes the helper harder to read and inconsistent with the rest of the file (which already uses import type ...). Consider adding import type { Project } from '../../types/project' at the top and using Project | null here instead.
| {tracks.length > 1 && ( | ||
| <select | ||
| value={effectiveTrackId} | ||
| onChange={(e) => { setSelectedTrackId(e.target.value); setSaveError(''); }} |
There was a problem hiding this comment.
The track selector <select> has no associated label (<label>/aria-label/aria-labelledby). For accessibility (screen readers), please provide an accessible name (e.g., add an aria-label like “Track to save preset from”, or render a visually-hidden <label> tied via htmlFor).
| onChange={(e) => { setSelectedTrackId(e.target.value); setSaveError(''); }} | |
| onChange={(e) => { setSelectedTrackId(e.target.value); setSaveError(''); }} | |
| aria-label="Track to save preset from" |
| value={name} | ||
| onChange={(e) => setName(e.target.value)} | ||
| onKeyDown={(e) => { if (e.key === 'Enter') handleSave(); }} | ||
| placeholder="Preset name..." |
There was a problem hiding this comment.
The preset name <input> relies on placeholder text only. For accessibility, please add an accessible name (e.g., aria-label="Preset name" or a <label> linked via htmlFor).
| placeholder="Preset name..." | |
| placeholder="Preset name..." | |
| aria-label="Preset name" |
1. Replace inline import('...').Project with proper import type
2. Add aria-label to track selector <select>
3. Add aria-label to preset name <input>
https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <input | ||
| ref={inputRef} | ||
| type="text" | ||
| value={value} | ||
| onChange={(e) => { setValue(e.target.value); setShowSuggestions(true); }} | ||
| onKeyDown={handleKeyDown} | ||
| onBlur={() => setShowSuggestions(false)} | ||
| onFocus={() => { if (value.trim()) setShowSuggestions(true); }} | ||
| placeholder="Add tag..." | ||
| className="w-full px-1.5 py-0.5 text-[10px] bg-zinc-800/50 border border-zinc-700/50 rounded text-zinc-300 placeholder:text-zinc-600 focus:outline-none focus:border-zinc-500/70" | ||
| /> |
There was a problem hiding this comment.
TagInput’s relies on placeholder text only, so it doesn’t have a reliable accessible name for screen readers. Add an explicit label (e.g., aria-label) or associate it with a visible so the “Add tag” control is discoverable via assistive tech.
| ['track', 'preset', 'manager', 'save', 'template', 'instrument'], | ||
| ['manage track presets', 'track templates', 'save track preset'], | ||
| async () => { | ||
| const { useUIStore } = await import('../store/uiStore'); |
There was a problem hiding this comment.
This file already has a static import { useUIStore } from '../store/uiStore' (used elsewhere), but the new Track Preset Manager command does a redundant dynamic import of uiStore before toggling state. Consider using the existing useUIStore import directly to avoid unnecessary code-splitting overhead and keep command implementations consistent.
| const { useUIStore } = await import('../store/uiStore'); |
| id: overrides.id ?? 'preset-1', | ||
| name: overrides.name ?? 'Warm Pad', | ||
| trackName: 'synth', | ||
| trackType: 'midi', |
There was a problem hiding this comment.
makePreset() sets trackType: 'midi', but TrackPreset.trackType is typed as TrackType (e.g. 'pianoRoll', 'sequencer', etc.) and does not include 'midi' (see src/types/project.ts). Using an invalid value here (even if cast) makes the tests less representative of real data and could hide UI issues around display/formatting of trackType. Update the test fixture to use a valid TrackType value.
| trackType: 'midi', | |
| trackType: 'pianoRoll', |
| const name = `Groove from ${track.displayName || track.trackName}`; | ||
| const gridBeats = clip.midiData?.grid | ||
| ? ({ '1/4': 1, '1/8': 0.5, '1/16': 0.25, '1/32': 0.125 }[clip.midiData.grid] ?? DEFAULT_GROOVE_GRID_BEATS) | ||
| : DEFAULT_GROOVE_GRID_BEATS; |
There was a problem hiding this comment.
The PianoRollGrid → beats mapping is duplicated here inline ({ '1/4': 1, ... }), but the same mapping already exists elsewhere (e.g. src/store/projectStore.ts:9764). Consider centralizing/reusing a shared helper/const to avoid these mappings drifting out of sync if grid options expand in the future.
1. Add aria-label to TagInput for screen reader accessibility
2. Fix makePreset test fixture to use valid TrackType ('pianoRoll')
3. Extract GRID_BEATS_MAP to PianoRollConstants, reuse in context menu
4. Remove redundant dynamic import of useUIStore in command palette
https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| userInstrumentPresets: state.userInstrumentPresets, | ||
| // Video recording settings | ||
| videoRecordingSettings: state.videoRecordingSettings, | ||
| // Groove pool | ||
| grooveStrength: state.grooveStrength, | ||
| }), |
There was a problem hiding this comment.
showTrackPresetManager was added to UIState but it isn’t included in the persisted partialize block (while other panel open/close flags are). This means the Track Preset Manager panel state won’t survive reloads unlike the rest of the UI panels; add showTrackPresetManager: state.showTrackPresetManager to partialize (or explicitly document/justify why it should not be persisted).
| export const GRID_BEATS_MAP: Record<PianoRollGrid, number> = { | ||
| '1/4': 1, | ||
| '1/8': 0.5, | ||
| '1/16': 0.25, | ||
| '1/32': 0.125, | ||
| }; |
There was a problem hiding this comment.
GRID_BEATS_MAP duplicates the existing gridSizeToBeats() logic in the same file, creating two sources of truth for the same mapping. Consider removing the map and reusing gridSizeToBeats (or implement one in terms of the other) to avoid future drift if supported grid values change.
| export const GRID_BEATS_MAP: Record<PianoRollGrid, number> = { | |
| '1/4': 1, | |
| '1/8': 0.5, | |
| '1/16': 0.25, | |
| '1/32': 0.125, | |
| }; | |
| const SUPPORTED_PIANO_ROLL_GRIDS: PianoRollGrid[] = ['1/4', '1/8', '1/16', '1/32']; | |
| export const GRID_BEATS_MAP: Record<PianoRollGrid, number> = Object.fromEntries( | |
| SUPPORTED_PIANO_ROLL_GRIDS.map((grid) => [grid, gridSizeToBeats(grid)]), | |
| ) as Record<PianoRollGrid, number>; |
| function collectProjectTags(project: Project | null): string[] { | ||
| if (!project) return []; | ||
| const tagSet = new Set<string>(); | ||
| for (const track of project.tracks) { | ||
| for (const clip of track.clips) { | ||
| if (clip.tags) clip.tags.forEach((t) => tagSet.add(t)); | ||
| } | ||
| } | ||
| return Array.from(tagSet).sort(); | ||
| } | ||
|
|
||
| function useAllProjectTags(): string[] { | ||
| const project = useProjectStore((s) => s.project); | ||
| return useMemo(() => collectProjectTags(project), [project]); | ||
| } |
There was a problem hiding this comment.
useAllProjectTags() recomputes tags by scanning every track/clip whenever the project object identity changes. Since project changes for many unrelated edits, this makes the Clip Inspector’s tag UI potentially O(total clips) per project update. Consider selecting a narrower slice of state (e.g., just project.tracks), memoizing tags in the store, or maintaining a derived tag index so this work only happens when clip tags actually change.
1. Derive GRID_BEATS_MAP from gridSizeToBeats() to avoid duplicate logic 2. Persist showTrackPresetManager in zustand partialize config 3. Narrow useAllProjectTags selector to project.tracks (not full project) to reduce unnecessary recomputation https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| value={editValue} | ||
| onChange={(e) => setEditValue(e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| onBlur={commitRename} |
There was a problem hiding this comment.
The inline rename shown while editing a groove name doesn’t have an accessible label (no aria-label / aria-labelledby). This makes the rename field hard to identify for screen readers. Add an explicit accessible name (and optionally tie it to the groove name) so the control is discoverable in accessibility trees.
| onBlur={commitRename} | |
| onBlur={commitRename} | |
| aria-label={`Rename groove template ${groove.name}`} |
| }, | ||
| setShowUndoHistoryPanel: (v) => set({ showUndoHistoryPanel: v }), | ||
| setShowTrackPresetManager: (v) => set({ showTrackPresetManager: v }), | ||
| setGrooveStrength: (v) => set({ grooveStrength: Math.max(0, Math.min(100, v)) }), |
There was a problem hiding this comment.
setGrooveStrength clamps to 0–100, but if it’s ever called with a non-finite value (e.g. NaN from parsing / corrupted persisted state), Math.min/Math.max will propagate NaN and the UI can end up persisting NaN% strength. Consider guarding with Number.isFinite (fallback to previous value or default) before clamping.
| setGrooveStrength: (v) => set({ grooveStrength: Math.max(0, Math.min(100, v)) }), | |
| setGrooveStrength: (v) => set((state) => ({ | |
| grooveStrength: Number.isFinite(v) ? Math.max(0, Math.min(100, v)) : state.grooveStrength, | |
| })), |
1. Guard setGrooveStrength against NaN/non-finite values (fallback to current state instead of propagating NaN) 2. Add aria-label to groove rename input for screen reader accessibility https://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7
Summary
Closes #1616
Test plan
npx tsc --noEmit— 0 errorsnpm test— 7049 passed (+29 new), 0 failuresnpx vite build— succeedshttps://claude.ai/code/session_01Cs6KGhDT13gNi3VgWhoKq7