test: add overscan coverage for VirtualList#1428
Conversation
github-actions
Bot
added
type:testing
📝 WalkthroughWalkthroughThe Skeleton widget was refactored from a complex pulse/shimmer rendering model to a simpler character-based animation system. The implementation now maintains width, height, and animation state, conditionally subscribes to a motion timer when motion capabilities are available, and renders by toggling a single character between dim and bright states. Tests were rewritten to verify subscription, animation timing, and lifecycle behavior. ChangesSkeleton Widget Animation Refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant Skeleton
participant caps.motion
participant Timer
User->>Skeleton: constructor(style, {variant, intervalMs, chars})
Skeleton->>caps.motion: check motion capability
alt caps.motion enabled AND animated true
Skeleton->>Timer: subscribe with 600ms callback
Timer->>Skeleton: toggle _frame every 600ms
Skeleton->>Skeleton: markDirty()
end
User->>Skeleton: setAnimated(true/false)
Skeleton->>Skeleton: manage timer subscription lifecycle
User->>Skeleton: setWidth(n) / setHeight(n)
Skeleton->>Skeleton: update dimensions, markDirty
User->>Skeleton: unmount()
Skeleton->>Timer: unsubscribe
Skeleton->>Skeleton: super.unmount()
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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/widgets/src/feedback/Skeleton.test.ts`:
- Around line 25-29: Remove the placeholder assertion expect(s).toBeDefined()
from the test in packages/widgets/src/feedback/Skeleton.test.ts; keep the
meaningful behavior check by instantiating the class (new Skeleton() or const s
= new Skeleton() if you prefer) only for its side effects and assert that
motion.timerPoolSubscribe has been called (referencing the Skeleton constructor
and motion.timerPoolSubscribe), and if you remove the variable assignment also
remove the unused s identifier to avoid linter warnings.
- Around line 75-80: The test only checks that setWidth/setHeight don't throw;
change it to assert observable behavior by verifying the dimensions and render
output update: create a Skeleton instance, spy on markDirty
(jest.spyOn(Skeleton.prototype, 'markDirty')) to assert it's called when calling
setWidth and setHeight, call s.setWidth(10) and s.setHeight(5), then assert the
instance reports the new sizes (e.g., s.getWidth()/s.getHeight() or public
width/height fields) and that s.render() (or the component's rendered output)
reflects those dimensions; use the Skeleton, setWidth, setHeight, markDirty and
render identifiers to locate the code to update.
In `@packages/widgets/src/feedback/Skeleton.ts`:
- Around line 5-9: The SkeletonOptions interface is missing properties used by
the constructor (opts.width, opts.height, opts.animated, opts.character); update
SkeletonOptions to declare these properties (e.g., width?: number | string,
height?: number | string, animated?: boolean, character?: string) and keep or
reconcile existing fields (variant, intervalMs, chars) as needed so the
constructor's references (in the Skeleton class constructor using opts.width /
opts.height / opts.animated / opts.character) match the interface and satisfy
TypeScript strict mode.
- Line 1: The import statement includes an unused type symbol "Color"; remove
"type Color" from the import list in the Skeleton module so the top line reads
something like import { type Screen, type Style, caps } from '`@termuijs/core`'; —
keep the other imported symbols (Screen, Style, caps) intact and run type checks
to ensure no other references to Color exist in the file.
🪄 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 Plus
Run ID: d7c566f7-f998-42c3-8f5e-f2fcaabe0005
📒 Files selected for processing (3)
packages/widgets/src/feedback/Skeleton.test.tspackages/widgets/src/feedback/Skeleton.tspackages/widgets/src/index.ts
| it('creates skeleton and subscribes to timer', () => { | ||
| const s = new Skeleton(); | ||
| expect(s).toBeDefined(); | ||
| expect(motion.timerPoolSubscribe).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Remove placeholder expect(s).toBeDefined() assertion.
The coding guidelines state that expect(widget).toBeDefined() is not a real test. The second assertion (toHaveBeenCalled) is meaningful, so just remove the placeholder.
it('creates skeleton and subscribes to timer', () => {
const s = new Skeleton();
- expect(s).toBeDefined();
expect(motion.timerPoolSubscribe).toHaveBeenCalled();
});As per coding guidelines: "Tests must be real. expect(true).toBe(true) and expect(widget).toBeDefined() are not tests."
📝 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('creates skeleton and subscribes to timer', () => { | |
| const s = new Skeleton(); | |
| expect(s).toBeDefined(); | |
| expect(motion.timerPoolSubscribe).toHaveBeenCalled(); | |
| }); | |
| it('creates skeleton and subscribes to timer', () => { | |
| const s = new Skeleton(); | |
| expect(motion.timerPoolSubscribe).toHaveBeenCalled(); | |
| }); |
🤖 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/widgets/src/feedback/Skeleton.test.ts` around lines 25 - 29, Remove
the placeholder assertion expect(s).toBeDefined() from the test in
packages/widgets/src/feedback/Skeleton.test.ts; keep the meaningful behavior
check by instantiating the class (new Skeleton() or const s = new Skeleton() if
you prefer) only for its side effects and assert that motion.timerPoolSubscribe
has been called (referencing the Skeleton constructor and
motion.timerPoolSubscribe), and if you remove the variable assignment also
remove the unused s identifier to avoid linter warnings.
Source: Coding guidelines
| it('setWidth / setHeight trigger markDirty safely', () => { | ||
| const s = new Skeleton(); | ||
|
|
||
| const screen1 = new Screen(10, 1); | ||
| skeleton.render(screen1); | ||
| // Initially, shimmerPos = 0, bandStart = 0, bandWidth = 2 -> indices 0 and 1 are '#' | ||
| expect(screen1.back[0].map(c => c.char).join('')).toBe('##--------'); | ||
|
|
||
| // Advance shimmerPos by 1 -> bandStart = 1 | ||
| if (timerCallback) timerCallback(); | ||
|
|
||
| const screen2 = new Screen(10, 1); | ||
| skeleton.render(screen2); | ||
| expect(screen2.back[0].map(c => c.char).join('')).toBe('-##-------'); | ||
| expect(() => s.setWidth(10)).not.toThrow(); | ||
| expect(() => s.setHeight(5)).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Test lacks observable behavior assertion.
This test only verifies that methods don't throw, which is a placeholder pattern. It should assert actual behavior — e.g., that render output reflects the new dimensions.
it('setWidth / setHeight trigger markDirty safely', () => {
- const s = new Skeleton();
+ const s = new Skeleton({}, { width: 2, height: 1 });
+ s.updateRect({ x: 0, y: 0, width: 10, height: 10 });
- expect(() => s.setWidth(10)).not.toThrow();
- expect(() => s.setHeight(5)).not.toThrow();
+ s.setWidth(3);
+ s.setHeight(2);
+
+ const screen = new Screen(10, 10);
+ s.render(screen);
+
+ // Verify render uses updated dimensions (3 wide, 2 tall)
+ expect(screen.back[0].slice(0, 4).map(c => c.char).join('')).toBe('░░░ ');
+ expect(screen.back[1].slice(0, 4).map(c => c.char).join('')).toBe('░░░ ');
});As per coding guidelines: "Tests must be real... Assert observable behavior or rendered output."
🤖 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/widgets/src/feedback/Skeleton.test.ts` around lines 75 - 80, The
test only checks that setWidth/setHeight don't throw; change it to assert
observable behavior by verifying the dimensions and render output update: create
a Skeleton instance, spy on markDirty (jest.spyOn(Skeleton.prototype,
'markDirty')) to assert it's called when calling setWidth and setHeight, call
s.setWidth(10) and s.setHeight(5), then assert the instance reports the new
sizes (e.g., s.getWidth()/s.getHeight() or public width/height fields) and that
s.render() (or the component's rendered output) reflects those dimensions; use
the Skeleton, setWidth, setHeight, markDirty and render identifiers to locate
the code to update.
Source: Coding guidelines
| // ───────────────────────────────────────────────────── | ||
|
|
||
| import { type Screen, type Style, type Color, caps, prefersReducedMotion } from '@termuijs/core'; | ||
| import { type Screen, type Style, type Color, caps } from '@termuijs/core'; |
There was a problem hiding this comment.
Remove unused Color import.
Color is imported but never used in this file.
-import { type Screen, type Style, type Color, caps } from '`@termuijs/core`';
+import { type Screen, type Style, caps } from '`@termuijs/core`';🤖 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/widgets/src/feedback/Skeleton.ts` at line 1, The import statement
includes an unused type symbol "Color"; remove "type Color" from the import list
in the Skeleton module so the top line reads something like import { type
Screen, type Style, caps } from '`@termuijs/core`'; — keep the other imported
symbols (Screen, Style, caps) intact and run type checks to ensure no other
references to Color exist in the file.
| export interface SkeletonOptions { | ||
| /** Animation style: 'pulse' alternates two chars; 'shimmer' scrolls a highlight band */ | ||
| variant?: 'pulse' | 'shimmer'; | ||
| /** Animation interval in ms (default: 600) */ | ||
| intervalMs?: number; | ||
| /** Characters for pulse: [dim, bright]. Default: ['░', '▒'] (ASCII fallback: ['-', '#']) */ | ||
| chars?: [string, string]; | ||
| /** Color for the bright state (optional) */ | ||
| color?: Color; | ||
| } |
There was a problem hiding this comment.
SkeletonOptions interface does not declare the properties actually used by the constructor.
The interface declares variant, intervalMs, and chars, but the constructor reads opts.width, opts.height, opts.animated, and opts.character. This causes TypeScript errors under strict mode since these properties don't exist on the interface.
🛠️ Proposed fix to align the interface with actual usage
export interface SkeletonOptions {
- variant?: 'pulse' | 'shimmer';
- intervalMs?: number;
- chars?: [string, string];
+ width?: number;
+ height?: number;
+ animated?: boolean;
+ character?: string;
}Also applies to: 20-28
🤖 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/widgets/src/feedback/Skeleton.ts` around lines 5 - 9, The
SkeletonOptions interface is missing properties used by the constructor
(opts.width, opts.height, opts.animated, opts.character); update SkeletonOptions
to declare these properties (e.g., width?: number | string, height?: number |
string, animated?: boolean, character?: string) and keep or reconcile existing
fields (variant, intervalMs, chars) as needed so the constructor's references
(in the Skeleton class constructor using opts.width / opts.height /
opts.animated / opts.character) match the interface and satisfy TypeScript
strict mode.
Karanjot786
left a comment
Karanjot786
left a comment
There was a problem hiding this comment.
Thanks for the contribution. There are issues to fix before this merges.
Issues:
-
PR content does not match its title or stated purpose
File: packages/widgets/src/feedback/Skeleton.ts, packages/widgets/src/feedback/Skeleton.test.ts
Problem: The PR title says "test: add overscan coverage for VirtualList" and the description says it closes #1426 by adding VirtualList overscan tests. However, the actual diff contains only changes toSkeleton.tsandSkeleton.test.ts— there is no VirtualList change anywhere in the diff. The branch appears to have been based on or merged with another branch that rewrites the Skeleton widget rather than targeting the correct base.
Fix: Rebase this branch onto the upstreammain, remove the Skeleton changes that don't belong here, and add only the VirtualList overscan tests described in the PR body. Then push again. -
The Skeleton changes in the diff contain breaking API issues (same as in PR #1430 from the same author)
Problem: Even if the Skeleton changes were intentional, the constructor readsopts.width,opts.height,opts.animated,opts.characterwhich are not declared onSkeletonOptions, and theshimmervariant andcharsoption are silently dropped — a breaking change.
Checklist before re-requesting review:
- Fix applied
- Tests pass locally (run: bun vitest run)
- PR title follows format: type(scope): description
Karanjot786
left a comment
Karanjot786
left a comment
There was a problem hiding this comment.
Thanks for the contribution. There are issues to fix before this merges.
Issues:
-
PR title and diff are completely mismatched
Problem: The PR title istest: add overscan coverage for VirtualList, which referencesVirtualList. However, the diff only touchespackages/widgets/src/feedback/Skeleton.ts,Skeleton.test.ts, andpackages/widgets/src/index.ts. There are no VirtualList changes at all.
Fix: Either (a) update the diff to actually add VirtualList overscan tests and revert the Skeleton changes, or (b) retitle the PR to accurately describe what it does. These appear to be the same Skeleton refactor from PR #1430 — please coordinate with that PR to avoid conflicting changes. -
Skeleton.ts accesses options not declared in
SkeletonOptions
File: packages/widgets/src/feedback/Skeleton.ts, lines ~20–23
Problem:opts.width,opts.height,opts.animated, andopts.characterare read in the constructor but not declared in theSkeletonOptionsinterface. This is a type contract violation.
Fix: Add these fields toSkeletonOptionsor remove the Skeleton changes from this PR.
Checklist before re-requesting review:
- Fix applied
- Tests pass locally (run: bun vitest run)
- PR title follows format: type(scope): description
|
Thanks for the contribution. There are issues to fix before this merges. Issues:
Checklist before re-requesting review:
|
Karanjot786
left a comment
Karanjot786
left a comment
There was a problem hiding this comment.
Thanks for the contribution. There are issues to fix before this merges.
Issues:
- PR contents do not match the title/issue
File: packages/widgets/src/feedback/Skeleton.ts, Skeleton.test.ts, packages/widgets/src/index.ts
Problem: This PR is titled "test: add overscan coverage for VirtualList" and claims to close #1426, but the diff contains zero changes to VirtualList or overscan code. Instead it rewrites packages/widgets/src/feedback/Skeleton.ts and its tests, removing theshimmervariant andprefersReducedMotionhandling, and introducing constructor options (width,height,animated,character) and methods (setWidth,setHeight) that do not exist onSkeletonOptions— this will fail typecheck and is an unrelated regression to an existing widget.
Fix: Please push the correct branch/diff containing VirtualList overscan test coverage. If this branch was pushed by mistake (wrong local branch), force-push the intended commits to feature/virtuallist-overscan.
Checklist before re-requesting review:
- Fix 1 applied
- Tests pass locally (run: bun vitest run)
- PR title follows format: type(scope): description
Karanjot786
left a comment
Karanjot786
left a comment
There was a problem hiding this comment.
Thanks for the contribution. There are issues to fix before this merges.
Issues:
-
PR title and body describe VirtualList overscan coverage, but the diff only modifies Skeleton files — no VirtualList code is touched
File: packages/widgets/src/feedback/Skeleton.ts, packages/widgets/src/feedback/Skeleton.test.ts
Problem: This PR appears to be submitted on the wrong branch or contains the wrong set of changes. The title says "test: add overscan coverage for VirtualList" but all changes are Skeleton widget refactors.
Fix: Either retarget this PR to match the actual changes (rename the title to reflect the Skeleton refactor), or push the intended VirtualList overscan tests and revert the Skeleton changes. -
Constructor reads properties not declared in
SkeletonOptions
File: packages/widgets/src/feedback/Skeleton.ts, lines ~29-32
Problem: The constructor accessesopts.width,opts.height,opts.animated, andopts.characterbutSkeletonOptionsonly declaresvariant,intervalMs, andchars. These accesses will always resolve toundefined— sothis._widthis always 10,this._animatedis always true, andthis._characteralways uses the unicode default, regardless of what the caller passes. The test that passes{ character: 'X' }will fail if TypeScript strict mode is enabled.
Fix: Add the missing properties toSkeletonOptions:width?: number,height?: number,animated?: boolean,character?: string. Remove the now-unusedvariant,intervalMs,charsfields (or keep them for back-compat). -
Breaking API removal without a major version bump
File: packages/widgets/src/feedback/Skeleton.ts
Problem: The PR removesvariant,intervalMs,chars, andcolorfromSkeletonOptionsand drops shimmer support entirely. Any callers usingnew Skeleton({}, { variant: 'shimmer' })orcharswill silently break — there is no deprecation notice or migration path.
Fix: Either keep backward-compatible fields (even if no-ops with a deprecation comment), or document this as a breaking change and ensure the semver bump reflects it. -
Missing newline at end of file
File: packages/widgets/src/feedback/Skeleton.ts (last line), packages/widgets/src/feedback/Skeleton.test.ts (last line)
Problem: Both modified files are missing a trailing newline, which violates the project's formatting conventions and may cause linter/formatter failures.
Fix: Add a newline character at the end of both files.
Checklist before re-requesting review:
- PR title and branch match the actual changes (VirtualList overscan vs Skeleton refactor)
-
SkeletonOptionsinterface updated to include all properties accessed in the constructor - Trailing newline added to both modified files
- Tests pass locally (run: bun vitest run packages/widgets/src/feedback/Skeleton.test.ts)
Karanjot786
left a comment
Karanjot786
left a comment
There was a problem hiding this comment.
Thanks for the contribution. There are issues to fix before this merges.
Issues:
-
PR title and body do not match the actual changes
File: PR metadata
Problem: The title says "test: add overscan coverage for VirtualList" and the body references "overscan behavior in VirtualList range computation", but the entire diff is about the Skeleton widget — no VirtualList files are touched. This makes the PR impossible to triage correctly.
Fix: Update the PR title and body to describe the Skeleton refactor, e.g. "refactor(widgets): simplify Skeleton widget API and update tests" -
SkeletonOptions interface is missing the new fields the constructor reads
File: packages/widgets/src/feedback/Skeleton.ts, lines 10-14 and constructor body ~line 30
Problem: The constructor reads opts.width, opts.height, opts.animated, and opts.character, but these properties are not declared in SkeletonOptions. They will always resolve to undefined at runtime, so setWidth/setHeight defaults silently come from an unrelated field, animated is always undefined (falsy), and character always falls back to the default — meaning the widget can never be configured via the constructor.
Fix: Add the missing fields to SkeletonOptions:export interface SkeletonOptions { variant?: 'pulse' | 'shimmer'; intervalMs?: number; chars?: [string, string]; width?: number; height?: number; animated?: boolean; character?: string; }
-
Test assertion for custom character will silently pass for the wrong reason
File: packages/widgets/src/feedback/Skeleton.test.ts — "respects custom character" test
Problem: new Skeleton({}, { character: 'X' }) passes character via SkeletonOptions, but because character is not in the interface, this._character never gets 'X' — it stays as the default '░'. The rendered output will be '░░░' not 'XXX', so the assertion is wrong. Once the interface is fixed (issue 2 above) this test should pass correctly.
Fix: Fix the interface first (issue 2), then verify bun vitest run confirms this test passes. -
prefersReducedMotion() removed — accessibility regression
File: packages/widgets/src/feedback/Skeleton.ts, constructor
Problem: The original code called prefersReducedMotion() from @termuijs/core to honour the OS-level reduced-motion preference. The new code only checks caps.motion, which is a capability flag ("does this terminal support animation"), not an accessibility preference. Users who have enabled Prefers Reduced Motion at the OS level will now still see animation.
Fix: Restore the prefersReducedMotion() guard. Import it from @termuijs/core and change the condition to:if (this._animated && caps.motion && !prefersReducedMotion()) {
-
Missing EOF newline on two files
File: packages/widgets/src/feedback/Skeleton.ts (last line), packages/widgets/src/feedback/Skeleton.test.ts (last line)
Problem: Both files are missing a trailing newline, which causes noisy diffs and fails most linters/editors.
Fix: Add a newline at the end of each file.
Checklist before re-requesting review:
- PR title and body updated to describe the Skeleton changes
- SkeletonOptions interface updated with width, height, animated, character fields
- Custom character test verified to pass after interface fix
- prefersReducedMotion() guard restored
- EOF newlines added to Skeleton.ts and Skeleton.test.ts
- Tests pass locally (run: bun vitest run)
|
Thanks for the contribution. There are issues to fix before this merges. Issues:
Checklist before re-requesting review:
|
|
The diff in this PR contains only Skeleton widget tests/changes, but the title and linked issue are about VirtualList overscan. It looks like the wrong branch was pushed. Please:
The Skeleton changes have blocking issues regardless:
Checklist:
|
2 participants
Closes #1426
Added test coverage for overscan behavior in VirtualList range computation.
Summary by CodeRabbit
Refactor
Tests