Open editable catalog workspaces by default#44
Merged
Conversation
📝 WalkthroughWalkthroughThis PR enhances the catalog workspace UI to support dictionary-aware role formatting, release cover upload/removal, and full-catalog loading while migrating app tests from server-backed to editable-workspace patterns. New display label utilities enable consistent dictionary-backed formatting across role facets and graph relations. ChangesCatalog UI Enhancement: Dictionaries, Release Covers, and Editable Workspaces
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 584e660b2e
ℹ️ 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".
Comment on lines
+287
to
+290
| queueMicrotask(() => { | ||
| if (isCurrent) { | ||
| setCoverLoadStatus('loading') | ||
| } |
There was a problem hiding this comment.
When the selected release changes, this effect only flips the status to loading and leaves the previous coverImage in state until loadRelease settles; if the new request is slow or fails, the detail panel for Release B can show Release A's cover under B's title. Clear coverImage when starting a load (or key this panel by releaseId) so release detail panels never display a stale cover.
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: 3
🧹 Nitpick comments (4)
src/features/catalog/CatalogGraphDetailPanel.tsx (3)
453-453: 💤 Low valueRedundant special-case check:
mediacoverage===mediacoverageis always true.Line 453 compares
normalizedTitle === 'mediacoverage'andnormalizedLabel === 'mediacoverage', which is equivalent to the general case already checked on line 449. This special case has no effect and can be removed.♻️ Proposed cleanup
(normalizedTitle === 'tracks' && normalizedLabel === 'tracklist') || (normalizedTitle === 'labels' && normalizedLabel === 'label') || - (normalizedTitle === 'ownedcopies' && normalizedLabel === 'ownedcopy') || - (normalizedTitle === 'mediacoverage' && normalizedLabel === 'mediacoverage') + (normalizedTitle === 'ownedcopies' && normalizedLabel === 'ownedcopy') )🤖 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/CatalogGraphDetailPanel.tsx` at line 453, Remove the redundant special-case comparison in CatalogGraphDetailPanel: the conditional fragment "(normalizedTitle === 'mediacoverage' && normalizedLabel === 'mediacoverage')" is always true when the general check already matches and should be deleted; update the conditional/boolean expression in the render logic (where normalizedTitle and normalizedLabel are used) to omit this redundant clause so behavior remains unchanged, leaving only the general case handling.
344-349: ⚡ Quick winPotential duplicate loading and error messages.
ReleaseCoverPanelalready manages and displays loading/error states (lines 386-387 in ReleaseDetail.tsx). This section adds additional status messages for cover loading, which could result in two concurrent "Loading cover..." or error messages on screen.♻️ Consider removing duplicate status UI
If
ReleaseCoverPanelalready handles upload/removal feedback, you can likely remove lines 344-349 or conditionally show them only during initial load (not during mutation pending states)./> - {coverLoadStatus === 'loading' ? ( - <p role="status">Loading cover...</p> - ) : null} - {coverLoadStatus === 'error' ? ( - <p role="alert">Cover image could not be loaded.</p> - ) : null} </section>🤖 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/CatalogGraphDetailPanel.tsx` around lines 344 - 349, Remove the duplicate cover loading/error messages in CatalogGraphDetailPanel by deleting or gating the coverLoadStatus conditional block (the JSX that renders "<p role='status'>Loading cover...</p>" and "<p role='alert'>Cover image could not be loaded.</p>") since ReleaseCoverPanel already presents those states; alternatively, if you need an initial-load-only message, change the condition to show those messages only when coverLoadStatus indicates initial fetch (e.g., not when a mutation/upload is pending) so ReleaseCoverPanel remains the single source of truth for upload/removal feedback.
390-396: ⚡ Quick winInconsistent "Artist" default role replacement logic.
The code removes the default "Artist" role only when
roleshas exactly one entry and that entry is "Artist" (line 390). If an artist already has["Artist", "Vocalist"]and a new role like "Producer" is detected, the result becomes["Artist", "Vocalist", "Producer"], keeping the generic "Artist" alongside more specific roles. This inconsistency could confuse users.♻️ Proposed fix for consistent default-role handling
Remove the default "Artist" role whenever a more specific role is added, not just when it's the only role:
const role = formatRoleFacet(candidate ?? '', dictionaries) if (!existing.roleSet.has(role)) { - if (existing.roles.length === 1 && existing.roles[0] === 'Artist') { + if (existing.roles.includes('Artist')) { existing.roles = [] + existing.roleSet.clear() } existing.roleSet.add(role) existing.roles.push(role) } }Alternatively, only add the default "Artist" role at line 398-400 if no other roles were ever added during the loop.
🤖 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/CatalogGraphDetailPanel.tsx` around lines 390 - 396, The current logic in CatalogGraphDetailPanel (inside the block that updates existing.roleSet and existing.roles) only removes the default "Artist" role when it's the sole entry; change it so that whenever you add a new role that is not "Artist" you remove any existing "Artist" entry (e.g., adjust the code around existing.roleSet.add(role) / existing.roles.push(role) to check if role !== 'Artist' and then remove 'Artist' from both existing.roleSet and existing.roles), or alternatively defer adding the default "Artist" role until after the loop and only add it if existing.roles is empty; update the logic in the same function to maintain consistency between existing.roleSet and existing.roles.src/App.server-navigation.test.tsx (1)
73-81: ⚡ Quick winMake hydration assertion less brittle and await post-navigation UI deterministically.
Hard-coding
13ties this test to internal request fan-out and can break on harmless API call changes. Also prefer awaiting the Tracks heading after click.Proposed test hardening
- await h.waitFor(() => { - expect(fetchMock.mock.calls).toHaveLength(13) - }) + await h.waitFor(() => { + expect(fetchMock.mock.calls.length).toBeGreaterThan(0) + }) + const callCountAfterReleases = fetchMock.mock.calls.length await user.click(h.screen.getByRole('link', { name: 'Tracks' })) - expect( - h.screen.getByRole('heading', { name: 'Track records' }), - ).toBeInTheDocument() - expect(fetchMock.mock.calls).toHaveLength(13) + expect( + await h.screen.findByRole('heading', { name: 'Track records' }), + ).toBeInTheDocument() + expect(fetchMock.mock.calls).toHaveLength(callCountAfterReleases)As per coding guidelines, "Use Testing Library for user-observable behavior; avoid tests that only verify implementation details".
🤖 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.server-navigation.test.tsx` around lines 73 - 81, Replace the brittle hard-coded fetch call count check and the synchronous heading assertion with a deterministic wait for the post-navigation UI: after calling user.click(...) on the 'Tracks' link (the user.click call in this test), remove or stop asserting expect(fetchMock.mock.calls).toHaveLength(13) and instead await the UI change using Testing Library (e.g., await h.screen.findByRole('heading', { name: 'Track records' }) or await waitFor(() => expect(h.screen.getByRole('heading', { name: 'Track records' })).toBeInTheDocument())), keeping fetchMock usage only for meaningful, behavior-level assertions if needed; reference fetchMock.mock.calls, user.click, and the 'Track records' heading to locate the lines to edit.
🤖 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 `@src/App.catalog-actions.test.tsx`:
- Around line 647-760: The test file exceeded the size guideline because three
large helper fixtures (searchResponseWithRelease,
graphResponseForReleaseWithDuplicateArtists, releaseDetailWithoutCover) are
defined inline; extract these functions into a new shared test fixtures module
(e.g., create test/catalogFixtures.ts), export them, then replace the inline
definitions by importing the exported helpers into
src/App.catalog-actions.test.tsx and update any references to call the imported
functions; ensure the new module exports the exact function names and that any
helper utilities used inside them (like h.jsonResponse) are available or
imported in the new file.
In `@src/features/catalog/ServerCatalogControls.tsx`:
- Around line 369-376: The rendered badges can produce duplicate keys because
formatRoleFacet(result.facets.roles) may map different raw roles to the same
display string; fix by deduplicating the formatted labels before passing them
into BadgeList. In the ServerCatalogControls.tsx cell that uses
result.facets.roles and formatRoleFacet, transform roles to formatted strings,
remove duplicates (e.g., via a Set while preserving order), then pass the
deduped array into <BadgeList values={...} ... /> so each badge key/value is
unique.
In `@src/features/catalog/ServerEntityWorkspace.tsx`:
- Around line 506-521: Extract the duplicate uniqueRoleOptions helper from
ServerEntityWorkspace.tsx (and the copy in ServerCatalogControls.tsx) into a
shared module (e.g. catalogDisplayLabels.ts) and update both files to import it;
keep the implementation using a Map and iterate over roles (drop the redundant
uniqueValues call). Also move the URL/param helpers parseEntitySearchParams,
buildEntityUrl and pushSelectionUrl into a separate module to reduce file size
and eliminate duplication, and update ServerEntityWorkspace.tsx to import those
helpers instead of defining them inline.
---
Nitpick comments:
In `@src/App.server-navigation.test.tsx`:
- Around line 73-81: Replace the brittle hard-coded fetch call count check and
the synchronous heading assertion with a deterministic wait for the
post-navigation UI: after calling user.click(...) on the 'Tracks' link (the
user.click call in this test), remove or stop asserting
expect(fetchMock.mock.calls).toHaveLength(13) and instead await the UI change
using Testing Library (e.g., await h.screen.findByRole('heading', { name: 'Track
records' }) or await waitFor(() => expect(h.screen.getByRole('heading', { name:
'Track records' })).toBeInTheDocument())), keeping fetchMock usage only for
meaningful, behavior-level assertions if needed; reference fetchMock.mock.calls,
user.click, and the 'Track records' heading to locate the lines to edit.
In `@src/features/catalog/CatalogGraphDetailPanel.tsx`:
- Line 453: Remove the redundant special-case comparison in
CatalogGraphDetailPanel: the conditional fragment "(normalizedTitle ===
'mediacoverage' && normalizedLabel === 'mediacoverage')" is always true when the
general check already matches and should be deleted; update the
conditional/boolean expression in the render logic (where normalizedTitle and
normalizedLabel are used) to omit this redundant clause so behavior remains
unchanged, leaving only the general case handling.
- Around line 344-349: Remove the duplicate cover loading/error messages in
CatalogGraphDetailPanel by deleting or gating the coverLoadStatus conditional
block (the JSX that renders "<p role='status'>Loading cover...</p>" and "<p
role='alert'>Cover image could not be loaded.</p>") since ReleaseCoverPanel
already presents those states; alternatively, if you need an initial-load-only
message, change the condition to show those messages only when coverLoadStatus
indicates initial fetch (e.g., not when a mutation/upload is pending) so
ReleaseCoverPanel remains the single source of truth for upload/removal
feedback.
- Around line 390-396: The current logic in CatalogGraphDetailPanel (inside the
block that updates existing.roleSet and existing.roles) only removes the default
"Artist" role when it's the sole entry; change it so that whenever you add a new
role that is not "Artist" you remove any existing "Artist" entry (e.g., adjust
the code around existing.roleSet.add(role) / existing.roles.push(role) to check
if role !== 'Artist' and then remove 'Artist' from both existing.roleSet and
existing.roles), or alternatively defer adding the default "Artist" role until
after the loop and only add it if existing.roles is empty; update the logic in
the same function to maintain consistency between existing.roleSet and
existing.roles.
🪄 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: 2377b17b-93ce-4b57-9fed-2be48a522223
📒 Files selected for processing (17)
src/App.auth.test.tsxsrc/App.catalog-actions.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/catalog/CatalogGraphDetailPanel.tsxsrc/features/catalog/CatalogWorkspace.tsxsrc/features/catalog/FilterSelect.tsxsrc/features/catalog/ServerCatalogControls.tsxsrc/features/catalog/ServerCatalogWorkspace.tsxsrc/features/catalog/ServerEntityWorkspace.tsxsrc/features/catalog/api/releaseClient.tssrc/features/catalog/catalogDisplayLabels.tssrc/features/releases/ReleaseDetail.tsx
Comment on lines
+647
to
+760
| function searchResponseWithRelease() { | ||
| return h.jsonResponse({ | ||
| items: [ | ||
| { | ||
| id: 'release-stripped', | ||
| type: 'release', | ||
| title: 'Stripped', | ||
| subtitle: 'Mute', | ||
| summary: 'Imported single release.', | ||
| matchedFields: ['title', 'credit.role'], | ||
| snippets: ['Depeche Mode · Stripped'], | ||
| facets: { | ||
| roles: ['mainArtist'], | ||
| media: [], | ||
| statuses: [], | ||
| tags: [], | ||
| labelId: 'label-mute', | ||
| collectorSignals: [], | ||
| }, | ||
| rank: 1, | ||
| }, | ||
| ], | ||
| limit: 100, | ||
| offset: 0, | ||
| total: 1, | ||
| }) | ||
| } | ||
|
|
||
| function graphResponseForReleaseWithDuplicateArtists() { | ||
| return h.jsonResponse({ | ||
| entity: { | ||
| id: 'release-stripped', | ||
| type: 'release', | ||
| title: 'Stripped', | ||
| subtitle: 'Mute', | ||
| summary: 'Imported single release.', | ||
| }, | ||
| sections: { | ||
| artists: [ | ||
| { | ||
| id: 'artist-depeche-mode', | ||
| type: 'artist', | ||
| title: 'Depeche Mode', | ||
| subtitle: 'Group', | ||
| relation: 'mainArtist', | ||
| }, | ||
| ], | ||
| credits: [ | ||
| { | ||
| id: 'artist-depeche-mode', | ||
| type: 'artist', | ||
| title: 'Depeche Mode', | ||
| subtitle: 'mainArtist', | ||
| relation: 'credit', | ||
| }, | ||
| ], | ||
| releases: [], | ||
| tracks: [ | ||
| { | ||
| id: 'track-stripped', | ||
| type: 'track', | ||
| title: 'Stripped', | ||
| subtitle: '1', | ||
| relation: 'tracklist', | ||
| }, | ||
| ], | ||
| ownedCopies: [], | ||
| labels: [ | ||
| { | ||
| id: 'label-mute', | ||
| type: 'label', | ||
| title: 'Mute', | ||
| subtitle: 'BONG 010', | ||
| relation: 'label', | ||
| }, | ||
| ], | ||
| playlists: [], | ||
| relations: [], | ||
| media: [], | ||
| }, | ||
| collectorSignals: [], | ||
| }) | ||
| } | ||
|
|
||
| function releaseDetailWithoutCover() { | ||
| return h.jsonResponse({ | ||
| id: 'release-stripped', | ||
| title: 'Stripped', | ||
| type: 'standalone', | ||
| year: 1986, | ||
| releaseDate: '1986-02-10', | ||
| genres: [], | ||
| tags: [], | ||
| coverImage: null, | ||
| isVariousArtists: false, | ||
| notOnLabel: false, | ||
| artistCredits: [ | ||
| { | ||
| artistId: 'artist-depeche-mode', | ||
| artistName: 'Depeche Mode', | ||
| role: 'mainArtist', | ||
| }, | ||
| ], | ||
| labels: [ | ||
| { | ||
| labelId: 'label-mute', | ||
| name: 'Mute', | ||
| catalogNumber: 'BONG 010', | ||
| hasNoCatalogNumber: false, | ||
| }, | ||
| ], | ||
| tracklist: [], | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
File exceeds size guideline.
The pipeline reports this file has grown to 760 lines, exceeding the 600-line limit. Consider extracting the new helper functions (searchResponseWithRelease, graphResponseForReleaseWithDuplicateArtists, releaseDetailWithoutCover) into a shared test fixtures module (e.g., test/catalogFixtures.ts) to keep test files focused and maintainable.
🧰 Tools
🪛 GitHub Actions: CI / 0_Verify.txt
[error] 760-760: file-size:check failed. File exceeds 600 lines (760 lines reported).
🪛 GitHub Actions: CI / Verify
[error] 760-760: file-size:check failed. File exceeds 600 lines (760).
🤖 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.catalog-actions.test.tsx` around lines 647 - 760, The test file
exceeded the size guideline because three large helper fixtures
(searchResponseWithRelease, graphResponseForReleaseWithDuplicateArtists,
releaseDetailWithoutCover) are defined inline; extract these functions into a
new shared test fixtures module (e.g., create test/catalogFixtures.ts), export
them, then replace the inline definitions by importing the exported helpers into
src/App.catalog-actions.test.tsx and update any references to call the imported
functions; ensure the new module exports the exact function names and that any
helper utilities used inside them (like h.jsonResponse) are available or
imported in the new file.
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
Verification
Summary by CodeRabbit
Release Notes
Tests
New Features
Refactor