Skip to content

Commit 22149e6

Browse files
committed
fix: 'Save screenshot' failed with "Failed to save screenshot" - defaulted to an unwritable cwd
Real bug reported via live testing: the Command Palette's screenshot command (re-enabled in the previous release) failed with a bare "Failed to save screenshot" error every time inside the companion container. Root cause confirmed directly: Textual's own default implementation saves to the current working directory, and the container's WORKDIR (/app) is root-owned while the container runs as a non-root user - os.access('/app', os.W_OK) confirmed False inside a real running container, os.access(home, os.W_OK) confirmed True. Replaced Textual's own auto-generated 'Save screenshot' command with our own, same title, calling deliver_screenshot(path=str(Path.home())) explicitly instead of leaving it to default to cwd - home directory always exists and is always writable by whoever's running this tool, and is already where this tool keeps its own state (state.py). Verified the fix directly inside a real built container image, not just reasoned about: confirmed cwd genuinely unwritable there, screenshot save now succeeds where it previously failed with the exact reported error. 243 tests passing (2 new: confirms the replacement command passes the home-directory path, confirms a failure is reported without crashing rather than left silent); verified sdist->wheel build. Version bumped to 0.1.21 for release.
1 parent a4d8822 commit 22149e6

4 files changed

Lines changed: 78 additions & 18 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ Confirmed NOT sufficient on its own under rootless Podman specifically
205205
Podman rather than a standard root-owned `dockerd` socket, this may
206206
need more digging into your specific setup.
207207

208-
Images are tagged by version (`:0.1.20`) and `:latest`, built and
208+
Images are tagged by version (`:0.1.21`) and `:latest`, built and
209209
published automatically on every release.
210210

211211
## Why not just fix ovos-cli-client / neon-cli-client?

ovos_tui_client/app.py

