feat(ux): three high-leverage context-menu essentials from #1764
Issue #1764 asked for a much larger surface (Reveal + Copy-path on every UI surface that references a file path, plus Rename in session menus). Per Nathan's curation we ship only the three highest-leverage pieces in this PR — they cover the three concrete user-visible frictions Cygnus reported, and leave the broader sweep for follow-up. ## 1. Copy file path in workspace tree right-click menu The tree's right-click already had Rename and Reveal in File Manager. Reveal is slow when the user just wants the path string for a terminal/editor — and there was no Copy-path action anywhere. Added "Copy file path" between Reveal and Delete. It POSTs to a new `/api/file/path` endpoint that resolves the relative tree-rooted path into the absolute on-disk path (the frontend can't compute it because only the server knows the workspace root) and writes the result to the OS clipboard via `navigator.clipboard.writeText()`. Falls back to the legacy execCommand pattern on browsers where the modern Clipboard API is gated. The new endpoint deliberately does NOT require the target to exist: copy-path on a recently-deleted file is still useful (paste into a terminal to investigate). `safe_resolve` continues to gate path traversal — the test suite pins this with a `../../../../../etc/passwd` attempt that 400s. ## 2. Rename in session three-dot menu Cygnus's specific ask: double-click rename in the sidebar is timing- sensitive — the first click frequently registers as "open the chat" before the second click arrives, so users open the conversation when they meant to rename it. Putting Rename in the menu eliminates the timing entirely. Added Rename as the FIRST item in `_openSessionActionMenu` (above Pin). It reuses the existing `startRename` closure attached to each session row — no duplicated state, no second API call out of band with the double-click path. Mechanism: the row builder now stores `el._startRename = startRename` and `el.dataset.sid = s.session_id`, so the menu can find the row by data-sid and call its closure directly. This keeps all the `_renamingSid`/`oldTitle`/`applyTitle` bookkeeping single-sourced. Read-only imported sessions skip the menu item via the same `_isReadOnlySession` gate the closure already uses. ## 3. Reveal-failed toast includes the resolved server-side path Cygnus posted a screenshot of a "Failed to reveal: not found" toast that dropped the path entirely. Without it the user can't tell which file the system expected — useful when a stale session row still references a deleted file. Server-side fix in `_handle_file_reveal`: instead of returning `bad(handler, "File not found", 404)`, return `bad(handler, f"File not found: {target}", 404)` where target is the resolved absolute path. Frontend toast also defends against err with no .message: `(err.message||err)` instead of `err.message` alone. Verified live: a missing-file reveal now produces: Failed to reveal: File not found: /home/hermes/workspace/missing-xyz.txt Cygnus's exact diagnostic-friction is gone. ## Tests * tests/test_1764_context_menu_essentials.py (new) - 13 source-level pinning tests - 6 live HTTP behaviour tests against the conftest test server * tests/test_1466_sidebar_cancel_clarify.py - Two assertion-window bumps (3200→4400, 3600→4800) to accommodate the new Rename action prepended to _openSessionActionMenu. The test relied on a fixed-byte-window function-body slice — comments added explaining why the bumps were needed. * All 9 locales got translations for the 5 new keys (copy_file_path, path_copied, path_copy_failed, session_rename, session_rename_desc) — locale parity tests pass. ## Verification Full pytest suite: 4671 passed, 2 skipped, 3 xpassed (matches pre-change baseline). Live browser verification on port 8789: - Right-click .git folder in workspace tree → menu shows Rename / Reveal in File Manager / Copy file path / Delete (red). - Click Copy file path → clipboard gets "/home/hermes/workspace/.git", toast confirms "File path copied to clipboard". - Open session three-dot menu → Rename conversation appears first with pencil icon, followed by Pin / Move / Archive / Duplicate / Delete in the same order as before. - Trigger reveal on a non-existent file → toast reads "Failed to reveal: File not found: /home/hermes/workspace/<filename>". The resolved server-side path is now visible in the failure. Refs nesquena/hermes-webui#1764.
This commit is contained in:
1 parent
922c3e530d
commit
f77a44fce2
6 files changed
+511
-4
No files matched your search
+39
-1
@@ -4022,6 +4022,9 @@ def handle_post(handler, parsed) -> bool:
|
||||
if parsed.path == "/api/file/reveal":
|
||||
return _handle_file_reveal(handler, body)
|
||||
|
||||
if parsed.path == "/api/file/path":
|
||||
return _handle_file_path(handler, body)
|
||||
|
||||
# ── Workspace management (POST) ──
|
||||
if parsed.path == "/api/workspaces/add":
|
||||
return _handle_workspace_add(handler, body)
|
||||
@@ -6619,7 +6622,13 @@ def _handle_file_reveal(handler, body):
|
||||
try:
|
||||
target = safe_resolve(Path(s.workspace), body["path"])
|
||||
if not target.exists():
|
||||
return bad(handler, "File not found", 404)
|
||||
# Include the resolved server-side path in the error message so
|
||||
# the frontend toast can show *which* file the system expected.
|
||||
# Useful when a stale session row still references a deleted file
|
||||
# (#1764 — Cygnus's screenshot showed a "Failed to reveal: not
|
||||
# found" toast that dropped the path entirely, leaving no clue
|
||||
# what was missing).
|
||||
return bad(handler, f"File not found: {target}", 404)
|
||||
|
||||
system = platform.system()
|
||||
if system == "Darwin":
|
||||
@@ -6635,6 +6644,35 @@ def _handle_file_reveal(handler, body):
|
||||
return bad(handler, _sanitize_error(e))
|
||||
|
||||
|
||||
def _handle_file_path(handler, body):
|
||||
"""Resolve a relative workspace-rooted path into an absolute on-disk path.
|
||||
|
||||
The right-click "Copy file path" action (#1764) wants to put the
|
||||
absolute path on the user's clipboard so they can paste it into a
|
||||
terminal, editor, or anywhere else without having to round-trip through
|
||||
the OS file browser. The frontend can't compute the absolute path on
|
||||
its own — `safe_resolve` joins against the session's workspace root
|
||||
which only the server knows. The handler here is a thin lookup; no
|
||||
filesystem mutation, no OS-specific dispatch. We do NOT require the
|
||||
target to exist (unlike `_handle_file_reveal`) — copying the path of a
|
||||
just-deleted file is still useful, and refusing would force callers
|
||||
to special-case 404s for an action that cannot fail destructively.
|
||||
"""
|
||||
try:
|
||||
require(body, "session_id", "path")
|
||||
except ValueError as e:
|
||||
return bad(handler, str(e))
|
||||
try:
|
||||
s = get_session(body["session_id"])
|
||||
except KeyError:
|
||||
return bad(handler, "Session not found", 404)
|
||||
try:
|
||||
target = safe_resolve(Path(s.workspace), body["path"])
|
||||
return j(handler, {"ok": True, "path": str(target)})
|
||||
except (ValueError, PermissionError, OSError) as e:
|
||||
return bad(handler, _sanitize_error(e))
|
||||
|
||||
|
||||
def _handle_workspace_add(handler, body):
|
||||
# Strip surrounding paired quotes BEFORE any further processing — macOS
|
||||
# Finder's "Copy as Pathname" wraps paths in single quotes, and users
|
||||
|
||||
@@ -341,6 +341,11 @@ const LOCALES = {
|
||||
delete_failed: 'Delete failed: ',
|
||||
reveal_in_finder: 'Reveal in File Manager',
|
||||
reveal_failed: 'Failed to reveal: ',
|
||||
copy_file_path: 'Copy file path',
|
||||
path_copied: 'File path copied to clipboard',
|
||||
path_copy_failed: 'Failed to copy path: ',
|
||||
session_rename: 'Rename conversation',
|
||||
session_rename_desc: 'Edit the title of this conversation',
|
||||
new_file_prompt: 'New file name (e.g. notes.md):',
|
||||
project_name_prompt: 'Project name:',
|
||||
created: 'Created ',
|
||||
@@ -1349,6 +1354,11 @@ const LOCALES = {
|
||||
delete_failed: '削除失敗: ',
|
||||
reveal_in_finder: 'ファイルマネージャーで表示',
|
||||
reveal_failed: '表示に失敗しました: ',
|
||||
copy_file_path: 'ファイルパスをコピー',
|
||||
path_copied: 'ファイルパスをクリップボードにコピーしました',
|
||||
path_copy_failed: 'パスのコピーに失敗しました: ',
|
||||
session_rename: '会話の名前を変更',
|
||||
session_rename_desc: 'この会話のタイトルを編集',
|
||||
new_file_prompt: '新しいファイル名 (例: notes.md):',
|
||||
project_name_prompt: 'プロジェクト名:',
|
||||
created: '作成しました: ',
|
||||
@@ -2277,6 +2287,11 @@ const LOCALES = {
|
||||
delete_failed: 'Не удалось удалить: ',
|
||||
reveal_in_finder: 'Показать в файловом менеджере',
|
||||
reveal_failed: 'Не удалось открыть: ',
|
||||
copy_file_path: 'Копировать путь к файлу',
|
||||
path_copied: 'Путь к файлу скопирован в буфер обмена',
|
||||
path_copy_failed: 'Не удалось скопировать путь: ',
|
||||
session_rename: 'Переименовать беседу',
|
||||
session_rename_desc: 'Изменить название этой беседы',
|
||||
new_file_prompt: 'Имя нового файла (например, notes.md):',
|
||||
project_name_prompt: 'Имя проекта:',
|
||||
created: 'Создано ',
|
||||
@@ -3200,6 +3215,11 @@ const LOCALES = {
|
||||
delete_failed: 'Error al eliminar: ',
|
||||
reveal_in_finder: 'Mostrar en el gestor de archivos',
|
||||
reveal_failed: 'Error al mostrar: ',
|
||||
copy_file_path: 'Copiar ruta del archivo',
|
||||
path_copied: 'Ruta del archivo copiada al portapapeles',
|
||||
path_copy_failed: 'Error al copiar la ruta: ',
|
||||
session_rename: 'Renombrar conversación',
|
||||
session_rename_desc: 'Editar el título de esta conversación',
|
||||
new_file_prompt: 'Nombre del archivo nuevo (p. ej. notes.md):',
|
||||
created: 'Creado ',
|
||||
create_failed: 'Error al crear: ',
|
||||
@@ -4130,6 +4150,11 @@ const LOCALES = {
|
||||
delete_failed: 'Löschen fehlgeschlagen: ',
|
||||
reveal_in_finder: 'Im Dateimanager anzeigen',
|
||||
reveal_failed: 'Anzeige fehlgeschlagen: ',
|
||||
copy_file_path: 'Dateipfad kopieren',
|
||||
path_copied: 'Dateipfad in die Zwischenablage kopiert',
|
||||
path_copy_failed: 'Pfad konnte nicht kopiert werden: ',
|
||||
session_rename: 'Unterhaltung umbenennen',
|
||||
session_rename_desc: 'Titel dieser Unterhaltung bearbeiten',
|
||||
new_file_prompt: 'Neuer Dateiname (z.B. notes.md):',
|
||||
project_name_prompt: 'Projektname:',
|
||||
created: 'Erstellt ',
|
||||
@@ -5091,6 +5116,11 @@ const LOCALES = {
|
||||
delete_failed: '\u5220\u9664\u5931\u8d25\uff1a',
|
||||
reveal_in_finder: '\u5728\u6587\u4ef6\u7ba1\u7406\u5668\u4e2d\u663e\u793a',
|
||||
reveal_failed: '\u663e\u793a\u5931\u8d25\uff1a',
|
||||
copy_file_path: '\u590d\u5236\u6587\u4ef6\u8def\u5f84',
|
||||
path_copied: '\u6587\u4ef6\u8def\u5f84\u5df2\u590d\u5236\u5230\u526a\u8d34\u677f',
|
||||
path_copy_failed: '\u590d\u5236\u8def\u5f84\u5931\u8d25\uff1a',
|
||||
session_rename: '\u91cd\u547d\u540d\u5bf9\u8bdd',
|
||||
session_rename_desc: '\u7f16\u8f91\u6b64\u5bf9\u8bdd\u7684\u6807\u9898',
|
||||
new_file_prompt: '\u65b0\u6587\u4ef6\u540d\uff08\u4f8b\u5982 notes.md\uff09\uff1a',
|
||||
project_name_prompt: '\u9879\u76ee\u540d\u79f0\uff1a',
|
||||
created: '\u5df2\u521b\u5efa ',
|
||||
@@ -5981,6 +6011,11 @@ const LOCALES = {
|
||||
delete_failed: '\u522a\u9664\u5931\u6557\uff1a',
|
||||
reveal_in_finder: '\u5728\u6a94\u6848\u7ba1\u7406\u54e1\u4e2d\u986f\u793a',
|
||||
reveal_failed: '\u986f\u793a\u5931\u6557\uff1a',
|
||||
copy_file_path: '\u8907\u88fd\u6a94\u6848\u8def\u5f91',
|
||||
path_copied: '\u6a94\u6848\u8def\u5f91\u5df2\u8907\u88fd\u5230\u526a\u8cbc\u7c3f',
|
||||
path_copy_failed: '\u8907\u88fd\u8def\u5f91\u5931\u6557\uff1a',
|
||||
session_rename: '\u91cd\u65b0\u547d\u540d\u5c0d\u8a71',
|
||||
session_rename_desc: '\u7de8\u8f2f\u6b64\u5c0d\u8a71\u7684\u6a19\u984c',
|
||||
new_file_prompt: '\u65b0\u6587\u4ef6\u540d\uff08\u4f8b\u5982 notes.md\uff09\uff1a',
|
||||
created: '\u5df2\u5275\u5efa ',
|
||||
create_failed: '\u5275\u5efa\u5931\u6557\uff1a',
|
||||
@@ -7009,6 +7044,11 @@ const LOCALES = {
|
||||
delete_failed: 'Falha ao excluir: ',
|
||||
reveal_in_finder: 'Mostrar no gerenciador de arquivos',
|
||||
reveal_failed: 'Falha ao mostrar: ',
|
||||
copy_file_path: 'Copiar caminho do arquivo',
|
||||
path_copied: 'Caminho do arquivo copiado para a área de transferência',
|
||||
path_copy_failed: 'Falha ao copiar caminho: ',
|
||||
session_rename: 'Renomear conversa',
|
||||
session_rename_desc: 'Editar o título desta conversa',
|
||||
new_file_prompt: 'Nome do novo arquivo (ex: notes.md):',
|
||||
project_name_prompt: 'Nome do projeto:',
|
||||
created: 'Criado ',
|
||||
@@ -7903,6 +7943,11 @@ const LOCALES = {
|
||||
delete_failed: '삭제 실패: ',
|
||||
reveal_in_finder: '파일 관리자에서 열기',
|
||||
reveal_failed: '표시 실패: ',
|
||||
copy_file_path: '파일 경로 복사',
|
||||
path_copied: '파일 경로가 클립보드에 복사되었습니다',
|
||||
path_copy_failed: '경로 복사 실패: ',
|
||||
session_rename: '대화 이름 변경',
|
||||
session_rename_desc: '이 대화의 제목 편집',
|
||||
new_file_prompt: 'New file name (e.g. notes.md):',
|
||||
project_name_prompt: 'Project name:',
|
||||
created: '생성됨: ',
|
||||
|
||||
@@ -9,6 +9,7 @@ const ICONS={
|
||||
dup:'<svg width="14" height="14" viewBox="0 0 16 16" fill="none" stroke="currentColor" stroke-width="1.3"><rect x="4.5" y="4.5" width="8.5" height="8.5" rx="1.5"/><path d="M3 11.5V3h8.5"/></svg>',
|
||||
trash:'<svg width="14" height="14" viewBox="0 0 16 16" fill="none" stroke="currentColor" stroke-width="1.3"><path d="M3.5 4.5h9M6.5 4.5V3h3v1.5M4.5 4.5v8.5h7v-8.5"/><line x1="7" y1="7" x2="7" y2="11"/><line x1="9" y1="7" x2="9" y2="11"/></svg>',
|
||||
more:'<svg width="14" height="14" viewBox="0 0 16 16" fill="currentColor" stroke="none"><circle cx="8" cy="3" r="1.25"/><circle cx="8" cy="8" r="1.25"/><circle cx="8" cy="13" r="1.25"/></svg>',
|
||||
edit:'<svg width="14" height="14" viewBox="0 0 16 16" fill="none" stroke="currentColor" stroke-width="1.3" stroke-linecap="round" stroke-linejoin="round"><path d="M11.5 2.5l2 2L5 13H3v-2z"/><path d="M10 4l2 2"/></svg>',
|
||||
};
|
||||
|
||||
// Tracks which session_id is currently being loaded. Used to discard stale
|
||||
@@ -1321,6 +1322,33 @@ function _openSessionActionMenu(session, anchorEl){
|
||||
const isExternalSession = isMessagingSession || isCliSession;
|
||||
const menu=document.createElement('div');
|
||||
menu.className='session-action-menu open';
|
||||
// Rename — first menu item by request (#1764). Double-click rename is
|
||||
// timing-sensitive: the first click frequently registers as "open the
|
||||
// chat" before the second click arrives, so users open the conversation
|
||||
// when they meant to rename it. Putting Rename in the menu eliminates
|
||||
// the timing entirely. Only shown for sessions that support rename
|
||||
// (read-only imported sessions skip it; same gate as startRename's
|
||||
// _isReadOnlySession check).
|
||||
if(!_isReadOnlySession(session)){
|
||||
menu.appendChild(_buildSessionAction(
|
||||
t('session_rename'),
|
||||
t('session_rename_desc'),
|
||||
ICONS.edit,
|
||||
()=>{
|
||||
closeSessionActionMenu();
|
||||
// Find the row for this session and call its attached startRename.
|
||||
// Falls back to a no-op toast if the row isn't currently rendered
|
||||
// (e.g. archived-and-hidden) — extremely rare since the menu only
|
||||
// opens from a visible row's three-dot button.
|
||||
const row=document.querySelector('.session-item[data-sid="'+session.session_id+'"]');
|
||||
if(row && typeof row._startRename === 'function'){
|
||||
row._startRename();
|
||||
} else if(typeof showToast==='function'){
|
||||
showToast(t('session_rename_failed_no_row')||'Could not start rename — row not found.', 3000, 'error');
|
||||
}
|
||||
}
|
||||
));
|
||||
}
|
||||
menu.appendChild(_buildSessionAction(
|
||||
session.pinned?t('session_unpin'):t('session_pin'),
|
||||
session.pinned?t('session_unpin_desc'):t('session_pin_desc'),
|
||||
@@ -2475,6 +2503,13 @@ function renderSessionListFromCache(){
|
||||
title.replaceWith(inp);
|
||||
setTimeout(()=>{inp.focus();inp.select();},10);
|
||||
};
|
||||
// Expose the rename closure on the row so the three-dot action menu
|
||||
// (`_openSessionActionMenu`, defined elsewhere) can trigger it without
|
||||
// needing a separate DOM hunt or a duplicate copy of all this state
|
||||
// (oldTitle / applyTitle / finish / _renamingSid bookkeeping). The
|
||||
// double-click path on this element still calls startRename() directly.
|
||||
el._startRename = startRename;
|
||||
el.dataset.sid = s.session_id;
|
||||
|
||||
// (Project dot is appended above, between title and timestamp, so it
|
||||
// sits outside the truncating title span and stays visible.)
|
||||
|
||||
+41
-1
@@ -6145,9 +6145,49 @@ function _showFileContextMenu(e, item){
|
||||
revealItem.style.cssText='padding:7px 14px;cursor:pointer;font-size:13px;color:var(--text);';
|
||||
revealItem.onmouseenter=()=>revealItem.style.background='var(--hover)';
|
||||
revealItem.onmouseleave=()=>revealItem.style.background='';
|
||||
revealItem.onclick=async()=>{menu.remove();try{await api('/api/file/reveal',{method:'POST',body:JSON.stringify({session_id:S.session.session_id,path:item.path})});}catch(err){showToast(t('reveal_failed')+err.message);}};
|
||||
revealItem.onclick=async()=>{menu.remove();try{await api('/api/file/reveal',{method:'POST',body:JSON.stringify({session_id:S.session.session_id,path:item.path})});}catch(err){showToast(t('reveal_failed')+(err.message||err));}};
|
||||
menu.appendChild(revealItem);
|
||||
|
||||
// Copy file path — resolves the absolute on-disk path on the server (so the
|
||||
// user gets the full /home/.../workspace/foo.py rather than the relative
|
||||
// path the file tree shows) and writes it to the OS clipboard. Useful for
|
||||
// pasting into terminals, editors, or other apps without taking the slower
|
||||
// Reveal-in-Finder round trip.
|
||||
const copyPathItem=document.createElement('div');
|
||||
copyPathItem.textContent=t('copy_file_path');
|
||||
copyPathItem.style.cssText='padding:7px 14px;cursor:pointer;font-size:13px;color:var(--text);';
|
||||
copyPathItem.onmouseenter=()=>copyPathItem.style.background='var(--hover)';
|
||||
copyPathItem.onmouseleave=()=>copyPathItem.style.background='';
|
||||
copyPathItem.onclick=async()=>{
|
||||
menu.remove();
|
||||
try{
|
||||
const r=await api('/api/file/path',{method:'POST',body:JSON.stringify({session_id:S.session.session_id,path:item.path})});
|
||||
const abs=(r&&r.path)||item.path;
|
||||
try{
|
||||
await navigator.clipboard.writeText(abs);
|
||||
showToast(t('path_copied'));
|
||||
}catch(clipErr){
|
||||
// Fallback for browsers where Clipboard API is gated (older Safari,
|
||||
// non-secure contexts). Use the legacy execCommand path against a
|
||||
// hidden textarea — this is the same pattern boot.js uses for the
|
||||
// "Copy" buttons on code blocks.
|
||||
const ta=document.createElement('textarea');
|
||||
ta.value=abs;
|
||||
ta.style.cssText='position:fixed;left:-9999px;top:-9999px;';
|
||||
document.body.appendChild(ta);
|
||||
ta.select();
|
||||
let copied=false;
|
||||
try{copied=document.execCommand('copy');}catch(_){}
|
||||
ta.remove();
|
||||
if(copied) showToast(t('path_copied'));
|
||||
else showToast(t('path_copy_failed')+(clipErr&&clipErr.message?clipErr.message:String(clipErr)));
|
||||
}
|
||||
}catch(err){
|
||||
showToast(t('path_copy_failed')+(err.message||err));
|
||||
}
|
||||
};
|
||||
menu.appendChild(copyPathItem);
|
||||
|
||||
// Divider + Delete
|
||||
const sep=document.createElement('hr');
|
||||
sep.style.cssText='border:none;border-top:1px solid var(--border);margin:4px 0;';
|
||||
|
||||
@@ -21,7 +21,12 @@ def _function_body(src: str, name: str, window: int = 1800) -> str:
|
||||
class TestSidebarCancelAction:
|
||||
def test_running_sidebar_sessions_get_stop_action(self):
|
||||
"""Running sessions need a context-menu cancel action even when not active pane."""
|
||||
body = _function_body(SESSIONS_JS, "_openSessionActionMenu", 3200)
|
||||
# Window bumped from 3200 → 4400 in #1764 to accommodate the new
|
||||
# Rename action item that lands at the top of _openSessionActionMenu.
|
||||
# The `session.active_stream_id` / cancelSessionStream / delete checks
|
||||
# are positional further down in the function, so growing the prefix
|
||||
# required growing this read window.
|
||||
body = _function_body(SESSIONS_JS, "_openSessionActionMenu", 4400)
|
||||
assert "session.active_stream_id" in body, (
|
||||
"sidebar action menu must detect per-session active_stream_id instead of S.activeStreamId"
|
||||
)
|
||||
@@ -67,7 +72,8 @@ class TestSidebarCancelAction:
|
||||
|
||||
def test_cli_sessions_hide_duplicate_and_delete_in_action_menu(self):
|
||||
"""Session action menu should hide duplicate/delete for CLI-origin sessions."""
|
||||
body = _function_body(SESSIONS_JS, "_openSessionActionMenu", 3600)
|
||||
# Window bumped 3600 → 4800 in #1764 (Rename action prepended).
|
||||
body = _function_body(SESSIONS_JS, "_openSessionActionMenu", 4800)
|
||||
assert "const isCliSession = _isCliSession(session);" in body
|
||||
assert "const isExternalSession = isMessagingSession || isCliSession;" in body
|
||||
assert "if(!isExternalSession)" in body
|
||||
|
||||
@@ -0,0 +1,343 @@
|
||||
"""Regression tests for issue #1764 — three context-menu essentials.
|
||||
|
||||
The issue asked for a much larger surface, but per Nathan's curation we
|
||||
ship only three high-leverage pieces in this PR:
|
||||
|
||||
1. **Copy file path** in the workspace tree right-click menu — resolves
|
||||
the absolute on-disk path on the server (so the user gets the full
|
||||
path, not the relative tree-rooted one) and writes it to the
|
||||
clipboard.
|
||||
|
||||
2. **Rename** in the session three-dot menu — Cygnus reported double-click
|
||||
rename being timing-sensitive (first click opens the chat before the
|
||||
second click arrives). Putting Rename in the menu eliminates the
|
||||
timing entirely.
|
||||
|
||||
3. **Reveal-failed toast includes the resolved path** — the existing
|
||||
handler returned bare "File not found" (404) and the frontend toast
|
||||
surfaced only `err.message`, dropping the path entirely. This makes
|
||||
it impossible for users to tell *which* file the system expected
|
||||
(e.g. a stale session row pointing at a deleted file). Now the
|
||||
server includes the resolved server-side path in the message.
|
||||
|
||||
These tests pin the source-level wiring — they do not exercise the live
|
||||
HTTP endpoints (those are covered by integration tests where they exist
|
||||
in the wider suite).
|
||||
"""
|
||||
from pathlib import Path
|
||||
import re
|
||||
|
||||
|
||||
ROOT = Path(__file__).resolve().parent.parent
|
||||
ROUTES = ROOT / "api" / "routes.py"
|
||||
UI = ROOT / "static" / "ui.js"
|
||||
SESSIONS = ROOT / "static" / "sessions.js"
|
||||
I18N = ROOT / "static" / "i18n.js"
|
||||
|
||||
|
||||
# ════════════════════════════════════════════════════════════════════
|
||||
# Item A — Copy file path in workspace tree right-click menu
|
||||
# ════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
class TestCopyFilePathMenuItem:
|
||||
def test_menu_item_present(self):
|
||||
"""The workspace file context menu must include a Copy file path
|
||||
action that calls the new /api/file/path endpoint and writes the
|
||||
result to the clipboard.
|
||||
"""
|
||||
src = UI.read_text(encoding="utf-8")
|
||||
# Item label is sourced via t('copy_file_path') — pin the call.
|
||||
assert "t('copy_file_path')" in src
|
||||
# Endpoint POSTed to.
|
||||
assert "/api/file/path" in src
|
||||
# Clipboard write.
|
||||
assert "navigator.clipboard.writeText(abs)" in src
|
||||
|
||||
def test_menu_item_has_clipboard_fallback(self):
|
||||
"""Some browsers gate the modern Clipboard API (older Safari, any
|
||||
non-secure context). The action must fall back to the legacy
|
||||
execCommand pattern so users on those browsers still get a copy.
|
||||
"""
|
||||
src = UI.read_text(encoding="utf-8")
|
||||
assert "document.execCommand('copy')" in src
|
||||
# Hidden textarea pattern — uses a fixed-position offscreen element
|
||||
# so the page doesn't visibly scroll when select() runs.
|
||||
assert "position:fixed;left:-9999px" in src
|
||||
|
||||
def test_menu_item_uses_path_copied_translation(self):
|
||||
"""The success toast keys must be wired to translatable strings,
|
||||
not hardcoded English.
|
||||
"""
|
||||
src = UI.read_text(encoding="utf-8")
|
||||
assert "t('path_copied')" in src
|
||||
assert "t('path_copy_failed')" in src
|
||||
|
||||
def test_endpoint_handler_present(self):
|
||||
"""Server-side endpoint must exist and route through the dispatcher."""
|
||||
src = ROUTES.read_text(encoding="utf-8")
|
||||
assert 'parsed.path == "/api/file/path"' in src
|
||||
assert "def _handle_file_path(handler, body):" in src
|
||||
# Must use safe_resolve to prevent path traversal.
|
||||
# Find the handler body and check.
|
||||
m = re.search(
|
||||
r"def _handle_file_path\(handler, body\):\s*(?:\"\"\".*?\"\"\")?\s*(.*?)(?=\ndef )",
|
||||
src,
|
||||
re.DOTALL,
|
||||
)
|
||||
assert m, "_handle_file_path body not found"
|
||||
body = m.group(1)
|
||||
assert "safe_resolve(Path(s.workspace)" in body
|
||||
assert "session_id" in body # require() check
|
||||
# Returns the absolute path as a string.
|
||||
assert 'j(handler, {"ok": True, "path": str(target)})' in body
|
||||
|
||||
def test_endpoint_handler_does_not_require_existence(self):
|
||||
"""Copy-path on a recently-deleted file is still useful (paste into
|
||||
terminal to investigate). The handler must not 404 on missing files.
|
||||
"""
|
||||
src = ROUTES.read_text(encoding="utf-8")
|
||||
m = re.search(
|
||||
r"def _handle_file_path\(handler, body\):.*?(?=\ndef )",
|
||||
src,
|
||||
re.DOTALL,
|
||||
)
|
||||
assert m
|
||||
body = m.group(0)
|
||||
# No exists() check — that's specifically what we want NOT to be
|
||||
# there. Distinguishing from _handle_file_reveal which does check.
|
||||
assert "exists()" not in body, (
|
||||
"Copy-path must not gate on exists() — copying a stale path is "
|
||||
"still useful for debugging deleted files."
|
||||
)
|
||||
|
||||
|
||||
# ════════════════════════════════════════════════════════════════════
|
||||
# Item B — Rename in session three-dot menu
|
||||
# ════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
class TestSessionRenameMenuItem:
|
||||
def test_rename_action_in_menu(self):
|
||||
"""The session three-dot menu (`_openSessionActionMenu`) must
|
||||
include Rename as the first item, gated on _isReadOnlySession.
|
||||
"""
|
||||
src = SESSIONS.read_text(encoding="utf-8")
|
||||
# Rename block must be inside _openSessionActionMenu.
|
||||
# Pin the structural anchor.
|
||||
assert "if(!_isReadOnlySession(session)){" in src
|
||||
assert "t('session_rename')" in src
|
||||
assert "t('session_rename_desc')" in src
|
||||
|
||||
def test_rename_dispatches_to_row_closure(self):
|
||||
"""The menu's rename action must trigger the existing startRename
|
||||
closure attached to the row element — no duplicated state, no
|
||||
separate API call out of band with the double-click path.
|
||||
"""
|
||||
src = SESSIONS.read_text(encoding="utf-8")
|
||||
# Row-attached closure invocation.
|
||||
assert "row._startRename" in src
|
||||
# Row lookup by data-sid.
|
||||
assert ".session-item[data-sid=" in src
|
||||
|
||||
def test_row_exposes_start_rename(self):
|
||||
"""The session row builder must attach `_startRename` to the row
|
||||
element so the menu (defined in a different function) can find it
|
||||
without duplicating the closure's state (oldTitle, applyTitle, the
|
||||
_renamingSid bookkeeping, etc.).
|
||||
"""
|
||||
src = SESSIONS.read_text(encoding="utf-8")
|
||||
assert "el._startRename = startRename" in src
|
||||
assert "el.dataset.sid = s.session_id" in src
|
||||
|
||||
def test_rename_appears_before_pin(self):
|
||||
"""Cygnus's specific ask: Rename should be at the top of the menu,
|
||||
not buried under Pin / Move / Archive / etc. Pin that ordering.
|
||||
"""
|
||||
src = SESSIONS.read_text(encoding="utf-8")
|
||||
rename_idx = src.find("t('session_rename')")
|
||||
pin_idx = src.find("t('session_pin')")
|
||||
assert rename_idx > 0 and pin_idx > 0
|
||||
assert rename_idx < pin_idx, (
|
||||
"Rename must appear before Pin in _openSessionActionMenu."
|
||||
)
|
||||
|
||||
def test_rename_translation_keys_present(self):
|
||||
"""English translation keys must exist for the new menu item."""
|
||||
src = I18N.read_text(encoding="utf-8")
|
||||
assert "session_rename: 'Rename conversation'" in src
|
||||
assert "session_rename_desc: 'Edit the title of this conversation'" in src
|
||||
|
||||
|
||||
# ════════════════════════════════════════════════════════════════════
|
||||
# Item C — reveal-failed toast includes the resolved path
|
||||
# ════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
class TestRevealFailedTostIncludesPath:
|
||||
def test_handler_includes_target_in_404_message(self):
|
||||
"""When `target.exists()` returns false, the 404 response body must
|
||||
include the resolved server-side path so the frontend toast can
|
||||
show users *which* file the system expected. Previously it was
|
||||
just "File not found" with no path — useless for diagnosing stale
|
||||
session rows.
|
||||
"""
|
||||
src = ROUTES.read_text(encoding="utf-8")
|
||||
# Find _handle_file_reveal body.
|
||||
m = re.search(
|
||||
r"def _handle_file_reveal\(handler, body\):.*?(?=\ndef )",
|
||||
src,
|
||||
re.DOTALL,
|
||||
)
|
||||
assert m, "_handle_file_reveal not found"
|
||||
body = m.group(0)
|
||||
# The bad() call for not-exists must include the path.
|
||||
assert 'f"File not found: {target}"' in body, (
|
||||
"Reveal handler must include the resolved path in the 404 message."
|
||||
)
|
||||
# And NOT the bare unhelpful message.
|
||||
# (We allow the substring 'File not found' because the new f-string
|
||||
# contains it as a prefix; pin via the f-string presence above.)
|
||||
assert 'bad(handler, "File not found", 404)' not in body, (
|
||||
"Old bare 'File not found' message must be removed."
|
||||
)
|
||||
|
||||
def test_existing_translation_key_unchanged(self):
|
||||
"""The frontend toast prefix `reveal_failed: 'Failed to reveal: '`
|
||||
is unchanged — the additional path comes from the server-side
|
||||
message, so the prefix + message concat still reads well.
|
||||
"""
|
||||
src = I18N.read_text(encoding="utf-8")
|
||||
assert "reveal_failed: 'Failed to reveal: '" in src
|
||||
|
||||
def test_reveal_call_site_uses_message_or_err(self):
|
||||
"""The frontend reveal handler call site must guard against err
|
||||
being a non-Error object (e.g. a network-layer reject without a
|
||||
.message). Previously `err.message` alone could produce
|
||||
"Failed to reveal: undefined" — we use `(err.message||err)`.
|
||||
"""
|
||||
src = UI.read_text(encoding="utf-8")
|
||||
# Match both possible forms (with or without parens).
|
||||
assert (
|
||||
"(err.message||err)" in src or "(err.message || err)" in src
|
||||
), "Reveal-failed toast must guard against err with no .message"
|
||||
|
||||
|
||||
|
||||
# ════════════════════════════════════════════════════════════════════
|
||||
# Behaviour tests — exercise the live HTTP endpoints against the
|
||||
# module-scoped test server (started by conftest.py at port 8788).
|
||||
# ════════════════════════════════════════════════════════════════════
|
||||
|
||||
|
||||
import json
|
||||
import pathlib
|
||||
import sys
|
||||
import urllib.error
|
||||
import urllib.request
|
||||
|
||||
sys.path.insert(0, str(pathlib.Path(__file__).parent))
|
||||
|
||||
from conftest import TEST_BASE # noqa: E402
|
||||
|
||||
|
||||
def _post(path, body=None, headers=None):
|
||||
data = json.dumps(body or {}).encode()
|
||||
h = {"Content-Type": "application/json"}
|
||||
if headers:
|
||||
h.update(headers)
|
||||
req = urllib.request.Request(TEST_BASE + path, data=data, headers=h)
|
||||
try:
|
||||
with urllib.request.urlopen(req, timeout=10) as r:
|
||||
return json.loads(r.read()), r.status
|
||||
except urllib.error.HTTPError as e:
|
||||
return json.loads(e.read()), e.code
|
||||
|
||||
|
||||
class TestFilePathEndpointBehaviour:
|
||||
"""End-to-end exercise of the new /api/file/path endpoint against the
|
||||
live test server."""
|
||||
|
||||
def _new_session(self):
|
||||
body, status = _post("/api/session/new", {})
|
||||
assert status == 200, body
|
||||
return body["session"]["session_id"]
|
||||
|
||||
def test_returns_absolute_path_for_relative_input(self):
|
||||
"""The endpoint must resolve a relative workspace-rooted path into
|
||||
the absolute on-disk path. This is the whole point — the frontend
|
||||
can't compute it because only the server knows the workspace root.
|
||||
"""
|
||||
sid = self._new_session()
|
||||
body, status = _post("/api/file/path", {"session_id": sid, "path": "."})
|
||||
assert status == 200, body
|
||||
assert body.get("ok") is True
|
||||
# Path should be absolute (starts with /).
|
||||
assert body.get("path", "").startswith("/"), body
|
||||
|
||||
def test_does_not_404_on_missing_file(self):
|
||||
"""Copy-path on a stale-but-recently-deleted file must still
|
||||
succeed — that's specifically what makes the action useful for
|
||||
debugging."""
|
||||
sid = self._new_session()
|
||||
body, status = _post(
|
||||
"/api/file/path",
|
||||
{"session_id": sid, "path": "definitely-does-not-exist-xyz123.tmp"},
|
||||
)
|
||||
assert status == 200, body
|
||||
assert body.get("ok") is True
|
||||
# Even though the file doesn't exist, we get back a resolved path.
|
||||
assert "definitely-does-not-exist-xyz123.tmp" in body.get("path", "")
|
||||
|
||||
def test_rejects_path_traversal(self):
|
||||
"""The endpoint must use safe_resolve, which rejects paths that
|
||||
escape the workspace root."""
|
||||
sid = self._new_session()
|
||||
body, status = _post(
|
||||
"/api/file/path",
|
||||
{"session_id": sid, "path": "../../../../../../etc/passwd"},
|
||||
)
|
||||
assert status == 400, body # safe_resolve raises ValueError → bad()
|
||||
# Error message must NOT include the attempted traversal target's
|
||||
# contents, just a generic safe-resolve message.
|
||||
assert "passwd" not in body.get("error", "").lower() or "outside" in body.get("error", "").lower()
|
||||
|
||||
def test_missing_session_id_returns_400(self):
|
||||
body, status = _post("/api/file/path", {"path": "foo.txt"})
|
||||
assert status == 400, body
|
||||
assert "session_id" in body.get("error", "")
|
||||
|
||||
def test_unknown_session_returns_404(self):
|
||||
body, status = _post(
|
||||
"/api/file/path", {"session_id": "fake-session-xyz", "path": "."}
|
||||
)
|
||||
assert status == 404, body
|
||||
assert "session" in body.get("error", "").lower()
|
||||
|
||||
|
||||
class TestRevealHandlerErrorIncludesPath:
|
||||
"""End-to-end check that the reveal endpoint's 404 includes the path."""
|
||||
|
||||
def _new_session(self):
|
||||
body, status = _post("/api/session/new", {})
|
||||
assert status == 200, body
|
||||
return body["session"]["session_id"]
|
||||
|
||||
def test_404_message_contains_resolved_path(self):
|
||||
"""Reveal of a missing file must surface the resolved server-side
|
||||
path in the error, so the frontend toast can show users *which*
|
||||
file was missing — useful when a stale row points at a deleted
|
||||
file (#1764)."""
|
||||
sid = self._new_session()
|
||||
body, status = _post(
|
||||
"/api/file/reveal",
|
||||
{"session_id": sid, "path": "missing-xyz-1764.txt"},
|
||||
)
|
||||
assert status == 404, body
|
||||
err = body.get("error", "")
|
||||
# Must include the filename in the resolved path.
|
||||
assert "missing-xyz-1764.txt" in err, (
|
||||
f"Reveal 404 message must include the resolved path, got: {err!r}"
|
||||
)
|
||||
# Must keep the human-readable prefix.
|
||||
assert "File not found" in err
|
||||
Reference in new issue
Block a user