feat(series): display multiple series memberships#597
Conversation
- Render every stored series membership on book details. - Link each displayed membership to its series detail route when an id is available. - Keep the primary series fields as a fallback when memberships are absent. Closes bookorbit#595
github-actions
Bot
added
the
client
📝 WalkthroughWalkthroughAdds multi-series support to the book details page. A ChangesMulti-series links
Estimated code review effort: 2 (Simple) | ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/features/book/components/detail/tabs/DetailsTab.vue (1)
806-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
Number.isIntegerover modulo check.
seriesIndex % 1 === 0works butNumber.isInteger(seriesIndex)is the idiomatic way to test for whole numbers and is clearer to readers.♻️ Optional refactor
function formatSeriesLabel(seriesName: string, seriesIndex: number | null): string { if (seriesIndex == null) return seriesName - const formattedIndex = seriesIndex % 1 === 0 ? Math.floor(seriesIndex) : seriesIndex + const formattedIndex = Number.isInteger(seriesIndex) ? seriesIndex : seriesIndex return `${seriesName} #${formattedIndex}` }Note:
Number.isIntegeralready guarantees the value is whole, soMath.floorbecomes unnecessary.🤖 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 `@client/src/features/book/components/detail/tabs/DetailsTab.vue` around lines 806 - 810, The whole-number check in formatSeriesLabel should use the idiomatic Number.isInteger test instead of the modulo expression. Update the logic in formatSeriesLabel to check whether seriesIndex is an integer with Number.isInteger(seriesIndex), and since that already guarantees a whole number, remove the unnecessary Math.floor branch while keeping the same output formatting.
🤖 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 `@client/src/features/book/components/detail/tabs/DetailsTab.vue`:
- Around line 812-824: The computed seriesLinks in DetailsTab.vue is returning
an empty array too early when seriesMemberships exists but all entries have
blank seriesName, which prevents the primary-series fallback from running.
Update the seriesLinks logic to only return mapped memberships when the filtered
list is non-empty, and otherwise allow the existing fallback branch that uses
props.book.seriesName and related fields to execute. Keep the fix centered on
the seriesLinks computed and its membership filtering/sorting path.
---
Nitpick comments:
In `@client/src/features/book/components/detail/tabs/DetailsTab.vue`:
- Around line 806-810: The whole-number check in formatSeriesLabel should use
the idiomatic Number.isInteger test instead of the modulo expression. Update the
logic in formatSeriesLabel to check whether seriesIndex is an integer with
Number.isInteger(seriesIndex), and since that already guarantees a whole number,
remove the unnecessary Math.floor branch while keeping the same output
formatting.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ad5c9f6-f070-4462-aa95-274c2bfb87d1
📒 Files selected for processing (2)
client/src/features/book/components/detail/tabs/DetailsTab.vueclient/src/features/book/components/detail/tabs/__tests__/DetailsTab.spec.ts
| const seriesLinks = computed<SeriesDisplayLink[]>(() => { | ||
| const memberships = props.book.seriesMemberships ?? [] | ||
| if (memberships.length > 0) { | ||
| return memberships | ||
| .filter((membership) => membership.seriesName.trim().length > 0) | ||
| .slice() | ||
| .sort((a, b) => a.displayOrder - b.displayOrder || a.seriesId - b.seriesId) | ||
| .map((membership) => ({ | ||
| key: `${membership.seriesId}-${membership.displayOrder}`, | ||
| seriesId: membership.seriesId, | ||
| label: formatSeriesLabel(membership.seriesName, membership.seriesIndex), | ||
| })) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fallback is skipped when all memberships have empty seriesName.
If seriesMemberships is non-empty but every entry has a blank seriesName, the .filter() produces an empty array and the computed returns []. The primary-series fallback at lines 826-833 is never reached because the outer if (memberships.length > 0) guard already passed. The user would see no series at all despite book.seriesName being populated.
🛡️ Proposed fix
const seriesLinks = computed<SeriesDisplayLink[]>(() => {
const memberships = props.book.seriesMemberships ?? []
if (memberships.length > 0) {
- return memberships
+ const links = memberships
.filter((membership) => membership.seriesName.trim().length > 0)
.slice()
.sort((a, b) => a.displayOrder - b.displayOrder || a.seriesId - b.seriesId)
.map((membership) => ({
key: `${membership.seriesId}-${membership.displayOrder}`,
seriesId: membership.seriesId,
label: formatSeriesLabel(membership.seriesName, membership.seriesIndex),
}))
+ if (links.length > 0) return links
}
if (!props.book.seriesName) return []📝 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.
| const seriesLinks = computed<SeriesDisplayLink[]>(() => { | |
| const memberships = props.book.seriesMemberships ?? [] | |
| if (memberships.length > 0) { | |
| return memberships | |
| .filter((membership) => membership.seriesName.trim().length > 0) | |
| .slice() | |
| .sort((a, b) => a.displayOrder - b.displayOrder || a.seriesId - b.seriesId) | |
| .map((membership) => ({ | |
| key: `${membership.seriesId}-${membership.displayOrder}`, | |
| seriesId: membership.seriesId, | |
| label: formatSeriesLabel(membership.seriesName, membership.seriesIndex), | |
| })) | |
| } | |
| const seriesLinks = computed<SeriesDisplayLink[]>(() => { | |
| const memberships = props.book.seriesMemberships ?? [] | |
| if (memberships.length > 0) { | |
| const links = memberships | |
| .filter((membership) => membership.seriesName.trim().length > 0) | |
| .slice() | |
| .sort((a, b) => a.displayOrder - b.displayOrder || a.seriesId - b.seriesId) | |
| .map((membership) => ({ | |
| key: `${membership.seriesId}-${membership.displayOrder}`, | |
| seriesId: membership.seriesId, | |
| label: formatSeriesLabel(membership.seriesName, membership.seriesIndex), | |
| })) | |
| if (links.length > 0) return links | |
| } |
🤖 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 `@client/src/features/book/components/detail/tabs/DetailsTab.vue` around lines
812 - 824, The computed seriesLinks in DetailsTab.vue is returning an empty
array too early when seriesMemberships exists but all entries have blank
seriesName, which prevents the primary-series fallback from running. Update the
seriesLinks logic to only return mapped memberships when the filtered list is
non-empty, and otherwise allow the existing fallback branch that uses
props.book.seriesName and related fields to execute. Keep the fix centered on
the seriesLinks computed and its membership filtering/sorting path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
🎉 This PR is included in version 2.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
github-actions
Bot
added
the
released
2 participants
Before you submit
What does this PR do?
Displays every stored series membership on the book details tab instead of showing only the primary series fields.
seriesMembershipsCloses #595
How did you test this?
pnpm exec vitest run src/features/book/components/detail/tabs/__tests__/DetailsTab.spec.tspnpm exec eslint src/features/book/components/detail/tabs/DetailsTab.vue src/features/book/components/detail/tabs/__tests__/DetailsTab.spec.ts --max-warnings 0Screenshots
UI change is limited to rendering additional series links in the existing book details header. Screenshots can be added on GitHub with a book that has multiple series memberships.
Anything non-obvious in the diff?
The book details tab renders the series links in two responsive header layouts, so the test expects each series link to appear twice.
Checklist
.env, personal configs)Summary by CodeRabbit
New Features
Bug Fixes