markdown preview: Fix panic when parsing empty image tag (#21616)

Closes #21534

While investigating the panic, I noticed that the code was pretty
complicated and decided to refactor parts of it to reduce redundancy.

Release Notes:

- Fixed an issue where the app could crash when opening the markdown
preview with a malformed image tag

crates/markdown_preview/src/markdown_elements.rs

@@ -18,22 +18,19 @@ pub enum ParsedMarkdownElement {
 }
 
 impl ParsedMarkdownElement {
-    pub fn source_range(&self) -> Range<usize> {
+    pub fn source_range(&self) -> Option<Range<usize>> {
-        match self {
+        Some(match self {
             Self::Heading(heading) => heading.source_range.clone(),
             Self::ListItem(list_item) => list_item.source_range.clone(),
             Self::Table(table) => table.source_range.clone(),
             Self::BlockQuote(block_quote) => block_quote.source_range.clone(),
             Self::CodeBlock(code_block) => code_block.source_range.clone(),
-            Self::Paragraph(text) => match &text[0] {
+            Self::Paragraph(text) => match text.get(0)? {
                 MarkdownParagraphChunk::Text(t) => t.source_range.clone(),
-                MarkdownParagraphChunk::Image(image) => match image {
+                MarkdownParagraphChunk::Image(image) => image.source_range.clone(),
-                    Image::Web { source_range, .. } => source_range.clone(),
-                    Image::Path { source_range, .. } => source_range.clone(),
-                },
             },
             Self::HorizontalRule(range) => range.clone(),
-        }
+        })
     }
 
     pub fn is_list_item(&self) -> bool {
@@ -289,104 +286,27 @@ impl Display for Link {
 /// A Markdown Image
 #[derive(Debug, Clone)]
 #[cfg_attr(test, derive(PartialEq))]
-pub enum Image {
+pub struct Image {
-    Web {
+    pub link: Link,
-        source_range: Range<usize>,
+    pub source_range: Range<usize>,
-        /// The URL of the Image.
+    pub alt_text: Option<SharedString>,
-        url: String,
-        /// Link URL if exists.
-        link: Option<Link>,
-        /// alt text if it exists
-        alt_text: Option<ParsedMarkdownText>,
-    },
-    ///  Image path on the filesystem.
-    Path {
-        source_range: Range<usize>,
-        /// The path as provided in the Markdown document.
-        display_path: PathBuf,
-        /// The absolute path to the item.
-        path: PathBuf,
-        /// Link URL if exists.
-        link: Option<Link>,
-        /// alt text if it exists
-        alt_text: Option<ParsedMarkdownText>,
-    },
 }
 
 impl Image {
     pub fn identify(
+        text: String,
         source_range: Range<usize>,
         file_location_directory: Option<PathBuf>,
-        text: String,
+    ) -> Option<Self> {
-        link: Option<Link>,
+        let link = Link::identify(file_location_directory, text)?;
-    ) -> Option<Image> {
+        Some(Self {
-        if text.starts_with("http") {
+            source_range,
-            return Some(Image::Web {
+            link,
-                source_range,
+            alt_text: None,
-                url: text,
+        })
-                link,
-                alt_text: None,
-            });
-        }
-        let path = PathBuf::from(&text);
-        if path.is_absolute() {
-            return Some(Image::Path {
-                source_range,
-                display_path: path.clone(),
-                path,
-                link,
-                alt_text: None,
-            });
-        }
-        if let Some(file_location_directory) = file_location_directory {
-            let display_path = path;
-            let path = file_location_directory.join(text);
-            return Some(Image::Path {
-                source_range,
-                display_path,
-                path,
-                link,
-                alt_text: None,
-            });
-        }
-        None
     }
 
-    pub fn with_alt_text(&self, alt_text: ParsedMarkdownText) -> Self {
+    pub fn set_alt_text(&mut self, alt_text: SharedString) {
-        match self {
+        self.alt_text = Some(alt_text);
-            Image::Web {
-                ref source_range,
-                ref url,
-                ref link,
-                ..
-            } => Image::Web {
-                source_range: source_range.clone(),
-                url: url.clone(),
-                link: link.clone(),
-                alt_text: Some(alt_text),
-            },
-            Image::Path {
-                ref source_range,
-                ref display_path,
-                ref path,
-                ref link,
-                ..
-            } => Image::Path {
-                source_range: source_range.clone(),
-                display_path: display_path.clone(),
-                path: path.clone(),
-                link: link.clone(),
-                alt_text: Some(alt_text),
-            },
-        }
-    }
-}
-
-impl Display for Image {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            Image::Web { url, .. } => write!(f, "{}", url),
-            Image::Path { display_path, .. } => write!(f, "{}", display_path.display()),
-        }
     }
 }

crates/markdown_preview/src/markdown_parser.rs

@@ -214,7 +214,7 @@ impl<'a> MarkdownParser<'a> {
                 break;
             }
 
-            let (current, _source_range) = self.current().unwrap();
+            let (current, _) = self.current().unwrap();
             let prev_len = text.len();
             match current {
                 Event::SoftBreak => {
@@ -314,56 +314,29 @@ impl<'a> MarkdownParser<'a> {
                             ));
                         }
                     }
-                    if let Some(mut image) = image.clone() {
+                    if let Some(image) = image.as_mut() {
-                        let is_valid_image = match image.clone() {
+                        text.truncate(text.len() - t.len());
-                            Image::Path { display_path, .. } => {
+                        image.set_alt_text(t.to_string().into());
-                                gpui::ImageSource::try_from(display_path).is_ok()
+                        if !text.is_empty() {
-                            }
+                            let parsed_regions = MarkdownParagraphChunk::Text(ParsedMarkdownText {
-                            Image::Web { url, .. } => gpui::ImageSource::try_from(url).is_ok(),
+                                source_range: source_range.clone(),
-                        };
+                                contents: text.clone(),
-                        if is_valid_image {
+                                highlights: highlights.clone(),
-                            text.truncate(text.len() - t.len());
+                                region_ranges: region_ranges.clone(),
-                            if !t.is_empty() {
+                                regions: regions.clone(),
-                                let alt_text = ParsedMarkdownText {
+                            });
-                                    source_range: source_range.clone(),
+                            text = String::new();
-                                    contents: t.to_string(),
+                            highlights = vec![];
-                                    highlights: highlights.clone(),
+                            region_ranges = vec![];
-                                    region_ranges: region_ranges.clone(),
+                            regions = vec![];
-                                    regions: regions.clone(),
+                            markdown_text_like.push(parsed_regions);
-                                };
-                                image = image.with_alt_text(alt_text);
-                            } else {
-                                let alt_text = ParsedMarkdownText {
-                                    source_range: source_range.clone(),
-                                    contents: "img".to_string(),
-                                    highlights: highlights.clone(),
-                                    region_ranges: region_ranges.clone(),
-                                    regions: regions.clone(),
-                                };
-                                image = image.with_alt_text(alt_text);
-                            }
-                            if !text.is_empty() {
-                                let parsed_regions =
-                                    MarkdownParagraphChunk::Text(ParsedMarkdownText {
-                                        source_range: source_range.clone(),
-                                        contents: text.clone(),
-                                        highlights: highlights.clone(),
-                                        region_ranges: region_ranges.clone(),
-                                        regions: regions.clone(),
-                                    });
-                                text = String::new();
-                                highlights = vec![];
-                                region_ranges = vec![];
-                                regions = vec![];
-                                markdown_text_like.push(parsed_regions);
-                            }
-
-                            let parsed_image = MarkdownParagraphChunk::Image(image.clone());
-                            markdown_text_like.push(parsed_image);
-                            style = MarkdownHighlightStyle::default();
                         }
+
+                        let parsed_image = MarkdownParagraphChunk::Image(image.clone());
+                        markdown_text_like.push(parsed_image);
+                        style = MarkdownHighlightStyle::default();
                         style.underline = true;
-                    };
+                    }
                 }
                 Event::Code(t) => {
                     text.push_str(t.as_ref());
@@ -395,10 +368,9 @@ impl<'a> MarkdownParser<'a> {
                     }
                     Tag::Image { dest_url, .. } => {
                         image = Image::identify(
+                            dest_url.to_string(),
                             source_range.clone(),
                             self.file_location_directory.clone(),
-                            dest_url.to_string(),
-                            link.clone(),
                         );
                     }
                     _ => {
@@ -926,6 +898,18 @@ mod tests {
         );
     }
 
+    #[gpui::test]
+    async fn test_empty_image() {
+        let parsed = parse("![]()").await;
+
+        let paragraph = if let ParsedMarkdownElement::Paragraph(text) = &parsed.children[0] {
+            text
+        } else {
+            panic!("Expected a paragraph");
+        };
+        assert_eq!(paragraph.len(), 0);
+    }
+
     #[gpui::test]
     async fn test_image_links_detection() {
         let parsed = parse("![test](https://blog.logrocket.com/wp-content/uploads/2024/04/exploring-zed-open-source-code-editor-rust-2.png)").await;
@@ -937,19 +921,12 @@ mod tests {
         };
         assert_eq!(
             paragraph[0],
-            MarkdownParagraphChunk::Image(Image::Web {
+            MarkdownParagraphChunk::Image(Image {
                 source_range: 0..111,
-                url: "https://blog.logrocket.com/wp-content/uploads/2024/04/exploring-zed-open-source-code-editor-rust-2.png".to_string(),
+                link: Link::Web {
-                link: None,
+                    url: "https://blog.logrocket.com/wp-content/uploads/2024/04/exploring-zed-open-source-code-editor-rust-2.png".to_string(),
-                alt_text: Some(
+                },
-                        ParsedMarkdownText {
+                alt_text: Some("test".into()),
-                        source_range: 0..111,
-                       contents: "test".to_string(),
-                       highlights: vec![],
-                     region_ranges: vec![],
-                      regions: vec![],
-                 },
-                  ),
             },)
         );
     }

crates/markdown_preview/src/markdown_preview_view.rs

@@ -192,11 +192,16 @@ impl MarkdownPreviewView {
                                 .group("markdown-block")
                                 .on_click(cx.listener(move |this, event: &ClickEvent, cx| {
                                     if event.down.click_count == 2 {
-                                        if let Some(block) =
+                                        if let Some(source_range) = this
-                                            this.contents.as_ref().and_then(|c| c.children.get(ix))
+                                            .contents
+                                            .as_ref()
+                                            .and_then(|c| c.children.get(ix))
+                                            .and_then(|block| block.source_range())
                                         {
-                                            let start = block.source_range().start;
+                                            this.move_cursor_to_block(
-                                            this.move_cursor_to_block(cx, start..start);
+                                                cx,
+                                                source_range.start..source_range.start,
+                                            );
                                         }
                                     }
                                 }))
@@ -410,7 +415,9 @@ impl MarkdownPreviewView {
         let mut last_end = 0;
         if let Some(content) = &self.contents {
             for (i, block) in content.children.iter().enumerate() {
-                let Range { start, end } = block.source_range();
+                let Some(Range { start, end }) = block.source_range() else {
+                    continue;
+                };
 
                 // Check if the cursor is between the last block and the current block
                 if last_end <= cursor && cursor < start {

crates/markdown_preview/src/markdown_renderer.rs

@@ -1,8 +1,8 @@
 use crate::markdown_elements::{
-    HeadingLevel, Image, Link, MarkdownParagraph, MarkdownParagraphChunk, ParsedMarkdown,
+    HeadingLevel, Link, MarkdownParagraph, MarkdownParagraphChunk, ParsedMarkdown,
     ParsedMarkdownBlockQuote, ParsedMarkdownCodeBlock, ParsedMarkdownElement,
     ParsedMarkdownHeading, ParsedMarkdownListItem, ParsedMarkdownListItemType, ParsedMarkdownTable,
-    ParsedMarkdownTableAlignment, ParsedMarkdownTableRow, ParsedMarkdownText,
+    ParsedMarkdownTableAlignment, ParsedMarkdownTableRow,
 };
 use gpui::{
     div, img, px, rems, AbsoluteLength, AnyElement, ClipboardItem, DefiniteLength, Div, Element,
@@ -13,7 +13,6 @@ use gpui::{
 use settings::Settings;
 use std::{
     ops::{Mul, Range},
-    path::Path,
     sync::Arc,
     vec,
 };
@@ -505,103 +504,41 @@ fn render_markdown_text(parsed_new: &MarkdownParagraph, cx: &mut RenderContext)
             }
 
             MarkdownParagraphChunk::Image(image) => {
-                let (link, source_range, image_source, alt_text) = match image {
+                let image_resource = match image.link.clone() {
-                    Image::Web {
+                    Link::Web { url } => Resource::Uri(url.into()),
-                        link,
+                    Link::Path { path, .. } => Resource::Path(Arc::from(path)),
-                        source_range,
-                        url,
-                        alt_text,
-                    } => (
-                        link,
-                        source_range,
-                        Resource::Uri(url.clone().into()),
-                        alt_text,
-                    ),
-                    Image::Path {
-                        link,
-                        source_range,
-                        path,
-                        alt_text,
-                        ..
-                    } => {
-                        let image_path = Path::new(path.to_str().unwrap());
-                        (
-                            link,
-                            source_range,
-                            Resource::Path(Arc::from(image_path)),
-                            alt_text,
-                        )
-                    }
                 };
 
-                let element_id = cx.next_id(source_range);
+                let element_id = cx.next_id(&image.source_range);
 
-                match link {
+                let image_element = div()
-                    None => {
+                    .id(element_id)
-                        let fallback_workspace = workspace_clone.clone();
+                    .child(img(ImageSource::Resource(image_resource)).with_fallback({
-                        let fallback_syntax_theme = syntax_theme.clone();
+                        let alt_text = image.alt_text.clone();
-                        let fallback_text_style = text_style.clone();
+                        {
-                        let fallback_alt_text = alt_text.clone();
+                            move || div().children(alt_text.clone()).into_any_element()
-                        let element_id_new = element_id.clone();
+                        }
-                        let element = div()
+                    }))
-                            .child(img(ImageSource::Resource(image_source)).with_fallback({
+                    .tooltip({
-                                move || {
+                        let link = image.link.clone();
-                                    fallback_text(
+                        move |cx| LinkPreview::new(&link.to_string(), cx)
-                                        fallback_alt_text.clone().unwrap(),
+                    })
-                                        element_id.clone(),
+                    .on_click({
-                                        &fallback_syntax_theme,
+                        let workspace = workspace_clone.clone();
-                                        code_span_bg_color,
+                        let link = image.link.clone();
-                                        fallback_workspace.clone(),
+                        move |_event, window_cx| match &link {
-                                        &fallback_text_style,
+                            Link::Web { url } => window_cx.open_url(url),
-                                    )
+                            Link::Path { path, .. } => {
+                                if let Some(workspace) = &workspace {
+                                    _ = workspace.update(window_cx, |workspace, cx| {
+                                        workspace.open_abs_path(path.clone(), false, cx).detach();
+                                    });
                                 }
-                            }))
+                            }
-                            .id(element_id_new)
+                        }
-                            .into_any();
+                    })
-                        any_element.push(element);
+                    .into_any();
-                    }
+                any_element.push(image_element);
-                    Some(link) => {
-                        let link_click = link.clone();
-                        let link_tooltip = link.clone();
-                        let fallback_workspace = workspace_clone.clone();
-                        let fallback_syntax_theme = syntax_theme.clone();
-                        let fallback_text_style = text_style.clone();
-                        let fallback_alt_text = alt_text.clone();
-                        let element_id_new = element_id.clone();
-                        let image_element = div()
-                            .child(img(ImageSource::Resource(image_source)).with_fallback({
-                                move || {
-                                    fallback_text(
-                                        fallback_alt_text.clone().unwrap(),
-                                        element_id.clone(),
-                                        &fallback_syntax_theme,
-                                        code_span_bg_color,
-                                        fallback_workspace.clone(),
-                                        &fallback_text_style,
-                                    )
-                                }
-                            }))
-                            .id(element_id_new)
-                            .tooltip(move |cx| LinkPreview::new(&link_tooltip.to_string(), cx))
-                            .on_click({
-                                let workspace = workspace_clone.clone();
-                                move |_event, window_cx| match &link_click {
-                                    Link::Web { url } => window_cx.open_url(url),
-                                    Link::Path { path, .. } => {
-                                        if let Some(workspace) = &workspace {
-                                            _ = workspace.update(window_cx, |workspace, cx| {
-                                                workspace
-                                                    .open_abs_path(path.clone(), false, cx)
-                                                    .detach();
-                                            });
-                                        }
-                                    }
-                                }
-                            })
-                            .into_any();
-                        any_element.push(image_element);
-                    }
-                }
             }
         }
     }
@@ -613,80 +550,3 @@ fn render_markdown_rule(cx: &mut RenderContext) -> AnyElement {
     let rule = div().w_full().h(px(2.)).bg(cx.border_color);
     div().pt_3().pb_3().child(rule).into_any()
 }
-
-fn fallback_text(
-    parsed: ParsedMarkdownText,
-    source_range: ElementId,
-    syntax_theme: &theme::SyntaxTheme,
-    code_span_bg_color: Hsla,
-    workspace: Option<WeakView<Workspace>>,
-    text_style: &TextStyle,
-) -> AnyElement {
-    let element_id = source_range;
-
-    let highlights = gpui::combine_highlights(
-        parsed.highlights.iter().filter_map(|(range, highlight)| {
-            let highlight = highlight.to_highlight_style(syntax_theme)?;
-            Some((range.clone(), highlight))
-        }),
-        parsed
-            .regions
-            .iter()
-            .zip(&parsed.region_ranges)
-            .filter_map(|(region, range)| {
-                if region.code {
-                    Some((
-                        range.clone(),
-                        HighlightStyle {
-                            background_color: Some(code_span_bg_color),
-                            ..Default::default()
-                        },
-                    ))
-                } else {
-                    None
-                }
-            }),
-    );
-    let mut links = Vec::new();
-    let mut link_ranges = Vec::new();
-    for (range, region) in parsed.region_ranges.iter().zip(&parsed.regions) {
-        if let Some(link) = region.link.clone() {
-            links.push(link);
-            link_ranges.push(range.clone());
-        }
-    }
-    let element = div()
-        .child(
-            InteractiveText::new(
-                element_id,
-                StyledText::new(parsed.contents.clone()).with_highlights(text_style, highlights),
-            )
-            .tooltip({
-                let links = links.clone();
-                let link_ranges = link_ranges.clone();
-                move |idx, cx| {
-                    for (ix, range) in link_ranges.iter().enumerate() {
-                        if range.contains(&idx) {
-                            return Some(LinkPreview::new(&links[ix].to_string(), cx));
-                        }
-                    }
-                    None
-                }
-            })
-            .on_click(
-                link_ranges,
-                move |clicked_range_ix, window_cx| match &links[clicked_range_ix] {
-                    Link::Web { url } => window_cx.open_url(url),
-                    Link::Path { path, .. } => {
-                        if let Some(workspace) = &workspace {
-                            _ = workspace.update(window_cx, |workspace, cx| {
-                                workspace.open_abs_path(path.clone(), false, cx).detach();
-                            });
-                        }
-                    }
-                },
-            ),
-        )
-        .into_any();
-    return element;
-}