[Codex] microsoft/pxt#10590 — Error Help - Part 1 - #39
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5b0c0a4e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const totalDisplaySteps = config.numberFinalStep ? steps.length : steps.length - 1; | ||
| return <TeachingBubble id="teachingBubble" | ||
| targetContent={tourSteps[currentStep]} | ||
| targetContent={steps[currentStep]} |
There was a problem hiding this comment.
Guard against empty tour steps before rendering bubble
If the AI returns an empty explanationSteps list, config.steps is empty and this passes undefined as targetContent, which later gets dereferenced in TeachingBubble (for example targetContent.onStepBegin / targetContent.title) and can crash the UI when users click “Help me understand” in blocks. Add a precondition to handle steps.length === 0 (e.g., show an error and close the tour) before rendering.
Useful? React with 👍 / 👎.
| <p aria-live="polite">{targetContent.description}</p> | ||
| <div className={`teaching-bubble-footer ${!hasSteps ? "no-steps" : ""}`}> | ||
| {hasSteps && <div className="teaching-bubble-steps" aria-live="polite"> | ||
| {hasSteps && <div className={classList("teaching-bubble-steps", forceHideSteps && "hidden")} aria-live="polite"> |
There was a problem hiding this comment.
Use the intended step-hide class name in teaching bubble
The component applies "hidden" when forceHideSteps is true, but the new stylesheet rule is defined for .teaching-bubble-steps.hide. Because of the mismatch, this rule is never used and the global .hidden { display:none } behavior takes over, which changes layout behavior from the intended visibility:hidden and causes the footer to reflow on the final step.
Useful? React with 👍 / 👎.
Overview
This is the first set of front-end changes for the "Error Help" feature. It adds a "Help me understand" button to the error list, which sends a request to our backend for assistance on the current errors in the error list. In blocks, it then displays help in the form of an editor tour. In text, it simply adds a small chunk of explanatory text to the top of the error list.
Part of https://github.com/microsoft/pxt-microbit/issues/6138
Breakdown
The key components of the change are:
errorHelp.tsfile.blocks.tsxwhile text-specific stuff stayed inmonaco.tsx...but I'll admit, it still feels a little scattered.What is NOT in this change
In the interest of keeping this change to a (somewhat) manageable size, I've decided to postpone the following items for future changes. Sub-issues filed here: https://github.com/microsoft/pxt-microbit/issues/6138
Must Haves
Nice to Haves
Try it: https://microbit.staging.pxt.io/app/67201f689bf390a088508664dd715cfef4585b54-3712e8e736
Mirrored from upstream PR:
https://github.com/microsoft/pxt/pull/10590Created automatically by pr-sxs-human-evals for code-review agent comparison.
(URL wrapped in a code span so GitHub does not create a cross-reference on the upstream timeline.)