Auto execute saved search note #7841
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 significant enhancement to how search notes function, enabling them to automatically execute their queries under specific conditions. The feature is activated by a dedicated label and includes intelligent handling to prevent repetitive executions within a session. Additionally, it integrates with the UI to automatically switch to a relevant tab, aiming to improve user efficiency and interaction with search results. 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 feature to automatically execute a search note if it has the #autoExecuteSearch label. The implementation uses sessionStorage to prevent re-executing the search for the same note within a session. The changes are mostly in SearchDefinitionTab.tsx. My review includes feedback on potential bugs related to a toggle command and missing dependencies in a useEffect hook, as well as a suggestion to improve code structure and performance.
| await refreshResults(); | ||
|
|
||
| if (noteContext?.viewScope?.viewMode === "default") { | ||
| parentComponent?.triggerCommand("toggleRibbonTabBookProperties", {}); |
There was a problem hiding this comment.
Using toggleRibbonTabBookProperties might lead to unexpected behavior. If the 'Collection Properties' tab is already active, this command will likely hide it, which seems contrary to the goal of ensuring it is visible. It would be safer to use a command that explicitly shows the tab, for example one with a name like showRibbonTabBookProperties, if one exists. If not, you might need to check if the tab is already active before triggering the toggle command to avoid accidentally hiding it.
| } | ||
| } | ||
| autoExecute(); | ||
| }, [note]); |
There was a problem hiding this comment.
This useEffect has missing dependencies. According to the rules of hooks, all values from the component scope that are used inside an effect should be listed in its dependency array. In this case, noteContext and refreshResults are used inside the effect but are missing from the dependency array. This can lead to stale closures and bugs. Please add them to the array. Note that refreshResults is redefined on every render, so adding it will cause this effect to run more often. The logic inside seems to handle this, but for better performance, refreshResults should be wrapped in useCallback in a separate change.
| }, [note]); | |
| }, [note, noteContext, refreshResults]); |
There was a problem hiding this comment.
@gemini-code-assist only note being changed should be a trigger to execute search. No need to trigger it more often than that.
There was a problem hiding this comment.
I understand you want the useEffect to trigger only when note changes. However, useEffect captures the values of variables from the render it was created in. If noteContext or refreshResults change between renders where note does not change, the autoExecute function inside your useEffect will still use the old (stale) versions of noteContext and refreshResults from the last time note changed. This can lead to subtle and hard-to-debug bugs.
React's "rules of hooks" require all values used inside an effect (that come from the component scope) to be listed in the dependency array to ensure the effect always operates with the latest values.
If you truly want the effect to only re-run when note changes, you need to ensure that noteContext and refreshResults themselves have stable references across renders:
- For
refreshResults, you should wrap it inuseCallbackto memoize the function. This will prevent it from being recreated on every render, thus keeping its reference stable. - For
noteContext, if it's a prop, its stability depends on the parent component. If the parent is recreating it on every render, you might need to memoize it in the parent usinguseMemoor ensure its structure doesn't change unnecessarily.
Until refreshResults and noteContext are guaranteed to be stable, the safest and most correct approach is to include them in the dependency array. Then, if you observe excessive re-renders, optimize refreshResults with useCallback and investigate noteContext's stability.
| const AUTO_EXEC_KEY = "lastAutoExecutedSearchNote"; | ||
| return { | ||
| load: () => sessionStorage.getItem(AUTO_EXEC_KEY), | ||
| save: (noteId: string) => sessionStorage.setItem(AUTO_EXEC_KEY, noteId), | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
Trilium doesn't use session storage and I'm not sure why it would need to be used here.
You have useNoteLabel which reads the label value for you automatically.
You can use useState or useRef to store the last executed note, not session storage.
There was a problem hiding this comment.
@eliandoran this was my path: useRef -> useState -> sessionStorage. The first two didn't work, there are commits for that. The react component is recreated every time and useRef.current is reset.
In order to keep react component I would have to use stayInDom in ribbon definition. This approach is worse than sessionStorage.
There was a problem hiding this comment.
It would fire useEffect when you navigate away to other notes (not of search type). There is an earlier return for that in useEffect, but still this is not good approach.
There was a problem hiding this comment.
I replaced sessionStorage with module level var, this approach better then both.
contributor
force-pushed
the
auto-execute-search
branch
from
97adfcf to
c15bb11
Compare
There was a problem hiding this comment.
It's fine to have an attribute to execute the auto-search but I think doing it from the SearchDefinitionTab is not the best place to do so since it's a ribbon tab (e.g. user configuration).
Have you noticed that if you press the Search button in the launch bar, the search is immediately executed?
I tracked down this behavior, you can find it in root_command_executor.ts and looking for searchNotesCommand. Notice how it forces the search via loadSearchNote.
We can do the same, but you'd have to find a good place to do so. Perhaps in search_result.tsx where the logic of search not executed yet is also determine.
P.S. Don't forget to update the documentation; new features cannot be undocumented.
It does this by using call to existing I could extract |
|
By the way, there is a “double list” bug in display search results. I first attributed this to the auto-search. But no, this is unrelated, and exists in main.
Search results are duplicated. When there are many results, you have double scrollbars too (moving in opposite directions) Reproducable both in current main HEAD 71b86b3, and in main's commit 435b856 this PR starts from Screenshots
|
|
@contributor , can you merge with the latest Edit: I see you mentioned it occurs in the latest main as well, weird. |
Set these attributes on root note: |
|
@contributor , apparently Should be fixed in d7ae2e4. |
Still doubled result. Both in search tab and collection tab |
|
Additionally, there is another, more severe bug (including current main 3afe6df). Collection and text notes stops to show their children notes (root has
Details
Collection with children: Text note with children: |
Can't reproduce this issue anymore, are you sure it's not a problem with the cache on your side?
Nice catch. I was aware of the bug but did not have specific steps to reproduce it. |
I'm sure. Double collection bug is still there. Empty collection bug is now fixed. |
|
@contributor , what commit are you on? |
|
|
@contributor , can't reproduce it... Recording.2025-11-25.202224.mp4 |
|
I retested again. Up until commit 976b1e1 double list is present. The commit fixes empty list. And apparently, this also fixes double list. |
what does that mean, practically? |
If you have a saved search note, I had an old abandoned "Saved searches" branch with various search notes. I added |
contributor
force-pushed
the
auto-execute-search
branch
from
6bb2e14 to
625c0ed
Compare
|
@eliandoran added docs. ready for review. |
Specifically, which tree node relative to the search note needs to be interacted with for the search to be triggered? As far as I can tell, "click to search" is a capability that already exists (via the refresh button, on the search note itself): 
It'd be good that:
Now, if the intent is to have "smart" search-note hierarchies that are kept up to date and rendered at all time (without needing explicit refresh, which is what |
Search note itself
Yes, this is what it does. Browsing saved searches as collections with one click on a tree node.
|
If the search note has to be clicked on, are you sure that we need this change? By your account, this is effectively the same as clicking on the refresh button which we already have (see screenshot above), on that same note. If the need is satisfied by a larger refresh icon, let's explore a UI solution that won't pull new attributes, configuration and complexity.
Then this contradicts what you just wrote. Which one is it?
Then let's make sure that, when the collection is pegged to a search note:
|
|
The attribute makes new functionality opt-in. With
Without it:
|
3 participants
If
#autoExecuteSearchlabel is present, it auto executes a search note.If ribbon's tab Collection Properties is present (and search results are not empty), it will auto switch to the tab too.
Context: #7704 (comment)