fix: select My Blocks category by ID after closing procedure editor#526
fix: select My Blocks category by ID after closing procedure editor#526
Conversation
selectCategoryByName('myBlocks') passes the toolbox item ID where the
method expects a translated display name. It also skips scrolling the
flyout. Use getToolboxItemById + setSelectedItem instead, matching the
pattern already used in handleCategorySelected.
Fixes scratchfoundation/scratch-blocks#3542
There was a problem hiding this comment.
Pull request overview
Fixes category selection after closing the custom procedure editor by selecting the My Blocks toolbox category via its toolbox item ID (instead of treating the ID as a translated display name), and adds a unit test to prevent regressions.
Changes:
- Update
handleCustomProceduresCloseto select My Blocks viagetToolboxItemById('myBlocks')+setSelectedItem(...). - Add a unit test covering the translated-name mismatch scenario for My Blocks category selection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/scratch-gui/src/containers/blocks.jsx | Switches My Blocks selection to ID-based lookup + setSelectedItem after closing the procedure editor. |
| packages/scratch-gui/test/unit/containers/blocks.test.js | Adds a unit test ensuring My Blocks is selected even when its display name is translated. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const toolbox = { | ||
| selectedItem: null, | ||
| getToolboxItemById: jest.fn(id => (id === 'myBlocks' ? myBlocksItem : null)), | ||
| // Simulates real selectCategoryByName: looks up by display name, not ID | ||
| selectCategoryByName: jest.fn(name => { | ||
| toolbox.selectedItem = (name === 'Mis Bloques') ? myBlocksItem : null; | ||
| }), | ||
| setSelectedItem: jest.fn(item => { | ||
| toolbox.selectedItem = item; | ||
| }) | ||
| }; |
There was a problem hiding this comment.
This test only asserts the end state (selectedItem) and would still pass if the implementation switched to selectCategoryByName(<translated name>), which wouldn’t verify the intended fix (selecting by toolbox item ID / setSelectedItem path). Consider strengthening the test by asserting getToolboxItemById('myBlocks') and setSelectedItem(myBlocksItem) are called (and optionally that selectCategoryByName is not called), so the test fails if the implementation regresses to name-based selection.
Test Results 3 files - 1 69 suites - 887 10m 34s ⏱️ - 2m 26s Results for commit 78ee55a. ± Comparison against base commit 1074d9e. This pull request removes 1920 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Resolves
Proposed Changes
selectCategoryByName('myBlocks')passes the toolbox item ID, where the method expects a translated display name. It also skips scrolling the flyout. UsegetToolboxItemById+setSelectedIteminstead, matching the pattern already used inhandleCategorySelected.Test Coverage
Added a test to verify that the correct item is selected.
HOLD
Since this is not a release-blocking issue, let's wait to merge this until after the initial release of the new scratch-blocks code.