fix(terminal): copy selected web terminal text#382
Conversation
📝 WalkthroughОбзорДобавлена поддержка системного copy shortcut (Ctrl/Cmd+C) в терминальный интерфейс. Функция обнаруживает нажатие copy shortcut при наличии выделенного текста и передаёт копирование нативному браузеру. Контроллер регистрирует пользовательский обработчик клавиатурных событий через новую точку расширения фасада терминала. Добавлены комплексные тесты для проверки логики и интеграции. ИзмененияПоддержка copy shortcut в терминале
🎯 3 (Средняя) | ⏱️ ~20 минутImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/terminal/src/web/terminal-copy-interaction.ts (1)
279-286:⚠️ Potential issue | 🟡 MinorВ
dispose()контроллера копирования нужно отключатьcustomKeyEventHandler(или явно полагаться наterminal.dispose)
packages/terminal/src/web/terminal-copy-interaction.ts(dispose(), ~279-286) снимает только DOM listeners, но не переустанавливаетterminal.attachCustomKeyEventHandler, поэтому на живом экземпляреterminalстарый handler продолжит работать. В текущем runtime обработчик должен очищаться вместе сcleanupTerminalResources(...); args.terminal.dispose()— но еслиcopyInteractionDisposable.dispose()будет вызван без полногоterminal.dispose, возможны ложные блокировки клавиш.
Решение: вdispose()переустановить pass-through handler (() => true) или зафиксировать это предположение док-кой/тестом.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/terminal/src/web/terminal-copy-interaction.ts` around lines 279 - 286, dispose() currently removes DOM listeners but leaves the terminal's custom key handler installed, which can block keys if the terminal itself isn't disposed; in dispose() of terminal-copy-interaction replace or clear the custom key handler by calling args.terminal.attachCustomKeyEventHandler with a pass-through handler (e.g., () => true) so the terminal no longer intercepts key events when copyInteractionDisposable.dispose() is invoked without full terminal.dispose(); reference dispose(), args.terminal.attachCustomKeyEventHandler, copyInteractionDisposable.dispose(), cleanupTerminalResources and args.terminal.dispose() to locate where to add this reset alongside the existing selectionDrag.dispose(), selectionContext.clear() and args.host.removeEventListener calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/terminal/src/web/terminal-copy-interaction.ts`:
- Around line 104-107: Keep the explicit selection length check in
shouldLetBrowserHandleTerminalCopyShortcut (don't remove
terminal.getSelection().length > 0) because
writeTerminalSelectionToClipboardData relies on the possibility that
terminal.hasSelection() may be true with an empty getSelection(); update the
function accordingly. Also update dispose() in terminal-copy-interaction.ts to
explicitly detach the custom key handler registered via
attachCustomKeyEventHandler (store and call the handler-disposer returned by
attachCustomKeyEventHandler) so copyInteractionDisposable.dispose() removes the
key handler even if terminal.dispose() is not called; if removal API is not
available, add a clear comment documenting the required cleanup contract
(cleanupTerminalResources must call terminal.dispose() after
copyInteractionDisposable.dispose()) and ensure callers respect that order.
In `@packages/terminal/tests/web/terminal-copy-interaction.test.ts`:
- Around line 67-86: Add a property-based test using fast-check that encodes the
invariant "selected(t) ∧ copyShortcut(e) ⟹ browserCopy(e,t)" for
shouldLetBrowserHandleTerminalCopyShortcut: generate key (c/C), modifiers
(altKey, ctrlKey, metaKey) and a boolean hasSelection, build event with
keyboardEvent(..., type: "keydown") and terminal via
terminalWithSelection("any", hasSelection ? "text" : ""), compute isCopyShortcut
= !mods.altKey && (mods.ctrlKey || mods.metaKey), set expected = isCopyShortcut
&& hasSelection, and assert shouldLetBrowserHandleTerminalCopyShortcut(event,
terminal) === expected; import fc from fast-check and place this property in the
existing terminal-copy-interaction.test.ts near the current unit tests to
replace or supplement the example assertions.
---
Outside diff comments:
In `@packages/terminal/src/web/terminal-copy-interaction.ts`:
- Around line 279-286: dispose() currently removes DOM listeners but leaves the
terminal's custom key handler installed, which can block keys if the terminal
itself isn't disposed; in dispose() of terminal-copy-interaction replace or
clear the custom key handler by calling
args.terminal.attachCustomKeyEventHandler with a pass-through handler (e.g., ()
=> true) so the terminal no longer intercepts key events when
copyInteractionDisposable.dispose() is invoked without full terminal.dispose();
reference dispose(), args.terminal.attachCustomKeyEventHandler,
copyInteractionDisposable.dispose(), cleanupTerminalResources and
args.terminal.dispose() to locate where to add this reset alongside the existing
selectionDrag.dispose(), selectionContext.clear() and
args.host.removeEventListener calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56480cba-80fb-4253-953a-acb8b5dbfecc
⛔ Files ignored due to path filters (2)
docs/pr-screenshots/issue-353/terminal-copy-pass.pngis excluded by!**/*.pngdocs/pr-screenshots/issue-353/terminal-copy-selected.pngis excluded by!**/*.png
📒 Files selected for processing (2)
packages/terminal/src/web/terminal-copy-interaction.tspackages/terminal/tests/web/terminal-copy-interaction.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: Lint
- GitHub Check: Snapshot
- GitHub Check: E2E (Browser command)
- GitHub Check: Test
- GitHub Check: Final build (windows-latest)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Functional Core, Imperative Shell (FCIS) pattern: CORE layer contains only pure functions with immutable data and mathematical operations; SHELL layer isolates all effects (IO, network, database). Strict dependency direction: SHELL → CORE (never reverse).
Never useany,unknown,eslint-disable,ts-ignore, orastype assertions (except in rigorously justified cases with documentation). Always use exhaustive union type analysis through.exhaustive()pattern matching.
All external dependencies must be wrapped through typed interfaces and injected via Effect-TS Layer pattern. Never call external services directly from CORE functions.
Use monadic composition with Effect-TS for all effects:Effect<Success, Error, Requirements>. Compose effects throughpipe()andEffect.flatMap(). Implement dependency injection via Layer pattern. Handle errors without try/catch blocks.
All functions must be pure in the CORE layer: no side effects (logging, console output, IO operations, mutations). Separate all side effects into the SHELL layer.
Use exhaustive pattern matching with Effect.Match instead of switch statements. Example:Match.value(item).pipe(Match.when(...), Match.exhaustive).
Document all functions with comprehensive TSDoc including:@pure(true/false),@effect(required services),@invariant(mathematical invariants),@precondition,@postcondition,@complexity(time and space),@throwsNever (errors must be typed in Effect).
Use functional comment markers for code clarity: CHANGE (brief description), WHY (mathematical/architectural justification), QUOTE(ТЗ) (requirement citation), REF (RTM or message ID), SOURCE (external source with quote), FORMAT THEOREM (∀x ∈ Domain: P(x) → Q(f(x))), PURITY (CORE|SHELL), EFFECT (Effect type signature), INVARIANT (mathematical invariant), COMPLEXITY (time/space).
Define all external service dependencies as Context.Tag classes with fully typed methods returning Effect types. Example: `class Da...
Files:
packages/terminal/src/web/terminal-copy-interaction.tspackages/terminal/tests/web/terminal-copy-interaction.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Forbidden constructs in CORE code:any,eslint-disable,ts-ignore,async/await, raw Promise chains (then/catch),Promise.all,try/catchfor logic control,console.*, switch statements (use Match with .exhaustive() instead)
All functions must use Effect-TS for composing effects:Effect<Success, Error, Requirements>. No direct async/await, Promise chains, or try/catch in product logic.
Functional comments must include: CHANGE, WHY, QUOTE(ТЗ) or n/a, REF, SOURCE or n/a, FORMAT THEOREM, PURITY (CORE|SHELL), EFFECT signature for SHELL functions, INVARIANT, and COMPLEXITY.
All data mutations must use immutable patterns (ReadonlyArray, readonly properties, Object.freeze); mutation in SHELL only when absolutely necessary and documented.
Files:
packages/terminal/src/web/terminal-copy-interaction.tspackages/terminal/tests/web/terminal-copy-interaction.test.ts
**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input
Files:
packages/terminal/src/web/terminal-copy-interaction.tspackages/terminal/tests/web/terminal-copy-interaction.test.ts
**/*.{py,js,ts,jsx,tsx,go,java,rb,php,sh,bash,c,cpp}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce path traversal or writes outside intended project/container state directories
Files:
packages/terminal/src/web/terminal-copy-interaction.tspackages/terminal/tests/web/terminal-copy-interaction.test.ts
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,sh,bash,yml,yaml,json,env*,toml,cfg,config,dockerfile,dockerignore}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files expose credentials, tokens, private-keys, or PII in source, generated config, logs, or CI output
Files:
packages/terminal/src/web/terminal-copy-interaction.tspackages/terminal/tests/web/terminal-copy-interaction.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/terminal/src/web/terminal-copy-interaction.tspackages/terminal/tests/web/terminal-copy-interaction.test.ts
**
⚙️ CodeRabbit configuration file
**: РОЛЬ: Математик-программист, специализирующийся на формально верифицируемой функциональной архитектуре.ЦЕЛЬ: Создавать математически доказуемые решения через функциональную парадигму с полным разделением чистых вычислений и контролируемых эффектов.
МОДЕЛЬ РАССУЖДЕНИЯ:
- Не выдавать “личные мнения”. Формировать вывод как результат симуляции профессионального обсуждения релевантных ролей
(архитектор Effect/FP, ревьюер типов, страж CORE↔SHELL, тест-инженер).- Если запрос сформулирован как “что думаешь”, отвечать в терминах аргументов ролей и выбирать решение
по критериям инвариантов, типовой безопасности и тестируемости (если пользователь явно просит выбор — выбрать и обосновать).ПРАВИЛО ПРОЦЕССА (НЕ ФОРМАТ ОТВЕТА):
В начале работы (внутренне) формулировать Deep Research вопрос:
"I am looking for code that does , is there existing code that can do this?"
Далее:
- если доступен проект/код — сперва искать и переиспользовать существующие паттерны (минимальный корректный diff),
- если проект недоступен — опираться на предоставленный контекст и явно фиксировать допущения,
- код писать только после формального понимания задачи (типы/инварианты → архитектура → код → тесты),
- источники указывать только если реально использован внешний материал; иначе
SOURCE: n/a.ИНСТРУМЕНТАЛЬНОЕ ПОВЕДЕНИЕ (ОБЯЗАТЕЛЬНО, НЕ ФОРМАТ ОТВЕТА):
- Агент всегда использует доступные инструменты среды (терминал, поиск по проекту, запуск тестов/скриптов, анализ сборки, web-ресёрч при необходимости)
для ресёрча, проверки гипотез и выполнения действий. Приоритет: проверяемость, воспроизводимость, минимальный риск.- Агент не предлагает “гайд” как замену действия. Если действие возможно выполнить инструментами — агент выполняет его сам,
затем сообщает, что было сделано и как повторить.- Любые инструкции (команды/процедуры) агент даёт только после собственной проверки на доступной среде.
Если проверить невозможно — явно фиксирует ограничение и перечисляе...
Files:
packages/terminal/src/web/terminal-copy-interaction.tspackages/terminal/tests/web/terminal-copy-interaction.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Implement property-based testing using fast-check for mathematical properties and invariants. Example:fc.property(fc.array(messageArbitrary), (messages) => isChronologicallySorted(sortMessagesByTimestamp(messages))).
Mock external dependencies in unit tests using Effect's testing utilities. Run tests without Effect runtime for speed. Example:Effect.provide(MockService), Effect.runPromise.
Files:
packages/terminal/tests/web/terminal-copy-interaction.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Property-based tests (fast-check) must verify mathematical invariants; unit tests must use Effect test utilities without async/await.
Files:
packages/terminal/tests/web/terminal-copy-interaction.test.ts
🔇 Additional comments (5)
packages/terminal/src/web/terminal-copy-interaction.ts (3)
20-26: LGTM!Типы корректно определены как readonly, опциональность
attachCustomKeyEventHandlerпозволяет graceful degradation для терминалов без поддержки custom key handlers.Also applies to: 28-35
74-78: LGTM!
80-103: LGTM!Документация TSDoc соответствует coding guidelines, функциональные комментарии полные, логика инверсии в
onTerminalKeyEventкорректна для xterm API.Also applies to: 206-216
packages/terminal/tests/web/terminal-copy-interaction.test.ts (2)
32-50: LGTM!Хелперы
keyboardEventиrequireKeyHandlerкорректно типизированы и упрощают создание тестовых данных.
88-108: LGTM!Интеграционный тест корректно проверяет регистрацию handler'а и возвращаемые значения для разных сценариев: keydown copy → false, keyup → true, plain key → true.
| export const shouldLetBrowserHandleTerminalCopyShortcut = ( | ||
| event: TerminalCopyKeyboardEvent, | ||
| terminal: TerminalSelectionTarget | ||
| ): boolean => isKeyboardCopyShortcut(event) && terminal.hasSelection() && terminal.getSelection().length > 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Проверка: есть ли случаи, где hasSelection() и getSelection() рассогласованы
rg -n "hasSelection|getSelection" --type=ts -C2Repository: ProverCoderAI/docker-git
Length of output: 9008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Прочитать ключевые участки файла с экспортом/логикой и dispose()
sed -n '1,220p' packages/terminal/src/web/terminal-copy-interaction.ts
echo "-----"
sed -n '220,360p' packages/terminal/src/web/terminal-copy-interaction.ts
echo "-----"
# 2) Быстрый поиск по символам/контексту обработчика ключей
rg -n "attachCustomKeyEventHandler|dispose\\(|unregister|keyHandler|onTerminalKeyEvent" packages/terminal/src/web/terminal-copy-interaction.tsRepository: ProverCoderAI/docker-git
Length of output: 11451
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Прочитать ключевые участки файла с экспортом/логикой и dispose()
sed -n '1,240p' packages/terminal/src/web/terminal-copy-interaction.ts
echo "-----"
sed -n '240,360p' packages/terminal/src/web/terminal-copy-interaction.ts
echo "-----"
# 2) Быстрый поиск по символам/контексту обработчика ключей
rg -n "attachCustomKeyEventHandler|dispose\\(|unregister|keyHandler|onTerminalKeyEvent" packages/terminal/src/web/terminal-copy-interaction.tsRepository: ProverCoderAI/docker-git
Length of output: 11461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' packages/terminal/src/web/terminal-copy-interaction.ts
echo "-----"
sed -n '240,420p' packages/terminal/src/web/terminal-copy-interaction.ts
echo "-----"
rg -n "attachCustomKeyEventHandler|dispose\\(|unregister|keyHandler|onTerminalKeyEvent|customKey" packages/terminal/src/web/terminal-copy-interaction.tsRepository: ProverCoderAI/docker-git
Length of output: 11461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "attachCustomKeyEventHandler" -S packages/terminal | head -n 50
rg -n "attachTerminalCopyInteraction" -S packages/terminal | head -n 50
rg -n "shouldLetBrowserHandleTerminalCopyShortcut" -S packages/terminal | head -n 50
# Посмотреть, как устроена реализация хост/terminal facade в тестах
fd "terminal-copy-interaction" packages/terminal/tests -t f | head -n 20
fd "copy-interaction" packages/terminal/tests -t f | head -n 20Repository: ProverCoderAI/docker-git
Length of output: 4178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) прочитать fixture с терминалами для hasSelection/getSelection
sed -n '1,220p' packages/terminal/tests/web/fixtures/terminal-copy-interaction.ts
echo "-----"
sed -n '220,420p' packages/terminal/tests/web/fixtures/terminal-copy-interaction.ts
echo "====="
# 2) прочитать тесты shouldLetBrowserHandleTerminalCopyShortcut (важно для проверки кейсов пустой выделенной строки)
sed -n '1,140p' packages/terminal/tests/web/terminal-copy-interaction.test.ts
echo "-----"
sed -n '140,260p' packages/terminal/tests/web/terminal-copy-interaction.test.ts
echo "====="
# 3) прочитать terminal-panel-runtime.ts участок где хранится copyInteractionDisposable и вызывается dispose
sed -n '1,220p' packages/terminal/src/web/terminal-panel-runtime.ts
echo "-----"
rg -n "copyInteractionDisposable|dispose\\(" packages/terminal/src/web/terminal-panel-runtime.tsRepository: ProverCoderAI/docker-git
Length of output: 28278
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) найти, где объявлен attachCustomKeyEventHandler в реализации terminal facade
rg -n "attachCustomKeyEventHandler" packages/terminal/src -S
# 2) найти реализацию createTerminalRuntime и cleanupTerminalResources (чтобы понять, что происходит с handlers при dispose терминала)
rg -n "function createTerminalRuntime|createTerminalRuntime\\(" packages/terminal/src/web -S
rg -n "cleanupTerminalResources\\(" packages/terminal/src/web -S
# 3) посмотреть файл terminal-panel-runtime-core.ts для cleanupTerminalResources и терминал dispose
sed -n '1,260p' packages/terminal/src/web/terminal-panel-runtime-core.tsRepository: ProverCoderAI/docker-git
Length of output: 8326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) найти cleanupTerminalResources и где там dispose terminal
rg -n "cleanupTerminalResources|terminal\\.dispose\\(|detachCustomKeyEventHandler|detach.*KeyEvent" packages/terminal/src/web -S
# 2) прочитать файл cleanupTerminalResources
sed -n '1,260p' packages/terminal/src/web/terminal-panel-cleanup-runtime.ts 2>/dev/null || true
sed -n '1,260p' packages/terminal/src/web/terminal-panel-cleanup-runtime.js 2>/dev/null || true
ls packages/terminal/src/web | rg "cleanup.*runtime"
# 3) поиск по detachCustomKeyEventHandler в repo
rg -n "detachCustomKeyEventHandler" -S packages/terminal/src packages/terminal/testsRepository: ProverCoderAI/docker-git
Length of output: 2126
Подправить оба замечания: длина selection тут не лишняя, а про key-handler важно контекстно
- Проверка
terminal.getSelection().length > 0вshouldLetBrowserHandleTerminalCopyShortcutне выглядит избыточной: в этом же модулеwriteTerminalSelectionToClipboardDataпослеterminal.hasSelection()дополнительно делаетselection.length === 0и отклоняет кейс рассогласования, значит контрактhasSelection() ⇒ non-empty getSelection()не гарантируется типом/инвариантом. dispose()вterminal-copy-interaction.tsдействительно не “отсоединяет”attachCustomKeyEventHandler, но при cleanup терминала вcleanupTerminalResourcesвызываетсяargs.terminal.dispose()(послеcopyInteractionDisposable.dispose()), поэтому key-handler cleanup фактически делегируетсяterminal.dispose(). Лучше документировать это ожидание/контрактом вызовов либо (если возможно) добавить явную detachment-операцию, еслиcopyInteractionDisposable.dispose()может вызываться без последующегоterminal.dispose().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/terminal/src/web/terminal-copy-interaction.ts` around lines 104 -
107, Keep the explicit selection length check in
shouldLetBrowserHandleTerminalCopyShortcut (don't remove
terminal.getSelection().length > 0) because
writeTerminalSelectionToClipboardData relies on the possibility that
terminal.hasSelection() may be true with an empty getSelection(); update the
function accordingly. Also update dispose() in terminal-copy-interaction.ts to
explicitly detach the custom key handler registered via
attachCustomKeyEventHandler (store and call the handler-disposer returned by
attachCustomKeyEventHandler) so copyInteractionDisposable.dispose() removes the
key handler even if terminal.dispose() is not called; if removal API is not
available, add a clear comment documenting the required cleanup contract
(cleanupTerminalResources must call terminal.dispose() after
copyInteractionDisposable.dispose()) and ensure callers respect that order.
| it("lets browser copy handle Ctrl/Cmd+C only for selected terminal text", () => { | ||
| const selectedTerminal = terminalWithSelection("any", "selected") | ||
| const emptyTerminal = terminalWithSelection("any", "") | ||
|
|
||
| expect(shouldLetBrowserHandleTerminalCopyShortcut(keyboardEvent("c", { ctrlKey: true }), selectedTerminal)) | ||
| .toBe(true) | ||
| expect(shouldLetBrowserHandleTerminalCopyShortcut(keyboardEvent("C", { metaKey: true }), selectedTerminal)) | ||
| .toBe(true) | ||
| expect(shouldLetBrowserHandleTerminalCopyShortcut(keyboardEvent("c", { ctrlKey: true }), emptyTerminal)) | ||
| .toBe(false) | ||
| expect( | ||
| shouldLetBrowserHandleTerminalCopyShortcut(keyboardEvent("c", { altKey: true, ctrlKey: true }), selectedTerminal) | ||
| ) | ||
| .toBe(false) | ||
| expect(shouldLetBrowserHandleTerminalCopyShortcut(keyboardEvent("v", { ctrlKey: true }), selectedTerminal)) | ||
| .toBe(false) | ||
| expect( | ||
| shouldLetBrowserHandleTerminalCopyShortcut(keyboardEvent("c", { ctrlKey: true, type: "keyup" }), selectedTerminal) | ||
| ).toBe(false) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Отсутствует property-based тест для инварианта copy shortcut.
Согласно coding guidelines, математические инварианты должны проверяться через fast-check. Инвариант selected(t) ∧ copyShortcut(e) ⟹ browserCopy(e,t) можно выразить property-based тестом.
♻️ Пример property-based теста
import * as fc from "fast-check"
const keyArbitrary = fc.constantFrom("c", "C")
const modifierArbitrary = fc.record({
altKey: fc.boolean(),
ctrlKey: fc.boolean(),
metaKey: fc.boolean()
})
it("shouldLetBrowserHandleTerminalCopyShortcut returns true iff copy shortcut with selection", () => {
fc.assert(
fc.property(keyArbitrary, modifierArbitrary, fc.boolean(), (key, mods, hasSelection) => {
const event = keyboardEvent(key, { ...mods, type: "keydown" })
const terminal = terminalWithSelection("any", hasSelection ? "text" : "")
const isCopyShortcut = !mods.altKey && (mods.ctrlKey || mods.metaKey)
const expected = isCopyShortcut && hasSelection
return shouldLetBrowserHandleTerminalCopyShortcut(event, terminal) === expected
})
)
})As per coding guidelines: "Property-based tests (fast-check) must verify mathematical invariants".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/terminal/tests/web/terminal-copy-interaction.test.ts` around lines
67 - 86, Add a property-based test using fast-check that encodes the invariant
"selected(t) ∧ copyShortcut(e) ⟹ browserCopy(e,t)" for
shouldLetBrowserHandleTerminalCopyShortcut: generate key (c/C), modifiers
(altKey, ctrlKey, metaKey) and a boolean hasSelection, build event with
keyboardEvent(..., type: "keydown") and terminal via
terminalWithSelection("any", hasSelection ? "text" : ""), compute isCopyShortcut
= !mods.altKey && (mods.ctrlKey || mods.metaKey), set expected = isCopyShortcut
&& hasSelection, and assert shouldLetBrowserHandleTerminalCopyShortcut(event,
terminal) === expected; import fc from fast-check and place this property in the
existing terminal-copy-interaction.test.ts near the current unit tests to
replace or supplement the example assertions.
Source: Coding guidelines
1 participant
Closes #353
Summary
Verification
SSH terminal: 4bd315a203bf,attached, repo contexthttp://host.docker.internal:43842/repo.git.COPY_ME_FROM_REAL_PRODUCT,clipboardDatareceived the exact text, copy event was prevented by the app, and the shell read completed withREAD:AFTER_COPY(no^Cinterrupt).Mathematical Guarantees
Proof Screenshots