feat: added support for masking passwords #13552
Conversation
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe PR forwards an optional password flag through the input-box flow and uses it to render a masked input. The main process includes options?.password in the showInputBox payload. The renderer adds password?: boolean to InputBoxOptions, QuickPickInput.svelte reads that flag and sets the input element type to "password" when true (otherwise "text"), and tests were added to assert the rendered input type based on the flag. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Out-of-scope changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed hel 8000 p? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Hey @gastoner - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/renderer/src/lib/dialogs/QuickPickInput.svelte:48` </location>
<code_context>
placeHolder = options?.placeHolder;
title = options?.title;
currentId = options?.id ?? 0;
+ password = options?.password ?? false;
if (options?.prompt) {
prompt = `${options.prompt} (${DEFAULT_PROMPT})`;
</code_context>
<issue_to_address>
Check for non-boolean values in 'options.password' to avoid unexpected input types.
Explicitly cast 'options.password' to a boolean to prevent type issues and ensure consistent behavior.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
password = options?.password ?? false;
=======
password = Boolean(options?.password);
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `packages/main/src/plugin/input-quickpick/input-quickpick-registry.ts:74` </location>
<code_context>
prompt: options?.prompt,
markdownDescription: options?.markdownDescription,
multiline: options?.multiline,
+ password: options?.password,
validate,
ignoreFocusOut: options?.ignoreFocusOut,
</code_context>
<issue_to_address>
Validate that 'options.password' is a boolean before passing it through.
Consider coercing 'options.password' to a boolean to prevent unexpected behavior if it is undefined or not a boolean.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
password: options?.password,
=======
password: !!options?.password,
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| placeHolder = options?.placeHolder; | ||
| title = options?.title; | ||
| currentId = options?.id ?? 0; | ||
| password = options?.password ?? false; |
There was a problem hiding this comment.
suggestion (bug_risk): Check for non-boolean values in 'options.password' to avoid unexpected input types.
Explicitly cast 'options.password' to a boolean to prevent type issues and ensure consistent behavior.
| password = options?.password ?? false; | |
| password = Boolean(options?.password); |
| prompt: options?.prompt, | ||
| markdownDescription: options?.markdownDescription, | ||
| multiline: options?.multiline, | ||
| password: options?.password, |
There was a problem hiding this comment.
suggestion: Validate that 'options.password' is a boolean before passing it through.
Consider coercing 'options.password' to a boolean to prevent unexpected behavior if it is undefined or not a boolean.
| password: options?.password, | |
| password: !!options?.password, |
| render(QuickPickInput, {}); | ||
|
|
||
| const area = await screen.findByPlaceholderText('Some placeholder'); | ||
| expect(area).toBeInTheDocument(); | ||
| expect(area).toHaveAttribute('type', type); |
There was a problem hiding this comment.
| render(QuickPickInput, {}); | |
| const area = await screen.findByPlaceholderText('Some placeholder'); | |
| expect(area).toBeInTheDocument(); | |
| expect(area).toHaveAttribute('type', type); | |
| const { getByPlaceholderText } = render(QuickPickInput, {}); | |
| const area = getByPlaceholderText('Some placeholder'); | |
| expect(area).toBeInstanceOf(HTMLInputElement); | |
| expect(area).toHaveAttribute('type', type); |
suggestion(non-blocking): personal preference
- to use result from render
- use
getinstead offind - verify instance
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
packages/main/src/plugin/input-quickpick/input-quickpick-registry.ts (1)
139-141: Bug: deleting from wrong map leaks QuickPick callbacksonQuickPickValuesSelected removes entries from callbacksInputBox instead of callbacksQuickPicks. This leaks QuickPick callbacks and can cause memory growth and incorrect behavior on subsequent selections.
Apply this fix:
- // remove the callback - this.callbacksInputBox.delete(id); + // remove the callback + this.callbacksQuickPicks.delete(id);
🧹 Nitpick comments (2)
packages/renderer/src/lib/dialogs/QuickPickInput.svelte (1)
335-343: Optional hardening: disable autocomplete/spellcheck for password inputsTo reduce accidental disclosure or storage, disable assist features when password is active.
<input bind:this={inputElement} on:input={onInputChange} - type={mode === 'InputBox' && password ? 'password' : 'text'} + type={mode === 'InputBox' && password ? 'password' : 'text'} + autocomplete={password ? 'new-password' : 'off'} + autocapitalize="off" + autocorrect="off" + spellcheck="false" bind:value={inputValue}packages/renderer/src/lib/dialogs/QuickPickInput.spec.ts (1)
211-214: Nit: clarify dynamic test titleCurrent '%s' placeholder receives the full object. Consider a clearer title using named params:
-test.each([ +test.each([ { password: true, type: 'password' }, { password: false, type: 'text' }, -])('Expect that input is displayed correctly with type %s', async ({ password, type }) => { +])('Expect input type is $type when password=$password', async ({ password, type }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/main/src/plugin/input-quickpick/input-quickpick-registry.ts(1 hunks)packages/renderer/src/lib/dialogs/QuickPickInput.spec.ts(1 hunks)packages/renderer/src/lib/dialogs/QuickPickInput.svelte(3 hunks)packages/renderer/src/lib/dialogs/quickpick-input.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/renderer/src/lib/dialogs/QuickPickInput.spec.ts (1)
packages/renderer/src/lib/dialogs/quickpick-input.ts (1)
InputBoxOptions(33-46)
🪛 GitHub Check: typecheck
packages/renderer/src/lib/dialogs/QuickPickInput.spec.ts
[failure] 217-217:
Property 'multiline' is missing in type '{ password: boolean; validate: false; placeHolder: string; prompt: string; id: number; }' but required in type 'InputBoxOptions'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linter, formatters
- GitHub Check: build website
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: smoke e2e tests (development)
- GitHub Check: unit tests / macos-15
- GitHub Check: smoke e2e tests (production)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: Windows
🔇 Additional comments (2)
packages/renderer/src/lib/dialogs/quickpick-input.ts (1)
45-46: Adding password?: boolean to InputBoxOptions looks correctThe new flag aligns with the intended API and is consumed in the renderer. No issues here.
packages/main/src/plugin/input-quickpick/input-quickpick-registry.ts (1)
74-76: Propagate password to renderer (LGTM)Including password: options?.password in the showInputBox payload correctly surfaces the flag to the frontend.
| test.each([ | ||
| { password: true, type: 'password' }, | ||
| { password: false, type: 'text' }, | ||
| ])('Expect that input is displayed correctly with type %s', async ({ password, type }) => { | ||
| const idRequest = 123; | ||
|
|
||
| const inputBoxOptions: InputBoxOptions = { | ||
| password: password, | ||
| validate: false, | ||
| placeHolder: 'Some placeholder', | ||
| prompt: 'Enter a value', | ||
| id: idRequest, | ||
| }; | ||
|
|
||
| receiveFunctionMock.mockImplementation((message: string, callback: (options: InputBoxOptions) => void) => { | ||
| if (message === 'showInputBox:add') { | ||
| callback(inputBoxOptions); | ||
| } | ||
| }); | ||
|
|
||
| render(QuickPickInput, {}); | ||
|
|
||
| const area = await screen.findByPlaceholderText('Some placeholder'); | ||
| expect(area).toBeInTheDocument(); | ||
| expect(area).toHaveAttribute('type', type); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a regression test: QuickPick filter should never be masked
After showing an InputBox with password=true, ensure a subsequent QuickPick renders a text input (not password). This guards against state leakage.
test('QuickPick filter is text even after password InputBox', async () => {
const idRequest = 123;
// First, show a password InputBox
const inputBoxOptions: InputBoxOptions = {
password: true,
multiline: false,
validate: false,
placeHolder: 'Secret',
prompt: 'Enter secret',
id: idRequest,
};
receiveFunctionMock.mockImplementation((message: string, cb: (opts: any) => void) => {
if (message === 'showInputBox:add') cb(inputBoxOptions);
});
render(QuickPickInput, {});
// Close InputBox
await userEvent.keyboard('{Escape}');
// Then, show a QuickPick
const quickPickOptions: QuickPickOptions = {
items: ['a', 'b'],
canPickMany: false,
placeHolder: 'Filter items',
prompt: '',
id: idRequest,
onSelectCallback: false,
};
receiveFunctionMock.mockImplementation((message: string, cb: (opts: any) => void) => {
if (message === 'showQuickPick:add') cb(quickPickOptions);
});
// Re-render for QuickPick
render(QuickPickInput, {});
const input = screen.getByPlaceholderText('Filter items');
expect(input).toHaveAttribute('type', 'text');
});🧰 Tools
🪛 GitHub Check: typecheck
[failure] 217-217:
Property 'multiline' is missing in type '{ password: boolean; validate: false; placeHolder: string; prompt: string; id: number; }' but required in type 'InputBoxOptions'.
🤖 Prompt for AI Agents
In packages/renderer/src/lib/dialogs/QuickPickInput.spec.ts around lines 211 to
237, add a regression test that ensures QuickPick filter input is always
type="text" after a password InputBox; implement it by first mocking
receiveFunctionMock to return an InputBoxOptions with password: true and
rendering QuickPickInput, then simulate closing the InputBox (await
userEvent.keyboard('{Escape}')), then mock receiveFunctionMock to return
QuickPickOptions (items, placeHolder 'Filter items', etc.), re-render
QuickPickInput and assert the filter input has attribute type='text'; make sure
to reset or use mockImplementationOnce/clearMocks between the two mocked
responses so the second mock isn't overridden by the first.
| let currentId = 0; | ||
| let title: string | undefined = ''; | ||
| let multiline = false; | ||
| let password = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset password state to avoid stale masking across dialogs
password isn’t reset in cleanup or when showing QuickPick, so subsequent QuickPick inputs may remain masked. Reset explicitly:
@@
let multiline = false;
-let password = false;
+let password = false;
@@
const showQuickPickCallback = (quickpickParameter: unknown): void => {
const options: QuickPickOptions | undefined = quickpickParameter as QuickPickOptions;
mode = 'QuickPick';
+ // ensure QuickPick filter is never masked
+ password = false;
@@
function cleanup(): void {
display = false;
inputValue = '';
placeHolder = '';
prompt = '';
validationEnabled = false;
onSelectCallbackEnabled = false;
quickPickCanPickMany = false;
quickPickSelectedFilteredIndex = 0;
quickPickSelectedIndex = 0;
ignoreFocusOut = false;
+ password = false;
}Also applies to: 48-49
🤖 Prompt for AI Agents
In packages/renderer/src/lib/dialogs/QuickPickInput.svelte around line 19 (and
also affecting lines 48-49), the let password = false state is not being reset
when the dialog is closed or reopened, which can cause subsequent QuickPick
dialogs to remain masked; update the component to explicitly reset password to
false whenever the QuickPick is shown (e.g., just before opening/showing the
dialog) and in the cleanup/destroy path (e.g., on close or onDestroy) so the
mask state does not persist across dialog instances.
| bind:this={inputElement} | ||
| on:input={onInputChange} | ||
| type="text" | ||
| type={password ? 'password' : 'text'} |
There was a problem hiding this comment.
Avoid masking the QuickPick filter; gate password by mode
Ensure the input is masked only for InputBox password fields, not for QuickPick filtering.
- type={password ? 'password' : 'text'}
+ type={mode === 'InputBox' && password ? 'password' : 'text'}📝 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.
| type={password ? 'password' : 'text'} | |
| type={mode === 'InputBox' && password ? 'password' : 'text'} |
🤖 Prompt for AI Agents
In packages/renderer/src/lib/dialogs/QuickPickInput.svelte around line 338, the
input type is set based solely on the password prop which causes QuickPick's
filter input to be masked; change the condition so the field is masked only when
the dialog mode is an InputBox with password enabled (i.e., check both mode ===
'input' (or the actual InputBox mode value used in the component) and password
=== true) and keep QuickPick filter inputs as plain text.
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
a90a3cc to
10f0438
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
What does this PR do?
Adds support to QuickPickInput to be able to mask password when passord is set to true
Screenshot / video of UI
Old:
Current:
What issues does this PR fix or reference?
Closes #13476
How to test this PR?
Unit tests
Steps in the issue