Skip to content

Conversation

sotayamashita
Copy link
Owner

@sotayamashita sotayamashita commented Sep 2, 2025

Before merging

Wait two weeks for comments from the original PR creator, just in case

Summary

This PR adopts the upstream feature that migrates from custom menu implementation to Obsidian's native menu system.

What's Changed

✨ Improvements: Native Obsidian Menu Integration

Migrates the highlighter menu from a custom DIV-based implementation to Obsidian's native menu system. This provides better integration with Obsidian's UI and improved keyboard navigation support.

📝 Changes Included

  • Menu System: Replaced custom menu div with native Obsidian menu items
  • Icons: Added custom icons to the native menu implementation
  • Styles: Removed custom container styles to leverage Obsidian's built-in menu styles
  • Accessibility: Improved keyboard navigation for menu selection

🎯 Benefits

  • ✅ Better integration with Obsidian's native UI
  • ✅ Improved keyboard navigation support
  • ✅ Cleaner codebase with less custom CSS
  • ✅ More consistent user experience with other Obsidian menus

Related Issues

Testing

  • ✅ Built successfully with npm run build
  • ✅ Menu appears correctly with native Obsidian styling
  • ✅ Custom icons display properly in menu items
  • ✅ Keyboard navigation works as expected
  • ✅ All highlighter options function correctly

Credits

Full credit goes to @andorx for the original implementation. This fork preserves the original authorship and includes proper attribution via cherry-pick -x.

Preview

CleanShot 2025-09-02 at 23 40 42

Summary by CodeRabbit

  • Style

    • Removed custom styling for the highlighter menu, including container layout, item spacing, icon alignment, animations, shadows, borders, and sizing. The menu now falls back to default browser appearance and may exhibit overflow or positioning differences.
  • Refactor

    • Removed local initialization of the highlighter menu in the UI layer, delegating menu creation/management to external context. This may affect whether the menu appears if not provided by the host environment.

Copy link

coderabbitai bot commented Sep 2, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Removed the highlighter menu’s TypeScript initialization and deleted its CSS styling block. No new logic or styles were added. No exported/public APIs were changed.

Changes

Cohort / File(s) Summary
Highlighter menu logic
src/ui/highlighterMenu.ts
Removed creation of the Menu instance (and a related commented config call), leaving references to menu.dom without local initialization.
Highlighter menu styles
styles.css
Deleted the .menu.highlighterContainer styles and nested menu item rules, removing layout/visual styling and fade animation for the highlighter menu.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant HM as HighlighterMenu
  participant M as Menu
  participant DOM as Document

  rect rgb(235, 245, 255)
  note over HM: Previous flow
  U->>HM: Trigger show menu
  HM->>M: new Menu()
  HM->>M: (optional) setUseNativeMenu(false)
  M-->>HM: menu.dom
  HM->>DOM: attach menu.dom
  end

  rect rgb(255, 240, 240)
  note over HM: Current flow after change
  U->>HM: Trigger show menu
  HM--xM: (Menu not instantiated)
  HM->>DOM: use menu.dom (undefined)
  DOM--xHM: Error/No-op
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I nibble through code with whiskers bright,
A menu vanished into night—
Styles unspooled like threads of hay,
TS forgot what to create today.
I tap my paw, then hop to mend,
So dom and menu meet again.
(\_/)</> ʕ•ᴥ•ʔ ✨

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/obsidian-menu-upstream-pr43

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 13f7e6a and f885aef.

📒 Files selected for processing (2)
  • src/ui/highlighterMenu.ts (0 hunks)
  • styles.css (0 hunks)

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

@sotayamashita sotayamashita merged commit b5c278e into main Sep 14, 2025
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Highlightr No Longer Working w Obsidian v0.15.3

2 participants