feat: add native showResetButton option to FormBuilder - #3228
Open
tmdeveloper007 wants to merge 3 commits into
Open
feat: add native showResetButton option to FormBuilder#3228tmdeveloper007 wants to merge 3 commits into
tmdeveloper007 wants to merge 3 commits into
Conversation
github-actions
Bot
added
type:feature
📝 WalkthroughWalkthroughFormBuilder now supports optional native reset-button rendering with configurable text. UI and Vitest JSX resolution use the local ChangesFormBuilder reset button
TermUI subproject update
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 5
🧹 Nitpick comments (2)
vitest.config.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
node:pathimport prefix.-import path from 'path'; +import path from 'node:path';As per coding guidelines, TypeScript files must use the
node:prefix for Node built-ins.🤖 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 `@vitest.config.ts` at line 2, Update the path import in vitest.config.ts to use the node:path prefix while preserving the existing path usage.Source: Coding guidelines
packages/ui/src/components/FormBuilder.test.tsx (1)
48-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert reset semantics, not only matching text.
getByText("Clear Form")can succeed from any matching screen text and does not prove that a button was rendered withtype="reset". Also cover the default-disabled and default-label behavior.🤖 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/ui/src/components/FormBuilder.test.tsx` around lines 48 - 56, Strengthen the FormBuilder reset-button test by querying the rendered button element and asserting its type is "reset", rather than only matching text. Add coverage for the default-disabled behavior and default label when showResetButton is omitted, using the existing FormBuilder test cases.
🤖 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/ui/src/components/FormBuilder.test.tsx`:
- Around line 49-53: Update the FormBuilder test to instantiate the real Screen
from `@termuijs/core`, mount the FormBuilder through that Screen, call
updateRect(), then invoke Screen.render() and assert the widget output using the
Screen lifecycle rather than `@termuijs/testing` render() or
TestInstance.getByText().
In `@packages/ui/src/components/FormBuilder.ts`:
- Around line 29-37: Update the allChildren initialization in FormBuilder so an
array-valued children input is copied into a new array before showResetButton
appends the reset button. Preserve the existing handling for single children and
empty children, while ensuring repeated renders never mutate the caller-owned
children array.
- Around line 24-37: Update FormBuilder’s reset-button creation to wire a reset
callback through the form context/JSX runtime, ensuring activating the generated
type="reset" button resets controlled form state. Add or propagate the
appropriate onReset handling alongside the existing submit flow, and connect it
to the reset button created when showResetButton is enabled.
In `@packages/ui/tsconfig.json`:
- Around line 6-16: Align the JSX package specifiers used by packages/ui
configuration with the installed dependency name `@termuijs/jsx`. Update
jsxImportSource and the paths mapping in tsconfig.json, plus the corresponding
Vitest alias/runtime subpath, so all references consistently resolve to the
exact installed package.
In `@vitest.config.ts`:
- Around line 5-11: Update the esbuild jsxImportSource and matching test alias
in the Vitest configuration to use the canonical `@termuijs/jsx` package
identifier, while preserving the existing path resolution to packages/jsx/src.
---
Nitpick comments:
In `@packages/ui/src/components/FormBuilder.test.tsx`:
- Around line 48-56: Strengthen the FormBuilder reset-button test by querying
the rendered button element and asserting its type is "reset", rather than only
matching text. Add coverage for the default-disabled behavior and default label
when showResetButton is omitted, using the existing FormBuilder test cases.
In `@vitest.config.ts`:
- Line 2: Update the path import in vitest.config.ts to use the node:path prefix
while preserving the existing path usage.
🪄 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: 4f831a6f-0449-47af-9f73-48e03c6a3e97
📒 Files selected for processing (5)
TermUIpackages/ui/src/components/FormBuilder.test.tsxpackages/ui/src/components/FormBuilder.tspackages/ui/tsconfig.jsonvitest.config.ts
Comment on lines
+49
to
+53
| const screen = render( | ||
| <FormBuilder showResetButton={true} resetButtonText="Clear Form"> | ||
| <text>Child content</text> | ||
| </FormBuilder> | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)FormBuilder\.(test\.)?tsx$|FormBuilder\.ts$|FormBuilder\.tsx$|TestingScreen|testing' || true
echo
echo "== FormBuilder.test.tsx relevant lines =="
if [ -f packages/ui/src/components/FormBuilder.test.tsx ]; then
nl -ba packages/ui/src/components/FormBuilder.test.tsx | sed -n '1,140p'
fi
echo
echo "== imports/usages of render/TestInstance in packages ui tests =="
rg -n "import .*render|from '.*render|TestInstance|Screen|updateRect\\(|\\.render\\(" packages/ui/src -g '*.test.{ts,tsx}' || trueRepository: Karanjot786/TermUI
Length of output: 1104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== FormBuilder.test.tsx lines =="
awk '{printf "%6d\t%s\n", NR, $0}' packages/ui/src/components/FormBuilder.test.tsx | sed -n '1,160p'
echo
echo "== testing package entrypoints =="
for f in packages/testing/src/render.ts packages/testing/src/queries.ts packages/testing/src/index.ts; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
echo
echo "== FormBuilder imports/usages =="
awk '{printf "%6d\t%s\n", NR, $0}' packages/ui/src/components/FormBuilder.ts | sed -n '1,220p'
echo
echo "== package ui test patterns =="
rg -n "import .*render|from '.*render|TestInstance|Screen|updateRect\\(|\\.render\\(" packages/ui/src -g '*.test.{ts,tsx}' || trueRepository: Karanjot786/TermUI
Length of output: 50375
Use the required real Screen test lifecycle.
This test uses render() from @termuijs/testing instead of constructing Screen from @termuijs/core, and the expected widget output is checked through TestInstance.getByText() rather than the required Screen/updateRect()/render() lifecycle.
🤖 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/ui/src/components/FormBuilder.test.tsx` around lines 49 - 53, Update
the FormBuilder test to instantiate the real Screen from `@termuijs/core`, mount
the FormBuilder through that Screen, call updateRect(), then invoke
Screen.render() and assert the widget output using the Screen lifecycle rather
than `@termuijs/testing` render() or TestInstance.getByText().
Source: Coding guidelines
Comment on lines
24
to
+37
| const value = { | ||
| submit: () => { | ||
| if (onSubmit) onSubmit(); | ||
| } | ||
| }; | ||
| return createElement(FormContext.Provider, { value }, children); | ||
| const allChildren = children | ||
| ? (Array.isArray(children) ? children : [children]) | ||
| : []; | ||
|
|
||
| if (showResetButton) { | ||
| allChildren.push( | ||
| createElement("button", { | ||
| type: "reset" | ||
| }, resetButtonText) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
rg -n 'onReset|type.?reset|reset\\(' packages/ui packages/widgets packages/jsxRepository: Karanjot786/TermUI
Length of output: 244
🏁 Script executed:
#!/bin/sh
set -u
printf 'Tracked relevant files:\n'
git ls-files | rg '(^|/)packages/(ui|widgets|jsx)/src/.+\.ts(x)?$|(^|/)packages/(ui|widgets|jsx)/package\.json$' || true
printf '\nSearch for reset/onReset/type reset/focus/input state hooks:\n'
rg -n 'onReset|type=\s*["'\'']reset["'\'']|type: ["'\'']reset["'\'']|resetButton|showResetButton|useState|resetInput|inputState|FormBuilder' packages/ui packages/widgets packages/jsx || trueRepository: Karanjot786/TermUI
Length of output: 29084
🏁 Script executed:
#!/bin/sh
set -u
printf 'FormBuilder.ts outline:\n'
ast-grep outline packages/ui/src/components/FormBuilder.ts --view expanded || true
printf '\nFormBuilder.ts relevant lines:\n'
cat -n packages/ui/src/components/FormBuilder.ts | sed -n '1,90p'
printf '\nReconciler button reset handling search:\n'
rg -n 'onClick|onReset|type|button|reset|resetForm|clear|useState|props' packages/jsx/src/reconciler.ts packages/jsx/src/vnode.ts packages/jsx/src/jsx-runtime.ts packages/ui/src/components/FormBuilder.test.tsx | sed -n '1,240p'Repository: Karanjot786/TermUI
Length of output: 16451
🏁 Script executed:
#!/bin/sh
set -u
printf 'Event application search:\n'
rg -n 'applyDelegatedEvents|delegated|addEventListener|handleKey|onClick|type|button|type: "button"|type: "reset"' packages/jsx/src packages/ui/src | sed -n '1,260p'
printf '\ncreateIntrinsicWidget button branch (if present):\n'
sed -n '122,182p' packages/jsx/src/reconciler.ts | cat -n
printf '\nEvent system source:\n'
sed -n '1,220p' packages/jsx/src/event-system.ts | cat -n
printf '\nButton widget source:\n'
sed -n '1,220p' packages/widgets/src/input/Button.ts | cat -n
printf '\nRead-only deterministic scan for reset-specific handlers:\n'
python3 - <<'PY'
from pathlib import Path
roots = [Path('packages/jsx/src'), Path('packages/ui/src'), Path('packages/widgets/src')]
terms = ['onReset', 'type: "reset"', "type: 'reset'", 'type="reset"', 'form reset', 'clearForm', 'resetForm']
for root in roots:
for p in root.rglob('*'):
if p.suffix not in {'.ts','.tsx'} or '.test' in str(p):
continue
text = p.read_text(errors='ignore')
if any(t in text for t in terms):
print(p)
for i,line in enumerate(text.splitlines(),1):
if any(t in line for t in terms):
print(f'{i}: {line}')
PYRepository: Karanjot786/TermUI
Length of output: 40437
Add reset handling for type="reset" buttons.
FormBuilder only creates a <button type="reset">, and FormContext/the JSX runtime do not expose or handle a reset callback. As-is, activating this button will not reset controlled form state. Implement handling for this prop or wire it through an onReset path.
🤖 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/ui/src/components/FormBuilder.ts` around lines 24 - 37, Update
FormBuilder’s reset-button creation to wire a reset callback through the form
context/JSX runtime, ensuring activating the generated type="reset" button
resets controlled form state. Add or propagate the appropriate onReset handling
alongside the existing submit flow, and connect it to the reset button created
when showResetButton is enabled.
Comment on lines
+29
to
+37
| const allChildren = children | ||
| ? (Array.isArray(children) ? children : [children]) | ||
| : []; | ||
|
|
||
| if (showResetButton) { | ||
| allChildren.push( | ||
| createElement("button", { | ||
| type: "reset" | ||
| }, resetButtonText) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not mutate the caller’s children array.
When children is already an array, allChildren aliases that array and push mutates parent-owned data. Re-rendering with the same array can append another reset button each time.
- const allChildren = children
- ? (Array.isArray(children) ? children : [children])
+ const allChildren = children
+ ? (Array.isArray(children) ? [...children] : [children])
: [];📝 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.
Suggested change
| const allChildren = children | |
| ? (Array.isArray(children) ? children : [children]) | |
| : []; | |
| if (showResetButton) { | |
| allChildren.push( | |
| createElement("button", { | |
| type: "reset" | |
| }, resetButtonText) | |
| const allChildren = children | |
| ? (Array.isArray(children) ? [...children] : [children]) | |
| : []; | |
| if (showResetButton) { | |
| allChildren.push( | |
| createElement("button", { | |
| type: "reset" | |
| }, resetButtonText) |
🤖 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/ui/src/components/FormBuilder.ts` around lines 29 - 37, Update the
allChildren initialization in FormBuilder so an array-valued children input is
copied into a new array before showResetButton appends the reset button.
Preserve the existing handling for single children and empty children, while
ensuring repeated renders never mutate the caller-owned children array.
Comment on lines
+6
to
+16
| "jsx": "react-jsx", | ||
| "jsxImportSource": "@termui/jsx", | ||
| "declaration": true, | ||
| "outDir": "dist", | ||
| "rootDir": "src", | ||
| "strict": true, | ||
| "esModuleInterop": true, | ||
| "skipLibCheck": true, | ||
| "baseUrl": ".", | ||
| "paths": { | ||
| "@termui/jsx": ["../jsx"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
fd -i 'jsx-runtime' packages/jsx
rg -n 'jsxImportSource|`@termui/jsx`' packages/ui packages/jsx vitest.config.tsRepository: Karanjot786/TermUI
Length of output: 1037
🏁 Script executed:
#!/bin/sh
set -eu
echo "== package manifests =="
for f in package.json packages/ui/package.json packages/jsx/package.json; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,160p' "$f"
fi
done
echo
echo "== relevant jsx runtime source =="
sed -n '1,220p' packages/jsx/src/jsx-runtime.ts
echo
echo "== all jsxImportSource occurrences =="
rg -n 'jsxImportSource|jsx-runtime|jsxRuntime' packages/ui packages/jsx package.json packages/ui/tsconfig.json vitest.config.tsRepository: Karanjot786/TermUI
Length of output: 11293
🏁 Script executed:
#!/bin/sh
set -eu
echo "== package names / tsconfig jsxImportSource mismatch =="
python3 - <<'PY'
import json
from pathlib import Path
ui = json.loads(Path("packages/ui/package.json").read_text())
jsx = json.loads(Path("packages/jsx/package.json").read_text())
config = json.loads(Path("packages/ui/tsconfig.json").read_text())
deps = set(ui.get("dependencies", {}).keys())
print("packages/ui/package.json name:", ui.get("name"))
print("packages/jsx/package.json name:", jsx.get("name"))
print("packages/ui/package.json dependencies include `@termui/jsx`:", "`@termui/jsx`" in deps)
print("packages/ui/package.json dependencies include `@termuijs/jsx`:", "`@termuijs/jsx`" in deps)
print("packages/ui/tsconfig.json jsxImportSource:", config.get("compilerOptions", {}).get("jsxImportSource"))
print("packages/ui/tsconfig.json paths:", config.get("compilerOptions", {}).get("paths"))
print("packages/jsx/package.json has jsx-runtime export:", "./jsx-runtime" in jsx.get("exports", {}))
PYRepository: Karanjot786/TermUI
Length of output: 604
Use the installed JSX package specifiers consistently.
packages/ui depends on @termuijs/jsx, but packages/ui/tsconfig.json and vitest.config.ts use @termui/jsx for jsxImportSource/paths. Keep both the package alias and the runtime subpath mapped to the package with that exact name, or align all references to @termuijs/jsx everywhere.
🤖 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/ui/tsconfig.json` around lines 6 - 16, Align the JSX package
specifiers used by packages/ui configuration with the installed dependency name
`@termuijs/jsx`. Update jsxImportSource and the paths mapping in tsconfig.json,
plus the corresponding Vitest alias/runtime subpath, so all references
consistently resolve to the exact installed package.
Comment on lines
+5
to
+11
| esbuild: { | ||
| jsx: 'automatic', | ||
| jsxImportSource: '@termui/jsx', | ||
| }, | ||
| test: { | ||
| alias: { | ||
| '@termui/jsx': path.resolve(__dirname, './packages/jsx/src'), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
rg -n '`@termuijsx/jsx`|`@termui/jsx`|__termuijs_instances' \
packages/ui packages/testing packages/jsx vitest.config.tsRepository: Karanjot786/TermUI
Length of output: 994
🏁 Script executed:
#!/bin/sh
set -eu
printf 'tracked files of interest:\n'
git ls-files | rg '(^|/)(`@termuijsx/jsx`|`@termui/jsx`|render\.ts|globals\.ts|package\.json|tsconfig\.json|vitest\.config\.ts)$|packages/(jsx|testing|ui)'
printf '\npackage manifests mentioning jsx identifiers:\n'
for f in package.json packages/*/package.json; do
[ -f "$f" ] || continue
if rg -q '"name"|"`@termui/jsx`"|"`@termuijsx/jsx`"|"jsx"' "$f"; then
echo "--- $f"
sed -n '1,220p' "$f"
fi
done
printf '\nimport/require usages of jsx package identifiers:\n'
rg -n "(`@termuijsx/jsx`|`@termui/jsx`|from ['\"]`@termui`" packages | head -200
printf '\nrender.ts relevant sections:\n'
sed -n '320,720p' packages/testing/src/render.ts
printf '\njsx globals.ts:\n'
sed -n '1,80p' packages/jsx/src/globals.tsRepository: Karanjot786/TermUI
Length of output: 49519
🏁 Script executed:
#!/bin/sh
set -eu
printf 'all occurrences of `@termuijsx/jsx` / `@termui/jsx` (text search):\n'
rg -n '`@termuijsx/jsx`|`@termui/jsx`|from\s+["'\'']`@termuijs/jsx`|jsxImportSource' . \
--glob '!node_modules' --glob '!dist' --glob '!build' \
--glob '!examples/**/.bun' --glob '!examples/**/dist' \
| head -500
printf '\nresolve config aliases and jsx source path from vitest config package-name semantics if possible:\n'
python3 - <<'PY'
from pathlib import Path
p = Path('packages/jsx/package.json')
print(p.read_text().splitlines()[8])
print('packages/jsx/src/jsx-runtime.ts exists:', Path('packages/jsx/src/jsx-runtime.ts').exists())
print('packages/jsx/src/jsx-runtime.d.ts exists:', Path('packages/jsx/src/jsx-runtime.d.ts').exists())
PY
printf '\nsource package jsx-runtime files:\n'
sed -n '1,120p' packages/jsx/src/jsx-runtime.ts
fs=$(find packages/jsx/src -maxdepth 1 -name 'jsx-runtime.*' -print | tr '\n' ', ')
echo "$fs"Repository: Karanjot786/TermUI
Length of output: 18224
Keep the config JSX runtime alias aligned with the package identifier.
vitest.config.ts compiles via @termui/jsx, but the published/internal package identity is @termuijs/jsx and the JSX runtime docs/runtime source also use @termuijs/jsx. This leaves this config potentially compiling tests through a different source path than the rest of the repo.
🤖 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 `@vitest.config.ts` around lines 5 - 11, Update the esbuild jsxImportSource and
matching test alias in the Vitest configuration to use the canonical
`@termuijs/jsx` package identifier, while preserving the existing path resolution
to packages/jsx/src.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of What Has Been Done
Added a
showResetButtonoption to theFormBuildercomponent that allows rendering a form reset button. WhenshowResetButton={true}, a<button type="reset">is appended to the form children with configurable text viaresetButtonText.Changes Made
showResetButtonandresetButtonTextprops; fixed children array handling to properly append the reset button@termui/jsxpath aliasImpact it Made
Allows forms built with FormBuilder to include a native reset button without manual child addition.
Closes #2706
Note: Please assign this PR to the
tmdeveloper007account.Summary by CodeRabbit
New Features
Bug Fixes