-
Notifications
You must be signed in to change notification settings - Fork 230
Fix Verilog UI resize bugs: preservation, visibility, and console errors #881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Walkthrough
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/simulator/src/plotArea.js (1)
444-459: Reinitialize the 2D context after re-acquiring the canvas.
plot()reassignsthis.canvasbut keeps the oldctx. If the old canvas was unmounted (or setup returned early),render()can draw to a stale/undefined context, yielding a blank diagram or errors. Recreatectxwhen the canvas changes.🛠️ Suggested fix
plot() { // Check if canvas is available (may be unmounted in Verilog mode) if (!this.canvas) { this.canvas = document.getElementById('plotArea') } if (!this.canvas) return + if (!embed && (!this.ctx || this.ctx.canvas !== this.canvas)) { + this.ctx = this.canvas.getContext('2d') + }v1/src/simulator/src/plotArea.js (1)
444-459: Reinitialize the 2D context after re-acquiring the canvas.
plot()reassignsthis.canvasbut keeps the oldctx. If the old canvas was unmounted (or setup returned early),render()can draw to a stale/undefined context, yielding a blank diagram or errors. Recreatectxwhen the canvas changes.🛠️ Suggested fix
plot() { // Check if canvas is available (may be unmounted in Verilog mode) if (!this.canvas) { this.canvas = document.getElementById('plotArea') } if (!this.canvas) return + if (!embed && (!this.ctx || this.ctx.canvas !== this.canvas)) { + this.ctx = this.canvas.getContext('2d') + }v1/src/components/Extra.vue (1)
31-47: Correct verilogOutput duplicate ID in mobile Verilog mode.Both
VerilogEditorPanel(v-show active whenisVerilogis true) andVerilogEditorPanelMobile(v-if renders whenshowMobileView && showVerilogPanel) can render simultaneously in mobile+Verilog mode, creating a duplicateverilogOutputID. The Verilog2CV.js code usesdocument.getElementById('verilogOutput')to display compiler output with styling, but this will target only the first occurrence, breaking output display for one of the panels. Either prevent simultaneous rendering by adding!simulatorMobileStore.showVerilogPanelto VerilogEditorPanel's v-show condition, or give each panel a unique verilogOutput ID.
🤖 Fix all issues with AI agents
In `@src/simulator/src/plotArea.js`:
- Around line 163-167: The plotArea.setup() early-return leaves the render
interval unstarted when the canvas is absent; ensure setup is re-invoked when
the TimingDiagramPanel remounts by calling plotArea.setup() (or
plotArea.resetup() if you already have that helper) from
TimingDiagramPanel.onMounted() or by triggering resetup when
simulatorMobileStore.isVerilog flips back to false; update the component mount
logic to call plotArea.setup()/resetup() so the interval that drives plot() is
created after the canvas is available.
|
@tachyons @Nihal4777 kindly review |
|
@Nihal4777 can you please review |

Fixes #880
Describe the changes you have made in this PR -
This PR resolves critical issues related to the Verilog UI instability during window resizing. Specifically, it fixes:
v-iftov-showand removing duplicate DOM IDs.!isVerilog).Uncaught TypeError: Cannot read properties of nullerrors caused by the timing diagram render loop accessing an unmounted canvas. Added null checks inplotArea.js.VerilogEditorPanelin a single root element.srcand thev1codebase.Screenshots of the UI changes (If any) -
fix.mp4
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
v-ifdestroying the editor component on resize (losing state) and the render loop trying to draw on a non-existent canvas.v-showto toggle visibility via CSS instead of unmounting the component, preserving the CodeMirror instance. I also added defensive programming (null checks) in the plotArea.js to handle the lifecycle differences gracefully. I used Vue's reactive store (simulatorMobileStore.isVerilog) to manage panel visibility declaratively rather than relying on brittle DOM manipulation.Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.