Desktop: Replace smalltalk with React Dialog to add password visibility in encryption setup#14739
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
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:
📝 WalkthroughWalkthroughReplaces the direct master-password prompt in EncryptionConfigScreen with an internal enable-encryption dialog implemented via component state and a Promise-like resolver; integrates password handling, validation and error paths, preserves existing master-password fallback, and updates styles and a password-input type alias. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
added
enhancement
|
I have read the CLA Document and I hereby sign the CLA |
![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/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx (1)
271-276: Narrow error type before accessing.message.For consistency with the existing pattern at lines 85-87, narrow the error type to avoid potential runtime issues if a non-Error value is thrown.
Proposed fix
try { await toggleAndSetupEncryption(EncryptionService.instance(), newEnabled, masterKey, ''); } catch (error) { - await dialogs.alert(error.message); + const message = error instanceof Error ? error.message : String(error); + await dialogs.alert(message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx` around lines 271 - 276, The catch block that calls dialogs.alert(error.message) should narrow the error type before accessing .message to avoid runtime issues; update the catch to check if the caught value is an instance of Error (or has a message property) and call dialogs.alert(error.message) for Error instances, otherwise call dialogs.alert(String(error)) or a fallback string; apply this change around the toggleAndSetupEncryption(...) call in the try/catch so it matches the pattern used earlier (lines ~85-87).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx`:
- Around line 239-258: onEnableEncryptionConfirm currently closes the password
prompt immediately, causing the dialog to disappear on validation failure; move
setEnableEncryptionPromptVisible(false) to after successful validation and after
toggleAndSetupEncryption completes so the dialog stays open on errors; ensure
the invalid-password alert uses localization via _('Invalid password. Please try
again. If you have forgotten your password you will need to reset it.') and when
catching errors from toggleAndSetupEncryption narrow the error type before
accessing .message (follow the existing pattern used elsewhere) and pass a
localized message to dialogs.alert; references: onEnableEncryptionConfirm,
masterPasswordIsValid, toggleAndSetupEncryption, EncryptionService.instance(),
dialogs.alert, props.masterPassword.
---
Nitpick comments:
In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx`:
- Around line 271-276: The catch block that calls dialogs.alert(error.message)
should narrow the error type before accessing .message to avoid runtime issues;
update the catch to check if the caught value is an instance of Error (or has a
message property) and call dialogs.alert(error.message) for Error instances,
otherwise call dialogs.alert(String(error)) or a fallback string; apply this
change around the toggleAndSetupEncryption(...) call in the try/catch so it
matches the pattern used earlier (lines ~85-87).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3c3c8f9-f0cf-4dcf-b222-6b551efaffba
📒 Files selected for processing (1)
packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx
This is not the case anymore, as a recent change means that the manage master password dialog will open instead when clicking enable encryption if the password is not yet set. So now the current smalltalk dialog will only open when needing to match against an existing password. A show password button here would still be nice, but the change is no longer a necessity |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx (1)
344-353: Consider adding accessibility attributes for the error state.The
PasswordInputcomponent acceptsaria-invalidandaria-errormessageprops (per context snippet 4). When an error is present, these attributes should be set to improve screen reader support.Proposed enhancement
+ const errorId = 'enable-encryption-error'; <PasswordInput inputId="enable-encryption-password" value={enableEncryptionPassword} onChange={onPasswordInputChange} + aria-invalid={!!enableEncryptionError} + aria-errormessage={enableEncryptionError ? errorId : undefined} /> {enableEncryptionError && ( - <div style={{ ...theme.textStyle, color: theme.colorError, marginTop: 10, marginBottom: 10 }}> + <div id={errorId} role="alert" style={{ ...theme.textStyle, color: theme.colorError, marginTop: 10, marginBottom: 10 }}> {enableEncryptionError} </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx` around lines 344 - 353, When render shows PasswordInput with enableEncryptionPassword and enableEncryptionError, mark the input as accessible by passing aria-invalid={!!enableEncryptionError} and aria-errormessage pointing to the error message element (e.g., "enable-encryption-password-error"); give the error <div> that id and only render aria-errormessage when enableEncryptionError exists so screen readers announce the message; update the PasswordInput props (from the onPasswordInputChange render) to include these attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx`:
- Around line 279-284: The catch block in the disable-encryption flow calls
dialogs.alert(error.message) without narrowing the caught value; update the
catch in the handler that calls
toggleAndSetupEncryption(EncryptionService.instance(), newEnabled, masterKey,
'') to check if error is an instance of Error before accessing .message and
otherwise fallback to String(error) (mirror the pattern used in
onEnableEncryptionConfirm), then pass that safe string to dialogs.alert so
non-Error throws can't cause a runtime crash.
---
Nitpick comments:
In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx`:
- Around line 344-353: When render shows PasswordInput with
enableEncryptionPassword and enableEncryptionError, mark the input as accessible
by passing aria-invalid={!!enableEncryptionError} and aria-errormessage pointing
to the error message element (e.g., "enable-encryption-password-error"); give
the error <div> that id and only render aria-errormessage when
enableEncryptionError exists so screen readers announce the message; update the
PasswordInput props (from the onPasswordInputChange render) to include these
attributes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc71c29a-48fd-4766-92f1-c5c434e1b579
📒 Files selected for processing (1)
packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx
Thanks for the context! By the way, this change is specifically for the scenario where a user has already set a password, disabled encryption, and then tries to re-enable it. During that re-enabling step, the smalltalk prompt still pops up, and they currently can't see the password they are typing to confirm. Aside from the eye icon, I thought replacing it would be a good change for UI consistency (using a native React ) and reducing reliance on the 3rd-party smalltalk dependency. What are your thoughts on this? Is it still a worthwhile improvement to keep? |
|
@himanshumishra1309 I had a quick look through your code change and in light of #14664 being merged, I think your change is over-engineered and potentially incorrect. You have effectively removed some of the logic from #14664 which was just added, and changed the conditions to show your dialog instead, which it's unclear whether it supports create new master password functionality as well. If you want to continue with this code change, I would suggest that you do not change any of the conditions for which the existing smalltalk dialog opens, but only replace this dialog with a React Dialog that includes a show password button. |
Sure, I'll work on this |
I have made the changes accordingly and kept the existing conditions intact, please let me know if there is any other sort of improvement required from my side. Thank you very much for your guidance. |
|
Hi @mrjo118, just following up on this PR. Based on your suggestion, I reverted the earlier changes and updated the implementation so that the existing conditions and flow remain exactly the same. The only thing changed now is replacing the smalltalk dialog with a React Dialog that includes the show/hide password toggle. I just wanted to ask if this approach looks good to you, or if you would like me to make any further changes. Happy to update the PR if something else is expected from my side. Thanks again for the guidance! |
|
@himanshumishra1309 The change looks ok now, but there is one thing that feels off. The width of the new dialog is not consistent with the width of the React Dialog when you click manage master password: 
It would be good if you can make the width match and the grey background shading as well. See in main.scss it has this css for the manage master password dialog: Change it to: And add the enable-encryption-dialog class to your dialog, to make the styling consistent. Then it will look like this: 
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx (1)
264-269:⚠️ Potential issue | 🟠 MajorThe OK path still dismisses the dialog before validation finishes.
Resolving
promptPromiseRefand hiding the modal in the OK handler reintroduces the earlier retry problem: ifmasterPasswordIsValid(...)fails, ortoggleAndSetupEncryption(...)throws, the user has to reopen the dialog and retype the password. With the current one-shot promise, the validation/toggle needs to happen before resolving and closing, or the dialog needs to re-arm itself after a failed attempt.Also applies to: 275-282, 303-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx` around lines 264 - 269, The OK handler currently calls promptPromiseRef.current(resolve) and hides the modal (setEnableEncryptionPromptVisible(false)) before running masterPasswordIsValid(...) and toggleAndSetupEncryption(...), which forces the user to re-open and retype on failure; change the flow so the handler runs validation and toggleAndSetupEncryption first and only on success call the stored resolve and then hide the modal (i.e., keep promptPromiseRef.current set and keep setEnableEncryptionPromptVisible(true) while validation/setup is in progress and on error show the validation error and do not resolve/close), and ensure cancel still resolves/nulls and clears promptPromiseRef appropriately; update the same pattern for the other instances referenced around lines 275-282 and 303-310.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx`:
- Around line 314-316: Update PasswordInput's exported types to reflect the
actual native input event and then remove any usage of `any` in the handler:
change the types in PasswordInput/types.ts to export ChangeEvent as
React.ChangeEvent<HTMLInputElement> and ChangeEventHandler as (event:
ChangeEvent) => void so the component's declared contract matches its runtime
behavior, then modify the handler in EncryptionConfigScreen.tsx
(onPasswordInputChange and the other similar handler near lines ~329) to accept
the typed ChangeEvent and call setEnableEncryptionPassword(event.target.value)
without using `any`.
---
Duplicate comments:
In `@packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx`:
- Around line 264-269: The OK handler currently calls
promptPromiseRef.current(resolve) and hides the modal
(setEnableEncryptionPromptVisible(false)) before running
masterPasswordIsValid(...) and toggleAndSetupEncryption(...), which forces the
user to re-open and retype on failure; change the flow so the handler runs
validation and toggleAndSetupEncryption first and only on success call the
stored resolve and then hide the modal (i.e., keep promptPromiseRef.current set
and keep setEnableEncryptionPromptVisible(true) while validation/setup is in
progress and on error show the validation error and do not resolve/close), and
ensure cancel still resolves/nulls and clears promptPromiseRef appropriately;
update the same pattern for the other instances referenced around lines 275-282
and 303-310.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6220323f-908b-44ee-adc5-b58bb75816f2
📒 Files selected for processing (2)
packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsxpackages/app-desktop/main.scss
Thank you for the guidance! I've pushed the fix, and the dialog now perfectly matches the width and background styling of the 'Manage master password' UI. Please let me know if anything else needs adjusting. I'll be happy to work on it. |
|
Also, thinking about consistency, right now, if a user clicks 'Disable encryption' or enters an incorrect password, they get the standard, system-style alert/confirm dialog boxes. Since we are updating this UI, would you like me to replace those alerts with this same custom React |
I noticed many inconsistencies with dialogs on this screen to be honest. Lets not go down that rabbit hole and stick to changing this one dialog |
|
Please address the rabbit's comment |
All fixed! Just pushed the changes requested by the rabbit. Please let me know if there is anything else required from my side! |
Hi! I pushed the fixes requested by the bot (updated both the PasswordInput and the EncryptionConfigScreen.tsx). I noticed the CI checks failed twice: once with an Android NDK download timeout, and just now on the macOS arm64 runner during the Yarn link phase (YN0009: root@workspace:. couldn't be built successfully). Since my changes were strictly related to React component typings, I believe these might be environment/cache flakes on the runners, so I didn't want to keep spamming empty commits. Let me know if you are able to review the code changes in the meantime! |
|
All tests have officially passed! Let me know if you need anything else from my end. |
|
Still has lint errors unfortunately: /home/runner/work/joplin/joplin/packages/app-desktop/gui/EncryptionConfigScreen/EncryptionConfigScreen.tsx /home/runner/work/joplin/joplin/packages/app-desktop/gui/PasswordInput/types.ts ✖ 3 problems (3 errors, 0 warnings) Error: Process completed with exit code 1. |
Thanks for pointing that out, I've fixed the lint issues and pushed the changes. Please let me know if anything else needs adjustment. |
|
CI still failed. Please come back 30-60 mins after you push a change to check whether it passed. In this case it looks like an intermittent CI failure, so either update the branch (if it is behind) or push an empty commit to rerun the CI |
Got it, thanks for the clarification! |
|
All review comments have been addressed and CI is passing. Let me know if anything else is needed! |
|
Hello, the top post needs to be updated with the latest screenshot, ideally in both dark and light theme |
I have updated the first post, let me know if anything else is required. |
|
Still some CodeRabbit's comments. Please confirm you'd like to complete this PR |
Yes, I’ve reviewed the remaining CodeRabbit comments and confirmed that all relevant points have been addressed in the latest changes. The current implementation is complete from my side and ready to be merged. Please let me know if anything else is required. |
| }; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Required because PasswordInput's ChangeEventHandler type is incorrect | ||
| const onPasswordInputChange = (event: any) => { |
There was a problem hiding this comment.
But that looks like a DOM event. Surely there's a type for this?
There was a problem hiding this comment.
You're right that this should ideally use React.ChangeEvent<HTMLInputElement>.
I initially updated the PasswordInput types (as suggested by CodeRabbit) so that this handler could be properly typed. However, that change was marked as out of scope for this PR, so I reverted it to keep the changes focused.
Since PasswordInput currently defines an incompatible onChange type, I've kept 'any' here as a temporary workaround to match the existing typing.
I can open a separate PR to fix the PasswordInput typing if needed or I can also update this handler here to use React.ChangeEvent<HTMLInputElement> rather than any if that’s preferred? please let me know which approach you’d like.
|
@mrjo118, does this change make sense to you? Was there an issue for it? Or was that related to other similar recent changes on the E2EE dialog boxes? |
Yes, the change does make sense, as it allows the password to be shown / hidden on the relevant dialog, which gives a better user experience even if there is no consequence of entering an incorrect password in this context |
…ty in encryption setup (laurent22#14739)
3 participants
Description
Currently, when a user clicks "Enable encryption", the app asks for their password but masks the input entirely. Since this password is going to be used to encrypt their entire database, making a typo here can cause a lot of stress.
This PR adds a "show password" (eye icon) toggle to that prompt so users can easily view and verify their password while typing.
Technical details & Implementation
Initially, I looked into just adding an HTML button to the existing prompt, but I noticed we were using the
dialogs.prompt()method there, which relies under the hood on the legacysmalltalklibrary. Becausesmalltalkgenerates its DOM outside of our React tree, we can't cleanly inject custom components or buttons into it.To solve this and give the user the right to view their password, I completely replaced the legacy
smalltalkprompt with our native, already-built React<Dialog>and<PasswordInput>components.Here is exactly how I refactored the flow:
enableEncryptionPromptVisibleandenableEncryptionPassword).await dialogs.prompt(...)call, clicking the "Enable encryption" button now just setsenableEncryptionPromptVisibletotrue. This opens our custom React Dialog.toggleAndSetupEncryption(...)function as before.Why is this better?
<PasswordInput>gives us the show/hide eye toggle for free, and using<Dialog>ensures this popup natively respects Joplin's existing CSS system (like thedialog-rootanddialog-contentwrappers for dark/light themes).Testing
Before:
After:
After all alterations(both light and dark mode):