-
Notifications
You must be signed in to change notification settings - Fork 6k
fix(providers): treat unicode punctuation as image path terminators #10106
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
Changes from all commits
8596728
22900a1
0ac4248
29c47af
e846c72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,8 +49,7 @@ pub fn detect_image_path(text: &str) -> Option<Cow<'_, str>> { | |
| }; | ||
|
|
||
| let terminator = text.get(end..).and_then(|rest| rest.chars().next()); | ||
| let terminated = | ||
| terminator.is_none_or(|c| c == '/' || c.is_whitespace() || c == '"' || c == '\''); | ||
| let terminated = terminator.is_none_or(is_path_terminator); | ||
|
|
||
| if terminated { | ||
| let mut floor = end.saturating_sub(MAX_PATH_LEN); | ||
|
|
@@ -63,7 +62,7 @@ pub fn detect_image_path(text: &str) -> Option<Cow<'_, str>> { | |
| let preceded_by_boundary = text | ||
| .get(..start) | ||
| .and_then(|prefix| prefix.chars().next_back()) | ||
| .is_none_or(|c| c.is_whitespace() || c == '"' || c == '\''); | ||
| .is_none_or(is_path_leading_boundary); | ||
| if !preceded_by_boundary { | ||
| continue; | ||
| } | ||
|
|
@@ -140,6 +139,30 @@ fn is_existing_image_path(candidate: &str) -> bool { | |
| path.is_absolute() && path.is_file() && is_image_file(path) | ||
| } | ||
|
|
||
| fn is_path_leading_boundary(c: char) -> bool { | ||
| c.is_whitespace() | ||
| || matches!( | ||
| c, | ||
| '"' | '\'' | '\u{00AB}' | '\u{00BB}' | '\u{2018}' | ||
| ..='\u{201F}' | '\u{2039}' | '\u{203A}' | ||
| ) | ||
| } | ||
|
|
||
| fn is_path_terminator(c: char) -> bool { | ||
| c == '/' | ||
| || c.is_whitespace() | ||
| || matches!( | ||
| c, | ||
| '"' | '\'' | '\u{00AB}' | '\u{00BB}' | '\u{2013}' | ||
| ..='\u{201F}' | '\u{2026}' | '\u{2039}' | '\u{203A}' | ||
| ) | ||
| || ('\u{2300}'..='\u{23FF}').contains(&c) | ||
| || ('\u{2600}'..='\u{27BF}').contains(&c) | ||
| || ('\u{2B00}'..='\u{2BFF}').contains(&c) | ||
| || ('\u{1F1E6}'..='\u{1F1FF}').contains(&c) | ||
| || ('\u{1F300}'..='\u{1FAFF}').contains(&c) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user appends a flag emoji immediately after an image path, e.g. Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 87b633a by adding the Regional Indicator Symbol range (U+1F1E6..U+1F1FF) and covering a flag emoji suffix in the unicode separator regression test. Local verification:
Comment on lines
+160
to
+163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user appends a common emoji outside these hard-coded ranges, such as Useful? React with 👍 / 👎.
Comment on lines
+159
to
+163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a user appends a keycap emoji immediately after an existing image path, e.g. Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| /// Case-insensitive ASCII substring search returning a byte index into | ||
| /// `haystack` (no allocation, so the index stays valid for slicing). | ||
| fn find_ascii_ci(haystack: &str, needle: &str, from: usize) -> Option<usize> { | ||
|
|
@@ -286,6 +309,10 @@ mod tests { | |
| assert_eq!(detect_image_path(&text).as_deref(), Some(png_path_str)); | ||
| let text = format!("describe '{}'", png_path_str); | ||
| assert_eq!(detect_image_path(&text).as_deref(), Some(png_path_str)); | ||
| let text = format!("describe “{}” please", png_path_str); | ||
| assert_eq!(detect_image_path(&text).as_deref(), Some(png_path_str)); | ||
| let text = format!("describe «{}» please", png_path_str); | ||
| assert_eq!(detect_image_path(&text).as_deref(), Some(png_path_str)); | ||
|
|
||
| // A stray closing quote in prose must not act as a terminator for an | ||
| // unquoted path. | ||
|
|
@@ -355,6 +382,53 @@ mod tests { | |
| assert_eq!(detect_image_path(&text).as_deref(), Some(png_path_str)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_detect_image_path_with_unicode_separators() { | ||
| let temp_dir = tempfile::tempdir().unwrap(); | ||
| let png_data = [0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]; | ||
| let png_path = temp_dir.path().join("photo.png"); | ||
| std::fs::write(&png_path, png_data).unwrap(); | ||
| let png_path_str = png_path.to_str().unwrap(); | ||
|
|
||
| assert_eq!( | ||
| detect_image_path(&format!("{png_path_str}🙂")).as_deref(), | ||
| Some(png_path_str) | ||
| ); | ||
| assert_eq!( | ||
| detect_image_path(&format!("{png_path_str}🇺🇸")).as_deref(), | ||
| Some(png_path_str) | ||
| ); | ||
| assert_eq!( | ||
| detect_image_path(&format!("{png_path_str}⌚")).as_deref(), | ||
| Some(png_path_str) | ||
| ); | ||
| assert_eq!( | ||
| detect_image_path(&format!("{png_path_str}⭐")).as_deref(), | ||
| Some(png_path_str) | ||
| ); | ||
| assert_eq!( | ||
| detect_image_path(&format!("{png_path_str}… more text")).as_deref(), | ||
| Some(png_path_str) | ||
| ); | ||
| assert_eq!( | ||
| detect_image_path(&format!("{png_path_str}\u{2014}more text")).as_deref(), | ||
| Some(png_path_str) | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| detect_image_path(&format!("{png_path_str}\u{200B}.backup")).as_deref(), | ||
| None | ||
| ); | ||
| assert_eq!( | ||
| detect_image_path(&format!("{png_path_str}\u{0301}")).as_deref(), | ||
| None | ||
| ); | ||
| assert_eq!( | ||
| detect_image_path(&format!("file:{png_path_str}")).as_deref(), | ||
| None | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_detect_image_path_ignores_urls_and_longer_extensions() { | ||
| let temp_dir = tempfile::tempdir().unwrap(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a user wraps the path in smart quotes, e.g.
“/tmp/photo.png”, detection still returnsNone: the trailing”now passes this terminator check, but the slash is skipped because the leading-boundary check still accepts only whitespace and ASCII quotes. Since ASCII-quoted paths are supported and this change adds Unicode quote terminators, the matching opening Unicode quotes should be accepted as path boundaries too.Useful? React with 👍 / 👎.