Skip to content

Conversation

@mdroidian
Copy link
Contributor

@mdroidian mdroidian commented Sep 16, 2025

Set the node summoning menu default trigger to '@' to improve discoverability and align with existing default setting patterns.


Linear Issue: ENG-459

Open in Cursor Open in Web

Summary by CodeRabbit

  • Bug Fixes

    • Ensures the node search trigger reliably defaults to “@” when no user setting is defined, preventing missing-trigger issues.
  • Refactor

    • Centralized retrieval of the node search trigger setting to improve consistency and maintainability across the app.

Co-authored-by: mclicks <mclicks@gmail.com>
@cursor
Copy link

cursor bot commented Sep 16, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@linear
Copy link

linear bot commented Sep 16, 2025

@supabase
Copy link

supabase bot commented Sep 16, 2025

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@mdroidian mdroidian marked this pull request as ready for review September 18, 2025 17:41
…igger

- Updated the NodeSearchMenuTriggerSetting and initObservers functions to use the getSetting utility for retrieving the default node search trigger, improving consistency and maintainability.
@mdroidian
Copy link
Contributor Author

@CodeRabbit full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 18, 2025

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 18, 2025

📝 Walkthrough

Walkthrough

Centralizes retrieval of the "node-search-trigger" setting by importing and using getSetting with a default "@" in DiscourseNodeSearchMenu.tsx and initializeObserversAndListeners.ts. Replaces direct extensionAPI.settings.get calls. No exported signatures changed; existing onChange and listener control flow remain intact.

Changes

Cohort / File(s) Summary
Centralized node-search-trigger setting
apps/roam/src/components/DiscourseNodeSearchMenu.tsx, apps/roam/src/utils/initializeObserversAndListeners.ts
Replace direct extensionAPI.settings.get("node-search-trigger") with getSetting("node-search-trigger", "@"); add imports for the helper; preserve existing control flow and update logic; introduce default value "@" when unset.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant L as Key Listener
  participant S as getSetting
  participant A as extensionAPI.settings
  participant M as DiscourseNodeSearchMenu

  U->>L: Type character
  L->>S: getSetting("node-search-trigger", "@")
  S->>A: settings.get("node-search-trigger")
  A-->>S: value | undefined
  note right of S: Returns stored value or default "@"
  S-->>L: trigger
  alt trigger matches typed char
    L->>M: Open node search menu
  else
    L-->>U: Ignore
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Set node summoning menu default trigger to @" succinctly and accurately summarizes the central change in this PR—switching the node-search trigger default to "@" as implemented in DiscourseNodeSearchMenu.tsx and initializeObserversAndListeners.tsx—so it is specific, concise, and meaningful for a teammate scanning history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/roam/src/utils/initializeObserversAndListeners.ts (1)

234-234: Trigger value is snapshotted at init; won’t reflect runtime setting changes.

If users change the trigger in settings, discourseNodeSearchTriggerListener will still use the old value until re-init. Fetch per event or subscribe to setting changes.

Apply this minimal change to read the latest value on each key event:

-  const customTrigger = getSetting("node-search-trigger", "@");
+  // Read the latest trigger on each key event to honor runtime changes

And inside discourseNodeSearchTriggerListener:

-  const discourseNodeSearchTriggerListener = (e: Event) => {
+  const discourseNodeSearchTriggerListener = (e: Event) => {
+    const customTrigger = getSetting("node-search-trigger", "@");

Alternatively, cache and update via a settings onChange hook if available.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b715b4 and fae3bf0.

📒 Files selected for processing (2)
  • apps/roam/src/components/DiscourseNodeSearchMenu.tsx (2 hunks)
  • apps/roam/src/utils/initializeObserversAndListeners.ts (2 hunks)
🔇 Additional comments (3)
apps/roam/src/components/DiscourseNodeSearchMenu.tsx (2)

28-28: Centralized setting retrieval looks good.

Importing getSetting aligns this component with the new default behavior and reduces duplication.


540-541: Default trigger initialization is correct; confirm helper semantics.

Assuming getSetting is synchronous and returns "" when explicitly set empty (vs. undefined), this is fine. If it’s async or needs extensionAPI context, wire that in or memoize via onloadArgs.

apps/roam/src/utils/initializeObserversAndListeners.ts (1)

48-48: LGTM on consolidating settings access.

Using getSetting here keeps the default consistent across the app.

@mdroidian mdroidian merged commit 338e99d into main Sep 18, 2025
6 checks passed
@mdroidian mdroidian deleted the cursor/ENG-459-set-node-summoning-menu-default-trigger-to-1677 branch September 18, 2025 21:09
@github-project-automation github-project-automation bot moved this to Done in General Sep 18, 2025
trangdoan982 pushed a commit that referenced this pull request Oct 3, 2025
* Fix: Default node search trigger to '@'

Co-authored-by: mclicks <mclicks@gmail.com>

* Refactor: Replace direct API calls with getSetting for node search trigger

- Updated the NodeSearchMenuTriggerSetting and initObservers functions to use the getSetting utility for retrieving the default node search trigger, improving consistency and maintainability.

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants