"Open note on server" menu item #7477
Conversation
dosubot
bot
added
the
size:M
Summary of ChangesHello @contributor, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a valuable new feature to the desktop application, providing users with the ability to open any note directly in a web browser via their configured sync server. This enhancement aims to improve workflow efficiency by allowing users to maintain a reference note in an external browser without disrupting their active work within the Electron application, thereby offering greater flexibility and reducing the likelihood of unintended edits or navigation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Open note on server' menu item, which is a useful feature for users who want to reference a note in their browser while working in the desktop application. The implementation is well-executed across the frontend and services. The new command, its executor, and the UI changes are all clear and follow existing patterns. The feature is also correctly made available only when a sync server is configured and the app is running in Electron.
I have one minor suggestion to improve code clarity by removing a redundant property in the React component. Overall, this is a solid contribution.
| <CommandItem command="openNoteCustom" icon="bx bx-customize" disabled={isSearchOrBook || isMac || !isElectron} text={t("note_actions.open_note_custom")} /> | ||
| <CommandItem command="showNoteSource" icon="bx bx-code" disabled={!hasSource} text={t("note_actions.note_source")} /> | ||
| {(syncServerHost && isElectron) && | ||
| <CommandItem command="openNoteOnServer" icon="bx bx-world" disabled={!syncServerHost} text={t("note_actions.open_note_on_server")} /> |
There was a problem hiding this comment.
The disabled={!syncServerHost} prop is redundant here. The CommandItem is only rendered if syncServerHost is truthy due to the conditional rendering on line 87. In that case, !syncServerHost will always evaluate to false. You can remove this prop for cleaner and more maintainable code.
| <CommandItem command="openNoteOnServer" icon="bx bx-world" disabled={!syncServerHost} text={t("note_actions.open_note_on_server")} /> | |
| <CommandItem command="openNoteOnServer" icon="bx bx-world" text={t("note_actions.open_note_on_server")} /> |
There was a problem hiding this comment.
@gemini-code-assist it is redundant, but I would still keep disabled attribute to be replaced with potential another check - for notes that cannot be opened on server. I haven't identified if there are such notes - notes like Options and User Guide are still openable.
There was a problem hiding this comment.
I understand your intention to keep the disabled attribute for potential future checks. However, the current disabled={!syncServerHost} is still redundant.
Here's why:
- The
CommandItemon line 88 is only rendered ifsyncServerHostis truthy (due to(syncServerHost && isElectron)on line 87). - If
syncServerHostis truthy, then!syncServerHostwill always evaluate tofalse. - Therefore, the
disabledprop will effectively always bedisabled={false}when thisCommandItemis visible.
If you plan to add more complex disabling logic in the future (e.g., based on the note type), you would modify this disabled prop anyway. Removing the currently redundant disabled={!syncServerHost} now would simplify the code without hindering future enhancements, as you'd be adding the new logic to a clean slate. The current disabled={!syncServerHost} doesn't serve as a useful placeholder for future, more complex disabling conditions.
| <CommandItem command="openNoteOnServer" icon="bx bx-world" disabled={!syncServerHost} text={t("note_actions.open_note_on_server")} /> | |
| <CommandItem command="openNoteOnServer" icon="bx bx-world" text={t("note_actions.open_note_on_server")} /> |
There was a problem hiding this comment.
@contributor I'd go here with gemini, had the same argument in my head. I can't imagine why the server shouldn't support a note type that is supported on desktop that would be instantly a Bug.
There was a problem hiding this comment.
For example, folder view - a note that shows content of a local folder. You are likely won't have such note on server due to security reason and file system path differences.
There was a problem hiding this comment.
@contributor I think even syncing up a potential folder view would be beneficial and it then would just open with error, it is only configured for X, Y, Z. Because the structure reminds the user oh wait I might have to open it up on my Computer.
There was a problem hiding this comment.
@contributor however I might see an edge case where you'd be right. Assume you upload images that you dont want to be shared on your trilium server instance. Then not having them synced up, maybe if there would be an option to hide this from the server, that would be beneficial, making the approach ofc worth keep having.
I would think this would resolve the comments then.
There was a problem hiding this comment.
Yes, local only (not synced) notes too. Evernote had it 15+ years ago. And if you don't trust the trilium sync server - that could be useful feature (even more true if they turn trilium to multi-user server)
|
@contributor: That's completely blowing-up the scope of this PR, but I would love to hear your comments on a more generic concept of "Sessions".
This solves several practical aspects
|
|
Sharing sessions between devices is quite advanced use case. I rarely need my desktop full state (as opposed to one or two pages) on mobile or web. Though opening session on another desktop may be useful. In fact, I use desktop app 99.99% of the time. No one can be productive on phone version. And it is hard to be productive in web version either (conflicting keyboard shortcuts, conflicting navigation between browser and trilium tabs, slower search performance etc). Though desktop app has its own nuisances. One of them - replacing important tab after clicking in the tree. This PR is work-arround for that. Alternative option is to open new tab on click in the tree (instead of ctrl+click). But that leads straight to another problem - tab management. Currently, you can't see the note titles in tiny tabs if you have more than 5-7 open. Vertical tabs would help a bit with that. Vertical tabs like Firefox's Tree Style Tree would definitely fix that. Using multiple windows in desktop desktop (with different workspaces) also don't go smoothly - keyboard shortcut opens search in first window, not in current window. No sticky differentiating titles on each window make it harder to switch between them. And others. So I hope that partial offload to browser via "Open note on server" could help with tab management too. |
|
@rom1dep your request reminds me about tmux 1. In general I agree about such a feature, however, I do not agree this PR should advance its scope this much, because talking about sessions is a whole new Box, that needs very careful thinking to get it right. So you should make it a new issue instead Footnotes |
|
I think the order should be switched Open note on Server should be just below Open note custom. To be more precise I actually i don't care about their exact order, but Open note externally, Open note custom and Open note on server should be grouped. 
Tips for reviewers faster testing on LinuxTesting more quickly can be done like this on linux. Since the menu cannot been without a sync server this might be the most efficient solution: |
Why? Because of menu item title? If we name it “View note on server” will there be less temptation for grouping? Regardless of the name we will use Here is how menu looks on server: Here is how it looks on desktop without sync server: Here is how it looks on desktop with sync server: This UI is consistent. Menu item "View source" is always third item in the group. The position of menu item in the PR prevents swapping of “Open note on server” and "View Source" for users that use both web and electron |
|
@contributor that view also makes sense. I think both ways are okay. So you can do that. |
|
@contributor if you think its ready, because I dont know if you mean its truely ready, a short notice on matrix towards elian might be useful, to clarify readiness for merge. Because this never went through a draft phase it makes unclear what state it is in right now. The code works as explained. Else, if not ready, pls convert to draft, to clarify communication. |
|
@eliandoran it is ready |
3 participants
This adds menu item "Open note on server" to note's three dot menu for desktop app
The main use case for this new item is to open a note for reference in your browser (you can also optionally turn zen mode on), while keep working in electron app with other notes and prevent accidental replacing the reference note with another note (which is very likely to happen unless you only use
Ctrl+Clickin the tree) ,The menu item “Open note on server” is only visible in electron app and if you have sync server configured. It goes as last item in its group on purpose - to keep existing items in original order and keep order in the group consistent between web and electron versions.