fix(quarto): wait for the document parse before restoring cached output - #15332
Conversation
|
E2E Tests 🚀 Why these tags?
More on automatic tags from changed files. |
midleman
changed the title
jmcphers
left a comment
jmcphers
left a comment
There was a problem hiding this comment.
I think that rather resolving the race with a retry, we should adjust the model so that callers can distinguish between "no cells found because there aren't any" and "no cells found because parsing isn't done yet"
b7fcd84 to
1da1cc3
Compare
midleman
changed the title
The cached-output restore read the model's cell list without checking whether it was current, so a restore that ran during a pending re-parse matched nothing by content hash and completed, dropping every cached output. The old guard inferred parse state from an empty cell list and only covered untitled documents with no cache on disk. Add isParsed and whenParsed() to IQuartoDocumentModel so callers can tell "this document has no code cells" from "the cells aren't known yet", and have the restore await the parse before either content-hash lookup.
1da1cc3 to
25ba1c7
Compare
Add a regression test for the restore that runs before the document is parsed, and one asserting that a parsed document reporting no cells completes the restore rather than waiting for a parse that will never add cells. Pin the ordering of the parse wait against the content-hash fallback: the fallback keys off the model's cells, so the hashes it receives show whether it ran before or after the parse. Cover the model contract directly: isParsed goes false for a pending debounced re-parse and true once it lands, and whenParsed() returns without waiting when the cells are already current.
25ba1c7 to
9aa080f
Compare
i made the updates as suggested. the model now has note that i removed the untitled deferral. its comment says it covers an untitled doc restored via hot exit whose content hasn't loaded yet, but a note: the e2e failure that sent me here was the save-as gate fixed in #15331. the real bug left is that main drops cached output whenever a cache is found while the cells are stale. the new tests fail there and pass here. |
| * if it already does. Callers that read `cells` to decide something about | ||
| * the document, rather than to react to an edit, should await this first. | ||
| */ | ||
| whenParsed(): Promise<void>; |
There was a problem hiding this comment.
Idiomatically we'd use a Barrier for this.
I did not! |
Replaces the separate timeout-field and onDidParse-event reads behind isParsed/whenParsed with a single Barrier, so both answers come from one object rather than two mechanisms that have to be kept in agreement. No behavior change.
2 participants
Summary
Restoring a Quarto document's cached inline output read the model's cell list without checking whether it was current. A restore that ran while a re-parse was still pending matched nothing by content hash and marked itself complete, dropping every cached output. The document model now reports its parse state, so the restore waits instead of inferring it from an empty cell list.
IQuartoDocumentModel.isParsedseparates "this document has no code cells" from "the cells aren't known yet"whenParsed()lets the restore await the parse instead of retrying on a parse eventNote: the demonstrable trigger is the 100ms debounced re-parse. The close+reopen ordering this PR was opened around could not be reproduced, and the e2e failure that led here was diagnosed and fixed separately in #15331.
Release Notes
New Features
Bug Fixes
Validation Steps
@:quarto @:web @:win
E2E Triage Diagnosis
🟢 High confidence -- CORRECTED (supersedes the parse-race summary below): on Save As the Quarto output cache is never rebound from the untitled URI to the saved document, because _transferCacheFromUntitled is gated on this._documentUri.scheme === (file) (quartoOutputManager.ts:391) and in web a saved document URI is vscode-remote. The post-save assertion still passes via the in-memory findCacheByContentHash fallback to the live untitled cache; that fallback cannot survive a window reload, so post-reload restore finds no cache under the saved URI and mints zero view zones.