refactor: convert tutorials.js to TypeScript#746
refactor: convert tutorials.js to TypeScript#746Nihal4777 merged 10 commits intoCircuitVerse:mainfrom
Conversation
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe file src/simulator/src/tutorials.ts was updated to add JSDoc and explicit typings: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulator/src/tutorials.ts (1)
106-131: Add runtime safety checks for DOM queries and type casts.Multiple unsafe operations that could cause runtime errors:
- Line 106: Non-null assertion on
.querySelector('.panelHeader')assumes the element exists- Line 108:
e.target as HTMLElementcast without validation - the target could be a text node or other non-HTMLElement- Lines 118-119:
nextElementSibling as HTMLElementcast without checking if it exists- Line 128: Non-null assertion on
.querySelector('.icon')without validationIf any element is missing or the wrong type, these will throw
TypeErrorat runtime.🛡️ Recommended defensive checks
export const tutorialWrapper = (): void => { const panelHighlight = new Driver() - document.querySelector('.panelHeader')!.addEventListener('click', (e:Event) => { + const panelHeader = document.querySelector('.panelHeader') + if (!panelHeader) return + + panelHeader.addEventListener('click', (e:Event) => { if (localStorage.tutorials === 'next') { - const target = e.target as HTMLElement + const target = e.target + if (!(target instanceof HTMLElement)) return + + const nextSibling = target.nextElementSibling + if (!(nextSibling instanceof HTMLElement)) return + panelHighlight.highlight({ element: '#guide_1', showButtons: false, popover: { title: 'Here are the elements', description: 'Select any element by clicking on it & then click anywhere on the grid to place the element.', position: 'right', offset: - (target.nextElementSibling as HTMLElement).offsetHeight + + nextSibling.offsetHeight + target.offsetTop - 45, }, }) localStorage.setItem('tutorials', 'done') } }, { once: true, }) - document.querySelector('.icon')!.addEventListener('click', () => { + const icon = document.querySelector('.icon') + if (icon) { + icon.addEventListener('click', () => { - panelHighlight.reset(true) - }) + panelHighlight.reset(true) + }) + } }
🤖 Fix all issues with AI agents
In @src/simulator/src/tutorials.ts:
- Line 141: The direct cast and .click() on
(document.querySelector('.draggable-panel .maximize') as HTMLElement).click()
can throw if the selector returns null; update the call to safely query the
element into a nullable variable (e.g., const el =
document.querySelector('.draggable-panel .maximize') as HTMLElement | null) and
only call el.click() when el is non-null (optionally handle the null case with a
safe fallback or a log message) so the runtime won't raise a TypeError.
- Line 144: The code stores the flag using
localStorage.setItem('tutorials_tour_done', String(true)) which is inconsistent
with earlier versions; change this to store the boolean-style value consistently
(e.g., localStorage.setItem('tutorials_tour_done', true) or use the agreed
string token like 'done') so it matches the pattern used for other tutorial keys
and the truthiness checks in setup.js; update the call that sets
'tutorials_tour_done' accordingly.
🧹 Nitpick comments (1)
src/simulator/src/tutorials.ts (1)
1-1: Consider upgradingdriver.jsfrom 0.9.8 to 1.4.0.The codebase uses
driver.jsversion 0.9.8. While no security vulnerabilities were found, the latest available version is 1.4.0 (released Nov 2025), representing four major versions of improvements and updates. However, this is a major version upgrade and should be tested thoroughly for breaking changes before merging.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/tutorials.ts
🔇 Additional comments (2)
src/simulator/src/tutorials.ts (2)
3-15: LGTM! Well-structured type definitions.The interfaces correctly capture the structure of the tour data with appropriate optional/required fields and a constrained position type.
17-17: LGTM! Explicit typing enhances type safety.The explicit
TourStep[]type annotation properly constrains the tour array structure.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulator/src/tutorials.ts (1)
116-140: Non-null assertions and unsafe casts may cause runtime errors.Several DOM access patterns here could fail:
- Lines 116, 138: Non-null assertions (
!) onquerySelectorwill throw if elements don't exist.- Line 128:
nextElementSiblingcan benull, and casting it directly toHTMLElementbefore accessingoffsetHeightwill fail.Consider adding null guards or optional chaining for robustness:
Proposed safer implementation
export const tutorialWrapper = (): void => { const panelHighlight = new Driver() - document.querySelector('.panelHeader')!.addEventListener('click', (e:Event) => { + const panelHeader = document.querySelector('.panelHeader') + if (!panelHeader) return + + panelHeader.addEventListener('click', (e: Event) => { if (localStorage.tutorials === 'next') { const target = e.target as HTMLElement + const sibling = target.nextElementSibling as HTMLElement | null + if (!sibling) return + panelHighlight.highlight({ element: '#guide_1', showButtons: false, popover: { title: 'Here are the elements', description: 'Select any element by clicking on it & then click anywhere on the grid to place the element.', position: 'right', offset: - (target.nextElementSibling as HTMLElement).offsetHeight + - target.offsetTop - + sibling.offsetHeight + target.offsetTop - 45, }, }) localStorage.setItem('tutorials', 'done') } }, { once: true, }) - document.querySelector('.icon')!.addEventListener('click', () => { - panelHighlight.reset(true) - }) + const icon = document.querySelector('.icon') + if (icon) { + icon.addEventListener('click', () => { + panelHighlight.reset(true) + }) + } }
🧹 Nitpick comments (1)
src/simulator/src/tutorials.ts (1)
153-162: Good null handling; consider simplifying the string conversion.The null check for
maximizeButtonis the correct pattern. However,String(true)is verbose—consider using the string literal'true'directly for clarity.- localStorage.setItem('tutorials', String(true)) + localStorage.setItem('tutorials', 'true')
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/tutorials.ts
🔇 Additional comments (3)
src/simulator/src/tutorials.ts (3)
3-15: Well-structured type definitions.The interfaces accurately model the Driver.js tour step structure with appropriate optional properties and a union type for position.
22-106: LGTM!The tour array is properly typed and all steps conform to the
TourStepinterface.
1-1: The project is already correctly pinned to Driver.js 0.9.x via"driver.js": "^0.9.8"in package.json, which prevents accidental upgrades to the incompatible 1.x release. The code uses the correct 0.9.x API (import Driver from 'driver.js', popoverclassName,position,offsetproperties). No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/simulator/src/tutorials.ts:
- Around line 128-130: The code accesses target.nextElementSibling as
HTMLElement without checking for null, so calling .offsetHeight may throw;
update the calculation that uses (target.nextElementSibling as
HTMLElement).offsetHeight to first test if target.nextElementSibling exists
(e.g., const sibling = target.nextElementSibling as HTMLElement | null) and use
sibling ? sibling.offsetHeight : 0 (or bail out/handle missing sibling) when
computing target.offsetTop - 45 to avoid runtime exceptions in the function that
performs this positioning.
- Line 161: The tutorial state uses two different localStorage keys ('tutorials'
and 'tutorials_tour_done'), causing inconsistent checks; standardize to the key
used by setup.js ('tutorials_tour_done') by changing all
localStorage.getItem/setItem calls in tutorials.ts that reference 'tutorials'
(the reads/writes around the functions that check/update tutorial state) to use
'tutorials_tour_done' instead, and remove any leftover writes of the old
'tutorials' key so all tutorial state reads and writes consistently use
'tutorials_tour_done'.
🧹 Nitpick comments (3)
src/simulator/src/tutorials.ts (3)
117-117: UselocalStorage.getItem()for type safety.Direct property access (
localStorage.tutorials) works but is less type-safe. PreferlocalStorage.getItem('tutorials')for explicit null handling.♻️ Suggested refactor
- if (localStorage.tutorials === 'next') { + if (localStorage.getItem('tutorials') === 'next') {
138-140: Apply defensive check pattern for consistency and safety.The
tutorialWrapperfunction uses non-null assertions on DOM selectors, but the.iconelement doesn't exist in the codebase. Additionally, this approach inconsistently differs from the defensive pattern used inshowTourGuide, which safely queries and checks for element existence before use.♻️ Suggested defensive check
- document.querySelector('.icon')!.addEventListener('click', () => { - panelHighlight.reset(true) - }) + const iconElement = document.querySelector('.icon') + if (iconElement) { + iconElement.addEventListener('click', () => { + panelHighlight.reset(true) + }) + }
116-116: Fix type annotation spacing and verify DOM selectors match actual element classes.The type annotation
(e:Event)should be(e: Event)per TypeScript conventions. Additionally, the non-null assertions on.panelHeaderand.iconare risky—these selectors won't find elements if they don't exist. Note that thePanelHeader.vuecomponent uses the classpanel-header(kebab-case), notpanelHeader, so the querySelector will fail. Since this function is currently unused but retained for future use, consider either updating the selectors to match the actual DOM structure or adding defensive null checks.♻️ Suggested improvements
- document.querySelector('.panelHeader')!.addEventListener('click', (e:Event) => { + const panelHeader = document.querySelector('.panel-header') + if (!panelHeader) return + panelHeader.addEventListener('click', (e: Event) => {- document.querySelector('.icon')!.addEventListener('click', () => { + const iconElement = document.querySelector('.icon') + if (!iconElement) return + iconElement.addEventListener('click', () => {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/tutorials.ts
🔇 Additional comments (3)
src/simulator/src/tutorials.ts (3)
3-15: LGTM! Well-structured type definitions.The
TourPopoverandTourStepinterfaces accurately model the Driver.js tour configuration. The position union type properly constrains values, and optional fields are correctly marked.
22-22: LGTM! Proper type annotation for tour configuration.Typing
tourasTourStep[]provides compile-time safety and IDE support.
154-158: LGTM! Excellent defensive null check pattern.The defensive check for the maximize button is well-implemented with proper type casting. This pattern prevents runtime errors and should serve as a model for other DOM queries in this file.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/simulator/src/tutorials.ts:
- Around line 139-141: The code uses a non-null assertion on
document.querySelector('.icon') before calling addEventListener, which will
throw if the element is missing; update the snippet that calls
document.querySelector('.icon')! and instead first check the returned element
(e.g., const icon = document.querySelector('.icon')) and only call
icon.addEventListener('click', ...) or use optional chaining to safely attach
the listener and then call panelHighlight.reset(true) inside that guarded block
so the listener is only added when the element exists.
- Line 116: The current use of the non-null assertion on
document.querySelector('.panelHeader') before calling addEventListener can throw
if the element is absent; change it to first get the element into a variable
(e.g., panelHeaderEl via document.querySelector('.panelHeader')) and then guard
with an existence check before calling panelHeaderEl.addEventListener(...), or
use optional chaining
(document.querySelector('.panelHeader')?.addEventListener(...)); ensure the
click handler callback remains the same.
- Line 162: The code currently sets localStorage.setItem('tutorials_tour_done',
'done') immediately when the tour starts, so users who close midway never see
the tour again; move this flag to run only when the tour is actually completed
or closed by attaching a handler to the Driver callbacks instead of setting it
at start: after calling animatedTourDriver.defineSteps(tour) and
animatedTourDriver.start(), override or wrap animatedTourDriver.onCloseClick
(and/or detect completion by wrapping animatedTourDriver.onNextClick/onPrevClick
to detect the last step) to call localStorage.setItem('tutorials_tour_done',
'done') only when the tour finishes/closed, and ensure you call the original
handlers (e.g., originalOnCloseClick) after setting the flag.
🧹 Nitpick comments (1)
src/simulator/src/tutorials.ts (1)
108-114: Consider removing or fixing unused exported function.The function is marked as "Not used currently" but remains exported. Combined with the dead code path issue (line 117), consider either:
- Removing it if not needed
- Fixing the localStorage logic and documenting when it will be used
- Making it private if it's only for future internal use
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/tutorials.ts
🔇 Additional comments (4)
src/simulator/src/tutorials.ts (4)
3-15: LGTM! Well-structured TypeScript interfaces.The interfaces properly type the Driver.js tour structure with appropriate optional fields and constrained string literals for positioning.
22-106: LGTM! Tour array properly typed.The tour array is correctly typed as
TourStep[]and all steps conform to the interface definitions.
155-159: LGTM! Proper null checking for DOM element.The maximizeButton access correctly guards against null with type casting and a conditional check.
117-135: Dead code: localStorage condition never satisfied.The condition checks for
'next'but this value is never set anywhere in the codebase. The only value set is'done'(lines 134, 162), meaning this entire code block will never execute. The functiontutorialWrapperis explicitly marked as "Not used currently" and "retained for future onboarding flows", confirming this is incomplete/abandoned logic.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/simulator/src/tutorials.ts (1)
114-147: Dead code: 'next' value is never set, making the function unused.Line 119 checks if
localStorage.getItem('tutorials_tour_done') === 'next', but this value is never set anywhere in the codebase. Only'done'(lines 136, 164) and'true'(in legacy versions) are assigned. This means the condition will always be false.While the code comments acknowledge this function is "Not used currently" and "retained for future onboarding flows," the dead code should either be removed or completed if intended for future use. If keeping it for future implementation, set the
'next'value appropriately inshowTourGuideto make the feature functional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/tutorials.ts
🔇 Additional comments (3)
src/simulator/src/tutorials.ts (3)
3-15: LGTM! Well-structured TypeScript interfaces.The interfaces provide clear type definitions for the tour configuration. The position union type properly restricts values to valid placements.
17-22: LGTM! Clear documentation and explicit typing.The documentation comment and explicit
TourStep[]typing improve code clarity and type safety.
152-170: I was unable to verify the claims in this review comment because the repository could not be accessed to search for localStorage references. To properly validate the breaking change assertion, I would need to confirm:
- Whether the old
'tutorials'key is actually used elsewhere in the codebase- Whether the new
'tutorials_tour_done'key has migration logic already in place- What the actual scope of user impact is
The review identifies a potential breaking change in the localStorage key and value format change, but without access to the full codebase context, I cannot confirm whether this migration is addressed elsewhere or if the suggested migration logic is necessary.
|
Summary of coderabbitai requested changes
|
|
Greetings @Nihal4777 , please review the changes and suggest if any changes are required. Thanks. |
Nihal4777
left a comment
Nihal4777
left a comment
There was a problem hiding this comment.
@abhiraj75, Thanks for the TS migration!
The library driver.js already bundles its own typings. We can probably remove the manual type declarations and rely on the bundled ones.
|
Sure @Nihal4777 , will look into it and get back to you! |
abhiraj75
force-pushed
the
abhiraj_branch
branch
from
b3eaea0 to
0717a4d
Compare
|
@Nihal4777 , this migration uses Driver.js's built-in types (Driver.Step, Driver.PopoverOptions) as requested. However, the original code uses className at the Step level and showButtons in the highlight method, which aren't included in the library's type definitions. I've used intersection types (Driver.Step & { className?: string }) to preserve the original functionality while maintaining type safety. The original logic remains unchanged. |
|
Hey @Nihal4777 , I have made the changes suggested by you , please review it once and suggest if something else needs to be implemented . Thanks |
|
Hey @Nihal4777 , please let me know if there needs to be any change with the implementation . Thanks! |
|
Hi @abhiraj75, Can you please confirm whether |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 64: The package currently depends on jsdom and cross-env versions that
require Node >=20; either update the project's Node requirement or revert these
deps to Node18-compatible releases: update the package.json "engines" entry to
>=20 (and adjust CI) if you want to move to Node 20+, or pin "jsdom" to a
Node‑18 compatible version and set "cross-env" back to ^7.x so they work with
Node 18; modify the "jsdom" and "cross-env" entries (or the "engines" field)
accordingly and run install/CI to verify.
♻️ Duplicate comments (1)
src/simulator/src/tutorials.ts (1)
143-151: **Tutorial completion flag is still set at tour start, not on completion.**The PR description states thattutorials_tour_doneis "now set only when tour is closed/finished using Driver.js's onReset callback," but the implementation still sets this flag immediately whenstart()is called (line 150). This means users who exit the tour early will never see it again.For Driver.js 0.9.x, the
onResetcallback was available and would be triggered when the tour closes or resets. You should move the localStorage write into theonResetconfiguration option:🐛 Proposed fix
const animatedTourDriver = new Driver({ animate: true, opacity: 0.8, padding: 5, showButtons: true, + onReset: () => { + localStorage.setItem('tutorials_tour_done', 'true') + }, }) /** * Launches the interactive tutorial tour for the simulator UI. */ export function showTourGuide(): void { const maximizeButton = document.querySelector('.draggable-panel .maximize') as HTMLElement | null if (maximizeButton) { maximizeButton.click() } animatedTourDriver.defineSteps(tour) animatedTourDriver.start() - localStorage.setItem('tutorials_tour_done', 'true') }
abhiraj75
force-pushed
the
abhiraj_branch
branch
from
31a22f0 to
81dff5d
Compare
|
hey @Nihal4777 , I've made the following changes => |
Nihal4777
left a comment
Nihal4777
left a comment
There was a problem hiding this comment.
Thanks @abhiraj75 for the changes. Just apply the changes in v1 as well.
|
Sure @Nihal4777 |
|
@Nihal4777 , Final implementation has been done as suggested by you . Please review it . Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@v1/src/simulator/src/tutorials.ts`:
- Around line 98-121: The function tutorialWrapper is unused and reads/writes
localStorage keys using 'tutorials', causing inconsistent state with the active
showTourGuide() flow which uses 'tutorials_tour_done'; either remove
tutorialWrapper entirely or update its localStorage calls (both
localStorage.getItem('tutorials') and localStorage.setItem('tutorials','done'))
to use 'tutorials_tour_done' so the key matches showTourGuide(), and ensure any
future use of tutorialWrapper is actually invoked or documented.
- Around line 133-150: The completion marker is being set immediately after
animatedTourDriver.start(), so early exits are falsely recorded; instead, add an
onReset callback to the Driver options (where animatedTourDriver is constructed)
that calls localStorage.setItem('tutorials_tour_done','true') and remove the
existing localStorage.setItem call from showTourGuide; update the Driver
instantiation (the new Driver({...}) for animatedTourDriver) to include onReset:
() => { localStorage.setItem('tutorials_tour_done','true') } so the key is
written only when the tour is actually closed/reset.
|
Thanks @Nihal4777 ! Do let me know if there is something else that I can take up ! Should I refactor the other js files in the src folder as well? |
2 participants
Part Of #661
Converted
src/simulator/src/tutorials.jsto TypeScript(tutorials.ts).No logic or runtime behaviour changes , only type annotations and interfaces added.
Frontend build passes locally.
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.