Resolve selected block from cursor via helper and simplify keydown handling#453
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralized block-selection and keyword-block creation/focus logic: added validators and resolvers ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Handler as ShortcutHandler
participant BlocklyCommon as Blockly.common
participant Cursor as Cursor
participant Window as window
User->>Handler: press Ctrl+`]` or Ctrl+.
Handler->>BlocklyCommon: query getSelected()?
alt BlocklyCommon returns block
BlocklyCommon-->>Handler: selectedBlock
else no selection
Handler->>Cursor: getSelectedBlockFromCursor(cursor)
alt cursor.getSourceBlock() present
Cursor-->>Handler: sourceBlock
else cursor provides sourceBlock_ / getCurNode()
Cursor-->>Handler: derivedBlock
else no cursor block
Handler->>Window: check window.currentBlock
Window-->>Handler: currentBlock or null
end
end
Handler->>Handler: event.preventDefault()
alt block found
Handler->>Handler: focusKeywordField(selectedBlock) after delay
else no block
Handler->>Handler: createKeywordBlockAtViewportCenter("keyword" / "keyword_block")
Handler->>Handler: focusKeywordField(newBlock) after delay
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/blockhandling.js`:
- Around line 27-31: The current branch returns currentNode.getSourceBlock()
directly which skips the fallback to currentNode.sourceBlock_ when
getSourceBlock() yields null/undefined; update the logic around
currentNode.getSourceBlock() to call the method, store its result, and if that
result is nullish return currentNode.sourceBlock_ (or null) — i.e., mirror the
pattern used earlier so getSourceBlock() result is used only when
non-null/undefined.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
main/blockhandling.js (1)
35-39:⚠️ Potential issue | 🟡 MinorFallback to
sourceBlock_bypassed whengetSourceBlock()returns null.Lines 35–37 return directly from
asBlocklyBlock(currentNode.getSourceBlock())without checking if the result is truthy. IfgetSourceBlock()returnsnull, the fallback tosourceBlock_on line 39 is never reached. This is inconsistent with the pattern at lines 19–24 which stores the result and only returns when truthy.Proposed fix to mirror the earlier fallback pattern
if (typeof currentNode.getSourceBlock === "function") { - return asBlocklyBlock(currentNode.getSourceBlock()); + const nodeSourceBlock = asBlocklyBlock(currentNode.getSourceBlock()); + if (nodeSourceBlock) { + return nodeSourceBlock; + } } return asBlocklyBlock(currentNode.sourceBlock_);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/blockhandling.js` around lines 35 - 39, The current branch returns asBlocklyBlock(currentNode.getSourceBlock()) without checking for null; change it to call currentNode.getSourceBlock(), store the result in a variable, and only return asBlocklyBlock(result) when that result is truthy so the fallback to currentNode.sourceBlock_ (via asBlocklyBlock) is reached otherwise; refer to currentNode.getSourceBlock, asBlocklyBlock, and currentNode.sourceBlock_ when locating where to apply this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@main/blockhandling.js`:
- Around line 35-39: The current branch returns
asBlocklyBlock(currentNode.getSourceBlock()) without checking for null; change
it to call currentNode.getSourceBlock(), store the result in a variable, and
only return asBlocklyBlock(result) when that result is truthy so the fallback to
currentNode.sourceBlock_ (via asBlocklyBlock) is reached otherwise; refer to
currentNode.getSourceBlock, asBlocklyBlock, and currentNode.sourceBlock_ when
locating where to apply this change.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
main/blockhandling.js (1)
35-39:⚠️ Potential issue | 🟡 MinorFallback to
sourceBlock_is bypassed whengetSourceBlock()returns null.The pattern here differs from lines 19–24. When
currentNode.getSourceBlock()exists but returnsnull, the function returnsnullimmediately without attempting thesourceBlock_fallback on line 39.,
🔧 Suggested fix to mirror the pattern at lines 19-24
if (typeof currentNode.getSourceBlock === "function") { - return asBlocklyBlock(currentNode.getSourceBlock()); + const nodeSourceBlock = asBlocklyBlock(currentNode.getSourceBlock()); + if (nodeSourceBlock) { + return nodeSourceBlock; + } } return asBlocklyBlock(currentNode.sourceBlock_);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/blockhandling.js` around lines 35 - 39, The current branch returns immediately when currentNode.getSourceBlock() exists even if it returns null, bypassing the intended fallback to currentNode.sourceBlock_; change the logic in the block (using currentNode.getSourceBlock, asBlocklyBlock, and currentNode.sourceBlock_) to call getSourceBlock(), store its result in a local (e.g., src = currentNode.getSourceBlock()), and if src is truthy return asBlocklyBlock(src) otherwise return asBlocklyBlock(currentNode.sourceBlock_).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@main/blockhandling.js`:
- Around line 35-39: The current branch returns immediately when
currentNode.getSourceBlock() exists even if it returns null, bypassing the
intended fallback to currentNode.sourceBlock_; change the logic in the block
(using currentNode.getSourceBlock, asBlocklyBlock, and currentNode.sourceBlock_)
to call getSourceBlock(), store its result in a local (e.g., src =
currentNode.getSourceBlock()), and if src is truthy return asBlocklyBlock(src)
otherwise return asBlocklyBlock(currentNode.sourceBlock_).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
main/blockhandling.js (1)
35-39:⚠️ Potential issue | 🟠 MajorPreserve fallback when
currentNode.getSourceBlock()is nullish.At Line 36, returning
asBlocklyBlock(currentNode.getSourceBlock())directly can returnnulland skip the fallback at Line 39. This reintroduces the nullish path the helper is meant to avoid.Suggested fix
if (typeof currentNode.getSourceBlock === "function") { - return asBlocklyBlock(currentNode.getSourceBlock()); + const nodeSourceBlock = asBlocklyBlock(currentNode.getSourceBlock()); + if (nodeSourceBlock) { + return nodeSourceBlock; + } } return asBlocklyBlock(currentNode.sourceBlock_);#!/bin/bash # Verify the nullish-fallback path is currently bypassed. rg -n -C3 'currentNode\.getSourceBlock|sourceBlock_' main/blockhandling.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/blockhandling.js` around lines 35 - 39, The current return uses asBlocklyBlock(currentNode.getSourceBlock()) which can be nullish and skips the intended fallback; change the logic so you call currentNode.getSourceBlock() once, check for a non-null/non-undefined result and only then pass it to asBlocklyBlock, otherwise call asBlocklyBlock(currentNode.sourceBlock_); reference currentNode.getSourceBlock, asBlocklyBlock, and currentNode.sourceBlock_ when updating the conditional to preserve the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main/blockhandling.js`:
- Around line 68-70: The call to workspace.getCursor?.()?.setCurNode?.(block)
can throw because Blockly Cursor.setCurNode expects an IFocusableNode, not a raw
Block; wrap that invocation the same way as at line 197: guard the call and
catch any exception, e.g. check existence of workspace.getCursor and setCurNode,
then try setCurNode(block) inside a try/catch and in the catch call setCurNode
with a fallback focusable (for example block.getSvgRoot?.() or another UI node)
or simply swallow the error after logging; update the code around block.select()
to use this defensive try/catch pattern referencing workspace.getCursor,
setCurNode, and block.
---
Duplicate comments:
In `@main/blockhandling.js`:
- Around line 35-39: The current return uses
asBlocklyBlock(currentNode.getSourceBlock()) which can be nullish and skips the
intended fallback; change the logic so you call currentNode.getSourceBlock()
once, check for a non-null/non-undefined result and only then pass it to
asBlocklyBlock, otherwise call asBlocklyBlock(currentNode.sourceBlock_);
reference currentNode.getSourceBlock, asBlocklyBlock, and
currentNode.sourceBlock_ when updating the conditional to preserve the fallback.
| block.select(); | ||
| workspace.getCursor?.()?.setCurNode?.(block); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Blockly version used by this repository, what argument type does Cursor.setCurNode(...) accept (Block instance vs AST node)?
💡 Result:
Cursor.setCurNode(...) (and LineCursor.setCurNode(...)) takes an AST-style “focusable node”, i.e. an object implementing IFocusableNode (commonly a Blockly.ASTNode), not a raw Block instance. [1]
🏁 Script executed:
# First, locate the blockhandling.js file
find . -name "blockhandling.js" -type fRepository: flipcomputing/flock
Length of output: 86
🏁 Script executed:
# Read the relevant section around lines 68-70
sed -n '60,80p' ./main/blockhandling.jsRepository: flipcomputing/flock
Length of output: 595
🏁 Script executed:
# Check package.json or similar for Blockly version
cat package.json 2>/dev/null || cat package-lock.json 2>/dev/null | grep -A5 -B5 "blockly" | head -30Repository: flipcomputing/flock
Length of output: 2651
🏁 Script executed:
# Check for other usages of setCurNode in the codebase
rg "setCurNode" -A3 -B3Repository: flipcomputing/flock
Length of output: 50376
🏁 Script executed:
# Get more context around the other setCurNode usage with try-catch
sed -n '80,95p' ./main/blockhandling.jsRepository: flipcomputing/flock
Length of output: 449
🏁 Script executed:
# Find the full context of the try-catch block with setCurNode
sed -n '200,230p' ./main/blockhandling.jsRepository: flipcomputing/flock
Length of output: 1073
🏁 Script executed:
# Find where the setCurNode is wrapped with try-catch
grep -n "navCursor.setCurNode" ./main/blockhandling.jsRepository: flipcomputing/flock
Length of output: 113
🏁 Script executed:
# Get context around line 197
sed -n '190,205p' ./main/blockhandling.jsRepository: flipcomputing/flock
Length of output: 677
Wrap setCurNode with error handling for Blockly API compatibility.
At line 69, setCurNode is called with a block parameter, but Blockly's Cursor.setCurNode() expects an IFocusableNode (an AST node), not a raw Block instance. This will throw in Blockly 12.3.1. Wrap the call defensively—this pattern is already used at line 197 in this file.
Suggested fix
block.select();
- workspace.getCursor?.()?.setCurNode?.(block);
+ const cursor = workspace.getCursor?.();
+ try {
+ cursor?.setCurNode?.(block);
+ } catch {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| block.select(); | |
| workspace.getCursor?.()?.setCurNode?.(block); | |
| block.select(); | |
| const cursor = workspace.getCursor?.(); | |
| try { | |
| cursor?.setCurNode?.(block); | |
| } catch {} | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main/blockhandling.js` around lines 68 - 70, The call to
workspace.getCursor?.()?.setCurNode?.(block) can throw because Blockly
Cursor.setCurNode expects an IFocusableNode, not a raw Block; wrap that
invocation the same way as at line 197: guard the call and catch any exception,
e.g. check existence of workspace.getCursor and setCurNode, then try
setCurNode(block) inside a try/catch and in the catch call setCurNode with a
fallback focusable (for example block.getSvgRoot?.() or another UI node) or
simply swallow the error after logging; update the code around block.select() to
use this defensive try/catch pattern referencing workspace.getCursor,
setCurNode, and block.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
main/blockhandling.js (2)
35-39:⚠️ Potential issue | 🟠 MajorKeep the
sourceBlock_fallback whengetSourceBlock()returns nothing.Line 36 returns immediately, so a nullish
currentNode.getSourceBlock()still skips thecurrentNode.sourceBlock_fallback on Line 39. That puts this helper back into the same failure mode it was meant to hide.Minimal fix
if (typeof currentNode.getSourceBlock === "function") { - return asBlocklyBlock(currentNode.getSourceBlock()); + const sourceBlock = asBlocklyBlock(currentNode.getSourceBlock()); + if (sourceBlock) { + return sourceBlock; + } } return asBlocklyBlock(currentNode.sourceBlock_);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/blockhandling.js` around lines 35 - 39, The current early return uses currentNode.getSourceBlock() without handling a null/undefined result and thus skips the fallback to currentNode.sourceBlock_; change the logic in the helper so that if typeof currentNode.getSourceBlock === "function" you call it once, store its result (e.g., const sb = currentNode.getSourceBlock()), and if sb is not nullish return asBlocklyBlock(sb); otherwise fall through to return asBlocklyBlock(currentNode.sourceBlock_); keep references to currentNode.getSourceBlock, asBlocklyBlock, and currentNode.sourceBlock_ so reviewers can locate the change.
72-75:⚠️ Potential issue | 🔴 CriticalGuard
setCurNodeagainst raw-block failures.Line 75 passes
blockstraight intoCursor.setCurNode(...). That API has already been flagged as cursor-node based, and a throw here now breaks both Ctrl+. / Ctrl+] focus paths; the same pattern also reappears inblocks/blocks.js.Minimal fix
- workspace.getCursor?.()?.setCurNode?.(block); + const cursor = workspace.getCursor?.(); + try { + cursor?.setCurNode?.(block); + } catch {}In the Blockly version used by this repository, what parameter type does `Cursor.setCurNode(...)` accept, and is passing a `Block` / `BlockSvg` instance supported?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main/blockhandling.js` around lines 72 - 75, The call to workspace.getCursor()?.setCurNode?.(block) can throw when passed a raw Block/BlockSvg; wrap the setCurNode invocation in a safe guard so it only runs when the cursor accepts this node or swallow the error: check that workspace.getCursor() and its setCurNode method exist, then either validate the block type (e.g., ensure it is a cursor-compatible node or convert to the node the cursor expects) or call setCurNode inside a try/catch and silently skip/log errors to avoid breaking focus paths; update the same pattern wherever workspace.getCursor()?.setCurNode?.(...) appears (e.g., in main/blockhandling.js and blocks/blocks.js) referencing the workspace.getCursor and setCurNode symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocks/blocks.js`:
- Around line 1687-1717: applyBlockDefinition currently only restores primitive
fields and direct inputs, so object-valued variable fields, chained next blocks,
and root-level defaults from toolbox.js (used by keyword-created snippets and
material/ui defaults like ui_input.SIZE or material.TEXTURE_SET) are lost;
update applyBlockDefinition to (1) iterate definition.fields and when a field
value is an object call the appropriate setter (e.g., use block.setFieldValue or
block.setVariableFieldValue if present) to restore named-variable defaults, (2)
recursively restore definition.next chains by locating/creating block.next
connections and applying applyBlockDefinition to chained definitions, and (3)
ensure resolved root block code path also invokes applyBlockDefinition so
root-level defaults are applied; reference applyToolboxSettings,
block.loadExtraState, block.setInputsInline, block.setFieldValue, block.render,
and the definition.next structure when implementing these additions.
- Around line 1751-1757: The loop in applyToolboxSettings (blocks.js) currently
checks input.shadow before input.block, causing shadow to short-circuit and
preventing configured list blocks (e.g., change_color.COLOR) from being created;
update the logic in the for-loop that iterates inputs to prefer input.block when
both exist by calling connectToolboxBlock(newBlock, inputName, input.block,
false) first if input.block is truthy, otherwise fall back to input.shadow via
connectToolboxBlock(newBlock, inputName, input.shadow, true), keeping the same
variables (newBlock, inputName, inputs, connectToolboxBlock) and behavior
otherwise.
In `@toolbox.js`:
- Around line 3134-3136: The keyword-to-toolbox resolution is regressing to the
shared `type` (e.g., "lists_create_with", "start", "when_clicked", "forever")
and therefore recreates the generic block rather than the matched variant;
update the keyword handling so it returns the exact matched toolbox definition
object instead of looking up the first toolbox entry by `type`, or alternatively
assign a unique `type` for each variant (e.g., give "colourlist", "playerstart",
"clickbox", "move8", "skyworld" unique type names) so the existing lookup
returns the correct snippet; locate the resolution logic in toolbox.js where
`keyword` entries are converted to `{ type: "...", keyword: "...", extraState:
... }` and change it to preserve/return the original toolbox definition or set
distinct type identifiers for those entries.
---
Duplicate comments:
In `@main/blockhandling.js`:
- Around line 35-39: The current early return uses currentNode.getSourceBlock()
without handling a null/undefined result and thus skips the fallback to
currentNode.sourceBlock_; change the logic in the helper so that if typeof
currentNode.getSourceBlock === "function" you call it once, store its result
(e.g., const sb = currentNode.getSourceBlock()), and if sb is not nullish return
asBlocklyBlock(sb); otherwise fall through to return
asBlocklyBlock(currentNode.sourceBlock_); keep references to
currentNode.getSourceBlock, asBlocklyBlock, and currentNode.sourceBlock_ so
reviewers can locate the change.
- Around line 72-75: The call to workspace.getCursor()?.setCurNode?.(block) can
throw when passed a raw Block/BlockSvg; wrap the setCurNode invocation in a safe
guard so it only runs when the cursor accepts this node or swallow the error:
check that workspace.getCursor() and its setCurNode method exist, then either
validate the block type (e.g., ensure it is a cursor-compatible node or convert
to the node the cursor expects) or call setCurNode inside a try/catch and
silently skip/log errors to avoid breaking focus paths; update the same pattern
wherever workspace.getCursor()?.setCurNode?.(...) appears (e.g., in
main/blockhandling.js and blocks/blocks.js) referencing the workspace.getCursor
and setCurNode symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48c606d2-ca4d-4a2d-9960-4f6f09ee4f54
📒 Files selected for processing (4)
blocks/blocks.jsmain/blockhandling.jsmain/translation.jstoolbox.js
| type: "lists_create_with", | ||
| keyword: "colourlist", | ||
| extraState: { itemCount: 2 }, |
There was a problem hiding this comment.
These keywords currently map back to the wrong toolbox entries.
The keyword flow resolves the match down to a type, then reloads the first toolbox definition for that type. Because these new keywords reuse shared types like lists_create_with, start, when_clicked, and forever, typing colourlist, playerstart, clickbox, move8, or skyworld will recreate the generic block instead of the intended material/snippet variant. Return the matched toolbox definition itself, or give these entries unique types.
Also applies to: 3735-3737, 3823-3825, 4187-4189, 4655-4657
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@toolbox.js` around lines 3134 - 3136, The keyword-to-toolbox resolution is
regressing to the shared `type` (e.g., "lists_create_with", "start",
"when_clicked", "forever") and therefore recreates the generic block rather than
the matched variant; update the keyword handling so it returns the exact matched
toolbox definition object instead of looking up the first toolbox entry by
`type`, or alternatively assign a unique `type` for each variant (e.g., give
"colourlist", "playerstart", "clickbox", "move8", "skyworld" unique type names)
so the existing lookup returns the correct snippet; locate the resolution logic
in toolbox.js where `keyword` entries are converted to `{ type: "...", keyword:
"...", extraState: ... }` and change it to preserve/return the original toolbox
definition or set distinct type identifiers for those entries.
1 participant
Motivation
Description
getSelectedBlockFromCursor(cursor)which safely extracts a block from cursor by checkinggetSourceBlock,getCurNode,getSourceBlockon the node, and falling back tonode.sourceBlock_if present.keydownhandler with a single call togetSelectedBlockFromCursor(cursor) || window.currentBlock, simplifying the flow and removing duplicated checks.Testing
npm testand linters withnpm run lint, and both completed without failures.Codex Task
Summary by CodeRabbit
Bug Fixes
Improvements
New Features