Repository: Textualize/textual

Issue:
`OptionList` duplicate ID detection not working as expected
Consider the following code:

```python
from textual.app import App, ComposeResult
from textual.widgets import OptionList, Log, Footer
from textual.widgets.option_list import Option

class OptionListAddBug(App[None]):

    BINDINGS = [
        (str(n), f"add({n})", f"Add {n}") for n in range(10)
    ]

    CSS = """
    OptionList, Log {
        height: 1fr;
        border: round cornflowerblue;
    }
    """

    def compose(self) -> ComposeResult:
        yield OptionList()
        yield Log()
        yield Footer()

    def action_add(self, value: int) -> None:
        try:
            self.query_one(OptionList).add_option(
                Option(f"Added {value} just fine", id=f"id-{value}")
            )
        except Exception as error:
            self.query_one(Log).write_line(f"{error!r}")


if __name__ == "__main__":
    OptionListAddBug().run()
```

If you run it and press keys 0-9, you'll find that corresponding options are added to the `OptionList`.

![Screenshot 2023-10-04 at 15 26 43](https://github.com/Textualize/textual/assets/28237/b66c94d8-7706-4d64-a0d9-3d5214a5ec13)

However, if you start the app afresh, then press (for example) <kbd>2</kbd> twice, resulting in a logged `DuplicateID` exception (expected and desired), after than you can't add any more options to the list, and the attempted added option is always reported as having the original duplicate ID:

![Screenshot 2023-10-04 at 15 28 53](https://github.com/Textualize/textual/assets/28237/d670fb29-1a25-47e6-a2ed-198f53ded001)

In the above I pressed <kbd>2</kbd> twice and then a number of other number keys.

It would appear that `OptionList` is getting every confused in its `id` tracking and/or duplicate detection.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 75d2eb356..428b546f8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - `Pilot.click`/`Pilot.hover` can't use `Screen` as a selector https://github.com/Textualize/textual/issues/3395
 - App exception when a `Tree` is initialized/mounted with `disabled=True` https://github.com/Textualize/textual/issues/3407
 - Fixed application freeze when pasting an emoji into an application on Windows https://github.com/Textualize/textual/issues/3178
+- Fixed duplicate option ID handling in the `OptionList` https://github.com/Textualize/textual/issues/3455
 
 ### Added
 
diff --git a/docs/blog/posts/textual-plotext.md b/docs/blog/posts/textual-plotext.md
index 82d16ecd9..22563164e 100644
--- a/docs/blog/posts/textual-plotext.md
+++ b/docs/blog/posts/textual-plotext.md
@@ -83,7 +83,7 @@ A key design goal of this widget is that you can develop your plots so that
 the resulting code looks very similar to that in the Plotext documentation.
 The core difference is that, where you'd normally import the `plotext`
 module `as plt` and then call functions via `plt`, you instead use the `plt`
-property made available by the widget.
+properly made available by the widget.
 
 You don't even need to call the `build` or `show` functions as
 `textual-plotext` takes care of this for you. You can see this in action in
@@ -102,7 +102,7 @@ it](https://github.com/Textualize/textual-plotext/blob/main/examples/textual_tow
 ![The Textual Towers weather history app](/blog/images/textual-plotext/weather.png)
 
 If you are an existing Plotext user who wants to turn your plots into full
-terminal applications, we think this will be very familiar and accessible.
+terminal applications, we think this will be a very familiar and accessible.
 If you're a Textual user who wants to add plots to your application, we
 think Plotext is a great library for this.
 
diff --git a/src/textual/widgets/_option_list.py b/src/textual/widgets/_option_list.py
index 60a804287..538f7029f 100644
--- a/src/textual/widgets/_option_list.py
+++ b/src/textual/widgets/_option_list.py
@@ -540,10 +540,6 @@ class OptionList(ScrollView, can_focus=True):
                 if content.id is not None:
                     # The option has an ID set, create a mapping from that
                     # ID to the option so we can use it later.
-                    if content.id in option_ids:
-                        raise DuplicateID(
-                            f"The option list already has an option with id '{content.id}'"
-                        )
                     option_ids[content.id] = option
                 option += 1
             else:
@@ -558,6 +554,30 @@ class OptionList(ScrollView, can_focus=True):
         # list, set the virtual size.
         self.virtual_size = Size(self.scrollable_content_region.width, len(self._lines))
 
+    def _duplicate_id_check(self, candidate_items: list[OptionListContent]) -> None:
+        """Check the items to be added for any duplicates.
+
+        Args:
+            candidate_items: The items that are going be added.
+
+        Raises:
+            DuplicateID: If there is an attempt to use a duplicate ID.
+        """
+        # We're only interested in options, and only those that have IDs.
+        new_options = [
+            item
+            for item in candidate_items
+            if isinstance(item, Option) and item.id is not None
+        ]
+        # Get the set of new IDs that we're being given.
+        new_option_ids = {option.id for option in new_options}
+        # Now check for duplicates, both internally amongst the new items
+        # incoming, and also against all the current known IDs.
+        if len(new_options) != len(new_option_ids) or not new_option_ids.isdisjoint(
+            self._option_ids
+        ):
+            raise DuplicateID("Attempt made to add options with duplicate IDs.")
+
     def add_options(self, items: Iterable[NewOptionListContent]) -> Self:
         """Add new options to the end of the option list.
 
@@ -569,12 +589,18 @@ class OptionList(ScrollView, can_focus=True):
 
         Raises:
             DuplicateID: If there is an attempt to use a duplicate ID.
+
+        Note:
+            All options are checked for duplicate IDs *before* any option is
+            added. A duplicate ID will cause none of the passed items to be
+            added to the option list.
         """
         # Only work if we have items to add; but don't make a fuss out of
         # zero items to add, just carry on like nothing happened.
         if items:
             # Turn any incoming values into valid content for the list.
             content = [self._make_content(item) for item in items]
+            self._duplicate_id_check(content)
             self._contents.extend(content)
             # Pull out the content that is genuine options and add them to the
             # list of options.


Test patch:
diff --git a/tests/option_list/test_option_list_create.py b/tests/option_list/test_option_list_create.py
index 7e0bbc49c..3310579b8 100644
--- a/tests/option_list/test_option_list_create.py
+++ b/tests/option_list/test_option_list_create.py
@@ -119,5 +119,37 @@ async def test_add_later() -> None:
 async def test_create_with_duplicate_id() -> None:
     """Adding an option with a duplicate ID should be an error."""
     async with OptionListApp().run_test() as pilot:
+        option_list = pilot.app.query_one(OptionList)
+        assert option_list.option_count == 5
+        with pytest.raises(DuplicateID):
+            option_list.add_option(Option("dupe", id="3"))
+        assert option_list.option_count == 5
+
+
+async def test_create_with_duplicate_id_and_subsequent_non_dupes() -> None:
+    """Adding an option with a duplicate ID should be an error."""
+    async with OptionListApp().run_test() as pilot:
+        option_list = pilot.app.query_one(OptionList)
+        assert option_list.option_count == 5
         with pytest.raises(DuplicateID):
-            pilot.app.query_one(OptionList).add_option(Option("dupe", id="3"))
+            option_list.add_option(Option("dupe", id="3"))
+        assert option_list.option_count == 5
+        option_list.add_option(Option("Not a dupe", id="6"))
+        assert option_list.option_count == 6
+        option_list.add_option(Option("Not a dupe", id="7"))
+        assert option_list.option_count == 7
+
+
+async def test_adding_multiple_duplicates_at_once() -> None:
+    """Adding duplicates together than aren't existing duplicates should be an error."""
+    async with OptionListApp().run_test() as pilot:
+        option_list = pilot.app.query_one(OptionList)
+        assert option_list.option_count == 5
+        with pytest.raises(DuplicateID):
+            option_list.add_options(
+                [
+                    Option("dupe", id="42"),
+                    Option("dupe", id="42"),
+                ]
+            )
+        assert option_list.option_count == 5