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.

Sign up for GitHub

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

Jump to bottom

fix: Don't crash when hovering hotbar menu index #7707

Merged
merged 13 commits into from
May 15, 2023
Merged

fix: Don't crash when hovering hotbar menu index #7707

merged 13 commits into from
May 15, 2023

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented May 9, 2023

  • Remove all uses of React.ReactNode as it is unsafe, replace them with usages of SafeReactNode which doesn't contain the '{}' type

closes #7703

@Nokel81 Nokel81 added bug Something isn't working area/hotbar labels May 9, 2023
@Nokel81 Nokel81 added this to the 6.5.0 milestone May 9, 2023
@Nokel81 Nokel81 requested a review from a team as a code owner May 9, 2023 19:51
@Nokel81 Nokel81 requested review from jansav and ixrock and removed request for a team May 9, 2023 19:51
@jansav
Copy link
Contributor

jansav commented May 10, 2023

I don't understand. Why is ReactNode not safe?

jansav
jansav reviewed May 10, 2023
View reviewed changes
packages/core/src/renderer/components/layout/main-layout.tsx Outdated
footer?: React.ReactNode;
children?: React.ReactNode | React.ReactNode[];
footer?: SafeReactNode;
children?: SingleOrMany<SafeReactNode>;
Copy link
Contributor

Choose a reason for hiding this comment

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

React.ReactNode is enough

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No it is not, but SafeReactNode (or StrictReactNode)

jansav
jansav requested changes May 10, 2023
View reviewed changes
Copy link
Contributor

@jansav jansav left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  1. We shouldn't introduce new type for ReactNode which is somehow magically safer than the original. If the motivation is to prevent developer from forgetting to add .get() to observable, then we have bigger issues... This would mean that developer has never tried the change because there's no way that it would ever work properly. Also unit tests would catch it.
  2. SingleOrMany is unnecessary because ReactNode accepts array of react nodes by default.
  3. Usage of React.Children.toArray is unnecessary because {children} would be enough.
  4. There's only one change in implementation and that doesn't have anything to do with hotbar, so how could it solve something for hotbar?
  5. Unit tests for the fix?

@Iku-turso
Copy link
Contributor

Iku-turso commented May 10, 2023 •
edited
Loading

It would seem to me that here the medicine of introducing custom types for a very specific purpose is worse than the sickness of forgetting to use an observable in any proper way. This human error should be abundantly apparent through unit- and even manual-testing... And if it is not that yet, addin unit tests to prove proper use would be far better time investment.

I propose adding said unit tests, doing the following actual fix, and calling it done.

-         {hotbar?.name}
+         {hotbar?.name.get()}

@Nokel81
Copy link
Collaborator Author

Nokel81 commented May 10, 2023

I propose also adding unit tests, but keeping the safer types as additional safety.

Iku-turso
Iku-turso reviewed May 11, 2023
View reviewed changes
packages/utility-features/utilities/src/isReactNode.ts
};
};

export type StrictReactNode =
Copy link
Contributor

Choose a reason for hiding this comment

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

It would make sense for react itself to expose this typing. Weird stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah..., might not be needed in react 18 but since we are in react 17 still that might be the problem.

Iku-turso
Iku-turso reviewed May 11, 2023
View reviewed changes
packages/utility-features/utilities/src/isReactNode.ts
| Iterable<StrictReactNode>
| boolean
| null
| undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if react 18 now permits rendering undefined, should we internally advocate that? WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably a good idea to remove, will try it.

Iku-turso

This comment was marked as outdated.

Sign in to view

Iku-turso
Iku-turso previously requested changes May 11, 2023
View reviewed changes
Copy link
Contributor

@Iku-turso Iku-turso left a comment

Choose a reason for hiding this comment

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

Ok with unit tests.

jansav
jansav previously approved these changes May 11, 2023
View reviewed changes
@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Nokel81 added 7 commits May 12, 2023 14:14
@Nokel81
fix: Don't crash when hovering hotbar menu index
2a8b2af 
- Remove all uses of React.ReactNode as it is unsafe, replace them with
  usages of SafeReactNode which doesn't contain the '{}' type

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Add unit tests to cover the hovering bug
98c39b8 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Update snapshots due to adding new data-testid
7b34672 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Remove unnecessary use of SingleOrMany<SafeReactNode>
7bc8a66 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Change name from SafeReactNode to StrictReactNode
78085d2 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Add documenting ref to StrictReactNode type
cc9c246 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Remove unnecessary use of toSafeReactChildrenArray
0cc7303 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
Nokel81 added 5 commits May 12, 2023 14:14
@Nokel81
chore: Fix spelling mistake in CRD type
65376e6 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Fix typo in CRD view
2d428ba 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Remove incorrect function documentation
8cd829e 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
fix: Fix StrictReactNode not accepting multiple children
2d46ff4 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81
chore: Add some behavioural tests for custom columns for custom resou…
7a65887 
…rces

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 force-pushed the fix/tooltip-crash branch from 0a63c75 to 7a65887 Compare May 12, 2023 18:14
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@Nokel81 Nokel81 dismissed Iku-turso’s stale review May 12, 2023 18:15

There are now unit tests

@Nokel81
chore: Update snapshot after rebase with master
996c16f 
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 merged commit 1a817d3 into master May 15, 2023
@Nokel81 Nokel81 deleted the fix/tooltip-crash branch May 15, 2023 15:56
@k8slens-bot k8slens-bot mentioned this pull request May 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabriel-mirantis gabriel-mirantis gabriel-mirantis approved these changes

@jansav jansav jansav left review comments

@Iku-turso Iku-turso Iku-turso left review comments

@ixrock ixrock Awaiting requested review from ixrock ixrock is a code owner automatically assigned from lensapp/lens-ide

Assignees
No one assigned
Labels
area/hotbar bug Something isn't working
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.

Crash when hovering or changing hot bars
4 participants
@Nokel81 @jansav @Iku-turso @gabriel-mirantis