Fix: password visibility toggle doesn't work due to disconnected reff - #3416
Fix: password visibility toggle doesn't work due to disconnected reff#3416Alexx3890 wants to merge 2 commits into
Conversation
## Summary Fixes the password visibility toggle in the `auth-flow` example. Previously, pressing `v` or `Enter` while the **Toggle** control was focused had no effect, and the `[Show]/[Hide]` label never updated. ## Root cause `LoginScreen` created a `passwordRef` with `useRef`, but never passed it to `PasswordInputJSX`. Instead, `PasswordInputJSX` created its own internal ref and used it to instantiate the `PasswordInput` widget. As a result, `LoginScreen`'s `passwordRef.current` always remained `null`, so calls to `toggleVisibility()` were no-ops. Additionally, the visibility label was derived from `passwordRef.current?.showText` during render. Since mutating a ref does not trigger a re-render, the label could become out of sync with the widget's visibility state. ## Fix * Pass the parent-owned `passwordRef` to `PasswordInputJSX` via a `widgetRef` prop instead of creating a separate internal ref. * Track password visibility using component state (`isVisible`) and synchronize it with the widget after calling `toggleVisibility()`. * Extract the password toggle logic into a shared `togglePasswordVisibility()` helper. * Reuse a single `submit()` handler for both the `enter` and `return` key bindings to avoid duplicated logic.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe auth flow connects ChangesAuthentication flow
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Possibly related PRs
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 |
![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, @ankit3890.
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
🤖 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 `@examples/auth-flow/src/index.tsx`:
- Line 100: Update PasswordInputJSX to return the existing PasswordInput type
instead of casting widgetRef.current to any. Preserve the current ref behavior,
and add an inline comment only if the TermUI JSX type system requires a
remaining workaround.
🪄 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: ff2d6b36-733a-4d6f-890d-f3cfe18d79cc
📒 Files selected for processing (1)
examples/auth-flow/src/index.tsx
There was a problem hiding this comment.
Pull request overview
This PR fixes the non-functional password visibility toggle in the examples/auth-flow login example by wiring the parent passwordRef into the PasswordInput instance and consolidating duplicated toggle/submit key handling.
Changes:
- Connects
LoginScreen’spasswordReftoPasswordInputJSXvia awidgetRefprop so parent actions reach the actual widget. - Adds local
isVisiblestate to re-render and keep the toggle label updated when visibility is toggled. - Refactors Enter/Return handling into a shared
submit()helper and centralizes visibility toggling logic.
Suppressed comments (2)
examples/auth-flow/src/index.tsx:197
- To keep the "[Show]/[Hide]" label synchronized with the PasswordInput widget (including Alt+V toggles while the password field is focused), pass the visibility-change callback from LoginScreen into PasswordInputJSX.
value={password}
onChange={setPassword}
placeholder="Enter password..."
isFocused={focusedIndex === 1}
widgetRef={passwordRef}
examples/auth-flow/src/index.tsx:98
- When PasswordInputJSX forwards key events to PasswordInput.handleKey(), Alt+V can toggle visibility inside the widget without updating the LoginScreen label. Invoke the visibility callback after handling Alt+V so the UI stays in sync.
useInput((_key: string, event: KeyEvent) => {
if (!isFocused || !widgetRef.current) return;
widgetRef.current.handleKey(event);
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function PasswordInputJSX({ | ||
| value, | ||
| onChange, | ||
| placeholder, | ||
| isFocused, | ||
| widgetRef, | ||
| }: { | ||
| value: string; | ||
| onChange: (val: string) => void; | ||
| placeholder?: string; | ||
| isFocused: boolean; | ||
| widgetRef: { current: PasswordInput | null }; | ||
| }) { |
Alexx3890
left a comment
Alexx3890
left a comment
There was a problem hiding this comment.
Addressed the CodeRabbit suggestion — removed as any from both
TextInputJSX and PasswordInputJSX, returning the concrete
TextInput / PasswordInput types instead, since ref.current /
widgetRef.current are already narrowed to non-null at that point.
Added a short inline comment explaining why these return a widget
instance rather than a standard JSX.Element (TermUI's JSX runtime
accepts widget instances directly as render output).
Also restored the key && guard in TextInputJSX's default case
that had been dropped in an earlier revision — without it, a
falsy/empty key for a non-printable keystroke could throw on
key.length.
One open item: I haven't run pnpm typecheck against this yet to
confirm TermUI's JSX pragma doesn't require these to satisfy
JSX.Element at the call site. If it does, we'll need a narrower
cast (e.g. as unknown as JSX.Element) instead of a bare return —
will confirm before merge.
Description
Fixes the password visibility toggle in the
examples/auth-flowlogin example by connecting the parentpasswordRefto thePasswordInputinstance and keeping the visibility label synchronized with the widget state. It also removes duplicated toggle and submit logic by introducing shared helper functions.Related Issue
Closes #3415
Which package(s)?
examples/auth-flowType 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 typecheck[CONTRIBUTING.md](https://chatgpt.com/c/CONTRIBUTING.md).type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
Screenshots / Recordings (UI changes)
N/A
Notes for the Reviewer
This PR fixes the password visibility toggle in the
examples/auth-flowexample by ensuring the parent component andPasswordInputJSXshare the samePasswordInputinstance. It also synchronizes the visibility label with the widget state and consolidates duplicated toggle and submit logic for easier maintenance.Summary by CodeRabbit
Bug Fixes
Refactor