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 failing note cursor #2735

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

snqb
Copy link
Collaborator

@snqb snqb commented Dec 25, 2024

fixes #2721, is ad-hoc fix, needs a deeper think to be not-ad-hoc

it was onFocus of a newly created note that triggered an incorrect dispatch of setCursor:

const simplePath = simplifyPath(state, path) || path
.
.
.
setCursor({
  path: simplePath,
  .
  .
}

the problem was: simplifyPath would return an empty array at times like this.
The and we'd get simplifyPath(state, path) || path) = [] || path=[]` , so the fallback never really worked 😄

After putting a more elaborate condition, and using path as a fallback, we get broken editor behavior: the part of the tree collapses(we go to a wrong path)

In the end, when I just did not allow [] paths, it fixed the thing. And the app, from my manual tests, works just fine. The cursor is there, everything is smooth.
setDescendant action puts the appropriate state, from what I observed.

and on the reasons why we even get to the case of simplifyPath returning [] here, I'm not really sure, and feels like, we should dig there, if we want more than just a ad-hoc bug fix.

Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your submission and for identifying that simplifyPath returns [].

As you know we maintain high code quality standards on this project, so I'm not able to accept an ad-hoc fix. For all bugs, we expect to identify the root cause which will inform an appropriate solution. In rare cases we may determine that the effort to create a proper solution is not justified, but that is not the case here. I would encourage you to trace simplifyPath to determine what is causing it to return [].

[] is not a valid Path, but it's not covered by the type system so we have to manually trace where it's coming from.

Note should probably not be simplifying the path at all, since it needs to maintain the cursor as-is. Check out the documentation for the Context View and let me know if I can help clarify anything. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Note on context child: Error: [] is not a valid path
2 participants