Docs/add example comments#1670
Conversation
github-actions
Bot
added
type:docs
📝 WalkthroughWalkthroughA new ChangesSpinner Widget, Exports, Tests, and Docs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 5❌ Failed checks (5 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/choosing-your-api.md`:
- Around line 51-56: The imperative widget example is overriding the
`render(buffer)` method when it should override `_renderSelf(screen)` instead.
Change the method signature from `render(buffer)` to `_renderSelf(screen)` and
adjust the implementation as necessary to work with the correct method.
Overriding `_renderSelf` ensures the example uses the proper render pipeline
that includes visibility checks, clipping, error handling, performance tracking,
border rendering, and child rendering, rather than bypassing these core
behaviors.
In `@packages/widgets/src/index.ts`:
- Around line 313-314: Update the module specifiers in the Spinner export
statements to include the `.js` file extension. Change both `export { Spinner }
from './Spinner'` and `export type { SpinnerProps } from './Spinner'` to use
`'./Spinner.js'` instead of `'./Spinner'` to match the consistent naming
convention used throughout this index file where all other module specifiers
include the `.js` extension.
In `@packages/widgets/src/Spinner.ts`:
- Line 3: The import statement for the Text component in Spinner.ts uses an
incorrect path that does not resolve. Update the import path from './Text' to
'./display/Text' to correctly reference the Text component which is located in
the display subdirectory.
- Line 31: The file Spinner.ts contains JSX syntax (the return statement with
the Text component) but has a .ts file extension, which should only contain
TypeScript code. Rename the file from Spinner.ts to Spinner.tsx to properly
indicate that it contains JSX code, which will allow TypeScript and supporting
tools to correctly parse and validate the JSX syntax in the file.
- Line 19: The condition at line 19 in Spinner.ts uses the non-existent
caps.noMotion property which will silently fail. Replace the caps.noMotion check
with a call to the prefersReducedMotion() function imported from `@termuijs/core`,
which properly checks the caps.motion property. This will ensure animations are
correctly disabled in NO_MOTION and CI environments.
In `@tests/helpers/Spinner.test.ts`:
- Line 7: The file Spinner.test.ts contains JSX syntax in the render call but
uses the .ts extension instead of .tsx. TypeScript only parses JSX in .tsx
files. Rename the file from Spinner.test.ts to Spinner.test.tsx to enable proper
JSX parsing by the TypeScript compiler.
🪄 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: 79c6966f-63da-4bbe-a667-e7acc2c846eb
📒 Files selected for processing (4)
docs/choosing-your-api.mdpackages/widgets/src/Spinner.tspackages/widgets/src/index.tstests/helpers/Spinner.test.ts
| render(buffer) { | ||
| const width = this.rect.width | ||
| const filled = Math.round(width * this.progress) | ||
| const bar = '█'.repeat(filled) + '░'.repeat(width - filled) | ||
| buffer.write(0, 0, bar, Style.fg('green')) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify recommended extension point in widgets
rg -n -C3 "protected _renderSelf\s*\(|render\s*\(screen" packages/widgets/src/base/Widget.tsRepository: Karanjot786/TermUI
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Check if _renderSelf method exists in Widget class
rg -n "_renderSelf" packages/widgets/src/base/Widget.tsRepository: Karanjot786/TermUI
Length of output: 240
🏁 Script executed:
#!/bin/bash
# Read the actual documentation file to see the code at lines 51-56
sed -n '45,65p' docs/choosing-your-api.mdRepository: Karanjot786/TermUI
Length of output: 610
🏁 Script executed:
#!/bin/bash
# Get more context from Widget.render to understand the full pipeline
sed -n '250,310p' packages/widgets/src/base/Widget.tsRepository: Karanjot786/TermUI
Length of output: 2278
🏁 Script executed:
#!/bin/bash
# Check if there are other incorrect render() overrides in the documentation
rg -n "render\s*\(buffer\)|render\s*\(screen\)" docs/choosing-your-api.mdRepository: Karanjot786/TermUI
Length of output: 84
The imperative widget example overrides the wrong render method.
For Widget, examples should implement _renderSelf(screen) rather than overriding render(...); overriding render bypasses core render pipeline behavior including visibility checks, clipping, error handling, performance tracking, border rendering, and child rendering as shown in Widget.render.
Suggested fix
- render(buffer) {
- const width = this.rect.width
- const filled = Math.round(width * this.progress)
- const bar = '█'.repeat(filled) + '░'.repeat(width - filled)
- buffer.write(0, 0, bar, Style.fg('green'))
- }
+ protected _renderSelf(screen: Screen): void {
+ const { x, y, width } = this.rect
+ const filled = Math.round(width * this.progress)
+ const bar = '█'.repeat(filled) + '░'.repeat(width - filled)
+ screen.writeString(x, y, bar, { fg: { type: 'named', name: 'green' } })
+ }📝 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.
| render(buffer) { | |
| const width = this.rect.width | |
| const filled = Math.round(width * this.progress) | |
| const bar = '█'.repeat(filled) + '░'.repeat(width - filled) | |
| buffer.write(0, 0, bar, Style.fg('green')) | |
| } | |
| protected _renderSelf(screen: Screen): void { | |
| const { x, y, width } = this.rect | |
| const filled = Math.round(width * this.progress) | |
| const bar = '█'.repeat(filled) + '░'.repeat(width - filled) | |
| screen.writeString(x, y, bar, { fg: { type: 'named', name: 'green' } }) | |
| } |
🤖 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 `@docs/choosing-your-api.md` around lines 51 - 56, The imperative widget
example is overriding the `render(buffer)` method when it should override
`_renderSelf(screen)` instead. Change the method signature from `render(buffer)`
to `_renderSelf(screen)` and adjust the implementation as necessary to work with
the correct method. Overriding `_renderSelf` ensures the example uses the proper
render pipeline that includes visibility checks, clipping, error handling,
performance tracking, border rendering, and child rendering, rather than
bypassing these core behaviors.
| export { Spinner } from './Spinner' | ||
| export type { SpinnerProps } from './Spinner' No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify duplicate Spinner exports in this module
rg -n -C1 "export\s+\{\s*Spinner\b" packages/widgets/src/index.tsRepository: Karanjot786/TermUI
Length of output: 455
🏁 Script executed:
fd -t f "Spinner" packages/widgets/src/ | head -20Repository: Karanjot786/TermUI
Length of output: 181
🏁 Script executed:
cat packages/widgets/src/index.ts | sed -n '310,320p'Repository: Karanjot786/TermUI
Length of output: 328
🏁 Script executed:
find packages/widgets/src -name "*Spinner*" -type fRepository: Karanjot786/TermUI
Length of output: 181
🏁 Script executed:
head -30 packages/widgets/src/Spinner.tsRepository: Karanjot786/TermUI
Length of output: 902
🏁 Script executed:
head -30 packages/widgets/src/feedback/Spinner.tsRepository: Karanjot786/TermUI
Length of output: 1257
Fix module specifier to match pattern used throughout the file.
./Spinner should be ./Spinner.js to match the .js extension pattern consistently used in this index file (e.g., ./feedback/Spinner.js, ./display/Rule.js). While this is a separate JSX hook-based Spinner component (distinct from the class-based Widget exported on line 137), the module specifier should follow the established convention.
Suggested fix
-export { Spinner } from './Spinner'
-export type { SpinnerProps } from './Spinner'
+export { Spinner } from './Spinner.js'
+export type { SpinnerProps } from './Spinner.js'📝 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.
| export { Spinner } from './Spinner' | |
| export type { SpinnerProps } from './Spinner' | |
| export { Spinner } from './Spinner.js' | |
| export type { SpinnerProps } from './Spinner.js' |
🤖 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/index.ts` around lines 313 - 314, Update the module
specifiers in the Spinner export statements to include the `.js` file extension.
Change both `export { Spinner } from './Spinner'` and `export type {
SpinnerProps } from './Spinner'` to use `'./Spinner.js'` instead of
`'./Spinner'` to match the consistent naming convention used throughout this
index file where all other module specifiers include the `.js` extension.
| @@ -0,0 +1,32 @@ | |||
| import { useEffect, useState } from '@termuijs/jsx' | |||
| import { caps } from '@termuijs/core' | |||
| import { Text } from './Text' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where Text is actually defined/exported
fd -i 'Text.ts' packages/widgets/src
rg -n -C2 'export\s+\{\s*Text\s*\}\s+from' packages/widgets/src/index.tsRepository: Karanjot786/TermUI
Length of output: 440
🏁 Script executed:
cat -n packages/widgets/src/Spinner.ts | head -20Repository: Karanjot786/TermUI
Length of output: 765
🏁 Script executed:
# Double-check if Text.ts exists at the root of src
ls -la packages/widgets/src/Text.ts 2>&1Repository: Karanjot786/TermUI
Length of output: 138
Fix broken import path for Text component.
The import path ./Text does not resolve; Text is located in the display subdirectory.
Suggested fix
-import { Text } from './Text'
+import { Text } from './display/Text.js'📝 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.
| import { Text } from './Text' | |
| import { Text } from './display/Text.js' |
🤖 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/Spinner.ts` at line 3, The import statement for the Text
component in Spinner.ts uses an incorrect path that does not resolve. Update the
import path from './Text' to './display/Text' to correctly reference the Text
component which is located in the display subdirectory.
| const frames = caps.unicode ? FRAMES_UNICODE : FRAMES_ASCII | ||
|
|
||
| useEffect(() => { | ||
| if (caps.noMotion) return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify caps API surface and motion helper usage
rg -n -C2 '\bcaps\.(noMotion|motion)\b|prefersReducedMotion\s*\(' packagesRepository: Karanjot786/TermUI
Length of output: 14341
🏁 Script executed:
#!/bin/bash
# Check if caps.noMotion is defined anywhere
rg -n '\bcaps\.noMotion\b' packages
# Check what's exported from core env-caps
rg -n '^export' packages/core/src/terminal/env-caps.ts
# Get exact content around line 19 of Spinner.ts
head -25 packages/widgets/src/Spinner.ts | tail -10Repository: Karanjot786/TermUI
Length of output: 561
🏁 Script executed:
#!/bin/bash
# Get the full definition of the caps object in env-caps.ts
sed -n '1,42p' packages/core/src/terminal/env-caps.ts
# Check what `@termuijs/core` exports (usually in index.ts or package.json exports)
find packages/core/src -name 'index.ts' -o -name 'index.js' | head -5
# Check if prefersReducedMotion is re-exported from the main entry point
rg -n 'prefersReducedMotion' packages/core/src/index.ts 2>/dev/null || echo "Not found in index.ts, checking package.json"Repository: Karanjot786/TermUI
Length of output: 1653
Use prefersReducedMotion() instead of the non-existent caps.noMotion.
caps.noMotion is not defined in the caps contract. The caps object exposes only color, unicode, motion, ci, and getters for background and keybindingMode. Using undefined caps.noMotion will silently fail, allowing animations to run in NO_MOTION=1 and CI environments.
Use the documented prefersReducedMotion() function exported from @termuijs/core, which correctly checks caps.motion.
Suggested fix
-import { caps } from '`@termuijs/core`'
+import { prefersReducedMotion } from '`@termuijs/core`'
...
- if (caps.noMotion) return
+ if (prefersReducedMotion()) return📝 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.
| if (caps.noMotion) return | |
| if (prefersReducedMotion()) return |
🤖 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/Spinner.ts` at line 19, The condition at line 19 in
Spinner.ts uses the non-existent caps.noMotion property which will silently
fail. Replace the caps.noMotion check with a call to the prefersReducedMotion()
function imported from `@termuijs/core`, which properly checks the caps.motion
property. This will ensure animations are correctly disabled in NO_MOTION and CI
environments.
| const spinnerChar = frames[frame] | ||
| const display = label ? `${spinnerChar} ${label}` : spinnerChar | ||
|
|
||
| return <Text color={color}>{display}</Text> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify .ts files containing JSX tags
rg -nP --type=ts '<[A-Z][A-Za-z0-9]*\b|</[A-Z][A-Za-z0-9]*>' -g '**/*.ts'Repository: Karanjot786/TermUI
Length of output: 50374
🏁 Script executed:
#!/bin/bash
# Check which Spinner file exists and its content
ls -la packages/widgets/src/ | grep -i spinner
echo "---"
# Check if .ts file exists with JSX
if [ -f "packages/widgets/src/Spinner.ts" ]; then
echo "=== Spinner.ts (first 35 lines) ==="
head -35 packages/widgets/src/Spinner.ts
fi
echo "---"
# Check if .tsx file exists
if [ -f "packages/widgets/src/Spinner.tsx" ]; then
echo "=== Spinner.tsx exists ==="
head -5 packages/widgets/src/Spinner.tsx
fiRepository: Karanjot786/TermUI
Length of output: 1054
File should be renamed from .ts to .tsx.
The file contains JSX syntax (<Text color={color}>{display}</Text>) which is incompatible with the .ts extension. TypeScript and supporting tools expect only TypeScript code in .ts files; JSX requires .tsx.
Suggested fix
- packages/widgets/src/Spinner.ts
+ packages/widgets/src/Spinner.tsx🧰 Tools
🪛 Biome (2.5.0)
[error] 31-31: expected > but instead found color
(parse)
[error] 31-31: Invalid assignment to <Text color
(parse)
[error] 31-31: unterminated regex literal
(parse)
🤖 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/Spinner.ts` at line 31, The file Spinner.ts contains JSX
syntax (the return statement with the Text component) but has a .ts file
extension, which should only contain TypeScript code. Rename the file from
Spinner.ts to Spinner.tsx to properly indicate that it contains JSX code, which
will allow TypeScript and supporting tools to correctly parse and validate the
JSX syntax in the file.
Source: Linters/SAST tools
|
|
||
| describe('Spinner', () => { | ||
| it('renders with label', () => { | ||
| const t = render(<Spinner label="Loading" />) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find test .ts files containing JSX
rg -nP --type=ts '<[A-Z][A-Za-z0-9]*\b|</[A-Z][A-Za-z0-9]*>' testsRepository: Karanjot786/TermUI
Length of output: 144
Rename this test file to .test.tsx.
The file tests/helpers/Spinner.test.ts contains JSX syntax (<Spinner label="Loading" />), which cannot be parsed by TypeScript in .ts files. Only .tsx files support JSX parsing.
Suggested fix
- tests/helpers/Spinner.test.ts
+ tests/helpers/Spinner.test.tsx🧰 Tools
🪛 Biome (2.5.0)
[error] 7-7: expected > but instead found label
(parse)
[error] 7-7: Invalid assignment to <Spinner label
(parse)
[error] 7-7: Expected an expression but instead found '>'.
(parse)
[error] 7-7: Expected an expression but instead found ')'.
(parse)
🤖 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 `@tests/helpers/Spinner.test.ts` at line 7, The file Spinner.test.ts contains
JSX syntax in the render call but uses the .ts extension instead of .tsx.
TypeScript only parses JSX in .tsx files. Rename the file from Spinner.test.ts
to Spinner.test.tsx to enable proper JSX parsing by the TypeScript compiler.
Source: Linters/SAST tools
1 participant
Description
This PR adds inline comments to example application configuration files to improve the onboarding experience for new TermUI contributors and users.
Changes Made
Why This Helps
jsxImportSource)Checklist
Type of Change
Closed #1668