Skip to content
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

Conversation

fkling
Copy link
Contributor

@fkling fkling commented Feb 13, 2025

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:

  1. Evaluating feature flags is always asynchronous. The first time HumanMessageEditor is rendered we will render the
    old editor.
  2. In the same component, useImperativeHandle is only run once when the component is mounted. This means we never
    update 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 always
updated. 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 flag
loading 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

  1. Enabled the feature flag for myself on S2.
  2. Ran the standalone web demo (cd client/web; pnpm dev)
  3. Verified that the new input used (typing @... looks slightly different)
  4. Submitting a query does not clear the input and generates an appropriate response.

@fkling
Copy link
Contributor Author

fkling commented Feb 13, 2025

This change is part of the following stack:

Change managed by git-spice.

@fkling fkling marked this pull request as ready for review February 13, 2025 20:55
@fkling
Copy link
Contributor Author

fkling commented Feb 13, 2025

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.

@fkling fkling self-assigned this Feb 13, 2025
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started web and I think it's using the v2 editor? Left a unblocking comment inline about using local config instead if it's for internal use only :)

image

Comment on lines +533 to +535
const experimentalPromptEditorEnabled = await firstValueFrom(
featureFlagProvider.evaluatedFeatureFlag(FeatureFlag.CodyExperimentalPromptEditor)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 :)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@fkling fkling force-pushed the fkling-srch-1667-no-results-returned-when-enabling-feature-for-queries branch from 92f08ce to 2429909 Compare February 15, 2025 10:23
@fkling fkling merged commit d80689c into main Feb 15, 2025
20 of 22 checks passed
@fkling fkling deleted the fkling-srch-1667-no-results-returned-when-enabling-feature-for-queries branch February 15, 2025 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants