Stepper: add a fixed-text hover popover capability to SyntaxProfile - #92
Merged
Conversation
A named function value (SyntaxProfile.functionValues) already gets a mu-term + hover popover showing its body, but there was no way to show a popover for something that isn't a steppable function value at all -- e.g. a builtin referenced as a bare value, or a name imported from a module -- since there is no body to expand. Add SyntaxProfile.hoverText: a node type can declare a property holding an already-formatted, static line of text (e.g. "built-in function print"); the host wraps that node's existing inline rendering in a Popover showing it, without collapsing/replacing the rendering the way functionValues does. Generic in the host -- any language opts in per node type, no per-language host code -- exactly mirroring how functionValues itself works. Needed by py-slang#404 (a builtin used as a value has no hover at all today) and the pop-up half of py-slang#406 (imported module functions). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hv4gVtVgskzLuPpPh3woaE
🦋 Changeset detectedLatest commit: 32b2d3a The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
5 tasks
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughAdds optional ChangesHover text popovers
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant SyntaxProfile
participant ProfileRenderedNode
participant ProfileHoverTextPopover
SyntaxProfile->>ProfileRenderedNode: provide hoverText rules
ProfileRenderedNode->>ProfileRenderedNode: match node type and textProp
ProfileRenderedNode->>ProfileHoverTextPopover: wrap non-empty template output
ProfileHoverTextPopover-->>ProfileRenderedNode: display fixed hover text
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/web/stepper/src/SubstVisualizer.tsx (1)
960-980: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused coverage for the new rendering contract.
Test direct and dotted
textPropvalues, missing or empty text, preserved inline template output, and the overlap behavior withfunctionValues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/web/stepper/src/SubstVisualizer.tsx` around lines 960 - 980, Add focused tests for the hover-text rendering logic around the profile hover rule: cover direct and dotted textProp lookup, missing or empty text, preservation of the template-generated inline output, and behavior when the node also matches functionValues. Reuse the existing SubstVisualizer test setup and assert both rendered content and Popover presence or absence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/web/stepper/src/SubstVisualizer.tsx`:
- Around line 960-980: Update the rendering flow around the function-value
handling and hoverText lookup so nodes matching both profile rules receive both
independent popover behaviors instead of returning before hoverText is
evaluated. Apply the fixed-text hover wrapper after the function-value branch,
or otherwise preserve both wrappers without early-skipping the hoverText logic;
use the existing hoverRule and ProfileHoverTextPopover symbols.
---
Nitpick comments:
In `@src/web/stepper/src/SubstVisualizer.tsx`:
- Around line 960-980: Add focused tests for the hover-text rendering logic
around the profile hover rule: cover direct and dotted textProp lookup, missing
or empty text, preservation of the template-generated inline output, and
behavior when the node also matches functionValues. Reuse the existing
SubstVisualizer test setup and assert both rendered content and Popover presence
or absence.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dfc1ca17-92f2-4d4c-95f8-d45851dc0c85
📒 Files selected for processing (3)
.changeset/stepper-hover-text-popover.mdsrc/common/stepper/src/index.tssrc/web/stepper/src/SubstVisualizer.tsx
CodeRabbit caught this on #92: the hoverText popover check only ran in the *else* branch (the plain-template render), so a node type listed in both functionValues and hoverText would silently get only the mu-term treatment -- contradicting the doc comment's own claim that the two are independent. Move the hoverText wrap after the if/else so it applies to whichever result either branch produced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hv4gVtVgskzLuPpPh3woaE
Member
Author
|
Fixed the actionable comment in 32b2d3a — the `hoverText` check now runs after the function-value if/else rather than only in the `else` branch, so a node type listed in both `functionValues` and `hoverText` gets both behaviors instead of silently losing the popover. Skipping the nitpick about adding unit tests for now — there's no existing test harness for |
martin-henz
added a commit
to source-academy/py-slang
that referenced
this pull request
Aug 7, 2026
* Stepper: give a built-in used as a value a hover popup is_function(print) had no way to show what `print` (unapplied) is: a bare built-in-named Identifier rendered like any other name, with no popup, unlike a user-defined function's mu-term hover. The obvious "hack" -- checking whether an Identifier's name matches a builtin -- already exists throughout reduce.ts/builtins.ts for actual evaluation (contractCall's built-in dispatch), but that check can't be baked into a real node type at translation time: a builtin's name can be locally shadowed (e.g. `def print(x): return is_function(print)` is valid, if confusing, Python -- see the extended discussion on py-slang#404), and py-slang's substitution model only resolves that shadowing as reduction proceeds, not statically up front. Add ast.ts's markBuiltins: a display-only pass getSteps.ts runs on each step's tree, after reduction, right before serialization. A bare Identifier surviving to that point genuinely refers to the built-in of that name -- any shadowing binding has already substituted the occurrence away by then, the same guarantee contractCall already relies on -- except inside a not-yet-invoked function value's own body, which is opaque cargo substitution hasn't reached yet, so the walk tracks bound names there exactly like substitute() does, to avoid mislabelling a future recursive self-reference as a builtin. Since this walk runs after a contraction's preRedex/postRedex are already captured by identity from the pre-relabeling tree, it also returns a correspondence map so getSteps.ts can translate a marker's redex into its (possibly relabeled) counterpart -- needed whenever the redex itself is what gets relabeled, e.g. breakpoint()'s own callee. Renders via the new Builtin node type + syntaxProfile.ts's hoverText rule, added to @sourceacademy/common-stepper/web-stepper in source-academy/plugins#92 (not yet published, hence the `as unknown as SyntaxProfile` cast, dropped once that version ships). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hv4gVtVgskzLuPpPh3woaE * Fix markBuiltins: sibling/pre-declaration shadowing at un-reduced steps CodeRabbit caught this on #409: markBuiltins walked a Program's or BlockStatement's sibling statements independently, all sharing the same `bound` set. At the very first ("Start of evaluation") step -- before any reduction/substitution has touched the tree -- this meant a later statement referencing a name a not-yet-processed earlier statement is about to declare (def print(x): ...\nprint(1)) got wrongly relabeled Builtin, since nothing had "removed" that forward reference from view yet the way live substitution eventually will. Special-case Program/BlockStatement to walk their body left to right, adding each statement's own declared name (a def's name, or a `x = ...` target) to `bound` before walking the statements after it -- simulating, for display, the same shadowing stepHead's actual declare-and-substitute step will produce once reduction gets there. Surfaced a second, related bug while fixing the first: a VariableDeclaration wasn't special-cased at all, so its own `id` (the declaration target itself, not a reference) got walked generically by the default mapChildren fallback and could be mislabelled too (e.g. a local `print = 5` inside a function body, checked before that statement is even reached). Added a VariableDeclaration case that only walks `init`, mirroring how substitute() already treats VariableDeclarator's `id` the same way. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hv4gVtVgskzLuPpPh3woaE * Fix prettier formatting in getSteps.test.ts CI's format:ci caught two lines over the print width, left over from an earlier commit. Applied prettier --write. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hv4gVtVgskzLuPpPh3woaE --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SyntaxProfile.functionValuesalready gives a named function value (adef/lambda) a mu-term + hover popover showing its body. There was no equivalent for something that isn't a steppable function value at all — a builtin referenced as a bare value, or a name imported from a module — since there is no body to expand.SyntaxProfile.hoverText: a language declares a node type + the (dotted) property holding an already-formatted, static line of popover text (e.g."built-in function print"). The host wraps that node's normal inline rendering — unchanged — in aPopovershowing the text. Fully generic: any language opts in per node type, no per-language host code, mirroring howfunctionValuesitself is implemented.@sourceacademy/common-stepperand@sourceacademy/web-stepperboth bumped (patch) via changeset — purely additive, backward-compatible.Test plan
yarn build— clean (the two pre-existingTS2322warnings onrenderFunctionArgumentsare unrelated, reproduce identically onmain)yarn eslint src/common/stepper/src/index.ts src/web/stepper/src/SubstVisualizer.tsx— cleanyarn test— 181/181 passing🤖 Generated with Claude Code
https://claude.ai/code/session_01Hv4gVtVgskzLuPpPh3woaE