Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR migrates Shepherd.js from Svelte components to vanilla TypeScript/JavaScript by removing all Svelte dependencies, converting component Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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.
| const sharedConfig = { | ||
| input: 'src/shepherd.ts', | ||
| // More aggressive tree shaking | ||
| treeshake: { | ||
| moduleSideEffects: false, | ||
| propertyReadSideEffects: false, | ||
| unknownGlobalSideEffects: false | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if shepherd.ts has CSS imports
rg -n "import.*\.css" --type=ts src/Repository: shipshapecode/shepherd
Length of output: 110
🏁 Script executed:
# Also look at the rollup config to understand the postcss setup
cat -n rollup.config.mjs | grep -A 30 "postcss"Repository: shipshapecode/shepherd
Length of output: 115
🏁 Script executed:
# Check if there are any CSS files in the src directory
fd -e css src/Repository: shipshapecode/shepherd
Length of output: 48
🏁 Script executed:
git ls-files | head -30Repository: shipshapecode/shepherd
Length of output: 646
🏁 Script executed:
find . -name "rollup.config.mjs" -o -name "shepherd.ts" -o -name "*.css" | grep -v node_modules | head -20Repository: shipshapecode/shepherd
Length of output: 642
🏁 Script executed:
cat -n shepherd.js/src/shepherd.ts | head -50Repository: shipshapecode/shepherd
Length of output: 772
🏁 Script executed:
cat -n shepherd.js/rollup.config.mjs | sed -n '1,100p'Repository: shipshapecode/shepherd
Length of output: 3321
🏁 Script executed:
# Search for CSS imports in shepherd.ts
rg "import.*\.css" shepherd.js/src/Repository: shipshapecode/shepherd
Length of output: 129
🏁 Script executed:
# Check if there's a dist or build directory showing successful CSS extraction
find . -path "./shepherd.js/dist*" -name "*.css" -type f 2>/dev/null | head -10Repository: shipshapecode/shepherd
Length of output: 48
🏁 Script executed:
# Check postcss plugin documentation or behavior by looking at package.json versions
cat shepherd.js/package.json | grep -A 5 -B 5 "postcss"Repository: shipshapecode/shepherd
Length of output: 735
🏁 Script executed:
# Search for any tests or configs that might indicate if CSS extraction is working
rg "shepherd\.css" shepherd.js/ --type=json --type=js --type=mjs | head -20Repository: shipshapecode/shepherd
Length of output: 93
🏁 Script executed:
# Check for any CI/test output or build artifacts that show if CSS is actually being extracted
cat shepherd.js/.github/workflows/test.yml | head -50Repository: shipshapecode/shepherd
Length of output: 136
🏁 Script executed:
# Look for any GitHub issue discussions about CSS not being extracted
rg "CSS" shepherd.js/ --type=md | grep -i "extract\|tree"Repository: shipshapecode/shepherd
Length of output: 48
🏁 Script executed:
# Check the actual postcss plugin documentation behavior with moduleSideEffects
web_search
What does rollup-plugin-postcss do with moduleSideEffects false treeshakingRepository: shipshapecode/shepherd
Length of output: 159
🌐 Web query:
rollup-plugin-postcss v4.0.2 moduleSideEffects tree shaking CSS imports
💡 Result:
In rollup-plugin-postcss v4.0.2, a CSS import like:
import "./style.css";is turned into a JS module that performs runtime CSS injection via style-inject (the plugin resolves style-inject/dist/style-inject.es)—i.e., it’s meant to be a side-effect import. [3]
However, v4.0.2 does not explicitly set Rollup’s moduleSideEffects flag for those generated modules, so if you enable aggressive tree-shaking (notably treeshake.moduleSideEffects: false, or presets that imply it), Rollup may drop “empty” imports whose exports aren’t used—this includes side-effect-only CSS imports. Rollup documents that treeshake.moduleSideEffects: false can suppress such imports. [1] Rollup also documents that plugins can prevent this by returning moduleSideEffects: true or "no-treeshake" from transform/load. [2]
What to do
- Don’t set
treeshake.moduleSideEffects: false(leave it at the defaulttrue) if you rely onimport "./x.css"being kept. [1] - Or use a function allowlist so CSS modules are always treated as having side effects, e.g.:
- return
truefor ids ending in.css(and any preprocessor extensions you use), and otherwise defer to your usual rule. [1]
- return
- If you’re authoring/forking the plugin: have the plugin’s
transform()returnmoduleSideEffects: true/"no-treeshake"for CSS modules to make this robust under aggressive treeshaking. [2]
Sources: Rollup treeshake.moduleSideEffects behavior [1], Rollup plugin moduleSideEffects hook return value [2], rollup-plugin-postcss v4.0.2 uses style-inject for injection [3].
🏁 Script executed:
# Find the actual workflow files
find . -name "*.yml" -path "*/.github/workflows/*" | head -10Repository: shipshapecode/shepherd
Length of output: 163
🏁 Script executed:
# Check if there are any build artifacts or test results that show CSS being generated
ls -la shepherd.js/dist/ 2>/dev/null || echo "dist/ not found"Repository: shipshapecode/shepherd
Length of output: 81
🏁 Script executed:
# Check the README or docs for build instructions
cat README.md | grep -A 20 "build"Repository: shipshapecode/shepherd
Length of output: 1882
moduleSideEffects: false will tree-shake side-effect-only CSS imports.
shepherd.ts contains a bare import './components/shepherd.css' (line 4), which is a side-effect-only import. With treeshake.moduleSideEffects: false, Rollup will drop this import because rollup-plugin-postcss v4.0.2 does not explicitly mark generated CSS modules as having side effects. This will result in missing styles in the final bundle.
Fix by using a function allowlist to preserve CSS imports:
Suggested fix
treeshake: {
- moduleSideEffects: false,
+ moduleSideEffects(id) {
+ return id.endsWith('.css');
+ },
propertyReadSideEffects: false,
unknownGlobalSideEffects: false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sharedConfig = { | |
| input: 'src/shepherd.ts', | |
| // More aggressive tree shaking | |
| treeshake: { | |
| moduleSideEffects: false, | |
| propertyReadSideEffects: false, | |
| unknownGlobalSideEffects: false | |
| } | |
| }; | |
| const sharedConfig = { | |
| input: 'src/shepherd.ts', | |
| // More aggressive tree shaking | |
| treeshake: { | |
| moduleSideEffects(id) { | |
| return id.endsWith('.css'); | |
| }, | |
| propertyReadSideEffects: false, | |
| unknownGlobalSideEffects: false | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@shepherd.js/rollup.config.mjs` around lines 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.
| // CJS build | ||
| { | ||
| ...sharedConfig, | ||
| output: { | ||
| dir: 'tmp/cjs', | ||
| entryFileNames: '[name].cjs', | ||
| format: 'cjs', | ||
| sourcemap: true, | ||
| exports: 'named' | ||
| }, | ||
| plugins: [ | ||
| ...plugins, | ||
| { | ||
| name: 'After CJS build tweaks', | ||
| closeBundle: async () => { | ||
| await execaCommand(`mkdir -p ./dist/cjs`, { | ||
| stdio: 'inherit' | ||
| }); | ||
|
|
||
| console.log('Move shepherd.cjs from tmp to dist'); | ||
|
|
||
| await execaCommand( | ||
| `mv ./tmp/cjs/shepherd.cjs ./tmp/cjs/shepherd.cjs.map ./dist/cjs/`, | ||
| { | ||
| stdio: 'inherit' | ||
| } | ||
| ); | ||
|
|
||
| console.log('Copy CJS declarations'); | ||
|
|
||
| await execaCommand( | ||
| `cp ./dist/js/shepherd.d.mts ./dist/cjs/shepherd.d.cts`, | ||
| { | ||
| stdio: 'inherit' | ||
| } | ||
| ); | ||
| } | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
CJS closeBundle depends on ESM build artifacts — fragile ordering assumption.
Line 183 copies ./dist/js/shepherd.d.mts (produced by the ESM closeBundle) to ./dist/cjs/shepherd.d.cts. This only works because Rollup processes array entries sequentially. If the build is ever parallelized (e.g., via --parallel or a different build orchestrator), this will fail silently or error out.
Consider adding a guard or a comment documenting the dependency.
🤖 Prompt for AI Agents
In `@shepherd.js/rollup.config.mjs` around lines 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.
| 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.
| if (targetIframe) { | ||
| const rect = targetIframe.getBoundingClientRect(); | ||
| offset.top += | ||
| rect.top + ((rect as unknown as Record<string, number>)['scrollTop'] ?? 0); | ||
| offset.left += | ||
| rect.left + ((rect as unknown as Record<string, number>)['scrollLeft'] ?? 0); | ||
| } |
There was a problem hiding this comment.
scrollTop/scrollLeft on DOMRect is always undefined — dead code.
getBoundingClientRect() returns a DOMRect which has no scrollTop or scrollLeft properties, so the ?? 0 fallback always applies. If the intent was to account for scroll offset of the iframe element, you'd need to read it from the element itself (e.g., targetIframe.scrollTop), not from its bounding rect.
🤖 Prompt for AI Agents
In `@shepherd.js/src/components/shepherd-modal.ts` around lines 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.
| if (isHTMLElement(text)) { | ||
| el.appendChild(text); | ||
| } else { | ||
| el.innerHTML = text as string; | ||
| } |
There was a problem hiding this comment.
innerHTML usage is intentional but warrants a defensive guard for non-string values.
The static analysis XSS warning is acknowledged — this is developer-supplied config, not end-user input, and the library has historically supported HTML content in step text. However, if text is undefined or null after the function check (e.g., the function returned undefined), this will render the literal string "undefined" or "null".
Proposed defensive guard
if (isHTMLElement(text)) {
el.appendChild(text);
- } else {
+ } else if (text) {
el.innerHTML = text as string;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isHTMLElement(text)) { | |
| el.appendChild(text); | |
| } else { | |
| el.innerHTML = text as string; | |
| } | |
| if (isHTMLElement(text)) { | |
| el.appendChild(text); | |
| } else if (text) { | |
| el.innerHTML = text as string; | |
| } |
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 22-22: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: el.innerHTML = text as string
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 22-22: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: el.innerHTML = text as string
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
In `@shepherd.js/src/components/shepherd-text.ts` around lines 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.
| updateStepOptions(options: StepOptions) { | ||
| Object.assign(this.options, options); | ||
|
|
||
| // @ts-expect-error TODO: get types for Svelte components | ||
| if (this.shepherdElementComponent) { | ||
| // @ts-expect-error TODO: get types for Svelte components | ||
| this.shepherdElementComponent.$set({ step: this }); | ||
| // Recreate the element with updated options | ||
| if (this.el) { | ||
| this.destroy(); | ||
| this._setupElements(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the destroy() method emits the 'destroy' event
rg -n "trigger\('destroy'\)|trigger\(\"destroy\"\)" -A2 -B2 src/Repository: shipshapecode/shepherd
Length of output: 110
🏁 Script executed:
# Check for listeners on the 'destroy' event
rg -n "\.on\('destroy'|\.on\(\"destroy\"|'destroy':|\"destroy\":" src/ --type ts --type js -C2Repository: shipshapecode/shepherd
Length of output: 110
🏁 Script executed:
# Verify the updateStepOptions implementation
rg -n "updateStepOptions" src/ -A10 --type tsRepository: shipshapecode/shepherd
Length of output: 110
🏁 Script executed:
git ls-files | head -20Repository: shipshapecode/shepherd
Length of output: 408
🏁 Script executed:
fd -t f "step\.ts" .Repository: shipshapecode/shepherd
Length of output: 91
🏁 Script executed:
# Verify the destroy() method emits 'destroy' event
rg -n "trigger\('destroy'\)|trigger\(\"destroy\"\)" shepherd.js/src/ -A2 -B2 --type tsRepository: shipshapecode/shepherd
Length of output: 278
🏁 Script executed:
# Check for listeners on the 'destroy' event
rg -n "\.on\('destroy'|\.on\(\"destroy\"|addEventListener.*destroy|'destroy':" shepherd.js/ -C2 --type ts --type jsRepository: shipshapecode/shepherd
Length of output: 1106
🏁 Script executed:
# Verify the updateStepOptions implementation
rg -n "updateStepOptions" shepherd.js/src/step.ts -A15 --type tsRepository: shipshapecode/shepherd
Length of output: 585
🏁 Script executed:
# Check _setupElements() implementation
rg -n "_setupElements" shepherd.js/src/step.ts -A20 --type tsRepository: shipshapecode/shepherd
Length of output: 1776
🏁 Script executed:
# Check if there are tests for updateStepOptions
rg -n "updateStepOptions" shepherd.js/test/ -A10 -B5 --type jsRepository: shipshapecode/shepherd
Length of output: 6625
🏁 Script executed:
# Check the bind.ts implementation to understand event handler cleanup
cat -n shepherd.js/src/utils/bind.tsRepository: shipshapecode/shepherd
Length of output: 2262
updateStepOptions triggers a spurious destroy event that breaks the semantic contract of the event.
When updateStepOptions() calls destroy() at line 480, it emits a destroy event (line 390) that signals permanent step destruction. However, this is an internal update operation, not actual destruction. Consumers listening for destroy events (e.g., bind.ts lines 50, 55 for event handler cleanup, or custom listeners for analytics) receive false signals and cannot distinguish between an actual destruction and an intermediate update step.
While event handlers are functionally re-attached via bindAdvance() in the subsequent _setupElements() call, the semantic contract is broken. This represents a behavioral change from the previous Svelte $set approach that updated the component in-place without any destroy/recreate cycle.
Consider extracting the teardown logic into a private method that doesn't emit the destroy event, or suppress the event emission during option updates.
Proposed fix: extract internal teardown without event emission
+ /**
+ * Tears down the element and tooltip without emitting 'destroy'.
+ * `@private`
+ */
+ _teardownElements() {
+ destroyTooltip(this);
+
+ if (this.shepherdElementComponent) {
+ this.shepherdElementComponent.cleanup();
+ this.shepherdElementComponent = undefined;
+ }
+
+ if (isHTMLElement(this.el)) {
+ this.el.remove();
+ this.el = null;
+ }
+
+ this._updateStepTargetOnHide();
+ }
+
/**
* Remove the step, delete the step's element, and destroy the FloatingUI instance for the step.
* Triggers `destroy` event
*/
destroy() {
- destroyTooltip(this);
-
- if (this.shepherdElementComponent) {
- this.shepherdElementComponent.cleanup();
- this.shepherdElementComponent = undefined;
- }
-
- if (isHTMLElement(this.el)) {
- this.el.remove();
- this.el = null;
- }
-
- this._updateStepTargetOnHide();
+ this._teardownElements();
this._originalTabIndexes.clear();
-
this.trigger('destroy');
}Then in updateStepOptions:
updateStepOptions(options: StepOptions) {
Object.assign(this.options, options);
if (this.shepherdElementComponent) {
if (this.el) {
- this.destroy();
+ this._teardownElements();
this._setupElements();
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| updateStepOptions(options: StepOptions) { | |
| Object.assign(this.options, options); | |
| // @ts-expect-error TODO: get types for Svelte components | |
| if (this.shepherdElementComponent) { | |
| // @ts-expect-error TODO: get types for Svelte components | |
| this.shepherdElementComponent.$set({ step: this }); | |
| // Recreate the element with updated options | |
| if (this.el) { | |
| this.destroy(); | |
| this._setupElements(); | |
| } | |
| } | |
| updateStepOptions(options: StepOptions) { | |
| Object.assign(this.options, options); | |
| if (this.shepherdElementComponent) { | |
| // Recreate the element with updated options | |
| if (this.el) { | |
| this._teardownElements(); | |
| this._setupElements(); | |
| } | |
| } | |
| } | |
| /** | |
| * Tears down the element and tooltip without emitting 'destroy'. | |
| * `@private` | |
| */ | |
| _teardownElements() { | |
| destroyTooltip(this); | |
| if (this.shepherdElementComponent) { | |
| this.shepherdElementComponent.cleanup(); | |
| this.shepherdElementComponent = undefined; | |
| } | |
| if (isHTMLElement(this.el)) { | |
| this.el.remove(); | |
| this.el = null; | |
| } | |
| this._updateStepTargetOnHide(); | |
| } | |
| /** | |
| * Remove the step, delete the step's element, and destroy the FloatingUI instance for the step. | |
| * Triggers `destroy` event | |
| */ | |
| destroy() { | |
| this._teardownElements(); | |
| this._originalTabIndexes.clear(); | |
| this.trigger('destroy'); | |
| } |
🤖 Prompt for AI Agents
In `@shepherd.js/src/step.ts` around lines 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.
| describe('disabled', () => { | ||
| it('should be enabled by default', () => { | ||
| const config = {}; | ||
| const button = createShepherdButton({}, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button.disabled).toBeFalsy(); | ||
| }); | ||
|
|
||
| it('is enabled when false', () => { | ||
| const config = { | ||
| disabled: false | ||
| }; | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
| const button = createShepherdButton({ disabled: false }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button.disabled).toBeFalsy(); | ||
| }); | ||
|
|
||
| it('can be disabled with boolean', () => { | ||
| const config = { | ||
| disabled: true | ||
| }; | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
| const button = createShepherdButton({ disabled: true }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button.disabled).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('can be disabled with function', () => { | ||
| const config = { | ||
| disabled: () => true | ||
| }; | ||
| const button = createShepherdButton({ disabled: () => true }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button.disabled).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('label - string', () => { | ||
| const config = { | ||
| label: 'Test' | ||
| }; | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
| const button = createShepherdButton({ label: 'Test' }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button).toHaveAttribute('aria-label', 'Test'); | ||
| }); | ||
|
|
||
| it('label - number', () => { | ||
| const config = { | ||
| label: 5 | ||
| }; | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
| const button = createShepherdButton({ label: 5 }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button).toHaveAttribute('aria-label', '5'); | ||
| }); | ||
|
|
||
| it('label - funtion', async () => { | ||
| it('label - function', () => { | ||
| const button = createShepherdButton({ label: () => 'Test' }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| expect(button).toHaveAttribute('aria-label', 'Test'); | ||
| }); | ||
|
|
||
| it('label - function re-creation uses updated value', () => { | ||
| let label = 'Test'; | ||
| const labelFunction = () => label; | ||
| const config = { | ||
| label: labelFunction | ||
| }; | ||
|
|
||
| const { container, rerender } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| const button = createShepherdButton({ label: () => label }, undefined); | ||
| container.appendChild(button); | ||
| expect(button).toHaveAttribute('aria-label', 'Test'); | ||
|
|
||
| label = 'Test 2'; | ||
|
|
||
| rerender({ | ||
| config: { label: () => label } | ||
| }); | ||
|
|
||
| await tick(); | ||
|
|
||
| const buttonUpdated = container.querySelector('.shepherd-button'); | ||
| const buttonUpdated = createShepherdButton({ label: () => label }, undefined); | ||
| container.appendChild(buttonUpdated); | ||
| expect(buttonUpdated).toHaveAttribute('aria-label', 'Test 2'); | ||
| }); | ||
|
|
||
| it('label - null', () => { | ||
| const config = { | ||
| label: null | ||
| }; | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
| const button = createShepherdButton({ label: null }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button).not.toHaveAttribute('aria-label'); | ||
| }); | ||
|
|
||
| it('label - undefined', () => { | ||
| const config = {}; | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
| const button = createShepherdButton({}, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button).not.toHaveAttribute('aria-label'); | ||
| }); | ||
|
|
||
| it('text - string', () => { | ||
| const config = { | ||
| text: 'Test' | ||
| }; | ||
| const button = createShepherdButton({ text: 'Test' }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const { container } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
| expect(button).toHaveTextContent('Test'); | ||
| }); | ||
|
|
||
| it('text - function', () => { | ||
| const button = createShepherdButton({ text: () => 'Test' }, undefined); | ||
| container.appendChild(button); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| expect(button).toHaveTextContent('Test'); | ||
| }); | ||
|
|
||
| it('text - function', async () => { | ||
| it('text - function re-creation uses updated value', () => { | ||
| let text = 'Test'; | ||
| const textFunction = () => text; | ||
| const config = { | ||
| text: textFunction | ||
| }; | ||
|
|
||
| const { container, rerender } = render(ShepherdButton, { | ||
| props: { | ||
| config | ||
| } | ||
| }); | ||
|
|
||
| const button = container.querySelector('.shepherd-button'); | ||
| const button = createShepherdButton({ text: () => text }, undefined); | ||
| container.appendChild(button); | ||
| expect(button).toHaveTextContent('Test'); | ||
|
|
||
| text = 'Test 2'; | ||
|
|
||
| rerender({ | ||
| config: { text: () => text } | ||
| }); | ||
|
|
||
| await tick(); | ||
|
|
||
| const buttonUpdated = container.querySelector('.shepherd-button'); | ||
| const buttonUpdated = createShepherdButton({ text: () => text }, undefined); | ||
| container.appendChild(buttonUpdated); | ||
| expect(buttonUpdated).toHaveTextContent('Test 2'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Label and text tests are incorrectly nested under the describe('disabled', ...) block.
All label tests (lines 45–76) and text tests (lines 92–116) are inside the describe('disabled', ...) block that starts at line 16 and closes at line 117. This appears to be a copy/restructure oversight — these tests should be in their own describe blocks at the same level.
Proposed structural fix
describe('disabled', () => {
// ... disabled tests (lines 17-43) ...
+ });
+ describe('label', () => {
it('label - string', () => {
// ...
});
// ... other label tests ...
+ });
+ describe('text', () => {
it('text - string', () => {
// ...
});
// ... other text tests ...
- });
+ });🤖 Prompt for AI Agents
In `@shepherd.js/test/unit/components/shepherd-button.spec.js` around lines 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.
1 participant
Summary by CodeRabbit
Release Notes
Refactor
New Features
Chores