Fix PIN input displaying double asterisks for each digit - #114
Conversation
The PinScreen component was rendering both a manually created maskedPin text element and a TextInput component with mask prop, causing each digit to display twice as many asterisks. Removed the redundant maskedPin rendering - TextInput already handles the masking via its mask prop. Fixes #113
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the PIN input field displayed two asterisks for each digit entered. The root cause was that both a manually rendered maskedPin text element and the TextInput component's built-in mask prop were being displayed simultaneously.
- Removed the redundant
maskedPinvariable and its rendering - Simplified the JSX structure by directly rendering the
TextInputcomponent - Added a regression test to detect if this double-masking pattern reappears
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/interactive.tsx | Removed redundant maskedPin variable and simplified PinScreen JSX to only use TextInput's built-in mask prop |
| src/interactive.test.ts | Added regression test using static source code analysis to detect the double-masking pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get the source directory (tests run from dist/, source is in src/) | ||
| const srcDir = __dirname.replace(/\/dist$/, '/src'); |
There was a problem hiding this comment.
The path resolution logic assumes tests run from the dist/ directory, but this may be fragile. The regex replacement /\/dist$/ will fail silently if the test is run from a different location (e.g., during development from the project root). Consider using a more robust approach like resolving relative to the test file location using join(__dirname, '..', '..', 'src') or checking if the path actually exists before attempting to read the file.
| // Get the source directory (tests run from dist/, source is in src/) | |
| const srcDir = __dirname.replace(/\/dist$/, '/src'); | |
| // Get the source directory: project root is parent of compiled test dir (e.g., dist/), source is in src/ | |
| const projectRoot = dirname(__dirname); | |
| const srcDir = join(projectRoot, 'src'); |
| const source = readFileSync(sourceFile, 'utf-8'); | ||
|
|
||
| // Find the PinScreen function body | ||
| const pinScreenMatch = source.match(/function PinScreen\([^)]*\)[^{]*\{([\s\S]*?)^function /m); |
There was a problem hiding this comment.
The regex pattern /function PinScreen\([^)]*\)[^{]*\{([\s\S]*?)^function /m is brittle and may break if the code is refactored (e.g., arrow functions, multiple functions on the same line, or minified code). Additionally, it captures everything until the next function declaration, which could fail if there are nested functions or no subsequent function. Consider using static analysis tools or AST parsing libraries like @babel/parser for more reliable code inspection, or alternatively, test the actual runtime behavior instead of parsing source code.
| it('should not render both manual maskedPin and TextInput with mask prop (causes double asterisks)', () => { | ||
| // Read the source file to check for the bug pattern | ||
| const sourceFile = join(srcDir, 'interactive.tsx'); | ||
| const source = readFileSync(sourceFile, 'utf-8'); | ||
|
|
||
| // Find the PinScreen function body | ||
| const pinScreenMatch = source.match(/function PinScreen\([^)]*\)[^{]*\{([\s\S]*?)^function /m); | ||
| assert.ok(pinScreenMatch, 'PinScreen function should exist'); | ||
|
|
||
| const pinScreenBody = pinScreenMatch[1]; | ||
|
|
||
| // Check if there's a maskedPin variable being rendered alongside TextInput with mask | ||
| const hasMaskedPinRendered = /\{maskedPin\}/.test(pinScreenBody ?? ''); | ||
| const hasTextInputWithMask = /TextInput[\s\S]*?mask=/.test(pinScreenBody ?? ''); | ||
|
|
||
| // The bug is when both maskedPin is rendered AND TextInput has mask prop | ||
| const hasBug = hasMaskedPinRendered && hasTextInputWithMask; | ||
|
|
||
| assert.strictEqual( | ||
| hasBug, | ||
| false, | ||
| 'PinScreen should not render both maskedPin text and TextInput with mask prop - this causes double asterisks. ' + | ||
| 'Either remove the maskedPin rendering or remove the mask prop from TextInput.' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The test uses static source code analysis instead of testing actual component behavior. This is an anti-pattern because: (1) it tests implementation details rather than behavior, (2) it's fragile and breaks with refactoring, and (3) it doesn't actually verify that the bug is fixed at runtime. For React components, consider using a proper testing approach such as rendering the component with ink-testing-library or at minimum testing that the exported component exists and has the expected structure. Static source analysis should be a last resort for testing.
- Add ink-testing-library for proper component testing - Replace source-code-parsing test with behavioral tests - Export PinScreen and PinScreenProps for testing - Add tests that demonstrate the double-masking bug pattern - Verify TextInput mask prop produces single mask per digit Addresses Copilot feedback on PR #114
* Improve PinScreen testing with ink-testing-library - Add ink-testing-library for proper component testing - Replace source-code-parsing test with behavioral tests - Export PinScreen and PinScreenProps for testing - Add tests that demonstrate the double-masking bug pattern - Verify TextInput mask prop produces single mask per digit Addresses Copilot feedback on PR #114 * Include tsx test files in ESLint relaxed rules
Summary
maskedPintext element -TextInputalready handles masking via itsmaskpropRoot Cause
The
PinScreencomponent was rendering both:maskedPin('•'.repeat(pin.length))TextInputcomponent withmask="•"Both were displayed simultaneously, causing double masking.
Test Plan
Fixes #113