chore: release 0.3.0#1
Conversation
📝 WalkthroughWalkthroughRestructures the tour library: extracts core service into src/core, adds DOM and overlay utilities (positioning, scroll manager, deepQuery, focus manager, step runner), refactors TourOverlay to use the new orchestrator, updates exports, bumps version to 0.3.0, and expands tests and CI coverage config. Changes
Sequence Diagram(s)mermaid Note: colored rectangles not required for this simple flow. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
73-78:⚠️ Potential issue | 🟠 MajorAlign
@vitest/coverage-v8andvitestversions.
@vitest/coverage-v8@3.2.4andvitest@3.0.0have a version mismatch. The Vitest coverage plugin maintains strict peer dependencies with the mainvitestpackage and requires closely aligned versions. Updatevitestto at least^3.2.4to match@vitest/coverage-v8, or vice versa, to avoid runtime compatibility issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 73 - 78, The dependency versions for `@vitest/coverage-v8` and vitest are misaligned; update one so they match (e.g., set "vitest" to "^3.2.4" to align with "@vitest/coverage-v8": "^3.2.4" or downgrade the coverage package to the vitest version), then run your package manager to update the lockfile and reinstall, and run the test suite to verify compatibility; target the package.json entries for "@vitest/coverage-v8" and "vitest" when making the change.
🧹 Nitpick comments (8)
src/dom/deep-query.ts (1)
11-14: Consider usingShadowRoottype instead of casting toDocument.The cast
as unknown as Documentworks but obscures the actual type.ShadowRoothasquerySelectorand can be used directly with a union type.♻️ Suggested type refinement
export function deepQuery( selector: string, - root: Element | Document = document.body, + root: Element | Document | ShadowRoot = document.body, ): Element | null { const found = root.querySelector(selector); if (found) return found; const children = root.querySelectorAll('*'); for (const element of children) { if (element.shadowRoot) { - const shadowResult = deepQuery( - selector, - element.shadowRoot as unknown as Document, - ); + const shadowResult = deepQuery(selector, element.shadowRoot); if (shadowResult) return shadowResult; } } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/deep-query.ts` around lines 11 - 14, The code is casting element.shadowRoot to Document for deepQuery; instead use the actual ShadowRoot type and update deepQuery's parameter types to accept both Document and ShadowRoot. Change the call to pass element.shadowRoot as ShadowRoot (or guard with if (element.shadowRoot) ...) and modify deepQuery's signature (and any related overloads) to accept Document | ShadowRoot so querySelector is correctly typed for shadowResult and no unsafe cast is needed.src/dom/positioning.ts (1)
69-77: Consider extracting magic number80as a named constant.The value
80appears twice (lines 71 and 76) for vertical centering offset inleft/rightplacements. A named constant likeTOOLTIP_VERTICAL_OFFSETwould improve readability and maintainability.♻️ Proposed refactor
export const TOOLTIP_W = 320; export const TOOLTIP_H_MAX = 270; export const GAP = 16; export const VIEWPORT_MARGIN = 24; +const TOOLTIP_VERTICAL_OFFSET = 80; // ... in getTooltipPosition: case 'right': return { - top: visibleCenterY - 80, + top: visibleCenterY - TOOLTIP_VERTICAL_OFFSET, left: rect.right + spotlightPadding + GAP, }; case 'left': return { - top: visibleCenterY - 80, + top: visibleCenterY - TOOLTIP_VERTICAL_OFFSET, left: rect.left - spotlightPadding - GAP - TOOLTIP_W, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/positioning.ts` around lines 69 - 77, The vertical offset value 80 used in the 'right' and 'left' placement cases should be extracted to a named constant (e.g., TOOLTIP_VERTICAL_OFFSET) to avoid magic numbers; replace both occurrences in the positioning logic where top is computed from visibleCenterY (currently "visibleCenterY - 80") with "visibleCenterY - TOOLTIP_VERTICAL_OFFSET", declare the constant near the other layout constants (alongside TOOLTIP_W, GAP, spotlightPadding) and ensure any exports or local scope match the file's pattern.src/dom/scroll-manager.ts (1)
20-36: Consider thatfitsInViewportcheck uses pre-scroll rect position.The
fitsInViewportcheck on line 23 usesrectobtained before any scrolling occurs (line 20). This should be fine since we're checking if the element's height fits within the viewport (which doesn't change with scrolling), but the variable namerectmight be slightly misleading as it represents the initial position.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/scroll-manager.ts` around lines 20 - 36, The pre-scroll bounding rect variable is misleadingly named `rect` in the scroll logic; rename it to `initialRect` or `elementRect` (the result of element.getBoundingClientRect()) and add a short comment near fitsInViewport usage to clarify this is the pre-scroll measurement used only to check element height vs viewport, leaving the rest of logic (scrollIntoView, desiredTop calculation using `placement`, `TOOLTIP_H_MAX`, `GAP`, `spotlightPadding`, and window.scrollTo) unchanged so it’s clear you aren’t relying on a post-scroll rect.src/core/tour-service.ts (1)
144-159: Consider whetheronSkipshould be called before clearingactiveTourId.Currently
tour?.onSkip?.()is called on line 158 afteractiveTourIdis set to null (line 155) andnotify()is called (line 157). If theonSkipcallback queriesisActive()orgetSnapshot(), it will see the tour as already inactive. This may or may not be the intended behavior—verify if callbacks should have access to the tour state before it's cleared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/tour-service.ts` around lines 144 - 159, The skipTour method currently invokes tour?.onSkip?.() after clearing activeTourId, resetting currentStepIndex and calling notify(), so any onSkip callback that calls isActive() or getSnapshot() will observe the tour as inactive; move the tour?.onSkip?.() invocation to before clearing activeTourId/currentStepIndex and before notify(), ensuring the callback can see the active state, while keeping the logic that pushes id into persistedState.dismissed, calls saveState(), then calls onSkip, then clears activeTourId and currentStepIndex and finally calls notify(); update the skipTour function (referenced by name) accordingly.test/positioning.test.ts (1)
61-67: Test assertions rely on specific constant values.The expected values
{ top: 166, left: 100 }and{ top: 24, left: 680 }are derived from the positioning constants. IfGAP,TOOLTIP_W,VIEWPORT_MARGIN, orTOOLTIP_H_MAXchange, these tests will fail. Consider documenting the calculation or importing constants to make the relationship explicit.For reference:
top: 166=100 (rect.top) + 40 (rect.height) + 10 (padding) + 16 (GAP)= 166 ✓left: 100=200 + 60 - 160= 100 ✓🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/positioning.test.ts` around lines 61 - 67, Replace hard-coded expected numbers with values computed from the same positioning constants so tests don’t break when GAP/TOOLTIP_W/VIEWPORT_MARGIN/TOOLTIP_H_MAX change: in the test file import the relevant constants (e.g., GAP, TOOLTIP_W, VIEWPORT_MARGIN, TOOLTIP_H_MAX) and compute expectedPosition using the same math used by getTooltipPosition (e.g., top = rect.top + rect.height + padding + GAP; left = rect.left + rect.width/2 - TOOLTIP_W/2) and compute expectedClamped by applying clampToViewport to the computed coordinates (or using VIEWPORT_MARGIN and TOOLTIP_W/TOOLTIP_H_MAX to derive the clamped values), then assert against those computed expectations instead of literal numbers.test/step-runner.test.ts (1)
58-62: Address Biome lint warning for forEach callback.The static analysis tool flags that the
forEachcallback should not return a value. Whileelement.remove()returnsvoid, adding explicit braces clarifies intent and silences the linter.🔧 Proposed fix
afterEach(() => { - cleanup.forEach(element => element.remove()); + cleanup.forEach(element => { element.remove(); }); cleanup.length = 0; vi.restoreAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/step-runner.test.ts` around lines 58 - 62, The linter warns about the arrow callback in the afterEach block returning a value; update the afterEach cleanup to use an explicit block-bodied callback so the forEach callback does not implicitly return anything — e.g., replace the current cleanup.forEach(element => element.remove()) call in the afterEach function with a block-bodied callback that calls element.remove(); (keep the rest of the teardown, including cleanup.length = 0 and vi.restoreAllMocks()).test/tour-overlay.test.ts (1)
68-72: Address Biome lint warning for forEach callback.Same pattern as in
test/step-runner.test.ts. Adding explicit braces clarifies intent.🔧 Proposed fix
afterEach(() => { - cleanup.forEach(element => element.remove()); + cleanup.forEach(element => { element.remove(); }); cleanup.length = 0; vi.restoreAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tour-overlay.test.ts` around lines 68 - 72, The Biome lint warning is caused by the concise arrow callback used in the afterEach cleanup; update the afterEach block so the forEach callback uses an explicit block body instead of an expression. Locate the afterEach function (symbol: afterEach) and change the cleanup.forEach callback to use braces (i.e., element => { element.remove(); }) while keeping the subsequent cleanup.length = 0 and vi.restoreAllMocks() lines unchanged.src/overlay/step-runner.ts (1)
93-111: Consider extracting magic numbers to named constants.The values
270,32, and16appear to represent tooltip dimensions and spacing but are not self-documenting. Consider extracting them to named constants for clarity and consistency with theGAP/VIEWPORT_MARGINconstants used elsewhere in the codebase.+const TOOLTIP_HEIGHT_ESTIMATE = 270; +const TOOLTIP_PADDING = 32; +const TOOLTIP_MARGIN = 16; + private shouldScrollIntoView(snapshot: ResolvedTourSnapshot): boolean { const rect = snapshot.targetRect; if (!rect) return false; const viewportHeight = window.innerHeight; const fits = - rect.height + 270 + 32 < viewportHeight; + rect.height + TOOLTIP_HEIGHT_ESTIMATE + TOOLTIP_PADDING < viewportHeight; const inView = fits ? rect.top >= 0 && rect.bottom <= viewportHeight && rect.left >= 0 && rect.right <= window.innerWidth : snapshot.step.placement === 'top' - ? rect.top >= 270 + 16 + this.options.spotlightPadding && + ? rect.top >= TOOLTIP_HEIGHT_ESTIMATE + TOOLTIP_MARGIN + this.options.spotlightPadding && rect.top < viewportHeight : rect.top >= 0 && rect.top < viewportHeight; return !inView; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/overlay/step-runner.ts` around lines 93 - 111, In shouldScrollIntoView, replace the magic numbers 270, 32, and 16 with descriptive named constants (e.g., TOOLTIP_HEIGHT, TOOLTIP_MARGIN/BOTTOM_PADDING, SPOTLIGHT_TOP_OFFSET or reuse existing GAP/VIEWPORT_MARGIN constants) declared near the top of the module so their meaning is clear; update the calculations (the fits check and the top comparisons) to use those constants instead of raw literals and ensure any combined expressions (like 270 + 16 + this.options.spotlightPadding) are expressed with named constants to improve readability and maintain consistency with other constants used in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/dom/positioning.ts`:
- Around line 84-88: The 'top' placement currently sets top to rect.top -
spotlightPadding - GAP which positions the tooltip's top edge too low; update
the calculation in the switch case handling 'top' so top = rect.top -
spotlightPadding - GAP - TOOLTIP_H_MAX (i.e., subtract TOOLTIP_H_MAX), keeping
the left logic unchanged; this aligns with how clampToViewport reserves space
for TOOLTIP_H_MAX and avoids overlap with the target (refer to TOOLTIP_W,
TOOLTIP_H_MAX, spotlightPadding, GAP, and the clampToViewport usage).
---
Outside diff comments:
In `@package.json`:
- Around line 73-78: The dependency versions for `@vitest/coverage-v8` and vitest
are misaligned; update one so they match (e.g., set "vitest" to "^3.2.4" to
align with "@vitest/coverage-v8": "^3.2.4" or downgrade the coverage package to
the vitest version), then run your package manager to update the lockfile and
reinstall, and run the test suite to verify compatibility; target the
package.json entries for "@vitest/coverage-v8" and "vitest" when making the
change.
---
Nitpick comments:
In `@src/core/tour-service.ts`:
- Around line 144-159: The skipTour method currently invokes tour?.onSkip?.()
after clearing activeTourId, resetting currentStepIndex and calling notify(), so
any onSkip callback that calls isActive() or getSnapshot() will observe the tour
as inactive; move the tour?.onSkip?.() invocation to before clearing
activeTourId/currentStepIndex and before notify(), ensuring the callback can see
the active state, while keeping the logic that pushes id into
persistedState.dismissed, calls saveState(), then calls onSkip, then clears
activeTourId and currentStepIndex and finally calls notify(); update the
skipTour function (referenced by name) accordingly.
In `@src/dom/deep-query.ts`:
- Around line 11-14: The code is casting element.shadowRoot to Document for
deepQuery; instead use the actual ShadowRoot type and update deepQuery's
parameter types to accept both Document and ShadowRoot. Change the call to pass
element.shadowRoot as ShadowRoot (or guard with if (element.shadowRoot) ...) and
modify deepQuery's signature (and any related overloads) to accept Document |
ShadowRoot so querySelector is correctly typed for shadowResult and no unsafe
cast is needed.
In `@src/dom/positioning.ts`:
- Around line 69-77: The vertical offset value 80 used in the 'right' and 'left'
placement cases should be extracted to a named constant (e.g.,
TOOLTIP_VERTICAL_OFFSET) to avoid magic numbers; replace both occurrences in the
positioning logic where top is computed from visibleCenterY (currently
"visibleCenterY - 80") with "visibleCenterY - TOOLTIP_VERTICAL_OFFSET", declare
the constant near the other layout constants (alongside TOOLTIP_W, GAP,
spotlightPadding) and ensure any exports or local scope match the file's
pattern.
In `@src/dom/scroll-manager.ts`:
- Around line 20-36: The pre-scroll bounding rect variable is misleadingly named
`rect` in the scroll logic; rename it to `initialRect` or `elementRect` (the
result of element.getBoundingClientRect()) and add a short comment near
fitsInViewport usage to clarify this is the pre-scroll measurement used only to
check element height vs viewport, leaving the rest of logic (scrollIntoView,
desiredTop calculation using `placement`, `TOOLTIP_H_MAX`, `GAP`,
`spotlightPadding`, and window.scrollTo) unchanged so it’s clear you aren’t
relying on a post-scroll rect.
In `@src/overlay/step-runner.ts`:
- Around line 93-111: In shouldScrollIntoView, replace the magic numbers 270,
32, and 16 with descriptive named constants (e.g., TOOLTIP_HEIGHT,
TOOLTIP_MARGIN/BOTTOM_PADDING, SPOTLIGHT_TOP_OFFSET or reuse existing
GAP/VIEWPORT_MARGIN constants) declared near the top of the module so their
meaning is clear; update the calculations (the fits check and the top
comparisons) to use those constants instead of raw literals and ensure any
combined expressions (like 270 + 16 + this.options.spotlightPadding) are
expressed with named constants to improve readability and maintain consistency
with other constants used in the file.
In `@test/positioning.test.ts`:
- Around line 61-67: Replace hard-coded expected numbers with values computed
from the same positioning constants so tests don’t break when
GAP/TOOLTIP_W/VIEWPORT_MARGIN/TOOLTIP_H_MAX change: in the test file import the
relevant constants (e.g., GAP, TOOLTIP_W, VIEWPORT_MARGIN, TOOLTIP_H_MAX) and
compute expectedPosition using the same math used by getTooltipPosition (e.g.,
top = rect.top + rect.height + padding + GAP; left = rect.left + rect.width/2 -
TOOLTIP_W/2) and compute expectedClamped by applying clampToViewport to the
computed coordinates (or using VIEWPORT_MARGIN and TOOLTIP_W/TOOLTIP_H_MAX to
derive the clamped values), then assert against those computed expectations
instead of literal numbers.
In `@test/step-runner.test.ts`:
- Around line 58-62: The linter warns about the arrow callback in the afterEach
block returning a value; update the afterEach cleanup to use an explicit
block-bodied callback so the forEach callback does not implicitly return
anything — e.g., replace the current cleanup.forEach(element =>
element.remove()) call in the afterEach function with a block-bodied callback
that calls element.remove(); (keep the rest of the teardown, including
cleanup.length = 0 and vi.restoreAllMocks()).
In `@test/tour-overlay.test.ts`:
- Around line 68-72: The Biome lint warning is caused by the concise arrow
callback used in the afterEach cleanup; update the afterEach block so the
forEach callback uses an explicit block body instead of an expression. Locate
the afterEach function (symbol: afterEach) and change the cleanup.forEach
callback to use braces (i.e., element => { element.remove(); }) while keeping
the subsequent cleanup.length = 0 and vi.restoreAllMocks() lines unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da87776b-f2d9-483e-bf14-b68dd4707864
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
.gitignoreREADME.mdpackage.jsonsrc/core/tour-service.tssrc/core/types.tssrc/dom/deep-query.tssrc/dom/positioning.tssrc/dom/scroll-manager.tssrc/dom/target-resolver.tssrc/index.tssrc/overlay/focus-manager.tssrc/overlay/step-runner.tssrc/overlay/types.tssrc/tour-overlay.tssrc/tour-service.tssrc/types.tssrc/utils/deep-query.tstest/declaration-surface.test.tstest/deep-query.test.tstest/positioning.test.tstest/step-runner.test.tstest/tour-overlay.test.tstest/tour-service.test.tsvitest.config.ts
| case 'top': | ||
| return { | ||
| top: rect.top - spotlightPadding - GAP, | ||
| left: rect.left + rect.width / 2 - TOOLTIP_W / 2, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's CSS that positions the tooltip from bottom edge for 'top' placement
rg -n "arrow-top|placement.*top" --type css --type ts -A 3 -B 3Repository: barryking/torchlit
Length of output: 2656
🏁 Script executed:
sed -n '80,105p' src/dom/positioning.tsRepository: barryking/torchlit
Length of output: 787
Subtract TOOLTIP_H_MAX from top placement calculation.
The top coordinate for top placement should account for the tooltip's height. The current code rect.top - spotlightPadding - GAP positions the tooltip's top edge at that coordinate, causing it to overlap the target. The clampToViewport function treats pos.top as the tooltip's top edge and reserves space for TOOLTIP_H_MAX below it, confirming the tooltip height must be subtracted in the initial positioning.
Suggested fix
case 'top':
return {
- top: rect.top - spotlightPadding - GAP,
+ top: rect.top - spotlightPadding - GAP - TOOLTIP_H_MAX,
left: rect.left + rect.width / 2 - TOOLTIP_W / 2,
};📝 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.
| case 'top': | |
| return { | |
| top: rect.top - spotlightPadding - GAP, | |
| left: rect.left + rect.width / 2 - TOOLTIP_W / 2, | |
| }; | |
| case 'top': | |
| return { | |
| top: rect.top - spotlightPadding - GAP - TOOLTIP_H_MAX, | |
| left: rect.left + rect.width / 2 - TOOLTIP_W / 2, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dom/positioning.ts` around lines 84 - 88, The 'top' placement currently
sets top to rect.top - spotlightPadding - GAP which positions the tooltip's top
edge too low; update the calculation in the switch case handling 'top' so top =
rect.top - spotlightPadding - GAP - TOOLTIP_H_MAX (i.e., subtract
TOOLTIP_H_MAX), keeping the left logic unchanged; this aligns with how
clampToViewport reserves space for TOOLTIP_H_MAX and avoids overlap with the
target (refer to TOOLTIP_W, TOOLTIP_H_MAX, spotlightPadding, GAP, and the
clampToViewport usage).
There was a problem hiding this comment.
Pull request overview
This PR releases Torchlit 0.3.0 by restructuring the library into a DOM-free core service (src/core) with DOM/overlay behavior moved into dedicated modules, while keeping the documented public entrypoints (torchlit, torchlit/service, torchlit/overlay).
Changes:
- Extracted the headless tour state engine into
src/coreand moved DOM-specific behaviors intosrc/domandsrc/overlay. - Refactored
TorchlitOverlayto compose newStepRunner,FocusManager, and DOM helpers instead of owning all orchestration logic. - Added/updated Vitest coverage + new regression/type-surface tests; bumped version to
0.3.0.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Enables v8 coverage reporting for src/** and outputs text/lcov. |
| test/tour-service.test.ts | Updates service tests to run in Node and validate DOM-free behavior + persistence via injected storage. |
| test/tour-overlay.test.ts | Updates overlay regressions for new orchestration (service attach, scroll restore, cached targets). |
| test/step-runner.test.ts | Adds unit tests for step preparation (beforeShow, route events, lazy targets, auto-advance). |
| test/positioning.test.ts | Adds unit tests for extracted positioning helpers. |
| test/deep-query.test.ts | Updates deepQuery tests to new module location and expands shadow DOM coverage. |
| test/declaration-surface.test.ts | Asserts torchlit/service declarations don’t reference lit. |
| src/utils/deep-query.ts | Keeps a compatibility re-export to the new DOM deepQuery implementation. |
| src/types.ts | Narrows “public” types by re-exporting core service types and keeping overlay-facing step/definition types. |
| src/tour-service.ts | Converts torchlit/service entrypoint to a thin re-export of the new core service. |
| src/tour-overlay.ts | Refactors overlay into a thinner composition layer using new DOM + overlay helper modules. |
| src/overlay/types.ts | Introduces ResolvedTourSnapshot (core snapshot + tour + resolved DOM target info). |
| src/overlay/step-runner.ts | Adds step orchestration (hooks, route event, lazy targets, scroll-into-view decisions). |
| src/overlay/focus-manager.ts | Extracts focus capture/restore and focus trap logic from the overlay. |
| src/index.ts | Keeps root barrel exports stable while pointing deepQuery at the new module. |
| src/dom/target-resolver.ts | Adds DOM target lookup + lazy target waiting via MutationObserver. |
| src/dom/scroll-manager.ts | Extracts scroll-into-view and end-of-tour scroll restoration logic. |
| src/dom/positioning.ts | Extracts placement, tooltip positioning, viewport clamping, and arrow positioning helpers. |
| src/dom/deep-query.ts | Moves deepQuery into DOM module. |
| package.json | Bumps version to 0.3.0 and adds @vitest/coverage-v8. |
| pnpm-lock.yaml | Locks @vitest/coverage-v8 and related dependency graph changes. |
| README.md | Updates docs to describe the new core/dom/overlay structure and DOM-free service entrypoint. |
| .gitignore | Adds coverage/ output directory. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| name: string; | ||
| trigger: 'first-visit' | 'manual'; | ||
| steps: TStep[]; | ||
| loop?: boolean; |
There was a problem hiding this comment.
core/types TourDefinition is missing onEndScroll, but the overlay reads tour.onEndScroll (e.g. in TorchlitOverlay.handleTourChange) and the public root types (src/types.ts) still define onEndScroll on TourDefinition. As-is, TypeScript users calling createTourService().register({ ... , onEndScroll: 'top' }) will hit excess-property errors even though the overlay supports the feature. Consider adding onEndScroll?: 'restore' | 'top' | 'none' to the core TourDefinition (it’s DOM-free) or otherwise widening the service’s accepted tour definition type so the root TourDefinition is assignable without casts.
| loop?: boolean; | |
| loop?: boolean; | |
| onEndScroll?: 'restore' | 'top' | 'none'; |
| override updated(changed: Map<string, unknown>) { | ||
| if (changed.has('service') && this.service) { | ||
| this.unsubscribe?.(); | ||
| this.attachService(); | ||
| } |
There was a problem hiding this comment.
When the service property changes, updated() unsubscribes and creates a new StepRunner, but it doesn't clear any auto-advance timer that might be running on the previous StepRunner instance. That can leave a stray timeout firing after the service swap. Clear/teardown the existing stepRunner (or call clearAutoAdvance()) before replacing it in attachService()/updated().
| const viewportHeight = window.innerHeight; | ||
| const fits = | ||
| rect.height + 270 + 32 < viewportHeight; | ||
| const inView = fits | ||
| ? rect.top >= 0 && | ||
| rect.bottom <= viewportHeight && | ||
| rect.left >= 0 && | ||
| rect.right <= window.innerWidth | ||
| : snapshot.step.placement === 'top' | ||
| ? rect.top >= 270 + 16 + this.options.spotlightPadding && | ||
| rect.top < viewportHeight | ||
| : rect.top >= 0 && rect.top < viewportHeight; |
There was a problem hiding this comment.
StepRunner.shouldScrollIntoView() hardcodes tooltip sizing constants (270, 32, 16) that duplicate values already defined in dom/positioning (TOOLTIP_H_MAX, GAP). This makes future UI tweaks easy to miss and can desync scroll behavior from positioning behavior. Prefer importing and using the shared constants/helpers instead of repeating the numbers here.
| export function deepQuery( | ||
| selector: string, | ||
| root: Element | Document = document.body, | ||
| ): Element | null { | ||
| const found = root.querySelector(selector); | ||
| if (found) return found; | ||
|
|
||
| const children = root.querySelectorAll('*'); | ||
| for (const element of children) { | ||
| if (element.shadowRoot) { | ||
| const shadowResult = deepQuery( | ||
| selector, | ||
| element.shadowRoot as unknown as Document, | ||
| ); | ||
| if (shadowResult) return shadowResult; |
There was a problem hiding this comment.
deepQuery accepts Element | Document, but the implementation also recurses into ShadowRoot and currently casts shadowRoot to Document. This is type-unsafe and makes it harder to reuse deepQuery starting from a shadow root. Consider widening the root parameter to include ShadowRoot (or a ParentNode/Document | Element | ShadowRoot-style type) so the recursion doesn’t require an incorrect cast.
| export function resolveTarget( | ||
| targetId: string, | ||
| targetAttribute = DEFAULT_TARGET_ATTR, | ||
| root: Element | Document = document.body, | ||
| ): Element | null { | ||
| if (!targetId || targetId === '_none_') return null; | ||
| return deepQuery(resolveTargetSelector(targetId, targetAttribute), root); | ||
| } |
There was a problem hiding this comment.
resolveTarget() takes root: Element | Document, but callers may reasonably want to search within a ShadowRoot (especially since deepQuery supports it). Widening this parameter type (and default) to include ShadowRoot would avoid forcing consumers into as unknown as Document casts and better matches the actual behavior.
| private attachService() { | ||
| this.unsubscribe = this.service.subscribe(snap => this.handleTourChange(snap)); | ||
| } | ||
| this.stepRunner = new StepRunner({ | ||
| getCurrentSnapshot: () => this.service.getSnapshot() as CoreTourSnapshot<TourStep> | null, | ||
| getTour: tourId => this.getTourDefinition(tourId), | ||
| nextStep: () => this.service.nextStep(), | ||
| spotlightPadding: this.service.spotlightPadding, | ||
| targetAttribute: this.service.targetAttribute, | ||
| dispatchRouteChange: route => this.dispatchRouteChange(route), | ||
| }); | ||
|
|
||
| /* ── Auto-advance ───────────────────────────────── */ | ||
| this.unsubscribe = this.service.subscribe(snapshot => { | ||
| void this.handleTourChange(snapshot as CoreTourSnapshot<TourStep> | null); | ||
| }); |
There was a problem hiding this comment.
attachService() uses casts for getSnapshot()/subscribe() (as CoreTourSnapshot<TourStep> | null). Since service is already typed as TourService<TourStep>, these casts should be unnecessary; keeping them can mask real type mismatches introduced later. Prefer removing the casts and letting the compiler enforce consistency between core and overlay snapshot types.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/dom/positioning.ts (1)
109-122: Consider removing unreachable default case.The
TourPlacementtype is exhaustive ('top' | 'bottom' | 'left' | 'right'), making the default case unreachable. However, keeping it as defensive coding is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/positioning.ts` around lines 109 - 122, The switch in getArrowClass has an unreachable default for the exhaustive TourPlacement type; remove the default branch and make the switch exhaustive by adding a compile-time check (e.g., after the cases assign placement to a never-typed variable or call an assertNever helper) so the TypeScript compiler will catch any future additions to TourPlacement — update getArrowClass to handle 'top'|'bottom'|'left'|'right' cases and include the never-based exhaustiveness check instead of returning 'arrow-bottom' in a default.src/overlay/step-runner.ts (1)
96-113: Consider simplifying the visibility check logic.The nested ternary in lines 102-110 is functionally correct but dense. Consider extracting into a helper function or using early returns for improved readability.
Suggested refactor
private shouldScrollIntoView(snapshot: ResolvedTourSnapshot): boolean { const rect = snapshot.targetRect; if (!rect) return false; const viewportHeight = window.innerHeight; const fits = rect.height + TOOLTIP_H_MAX + TARGET_CONTEXT_MARGIN < viewportHeight; - const inView = fits - ? rect.top >= 0 && - rect.bottom <= viewportHeight && - rect.left >= 0 && - rect.right <= window.innerWidth - : snapshot.step.placement === 'top' - ? rect.top >= TOOLTIP_H_MAX + GAP + this.options.spotlightPadding && - rect.top < viewportHeight - : rect.top >= 0 && rect.top < viewportHeight; + + let inView: boolean; + if (fits) { + inView = rect.top >= 0 && + rect.bottom <= viewportHeight && + rect.left >= 0 && + rect.right <= window.innerWidth; + } else if (snapshot.step.placement === 'top') { + inView = rect.top >= TOOLTIP_H_MAX + GAP + this.options.spotlightPadding && + rect.top < viewportHeight; + } else { + inView = rect.top >= 0 && rect.top < viewportHeight; + } return !inView; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/overlay/step-runner.ts` around lines 96 - 113, The nested ternary inside shouldScrollIntoView makes the inView calculation hard to read; extract the visibility logic into a small helper (e.g., isRectInView or computeInViewForPlacement) or rewrite with early returns: compute fits as now, then if fits call a clear helper that checks full-viewport containment using rect.top/bottom/left/right and window dimensions; else branch by snapshot.step.placement === 'top' and use explicit conditions that reference TOOLTIP_H_MAX, GAP and this.options.spotlightPadding; keep the final return as !inView. Ensure the helper takes the rect, placement and relevant constants so callers are clearer and unit-testable (target: function shouldScrollIntoView, snapshot.targetRect, snapshot.step.placement, TOOLTIP_H_MAX, GAP, this.options.spotlightPadding).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/dom/positioning.ts`:
- Around line 109-122: The switch in getArrowClass has an unreachable default
for the exhaustive TourPlacement type; remove the default branch and make the
switch exhaustive by adding a compile-time check (e.g., after the cases assign
placement to a never-typed variable or call an assertNever helper) so the
TypeScript compiler will catch any future additions to TourPlacement — update
getArrowClass to handle 'top'|'bottom'|'left'|'right' cases and include the
never-based exhaustiveness check instead of returning 'arrow-bottom' in a
default.
In `@src/overlay/step-runner.ts`:
- Around line 96-113: The nested ternary inside shouldScrollIntoView makes the
inView calculation hard to read; extract the visibility logic into a small
helper (e.g., isRectInView or computeInViewForPlacement) or rewrite with early
returns: compute fits as now, then if fits call a clear helper that checks
full-viewport containment using rect.top/bottom/left/right and window
dimensions; else branch by snapshot.step.placement === 'top' and use explicit
conditions that reference TOOLTIP_H_MAX, GAP and this.options.spotlightPadding;
keep the final return as !inView. Ensure the helper takes the rect, placement
and relevant constants so callers are clearer and unit-testable (target:
function shouldScrollIntoView, snapshot.targetRect, snapshot.step.placement,
TOOLTIP_H_MAX, GAP, this.options.spotlightPadding).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e81efe6-87dc-4511-8b1e-1e1d45c80848
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
SECURITY.mdpackage.jsonsrc/dom/deep-query.tssrc/dom/positioning.tssrc/dom/scroll-manager.tssrc/overlay/step-runner.tstest/positioning.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/positioning.test.ts
Summary
This PR re-architects Torchlit’s internals to make torchlit/service a pure, DOM-free core while preserving the documented public import paths:
It also bumps the package version to 0.3.0 to reflect the service-layer architecture and type-surface changes.
What changed
#Notable API / type changes
Update
This PR now also includes a small follow-up pass based on review feedback:
vitestwith@vitest/coverage-v8ShadowRootcast indeepQuerySECURITY.mdto mark0.3.xas the supported release line