Skip to content

Conversation

@sid597
Copy link
Collaborator

@sid597 sid597 commented Oct 31, 2025

Summary by CodeRabbit

  • New Features
    • Added a warning notification to detect and alert users of conflicting scripts that may affect functionality.

@linear
Copy link

linear bot commented Oct 31, 2025

@supabase
Copy link

supabase bot commented Oct 31, 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 ↗︎.

Copy link
Collaborator Author

sid597 commented Oct 31, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@sid597 sid597 changed the title Check if left sidebar script is loaded in graph ENG-862 Check if left sidebar script is loaded in graph Oct 31, 2025
@sid597 sid597 changed the title ENG-862 Check if left sidebar script is loaded in graph ENG-862: Check if user has two sidebar's installed Oct 31, 2025
@sid597 sid597 marked this pull request as ready for review October 31, 2025 17:31
@sid597
Copy link
Collaborator Author

sid597 commented Oct 31, 2025

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

📝 Walkthrough

Walkthrough

Adds runtime detection after Discourse floating menu installation to check for an existing Roam left-sidebar script. If found, displays a warning toast with a fixed message and 10-second timeout before returning extension UI elements.

Changes

Cohort / File(s) Change Summary
Runtime Script Detection
apps/roam/src/index.ts
Adds conditional runtime check querying for existing left-sidebar script; displays warning toast notification if detected. No signature changes to exported functions.

Sequence Diagram

sequenceDiagram
    participant User
    participant Extension
    participant DOM

    User->>Extension: Initialize extension
    Extension->>Extension: Install Discourse floating menu
    
    rect rgba(100, 150, 200, 0.2)
    Note over Extension,DOM: New: Runtime script detection
    Extension->>DOM: Query for left-sidebar script
    alt Script detected
        Extension->>User: Display warning toast (10s)
    else No script found
        Note over Extension: Continue normally
    end
    end
    
    Extension->>User: Return extension UI elements
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify the DOM query selector correctly identifies the target left-sidebar script
  • Confirm the warning message content and toast timeout duration are intentional
  • Check that the detection logic placement after menu installation is the correct execution order

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "ENG-862: Check if user has two sidebar's installed" is partially related to the actual changeset. The title correctly identifies that the change involves checking for multiple sidebars, which aligns with the pull request's objective to detect an existing Roam left-sidebar script and display a warning. However, the title lacks specificity about what is being checked (specifically the Roam left-sidebar), what action is taken (showing a warning toast), and contains a minor grammatical error ("sidebar's" should be "sidebars"). Despite these shortcomings, the title refers to a real aspect of the change and meets the criteria for partial relevance.

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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50ac115 and 2a8d328.

📒 Files selected for processing (1)
  • apps/roam/src/index.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/main.mdc)

**/*.{ts,tsx}: Prefer type over interface
Use explicit return types for functions
Avoid any types when possible

Files:

  • apps/roam/src/index.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/main.mdc)

**/*.{ts,tsx,js,jsx}: Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use Prettier with the project's configuration
Maintain consistent naming conventions: PascalCase for components and types
Maintain consistent naming conventions: camelCase for variables and functions
Maintain consistent naming conventions: UPPERCASE for constants

Files:

  • apps/roam/src/index.ts
apps/roam/**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

apps/roam/**/*.{js,jsx,ts,tsx}: Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API

Files:

  • apps/roam/src/index.ts
🧠 Learnings (5)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-08-25T15:53:21.799Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 372
File: apps/roam/src/components/DiscourseNodeMenu.tsx:116-116
Timestamp: 2025-08-25T15:53:21.799Z
Learning: In apps/roam/src/components/DiscourseNodeMenu.tsx, when handling tag insertion, multiple leading hashtags (like ##foo) should be preserved as they represent user intent, not normalized to a single hashtag. The current regex /^#/ is correct as it only removes one leading # before adding one back, maintaining any additional hashtags the user intended.

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Roam Research extension

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API

Applied to files:

  • apps/roam/src/index.ts
🔇 Additional comments (1)
apps/roam/src/index.ts (1)

149-151: LGTM! Detection logic is straightforward.

The script detection uses a specific selector that correctly identifies the conflicting left-sidebar script.

@sid597 sid597 force-pushed the eng-862-check-if-user-has-two-sidebars-installed branch from 6816506 to 92d1491 Compare November 2, 2025 04:15
@sid597 sid597 requested review from maparent and mdroidian November 2, 2025 04:38
@sid597 sid597 merged commit 1642ff7 into main Nov 4, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this to Done in General Nov 4, 2025
@sid597 sid597 deleted the eng-862-check-if-user-has-two-sidebars-installed branch November 4, 2025 15:54
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