Repository navigation
feat(music): surface recommendationReason in nowplaying and queue - #1864
LucasSantana-Dev merged 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
No issues found across 8 files
Auto-approved: Display-only metadata plumbing: forwards existing recommendationReason to nowplaying, queue, and SSE state. No rollout, auth, or data change.
Re-trigger cubic
f53f13a to
0b90982
Compare
Kimi review (
|
|
Addressed in 68bc06f. recommendationReason now lives on the shared TrackMetadata type and trackToData uses a Track projection. discord-player metadata is validated at the untyped queue boundary, so the production and test casts are gone. The focused embed tests pass. The branch is up to date with main. |
There was a problem hiding this comment.
Correcting an earlier version of this review: I'd framed the mappers.ts metadata read as an unvalidated trust boundary and implied the embed field needed a hard length guard. Having traced where recommendationReason actually comes from, both are weaker than I made them sound. Details below, but neither is a blocker. Apologies for the initial overstatement.
This is good work. Splitting trackToData (typed TrackMetadata) from playerTrackToData (unknown metadata, validated) is the right separation, and it let you delete a pile of as never casts from the specs, which is a real improvement to those tests. rejects invalid recommendation metadata from discord-player is exactly the test I'd want to see for the boundary that genuinely is one.
Two nits, both optional:
Consistency between the two read paths. playerTrackToData validates because discord-player's metadata is unknown. mapTrack reads the same field but just declares the shape it expects:
interface RawTrack {
metadata?: { isAutoplay?: boolean; recommendationReason?: string } | null
}
const reason = track.metadata?.recommendationReasonIn practice this is fine: the value is written by our own markAsAutoplayTrack (packages/bot/src/utils/music/autoplay/queueMarkers.ts:28) from serializeBasis, which always returns a string. So it isn't the untrusted input I first called it. It's still an interface asserting a shape over data typed unknown upstream, and the two paths now disagree about whether that needs checking. A typeof reason === 'string' here would make them agree cheaply. Your call whether it's worth it.
Field length. serializeBasis builds sourceLabel • signal • signal... from basis.signals, and some of those signals come back from Last.fm and Spotify, so the length isn't bounded by anything we control. Discord rejects embed field values over 1024 characters and the whole embed over 6000, and the failure mode is the entire now-playing embed failing to send rather than one field being dropped. Realistically these strings are short and I'd be surprised if it ever bit. But it's the first genuinely open-ended field in that embed (the others are durations and source names), so a truncate wouldn't be paranoid.
Merge order: this touches pages/Music.tsx and QueueList.tsx, which #1866 rewrites, and Music.tsx is also in #1867. I'll sequence the three rather than letting them collide.
On the red checks: not yours. Security was an unpassable repo-wide gate, kimi-review fails on every PR because an API key isn't set (#1877), and the Mutation/Build failures are a stale lockfile on main. Fixed in #1876; rebase once it lands.
|
Heads-up: the CI blockers I mentioned are fixed on What that clears:
Please rebase onto The review feedback above is separate and still stands. |
da5862f to
1fcb9b3
Compare
|
Addressed in 11e0a4f (squashed, rebased onto main).
|
1fcb9b3 to
11e0a4f
Compare
LucasSantana-Dev
left a comment
There was a problem hiding this comment.
Review: changes-required
Feature plumbing is correct and includes a genuine hero bug fix, but this PR breaks nowplaying.spec.ts.
P1 — broken test mock: packages/bot/src/functions/music/commands/songinfo.ts:30 switches from trackToData to playerTrackToData, but nowplaying.spec.ts (whose execute delegates to songinfo via nowplaying.ts:10) is not updated. Its jest.mock('../../../utils/general/responseEmbeds') factory exports only trackToData and buildTrackEmbed, so playerTrackToData is undefined inside the mocked module. The two tests that reach the mapper reject with TypeError: playerTrackToData is not a function, and line 92's trackToDataMock assertion no longer holds. Verified the suite is green on main today (5/5), so this PR turns it red and blocks the bot CI gate. Fix: export playerTrackToData from the mock factory and point the assertion at it (or assert on buildTrackEmbed only).
P3 (nit): buildCommandTrackEmbed now surfaces the "Why this track" field on skip/pause/seek/replay/previous/skipto embeds too, not just /nowplaying and /songinfo as the description states. Almost certainly desirable; worth a one-line note in the PR body.
What's good: the Music.tsx:160 hero fix is real — state.tracks[0] excludes the current track (mappers.ts:82-84), so the hero was displaying the next track under "Now Playing". The 1024-char embed field cap defends against Discord 400s on pathological Last.fm/Spotify reasons, and normalizeTrackMetadata validates the unknown metadata at the trust boundary with a dedicated test.
Plumb recommendationReason through shared types, now-playing, and the queue list. Validate string metadata at the discord-player boundary and in mapTrack. Truncate the Why this track embed field to Discord's 1024 char cap.
11e0a4f to
a4f28ea
Compare
|
Addressed the changes-required review. P1: PR body note:
Rebased onto main. |
## Summary Two structural failures block every fork PR's required checks (seen on #1863, #1864, #1865, #1866, #1867, #1674 after their CI was approved): - **SonarCloud Scan (required) hard-fails on forks**: fork PRs get no secrets, so `SONAR_TOKEN` is never present and the token-policy step exits 1. Now the sonar job is skipped for fork PRs (a skipped required check counts as passing). Same pattern deploy-staging already uses. - **danger 403s on forks**: `review-tools.yml` ran on `pull_request`, where the fork token is forced read-only and the comment POST fails with 403. Switched to `pull_request_target`; the reusable workflow checks out and executes base-repo code only (documented in the file header, same safety rule as the other target workflows). ## Test plan - [x] actionlint clean on both files - [ ] Next push to an external contributor PR: SonarCloud Scan shows skipped, danger posts its comment After merge I will update the seven open contributor branches to main so they pick this up. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Unblocks fork PRs by fixing CI gates for SonarCloud and `danger`. Fork PRs now pass required checks without secrets and get review comments. - Bug Fixes - Skip SonarCloud Scan on fork PRs to avoid failing when `SONAR_TOKEN` is unavailable (skipped required check counts as passing). - Run review tools on `pull_request_target` so `danger` can comment on forks; workflow executes base-repo code only. <sup>Written for commit 316f567. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1898?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
All findings addressed and verified in the follow-up commits. Thanks for the quick turnaround.
|
A note from the maintainer side: sorry this PR waited as long as it did for a proper review, and sorry for the rounds of branch updates and re-running checks today. The churn was on our side, not yours. Also thank you for the quick turnaround on the review feedback: every finding was addressed correctly, and I have dismissed the earlier change request. Your PRs exposed real gaps in how this repo handled external contributions: CI runs sat in a silent approval queue, some gates could never pass on fork PRs (SonarCloud, danger), and the team had no notification when external PRs arrived. Those are all fixed as of today:
Your branch is up to date and the full suite is green. Thanks for the patience and for the contribution. External contributors are very welcome here. |
🤖 I have created a release *beep* *boop* --- <details><summary>2.38.0</summary> ## [2.38.0](v2.37.3...v2.38.0) (2026-07-27) ### Features * **bot:** add /ticket-setup for support category and agent role ([#1863](#1863)) ([3f4af39](3f4af39)) * **frontend:** per-action loading and connection gating on music controls ([#1866](#1866)) ([2dda60f](2dda60f)) * **frontend:** show stale progress when music SSE lags ([#1867](#1867)) ([4952e73](4952e73)) * **music:** surface recommendationReason in nowplaying and queue ([#1864](#1864)) ([960fd62](960fd62)) * **ops:** blue/green zero-downtime deploys — Phase 1 web tier ([#1786](#1786)) ([f5f7597](f5f7597)) ### Bug Fixes * **docker:** make compose stack boot from a fresh .env ([#1674](#1674)) ([babe0ef](babe0ef)) * **docker:** treat an empty db password as missing in compose guards ([#1881](#1881)) ([718c0ad](718c0ad)) * **frontend:** make landing page usable at mobile widths ([#1865](#1865)) ([6190350](6190350)) * **frontend:** stop hero grid columns overflowing on narrow viewports ([#1874](#1874)) ([ce5cea0](ce5cea0)) * **invite:** add /invite where cloudflare pages reads it ([#1895](#1895)) ([0528f66](0528f66)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
recommendationReasonis already set on autoplay-picked tracks and shown in/queue, but it never reached/nowplayingor the dashboard queue.Fix
trackToData/buildTrackEmbed: forward the reason and show a "Why this track" field on/nowplaying(and/songinfo).mapTrack(web music SSE state): includerecommendationReasonso the dashboard receives it.TrackInfo+QueueList: render the reason under the author when present.state.currentTrackovertracks[0], and show the reason when present.Verification
Checklist
Destructive / irreversible interaction (Tier A)
Not applicable. Display-only metadata plumbing.
Feature-removal sweep
Not applicable.
Fixes #1770
Summary by cubic
Surface autoplay
recommendationReasonacross bot embeds, SSE state, and the dashboard so users see “Why this track” in /nowplaying, /songinfo, and the queue. Prefer the livecurrentTrackin the hero, and cap long reasons to avoid Discord embed overflows. Fixes #1770.New Features
playerTrackToData/trackToDataandbuildTrackEmbed.recommendationReasonin web music SSE (mapTrack) and render it in QueueList and the now-playing hero.TrackMetadataand frontendTrackInfotypes.Bug Fixes
state.currentTrackovertracks[0].recommendationReasonat thediscord-playerboundary and inmapTrack; ignore invalid metadata.Written for commit 802b0dd. Summary will update on new commits.