Add isLoading checking for the Example field in Create Listing Modal#4250
Add isLoading checking for the Example field in Create Listing Modal#4250lucaslyl wants to merge 3 commits into
Conversation
| {{/if}} | ||
| {{#unless this.instancesSearch.isLoading}} | ||
| {{#if this.instanceOptions.length}} | ||
| <FieldContainer @label='Examples' class='field'> |
There was a problem hiding this comment.
add loading checking and also the instance length check else the fieldcontainer will still display if the search result is empty
There was a problem hiding this comment.
{{#if this.instanceOptions.length}} =. showExampleRow
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbaa69cd63
ℹ️ 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".
| getOwner(this)!, | ||
| () => (this.codeRef ? { filter: { type: this.codeRef } } : undefined), | ||
| undefined, | ||
| { isLive: true }, |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Seems good comment for leaking abstraction
There was a problem hiding this comment.
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.
Preview deployments |
There was a problem hiding this comment.
Pull request overview
This PR improves the “Create Listing” modal’s Examples behavior by switching its instance lookup to live search, and tightens the search resource’s live-subscription lifecycle so subscriptions are torn down when the query is cleared.
Changes:
- Add live-subscription teardown when
SearchResource’s query becomesundefined, plus a helper for unsubscribing. - Add an integration test to verify live search stops reacting to realm index events after the query is cleared.
- Update Create Listing modal to use live search for instances and hide the Examples field while the search is loading.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/host/app/resources/search.ts |
Adds teardownLiveSubscriptions() and clears live subscriptions when the query is absent. |
packages/host/tests/integration/resources/search-test.ts |
Adds coverage ensuring live searches stop re-fetching after clearing the query. |
packages/host/app/components/operator-mode/create-listing-modal.gts |
Enables live search for example instances and guards the Examples field behind isLoading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private teardownLiveSubscriptions() { | ||
| for (let subscription of this.subscriptions) { | ||
| subscription.unsubscribe(); | ||
| } | ||
| this.subscriptions = []; | ||
| } |
There was a problem hiding this comment.
SearchResource.modify() never tears down existing realm subscriptions when isLive flips from true to false (while query remains defined). In that case this.subscriptions stays active and the callback will keep triggering search.perform(...) even though the resource is no longer live. Consider calling teardownLiveSubscriptions() when !isLive (or when transitioning from live→non-live) to ensure subscriptions reflect the current args.
| getOwner(this)!, | ||
| () => (this.codeRef ? { filter: { type: this.codeRef } } : undefined), | ||
| undefined, | ||
| { isLive: true }, |
There was a problem hiding this comment.
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.
| { isLive: true }, | |
| { isLive: false }, |
| {{#unless this.instancesSearch.isLoading}} | ||
| {{#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}} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
make sense ,agree with this
0fc3e47 to
85c5592
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Simulate modal close: set query to undefined | ||
| args = { ...args, query: undefined as any }; | ||
| search.modify([], args); |
There was a problem hiding this comment.
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).
| ); | ||
|
|
||
| // Simulate modal close: set query to undefined | ||
| args = { ...args, query: undefined as any }; |
There was a problem hiding this comment.
Same here: avoid query: undefined as any—it bypasses the intended Query | undefined typing and can mask real type regressions in the test.
| args = { ...args, query: undefined as any }; | |
| args = { ...args, query: undefined }; |
| </FieldContainer> | ||
| {{/if}} | ||
| {{#unless this.instancesSearch.isLoading}} | ||
| {{#if this.instanceOptions.length}} |
There was a problem hiding this comment.
still need to remove this right
| getOwner(this)!, | ||
| () => (this.codeRef ? { filter: { type: this.codeRef } } : undefined), | ||
| undefined, | ||
| { isLive: true }, |
There was a problem hiding this comment.
Seems good comment for leaking abstraction
|
Closing this, since it is being handled in #4283 due to a new requirement for this example field. |
linear: https://linear.app/cardstack/issue/CS-10532/fix-example-missing-in-the-create-listing-modal