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(ai-help): send correct context when editing question #10511

Merged
merged 5 commits into from Feb 20, 2024

Conversation

fiji-flo
Copy link
Contributor

@fiji-flo fiji-flo commented Feb 14, 2024

Summary

Traverse the message tree in a correct way when gathering context.

Problem

We sent the wrong context when editing the first question (namely including the question/answer of the original version of the question), because we defaulted to 0 when traversing the message tree for context.

Solution

Add a traverseWithDefault flag to only default to 0 when initializing.

Screenshots

Before After
image image
image image

We defaulted to the 0 when traversing the tree for context.
This breaks editing question.
@fiji-flo fiji-flo requested a review from a team as a code owner February 14, 2024 09:27
@github-actions github-actions bot added plus work around features related to MDN Plus plus:ai-help labels Feb 14, 2024
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Can you add unit tests for the stateToMessagePath() function?

@caugner caugner marked this pull request as draft February 14, 2024 16:18
@fiji-flo fiji-flo marked this pull request as ready for review February 14, 2024 17:15
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, and tested locally, just two nits:

  1. Making the third parameter of stateToMessagePath() an options object.
  2. Updating the PR description (I already added screenshots) to reflect the latest insights of what the problem and solution was (it wasn't only defaulting to 0, was it?).

@@ -110,27 +110,32 @@ export interface MessageTreeState {

export function stateToMessagePath(
state: MessageTreeState,
path: number[]
path: number[],
traverseWithDefault: boolean = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, one last nit is that it would be better to make this an options object:

Suggested change
traverseWithDefault: boolean = false
{ traverseWithDefault = false }: { traverseWithDefault: boolean }

Like this, it is more clear where we're calling it what the true stands for.

@caugner caugner changed the title fix(ai-help): send correct questions when editing fix(ai-help): send correct context when editing question Feb 15, 2024
@fiji-flo fiji-flo merged commit c49c125 into main Feb 20, 2024
15 checks passed
@fiji-flo fiji-flo deleted the MP-890-fix-ai-help-tree branch February 20, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plus:ai-help plus work around features related to MDN Plus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants