Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
f11b47f to
10445e6
Compare
ea615c9 to
95e291a
Compare
29817b9 to
9cd0a2c
Compare
10445e6 to
0fb7fc7
Compare
db18fd8 to
23cedec
Compare
0fb7fc7 to
62b0359
Compare
23cedec to
496a3ba
Compare
62b0359 to
40379a1
Compare
496a3ba to
0238148
Compare
40379a1 to
ac5f761
Compare
0238148 to
8dbd24c
Compare
a671592 to
6e0130f
Compare
77d1c95 to
0d233ba
Compare
0d233ba to
cf49c4c
Compare
6e0130f to
050f93b
Compare
cf49c4c to
122fce8
Compare
mdroidian
left a comment
There was a problem hiding this comment.
I was under the assumption that we were going to run both the legacy settings and new blockProps at the same time while the migration was happening. It looks like this PR is just migrating to the blockProps. When is the migration supposed to take place? How are we going to test it?
Also, please make sure to save all files with prettier before requesting review.
| key: keyof FeatureFlags, | ||
| value: boolean, | ||
| ): void => { | ||
| window.dispatchEvent( |
There was a problem hiding this comment.
Why do we need a pullWatch and a dispatchEvent? Why don't we go directly to the dispatchEvent and skip the pullWatch? I'm worried that we are adding additional infrastruction that is unnecessary.
| editor.updateShapes([{ id: arrow.id, type: target.type }]); | ||
| } | ||
| if (getSetting<boolean>(USE_REIFIED_RELATIONS, false)) { | ||
| if (getFeatureFlag("Reified Relation Triples")) { |
There was a problem hiding this comment.
It looks like this is only calling blockProp based settings? I was under the assumption that we were going to run both at the same time while the migration was happening.
| // "Suggestive Mode Enabled": (newValue) => { ... }, | ||
| // "Reified Relation Triples": (newValue) => { ... }, | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| "Enable Left Sidebar": (newValue) => { |
There was a problem hiding this comment.
We've discussed the naming before, should be sentence case. #664 (review)
| onCancel={() => setIsInstructionOpen(false)} | ||
| confirmButtonText="Reload Graph" | ||
| cancelButtonText="Later" | ||
| confirmButtonText="Got it" |
122fce8 to
fcfa83f
Compare
050f93b to
c022156
Compare
c022156 to
cf56730
Compare
fcfa83f to
1adcb4e
Compare
| return createCleanupFn(watches); | ||
| }; |
There was a problem hiding this comment.
🔴 Pull watchers for feature flags are never initialized
The setupPullWatchOnSettingsPage function is defined in pullWatchers.ts but is never called anywhere in the codebase. This means the feature flag change handlers will never be triggered.
Click to expand
Impact
When users toggle feature flags via the FeatureFlagPanel component:
setFeatureFlagis called (accessors.ts:277-286)- The block props are updated in Roam
- But the pull watcher that should detect this change and call the handlers is never set up
emitFeatureFlagChangeis never called- React components using
useFeatureFlaghook won't re-render - Side effects like
remountLeftSidebar(),unmountLeftSidebar(),initializeSupabaseSync(), andsetSyncActivity()will never execute
Actual vs Expected
Actual: The feature flag handlers in pullWatchers.ts:96-121 are defined but never invoked because setupPullWatchOnSettingsPage is never called.
Expected: setupPullWatchOnSettingsPage should be called during plugin initialization (e.g., in index.ts) to set up the pull watchers that trigger the handlers when feature flags change.
Evidence
Searching the codebase shows setupPullWatchOnSettingsPage is only exported, never imported or called:
apps/roam/src/components/settings/utils/pullWatchers.ts:export const setupPullWatchOnSettingsPage = (
(Refers to lines 184-253)
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Closing because too many changes that we don't want to merge in main at this point |

https://www.loom.com/share/cafa18f38454407f85b2f25da6a5180e
new checkbox component for feature flag get and set in props, migrate to block prop based settings
use getFeatureFlag()
rerender left sidebar on enable/disable