Remove svelte and use vanilla TS #3352
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces Svelte UI components with TypeScript factory functions that produce DOM/SVG elements, removes Svelte tooling and deps, adds CommonJS build/exports, migrates tests to DOM factories, introduces DOM helpers, and extracts component CSS. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Step as Step
participant Factory as createShepherdElement()
participant Dialog as HTMLDialogElement
participant DOM as Document
App->>Step: step.show()
Step->>Factory: createShepherdElement({ step, descriptionId, labelId })
Factory->>Dialog: build dialog (header, content, footer, arrow)
Factory->>Dialog: attach keydown & focus handlers
Factory->>DOM: return { element: Dialog, cleanup() }
Step->>DOM: append Dialog to container
Note over Dialog: User interacts (clicks, keys)
Dialog->>Step: invoke handlers (cancel, next, back)
App->>Step: step.hide() / step.cancel()
Step->>Factory: call cleanup()
Factory->>Dialog: remove listeners
Step->>DOM: remove Dialog
sequenceDiagram
participant App as Application
participant Tour as Tour
participant ModalFactory as createShepherdModal()
participant SVG as SVG Overlay
participant RAF as requestAnimationFrame
App->>Tour: initialize()
Tour->>ModalFactory: createShepherdModal(container)
ModalFactory->>SVG: create overlay SVG + path
App->>Tour: startStep()
Tour->>ModalFactory: setupForStep(step)
alt step.useModalOverlay
ModalFactory->>SVG: show()
SVG->>RAF: start RAF loop
RAF->>SVG: positionModal() updates openings based on targets/iframes
else
ModalFactory->>SVG: hide()
end
App->>Tour: end()
Tour->>ModalFactory: destroy()
ModalFactory->>RAF: cancel loop and remove SVG
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 7
🤖 Fix all issues with AI agents
In `@shepherd.js/rollup.config.mjs`:
- Around line 152-191: The CJS build's plugin named "After CJS build tweaks"
relies on an ESM artifact (it copies ./dist/js/shepherd.d.mts to
./dist/cjs/shepherd.d.cts) inside its closeBundle hook, which breaks if builds
run in parallel; modify the closeBundle in that plugin to first check for the
existence of ./dist/js/shepherd.d.mts (and retry/wait or throw a clear error)
before attempting the cp/mv commands, or explicitly document the ordering
dependency with a comment referencing the plugin name and the closeBundle hook
so future maintainers know the CJS step depends on the ESM output; ensure any
failure logs a clear message including the missing path.
- Around line 70-78: The treeshake setting in sharedConfig currently sets
moduleSideEffects: false which causes Rollup to drop side-effect-only CSS
imports like the bare import './components/shepherd.css' in shepherd.ts; change
moduleSideEffects to a function allowlist that returns true for CSS imports
(e.g., check id.endsWith('.css') or match './components/shepherd.css') so Rollup
preserves those side-effectful CSS modules while keeping false for other
modules—update the treeshake.moduleSideEffects property in the sharedConfig
object accordingly.
In `@shepherd.js/src/components/shepherd-modal.ts`:
- Around line 252-258: The code reads scrollTop/scrollLeft from the DOMRect
returned by getBoundingClientRect (dead code); update the offset calculation in
the targetIframe block to use the iframe element's scroll offsets instead of the
rect's properties — e.g., call getBoundingClientRect() to get rect.top/left and
then add targetIframe.scrollTop and targetIframe.scrollLeft (or
targetIframe.contentWindow?.scrollY/scrollX if you intend document scroll inside
the iframe) when computing offset.top/offset.left, and remove the bogus DOMRect
cast and ?? 0 fallback.
- Around line 104-113: The current loop over elementsToHighlight incorrectly
skips all occurrences because it checks indexOf(el) !== lastIndexOf(el); change
the logic to keep the first occurrence and skip later duplicates by comparing
the current index to the first index (e.g., only continue when
elementsToHighlight.indexOf(el) !== currentIndex), or better yet deduplicate
elementsToHighlight before the loop (e.g., create a filtered array via a Set or
array.filter((el,i,a)=>a.indexOf(el)===i)) and then iterate that deduplicated
array; update the code around the loop that references elementsToHighlight to
use the deduplicated list so at least one opening is created per unique element.
In `@shepherd.js/src/components/shepherd-text.ts`:
- Around line 20-24: The current branch sets el.innerHTML = text without
guarding against non-string values, which will render "undefined" or "null" if
text is missing; update the branch that handles non-HTMLElement text (the else
after isHTMLElement(text)) to only assign innerHTML when text is a string (e.g.,
check typeof text === 'string') and otherwise set el.innerHTML to an empty
string (or another safe default), keeping the isHTMLElement, el.appendChild,
el.innerHTML and text identifiers to locate the change.
In `@shepherd.js/src/step.ts`:
- Around line 474-483: updateStepOptions currently calls destroy(), which emits
the public "destroy" event and misleads consumers; instead implement a private
teardown method (e.g., _teardownInternal or teardownWithoutEmit) that performs
the same DOM/component cleanup that destroy() does but does not emit the
"destroy" event, and call that from updateStepOptions before _setupElements();
keep the existing public destroy() unchanged (it should still emit the event)
and ensure _setupElements() continues to call bindAdvance() or equivalent to
reattach handlers after the internal teardown.
In `@shepherd.js/test/unit/components/shepherd-button.spec.js`:
- Around line 16-117: The label and text test cases are incorrectly nested
inside the describe('disabled', ...) block; extract the label tests (those using
{ label: ... } and references to aria-label) and the text tests (those using {
text: ... } and toHaveTextContent) into their own top-level describe blocks
(e.g., describe('label', ...) and describe('text', ...)) placed at the same
level as describe('disabled', ...); locate the tests by the createShepherdButton
calls that pass label or text and move each group so describe('disabled', ... )
only contains disabled-related it() cases while label- and text-related it()
cases are grouped under their respective describe blocks.
🧹 Nitpick comments (12)
shepherd.js/package.json (1)
21-34: CJS + ESM dual export setup looks correct.The conditional exports are properly structured, and
.cjsextension ensures CommonJS semantics regardless of the"type": "module"field. Good that@arethetypeswrong/cliis in devDependencies and thetypes:check:attwscript validates the package exports.One minor note: there's no top-level
"types"field alongside"main". Tools that don't understand the"exports"map (e.g., TypeScript < 4.7 with oldermoduleResolutionsettings) won't resolve type declarations for the CJS entry. If you want to support those consumers, consider adding:"types": "./dist/cjs/shepherd.d.cts",Not critical given modern tooling and the breaking-change nature of this PR.
shepherd.js/src/utils/dom.ts (1)
40-44: Consider using a more explicit event listener prefix likeon-oron:.The
onprefix check (key.startsWith('on')) could inadvertently match non-event attributes (e.g., hypotheticaloneTimeUse). The AI summary mentionson-as the convention, but the implementation useson(camelCase style likeonClick). This works fine for the current internal usage but is worth noting if this utility is extended.shepherd.js/src/components/shepherd-title.ts (1)
14-15: ConsidertextContentinstead ofinnerHTMLfor the title.Since
StringOrStringFunctionresolves to a plain string,textContentwould be safer and sufficient here unless HTML-in-titles is an intentional feature. If HTML support is needed, a brief doc comment clarifying that would help.Proposed change
const resolvedTitle = isFunction(title) ? title() : title; - el.innerHTML = resolvedTitle; + el.textContent = resolvedTitle;shepherd.js/src/components/shepherd-button.ts (2)
34-36:innerHTMLwith user-supplied button text — XSS surface by design.The
StepOptionsButton.textJSDoc says "The HTML text of the button," so this is intentional. However, sincetextcan come from a dynamic function (getConfigOption), any user who passes unsanitized input through the config is exposed. Consider documenting this trust boundary or offering atextContentpath for plain-text labels.
23-32: Class string can contain leading/trailing whitespace and double spaces.When
config.classesisundefined, the template literal produces a leading space (e.g.," shepherd-button "). Similarly, whensecondaryis false, there's a trailing space. While not functionally broken,classListand selector matching can be subtly affected by stray whitespace in some environments.♻️ Suggested cleanup
- const btn = h('button', { - 'aria-label': label || null, - class: `${config.classes || ''} shepherd-button ${ - config.secondary ? 'shepherd-button-secondary' : '' - }`, + const classes = [ + config.classes, + 'shepherd-button', + config.secondary ? 'shepherd-button-secondary' : null + ].filter(Boolean).join(' '); + + const btn = h('button', { + 'aria-label': label || null, + class: classes,shepherd.js/test/unit/components/shepherd-content.spec.js (1)
1-82: Tests are correct, but coverage is limited to header rendering.The header conditional logic is well-tested across all four scenarios. However, the test suite only covers the header portion of
createShepherdContent. There are no tests for text rendering (step.options.text) or footer rendering (step.options.buttons).shepherd.js/src/step.ts (1)
686-688: Duplicatethis.el.hidden = falseassignment.Lines 673-674 and 686-688 both set
this.el.hidden = false. This appears to be a pre-existing duplication (both are unchanged lines), but since this file is being reworked it's a good time to clean it up.shepherd.js/test/unit/components/shepherd-modal.spec.js (1)
432-441: SkippedsetupForSteptests reduce coverage.The comment explains the spy limitation, but
setupForStepis a key integration point (it wires up per-step modal behavior). Consider testing the observable outcome instead — e.g., after callingsetupForStep(step)with a step configured for modal overlay, assert that the modal element has theshepherd-modal-is-visibleclass and that the pathdattribute reflects the target element's position.shepherd.js/test/unit/components/shepherd-button.spec.js (1)
18-18: Passingundefinedasstepis fragile and masks potential NPEs.Every call uses
createShepherdButton({...}, undefined), but the factory signature expects aStepinstance (e.g.,config.action.bind(step.tour)would throw ifactionwere set). Consider creating a minimal mock/stubStepto avoid silent breakage if the factory internals ever touchstepfor the options being tested (e.g.,getConfigOptionreceivesstep).shepherd.js/test/unit/components/shepherd-element.spec.js (1)
6-8:keyCodeis deprecated in favor ofkeyorcode.Both the helper and the production code (
shepherd-element.tslines 50, 93, 100, 107) use the deprecatedKeyboardEvent.keyCode. This is fine for now since it matches, but worth noting as a follow-up to migrate both toevent.key(e.g.,'Escape','ArrowLeft','ArrowRight','Tab').shepherd.js/src/components/shepherd-element.ts (1)
48-117:keyCodeis deprecated — consider migrating toevent.key.
KeyboardEvent.keyCodeis deprecated in modern browsers. Usingevent.key('Tab','Escape','ArrowLeft','ArrowRight') would be more future-proof and readable. Not urgent for this PR since it's a straight port from the Svelte version.shepherd.js/src/components/shepherd-modal.ts (1)
46-294: No publicdestroy/disposemethod to remove the SVG element and thetouchmovelistener.The factory appends the SVG to
container(line 66) and registers atouchmovelistener on it (line 61), but the returned API has no way to tear these down. If the modal is recreated (e.g., on re-init), the old SVG and listener would leak. Consider adding adestroy()method that removes the element and its listeners.
| for (const el of elementsToHighlight) { | ||
| if (!el) continue; | ||
|
|
||
| // Skip duplicate elements | ||
| if ( | ||
| elementsToHighlight.indexOf(el) !== | ||
| elementsToHighlight.lastIndexOf(el) | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Duplicate-element deduplication removes ALL occurrences instead of keeping one.
When the same element appears more than once in elementsToHighlight, indexOf(el) !== lastIndexOf(el) is true for every occurrence — so every copy is skipped and no opening is created for that element at all.
Proposed fix — skip only subsequent duplicates
for (const el of elementsToHighlight) {
if (!el) continue;
- // Skip duplicate elements
- if (
- elementsToHighlight.indexOf(el) !==
- elementsToHighlight.lastIndexOf(el)
- ) {
- continue;
- }
+ // Skip duplicate elements (keep only the first occurrence)
+ if (elementsToHighlight.indexOf(el) !== elementsToHighlight.lastIndexOf(el) &&
+ elementsToHighlight.indexOf(el) !== elementsToHighlight.indexOf(el, 0)) {
+ continue;
+ }Or more simply, deduplicate the array upfront:
const elementsToHighlight = [
targetElement,
...(extraHighlights || [])
];
+ const uniqueElements = [...new Set(elementsToHighlight)];
const newOpenings: OpeningProperty[] = [];
- for (const el of elementsToHighlight) {
+ for (const el of uniqueElements) {
if (!el) continue;
-
- // Skip duplicate elements
- if (
- elementsToHighlight.indexOf(el) !==
- elementsToHighlight.lastIndexOf(el)
- ) {
- continue;
- }🤖 Prompt for AI Agents
In `@shepherd.js/src/components/shepherd-modal.ts` around lines 104 - 113, The
current loop over elementsToHighlight incorrectly skips all occurrences because
it checks indexOf(el) !== lastIndexOf(el); change the logic to keep the first
occurrence and skip later duplicates by comparing the current index to the first
index (e.g., only continue when elementsToHighlight.indexOf(el) !==
currentIndex), or better yet deduplicate elementsToHighlight before the loop
(e.g., create a filtered array via a Set or
array.filter((el,i,a)=>a.indexOf(el)===i)) and then iterate that deduplicated
array; update the code around the loop that references elementsToHighlight to
use the deduplicated list so at least one opening is created per unique element.
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)
.prettierrc.js (1)
16-16:⚠️ Potential issue | 🟡 MinorLeftover Svelte override after removing
prettier-plugin-svelte.The
prettier-plugin-sveltewas removed from the plugins array, but the*.svelteoverride still references thesvelteparser. This is dead configuration and should be removed for consistency.🧹 Proposed fix
- overrides: [ - { - files: '*.astro', - options: { - parser: 'astro' - } - }, - { files: '*.svelte', options: { parser: 'svelte' } } - ] + overrides: [ + { + files: '*.astro', + options: { + parser: 'astro' + } + } + ]
🤖 Fix all issues with AI agents
In `@shepherd.js/src/components/shepherd-modal.ts`:
- Around line 46-65: The factory createShepherdModal currently appends the SVG
element (element) and attaches a touchmove listener (_preventModalOverlayTouch)
but never exposes a way to tear it down; add a destroy() method to the
ShepherdModalAPI that removes the touchmove listener from element (using the
same _preventModalOverlayTouch reference), cancels any scheduled RAF (rafId) and
removes the SVG element from its parent (element.remove()), then include this
destroy function in the returned API object from createShepherdModal so callers
can fully clean up the modal.
In `@shepherd.js/test/unit/components/shepherd-button.spec.js`:
- Around line 45-57: Test passes a number for the label which violates the
StepOptionsButton type (StringOrStringFunction = string | (() => string));
update the test to use a string or function instead of 5 (e.g., { label: '5' }
or { label: () => '5' }) so it matches the contract used by createShepherdButton
and avoids TypeScript errors if converted to .ts.
🧹 Nitpick comments (5)
shepherd.js/src/utils/dom.ts (1)
37-47: Consider handlingvalue === truefor boolean HTML attributes.When
valueistrueand the key is not'disabled'(e.g.,'hidden','required','autofocus'),setAttribute(key, 'true')is called, which sets the attribute to the string"true". For standard boolean HTML attributes, the correct representation is the empty string or the attribute name itself (e.g.,setAttribute('hidden', '')). In practice this still works in browsers (any value makes a boolean attribute truthy), but'open'on line 128 ofshepherd-element.tsis set to the string'true'via this path, producing<dialog open="true">instead of the more canonical<dialog open="">.This is a very minor correctness nit — browsers handle it fine.
🔧 Optional: canonical boolean attribute handling
} else if (key === 'disabled' && value === true) { (el as HTMLButtonElement).disabled = true; + } else if (value === true) { + el.setAttribute(key, ''); } else { el.setAttribute(key, String(value)); }shepherd.js/src/components/shepherd-element.ts (2)
6-9:e.keyCodeis deprecated — prefere.keystring comparisons.
KeyboardEvent.keyCodeis deprecated in the DOM spec. Usinge.keyis the modern, more readable approach and avoids magic numbers.♻️ Proposed refactor
-const KEY_TAB = 9; -const KEY_ESC = 27; -const LEFT_ARROW = 37; -const RIGHT_ARROW = 39;Then in
handleKeyDown:- const handleKeyDown = (e: KeyboardEvent) => { - const { tour } = step; - switch (e.keyCode) { - case KEY_TAB: + const handleKeyDown = (e: KeyboardEvent) => { + const { tour } = step; + switch (e.key) { + case 'Tab': ... - case KEY_ESC: + case 'Escape': ... - case LEFT_ARROW: + case 'ArrowLeft': ... - case RIGHT_ARROW: + case 'ArrowRight': ...Also applies to: 48-113
23-41: Type ofhasTitleisstring | false, notboolean.
step.options?.title ?? falseyields the title string (or the result of calling it if it were already resolved) when truthy, orfalsewhen nullish. This works correctly in the conditional on line 122 (hasTitle ? ... : ''), but the inferred type isstring | falserather than a cleanboolean. Consider using!!for clarity:- const hasTitle = step.options?.title ?? false; + const hasTitle = !!step.options?.title;This is a minor readability nit — behavior is identical.
shepherd.js/test/unit/components/shepherd-modal.spec.js (1)
433-440: SkippedsetupForSteptests leave a coverage gap that's straightforward to fill.Instead of spying on
hide/show, you can assert the observable outcome directly: check whethermodal.getElement()has theshepherd-modal-is-visibleclass after callingsetupForStepwith a mock step. For example:it('useModalOverlay: false, hides modal', () => { const modal = createShepherdModal(container); modal.show(); // start visible modal.setupForStep({ options: {}, tour: { options: { useModalOverlay: false } } }); expect(modal.getElement()).not.toHaveClass('shepherd-modal-is-visible'); });You'd also need to call
modal.hide()inafterEach(or add acleanup/destroyto the API) to cancel the rAF loop started by_styleForStepwhenuseModalOverlay: true.shepherd.js/src/components/shepherd-modal.ts (1)
200-215: Minor: settingrafId = undefinedbefore scheduling the next frame creates a brief window where cleanup can't cancel the loop.In
rafLoop,rafIdis set toundefinedat the top (Line 201), then a new frame is requested at the bottom (Line 211). If_cleanupStepEventListenersis called during the synchronous execution ofpositionModal(e.g., via a step callback that triggers cleanup), thecancelAnimationFramecheck at Line 179 would seerafIdasundefinedand miss cancelling the already-queued next frame.A safer pattern is to only clear
rafIdwhen explicitly cancelled:Proposed fix
const rafLoop = () => { - rafId = undefined; positionModal( modalOverlayOpeningPadding, modalOverlayOpeningRadius, modalOverlayOpeningXOffset + iframeOffset.left, modalOverlayOpeningYOffset + iframeOffset.top, scrollParent, step.target, step._resolvedExtraHighlightElements ); rafId = requestAnimationFrame(rafLoop); }; - rafLoop(); + rafId = requestAnimationFrame(rafLoop); _addStepEventListeners();
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shepherd.js/src/step.ts (1)
644-648:⚠️ Potential issue | 🟠 Major
_setupElementscallsdestroy()(not_teardownElements) whenelis defined — this emits adestroyevent during re-show.On line 647, if
this.elis not undefined (e.g., a step being shown again),this.destroy()is called, which triggers the publicdestroyevent. This is inconsistent with theupdateStepOptionsfix: here too the intent is internal teardown before recreation, not permanent destruction. The_showmethod calls_setupElementson every show (line 672), so if a step is shown, hidden, then shown again,this.elwill be non-null and adestroyevent fires mid-show.Suggested fix
_setupElements() { if (!isUndefined(this.el)) { - this.destroy(); + this._teardownElements(); }
🧹 Nitpick comments (2)
shepherd.js/rollup.config.mjs (2)
162-163: CJS build re-runs PostCSS extraction unnecessarily.The shared
pluginsarray includes thepostcss({ extract: 'css/shepherd.css' })plugin, so the CJS build will also extract CSS intotmp/cjs/css/shepherd.css— which is never used. Consider either excluding PostCSS from the CJS plugins or accepting the minor build-time cost.Possible fix
plugins: [ - ...plugins, + ...plugins.filter((p) => p.name !== 'postcss'), { name: 'After CJS build tweaks',
114-119: Consider invokingtscdirectly instead of vianpx.Since
typescriptis already a devDependency and available innode_modules/.bin, usingnpx tscadds unnecessary lookup overhead on every build. A directtsc(orpnpm tsc) call would be marginally faster and more predictable.
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (13)
🛟 Help
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@shepherd.js/src/components/shepherd-modal.ts`:
- Around line 110-123: The containment check mixes clipped geometry for el
(using _getVisibleHeight(el, scrollParent)) with unclipped geometry for
otherElement (otherElement.getBoundingClientRect()), which can falsely mark el
as contained; update the isContained logic to compare both elements using the
same clipped/visible geometry (e.g., call _getVisibleHeight or a new helper to
get visible x,y,width,height for otherElement using the same scrollParent) so
containment uses comparable visible rectangles; reference _getVisibleHeight,
elementsToHighlight, isContained, el and otherElement when making the change.
🧹 Nitpick comments (1)
shepherd.js/src/components/shepherd-modal.ts (1)
207-218:rafIdis cleared beforepositionModalruns — reentrant cleanup can't cancel this frame.Setting
rafId = undefined(line 208) beforepositionModal(line 209) means if any code path duringpositionModalcalls_cleanupStepEventListeners, thecancelAnimationFrameis a no-op, and line 218 schedules a new frame anyway. Moving the assignment afterrequestAnimationFrameavoids this:Suggested tweak
const rafLoop = () => { - rafId = undefined; positionModal( modalOverlayOpeningPadding, modalOverlayOpeningRadius, modalOverlayOpeningXOffset + iframeOffset.left, modalOverlayOpeningYOffset + iframeOffset.top, scrollParent, step.target, step._resolvedExtraHighlightElements ); rafId = requestAnimationFrame(rafLoop); }; + + rafId = requestAnimationFrame(rafLoop); - - rafLoop();This also avoids the first
positionModalcall being synchronous (outside RAF), which is a minor behavioral difference but aligns the first frame with subsequent ones.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@shepherd.js/src/components/shepherd-modal.ts`:
- Around line 250-262: The loop that walks up frames (starting from
el.ownerDocument.defaultView assigned to targetWindow) can throw a SecurityError
when comparing or accessing window.top/parent in cross-origin iframes; wrap the
cross-origin-sensitive operations in a try/catch and break out safely on
exception. Specifically, guard the while loop body that uses targetWindow !==
window.top, targetWindow.parent, targetWindow.frameElement, and
targetIframe.getBoundingClientRect/scrollTop/scrollLeft (referenced by
targetWindow, targetIframe and the surrounding while loop) by catching security
errors and stopping traversal so the offset accumulation stops without throwing.
In `@shepherd.js/test/unit/components/shepherd-modal.spec.js`:
- Around line 433-481: Update the test "skips duplicate elements in
extraHighlights" to reflect the corrected dedup behavior in positionModal: after
fixing the dedup logic (e.g., using a Set) one duplicate should be collapsed to
a single kept element, so the SVG path should contain 3 cutouts (outer path +
main target + one extra highlight). Locate the test in shepherd-modal.spec.js
that calls positionModal with [sharedElement, sharedElement], and change the
assertion that computes cutouts from expecting 2 to expecting 3 (the computed
variable using modal.getElement()/querySelector('path') and splitting d on 'Z').
🧹 Nitpick comments (4)
shepherd.js/src/components/shepherd-modal.ts (2)
213-228:rafIdis brieflyundefinedinsiderafLoop, creating a small cancellation gap.At line 214
rafIdis set toundefinedbefore the synchronouspositionModalcall. IfpositionModal(or anything it triggers synchronously) calls_cleanupStepEventListeners, the subsequentrequestAnimationFrameat line 224 won't be tracked or cancelled. Consider deferring the reset or restructuring:Suggested approach
const rafLoop = () => { - rafId = undefined; positionModal( modalOverlayOpeningPadding, modalOverlayOpeningRadius, modalOverlayOpeningXOffset + iframeOffset.left, modalOverlayOpeningYOffset + iframeOffset.top, scrollParent, step.target, step._resolvedExtraHighlightElements ); rafId = requestAnimationFrame(rafLoop); };Or guard the re-schedule with a
disposedflag.
231-243:_getScrollParentmay fail for elements inside iframes due to cross-realminstanceof HTMLElement.When
eloriginates from an iframe's document,el instanceof HTMLElementchecks against the parent window'sHTMLElementconstructor, which returnsfalse. This would causeoverflowYto always befalse, and the function would recurse up without ever finding a scrollable parent inside the iframe.This is a known cross-realm limitation and may not be an issue if targets are always in the same document, but worth noting given the iframe offset support.
shepherd.js/test/unit/components/shepherd-modal.spec.js (2)
14-16: Consider callingmodal.destroy()inafterEachto prevent window-level listener leaks.Tests that invoke
setupForStepwithuseModalOverlay: true(e.g., the_getScrollParentand_getIframeOffsettests) add atouchmovelistener onwindowvia_addStepEventListeners, but onlycontainer.remove()is called in teardown. The window listener survives, potentially polluting subsequent tests.Suggested pattern
+ let modal; + beforeEach(() => { container = document.createElement('div'); document.body.appendChild(container); }); afterEach(() => { + if (modal) { + modal.destroy(); + modal = null; + } container.remove(); });Then assign
modal = createShepherdModal(container)in each test (or restructure per describe block).
654-722: Modifyingdocument.defaultViewis fragile — it affects all elements sharing the document.The test overrides
targetEl.ownerDocument.defaultView, but in jsdom all elements share onedocument, so this temporarily breaks the globaldocument.defaultViewfor everything. The careful save/restore at lines 704–716 mitigates this, but any assertion failure before the restore would leave the document in a broken state for all subsequent tests.Consider wrapping the assertion + restore in a
try/finallyblock:Suggestion
modal.setupForStep(step); - // Restore defaultView before any assertions (jsdom needs it for instanceof checks) - if (origDescriptor) { - Object.defineProperty( - targetEl.ownerDocument, - 'defaultView', - origDescriptor - ); - } else { - Object.defineProperty(targetEl.ownerDocument, 'defaultView', { - value: window, - configurable: true - }); - } - - expect(modal.getElement()).toHaveClass('shepherd-modal-is-visible'); + try { + expect(modal.getElement()).toHaveClass('shepherd-modal-is-visible'); + } finally { + if (origDescriptor) { + Object.defineProperty( + targetEl.ownerDocument, + 'defaultView', + origDescriptor + ); + } else { + Object.defineProperty(targetEl.ownerDocument, 'defaultView', { + value: window, + configurable: true + }); + } + }
| it('skips duplicate elements in extraHighlights', () => { | ||
| const modal = createShepherdModal(container); | ||
|
|
||
| const sharedElement = { | ||
| getBoundingClientRect() { | ||
| return { | ||
| height: 100, | ||
| x: 50, | ||
| y: 50, | ||
| width: 100, | ||
| top: 50, | ||
| bottom: 150, | ||
| left: 50, | ||
| right: 150 | ||
| }; | ||
| } | ||
| }; | ||
|
|
||
| modal.positionModal( | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| null, | ||
| { | ||
| getBoundingClientRect() { | ||
| return { | ||
| height: 250, | ||
| x: 20, | ||
| y: 20, | ||
| width: 500, | ||
| top: 20, | ||
| bottom: 270, | ||
| left: 20, | ||
| right: 520 | ||
| }; | ||
| } | ||
| }, | ||
| // Pass the same element twice — both duplicates are skipped | ||
| [sharedElement, sharedElement] | ||
| ); | ||
|
|
||
| unmount(modalComponent); | ||
| const modalPath = modal.getElement().querySelector('path'); | ||
| const d = modalPath.getAttribute('d'); | ||
| // Duplicate elements are both skipped, only the main target cutout remains | ||
| // Outer path close + target cutout close = 2 Z's | ||
| const cutouts = d.split('Z').length - 1; | ||
| expect(cutouts).toBe(2); | ||
| }); |
There was a problem hiding this comment.
This test asserts the current (buggy) dedup behavior.
The duplicate-dedup logic in positionModal (lines 99–108 of shepherd-modal.ts) removes all occurrences of a duplicate element rather than keeping one. This test validates that buggy behavior by asserting only 2 cutouts. Once the dedup fix is applied (e.g., using new Set()), this test should be updated to expect 3 cutouts (outer path + main target + 1 deduplicated extra highlight).
🤖 Prompt for AI Agents
In `@shepherd.js/test/unit/components/shepherd-modal.spec.js` around lines 433 -
481, Update the test "skips duplicate elements in extraHighlights" to reflect
the corrected dedup behavior in positionModal: after fixing the dedup logic
(e.g., using a Set) one duplicate should be collapsed to a single kept element,
so the SVG path should contain 3 cutouts (outer path + main target + one extra
highlight). Locate the test in shepherd-modal.spec.js that calls positionModal
with [sharedElement, sharedElement], and change the assertion that computes
cutouts from expecting 2 to expecting 3 (the computed variable using
modal.getElement()/querySelector('path') and splitting d on 'Z').
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@shepherd.js/src/components/shepherd-button.css`:
- Around line 1-10: Update the .shepherd-button rule to use a fully opaque white
text color (e.g. `#fff` or rgba(255,255,255,1)) so the text/background contrast
meets WCAG AA; replace the broad transition: all 0.5s ease with a narrower list
such as transition: background-color, color 0.5s ease to avoid animating every
property; and remove or adjust the .shepherd-button:hover rule if it currently
sets the same color as the default state (make hover differ or delete the hover
rule).
In `@shepherd.js/src/components/shepherd-button.ts`:
- Around line 35-37: The assignment btn.innerHTML = text in shepherd-button.ts
introduces an XSS risk; replace it with btn.textContent = text (or sanitize the
HTML before setting innerHTML) so plain text is rendered safely, or explicitly
document and expose an option/property (e.g., allowHtml or htmlText) on the
ShepherdButton component if consumers must pass trusted HTML; update the setter
where btn.innerHTML is used to reference this new behavior (btn.textContent or
the sanitized/conditional path) and adjust any related docs/comments
accordingly.
In `@shepherd.js/src/components/shepherd-title.ts`:
- Around line 15-16: The current assignment uses el.innerHTML with resolvedTitle
(computed via isFunction(title) ? title() : title), which allows untrusted HTML
and creates an XSS risk; change this to set el.textContent by default and
introduce an explicit opt-in (e.g., a new allowHtml / allowHTML boolean in the
component options or props) that, when true, will use innerHTML after clearly
documenting that callers are responsible for sanitizing; update the code paths
that compute resolvedTitle and the DOM write (the resolvedTitle variable,
isFunction(title) branch, and the el assignment) to use textContent unless
allowHtml is enabled and add a brief doc comment about the responsibility for
sanitization when HTML is allowed.
🧹 Nitpick comments (9)
shepherd.js/src/components/shepherd-modal.ts (1)
210-226:iframeOffsetis captured once but used across rAF frames.
_getIframeOffsetis called once at line 211, then the result is reused in everyrafLoopiteration. If the page layout shifts (e.g., responsive resize, lazy-loaded content pushing the iframe), the offset will be stale until the nextsetupForStepcall. Consider moving it insiderafLoopif accuracy during layout shifts matters — the cost is negligible.shepherd.js/src/components/shepherd-header.ts (1)
7-18: Clean composition pattern — header factory looks good.The conditional rendering of title and cancel icon is straightforward and correct.
Nit on line 14: optional chaining would be slightly more concise:
Optional chaining
- if (step.options.cancelIcon && step.options.cancelIcon.enabled) { + if (step.options.cancelIcon?.enabled) {shepherd.js/src/components/shepherd-button.css (1)
12-15: Redundantcolordeclaration in hover state.Line 14's
color: rgba(255, 255, 255, 0.75)is identical to the base.shepherd-buttoncolor on line 5. It can be removed unless it's there intentionally to prevent inheritance issues.shepherd.js/src/components/shepherd-element.css (1)
37-49: Minor inconsistency: mixed pseudo-element selector syntax.Line 38 uses
::before(modern double-colon) while line 45 uses:before(legacy single-colon). Both work in all browsers, but it's slightly inconsistent. Consider standardizing on::beforethroughout.Consistency fix
-.shepherd-arrow:before { +.shepherd-arrow::before { content: '';shepherd.js/src/components/shepherd-button.ts (2)
24-33: Class string may contain extra whitespace.When
config.classesisundefined, the template literal produces" shepherd-button ..."with a leading space. Similarly, whensecondaryis false, a trailing space remains. While browsers tolerate extra whitespace in theclassattribute, it produces a messier DOM.Cleaner class construction using filter/join
- const btn = h('button', { - 'aria-label': label || null, - class: `${config.classes || ''} shepherd-button ${ - config.secondary ? 'shepherd-button-secondary' : '' - }`, + const classes = [ + config.classes, + 'shepherd-button', + config.secondary ? 'shepherd-button-secondary' : null + ] + .filter(Boolean) + .join(' '); + + const btn = h('button', { + 'aria-label': label || null, + class: classes,
6-11:getConfigOptionreturn type isunknown— callers use unsafe casts.The function returns
unknown, forcing callers to useas stringcasts (e.g., line 36). Consider using a generic to preserve type safety:Generic version
-function getConfigOption(option: unknown, step: Step): unknown { +function getConfigOption<T>(option: T | ((...args: unknown[]) => T), step: Step): T { if (isFunction(option)) { - return option.call(step); + return option.call(step) as T; } - return option; + return option as T; }shepherd.js/src/components/shepherd-element.ts (3)
155-177: Focusable-element query runs before the dialog is in the DOM — currently correct but fragile.The
querySelectorAllon Lines 159–162 works because children are already appended toelement. However, the list is never refreshed, so dynamically added focusable elements (e.g., viaupdateStepOptions) won't participate in tab trapping. If this mirrors the prior Svelte behavior, it's acceptable for now, but worth a// TODOnoting the limitation.Also, on Line 168,
attachToElement.tabIndex = 0unconditionally makes the target tabbable. The original is stored, but if the element already had a meaningfultabIndex(e.g.,-1for programmatic-only focus), overriding it to0changes the page's tab order outside the tour. This is likely carried over from the Svelte implementation, so flagging for awareness only.
24-27:cleanup()does not null out closure references — minor memory-leak surface.After
cleanup()is called, the closure still holds references tostep,attachToElement, and the DOM arrays. In the normal flow_teardownElementsalso dropsthis.eland the component reference, so GC should collect the whole graph. No action required unless long-lived step instances are reused without full teardown.Also applies to: 179-183
49-114: Replace deprecatede.keyCodewithe.keystring comparisons.
KeyboardEvent.keyCodehas been deprecated and is not recommended for modern development. Usinge.keyis more readable and future-proof.Proposed refactor
-const KEY_TAB = 9; -const KEY_ESC = 27; -const LEFT_ARROW = 37; -const RIGHT_ARROW = 39; +const KEY_TAB = 'Tab'; +const KEY_ESC = 'Escape'; +const LEFT_ARROW = 'ArrowLeft'; +const RIGHT_ARROW = 'ArrowRight';Then replace
e.keyCodewithe.key:- switch (e.keyCode) { + switch (e.key) {
1 participant
Summary by CodeRabbit
New Features
Style
Tests
Chores