Add listing field with CardPill and change action to create PR modal #4287
Conversation
…modal Replace static listing name and realm fields with a CardPill component that links to the listing, and add a "Change" button to select a different listing via the catalog chooser. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
Preview deployments |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ce376cf64
ℹ️ 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".
| <CardPill | ||
| @cardId={{this.listingId}} | ||
| @urlForRealmLookup={{this.listingId}} | ||
| @displayTitle={{this.listingTitle}} | ||
| class='listing-pill' |
There was a problem hiding this comment.
Wire listing pill click to open selected listing
The new listing field renders CardPill without an @onClick handler, so it stays non-interactive and does not navigate anywhere. In CardPill, click behavior is only enabled when @onClick is provided, so this change does not actually provide a linked listing field; users cannot open/verify the selected listing before submitting the PR.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We don't want users to click to view the listing in the modal. The purpose of this field is to allow users to select a listing for submission only. So the current non-interactive behavior is correct
Host Test Results 1 files ±0 1 suites ±0 2h 13m 13s ⏱️ - 1m 11s Results for commit 2ce376c. ± Comparison against base commit 20a185a. This pull request removes 2 and adds 5 tests. Note that renamed tests count towards both. |
There was a problem hiding this comment.
Pull request overview
Updates the operator-mode “Create PR” modal to display the selected listing as a CardPill (instead of separate listing/realm text fields) and introduces a “Change” action to re-select a listing via the catalog chooser.
Changes:
- Replace static listing/realm display with a
CardPillin the Create PR modal UI. - Add “Change” listing action powered by
chooseCard, and update submission to use the selected listing. - Expand integration tests to cover updated modal behaviors (pill, absence of realm field, cancel, submit success).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/host/app/components/operator-mode/create-pr-modal.gts | Renders listing as CardPill, adds “Change” listing chooser flow, adjusts submission payload and modal copy/styles. |
| packages/host/tests/integration/components/create-pr-modal-test.gts | Updates/extends modal integration tests for the new UI and behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{#if this.listingId}} | ||
| <CardPill | ||
| @cardId={{this.listingId}} | ||
| @urlForRealmLookup={{this.listingId}} | ||
| @displayTitle={{this.listingTitle}} | ||
| class='listing-pill' | ||
| /> |
There was a problem hiding this comment.
The listing “pill” is rendered as a non-interactive CardPill (no @OnClick), so it doesn’t actually link/open the listing as described in the PR/issue. Consider wiring an @onClick handler (e.g. using operatorModeStateService.openCardInInteractMode(this.listingId, 'isolated')) or otherwise making the pill an anchor/button so users can navigate to the listing from the modal.
There was a problem hiding this comment.
- The CardPill is used as a label/indicator, same pattern as attachments.gts which also uses CardPill without @OnClick
- The modal already has a "Change" button for interaction — making the pill itself clickable would be redundant
- The PR modal's purpose is to create a PR, not to navigate to the listing — opening the listing would take the user away from the modal flow
| let realm = | ||
| this.realm.realmOfURL(new URL(currentListingId))?.href ?? payload.realm; |
There was a problem hiding this comment.
When submitting after a listing has been changed, realmOfURL(new URL(currentListingId)) can return undefined if that realm isn’t currently in realm.realms, and the code silently falls back to payload.realm. That can submit a PR request against the wrong realm. Safer options: (a) throw if the realm can’t be derived for the selected listing, or (b) derive the realm deterministically from currentListingId (instead of falling back to the original payload realm).
| let realm = | |
| this.realm.realmOfURL(new URL(currentListingId))?.href ?? payload.realm; | |
| let realmURL = this.realm.realmOfURL(new URL(currentListingId)); | |
| if (!realmURL) { | |
| throw new Error('Cannot determine realm for selected listing'); | |
| } | |
| let realm = realmURL.href; |
| assert.dom('[data-test-create-pr-realm]').exists(); | ||
| assert.dom('[data-test-create-pr-change-listing-button]').doesNotExist(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The new “Change” listing workflow isn’t covered by tests (only the absence case is asserted). Add a test that makes the chooser available (and stubs chooseCard to return a different listingId), clicks the Change button, and asserts that the pill updates and the submit action uses the newly selected listingId.
| test('can change listing via catalog chooser and submit with new listingId', async function (assert) { | |
| await renderComponent( | |
| class TestDriver extends GlimmerComponent { | |
| <template><OperatorMode @onClose={{noop}} /></template> | |
| }, | |
| ); | |
| ctx.operatorModeStateService.showCreatePRModal({ | |
| realm: testRealmURL, | |
| listingId: `${testRealmURL}Listing/1`, | |
| listingName: 'My Listing', | |
| }); | |
| // Make the catalog chooser available and stub chooseCard to return a different listing | |
| let newListingId = `${testRealmURL}Listing/2`; | |
| let newListingName = 'My Other Listing'; | |
| // Depending on how setupOperatorModeTests exposes the chooser, adapt this stub: | |
| // here we assume a chooser object on the test context. | |
| // @ts-ignore - test-time mutation | |
| ctx.catalogChooser = ctx.catalogChooser || {}; | |
| // @ts-ignore - test-time mutation | |
| ctx.catalogChooser.isAvailable = true; | |
| // @ts-ignore - test-time mutation | |
| ctx.catalogChooser.chooseCard = async () => { | |
| return { | |
| listingId: newListingId, | |
| listingName: newListingName, | |
| }; | |
| }; | |
| let submittedListingId: string | null = null; | |
| // Stub the submit handler to capture the listingId used when submitting | |
| // @ts-ignore - test-time mutation | |
| ctx.operatorModeStateService.submitCreatePR = (options: { listingId: string }) => { | |
| submittedListingId = options.listingId; | |
| }; | |
| await waitFor('[data-test-create-pr-modal]'); | |
| // Initially shows the original listing | |
| assert.dom('[data-test-create-pr-listing-name]').includesText('My Listing'); | |
| // Click the Change button to open the chooser and select the new listing | |
| await click('[data-test-create-pr-change-listing-button]'); | |
| // After choosing, the pill should update to the new listing name | |
| assert.dom('[data-test-create-pr-listing-name]').includesText(newListingName); | |
| // Submit the PR and assert the new listingId is used | |
| await click('[data-test-create-pr-submit-button]'); | |
| assert.strictEqual(submittedListingId, newListingId, 'submit uses newly selected listingId'); | |
| }); |
linear: https://linear.app/cardstack/issue/CS-10556/create-pr-modal-should-display-a-field-to-linked-to-the-lisitng
Replace static listing name and realm fields with a CardPill component that links to the listing, and add a "Change" button to select a different listing via the catalog chooser.
Demo
Screen.Recording.2026-03-31.at.10.25.50.PM.mov