Skip to content

testing: cover diff viewer and logs console behavior#373

Merged
RtlZeroMemory merged 1 commit intomainfrom
feature/testing-diff-logs-behavior
Apr 15, 2026
Merged

testing: cover diff viewer and logs console behavior#373
RtlZeroMemory merged 1 commit intomainfrom
feature/testing-diff-logs-behavior

Conversation

@RtlZeroMemory
Copy link
Copy Markdown
Owner

@RtlZeroMemory RtlZeroMemory commented Apr 14, 2026

Summary

  • add app-level diff viewer and logs console behavior coverage for hover-wheel scrolling and click focus
  • remove weak VNode-construction checks that were redundant with stronger factory and smoke coverage

Families Covered

  • diffViewer
  • logsConsole

Tests Added, Rewritten, Removed

  • added app-level tests in packages/core/src/app/__tests__/widgetBehavior.contracts.test.ts for diff viewer hover-wheel scroll and click focus
  • added app-level tests in the same file for logs console hover-wheel scroll and click focus
  • rewrote no existing tests in this PR
  • removed weak VNode-construction checks in packages/core/src/widgets/__tests__/editors.test.ts for diffViewer and logsConsole because factory and smoke coverage already covered that surface and the new behavior tests now protect the visible contract

Implementation Bugs Fixed

  • none; the behavior-first tests passed once added

Commands Run

  • ./node_modules/.bin/tsc -b packages/core/tsconfig.json --pretty false
  • node --test packages/core/dist/app/__tests__/widgetBehavior.contracts.test.js packages/core/dist/app/__tests__/widgetRenderer.integration.test.js packages/core/dist/runtime/__tests__/wheelRouting.test.js packages/core/dist/widgets/__tests__/editors.test.js packages/core/dist/widgets/__tests__/vnode.factory.test.js packages/core/dist/widgets/__tests__/widgetRenderSmoke.test.js

Unresolved Areas Left Explicit

  • diff hunk-action keyboard semantics remain outside this PR because the current public contract is thinner there
  • logs clear and expand keyboard semantics remain outside this PR because the current public contract is thinner there
  • tool approval dialog routing remains separate because its public routing contract is still thinner

Dependency Note

  • none

Summary by CodeRabbit

  • Tests
    • Added behavior tests verifying scroll interactions update scroll position and do not steal focus unless clicked for the diff viewer and logs console widgets.
    • Removed redundant editor widget test cases that were consolidated into the new behavior contract tests.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 14, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5aa72ac2-8e76-4eca-9975-68044589f6a7

📥 Commits

Reviewing files that changed from the base of the PR and between 5327e21 and f191e6d.

📒 Files selected for processing (2)
  • packages/core/src/app/__tests__/widgetBehavior.contracts.test.ts
  • packages/core/src/widgets/__tests__/editors.test.ts
💤 Files with no reviewable changes (1)
  • packages/core/src/widgets/tests/editors.test.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/core/src/app/tests/widgetBehavior.contracts.test.ts

📝 Walkthrough

Walkthrough

Added new behavior contract tests for ui.diffViewer and ui.logsConsole that exercise scroll interactions and focus transitions; removed two VNode construction tests for those widgets from the editors test file. No public APIs were changed.

Changes

Cohort / File(s) Summary
Behavior contracts tests (new/modified)
packages/core/src/app/__tests__/widgetBehavior.contracts.test.ts
Added tests that construct ui.diffViewer and ui.logsConsole inside a controlled scrollTop state and assert: wheel/hover triggers onScroll and updates rendered ui.text while focus remains null; mouse-down → mouse-up sets widget focus to its id ("diff"/"logs").
Editors tests (removed cases)
packages/core/src/widgets/__tests__/editors.test.ts
Removed two VNode construction tests: one asserting ui.diffViewer emits kind: "diffViewer" for unified/sideBySide modes (including props.mode and sideBySide.props.scrollTop checks), and one asserting ui.logsConsole VNode creation with specific filter and numeric props.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

🐰
Hopping through tests with a twitchy nose,
I chase the scroll where the code wind blows.
Wheel and click, a careful hop,
Focus lands where the cookies stop.
Tiny contracts, tidy trails—I pose.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'testing: cover diff viewer and logs console behavior' directly and accurately describes the main change: adding new behavior contract tests for diff viewer and logs console widgets.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/testing-diff-logs-behavior

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/src/app/__tests__/widgetBehavior.contracts.test.ts`:
- Around line 910-912: The test declares the variable "text" with let but it is
only assigned once; change the declaration to const to satisfy
lint/style/useConst. Locate the two occurrences where text is created via
createTestRenderer(...).render(view()).toText() (around the tests using
createTestRenderer and view()) and replace let text with const text in both
places so the variable is immutable and CI/linter will pass.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 05c0ee60-7ef7-4bfd-a462-95830fe1bae2

📥 Commits

Reviewing files that changed from the base of the PR and between 9df8069 and 5327e21.

📒 Files selected for processing (2)
  • packages/core/src/app/__tests__/widgetBehavior.contracts.test.ts
  • packages/core/src/widgets/__tests__/editors.test.ts
💤 Files with no reviewable changes (1)
  • packages/core/src/widgets/tests/editors.test.ts

Comment on lines +910 to +912
let text = createTestRenderer({ viewport: { cols: 28, rows: 6 } })
.render(view())
.toText();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use const for text to fix Biome and unblock CI.

On Line 910 and Line 959, text is declared with let but assigned only once in each test, which triggers lint/style/useConst.

Suggested patch
-    let text = createTestRenderer({ viewport: { cols: 28, rows: 6 } })
+    const text = createTestRenderer({ viewport: { cols: 28, rows: 6 } })
       .render(view())
       .toText();
@@
-    let text = createTestRenderer({ viewport: { cols: 28, rows: 6 } })
+    const text = createTestRenderer({ viewport: { cols: 28, rows: 6 } })
       .render(view())
       .toText();

Also applies to: 959-961

🧰 Tools
🪛 GitHub Actions: ci

[error] 910-912: Biome lint/style/useConst: 'text' is never reassigned. This let declares a variable that is only assigned once. Safe fix: use const instead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/app/__tests__/widgetBehavior.contracts.test.ts` around
lines 910 - 912, The test declares the variable "text" with let but it is only
assigned once; change the declaration to const to satisfy lint/style/useConst.
Locate the two occurrences where text is created via
createTestRenderer(...).render(view()).toText() (around the tests using
createTestRenderer and view()) and replace let text with const text in both
places so the variable is immutable and CI/linter will pass.

@RtlZeroMemory RtlZeroMemory force-pushed the feature/testing-diff-logs-behavior branch from 5327e21 to f191e6d Compare April 15, 2026 07:00
@RtlZeroMemory RtlZeroMemory merged commit 1555d80 into main Apr 15, 2026
34 checks passed
@RtlZeroMemory RtlZeroMemory deleted the feature/testing-diff-logs-behavior branch April 15, 2026 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant