Skip to content

Making Arrow filtering and Center Alignment toggles persistent across CSE updates#3887

Closed
ThatLi wants to merge 40 commits into
masterfrom
main
Closed

Making Arrow filtering and Center Alignment toggles persistent across CSE updates#3887
ThatLi wants to merge 40 commits into
masterfrom
main

Conversation

@ThatLi
Copy link
Copy Markdown
Contributor

@ThatLi ThatLi commented May 22, 2026

Description

This makes the toggles for arrow filtering and center alignment persistent across Editor resets from the shift + Enter, etc.
As outlined by the suggestions made by prof. Martin and the other frontend teams below.

image

This resolves #3788 , but this is achieved without using Redux.

How to test

Using the code excerpt below:

function foo(n) {
    const msg = "Frame";
}
const arr = [];
for (let j = 0; j < 3; j = j + 1) {
    foo(j);
    arr[j] = [j];
}

Tick the toggles for centerAlignment and deselect one of the arrow filter, press shift + Enter or click 'Run'.
Observe that the toggles are not reset to their default status anymore, the choices are now persistent!

Checklist

  • I have tested this code
  • I have updated the documentation

Akshay-2007-1 and others added 30 commits February 19, 2026 15:52
Only dead frames disappear as of right now, for function objects and the rest, work to be done

Animations : will be done by Keying!
	function closure objects
	Generic Arrow
	Text (recursively automated)
	Arrays and hence pairs as well
Finally processed the logic by recomputing the envTree for the specific step when the toggle is on
1. Fixed the snapshot and lint erros
2. Added the disable feature (as suggested by @sayomaki) where to suggest that its CLEAR and not HIDE, we disable after pressing once!!
3. Changed all instances of hide to clear in our current implementation!
…am's invariant!

Now the frames do move by violating their invariant ONLY if the Clear mode is ON
1. Capped out the func desc box's widths
2. Click to reveal FULL length descriptions in normal mode
3. Fixed gaps between the FRAMES in PRINTABLE mode
…g set and was capped out!

1. This makes every other test case which we have made look MUCH better!
2. Secondly, we also fixed a bug where recursive closure objects' description boxes' width and length was NOT being
	accounted for
…cription spacing

- normalize Frame width semantics:
  - totalWidth now represents only frame block width
  - totalDataWidth is computed as true overflow beyond frame right edge
  - remove pre-estimation path that double-counted spacing in some cases
- update Level width derivation to include last frame totalDataWidth
- improve printable mode readability for function descriptions:
  - add small vertical offset above printable fn description labels
  - add tiny bottom gap below printable fn description boxes
  - propagate new spacing to Binding and ArrayValue height calculations

This reduces excessive horizontal gaps and mitigates small printable-mode overlaps between fn description boxes
and nearby array/binding content.
Also removed the excess frame offset width given for safety that was NOT dynamic!
So now, the frame width gapping is dynamic!
- Populates both Normal and Printable caches upfront in drawCse.
- Removes redundant cache generation logic from redraw.
- Prevents premature cache clearing in clearCse to maintain stability.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request modifies the CSE machine to maintain the user's current step during updates and adjust layout behavior. The review identifies a critical infinite loop in SideContentCseMachine.tsx triggered by evaluation calls within the component lifecycle. Additionally, the reviewer noted redundant state management for center alignment in Playground.tsx and unnecessary coordinate reassignment in CseMachineLayout.tsx, suggesting that existing internal logic already handles these cases more robustly.

// Re-run the stepper at the preserved step so the display remains at the
// same step after editor update while still refreshing the CSE internals.
this.sliderShift(preservedStep);
this.sliderRelease(preservedStep);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

Calling this.sliderRelease(preservedStep) inside componentDidUpdate will trigger an infinite loop of evaluations.

When an evaluation finishes, needCseUpdate transitions from true to false, triggering this block. sliderRelease then calls handleEditorEval, which dispatches evalEditor. In Playground.tsx, evalEditor sets updateCse (the source for needCseUpdate) back to true. Once that evaluation completes, needCseUpdate becomes false again, and the cycle repeats.

If the goal is to restore the step after a code change, this logic needs a guard to prevent re-triggering evaluation if it was already initiated by this restoration process, or the step reset logic in Playground.tsx should be adjusted instead.

Comment on lines +427 to +431
const wasCenterAligned = CseMachine.getCenterAlignment();
CseMachine.clearCachedLayouts();
if (wasCenterAligned) {
CseMachine.toggleCenterAlignment();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Manually toggling center alignment here is redundant and potentially ineffective.

  1. Redundancy: CseMachine.drawCse already handles the temporary state management (setting centerAlignment to false) required to build the layout cache internally (see CseMachine.tsx lines 344-363).
  2. Inconsistency: Since evalEditor is an asynchronous operation (it dispatches a saga), the toggle-back at line 447 will execute almost immediately, likely before the CSE machine actually performs its draw. This means the machine will still see the 'on' state during its execution.
  3. Side Effects: toggleCenterAlignment sets the centerAlignmentToggled flag, which may trigger unnecessary coordinate recalculations in redraw.
Suggested change
const wasCenterAligned = CseMachine.getCenterAlignment();
CseMachine.clearCachedLayouts();
if (wasCenterAligned) {
CseMachine.toggleCenterAlignment();
}
CseMachine.clearCachedLayouts();
References
  1. When refactoring components, avoid introducing state tracking mechanisms if the current behavior is an intentional design choice.

Comment on lines +446 to 448
if (wasCenterAligned) {
CseMachine.toggleCenterAlignment();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

This toggle-back logic should be removed along with the initial toggle at line 430. As noted previously, the internal state management in CseMachine.drawCse is sufficient and more robust for handling layout cache generation.

References
  1. When refactoring components, avoid introducing state tracking mechanisms if the current behavior is an intentional design choice.

Comment thread src/features/cseMachine/CseMachineLayout.tsx
@coveralls
Copy link
Copy Markdown

Coverage Report for CI Build 26277734870

Coverage decreased (-0.001%) to 42.321%

Details

  • Coverage decreased (-0.001%) from the base build.
  • Patch coverage: 7 uncovered changes across 2 files (3 of 10 lines covered, 30.0%).
  • 1 coverage regression across 1 file.

Uncovered Changes

File Changed Covered %
src/pages/playground/Playground.tsx 4 0 0.0%
src/commons/sideContent/content/SideContentCseMachine.tsx 3 0 0.0%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File Lines Losing Coverage Coverage
src/pages/playground/Playground.tsx 1 54.49%

Coverage Stats

Coverage Status
Relevant Lines: 13426
Covered Lines: 6124
Line Coverage: 45.61%
Relevant Branches: 6935
Covered Branches: 2493
Branch Coverage: 35.95%
Branches in Coverage %: Yes
Coverage Strength: 27.37 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

CSE Machine: option selection does not survive "Run"

6 participants