[ENG-1237] Create feature flag infra for Obsdian #672
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughIntroduces a feature flag management system for Obsidian with a modal UI accessible via keyboard shortcut (Mod+Shift+A). Adds a new FeatureFlagModal component that renders a React-based FeatureFlagSettings UI, with constants defining available feature flags (currently DATABASE_SYNC) and integration into the command registration system. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @apps/obsidian/src/components/FeatureFlagSettings.tsx:
- Around line 10-15: The handleSave function currently calls
plugin.saveSettings() but never writes the feature flag values into
plugin.settings; update handleSave so it assigns the component's feature-flag
state into plugin.settings.featureFlags (e.g., plugin.settings.featureFlags = {
databaseSync: databaseSync, ... } or by spreading the local flags state) before
awaiting plugin.saveSettings(), then keep the Notice and
setHasUnsavedChanges(false) as-is.
- Around line 5-8: The component FeatureFlagSettings currently initializes
hasUnsavedChanges via useState but never updates it, leaving the Save button
disabled; add a UI control (e.g., checkbox or toggle) bound to the feature flag
state inside FeatureFlagSettings and in its onChange handler call
setHasUnsavedChanges(true) (and update the flag value via usePlugin or your
existing state) so any user change enables saving; ensure the control references
the same feature flag used elsewhere in FeatureFlagSettings and update the Save
button state accordingly.
In @apps/obsidian/src/constants.ts:
- Around line 75-80: Feature flag DATABASE_SYNC is declared in FEATURE_FLAGS but
not wired into the FeatureFlagSettings UI: update the FeatureFlagSettings
component to import FEATURE_FLAGS and add a toggle/checkbox labeled "Enable
Database Sync" that reads its initial state from
plugin.settings[FEATURE_FLAGS.DATABASE_SYNC] (defaulting to false if unset) and
writes updates back to plugin.settings then calls plugin.save() (or the existing
settings persistence helper) on change; ensure the toggle's checked value is
bound to the component state so UI reflects saved value and use
FEATURE_FLAGS.DATABASE_SYNC as the unique key for all reads/writes.
In @apps/obsidian/src/utils/registerCommands.ts:
- Around line 135-144: Replace the global keyboard listener registered via
plugin.registerDomEvent with an Obsidian command registered via this.addCommand
/ plugin.addCommand so the shortcut is managed by Obsidian (appears in the
command palette and is configurable). Specifically, remove the
plugin.registerDomEvent block and instead register a command with a unique id
and name that opens FeatureFlagModal in its callback (use
FeatureFlagModal(plugin.app, plugin).open()), and supply a hotkey entry such as
{ modifiers: ["Mod", "Shift"], key: "f" } (Mod+Shift+F) to avoid Mod+Shift+A
conflicts so users can rebind it in settings.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/obsidian/src/components/FeatureFlagModal.tsxapps/obsidian/src/components/FeatureFlagSettings.tsxapps/obsidian/src/constants.tsapps/obsidian/src/utils/registerCommands.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/obsidian/src/utils/registerCommands.tsapps/obsidian/src/components/FeatureFlagSettings.tsxapps/obsidian/src/components/FeatureFlagModal.tsxapps/obsidian/src/constants.ts
apps/obsidian/**
📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)
apps/obsidian/**: Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Follow the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for UI and code styling
Use Lucide and custom Obsidian icons alongside detailed elements to provide visual representation of features in platform-native UI
Files:
apps/obsidian/src/utils/registerCommands.tsapps/obsidian/src/components/FeatureFlagSettings.tsxapps/obsidian/src/components/FeatureFlagModal.tsxapps/obsidian/src/constants.ts
🧬 Code graph analysis (2)
apps/obsidian/src/utils/registerCommands.ts (1)
apps/obsidian/src/components/FeatureFlagModal.tsx (1)
FeatureFlagModal(9-44)
apps/obsidian/src/components/FeatureFlagSettings.tsx (1)
apps/obsidian/src/components/PluginContext.tsx (1)
usePlugin(8-14)
🔇 Additional comments (1)
apps/obsidian/src/components/FeatureFlagModal.tsx (1)
9-43: LGTM! Clean modal implementation with proper React lifecycle management.The modal correctly:
- Manages React root creation and cleanup
- Wraps the settings component with necessary context providers
- Uses StrictMode for better development experience
- Prevents memory leaks by unmounting the root in
onClose
There was a problem hiding this comment.
Let’s mirror Roam more closely.
In Roam, users first open the Discourse Graph settings, then toggle visibility with Ctrl+Shift+A. In Roam this surface is called the Admin Panel. Renaming it to match would improve consistency and clarity, especially since it’s not limited to feature flags and can contain broader configuration.
Maintaining a 1:1 naming and interaction model with Roam should also make the mental model clearer and reduce friction.
Lastly, there are a few CodeRabbit suggestions that were brought up that seem valid. I see they have been resolved but with no code changes. Could you go back through them and add the reason as to why they were resolved with no action taken, please? Specifically why the feature flag isn't wired up. If it is planned to be wired up in another task/PR, let's link to it.
|
@mdroidian yes that makes sense. i changed the implementation to hide the panel in Settings and open it with Cmd+Shift+key now. the rest of the code was just placeholder so that's why I resolved them without changing the code |
trangdoan982
deleted the
eng-1237-create-feature-flag-infra-for-obsdian
branch
https://www.loom.com/share/e6108dd285b74ae5a1343441ab749269
This PR should be reviewed and merged before #656
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.