Mobile: Fixes #15060: Fix shared note not persisted to active notebook#15064
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates mobile share handling and note reload/init flows: load missing folder asynchronously when needed, apply and persist shared title/body to the reloaded Note entity, use the active folder when handling shares, and add a Jest test that simulates a provisional shared note and calls unmount for cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant ShareExt as ShareExtension
participant Root as AppRoot
participant FolderSvc as Folder (getValidActiveFolder / load)
participant NoteSvc as Note (reloadNote / save)
participant NoteComp as NoteScreen
ShareExt->>Root: deliver sharedData (title, text)
Root->>FolderSvc: getValidActiveFolder()
alt active folder found
FolderSvc-->>Root: activeFolder.id
Root->>NoteComp: handleShared(sharedData, parent_id=activeFolder.id)
else no active folder
FolderSvc-->>Root: null
Root->>ShareExt: close() and show error
end
NoteComp->>NoteSvc: reloadNote(noteId)
NoteSvc->>FolderSvc: Folder.byId(note.parent_id)
alt folder not found
FolderSvc->>FolderSvc: load(note.parent_id)
FolderSvc-->>NoteSvc: folder
end
NoteSvc-->>NoteComp: NoteEntity
NoteComp->>NoteSvc: save(fieldsToSave) -- when shared title/body present
NoteSvc-->>NoteComp: updated NoteEntity
NoteComp-->>NoteComp: setState(note, lastSavedNote)
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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]](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-mobile/components/screens/Note/Note.test.tsx (1)
407-428: Please assert the resolved notebook as well.This catches the
parent_idoverwrite race, but it does not prove the newFolder.load()fallback is working: the provisional note already has the correctparent_idon Line 409, so this would still pass if the picker kept showing'...'. Adding one assertion against the rendered notebook label after mount would keep both root causes under test.As per coding guidelines, "Focus on testing essential behaviour and edge cases — avoid adding tests for every minor detail".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-mobile/components/screens/Note/Note.test.tsx` around lines 407 - 428, The test "should set title, body, and parent_id correctly when a note is created via share" currently only checks note fields; add an assertion that the rendered notebook label (the visible notebook/folder name) resolves to the folder title after mounting to ensure the Folder.load() fallback ran; locate the test using the it(...) block, the rendered component WrappedNoteScreen, and helper waitForNoteToMatch/note id, then after render (or after waitForNoteToMatch) query the screen for the notebook label text and assert it equals 'Share target folder' so the UI shows the resolved notebook rather than the provisional '...'.
🤖 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/lib/components/shared/note-screen-shared.ts`:
- Around line 344-360: The code assumes the local variable note exists and uses
note.id (and spreads note into updatedNote) even if reloadNote() returned
undefined; guard this by checking the note value before using it — e.g., if note
is falsy, skip building fieldsToSave/updatedNote and avoid calling
Note.save/comp.setState so the component remains in its loading state; locate
the logic around reloadNote, the variables updatedNote, fieldsToSave, Note.save
and comp.setState to implement the early-return or conditional branch that
prevents dereferencing note.id when note is missing.
---
Nitpick comments:
In `@packages/app-mobile/components/screens/Note/Note.test.tsx`:
- Around line 407-428: The test "should set title, body, and parent_id correctly
when a note is created via share" currently only checks note fields; add an
assertion that the rendered notebook label (the visible notebook/folder name)
resolves to the folder title after mounting to ensure the Folder.load() fallback
ran; locate the test using the it(...) block, the rendered component
WrappedNoteScreen, and helper waitForNoteToMatch/note id, then after render (or
after waitForNoteToMatch) query the screen for the notebook label text and
assert it equals 'Share target folder' so the UI shows the resolved notebook
rather than the provisional '...'.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ecf60759-1f49-4bea-9d01-67cc784d3bee
📒 Files selected for processing (2)
packages/app-mobile/components/screens/Note/Note.test.tsxpackages/lib/components/shared/note-screen-shared.ts
coderabbitai
bot
added
Sharing
and removed
bug
|
I recently added a getValidActiveFolder function, which is used for validation when creating a new note manually, to ensure only a non trashed active folder can be used when creating a new note. I think it would make sense to utilise this function for determining the folder for notes created via the share to Joplin as well. If getValidActiveFolder returns null, it should ideally throw an error to the UI (i.e. when no untrashed notebooks exist) |
coderabbitai
bot
added
bug
|
Thank you for the suggestion that makes sense I have updated handleShareData in root.tsx to use getValidActiveFolder instead of the previous selectedFolderId ?? defaultFolder() approach |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-mobile/root.tsx`:
- Around line 589-595: When Folder.getValidActiveFolder() returns null the
shared data is silently dropped; instead, in the else branch (where
reg.logger().info is called) dispatch a user-visible error/notification and
explicitly terminate the share flow before returning. Use this.props.dispatch to
(1) show an error/toast or set an error state describing "No active folder to
accept share" and (2) dispatch or invoke the existing share-close/finish action
or callback used by the share flow (the same mechanism used after handleShared
succeeds) so the UI exits the share flow cleanly; keep the reg.logger() call but
ensure the UI error + share termination are performed and then return.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35e8e6da-3586-4e01-91be-0fb9c6e98a3c
📒 Files selected for processing (1)
packages/app-mobile/root.tsx
|
Tested the latest changes and it looks good to me! I also see that the title field is populated with metadata from the page which is even better. See video: studio64_VilXu84bKH.mp4EDIT: I've also created a separate issue which I noticed during testing #15071 |
|
Thank you for testing glad it's working! I will take a look at #15071 as well |
3 participants
Fixes #15060
Problem
When sharing a link or text to Joplin from another app (e.g. Chrome), the new note was not being saved to the active notebook. The notebook picker showed "..." and the note only appeared under All Notes
Two root causes:
Folder.byIdonly searches folders already in Redux state : if the target folder wasn't loaded yet, the picker showed nothing.initState, after reloadNote sets the note viacomp.setState, React doesn't flush state synchronously. The shared data handler immediately readcomp.state.note, which was still the initial empty note (parent_id = ''), and scheduled a save that overwrote the correctparent_idin the databaseFix
Added a fallback to
Folder.loadin reloadNote so the folder is fetched from the database if it's not in Redux stateIn
initState, instead of reading fromcomp.state.note(which may be stale), the note returned directly fromreloadNoteis used. Title and body from shared data are saved as a partial update (Note.save({ id, title, body })), which leavesparent_iduntouched in the databaseTook existing behaviour in version 3.5 as reference
Test Plan
A test was added in
Note.test.tsxthat creates a provisional note inside a specific folder and dispatches aNAV_GOaction withsharedData(title + URL). It then waits for the note in the database to have the correct title, body, and parent_id directly verifying that the race condition is resolved and the note lands in the right notebookTested Manually:
fix.for.joplin.mp4
AI Assistance Disclosure:
Tool AI help in improving wording of PR description