fix: correct typo preventing window_update and layout_update#1151
fix: correct typo preventing window_update and layout_update#1151nightcityblade wants to merge 3 commits intofossasia:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFixes a typo in ApiProvider’s update flag logic so that window and layout incremental updates are correctly detected and passed to the respective message handlers. Sequence diagram for corrected window_update and layout_update handlingsequenceDiagram
participant Client
participant ApiProvider
participant apiHandlers
participant WindowHandler
participant LayoutHandler
Client->>ApiProvider: postMessage cmd (command = window_update)
ApiProvider->>ApiProvider: switch on cmd.command
ApiProvider->>apiHandlers: onWindowMessage({ cmd: cmd, update: cmd.command == window_update })
apiHandlers->>WindowHandler: handleWindowMessage(cmd, update = true)
Client->>ApiProvider: postMessage cmd (command = layout_update, data)
ApiProvider->>ApiProvider: switch on cmd.command
ApiProvider->>apiHandlers: onLayoutMessage({ cmd: cmd.data, update: cmd.command == layout_update })
apiHandlers->>LayoutHandler: handleLayoutMessage(cmd.data, update = true)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="js/api/ApiProvider.js" line_range="117" />
<code_context>
apiHandlers.current.onWindowMessage({
cmd: cmd,
- update: cmd.commmand == 'window_update',
+ update: cmd.command == 'window_update',
});
break;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use strict equality (`===`) instead of loose equality (`==`) for the command comparison.
This avoids type-coercion edge cases if `cmd.command` is ever not a string, reducing the risk of subtle comparison bugs.
</issue_to_address>
### Comment 2
<location path="js/api/ApiProvider.js" line_range="130" />
<code_context>
apiHandlers.current.onLayoutMessage({
cmd: cmd.data,
- update: cmd.commmand == 'layout_update',
+ update: cmd.command == 'layout_update',
});
break;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Prefer strict equality (`===`) over loose equality (`==`) for the layout command check.
This will avoid type-coercion issues and align with common JavaScript style conventions.
Suggested implementation:
```javascript
cmd: cmd,
update: cmd.command === 'window_update',
});
break;
```
```javascript
case 'reload':
case 'layout_update':
apiHandlers.current.onLayoutMessage({
cmd: cmd.data,
update: cmd.command === 'layout_update',
});
break;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Good point from Sourcery! Updated both comparisons to use strict equality ( |
|
Thanks for the contribution! A process note. We have automatic Copilot PR reviews enabled on this repository. These reviews are only triggered if the contributor has GitHub Copilot enabled and an active license on their own account. Please enable Copilot in your GitHub settings if you have access. In many regions, free licenses are available through educational institutions or developer programs. Enabling Copilot helps us speed up the auto review process and reduces manual review overhead for the core team. |
There was a problem hiding this comment.
Pull request overview
Fixes a typo in the WebSocket message handler so incremental updates (window_update, layout_update) can be detected and applied by the frontend, addressing #1126.
Changes:
- Correct
cmd.commmand→cmd.commandwhen computing theupdateflag forwindow_update. - Correct
cmd.commmand→cmd.commandwhen computing theupdateflag forlayout_update.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -127,7 +127,7 @@ const ApiProvider = ({ children }) => { | |||
| case 'layout_update': | |||
| apiHandlers.current.onLayoutMessage({ | |||
| cmd: cmd.data, | |||
There was a problem hiding this comment.
onLayoutMessage in js/main.js is defined as ({ data, update }) => { ... }, but this provider calls it with { cmd: cmd.data, update: ... }. With this PR, update becomes true for layout_update, so parseLayoutsFromServer(data) will receive undefined and crash when reading layoutJSON.length. Pass the payload under the data key (or align the handler signature) to avoid runtime errors and actually apply layout updates.
| cmd: cmd.data, | |
| data: cmd.data, |
|
Thanks for the note. I understand the repository preference, but I do not have Copilot enabled on this account, so I cannot trigger the automatic Copilot review from my side. The code change itself is already updated to address the earlier review feedback, so if there is any additional manual feedback on the patch I’m happy to address it promptly. |
|
@nightcityblade |
Fixes #1126
The update flag in
ApiProvider.jswas computed usingcmd.commmand(3 m's) instead ofcmd.command, causingwindow_updateandlayout_updateincremental updates to never be applied.Changes:
cmd.commmand→cmd.command(window_update)cmd.commmand→cmd.command(layout_update)Summary by Sourcery
Fix incremental update handling for window and layout messages in the API provider.
Bug Fixes: