fix: non-nullable check for block variable and removed ! in layout_ #7536

rashmi29 · 2023-09-22T22:34:04Z

The basics

I validated my changes

The details

Resolves

Fixes #7523

Proposed Changes

Check whether block is undefined after setting const block = item.block.
continue if block doesn't exist.
Remove ! from after the remaining uses of block within the for loop.

Reason for Changes

Non nullable check was not present in code. That might break the code when variable has no value assigned.

Test Coverage

I did npm run and tested all the buttons before and after and checked the entire functionality after my change.

Documentation

No documentation required

Additional Information

google-cla · 2023-09-22T22:34:07Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

github-actions

Welcome! It looks like this is your first pull request in Blockly, so here are a couple of tips:

You can find tips about contributing to Blockly and how to
validate your changes on our
developer site.
All contributors must sign the Google Contributor License
Agreement (CLA). If the google-cla bot leaves a comment on this
PR, make sure you follow the instructions.
We use conventional commits
to make versioning the package easier. Make sure your commit
message is in the proper format
or learn how to fix it.
If any of the other checks on this PR fail, you can click on
them to learn why. It might be that your change caused a test
failure, or that you need to double-check the
style guide.

Thank you for opening this PR! A member of the Blockly team will review it soon.

rachel-fenichel · 2023-09-22T22:39:58Z

Thanks for grabbing this issue!

Next steps:

Run npm run format to fix some formatting issues and commit
Sign the CLA, following the instructions from the bot above

I'll keep an eye on this PR and approve workflow runs as needed.

rachel-fenichel · 2023-09-22T22:48:50Z

core/flyout_vertical.ts

