Conversation
Reviewer's GuideAdds collaborative Supabase-powered lobbies and drawing tools on the map, switches coordinates to game units (0–160), refactors interactions/rendering to support drawing/eraser/cursor display, and enhances the UI (icons, layout, script loading, mobile tweaks) plus a GitHub Pages workflow update with Supabase secret injection. Sequence diagram for collaborative drawing in a Supabase lobbysequenceDiagram
actor User
participant AppDraw
participant AppLobby
participant Supabase
participant OtherClient
participant MapRenderer
User->>AppDraw: setTool("pen")
User->>AppDraw: startStroke(px, py)
User->>AppDraw: continueStroke(px, py)*
User->>AppDraw: finishStroke()
AppDraw->>AppLobby: sendDrawing(tool, color, points, width, label)
AppLobby->>Supabase: insert into drawings
Supabase-->>AppLobby: postgres_changes INSERT drawings
AppLobby->>MapRenderer: onDrawing(stroke)
AppLobby-->>OtherClient: onDrawing callback
OtherClient->>MapRenderer: draw(view, MAP, ...)
File-Level Changes
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 6 security issues, 2 other issues, and left some high level feedback:
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
cursorCoords.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
info.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
row.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- UIPanels now calls renderMap() inside lobby-related handlers, but renderMap isn’t defined or stored in the module scope; make sure init() accepts and saves renderMap (and any other callbacks like saveState) so those calls don’t fail at runtime.
- The Supabase URL/KEY injection in the GitHub Pages workflow assumes js/features/lobby.js always exists and should be modified; consider guarding the sed calls (or the lobby feature itself) so forks or environments without Supabase credentials don’t hit runtime errors when AppLobby methods are invoked.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- UIPanels now calls renderMap() inside lobby-related handlers, but renderMap isn’t defined or stored in the module scope; make sure init() accepts and saves renderMap (and any other callbacks like saveState) so those calls don’t fail at runtime.
- The Supabase URL/KEY injection in the GitHub Pages workflow assumes js/features/lobby.js always exists and should be modified; consider guarding the sed calls (or the lobby feature itself) so forks or environments without Supabase credentials don’t hit runtime errors when AppLobby methods are invoked.
## Individual Comments
### Comment 1
<location path="js/ui/panels.js" line_range="103-112" />
<code_context>
+ // Lobby кнопки
</code_context>
<issue_to_address>
**issue (bug_risk):** The `bind` function references `renderMap` and other globals without them being wired into the UIPanels module.
In `bind`, you invoke `renderMap()` and use `AppPoints`, `AppWeapons`, `UIInputs`, `UIResults`, `AppShare`, and `AppLobby`. Inside the UIPanels closure, only `storage` and locals (`theme`, `showTowers`, `onChange`) are defined. `renderMap` is now passed into `UIPanels.init` from `index.js`, but `init` still appears to accept only a callback and does not persist `renderMap` or `saveState` for use in `bind`, so `renderMap` will be undefined when the lobby buttons are used.
Please update `UIPanels.init(opts)` to accept the options object, store `opts.renderMap` (and `opts.saveState` if needed) in module-level variables, and use those inside `bind`. If `renderMap` is meant to be global instead, attach it to `window` and reference `window.renderMap` explicitly.
</issue_to_address>
### Comment 2
<location path="js/features/draw.js" line_range="110-19" />
<code_context>
+ function cancelStroke() { isDrawing = false; currentStroke = null; }
+
+ // ─── Ластик: удаляет ТОЛЬКО свои штрихи (линии, линейки, метки) ───
+ function eraseAt(sx, sy, view) {
+ const connected = window.AppLobby && window.AppLobby.isConnected();
+ const strokes = connected ? window.AppLobby.getDrawings() : localDrawings;
+ const myId = connected ? window.AppLobby.getMyId() : 'local';
+ const R = 12; // радиус захвата в px
+ let removed = false;
+
+ for (let i = strokes.length - 1; i >= 0; i--) {
+ const st = strokes[i];
+ if (st.playerId !== myId) continue; // стираем только своё
+
+ let hit = false;
+ for (const p of st.points || []) {
+ const s = utils.worldToScreen(
+ utils.percentToMeters(p.x, mapSize),
+ utils.percentToMeters(p.y, mapSize),
+ view
+ );
+ if (Math.hypot(s.x - sx, s.y - sy) <= R) { hit = true; break; }
+ }
+
+ if (hit) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Eraser hit detection ignores stroke width, which can make thick lines harder to erase consistently.
The eraser radius is fixed at `R = 12` px and doesn’t account for `stroke.width`, so thicker strokes occupy more visual area than the erase threshold, making erasing feel inconsistent.
Consider scaling `R` by `stroke.width` (e.g. `const R = 8 + 2 * (st.width || 1);`) or computing distance to the stroke segments instead of just sampled points to keep erasing behaviour consistent across different line thicknesses.
Suggested implementation:
```javascript
const strokes = connected ? window.AppLobby.getDrawings() : localDrawings;
const myId = connected ? window.AppLobby.getMyId() : 'local';
const baseEraseRadius = 8; // базовый радиус захвата в px, масштабируется по толщине линии
let removed = false;
```
```javascript
const st = strokes[i];
if (st.playerId !== myId) continue; // стираем только своё
// масштабируем радиус стирания в зависимости от толщины штриха,
// чтобы толстые линии стирались так же предсказуемо, как тонкие
const strokeRadius = baseEraseRadius + 2 * (st.width || 1);
let hit = false;
for (const p of st.points || []) {
const s = utils.worldToScreen(
utils.percentToMeters(p.x, mapSize),
utils.percentToMeters(p.y, mapSize),
view
);
if (Math.hypot(s.x - sx, s.y - sy) <= strokeRadius) { hit = true; break; }
}
```
</issue_to_address>
### Comment 3
<location path="js/map/interactions.js" line_range="138-140" />
<code_context>
cursorCoords.innerHTML =
`<span>x${utils.gameCoord(wpt.x)}</span>` +
`<span>y${utils.gameCoord(wpt.y)}</span>`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="js/map/interactions.js" line_range="138-140" />
<code_context>
cursorCoords.innerHTML =
`<span>x${utils.gameCoord(wpt.x)}</span>` +
`<span>y${utils.gameCoord(wpt.y)}</span>`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `cursorCoords.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 5
<location path="js/ui/panels.js" line_range="59" />
<code_context>
info.innerHTML = `<span class="lobby-code">Код: <b>${code}</b></span>`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 6
<location path="js/ui/panels.js" line_range="59" />
<code_context>
info.innerHTML = `<span class="lobby-code">Код: <b>${code}</b></span>`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `info.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 7
<location path="js/ui/panels.js" line_range="68-72" />
<code_context>
row.innerHTML = `
<span class="player-color" style="background:${p.color}"></span>
<span class="player-name">${p.name} ${isMe ? '(вы)' : ''}</span>
${!isMe ? `<input type="checkbox" ${!isHidden ? 'checked' : ''} data-pid="${p.playerId}">` : ''}
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 8
<location path="js/ui/panels.js" line_range="68-72" />
<code_context>
row.innerHTML = `
<span class="player-color" style="background:${p.color}"></span>
<span class="player-name">${p.name} ${isMe ? '(вы)' : ''}</span>
${!isMe ? `<input type="checkbox" ${!isHidden ? 'checked' : ''} data-pid="${p.playerId}">` : ''}
`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `row.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+103
to
+112
| // Lobby кнопки | ||
| document.getElementById('createLobbyBtn').addEventListener('click', async () => { | ||
| const code = await AppLobby.create(AppPoints.getA(), AppPoints.getB(), AppWeapons.get()); | ||
| if (code) { | ||
| AppShare.showToast(`Лобби создано: ${code}`, 'success'); | ||
| renderLobbyPlayers(); | ||
| } | ||
| }); | ||
|
|
||
| document.getElementById('joinLobbyBtn').addEventListener('click', () => { |
There was a problem hiding this comment.
issue (bug_risk): The bind function references renderMap and other globals without them being wired into the UIPanels module.
In bind, you invoke renderMap() and use AppPoints, AppWeapons, UIInputs, UIResults, AppShare, and AppLobby. Inside the UIPanels closure, only storage and locals (theme, showTowers, onChange) are defined. renderMap is now passed into UIPanels.init from index.js, but init still appears to accept only a callback and does not persist renderMap or saveState for use in bind, so renderMap will be undefined when the lobby buttons are used.
Please update UIPanels.init(opts) to accept the options object, store opts.renderMap (and opts.saveState if needed) in module-level variables, and use those inside bind. If renderMap is meant to be global instead, attach it to window and reference window.renderMap explicitly.
| document.querySelectorAll('.draw-tool').forEach(btn => { | ||
| btn.classList.toggle('active', btn.dataset.tool === tool); | ||
| }); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Eraser hit detection ignores stroke width, which can make thick lines harder to erase consistently.
The eraser radius is fixed at R = 12 px and doesn’t account for stroke.width, so thicker strokes occupy more visual area than the erase threshold, making erasing feel inconsistent.
Consider scaling R by stroke.width (e.g. const R = 8 + 2 * (st.width || 1);) or computing distance to the stroke segments instead of just sampled points to keep erasing behaviour consistent across different line thicknesses.
Suggested implementation:
const strokes = connected ? window.AppLobby.getDrawings() : localDrawings;
const myId = connected ? window.AppLobby.getMyId() : 'local';
const baseEraseRadius = 8; // базовый радиус захвата в px, масштабируется по толщине линии
let removed = false; const st = strokes[i];
if (st.playerId !== myId) continue; // стираем только своё
// масштабируем радиус стирания в зависимости от толщины штриха,
// чтобы толстые линии стирались так же предсказуемо, как тонкие
const strokeRadius = baseEraseRadius + 2 * (st.width || 1);
let hit = false;
for (const p of st.points || []) {
const s = utils.worldToScreen(
utils.percentToMeters(p.x, mapSize),
utils.percentToMeters(p.y, mapSize),
view
);
if (Math.hypot(s.x - sx, s.y - sy) <= strokeRadius) { hit = true; break; }
}
Comment on lines
+138
to
+140
| cursorCoords.innerHTML = | ||
| `<span>x${utils.gameCoord(wpt.x)}</span>` + | ||
| `<span>y${utils.gameCoord(wpt.y)}</span>`; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
Comment on lines
+138
to
+140
| cursorCoords.innerHTML = | ||
| `<span>x${utils.gameCoord(wpt.x)}</span>` + | ||
| `<span>y${utils.gameCoord(wpt.y)}</span>`; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a cursorCoords.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| const players = window.AppLobby.getPlayers(); | ||
| const myId = window.AppLobby.getMyId(); | ||
|
|
||
| info.innerHTML = `<span class="lobby-code">Код: <b>${code}</b></span>`; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| const players = window.AppLobby.getPlayers(); | ||
| const myId = window.AppLobby.getMyId(); | ||
|
|
||
| info.innerHTML = `<span class="lobby-code">Код: <b>${code}</b></span>`; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a info.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
Comment on lines
+68
to
+72
| row.innerHTML = ` | ||
| <span class="player-color" style="background:${p.color}"></span> | ||
| <span class="player-name">${p.name} ${isMe ? '(вы)' : ''}</span> | ||
| ${!isMe ? `<input type="checkbox" ${!isHidden ? 'checked' : ''} data-pid="${p.playerId}">` : ''} | ||
| `; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
Comment on lines
+68
to
+72
| row.innerHTML = ` | ||
| <span class="player-color" style="background:${p.color}"></span> | ||
| <span class="player-name">${p.name} ${isMe ? '(вы)' : ''}</span> | ||
| ${!isMe ? `<input type="checkbox" ${!isHidden ? 'checked' : ''} data-pid="${p.playerId}">` : ''} | ||
| `; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a row.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add collaborative lobbies and shared map annotation tools while modernizing coordinate input, map interaction, and interface controls.
New Features:
Enhancements:
CI:
Deployment:
Documentation:
Chores: