Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/client/src/translations/en/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@
"workspace_template": "This note will appear in the selection of available template when creating new note, but only when hoisted into a workspace containing this template",
"search_home": "new search notes will be created as children of this note",
"workspace_search_home": "new search notes will be created as children of this note when hoisted to some ancestor of this workspace note",
"auto_execute_search": "Automatically executes the search defined in a saved search note and switches to the Collection Properties tab if any notes match the query",
"inbox": "default inbox location for new notes - when you create a note using \"new note\" button in the sidebar, notes will be created as child notes in the note marked as with <code>#inbox</code> label.",
"workspace_inbox": "default inbox location for new notes when hoisted to some ancestor of this workspace note",
"sql_console_home": "default location of SQL console notes",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ const ATTR_HELP: Record<string, Record<string, string>> = {
workspaceTemplate: t("attribute_detail.workspace_template"),
searchHome: t("attribute_detail.search_home"),
workspaceSearchHome: t("attribute_detail.workspace_search_home"),
autoExecuteSearch: t("attribute_detail.auto_execute_search"),
inbox: t("attribute_detail.inbox"),
workspaceInbox: t("attribute_detail.workspace_inbox"),
sqlConsoleHome: t("attribute_detail.sql_console_home"),
Expand Down
31 changes: 30 additions & 1 deletion apps/client/src/widgets/ribbon/SearchDefinitionTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import RenameNoteBulkAction from "../bulk_actions/note/rename_note";
import { getErrorMessage } from "../../services/utils";
import "./SearchDefinitionTab.css";

export default function SearchDefinitionTab({ note, ntxId, hidden }: TabContext) {
export default function SearchDefinitionTab({ note, ntxId, hidden, noteContext }: TabContext) {
const parentComponent = useContext(ParentComponent);
const [ searchOptions, setSearchOptions ] = useState<{ availableOptions: SearchOption[], activeOptions: SearchOption[] }>();
const [ error, setError ] = useState<{ message: string }>();
Expand Down Expand Up @@ -73,6 +73,27 @@ export default function SearchDefinitionTab({ note, ntxId, hidden }: TabContext)
}
});

useEffect(() => {
async function autoExecute() {
if (!note || note.type !== "search" || !note.hasLabel("autoExecuteSearch")) {
executionState.save("");
return;
}

const lastExecutedNoteId = executionState.load();
if (lastExecutedNoteId !== note.noteId) {
executionState.save(note.noteId);

await refreshResults();

if (noteContext?.viewScope?.viewMode === "default" && note.children.length > 0) {
parentComponent?.triggerCommand("toggleRibbonTabBookProperties", {});
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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]);
Copy link
Contributor

Choose a reason for hiding this comment

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

high

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.

Suggested change
}, [note]);
}, [note, noteContext, refreshResults]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gemini-code-assist only note being changed should be a trigger to execute search. No need to trigger it more often than that.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 in useCallback to 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 using useMemo or 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.


return (
<div className="search-definition-widget">
<div className="search-settings">
Expand Down Expand Up @@ -160,6 +181,14 @@ export default function SearchDefinitionTab({ note, ntxId, hidden }: TabContext)
)
}

const executionState = function() {
let lastAutoExecutedSearchNoteId = "";
return {
load: () => lastAutoExecutedSearchNoteId,
save: (noteId: string) => lastAutoExecutedSearchNoteId = noteId,
};
}();

function BulkActionsList({ note }: { note: FNote }) {
const [ bulkActions, setBulkActions ] = useState<RenameNoteBulkAction[]>();

Expand Down
Loading
Loading