@@ -233,29 +233,32 @@ export class VerticalFlyout extends Flyout {
    for (let i = 0, item; (item = contents[i]); i++) {
      if (item.type === 'block') {
        const block = item.block;
-        const allBlocks = block!.getDescendants(false);
+        if(block == undefined || block == null) {


This can be just if (block) {

rachanavishwanath · 2023-09-22T22:56:27Z

core/flyout_vertical.ts

@@ -233,29 +233,32 @@ export class VerticalFlyout extends Flyout {
    for (let i = 0, item; (item = contents[i]); i++) {
      if (item.type === 'block') {
        const block = item.block;
-        const allBlocks = block!.getDescendants(false);
+        if (block == undefined || block == null) {


please squash your commits

rachanavishwanath · 2023-09-22T22:59:04Z

core/flyout_vertical.ts

@@ -233,29 +233,32 @@ export class VerticalFlyout extends Flyout {
    for (let i = 0, item; (item = contents[i]); i++) {
      if (item.type === 'block') {
        const block = item.block;
-        const allBlocks = block!.getDescendants(false);
+        if (block == undefined || block == null) {


This check is redundant, block == null checks both null and undefined. We would both checks with strict equality i.e. block === undefined || block === null

In this case if (block) will do fine, because we aren't trying to distinguish between the two cases.

If I just add if (block) than it shows "'block' is possibly 'undefined'." error. So, I added condition if (block == null) as suggested by rachna.

rachel-fenichel · 2023-09-22T23:57:15Z

Hi @rashmi29,

I'm signing off for the weekend but I will take a look again on Monday. Thanks again for contributing, and have a great Grace Hopper if you're attending!

humera811 · 2023-09-24T20:32:46Z

nit: In the description section under Resolves you can add a link to the issue

  Resolves 
            Fixes #7523 : Check for undefined in vertical flyout layout function

humera811 · 2023-09-24T20:57:53Z

core/flyout_vertical.ts

+        if (block == null) {
+          continue;
+        }
+        const allBlocks = block.getDescendants(false);
        for (let j = 0, child; (child = allBlocks[j]); j++) {
          // Mark blocks as being inside a flyout.  This is used to detect and
          // prevent the closure of the flyout if the user right-clicks on such
          // a block.
          child.isInFlyout = true;
        }


Suggestion:

if (block) { const allBlocks = block.getDescendants(false); for (let j = 0, child; (child = allBlocks[j]); j++) { // Mark blocks as being inside a flyout. This is used to detect and // prevent the closure of the flyout if the user right-clicks on such // a block. child.isInFlyout = true; }

or we could just do

if (!block) continue; const allBlocks = block.getDescendants(false); for (let j = 0, child; (child = allBlocks[j]); j++) { // Mark blocks as being inside a flyout. This is used to detect and // prevent the closure of the flyout if the user right-clicks on such // a block. child.isInFlyout = true; }

maribethb

Looks great! Thank you so much for the PR!

approved by maribethb

rashmi29 and others added 4 commits September 22, 2023 14:38

Fix: 7523 - non-nullable check for block variable and removed !

89bc156

Merge branch 'google:osd' into osd

4589a17

Fix: non nullable check for block variable

a9a0d84

Merge branch 'osd' of https://github.com/rashmi29/blockly into osd

dfb6705

rashmi29 requested a review from a team as a code owner September 22, 2023 22:34

rashmi29 requested a review from maribethb September 22, 2023 22:34

github-actions bot assigned maribethb Sep 22, 2023

rachel-fenichel changed the title ~~Fix : non-nullable check for block variable and removed ! in layout_~~ fix: non-nullable check for block variable and removed ! in layout_ Sep 22, 2023

github-actions bot added the PR: fix Fixes a bug label Sep 22, 2023

github-actions bot reviewed Sep 22, 2023

View reviewed changes

rachel-fenichel assigned rachel-fenichel and unassigned maribethb Sep 22, 2023

rachel-fenichel requested review from rachel-fenichel and removed request for maribethb September 22, 2023 22:37

rachel-fenichel previously requested changes Sep 22, 2023

View reviewed changes

fix: fixed styling issue

0008925

rachanavishwanath reviewed Sep 22, 2023

View reviewed changes

rashmi29 and others added 2 commits September 22, 2023 16:27

Merge branch 'google:osd' into osd

5cc1ff6

fix: updated block variable nullable condition

ddee1ca

humera811 reviewed Sep 24, 2023

View reviewed changes

rashmi29 and others added 3 commits September 24, 2023 18:16

Merge branch 'google:osd' into osd

dce666c

Updated nullable condition for block variable

a6bcdc7

Merge branch 'osd' of https://github.com/rashmi29/blockly into osd

f95279a

maribethb self-requested a review September 25, 2023 17:46

maribethb self-assigned this Sep 25, 2023

github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Sep 25, 2023

maribethb approved these changes Sep 25, 2023

View reviewed changes

maribethb merged commit 19e9115 into google:osd Sep 25, 2023
9 checks passed

maribethb linked an issue Sep 27, 2023 that may be closed by this pull request

Check for undefined in vertical flyout layout function #7523

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: non-nullable check for block variable and removed ! in layout_ #7536

fix: non-nullable check for block variable and removed ! in layout_ #7536

rashmi29 commented Sep 22, 2023 •

edited by maribethb

Loading

google-cla bot commented Sep 22, 2023

github-actions bot left a comment

rachel-fenichel commented Sep 22, 2023

rachel-fenichel Sep 22, 2023

rachanavishwanath Sep 22, 2023

rachanavishwanath Sep 22, 2023

rachel-fenichel Sep 22, 2023

rashmi29 Sep 22, 2023

rachel-fenichel commented Sep 22, 2023

humera811 commented Sep 24, 2023 •

edited

Loading

humera811 Sep 24, 2023 •

edited

Loading

maribethb left a comment

fix: non-nullable check for block variable and removed ! in layout_ #7536

fix: non-nullable check for block variable and removed ! in layout_ #7536

Conversation

rashmi29 commented Sep 22, 2023 • edited by maribethb Loading

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Documentation

Additional Information

google-cla bot commented Sep 22, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

rachel-fenichel commented Sep 22, 2023

rachel-fenichel Sep 22, 2023

Choose a reason for hiding this comment

rachanavishwanath Sep 22, 2023

Choose a reason for hiding this comment

rachanavishwanath Sep 22, 2023

Choose a reason for hiding this comment

rachel-fenichel Sep 22, 2023

Choose a reason for hiding this comment

rashmi29 Sep 22, 2023

Choose a reason for hiding this comment

rachel-fenichel commented Sep 22, 2023

humera811 commented Sep 24, 2023 • edited Loading

humera811 Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

maribethb left a comment

Choose a reason for hiding this comment

rashmi29 commented Sep 22, 2023 •

edited by maribethb

Loading

humera811 commented Sep 24, 2023 •

edited

Loading

humera811 Sep 24, 2023 •

edited

Loading