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

#7735: adds very basic null checks to prevent NPE #7754

Merged

Conversation

grahamlangford
Copy link
Collaborator

@grahamlangford grahamlangford commented Feb 28, 2024

What does this PR do?

Discussion

  • Tried adding the file to tsconfig.strictNullChecks.json, but that threw 500+ errors (not surprising since it imports from editorSelectors)

Checklist

@grahamlangford grahamlangford linked an issue Feb 28, 2024 that may be closed by this pull request
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 72.14%. Comparing base (0ee8f0a) to head (aa0f4bc).
Report is 1 commits behind head on main.

Files Patch % Lines
...ab/editorNodeConfigPanel/EditorNodeConfigPanel.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7754   +/-   ##
=======================================
  Coverage   72.13%   72.14%           
=======================================
  Files        1269     1269           
  Lines       39765    39768    +3     
  Branches     7375     7378    +3     
=======================================
+ Hits        28685    28689    +4     
+ Misses      11080    11079    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +40 to +42
blockConfig,
} = useSelector(selectActiveNodeInfo) ?? {};
const { comments } = blockConfig ?? {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think this can be simplified to:

    blockConfig: { comments = {} },
  } = useSelector(selectActiveNodeInfo) ?? {};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it would have to be:

  const {
    blockId: brickId,
    path: brickFieldName,
    blockConfig: { comments },
  } = useSelector(selectActiveNodeInfo) ?? { blockConfig: {} };

Which I find harder to read.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right, the actual right way to right this is:

  const {
    blockId: brickId,
    path: brickFieldName,
    blockConfig: { comments} = {},
  } = activeNodeInfo ?? {};

e: but in the above case comments would still be undefined, so your suggestion works better

Copy link

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@grahamlangford grahamlangford merged commit 1e058db into main Feb 28, 2024
25 checks passed
@grahamlangford grahamlangford deleted the 7735-page-editor-npe-destructure-property-blockid branch February 28, 2024 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page Editor NPE (destructure property blockId)
3 participants