Better memory input #459
Better memory input #459
Conversation
✅ Deploy Preview for pvm-debugger ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe MemoryEditor component undergoes a comprehensive refactor replacing simple editing state with a focus-driven, multi-ref system. The change introduces nibble-based hex editing, per-byte hex and ASCII inputs, keyboard navigation, paste handling, and input validation for precise byte-level editing workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/HostCallDialog/MemoryEditor.tsx (3)
49-76: Consider adding an abort mechanism for rapid address/length changes.If the user rapidly changes address or length, multiple async
readMemorycalls can race. The last to complete (not necessarily the last triggered) will set the data, potentially showing stale results.This is likely acceptable for a debugger UI, but adding an abort controller or a request ID check would make it more robust.
useEffect(() => { + let cancelled = false; const loadMemory = async () => { if (address === null || length === null || length <= 0) { setData(null); return; } if (length > MAX_LENGTH) { setError(`Length cannot exceed ${MAX_LENGTH} bytes`); return; } setIsLoading(true); setError(null); try { const memoryData = await readMemory(address, length); + if (cancelled) return; setData(memoryData); } catch (e) { + if (cancelled) return; setError(e instanceof Error ? e.message : "Failed to read memory"); setData(null); } finally { + if (!cancelled) setIsLoading(false); - setIsLoading(false); } }; loadMemory(); + return () => { cancelled = true; }; }, [address, length, readMemory]);
416-439: Addaria-labelfor accessibility.The hex and ASCII inputs lack accessible labels. Screen reader users won't know which byte address they're editing.
<input key={i} ref={(el) => { inputRefs.current[byteIndex] = el; }} + aria-label={`Hex byte at offset ${(address! + byteIndex).toString(16).toUpperCase()}`} className={`w-4 h-4 text-center font-mono text-xs uppercase rounded border ${
460-481: Addaria-labelfor ASCII inputs as well.Same accessibility concern applies to the ASCII input cells.
<input key={i} ref={(el) => { asciiRefs.current[byteIndex] = el; }} + aria-label={`ASCII byte at offset ${(address! + byteIndex).toString(16).toUpperCase()}`} className={`w-4 h-4 text-center font-mono text-xs rounded border p-0 ${
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/HostCallDialog/MemoryEditor.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/components/**/*.{ts,tsx}: Keep reusable UI components insrc/components/
Write TypeScript-first functional components
Files:
src/components/HostCallDialog/MemoryEditor.tsx
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Use two-space indentation in TypeScript files
Use descriptive filenames in kebab-case (e.g.,memory-range-panel.tsx)
Validate external inputs with Zod when parsing user data
Files:
src/components/HostCallDialog/MemoryEditor.tsx
src/**/*.{css,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Favor Tailwind utilities from
globals.cssbefore adding new CSS files
Files:
src/components/HostCallDialog/MemoryEditor.tsx
🔇 Additional comments (14)
src/components/HostCallDialog/MemoryEditor.tsx (14)
1-11: LGTM!Imports, constants, and interface definition are clean and appropriate for the component's functionality.
13-35: LGTM!Helper functions for parsing and formatting are correctly implemented.
45-47: LGTM!State and refs are appropriately initialized for the focus-driven editing workflow.
78-95: LGTM!Address and length update callbacks handle parsing and clamping correctly.
97-128: LGTM!Focus management callbacks properly clamp indices and use
requestAnimationFramefor reliable DOM manipulation.
130-139: LGTM!Immutable update pattern with proper byte masking.
141-156: LGTM!Nibble-level hex editing logic is correctly implemented with proper bit masking.
158-182: LGTM!Nibble navigation helpers correctly handle boundary conditions.
184-229: LGTM!Comprehensive keyboard handling for hex editing with proper nibble-level navigation.
231-260: LGTM!Input validation and hex character filtering work correctly with proper nibble advancement.
262-287: LGTM!Paste handling correctly processes hex strings with proper prefix stripping and odd-length padding.
289-323: LGTM!ASCII input and paste handlers correctly process character data with byte masking.
325-350: LGTM!ASCII keyboard navigation is consistent with hex editing behavior.
443-451: LGTM!Padding for incomplete rows maintains visual alignment.
1 participant
better memory input for default host call dialog