Skip to content
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.

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

Playground instance previewed in edit view should save changes #2165

Merged
merged 6 commits into from
Feb 20, 2025

Conversation

FadhlanR
Copy link
Contributor

@FadhlanR FadhlanR commented Feb 19, 2025

  • Refactored the auto-save handler by moving it to the store service, based on @habdelra's suggestion.
  • Made cardResource in the playground panel an auto-saving card.

Copy link

github-actions bot commented Feb 19, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   23m 16s ⏱️ +35s
771 tests +2  769 ✔️ +2  2 💤 ±0  0 ±0 
776 runs  +2  774 ✔️ +2  2 💤 ±0  0 ±0 

Results for commit 1973853. ± Comparison against base commit 22ebcca.

This pull request removes 1 and adds 3 tests. Note that renamed tests count towards both.
Chrome 133.0 ‑ Acceptance | AI Assistant tests: can display and remove auto attached card
Chrome 133.0 ‑ Acceptance | AI Assistant tests: can display and remove auto attached file
Chrome 133.0 ‑ Acceptance | code-submode | playground panel: trigger auto saved in edit format
Chrome 133.0 ‑ Integration | realm indexing and querying: absolute urls will be serialised into relative into relative code-ref fields

♻️ This comment has been updated with latest results.

@FadhlanR FadhlanR marked this pull request as ready for review February 19, 2025 14:42
Comment on lines +319 to +321
if (!this.cardApiCache) {
this.cardApiCache = await this.cardService.getAPI();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to cache this--it's actually cached in the loader, you can just get it each time that you need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's cached so that it can be reused in the unloadResource function. Otherwise, unloadResource would have to be an async function, but we need it to be synchronous for use in registerDestructor. Do you have any other idea how to get cardAPI in unreloadResource?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah! I see! make sense let's leave it as is

Copy link
Contributor

@habdelra habdelra left a comment

Choose a reason for hiding this comment

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

this looks really good! exactly like we discussed

Copy link
Contributor

@tintinthong tintinthong left a comment

Choose a reason for hiding this comment

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

I have tested this and I dont see any degression in auto save. LGTM

@FadhlanR FadhlanR merged commit 868be56 into main Feb 20, 2025
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants