Skip to content

Mobile: Resolves #16002: Add note lock UI - #16069

Open
keshav0479 wants to merge 8 commits into
laurent22:devfrom
keshav0479:gsoc-local-note-encryption-mobile-lock-ui
Open

Mobile: Resolves #16002: Add note lock UI#16069
keshav0479 wants to merge 8 commits into
laurent22:devfrom
keshav0479:gsoc-local-note-encryption-mobile-lock-ui

Conversation

@keshav0479

Copy link
Copy Markdown
Contributor

Summary

Adds the mobile note lock UI behind the existing feature flag. Resolves #16002.

This was reviewed as keshav0479#4 by mrjo. The first commit is that reviewed work on current dev, and the later changes are separate commits.

Enable/Disable encryption and Lock encrypted notes live in the note screen menu and encrypted notes show a padlock in the note list. A locked note opens read-only with the locked panel instead of the body, and unlocking decrypts it in place. Enabling with no password asks to set one up and goes to the note lock config section, and the panel has a matching set up button.

Saves write the lock state, encrypted body and extracted resource ids together, and pending saves re-encrypt with the key captured at load. The last commit fixes two bugs found in testing: the to-do checkbox on a locked note now shows an error and reverts the tick (same treatment as desktop), and an internal marker no longer counts as a note modification, which could hide the locked panel and expose the encrypted body.

With the flag off, the note screen behaves exactly as before.

Testing

  • yarn tsc
  • yarn workspace @joplin/lib test (note-screen-shared, noteLock)
  • yarn workspace @joplin/app-mobile test Note.test
  • manually tested on the web app: enable/disable, wrong and correct password, manual and auto lock, the setup prompt and set up button, the locked to-do checkbox on the list and the note screen, flag off behaving as before

Video

Full run-through:

video_20260728_222015.mp4

AI Assistance Disclosure

I used AI tools while working on this PR for code suggestions and review, checking scope and tests, and drafting parts of this description, including the disclosure. I reviewed the final changes and reran the tests listed above myself.

Comment thread packages/app-mobile/components/screens/Note/Note.tsx Outdated
@coderabbitai coderabbitai Bot added enhancement Feature requests and code enhancements mobile All mobile platforms security labels Jul 28, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Comment thread packages/app-mobile/components/screens/Note/Note.tsx
Comment thread packages/lib/components/shared/note-screen-shared.ts
@joplin-coderabbit-cleanup joplin-coderabbit-cleanup Bot deleted a comment from coderabbitai Bot Jul 28, 2026
@mrjo118

mrjo118 commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Possibly a flaky test introduced by the desktop PR, as it failed here (I triggered a rerun):

