From 789ae5444e33e443ce7cc226f25d410eabbaa5e3 Mon Sep 17 00:00:00 2001 From: Dariusz Jarosz Date: Mon, 12 Oct 2026 12:47:31 -0500 Subject: [PATCH 1/8] Resolve duplicate id error when uploading attachments in `bely-cli tui` --- tools/developer_tools/bely-cli/PLAN.md | 37 ++++++++++++ .../src/bely_cli/tui/screens/attachments.py | 14 +++-- .../bely-cli/test/test_tui_screens.py | 60 +++++++++++++++++++ 3 files changed, 107 insertions(+), 4 deletions(-) create mode 100644 tools/developer_tools/bely-cli/PLAN.md diff --git a/tools/developer_tools/bely-cli/PLAN.md b/tools/developer_tools/bely-cli/PLAN.md new file mode 100644 index 000000000..baae57f56 --- /dev/null +++ b/tools/developer_tools/bely-cli/PLAN.md @@ -0,0 +1,37 @@ +# Fix attachment image preview race + +## Context + +Rapid attachment/file preview changes can leave the previous image widget mounted while a new image preview callback mounts another widget with the same fixed ID. Textual then raises `DuplicateIds` for `attachment-image` (and the local-file preview has the same asynchronous removal pattern). SSH/terminal image rendering can expose this timing race even when local rendering does not, because the current code starts deferred removal but mounts the replacement without waiting for removal to finish. + +## Approach + +First reproduce the reported failure deterministically in a Textual test by mounting an initial fixed-ID image widget, initiating the existing deferred removal path, and immediately running the next image callback. Confirm the test fails with `DuplicateIds` before changing production code. + +Then move image-container replacement into each async UI callback: await `remove_children()`, re-check the existing preview token after that await, and only then mount the current image. This serializes Textual’s deferred removal with mounting, prevents duplicate fixed IDs, and prevents a callback that became stale while awaiting removal from displaying old content. Apply the same lifecycle handling to remote attachment previews and local upload-file previews; retain selection-time clearing for text/metadata previews. + +## Files to modify + +- `src/bely_cli/tui/screens/attachments.py` — coordinate asynchronous image-widget replacement. +- `test/test_tui_screens.py` — add regression coverage for repeated/racing image previews (exact test location to confirm). + +## Reuse + +- Existing `_preview_token` guards in both `AttachmentScreen` and `AttachmentFileScreen`. +- Existing preview methods and Textual test-app patterns in `test/test_tui_screens.py`. + +## Steps + +- [x] **Milestone 1 — reproducer only:** Add and run a focused Textual regression test against unchanged production code to demonstrate the reported `DuplicateIds` failure. Pause and hand control back for review; do not implement the fix yet. +- [x] **Milestone 2 — minimal fix:** After user approval, update `_show_image()` to await removal of any existing media child, revalidate the token, and mount exactly one image widget. Keep selection-time clearing so text/metadata selections promptly remove prior images. Re-run the reproducer and pause for review. +- [x] **Milestone 3 — parallel path and edge case:** After user approval, apply equivalent handling and regression coverage to `_show_local_image()`, and add a stale-token assertion so an outdated callback cannot mount after replacement awaits. Run focused tests and pause for review. +- [x] **Milestone 4 — broader verification:** After user approval, run the requested broader test suite and report results for final review. +- [x] Update user-facing documentation only if behavior/surface changes (none expected). + +## Verification + +- Record the focused reproducer failing before the fix and pause for review. +- After approval and implementation, record the same reproducer passing and pause again. +- Run focused attachment/TUI screen tests. +- Run the full test suite via `uv run pytest -n auto test` (or `./run_test.sh`). +- Manually move rapidly between image and non-image attachments/local files and confirm no traceback or stale image appears. diff --git a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/attachments.py b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/attachments.py index a724bbeb6..575f8bc1d 100644 --- a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/attachments.py +++ b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/attachments.py @@ -263,8 +263,11 @@ def _load_local_image(self, path, token): async def _show_local_image(self, token, widget_cls, image): if token != self._preview_token: return - await self.query_one("#attachment-file-media", Vertical).mount( - widget_cls(image, id="attachment-file-image")) + media = self.query_one("#attachment-file-media", Vertical) + await media.remove_children() + if token != self._preview_token: + return + await media.mount(widget_cls(image, id="attachment-file-image")) def _preview_local_failed(self, token, message): if token == self._preview_token: @@ -429,8 +432,11 @@ def _load_image(self, attachment, token): async def _show_image(self, token, widget_cls, image): if token != self._preview_token: return - await self.query_one("#attachment-media", Vertical).mount( - widget_cls(image, id="attachment-image")) + media = self.query_one("#attachment-media", Vertical) + await media.remove_children() + if token != self._preview_token: + return + await media.mount(widget_cls(image, id="attachment-image")) def _preview_failed(self, token, message): if token == self._preview_token: diff --git a/tools/developer_tools/bely-cli/test/test_tui_screens.py b/tools/developer_tools/bely-cli/test/test_tui_screens.py index 22777432d..d73e65439 100644 --- a/tools/developer_tools/bely-cli/test/test_tui_screens.py +++ b/tools/developer_tools/bely-cli/test/test_tui_screens.py @@ -184,6 +184,45 @@ async def test_preview_toggle_hides_preview_and_expands_tree(self): await pilot.press("escape") await task.wait() + async def test_replacing_local_image_waits_for_existing_widget_removal(self): + app = App() + async with app.run_test() as pilot: + task = app.run_worker(app.push_screen_wait(AttachmentFileScreen("/tmp"))) + await pilot.pause() + screen = app.screen + media = screen.query_one("#attachment-file-media") + await media.mount(Static("first", id="attachment-file-image")) + + media.remove_children() + await screen._show_local_image( + screen._preview_token, Static, "replacement") + + self.assertEqual(len(media.children), 1) + self.assertEqual(media.children[0].id, "attachment-file-image") + await pilot.press("escape") + await task.wait() + + async def test_stale_local_image_is_not_mounted_after_removal(self): + app = App() + async with app.run_test() as pilot: + task = app.run_worker(app.push_screen_wait(AttachmentFileScreen("/tmp"))) + await pilot.pause() + screen = app.screen + media = screen.query_one("#attachment-file-media") + removal = media.remove_children() + + async def remove_and_change_token(): + await removal + screen._preview_token += 1 + + with patch.object(media, "remove_children", return_value=remove_and_change_token()): + await screen._show_local_image( + screen._preview_token, Static, "stale") + + self.assertEqual(len(media.children), 0) + await pilot.press("escape") + await task.wait() + async def test_entering_directory_changes_tree_root(self): app = App() async with app.run_test() as pilot: @@ -227,6 +266,27 @@ async def test_lists_previews_and_copies_attachment(self): copy.assert_called_once_with("![notes.txt](/download/notes.txt)") await pilot.press("escape") + async def test_replacing_image_waits_for_existing_widget_removal(self): + api = FakeLogbookApi() + api.get_log_entry_attachments = lambda **kwargs: [] + session = FakeSession(api) + app = BelyTuiApp(session) + with patch("bely_cli.tui.app.config.get_setting", return_value=None): + async with app.run_test() as pilot: + await app.push_screen(AttachmentScreen( + session, SimpleNamespace(id=42, name="Doc"), SimpleNamespace(log_id=10))) + await pilot.pause() + screen = app.screen + media = screen.query_one("#attachment-media") + await media.mount(Static("first", id="attachment-image")) + + media.remove_children() + await screen._show_image( + screen._preview_token, Static, "replacement") + + self.assertEqual(len(media.children), 1) + self.assertEqual(media.children[0].id, "attachment-image") + async def test_delete_confirms_then_refreshes(self): attachment = self._attachment() api = FakeLogbookApi() From 2fbdefd5192267882b96c15ee4f740558094db9d Mon Sep 17 00:00:00 2001 From: Dariusz Jarosz Date: Mon, 12 Oct 2026 13:23:15 -0500 Subject: [PATCH 2/8] Add better lables for add reply and toggle reply. Disable toggle reply when no replies are found. --- tools/developer_tools/bely-cli/README.md | 4 ++-- .../bely-cli/src/bely_cli/tui/screens/browse.py | 7 +++++-- .../developer_tools/bely-cli/test/test_tui_app.py | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/tools/developer_tools/bely-cli/README.md b/tools/developer_tools/bely-cli/README.md index b749c77bf..bbb289a95 100644 --- a/tools/developer_tools/bely-cli/README.md +++ b/tools/developer_tools/bely-cli/README.md @@ -417,8 +417,8 @@ on — `i` disappears once you drill into entries, and `s` / `y` / `e` / `f` / ` | `s` | Entries level only: save the highlighted entry's markdown to a file in the current directory. | | `y` | Entries level only: copy a `bely-cli entry get` reference for the highlighted entry to the clipboard. | | `e` | Entries level only: open the highlighted entry in `$EDITOR`; if you change it, offers to save the result back to the server (a mutation, so this is where the app authenticates if it hasn't already). | -| `p` | Entries level only: reply to the highlighted entry's top-level thread. Attachments are supported. | -| `t` | Entries level only: collapse/expand the reply thread under the highlighted entry (or its parent, if the highlight is on a reply). Replies start expanded. | +| `p` | Entries level only: add a reply to the highlighted entry's top-level thread. Attachments are supported. | +| `t` | Entries level only, when the document has replies: toggle (collapse/expand) the reply thread under the highlighted entry, or its parent if the highlight is on a reply. Replies start expanded. | | `Shift+A` | Entries level only: open the highlighted entry's attachment browser. | | `x` | Document/entry levels only: delete the highlighted document, entry, or reply after destructive confirmation. Documents containing entries require a second confirmation by typing the document name exactly. | | `i` | Logbook/document levels only: toggle the side info panel. | diff --git a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py index 02638c259..2629bbd75 100644 --- a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py +++ b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py @@ -109,13 +109,13 @@ class BrowseScreen(Screen): Binding("q", "quit_app", "Quit"), Binding("slash", "focus_filter", "Filter"), Binding("f", "toggle_full", "Full"), - Binding("t", "toggle_replies", "Replies"), + Binding("t", "toggle_replies", "Toggle replies"), Binding("s", "save_entry", "Save"), Binding("y", "copy_reference", "Copy ref"), Binding("e", "open_editor", "Edit in editor"), Binding("n", "new_entry", "New entry"), Binding("u", "update_entry", "Edit in TUI"), - Binding("p", "reply", "Reply"), + Binding("p", "reply", "Add reply"), # Printable shifted letters arrive from terminals as the uppercase character. Binding("A", "attachments", "Attachments"), Binding("x", "delete", "Delete"), @@ -688,6 +688,9 @@ def check_action(self, action, parameters): return None if action == "new_entry" and self._current_doc() is None: return None + if action == "toggle_replies" and not any( + getattr(entry, "log_replies", None) for entry in self.entry_tree): + return None if action == "reply" and self._current_node() is None: return None if action == "delete" and self._current_item() is None: diff --git a/tools/developer_tools/bely-cli/test/test_tui_app.py b/tools/developer_tools/bely-cli/test/test_tui_app.py index 017541734..92ccda573 100644 --- a/tools/developer_tools/bely-cli/test/test_tui_app.py +++ b/tools/developer_tools/bely-cli/test/test_tui_app.py @@ -724,8 +724,22 @@ async def test_reply_binding_only_available_for_entries_with_selection(self): self.assertIsNone(screen.check_action("reply", ())) await self._open_entries(pilot) self.assertTrue(screen.check_action("reply", ())) + self.assertIsNone(screen.check_action("toggle_replies", ())) self.assertTrue(screen.check_action("refresh_level", ())) + async def test_toggle_replies_binding_available_when_document_has_replies(self): + api = FakeLogbookApiWithReplies() + app = BelyTuiApp(FakeSession(LogbookData(api)), limit=10, mode="lookup") + async with app.run_test() as pilot: + await self._open_entries(pilot) + self.assertTrue(app.screen.check_action("toggle_replies", ())) + self.assertTrue(app.screen.check_action("reply", ())) + + def test_reply_binding_labels_distinguish_adding_from_toggling(self): + bindings = {binding.key: binding.description for binding in BrowseScreen.BINDINGS} + self.assertEqual(bindings["t"], "Toggle replies") + self.assertEqual(bindings["p"], "Add reply") + async def test_update_entry_key_opens_compose_prefilled_and_cancel_returns(self): api = FakeLogbookApi() data = LogbookData(api) From c0ae1b8be22546a4f4301e922efeb3a70d0d46d2 Mon Sep 17 00:00:00 2001 From: Dariusz Jarosz Date: Mon, 12 Oct 2026 13:25:04 -0500 Subject: [PATCH 3/8] resolve test --- tools/developer_tools/bely-cli/test/test_tui_app.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tools/developer_tools/bely-cli/test/test_tui_app.py b/tools/developer_tools/bely-cli/test/test_tui_app.py index 92ccda573..fd5cce100 100644 --- a/tools/developer_tools/bely-cli/test/test_tui_app.py +++ b/tools/developer_tools/bely-cli/test/test_tui_app.py @@ -428,7 +428,10 @@ async def test_footer_shortcuts_track_the_current_level(self): self.assertIsNone(screen.check_action("toggle_info", ())) for action in entry_only_actions: - self.assertTrue(screen.check_action(action, ())) + if action == "toggle_replies": + self.assertIsNone(screen.check_action(action, ())) + else: + self.assertTrue(screen.check_action(action, ())) async def test_escape_at_landing_screen_does_not_quit(self): # BrowseScreen is the landing screen in mode="app" too; only `q` quits. From 6dc9557c35564f39b015e72a90cac50254d7c745 Mon Sep 17 00:00:00 2001 From: Dariusz Jarosz Date: Mon, 12 Oct 2026 13:37:50 -0500 Subject: [PATCH 4/8] Add a new shortcuts page that allows user to see relevant shortcuts and search for new ones. --- tools/developer_tools/bely-cli/README.md | 11 +- .../src/bely_cli/tui/screens/browse.py | 17 +- .../src/bely_cli/tui/screens/shortcuts.py | 146 ++++++++++++++++++ .../bely-cli/test/test_tui_app.py | 25 +++ .../bely-cli/test/test_tui_screens.py | 69 +++++++++ 5 files changed, 264 insertions(+), 4 deletions(-) create mode 100644 tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py diff --git a/tools/developer_tools/bely-cli/README.md b/tools/developer_tools/bely-cli/README.md index bbb289a95..1dd9410fd 100644 --- a/tools/developer_tools/bely-cli/README.md +++ b/tools/developer_tools/bely-cli/README.md @@ -404,11 +404,18 @@ you highlight each entry) — and, for a reply, which entry it's a reply to. Keys: -The footer at the bottom of the screen only ever shows the keys that apply to the level you're -on — `i` disappears once you drill into entries, and `s` / `y` / `e` / `f` / `t` only appear there. +Press `?` to open a categorized keyboard-shortcut reference. With its search box empty it +shows the shortcuts relevant to the current browse level; type a key name such as `ctrl+s` +or an action such as `reply` to search the complete TUI shortcut catalog. Search matches +shortcut notation and action text. Press `Esc` or `?` to close it. + +The footer at the bottom of the screen only ever shows the keys that apply to the level and +data you're viewing — `i` disappears once you drill into entries, and `s` / `y` / `e` / `f` +only appear there. `t` appears at the entries level only when the document has replies. | Key | Action | |-----|--------| +| `?` | Open the searchable, categorized keyboard-shortcut reference. | | `Up` / `Down`, `PgUp` / `PgDn` | Move the highlight; the preview/info panel follows. | | `/` | Reveal and focus the filter box; incrementally filters the current table (case-insensitive substring, matched against the entry text — not the reply tree's glyphs or reply count). | | `Enter` | In the filter box, return focus to the table. Elsewhere, drill into the highlighted row, or select the entry at the entries level. | diff --git a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py index 2629bbd75..a53eccbf9 100644 --- a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py +++ b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py @@ -107,6 +107,7 @@ class BrowseScreen(Screen): Binding("escape", "back", "Back"), Binding("backspace", "back", "Back", show=False), Binding("q", "quit_app", "Quit"), + Binding("question_mark", "show_shortcuts", "Shortcuts"), Binding("slash", "focus_filter", "Filter"), Binding("f", "toggle_full", "Full"), Binding("t", "toggle_replies", "Toggle replies"), @@ -669,6 +670,16 @@ def _exit_top(self): def action_quit_app(self): self.app.exit(None) + def action_show_shortcuts(self): + from .shortcuts import ShortcutScreen + + context = { + self.LEVEL_TYPES: "types", + self.LEVEL_DOCS: "docs", + self.LEVEL_ENTRIES: "entries", + }[self.level] + self.app.push_screen(ShortcutScreen(context, has_replies=self._has_replies())) + def action_focus_filter(self): filt = self.query_one("#filter", Input) filt.display = True @@ -688,8 +699,7 @@ def check_action(self, action, parameters): return None if action == "new_entry" and self._current_doc() is None: return None - if action == "toggle_replies" and not any( - getattr(entry, "log_replies", None) for entry in self.entry_tree): + if action == "toggle_replies" and not self._has_replies(): return None if action == "reply" and self._current_node() is None: return None @@ -701,6 +711,9 @@ def check_action(self, action, parameters): return None return True + def _has_replies(self): + return any(getattr(entry, "log_replies", None) for entry in self.entry_tree) + def action_refresh_level(self): # Do not start another entry refresh while its preview state is being # restored; repeated refreshes would otherwise race over one snapshot. diff --git a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py new file mode 100644 index 000000000..96bd65d04 --- /dev/null +++ b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py @@ -0,0 +1,146 @@ +"""Searchable keyboard-shortcut reference for the TUI.""" + +from typing import NamedTuple + +from textual import events +from textual.app import ComposeResult +from textual.binding import Binding +from textual.containers import Vertical, VerticalScroll +from textual.widgets import Input, Static + +from .dialog import DialogScreen + + +class Shortcut(NamedTuple): + key: str + action: str + category: str + contexts: tuple[str, ...] + + +CATEGORY_ORDER = ( + "Navigation", + "Browsing and display", + "Documents and entries", + "Attachments", + "Dialogs and editing", + "Application", +) + +ALL_SHORTCUTS = ( + Shortcut("Up / Down", "Move the highlight", "Navigation", ("types", "docs", "entries", "attachments")), + Shortcut("PgUp / PgDn", "Move by one page", "Navigation", ("types", "docs", "entries", "attachments")), + Shortcut("Enter", "Open or select the highlighted item", "Navigation", ("types", "docs", "entries")), + Shortcut("Esc / Backspace", "Go back one level", "Navigation", ("types", "docs", "entries")), + Shortcut("/", "Filter the current table", "Browsing and display", ("types", "docs", "entries")), + Shortcut("i", "Toggle the details panel", "Browsing and display", ("types", "docs")), + Shortcut("f", "Toggle the full-width entry preview", "Browsing and display", ("entries",)), + Shortcut("t", "Toggle replies", "Browsing and display", ("entries_with_replies",)), + Shortcut("r", "Refresh the current level", "Browsing and display", ("types", "docs", "entries")), + Shortcut("d", "Create a document", "Documents and entries", ("types", "docs")), + Shortcut("n", "Add a top-level entry", "Documents and entries", ("docs", "entries")), + Shortcut("p", "Add a reply", "Documents and entries", ("entries",)), + Shortcut("u", "Edit the highlighted entry in the TUI", "Documents and entries", ("entries",)), + Shortcut("e", "Edit the highlighted entry in $EDITOR", "Documents and entries", ("entries",)), + Shortcut("s", "Save the highlighted entry to a file", "Documents and entries", ("entries",)), + Shortcut("y", "Copy a CLI reference", "Documents and entries", ("entries",)), + Shortcut("x", "Delete the highlighted document or entry", "Documents and entries", ("docs", "entries")), + Shortcut("Shift+A", "Open the attachment browser", "Attachments", ("entries",)), + Shortcut("u", "Upload an attachment", "Attachments", ("attachments",)), + Shortcut("y", "Copy an attachment reference", "Attachments", ("attachments",)), + Shortcut("x", "Delete an attachment", "Attachments", ("attachments",)), + Shortcut("p", "Toggle the local-file preview", "Attachments", ("attachment_picker",)), + Shortcut("/", "Filter filenames", "Attachments", ("attachment_picker",)), + Shortcut("Tab / Right", "Complete a filesystem path", "Attachments", ("attachment_picker",)), + Shortcut("Ctrl+U", "Clear the filesystem path", "Attachments", ("attachment_picker",)), + Shortcut("Ctrl+Shift+A", "Select the filesystem path", "Attachments", ("attachment_picker",)), + Shortcut("Ctrl+S", "Submit or save the current dialog", "Dialogs and editing", ("dialogs",)), + Shortcut("Esc", "Cancel or close the current dialog", "Dialogs and editing", ("dialogs", "attachments")), + Shortcut("Tab / Shift+Tab", "Move between fields and buttons", "Dialogs and editing", ("dialogs",)), + Shortcut("Arrow keys", "Move between dialog buttons", "Dialogs and editing", ("dialogs",)), + Shortcut("Space", "Toggle an item in a multi-select picker", "Dialogs and editing", ("dialogs",)), + Shortcut("?", "Open or close keyboard shortcuts", "Application", ("types", "docs", "entries")), + Shortcut("Ctrl+P", "Open the command palette", "Application", ("types", "docs", "entries")), + Shortcut("q", "Quit", "Application", ("types", "docs", "entries")), +) + + +def shortcuts_for(context, *, has_replies=False, query=""): + """Return relevant shortcuts, or key/action matches from the full catalog.""" + if query: + needle = query.casefold() + return [ + shortcut for shortcut in ALL_SHORTCUTS + if needle in shortcut.key.casefold() or needle in shortcut.action.casefold() + ] + contexts = {context} + if context == "entries" and has_replies: + contexts.add("entries_with_replies") + return [shortcut for shortcut in ALL_SHORTCUTS if contexts.intersection(shortcut.contexts)] + + +def format_shortcuts(shortcuts): + """Format shortcut rows under stable logical category headings.""" + lines = [] + for category in CATEGORY_ORDER: + rows = [shortcut for shortcut in shortcuts if shortcut.category == category] + if not rows: + continue + if lines: + lines.append("") + lines.append(category) + lines.append("─" * len(category)) + width = max(len(shortcut.key) for shortcut in rows) + lines.extend(f"{shortcut.key:<{width}} {shortcut.action}" for shortcut in rows) + return "\n".join(lines) if lines else "No shortcuts match that key." + + +class ShortcutFilterInput(Input): + """Search field where `?` closes help instead of entering the character.""" + + def on_key(self, event: events.Key): + if event.key == "question_mark": + event.prevent_default() + event.stop() + self.screen.dismiss(None) + + +class ShortcutScreen(DialogScreen): + """Modal shortcut list, relevant by default and global while searching.""" + + DEFAULT_CSS = """ + #shortcut-dialog { width: 76; height: 80%; } + #shortcut-filter { margin: 1 0; } + #shortcut-scroll { height: 1fr; } + #shortcut-list { height: auto; } + """ + + BINDINGS = [ + Binding("question_mark", "cancel", "Close", show=False, priority=True), + ] + + def __init__(self, context, *, has_replies=False): + super().__init__() + self.context = context + self.has_replies = has_replies + + def compose(self) -> ComposeResult: + with Vertical(id="shortcut-dialog", classes="dialog"): + yield Static("Keyboard shortcuts · Esc/? close", id="shortcut-title") + yield ShortcutFilterInput( + placeholder="Search keys and actions", id="shortcut-filter") + with VerticalScroll(id="shortcut-scroll"): + yield Static(id="shortcut-list") + + def on_mount(self): + self._show("") + self.query_one("#shortcut-filter", Input).focus() + + def on_input_changed(self, event): + if event.input.id == "shortcut-filter": + self._show(event.value) + + def _show(self, query): + rows = shortcuts_for( + self.context, has_replies=self.has_replies, query=query.strip()) + self.query_one("#shortcut-list", Static).update(format_shortcuts(rows)) diff --git a/tools/developer_tools/bely-cli/test/test_tui_app.py b/tools/developer_tools/bely-cli/test/test_tui_app.py index fd5cce100..3817899dc 100644 --- a/tools/developer_tools/bely-cli/test/test_tui_app.py +++ b/tools/developer_tools/bely-cli/test/test_tui_app.py @@ -433,6 +433,31 @@ async def test_footer_shortcuts_track_the_current_level(self): else: self.assertTrue(screen.check_action(action, ())) + async def test_question_mark_opens_relevant_shortcut_modal(self): + data = LogbookData(FakeLogbookApi()) + app = BelyTuiApp(FakeSession(data), limit=10, mode="lookup") + async with app.run_test() as pilot: + await pilot.pause() + browse_screen = app.screen + await pilot.press("?") + await pilot.pause() + self.assertEqual(type(app.screen).__name__, "ShortcutScreen") + self.assertEqual(app.screen.context, "types") + self.assertFalse(app.screen.has_replies) + await pilot.press("?") + await pilot.pause() + self.assertIs(app.screen, browse_screen) + + async def test_entry_shortcut_modal_knows_document_has_replies(self): + data = LogbookData(FakeLogbookApiWithReplies()) + app = BelyTuiApp(FakeSession(data), limit=10, mode="lookup") + async with app.run_test() as pilot: + await self._open_entries(pilot) + await pilot.press("?") + await pilot.pause() + self.assertEqual(app.screen.context, "entries") + self.assertTrue(app.screen.has_replies) + async def test_escape_at_landing_screen_does_not_quit(self): # BrowseScreen is the landing screen in mode="app" too; only `q` quits. data = LogbookData(FakeLogbookApi()) diff --git a/tools/developer_tools/bely-cli/test/test_tui_screens.py b/tools/developer_tools/bely-cli/test/test_tui_screens.py index d73e65439..af3a84ac6 100644 --- a/tools/developer_tools/bely-cli/test/test_tui_screens.py +++ b/tools/developer_tools/bely-cli/test/test_tui_screens.py @@ -34,6 +34,9 @@ from bely_cli.tui.screens.login import LoginScreen from bely_cli.tui.screens.newdoc import NewDocScreen from bely_cli.tui.screens.picker import PickerScreen +from bely_cli.tui.screens.shortcuts import ( + ALL_SHORTCUTS, CATEGORY_ORDER, ShortcutScreen, format_shortcuts, shortcuts_for, +) from test.api_helpers import api_fake, method_mock @@ -102,6 +105,72 @@ def authenticated_api(self): return self.factory.get_logbook_api() +class ShortcutScreenTests(unittest.IsolatedAsyncioTestCase): + def test_catalog_covers_categories_and_orders_them_logically(self): + self.assertEqual( + {shortcut.category for shortcut in ALL_SHORTCUTS}, set(CATEGORY_ORDER)) + rendered = format_shortcuts(ALL_SHORTCUTS) + positions = [rendered.index(category) for category in CATEGORY_ORDER] + self.assertEqual(positions, sorted(positions)) + + def test_relevance_and_search_of_full_catalog(self): + type_keys = {shortcut.key for shortcut in shortcuts_for("types")} + self.assertIn("d", type_keys) + self.assertNotIn("t", type_keys) + self.assertNotIn("Ctrl+S", type_keys) + + entry_keys = {shortcut.key for shortcut in shortcuts_for("entries")} + self.assertIn("p", entry_keys) + self.assertNotIn("t", entry_keys) + entry_reply_keys = { + shortcut.key for shortcut in shortcuts_for("entries", has_replies=True)} + self.assertIn("t", entry_reply_keys) + + matches = shortcuts_for("types", query="ctrl+s") + self.assertTrue(matches) + self.assertTrue(all("ctrl+s" in shortcut.key.casefold() for shortcut in matches)) + reply_matches = shortcuts_for("types", query="repl") + self.assertEqual( + {shortcut.action for shortcut in reply_matches}, + {"Toggle replies", "Add a reply"}, + ) + + async def test_modal_filters_and_closes_with_question_mark(self): + app = App() + async with app.run_test() as pilot: + task = app.run_worker( + app.push_screen_wait(ShortcutScreen("types", has_replies=False))) + await pilot.pause() + screen = app.screen + listing = screen.query_one("#shortcut-list", Static) + self.assertNotIn("Toggle replies", str(listing.render())) + + filter_input = screen.query_one("#shortcut-filter", Input) + filter_input.value = "ctrl+s" + await pilot.pause() + self.assertIn("Submit or save", str(listing.render())) + filter_input.value = "repl" + await pilot.pause() + self.assertIn("Toggle replies", str(listing.render())) + self.assertIn("Add a reply", str(listing.render())) + filter_input.value = "not-a-shortcut" + await pilot.pause() + self.assertIn("No shortcuts match that key.", str(listing.render())) + + filter_input.value = "" + await pilot.pause() + await pilot.press("?") + self.assertIsNone(await task.wait()) + + async def test_modal_closes_with_escape(self): + app = App() + async with app.run_test() as pilot: + task = app.run_worker(app.push_screen_wait(ShortcutScreen("entries"))) + await pilot.pause() + await pilot.press("escape") + self.assertIsNone(await task.wait()) + + class AttachmentPreviewKindTests(unittest.TestCase): def test_classifies_preview_types(self): self.assertEqual(preview_kind("plot.PNG"), "image") From 2c27a412b9e18a68fcf029af4d7eea5af7bcfa5c Mon Sep 17 00:00:00 2001 From: Dariusz Jarosz Date: Mon, 12 Oct 2026 14:12:41 -0500 Subject: [PATCH 5/8] Improve shortcut detection --- .../bely-cli/src/bely_cli/tui/screens/shortcuts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py index 96bd65d04..00bbc13ae 100644 --- a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py +++ b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py @@ -99,7 +99,7 @@ class ShortcutFilterInput(Input): """Search field where `?` closes help instead of entering the character.""" def on_key(self, event: events.Key): - if event.key == "question_mark": + if event.key in {"question_mark", "?"} or event.character == "?": event.prevent_default() event.stop() self.screen.dismiss(None) From 13657c29552ef58f64ca74526564caecfcbc2904 Mon Sep 17 00:00:00 2001 From: Dariusz Jarosz Date: Mon, 12 Oct 2026 14:23:02 -0500 Subject: [PATCH 6/8] Add improved styling --- .../src/bely_cli/tui/screens/shortcuts.py | 70 +++++++++++++++---- .../bely-cli/test/test_tui_screens.py | 17 +++-- 2 files changed, 68 insertions(+), 19 deletions(-) diff --git a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py index 00bbc13ae..a77664aba 100644 --- a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py +++ b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/shortcuts.py @@ -6,7 +6,7 @@ from textual.app import ComposeResult from textual.binding import Binding from textual.containers import Vertical, VerticalScroll -from textual.widgets import Input, Static +from textual.widgets import Input, Label, Static from .dialog import DialogScreen @@ -79,13 +79,19 @@ def shortcuts_for(context, *, has_replies=False, query=""): return [shortcut for shortcut in ALL_SHORTCUTS if contexts.intersection(shortcut.contexts)] +def grouped_shortcuts(shortcuts): + """Return non-empty shortcut categories in display order.""" + return [ + (category, [shortcut for shortcut in shortcuts if shortcut.category == category]) + for category in CATEGORY_ORDER + if any(shortcut.category == category for shortcut in shortcuts) + ] + + def format_shortcuts(shortcuts): """Format shortcut rows under stable logical category headings.""" lines = [] - for category in CATEGORY_ORDER: - rows = [shortcut for shortcut in shortcuts if shortcut.category == category] - if not rows: - continue + for category, rows in grouped_shortcuts(shortcuts): if lines: lines.append("") lines.append(category) @@ -113,6 +119,24 @@ class ShortcutScreen(DialogScreen): #shortcut-filter { margin: 1 0; } #shortcut-scroll { height: 1fr; } #shortcut-list { height: auto; } + .shortcut-section-header { + width: 100%; + height: 1; + margin: 1 0 0 0; + padding: 0 1; + background: $primary; + color: $text; + text-style: bold; + } + .shortcut-rows { + height: auto; + padding: 0 1; + } + .shortcut-empty { + height: auto; + padding: 1; + color: $text-muted; + } """ BINDINGS = [ @@ -130,17 +154,37 @@ def compose(self) -> ComposeResult: yield ShortcutFilterInput( placeholder="Search keys and actions", id="shortcut-filter") with VerticalScroll(id="shortcut-scroll"): - yield Static(id="shortcut-list") + yield Vertical(id="shortcut-list") - def on_mount(self): - self._show("") + async def on_mount(self): + await self._show("") self.query_one("#shortcut-filter", Input).focus() - def on_input_changed(self, event): + async def on_input_changed(self, event): if event.input.id == "shortcut-filter": - self._show(event.value) + await self._show(event.value) - def _show(self, query): - rows = shortcuts_for( + async def _show(self, query): + shortcuts = shortcuts_for( self.context, has_replies=self.has_replies, query=query.strip()) - self.query_one("#shortcut-list", Static).update(format_shortcuts(rows)) + listing = self.query_one("#shortcut-list", Vertical) + await listing.remove_children() + groups = grouped_shortcuts(shortcuts) + if not groups: + await listing.mount( + Static("No shortcuts match that key.", classes="shortcut-empty")) + return + widgets = [] + for category, rows in groups: + width = max(len(shortcut.key) for shortcut in rows) + widgets.extend(( + Label(category, classes="shortcut-section-header"), + Static( + "\n".join( + f"[b]{shortcut.key:<{width}}[/b] {shortcut.action}" + for shortcut in rows + ), + classes="shortcut-rows", + ), + )) + await listing.mount_all(widgets) diff --git a/tools/developer_tools/bely-cli/test/test_tui_screens.py b/tools/developer_tools/bely-cli/test/test_tui_screens.py index af3a84ac6..4869321d6 100644 --- a/tools/developer_tools/bely-cli/test/test_tui_screens.py +++ b/tools/developer_tools/bely-cli/test/test_tui_screens.py @@ -142,20 +142,25 @@ async def test_modal_filters_and_closes_with_question_mark(self): app.push_screen_wait(ShortcutScreen("types", has_replies=False))) await pilot.pause() screen = app.screen - listing = screen.query_one("#shortcut-list", Static) - self.assertNotIn("Toggle replies", str(listing.render())) + def listing_text(): + return "\n".join( + str(widget.render()) + for widget in screen.query(".shortcut-rows, .shortcut-empty") + ) + + self.assertNotIn("Toggle replies", listing_text()) filter_input = screen.query_one("#shortcut-filter", Input) filter_input.value = "ctrl+s" await pilot.pause() - self.assertIn("Submit or save", str(listing.render())) + self.assertIn("Submit or save", listing_text()) filter_input.value = "repl" await pilot.pause() - self.assertIn("Toggle replies", str(listing.render())) - self.assertIn("Add a reply", str(listing.render())) + self.assertIn("Toggle replies", listing_text()) + self.assertIn("Add a reply", listing_text()) filter_input.value = "not-a-shortcut" await pilot.pause() - self.assertIn("No shortcuts match that key.", str(listing.render())) + self.assertIn("No shortcuts match that key.", listing_text()) filter_input.value = "" await pilot.pause() From 983df0e6d182bd1ca64dc4aba763ae17732f0f6d Mon Sep 17 00:00:00 2001 From: Dariusz Jarosz Date: Mon, 12 Oct 2026 14:23:27 -0500 Subject: [PATCH 7/8] clean up --- tools/developer_tools/bely-cli/PLAN.md | 37 -------------------------- 1 file changed, 37 deletions(-) delete mode 100644 tools/developer_tools/bely-cli/PLAN.md diff --git a/tools/developer_tools/bely-cli/PLAN.md b/tools/developer_tools/bely-cli/PLAN.md deleted file mode 100644 index baae57f56..000000000 --- a/tools/developer_tools/bely-cli/PLAN.md +++ /dev/null @@ -1,37 +0,0 @@ -# Fix attachment image preview race - -## Context - -Rapid attachment/file preview changes can leave the previous image widget mounted while a new image preview callback mounts another widget with the same fixed ID. Textual then raises `DuplicateIds` for `attachment-image` (and the local-file preview has the same asynchronous removal pattern). SSH/terminal image rendering can expose this timing race even when local rendering does not, because the current code starts deferred removal but mounts the replacement without waiting for removal to finish. - -## Approach - -First reproduce the reported failure deterministically in a Textual test by mounting an initial fixed-ID image widget, initiating the existing deferred removal path, and immediately running the next image callback. Confirm the test fails with `DuplicateIds` before changing production code. - -Then move image-container replacement into each async UI callback: await `remove_children()`, re-check the existing preview token after that await, and only then mount the current image. This serializes Textual’s deferred removal with mounting, prevents duplicate fixed IDs, and prevents a callback that became stale while awaiting removal from displaying old content. Apply the same lifecycle handling to remote attachment previews and local upload-file previews; retain selection-time clearing for text/metadata previews. - -## Files to modify - -- `src/bely_cli/tui/screens/attachments.py` — coordinate asynchronous image-widget replacement. -- `test/test_tui_screens.py` — add regression coverage for repeated/racing image previews (exact test location to confirm). - -## Reuse - -- Existing `_preview_token` guards in both `AttachmentScreen` and `AttachmentFileScreen`. -- Existing preview methods and Textual test-app patterns in `test/test_tui_screens.py`. - -## Steps - -- [x] **Milestone 1 — reproducer only:** Add and run a focused Textual regression test against unchanged production code to demonstrate the reported `DuplicateIds` failure. Pause and hand control back for review; do not implement the fix yet. -- [x] **Milestone 2 — minimal fix:** After user approval, update `_show_image()` to await removal of any existing media child, revalidate the token, and mount exactly one image widget. Keep selection-time clearing so text/metadata selections promptly remove prior images. Re-run the reproducer and pause for review. -- [x] **Milestone 3 — parallel path and edge case:** After user approval, apply equivalent handling and regression coverage to `_show_local_image()`, and add a stale-token assertion so an outdated callback cannot mount after replacement awaits. Run focused tests and pause for review. -- [x] **Milestone 4 — broader verification:** After user approval, run the requested broader test suite and report results for final review. -- [x] Update user-facing documentation only if behavior/surface changes (none expected). - -## Verification - -- Record the focused reproducer failing before the fix and pause for review. -- After approval and implementation, record the same reproducer passing and pause again. -- Run focused attachment/TUI screen tests. -- Run the full test suite via `uv run pytest -n auto test` (or `./run_test.sh`). -- Manually move rapidly between image and non-image attachments/local files and confirm no traceback or stale image appears. From af8b951f7470c95a401ae623f3e14917624cf6dd Mon Sep 17 00:00:00 2001 From: Dariusz Jarosz Date: Mon, 12 Oct 2026 14:38:13 -0500 Subject: [PATCH 8/8] Resolve issue with entry not auto-refreshing after a change. --- .../src/bely_cli/tui/screens/browse.py | 28 +++++++++++-- .../bely-cli/test/test_tui_app.py | 39 ++++++++++++++++++- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py index a53eccbf9..f9b2fb0e3 100644 --- a/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py +++ b/tools/developer_tools/bely-cli/src/bely_cli/tui/screens/browse.py @@ -150,6 +150,7 @@ def __init__(self, session, limit, *, select_mode=True, source="types", root=Fal self._restoring_entry_preview = False self._restoring_entry_id = None self._pending_doc_row = None + self._entry_refresh_pending = False def compose(self) -> ComposeResult: yield Header() @@ -174,6 +175,11 @@ def on_mount(self): def on_screen_resume(self): self._update_auth_status() + if self._entry_refresh_pending and self.level == self.LEVEL_ENTRIES: + self._entry_refresh_pending = False + entry = self._current_entry() + if entry is not None: + self._reload_entry(entry.log_id) # -- nav widget -- @@ -959,9 +965,7 @@ async def _edit_entry_externally(self, entry): ) return - self.data.invalidate("entries", doc_id=self.sel_doc.id) - self.show_level( - self.LEVEL_ENTRIES, preserve_filter=True, preserve_entry_position=True) + self._reload_entry(entry.log_id) self.notify("Entry saved.") def action_attachments(self): @@ -975,8 +979,21 @@ def action_attachments(self): self.session, self.sel_doc, entry, on_changed=self._refresh_attachment_entry)) def _refresh_attachment_entry(self): - """Reload the entry after its attachment references change.""" + """Reload the entry once the attachment modal closes.""" self.data.invalidate("entries", doc_id=self.sel_doc.id) + self._entry_refresh_pending = True + + def _reload_entry(self, log_id): + """Fetch and rerender one mutated entry while preserving its position.""" + self.data.invalidate("entries", doc_id=self.sel_doc.id) + self._pending_entry_restore = ( + log_id, + None, + self._nav().cursor_row or 0, + self.query_one("#preview", VerticalScroll).scroll_y, + ) + self._restoring_entry_preview = True + self._restoring_entry_id = log_id self.show_level( self.LEVEL_ENTRIES, preserve_filter=True, preserve_entry_position=True) @@ -1019,6 +1036,9 @@ async def _run_compose(self, doc, entry, reply_to=None): return self.sel_doc = doc self.data.invalidate("entries", doc_id=doc.id) + if entry is not None: + self._reload_entry(saved.log_id) + return if reply_to is not None: self._collapsed.discard(reply_to.log_id) saved_id = getattr(saved, "log_id", None) or reply_to.log_id diff --git a/tools/developer_tools/bely-cli/test/test_tui_app.py b/tools/developer_tools/bely-cli/test/test_tui_app.py index 3817899dc..4ca2fcad3 100644 --- a/tools/developer_tools/bely-cli/test/test_tui_app.py +++ b/tools/developer_tools/bely-cli/test/test_tui_app.py @@ -59,6 +59,9 @@ def get_log_documents(self, logbook_type_id, limit): )] def get_log_entries(self, log_document_id, load_replies, load_reactions): + saved_entry = getattr(self, "saved_entry", None) + if saved_entry is not None: + return [saved_entry] return [SimpleNamespace( log_id=100, entered_by_username="alice", entered_on_date_time=None, last_modified_by_username=None, last_modified_on_date_time=None, @@ -72,7 +75,8 @@ def get_log_entry_template(self, log_document_id): return SimpleNamespace(log_id=None, log_entry="") def add_update_log_entry(self, log_entry): - log_entry.log_id = 101 + log_entry.log_id = log_entry.log_id or 101 + self.saved_entry = log_entry return log_entry @@ -676,10 +680,12 @@ async def test_shift_a_opens_attachments_for_highlighted_entry(self): self.assertEqual(app.screen.entry.log_id, 100) app.screen.on_changed() await pilot.pause() - self.assertEqual(get_entries.call_count, 2) + self.assertEqual(get_entries.call_count, 1) await pilot.press("escape") await pilot.pause() + await pilot.pause() self.assertIs(app.screen, browse_screen) + self.assertEqual(get_entries.call_count, 2) async def test_delete_populated_document_requires_typed_confirmation(self): api = FakeLogbookApi() @@ -854,6 +860,35 @@ async def test_edit_key_with_editor_changes_offers_save_and_saves(self): self.assertEqual(type(app.screen).__name__, "BrowseScreen") saved_entry = save_call.call_args.kwargs["log_entry"] self.assertIn("edited in $EDITOR", saved_entry.log_entry) + self.assertIn("edited in $EDITOR", screen._current_entry().log_entry) + self.assertIn( + "edited in $EDITOR", + screen.query_one("#body-md", Markdown).source, + ) + + async def test_tui_edit_refreshes_entry_content_and_preview(self): + api = FakeLogbookApi() + data = LogbookData(api) + session = FakeSession(data, factory=FakeFactory(api=api)) + app = BelyTuiApp(session, limit=10, mode="lookup") + async with app.run_test() as pilot: + await self._open_entries(pilot) + screen = app.screen + await pilot.press("u") + await pilot.pause() + await pilot.pause() + + from textual.widgets import Button, TextArea + + app.screen.query_one("#compose-area", TextArea).text = "updated in TUI" + app.screen.query_one("#compose-save", Button).press() + await pilot.pause() + await pilot.pause() + await pilot.pause() + + self.assertIs(app.screen, screen) + self.assertEqual(screen._current_entry().log_entry, "updated in TUI") + self.assertEqual(screen.query_one("#body-md", Markdown).source, "updated in TUI") class ThemePersistenceTests(unittest.IsolatedAsyncioTestCase):