fix(web): preserve terminal selection copy#322
Conversation
📝 WalkthroughWalkthroughPR добавляет модуль для принудительного копирования выделения терминала, модуль обработки ввода и новый обработчик wheel; runtime-core рефакторится с переэкспортами, copy/wheel интегрируются в lifecycle монтированного терминала. Добавлены unit-тесты и правка схемы API auth snapshot. ChangesTerminal I/O и Copy Interaction
🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/src/web/terminal-copy-interaction.ts`:
- Around line 75-79: The current shouldForceTerminalSelectionContext function
incorrectly forces the context-selection without checking mouse tracking; update
the boolean condition in shouldForceTerminalSelectionContext (which takes
TerminalMouseButtonEvent and TerminalCopyInteractionTerminal) to also require
active mouse tracking by checking the terminal's mouse tracking state (e.g.,
terminal.mouseTrackingMode === 'active' or terminal.isMouseTrackingActive()) so
it returns true only when isSecondaryMouseButton(event) &&
terminal.hasSelection() && mouse tracking is active.
In `@packages/app/src/web/terminal-panel-input.ts`:
- Around line 36-60: sendTerminalClientMessage and attachTerminalInput perform
direct IO (socket.send, terminal.onData, terminal.scrollToBottom) which violates
the FCIS pattern; refactor by extracting IO behind typed Effect-TS services and
inject them via Layer.Context.Tags: create a WebSocketService (wrap socket.send)
and a TerminalIOService (wrap terminal.onData and scrollToBottom and pasteGuard
checks) and replace direct calls in sendTerminalClientMessage and
attachTerminalInput with calls to those service methods (using
TerminalClientSocketRef/TerminalInputTarget only as data carriers), then provide
implementations via Layer.effect in the SHELL layer so this module contains
pure, testable orchestration calling the services instead of performing raw
DOM/WebSocket IO.
In `@packages/app/tests/docker-git/terminal-copy-interaction.test.ts`:
- Around line 21-74: Add property-based tests using fast-check (fc.property) to
assert invariants for the pure functions: for
shouldForceBrowserTerminalSelection generate button numbers and
mouseTrackingMode values (include "any","drag","none","vt200","x10") and assert
result === (button === 0 && mouseTrackingMode !== "none"); for
shouldForceTerminalSelectionContext generate button values and a boolean
hasSelection and assert result === (button === 2 && hasSelection); for
writeTerminalSelectionToClipboardData generate clipboardData as null or an
object with setData and selection as empty or non-empty string (use
terminalWithSelection to create terminal selection) and assert result ===
(clipboardData !== null && selection.length > 0) and that setData is invoked
only when invariant true; import fast-check and add these
fc.assert(fc.property(...)) cases into the existing
terminal-copy-interaction.test.ts alongside the example tests so the intended
invariants are covered.
In `@packages/app/tests/docker-git/terminal-panel-runtime-core.test.ts`:
- Around line 64-110: Add property-based tests using fast-check (import as fc)
that assert invariants for isTerminalMouseReportInput, attachTerminalInput
scroll behavior, and paste-guard suppression: (1) generate arbitrary strings and
assert isTerminalMouseReportInput returns true for strings constructed to match
xterm mouse encodings (e.g. patterns using ESC '[' 'M' and ESC '[<' sequences)
and false for other random strings — reference isTerminalMouseReportInput; (2)
for any random input string, use createTerminalInputHarness,
createOpenSocketRef, passThroughPasteGuard and attachTerminalInput to assert
that when isTerminalMouseReportInput(input) is true the harness
input.state.scrolls does not increase and the socket receives the exact input,
and when it is false the harness scrolls once and the socket receives the input
— reference attachTerminalInput, createTerminalInputHarness,
createOpenSocketRef, passThroughPasteGuard; (3) assert that when using a
pasteGuard with shouldSuppressTerminalInput returning true (create a pasteGuard
object like in the existing test with noop) then for any generated string
input.state.scrolls remains unchanged and nothing is sent — reference
pasteGuard, noop; structure each as fc.property(...) with appropriate generators
(fc.string, fc.constantFrom for specific prefixes, oneof/combinations) and
assertions mirroring the existing example tests.
🪄 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: 0c039640-daef-4432-82dd-197aad47925e
📒 Files selected for processing (6)
packages/app/src/web/terminal-copy-interaction.tspackages/app/src/web/terminal-panel-input.tspackages/app/src/web/terminal-panel-runtime-core.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/tests/docker-git/terminal-copy-interaction.test.tspackages/app/tests/docker-git/terminal-panel-runtime-core.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 (OpenCode)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: E2E (Browser command)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (Login context)
- GitHub Check: Lint
- GitHub Check: Test
- 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/tests/docker-git/terminal-copy-interaction.test.tspackages/app/src/web/terminal-panel-input.tspackages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-copy-interaction.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/src/web/terminal-panel-runtime-core.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.tspackages/app/tests/docker-git/terminal-panel-runtime-core.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/tests/docker-git/terminal-copy-interaction.test.tspackages/app/src/web/terminal-panel-input.tspackages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-copy-interaction.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/src/web/terminal-panel-runtime-core.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.tspackages/app/tests/docker-git/terminal-panel-runtime-core.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/tests/docker-git/terminal-copy-interaction.test.tspackages/app/src/web/terminal-panel-input.tspackages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-copy-interaction.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/src/web/terminal-panel-runtime-core.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/tests/docker-git/terminal-copy-interaction.test.tspackages/app/src/web/terminal-panel-input.tspackages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-copy-interaction.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/src/web/terminal-panel-runtime-core.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/tests/docker-git/terminal-copy-interaction.test.tspackages/app/src/web/terminal-panel-input.tspackages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-copy-interaction.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/src/web/terminal-panel-runtime-core.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/app/tests/docker-git/terminal-copy-interaction.test.tspackages/app/src/web/terminal-panel-input.tspackages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-copy-interaction.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/src/web/terminal-panel-runtime-core.ts
🔇 Additional comments (3)
packages/app/src/web/terminal-panel-runtime.ts (1)
3-3: LGTM!Also applies to: 26-37, 41-48, 60-60, 102-105, 125-125, 170-170
packages/app/src/web/terminal-panel-input.ts (1)
25-34: LGTM!packages/app/src/web/terminal-panel-runtime-core.ts (1)
21-22: LGTM!Also applies to: 37-37, 91-94
| export const shouldForceTerminalSelectionContext = ( | ||
| event: TerminalMouseButtonEvent, | ||
| terminal: TerminalCopyInteractionTerminal | ||
| ): boolean => isSecondaryMouseButton(event) && terminal.hasSelection() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Ограничьте форсирование context-selection только активным mouse tracking.
На Line 78 сейчас условие не учитывает mouseTrackingMode, из-за чего модификатор форсируется даже при "none" и меняет поведение вне целевого кейса.
Предлагаемое изменение
export const shouldForceTerminalSelectionContext = (
event: TerminalMouseButtonEvent,
terminal: TerminalCopyInteractionTerminal
-): boolean => isSecondaryMouseButton(event) && terminal.hasSelection()
+): boolean =>
+ isSecondaryMouseButton(event) &&
+ hasActiveMouseTracking(terminal) &&
+ terminal.hasSelection()🤖 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/src/web/terminal-copy-interaction.ts` around lines 75 - 79, The
current shouldForceTerminalSelectionContext function incorrectly forces the
context-selection without checking mouse tracking; update the boolean condition
in shouldForceTerminalSelectionContext (which takes TerminalMouseButtonEvent and
TerminalCopyInteractionTerminal) to also require active mouse tracking by
checking the terminal's mouse tracking state (e.g., terminal.mouseTrackingMode
=== 'active' or terminal.isMouseTrackingActive()) so it returns true only when
isSecondaryMouseButton(event) && terminal.hasSelection() && mouse tracking is
active.
| export const sendTerminalClientMessage = ( | ||
| socketRef: TerminalClientSocketRef, | ||
| message: TerminalClientMessage | ||
| ): void => { | ||
| const socket = socketRef.current | ||
| if (socket === null || socket.readyState !== WebSocket.OPEN) { | ||
| return | ||
| } | ||
| socket.send(JSON.stringify(message)) | ||
| } | ||
|
|
||
| export const attachTerminalInput = ( | ||
| terminal: TerminalInputTarget, | ||
| socketRef: TerminalClientSocketRef, | ||
| pasteGuard: TerminalPasteGuard | ||
| ) => | ||
| terminal.onData((data) => { | ||
| if (pasteGuard.shouldSuppressTerminalInput(data)) { | ||
| return | ||
| } | ||
| if (!isTerminalMouseReportInput(data)) { | ||
| terminal.scrollToBottom() | ||
| } | ||
| sendTerminalClientMessage(socketRef, { data, type: "input" }) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Проверка, что в модуле есть прямые IO-вызовы без Effect-композиции
rg -n "socket\.send\(|terminal\.onData\(|scrollToBottom\(|Effect\." packages/app/src/web/terminal-panel-input.tsRepository: ProverCoderAI/docker-git
Length of output: 178
🏁 Script executed:
# Проверка полного содержимого файла
cat -n packages/app/src/web/terminal-panel-input.tsRepository: ProverCoderAI/docker-git
Length of output: 2352
🏁 Script executed:
# Поиск использования Effect и Layer в веб-части приложения
rg -l "Effect\.|Context\.Tag|Layer\." packages/app/src/web/ | head -10Repository: ProverCoderAI/docker-git
Length of output: 539
🏁 Script executed:
# Поиск паттернов Effect для WebSocket/сокетов в кодовой базе
rg -B2 -A5 "Context\.Tag|Layer\.effect" packages/app/src/web/ | head -50Repository: ProverCoderAI/docker-git
Length of output: 50
🏁 Script executed:
# Поиск использования sendTerminalClientMessage и attachTerminalInput
rg -B3 -A3 "sendTerminalClientMessage|attachTerminalInput" packages/app/src/web/ --type tsRepository: ProverCoderAI/docker-git
Length of output: 4947
🏁 Script executed:
# Проверка terminal-panel-runtime-core.ts на предмет Effect паттернов
cat packages/app/src/web/terminal-panel-runtime-core.ts | head -100Repository: ProverCoderAI/docker-git
Length of output: 3232
🏁 Script executed:
# Поиск Context.Tag или Layer определений в веб приложении
rg "extends Context\.Tag|Layer\." packages/app/src/web/ -A3Repository: ProverCoderAI/docker-git
Length of output: 50
🏁 Script executed:
# Посмотреть как используется attachTerminalInput в terminal-panel-runtime.ts
rg -B5 -A10 "inputDisposable.*attachTerminalInput" packages/app/src/web/terminal-panel-runtime.tsRepository: ProverCoderAI/docker-git
Length of output: 489
🏁 Script executed:
# Полный контекст использования в terminal-panel-runtime.ts
cat packages/app/src/web/terminal-panel-runtime.ts | head -150Repository: ProverCoderAI/docker-git
Length of output: 4793
🏁 Script executed:
# Проверить структуру проекта и наличие Effect-based сервисов
find packages/app/src -name "*.ts" -type f | xargs rg "extends Context\.Tag" 2>/dev/null | head -20Repository: ProverCoderAI/docker-git
Length of output: 50
🏁 Script executed:
# Проверить git статус файла - новый или существующий
git -C packages/app/src/web log --oneline terminal-panel-input.ts 2>/dev/null | head -5Repository: ProverCoderAI/docker-git
Length of output: 119
🏁 Script executed:
# Проверить если есть PR description или сообщение о коммите
git -C packages/app log --oneline -20 --all 2>/dev/null | head -20Repository: ProverCoderAI/docker-git
Length of output: 1037
Рефакторинг под Effect + Layer необходимо выполнить в отдельной архитектурной задаче, но текущий fix содержит нарушения.
Функции sendTerminalClientMessage и attachTerminalInput выполняют IO побочные эффекты напрямую (строки 44, 52, 57):
socket.send()— прямой вызов WebSocket APIterminal.onData()— прямая подписка на событиеterminal.scrollToBottom()— прямая DOM-мутация
Согласно кодовым гайдлайнам, все внешние зависимости должны быть обёрнуты через Effect-TS Layer с типизированными Context.Tag. Текущая реализация нарушает контракт FCIS-паттерна: IO-логика должна находиться в SHELL слое через Effect-композицию, оставляя здесь только чистые функции и монадическую композицию.
Этот рефакторинг требует создания типизированных сервисов (WebSocketService, TerminalIOService) через Context.Tag и их инъекции через Layer.effect. Рекомендуется выполнить как отдельную архитектурную задачу для всего модуля, а не в текущем fix-коммите.
🤖 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/src/web/terminal-panel-input.ts` around lines 36 - 60,
sendTerminalClientMessage and attachTerminalInput perform direct IO
(socket.send, terminal.onData, terminal.scrollToBottom) which violates the FCIS
pattern; refactor by extracting IO behind typed Effect-TS services and inject
them via Layer.Context.Tags: create a WebSocketService (wrap socket.send) and a
TerminalIOService (wrap terminal.onData and scrollToBottom and pasteGuard
checks) and replace direct calls in sendTerminalClientMessage and
attachTerminalInput with calls to those service methods (using
TerminalClientSocketRef/TerminalInputTarget only as data carriers), then provide
implementations via Layer.effect in the SHELL layer so this module contains
pure, testable orchestration calling the services instead of performing raw
DOM/WebSocket IO.
| describe("terminal copy interaction", () => { | ||
| it("forces browser selection for primary mouse input while terminal mouse tracking is active", () => { | ||
| expect(shouldForceBrowserTerminalSelection({ button: 0 }, terminalWithSelection("any", ""))).toBe(true) | ||
| expect(shouldForceBrowserTerminalSelection({ button: 0 }, terminalWithSelection("drag", ""))).toBe(true) | ||
| expect(shouldForceBrowserTerminalSelection({ button: 0 }, terminalWithSelection("none", ""))).toBe(false) | ||
| expect(shouldForceBrowserTerminalSelection({ button: 2 }, terminalWithSelection("any", ""))).toBe(false) | ||
| }) | ||
|
|
||
| it("forces context-menu clicks into selection mode only when selected terminal text exists", () => { | ||
| expect(shouldForceTerminalSelectionContext({ button: 2 }, terminalWithSelection("any", "selected"))).toBe(true) | ||
| expect(shouldForceTerminalSelectionContext({ button: 2 }, terminalWithSelection("any", ""))).toBe(false) | ||
| expect(shouldForceTerminalSelectionContext({ button: 0 }, terminalWithSelection("any", "selected"))).toBe(false) | ||
| }) | ||
|
|
||
| it("uses Shift as the forced selection modifier on non-Mac platforms", () => { | ||
| const event = { altKey: false, shiftKey: false } | ||
|
|
||
| expect(forceTerminalSelectionModifier(event, "Win32")).toBe(true) | ||
| expect(event).toEqual({ altKey: false, shiftKey: true }) | ||
| }) | ||
|
|
||
| it("uses Alt as the forced selection modifier on Mac platforms", () => { | ||
| const event = { altKey: false, shiftKey: false } | ||
|
|
||
| expect(forceTerminalSelectionModifier(event, "MacIntel")).toBe(true) | ||
| expect(event).toEqual({ altKey: true, shiftKey: false }) | ||
| }) | ||
|
|
||
| it("writes xterm selection text into clipboard data", () => { | ||
| const writes: Array<{ readonly data: string; readonly format: string }> = [] | ||
| const clipboardData = { | ||
| setData: (format: string, data: string) => { | ||
| writes.push({ data, format }) | ||
| } | ||
| } | ||
|
|
||
| expect(writeTerminalSelectionToClipboardData(terminalWithSelection("any", "line one\nline two"), clipboardData)) | ||
| .toBe( | ||
| true | ||
| ) | ||
| expect(writes).toEqual([{ data: "line one\nline two", format: "text/plain" }]) | ||
| }) | ||
|
|
||
| it("does not handle copy when xterm has no selection or clipboard data", () => { | ||
| const clipboardData = { | ||
| setData: () => { | ||
| expect.fail("clipboard data should not be written") | ||
| } | ||
| } | ||
|
|
||
| expect(writeTerminalSelectionToClipboardData(terminalWithSelection("any", ""), clipboardData)).toBe(false) | ||
| expect(writeTerminalSelectionToClipboardData(terminalWithSelection("any", "selected"), null)).toBe(false) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Проверка наличия property-based тестов в этом файле
rg -n --type=ts 'fc\.property|from "fast-check"' packages/app/tests/docker-git/terminal-copy-interaction.test.tsRepository: ProverCoderAI/docker-git
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find all test files related to terminal copy or docker-git functionality
fd --type f '\.test\.ts' packages/app/tests/docker-git/ | head -20Repository: ProverCoderAI/docker-git
Length of output: 1207
🏁 Script executed:
#!/bin/bash
# Search for the actual implementation of these functions
fd --type f '\.ts$' packages/app --path '*docker-git*' --path '*terminal*' | grep -v test | head -20Repository: ProverCoderAI/docker-git
Length of output: 302
🏁 Script executed:
#!/bin/bash
# Check the test file to see its structure and imports
head -20 packages/app/tests/docker-git/terminal-copy-interaction.test.tsRepository: ProverCoderAI/docker-git
Length of output: 664
🏁 Script executed:
#!/bin/bash
# Find the implementation file
cat packages/app/src/web/terminal-copy-interaction.ts 2>/dev/null || cat packages/app/src/web/terminal-copy-interaction.jsRepository: ProverCoderAI/docker-git
Length of output: 4376
🏁 Script executed:
#!/bin/bash
# Check if there's a PR description or issue context
git log --oneline -1
git log -1 --format=%bRepository: ProverCoderAI/docker-git
Length of output: 120
Добавьте property-based проверки инвариантов copy interaction функций.
Кодовые руководства требуют использования fast-check для математических инвариантов в .test.ts файлах. Текущее покрытие example-based; необходимо добавить fc.property для следующих чистых функций:
shouldForceBrowserTerminalSelection: доменизация поbutton ∈ {0, 2, ...} × mouseTrackingMode ∈ {"any", "drag", "none", "vt200", "x10"}с инвариантомresult ≡ (button === 0 ∧ mouseTrackingMode ≠ "none")shouldForceTerminalSelectionContext: доменизация поbutton × hasSelectionс инвариантомresult ≡ (button === 2 ∧ hasSelection())writeTerminalSelectionToClipboardData: доменизация поclipboardData ∈ {null} ∪ ClipboardData × selection ∈ {"" | "non-empty"}с инвариантомresult ≡ (clipboardData ≠ null ∧ selection.length > 0)
🤖 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
21 - 74, Add property-based tests using fast-check (fc.property) to assert
invariants for the pure functions: for shouldForceBrowserTerminalSelection
generate button numbers and mouseTrackingMode values (include
"any","drag","none","vt200","x10") and assert result === (button === 0 &&
mouseTrackingMode !== "none"); for shouldForceTerminalSelectionContext generate
button values and a boolean hasSelection and assert result === (button === 2 &&
hasSelection); for writeTerminalSelectionToClipboardData generate clipboardData
as null or an object with setData and selection as empty or non-empty string
(use terminalWithSelection to create terminal selection) and assert result ===
(clipboardData !== null && selection.length > 0) and that setData is invoked
only when invariant true; import fast-check and add these
fc.assert(fc.property(...)) cases into the existing
terminal-copy-interaction.test.ts alongside the example tests so the intended
invariants are covered.
| it("detects xterm mouse report input encodings", () => { | ||
| expect(isTerminalMouseReportInput("\u001B[M !!")).toBe(true) | ||
| expect(isTerminalMouseReportInput("\u001B[<64;10;5M")).toBe(true) | ||
| expect(isTerminalMouseReportInput("\u001B[<0;10;5m")).toBe(true) | ||
| expect(isTerminalMouseReportInput("\u001B[64;10;5M")).toBe(true) | ||
| expect(isTerminalMouseReportInput("\u001B[2M")).toBe(false) | ||
| expect(isTerminalMouseReportInput("a")).toBe(false) | ||
| }) | ||
|
|
||
| it("scrolls to bottom for regular terminal input before sending it to the socket", () => { | ||
| const input = createTerminalInputHarness() | ||
| const { sent, socketRef } = createOpenSocketRef() | ||
|
|
||
| const disposable = attachTerminalInput(input.terminal, socketRef, passThroughPasteGuard) | ||
| input.emit("a") | ||
| disposable.dispose() | ||
|
|
||
| expect(input.state.scrolls).toBe(1) | ||
| expect(input.state.disposed).toBe(1) | ||
| expect(sent).toEqual([JSON.stringify({ data: "a", type: "input" })]) | ||
| }) | ||
|
|
||
| it("keeps the viewport stable for terminal mouse reports", () => { | ||
| const input = createTerminalInputHarness() | ||
| const { sent, socketRef } = createOpenSocketRef() | ||
|
|
||
| attachTerminalInput(input.terminal, socketRef, passThroughPasteGuard) | ||
| input.emit("\u001B[<64;10;5M") | ||
|
|
||
| expect(input.state.scrolls).toBe(0) | ||
| expect(sent).toEqual([JSON.stringify({ data: "\u001B[<64;10;5M", type: "input" })]) | ||
| }) | ||
|
|
||
| it("does not scroll or send input suppressed by the paste guard", () => { | ||
| const input = createTerminalInputHarness() | ||
| const { sent, socketRef } = createOpenSocketRef() | ||
| const pasteGuard = { | ||
| shouldSuppressTerminalInput: () => true, | ||
| suppressNextNativeImagePaste: noop | ||
| } | ||
|
|
||
| attachTerminalInput(input.terminal, socketRef, pasteGuard) | ||
| input.emit("\u0016") | ||
|
|
||
| expect(input.state.scrolls).toBe(0) | ||
| expect(sent).toEqual([]) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Найти файл с тестом
fd -t f "terminal-panel-runtime-core.test.ts" --type fRepository: ProverCoderAI/docker-git
Length of output: 134
🏁 Script executed:
# Проверить наличие fast-check в зависимостях
fd -t f "package.json" | head -5Repository: ProverCoderAI/docker-git
Length of output: 205
🏁 Script executed:
# Посмотреть структуру каталога с тестами
ls -la packages/app/tests/docker-git/ 2>/dev/null || echo "Путь не найден, ищу альтернативы"Repository: ProverCoderAI/docker-git
Length of output: 5404
🏁 Script executed:
# Читаем содержимое тестового файла
cat -n packages/app/tests/docker-git/terminal-panel-runtime-core.test.tsRepository: ProverCoderAI/docker-git
Length of output: 3971
🏁 Script executed:
# Проверяем наличие fast-check в зависимостях
rg -A 5 "fast-check" package.json packages/app/package.json 2>/dev/null || echo "fast-check не найден"Repository: ProverCoderAI/docker-git
Length of output: 387
🏁 Script executed:
# Проверяем, какой тестовый фреймворк используется
rg -t js -t ts "import.*vitest|from.*vitest|import.*jest|from.*jest" packages/app/tests/docker-git/terminal-panel-runtime-core.test.tsRepository: ProverCoderAI/docker-git
Length of output: 173
🏁 Script executed:
# Найти реализацию функций из импорта
fd -t f "terminal-panel-input.js" packages/app/src/web/Repository: ProverCoderAI/docker-git
Length of output: 50
🏁 Script executed:
# Посмотреть содержимое файла с реализацией
cat -n packages/app/src/web/terminal-panel-input.js 2>/dev/null || find packages/app/src -name "*terminal-panel-input*" -type fRepository: ProverCoderAI/docker-git
Length of output: 113
🏁 Script executed:
# Читаем реализацию функций
cat -n packages/app/src/web/terminal-panel-input.tsRepository: ProverCoderAI/docker-git
Length of output: 2352
🏁 Script executed:
# Проверим, есть ли уже property-based тесты в файле или в других файлах того же каталога
rg -l "fc\.|fast-check" packages/app/tests/docker-git/ | head -10Repository: ProverCoderAI/docker-git
Length of output: 487
🏁 Script executed:
# Проверим паттерны в guideline для этого файла
rg "test\.(ts|tsx)" <<< "**/*.test.{ts,tsx}"Repository: ProverCoderAI/docker-git
Length of output: 50
Добавьте property-based тесты для проверки инвариантов распознавания мышиных событий и поведения скролла.
Текущие тесты покрывают только фиксированные примеры. Для доказуемости инвариантов (isTerminalMouseReportInput, условие скролла, подавление вводом из paste guard) требуются property-based сценарии через fc.property(...).
Пример минимального направления
+import fc from "fast-check"
+
+it("сохраняет инвариант скролла для не-mouse ввода", () => {
+ fc.assert(
+ fc.property(fc.string().filter((s) => !isTerminalMouseReportInput(s)), (s) => {
+ const input = createTerminalInputHarness()
+ const { socketRef } = createOpenSocketRef()
+ attachTerminalInput(input.terminal, socketRef, passThroughPasteGuard)
+ input.emit(s)
+ expect(input.state.scrolls).toBe(1)
+ })
+ )
+})По guideline **/*.test.{ts,tsx}: 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-panel-runtime-core.test.ts` around
lines 64 - 110, Add property-based tests using fast-check (import as fc) that
assert invariants for isTerminalMouseReportInput, attachTerminalInput scroll
behavior, and paste-guard suppression: (1) generate arbitrary strings and assert
isTerminalMouseReportInput returns true for strings constructed to match xterm
mouse encodings (e.g. patterns using ESC '[' 'M' and ESC '[<' sequences) and
false for other random strings — reference isTerminalMouseReportInput; (2) for
any random input string, use createTerminalInputHarness, createOpenSocketRef,
passThroughPasteGuard and attachTerminalInput to assert that when
isTerminalMouseReportInput(input) is true the harness input.state.scrolls does
not increase and the socket receives the exact input, and when it is false the
harness scrolls once and the socket receives the input — reference
attachTerminalInput, createTerminalInputHarness, createOpenSocketRef,
passThroughPasteGuard; (3) assert that when using a pasteGuard with
shouldSuppressTerminalInput returning true (create a pasteGuard object like in
the existing test with noop) then for any generated string input.state.scrolls
remains unchanged and nothing is sent — reference pasteGuard, noop; structure
each as fc.property(...) with appropriate generators (fc.string, fc.constantFrom
for specific prefixes, oneof/combinations) and assertions mirroring the existing
example tests.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/src/web/api-auth-schema.ts`:
- Around line 31-32: Добавьте функциональные комментарии рядом с определениями
grokAuthEntries и grokAuthPath (где используется Schema.optionalWith) используя
маркеры CHANGE, WHY, QUOTE/REF и INVARIANT: в CHANGE кратко опишите внесённое
поле и значение по умолчанию; в WHY объясните почему дефолты (0 и "") сохраняют
backward compatibility; в QUOTE или REF укажите связанное требование/issue/PR; в
INVARIANT зафиксируйте гарантию «при отсутствии поля применяется default» и типы
(Number/String). Обновите описание PR `#322` или добавьте ссылку на
соответствующее issue в QUOTE/REF, чтобы изменение схемы было задокументировано
в спецификации.
In `@packages/app/tests/docker-git/api-auth-schema.test.ts`:
- Around line 25-47: Add a new test case in
packages/app/tests/docker-git/api-auth-schema.test.ts that supplies a snapshot
object including grokAuthEntries and grokAuthPath and asserts those values are
preserved (not defaulted); use the same test structure as the existing case but
call the decoder (either decodeAuthSnapshotResponse or
ParseResult.decodeUnknownEither(Schema.parseJson(AuthSnapshotResponseSchema)))
on a JSON string that includes grokAuthEntries: 5 and grokAuthPath:
"/home/dev/.docker-git/.orch/auth/grok", then assert Either.isRight(decoded) and
that decoded.right.snapshot.grokAuthEntries === 5 and
decoded.right.snapshot.grokAuthPath === "/home/dev/.docker-git/.orch/auth/grok".
- Around line 22-23: Add comprehensive TSDoc to the helper function
decodeAuthSnapshotResponse: describe its purpose (decodes a
LegacyAuthSnapshotResponse via AuthSnapshotResponseSchema and
ParseResult.decodeUnknownEither), document the payload parameter and the fact
the function returns an Either that represents both successful and failed
decodes (use the precise return type in the comment), and include required tags:
`@pure` true, `@effect` none (or list any required services if applicable),
`@invariant` (if any decoding invariants apply), `@precondition` (expected shape of
payload), `@postcondition` (Either contains decoded value or decode errors), and
`@complexity` (time/space). Reference the function name decodeAuthSnapshotResponse
and the schema symbol AuthSnapshotResponseSchema so the maintainer can locate
and verify the doc.
🪄 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: 03773233-11ba-46be-894e-7647440551e3
📒 Files selected for processing (2)
packages/app/src/web/api-auth-schema.tspackages/app/tests/docker-git/api-auth-schema.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 (Clone auto-open SSH)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (Browser command)
- GitHub Check: Test
- GitHub Check: Final build (windows-latest)
- GitHub Check: Lint
🧰 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/api-auth-schema.tspackages/app/tests/docker-git/api-auth-schema.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/api-auth-schema.tspackages/app/tests/docker-git/api-auth-schema.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/api-auth-schema.tspackages/app/tests/docker-git/api-auth-schema.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/api-auth-schema.tspackages/app/tests/docker-git/api-auth-schema.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/api-auth-schema.tspackages/app/tests/docker-git/api-auth-schema.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/api-auth-schema.tspackages/app/tests/docker-git/api-auth-schema.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/api-auth-schema.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/api-auth-schema.test.ts
| grokAuthEntries: Schema.optionalWith(Schema.Number, { default: () => 0 }), | ||
| grokAuthPath: Schema.optionalWith(Schema.String, { default: () => "" }), |
There was a problem hiding this comment.
Добавьте функциональные комментарии для документирования изменения схемы.
Согласно guidelines, все изменения должны быть документированы функциональными комментариями с метками CHANGE (что изменилось), WHY (почему это необходимо для backward compatibility), QUOTE(ТЗ) или REF (ссылка на требование/issue), и INVARIANT (гарантия дефолтных значений при отсутствии полей).
Кроме того, это изменение не упомянуто в описании PR #322, которое посвящено terminal copy interaction. В соответствии с принципом spec-driven development, каждое изменение поведения должно быть явно задокументировано в спецификации PR или связанном issue.
📝 Предлагаемые функциональные комментарии
+ // CHANGE: grokAuthEntries и grokAuthPath стали опциональными с дефолтами
+ // WHY: Backward compatibility — контроллеры без Grok-провайдера не отправляют эти поля
+ // QUOTE(ТЗ): n/a (уточните в описании PR или issue `#317`)
+ // REF: PR `#322` commit d66d8f2
+ // INVARIANT: ∀ snapshot without grok fields: grokAuthEntries = 0 ∧ grokAuthPath = ""
+ // PURITY: CORE
grokAuthEntries: Schema.optionalWith(Schema.Number, { default: () => 0 }),
grokAuthPath: Schema.optionalWith(Schema.String, { default: () => "" }),Согласно coding guidelines: "Use functional comment markers for code clarity: CHANGE (brief description), WHY (mathematical/architectural justification), QUOTE(ТЗ) (requirement citation), REF (RTM or message ID)".
🤖 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/src/web/api-auth-schema.ts` around lines 31 - 32, Добавьте
функциональные комментарии рядом с определениями grokAuthEntries и grokAuthPath
(где используется Schema.optionalWith) используя маркеры CHANGE, WHY, QUOTE/REF
и INVARIANT: в CHANGE кратко опишите внесённое поле и значение по умолчанию; в
WHY объясните почему дефолты (0 и "") сохраняют backward compatibility; в QUOTE
или REF укажите связанное требование/issue/PR; в INVARIANT зафиксируйте гарантию
«при отсутствии поля применяется default» и типы (Number/String). Обновите
описание PR `#322` или добавьте ссылку на соответствующее issue в QUOTE/REF, чтобы
изменение схемы было задокументировано в спецификации.
| const decodeAuthSnapshotResponse = (payload: LegacyAuthSnapshotResponse) => | ||
| ParseResult.decodeUnknownEither(Schema.parseJson(AuthSnapshotResponseSchema))(JSON.stringify(payload)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Добавьте TSDoc для helper-функции.
Согласно guidelines, все функции должны иметь comprehensive TSDoc, включая описание параметров, возвращаемого типа, и @pure маркер. Для этой helper-функции также полезно документировать, что она возвращает Either для тестирования как успешных, так и неуспешных случаев декодирования.
📝 Предлагаемый TSDoc
+/**
+ * Декодирует legacy auth snapshot response через JSON-парсер схемы.
+ * `@param` payload - Legacy payload без Grok-полей
+ * `@returns` Either с успешным декодированием или ошибкой ParseResult
+ * `@pure` true
+ * `@complexity` O(n) где n — размер payload
+ */
const decodeAuthSnapshotResponse = (payload: LegacyAuthSnapshotResponse) =>
ParseResult.decodeUnknownEither(Schema.parseJson(AuthSnapshotResponseSchema))(JSON.stringify(payload))Согласно coding guidelines: "Document all functions with comprehensive TSDoc including: @pure (true/false), @effect (required services), @invariant (mathematical invariants), @precondition, @postcondition, @complexity (time and space)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const decodeAuthSnapshotResponse = (payload: LegacyAuthSnapshotResponse) => | |
| ParseResult.decodeUnknownEither(Schema.parseJson(AuthSnapshotResponseSchema))(JSON.stringify(payload)) | |
| /** | |
| * Декодирует legacy auth snapshot response через JSON-парсер схемы. | |
| * `@param` payload - Legacy payload без Grok-полей | |
| * `@returns` Either с успешным декодированием или ошибкой ParseResult | |
| * `@pure` true | |
| * `@complexity` O(n) где n — размер payload | |
| */ | |
| const decodeAuthSnapshotResponse = (payload: LegacyAuthSnapshotResponse) => | |
| ParseResult.decodeUnknownEither(Schema.parseJson(AuthSnapshotResponseSchema))(JSON.stringify(payload)) |
🤖 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/api-auth-schema.test.ts` around lines 22 - 23,
Add comprehensive TSDoc to the helper function decodeAuthSnapshotResponse:
describe its purpose (decodes a LegacyAuthSnapshotResponse via
AuthSnapshotResponseSchema and ParseResult.decodeUnknownEither), document the
payload parameter and the fact the function returns an Either that represents
both successful and failed decodes (use the precise return type in the comment),
and include required tags: `@pure` true, `@effect` none (or list any required
services if applicable), `@invariant` (if any decoding invariants apply),
`@precondition` (expected shape of payload), `@postcondition` (Either contains
decoded value or decode errors), and `@complexity` (time/space). Reference the
function name decodeAuthSnapshotResponse and the schema symbol
AuthSnapshotResponseSchema so the maintainer can locate and verify the doc.
| describe("web auth api schema", () => { | ||
| it("accepts auth snapshots from controllers without Grok fields", () => { | ||
| const decoded = decodeAuthSnapshotResponse({ | ||
| snapshot: { | ||
| claudeAuthEntries: 3, | ||
| claudeAuthPath: "/home/dev/.docker-git/.orch/auth/claude", | ||
| geminiAuthEntries: 2, | ||
| geminiAuthPath: "/home/dev/.docker-git/.orch/auth/gemini", | ||
| gitTokenEntries: 0, | ||
| gitUserEntries: 0, | ||
| githubTokenEntries: 1, | ||
| globalEnvPath: "/home/dev/.docker-git/.orch/env/global.env", | ||
| totalEntries: 1 | ||
| } | ||
| }) | ||
|
|
||
| expect(Either.isRight(decoded)).toBe(true) | ||
| if (Either.isRight(decoded)) { | ||
| expect(decoded.right.snapshot.grokAuthEntries).toBe(0) | ||
| expect(decoded.right.snapshot.grokAuthPath).toBe("") | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Неполное покрытие тестами: отсутствует кейс с Grok-полями.
Тест проверяет только legacy-случай (snapshot без Grok-полей), но не верифицирует, что схема корректно декодирует современные snapshots с присутствующими grokAuthEntries и grokAuthPath. Необходимо добавить тест, который проверяет, что явно переданные значения не перезаписываются дефолтами.
🧪 Предлагаемый дополнительный тест
it("preserves explicit Grok fields when present", () => {
const decoded = ParseResult.decodeUnknownEither(
Schema.parseJson(AuthSnapshotResponseSchema)
)(JSON.stringify({
snapshot: {
claudeAuthEntries: 3,
claudeAuthPath: "/home/dev/.docker-git/.orch/auth/claude",
geminiAuthEntries: 2,
geminiAuthPath: "/home/dev/.docker-git/.orch/auth/gemini",
grokAuthEntries: 5,
grokAuthPath: "/home/dev/.docker-git/.orch/auth/grok",
gitTokenEntries: 0,
gitUserEntries: 0,
githubTokenEntries: 1,
globalEnvPath: "/home/dev/.docker-git/.orch/env/global.env",
totalEntries: 1
}
}))
expect(Either.isRight(decoded)).toBe(true)
if (Either.isRight(decoded)) {
expect(decoded.right.snapshot.grokAuthEntries).toBe(5)
expect(decoded.right.snapshot.grokAuthPath).toBe("/home/dev/.docker-git/.orch/auth/grok")
}
})Согласно coding guidelines для property-based testing: хотя здесь можно было бы использовать fast-check для генерации различных комбинаций полей, минимально необходимо покрыть оба основных сценария (legacy и modern).
🤖 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/api-auth-schema.test.ts` around lines 25 - 47,
Add a new test case in packages/app/tests/docker-git/api-auth-schema.test.ts
that supplies a snapshot object including grokAuthEntries and grokAuthPath and
asserts those values are preserved (not defaulted); use the same test structure
as the existing case but call the decoder (either decodeAuthSnapshotResponse or
ParseResult.decodeUnknownEither(Schema.parseJson(AuthSnapshotResponseSchema)))
on a JSON string that includes grokAuthEntries: 5 and grokAuthPath:
"/home/dev/.docker-git/.orch/auth/grok", then assert Either.isRight(decoded) and
that decoded.right.snapshot.grokAuthEntries === 5 and
decoded.right.snapshot.grokAuthPath === "/home/dev/.docker-git/.orch/auth/grok".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/app/tests/docker-git/terminal-panel-runtime-core.test.ts (1)
64-110: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winДобавьте property-based проверки инвариантов, а не только фиксированные примеры.
На Line 64-110 проверяются ключевые инварианты (mouse-report detection, scroll/send, paste-guard), но только на нескольких заранее выбранных строках. Для устойчивой верификации лучше добавить
fast-checkсценарии (fc.property) с генераторами строк/escape-последовательностей и инвариантными assertions.Пример направления
+import fc from "fast-check" + +it("соблюдает инвариант: mouse report не вызывает scrollToBottom", () => { + fc.assert( + fc.property( + fc.constantFrom("\u001B[M !!", "\u001B[<64;10;5M", "\u001B[<0;10;5m", "\u001B[64;10;5M"), + (data) => { + const input = createTerminalInputHarness() + const { sent, socketRef } = createOpenSocketRef() + attachTerminalInput(input.terminal, socketRef, passThroughPasteGuard) + input.emit(data) + expect(input.state.scrolls).toBe(0) + expect(sent).toEqual([JSON.stringify({ data, type: "input" })]) + } + ) + ) +})As per coding guidelines,
**/*.test.{ts,tsx}: 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-panel-runtime-core.test.ts` around lines 64 - 110, Add property-based tests using fast-check to exercise the terminal invariants instead of only fixed examples: write fc.property cases that generate arbitrary strings/escape sequences and assert that isTerminalMouseReportInput(input) matches the expected predicate-based classification (use the same detection logic to build expected labels), and separate properties that feed generated inputs into attachTerminalInput (via createTerminalInputHarness + createOpenSocketRef and passThroughPasteGuard) to assert invariants — e.g., for inputs classified as mouse-report the harness.state.scrolls stays 0 and socket sends that exact input, for non-mouse inputs the harness scrolls once and sends the input, and for a paste-guard property (use a pasteGuard with shouldSuppressTerminalInput => true) assert no scroll and no sends; reference isTerminalMouseReportInput, attachTerminalInput, createTerminalInputHarness, createOpenSocketRef, passThroughPasteGuard and use fc.string / custom generators for escape sequences to implement these properties.
🤖 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/terminal-wheel-scroll.test.ts`:
- Around line 116-219: Add a fast-check property test for
resolveTerminalWheelScrollDelta to validate invariants across wide inputs:
generate deltaMode (0,1,2), finite deltaY (non-NaN), previousPixelDeltaY (>=0),
and rows (>=0), call resolveTerminalWheelScrollDelta and assert (1)
returned.lines is an integer and when deltaMode===2 and rows>0 lines equals
rows*sign(deltaY) (or 1 when rows===0), (2) nextPixelDeltaY is a finite number
in the expected residual range (non-negative and less than the pixel-per-line
threshold used by resolveTerminalWheelScrollDelta), and (3) accumulation
property: if you call resolveTerminalWheelScrollDelta twice feeding
second.previousPixelDeltaY = first.nextPixelDeltaY the sum of lines equals the
result of a single call with previousPixelDeltaY=original and deltaY equal to
the two deltaYs summed; add this property test to the same suite
(terminal-wheel-scroll.test.ts) alongside existing example tests and import
fast-check utilities and resolveTerminalWheelScrollDelta symbol.
---
Duplicate comments:
In `@packages/app/tests/docker-git/terminal-panel-runtime-core.test.ts`:
- Around line 64-110: Add property-based tests using fast-check to exercise the
terminal invariants instead of only fixed examples: write fc.property cases that
generate arbitrary strings/escape sequences and assert that
isTerminalMouseReportInput(input) matches the expected predicate-based
classification (use the same detection logic to build expected labels), and
separate properties that feed generated inputs into attachTerminalInput (via
createTerminalInputHarness + createOpenSocketRef and passThroughPasteGuard) to
assert invariants — e.g., for inputs classified as mouse-report the
harness.state.scrolls stays 0 and socket sends that exact input, for non-mouse
inputs the harness scrolls once and sends the input, and for a paste-guard
property (use a pasteGuard with shouldSuppressTerminalInput => true) assert no
scroll and no sends; reference isTerminalMouseReportInput, attachTerminalInput,
createTerminalInputHarness, createOpenSocketRef, passThroughPasteGuard and use
fc.string / custom generators for escape sequences to implement these
properties.
🪄 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: 3b45816f-c447-49dc-82e4-76ebdabc998b
📒 Files selected for processing (4)
packages/app/src/web/terminal-panel-runtime.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.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 (OpenCode)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Runtime volumes + SSH)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: E2E (Browser command)
- 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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-panel-runtime.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-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-panel-runtime.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-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-panel-runtime.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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-panel-runtime.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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-panel-runtime.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-panel-runtime.ts
🔇 Additional comments (1)
packages/app/tests/docker-git/terminal-panel-runtime-core.test.ts (1)
1-63: LGTM!Also applies to: 111-112
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
packages/app/tests/docker-git/terminal-panel-runtime-core.test.ts (1)
64-110: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftProperty-based тесты для инвариантов всё ещё отсутствуют (повторное замечание).
Как было отмечено в предыдущем ревью, текущие тесты покрывают только фиксированные примеры. Согласно guideline для
**/*.test.{ts,tsx}, требуется реализация property-based тестирования через fast-check для проверки математических инвариантов:Инварианты, которые должны быть проверены:
- Детекция mouse report: ∀x: pattern(x) → isTerminalMouseReportInput(x) = true
- Корреляция скролла: ∀x: isTerminalMouseReportInput(x) ⇒ scrolls = 0 ∧ ¬isTerminalMouseReportInput(x) ∧ ¬suppressed(x) ⇒ scrolls = 1
- Гарантии подавления: ∀x: suppressed(x) ⇒ scrolls = 0 ∧ sent = []
Пример property-based теста для инварианта скролла
import fc from "fast-check" it("сохраняет инвариант скролла: mouse reports не вызывают scrollToBottom", () => { fc.assert( fc.property( fc.oneof( fc.constant("\u001B[M "), fc.record({ button: fc.integer({ min: 0, max: 255 }), x: fc.integer({ min: 1, max: 223 }), y: fc.integer({ min: 1, max: 223 }) }).map(({ button, x, y }) => `\u001B[<${button};${x};${y}M`) ), (mouseReport) => { const input = createTerminalInputHarness() const { socketRef } = createOpenSocketRef() attachTerminalInput(input.terminal, socketRef, passThroughPasteGuard) input.emit(mouseReport) expect(input.state.scrolls).toBe(0) } ) ) }) it("сохраняет инвариант скролла: не-mouse ввод всегда вызывает scrollToBottom", () => { fc.assert( fc.property( fc.string().filter((s) => !isTerminalMouseReportInput(s) && s.length > 0), (regularInput) => { const input = createTerminalInputHarness() const { socketRef } = createOpenSocketRef() attachTerminalInput(input.terminal, socketRef, passThroughPasteGuard) input.emit(regularInput) expect(input.state.scrolls).toBe(1) } ) ) })As per coding guidelines:
**/*.test.{ts,tsx}: 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-panel-runtime-core.test.ts` around lines 64 - 110, Add property-based tests using fast-check to verify the three invariants: use isTerminalMouseReportInput, attachTerminalInput, createTerminalInputHarness, createOpenSocketRef and passThroughPasteGuard to drive inputs; for the mouse-report invariant, generate CSI mouse sequences (e.g. map records to `\u001B[<${button};${x};${y}M` and include `\u001B[M...` constants) and assert isTerminalMouseReportInput(x) is true; for the scroll correlation, fc.property over strings filtered to non-mouse non-empty inputs to assert input.state.scrolls === 1 after emit, and over mouse reports to assert scrolls === 0; for suppression, supply a pasteGuard with shouldSuppressTerminalInput returning true, generate arbitrary strings, emit them and assert scrolls === 0 and no socket sends; use fc.assert(fc.property(...)) and the existing createOpenSocketRef to observe sent messages.
🤖 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/src/web/terminal-panel-runtime.ts`:
- Around line 193-197: Refactor duplicate logic by extracting a shared predicate
isBrowserTerminalSession(session: TerminalLifecycleArgs["session"]) that returns
session.browserProjectId !== undefined, then update
shouldAllowTerminalMouseTracking and shouldSuppressTerminalAlternateScreen to
call isBrowserTerminalSession(session) so intent is explicit and future changes
to "browser-based" detection are centralized.
- Around line 196-197: Add a comprehensive TSDoc block immediately above the
shouldSuppressTerminalAlternateScreen function that describes what the function
does, documents the parameter "session: TerminalLifecycleArgs['session']" and
the boolean return value, and includes the required tags: `@pure`, `@effect` (or
`@effect` dependencies) listing any external influences (none if none), `@invariant`
describing the logical invariant, `@precondition` (e.g., session is defined or
shape expectations), `@postcondition` (what true/false means), and `@complexity`
(use O(1)); ensure the text is concise and follows the repo’s TSDoc style
guidelines.
In `@packages/app/src/web/terminal-wheel-scroll.ts`:
- Around line 121-152: The attachTerminalWheelScroll function currently performs
DOM side effects and holds mutable closure state (previousPixelDeltaY) directly;
refactor it into an Effect-TS service: create a Context.Tag/Service (e.g.,
TerminalWheelScrollService) exposing attach: (args: TerminalWheelScrollArgs) =>
Effect<never, never, void>, and implement attach using Effect.acquireRelease to
register the wheel listener (the same onWheel logic but moved into the
Effect.sync acquire) and to remove it in the release, then expose the service
via a Layer (makeTerminalWheelScrollService) and call
Effect.scoped/Effect.asVoid so all resource acquisition/finalization is typed
and composable instead of using attachTerminalWheelScroll,
target.addEventListener, previousPixelDeltaY, and the returned dispose closure.
- Around line 61-119: Add the required functional comment blocks per coding
guidelines above the constant group (wheelPixelDeltaMode, wheelLineDeltaMode,
wheelPageDeltaMode, pixelsPerTerminalLine) and above each exported/important
function (shouldHandleTerminalWheelScroll, resolveTerminalWheelScrollTarget,
resolveTerminalWheelScrollDelta, validTerminalRows, finiteDelta and the three
predicate helpers) including the markers: CHANGE, WHY, QUOTE(ТЗ) or n/a, REF,
SOURCE or n/a, FORMAT THEOREM (formal property where applicable), PURITY
(CORE|SHELL), EFFECT signature for any SHELL functions, INVARIANT (mathematical
invariant e.g. bounds for nextPixelDeltaY) and COMPLEXITY (time/space); keep the
comments concise, reference the relevant symbol names in the comment, and place
the FORMAT THEOREM and INVARIANT that precisely describe
resolveTerminalWheelScrollDelta’s accumulation invariant and
pixelsPerTerminalLine relationship.
- Around line 1-152: The three exported functions lack the required
comprehensive TSDoc; add complete comments for shouldHandleTerminalWheelScroll,
resolveTerminalWheelScrollDelta, and attachTerminalWheelScroll that document
parameters and return types and include the mandated tags: `@pure` (or `@effect` if
side-effects), `@effect` dependencies (if any), `@invariant`(s) (mathematical
relationships such as pixel/line invariants), `@precondition` and `@postcondition`,
and `@complexity` (Big-O time/space); place each TSDoc immediately above its
export, mirror the example in the review (brief description, examples, and the
specified tags), and ensure the invariants reflect the logic in
resolveTerminalWheelScrollDelta (pixel accumulation, line/page/line-mode
behavior) and that attachTerminalWheelScroll notes its event side-effects and
disposal behavior.
- Line 64: Добавьте развёрнутый комментарий над константой
pixelsPerTerminalLine, объясняющий откуда взято значение 15 (например, что это
эмпирическое значение, согласованное с высотой строки в используемой версии
xterm.js при стандартных шрифтах) и указать последствия (что значение будет
некорректно при изменении шрифта/масштаба) и рекомендовать альтернативы (делать
значение настраиваемым или вычислять динамически через API терминала). Ссылка в
комментарии должна упомянуть имя константы pixelsPerTerminalLine и, если
использовалось, источник измерений или версия xterm.js, чтобы другие
разработчики могли при необходимости пересчитать или заменить константу.
- Around line 100-119: Add a clear doc comment above
resolveTerminalWheelScrollDelta describing the pixel-mode invariant: that
finiteDelta(previousPixelDeltaY) + finiteDelta(delta.deltaY) is decomposed
exactly into lines * pixelsPerTerminalLine + nextPixelDeltaY (with
nextPixelDeltaY in the residual range), and explicitly note that Math.trunc is
used (rounds toward zero) so negative accumulations produce negative lines and a
negative nextPixelDeltaY residual (showing the -22 => lines = -1,
nextPixelDeltaY = -7 example). Reference the symbols previousPixelDeltaY,
deltaY, lines, nextPixelDeltaY, and pixelsPerTerminalLine and state the expected
residual range and intent for future maintainers; keep the comment concise and
place it immediately above the resolveTerminalWheelScrollDelta function.
In `@packages/app/tests/docker-git/terminal-panel-runtime-core.test.ts`:
- Around line 65-68: The tests using isTerminalMouseReportInput contain several
raw escape sequences that are hard to read; add concise inline comments next to
each assertion explaining what each escape sequence represents (e.g., CSI mouse
report formats, SGR vs X10/Xterm styles, and which char denotes press/release),
and apply the same commenting style to the other occurrence around line 106;
reference the isTerminalMouseReportInput assertions to locate where to add the
explanatory comments.
- Line 42: Replace the magic number for socket ready state with the
WebSocket.OPEN constant: in the test where the mock socket uses readyState: 1
inside the send: (data: string) => { ... } block, change it to readyState:
WebSocket.OPEN and ensure the test's beforeEach mock of WebSocket includes the
OPEN property (it already stubs { OPEN: 1 }); update any other readyState checks
in the same test to use WebSocket.OPEN for clarity.
In `@packages/app/tests/docker-git/terminal-query-suppression.test.ts`:
- Around line 255-265: Add a new test case alongside the existing one that calls
installTerminalQuerySuppression(mock.terminal, { suppressAlternateScreen: true
}) (omit or set allowMouseTracking to false) and then uses
privateModeHandlers(mock) with expectPrivateModesHandled(handlers,
ALTERNATE_SCREEN_MODES, true) and expectPrivateModesHandled(handlers,
MOUSE_TRACKING_MODES, false) to verify alternate screen suppression works
independently of mouse-tracking configuration; reference
installTerminalQuerySuppression, privateModeHandlers, ALTERNATE_SCREEN_MODES,
and MOUSE_TRACKING_MODES to locate where to add the test.
In `@packages/app/tests/docker-git/terminal-wheel-scroll.test.ts`:
- Around line 151-284: Tests directly create mocks and run imperative setup
instead of using Effect test utilities; update the failing/specified tests
(those using createWheelHost, createTerminal, attachTerminalWheelScroll,
createWheelEvent, dispatchWheel) to use Effect-based patterns: convert relevant
it(...) blocks to it.effect(...) and wrap the test body in
Effect.gen/Effect.sync so mocks and setup/teardown are created inside an Effect,
return the assertions' data from the Effect and assert on that result; ensure
you still dispose the disposable inside the Effect and avoid requiring the
Effect runtime for the whole test suite by using Effect's testing helpers per
guidelines.
---
Duplicate comments:
In `@packages/app/tests/docker-git/terminal-panel-runtime-core.test.ts`:
- Around line 64-110: Add property-based tests using fast-check to verify the
three invariants: use isTerminalMouseReportInput, attachTerminalInput,
createTerminalInputHarness, createOpenSocketRef and passThroughPasteGuard to
drive inputs; for the mouse-report invariant, generate CSI mouse sequences (e.g.
map records to `\u001B[<${button};${x};${y}M` and include `\u001B[M...`
constants) and assert isTerminalMouseReportInput(x) is true; for the scroll
correlation, fc.property over strings filtered to non-mouse non-empty inputs to
assert input.state.scrolls === 1 after emit, and over mouse reports to assert
scrolls === 0; for suppression, supply a pasteGuard with
shouldSuppressTerminalInput returning true, generate arbitrary strings, emit
them and assert scrolls === 0 and no socket sends; use
fc.assert(fc.property(...)) and the existing createOpenSocketRef to observe sent
messages.
🪄 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: 2e6e1fe0-ffed-4dc2-8e2f-855208cb0410
📒 Files selected for processing (6)
packages/app/src/web/terminal-panel-runtime.tspackages/app/src/web/terminal-query-suppression.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.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). (8)
- GitHub Check: E2E (Clone auto-open SSH)
- GitHub Check: Lint
- GitHub Check: E2E (Login context)
- GitHub Check: E2E (Clone cache)
- GitHub Check: E2E (OpenCode)
- GitHub Check: E2E (Browser command)
- GitHub Check: E2E (Runtime volumes + SSH)
- 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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-query-suppression.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-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-query-suppression.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-panel-runtime-core.test.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-query-suppression.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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-query-suppression.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/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-query-suppression.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/app/tests/docker-git/terminal-panel-runtime-core.test.tspackages/app/src/web/terminal-panel-runtime.tspackages/app/tests/docker-git/terminal-query-suppression.test.tspackages/app/tests/docker-git/terminal-wheel-scroll.test.tspackages/app/src/web/terminal-wheel-scroll.tspackages/app/src/web/terminal-query-suppression.ts
🔇 Additional comments (11)
packages/app/src/web/terminal-query-suppression.ts (4)
3-6: LGTM!
48-48: LGTM!Also applies to: 57-57
76-82: LGTM!
170-173: LGTM!packages/app/tests/docker-git/terminal-query-suppression.test.ts (4)
130-134: LGTM!
136-153: LGTM!
267-271: LGTM!
281-289: LGTM!packages/app/tests/docker-git/terminal-wheel-scroll.test.ts (1)
151-284: LGTM!packages/app/src/web/terminal-panel-runtime.ts (2)
27-28: LGTM!Also applies to: 43-51, 130-140, 176-181
208-213: Интеграция suppressAlternateScreen верифицирована и корректна.Поле
suppressAlternateScreenправильно встроено в стек:
- ✅
TerminalRuntimeOptionsсодержитquerySuppression?: TerminalQuerySuppressionOptions- ✅
installTerminalQuerySuppressionпринимает и передаёт опции в логику подавления- ✅
suppressAlternateScreen === trueблокирует alternate screen escape sequences черезALTERNATE_SCREEN_PRIVATE_MODESИспользование в строках 208-213 корректно.
| const shouldAllowTerminalMouseTracking = (session: TerminalLifecycleArgs["session"]): boolean => | ||
| session.browserProjectId !== undefined | ||
|
|
||
| const shouldSuppressTerminalAlternateScreen = (session: TerminalLifecycleArgs["session"]): boolean => | ||
| session.browserProjectId !== undefined |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Нарушение DRY: дублирование логики с shouldAllowTerminalMouseTracking.
Функции shouldAllowTerminalMouseTracking (строка 193) и shouldSuppressTerminalAlternateScreen (строка 196) имеют идентичную реализацию: обе возвращают session.browserProjectId !== undefined. Это нарушает принцип DRY и затрудняет будущие изменения логики определения browser-based проектов.
Рассмотрите возможность введения базовой функции isBrowserTerminalSession и использования её в обеих функциях для явного выражения намерения.
♻️ Предлагаемый рефакторинг
+/**
+ * Determines whether terminal session is running in browser-based project context.
+ *
+ * `@param` session - Terminal session configuration
+ * `@returns` true if session has an associated browser project identifier
+ *
+ * `@pure` true
+ * `@complexity` O(1)
+ */
+const isBrowserTerminalSession = (session: TerminalLifecycleArgs["session"]): boolean =>
+ session.browserProjectId !== undefined
+
+/**
+ * Determines whether mouse tracking should be allowed for this terminal session.
+ *
+ * `@remarks` Browser project terminals allow mouse tracking for interactive applications.
+ */
const shouldAllowTerminalMouseTracking = (session: TerminalLifecycleArgs["session"]): boolean =>
- session.browserProjectId !== undefined
+ isBrowserTerminalSession(session)
+/**
+ * Determines whether alternate screen should be suppressed for this terminal session.
+ *
+ * `@remarks` Browser project terminals suppress alternate screen to preserve scrollback history.
+ */
const shouldSuppressTerminalAlternateScreen = (session: TerminalLifecycleArgs["session"]): boolean =>
- session.browserProjectId !== undefined
+ isBrowserTerminalSession(session)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shouldAllowTerminalMouseTracking = (session: TerminalLifecycleArgs["session"]): boolean => | |
| session.browserProjectId !== undefined | |
| const shouldSuppressTerminalAlternateScreen = (session: TerminalLifecycleArgs["session"]): boolean => | |
| session.browserProjectId !== undefined | |
| /** | |
| * Determines whether terminal session is running in browser-based project context. | |
| * | |
| * `@param` session - Terminal session configuration | |
| * `@returns` true if session has an associated browser project identifier | |
| * | |
| * `@pure` true | |
| * `@complexity` O(1) | |
| */ | |
| const isBrowserTerminalSession = (session: TerminalLifecycleArgs["session"]): boolean => | |
| session.browserProjectId !== undefined | |
| /** | |
| * Determines whether mouse tracking should be allowed for this terminal session. | |
| * | |
| * `@remarks` Browser project terminals allow mouse tracking for interactive applications. | |
| */ | |
| const shouldAllowTerminalMouseTracking = (session: TerminalLifecycleArgs["session"]): boolean => | |
| isBrowserTerminalSession(session) | |
| /** | |
| * Determines whether alternate screen should be suppressed for this terminal session. | |
| * | |
| * `@remarks` Browser project terminals suppress alternate screen to preserve scrollback history. | |
| */ | |
| const shouldSuppressTerminalAlternateScreen = (session: TerminalLifecycleArgs["session"]): boolean => | |
| isBrowserTerminalSession(session) |
🤖 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/src/web/terminal-panel-runtime.ts` around lines 193 - 197,
Refactor duplicate logic by extracting a shared predicate
isBrowserTerminalSession(session: TerminalLifecycleArgs["session"]) that returns
session.browserProjectId !== undefined, then update
shouldAllowTerminalMouseTracking and shouldSuppressTerminalAlternateScreen to
call isBrowserTerminalSession(session) so intent is explicit and future changes
to "browser-based" detection are centralized.
| const shouldSuppressTerminalAlternateScreen = (session: TerminalLifecycleArgs["session"]): boolean => | ||
| session.browserProjectId !== undefined |
There was a problem hiding this comment.
Отсутствует TSDoc для новой функции.
Функция shouldSuppressTerminalAlternateScreen добавлена без TSDoc комментариев. Согласно coding guidelines, все функции должны иметь comprehensive TSDoc с описанием параметров, возвращаемого значения, маркером @pure, и @complexity.
📝 Предлагаемое исправление
+/**
+ * Determines whether alternate screen escape sequences should be suppressed for this terminal session.
+ *
+ * `@param` session - Terminal session configuration
+ * `@returns` true if alternate screen mode should be disabled (suppressed) for browser-based projects
+ *
+ * `@pure` true
+ * `@complexity` O(1) time, O(1) space
+ *
+ * `@remarks`
+ * Alternate screen suppression prevents applications like vim/less from switching buffers,
+ * keeping all output in the scrollback history for browser project terminals.
+ */
const shouldSuppressTerminalAlternateScreen = (session: TerminalLifecycleArgs["session"]): boolean =>
session.browserProjectId !== undefinedAs per coding guidelines, **/*.{ts,tsx}: TypeScript functions must include comprehensive TSDoc with parameters, return types, @pure marker, @effect dependencies, @invariant (mathematical), @precondition, @postcondition, and @complexity O-notation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shouldSuppressTerminalAlternateScreen = (session: TerminalLifecycleArgs["session"]): boolean => | |
| session.browserProjectId !== undefined | |
| /** | |
| * Determines whether alternate screen escape sequences should be suppressed for this terminal session. | |
| * | |
| * `@param` session - Terminal session configuration | |
| * `@returns` true if alternate screen mode should be disabled (suppressed) for browser-based projects | |
| * | |
| * `@pure` true | |
| * `@complexity` O(1) time, O(1) space | |
| * | |
| * `@remarks` | |
| * Alternate screen suppression prevents applications like vim/less from switching buffers, | |
| * keeping all output in the scrollback history for browser project terminals. | |
| */ | |
| const shouldSuppressTerminalAlternateScreen = (session: TerminalLifecycleArgs["session"]): boolean => | |
| session.browserProjectId !== undefined |
🤖 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/src/web/terminal-panel-runtime.ts` around lines 196 - 197, Add a
comprehensive TSDoc block immediately above the
shouldSuppressTerminalAlternateScreen function that describes what the function
does, documents the parameter "session: TerminalLifecycleArgs['session']" and
the boolean return value, and includes the required tags: `@pure`, `@effect` (or
`@effect` dependencies) listing any external influences (none if none), `@invariant`
describing the logical invariant, `@precondition` (e.g., session is defined or
shape expectations), `@postcondition` (what true/false means), and `@complexity`
(use O(1)); ensure the text is concise and follows the repo’s TSDoc style
guidelines.
| export type TerminalWheelMouseTrackingMode = "any" | "drag" | "none" | "vt200" | "x10" | ||
|
|
||
| export type TerminalWheelScrollBufferType = "alternate" | "normal" | ||
|
|
||
| export type TerminalWheelScrollBuffer = { | ||
| readonly active: { | ||
| readonly baseY: number | ||
| readonly type: TerminalWheelScrollBufferType | ||
| readonly viewportY: number | ||
| } | ||
| } | ||
|
|
||
| export type TerminalWheelScrollTerminal = { | ||
| readonly buffer?: TerminalWheelScrollBuffer | undefined | ||
| readonly element?: TerminalWheelScrollTarget | null | undefined | ||
| readonly modes: { | ||
| readonly mouseTrackingMode: TerminalWheelMouseTrackingMode | ||
| } | ||
| readonly rows: number | ||
| readonly scrollLines: (amount: number) => void | ||
| } | ||
|
|
||
| type TerminalWheelScrollEvent = { | ||
| readonly deltaMode: number | ||
| readonly deltaY: number | ||
| readonly stopImmediatePropagation?: () => void | ||
| readonly preventDefault: () => void | ||
| readonly stopPropagation: () => void | ||
| } | ||
|
|
||
| type TerminalWheelScrollTarget = { | ||
| readonly addEventListener: ( | ||
| type: "wheel", | ||
| listener: (event: TerminalWheelScrollEvent) => void, | ||
| options: AddEventListenerOptions | ||
| ) => void | ||
| readonly removeEventListener: ( | ||
| type: "wheel", | ||
| listener: (event: TerminalWheelScrollEvent) => void, | ||
| options: boolean | ||
| ) => void | ||
| } | ||
|
|
||
| type TerminalWheelScrollDelta = { | ||
| readonly deltaMode: number | ||
| readonly deltaY: number | ||
| readonly previousPixelDeltaY: number | ||
| readonly rows: number | ||
| } | ||
|
|
||
| export type ResolvedTerminalWheelScrollDelta = { | ||
| readonly lines: number | ||
| readonly nextPixelDeltaY: number | ||
| } | ||
|
|
||
| type TerminalWheelScrollArgs = { | ||
| readonly host: TerminalWheelScrollTarget | ||
| readonly terminal: TerminalWheelScrollTerminal | ||
| } | ||
|
|
||
| const wheelPixelDeltaMode = 0 | ||
| const wheelLineDeltaMode = 1 | ||
| const wheelPageDeltaMode = 2 | ||
| const pixelsPerTerminalLine = 15 | ||
|
|
||
| const hasActiveMouseTracking = (terminal: TerminalWheelScrollTerminal): boolean => | ||
| terminal.modes.mouseTrackingMode !== "none" | ||
|
|
||
| const hasActiveAlternateBuffer = (terminal: TerminalWheelScrollTerminal): boolean => | ||
| terminal.buffer?.active.type === "alternate" | ||
|
|
||
| const hasScrollableTerminalHistory = (terminal: TerminalWheelScrollTerminal): boolean => { | ||
| const activeBuffer = terminal.buffer?.active | ||
| return activeBuffer !== undefined && activeBuffer.type === "normal" && activeBuffer.baseY > 0 | ||
| } | ||
|
|
||
| export const shouldHandleTerminalWheelScroll = (terminal: TerminalWheelScrollTerminal): boolean => | ||
| hasActiveMouseTracking(terminal) || | ||
| hasActiveAlternateBuffer(terminal) || | ||
| hasScrollableTerminalHistory(terminal) | ||
|
|
||
| const resolveTerminalWheelScrollTarget = ( | ||
| { host, terminal }: TerminalWheelScrollArgs | ||
| ): TerminalWheelScrollTarget => terminal.element ?? host | ||
|
|
||
| const validTerminalRows = (rows: number): number => { | ||
| if (!Number.isFinite(rows) || rows < 1) { | ||
| return 1 | ||
| } | ||
| return Math.trunc(rows) | ||
| } | ||
|
|
||
| const finiteDelta = (delta: number): number => { | ||
| if (!Number.isFinite(delta)) { | ||
| return 0 | ||
| } | ||
| return delta | ||
| } | ||
|
|
||
| export const resolveTerminalWheelScrollDelta = ( | ||
| delta: TerminalWheelScrollDelta | ||
| ): ResolvedTerminalWheelScrollDelta => { | ||
| const deltaY = finiteDelta(delta.deltaY) | ||
| if (delta.deltaMode === wheelLineDeltaMode) { | ||
| return { lines: Math.trunc(deltaY), nextPixelDeltaY: 0 } | ||
| } | ||
| if (delta.deltaMode === wheelPageDeltaMode) { | ||
| return { lines: Math.trunc(deltaY * validTerminalRows(delta.rows)), nextPixelDeltaY: 0 } | ||
| } | ||
| if (delta.deltaMode !== wheelPixelDeltaMode) { | ||
| return { lines: Math.trunc(deltaY), nextPixelDeltaY: 0 } | ||
| } | ||
| const nextPixelDeltaY = finiteDelta(delta.previousPixelDeltaY) + deltaY | ||
| const lines = Math.trunc(nextPixelDeltaY / pixelsPerTerminalLine) | ||
| return { | ||
| lines, | ||
| nextPixelDeltaY: nextPixelDeltaY - lines * pixelsPerTerminalLine | ||
| } | ||
| } | ||
|
|
||
| export const attachTerminalWheelScroll = ( | ||
| args: TerminalWheelScrollArgs | ||
| ): { readonly dispose: () => void } => { | ||
| let previousPixelDeltaY = 0 | ||
| const target = resolveTerminalWheelScrollTarget(args) | ||
| const onWheel = (event: TerminalWheelScrollEvent): void => { | ||
| if (!shouldHandleTerminalWheelScroll(args.terminal)) { | ||
| return | ||
| } | ||
| const scrollDelta = resolveTerminalWheelScrollDelta({ | ||
| deltaMode: event.deltaMode, | ||
| deltaY: event.deltaY, | ||
| previousPixelDeltaY, | ||
| rows: args.terminal.rows | ||
| }) | ||
| previousPixelDeltaY = scrollDelta.nextPixelDeltaY | ||
| event.preventDefault() | ||
| event.stopPropagation() | ||
| event.stopImmediatePropagation?.() | ||
| if (scrollDelta.lines !== 0) { | ||
| args.terminal.scrollLines(scrollDelta.lines) | ||
| } | ||
| } | ||
|
|
||
| target.addEventListener("wheel", onWheel, { capture: true, passive: false }) | ||
|
|
||
| return { | ||
| dispose: () => { | ||
| target.removeEventListener("wheel", onWheel, true) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Критическое нарушение: отсутствует TSDoc для всех функций.
Согласно coding guidelines, все функции должны включать comprehensive TSDoc с параметрами, возвращаемыми типами, маркером @pure, @effect зависимостями, @invariant (математические), @precondition, @postcondition и @complexity O-notation.
В текущем файле TSDoc полностью отсутствует для экспортируемых функций:
shouldHandleTerminalWheelScroll(строка 77)resolveTerminalWheelScrollDelta(строка 100)attachTerminalWheelScroll(строка 121)
📝 Пример требуемого TSDoc
/**
* Determines whether terminal should intercept wheel events for manual scroll handling.
*
* `@param` terminal - Terminal instance with buffer state and mouse tracking mode
* `@returns` true if wheel events should be captured and preventDefault called
*
* `@pure` true
* `@invariant` Returns true iff (mouseTracking active) OR (alternate buffer active) OR (normal buffer has scrollback history with baseY > 0)
* `@complexity` O(1) time, O(1) space
*
* `@example`
* ```typescript
* shouldHandleTerminalWheelScroll({ modes: { mouseTrackingMode: "any" }, rows: 24 }) // true
* shouldHandleTerminalWheelScroll({ modes: { mouseTrackingMode: "none" }, buffer: { active: { type: "normal", baseY: 0 } }, rows: 24 }) // false
* ```
*/
export const shouldHandleTerminalWheelScroll = (terminal: TerminalWheelScrollTerminal): boolean =>
// ...
/**
* Converts WheelEvent delta values into terminal scroll line count with pixel accumulation.
*
* `@param` delta - Wheel event delta mode, deltaY, accumulated pixel remainder, and terminal rows
* `@returns` Computed line count (integer) and next pixel remainder for accumulation
*
* `@pure` true
* `@invariant` For pixel mode: lines * pixelsPerTerminalLine + nextPixelDeltaY === previousPixelDeltaY + deltaY
* `@invariant` For line mode: lines === trunc(deltaY) and nextPixelDeltaY === 0
* `@invariant` For page mode: lines === trunc(deltaY * validRows) and nextPixelDeltaY === 0
* `@invariant` |nextPixelDeltaY| < pixelsPerTerminalLine
* `@complexity` O(1) time, O(1) space
*
* `@example`
* ```typescript
* resolveTerminalWheelScrollDelta({ deltaMode: 0, deltaY: 7, previousPixelDeltaY: 0, rows: 24 })
* // => { lines: 0, nextPixelDeltaY: 7 }
* ```
*/
export const resolveTerminalWheelScrollDelta = (
// ...As per coding guidelines, **/*.{ts,tsx}: TypeScript functions must include comprehensive TSDoc with parameters, return types, @pure marker, @effect dependencies, @invariant (mathematical), @precondition, @postcondition, and @complexity O-notation.
🤖 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/src/web/terminal-wheel-scroll.ts` around lines 1 - 152, The
three exported functions lack the required comprehensive TSDoc; add complete
comments for shouldHandleTerminalWheelScroll, resolveTerminalWheelScrollDelta,
and attachTerminalWheelScroll that document parameters and return types and
include the mandated tags: `@pure` (or `@effect` if side-effects), `@effect`
dependencies (if any), `@invariant`(s) (mathematical relationships such as
pixel/line invariants), `@precondition` and `@postcondition`, and `@complexity` (Big-O
time/space); place each TSDoc immediately above its export, mirror the example
in the review (brief description, examples, and the specified tags), and ensure
the invariants reflect the logic in resolveTerminalWheelScrollDelta (pixel
accumulation, line/page/line-mode behavior) and that attachTerminalWheelScroll
notes its event side-effects and disposal behavior.
| const wheelPixelDeltaMode = 0 | ||
| const wheelLineDeltaMode = 1 | ||
| const wheelPageDeltaMode = 2 | ||
| const pixelsPerTerminalLine = 15 | ||
|
|
||
| const hasActiveMouseTracking = (terminal: TerminalWheelScrollTerminal): boolean => | ||
| terminal.modes.mouseTrackingMode !== "none" | ||
|
|
||
| const hasActiveAlternateBuffer = (terminal: TerminalWheelScrollTerminal): boolean => | ||
| terminal.buffer?.active.type === "alternate" | ||
|
|
||
| const hasScrollableTerminalHistory = (terminal: TerminalWheelScrollTerminal): boolean => { | ||
| const activeBuffer = terminal.buffer?.active | ||
| return activeBuffer !== undefined && activeBuffer.type === "normal" && activeBuffer.baseY > 0 | ||
| } | ||
|
|
||
| export const shouldHandleTerminalWheelScroll = (terminal: TerminalWheelScrollTerminal): boolean => | ||
| hasActiveMouseTracking(terminal) || | ||
| hasActiveAlternateBuffer(terminal) || | ||
| hasScrollableTerminalHistory(terminal) | ||
|
|
||
| const resolveTerminalWheelScrollTarget = ( | ||
| { host, terminal }: TerminalWheelScrollArgs | ||
| ): TerminalWheelScrollTarget => terminal.element ?? host | ||
|
|
||
| const validTerminalRows = (rows: number): number => { | ||
| if (!Number.isFinite(rows) || rows < 1) { | ||
| return 1 | ||
| } | ||
| return Math.trunc(rows) | ||
| } | ||
|
|
||
| const finiteDelta = (delta: number): number => { | ||
| if (!Number.isFinite(delta)) { | ||
| return 0 | ||
| } | ||
| return delta | ||
| } | ||
|
|
||
| export const resolveTerminalWheelScrollDelta = ( | ||
| delta: TerminalWheelScrollDelta | ||
| ): ResolvedTerminalWheelScrollDelta => { | ||
| const deltaY = finiteDelta(delta.deltaY) | ||
| if (delta.deltaMode === wheelLineDeltaMode) { | ||
| return { lines: Math.trunc(deltaY), nextPixelDeltaY: 0 } | ||
| } | ||
| if (delta.deltaMode === wheelPageDeltaMode) { | ||
| return { lines: Math.trunc(deltaY * validTerminalRows(delta.rows)), nextPixelDeltaY: 0 } | ||
| } | ||
| if (delta.deltaMode !== wheelPixelDeltaMode) { | ||
| return { lines: Math.trunc(deltaY), nextPixelDeltaY: 0 } | ||
| } | ||
| const nextPixelDeltaY = finiteDelta(delta.previousPixelDeltaY) + deltaY | ||
| const lines = Math.trunc(nextPixelDeltaY / pixelsPerTerminalLine) | ||
| return { | ||
| lines, | ||
| nextPixelDeltaY: nextPixelDeltaY - lines * pixelsPerTerminalLine | ||
| } | ||
| } |
There was a problem hiding this comment.
Критическое нарушение: отсутствуют обязательные функциональные комментарии.
Согласно coding guidelines, код должен содержать функциональные маркеры: CHANGE (краткое описание), WHY (математическое/архитектурное обоснование), QUOTE(ТЗ) (ссылка на требование или n/a), FORMAT THEOREM (∀x ∈ Domain: P(x) → Q(f(x))), PURITY (CORE|SHELL), INVARIANT (математический инвариант), COMPLEXITY (time/space).
В текущем коде эти маркеры полностью отсутствуют.
📋 Пример требуемых комментариев
// CHANGE: Constants for wheel event deltaMode values and pixel-to-line conversion
// WHY: WheelEvent.deltaMode follows DOM_DELTA_* constants (0=pixel, 1=line, 2=page)
// QUOTE(ТЗ): n/a (follows DOM Level 3 Events Specification)
// SOURCE: https://www.w3.org/TR/uievents/#dom-wheelevent-deltamode
// PURITY: CORE
const wheelPixelDeltaMode = 0
const wheelLineDeltaMode = 1
const wheelPageDeltaMode = 2
const pixelsPerTerminalLine = 15 // WHY: Empirical value matching xterm.js default line height
// CHANGE: Converts wheel delta into terminal scroll lines with sub-line pixel accumulation
// WHY: Pixel-mode wheel events yield fractional lines; accumulate remainder across events for smooth scroll
// FORMAT THEOREM: ∀delta ∈ WheelDelta: lines * 15 + nextPixelDeltaY = previousPixelDeltaY + deltaY (when deltaMode=0)
// PURITY: CORE
// INVARIANT: |nextPixelDeltaY| < pixelsPerTerminalLine
// COMPLEXITY: O(1) time, O(1) space
export const resolveTerminalWheelScrollDelta = (
delta: TerminalWheelScrollDelta
): ResolvedTerminalWheelScrollDelta => {
const deltaY = finiteDelta(delta.deltaY)
if (delta.deltaMode === wheelLineDeltaMode) {
// CHANGE: Line mode: deltaY directly represents line count
return { lines: Math.trunc(deltaY), nextPixelDeltaY: 0 }
}
// ...As per coding guidelines, **/*.{ts,tsx,js,jsx}: 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.
🤖 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/src/web/terminal-wheel-scroll.ts` around lines 61 - 119, Add the
required functional comment blocks per coding guidelines above the constant
group (wheelPixelDeltaMode, wheelLineDeltaMode, wheelPageDeltaMode,
pixelsPerTerminalLine) and above each exported/important function
(shouldHandleTerminalWheelScroll, resolveTerminalWheelScrollTarget,
resolveTerminalWheelScrollDelta, validTerminalRows, finiteDelta and the three
predicate helpers) including the markers: CHANGE, WHY, QUOTE(ТЗ) or n/a, REF,
SOURCE or n/a, FORMAT THEOREM (formal property where applicable), PURITY
(CORE|SHELL), EFFECT signature for any SHELL functions, INVARIANT (mathematical
invariant e.g. bounds for nextPixelDeltaY) and COMPLEXITY (time/space); keep the
comments concise, reference the relevant symbol names in the comment, and place
the FORMAT THEOREM and INVARIANT that precisely describe
resolveTerminalWheelScrollDelta’s accumulation invariant and
pixelsPerTerminalLine relationship.
| const wheelPixelDeltaMode = 0 | ||
| const wheelLineDeltaMode = 1 | ||
| const wheelPageDeltaMode = 2 | ||
| const pixelsPerTerminalLine = 15 |
There was a problem hiding this comment.
Недокументированная магическая константа.
Значение pixelsPerTerminalLine = 15 не имеет объяснения откуда взято. Это эмпирическое значение, зависящее от реальной высоты строки в xterm.js? Или это фиксированное предположение? Если высота строки терминала изменится (например, через опции шрифта), останется ли это значение корректным?
Добавьте комментарий с обоснованием и источником значения.
📝 Предлагаемое исправление
const wheelPixelDeltaMode = 0
const wheelLineDeltaMode = 1
const wheelPageDeltaMode = 2
-const pixelsPerTerminalLine = 15
+// WHY: Approximate pixel height of one terminal line in xterm.js with default font size
+// SOURCE: Empirically matches xterm.js default rendering metrics
+// NOTE: If terminal font size is configurable, this should be derived from terminal.options or measured dynamically
+const pixelsPerTerminalLine = 15🤖 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/src/web/terminal-wheel-scroll.ts` at line 64, Добавьте
развёрнутый комментарий над константой pixelsPerTerminalLine, объясняющий откуда
взято значение 15 (например, что это эмпирическое значение, согласованное с
высотой строки в используемой версии xterm.js при стандартных шрифтах) и указать
последствия (что значение будет некорректно при изменении шрифта/масштаба) и
рекомендовать альтернативы (делать значение настраиваемым или вычислять
динамически через API терминала). Ссылка в комментарии должна упомянуть имя
константы pixelsPerTerminalLine и, если использовалось, источник измерений или
версия xterm.js, чтобы другие разработчики могли при необходимости пересчитать
или заменить константу.
| export const attachTerminalWheelScroll = ( | ||
| args: TerminalWheelScrollArgs | ||
| ): { readonly dispose: () => void } => { | ||
| let previousPixelDeltaY = 0 | ||
| const target = resolveTerminalWheelScrollTarget(args) | ||
| const onWheel = (event: TerminalWheelScrollEvent): void => { | ||
| if (!shouldHandleTerminalWheelScroll(args.terminal)) { | ||
| return | ||
| } | ||
| const scrollDelta = resolveTerminalWheelScrollDelta({ | ||
| deltaMode: event.deltaMode, | ||
| deltaY: event.deltaY, | ||
| previousPixelDeltaY, | ||
| rows: args.terminal.rows | ||
| }) | ||
| previousPixelDeltaY = scrollDelta.nextPixelDeltaY | ||
| event.preventDefault() | ||
| event.stopPropagation() | ||
| event.stopImmediatePropagation?.() | ||
| if (scrollDelta.lines !== 0) { | ||
| args.terminal.scrollLines(scrollDelta.lines) | ||
| } | ||
| } | ||
|
|
||
| target.addEventListener("wheel", onWheel, { capture: true, passive: false }) | ||
|
|
||
| return { | ||
| dispose: () => { | ||
| target.removeEventListener("wheel", onWheel, true) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Критическое нарушение: отсутствует интеграция с Effect-TS для side effects.
Функция attachTerminalWheelScroll выполняет side effects (регистрация обработчиков событий DOM, мутабельное состояние в замыкании на строке 124), но не использует Effect-TS паттерн. Согласно coding guidelines, все effects должны быть типизированы через Effect<Success, Error, Requirements> и композироваться через pipe() и Effect.flatMap(). Регистрация внешних ресурсов с финализацией должна использовать Effect.acquireRelease в сочетании с Effect.scoped.
Текущая реализация нарушает принцип разделения CORE/SHELL и делает невозможным тестирование без реальных side effects.
🔧 Рекомендуемая архитектура с Effect-TS
Определить Context.Tag для wheel scroll сервиса:
import { Context, Effect, Layer } from "effect"
export class TerminalWheelScrollService extends Context.Tag("TerminalWheelScrollService")<
TerminalWheelScrollService,
{
readonly attach: (args: TerminalWheelScrollArgs) => Effect.Effect<void, never, never>
}
>() {}Реализовать через Layer с acquireRelease:
export const makeTerminalWheelScrollService = Layer.succeed(
TerminalWheelScrollService,
{
attach: (args) =>
Effect.acquireRelease(
Effect.sync(() => {
let previousPixelDeltaY = 0
const target = resolveTerminalWheelScrollTarget(args)
const onWheel = (event: TerminalWheelScrollEvent): void => {
// ... existing logic
}
target.addEventListener("wheel", onWheel, { capture: true, passive: false })
return { target, onWheel }
}),
({ target, onWheel }) =>
Effect.sync(() => {
target.removeEventListener("wheel", onWheel, true)
})
).pipe(Effect.scoped, Effect.asVoid)
}
)As per coding guidelines, **/*.{ts,tsx}: All external dependencies must be wrapped through typed interfaces and injected via Effect-TS Layer pattern. Resources with finalization must use ONLY Effect.acquireRelease combined with Effect.scoped.
🤖 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/src/web/terminal-wheel-scroll.ts` around lines 121 - 152, The
attachTerminalWheelScroll function currently performs DOM side effects and holds
mutable closure state (previousPixelDeltaY) directly; refactor it into an
Effect-TS service: create a Context.Tag/Service (e.g.,
TerminalWheelScrollService) exposing attach: (args: TerminalWheelScrollArgs) =>
Effect<never, never, void>, and implement attach using Effect.acquireRelease to
register the wheel listener (the same onWheel logic but moved into the
Effect.sync acquire) and to remove it in the release, then expose the service
via a Layer (makeTerminalWheelScrollService) and call
Effect.scoped/Effect.asVoid so all resource acquisition/finalization is typed
and composable instead of using attachTerminalWheelScroll,
target.addEventListener, previousPixelDeltaY, and the returned dispose closure.
| socketRef: { | ||
| current: { | ||
| readyState: 1, | ||
| send: (data: string) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Опциональное улучшение: используйте константу WebSocket.OPEN вместо магического числа.
Значение readyState: 1 соответствует WebSocket.OPEN. Для явности можно использовать константу:
- readyState: 1,
+ readyState: WebSocket.OPEN,В контексте теста, где beforeEach заглушает WebSocket объектом { OPEN: 1 }, это улучшит читаемость.
🤖 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-panel-runtime-core.test.ts` at line
42, Replace the magic number for socket ready state with the WebSocket.OPEN
constant: in the test where the mock socket uses readyState: 1 inside the send:
(data: string) => { ... } block, change it to readyState: WebSocket.OPEN and
ensure the test's beforeEach mock of WebSocket includes the OPEN property (it
already stubs { OPEN: 1 }); update any other readyState checks in the same test
to use WebSocket.OPEN for clarity.
| expect(isTerminalMouseReportInput("\u001B[M !!")).toBe(true) | ||
| expect(isTerminalMouseReportInput("\u001B[<64;10;5M")).toBe(true) | ||
| expect(isTerminalMouseReportInput("\u001B[<0;10;5m")).toBe(true) | ||
| expect(isTerminalMouseReportInput("\u001B[64;10;5M")).toBe(true) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Опциональное улучшение: добавьте комментарии к управляющим последовательностям и escape-символам.
Для улучшения читаемости и поддерживаемости тестов рекомендуется добавить пояснительные комментарии к escape-последовательностям:
Примеры комментариев
it("detects xterm mouse report input encodings", () => {
+ // ESC[M followed by three bytes (legacy X10 mouse encoding)
expect(isTerminalMouseReportInput("\u001B[M !!")).toBe(true)
+ // ESC[<button;x;y M (SGR mouse press encoding)
expect(isTerminalMouseReportInput("\u001B[<64;10;5M")).toBe(true)
+ // ESC[<button;x;y m (SGR mouse release encoding)
expect(isTerminalMouseReportInput("\u001B[<0;10;5m")).toBe(true)
// ...
})
it("does not scroll or send input suppressed by the paste guard", () => {
// ...
+ // \u0016 is Ctrl+V (paste command)
input.emit("\u0016")
// ...
})Also applies to: 106-106
🤖 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-panel-runtime-core.test.ts` around
lines 65 - 68, The tests using isTerminalMouseReportInput contain several raw
escape sequences that are hard to read; add concise inline comments next to each
assertion explaining what each escape sequence represents (e.g., CSI mouse
report formats, SGR vs X10/Xterm styles, and which char denotes press/release),
and apply the same commenting style to the other occurrence around line 106;
reference the isTerminalMouseReportInput assertions to locate where to add the
explanatory comments.
| it("blocks alternate screen modes when project terminals preserve xterm scrollback", () => { | ||
| const mock = createMockTerminal() | ||
| installTerminalQuerySuppression(mock.terminal, { | ||
| allowMouseTracking: true, | ||
| suppressAlternateScreen: true | ||
| }) | ||
| const handlers = privateModeHandlers(mock) | ||
|
|
||
| expectPrivateModesHandled(handlers, ALTERNATE_SCREEN_MODES, true) | ||
| expectPrivateModesHandled(handlers, MOUSE_TRACKING_MODES, false) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Рассмотрите добавление теста для suppressAlternateScreen: true без allowMouseTracking.
Текущий тест комбинирует обе опции. Для полноты покрытия можно добавить отдельный кейс, проверяющий независимость опций — что alternate screen блокируется, а mouse tracking тоже блокируется (поведение по умолчанию).
♻️ Дополнительный тест-кейс
+ it("blocks alternate screen modes independently from mouse tracking configuration", () => {
+ const mock = createMockTerminal()
+ installTerminalQuerySuppression(mock.terminal, {
+ suppressAlternateScreen: true
+ })
+ const handlers = privateModeHandlers(mock)
+
+ expectPrivateModesHandled(handlers, ALTERNATE_SCREEN_MODES, true)
+ expectPrivateModesHandled(handlers, MOUSE_TRACKING_MODES, true)
+ expectPrivateModesHandled(handlers, [FOCUS_REPORTING_MODE], true)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("blocks alternate screen modes when project terminals preserve xterm scrollback", () => { | |
| const mock = createMockTerminal() | |
| installTerminalQuerySuppression(mock.terminal, { | |
| allowMouseTracking: true, | |
| suppressAlternateScreen: true | |
| }) | |
| const handlers = privateModeHandlers(mock) | |
| expectPrivateModesHandled(handlers, ALTERNATE_SCREEN_MODES, true) | |
| expectPrivateModesHandled(handlers, MOUSE_TRACKING_MODES, false) | |
| }) | |
| it("blocks alternate screen modes when project terminals preserve xterm scrollback", () => { | |
| const mock = createMockTerminal() | |
| installTerminalQuerySuppression(mock.terminal, { | |
| allowMouseTracking: true, | |
| suppressAlternateScreen: true | |
| }) | |
| const handlers = privateModeHandlers(mock) | |
| expectPrivateModesHandled(handlers, ALTERNATE_SCREEN_MODES, true) | |
| expectPrivateModesHandled(handlers, MOUSE_TRACKING_MODES, false) | |
| }) | |
| it("blocks alternate screen modes independently from mouse tracking configuration", () => { | |
| const mock = createMockTerminal() | |
| installTerminalQuerySuppression(mock.terminal, { | |
| suppressAlternateScreen: true | |
| }) | |
| const handlers = privateModeHandlers(mock) | |
| expectPrivateModesHandled(handlers, ALTERNATE_SCREEN_MODES, true) | |
| expectPrivateModesHandled(handlers, MOUSE_TRACKING_MODES, true) | |
| expectPrivateModesHandled(handlers, [FOCUS_REPORTING_MODE], 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/app/tests/docker-git/terminal-query-suppression.test.ts` around
lines 255 - 265, Add a new test case alongside the existing one that calls
installTerminalQuerySuppression(mock.terminal, { suppressAlternateScreen: true
}) (omit or set allowMouseTracking to false) and then uses
privateModeHandlers(mock) with expectPrivateModesHandled(handlers,
ALTERNATE_SCREEN_MODES, true) and expectPrivateModesHandled(handlers,
MOUSE_TRACKING_MODES, false) to verify alternate screen suppression works
independently of mouse-tracking configuration; reference
installTerminalQuerySuppression, privateModeHandlers, ALTERNATE_SCREEN_MODES,
and MOUSE_TRACKING_MODES to locate where to add the test.
| describe("terminal wheel scroll", () => { | ||
| it("converts line and page wheel deltas into terminal scroll lines", () => { | ||
| expect(resolveTerminalWheelScrollDelta({ | ||
| deltaMode: 1, | ||
| deltaY: 3.8, | ||
| previousPixelDeltaY: 9, | ||
| rows: 24 | ||
| })).toEqual({ lines: 3, nextPixelDeltaY: 0 }) | ||
| expect(resolveTerminalWheelScrollDelta({ | ||
| deltaMode: 1, | ||
| deltaY: -2.8, | ||
| previousPixelDeltaY: 9, | ||
| rows: 24 | ||
| })).toEqual({ lines: -2, nextPixelDeltaY: 0 }) | ||
| expect(resolveTerminalWheelScrollDelta({ | ||
| deltaMode: 2, | ||
| deltaY: 1, | ||
| previousPixelDeltaY: 9, | ||
| rows: 24 | ||
| })).toEqual({ lines: 24, nextPixelDeltaY: 0 }) | ||
| expect(resolveTerminalWheelScrollDelta({ | ||
| deltaMode: 2, | ||
| deltaY: 1, | ||
| previousPixelDeltaY: 9, | ||
| rows: 0 | ||
| })).toEqual({ lines: 1, nextPixelDeltaY: 0 }) | ||
| }) | ||
|
|
||
| it("accumulates pixel wheel deltas until they cross a terminal line", () => { | ||
| const first = resolveTerminalWheelScrollDelta({ | ||
| deltaMode: 0, | ||
| deltaY: 7, | ||
| previousPixelDeltaY: 0, | ||
| rows: 24 | ||
| }) | ||
| const second = resolveTerminalWheelScrollDelta({ | ||
| deltaMode: 0, | ||
| deltaY: 8, | ||
| previousPixelDeltaY: first.nextPixelDeltaY, | ||
| rows: 24 | ||
| }) | ||
|
|
||
| expect(first).toEqual({ lines: 0, nextPixelDeltaY: 7 }) | ||
| expect(second).toEqual({ lines: 1, nextPixelDeltaY: 0 }) | ||
| expect(resolveTerminalWheelScrollDelta({ | ||
| deltaMode: 0, | ||
| deltaY: -30, | ||
| previousPixelDeltaY: 0, | ||
| rows: 24 | ||
| })).toEqual({ lines: -2, nextPixelDeltaY: 0 }) | ||
| expect(resolveTerminalWheelScrollDelta({ | ||
| deltaMode: 0, | ||
| deltaY: Number.NaN, | ||
| previousPixelDeltaY: 0, | ||
| rows: 24 | ||
| })).toEqual({ lines: 0, nextPixelDeltaY: 0 }) | ||
| }) | ||
|
|
||
| it("scrolls xterm history and stops wheel mouse reports while mouse tracking is active", () => { | ||
| const activeModes: ReadonlyArray<TerminalWheelMouseTrackingMode> = ["x10", "vt200", "drag", "any"] | ||
|
|
||
| for (const mode of activeModes) { | ||
| const host = createWheelHost() | ||
| const { scrolls, terminal } = createTerminal(mode) | ||
| const { event, state } = createWheelEvent(1, 3) | ||
|
|
||
| const disposable = attachTerminalWheelScroll({ host: host.host, terminal }) | ||
| host.dispatch(event) | ||
| disposable.dispose() | ||
|
|
||
| expect(host.state.added).toBe(1) | ||
| expect(host.state.removed).toBe(1) | ||
| expect(host.state.wheelOptions).toEqual({ capture: true, passive: false }) | ||
| expectWheelEventStopped(state) | ||
| expect(scrolls).toEqual([3]) | ||
| } | ||
| }) | ||
|
|
||
| it("still stops sub-line wheel events while accumulating pixel deltas", () => { | ||
| const { scrolls, terminal } = createTerminal("any") | ||
| const { state } = dispatchWheel({ deltaMode: 0, deltaY: 1, terminal }) | ||
|
|
||
| expectWheelEventStopped(state) | ||
| expect(scrolls).toEqual([]) | ||
| }) | ||
|
|
||
| it("registers the wheel handler on xterm's terminal element when available", () => { | ||
| const host = createWheelHost() | ||
| const element = createWheelHost() | ||
| const { scrolls, terminal } = createTerminal("any", 24, { element: element.host }) | ||
| const { event, state } = createWheelEvent(1, 2) | ||
|
|
||
| const disposable = attachTerminalWheelScroll({ host: host.host, terminal }) | ||
| element.dispatch(event) | ||
| disposable.dispose() | ||
|
|
||
| expect(host.state.added).toBe(0) | ||
| expect(host.state.removed).toBe(0) | ||
| expect(element.state.added).toBe(1) | ||
| expect(element.state.removed).toBe(1) | ||
| expectWheelEventStopped(state) | ||
| expect(scrolls).toEqual([2]) | ||
| }) | ||
|
|
||
| it("stops wheel events while the alternate buffer is active", () => { | ||
| const { scrolls, terminal } = createTerminal("none", 24, { | ||
| buffer: terminalBuffer("alternate", 0) | ||
| }) | ||
| const { state } = dispatchWheel({ deltaMode: 1, deltaY: 4, terminal }) | ||
|
|
||
| expectWheelEventStopped(state) | ||
| expect(scrolls).toEqual([4]) | ||
| }) | ||
|
|
||
| it("scrolls normal-buffer history even when mouse tracking is inactive", () => { | ||
| const { scrolls, terminal } = createTerminal("none", 24, { | ||
| buffer: terminalBuffer("normal", 20) | ||
| }) | ||
| const { state } = dispatchWheel({ deltaMode: 1, deltaY: -2, terminal }) | ||
|
|
||
| expectWheelEventStopped(state) | ||
| expect(scrolls).toEqual([-2]) | ||
| }) | ||
|
|
||
| it("lets xterm handle wheel events normally when mouse tracking is inactive", () => { | ||
| const { scrolls, terminal } = createTerminal("none", 24, { | ||
| buffer: terminalBuffer("normal", 0) | ||
| }) | ||
| const { state } = dispatchWheel({ deltaMode: 0, deltaY: 45, terminal }) | ||
|
|
||
| expectWheelEventPassedThrough(state) | ||
| expect(scrolls).toEqual([]) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Рекомендуется использовать Effect test utilities для consistency.
Хотя тесты используют @effect/vitest (строка 1), они не используют Effect runtime паттерны, рекомендованные coding guidelines. Согласно guidelines: "Mock external dependencies in unit tests using Effect's testing utilities. Run tests without Effect runtime for speed."
В данном случае тесты создают моки напрямую через plain objects. Хотя это работает, для консистентности с остальной кодовой базой можно рассмотреть использование Effect.gen паттерна для организации test setup/teardown, особенно если основной код будет переписан на Effect-TS (см. комментарий к terminal-wheel-scroll.ts).
🧪 Пример Effect-based test style (опционально)
import { Effect } from "effect"
import { describe, expect, it } from "`@effect/vitest`"
describe("terminal wheel scroll", () => {
it.effect("scrolls xterm history with Effect test utilities", () =>
Effect.gen(function* (_) {
const host = createWheelHost()
const { scrolls, terminal } = createTerminal("any")
const program = yield* Effect.sync(() => {
const disposable = attachTerminalWheelScroll({ host: host.host, terminal })
const { event, state } = createWheelEvent(1, 3)
host.dispatch(event)
disposable.dispose()
return { state, scrolls, hostState: host.state }
})
expect(program.hostState.added).toBe(1)
expect(program.scrolls).toEqual([3])
})
)
})As per coding guidelines, **/*.test.{ts,tsx}: Mock external dependencies in unit tests using Effect's testing utilities.
🤖 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-wheel-scroll.test.ts` around lines 151
- 284, Tests directly create mocks and run imperative setup instead of using
Effect test utilities; update the failing/specified tests (those using
createWheelHost, createTerminal, attachTerminalWheelScroll, createWheelEvent,
dispatchWheel) to use Effect-based patterns: convert relevant it(...) blocks to
it.effect(...) and wrap the test body in Effect.gen/Effect.sync so mocks and
setup/teardown are created inside an Effect, return the assertions' data from
the Effect and assert on that result; ensure you still dispose the disposable
inside the Effect and avoid requiring the Effect runtime for the whole test
suite by using Effect's testing helpers per guidelines.
Siriusmene
left a comment
Siriusmene
left a comment
There was a problem hiding this comment.
Reviewed in CodeRabbit Change Stack
Siriusmene
left a comment
Siriusmene
left a comment
There was a problem hiding this comment.
Reviewed in CodeRabbit Change Stack
2 participants
Summary
47,1047, and1049terminal.scrollLines, so mouse wheel moves terminal history instead of shell command historyMathematical guarantees
Proof screenshots
Tests
bun --cwd packages/app vitest run tests/docker-git/terminal-wheel-scroll.test.ts tests/docker-git/terminal-query-suppression.test.ts tests/docker-git/terminal-panel-runtime-core.test.ts tests/docker-git/terminal-copy-interaction.test.tsbun run --cwd packages/app lint:testsbun run --cwd packages/app lintbun run --cwd packages/app checkbun run --cwd packages/app build:webbun --cwd packages/app vitest runFollow-up to #320 after it was merged.