Fix : MiniMap Not Rendering and Make It Visible #507
Conversation
WalkthroughThe pull request refactors the minimap functionality in the simulator. A new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant MM as miniMapArea
participant Cnv as HTMLCanvasElement
participant Win as Window
participant Log as Console
App->>MM: setup()
MM->>Cnv: Verify canvas element exists
alt Canvas exists
MM->>Win: Retrieve window.width & window.height
MM->>MM: Calculate minX, maxX, minY, maxY via ternary operators
MM->>MM: Set dimensions using template literals
else Canvas missing
MM->>Log: Log warning about missing canvas
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v0/src/simulator/src/minimap.js(2 hunks)
🧰 Additional context used
🪛 GitHub Check: ESLint
v0/src/simulator/src/minimap.js
[failure] 111-111: disallow the use of undeclared variables unless mentioned in /*global */ comments
'updateOrder' is not defined.
🪛 ESLint
v0/src/simulator/src/minimap.js
[error] 111-111: 'updateOrder' is not defined.
(no-undef)
🔇 Additional comments (5)
v0/src/simulator/src/minimap.js (5)
39-53: Good refactoring of boundary calculationsThe use of ternary operators for calculating boundaries improves readability. Good work!
71-75: Good practice using template literalsThe switch to template literals for style assignments is cleaner and more readable.
112-144: Good modernization of loopsThe switch from
vartoletand use offor...ofloops improves code quality and reduces potential scoping issues.
160-160: Good use of Date.now()Using
Date.now()instead ofnew Date().getTime()is more concise and slightly more efficient.
170-170:❓ Verification inconclusive
Verify timing logic for minimap removal
The condition has been changed from
<=to>=which inverts the logic. Ensure this is intentional and correct.
🏁 Script executed:
#!/bin/bash # Find the original implementation to compare echo "Searching for previous implementation of removeMiniMap..." git log -p -- "v0/src/simulator/src/minimap.js" | grep -A 5 -B 5 "lastMiniMapShown.*Date"Length of output: 124
Action: Confirm the Intended Inversion of the Timing Logic
The condition now reads:if (lastMiniMapShown + 2000 >= Date.now()) {which is the inverse of the previous
<=operator. No historical diff context was found to indicate an intentional change, so please verify that switching to>=correctly captures the minimap's removal timing behavior. If this adjustment was made on purpose, consider adding a brief explanatory note in the commit message.
|
cc : @vedant-jain03 @niladrix719 @aryanndwi123 can you please look my PR. the codeclimate test failing cause of similarity of code with just axis name changes 'x' and 'y'. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
v0/src/simulator/src/minimap.js (2)
34-35:⚠️ Potential issueUse proper window dimension properties
window.heightandwindow.widthare not standard browser properties. You should usewindow.innerHeightandwindow.innerWidthinstead.- this.pageHeight = window.height // Make sure you define `height` correctly in your environment. - this.pageWidth = window.width // Make sure you define `width` correctly in your environment. + this.pageHeight = window.innerHeight + this.pageWidth = window.innerWidth
5-5: 🛠️ Refactor suggestionUse a proper theme import rather than a hardcoded value
The
lightModeconstant is currently hardcoded tofalse, but the commented code suggests it should be using anisLightMode()function. This approach won't dynamically respond to theme changes.- const lightMode = false // isLightMode() + import { isLightMode } from './themer/themer' // Adjust import path as needed + const lightMode = isLightMode()
🧹 Nitpick comments (4)
v0/src/simulator/src/minimap.js (4)
7-7: Use let instead of var and combine declaration with initializationUsing
varis outdated in modern JavaScript. Since this object is initialized later, you should useletfor more appropriate scoping.- var miniMapArea + let miniMapArea
20-21: Use proper JSDoc format for global variablesThe comment for global
updateOrderdoesn't follow proper JSDoc format. If you want to declare it as a global dependency, use the@globaltag.- * global updateOrder + * @global {Array} updateOrder - Array determining the order of elements to render
28-32: Redundant canvas initializationThe canvas is already initialized at line 23, making this re-initialization redundant. However, the added null check is good for error handling.
- this.canvas = document.getElementById('miniMapArea') - if (!this.canvas) { + if (!this.canvas) { console.warn('miniMapArea canvas element not found') return }
161-161: Use let instead of var for better scopingUsing
varis outdated in modern JavaScript. Useletfor variables that will be reassigned.- var lastMiniMapShown = 0 + let lastMiniMapShown = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v0/src/simulator/src/minimap.js(2 hunks)
🔇 Additional comments (9)
v0/src/simulator/src/minimap.js (9)
39-55: Good refactoring using ternary operatorsThe refactoring of the min/max calculations using ternary operators improves readability while maintaining the same logic. Well done!
73-77: Good error handling for miniMap element dimensionsAdding a null check before setting the element dimensions is a good practice. The use of template literals for style property values is also a modern approach.
87-87: Improved drawing efficiencyUsing
fillRect()directly instead of separaterect()andfill()calls is more efficient and concise.
97-97: Improved line visibilitySetting the line width to 2 improves the visibility of the minimap elements, which addresses one of the PR objectives.
153-157: Inconsistent canvas referencesIn the
clearmethod, you're creating a new reference to the canvas element withdocument.getElementById('miniMapArea')while also usingthis.canvas. This is redundant and could lead to confusion.clear() { if (lightMode) return - const canvas = document.getElementById('miniMapArea') - if (canvas) { + if (this.canvas) { $('#miniMapArea').css('z-index', '-1') this.ctx.clearRect(0, 0, this.canvas.width, this.canvas.height) } }
164-164: Good use of Date.now()Using
Date.now()instead ofnew Date().getTime()is more concise and efficient.
167-177: Good early returns and strict equality checksThe use of early returns and strict equality checks improves the code's clarity and reliability. The timeout logic ensures the minimap is only hidden after being visible for at least 2 seconds.
182-182: Good code organizationMoving the export statement to the end of the file improves code organization and readability.
113-113:✅ Verification successful
Fixed undefined variable issue
Properly referencing
window.updateOrderwith a fallback to an empty array fixes the undefined variable issue highlighted in previous code scans.
🏁 Script executed:
#!/bin/bash # Check if updateOrder is properly defined in the global scope echo "Checking references to updateOrder in the codebase:" rg -A 2 "updateOrder\s*=|window.updateOrder" --type jsLength of output: 731
Review Approved: Proper Global Variable Initialization in
minimap.jsThe change to use a fallback value with
window.updateOrderinv0/src/simulator/src/minimap.jsis verified. The shell script output confirms thatwindow.updateOrderis indeed defined in the corresponding setup files, ensuring that the code no longer encounters an undefined variable.
- The snippet
const lst = window.updateOrder || []safely defaults to an empty array ifwindow.updateOrderisn’t set.- References in both
v0/src/simulator/src/setup.jsandv1/src/simulator/src/setup.jsconfirm that the global variable is appropriately defined.
There was a problem hiding this comment.
Thanks @gitsofaryan for the contribution. I have tested the minimap rendering in the preview URL ✅
I have some comments regarding code, PTAL!
| @@ -2,79 +2,80 @@ import simulationArea from './simulationArea' | |||
| import { colors } from './themer/themer' | |||
| import { layoutModeGet } from './layoutMode' | |||
|
|
|||
| const lightMode = false // isLightMode() | |||
There was a problem hiding this comment.
| const lightMode = false // isLightMode() | |
| const isLightMode = false |
There was a problem hiding this comment.
thanks for the suggestion, i will update the variable name.
| this.pageY = this.pageHeight - globalScope.oy | ||
| this.pageX = this.pageWidth - globalScope.ox | ||
| // CodeClimate ignore similar lines check |
There was a problem hiding this comment.
added this ignore comment to suppress the CodeClimate duplication block warning because the pageX and pageY calculations are essential for positioning in the minimap logic. and it looked similar block of code to code climate.
| this.maxY = (simulationArea.maxHeight !== undefined) | ||
| ? Math.max(simulationArea.maxHeight, this.pageY / globalScope.scale) | ||
| : this.pageY / globalScope.scale | ||
| // CodeClimate ignore similar lines check |
| this.minX = (simulationArea.minWidth !== undefined) | ||
| ? Math.min(simulationArea.minWidth, -globalScope.ox / globalScope.scale) | ||
| : -globalScope.ox / globalScope.scale | ||
| // CodeClimate ignore similar lines check |
There was a problem hiding this comment.
used terneray style to make coed look concise and shorter. if you'd like, i can refactor it to prev if else version.
| resolve(ratio) { | ||
| if (lightMode) return | ||
|
|
||
| this.ctx.fillStyle = '#ddd' | ||
| this.ctx.beginPath() | ||
| this.ctx.lineWidth = 2 // Thicker lines for better visibility |
There was a problem hiding this comment.
Do we have same lineWidth in legacy simulator?
There was a problem hiding this comment.
in the legacy simulator minimap code, their is no explicit mention of line width, it is set with value as it is from canvas. this is new change to make the minimap visible.
|
Also, please create a issue and attach to this PR! |
|
We cannot merge it with v0. Updates need to be made in src/ for now. Let's discuss this tomorrow in the mentors' meeting because many contributors are facing this issue @vedant-jain03 |
|
@vedant-jain03 @niladrix719 |
Describe the changes you have made in this PR -
This PR fixes a bug where the miniMap (that small overview map in the corner) wasn’t showing up at all. The map was either not rendering correctly, or just completely invisible.
Changes I Made -
✅ Added checks to make sure the miniMap only tries to show when it's actually supposed to.
✅ Fixed some drawing issues so the miniMap correctly updates and displays the components and wires.
✅ Made sure the miniMap correctly resizes to fit the whole workspace, so it doesn’t get stuck or disappear.
✅ Double-checked that it only shows up in dark mode, and it’s fully hidden in light mode (like it was meant to).
After Changes -
✅ MiniMap now shows up when it should.
✅ It updates as you add/remove components.
✅ It hides in light mode like expected.
✅ No more console errors related to miniMap rendering.
Screenshots of the changes (If any) -
Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.
Summary by CodeRabbit
Bug Fixes
Refactor