Uh oh!
There was an error while loading. Please reload this page.
fix(app): preserve terminal selection after mouseup - #332
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughPR расширяет типы mouse-событий терминала опциональными полями координат и модификаторов, добавляет логику подавления и реплея mouseup при завершении принудительного выделения текста, вынимает тестовую инфраструктуру в отдельный файл фикстур и добавляет тесты для верификации нового поведения. ChangesОбработка завершения терминального выделения мышью
Sequence DiagramsequenceDiagram
participant Terminal
participant Controller as TerminalSelectController
participant Target as selectionDragTarget
Terminal->>Controller: native mouseup event
alt forcedSelectionDrag активен
Controller->>Controller: preventDefault()
Controller->>Controller: stopPropagation()
Controller->>Controller: clearSelectionDrag()
Controller->>Target: dispatchEvent(forced mouseup)
else forcedSelectionDrag неактивен
Controller->>Target: пропустить mouseup
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Possibly related PRs
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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/app/tests/docker-git/fixtures/terminal-copy-interaction.ts`:
- Around line 202-208: dispatchEvent currently only routes mouse events via
dispatchMouse so "copy" listeners never run; update dispatchEvent to detect
clipboard/copy events (e.g. event.type === "copy" or a helper like
isCopyTestEventType) and invoke the appropriate handler (mirror the existing
dispatchMouse pattern — e.g. call this.dispatchCopy or this.dispatchClipboard
with the event) so addEventListener("copy", ...) registered in the fixture is
actually executed; keep existing behavior of pushing to this.dispatchedEvents
and returning !event.defaultPrevented.
In `@packages/app/tests/docker-git/terminal-copy-interaction.test.ts`:
- Around line 108-173: Add fast-check property-based tests that assert the
invariants currently covered by example cases: for attachTerminalCopyInteraction
with terminalWithSelection("any", "") (active tracking) generate arbitrary
mouseup event properties (clientX, clientY, screenX, screenY, button, buttons,
shiftKey, altKey, etc.) and assert each run that the original host-reported
mouseup is suppressed (up.preventDefaultCalls > 0,
up.stopImmediatePropagationCalls > 0, host-reported mouseReportEvents is empty),
exactly one replay is dispatched to documentTarget
(documentTarget.dispatchedEvents length === 1 and
expectSingleMouseEvent(finalizedSelectionEvents) exists and is not the original
event), and the replayed event preserves position/modifiers but normalizes
button/buttons as in the example; likewise add a property test for
terminalWithSelection("none", "") (inactive tracking) asserting no suppression
(preventDefault/stopImmediatePropagation/stopPropagation calls remain 0) and the
host mouseReportEvents contains the original event; reuse helpers from the test
(FakeTerminalCopyEventTarget, FakeTerminalCopyHost, mouseEvent,
expectSingleMouseEvent, attachTerminalCopyInteraction) and shrink inputs to
primitives for clearer failures.
🪄 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: 2083a3f8-6099-4d08-9fcc-c50390e2b8fb
📒 Files selected for processing (3)
packages/app/src/web/terminal-copy-interaction.tspackages/app/tests/docker-git/fixtures/terminal-copy-interaction.tspackages/app/tests/docker-git/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). (9)
- GitHub Check: E2E (Browser command)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Clone cache)
- GitHub Check: Final build (windows-latest)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{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/app/src/web/terminal-copy-interaction.tspackages/app/tests/docker-git/fixtures/terminal-copy-interaction.tspackages/app/tests/docker-git/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/app/src/web/terminal-copy-interaction.tspackages/app/tests/docker-git/fixtures/terminal-copy-interaction.tspackages/app/tests/docker-git/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/app/src/web/terminal-copy-interaction.tspackages/app/tests/docker-git/fixtures/terminal-copy-interaction.tspackages/app/tests/docker-git/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/app/src/web/terminal-copy-interaction.tspackages/app/tests/docker-git/fixtures/terminal-copy-interaction.tspackages/app/tests/docker-git/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/app/src/web/terminal-copy-interaction.tspackages/app/tests/docker-git/fixtures/terminal-copy-interaction.tspackages/app/tests/docker-git/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/app/src/web/terminal-copy-interaction.tspackages/app/tests/docker-git/fixtures/terminal-copy-interaction.tspackages/app/tests/docker-git/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/app/tests/docker-git/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/app/tests/docker-git/terminal-copy-interaction.test.ts
🔇 Additional comments (12)
packages/app/src/web/terminal-copy-interaction.ts (10)
33-45: LGTM!
47-65: LGTM!
143-145: LGTM!
147-164: LGTM!
166-192: LGTM!
194-203: LGTM!
205-209: LGTM!
211-216: LGTM!
242-258: LGTM!
235-240: LGTM!packages/app/tests/docker-git/fixtures/terminal-copy-interaction.ts (1)
1-201: LGTM!Also applies to: 209-265
packages/app/tests/docker-git/terminal-copy-interaction.test.ts (1)
12-19: LGTM!Also applies to: 201-201
| dispatchEvent(event: Event): boolean { | ||
| this.dispatchedEvents.push(event) | ||
| if (isMouseTestEventType(event.type) && isTerminalCopyTestMouseEvent(event)) { | ||
| this.dispatchMouse(event.type, event) | ||
| } | ||
| return !event.defaultPrevented | ||
| } |
There was a problem hiding this comment.
copy-события не доставляются подписчикам в dispatchEvent.
Сейчас диспетчер вызывает обработчики только для mouse-событий. В итоге addEventListener("copy", ...) в фикстуре регистрируется, но никогда не исполняется, что может скрыть регрессии copy-flow в тестах.
💡 Предлагаемый фикс
+const isTerminalCopyTestClipboardEvent = (+ event: Event+): event is Event & TerminalCopyTestClipboardEvent => "clipboardData" in event+
export class FakeTerminalCopyEventTarget {
@@
dispatchEvent(event: Event): boolean {
this.dispatchedEvents.push(event)
+ if (event.type === "copy" && isTerminalCopyTestClipboardEvent(event)) {+ for (const entry of this.listeners) {+ if (entry.type === "copy") {+ entry.listener(event)+ }+ }+ return !event.defaultPrevented+ }
if (isMouseTestEventType(event.type) && isTerminalCopyTestMouseEvent(event)) {
this.dispatchMouse(event.type, event)
}
return !event.defaultPrevented
}🤖 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/app/tests/docker-git/fixtures/terminal-copy-interaction.ts` around
lines 202 - 208, dispatchEvent currently only routes mouse events via
dispatchMouse so "copy" listeners never run; update dispatchEvent to detect
clipboard/copy events (e.g. event.type === "copy" or a helper like
isCopyTestEventType) and invoke the appropriate handler (mirror the existing
dispatchMouse pattern — e.g. call this.dispatchCopy or this.dispatchClipboard
with the event) so addEventListener("copy", ...) registered in the fixture is
actually executed; keep existing behavior of pushing to this.dispatchedEvents
and returning !event.defaultPrevented.
| it("suppresses real mouseup reports and replays a document mouseup for selection finalization", () => { | ||
| const documentTarget = new FakeTerminalCopyEventTarget() | ||
| const host = new FakeTerminalCopyHost(documentTarget) | ||
| const finalizedSelectionEvents: Array<TerminalCopyTestMouseEvent> = [] | ||
| const mouseReportEvents: Array<TerminalCopyTestMouseEvent> = [] | ||
| documentTarget.addBubbleMouseListener("mouseup", (event) => { | ||
| finalizedSelectionEvents.push(event) | ||
| }) | ||
| host.addBubbleMouseListener("mouseup", (event) => { | ||
| mouseReportEvents.push(event) | ||
| }) | ||
| const disposable = attachTerminalCopyInteraction({ host, terminal: terminalWithSelection("any", "") }) | ||
| const up = mouseEvent(0, "mouseup", { | ||
| clientX: 12, | ||
| clientY: 34, | ||
| screenX: 56, | ||
| screenY: 78 | ||
| }) | ||
| host.dispatchMouse("mousedown", mouseEvent(0)) | ||
| host.dispatchBubblingMouse("mouseup", up) | ||
| expect(up.shiftKey).toBe(true) | ||
| expect(up.preventDefaultCalls).toBe(1) | ||
| expect(up.stopImmediatePropagationCalls).toBe(1) | ||
| expect(up.stopPropagationCalls).toBeGreaterThanOrEqual(1) | ||
| expect(mouseReportEvents).toEqual([]) | ||
| expect(documentTarget.dispatchedEvents).toHaveLength(1) | ||
| const replayed = expectSingleMouseEvent(finalizedSelectionEvents) | ||
| expect(replayed).not.toBe(up) | ||
| expect(replayed.button).toBe(0) | ||
| expect(replayed.buttons).toBe(0) | ||
| expect(replayed.clientX).toBe(12) | ||
| expect(replayed.clientY).toBe(34) | ||
| expect(replayed.screenX).toBe(56) | ||
| expect(replayed.screenY).toBe(78) | ||
| expect(replayed.shiftKey).toBe(true) | ||
| expect(replayed.altKey).toBe(false) | ||
| expectNoDragListeners(documentTarget) | ||
| disposable.dispose() | ||
| }) | ||
| it("does not suppress or replay mouseup when mouse tracking is inactive", () => { | ||
| const documentTarget = new FakeTerminalCopyEventTarget() | ||
| const host = new FakeTerminalCopyHost(documentTarget) | ||
| const mouseReportEvents: Array<TerminalCopyTestMouseEvent> = [] | ||
| host.addBubbleMouseListener("mouseup", (event) => { | ||
| mouseReportEvents.push(event) | ||
| }) | ||
| const disposable = attachTerminalCopyInteraction({ host, terminal: terminalWithSelection("none", "") }) | ||
| const up = mouseEvent(0, "mouseup") | ||
| host.dispatchMouse("mousedown", mouseEvent(0)) | ||
| host.dispatchBubblingMouse("mouseup", up) | ||
| expect(up.shiftKey).toBe(false) | ||
| expect(up.preventDefaultCalls).toBe(0) | ||
| expect(up.stopImmediatePropagationCalls).toBe(0) | ||
| expect(up.stopPropagationCalls).toBe(0) | ||
| expect(documentTarget.dispatchedEvents).toEqual([]) | ||
| expect(mouseReportEvents).toEqual([up]) | ||
| disposable.dispose() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Добавьте property-based проверки инвариантов для новых mouseup сценариев.
Новые кейсы проверяют фиксированные примеры, но здесь удобно и нужно закрепить инварианты (active tracking: всегда suppress реального mouseup и ровно один replay; inactive tracking: never suppress/replay) через fast-check.
As per coding guidelines "Implement property-based testing using fast-check for mathematical properties and 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/app/tests/docker-git/terminal-copy-interaction.test.ts` around lines
108 - 173, Add fast-check property-based tests that assert the invariants
currently covered by example cases: for attachTerminalCopyInteraction with
terminalWithSelection("any", "") (active tracking) generate arbitrary mouseup
event properties (clientX, clientY, screenX, screenY, button, buttons, shiftKey,
altKey, etc.) and assert each run that the original host-reported mouseup is
suppressed (up.preventDefaultCalls > 0, up.stopImmediatePropagationCalls > 0,
host-reported mouseReportEvents is empty), exactly one replay is dispatched to
documentTarget (documentTarget.dispatchedEvents length === 1 and
expectSingleMouseEvent(finalizedSelectionEvents) exists and is not the original
event), and the replayed event preserves position/modifiers but normalizes
button/buttons as in the example; likewise add a property test for
terminalWithSelection("none", "") (inactive tracking) asserting no suppression
(preventDefault/stopImmediatePropagation/stopPropagation calls remain 0) and the
host mouseReportEvents contains the original event; reuse helpers from the test
(FakeTerminalCopyEventTarget, FakeTerminalCopyHost, mouseEvent,
expectSingleMouseEvent, attachTerminalCopyInteraction) and shrink inputs to
primitives for clearer failures.
Uh oh!
There was an error while loading. Please reload this page.
Fixes#331.
Summary
Verification
Mathematical guarantees