Lines changed: 37 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -995,23 +995,22 @@ def get_system_commands(self, screen: Screen):
995995
nothing at all) is filtered out below - it's the exact same
996996
show-help-panel action our own 'Help: Toggle panel' entry
997997
already calls, so keeping both would just be two entries doing
998-
the same thing under different names. 'Screenshot' (titled
999-
"Save screenshot" - saves an SVG snapshot of the current
1000-
terminal view) is deliberately kept: an earlier pass through
1001-
this file tried to filter it out as "not useful for this
1002-
tool", but that's been reconsidered - an SVG capture of
1003-
exactly what's on screen is a genuinely handy way to save or
1004-
share a specific debugging moment.
1005-
1006-
No separate description/help line under any entry - state
1007-
(checked/unchecked, active/inactive) is embedded directly in
1008-
the title text instead (e.g. "Log: Source: skills
1009-
(checked)"), matching how the dynamic Provider-based entries
1010-
already work. One line per command everywhere, nothing more
1011-
than necessary shown."""
998+
the same thing under different names.
999+
1000+
Textual's own default 'Save screenshot' command is ALSO
1001+
filtered out here, replaced with our own below - not because
1002+
the feature isn't wanted (it is), but because Textual's own
1003+
version saves to the current working directory by default,
1004+
which real testing confirmed fails with "Failed to save
1005+
screenshot" whenever that directory isn't writable - reliably
1006+
the case inside the companion container specifically (WORKDIR
1007+
/app is root-owned, the container runs as a non-root user).
1008+
Our replacement passes an explicit, always-writable path
1009+
(the user's home directory) instead."""
10121010
for cmd in super().get_system_commands(screen):
1013-
if "help panel" not in cmd.title.lower():
1011+
if "help panel" not in cmd.title.lower() and cmd.title != "Save screenshot":
10141012
yield cmd
1013+
yield SystemCommand("Save screenshot", "", self._save_screenshot)
10151014
yield SystemCommand("Help: Toggle panel", "", self.action_toggle_help_panel)
10161015
yield SystemCommand("Focus: Logs", "", self.action_focus_logs)
10171016
yield SystemCommand("Focus: Conversation", "", self.action_focus_conversation)
@@ -1084,6 +1083,29 @@ def _deselect_all_skills(self) -> None:
10841083
self.skill_enabled[skill_id] = False
10851084
self._rerender_logs()
10861085

1086+
def _save_screenshot(self) -> None:
1087+
"""Replacement for Textual's own default 'Save screenshot'
1088+
command - see get_system_commands()'s docstring for why this
1089+
exists instead of just using Textual's built-in one directly
1090+
(it defaults to the current working directory, which real
1091+
testing confirmed isn't reliably writable, especially inside
1092+
the companion container). Home directory instead - always
1093+
exists, always writable by the user running this tool, and is
1094+
already where this tool keeps its own state (state.py).
1095+
1096+
Scheduled via set_timer(), same as Textual's own version -
1097+
matters because the command palette itself is still visually
1098+
on screen at the exact moment this callback fires, and would
1099+
otherwise end up IN the screenshot rather than showing the app
1100+
underneath it."""
1101+
def _do_save():
1102+
try:
1103+
path = self.deliver_screenshot(path=str(Path.home()))
1104+
self._write_status(f"Screenshot saved to {path}" if path else "Screenshot saved")
1105+
except Exception as e:
1106+
self._write_status(f"Failed to save screenshot: {e}")
1107+
self.set_timer(0.1, _do_save)
1108+
10871109
def action_toggle_help_panel(self) -> None:
10881110
"""F1 (and 'Help: Toggle panel' in the Command Palette) toggle
10891111
Textual's own built-in HelpPanel - a genuine side panel docked

tests/test_command_palette.py

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,13 +315,51 @@ async def test_screenshot_command_is_available(tmp_path):
315315
to be filtered out here as "not useful for this tool" -
316316
reconsidered: it's a genuinely handy way to save/share a specific
317317
debugging moment, and Textual already provides it for free, so
318-
it's no longer filtered."""
318+
it's no longer filtered - though see the next test, it's actually
319+
OUR OWN replacement command by this point, not Textual's default
320+
one directly (same title, different implementation)."""
319321
app = _app_with_fake_bus(tmp_path)
320322
async with app.run_test() as pilot:
321323
titles = [cmd.title for cmd in app.get_system_commands(app.screen)]
322324
assert any("screenshot" in t.lower() for t in titles)
323325

324326

327+
@pytest.mark.asyncio
328+
async def test_save_screenshot_uses_home_directory_not_cwd(tmp_path):
329+
"""Real bug found via live testing: Textual's own default 'Save
330+
screenshot' command saves to the current working directory, which
331+
isn't reliably writable - confirmed failing inside the companion
332+
container specifically (WORKDIR /app is root-owned, container runs
333+
as a non-root user), with a real "Failed to save screenshot" error
334+
shown to the user. Our own replacement command passes an explicit
335+
home-directory path instead - this confirms it does, without
336+
actually writing a real screenshot file (deliver_screenshot itself
337+
is Textual's own, already-tested machinery, not something this
338+
project needs to re-verify)."""
339+
from pathlib import Path
340+
app = _app_with_fake_bus(tmp_path)
341+
async with app.run_test() as pilot:
342+
with patch.object(app, "deliver_screenshot", return_value="/home/fake/screenshot.svg") as mock_deliver:
343+
app._save_screenshot()
344+
await pilot.pause(0.2) # the real command schedules via set_timer(0.1, ...)
345+
mock_deliver.assert_called_once_with(path=str(Path.home()))
346+
conv = app.query_one("#conversation", RichLog)
347+
text = "\n".join(str(line) for line in conv.lines)
348+
assert "screenshot.svg" in text.lower() or "saved" in text.lower()
349+
350+
351+
@pytest.mark.asyncio
352+
async def test_save_screenshot_reports_failure_without_crashing(tmp_path):
353+
app = _app_with_fake_bus(tmp_path)
354+
async with app.run_test() as pilot:
355+
with patch.object(app, "deliver_screenshot", side_effect=OSError("disk full")):
356+
app._save_screenshot()
357+
await pilot.pause(0.2) # must not raise
358+
conv = app.query_one("#conversation", RichLog)
359+
text = "\n".join(str(line) for line in conv.lines)
360+
assert "failed" in text.lower()
361+
362+
325363
@pytest.mark.asyncio
326364
async def test_other_textual_defaults_are_not_filtered(tmp_path):
327365
"""Confirms the filter is specific to 'Keys' (redundant with our

version.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
VERSION_MAJOR = 0
22
VERSION_MINOR = 1
3-
VERSION_BUILD = 20
3+
VERSION_BUILD = 21
44
VERSION_ALPHA = 0
55
# END_VERSION_BLOCK

0 commit comments

Comments
 (0)