[@joplin/app-desktop]: Summary of all failing tests
[@joplin/app-desktop]: FAIL gui/NoteEditor/utils/useFormNote.test.ts (78.388 s)
[@joplin/app-desktop]: ● useFormNote › should report a decryption failure instead of throwing, without producing a form note
[@joplin/app-desktop]:
[@joplin/app-desktop]: expect(jest.fn()).toHaveBeenLastCalledWith(...expected)
[@joplin/app-desktop]:
[@joplin/app-desktop]: Expected: true
[@joplin/app-desktop]: Received
[@joplin/app-desktop]: 2: true
[@joplin/app-desktop]: -> 3: false
[@joplin/app-desktop]:
[@joplin/app-desktop]: Number of calls: 3
[@joplin/app-desktop]:
[@joplin/app-desktop]: 112 | });
[@joplin/app-desktop]: 113 | expect(render.result.current.loadBlocked).toBe(false);
[@joplin/app-desktop]: > 114 | expect(onDecryptFailedChange).toHaveBeenLastCalledWith(true);
[@joplin/app-desktop]: | ^
[@joplin/app-desktop]: 115 | render.unmount();
[@joplin/app-desktop]: 116 | } finally {
[@joplin/app-desktop]: 117 | isUnlockedMock.mockRestore();
[@joplin/app-desktop]:
[@joplin/app-desktop]: at Object. (gui/NoteEditor/utils/useFormNote.test.ts:114:34)
[@joplin/app-desktop]:
[@joplin/app-desktop]:
[@joplin/app-desktop]: Test Suites: 1 failed, 37 passed, 38 total
[@joplin/app-desktop]: Tests: 1 failed, 216 passed, 217 total
[@joplin/app-desktop]: Snapshots: 0 total
[@joplin/app-desktop]: Time: 132.321 s
[@joplin/app-desktop]: Ran all test suites.

@keshav0479

Copy link
Copy Markdown
Contributor Author

Had a deeper look at this, it's not the test, it's a small bug from the desktop PR.

The session effect in useFormNote should skip the first render, but it checks against undefined while usePrevious starts at null, so it refreshes the note on mount. That second load fires another pair of decrypt callbacks and the test asserts on the last one. Happens locally too, the test just usually wins the race.

Fix is to seed usePrevious with the current value, plus an await on ItemChange.waitForAllSaved() in the test. Putting both on this PR as a separate commit.

Also prevBuiltInEditorVisible just above has the same problem, from #11296. Not touching it here since it was there before this, can raise it separately if you want.

@coderabbitai coderabbitai Bot added the desktop All desktop platforms label Jul 29, 2026
@mrjo118

mrjo118 commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Please put the test fix on a separate PR so it can be merged quicker to fix the flaky test. This PR's changes are not affected by the change anyway aside from intermittent ci failure.

Also prevBuiltInEditorVisible just above has the same problem, from #11296

If it does not cause any ui bug or a failing test, but is just inefficient, I'd say leave it as is. But if there is either a bug or a failing test, describe the reproduction steps and fix it on the same PR if it is a simple change.

@joplin-coderabbit-cleanup joplin-coderabbit-cleanup Bot deleted a comment from coderabbitai Bot Jul 29, 2026
@mrjo118

mrjo118 commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Given that the kebab menu is redesigned and now has icons for the built in options, can you add a lock / unlock icon to the enabled / disable decryption options?

Screenshot 2026-07-30-3

I think outline lock / unlock icons from either ion icons or material icons would look good, depending on which one has a line thickness more consistent with the icons when beneath them. Or use the same icon set as the others if they use something different and they have their own lock icons which look good next to the others:

Screenshot 2026-07-30-1 Screenshot 2026-07-30-2

Comment thread packages/lib/components/shared/note-screen-shared.ts Outdated
Comment thread packages/lib/components/shared/note-screen-shared.ts Outdated
Comment thread packages/app-mobile/components/screens/Note/Note.tsx Outdated
Comment thread packages/app-mobile/components/screens/Note/NoteLockPanel.tsx
Comment thread packages/app-mobile/components/screens/Note/Note.tsx
@coderabbitai coderabbitai Bot removed the desktop All desktop platforms label Aug 1, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

@joplin-coderabbit-cleanup joplin-coderabbit-cleanup Bot deleted a comment from coderabbitai Bot Aug 1, 2026
@mrjo118

mrjo118 commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

I see you went for material icons. I forgot that the lock all notes action needs an icon too, and you set it to the same as enable encryption.

I think it would be better to make lock all notes have the lock icon, and use these material icons for enable / disable encryption:

Screenshot_2026-08-01_164321 Screenshot_2026-08-01_164313

@keshav0479

Copy link
Copy Markdown
Contributor Author

Sorry for delay earlier, power went right after i pushed so couldn't get back on this.

Lock all keeps the plain lock now, enable gets its own. I did try the open lock for disable first, but at menu size it looked almost the same as the closed one.
image

On yours, the material icons package is already in the app, but the font only has the filled enhanced-encryption and no-encryption, there's no outlined version of either. Everything else in that menu is outline so they'd stand out a bit. Went with lock-plus-outline and lock-off-outline.

Looks like this, how is it:
image
image

@mrjo118

mrjo118 commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

On yours, the material icons package is already in the app, but the font only has the filled enhanced-encryption and no-encryption, there's no outlined version of either. Everything else in that menu is outline so they'd stand out a bit. Went with lock-plus-outline and lock-off-outline.

Yes, lock-plus-outline and lock-off-outline look good to me

@joplin-coderabbit-cleanup joplin-coderabbit-cleanup Bot deleted a comment from coderabbitai Bot Aug 2, 2026
@laurent22

Copy link
Copy Markdown
Owner

Please address these comments from Claude:

1. Event listener leak — buildStartupTasks.ts:351 eventManager.on(EventName.NoteLockSessionChange, …) is registered with no matching off. Confirm startup runs exactly once per process; if not (profile switch / re-init), this double-dispatches. Either guarantee single registration or make it idempotent.

2. setState-after-unmount — NoteLockPanel.tsx:68-79 unlock() awaits WebCrypto then calls onUnlocked?.() / setErrorMessage / setUnlocking with no mounted check. Switching notes mid-unlock warns. Guard it.

3. isDecrypted casts everywhere — note-screen-shared.ts lines 181, 216, 267, 294 (and more) (note as Record<string, unknown>).isDecrypted is the any-adjacent pattern CLAUDE.md forbids, repeated ~8x in shared lib code that desktop also uses. Add an optional isDecrypted?: boolean to the note type (or a narrow interface) and access it as a real field.

4. Missing test for the component wiring — Note.tsx The lib helper is well tested, but the subtlest path isn't: enabling from the menu leaves a pending save that must re-encrypt with the key captured at load (noteLockNoteStateChange_scheduleSave). Add a Note.test.tsx case for enable-then-lock-before-save.

5. (Minor) Raw error.message in UI — NoteLockPanel.tsx:76, Note.tsx:909 Only OperationError is mapped to a friendly string; everything else dumps an internal message. Add a generic fallback.

@mrjo118

mrjo118 commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

I've given this a test on Android emulator. One thing I found is that when you setup the password, when the save button becomes enabled, the button state is not correct. It still looks disabled because the text colour changes, but the button colour doesn't:

studio64_KdZsEGk6iU.mp4

The same applies to the unlock button in the unlock note overlay:

Screenshot 2026-08-07-2

And when entering an incorrect password I get this error, instead of a user friendly one:

Screenshot 2026-08-07-3

Also when opening the unlock dialog when enabling encryption, the popup just looks like this (it's usable on emulator, but the typed value in the input cannot be seen and I have to press enter to submit the value):

Screenshot 2026-08-07-1

Additionally, if a todo is undecryptable, if you toggle the checkbox, it will bypass the undecryptable overlay and show the editor with the cipher text, as well as breaking the navigation history:

uyYa6CazsW.mp4

Finally, for undecryptable notes, when the session is unlocked the tags option is enabled in the menu, which allows you to change tags. It should be disabled:

Screenshot 2026-08-07-4

@keshav0479

Copy link
Copy Markdown
Contributor Author

I have my exams till Aug 20 so I'm mostly off until then. Will pick up everything here right after, Claude findings, emulator bugs and the #16160 wording.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Feature requests and code enhancements mobile All mobile platforms security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add mobile note lock UI

4 participants