Fix listing detection to use inheritance check instead of name suffix #3858
Conversation
Host Test Results 1 files ± 0 1 suites ±0 3h 6m 38s ⏱️ + 1h 28m 33s For more details on these failures, see this check. Results for commit 8fb7d56. ± Comparison against base commit 01aaae4. ♻️ This comment has been updated with latest results. |
packages/base/card-menu-items.ts
Outdated
| if ( | ||
| codeRef && | ||
| isResolvedCodeRef(codeRef) && | ||
| codeRef.module.includes('catalog-app/listing/listing') |
There was a problem hiding this comment.
this isn't a very precise check. the dir it's checking for is not rooted and we are not making any assertions about the origin of the URL. it's also awkward that a card needs to delegate to logic in its enclosing card in order to render a thing that is specific to the enclosing card. usually delegation works in the opposite direction. is there a better way we can supply this functionality by having the listing card actually provide the menu items as opposed to having this card provide listing menu item--maybe like a {{yield}}. my fear is that this card will be a dumping ground for menu items for all sorts of other cards if they also follow this same pattern. it also probably exposes a short coming in the API if this card needs to change in order to provide menu items for other cards
There was a problem hiding this comment.
my recommendation
- introduce symbol for listing
- improve isListingCard to check if the symbol is in the listing definition for createListing
- move create a PR command out of cardDef and move it to listing menu item
- dont do any of the recursive checks
There was a problem hiding this comment.
@habdelra there is one part which addresses your concern which we have removed ie the Create PR command sitting on CardDef
BUT I know there is another place that will make you concern too -- is here
boxel/packages/base/card-menu-items.ts
Line 158 in 94b95ef
But, the reason we use this is bcos we wudn want to display create listing for a listing card wud we. I think we only want to have that option if we are on a CardDef instance which is not a listing card.
Do you mean? if we want to exclude this command from appearing, we shud do it on the listing class too via filtering out. Perhaps thats the correct way
There was a problem hiding this comment.
so looking thru this component it looks like lucas was just following a pattern that was established. there is one other places where we are doing this same thing. in general i disagree with these. I think this is the code review that finally brought the issue to my attention. If I look at that component, the menu items that feel really specific to particular card types and not just a general item are:
- make a PR
- create listing with AI
I think all the other menu items in there are not specific to the card type--any card can participate. so the method name getDefaultCardItems() makes lots of sense for those general items. But i think if you have a menu item that is specific to a particular card type it cannot be a "default item", because not all cards participate in this behavior.
All card defs have the ability to specify custom menu items as part of their CardDef. why can't we add this into the listing card def class?
There was a problem hiding this comment.
Ok i based upon ur reply. I think we r aligned. I will get lucas logic moved to listing card so it becomes clear
…gs-cards-with-listing-in-name
| if (updateSpecs) { | ||
| menuItems = [...menuItems, updateSpecs]; | ||
| } | ||
| const makeAPR = this.getMakeAPRMenuItem(params); |
There was a problem hiding this comment.
probably can get better naming here
There was a problem hiding this comment.
Sure, I’ll rename it to "getCreatePRMenuItem".
packages/base/card-menu-items.ts
Outdated
| @@ -230,5 +216,5 @@ function isIndexCard(card: CardDef): boolean { | |||
| } | |||
|
|
|||
| function isListingCard(card: CardDef): boolean { | |||
There was a problem hiding this comment.
cant u just remove this helper all together
There was a problem hiding this comment.
move it into runtime-common. Surely other places will want to use it
There was a problem hiding this comment.
in that implementation, use isCardDef() + isListing inside
There was a problem hiding this comment.
Sure, thanks for the advice. I’ve addressed your feedback.
Preview deployments |
…gs-cards-with-listing-in-name
linear:https://linear.app/cardstack/issue/CS-10067/islistingcard-check-incorrectly-flags-cards-with-listing-in-name
Problem:
Previously there is an airbnbListing card (airbnb-listing.gts) doesn't show the "Create listing with AI" button in the menu. After investigating, I found that the isListingCard function checks whether the class name ends with "Listing" to determine if a card is a catalog listing. I think we can improve this function since users may create their own cards with class names ending in "Listing".
Changes
The new implementation (renamed to isListingSubclass) traverses the inheritance chain using getAncestor() and checks if any ancestor's module path includes 'catalog-app/listing/listing'. This correctly identifies only cards that actually inherit from the catalog Listing base class.
Before:
Screen.Recording.2026-01-16.at.5.03.45.PM.mov
After: