Added Tone.Js Playback #27
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces audio synthesis capabilities to the application by integrating Tone.js into the notes and grid components. The changes focus on adding interactive audio playback functionality, allowing users to play musical notes through a graphical interface. A new singleton class manages piano sample loading, and the components are updated to support different synthesizer types and provide a more engaging musical experience. Changes
Sequence DiagramsequenceDiagram
participant User
participant NotesPage
participant Synthesizer
participant AudioContext
User->>NotesPage: Click on note
NotesPage->>AudioContext: Start audio context
AudioContext-->>NotesPage: Context ready
NotesPage->>Synthesizer: Play note
Synthesizer-->>User: Produce sound
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/notes/page.tsx (1)
22-31: playNote function
Handling octave formatting and starting the audio context is a neat solution. Consider a way to configure the note duration if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
app/notes/page.tsx(4 hunks)components/Grid.tsx(2 hunks)package.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/Grid.tsx
[error] 9-38: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 16-16: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 16-16: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 90-90: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🔇 Additional comments (21)
components/Grid.tsx (11)
3-3: Imported React, useState, useEffect
This import looks fine and aligns well with the usage below.
8-8: Tone.js import is correct
Bringing in Tone.js viaimport * as Toneis consistent with usage throughout this file.
40-40: SynthType enum-like type
Defining this union type is straightforward and clear.
45-45: Optional synthType in GridProps
This approach is flexible. The default prop value is handled elsewhere, ensuring no runtime issues.
52-52: Default synthType assignment
DestructuringsynthType = "poly"clarifies the default. Good use of TypeScript optional.Also applies to: 53-57
59-63: Instrument and loading states
Storing the current instrument and loading state is well-structured, making the UI more responsive.
111-111: Spacing or blank line
No issue here; the blank line adds clarity.
112-115: Preloading piano
Eager loading of the piano sampler can improve user experience. This is a good usage pattern.
117-145: playNote logic
The approach to compute note/octave is creative. Confirm if the infinite octave increments are desirable or if you might cap at a certain octave. Keep an eye on performance for large iterations.
147-149: Conditional loading message
Displaying a loading message is user-friendly.
270-270: Grid cell styling and click handler
Adding hover and active states in combination withonClickis effective for interactive UI.Also applies to: 276-276
app/notes/page.tsx (7)
1-1: "use client" directive
This ensures proper client-side rendering.
5-5: Tone.js import
Consistently importing Tone.js aligns with usage.
9-10: Synth state
Good approach to manage the synth instance in React state.
11-20: useEffect for synth initialization
Creating and disposing the synth on mount/unmount is correct.
54-55: Clickable note elements
Good addition of cursor and hover styles for improved UX.
71-71: Styling each note
Dynamically styling based on color/textColor is well-handled.Also applies to: 76-76
98-98: Sharps/flats styling
Providing a distinct style for sharps highlights note categories effectively.Also applies to: 103-103
package.json (3)
12-12: Legacy Next.js reference
Still present at line 12. Check if it’s intentionally kept or should be fully removed since your environment may not rely on it.
15-15: tone dependency
Adding"tone": "^15.0.4"is essential for the new playback features.
18-18: Re-adding devDependencies
Confirm that the re-addition is correct. No version changes appear, so it might be a housekeeping reorganization.Also applies to: 24-25, 26-26
| useEffect(() => { | ||
| let newInstrument: typeof instrument = null; | ||
| let mounted = true; | ||
|
|
||
| const setupInstrument = async () => { | ||
| setIsLoading(true); | ||
|
|
||
| switch (synthType) { | ||
| case "piano": | ||
| newInstrument = await PianoSamplerSingleton.getInstance(); | ||
| break; | ||
|
|
||
| case "poly": | ||
| newInstrument = new Tone.PolySynth(Tone.Synth, { | ||
| oscillator: { | ||
| type: "triangle", | ||
| }, | ||
| envelope: { | ||
| attack: 0.02, | ||
| decay: 0.1, | ||
| sustain: 0.3, | ||
| release: 1, | ||
| }, | ||
| }).toDestination(); | ||
| break; | ||
|
|
||
| case "basic": | ||
| default: | ||
| newInstrument = new Tone.Synth().toDestination(); | ||
| break; | ||
| } | ||
|
|
||
| if (mounted) { | ||
| setInstrument(newInstrument); | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| setupInstrument(); | ||
|
|
||
| return () => { | ||
| mounted = false; | ||
| if (newInstrument && synthType !== "piano") { | ||
| newInstrument.dispose(); | ||
| } | ||
| }; | ||
| }, [synthType]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Instrument setup and cleanup
The logic for initializing different instruments and disposing them when unmounted is solid. Consider removing the redundant “case ‘basic’:” if you always fall back to default in the switch statement.
switch (synthType) {
case "piano":
...
break;
case "poly":
...
break;
- case "basic":
- default:
+ default:
newInstrument = new Tone.Synth().toDestination();
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| let newInstrument: typeof instrument = null; | |
| let mounted = true; | |
| const setupInstrument = async () => { | |
| setIsLoading(true); | |
| switch (synthType) { | |
| case "piano": | |
| newInstrument = await PianoSamplerSingleton.getInstance(); | |
| break; | |
| case "poly": | |
| newInstrument = new Tone.PolySynth(Tone.Synth, { | |
| oscillator: { | |
| type: "triangle", | |
| }, | |
| envelope: { | |
| attack: 0.02, | |
| decay: 0.1, | |
| sustain: 0.3, | |
| release: 1, | |
| }, | |
| }).toDestination(); | |
| break; | |
| case "basic": | |
| default: | |
| newInstrument = new Tone.Synth().toDestination(); | |
| break; | |
| } | |
| if (mounted) { | |
| setInstrument(newInstrument); | |
| setIsLoading(false); | |
| } | |
| }; | |
| setupInstrument(); | |
| return () => { | |
| mounted = false; | |
| if (newInstrument && synthType !== "piano") { | |
| newInstrument.dispose(); | |
| } | |
| }; | |
| }, [synthType]); | |
| useEffect(() => { | |
| let newInstrument: typeof instrument = null; | |
| let mounted = true; | |
| const setupInstrument = async () => { | |
| setIsLoading(true); | |
| switch (synthType) { | |
| case "piano": | |
| newInstrument = await PianoSamplerSingleton.getInstance(); | |
| break; | |
| case "poly": | |
| newInstrument = new Tone.PolySynth(Tone.Synth, { | |
| oscillator: { | |
| type: "triangle", | |
| }, | |
| envelope: { | |
| attack: 0.02, | |
| decay: 0.1, | |
| sustain: 0.3, | |
| release: 1, | |
| }, | |
| }).toDestination(); | |
| break; | |
| default: | |
| newInstrument = new Tone.Synth().toDestination(); | |
| break; | |
| } | |
| if (mounted) { | |
| setInstrument(newInstrument); | |
| setIsLoading(false); | |
| } | |
| }; | |
| setupInstrument(); | |
| return () => { | |
| mounted = false; | |
| if (newInstrument && synthType !== "piano") { | |
| newInstrument.dispose(); | |
| } | |
| }; | |
| }, [synthType]); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
| class PianoSamplerSingleton { | ||
| private static instance: Tone.Sampler | null = null; | ||
| private static isLoading = false; | ||
| private static loadPromise: Promise<void> | null = null; | ||
|
|
||
| static async getInstance(): Promise<Tone.Sampler> { | ||
| if (!this.instance && !this.loadPromise) { | ||
| this.isLoading = true; | ||
| this.loadPromise = new Promise((resolve) => { | ||
| const sampler = new Tone.Sampler({ | ||
| urls: { | ||
| C4: "C4.mp3", | ||
| "D#4": "Ds4.mp3", | ||
| "F#4": "Fs4.mp3", | ||
| A4: "A4.mp3", | ||
| }, | ||
| baseUrl: "https://tonejs.github.io/audio/salamander/", | ||
| onload: () => { | ||
| this.instance = sampler; | ||
| this.isLoading = false; | ||
| resolve(); | ||
| }, | ||
| }).toDestination(); | ||
| }); | ||
| } | ||
| await this.loadPromise; | ||
| return this.instance!; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor static-only class to simpler structure
A class with only static members adds overhead and is flagged by static analysis. Consider refactoring to a simple module-level function or object. Also, replace this references with the class name for clarity in static methods.
-class PianoSamplerSingleton {
- private static instance: Tone.Sampler | null = null;
- ...
-}
+let pianoSampler: Tone.Sampler | null = null;
+let isSamplerLoading = false;
+let loadPromise: Promise<void> | null = null;
+
+export async function getPianoSamplerInstance(): Promise<Tone.Sampler> {
+ if (!pianoSampler && !loadPromise) {
+ isSamplerLoading = true;
+ loadPromise = new Promise((resolve) => {
+ const sampler = new Tone.Sampler({ ... }).toDestination();
+ ...
+ });
+ }
+ await loadPromise;
+ return pianoSampler!;
+}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-38: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 16-16: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 16-16: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 17-17: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 29-29: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 35-35: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 36-36: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
Summary by CodeRabbit
Release Notes
New Features
Dependencies
Performance