feat multiselect#408
Open
K1mmyn wants to merge 15 commits into
Open
Conversation
…rds implementation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements multi-component selection across the canvas editor: selection is now an array of IDs, pointer handlers support group selection/drag/resize, component APIs accept ID arrays, history records batched ChangeRecords with a unified undo/redo, clipboard/context/keyboard/topbar/text UI updated to the new model. ChangesMulti-Selection Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
K1mmyn
requested review from
del-ereno,
harbassan and
justinedagreat
and removed request for
harbassan and
justinedagreat
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/features/authoring/scene/operations/text.ts (1)
15-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNo guard for empty
idarray before accessingid[0].If
idis an empty array,id[0]returnsundefined, andgetComponentProp(undefined, "document")will fail. This applies to all text operations that accessid[0].🛡️ Proposed fix to add early return
export const insertChar = modify( (id: string[], cursor: ModelCursor, char: string) => { + if (!id.length) return cursor; const doc = getComponentProp(id[0], "document") as ModelDocument;Apply similar guards to
deleteChar,deleteSelection,createBlock,applySelectionStyle, andmergeDocs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/scene/operations/text.ts` around lines 15 - 43, The functions that call getComponentProp(id[0], "document") (notably insertChar) lack a guard for an empty id array; add an early-return guard at the start of insertChar (and mirror in deleteChar, deleteSelection, createBlock, applySelectionStyle, mergeDocs) that checks if id.length === 0 or !id[0] and returns a no-op or error result before calling getComponentProp, ensuring subsequent code never passes undefined to getComponentProp.frontend/src/features/authoring/handlers/keyboard/keyboard.ts (1)
27-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
selected.length > 0checks; array truthiness is incorrect here.On Line 27 and Lines 36–46, empty selection (
[]) still passes current guards, so duplicate/reorder/component ops can fire with no targets.Suggested fix
export function handleGlobal(e: KeyboardEvent) { const mode = useEditorStore.getState().mode; const { selected } = useEditorStore.getState(); + const hasSelection = selected.length > 0; @@ - else if (selected) handleComponentOperations(e, selected); + else if (hasSelection) handleComponentOperations(e, selected); } @@ function handleCtrlOperations(e: KeyboardEvent) { const { selected, setSelected } = useEditorStore.getState(); + const hasSelection = selected.length > 0; @@ - else if (e.key === "d" && selected) { + else if (e.key === "d" && hasSelection) { @@ - } else if (e.key === "ArrowUp" && selected) { + } else if (e.key === "ArrowUp" && hasSelection) { @@ - } else if (e.key === "ArrowDown" && selected) { + } else if (e.key === "ArrowDown" && hasSelection) {Also applies to: 36-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/handlers/keyboard/keyboard.ts` at line 27, The guards that check the current selection use the array's truthiness, which treats an empty array as truthy and allows ops to run with no targets; update all checks that use `selected` as a boolean (including the branch that calls `handleComponentOperations(e, selected)` and the duplicate/reorder handlers) to explicitly verify `selected.length > 0` before proceeding so operations only run when there are actual selected items.frontend/src/features/authoring/handlers/keyboard/clipboard.ts (1)
24-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix selection guards for array-based
selected.On Lines 24, 33, and 50,
selectedis an array; truthiness checks treat[]as selected. This can incorrectly intercept clipboard events and route into text-mode paste without targets.Suggested fix
export function copy(e: ClipboardEvent) { const { selected } = useEditorStore.getState(); - if (!selected) return; + if (selected.length === 0) return; @@ export function cut(e: ClipboardEvent) { const { selected, setSelected } = useEditorStore.getState(); - if (!selected) return; + if (selected.length === 0) return; @@ - if (selected && mode.includes("text")) { + if (selected.length > 0 && mode.includes("text")) {Also applies to: 33-33, 50-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/handlers/keyboard/clipboard.ts` at line 24, The guard uses truthiness on the array variable `selected`, but an empty array is truthy; update all clipboard handler guards that check `selected` (the occurrences around the current checks at lines ~24, ~33, ~50) to explicitly test for an empty array and null/undefined, e.g. replace `if (!selected) return;` with `if (!selected || selected.length === 0) return;` (or `if (!selected?.length) return;`) so empty selection arrays don't incorrectly route clipboard events into text-mode paste.
🧹 Nitpick comments (5)
frontend/src/features/authoring/util.ts (1)
38-40: ⚡ Quick winAvoid duplicating component-wise multiply logic.
scaleVecByScaleVecduplicatesmultiplybehavior, which increases API/maintenance surface without adding behavior. Prefer delegating tomultiply(or replacing call sites withmultiply) to keep one source of truth.♻️ Proposed refactor
export function scaleVecByScaleVec(v: Vec2, scale: Vec2) { - return { x: v.x * scale.x, y: v.y * scale.y }; + return multiply(v, scale); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/util.ts` around lines 38 - 40, The function scaleVecByScaleVec duplicates the component-wise multiply logic already provided by multiply for Vec2; update scaleVecByScaleVec to delegate to multiply (e.g., return multiply(v, scale)) or remove scaleVecByScaleVec and update call sites to use multiply directly so there is a single source of truth for component-wise multiplication of Vec2 values; locate the symbol scaleVecByScaleVec and the existing multiply(Vec2, Vec2) implementation and refactor accordingly.frontend/src/features/authoring/scene/operations/component.ts (1)
191-197: ⚖️ Poor tradeoffMultiple history entries created for single z-index operation.
Each
modifyComponentPropcall triggers themodifywrapper, creating a separate history entry. For a multi-select bring-forward operation, this results in multiple undo steps instead of one atomic batch operation.Consider either:
- Wrapping the entire operation in a single history batch
- Using the raw mutation function without the
modifywrapper, then manually recording history once🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/scene/operations/component.ts` around lines 191 - 197, The loop over sortedComponents calls modifyComponentProp for each item, which invokes the modify history wrapper and creates multiple undo entries; change this to perform all zIndex changes in a single history transaction by either wrapping the loop in the existing batch history API (use the modify/history batch function that currently wraps single ops) or call the underlying raw mutation (bypass modifyComponentProp's modify wrapper) to set each component's zIndex and then record one history entry once after the loop; locate sortedComponents and modifyComponentProp in component.ts and implement one atomic history operation instead of multiple modifyComponentProp calls.frontend/src/features/authoring/canvas/handles/RotationHandle.tsx (1)
3-3: 💤 Low valueRemove unused import.
useVisualSceneis imported but never used in this file.♻️ Remove unused import
import { getBoxCenter, rotate } from "../../util"; import useEditorStore from "../../stores/editor"; -import useVisualScene from "../../stores/visual"; import { getSelectedComponentBounds } from "../../handlers/pointer/pointer";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/canvas/handles/RotationHandle.tsx` at line 3, The file imports useVisualScene but never uses it; remove the unused import statement for useVisualScene from RotationHandle.tsx to clean up imports and avoid linter errors—locate the import line that reads "import useVisualScene from \"../../stores/visual\";" and delete it (ensure no other code in RotationHandle references useVisualScene afterwards).frontend/src/features/authoring/handlers/pointer/pointer.ts (1)
86-106: 💤 Low valueRemove debug comments before merging.
These TODO/DONE tracking comments and the
TEMPORARYmarker should be removed before merge. They document implementation progress but add noise to the production codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/handlers/pointer/pointer.ts` around lines 86 - 106, Remove the temporary debug block and all TODO/DONE/TEMPORARY comments: delete the conditional that forcibly adds id to selected (the if (!selected.includes(id)) { setSelected([...selected, id]); } block) and remove the surrounding progress-tracking comments (lines with TODO/DONE/! TEMPORARY and related notes) so the pointer handler only contains production logic; ensure no behavior relied on that debug branch remains and run tests to confirm no regressions in selection behavior.frontend/src/features/authoring/handlers/pointer/resize.ts (1)
127-148: ⚡ Quick winDead code: speech-triangle handling is unreachable.
Since
typeis hardcoded to"box"on line 127, the conditionsif (type === "speech")on lines 137 and 144 will never evaluate to true. Either remove the dead code or restore the dynamic type determination.♻️ Option A: Remove dead speech code
let type = "box"; let verts = modifyVerts(bounds.verts, coords, position); if (!coords.includes(2)) { // none of these apply to the speech triangle // shift modifier if (fixed && !coords.includes(BOX_CENTER_VALUE)) { lockAspect(bounds.verts, verts, coords); } - if (type === "speech") { - verts[2] = getNewTail(bounds.verts, verts, coords); - } - // alt modifier if (anchorCenter) { const mirrored = mirror(verts, center, coords); - if (type === "speech") { - mirrored[2] = getNewTail(verts, mirrored, inverse(coords)); - } verts = mirrored; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/handlers/pointer/resize.ts` around lines 127 - 148, The code sets type = "box" making the speech-specific branches (if (type === "speech")) in resize logic unreachable; either remove those dead branches (calls to getNewTail and the mirrored tail handling) or restore dynamic type detection by computing type from the shape/metadata used by modifyVerts/bounds (e.g., derive type = getPointerType(bounds) or use an existing property on bounds/verts) so the getNewTail(bounds.verts, verts, coords) and mirrored[2] = getNewTail(verts, mirrored, inverse(coords)) calls run only when type actually equals "speech"; update usages of type, verts, and related helpers (modifyVerts, lockAspect, mirror, inverse) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/features/authoring/AuthoringToolPage.jsx`:
- Around line 52-53: The code currently calls replace(...) with the result of
scenes.find(...) or scenes[0], which can be undefined and crash replace() when
structuredClone is used; update the logic in AuthoringToolPage.jsx to first
locate the scene with scenes.find((s) => s._id === activeScene) and check that
it is truthy (and that scenes.length > 0) before calling replace; if the find
returns undefined or scenes is empty, either skip calling replace or pass a safe
fallback (e.g., a newly constructed default scene object) so replace() never
receives undefined; ensure you update the branch that uses replace, referencing
the same replace, scenes, and activeScene identifiers.
In `@frontend/src/features/authoring/canvas/handles/ConstrainedHandle.tsx`:
- Around line 10-15: The component uses useEditorStore.getState() to read mode
inside ResizeHandle, which doesn't subscribe to changes so pointerEvents won't
update; replace that call with the selector hook (e.g. const mode =
useEditorStore(s => s.mode)) so ResizeHandle subscribes to mode updates and will
re-render when mode changes; ensure the pointerEvents prop still reads the new
mode variable and remove the getState() usage (leave other calls like
getSelectedComponentBounds() unchanged unless they also need subscription).
In `@frontend/src/features/authoring/canvas/handles/RotationHandle.tsx`:
- Around line 6-11: The component reads mode via useEditorStore.getState() which
breaks reactivity; replace that with the selector hook so RotationHandle
subscribes to mode changes (e.g., useEditorStore(state => state.mode)) and use
the selected-mode variable wherever mode is referenced (not
useEditorStore.getState()); ensure pointerEvents prop and any other
mode-dependent usages in RotationHandle use this subscribed mode so the
component re-renders when mode changes.
In `@frontend/src/features/authoring/canvas/Overlay.tsx`:
- Around line 45-55: The Overlay component currently reads state with
useEditorStore.getState() and useVisualScene.getState(), which bypasses
subscriptions and prevents re-renders; replace those calls with selector hooks
like const { selected, mode, createType, mutationBounds } = useEditorStore(state
=> ({ selected: state.selected, mode: state.mode, createType: state.createType,
mutationBounds: state.mutationBounds })) and const components =
useVisualScene(state => state.components) (use shallow or individual selectors
if needed), then compute primaryComponent from components and selected
(primaryComponent = selected.length ? components[selected[0]] : null) and keep
getSelectedComponentBounds() usage if it already derives from subscribed state
so the Overlay will re-render on selection/mode/mutationBounds/components
changes.
In `@frontend/src/features/authoring/handlers/keyboard/clipboard.ts`:
- Around line 55-56: Wrap the JSON.parse calls that parse the external clipboard
data (the code that assigns parsed/parsed = JSON.parse(components) and then
items = Array.isArray(parsed) ? parsed : [parsed]) in a try/catch so malformed
clipboard content doesn't throw and abort the handler; on parse failure
log/debug the error (or silently ignore) and set items to an empty array or
return early to fail gracefully. Apply the same defensive try/catch pattern to
the other parse site that uses JSON.parse in this file (the second instance
around the parsed/items assignment).
- Line 52: The code uses the non-null assertion selection.start! to set let
cursor = selection.start which can throw if selection.start is undefined; update
the paste/merge handler in clipboard.ts to guard against a missing cursor by
checking selection.start (or selection) before using it, e.g., if
(!selection?.start) return early or compute a safe fallback position, and
replace selection.start! with the guarded value when assigning to the cursor
variable.
In `@frontend/src/features/authoring/handlers/pointer/pointer.ts`:
- Around line 242-245: findComponentRotation currently dereferences
components[id] and will throw if the component is missing; update the function
to check useVisualScene.getState().components[id] (and its .bounds) for
null/undefined and return a safe default rotation (e.g., 0) when missing instead
of accessing .bounds.rotation directly; keep the function name
findComponentRotation and the components retrieval via
useVisualScene.getState().components so callers remain unchanged.
- Around line 293-303: handleTextSelection reads selected[0] without ensuring
the selected array has elements, which can throw when state is inconsistent; add
an early-return guard at the top of handleTextSelection that checks selected
exists and selected.length > 0 (or !selected[0]) before using it, so the
function returns immediately if nothing is selected; keep the rest of the logic
(calling useVisualScene.getState(), parseHit, getRelativePosition, and
setVisualSelection) unchanged and only run when the guard passes.
- Around line 204-215: getNewResizePosition currently mutates the input verts
array (e.g., currentBounds.verts) which corrupts component state; clone the
verts input at the start (e.g., create a new array via slice/map/spread) and
perform all modifications against that cloned array (update verts[i] on the
clone) so the original currentBounds.verts is never mutated, then return the
cloned array; refer to the getNewResizePosition function and the verts variable
when making this change.
In `@frontend/src/features/authoring/scene/history.ts`:
- Around line 78-79: handleUndoRedo currently calls switchToScene(sceneID) but
proceeds even if the switch fails, risking applying batches to the wrong scene;
update handleUndoRedo (and the similar block around lines 108-114) to check the
result of switchToScene(sceneID) and immediately abort/return (no further
mutation or applyBatch calls) when the switch fails or returns falsy, ensuring
no batch/commit is applied unless the active scene was successfully switched.
- Around line 80-87: The snapshot code in ids.map assumes getComponent(id)
returns a component and calls structuredClone(comp) unguarded; update the logic
that builds validChanges (the ids.map / HistoryObject creation) to check the
result of getComponent(id) and skip or omit entries where comp is null/undefined
so you never store an invalid state for the inverse history. Locate the
validChanges creation (ids.map), call getComponent(id) into a local variable
(comp), and only push/return a HistoryObject with sceneId, id, and
structuredClone(comp) when comp is truthy (otherwise filter it out), ensuring
TypeScript type narrowing for HistoryObject and preventing later restore/render
crashes.
In `@frontend/src/features/authoring/scene/operations/component.ts`:
- Around line 124-129: duplicateComponent calls
structuredClone(getComponent(id)) without guarding for getComponent returning
undefined, which causes parseComponent(undefined) to throw; update
duplicateComponent to check the result of getComponent(id) before
cloning/processing (e.g., const comp = getComponent(id); if (!comp) return
null/skip or throw) and only call structuredClone and parseComponent when comp
is defined, referencing duplicateComponent, getComponent, structuredClone, and
parseComponent to locate the change.
In `@frontend/src/features/authoring/scene/operations/modifiers.ts`:
- Around line 26-55: The wrapper modify() is recording ChangeRecords for ids
that don't exist (prevState undefined) which breaks undo; before building
previousStates and later visual updates, filter the incoming ids via
getComponent to only include existing components. Keep calling fn(...args) with
the original args, but compute previousStates = validIds.map(id => ({ id,
prevState: structuredClone(getComponent(id)) })) where validIds = ids.filter(id
=> getComponent(id) != null), pass that to updateHistory(previousStates), and
iterate validIds (not ids) when calling
useVisualScene.getState().updateComponent(buildVisualComponent(...)). Ensure you
reference modify, getComponent, ChangeRecord, updateHistory,
useVisualScene.getState().updateComponent, and buildVisualComponent when
applying the change.
In `@frontend/src/features/authoring/stores/editor.ts`:
- Around line 74-76: The code dereferences getComponent(mainTarget).type without
guarding for null; change to first fetch const comp = getComponent(mainTarget)
and check comp is non-null and comp.type === "textbox" before calling
getStyleForSelection(mainTarget, selection), so avoid calling .type on null and
skip the style call when comp is null.
In `@frontend/src/features/authoring/text/cursor.ts`:
- Around line 131-137: In syncVisualCursor(), remove the visualSelection.start
requirement so the function only requires a single selected component and a
model selection start: delete the "!editorState.visualSelection.start" check
from the first guard (and remove the redundant second guard if you keep the
selection.start check there) and ensure the code uses
"editorState.selection.start" as the required gate along with
"editorState.selected.length === 1"; reference symbols: syncVisualCursor,
editorState.selected, editorState.selection.start,
editorState.visualSelection.start.
In `@frontend/src/features/authoring/topbar/Topbar.tsx`:
- Around line 35-37: The render guard currently checks "selected &&" which
treats an empty array as truthy and causes property controls/actions to render
incorrectly; change those guards to use the computed hasSelection (const
hasSelection = selected && selected.length > 0) instead of "selected &&"
wherever you reference selection (notably the block using component =
hasSelection ? getComponent(selected[0]) : null and the render blocks around the
previous "selected &&" checks between the property/actions area), so that you
only access selected[0], call getComponent, and render property/action controls
when hasSelection is true.
---
Outside diff comments:
In `@frontend/src/features/authoring/handlers/keyboard/clipboard.ts`:
- Line 24: The guard uses truthiness on the array variable `selected`, but an
empty array is truthy; update all clipboard handler guards that check `selected`
(the occurrences around the current checks at lines ~24, ~33, ~50) to explicitly
test for an empty array and null/undefined, e.g. replace `if (!selected)
return;` with `if (!selected || selected.length === 0) return;` (or `if
(!selected?.length) return;`) so empty selection arrays don't incorrectly route
clipboard events into text-mode paste.
In `@frontend/src/features/authoring/handlers/keyboard/keyboard.ts`:
- Line 27: The guards that check the current selection use the array's
truthiness, which treats an empty array as truthy and allows ops to run with no
targets; update all checks that use `selected` as a boolean (including the
branch that calls `handleComponentOperations(e, selected)` and the
duplicate/reorder handlers) to explicitly verify `selected.length > 0` before
proceeding so operations only run when there are actual selected items.
In `@frontend/src/features/authoring/scene/operations/text.ts`:
- Around line 15-43: The functions that call getComponentProp(id[0], "document")
(notably insertChar) lack a guard for an empty id array; add an early-return
guard at the start of insertChar (and mirror in deleteChar, deleteSelection,
createBlock, applySelectionStyle, mergeDocs) that checks if id.length === 0 or
!id[0] and returns a no-op or error result before calling getComponentProp,
ensuring subsequent code never passes undefined to getComponentProp.
---
Nitpick comments:
In `@frontend/src/features/authoring/canvas/handles/RotationHandle.tsx`:
- Line 3: The file imports useVisualScene but never uses it; remove the unused
import statement for useVisualScene from RotationHandle.tsx to clean up imports
and avoid linter errors—locate the import line that reads "import useVisualScene
from \"../../stores/visual\";" and delete it (ensure no other code in
RotationHandle references useVisualScene afterwards).
In `@frontend/src/features/authoring/handlers/pointer/pointer.ts`:
- Around line 86-106: Remove the temporary debug block and all
TODO/DONE/TEMPORARY comments: delete the conditional that forcibly adds id to
selected (the if (!selected.includes(id)) { setSelected([...selected, id]); }
block) and remove the surrounding progress-tracking comments (lines with
TODO/DONE/! TEMPORARY and related notes) so the pointer handler only contains
production logic; ensure no behavior relied on that debug branch remains and run
tests to confirm no regressions in selection behavior.
In `@frontend/src/features/authoring/handlers/pointer/resize.ts`:
- Around line 127-148: The code sets type = "box" making the speech-specific
branches (if (type === "speech")) in resize logic unreachable; either remove
those dead branches (calls to getNewTail and the mirrored tail handling) or
restore dynamic type detection by computing type from the shape/metadata used by
modifyVerts/bounds (e.g., derive type = getPointerType(bounds) or use an
existing property on bounds/verts) so the getNewTail(bounds.verts, verts,
coords) and mirrored[2] = getNewTail(verts, mirrored, inverse(coords)) calls run
only when type actually equals "speech"; update usages of type, verts, and
related helpers (modifyVerts, lockAspect, mirror, inverse) accordingly.
In `@frontend/src/features/authoring/scene/operations/component.ts`:
- Around line 191-197: The loop over sortedComponents calls modifyComponentProp
for each item, which invokes the modify history wrapper and creates multiple
undo entries; change this to perform all zIndex changes in a single history
transaction by either wrapping the loop in the existing batch history API (use
the modify/history batch function that currently wraps single ops) or call the
underlying raw mutation (bypass modifyComponentProp's modify wrapper) to set
each component's zIndex and then record one history entry once after the loop;
locate sortedComponents and modifyComponentProp in component.ts and implement
one atomic history operation instead of multiple modifyComponentProp calls.
In `@frontend/src/features/authoring/util.ts`:
- Around line 38-40: The function scaleVecByScaleVec duplicates the
component-wise multiply logic already provided by multiply for Vec2; update
scaleVecByScaleVec to delegate to multiply (e.g., return multiply(v, scale)) or
remove scaleVecByScaleVec and update call sites to use multiply directly so
there is a single source of truth for component-wise multiplication of Vec2
values; locate the symbol scaleVecByScaleVec and the existing multiply(Vec2,
Vec2) implementation and refactor accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b49b47c-a3e3-4c91-96df-a357e09dc35b
📒 Files selected for processing (25)
frontend/src/features/authoring/AuthoringToolPage.jsxfrontend/src/features/authoring/canvas/Overlay.tsxfrontend/src/features/authoring/canvas/handles/ConstrainedHandle.tsxfrontend/src/features/authoring/canvas/handles/RotationHandle.tsxfrontend/src/features/authoring/handlers/keyboard/clipboard.tsfrontend/src/features/authoring/handlers/keyboard/keyboard.tsfrontend/src/features/authoring/handlers/keyboard/text.tsfrontend/src/features/authoring/handlers/pointer/ComponentContext.tsxfrontend/src/features/authoring/handlers/pointer/context.tsfrontend/src/features/authoring/handlers/pointer/create.tsfrontend/src/features/authoring/handlers/pointer/pointer.tsfrontend/src/features/authoring/handlers/pointer/resize.tsfrontend/src/features/authoring/scene/history.tsfrontend/src/features/authoring/scene/operations/component.tsfrontend/src/features/authoring/scene/operations/modifiers.tsfrontend/src/features/authoring/scene/operations/text.tsfrontend/src/features/authoring/scene/scene.tsfrontend/src/features/authoring/stores/editor.tsfrontend/src/features/authoring/text/Cursor.tsxfrontend/src/features/authoring/text/Highlight.tsxfrontend/src/features/authoring/text/Text.tsxfrontend/src/features/authoring/text/cursor.tsfrontend/src/features/authoring/topbar/ShapeSection.tsxfrontend/src/features/authoring/topbar/Topbar.tsxfrontend/src/features/authoring/util.ts
Contributor
Author
|
code rabbit comments should be fixed now |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/authoring/scene/operations/modifiers.ts (1)
60-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
removeshould guard against missing components likemodifydoes.If
getComponent(id)returnsundefined,structuredClone(comp)storesundefinedasprevState, violating theChangeRecordtype (Component | null). During undo, theisDelete = batch[0].state === nullcheck will befalseforundefined, causing the batch to take the restore path instead of delete, corrupting state.🔧 Proposed fix to filter valid components
export function remove(ids: string[], history = true) { - const previousStates: ChangeRecord[] = ids.map((id) => { - const comp = getComponent(id); - return { - id, - prevState: structuredClone(comp), - }; - }); + const previousStates: ChangeRecord[] = ids + .map((id) => { + const comp = getComponent(id); + if (!comp) return null; + return { + id, + prevState: structuredClone(comp), + }; + }) + .filter((record): record is ChangeRecord => record !== null); + + const validIds = previousStates.map((r) => r.id); - ids.forEach((id) => { + validIds.forEach((id) => { delete getScene().components[id]; useVisualScene.getState().deleteComponent(id); }); - if (history) updateHistory(previousStates); + if (history && previousStates.length) updateHistory(previousStates); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/features/authoring/scene/operations/modifiers.ts` around lines 60 - 67, The remove function currently stores undefined prevState when getComponent(id) returns undefined; change remove so it mirrors modify by filtering out IDs with no component: for each id call getComponent(id), skip any undefined results, and only push ChangeRecord entries with id and prevState: structuredClone(comp) for existing components; ensure the later arrays/operations (previousStates, any batch creation) use this filtered list so no ChangeRecord.prevState is undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/features/authoring/scene/operations/modifiers.ts`:
- Around line 60-67: The remove function currently stores undefined prevState
when getComponent(id) returns undefined; change remove so it mirrors modify by
filtering out IDs with no component: for each id call getComponent(id), skip any
undefined results, and only push ChangeRecord entries with id and prevState:
structuredClone(comp) for existing components; ensure the later
arrays/operations (previousStates, any batch creation) use this filtered list so
no ChangeRecord.prevState is undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f813c1e8-fa26-4013-b950-2611cd90ae7c
📒 Files selected for processing (12)
frontend/src/features/authoring/AuthoringToolPage.jsxfrontend/src/features/authoring/canvas/Overlay.tsxfrontend/src/features/authoring/canvas/handles/ConstrainedHandle.tsxfrontend/src/features/authoring/canvas/handles/RotationHandle.tsxfrontend/src/features/authoring/handlers/keyboard/clipboard.tsfrontend/src/features/authoring/handlers/pointer/pointer.tsfrontend/src/features/authoring/scene/history.tsfrontend/src/features/authoring/scene/operations/component.tsfrontend/src/features/authoring/scene/operations/modifiers.tsfrontend/src/features/authoring/stores/editor.tsfrontend/src/features/authoring/text/cursor.tsfrontend/src/features/authoring/topbar/Topbar.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/features/authoring/canvas/Overlay.tsx
- frontend/src/features/authoring/scene/operations/component.ts
- frontend/src/features/authoring/topbar/Topbar.tsx
- frontend/src/features/authoring/handlers/pointer/pointer.ts
- frontend/src/features/authoring/handlers/keyboard/clipboard.ts
harbassan
requested changes
Jun 5, 2026
harbassan
left a comment
harbassan
left a comment
Contributor
There was a problem hiding this comment.
placeholder for the minor things we talked about at the meeting, which was function and variable naming, some unnecessary rotation modifications, and the location / potentially unnecessary complexity of some of the logic / use of helpers.
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
2 participants
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.
Issue
// * DONE Text Selection Broken
// * DONE Clipboard text selection
// * DONE Front back implementation
// * DONE Resize
// * DONE Fix copy and paste
// * DONE Object creation
// * DONE fix object mutationbounds visual
// * DONE fix delete
// * DONE Fix Undo Redo
// * DONE Fix Copy Paste
Maybe a few more
Next Cycle
// ! TODO MultiSelect rotation
// ! TODO Text mode needs undo redo support
// ! TODO Paste undo redo bug where undo only undos one at a time
// ! Clicking Duplicate and dragging cause unexpected behaviour
Typescript type to add?
// ! ERROR npm i --save-dev @types/uuid for another type
Solution
the correct ones, we can discuss all the changes in the meeting
Risk
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes