feat: add Miller columns JSON explorer and professional playback cont…#44
Conversation
…rols * Introduced `JsonFinder`, a Miller columns-style explorer for navigating, editing, and deleting JSON data with breadcrumb support. * Integrated `JsonFinder` into the main application, replacing the basic textarea editor for managing SDUI configurations. * Implemented bulk editing functionality in `JsonFinder` allowing path-based field updates via formulas (e.g., `+10`, `*0.5`). * Redesigned the preview player with professional controls, including SVG-based play/pause/step buttons and a custom-styled seek bar. * Added media detection to the JSON explorer to provide inline previews for image, video, and audio URLs. * Enhanced keyboard navigation within the explorer, supporting arrow keys for navigation and "Enter" or "E" for editing. * Updated global styles for range inputs and introduced reusable player button components.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces a new ChangesJSON Editor Integration & Playback UI
Sequence DiagramsequenceDiagram
participant User
participant App
participant JsonFinder
participant PreviewPanel
User->>JsonFinder: load or edit JSON
JsonFinder->>JsonFinder: parse and load from localStorage
User->>JsonFinder: select leaf in column view
JsonFinder->>PreviewPanel: display metadata & value
User->>PreviewPanel: double-click to edit
PreviewPanel->>PreviewPanel: enter edit mode
User->>PreviewPanel: save changes
PreviewPanel->>JsonFinder: persist to nested object
JsonFinder->>JsonFinder: update localStorage
JsonFinder->>App: onUpdate callback with new sdui
App->>App: update sdui state & recalc maxFrames
User->>App: seek or play/pause playback
App->>App: update currentFrame via skipFrames
App->>App: render preview at currentFrame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR introduces a sophisticated Miller columns JSON explorer with bulk editing capabilities and professional playback controls. The implementation demonstrates strong UI/UX design and comprehensive functionality.
Critical Issues Found (4)
I've identified 4 critical defects that must be fixed before merge:
- Division by zero vulnerability in bulk formula processing - will crash with
/0formula - Array bounds validation missing in
setValueAtPath- causes silent failures with invalid indices - Array bounds validation missing in
deleteAtPath- causes incorrect behavior with out-of-bounds indices - Unhandled clipboard API rejection - causes console errors in restricted contexts
All issues have specific code suggestions that can be applied directly from the GitHub UI.
Files Changed
web-sdui/src/App.tsx- Integrated JsonFinder component with improved player controlsweb-sdui/src/components/JsonFinder.tsx- New Miller columns explorer (1790 lines)web-sdui/src/index.css- Enhanced styling for player controls
Please address the critical issues before merging.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| navigator.clipboard.writeText(text).then(() => { | ||
| setCopied(key); | ||
| setTimeout(() => setCopied(null), 1400); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛑 Crash Risk: Missing error handling for clipboard write operation. When clipboard API fails (e.g., in non-HTTPS contexts or when permissions are denied), unhandled promise rejection will cause console errors and potentially break the UI.
| navigator.clipboard.writeText(text).then(() => { | |
| setCopied(key); | |
| setTimeout(() => setCopied(null), 1400); | |
| }); | |
| }; | |
| const copy = (text: string, key: string) => { | |
| navigator.clipboard.writeText(text).then(() => { | |
| setCopied(key); | |
| setTimeout(() => setCopied(null), 1400); | |
| }).catch(() => { | |
| // Silently fail if clipboard access is denied | |
| }); |
| const idx = typeof head === "number" ? head : parseInt(head, 10); | ||
| const copy = [...root]; | ||
| copy[idx] = setValueAtPath(copy[idx], tail, newValue); | ||
| return copy; |
There was a problem hiding this comment.
🛑 Logic Error: Missing boundary check causes array index out of bounds. When an array item at idx doesn't exist in the nested path, accessing copy[idx] will be undefined, causing setValueAtPath to fail silently or crash when trying to traverse deeper. Add validation before recursive call.
| const idx = typeof head === "number" ? head : parseInt(head, 10); | |
| const copy = [...root]; | |
| copy[idx] = setValueAtPath(copy[idx], tail, newValue); | |
| return copy; | |
| const idx = typeof head === "number" ? head : parseInt(head, 10); | |
| if (idx < 0 || idx >= root.length) return root; | |
| const copy = [...root]; | |
| copy[idx] = setValueAtPath(copy[idx], tail, newValue); |
| if (op === '+') newVal = currentVal + valNum; | ||
| else if (op === '-') newVal = currentVal - valNum; | ||
| else if (op === '*') newVal = currentVal * valNum; | ||
| else if (op === '/') newVal = currentVal / valNum; |
There was a problem hiding this comment.
🛑 Division by Zero Crash Risk: Missing validation before division operation. When formula is /0, this will cause division by zero, resulting in Infinity which could break JSON serialization and cause application crashes.
| else if (op === '/') newVal = currentVal / valNum; | |
| else if (op === '/') newVal = valNum !== 0 ? currentVal / valNum : currentVal; |
| const idx = typeof head === "number" ? head : parseInt(head, 10); | ||
| if (tail.length === 0) { | ||
| const copy = [...root]; | ||
| copy.splice(idx, 1); | ||
| return copy; | ||
| } | ||
| const copy = [...root]; | ||
| copy[idx] = deleteAtPath(copy[idx], tail); | ||
| return copy; |
There was a problem hiding this comment.
🛑 Logic Error: Missing boundary check causes array index out of bounds. When idx is beyond array bounds, copy[idx] will be undefined and calling deleteAtPath on undefined will cause incorrect behavior or crashes.
| const idx = typeof head === "number" ? head : parseInt(head, 10); | |
| if (tail.length === 0) { | |
| const copy = [...root]; | |
| copy.splice(idx, 1); | |
| return copy; | |
| } | |
| const copy = [...root]; | |
| copy[idx] = deleteAtPath(copy[idx], tail); | |
| return copy; | |
| const idx = typeof head === "number" ? head : parseInt(head, 10); | |
| if (idx < 0 || idx >= root.length) return root; | |
| if (tail.length === 0) { | |
| const copy = [...root]; | |
| copy.splice(idx, 1); | |
| return copy; | |
| } | |
| const copy = [...root]; | |
| copy[idx] = deleteAtPath(copy[idx], tail); |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly upgrades the user interface by replacing the basic JSON text editor with a column-based explorer component, JsonFinder, and enhancing the video player with professional controls and frame-skipping functionality. The code review identifies several areas for improvement, including optimizing performance by moving inlined styles and stabilizing global event listeners, enhancing type safety by removing explicit any annotations, and fixing a potential bug in the bulk editing logic where division by zero could lead to invalid JSON values.
| if (sdui && sdui.views) { | ||
| let max = 0; | ||
| parsed.views.forEach((v: any) => { | ||
| sdui.views.forEach((v: any) => { |
There was a problem hiding this comment.
The use of any here bypasses TypeScript's type checking. Since sdui.views is an array of MotionViewProps, the type can be inferred automatically by removing the explicit any annotation.
| sdui.views.forEach((v: any) => { | |
| sdui.views.forEach((v) => { |
|
|
||
| if (!Array.isArray(target)) return; | ||
|
|
||
| const newRoot = JSON.parse(JSON.stringify(root)); |
There was a problem hiding this comment.
Using JSON.parse(JSON.stringify(root)) for deep cloning is inefficient for large JSON objects and can be slow. For a JSON explorer that might handle significant datasets, consider using the native structuredClone() method which is more performant and handles more data types.
| const newRoot = JSON.parse(JSON.stringify(root)); | |
| const newRoot = structuredClone(root); |
| if (op === '+') newVal = currentVal + valNum; | ||
| else if (op === '-') newVal = currentVal - valNum; | ||
| else if (op === '*') newVal = currentVal * valNum; | ||
| else if (op === '/') newVal = currentVal / valNum; |
There was a problem hiding this comment.
Potential division by zero. If valNum is 0, newVal will become Infinity. Since Infinity is not a valid JSON value, JSON.stringify will convert it to null during serialization, which may lead to unintended data loss in the configuration.
| else if (op === '/') newVal = currentVal / valNum; | |
| else if (op === '/') newVal = valNum !== 0 ? currentVal / valNum : currentVal; |
| useEffect(() => { | ||
| const onKey = (e: KeyboardEvent) => { | ||
| // Ignore if typing in an input/textarea | ||
| const target = e.target as HTMLElement; | ||
| if (target.tagName === "INPUT" || target.tagName === "TEXTAREA" || target.tagName === "SELECT") { | ||
| if (e.key === "Escape") { | ||
| (target as HTMLElement).blur(); | ||
| setKeyboardMode(true); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (!root) return; | ||
|
|
||
| const maxCol = columns.length - 1; | ||
| const currentCol = columns[focusedCol]; | ||
| if (!currentCol) return; | ||
|
|
||
| const maxRow = currentCol.items.length - 1; | ||
|
|
||
| switch (e.key) { | ||
| case "ArrowDown": { | ||
| e.preventDefault(); | ||
| setKeyboardMode(true); | ||
| setFocusedRow((prev) => { | ||
| const next = prev < 0 ? 0 : Math.min(prev + 1, maxRow); | ||
| return next; | ||
| }); | ||
| break; | ||
| } | ||
| case "ArrowUp": { | ||
| e.preventDefault(); | ||
| setKeyboardMode(true); | ||
| setFocusedRow((prev) => { | ||
| const next = prev < 0 ? maxRow : Math.max(prev - 1, 0); | ||
| return next; | ||
| }); | ||
| break; | ||
| } | ||
| case "ArrowRight": { | ||
| e.preventDefault(); | ||
| setKeyboardMode(true); | ||
| const item = currentCol.items[focusedRow >= 0 ? focusedRow : 0]; | ||
| if (item && isExpandable(item.value)) { | ||
| handleSelect(focusedCol, item); | ||
| setFocusedCol((prev) => Math.min(prev + 1, maxCol + 1)); | ||
| setFocusedRow(0); | ||
| } else if (focusedCol < maxCol) { | ||
| setFocusedCol((prev) => prev + 1); | ||
| setFocusedRow(0); | ||
| } | ||
| break; | ||
| } | ||
| case "ArrowLeft": { | ||
| e.preventDefault(); | ||
| setKeyboardMode(true); | ||
| if (focusedCol > 0) { | ||
| setFocusedCol((prev) => prev - 1); | ||
| setFocusedRow(-1); | ||
| } | ||
| break; | ||
| } | ||
| case "Enter": { | ||
| e.preventDefault(); | ||
| setKeyboardMode(true); | ||
| const item = currentCol.items[focusedRow >= 0 ? focusedRow : 0]; | ||
| if (item) { | ||
| handleSelect(focusedCol, item); | ||
| if (isExpandable(item.value)) { | ||
| setFocusedCol((prev) => Math.min(prev + 1, maxCol + 1)); | ||
| setFocusedRow(0); | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| case "Escape": { | ||
| setEditingKey(null); | ||
| if (selectedItem) { | ||
| setSelectedItem(null); | ||
| } | ||
| break; | ||
| } | ||
| case "Delete": | ||
| case "Backspace": { | ||
| if (selectedItem && focusedRow >= 0) { | ||
| e.preventDefault(); | ||
| handleDelete(); | ||
| setFocusedRow((prev) => Math.max(prev - 1, 0)); | ||
| } | ||
| break; | ||
| } | ||
| case "e": | ||
| case "E": { | ||
| if (!e.ctrlKey && !e.metaKey && !e.altKey) { | ||
| const item = currentCol.items[focusedRow >= 0 ? focusedRow : 0]; | ||
| if (item && !isExpandable(item.value)) { | ||
| e.preventDefault(); | ||
| handleDoubleClick(item); | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| case "Home": { | ||
| e.preventDefault(); | ||
| setKeyboardMode(true); | ||
| setFocusedRow(0); | ||
| break; | ||
| } | ||
| case "End": { | ||
| e.preventDefault(); | ||
| setKeyboardMode(true); | ||
| setFocusedRow(maxRow); | ||
| break; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("keydown", onKey); | ||
| return () => window.removeEventListener("keydown", onKey); | ||
| }, [root, columns, focusedCol, focusedRow, selectedItem, handleSelect, handleDelete, handleDoubleClick]); |
There was a problem hiding this comment.
The global keydown event listener is re-registered on every state change (e.g., every time the user moves the selection with arrow keys) due to the extensive dependency array. This is inefficient and can cause performance degradation as the JSON complexity increases. Consider using a useRef to store the current state or a useReducer to keep the event handler stable. Additionally, attaching the listener to window is less idiomatic for a component; consider using a local onKeyDown on a focusable container element.
|
|
||
| return ( | ||
| <div className="jf-root" onClick={() => setKeyboardMode(false)}> | ||
| <style>{css}</style> |
There was a problem hiding this comment.
Inlining a <style> tag containing an @import inside the render function is a performance bottleneck. The browser may re-parse the CSS and potentially re-trigger the font download on every re-render (which happens on every keyboard navigation step). Move the CSS to a global stylesheet or use useMemo to prevent unnecessary re-processing of the style block.
1 participant
…rols
JsonFinder, a Miller columns-style explorer for navigating, editing, and deleting JSON data with breadcrumb support.JsonFinderinto the main application, replacing the basic textarea editor for managing SDUI configurations.JsonFinderallowing path-based field updates via formulas (e.g.,+10,*0.5).Summary by CodeRabbit
New Features
Style