Roadmap 29-33 Discogs review UI#49
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 60 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThis PR adds comprehensive Discogs external metadata integration across artist, release, and track entities. It introduces search → review → apply workflows with compile-aware tracklist handling, multi-role credit support, and full-catalog hydration for workspace navigation. ChangesDiscogs external metadata integration for artists, releases, and tracks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a7fa5010c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .split(',') | ||
| .map((part) => part.trim()) | ||
| .filter(Boolean) | ||
| .map((part) => toCreditRole(part)), |
There was a problem hiding this comment.
When the Discogs track detail draft returns dictionary codes such as mainArtist (the client fixture for /api/external-metadata/discogs/tracks/... uses that shape), applying Credits stores the literal code because this path only trims with toCreditRole instead of resolving against dictionaries.creditRole like the release form does. The resulting track shows a mainArtist pill and hasMainArtistRole no longer recognizes it as the main artist in the UI, so Discogs track updates can leave credits/display state inconsistent until reloaded from the server.
Useful? React with 👍 / 👎.
Comment on lines
+515
to
+516
| .split(',') | ||
| .map((part) => part.trim()) |
There was a problem hiding this comment.
For Discogs roles that contain commas inside qualifiers, such as Written-By [Music, Lyrics], applying a release or tracklist draft splits the single role into Written-By [Music and Lyrics]. The review panel already avoids this with bracket-aware splitting, so users can review one role but persist broken role labels when they apply Artists or Tracklist from Discogs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/catalog/api/catalogEntityMappers.ts (1)
145-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoles array should deduplicate like the primary credit path.
The fallback credit mapping (lines 145-155) builds a
rolesarray but doesn't deduplicate, while the primary path (viatoReleaseArtistCredit→creditRolesFromDto) wraps the mapped roles innew Set(...)to deduplicate. If a credit has duplicate role codes or codes that map to the same label, the two paths will produce inconsistent results.🔧 Recommended fix to deduplicate roles
Replace the inline mapping with a call to the shared helper:
: credits.map((credit) => ({ artistId: credit.contributorArtistId, artist: artistsById.get(credit.contributorArtistId)?.name ?? credit.contributorName, - role: creditRoleLabel(credit.role, dictionaries), - roles: (credit.roles && credit.roles.length > 0 - ? credit.roles - : [credit.role] - ).map((role) => creditRoleLabel(role, dictionaries)), + ...(() => { + const roles = creditRolesFromDto(credit, dictionaries) + return { + role: roles[0] ?? creditRoleLabel(credit.role, dictionaries), + roles, + } + })(), }))Or import and call
creditRolesFromDtodirectly to ensure consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/catalog/api/catalogEntityMappers.ts` around lines 145 - 155, The fallback mapping builds roles without deduplication causing inconsistency with the primary path; replace the inline roles mapping with the shared helper creditRolesFromDto (or call toReleaseArtistCredit) so role labels are deduped the same way—i.e., pass the DTO role list into creditRolesFromDto (or reuse toReleaseArtistCredit logic) and assign its result to roles instead of mapping manually.
🧹 Nitpick comments (3)
src/app/AuthenticatedApp.tsx (1)
594-602: ⚡ Quick winDeduplicate full-catalog route membership to avoid contract drift.
Line 594 redefines the same route set already exported as
manualEntryRoutesinsrc/app/renderWorkspace.tsx(Lines 35-43). Keeping two independent sets can desynchronize hydration gating from workspace behavior. Reusing one source of truth avoids that class of routing bugs.♻️ Proposed refactor
-const fullCatalogRoutes = new Set<AppRoutePath>([ - '/artists', - '/labels', - '/owned-items', - '/playlists', - '/relations', - '/releases', - '/tracks', -]) +const fullCatalogRoutes: ReadonlySet<AppRoutePath> = manualEntryRoutes🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/AuthenticatedApp.tsx` around lines 594 - 602, The duplicated Set<AppRoutePath> declared as fullCatalogRoutes should be replaced with the single exported manualEntryRoutes to avoid diverging route definitions; locate the fullCatalogRoutes declaration in AuthenticatedApp.tsx and import/use manualEntryRoutes (the exported symbol from renderWorkspace.tsx) instead of redefining the route Set so hydration gating and workspace behavior share the same source of truth.src/features/tracks/TrackEntryForm.tsx (1)
503-531: ⚡ Quick winPotential key collision in role pill rendering.
At line 504, the key uses only
role, which could lead to React key collisions if a credit somehow has duplicate roles in itsrolesarray (despite deduplication in the apply logic). Consider usingkey={${role}-${index}}or ensuring roles are always unique.However, the grouping logic (lines 289-290) uses a
Setto deduplicate roles, so duplicate roles within a single credit should not occur in practice. This is likely safe.🔐 Defensive key generation
- {credit.roles.map((role) => ( + {credit.roles.map((role, roleIndex) => ( - <span className="release-artist-role-pill" key={role}> + <span className="release-artist-role-pill" key={`${role}-${roleIndex}`}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/tracks/TrackEntryForm.tsx` around lines 503 - 531, The role pill list uses role as the React key (in credit.roles.map) which risks collisions if duplicates appear; update the key to a stable composite (e.g., include the parent credit id or the map index) such as combining credit.id and role (or role and index) so each <span> rendered in credit.roles.map has a unique key; keep the existing remove handler (setCredits/currentCredits/currentCredit.id) unchanged.src/features/releases/DiscogsReleaseLookupPanel.tsx (1)
742-788: ⚡ Quick winExtract duplicated compilation detection logic to a shared utility.
This compilation detection logic (lines 742-788) is duplicated in
ReleaseEntryForm.tsx(lines 650-699) with the same implementation. The functionsnormalizedSet,normalizeText,setsEqual, and the main detection logic are identical across both files.Extract these utilities to a shared module (e.g.,
src/features/releases/compilationDetectionUtils.ts) to prevent divergence and reduce maintenance burden. Compilation detection is core business logic for Discogs integration—having it in two places increases the risk that future bug fixes or enhancements will only be applied to one copy.♻️ Suggested extraction
Create a new shared utility file:
// src/features/releases/compilationDetectionUtils.ts import type { ExternalMetadataReleaseDetailDto } from '../catalog/catalogApi' export function hasCompilationTrackArtists( detail: ExternalMetadataReleaseDetailDto ): boolean { const releaseMainArtists = normalizedSet( detail.draft.artistCredits .filter((credit) => normalizeText(credit.role) === 'mainartist') .map((credit) => credit.name) ) const releaseArtists = releaseMainArtists.size > 0 ? releaseMainArtists : normalizedSet(detail.draft.artistCredits.map((credit) => credit.name)) return detail.draft.tracklist.some((track) => { const trackMainArtists = normalizedSet( track.artistCredits .filter((credit) => normalizeText(credit.role) === 'mainartist') .map((credit) => credit.name) ) if (trackMainArtists.size === 0) { return false } return !setsEqual(releaseArtists, trackMainArtists) }) } function normalizedSet(values: string[]) { return new Set(values.map(normalizeText).filter(Boolean)) } function normalizeText(value: string) { return value.trim().toLowerCase() } function setsEqual(left: Set<string>, right: Set<string>) { if (left.size !== right.size) { return false } for (const value of left) { if (!right.has(value)) { return false } } return true }Then import and use in both
DiscogsReleaseLookupPanel.tsxandReleaseEntryForm.tsx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/releases/DiscogsReleaseLookupPanel.tsx` around lines 742 - 788, Duplicate compilation-detection logic exists in DiscogsReleaseLookupPanel.tsx and ReleaseEntryForm.tsx; extract it into a shared utility module and import it from both places. Move the implementation of hasCompilationTrackArtists plus its helpers (normalizedSet, normalizeText, setsEqual) into a new shared module, export hasCompilationTrackArtists (helpers can be module-private or exported if needed), then replace the local implementations in DiscogsReleaseLookupPanel.tsx and ReleaseEntryForm.tsx with an import of hasCompilationTrackArtists and remove the duplicated helper functions from both files so both components call the single shared function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.superdesign/design-system.md:
- Line 32: The design doc requires an explicit review impact for "External
Source", but the shipped UI/tests do not surface an External Source review
group; either (A) implement the missing impact group in the UI and tests by
adding the External Source review UI and corresponding assertions where release
lookup behavior is handled (see DiscogsReleaseLookupPanel and the
discogs-release-autocomplete test suite) so the doc matches implementation, or
(B) update the design-system.md text to remove or relax the External Source
requirement to align with current product behavior and tests (adjusting the
release review-impact list that currently includes Core, Artists, Labels,
Tracklist, and External Source). Ensure whichever path you choose is reflected
both in the UI/test code (DiscogsReleaseLookupPanel and the related tests) and
the design doc so the contract is consistent.
In `@src/features/artists/DiscogsArtistLookupPanel.tsx`:
- Around line 67-73: In handleSearch, prevent blank/whitespace queries by
trimming the input and short-circuiting before calling searchDiscogsArtists:
call const q = query.trim(), if q is empty then setStatus to a message like
'Enter an artist name to search.' (and optionally clear candidates with
setCandidates([]) and setSelectedDetail(null)), and return early; otherwise call
searchDiscogsArtists with q (and limit) as before. Ensure you reference
handleSearch, query, searchDiscogsArtists, setStatus, setCandidates, and
setSelectedDetail in the change.
In `@src/features/releases/ReleaseArtistCreditsSection.tsx`:
- Around line 166-200: The bug is that addCreditRole/removeCreditRole use the
render-captured credit.roles snapshot, causing lost updates; update them to
compute mutations from the live state inside the setArtistCredits updater using
currentCredit (the mapped value). Specifically, in
addCreditRole/removeCreditRole, remove reliance on the outer credit.roles and
instead in the credits.map branch for currentCredit.id === credit.id compute
const existing = currentCredit.roles; if adding, if existing.includes(role)
return currentCredit else return { ...currentCredit, role: currentCredit.role ||
role, roles: [...existing, role] }; if removing, compute const nextRoles =
existing.filter(r => r !== role) and return { ...currentCredit, role:
nextRoles[0] ?? '', roles: nextRoles } so rapid consecutive edits operate on the
latest state.
In `@src/features/releases/ReleaseDetail.tsx`:
- Around line 110-112: The actions container rendering guard currently checks
onEdit, onUpdateViaDiscogs, and onEditLocalFiles && localTracks.length > 0 but
omits onDelete, so the delete control can be hidden; update the conditional that
renders the "detail-actions" block to include onDelete (e.g., add || onDelete to
the existing expression) so the actions container is shown when onDelete is
provided; ensure you reference the same variables used in the file (onEdit,
onUpdateViaDiscogs, onEditLocalFiles, localTracks, onDelete) and preserve the
existing localTracks length check.
In `@src/features/tracks/tracks.css`:
- Around line 45-51: The .discogs-track-lookup .discogs-search-form currently
forces three columns and compresses controls on small screens; inside the
existing small-screen media query add a fallback that sets .discogs-track-lookup
.discogs-search-form { grid-template-columns: repeat(1, minmax(0, 1fr)); } and
adjust the button alignment (e.g., .discogs-track-lookup .discogs-search-form >
.button { justify-self: stretch; } or justify-self: start as preferred) so the
form collapses to a single column and text/controls fit on mobile.
---
Outside diff comments:
In `@src/features/catalog/api/catalogEntityMappers.ts`:
- Around line 145-155: The fallback mapping builds roles without deduplication
causing inconsistency with the primary path; replace the inline roles mapping
with the shared helper creditRolesFromDto (or call toReleaseArtistCredit) so
role labels are deduped the same way—i.e., pass the DTO role list into
creditRolesFromDto (or reuse toReleaseArtistCredit logic) and assign its result
to roles instead of mapping manually.
---
Nitpick comments:
In `@src/app/AuthenticatedApp.tsx`:
- Around line 594-602: The duplicated Set<AppRoutePath> declared as
fullCatalogRoutes should be replaced with the single exported manualEntryRoutes
to avoid diverging route definitions; locate the fullCatalogRoutes declaration
in AuthenticatedApp.tsx and import/use manualEntryRoutes (the exported symbol
from renderWorkspace.tsx) instead of redefining the route Set so hydration
gating and workspace behavior share the same source of truth.
In `@src/features/releases/DiscogsReleaseLookupPanel.tsx`:
- Around line 742-788: Duplicate compilation-detection logic exists in
DiscogsReleaseLookupPanel.tsx and ReleaseEntryForm.tsx; extract it into a shared
utility module and import it from both places. Move the implementation of
hasCompilationTrackArtists plus its helpers (normalizedSet, normalizeText,
setsEqual) into a new shared module, export hasCompilationTrackArtists (helpers
can be module-private or exported if needed), then replace the local
implementations in DiscogsReleaseLookupPanel.tsx and ReleaseEntryForm.tsx with
an import of hasCompilationTrackArtists and remove the duplicated helper
functions from both files so both components call the single shared function.
In `@src/features/tracks/TrackEntryForm.tsx`:
- Around line 503-531: The role pill list uses role as the React key (in
credit.roles.map) which risks collisions if duplicates appear; update the key to
a stable composite (e.g., include the parent credit id or the map index) such as
combining credit.id and role (or role and index) so each <span> rendered in
credit.roles.map has a unique key; keep the existing remove handler
(setCredits/currentCredits/currentCredit.id) unchanged.
🪄 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: 1145d435-9c49-4f28-8605-25a1d7cb0a71
📒 Files selected for processing (57)
.superdesign/design-system.mdsrc/App.auth.test.tsxsrc/App.catalog-actions.test.tsxsrc/App.discogs-artist-autocomplete.test.tsxsrc/App.discogs-release-autocomplete.test.tsxsrc/App.discogs-track-autocomplete.test.tsxsrc/App.owned-items-inventory.test.tsxsrc/App.relation-credit-navigation.test.tsxsrc/App.search-v1.test.tsxsrc/App.server-navigation.test.tsxsrc/app/AuthenticatedApp.tsxsrc/app/renderWorkspace.tsxsrc/features/artists/ArtistDetail.tsxsrc/features/artists/ArtistsWorkspace.tsxsrc/features/artists/DiscogsArtistLookupPanel.tsxsrc/features/artists/artistsData.tssrc/features/catalog/api/artistLabelClient.tssrc/features/catalog/api/catalogEntityMappers.tssrc/features/catalog/api/catalogRequestMappers.tssrc/features/catalog/api/catalogTypes.tssrc/features/catalog/api/catalogValueMappers.tssrc/features/catalog/api/externalMetadataClient.tssrc/features/catalog/api/httpClient.tssrc/features/catalog/api/releaseClient.tssrc/features/catalog/api/trackClient.tssrc/features/catalog/catalogApi.mutations.test.tssrc/features/catalog/catalogApi.tssrc/features/catalog/externalMetadataClient.test.tssrc/features/manualEntry/manual-entry.csssrc/features/releases/CreditRolePicker.tsxsrc/features/releases/DiscogsReleaseLookupPanel.tsxsrc/features/releases/ReleaseArtistCreditsSection.tsxsrc/features/releases/ReleaseCoreSection.tsxsrc/features/releases/ReleaseDetail.tsxsrc/features/releases/ReleaseEntryForm.tsxsrc/features/releases/ReleaseEntryFormTypes.tssrc/features/releases/ReleaseTrackArtistCreditChip.tsxsrc/features/releases/ReleaseTrackDetail.tsxsrc/features/releases/ReleaseTracklistSection.tsxsrc/features/releases/ReleasesWorkspace.tsxsrc/features/releases/discogsReleaseTrackRows.test.tssrc/features/releases/discogsReleaseTrackRows.tssrc/features/releases/release-form.csssrc/features/releases/release-tracklist.csssrc/features/releases/releaseFormHelpers.tssrc/features/releases/releaseSubmit.tssrc/features/releases/releasesData.tssrc/features/releases/useReleaseTrackDrafts.tssrc/features/tracks/DiscogsTrackLookupPanel.tsxsrc/features/tracks/TrackDetail.tsxsrc/features/tracks/TrackEntryForm.tsxsrc/features/tracks/TracksWorkspace.tsxsrc/features/tracks/trackDisplayHelpers.tssrc/features/tracks/tracks.csssrc/features/tracks/tracksData.tssrc/styles/responsive.cssvitest.config.ts
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
Validation
Also visually checked the role picker popover in the local browser: the custom menu uses max-height and overflow-y auto, so long role lists scroll.
Summary by CodeRabbit
Release Notes
New Features
Documentation