feat(quick): Add Card and Box container builder#1284
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesContainer Widget Builders
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR to TermUI, @RajSurajR.
Before your PR merges:
- ⭐ Star the repo. Required. The
star-checkjob blocks your merge otherwise. - ✅ All checks green:
build,test,typecheck. - 🏷 PR title follows
type: short description. Example:fix: handle empty list. - 🔗 Link your closing issue in the description.
GSSoC 2026 points come from labels after merge:
gssoc:approved. +50 base points.level:beginner/intermediate/advanced/critical. +20 / +35 / +55 / +80.quality:clean/exceptional. x 1.2 / x 1.5.type:*. Stackable bonus.
Your reviewer responds within 48 hours. Ping @Karanjot786 on Discord for urgent help.
🚀 Welcome to the cohort.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/quick/src/widgets.ts (1)
501-515: 💤 Low valueConsider using property shorthand.
Line 507 uses
borderColor: borderColorwhich can be shortened to justborderColorusing ES6 property shorthand.♻️ Suggested simplification
const container = new Box({ border: 'single', - borderColor: borderColor, + borderColor, });🤖 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/quick/src/widgets.ts` around lines 501 - 515, The object passed to the Box constructor uses the redundant key/value pair borderColor: borderColor; update the box function to use ES6 property shorthand by replacing that pair with just borderColor. Locate the box function (export function box) where a new Box({ border: 'single', borderColor: borderColor }) is constructed and change it to new Box({ border: 'single', borderColor }) so the variable declared above (borderColor) is used via shorthand; no other behavior changes needed.
🤖 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/quick/src/widgets.test.ts`:
- Around line 82-127: The tests in widgets.test.ts are asserting private fields
(_title, _borderColor, _style) instead of rendering and checking visible output;
update each spec that references card, box or text to import Screen from
`@termuijs/core`, create a Screen instance with appropriate width/height, call
widget.updateRect({x,y,width,height}) and widget.render(screen), then assert
observable output from the screen buffer (e.g., screen.getLine(lineIndex)
contains the title text or border characters and child text) rather than
inspecting internal properties; ensure you keep references to the same symbols
(card, box, text, Screen, updateRect, render, getLine) so tests remain focused
on rendered behavior.
---
Nitpick comments:
In `@packages/quick/src/widgets.ts`:
- Around line 501-515: The object passed to the Box constructor uses the
redundant key/value pair borderColor: borderColor; update the box function to
use ES6 property shorthand by replacing that pair with just borderColor. Locate
the box function (export function box) where a new Box({ border: 'single',
borderColor: borderColor }) is constructed and change it to new Box({ border:
'single', borderColor }) so the variable declared above (borderColor) is used
via shorthand; no other behavior changes needed.
🪄 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: 01cef2a5-b3f2-40b1-99a0-a128b2242b58
📒 Files selected for processing (3)
packages/quick/src/index.tspackages/quick/src/widgets.test.tspackages/quick/src/widgets.ts
RajSurajR
changed the title
Karanjot786
left a comment
Karanjot786
left a comment
There was a problem hiding this comment.
Code review complete — CI is awaiting maintainer approval to run (fork workflow gate). Reviewing code only per policy.
Code looks good:
card()andbox()builders follow the existing quick-widget pattern exactly- Both correctly delegate to
CardandBoxfrom@termuijs/widgetswith proper constructor signatures borderColordefaults to{ type: 'named', name: 'brightBlack' }— consistent with the Color object type used across the codebase (not a string)- Both are exported from
packages/quick/src/index.tswith their option types - Tests cover:
instanceof Widget,childrenpresence,titleforwarding, andborderColordefault — good coverage - No
console.log, no newas anyon public surface
Waiting on: CI green before merge. Once the workflow approval is granted and CI passes, this is ready to squash-merge.
|
Hey @Karanjot786 PR #1284 is Open . is any problem in this PR. |
|
Hey @Karanjot786 . |
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:
-
(widget as any)._fieldaccess in tests
File: packages/quick/src/widgets.test.ts, lines 106, 112, 113, 126
Problem: The tests access(w as any)._titleand(w as any)._borderColor/(w as any)._style?.borderColor, which casts widget internals throughany. This pattern is explicitly disallowed — it couples tests to private fields and will silently pass even if the implementation changes. Thecard()andbox()builder tests should assert on observable behaviour (rendered output,childrencontents, or public style accessors) rather than private state.
Fix: Drive assertions through the public API. For example, add atitlegetter toCard(it already stores the value for rendering) or assert that the rendered screen output contains the title string. For border colour, check the rendered cell attributes on a small screen rather than reading a private field. -
Missing
build-and-testCI check
Thebuild-and-testandbenchworkflows did not run for this PR. This typically happens when the branch is too far behindmain. Please rebase onto the latestmainand push to trigger a full CI run before this can be reviewed further.
Fix:git fetch origin && git rebase origin/main && git push --force-with-lease
Checklist:
- Fix applied
- Tests pass (bun vitest run)
- PR title: 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:
-
(widget as any)._fieldaccess in test file
File: packages/quick/src/widgets.test.ts, lines ~104, ~109-110, ~125
Problem: Tests access private widget internals via(w as any)._title,(w as any)._borderColor, and(w as any)._stylecasts. This is a project-level blocking rule — tests must not pierce widget encapsulation withas anyto read private fields. It couples tests to implementation details that can change without notice.
Fix: Assert behaviour through the public API instead. For the title, verify it appears in the rendered output (e.g.instance.toString()via@termuijs/testing). For border colour, expose a read-only public getter onCard/Boxif the property needs to be observable, or simply trust that the style is applied by checking the rendered border character colour. -
CI is missing the
build-and-testandbenchworkflow runs
Problem: Only 4 checks ran (CodeRabbit, path-labels, star-check, title-labels). Thebuild-and-testandbenchjobs did not execute, which means the build and full test suite have not been validated against this branch.
Fix: Push a new commit (even a no-opchore: trigger CI) to re-trigger all workflow runs and confirm the build passes.
Checklist:
- Fix applied
- Tests pass (bun vitest run)
- PR title: type(scope): description
RajSurajR
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/quick/src/widgets.ts`:
- Around line 499-510: The monkey-patch code in the block starting with
origRenderBorder has three issues to fix. First, import the Screen type from
'`@termuijs/core`' if not already imported, and replace the screen parameter type
annotation of any with Screen type. Second, add explanatory comments for each
any cast on lines 500, 502, and 505 following the project's guidelines that
require comments explaining why any is necessary. Third, replace the silent
catch (_e) block that swallows all errors without logging or re-throwing with
proper error handling that at least logs the error to aid debugging, rather than
silently ignoring failures in _renderSelf. This ensures guideline compliance
while maintaining visibility into runtime issues.
🪄 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: 4d9a0c9d-d1e1-44ea-93d8-2ed8bcaef1e1
📒 Files selected for processing (2)
packages/quick/src/widgets.test.tspackages/quick/src/widgets.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/quick/src/widgets.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/quick/src/index.ts`:
- Around line 35-41: In packages/quick/src/index.ts, resolve two merge conflicts
by removing the git conflict markers and retaining all exports from both sides.
At lines 35-41, remove the conflict markers (<<<<<<< HEAD, =======, >>>>>>>
upstream/main) and keep all four widget shorthand exports: card, box, tabs, and
select. At lines 68-74, similarly remove the conflict markers and keep all four
type exports: QuickCardOptions, QuickBoxOptions, QuickTabsOptions, and
QuickSelectOptions. This will allow TypeScript to compile by merging both the
new exports from this PR and the existing upstream exports rather than choosing
one side or the other.
🪄 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: e6b86a63-b8f3-4a9b-9fd0-38048d14835e
📒 Files selected for processing (3)
packages/quick/src/index.tspackages/quick/src/widgets.test.tspackages/quick/src/widgets.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/quick/src/widgets.ts
- packages/quick/src/widgets.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:
-
Private field access via
as anyincard()— monkey-patching_renderBorderand_renderSelf
File: packages/quick/src/widgets.ts, lines ~467–479
Problem: The block afterc.addChild()reads and overwrites(c as any)._renderBorderand calls(c as any)._renderSelf, which are private internals of theCardwidget. Accessing private fields throughas anycasts is a hard rule violation in this repo: it couples the builder tightly to implementation details that can change without notice, silently breaks if those methods are renamed/removed, and bypasses TypeScript's access guarantees. The swallowedcatch (_e) {}also hides real rendering errors.
Fix: Remove the entire monkey-patch block (theconst origRenderBorder = ...through the closing}). If theCardwidget requires additional configuration to render its title correctly, pass all needed options through the constructor arguments (already done above:{ title: opts?.title, borderColor }) and letCardhandle its own rendering. IfCard's public API is insufficient, open a separate issue/PR to extend it rather than patching internals here. -
screen: anyparameter in the monkey-patch closure
File: packages/quick/src/widgets.ts, line ~472
Problem: The anonymous functionfunction (screen: any)inside the monkey-patch uses an untypedanyparameter on what is effectively a public render method. Even if the surrounding block is removed (see #1), explicitanyon method parameters in non-test source files is a BLOCKING violation.
Fix: Resolved automatically by removing the monkey-patch block per issue #1.
Checklist before re-requesting review:
- Remove the
_renderBorder/_renderSelfmonkey-patch block fromcard()entirely - Verify title still renders correctly in the test (
bun vitest run packages/quick) - 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:
|
|
Two blocking issues:
Checklist:
|
2 participants
Description
This PR introduces the
card()andbox()container builders to the@termuijs/quickpackage. Thecard()function generates a padded widget featuring a custom title embedded within its top border frame, whereas thebox()function serves as a minimalist, thin-bordered utility wrapper for child layouts.Related Issue
Closes #512
Which package(s)?
@termuijs/quick(packages/quick)Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/6c6b9500-52a8-4c78-84fb-97d5e17753e3Screenshots / Recordings (UI changes)
Notes for the Reviewer
Summary by CodeRabbit
New Features
cardandboxwidget builders for simpler structured layout creation.cardsupports an optionaltitle; both widgets support optional border color customization (defaulting tobrightBlack).card/boxand their option types (replacing the previoustabs/selectshorthand exports).Tests
cardandbox: export verification, child attachment, title rendering oncard, and default border character styling.