Draft
Conversation
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 59 files
Confidence score: 3/5
- There is concrete regression risk in
packages/editor/src/ui/inspector/sections/devtools.tsx: selecting the Template item can throw onJSON.parse, which can break the devtools panel in normal use. - Input/state handling issues in
packages/editor/src/ui/inspector/components/use-drag-to-change.tsandpackages/editor/src/ui/inspector/sections/fallback.tsxcan leave stale drag UI state or dispatch fallback updates with mismatched keys duringvariableIdchanges. - Several medium-severity value-parsing/unit issues (
packages/editor/src/ui/inspector/components/use-numeric-input.ts,packages/editor/src/ui/inspector/inspector/utils/resolve-theme-defaults.ts,packages/editor/src/ui/inspector/inspector/utils/parse-attributes.ts) may silently alter edited CSS values, so this is not fully low-risk yet. - Pay close attention to
packages/editor/src/ui/inspector/sections/devtools.tsx,packages/editor/src/ui/inspector/components/use-drag-to-change.ts, andpackages/editor/src/ui/inspector/sections/fallback.tsx- they carry the highest user-facing breakage risk and state consistency concerns.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/editor/src/ui/inspector/components/use-numeric-input.ts">
<violation number="1" location="packages/editor/src/ui/inspector/components/use-numeric-input.ts:106">
P2: `Number(displayValue) || fallbackValue` treats 0 as empty, so ArrowUp/Down will jump from 0 to the fallback value. Use an explicit empty/NaN check so 0 stays 0.</violation>
</file>
<file name="packages/editor/src/ui/inspector/components/border-radius-picker.tsx">
<violation number="1" location="packages/editor/src/ui/inspector/components/border-radius-picker.tsx:256">
P2: Use `parseFloat` instead of `parseInt` so fractional border-radius values are preserved.</violation>
</file>
<file name="packages/editor/src/ui/inspector/components/border-picker.tsx">
<violation number="1" location="packages/editor/src/ui/inspector/components/border-picker.tsx:73">
P2: `expanded` is derived from `styleObject` only on mount, so changes to `styleObject` won’t update the UI mode. Sync this state when `styleObject` changes or derive it directly to keep the inspector consistent when switching elements.</violation>
</file>
<file name="packages/editor/src/ui/inspector/sections/devtools.tsx">
<violation number="1" location="packages/editor/src/ui/inspector/sections/devtools.tsx:29">
P2: Guard the "empty" option before calling JSON.parse; otherwise selecting the Template item throws and breaks the devtools panel.</violation>
</file>
<file name="packages/editor/src/ui/inspector/sections/attributes.tsx">
<violation number="1" location="packages/editor/src/ui/inspector/sections/attributes.tsx:27">
P2: The Label uses htmlFor={input.prop} but none of the rendered controls set id={input.prop}, so the label isn’t associated with the input. Add matching ids (and update NumberInput to accept/pass one) so label click/AT works.</violation>
</file>
<file name="packages/editor/src/ui/inspector/USAGE.md">
<violation number="1" location="packages/editor/src/ui/inspector/USAGE.md:63">
P3: Close the JSX comment with `}` so the breadcrumb example is valid JSX.</violation>
</file>
<file name="packages/editor/src/ui/inspector/components/use-drag-to-change.ts">
<violation number="1" location="packages/editor/src/ui/inspector/components/use-drag-to-change.ts:68">
P2: Handle pointer cancellation as well as pointer up so drag state and global cursor/user-select styles are always reset.</violation>
</file>
<file name="packages/editor/src/ui/inspector/inspector/utils/resolve-theme-defaults.ts">
<violation number="1" location="packages/editor/src/ui/inspector/inspector/utils/resolve-theme-defaults.ts:51">
P2: Percentage values are returned even when the target unit is `px`, so percent-based padding/border styles will be misinterpreted as pixels. Return an empty string (or otherwise skip) when targetUnit is `px`.</violation>
</file>
<file name="packages/editor/src/ui/inspector/inspector/utils/parse-attributes.ts">
<violation number="1" location="packages/editor/src/ui/inspector/inspector/utils/parse-attributes.ts:106">
P2: Defaulting unitless numeric CSS values to "px" will rewrite unitless styles (like line-height/opacity) as pixel values when the user edits them. Preserve the absence of a unit so edits round-trip correctly.</violation>
</file>
<file name="packages/editor/src/ui/inspector/sections/fallback.tsx">
<violation number="1" location="packages/editor/src/ui/inspector/sections/fallback.tsx:64">
P2: The ref-update `useEffect` runs before the cleanup effect, so on `variableId` changes the cleanup dispatch can use the *new* variableKey/displayKey while comparing against the previous fallback. This can send fallback updates to the wrong variable. Move the ref update below the cleanup effect (or otherwise ensure cleanup reads the previous variable context).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (e.key === 'ArrowUp' || e.key === 'ArrowDown') { | ||
| e.preventDefault(); | ||
| const step = e.shiftKey ? 10 : 1; | ||
| const current = Number(displayValue) || fallbackValue || 0; |
Contributor
There was a problem hiding this comment.
P2: Number(displayValue) || fallbackValue treats 0 as empty, so ArrowUp/Down will jump from 0 to the fallback value. Use an explicit empty/NaN check so 0 stays 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/components/use-numeric-input.ts, line 106:
<comment>`Number(displayValue) || fallbackValue` treats 0 as empty, so ArrowUp/Down will jump from 0 to the fallback value. Use an explicit empty/NaN check so 0 stays 0.</comment>
<file context>
@@ -0,0 +1,119 @@
+ if (e.key === 'ArrowUp' || e.key === 'ArrowDown') {
+ e.preventDefault();
+ const step = e.shiftKey ? 10 : 1;
+ const current = Number(displayValue) || fallbackValue || 0;
+ const next = Math.max(
+ min ?? -Infinity,
</file context>
|
|
||
| const [topLeft, topRight, bottomRight, bottomLeft] = value | ||
| .split(' ') | ||
| .map((v) => parseInt(v, 10)); |
Contributor
There was a problem hiding this comment.
P2: Use parseFloat instead of parseInt so fractional border-radius values are preserved.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/components/border-radius-picker.tsx, line 256:
<comment>Use `parseFloat` instead of `parseInt` so fractional border-radius values are preserved.</comment>
<file context>
@@ -0,0 +1,301 @@
+
+ const [topLeft, topRight, bottomRight, bottomLeft] = value
+ .split(' ')
+ .map((v) => parseInt(v, 10));
+
+ return {
</file context>
| onChange, | ||
| presetColors, | ||
| }: BorderPickerProps) { | ||
| const [expanded, setExpanded] = React.useState( |
Contributor
There was a problem hiding this comment.
P2: expanded is derived from styleObject only on mount, so changes to styleObject won’t update the UI mode. Sync this state when styleObject changes or derive it directly to keep the inspector consistent when switching elements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/components/border-picker.tsx, line 73:
<comment>`expanded` is derived from `styleObject` only on mount, so changes to `styleObject` won’t update the UI mode. Sync this state when `styleObject` changes or derive it directly to keep the inspector consistent when switching elements.</comment>
<file context>
@@ -0,0 +1,370 @@
+ onChange,
+ presetColors,
+}: BorderPickerProps) {
+ const [expanded, setExpanded] = React.useState(
+ () => !allSidesEqual(styleObject),
+ );
</file context>
| // Defer to avoid flushSync during React render cycle | ||
| queueMicrotask(() => { | ||
| editor.commands.clearContent(); | ||
| editor.commands.setContent(JSON.parse(newValue)); |
Contributor
There was a problem hiding this comment.
P2: Guard the "empty" option before calling JSON.parse; otherwise selecting the Template item throws and breaks the devtools panel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/sections/devtools.tsx, line 29:
<comment>Guard the "empty" option before calling JSON.parse; otherwise selecting the Template item throws and breaks the devtools panel.</comment>
<file context>
@@ -0,0 +1,49 @@
+ // Defer to avoid flushSync during React render cycle
+ queueMicrotask(() => {
+ editor.commands.clearContent();
+ editor.commands.setContent(JSON.parse(newValue));
+ });
+ }}
</file context>
| <Section title="Attributes"> | ||
| {inputs.map((input) => ( | ||
| <PropRow key={input.prop}> | ||
| <Label title={input.label} className="truncate" htmlFor={input.prop}> |
Contributor
There was a problem hiding this comment.
P2: The Label uses htmlFor={input.prop} but none of the rendered controls set id={input.prop}, so the label isn’t associated with the input. Add matching ids (and update NumberInput to accept/pass one) so label click/AT works.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/sections/attributes.tsx, line 27:
<comment>The Label uses htmlFor={input.prop} but none of the rendered controls set id={input.prop}, so the label isn’t associated with the input. Add matching ids (and update NumberInput to accept/pass one) so label click/AT works.</comment>
<file context>
@@ -0,0 +1,94 @@
+ <Section title="Attributes">
+ {inputs.map((input) => (
+ <PropRow key={input.prop}>
+ <Label title={input.label} className="truncate" htmlFor={input.prop}>
+ {input.label}
+ </Label>
</file context>
| dragProps: { | ||
| onPointerDown, | ||
| onPointerMove, | ||
| onPointerUp, |
Contributor
There was a problem hiding this comment.
P2: Handle pointer cancellation as well as pointer up so drag state and global cursor/user-select styles are always reset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/components/use-drag-to-change.ts, line 68:
<comment>Handle pointer cancellation as well as pointer up so drag state and global cursor/user-select styles are always reset.</comment>
<file context>
@@ -0,0 +1,72 @@
+ dragProps: {
+ onPointerDown,
+ onPointerMove,
+ onPointerUp,
+ style: { cursor: 'ew-resize' } as React.CSSProperties,
+ },
</file context>
| return Math.round(parsed.numeric * bodyFontSize); | ||
| } | ||
|
|
||
| if (parsed.unit === '%') { |
Contributor
There was a problem hiding this comment.
P2: Percentage values are returned even when the target unit is px, so percent-based padding/border styles will be misinterpreted as pixels. Return an empty string (or otherwise skip) when targetUnit is px.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/inspector/utils/resolve-theme-defaults.ts, line 51:
<comment>Percentage values are returned even when the target unit is `px`, so percent-based padding/border styles will be misinterpreted as pixels. Return an empty string (or otherwise skip) when targetUnit is `px`.</comment>
<file context>
@@ -0,0 +1,182 @@
+ return Math.round(parsed.numeric * bodyFontSize);
+ }
+
+ if (parsed.unit === '%') {
+ return targetUnit === '%' ? parsed.numeric : parsed.numeric;
+ }
</file context>
| // Detect number with unit (e.g., "20px", "1.5em", "100%") | ||
| const numericMatch = strValue.match(/^(-?\d*\.?\d+)(px|%|em|rem|vh|vw)?$/); | ||
| if (numericMatch) { | ||
| const detectedUnit = numericMatch[2] || 'px'; |
Contributor
There was a problem hiding this comment.
P2: Defaulting unitless numeric CSS values to "px" will rewrite unitless styles (like line-height/opacity) as pixel values when the user edits them. Preserve the absence of a unit so edits round-trip correctly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/inspector/utils/parse-attributes.ts, line 106:
<comment>Defaulting unitless numeric CSS values to "px" will rewrite unitless styles (like line-height/opacity) as pixel values when the user edits them. Preserve the absence of a unit so edits round-trip correctly.</comment>
<file context>
@@ -0,0 +1,124 @@
+ // Detect number with unit (e.g., "20px", "1.5em", "100%")
+ const numericMatch = strValue.match(/^(-?\d*\.?\d+)(px|%|em|rem|vh|vw)?$/);
+ if (numericMatch) {
+ const detectedUnit = numericMatch[2] || 'px';
+ return {
+ label: humanizeString(prop),
</file context>
| (variableItem?.type as VariableType) ?? 'string'; | ||
| const variableType = localType ?? serverType; | ||
|
|
||
| React.useEffect(() => { |
Contributor
There was a problem hiding this comment.
P2: The ref-update useEffect runs before the cleanup effect, so on variableId changes the cleanup dispatch can use the new variableKey/displayKey while comparing against the previous fallback. This can send fallback updates to the wrong variable. Move the ref update below the cleanup effect (or otherwise ensure cleanup reads the previous variable context).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/sections/fallback.tsx, line 64:
<comment>The ref-update `useEffect` runs before the cleanup effect, so on `variableId` changes the cleanup dispatch can use the *new* variableKey/displayKey while comparing against the previous fallback. This can send fallback updates to the wrong variable. Move the ref update below the cleanup effect (or otherwise ensure cleanup reads the previous variable context).</comment>
<file context>
@@ -0,0 +1,268 @@
+ (variableItem?.type as VariableType) ?? 'string';
+ const variableType = localType ?? serverType;
+
+ React.useEffect(() => {
+ setLocalType(null);
+ }, [variableId]);
</file context>
gabrielmfern
force-pushed
the
feat/global-inspector
branch
from
c8f4e0b to
57ac343
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/editor/src/ui/inspector/USAGE.md">
<violation number="1" location="packages/editor/src/ui/inspector/USAGE.md:47">
P2: The docs example uses `<Inspector>` as a component, but the exported API only supports `Inspector.Root`, `Inspector.Panel`, `Inspector.Breadcrumb`, and `Inspector.DefaultField`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/editor/src/ui/inspector/explorations.md">
<violation number="1" location="packages/editor/src/ui/inspector/explorations.md:59">
P2: The example callback parameter was renamed to `context`, but the body still uses `target` as a standalone variable. Update the snippet to read `context.target` (or destructure `target`) so the documented API example is valid.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| ```tsx | ||
| <Inspector>{( | ||
| context: |
Contributor
There was a problem hiding this comment.
P2: The example callback parameter was renamed to context, but the body still uses target as a standalone variable. Update the snippet to read context.target (or destructure target) so the documented API example is valid.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/editor/src/ui/inspector/explorations.md, line 59:
<comment>The example callback parameter was renamed to `context`, but the body still uses `target` as a standalone variable. Update the snippet to read `context.target` (or destructure `target`) so the documented API example is valid.</comment>
<file context>
@@ -56,8 +56,18 @@ and also give a good DX:
<Inspector>{(
- target: 'doc' | 'text' | FocusedNode,
- setAttribute: (name: string, value: unknown) => void
+ context:
+ | {
+ target: 'doc',
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/editor/src/ui/inspector/document.tsx">
<violation number="1" location="packages/editor/src/ui/inspector/document.tsx:248">
P1: Missing fallback return causes this component to return `undefined` when `inspectorTarget !== 'doc'`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/editor/examples/src/examples/document-inspector.tsx">
<violation number="1" location="packages/editor/examples/src/examples/document-inspector.tsx:224">
P2: Using `label` as the input `id` creates invalid and non-unique DOM ids; generate a stable unique id per row instead of reusing display text.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
gabrielmfern
force-pushed
the
feat/global-inspector
branch
from
5892ed9 to
ca59250
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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.
Summary by cubic
Adds a theme-aware Inspector to edit document, text, and node styles with unit-aware controls and document color presets. Exposes a simple, render‑props API and ships minimal CSS plus a new example.
New Features
Inspector.Provider,Inspector.Document, and render‑propsInspector.BreadcrumbwithpathFromRoot; exported from@react-email/editor/ui.@react-email/editor/styles/inspector.css(also imported by the default theme).examples/document-inspector.tsxto the examples app.Refactors
useEvent(eventName, handler, options)for React subscriptions.@react-email/editor/core; removed legacy utils types.Written for commit ca59250. Summary will update on new commits.