OBEE-26 FE: add user setting for opt-in LLM capabilities - #1359
OBEE-26 FE: add user setting for opt-in LLM capabilities#1359tslil-topos wants to merge 1 commit into
Conversation
## Design decisions * Store frontend-owned user settings in a separate `settings` subtree of the existing Automerge user-state document. Missing settings resolve to disabled, so existing users do not require a migration. * Expose resolved settings and shallow patch updates through Solid context following automerge. * Inference keys are requested only after the authenticated user's settings have loaded and LLM capabilities are enabled. * Keep LLM Conversation documents in raw user state, but hide them from document lists and picker completions while the setting is disabled. ## Tests Add a frontend behavior test verifying that disabled LLM capabilities prevent inference-key provisioning and hide LLM Conversation documents.
Notion Integration |
| if (doc.settings === undefined) { | ||
| doc.settings = {}; | ||
| } | ||
| Object.assign(doc.settings, patch); |
There was a problem hiding this comment.
looking around this appears to be the standard idiom for applying shallow mutations. I admit to not really understanding what's going on here, if there's a better way i'd be happy to adopt.
There was a problem hiding this comment.
Usually we expose the document handle and call docHandle.change with a specific change, i.e., setting some specific field, rather than wrapping the docHandle.change with a patch.
epatters
added
enhancement
There was a problem hiding this comment.
Thanks for making a start on this! Solidjs definitely takes some getting used to because it has lots of idioms (sometimes actual requirements) that aren't enforced by the type system or API but that the programmer is supposed to understand.
I left a few comments below.
A question I have for @kasbah (who should also review this PR :) is whether it's safe to add new fields to the user state solely on the frontend side, without ever persisting them to the DB. I know that the backend regularly updates the user state but I don't recall how destructive that is. My recollection of the design is that the user state is intended to be ephemeral.
| const [userState, setUserState] = createStore<UserState>(INITIAL_USER_STATE); | ||
| const [userState, setUserState] = createStore<AppUserState>(INITIAL_USER_STATE); | ||
| const [isReady, setIsReady] = createSignal(false); | ||
| const settings = createMemo<UserSettings>(() => ({ |
There was a problem hiding this comment.
A Solidjs store has fine-grained reactivity, and the settings data is just a field in that store, so it shouldn't be necessary, and doesn't feel very idiomatic, to construct this memo derived from the user state. Consumers of the settings can just pull out the fields they need from the user state on demand.
There was a problem hiding this comment.
thanks, my approach was indeed convoluted here, i'm working on making this and the other simplifications as i come to understand the solid way ™️
| return ( | ||
| <UserStateContext.Provider value={userState}>{props.children}</UserStateContext.Provider> | ||
| <UserStateContext.Provider value={userState}> | ||
| <UserSettingsContext.Provider value={settingsContext}> |
There was a problem hiding this comment.
Building on the previous comment, on my understanding, you shouldn't need to create a separate context and provider for the settings, since it's already available through the context and provider for the user state.
| const firebaseApp = useFirebaseApp(); | ||
| const [userState, setUserState] = createStore<UserState>(INITIAL_USER_STATE); | ||
| const [userState, setUserState] = createStore<AppUserState>(INITIAL_USER_STATE); | ||
| const [isReady, setIsReady] = createSignal(false); |
There was a problem hiding this comment.
Is this isReady signal necessary? Outside of tests we don't usually wait around for docHandle.whenReady.
| if (doc.settings === undefined) { | ||
| doc.settings = {}; | ||
| } | ||
| Object.assign(doc.settings, patch); |
There was a problem hiding this comment.
Usually we expose the document handle and call docHandle.change with a specific change, i.e., setting some specific field, rather than wrapping the docHandle.change with a patch.
|
thanks for the review Evan! I hope to have this resolved by (your) Monday SOD. |
2 participants
Caveat emptor
I don't really understand Solid/JSX yet. I tried to follow existing codebase conventions, but the mapping of my intent onto code may be imperfect here. In particular, the UI is ugly. I leave that to someone more capable.
What this achieves
Unless you tick this box, you can't:
Design decisions
Store frontend-owned user settings in a separate
settingssubtree of the existing Automerge user-state document. Missing settings resolve to disabled, so existing users do not require a migration.Expose resolved settings and shallow patch updates through Solid context following automerge.
Inference keys are requested only after the authenticated user's settings have loaded and LLM capabilities are enabled.
Keep LLM Conversation documents in raw user state, but hide them from document lists and picker completions while the setting is disabled.
Implementation details
The fundamental way that this is all achieved is by making a new
AppUserStateand migrating uses of
UserState(which is the document controlled, in some sense, by the back-end) toAppUserStatein the FE.Tests
Add a frontend behavior test verifying that disabled LLM capabilities prevent inference-key provisioning and hide LLM Conversation documents.