Fix breakpoints state going out of sync #2647
Merged
Conversation
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
ianyong
commented
Sep 2, 2023
|
|
||
| // Write editor state for the active editor tab to the store. | ||
| handleEditorValueChange(props.editorTabIndex, newCode); | ||
| handleEditorUpdateBreakpointsRef.current( |
There was a problem hiding this comment.
No idea why this callback function is wrapped in a ref but I'm afraid that removing it will cause some other regression elsewhere.
martin-henz
approved these changes
Sep 3, 2023
RichDom2185
pushed a commit
to NUS-CS1101S/cadet-frontend
that referenced
this pull request
Sep 10, 2023
Co-authored-by: Martin Henz <henz@comp.nus.edu.sg>
RichDom2185
pushed a commit
to NUS-CS1101S/cadet-frontend
that referenced
this pull request
Sep 10, 2023
Co-authored-by: Martin Henz <henz@comp.nus.edu.sg>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
In #2269 & #2346, the behaviour of breakpoints was updated to be consistent with most code editors & IDEs available. However, due to the way the
Editorcomponent and the workspaces are implemented, theEditormaintains its own source of truth that is separate from the Redux store. The bug in #2646 occurs when the breakpoints state in the Redux store and theEditorcomponent become unsynchronised.To fix this issue, we write the current
Editorbreakpoints state to the Redux store whenever theEditorvalue updates. Ideally, theEditorshould rely on the Redux store as the single source of truth, but this would require major refactoring and I'm not confident of not breaking other things along the way due to the lack of comprehensive tests + the rather hacky implementation of theEditor.Fixes #2646.
Type of change
How to test
Replicate
Checklist
Ideally, I would write regression tests here, but testing this particular logic requires some test framework that can simulate user input such as Cypress (and setting up such a test framework is beyond the scope of this PR).