Add listing attributes for display and filtering - #1683
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces reusable listing attributes with encrypted persistence, admin CRUD and listing assignment, dashboard filtering, public rendering, shared admin abstractions, migrations, styling, and tests. ChangesListing Attributes Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant attributesRoutes
participant attributesDB as shared/db/attributes.ts
participant ListingPage
Admin->>attributesRoutes: POST listing attribute selections
attributesRoutes->>attributesDB: prune and persist option IDs
attributesDB-->>attributesRoutes: saved assignments
attributesRoutes-->>Admin: redirect with success flash
Admin->>ListingPage: open Attributes tab
ListingPage->>attributesDB: load all options and selected IDs
attributesDB-->>ListingPage: attribute panel data
sequenceDiagram
participant Visitor
participant PublicPages
participant attributesDB as shared/db/attributes.ts
participant PublicTemplates
Visitor->>PublicPages: request listing page
PublicPages->>attributesDB: getSelectedAttributesForListings
attributesDB-->>PublicPages: attributesByListing
PublicPages->>PublicTemplates: render listing data with attributes
PublicTemplates-->>Visitor: listing cards or ticket page with attributes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 8
🤖 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/features/admin/attributes.ts`:
- Line 164: The attribute option activity logs in the add/edit/delete paths are
using attribute.id, which produces unreadable numeric entries instead of the
attribute name. Update the logActivity calls in the attribute option handlers to
use attributeNameFlat(attribute) consistently, matching the existing readable
logging used elsewhere in this file and the attribute create/update/delete
messages.
- Around line 288-299: The `moveAttributeHandler` flow is decrypting full
attribute/option payloads just to locate ids, which is unnecessary work. Update
`handle` in `moveAttributeHandler` and the `loadContext` /
`handleListingAttributesPost` paths to use a lightweight non-decrypting lookup
helper instead of `getAllAttributesWithOptions()` and
`getAttributeWithOptions()`. Reuse or introduce the lightweight id-only approach
suggested in `src/shared/db/attributes.ts` so `swapAttributeOrder`, `readIds`,
and related lookups operate on ids without decrypting option text.
In `@src/features/admin/dashboard.ts`:
- Around line 86-106: `loadListingAttributeFilterContext` is fetching/decrypting
attributes for every listing even though `adminDashboardPage` only filters
against `filterSource` (`activeListings`). Update the helper to build
`attributesByListing` from the `filterSource` ids instead of `listings` ids, and
keep `attributeFilters`/`selectedAttributeFiltersFromRequest` wired through the
existing `getSelectedAttributesForListings` and
`attributeFilterGroupsForListings` flow so only the listings that participate in
filtering are loaded.
- Line 139: The active listings derivation in adminDashboardPage is using native
Array.prototype.filter instead of the FP helper. Update the activeListings
calculation in dashboard.ts to use the curried filter from `#fp`, matching the
existing style used for the same logic in adminDashboardPage and keeping the
code consistent with the FP-style guidelines.
In `@src/shared/db/attributes.ts`:
- Around line 267-292: The current getSelectedAttributesForListings path
decrypts the same attribute/option separately for each listing because
selectedRowsForListing feeds groupAttributeRows per listing. Update the shared
flow in attributes.ts so decrypting happens once per unique attribute_id and
option_id, then rebuild each listing’s grouped result from those shared
decrypted objects without mixing options across listings. Keep the per-listing
filtering semantics intact when adjusting
selectedRowsForListing/groupAttributeRows, and verify the behavior with the
existing attributes tests.
- Around line 159-187: `getAllAttributesWithOptions` is doing expensive full
decryption even when callers only need ids, such as `moveAttributeHandler` and
`handleListingAttributesPost`’s `readIds`. Add lightweight id-only helpers in
`src/shared/db/attributes.ts` for ordered attribute ids and attribute option
ids, and update those callers to use them instead of
`getAllAttributesWithOptions`. Keep
`groupAttributeRows`/`getAttributeWithOptions` for the places that actually need
decrypted `name` and `text`.
In `@src/ui/static/style.scss`:
- Around line 238-258: The spacing in listing-attributes currently relies on
--space-xs, which is not defined in the checked-in token set, so the gap styles
may not apply. Update the listing-attributes rules in style.scss to either
define --space-xs in the shared tokens or replace it with an existing spacing
token already used elsewhere, and keep the selectors .listing-attributes and
.listing-attributes > div aligned with the intended compact layout.
In `@test/lib/server-attributes.test.ts`:
- Around line 359-378: The test name in server-attributes.test.ts suggests
duplicate option ids are being deduplicated, but the current payload only checks
invalid-id filtering. Update the repeated-options test around
postRepeatedOptions and getListingAttributeOptionIds so it submits an actual
duplicate option id (for example, repeat one of the valid attribute.options ids)
while still including an invalid id if needed, then assert the saved ids are
unique and only the valid options remain. This will exercise the
unique(optionIds) path in setListingAttributeOptions instead of only the pruning
behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b00c9ce4-0696-4f10-8b54-041a72025404
📒 Files selected for processing (41)
src/features/admin/attributes.tssrc/features/admin/dashboard.tssrc/features/admin/index.tssrc/features/admin/listing-choice-post.tssrc/features/admin/listing-page-data.tssrc/features/admin/listing-page.tssrc/features/admin/questions.tssrc/features/public/pages.tssrc/features/public/ticket-submit.tssrc/features/public/types.tssrc/locales/en/attributes.jsonsrc/locales/en/entity-pages.jsonsrc/locales/en/index.tssrc/locales/en/terms.jsonsrc/shared/db/attributes.tssrc/shared/db/listings.tssrc/shared/db/migrations.tssrc/shared/db/migrations/2026-07-09_listing_attributes.tssrc/shared/db/migrations/schema.tssrc/shared/listing-attribute-filter.tssrc/ui/static/style.scsssrc/ui/templates/admin/attributes.tsxsrc/ui/templates/admin/dashboard.tsxsrc/ui/templates/admin/listing-attribute-filters.tssrc/ui/templates/admin/listing-panel-frame.tsxsrc/ui/templates/admin/nav.tsxsrc/ui/templates/admin/questions.tsxsrc/ui/templates/components/aggregate-sections.tsxsrc/ui/templates/components/reorder-table.tsxsrc/ui/templates/public/homepage.tsxsrc/ui/templates/public/listing-attributes.tssrc/ui/templates/public/reservations.tsxtest/lib/db/migration-schema-guard.test.tstest/lib/server-attributes.test.tstest/lib/server-listings-filter.test.tstest/shared/db/attributes.test.tstest/shared/db/listings/delete.test.tstest/shared/listing-attribute-filter.test.tstest/test-utils.tstest/test-utils/db-helpers/attributes.tstest/ui/templates/admin/listing-attribute-filters.test.ts
![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: 8e7650e10e
ℹ️ 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".
| listingAttributes={ | ||
| singleListing | ||
| ? attributesByListing.get(singleListing.id) | ||
| : undefined |
There was a problem hiding this comment.
When a customer opens a combined booking page such as /ticket/a+b or arrives from the cart with more than one listing, singleListing is null, so this passes undefined and the fetched attributesByListing is never rendered anywhere else in the listing rows. That makes public attributes disappear on multi-listing ticket pages even though renderCtx loaded them for every page listing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 86e812c. The multi-listing ticket page now renders each listing's selected attributes on its own row — renderListingRow takes a pre-rendered attributesHtml string threaded from attributesByListing, so every row shows its own attributes the same way the single-listing header already did. Regression test in server-listings-filter.test.ts: "shows each listing's attributes on a multi-listing ticket page".
| await Promise.all([ | ||
| buildTicketListingsWithGroupCapacity(listings), | ||
| buildDailyDateFilter(listings, requestedDate), | ||
| soldOutPackageIds(groups, requestedDate), | ||
| getSelectedAttributesForListings(listings.map((listing) => listing.id)), |
There was a problem hiding this comment.
This only loads attributes for /listings; the public order catalog still renders listing cards through orderGalleryPage from src/features/public/order.ts without loading or passing attributesByListing, so when settings.orderEnabled is used as the storefront the same listing attributes remain invisible until after selection. Please thread these selected attributes through the order gallery card renderer as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 86e812c. handleOrder now fetches attributesByListing via getSelectedAttributesForListings and threads it through orderGalleryPage → renderOrderCard, so each order card (selectable and unavailable) shows its selected attributes. Regression test in server-order.test.ts: "shows selected listing attributes on order cards".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/db/migrations/schema.ts (1)
872-894: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the no-FK cleanup ownership, matching house style.
The new join table's comment explains the design rationale but, unlike
group_listings,listing_parents, andmodifier_listings, doesn't state which delete path is responsible for pruning orphaned rows once attributes/options/listings are removed (there are no FKs enforcing this).📝 Suggested comment addition
[ - // A selected option on a listing. The option points to its attribute, so the - // listing does not need a separate attribute row: one path handles one or - // many selected options. + // A selected option on a listing. The option points to its attribute, so the + // listing does not need a separate attribute row: one path handles one or + // many selected options. No FKs (house style); listing/attribute/option + // deletion paths prune this table explicitly. "listing_attribute_options",🤖 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/shared/db/migrations/schema.ts` around lines 872 - 894, Update the comment for the listing_attribute_options table to document that it has no foreign keys and identify the application delete/cleanup path responsible for removing orphaned rows when listings, attributes, or options are deleted, matching the wording and style used by group_listings, listing_parents, and modifier_listings.
🤖 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.
Outside diff comments:
In `@src/shared/db/migrations/schema.ts`:
- Around line 872-894: Update the comment for the listing_attribute_options
table to document that it has no foreign keys and identify the application
delete/cleanup path responsible for removing orphaned rows when listings,
attributes, or options are deleted, matching the wording and style used by
group_listings, listing_parents, and modifier_listings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f46257fe-b01b-4065-be57-d7fd15c53da6
📒 Files selected for processing (3)
src/shared/db/migrations.tssrc/shared/db/migrations/schema.tstest/lib/db/migration-schema-guard.test.ts
stefan-burke
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/templates/public/reservations.tsx (2)
790-841: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClosed listings never show
attributesHtml.The
isClosedbranch (Lines 801-809) omitsattributesHtmlwhile theisSoldOutbranch (Line 817) and the normal branch (Line 836) both render it. This diverges fromorder-gallery.tsx'sunavailableCard, which rendersattributesHtmlfor both closed and sold-out states. A listing with selected attributes will silently lose that info on ticket pages once it closes, but keep showing it on the/ordergallery — an inconsistent, easy-to-miss UX regression.🐛 Proposed fix
if (isClosed) { return ` <div class="ticket-row sold-out"> ${imageHtml} <label>${escapeHtml(listing.name)}</label> + ${attributesHtml} <span class="sold-out-label">${t("public.registration_closed")}</span> </div> `; }🤖 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/ui/templates/public/reservations.tsx` around lines 790 - 841, Update the isClosed branch of renderListingRow to render attributesHtml alongside the listing name and closed-status label, matching the isSoldOut and normal branches and the unavailableCard behavior in order-gallery.tsx.
1495-1567: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPackage member rows still need attribute rendering.
attributesByListingonly reachesbuildListingRows, so listings rendered throughrenderPackageControls/renderPackageSectionnever passrenderListingAttributes(...). Package-only listings will drop their selected attributes on public ticket pages; thread the attributes map through the package render path and render it inrenderPackageMemberRow.🤖 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/ui/templates/public/reservations.tsx` around lines 1495 - 1567, Thread opts.attributesByListing through renderPackageControls and renderPackageSection into renderPackageMemberRow, and update each corresponding signature and call site. In renderPackageMemberRow, use the listing’s attributes map to invoke renderListingAttributes(...) so package member rows render selected attributes consistently with buildListingRows.
🤖 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 `@test/lib/server-listings-filter.test.ts`:
- Around line 222-239: Strengthen the test “shows each listing's attributes on a
multi-listing ticket page” by assigning distinct attributes to both listing1 and
listing2, then use scoped assertions for each listing’s rendered section to
verify its own attribute name and option are present and the other listing’s
attribute is absent. Avoid relying only on response-wide text checks so
incorrect association or omission is detected.
---
Outside diff comments:
In `@src/ui/templates/public/reservations.tsx`:
- Around line 790-841: Update the isClosed branch of renderListingRow to render
attributesHtml alongside the listing name and closed-status label, matching the
isSoldOut and normal branches and the unavailableCard behavior in
order-gallery.tsx.
- Around line 1495-1567: Thread opts.attributesByListing through
renderPackageControls and renderPackageSection into renderPackageMemberRow, and
update each corresponding signature and call site. In renderPackageMemberRow,
use the listing’s attributes map to invoke renderListingAttributes(...) so
package member rows render selected attributes consistently with
buildListingRows.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92b08a89-9cfa-41ce-bdb5-caa61be79653
📒 Files selected for processing (5)
src/features/public/order.tssrc/ui/templates/public/order-gallery.tsxsrc/ui/templates/public/reservations.tsxtest/lib/server-listings-filter.test.tstest/lib/server-order.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86e812c677
ℹ️ 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".
| opts.hideQuantity, | ||
| opts.prefill, | ||
| (info) => (memberIds.has(info.listing.id) ? undefined : opts.childCtx), | ||
| opts.attributesByListing ?? new Map(), |
There was a problem hiding this comment.
On booking pages that contain packages, the package sections have already been rendered through renderPackageSection/renderPackageMemberRow before this standalone-row call receives attributesByListing, and those package helpers never receive the map. A listing sold only as a visible package member therefore still hides its selected attributes on /ticket/<package> or mixed package carts, even though renderCtx loaded attributes for every page listing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1fb040a. renderPackageMemberRow, renderPackageControls, and renderPackageSection now take attributesByListing and render each member's selected attributes via renderListingAttributes(attributesByListing.get(member.listing.id)), threaded from buildPageListingRows. Package member rows show their attributes the same way standalone rows do.
| filters={filters} | ||
| listings={activeListings} | ||
| headerHtml={attributeFilterHtml} | ||
| listings={filterByAttribute(activeListings)} |
There was a problem hiding this comment.
This makes the /admin/listings table respect the selected attribute filters, but the export link still points to /admin/listings/csv and handleListingsCsvExport only applies listingTypeFromRequest. When an owner filters the listing index by an attribute and clicks Export CSV, the downloaded file includes listings that are no longer in the table, so the export can be misleading; carry these attribute params through and apply the same filter server-side.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1fb040a. The CSV export handler now loads loadListingAttributeFilterContext and applies filterListingsByAttributes alongside the existing type filter, so the download respects the same attribute filters as the table. The export link in the listings index also carries the active attribute params via csvExportHref. Regression tests in server-listings-filter.test.ts: "CSV export link carries the active attribute filter" and "CSV export respects attribute filter".
| export const getListingAttributeOptionIds = ( | ||
| listingId: number, | ||
| ): Promise<number[]> => listingAttributeOptions.getIds(listingId); |
There was a problem hiding this comment.
AGENTS.md explicitly says not to export a name that is just another name for an existing method, and this wrapper adds no behavior over listingAttributeOptions.getIds (the adjacent setter follows the same pattern because setIds already dedupes). Keeping both names gives the same internal mechanism two APIs; internal callers should use listingAttributeOptions.getIds/setIds directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1fb040a. Removed the getListingAttributeOptionIds alias export — callers (listing-page-data.ts and the tests) now use listingAttributeOptions.getIds directly, per the AGENTS.md guidance against alias exports. setListingAttributeOptions stays because it adds unique() dedup — a real transformation, not a pure alias.
| hideQuantity = false, | ||
| prefill?: TicketPrefill, | ||
| childCtx?: ChildRenderCtx, | ||
| attributesHtml = "", |
There was a problem hiding this comment.
When a multi-listing booking page contains a listing whose registration is closed, the new attributesHtml value is passed into renderListingRow but the isClosed branch returns before rendering it, unlike sold-out and bookable rows. Fresh evidence for this narrower case is the newly added parameter still being skipped by the closed-row return, so closed listings lose their public attributes on /ticket/a+b.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1fb040a. The isClosed branch of renderListingRow now renders attributesHtml before the sold-out label, matching the sold-out and bookable branches. Closed listings keep their public attributes on multi-listing pages.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fb040ae0e
ℹ️ 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".
| const listings = filterListingsByType(type)(await loadSortedListings()); | ||
| const csv = generateListingsCsv(listings, settings.timezone); | ||
| const { activeAttributeFilters, attributesByListing } = | ||
| await loadListingAttributeFilterContext(request, listings); |
There was a problem hiding this comment.
When /admin/listings/csv combines type= with an attribute option that exists on another listing type, this newly added load builds activeAttributeFilters from the already type-filtered listings, so selectedAttributeFiltersFromRequest treats that attribute param as invalid and drops it. For example, ?type=daily&attribute_1=<standard-only option> exports all daily listings instead of the empty intersection that the type+attribute filters describe; load the attribute filter context from the full listing set and then apply the type filter so valid attribute params are preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ecce9a2. loadListingAttributeFilterContext now loads from the full listing set (before filterListingsByType narrows it), then the type filter is applied after. So an attribute on a different listing type is still recognised by selectedAttributeFiltersFromRequest instead of being silently dropped.
| getSelectedAttributesForListings( | ||
| ctx.listings.map((entry) => entry.listing.id), | ||
| ), |
There was a problem hiding this comment.
When a parent listing renders selectable add-on/child listings on /ticket/<parent>, those child rows are built from ctx.childrenByParentId, but this new attributes load only requests ctx.listings. A selected attribute on the child listing is therefore unavailable to the render path and never appears beside the child option, even though the buyer is choosing that child on the ticket page; include the child ids in this load and thread them through the child block renderer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ecce9a2. The getSelectedAttributesForListings call in renderCtx now includes child listing ids (from ctx.childrenByParentId → children), so attributes on add-on/child listings are loaded and available to the render path alongside the parent listings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0287ad58bd
ℹ️ 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".
| getSelectedAttributesForListings([ | ||
| ...ctx.listings.map((entry) => entry.listing.id), | ||
| ...children.map((child) => child.id), | ||
| ]), |
There was a problem hiding this comment.
Fresh evidence after the previous thread: this now loads child IDs, but the returned attributesByListing is only passed to ticketPage for listing/package rows; ChildRenderCtx still has no attributes map and renderChildOption/renderSoleChildOption render only the child name/price. For /ticket/<parent> with a selectable add-on child, the child's selected attributes therefore remain invisible despite being fetched; thread this map into the child renderer and render it beside each child option.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 64ed02b. attributesByListing is now threaded into ChildRenderCtx and renderChildOption/renderSoleChildOption render each child's selected attributes via renderListingAttributes(attributesByListing.get(child.listing.id)). Child add-on options show their attributes on parent ticket pages.
| export const listingAttributeOptions = linkTableSide( | ||
| "listing_attribute_options", | ||
| "listing_id", | ||
| "option_id", | ||
| ); |
There was a problem hiding this comment.
This new per-listing link table is only written from the Attributes tab; the single-listing duplicate path (buildDuplicateListingInput plus the create afterWrite) and group duplicate path copy memberships, prices, and child edges but never read listingAttributeOptions.getIds(sourceId) or insert those option ids for the clone. In the normal workflow of duplicating a listing or group for another date, the public attribute labels disappear from the new listings and attribute filters no longer find them, even though the rest of the listing settings are preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 64ed02b. Added copyListingAttributeOptionsTx which copies listing_attribute_options rows from source to clone within the same transaction as the other duplicate copies. Called from both the single-listing duplicate afterWrite (writeCreateListingGroups) and the group duplicate batch path (bulk-actions.ts). Regression test in server-attributes.test.ts: "copies attribute selections onto the duplicate".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64ed02b771
ℹ️ 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".
| child, | ||
| childDatesById, | ||
| )}>${label}</p>${priceHtml}`; | ||
| )}>${label}</p>${priceHtml}${attributesHtml}`; |
There was a problem hiding this comment.
When the only bookable child is hidden, renderSoleChildOption already suppresses the child name and price (visible ? ... : ""), and the existing render-selector test documents that nothing visible should identify that child. Appending attributesHtml unconditionally here leaks any selected attribute names/options for that hidden child on the public ticket page, so a hidden add-on with attributes can still be exposed to buyers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this slipped through the merge. #1683 was merged (as 09e63164) before this thread could be addressed, so the leak landed on main: renderSoleChildOption appended attributesHtml unconditionally after the suppressed label, leaking a hidden sole child’s attribute names/options to the public ticket page.
Fixed in follow-up #1708: attributesHtml is now gated on the same visible flag that already suppresses the name and price (reservations.tsx:665), so a hidden sole child keeps its data markers and pay-more price input (the fold/compat scripts need them) but shows nothing identifying.
Regression tests in render-selector.test.ts: a visible sole child shows its attributes (positive), and a hidden sole child does not leak them (the bug — fails before the fix, passes after).
The order gallery and multi-listing ticket pages loaded attributes for every listing but never rendered them. Now each order card shows its selected attributes, and each listing row on a multi-listing ticket page shows its own attributes (the single-listing header already did).
…ows, CSV export filter, remove alias - Thread attributesByListing through renderPackageMemberRow so package member rows show their selected attributes (same as standalone rows) - Render attributesHtml in the isClosed branch of renderListingRow so closed listings still show their attributes on multi-listing pages - Carry the active type + attribute filters through to the CSV export link and handler so the download stays aligned with the filtered table - Remove getListingAttributeOptionIds alias export (per AGENTS.md — callers use listingAttributeOptions.getIds directly) - Strengthen the multi-listing attribute test to assign distinct attributes to both listings
- Replace array .map/.filter/.flatMap and for...of with curried map, filter, flatMap, reduce, and pipe from #fp in listing-attribute-filter.ts and attributes.ts (AGENTS.md: prefer #fp over imperative loops) - Inline col.generated/col.encrypted directly in attributeOptionsTable schema to eliminate the jscpd clone with questions/tables.ts - Drop unused export on AttributeFilterOption type (no external caller)
…sting ids - CSV export: load loadListingAttributeFilterContext from the full listing set (before filterListingsByType) so an attribute that only exists on a different listing type is still recognised rather than silently dropped - Ticket page: include child listing ids (from childrenByParentId) in getSelectedAttributesForListings so attributes on add-on/child listings render on parent ticket pages
Add tests that pass attributesByListing with real data through the single-package and multi-package+standalone rendering paths in buildPageListingRows, covering the previously uncovered lines (1537-1542, 1545-1552, 1572-1576) and their ?? branches.
… listing/group duplicate - Thread attributesByListing into ChildRenderCtx and render child attributes in renderChildOption/renderSoleChildOption so add-on child listings show their attributes on parent ticket pages - Add copyListingAttributeOptionsTx and call it from the single-listing duplicate afterWrite path so attribute selections are copied onto clones - Copy attribute selections in the group duplicate batch path too - Regression test: 'copies attribute selections onto the duplicate'
…d ?? branches The three ?? new Map() fallbacks were dead code — ticketPage always passes attributesByListing (defaulted to new Map() at the ticketPage level), so the ?? right-hand side never fired. Making the property required and destructuring it removes the uncovered branches.
1 participant
What changed
This adds a multiple-choice attributes system for listings.
Owners can now create listing attributes, add options to them, reorder both attributes and options, and choose the options that apply to each listing from the listing page. Attributes show on public listing cards (homepage, order gallery, and multi-listing ticket pages), single and multi-listing ticket page headers and rows, and admin listing pages can be filtered by selected attribute options.
Why
Some listings need simple labels such as level, format, audience, or location style. These should help people understand and sort listings, without changing how bookings work.
What this means for users
Operators can keep useful listing details in one reusable place instead of repeating them in listing descriptions. Visitors see those details on every public surface — the homepage card grid, the order gallery, and the ticket/booking page (whether single-listing or multi-listing). Booking capacity, pricing, checkout, and registration rules are unchanged.
Checks
deno task precommitSummary by CodeRabbit