ENG-1191: add new setting component for feature flag #633
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
sid597
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughIntroduces a feature flag management system consisting of a Zod schema for feature flags, utility functions to read and write flags to block-level settings storage, and a React UI component for toggling feature flags with a Blueprint Checkbox. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
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: 4
🧹 Nitpick comments (1)
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsx (1)
17-23: Consider error handling for feature flag operations.
getFeatureFlagmay throw if the Zod schema validation fails, andsetFeatureFlagperforms a void-ed async operation that could fail silently. Consider wrapping these in try-catch blocks or providing user feedback on errors.Note: This component is currently non-functional due to the empty
FeatureFlagsSchema(seezodSchemaForSettings.tsreview). Once feature flags are added to the schema, this logic will work as intended.Example: Basic error handling
const [value, setValue] = useState(() => { try { return getFeatureFlag(featureKey); } catch (error) { console.error(`Failed to load feature flag ${String(featureKey)}:`, error); return false; // safe default } }); const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { const { checked } = e.target; try { setFeatureFlag(featureKey, checked); setValue(checked); } catch (error) { console.error(`Failed to set feature flag ${String(featureKey)}:`, error); // Optionally revert UI state or show user notification } };
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsxapps/roam/src/utils/manageFeatureFlag.tsapps/roam/src/utils/zodSchemaForSettings.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/roam/src/components/settings/BlockPropFeatureFlagPanel.tsxapps/roam/src/utils/zodSchemaForSettings.tsapps/roam/src/utils/manageFeatureFlag.ts
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsxapps/roam/src/utils/zodSchemaForSettings.tsapps/roam/src/utils/manageFeatureFlag.ts
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsxapps/roam/src/utils/zodSchemaForSettings.tsapps/roam/src/utils/manageFeatureFlag.ts
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsxapps/roam/src/utils/zodSchemaForSettings.tsapps/roam/src/utils/manageFeatureFlag.ts
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsxapps/roam/src/utils/zodSchemaForSettings.tsapps/roam/src/utils/manageFeatureFlag.ts
🧠 Learnings (3)
📚 Learning: 2025-12-22T05:43:01.282Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 630
File: apps/roam/src/utils/settingsUsingBlockProps.ts:64-66
Timestamp: 2025-12-22T05:43:01.282Z
Learning: In `apps/roam/src/utils/settingsUsingBlockProps.ts`, the `setBlockPropBasedSettings` function expects callers to ensure that the `value` parameter matches the expected type for the given `keys` path. Type validation is handled at the caller side rather than within the utility function.
Applied to files:
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsxapps/roam/src/utils/manageFeatureFlag.ts
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,jsx,js,css,scss} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsx
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/utils/manageFeatureFlag.ts
🧬 Code graph analysis (2)
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsx (2)
apps/roam/src/utils/zodSchemaForSettings.ts (1)
FeatureFlags(8-8)apps/roam/src/utils/manageFeatureFlag.ts (2)
getFeatureFlag(8-18)setFeatureFlag(20-27)
apps/roam/src/utils/manageFeatureFlag.ts (3)
apps/roam/src/utils/zodSchemaForSettings.ts (2)
FeatureFlags(8-8)FeatureFlagsSchema(4-5)apps/roam/src/data/blockPropsSettingsConfig.ts (1)
TOP_LEVEL_BLOCK_PROP_KEYS(2-2)apps/roam/src/utils/settingsUsingBlockProps.ts (2)
getBlockPropBasedSettings(6-48)setBlockPropBasedSettings(50-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (2)
apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsx (1)
25-36: Well-structured component using Blueprint and roamjs-components.The component correctly uses BlueprintJS 3
Checkboxand roamjs-components utilities (Description,idToTitle) for consistent UI, aligning with the coding guidelines for the Roam extension.As per coding guidelines and learnings, "Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension."
apps/roam/src/utils/manageFeatureFlag.ts (1)
20-27: Proper use of void for async operation.The
voidkeyword correctly indicates that the promise returned bysetBlockPropBasedSettingsis intentionally not awaited, which is appropriate for a fire-and-forget write operation.Based on learnings,
setBlockPropBasedSettingsrelies on caller-side type validation. Sincevalueis typed asbooleanand feature flag keys will be defined in the schema, this usage is correct.Based on learnings, "In
apps/roam/src/utils/settingsUsingBlockProps.ts, thesetBlockPropBasedSettingsfunction expects callers to ensure that thevalueparameter matches the expected type for the givenkeyspath."
| export const BlockPropFeatureFlagPanel = ({ | ||
| title, | ||
| description, | ||
| featureKey, | ||
| }: { | ||
| title: string; | ||
| description: string; | ||
| featureKey: keyof FeatureFlags; | ||
| }) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
Per coding guidelines, functions should have explicit return types. Add : React.JSX.Element or : JSX.Element to the component signature.
🔎 Proposed fix
export const BlockPropFeatureFlagPanel = ({
title,
description,
featureKey,
}: {
title: string;
description: string;
featureKey: keyof FeatureFlags;
-}) => {
+}): React.JSX.Element => {As per coding guidelines, "Use explicit return types for functions."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const BlockPropFeatureFlagPanel = ({ | |
| title, | |
| description, | |
| featureKey, | |
| }: { | |
| title: string; | |
| description: string; | |
| featureKey: keyof FeatureFlags; | |
| }) => { | |
| export const BlockPropFeatureFlagPanel = ({ | |
| title, | |
| description, | |
| featureKey, | |
| }: { | |
| title: string; | |
| description: string; | |
| featureKey: keyof FeatureFlags; | |
| }): React.JSX.Element => { |
🤖 Prompt for AI Agents
In apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsx around lines
8-16, the functional component is missing an explicit return type; update the
component signature to include a React return type (e.g., add `:
React.JSX.Element` or `: JSX.Element` after the parameter list) so the function
has an explicit return type per coding guidelines; ensure necessary React types
are available from imports (add `import React from "react"` or import JSX types
if your TSX setup requires it) and then re-run typechecks.
| export const getFeatureFlag = (key: keyof FeatureFlags): boolean => { | ||
| const featureFlagKey = TOP_LEVEL_BLOCK_PROP_KEYS.featureFlags; | ||
|
|
||
| const { blockProps } = getBlockPropBasedSettings({ | ||
| keys: [featureFlagKey], | ||
| }); | ||
|
|
||
| const flags = FeatureFlagsSchema.parse(blockProps || {}); | ||
|
|
||
| return flags[key]; | ||
| }; |
There was a problem hiding this comment.
getFeatureFlag will fail with the current empty schema.
Since FeatureFlagsSchema is an empty object, flags[key] at Line 17 attempts to access a property that doesn't exist on the validated object type. TypeScript treats keyof FeatureFlags as never, and runtime access will return undefined (not boolean).
Once feature flags are added to the schema, consider adding a fallback default value to handle cases where a flag isn't yet defined:
🔎 Suggested improvement for robustness
export const getFeatureFlag = (key: keyof FeatureFlags): boolean => {
const featureFlagKey = TOP_LEVEL_BLOCK_PROP_KEYS.featureFlags;
const { blockProps } = getBlockPropBasedSettings({
keys: [featureFlagKey],
});
const flags = FeatureFlagsSchema.parse(blockProps || {});
- return flags[key];
+ return flags[key] ?? false; // Default to false if key is missing
};This ensures the function always returns a boolean even if a flag isn't explicitly set in storage.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getFeatureFlag = (key: keyof FeatureFlags): boolean => { | |
| const featureFlagKey = TOP_LEVEL_BLOCK_PROP_KEYS.featureFlags; | |
| const { blockProps } = getBlockPropBasedSettings({ | |
| keys: [featureFlagKey], | |
| }); | |
| const flags = FeatureFlagsSchema.parse(blockProps || {}); | |
| return flags[key]; | |
| }; | |
| export const getFeatureFlag = (key: keyof FeatureFlags): boolean => { | |
| const featureFlagKey = TOP_LEVEL_BLOCK_PROP_KEYS.featureFlags; | |
| const { blockProps } = getBlockPropBasedSettings({ | |
| keys: [featureFlagKey], | |
| }); | |
| const flags = FeatureFlagsSchema.parse(blockProps || {}); | |
| return flags[key] ?? false; // Default to false if key is missing | |
| }; |
🤖 Prompt for AI Agents
In apps/roam/src/utils/manageFeatureFlag.ts around lines 8–18, getFeatureFlag
currently reads flags[key] from an empty/partial schema which can be undefined
and TypeScript treats keyof FeatureFlags as never; change the function to always
return a boolean by providing a safe fallback and correct typing: ensure
FeatureFlagsSchema.parse returns a partial/optional shape or cast the parsed
result to Partial<FeatureFlags>, then return a boolean using a fallback (e.g.,
flags[key] ?? false or Boolean(flags[key])) so the function never returns
undefined.
| /* eslint-disable @typescript-eslint/naming-convention */ | ||
| export const FeatureFlagsSchema = z.object({ | ||
| }); | ||
| /* eslint-disable @typescript-eslint/naming-convention */ |
There was a problem hiding this comment.
Fix the ESLint comment pairing.
Line 6 duplicates the eslint-disable comment instead of re-enabling the rule. This leaves the naming-convention rule disabled for the remainder of the file.
🔎 Proposed fix
/* eslint-disable @typescript-eslint/naming-convention */
export const FeatureFlagsSchema = z.object({
});
-/* eslint-disable @typescript-eslint/naming-convention */
+/* eslint-enable @typescript-eslint/naming-convention */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* eslint-disable @typescript-eslint/naming-convention */ | |
| export const FeatureFlagsSchema = z.object({ | |
| }); | |
| /* eslint-disable @typescript-eslint/naming-convention */ | |
| /* eslint-disable @typescript-eslint/naming-convention */ | |
| export const FeatureFlagsSchema = z.object({ | |
| }); | |
| /* eslint-enable @typescript-eslint/naming-convention */ |
🤖 Prompt for AI Agents
In apps/roam/src/utils/zodSchemaForSettings.ts around lines 3 to 6, the ESLint
comment at line 6 mistakenly duplicates `/* eslint-disable
@typescript-eslint/naming-convention */` instead of re-enabling the rule; change
the second comment to `/* eslint-enable @typescript-eslint/naming-convention */`
(or remove the disable and scope the disable/enable pair properly around the
specific code) so the naming-convention rule is only disabled for the intended
block.
| export const FeatureFlagsSchema = z.object({ | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find apps/roam -type f -name "*zodSchemaForSettings*" 2>/dev/nullRepository: DiscourseGraphs/discourse-graph
Length of output: 118
🏁 Script executed:
cat -n apps/roam/src/utils/zodSchemaForSettings.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 385
🏁 Script executed:
rg -n "FeatureFlagsSchema|getFeatureFlag|setFeatureFlag|BlockPropFeatureFlagPanel" apps/roam/src --type ts --type tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 102
🏁 Script executed:
rg -n "FeatureFlag" apps/roam/src -A 3 -B 1 --type ts --type tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 102
🏁 Script executed:
rg -n "zodSchemaForSettings" apps/roam/src --type ts --type tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 102
🏁 Script executed:
rg -n "import.*FeatureFlagsSchema|from.*zodSchemaForSettings" apps/roam/src --type ts --type tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 102
🏁 Script executed:
find apps/roam/src -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "feature\|flag" {} \; 2>/dev/nullRepository: DiscourseGraphs/discourse-graph
Length of output: 568
🏁 Script executed:
cd apps/roam/src/utils && git log --oneline -n 20 zodSchemaForSettings.ts 2>/dev/null || echo "No git history available"Repository: DiscourseGraphs/discourse-graph
Length of output: 125
🏁 Script executed:
head -20 apps/roam/src/utils/zodSchemaForSettings.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 329
🏁 Script executed:
rg -n "TODO|FIXME|feature.flag|TODO.*feature" apps/roam/src --type ts --type tsx -i | head -20Repository: DiscourseGraphs/discourse-graph
Length of output: 102
🏁 Script executed:
cat -n apps/roam/src/components/settings/BlockPropFeatureFlagPanel.tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 1329
🏁 Script executed:
cat -n apps/roam/src/utils/isFlagEnabled.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 874
🏁 Script executed:
cat -n apps/roam/src/utils/manageFeatureFlag.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 1073
🏁 Script executed:
cat -n apps/roam/src/utils/pullWatchBlockPropsBasedSettings.ts | head -50Repository: DiscourseGraphs/discourse-graph
Length of output: 720
🏁 Script executed:
cat -n apps/roam/src/data/blockPropsSettingsConfig.tsRepository: DiscourseGraphs/discourse-graph
Length of output: 259
Add at least one feature flag definition to make the type system functional.
FeatureFlagsSchema is empty, making keyof FeatureFlags resolve to never. The component BlockPropFeatureFlagPanel and functions getFeatureFlag/setFeatureFlag cannot accept any flag name, preventing the feature flag system from being used.
Add at least one flag definition:
export const FeatureFlagsSchema = z.object({
+ exampleFeature: z.boolean().default(false),
});Alternatively, document in a comment that this PR establishes foundational infrastructure with flag definitions to be added in follow-up PRs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const FeatureFlagsSchema = z.object({ | |
| }); | |
| export const FeatureFlagsSchema = z.object({ | |
| exampleFeature: z.boolean().default(false), | |
| }); |
🤖 Prompt for AI Agents
In apps/roam/src/utils/zodSchemaForSettings.ts around lines 4-5,
FeatureFlagsSchema is currently empty which causes keyof FeatureFlags to be
never and breaks consumers; either add at least one concrete flag definition
(e.g., a boolean flag name with z.boolean()) so the type system yields usable
keys, or add a clear file comment stating this PR only adds infrastructure and
that actual flag entries will be added in a follow-up PR — update exports/types
accordingly so BlockPropFeatureFlagPanel, getFeatureFlag and setFeatureFlag
accept real flag names.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.