[APP-4267] Show Mermaid render failure callout#10432
Conversation
Co-Authored-By: Oz <oz-agent@warp.dev>
Rebuild Mermaid block layout after visible layout-affecting asset loads complete in editable editor states, so permanent Mermaid render failures use the compact failure height instead of retaining the loading placeholder height. Co-Authored-By: Oz <oz-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 |
![oz-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 adds an explicit Mermaid failure/timeout callout by extending the shared Image element with opt-in failed-load and timeout backup elements, then wiring Mermaid diagrams to use those states.
Concerns
- The new process-global image timeout map can grow without bound because entries are only cleared from
paintafter an asset is later observed as loaded, failed, or evicted. Stuck renders and image elements removed before resolution leave their keys behind.
Security
- User-controlled Markdown can introduce many unique Mermaid asset sources, so the unbounded timeout map creates a resource-growth path in the renderer process.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| }; | ||
|
|
||
| lazy_static! { | ||
| static ref IMAGE_LOAD_TIMEOUT_STARTED_AT: Mutex<HashMap<u64, Instant>> = |
There was a problem hiding this comment.
paint later sees the asset as loaded, failed, or evicted; a stuck load or a removed image element leaves its timeout key forever, so user-controlled Mermaid sources can grow it without bound. Add expiry/eviction or tie this state to the asset cache lifecycle instead of an unbounded static map.
| pub fn on_load_failure(mut self, element: Box<dyn Element>) -> Self { | ||
| self.failed_to_load_element = Some(element); | ||
| self | ||
| } | ||
|
|
||
| pub fn on_load_timeout(mut self, timeout: Duration, element: Box<dyn Element>) -> Self { |
There was a problem hiding this comment.
Its fine cuz this was the pattern already used for before_load_element but it probably wouldve been better to make these kind of API take a closure so the element doesn't need to be unnecessarily constructed if its not gonna be used
|
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 |
There was a problem hiding this comment.
Overview
This PR adds explicit Mermaid image failure and timeout fallback rendering, compact layout for permanent Mermaid load failures, and focused coverage for the shared Image fallback behavior and Mermaid sizing.
Concerns
- No blocking correctness, security, or error-handling concerns found in the reviewed diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
2 participants
Description
Adds an explicit failure state for Mermaid diagrams so rendered markdown surfaces no longer leave users on an indefinite “Rendering Mermaid diagram…” placeholder when rendering fails or stays unresolved too long.
This PR:
Imageelement with opt-in failed-load and load-timeout backup elements, preserving existing behavior for callers that do not opt in.Failed to render Mermaid diagramfor permanent load failures and after the Mermaid render timeout.Agent conversation: https://staging.warp.dev/conversation/2a1dc8f7-5ddb-4b22-bdea-384e2281624a
Linked Issue
https://linear.app/warpdotdev/issue/APP-4267/show-failure-callout-when-mermaid-diagram-rendering-gets-stuck
ready-to-specorready-to-implement.Testing
Automated validation:
cargo fmt --manifest-path /Users/zach/Projects/warp_3/Cargo.toml --all --checkPATH=/tmp/warp-corepack-bin:$PATH cargo nextest run --manifest-path /Users/zach/Projects/warp_3/Cargo.toml --no-fail-fast --workspace mermaid imagePATH=/tmp/warp-corepack-bin:$PATH cargo clippy --manifest-path /Users/zach/Projects/warp_3/Cargo.toml --workspace --all-targets --all-features --tests -- -D warningsNew/updated test coverage includes:
Mermaid layout uses the existing default height while loading.
Failed Mermaid assets use a compact failed-callout height.
Imageprefers a provided failed-load element for failed assets.Imagefalls back to the before-load element for failed assets when no failed-load element is provided.Loading images switch to a timeout element after the configured timeout.
Timeout start state survives rebuilding an
Imageelement for the same asset source.I have manually tested my changes locally with
./script/runScreenshots / Videos
Mermaid render failure callout captured from the PR branch with a forced Mermaid asset-load failure:
Agent Mode
CHANGELOG-BUG-FIX: Fixed Mermaid diagrams showing an indefinite rendering placeholder when rendering fails or times out.
Co-Authored-By: Oz oz-agent@warp.dev