[Editor][Typing] Fixing missing types in perseus-editor/src/components#3448
[Editor][Typing] Fixing missing types in perseus-editor/src/components#3448catandthemachines wants to merge 10 commits intomainfrom
Conversation
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +18 B (0%) Total Size: 495 kB
ℹ️ View Unchanged
|
|
|
||
| type Props = { | ||
| onDrop: (e: DragEvent) => void; | ||
| component?: any; |
There was a problem hiding this comment.
Removing this property as it appears none of components that reference DragTarget actually use this functionality. Might as well delete it.
There was a problem hiding this comment.
Yes! We do have some patterns where there's a feature to override/customize something that isn't actually used anymore. Thanks for simplifying!
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (9c22821) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3448If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3448If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3448 |
| if (pressedKey === "ArrowDown" && focusedElement.nextElementSibling) { | ||
| event.preventDefault(); | ||
| focusedElement.nextSibling.focus(); | ||
| (focusedElement.nextElementSibling as HTMLElement).focus(); |
There was a problem hiding this comment.
I don't like all this usage of as. Let me follow up and see if there's something I can change here.
| snapStep: [number, number]; | ||
| valid: boolean; | ||
| backgroundImage: any; | ||
| backgroundImage: PerseusImageBackground; |
There was a problem hiding this comment.
Question: is this always guaranteed? 🤔
…ds to components folder in perseus-editor.
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review once |
| import * as React from "react"; | ||
|
|
||
| type Props = { | ||
| onDrop: (e: DragEvent) => void; | ||
| component?: any; | ||
| shouldDragHighlight: (any) => boolean; | ||
| style?: any; | ||
| children?: any; | ||
| onDrop: (e: React.MouseEvent) => unknown; | ||
| shouldDragHighlight: (e: React.MouseEvent) => boolean; | ||
| style?: React.CSSProperties; | ||
| children?: React.ReactNode; | ||
| className?: string; | ||
| }; |
There was a problem hiding this comment.
🔴 The drag event handler types in drag-target.tsx are incorrectly set to React.MouseEvent instead of React.DragEvent<HTMLElement>. This caused a concrete downstream workaround in editor.tsx, where handleDrop must use a fragile "dataTransfer" in e runtime check to access e.dataTransfer — a property TypeScript cannot see on React.MouseEvent but which is always present at runtime on actual drag events.
Extended reasoning...
What the bug is
The PR changes the type of all drag-related event parameters in drag-target.tsx from the correct native DragEvent (before the PR) to React.MouseEvent (after the PR). The affected signatures are:
Props.onDrop: (e: React.MouseEvent) => unknownProps.shouldDragHighlight: (e: React.MouseEvent) => booleanhandleDrop(e: React.MouseEvent)handleDragOver(e: React.MouseEvent)handleDragEnter(e: React.MouseEvent)
The correct React type for these drag-specific events is React.DragEvent<HTMLElement>, which extends React.MouseEvent and adds the essential dataTransfer: DataTransfer property.
The specific code path that triggers it
DragTarget passes this.handleDrop directly to <View onDrop={...}>. At runtime, the browser fires a real DragEvent (which has dataTransfer). However, TypeScript only sees React.MouseEvent, which has no dataTransfer property. When editor.tsx consumes the onDrop prop typed as (e: React.MouseEvent) => unknown, it cannot access e.dataTransfer directly without a cast or runtime guard.
Why existing code does not prevent it
Because React.DragEvent<T> extends React.MouseEvent<T>, TypeScript structural typing means a (e: React.MouseEvent) => void handler is assignable where a React.DragEventHandler is expected (function contravariance). So no compile error surfaces at the call site, and runtime behavior is preserved because browsers correctly fire DragEvent objects regardless of what TypeScript thinks.
Impact and the resulting workaround
The incorrect type forced editor.tsx to add a defensive runtime guard:
handleDrop = (e: React.MouseEvent) => {
const dataTransfer =
"dataTransfer" in e
? (e.dataTransfer as DataTransfer | null)
: null;The "dataTransfer" in e check will always be true at runtime, but TypeScript cannot know this because the type is wrong. The subsequent cast as DataTransfer | null is also forced by the incorrect typing. With React.DragEvent<HTMLElement>, e.dataTransfer would be directly accessible with no guard or cast needed.
Step-by-step proof
- User drags an image file onto the editor textarea.
- Browser fires a native
DragEventon the<View>element. - React wraps it in a synthetic event;
DragTarget.handleDrop(e: React.MouseEvent)is called, but at runtimeeis actually aReact.DragEventwithe.dataTransferpopulated. DragTargetcallsthis.props.onDrop(e)typed as(e: React.MouseEvent) => unknown.editor.tsx'shandleDropreceivese. TypeScript seesReact.MouseEventwith nodataTransfer, so direct access would be a compile-time error.- The workaround
"dataTransfer" in ealways evaluates totrue, and the cast is applied — the image upload proceeds. - If the workaround were removed and
e.dataTransferaccessed directly, TypeScript would flag it as an error even though it works at runtime.
How to fix it
Change all affected types in drag-target.tsx from React.MouseEvent to React.DragEvent<HTMLElement>, and update editor.tsx's handleDrop signature to match, allowing e.dataTransfer to be accessed directly without the runtime guard or cast.
There was a problem hiding this comment.
This was on purpose to try and use WB View component over a div element. I want to double check this implementation in a ZND to confirm it works. But yes I don't care for the casting to a different element and afraid it could cause issues with dragging images. 😬
| }; | ||
|
|
||
| handleDrop: (e: DragEvent) => void = (e: DragEvent) => { | ||
| handleDrop = (e: React.MouseEvent) => { |
There was a problem hiding this comment.
The original DragEvent seemed to be correct though. Why did this need to change?
There was a problem hiding this comment.
I added this in the comment above, but I'm trying to avoid using a div element and use a WB's View, which does not expose a DragEvent. I want to double check if it works. If not I'll go back to the div element.
| type Props = { | ||
| onChange: ( | ||
| newProps: Record<string, unknown>, | ||
| callback?: () => unknown, |
There was a problem hiding this comment.
I was looking at something else the other day related to this and we might not even use this callback parameter anywhere anymore.
No action required.
|
|
||
| type Props = { | ||
| onDrop: (e: DragEvent) => void; | ||
| component?: any; |
There was a problem hiding this comment.
Yes! We do have some patterns where there's a feature to override/customize something that isn't actually used anymore. Thanks for simplifying!
2 participants
Summary:
Adding missing types to perseus-editor/src/components folder. This is important to minimize regressions and create a more stable editor experiences.
I also moved the associated stories for components back into the proper folder type so it now appears in our Storybook again.
Issue: LEMS-2748
Test plan:
Run
pnpm testAnd because this is old legacy code, will create a ZND and test this in our editor to double check no glaring type issues.
ZND PR: https://github.com/Khan/frontend/pull/9938