Panics · P22 Collaboration: marks outside their text stay out under the owner's index; accept answers None; zero reactions and stale masked offsets hold

examples/gallery/pages/collab/talk.rs

@@ -261,7 +261,10 @@ const BASE: &str = "A lift blends color toward white. Half a lift sits midway be
 fn applied(base: &str, suggestions: Vec<Suggestion>) -> String {
     let (mut text, mut left) = (base.to_string(), suggestions);
     while !left.is_empty() {
-        (text, left) = accept(&text, &left, 0);
+        let Some(taken) = accept(&text, &left, 0) else {
+            break;
+        };
+        (text, left) = taken;
     }
     text
 }
@@ -306,11 +309,16 @@ pub fn changes(window: &mut Window, cx: &mut App) -> impl IntoElement + use<> {
                     let pending = suggested(&base_text, field.read(cx).text(), &you());
                     match decision {
                         Decision::Accept(ix) => {
-                            set(&decided, accept(&base_text, &pending, ix).0, cx)
+                            if let Some((text, _)) = accept(&base_text, &pending, ix) {
+                                set(&decided, text, cx)
+                            }
                         }
                         Decision::AcceptAll => set(&decided, applied(&base_text, pending), cx),
                         Decision::Reject(ix) => {
                             let mut rest = pending;
+                            if ix >= rest.len() {
+                                return;
+                            }
                             rest.remove(ix);
                             let text = applied(&base_text, rest);
                             field.update(cx, |input, cx| input.set_text(text, cx));

src/collab/annotate.rs

@@ -26,6 +26,7 @@ pub struct AnnotatedText {
     id: ElementId,
     text: SharedString,
     annotations: Rc<Vec<Annotation>>,
+    kept: Rc<Vec<usize>>,
     picked: Option<usize>,
     on_pick: Option<Pick>,
 }
@@ -38,15 +39,25 @@ impl AnnotatedText {
         annotations: impl IntoIterator<Item = Annotation>,
     ) -> Self {
         let text = text.into();
-        let annotations: Vec<Annotation> = annotations.into_iter().collect();
+        let (kept, annotations): (Vec<usize>, Vec<Annotation>) = annotations
+            .into_iter()
+            .enumerate()
+            .filter(|(_, annotation)| {
+                let inside = text.get(annotation.range.clone()).is_some();
+                if !inside {
+                    log::error!(
+                        "annotated text: {:?} lies outside the text; left out",
+                        annotation.range
+                    );
+                }
+                inside
+            })
+            .unzip();
         for annotation in &annotations {
-            let range = &annotation.range;
             assert!(
-                range.start < range.end
-                    && range.end <= text.len()
-                    && text.is_char_boundary(range.start)
-                    && text.is_char_boundary(range.end),
-                "annotation {range:?} lies outside the text"
+                !annotation.range.is_empty(),
+                "annotation {:?} is empty",
+                annotation.range
             );
         }
         assert!(
@@ -59,6 +70,7 @@ impl AnnotatedText {
             id: id.into(),
             text,
             annotations: Rc::new(annotations),
+            kept: Rc::new(kept),
             picked: None,
             on_pick: None,
         }
@@ -78,6 +90,9 @@ impl AnnotatedText {
 
 impl RenderOnce for AnnotatedText {
     fn render(self, _: &mut Window, cx: &mut App) -> impl IntoElement {
+        if let Some(picked) = self.picked.filter(|picked| !self.kept.contains(picked)) {
+            log::error!("annotated text: annotation {picked} is not shown; none picked");
+        }
         let thickness = cx.theme().underline_thickness();
         let highlights: Vec<(Range<usize>, HighlightStyle)> = self
             .annotations
@@ -85,7 +100,7 @@ impl RenderOnce for AnnotatedText {
             .enumerate()
             .map(|(ix, annotation)| {
                 let color = annotation.author.color(cx);
-                let wash = if self.picked == Some(ix) {
+                let wash = if self.picked == Some(self.kept[ix]) {
                     PICKED
                 } else {
                     WASH
@@ -123,6 +138,7 @@ impl RenderOnce for AnnotatedText {
         });
         match self.on_pick {
             Some(pick) => text.on_click(ranges, move |ix, window, cx| {
+                let ix = self.kept[ix];
                 log::info!("annotated text: picked {ix}");
                 pick(ix, window, cx)
             }),
@@ -130,3 +146,22 @@ impl RenderOnce for AnnotatedText {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{AnnotatedText, Annotation};
+    use crate::collab::Peer;
+
+    #[test]
+    fn an_annotation_outside_the_text_is_left_out() {
+        let note = |range| Annotation {
+            range,
+            author: Peer::new("ada", "Ada", 0),
+            note: None,
+        };
+        let text = AnnotatedText::new("text", "éabc", [note(1..2), note(2..3)]);
+        assert_eq!(text.annotations.len(), 1);
+        assert_eq!(text.annotations[0].range, 2..3);
+        assert_eq!(*text.kept, [1], "a pick names the owner's index");
+    }
+}

src/collab/peers.rs

@@ -102,6 +102,11 @@ impl PresenceAvatars {
 
 impl RenderOnce for PresenceAvatars {
     fn render(self, _: &mut Window, cx: &mut App) -> impl IntoElement {
+        if let Some(following) = &self.following
+            && !self.peers.iter().any(|peer| &peer.key == following)
+        {
+            log::error!("presence: {following} left; following none");
+        }
         let size = AvatarSize::Sm;
         let rest = self.peers.len().saturating_sub(self.max);
         let shown: Vec<AnyElement> = self

src/collab/reactions.rs

@@ -33,7 +33,10 @@ pub fn toggled(reactions: &[Reaction], emoji: &str) -> Vec<Reaction> {
         .position(|reaction| reaction.emoji.as_ref() == emoji)
     {
         Some(ix) if next[ix].mine => {
-            next[ix].count -= 1;
+            if next[ix].count == 0 {
+                log::error!("reactions: {emoji} counts none yet is yours; it comes off");
+            }
+            next[ix].count = next[ix].count.saturating_sub(1);
             next[ix].mine = false;
             if next[ix].count == 0 {
                 next.remove(ix);
@@ -191,6 +194,11 @@ impl RenderOnce for Reactions {
 mod tests {
     use super::*;
 
+    #[test]
+    fn yours_at_no_count_comes_off() {
+        assert_eq!(toggled(&[reaction("👍", 0, true)], "👍"), []);
+    }
+
     fn reaction(emoji: &str, count: usize, mine: bool) -> Reaction {
         Reaction {
             emoji: emoji.to_string().into(),

src/collab/sidebar.rs

@@ -118,6 +118,11 @@ impl RenderOnce for CommentSidebar {
                 })
             })
         });
+        if let Some(active) = &self.active
+            && !self.threads.iter().any(|thread| &thread.key == active)
+        {
+            log::error!("comment sidebar: thread {active} is gone; none active");
+        }
         let shown: Vec<Thread> = self
             .threads
             .into_iter()

src/collab/tests.rs

@@ -3,11 +3,11 @@ use gpui::{
     TestAppContext, VisualTestContext, Window, div,
 };
 
-use super::{FollowMode, Peer, Reply};
+use super::{Decision, FollowMode, Peer, Reply, Suggestion, TrackChanges};
 use crate::{
     forms,
     forms::{Input, TextInput},
-    theme::Theme,
+    theme::{ActiveTheme, ControlSize, Theme},
 };
 
 /// A field outside a followed view and one inside it, with what they asked: replies sent, follows stopped.
@@ -145,3 +145,70 @@ fn without_resolving_one_list_takes_keyboard_picks(cx: &mut TestAppContext) {
     let picked = board.read_with(cx, |board, _| board.picked.clone());
     assert_eq!(picked, vec![gpui::SharedString::from("a")]);
 }
+
+/// Two suggestions whose first falls inside a character once the text changes, and the decisions asked.
+struct Changing {
+    changed: bool,
+    decided: Vec<Decision>,
+}
+
+impl Render for Changing {
+    fn render(&mut self, _: &mut Window, cx: &mut Context<Self>) -> impl IntoElement {
+        let owner = cx.entity();
+        let suggestion = |range, text: &str| Suggestion {
+            author: Peer::new("ada", "Ada", 0),
+            range,
+            text: text.to_string().into(),
+        };
+        let (base, second) = match self.changed {
+            false => ("abc", 1..2),
+            true => ("éabc", 2..3),
+        };
+        let pending = [suggestion(0..1, "X"), suggestion(second, "B")];
+        div()
+            .size_full()
+            .child(
+                TrackChanges::new("track", base, pending).on_decide(move |decision, _, cx| {
+                    owner.update(cx, |owner, _| owner.decided.push(decision))
+                }),
+            )
+    }
+}
+
+#[gpui::test]
+fn a_press_on_a_suggestion_left_out_decides_nothing(cx: &mut TestAppContext) {
+    cx.update(Theme::init);
+    let (view, cx) = cx.add_window_view(|_, _| Changing {
+        changed: false,
+        decided: Vec::new(),
+    });
+    cx.run_until_parked();
+    let row = cx
+        .debug_bounds("suggestion 0")
+        .expect("the first suggestion");
+    let at = cx.update(|window, cx| {
+        let button = cx
+            .theme()
+            .control_height(ControlSize::Sm)
+            .to_pixels(window.rem_size());
+        let gap = gpui::rems(0.5).to_pixels(window.rem_size());
+        gpui::point(row.right() - button - gap - button / 2.0, row.center().y)
+    });
+    cx.simulate_click(at, gpui::Modifiers::none());
+    assert_eq!(
+        view.read_with(cx, |view, _| view.decided.clone()),
+        [Decision::Accept(0)]
+    );
+    cx.simulate_mouse_down(at, gpui::MouseButton::Left, gpui::Modifiers::none());
+    view.update(cx, |view, cx| {
+        view.changed = true;
+        cx.notify();
+    });
+    cx.run_until_parked();
+    cx.simulate_mouse_up(at, gpui::MouseButton::Left, gpui::Modifiers::none());
+    cx.run_until_parked();
+    assert_eq!(
+        view.read_with(cx, |view, _| view.decided.clone()),
+        [Decision::Accept(0)]
+    );
+}

src/collab/track.rs

@@ -158,9 +158,23 @@ pub fn suggested(base: &str, edited: &str, author: &Peer) -> Vec<Suggestion> {
     found
 }
 
-/// `base` with suggestion `ix` taken, and the others moved to match.
-pub fn accept(base: &str, suggestions: &[Suggestion], ix: usize) -> (String, Vec<Suggestion>) {
-    let taken = &suggestions[ix];
+/// `base` with suggestion `ix` taken, and the others moved to match; none when `ix` or its range left `base`.
+pub fn accept(
+    base: &str,
+    suggestions: &[Suggestion],
+    ix: usize,
+) -> Option<(String, Vec<Suggestion>)> {
+    let Some(taken) = suggestions.get(ix) else {
+        log::error!("track changes: no suggestion {ix} of {}", suggestions.len());
+        return None;
+    };
+    if base.get(taken.range.clone()).is_none() {
+        log::error!(
+            "track changes: suggestion {:?} lies outside the text",
+            taken.range
+        );
+        return None;
+    }
     let text = format!(
         "{}{}{}",
         &base[..taken.range.start],
@@ -181,7 +195,7 @@ pub fn accept(base: &str, suggestions: &[Suggestion], ix: usize) -> (String, Vec
             suggestion
         })
         .collect();
-    (text, rest)
+    Some((text, rest))
 }
 
 /// What a suggestion does, in a line: replace, add or delete, with the words.
@@ -206,6 +220,7 @@ pub struct TrackChanges {
     id: ElementId,
     base: SharedString,
     suggestions: Vec<Suggestion>,
+    kept: Vec<usize>,
     on_decide: Option<OnDecide>,
 }
 
@@ -217,15 +232,22 @@ impl TrackChanges {
         suggestions: impl IntoIterator<Item = Suggestion>,
     ) -> Self {
         let base = base.into();
-        let suggestions: Vec<Suggestion> = suggestions.into_iter().collect();
+        let (kept, suggestions): (Vec<usize>, Vec<Suggestion>) = suggestions
+            .into_iter()
+            .enumerate()
+            .filter(|(_, suggestion)| {
+                let inside = base.get(suggestion.range.clone()).is_some();
+                if !inside {
+                    log::error!(
+                        "track changes: {:?} lies outside the text; left out",
+                        suggestion.range
+                    );
+                }
+                inside
+            })
+            .unzip();
         for suggestion in &suggestions {
             let range = &suggestion.range;
-            assert!(
-                range.end <= base.len()
-                    && base.is_char_boundary(range.start)
-                    && base.is_char_boundary(range.end),
-                "suggestion {range:?} lies outside the text"
-            );
             assert!(
                 !(range.is_empty() && suggestion.text.is_empty()),
                 "suggestion at {range:?} changes nothing"
@@ -241,6 +263,7 @@ impl TrackChanges {
             id: id.into(),
             base,
             suggestions,
+            kept,
             on_decide: None,
         }
     }
@@ -297,8 +320,10 @@ impl RenderOnce for TrackChanges {
         };
         let pending = !self.suggestions.is_empty() && self.on_decide.is_some();
         let rows = self.suggestions.iter().enumerate().map(|(ix, suggestion)| {
-            let id = |what: &str| (self.id.clone(), format!("{what}-{ix}"));
+            let at = self.kept[ix];
+            let id = |what: &str| (self.id.clone(), format!("{what}-{at}"));
             div()
+                .debug_selector(move || format!("suggestion {at}"))
                 .flex()
                 .items_center()
                 .gap_2()
@@ -327,14 +352,14 @@ impl RenderOnce for TrackChanges {
                             .variant(ButtonVariant::Ghost)
                             .size(ControlSize::Sm)
                             .tooltip("Accept")
-                            .on_click(decide(Decision::Accept(ix))),
+                            .on_click(decide(Decision::Accept(at))),
                     )
                     .child(
                         IconButton::new(id("reject"), IconName::X)
                             .variant(ButtonVariant::Ghost)
                             .size(ControlSize::Sm)
                             .tooltip("Reject")
-                            .on_click(decide(Decision::Reject(ix))),
+                            .on_click(decide(Decision::Reject(at))),
                     )
                 })
         });
@@ -430,9 +455,9 @@ mod tests {
             found,
             [suggestion(2..6, "tint"), suggestion(14..14, "every ")]
         );
-        let (text, rest) = accept("A lift blends color toward white.", &found, 0);
+        let (text, rest) = accept("A lift blends color toward white.", &found, 0).unwrap();
         assert_eq!(
-            accept(&text, &rest, 0).0,
+            accept(&text, &rest, 0).unwrap().0,
             "A tint blends every color toward white."
         );
     }
@@ -441,16 +466,29 @@ mod tests {
     fn accepting_one_moves_the_rest() {
         let base = "A lift blends color.";
         let all = [suggestion(2..6, "tint"), suggestion(14..14, "every ")];
-        let (text, rest) = accept(base, &all, 0);
+        let (text, rest) = accept(base, &all, 0).unwrap();
         assert_eq!(text, "A tint blends color.");
         assert_eq!(
             rest,
             [suggestion(14..14, "every ")],
             "same length, same place"
         );
-        let (text, rest) = accept(base, &[suggestion(2..6, "shade of"), all[1].clone()], 0);
+        let (text, rest) =
+            accept(base, &[suggestion(2..6, "shade of"), all[1].clone()], 0).unwrap();
         assert_eq!(text, "A shade of blends color.");
-        let (text, _) = accept(&text, &rest, 0);
+        let (text, _) = accept(&text, &rest, 0).unwrap();
         assert_eq!(text, "A shade of blends every color.");
     }
+
+    #[test]
+    fn a_suggestion_gone_or_outside_the_text_is_not_taken() {
+        let base = "A lift blends.";
+        let inside = suggestion(2..6, "tint");
+        assert_eq!(accept(base, std::slice::from_ref(&inside), 1), None);
+        assert_eq!(accept("A", std::slice::from_ref(&inside), 0), None);
+        let stale = suggestion(0..40, "");
+        let track = TrackChanges::new("track", base, [stale, inside.clone()]);
+        assert_eq!(track.suggestions, [inside]);
+        assert_eq!(track.kept, [1], "decisions name the owner's index");
+    }
 }

src/forms/text/edit.rs

@@ -69,9 +69,12 @@ pub(crate) fn from_utf16(text: &str, offset: usize) -> usize {
     text.len()
 }
 
-/// Display offset of a content offset when each grapheme shows as `bullet`.
+/// Display offset of a content offset when each grapheme shows as `bullet`; an offset the text lost clamps into it.
 pub(crate) fn masked_offset(text: &str, offset: usize, bullet: char) -> usize {
-    text[..offset].graphemes(true).count() * bullet.len_utf8()
+    text[..text.floor_char_boundary(offset)]
+        .graphemes(true)
+        .count()
+        * bullet.len_utf8()
 }
 
 /// Content offset of a display offset in masked text.
@@ -182,6 +185,16 @@ mod tests {
         assert_eq!(masked_offset(text, 6, '•'), 9);
         assert_eq!(unmasked_offset(text, 9, '•'), 6);
         assert_eq!(unmasked_offset(text, 12, '•'), text.len());
+        assert_eq!(
+            masked_offset("a", 2, '•'),
+            3,
+            "an offset past the text clamps"
+        );
+        assert_eq!(
+            masked_offset(text, 4, '•'),
+            6,
+            "inside a character, before it"
+        );
     }
 
     #[test]

tasks/panics.md

@@ -36,7 +36,7 @@ Each item reads every site in its folders, fixes what the rules name, updates ca
 - [x] P19 Git — `src/git`
 - [x] P20 Debug — `src/debug`
 - [x] P21 Documents — `src/documents`
-- [ ] P22 Collaboration — `src/collab`
+- [x] P22 Collaboration — `src/collab`
 - [ ] P23 AI Chat — `src/chat`
 - [ ] P24 Agent — `src/agent`
 - [ ] P25 Generative — `src/generative`
@@ -87,3 +87,4 @@ One line per item: what changed, what stayed and why, review rounds.
 - P19: a blame that lags its code draws every line, bare where no owner fits; `git::resolve` returns `Option` for a conflict gone from the text. Kept: diff groups, merge regions, similar's pinned resolutions. Review: 2 rounds.
 - P20: a hex selection off the bytes and a stack frame past the stack select none; a flame focus keeps what the profile holds; the timeline's wheel reports only finite, positive ranges from finite factors; a dump's last byte must have an address; flame paints log. Kept: disassembly patterns, `zoomed`'s contract. Review: 3 rounds.
 - P21: block editor listeners painted for a block, field, cell or kind since gone log and change nothing, a dropped picture drag leaves undo alone; a visual block press or edit over text the owner replaced is dropped; nested images parse; a version or find hit past its list selects none. Review: 4 rounds.
+- P22: annotations and suggestions outside their text log and stay out, picks and decisions keeping the owner's index; `accept` returns `Option`; a gone pick, active thread or followed peer logs; yours at no count comes off; a masked field clamps a stale offset, so remote cursors and selections draw. Kept: a thread's comments, presence max, one list's order, empty marks, constant role and mode tables. Review: 3 rounds.