Rewrite plugin in TypeScript with rendering performance optimizations#1
Rewrite plugin in TypeScript with rendering performance optimizations#1kelchm wants to merge 2 commits into
Conversation
- Migrate from JavaScript to TypeScript with strict mode - Cache processed album art buffer (only re-process on track change) - Cache text SVG (only rebuild when track name/artist changes) - Render only the needed 200x100 dial half instead of full 400x100 then crop - Inline progress bar into text area SVG layer (eliminate extra composite) - Throttle media event rendering to max once per 500ms - Skip keypad re-renders when track info hasn't changed - Use JPEG encoding (vs PNG) for LCD output - smaller payloads, faster encode - Use Promise.all for parallel action updates - Replace file-based debug logging with SDK logger - Use class-based SingletonAction registration (proper SDK pattern) - Track LCD positions via Map instead of injecting properties on action objects - Replace build.js file-copy with tsc - Remove test-renderer.js (was a manual smoke test) https://claude.ai/code/session_01X7DW5Xkw2PhRTd3tabdrcj
📝 WalkthroughWalkthroughReplaced JavaScript implementations and build script with TypeScript equivalents: removed Changes
Sequence Diagram(s)sequenceDiagram
participant StreamDeck as Stream Deck
participant Plugin as plugin.ts
participant Scheduler as Render Scheduler
participant NowPlaying as NowPlaying Service
participant Renderer as renderer.ts
participant Actions as Keypad/LCD Actions
NowPlaying->>Plugin: emit nowPlayingEvent(trackInfo)
Plugin->>Plugin: update currentTrackInfo
Plugin->>Scheduler: scheduleRender()
alt Render already scheduled?
Scheduler->>Scheduler: set queuedUpdate flag
else First schedule
Scheduler->>Scheduler: wait RENDER_THROTTLE_MS
end
Note over Scheduler: Timer fires
Scheduler->>Actions: iterate keypadContexts
Actions->>Renderer: render(trackInfo, "left")
Renderer-->>Actions: base64 image
Actions->>StreamDeck: updateTitle/updateImage
Scheduler->>Actions: iterate lcdContexts
Actions->>Renderer: render(trackInfo, "right")
Renderer-->>Actions: base64 image
Actions->>StreamDeck: updateImage(position)
alt queuedUpdate flag set?
Scheduler->>Scheduler: schedule another render
end
User->>StreamDeck: press keypad / rotate dial
StreamDeck->>Actions: keydown / dialRotate event
Actions->>NowPlaying: playPause() / seekTo(offset)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR migrates the Stream Deck Now Playing plugin from JavaScript to strict TypeScript while introducing several rendering and update-path performance optimizations.
Changes:
- Rewrites the plugin and renderer in TypeScript and switches the build to
tscoutputting tobin/. - Adds caching/throttling to reduce redundant keypad/LCD updates and optimize rendering work.
- Updates output encoding and simplifies composition steps to reduce payload/processing overhead.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds strict TypeScript compiler configuration targeting ES2022 and emitting to bin/. |
| src/renderer.ts | New Sharp-based renderer with caching, half-width dial rendering, and JPEG output. |
| src/plugin.ts | New TypeScript plugin implementation using SingletonAction pattern, throttled renders, and cached keypad updates. |
| package.json | Switches build to tsc, adds clean/rebuild scripts, and adds TS/@types dev dependencies. |
| package-lock.json | Locks added TypeScript and Node typings dependencies. |
| src/renderer.js | Removes legacy JavaScript renderer implementation. |
| src/plugin.js | Removes legacy JavaScript plugin implementation and file-based debug logging. |
| build.js | Removes file-copy build script in favor of tsc. |
| test-renderer.js | Removes manual renderer smoke test script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const progressWidth = | ||
| duration && duration > 0 && currentTime != null | ||
| ? Math.round((currentTime / duration) * BAR_WIDTH) | ||
| : 0; |
There was a problem hiding this comment.
progressWidth can exceed BAR_WIDTH when currentTime > duration (or be NaN if inputs are unexpected), which will draw the fill rect beyond the background bar. Clamp the computed width to [0, BAR_WIDTH] before emitting the fill rectangle to keep rendering stable.
| const progressWidth = | |
| duration && duration > 0 && currentTime != null | |
| ? Math.round((currentTime / duration) * BAR_WIDTH) | |
| : 0; | |
| let progressWidth = 0; | |
| if (duration && duration > 0 && currentTime != null) { | |
| const rawWidth = Math.round((currentTime / duration) * BAR_WIDTH); | |
| progressWidth = Number.isFinite(rawWidth) | |
| ? Math.min(Math.max(rawWidth, 0), BAR_WIDTH) | |
| : 0; | |
| } |
| // Left dial: album art (100x100) + beginning of text area | ||
| const artBuffer = await processArtwork(trackInfo.thumbnail); | ||
| if (artBuffer) { | ||
| composites.push({ input: artBuffer, left: 0, top: 0 }); | ||
| } | ||
|
|
||
| // Text + progress bar, offset to start after artwork | ||
| const title = trackInfo.trackName || "Unknown Track"; | ||
| const artist = artistString(trackInfo.artist); | ||
| const textSvg = getTextSvg(title, artist); | ||
| composites.push({ input: Buffer.from(textSvg), left: TEXT_X, top: 0 }); | ||
|
|
||
| const progressSvg = buildProgressBarSvg( | ||
| trackInfo.trackDuration, | ||
| trackInfo.trackProgress, | ||
| ); | ||
| composites.push({ | ||
| input: Buffer.from(progressSvg), | ||
| left: TEXT_X, | ||
| top: 0, | ||
| }); | ||
| } else { | ||
| // Right dial: continuation of text area (already handled by left in the | ||
| // two-dial layout). Show a simplified view with just track info. |
There was a problem hiding this comment.
For the left dial, the text/progress SVG (width TEXT_AREA_WIDTH = 192) is composited at left: TEXT_X (108) onto a 200px-wide canvas, so only ~92px of the text area is visible and most track text will be clipped. If the intent is “left dial = album art, right dial = track info” (per manifest tooltip), consider not rendering text on the left dial and instead render the full text/progress only on the right dial (or, if you want a continuous 2-dial layout, render the same full-width text layer but offset it by -DIAL_WIDTH for the right dial).
| // Left dial: album art (100x100) + beginning of text area | |
| const artBuffer = await processArtwork(trackInfo.thumbnail); | |
| if (artBuffer) { | |
| composites.push({ input: artBuffer, left: 0, top: 0 }); | |
| } | |
| // Text + progress bar, offset to start after artwork | |
| const title = trackInfo.trackName || "Unknown Track"; | |
| const artist = artistString(trackInfo.artist); | |
| const textSvg = getTextSvg(title, artist); | |
| composites.push({ input: Buffer.from(textSvg), left: TEXT_X, top: 0 }); | |
| const progressSvg = buildProgressBarSvg( | |
| trackInfo.trackDuration, | |
| trackInfo.trackProgress, | |
| ); | |
| composites.push({ | |
| input: Buffer.from(progressSvg), | |
| left: TEXT_X, | |
| top: 0, | |
| }); | |
| } else { | |
| // Right dial: continuation of text area (already handled by left in the | |
| // two-dial layout). Show a simplified view with just track info. | |
| // Left dial: album art only (per tooltip: album art on left dial) | |
| const artBuffer = await processArtwork(trackInfo.thumbnail); | |
| if (artBuffer) { | |
| composites.push({ input: artBuffer, left: 0, top: 0 }); | |
| } | |
| } else { | |
| // Right dial: full track info (text + progress bar) |
| function updateAllActions(): void { | ||
| // Keypads: skip if nothing visually changed | ||
| const newKeypadKey = keypadCacheKey(currentTrackInfo); | ||
| if (newKeypadKey !== lastKeypadCacheKey) { | ||
| lastKeypadCacheKey = newKeypadKey; | ||
| const keypadUpdates = Array.from(keypadContexts).map((action) => | ||
| updateKeypadAction(action).catch((err) => | ||
| logger.error("Keypad update error:", err), | ||
| ), | ||
| ); | ||
| Promise.all(keypadUpdates); | ||
| } | ||
|
|
||
| // LCDs: always update (progress bar changes) | ||
| const lcdUpdates = Array.from(lcdContexts).map((action) => { | ||
| const position = lcdPositions.get(action.id) ?? "left"; | ||
| return updateLCDAction(action, position).catch((err) => | ||
| logger.error("LCD update error:", err), | ||
| ); | ||
| }); | ||
| Promise.all(lcdUpdates); | ||
| } |
There was a problem hiding this comment.
Promise.all(...) is called but not awaited/returned in updateAllActions(), so it doesn’t provide backpressure and makes it easy to accidentally introduce unhandled rejections later (and concurrent render passes can overlap). Consider making updateAllActions async and awaiting the Promise.all calls (and calling it with void updateAllActions() from scheduleRender), or drop Promise.all and explicitly fire-and-forget with void for clarity.
| "sharp": "^0.34.5" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^25.5.0", |
There was a problem hiding this comment.
The plugin runtime is Node.js 20 (manifest.json Nodejs.Version), but the PR adds @types/node v25.x. Using a types major that’s newer than the runtime can allow APIs in type-checking that don’t exist at runtime. Consider pinning @types/node to a 20.x version (or matching the declared runtime) to keep types aligned with production behavior.
| "@types/node": "^25.5.0", | |
| "@types/node": "^20.0.0", |
| "build": "tsc", | ||
| "clean": "rm -rf bin", | ||
| "rebuild": "npm run clean && npm run build" |
There was a problem hiding this comment.
clean uses rm -rf bin, which won’t work in default Windows shells. Since this repo targets Windows/macOS, consider a cross-platform approach (e.g., rimraf, del-cli, or a small Node script) so contributors can run npm run clean/rebuild everywhere.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/renderer.ts (2)
28-35: Global caches assume single-track rendering scenario.The module-level caches for thumbnail and text SVG work well for the current single-track-at-a-time use case. However, if the plugin ever needs to render different tracks concurrently (e.g., multiple media sources), this would cause cache thrashing. This is fine for now but worth documenting.
Consider adding a brief comment:
// --- Caches --- +// NOTE: Single-track caching assumes one active media source at a time. +// For multi-source support, consider keyed caches (Map) instead. /** Cache key for the last rendered album art thumbnail. */ let cachedThumbnailSrc: string | undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer.ts` around lines 28 - 35, The module-level caches (cachedThumbnailSrc, cachedArtBuffer, cachedTextKey, cachedTextSvg) assume only one track is rendered at a time and can cause cache thrashing if multiple different tracks are rendered concurrently; add a concise comment above these declarations stating this single-track assumption, warn that concurrent/multi-source rendering would require per-track keys or a map-based cache (e.g., Map<string, Buffer|string> keyed by track id or "trackName\0artist"), and mention that changing to per-track caches is the intended mitigation if concurrent rendering is needed in the future.
54-59: Edge case: malformed data URLs could cause issues.The
split(",")[1]approach assumes exactly one comma separating the MIME header from the base64 data. While rare, malformed data URLs could produce unexpected results. Consider a more defensive approach if robustness is a concern.♻️ More defensive extraction
function extractImageBuffer(dataUrl: string): Buffer { if (dataUrl.startsWith("data:")) { - return Buffer.from(dataUrl.split(",")[1], "base64"); + const commaIndex = dataUrl.indexOf(","); + if (commaIndex === -1) { + throw new Error("Invalid data URL: missing comma separator"); + } + return Buffer.from(dataUrl.substring(commaIndex + 1), "base64"); } return Buffer.from(dataUrl, "base64"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer.ts` around lines 54 - 59, The extractImageBuffer function assumes dataUrl.split(",")[1] exists; make it defensive by checking that dataUrl starts with "data:" and contains a comma or a "base64," marker before slicing; if the separator is missing or the base64 payload is empty, throw or return a clear error. Update extractImageBuffer to locate the first comma via indexOf or match a /^data:.*;base64,/ regex, slice the substring after that separator, validate it's non-empty, and then call Buffer.from(payload, "base64") so malformed data URLs don't produce undefined behavior.src/plugin.ts (1)
244-254: Seek direction is based on tick sign, but magnitude is ignored.The dial rotation handler uses a fixed 5-second seek regardless of how fast/far the user rotates the dial. The
ticksvalue indicates rotation magnitude, but only its sign is used. This may feel unresponsive for quick rotations.♻️ Consider scaling seek amount by tick count
override async onDialRotate( ev: DialRotateEvent<LcdSettings>, ): Promise<void> { if (!nowPlayingInstance) return; const ticks = ev.payload.ticks; - const seekMs = ticks > 0 ? 5000 : -5000; + const seekMs = ticks * 2000; // 2 seconds per tick, direction preserved const current = currentTrackInfo?.trackProgress ?? 0; await nowPlayingInstance.seekTo(current + seekMs).catch((err) => { logger.error("Error seeking:", err); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plugin.ts` around lines 244 - 254, The onDialRotate handler currently uses only the sign of ev.payload.ticks and always seeks ±5000ms; change it to scale the seek amount by the tick magnitude (e.g., seekMs = ev.payload.ticks * 5000) so fast rotations jump farther, ensure you handle zero ticks (no-op) and negative ticks correctly, call nowPlayingInstance.seekTo(current + seekMs) as before, and keep the existing catch/logger.error behavior (referencing onDialRotate, DialRotateEvent, ev.payload.ticks, seekMs, nowPlayingInstance.seekTo, and currentTrackInfo).package.json (1)
8-8: Cross-platform compatibility:rm -rfwon't work on Windows.The
cleanscript uses Unix-specificrm -rfwhich will fail on Windows. Consider using a cross-platform solution.♻️ Suggested fix using rimraf or shx
Option 1: Use
rimraf(add as devDependency):- "clean": "rm -rf bin", + "clean": "rimraf bin",Option 2: Use Node.js built-in (no extra dependency):
- "clean": "rm -rf bin", + "clean": "node -e \"fs.rmSync('bin',{recursive:true,force:true})\"",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 8, The "clean" npm script currently uses the Unix-only command "rm -rf" and will fail on Windows; update the package.json "clean" script to a cross-platform approach (e.g., replace the value for the "clean" script with a call to "rimraf bin" after adding rimraf as a devDependency, or use "shx rm -rf bin" with shx as a devDependency, or use a Node.js one-liner that removes the "bin" directory), and ensure the chosen tool (rimraf or shx) is added to devDependencies and package.json scripts updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plugin.ts`:
- Around line 164-185: The updateAllActions function launches keypad and LCD
update promises but does not await the Promise.all calls, which can hide errors
and cause race conditions; make updateAllActions async and await both
Promise.all results (for the keypadUpdates and lcdUpdates), keeping the existing
per-promise .catch handlers or alternatively remove per-promise .catch and let
Promise.all reject so callers can observe failures; locate the logic around
keypadCacheKey/lastKeypadCacheKey, keypadContexts, updateKeypadAction and
lcdContexts/updateLCDAction/lcdPositions and add the awaits there (or add a
clear comment if intentional fire-and-forget behavior is required).
---
Nitpick comments:
In `@package.json`:
- Line 8: The "clean" npm script currently uses the Unix-only command "rm -rf"
and will fail on Windows; update the package.json "clean" script to a
cross-platform approach (e.g., replace the value for the "clean" script with a
call to "rimraf bin" after adding rimraf as a devDependency, or use "shx rm -rf
bin" with shx as a devDependency, or use a Node.js one-liner that removes the
"bin" directory), and ensure the chosen tool (rimraf or shx) is added to
devDependencies and package.json scripts updated accordingly.
In `@src/plugin.ts`:
- Around line 244-254: The onDialRotate handler currently uses only the sign of
ev.payload.ticks and always seeks ±5000ms; change it to scale the seek amount by
the tick magnitude (e.g., seekMs = ev.payload.ticks * 5000) so fast rotations
jump farther, ensure you handle zero ticks (no-op) and negative ticks correctly,
call nowPlayingInstance.seekTo(current + seekMs) as before, and keep the
existing catch/logger.error behavior (referencing onDialRotate, DialRotateEvent,
ev.payload.ticks, seekMs, nowPlayingInstance.seekTo, and currentTrackInfo).
In `@src/renderer.ts`:
- Around line 28-35: The module-level caches (cachedThumbnailSrc,
cachedArtBuffer, cachedTextKey, cachedTextSvg) assume only one track is rendered
at a time and can cause cache thrashing if multiple different tracks are
rendered concurrently; add a concise comment above these declarations stating
this single-track assumption, warn that concurrent/multi-source rendering would
require per-track keys or a map-based cache (e.g., Map<string, Buffer|string>
keyed by track id or "trackName\0artist"), and mention that changing to
per-track caches is the intended mitigation if concurrent rendering is needed in
the future.
- Around line 54-59: The extractImageBuffer function assumes
dataUrl.split(",")[1] exists; make it defensive by checking that dataUrl starts
with "data:" and contains a comma or a "base64," marker before slicing; if the
separator is missing or the base64 payload is empty, throw or return a clear
error. Update extractImageBuffer to locate the first comma via indexOf or match
a /^data:.*;base64,/ regex, slice the substring after that separator, validate
it's non-empty, and then call Buffer.from(payload, "base64") so malformed data
URLs don't produce undefined behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff2f800d-861f-4106-b5dc-5dee2ee5184b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
build.jspackage.jsonsrc/plugin.jssrc/plugin.tssrc/renderer.jssrc/renderer.tstest-renderer.jstsconfig.json
💤 Files with no reviewable changes (4)
- build.js
- test-renderer.js
- src/renderer.js
- src/plugin.js
| function updateAllActions(): void { | ||
| // Keypads: skip if nothing visually changed | ||
| const newKeypadKey = keypadCacheKey(currentTrackInfo); | ||
| if (newKeypadKey !== lastKeypadCacheKey) { | ||
| lastKeypadCacheKey = newKeypadKey; | ||
| const keypadUpdates = Array.from(keypadContexts).map((action) => | ||
| updateKeypadAction(action).catch((err) => | ||
| logger.error("Keypad update error:", err), | ||
| ), | ||
| ); | ||
| Promise.all(keypadUpdates); | ||
| } | ||
|
|
||
| // LCDs: always update (progress bar changes) | ||
| const lcdUpdates = Array.from(lcdContexts).map((action) => { | ||
| const position = lcdPositions.get(action.id) ?? "left"; | ||
| return updateLCDAction(action, position).catch((err) => | ||
| logger.error("LCD update error:", err), | ||
| ); | ||
| }); | ||
| Promise.all(lcdUpdates); | ||
| } |
There was a problem hiding this comment.
Missing await on Promise.all calls may hide errors and cause timing issues.
The Promise.all calls on lines 174 and 184 are not awaited, meaning updateAllActions returns immediately while updates run in the background. While the individual promises have .catch() handlers, the function signature and behavior suggest these should complete before returning. This could cause subtle race conditions if updateAllActions is called rapidly.
🔧 Proposed fix to await the promises
-function updateAllActions(): void {
+async function updateAllActions(): Promise<void> {
// Keypads: skip if nothing visually changed
const newKeypadKey = keypadCacheKey(currentTrackInfo);
if (newKeypadKey !== lastKeypadCacheKey) {
lastKeypadCacheKey = newKeypadKey;
const keypadUpdates = Array.from(keypadContexts).map((action) =>
updateKeypadAction(action).catch((err) =>
logger.error("Keypad update error:", err),
),
);
- Promise.all(keypadUpdates);
+ await Promise.all(keypadUpdates);
}
// LCDs: always update (progress bar changes)
const lcdUpdates = Array.from(lcdContexts).map((action) => {
const position = lcdPositions.get(action.id) ?? "left";
return updateLCDAction(action, position).catch((err) =>
logger.error("LCD update error:", err),
);
});
- Promise.all(lcdUpdates);
+ await Promise.all(lcdUpdates);
}Note: If the fire-and-forget behavior is intentional for performance reasons (to not block the throttle timer), consider adding a comment explaining this design choice and ensure the .catch() handlers are sufficient for error handling.
📝 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.
| function updateAllActions(): void { | |
| // Keypads: skip if nothing visually changed | |
| const newKeypadKey = keypadCacheKey(currentTrackInfo); | |
| if (newKeypadKey !== lastKeypadCacheKey) { | |
| lastKeypadCacheKey = newKeypadKey; | |
| const keypadUpdates = Array.from(keypadContexts).map((action) => | |
| updateKeypadAction(action).catch((err) => | |
| logger.error("Keypad update error:", err), | |
| ), | |
| ); | |
| Promise.all(keypadUpdates); | |
| } | |
| // LCDs: always update (progress bar changes) | |
| const lcdUpdates = Array.from(lcdContexts).map((action) => { | |
| const position = lcdPositions.get(action.id) ?? "left"; | |
| return updateLCDAction(action, position).catch((err) => | |
| logger.error("LCD update error:", err), | |
| ); | |
| }); | |
| Promise.all(lcdUpdates); | |
| } | |
| async function updateAllActions(): Promise<void> { | |
| // Keypads: skip if nothing visually changed | |
| const newKeypadKey = keypadCacheKey(currentTrackInfo); | |
| if (newKeypadKey !== lastKeypadCacheKey) { | |
| lastKeypadCacheKey = newKeypadKey; | |
| const keypadUpdates = Array.from(keypadContexts).map((action) => | |
| updateKeypadAction(action).catch((err) => | |
| logger.error("Keypad update error:", err), | |
| ), | |
| ); | |
| await Promise.all(keypadUpdates); | |
| } | |
| // LCDs: always update (progress bar changes) | |
| const lcdUpdates = Array.from(lcdContexts).map((action) => { | |
| const position = lcdPositions.get(action.id) ?? "left"; | |
| return updateLCDAction(action, position).catch((err) => | |
| logger.error("LCD update error:", err), | |
| ); | |
| }); | |
| await Promise.all(lcdUpdates); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/plugin.ts` around lines 164 - 185, The updateAllActions function launches
keypad and LCD update promises but does not await the Promise.all calls, which
can hide errors and cause race conditions; make updateAllActions async and await
both Promise.all results (for the keypadUpdates and lcdUpdates), keeping the
existing per-promise .catch handlers or alternatively remove per-promise .catch
and let Promise.all reject so callers can observe failures; locate the logic
around keypadCacheKey/lastKeypadCacheKey, keypadContexts, updateKeypadAction and
lcdContexts/updateLCDAction/lcdPositions and add the awaits there (or add a
clear comment if intentional fire-and-forget behavior is required).
500ms made the progress bar noticeably choppy. 250ms keeps the render rate reasonable while maintaining smooth visual updates. https://claude.ai/code/session_01X7DW5Xkw2PhRTd3tabdrcj
Summary by CodeRabbit
New Features
Chores