Open
Conversation
|
| Filename | Overview |
|---|---|
| apps/server/src/covers.ts | Core JXL/WebP generation logic. Uses a temp dir with proper finally cleanup, gracefully degrades when cjxl or ImageMagick are unavailable, and correctly handles PNG/non-PNG source formats. Logic is sound. |
| apps/web/src/components/autoplayer/CoverImage.tsx | New <picture> component with correct JXL → WebP → PNG source priority ordering and proper null guard. The as string cast on fallbackSrc is safe given the preceding null check. Clean implementation. |
| apps/web/src/services/cover.ts | Adds createPreviewCover for server-action-side preview generation with temp-dir cleanup. Node.js imports are acceptable here since the file is only called from a TanStack Start server.action. The approach mirrors apps/server/src/covers.ts cleanly. |
| apps/web/src/services/storage.ts | Cover saving switched from raw base64 to SongCover object with data-URL writing. The cover?.pngUrl guard silently skips all cover data if only WebP/JXL URLs are present — safe today but fragile if the PNG-first invariant ever breaks. |
| apps/server/src/external/storage.ts | NFS save updated to copy all three cover formats from local disk. The cover?.pngUrl && !startsWith("data:") outer guard means JXL/WebP files are silently skipped if pngUrl is absent, even when those files exist on disk. |
| apps/server/src/worker/song-worker.ts | Correctly updated to use the new async saveCover and SongCover object. The PNG disk-read block in saveAndFinalize is now redundant for the normal code path (file copy is used instead), causing a wasted I/O on each save. |
| apps/web/src/hooks/useRoomConnection.ts | Protocol mismatch detection added cleanly: stops reconnect loop, closes socket, shows a persistent toast. The protocolErrorRef reset on roomId change ensures a fresh connection attempt when navigating to a different room. |
| packages/shared/src/protocol.ts | Protocol bumped to v2, SongCoverSchema added, coverUrl replaced with cover in SongDataSchema, and PROTOCOL_MISMATCH error code added. All schema changes are consistent and backward-incompatible clients are handled via the new mismatch flow. |
Comments Outside Diff (1)
-
apps/server/src/worker/song-worker.ts, line 1119-1136 (link)Redundant PNG disk read — file is not used by
saveSongToNfsWhen
this.song.cover?.pngUrlis a local file path, this block reads the PNG intocoverBase64ForNfs. However,saveSongToNfswill take thecover?.pngUrl && !startsWith("data:")branch and copy files directly from disk, ignoringcoverPngBase64entirely. The disk read only produces observable value whencover.pngUrlis a data URL (the error-fallback case), but in that case this block is skipped entirely.Consider removing the disk-read block and relying solely on
this.coverBase64(populated at generation time) as the base64 fallback:let coverBase64ForNfs: string | null = this.coverBase64; this.coverBase64 = null; // free memory // Note: saveSongToNfs copies files directly when cover.pngUrl is a local path. // coverPngBase64 is only used when the pngUrl is an inline data URL (error fallback).
This avoids an unnecessary file read on every normal save path.
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/server/src/worker/song-worker.ts Line: 1119-1136 Comment: **Redundant PNG disk read — file is not used by `saveSongToNfs`** When `this.song.cover?.pngUrl` is a local file path, this block reads the PNG into `coverBase64ForNfs`. However, `saveSongToNfs` will take the `cover?.pngUrl && !startsWith("data:")` branch and copy files directly from disk, ignoring `coverPngBase64` entirely. The disk read only produces observable value when `cover.pngUrl` is a data URL (the error-fallback case), but in that case this block is skipped entirely. Consider removing the disk-read block and relying solely on `this.coverBase64` (populated at generation time) as the base64 fallback: ```ts let coverBase64ForNfs: string | null = this.coverBase64; this.coverBase64 = null; // free memory // Note: saveSongToNfs copies files directly when cover.pngUrl is a local path. // coverPngBase64 is only used when the pngUrl is an inline data URL (error fallback). ``` This avoids an unnecessary file read on every normal save path. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/web/src/services/storage.ts
Line: 134-148
Comment:
**PNG-presence guard skips all cover saving**
The `if (cover?.pngUrl)` guard means that if `pngUrl` is `null` but `webpUrl` or `jxlUrl` are set, no cover data is written to NFS at all. While the current `saveCover` implementation guarantees PNG is always generated first (so this state can't arise today), `cover.webpUrl` and `cover.jxlUrl` checks are more future-proof and consistent with the server-side `storage.ts` behaviour.
Consider guarding on any available URL:
```suggestion
if (cover?.pngUrl || cover?.webpUrl || cover?.jxlUrl) {
const coverEntries = [
{ url: cover.pngUrl, output: "cover.png" },
{ url: cover.webpUrl, output: "cover.webp" },
{ url: cover.jxlUrl, output: "cover.jxl" },
];
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/server/src/external/storage.ts
Line: 129-151
Comment:
**PNG-only gate skips JXL/WebP file copy when `pngUrl` is absent**
The outer condition `cover?.pngUrl && !cover.pngUrl.startsWith("data:")` means that if `pngUrl` is `null` (e.g., a future code path that produces only WebP/JXL), none of the derived files are copied to the NFS song directory even though their local paths could be valid. The inner loop already skips null entries safely, so the outer guard could use any URL to decide whether to enter the copy block:
```suggestion
if (
(cover?.pngUrl || cover?.webpUrl || cover?.jxlUrl) &&
![cover?.pngUrl, cover?.webpUrl, cover?.jxlUrl].every((u) =>
u == null || u.startsWith("data:"),
)
) {
```
Alternatively, retaining the `pngUrl`-only guard is acceptable if the PNG-first invariant is treated as a hard requirement, but a comment explaining this assumption would help future maintainers.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/server/src/worker/song-worker.ts
Line: 1119-1136
Comment:
**Redundant PNG disk read — file is not used by `saveSongToNfs`**
When `this.song.cover?.pngUrl` is a local file path, this block reads the PNG into `coverBase64ForNfs`. However, `saveSongToNfs` will take the `cover?.pngUrl && !startsWith("data:")` branch and copy files directly from disk, ignoring `coverPngBase64` entirely. The disk read only produces observable value when `cover.pngUrl` is a data URL (the error-fallback case), but in that case this block is skipped entirely.
Consider removing the disk-read block and relying solely on `this.coverBase64` (populated at generation time) as the base64 fallback:
```ts
let coverBase64ForNfs: string | null = this.coverBase64;
this.coverBase64 = null; // free memory
// Note: saveSongToNfs copies files directly when cover.pngUrl is a local path.
// coverPngBase64 is only used when the pngUrl is an inline data URL (error fallback).
```
This avoids an unnecessary file read on every normal save path.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Merge origin/main into t3code/cover-imag..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
134
to
+148
| // Trim trailing silence from audio | ||
| const trimResult = await trimTrailingSilence(audioFile); | ||
|
|
||
| if (coverBase64) { | ||
| const coverBuffer = Buffer.from(coverBase64, "base64"); | ||
| fs.writeFileSync(path.join(songDir, "cover.png"), coverBuffer); | ||
| if (cover?.pngUrl) { | ||
| const coverEntries = [ | ||
| { url: cover.pngUrl, output: "cover.png" }, | ||
| { url: cover.webpUrl, output: "cover.webp" }, | ||
| { url: cover.jxlUrl, output: "cover.jxl" }, | ||
| ]; | ||
| for (const entry of coverEntries) { | ||
| if (!entry.url) continue; | ||
| if (entry.url.startsWith("data:")) { | ||
| writeDataUrlFile(entry.url, path.join(songDir, entry.output)); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The if (cover?.pngUrl) guard means that if pngUrl is null but webpUrl or jxlUrl are set, no cover data is written to NFS at all. While the current saveCover implementation guarantees PNG is always generated first (so this state can't arise today), cover.webpUrl and cover.jxlUrl checks are more future-proof and consistent with the server-side storage.ts behaviour.
Consider guarding on any available URL:
Suggested change
| // Trim trailing silence from audio | |
| const trimResult = await trimTrailingSilence(audioFile); | |
| if (coverBase64) { | |
| const coverBuffer = Buffer.from(coverBase64, "base64"); | |
| fs.writeFileSync(path.join(songDir, "cover.png"), coverBuffer); | |
| if (cover?.pngUrl) { | |
| const coverEntries = [ | |
| { url: cover.pngUrl, output: "cover.png" }, | |
| { url: cover.webpUrl, output: "cover.webp" }, | |
| { url: cover.jxlUrl, output: "cover.jxl" }, | |
| ]; | |
| for (const entry of coverEntries) { | |
| if (!entry.url) continue; | |
| if (entry.url.startsWith("data:")) { | |
| writeDataUrlFile(entry.url, path.join(songDir, entry.output)); | |
| continue; | |
| } | |
| if (cover?.pngUrl || cover?.webpUrl || cover?.jxlUrl) { | |
| const coverEntries = [ | |
| { url: cover.pngUrl, output: "cover.png" }, | |
| { url: cover.webpUrl, output: "cover.webp" }, | |
| { url: cover.jxlUrl, output: "cover.jxl" }, | |
| ]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/src/services/storage.ts
Line: 134-148
Comment:
**PNG-presence guard skips all cover saving**
The `if (cover?.pngUrl)` guard means that if `pngUrl` is `null` but `webpUrl` or `jxlUrl` are set, no cover data is written to NFS at all. While the current `saveCover` implementation guarantees PNG is always generated first (so this state can't arise today), `cover.webpUrl` and `cover.jxlUrl` checks are more future-proof and consistent with the server-side `storage.ts` behaviour.
Consider guarding on any available URL:
```suggestion
if (cover?.pngUrl || cover?.webpUrl || cover?.jxlUrl) {
const coverEntries = [
{ url: cover.pngUrl, output: "cover.png" },
{ url: cover.webpUrl, output: "cover.webp" },
{ url: cover.jxlUrl, output: "cover.jxl" },
];
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
129
to
151
| // Trim trailing silence from audio | ||
| const trimResult = await trimTrailingSilence(audioFile); | ||
|
|
||
| if (coverBase64) { | ||
| const coverBuffer = Buffer.from(coverBase64, "base64"); | ||
| if (cover?.pngUrl && !cover.pngUrl.startsWith("data:")) { | ||
| const coverFilenames = [ | ||
| { url: cover.pngUrl, output: "cover.png" }, | ||
| { url: cover.webpUrl, output: "cover.webp" }, | ||
| { url: cover.jxlUrl, output: "cover.jxl" }, | ||
| ]; | ||
| for (const entry of coverFilenames) { | ||
| if (!entry.url || entry.url.startsWith("data:")) continue; | ||
| const sourcePath = path.resolve( | ||
| import.meta.dirname, | ||
| "../../../../data/covers", | ||
| path.basename(entry.url), | ||
| ); | ||
| if (fs.existsSync(sourcePath)) { | ||
| fs.copyFileSync(sourcePath, path.join(songDir, entry.output)); | ||
| } | ||
| } | ||
| } else if (coverPngBase64) { | ||
| const coverBuffer = Buffer.from(coverPngBase64, "base64"); | ||
| fs.writeFileSync(path.join(songDir, "cover.png"), coverBuffer); |
There was a problem hiding this comment.
pngUrl is absent
The outer condition cover?.pngUrl && !cover.pngUrl.startsWith("data:") means that if pngUrl is null (e.g., a future code path that produces only WebP/JXL), none of the derived files are copied to the NFS song directory even though their local paths could be valid. The inner loop already skips null entries safely, so the outer guard could use any URL to decide whether to enter the copy block:
Suggested change
| // Trim trailing silence from audio | |
| const trimResult = await trimTrailingSilence(audioFile); | |
| if (coverBase64) { | |
| const coverBuffer = Buffer.from(coverBase64, "base64"); | |
| if (cover?.pngUrl && !cover.pngUrl.startsWith("data:")) { | |
| const coverFilenames = [ | |
| { url: cover.pngUrl, output: "cover.png" }, | |
| { url: cover.webpUrl, output: "cover.webp" }, | |
| { url: cover.jxlUrl, output: "cover.jxl" }, | |
| ]; | |
| for (const entry of coverFilenames) { | |
| if (!entry.url || entry.url.startsWith("data:")) continue; | |
| const sourcePath = path.resolve( | |
| import.meta.dirname, | |
| "../../../../data/covers", | |
| path.basename(entry.url), | |
| ); | |
| if (fs.existsSync(sourcePath)) { | |
| fs.copyFileSync(sourcePath, path.join(songDir, entry.output)); | |
| } | |
| } | |
| } else if (coverPngBase64) { | |
| const coverBuffer = Buffer.from(coverPngBase64, "base64"); | |
| fs.writeFileSync(path.join(songDir, "cover.png"), coverBuffer); | |
| if ( | |
| (cover?.pngUrl || cover?.webpUrl || cover?.jxlUrl) && | |
| ![cover?.pngUrl, cover?.webpUrl, cover?.jxlUrl].every((u) => | |
| u == null || u.startsWith("data:"), | |
| ) | |
| ) { |
Alternatively, retaining the pngUrl-only guard is acceptable if the PNG-first invariant is treated as a hard requirement, but a comment explaining this assumption would help future maintainers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/external/storage.ts
Line: 129-151
Comment:
**PNG-only gate skips JXL/WebP file copy when `pngUrl` is absent**
The outer condition `cover?.pngUrl && !cover.pngUrl.startsWith("data:")` means that if `pngUrl` is `null` (e.g., a future code path that produces only WebP/JXL), none of the derived files are copied to the NFS song directory even though their local paths could be valid. The inner loop already skips null entries safely, so the outer guard could use any URL to decide whether to enter the copy block:
```suggestion
if (
(cover?.pngUrl || cover?.webpUrl || cover?.jxlUrl) &&
![cover?.pngUrl, cover?.webpUrl, cover?.jxlUrl].every((u) =>
u == null || u.startsWith("data:"),
)
) {
```
Alternatively, retaining the `pngUrl`-only guard is acceptable if the PNG-first invariant is treated as a hard requirement, but a comment explaining this assumption would help future maintainers.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Somewhere a GPU is overheating so I don't have to think.