Skip to content

Add isLoading checking for the Example field in Create Listing Modal #4250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lucaslyl wants to merge 3 commits into from
Closed

Add isLoading checking for the Example field in Create Listing Modal #4250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
85c5592
Fix missing examples in create listing modal and search resource sub…
lucaslyl Mar 27, 2026
8f19f26
fix lint
lucaslyl Mar 27, 2026
c2d7999
fix copilot feedback
lucaslyl Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 27 additions & 17 deletions packages/host/app/components/operator-mode/create-listing-modal.gts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,12 @@ export default class CreateListingModal extends Component<Signature> {

@tracked private selectedExamples: PickerOption[] = [];

private instancesSearch = getSearch<CardDef>(this, getOwner(this)!, () =>
this.codeRef ? { filter: { type: this.codeRef } } : undefined,
private instancesSearch = getSearch<CardDef>(
this,
getOwner(this)!,
() => (this.codeRef ? { filter: { type: this.codeRef } } : undefined),
() => (this.payload?.targetRealm ? [this.payload.targetRealm] : undefined),
{ isLive: true },
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid enabling live search without unsubscribe on close

Setting isLive: true here introduces a persistent background subscription after the modal is dismissed. CreateListingModal is always mounted (operator-mode/container.gts renders it unconditionally), and when the payload is cleared codeRef becomes undefined; in SearchResource.modify() (packages/host/app/resources/search.ts) that path returns early on query === undefined without unsubscribing existing live subscriptions or clearing #previousQuery. As a result, later realm index events can continue triggering search.perform(...) for the stale query even while the modal is closed, causing unnecessary live refresh traffic/work until the whole component is destroyed.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@tintinthong tintinthong Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good comment for leaking abstraction

Copy link
Copy Markdown
Contributor

@habdelra habdelra Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so one observastion Ed noted way back when we did the card resources refactor is that "isLive" is kind of a lie. when you have a store that is driving your state, everything is always live. in fact you actually have to go thru weird acrobatics to make things not live. perhaps this is the same.

Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getSearch(..., { isLive: true }) call does not provide realms, so the search resource will treat it as “all available realms” and will subscribe live to every realm in realmServer.availableRealmURLs. That can be very expensive (many live subscriptions + refreshes) for a modal; consider constraining realms (e.g. targetRealm and/or the realm of codeRef.module) or keeping this search non-live unless you explicitly need cross-realm live updates.

Suggested change
{ isLive: true },
{ isLive: false },

Copilot uses AI. Check for mistakes.
);

private get instances(): CardDef[] {
Expand Down Expand Up @@ -119,6 +123,10 @@ export default class CreateListingModal extends Component<Signature> {
: this.initialSelected;
}

private get isCreateDisabled(): boolean {
return this.createListing.isRunning || this.instancesSearch.isLoading;
}

@action private onExampleChange(selected: PickerOption[]) {
this.selectedExamples = selected;
}
Expand Down Expand Up @@ -208,20 +216,22 @@ export default class CreateListingModal extends Component<Signature> {
</div>
</FieldContainer>

{{#if this.instanceOptions.length}}
<FieldContainer @label='Examples' class='field'>
<div class='field-contents' data-test-examples-container>
<CardInstancePicker
@placeholder='Select examples to include in the listing'
@options={{this.instanceOptions}}
@selected={{this.effectiveSelected}}
@onChange={{this.onExampleChange}}
@maxSelectedDisplay={{3}}
data-test-create-listing-examples
/>
</div>
</FieldContainer>
{{/if}}
{{#unless this.instancesSearch.isLoading}}
{{#if this.instanceOptions.length}}
Copy link
Copy Markdown
Contributor

@tintinthong tintinthong Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still need to remove this right

Copy link
Copy Markdown
Contributor

@tintinthong tintinthong Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .length part

<FieldContainer @label='Examples' class='field'>
Copy link
Copy Markdown
Contributor Author

@lucaslyl lucaslyl Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add loading checking and also the instance length check else the fieldcontainer will still display if the search result is empty

Copy link
Copy Markdown
Contributor Author

@lucaslyl lucaslyl Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{{#if this.instanceOptions.length}} =. showExampleRow

<div class='field-contents' data-test-examples-container>
<CardInstancePicker
@placeholder='Select examples to include in the listing'
@options={{this.instanceOptions}}
@selected={{this.effectiveSelected}}
@onChange={{this.onExampleChange}}
@maxSelectedDisplay={{3}}
data-test-create-listing-examples
/>
</div>
</FieldContainer>
{{/if}}
{{/unless}}
Comment on lines +219 to +234
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Examples picker is now hidden while instancesSearch.isLoading, but the modal’s Create button remains enabled. If the user clicks Create before the search finishes, selectedExampleURLs can resolve to an empty list (because instances/instanceOptions haven’t populated yet), resulting in a listing created without the user’s intended examples. Consider disabling Create while instancesSearch.isLoading and/or having selectedExampleURLs fall back to payload.openCardIds when options haven’t loaded yet.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@lucaslyl lucaslyl Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense ,agree with this


</:content>
<:footer>
Expand Down Expand Up @@ -249,7 +259,7 @@ export default class CreateListingModal extends Component<Signature> {
@kind='primary'
@size='tall'
@loading={{this.createListing.isRunning}}
@disabled={{this.createListing.isRunning}}
@disabled={{this.isCreateDisabled}}
{{on 'click' (perform this.createListing)}}
{{onKeyMod 'Enter'}}
data-test-create-listing-confirm-button
Expand Down
6 changes: 6 additions & 0 deletions packages/host/app/resources/search.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,12 @@ export class SearchResource<
}

if (query === undefined) {
this.#previousQueryString = undefined;
this.#previousQuery = undefined;
for (let subscription of this.subscriptions) {
subscription.unsubscribe();
}
this.subscriptions = [];
Comment thread
lucaslyl marked this conversation as resolved.
return;
}

Expand Down
129 changes: 129 additions & 0 deletions packages/host/tests/integration/resources/search-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,135 @@ module(`Integration | search resource`, function (hooks) {
);
});

test(`setting query to undefined clears cached state and re-runs search on next query`, async function (assert) {
let query: Query | undefined = {
filter: {
on: {
module: `${testRealmURL}book`,
name: 'Book',
},
eq: {
'author.lastName': 'Abdel-Rahman',
},
},
};
let args = {
query,
realms: [testRealmURL],
isLive: false,
isAutoSaved: false,
storeService,
owner: this.owner,
} satisfies SearchResourceArgs['named'];

let search = getSearchResourceForTest(loaderService, () => ({
named: args,
}));
await search.loaded;
assert.strictEqual(
search.instances.length,
2,
'initial search returns 2 books',
);

// Simulate modal close: set query to undefined
args = { ...args, query: undefined };
search.modify([], args);
Comment on lines +625 to +627
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using query: undefined as any weakens type-safety in a place where query is already typed as Query | undefined. Prefer passing plain undefined (and if the satisfies SearchResourceArgs['named'] constraint blocks that, it’s a sign the args type should be adjusted rather than casting away types).

Copilot uses AI. Check for mistakes.
await settled();

// Simulate modal reopen with same query
args = { ...args, query };
search.modify([], args);
await search.loaded;

assert.strictEqual(
search.instances.length,
2,
'search re-runs after query was set to undefined and back',
);
});
Comment thread
lucaslyl marked this conversation as resolved.

test(`setting query to undefined tears down live subscriptions`, async function (assert) {
let query: Query | undefined = {
filter: {
on: {
module: `${testRealmURL}book`,
name: 'Book',
},
eq: {
'author.lastName': 'Abdel-Rahman',
},
},
};
let args = {
query,
realms: [testRealmURL],
isLive: true,
isAutoSaved: false,
storeService,
owner: this.owner,
} satisfies SearchResourceArgs['named'];

let search = getSearchResourceForTest(loaderService, () => ({
named: args,
}));
await search.loaded;
assert.strictEqual(
search.instances.length,
2,
'initial live search returns 2 books',
);

// Simulate modal close: set query to undefined
args = { ...args, query: undefined };
search.modify([], args);
await settled();

// Write a new matching card while "modal is closed"
await realm.write(
'books/4.json',
JSON.stringify({
data: {
type: 'card',
attributes: {
author: {
firstName: 'New',
lastName: 'Abdel-Rahman',
},
editions: 0,
pubDate: '2024-01-01',
},
meta: {
adoptsFrom: {
module: `${testRealmURL}book`,
name: 'Book',
},
},
},
} as LooseSingleCardDocument),
);

await settled();

// Instances should NOT have updated since subscriptions were torn down
assert.strictEqual(
search.instances.length,
2,
'live subscription was torn down — no update while query is undefined',
);

// Simulate modal reopen: restore query
args = { ...args, query };
search.modify([], args);
await search.loaded;

assert.strictEqual(
search.instances.length,
3,
'fresh search on reopen picks up the new card',
);
});

test(`can search for file-meta instances using SearchResource`, async function (assert) {
let query: Query = {
filter: {
Expand Down
Loading