-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Make it possible to dogfood new prompt editor #7094
fix: Make it possible to dogfood new prompt editor #7094
Conversation
This change is part of the following stack: Change managed by git-spice. |
I've added reviewers suggested by GitHub because I don't which of the cody teams responsibility this is. Feel free to add more appropriate reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const experimentalPromptEditorEnabled = await firstValueFrom( | ||
featureFlagProvider.evaluatedFeatureFlag(FeatureFlag.CodyExperimentalPromptEditor) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const experimentalPromptEditorEnabled = await firstValueFrom( | |
featureFlagProvider.evaluatedFeatureFlag(FeatureFlag.CodyExperimentalPromptEditor) | |
) | |
const experimentalPromptEditorEnabled = configuration.internalUnstable |
If this is only for internal / dev to dogfood, maybe we can use the local editor settings instead of a feature flag? We have a (less-known) configuration used for enabling features for internal dogfood that we could re-use if it helps :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would, but how does this work for Cody web? Would be good to dogfood there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge this as is since we also want to test it in cody web.
92f08ce
to
2429909
Compare
Closes https://linear.app/sourcegraph/issue/SRCH-1667/no-results-returned-when-enabling-feature-for-queries
This PR fixes an issue with the prompt input being cleared out when a query is submitted and the new input is enabled.
Root cause
The root cause for this issue is twofold:
HumanMessageEditor
is rendered we will render theold editor.
useImperativeHandle
is only run once when the component is mounted. This means we neverupdate the ref with the ref from the new editor once the feature flag updates.
The consequence of this is that everywhere else where
editorRef
is used we have a reference to the old editor API,which has been destroyed since. Reading the value from it will of course be empty, which in turn causes the input value
of the rendered editor to be overwritten.
Solution
It seems obvious to remove the empty dependency list from the
useImperativeHandle
call to make that the ref is alwaysupdated. We do this in other places too (
Transcript.tsx
).However, Vova pointed out that there can still be race conditions between accessing
humanEditoRef
and the feature flagloading and suggested that we should retrieve the feature flag value before we render any UI.
I've extended the object returned by
getConfigForWebview
but I'm not sure whether that's the right place to put it.I've also heard that we want to make as few network requests as possible during initialization, which is understandable.
Important
I'd like some guidance from the cody team on where put this setting to ensure avoid the aforementioned problem.
Test plan
cd client/web; pnpm dev
)@...
looks slightly different)