[APP-5344] Defer layout for AI plan document editors - #15579
Conversation
Restoring a conversation replays every CreateDocuments/EditDocuments action, rebuilding a NotebooksEditorModel per document revision. NotebooksEditorModel hardcoded RenderState's lazy_layout to false, so every one of those editors font-shaped its entire markdown document synchronously during restore, even though archived revisions are only reachable via version history and are almost never opened. Measured while restoring a plan-heavy conversation: 175 eager layout passes covering 41,513 blocks (avg 237, max 646 per delta) inside ~2s, 164 of them owned by NotebooksEditorModel. Thread a lazy_layout flag through new_internal, add new_unbound_lazy, and use it for AIDocumentModel's editors so layout happens on first element layout -- the same mechanism code diff views already rely on. handle_render_model_event already early-returns while rte_window_id is None, so unbound documents were not acting on layout events during restore anyway. Co-Authored-By: Warp <agent@warp.dev>
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![warp-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR introduces lazy layout for AI plan document editors by threading a lazy_layout flag through NotebooksEditorModel and switching AI documents to the new lazy unbound constructor.
Concerns
- Lazy-restored notebook editors can miss the child-model rebuild needed for code blocks, embedded items, and rendered Mermaid controls when they are first displayed.
- One added public doc comment documents a specific caller path rather than keeping the constructor docs focused on behavior.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /warp-agent-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Only conversation restore and version archiving defer layout now. Documents that are about to be shown -- agent CreateDocuments, streaming plans, and opening a saved plan from Warp Drive -- lay out on creation as before. This keeps the full measured win, since every editor converted in the restore trace came from the restore paths, while leaving live document behavior byte-identical. Also add paired tests for child model population on an editor that is unbound when its content is set, which is what restore does. handle_render_model_event drops render events while rte_window_id is None, so the LayoutUpdated emitted when content is first set never reaches child_models. Recovery comes from the first layout reporting a viewport width change, which drives a debounced rebuild_layout whose buffer edit emits a second LayoutUpdated. The tests assert the eager and deferred paths behave identically, so restored plans keep their NotebookCommand/NotebookEmbed models for code blocks, embedded items, and Mermaid controls. Co-Authored-By: Warp <agent@warp.dev>
EditorLayoutTiming::OnCreate read as though layout happened exactly once at
creation, when it just means layout is not deferred. LayoutTiming::{Eager, Lazy}
matches the vocabulary already used by RenderState's lazy_layout flag.
Also trim the comments added by this branch to the repo guidelines: drop the
call-site enumeration from the doc comments, keep the inline comments to
non-obvious rationale for choosing eager over lazy, and stop narrating steps in
the test helper's doc comment.
Co-Authored-By: Warp <agent@warp.dev>
|
/warp-agent-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds a lazy layout path for unbound AI plan document editors during conversation/document restoration, while keeping eagerly displayed creation/opening paths on the existing eager layout behavior. It also adds focused notebook editor tests covering child model recovery after an unbound editor is later bound and laid out.
Concerns
- No blocking correctness, security, spec-drift, comment-guideline, or test-guideline concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /warp-agent-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
@kevinyang372 is the better choice for reviewer here; will leave it to him. |
2 participants
Description
Restoring a conversation replays every
CreateDocuments/EditDocumentsaction to rehydrate plan documents (restore_ai_documents_from_exchanges). Each revision builds aNotebooksEditorModel, and that type hardcodedRenderState'slazy_layouttofalse, so every revision font-shaped its entire markdown document synchronously during startup — including archived revisions that are only reachable through version history and are almost never opened.Two things made that expensive:
reset_with_markdowngoes throughBuffer::replace, whoseEditDelta.new_linesisstyled_blocks_in_range(1..max_charoffset)— the full buffer, not an incremental edit.create_new_document_versionsnapshots the current content into version history by serializing it back to markdown and re-parsing it into a fresh editor, and then the caller resets the live editor with the next revision. In the captured trace, 94 of 174 adjacent layout passes are byte-identical, back-to-back duplicates for exactly this reason.Cost therefore scales as revisions × document size, and plan documents grow over the course of a conversation, so long planning sessions degrade sharply.
Measurements
Restoring the same plan-heavy conversation, with
RenderStateinstrumented locally:The 75 remaining passes are ~3-block comment markdown editors, not plan documents.
Approach
NotebooksEditorModel::new_internaltakes alazy_layoutflag, exposed asnew_unbound_lazy. Layout then happens on first element layout viaRichTextElement::layout→try_layout_pending_edits, which is the same deferralCodeEditorViewalready uses for code diffs.A
LayoutTimingenum picks per call site, so only bulk rehydration defers:Lazy—restore_document,apply_persisted_content, andcreate_new_document_version(the archived-revision snapshot).Eager—create_document(agentCreateDocuments),get_or_create_streaming_document_for_create_documents(streaming plans, which auto-open their pane), andcreate_document_from_notebook(opening a saved plan from Warp Drive).Every editor converted in the trace came from the restore paths, so this keeps the full win while leaving documents that are about to be displayed byte-identical to master. Bound editors — notebooks and comment editors — are untouched and still use
new/new_unbound.Not included here: the archived-revision snapshot still serializes and re-parses the document even though it no longer font-shapes it. Storing the markdown and materializing that editor on demand would remove the remaining duplicated work; worth doing as a follow-up if restore is still hot.
Linked Issue
Tracked in Linear as APP-5344. No corresponding GitHub issue.
ready-to-specorready-to-implement.Testing
./script/runVerified by instrumenting
RenderState::add_pending_editandtry_layout_pending_edits, then restoring the same conversation on a locally-run build before and after; the numbers above come from those two runs.LAZY-FLUSHwas 0 in both, confirming the deferred layouts are genuinely never performed for documents that are never opened — deferral is elimination here, not postponement.New tests
test_eager_layout_populates_child_models_after_bindingandtest_deferred_layout_populates_child_models_after_bindingshare a body and cover the case review flagged: an editor that is unbound when its content is set, which is exactly what restore does.handle_render_model_eventdrops render events whilerte_window_idisNone, so theLayoutUpdatedemitted when content is first set never reacheschild_models. That holds with and without deferred layout — both tests assert zero child models at that point. Recovery comes from the first layout reporting a viewport width change (ViewportState::viewport_sizeflagsneeds_layoutwhenever the width differs, and an unbound editor starts at width 0), which drives a debouncedrebuild_layoutwhose buffer edit emits a secondLayoutUpdated. Both tests then assert the code block has itsNotebookCommandmodel.The pair exists so the eager path acts as a live control: if deferred layout ever diverges, one test passes and the other fails. This is what guards restored plans keeping their code block, embedded item, and Mermaid controls.
ChildModels::updatereadscontent.outline_blocks()— the buffer, not the laid-out render tree — so it does not depend on layout having been applied.Other coverage
ai_document— 15/15 pass, including version isolation, revert, and streaming-vs-reset equivalence.notebooks::editor— 86/86 pass.test_copy_ai_document_as_markdown_from_overflow_menucreates a document throughcreate_document, opens the pane, and blocks on a layout-dependent assertion (the overflow button's position must land in the position cache) before copying as markdown../script/formatand all threecargo clippy ... -D warningsinvocations fromscript/presubmitpass.Reviewer note:
NotebooksEditorModelhas not previously run under deferred layout, so a spot-check of opening a restored plan pane — one with a code block and a Mermaid diagram — and switching between versions is still worthwhile.Screenshots / Videos
N/A — no visual change intended.
Agent Mode
CHANGELOG-IMPROVEMENT: Faster conversation restore for conversations containing plan documents.