ENG-1456: Migrate personal boolean flag consumers (getSetting → getPersonalSetting)#814
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR migrates feature flag and preference retrievals from global extension settings (via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
11a99fe to
c635833
Compare
63856a1 to
368b3f5
Compare
c635833 to
b1ace1a
Compare
368b3f5 to
5c11932
Compare
b1ace1a to
c90e2c8
Compare
5c11932 to
4843dfb
Compare
c90e2c8 to
3b1643c
Compare
4843dfb to
c18d483
Compare
c18d483 to
c10cb49
Compare
3b1643c to
ee38a5f
Compare
c10cb49 to
d48c767
Compare
ee38a5f to
c7c04c6
Compare
d48c767 to
7995a91
Compare
c7c04c6 to
2f2cf80
Compare
7995a91 to
75d06ff
Compare
080ebb1 to
f1855a5
Compare
75d06ff to
3e2cebd
Compare
3e2cebd to
4051fe3
Compare
5af843f to
dd836c8
Compare
4051fe3 to
0c3862e
Compare
dd836c8 to
838709c
Compare
0c3862e to
56e9e66
Compare
838709c to
bcc0697
Compare
56e9e66 to
d4bb8a1
Compare
bcc0697 to
9d90ba0
Compare
d4bb8a1 to
896fcaa
Compare
f8c803d to
fe4caa1
Compare
896fcaa to
e32349c
Compare
mdroidian
left a comment
There was a problem hiding this comment.
A couple comments.
- Let's make a task to change the
getPersonalSettingto a typed value or shared constant. - Could you communicate the context for why we are removing
initialValueand what it's being replaced with?
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const isOverlayEnabled = useMemo( | ||
| () => getSetting(DISCOURSE_CONTEXT_OVERLAY_IN_CANVAS_KEY, false), | ||
| () => getPersonalSetting<boolean>(["Overlay in canvas"]), |
There was a problem hiding this comment.
Could we promote this to a shared constant or typed key? That would give us autocomplete and eliminate the risk of subtle string mismatches.
Ideally that's something we look into doing before merging to main.
There was a problem hiding this comment.
We can, it depends on what the end goal is. I am coming from the pov that this is intermediate code that is not going to be merged to main, we are merging all this to a staging branch, testing it thoroughly internally then removing the intermediate code i.e both the dual-write and dual-read.
So if this is intermediate then we should not update this code, if the end goal is that we will merge this main and then do real world testing and monitoring, and at some point in future cleanup the intermediate code, then we should do this.
There was a problem hiding this comment.
It can be for the future, as long as it is tracked somewhere. Ideally a linear ticket.
There was a problem hiding this comment.
| title="Overlay" | ||
| description="Whether or not to overlay discourse context information over discourse node references." | ||
| settingKeys={["Discourse context overlay"]} | ||
| initialValue={getSetting<boolean>("discourse-context-overlay", false)} |
There was a problem hiding this comment.
What is initialValue being replaced with?
There was a problem hiding this comment.
Before: Callers read the value manually and passed it as initialValue:
<PersonalFlagPanel
settingKeys={["Discourse context overlay"]}
initialValue={getSetting<boolean>("discourse-context-overlay",
false)}
/>
After: PersonalFlagPanel reads it internally — same lookup, just moved inside:
<PersonalFlagPanel
settingKeys={["Discourse context overlay"]}
/>
The wrapper does getPersonalSetting(settingKeys) which calls the
same getSetting(legacyKey, zodDefault) under the hood. The legacy
key mapping ("Discourse context overlay" → "discourse-context-overlay") and default (false) come from the
accessor layer now instead of the call site.
This was set up in ENG-1472 — PersonalFlagPanel (#813) already self-reads,
so the initialValue props were redundant.
fe4caa1 to
1657b2b
Compare
…ult reads (#813) * ENG-1472: Refactor BlockPropSettingPanels to add accessor-backed default reads (with initialValue fallback) * review, fix
e32349c to
185006d
Compare
6ecf11f
into
eng-1472-refactor-blockpropsettingpanels-to-add-accessor-backed

https://www.loom.com/share/e63f0beb46e84ee7b0d279174a37942e
Summary by CodeRabbit