Repository navigation
feat(tutorial): add reusable onboarding engine - #136
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds the private ChangesTutorial engine
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to The PR adds the tutorial engine and supporting integrations with no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Host as Tutorial harness
participant Director as Tutorial director
participant Store as Progress store
participant Presenter as Driver.js presenter
Host->>Director: Observe actions, signals, and acknowledgements
Director->>Store: Persist progress
Director->>Presenter: Publish tutorial snapshot
Presenter->>Host: Present the current step
Host->>Presenter: Trigger replay or target refresh
Presenter->>Director: Request replay or acknowledgement
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
@codex review\n\nLatest patch |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e6137543b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
![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
🧹 Nitpick comments (13)
examples/tutorial-driver/vite.config.ts (2)
20-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winString aliases match by prefix, so the order is load-bearing.
'@mpgd/tutorial/driver'is a prefix of'@mpgd/tutorial/driver.css'. The list works only because the CSS entry comes first. A future reordering silently resolves the stylesheet tosrc/driver.ts. Use exact patterns to remove the ordering dependency.♻️ Proposed refactor
{ - find: '`@mpgd/tutorial/driver`', + find: /^`@mpgd`\/tutorial\/driver$/u, replacement: resolve(import.meta.dirname, '../../packages/tutorial/src/driver.ts'), },Apply the same anchored pattern to the remaining entries.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/tutorial-driver/vite.config.ts` around lines 20 - 43, Update the alias find patterns in the Vite resolve.alias configuration so every entry matches its complete package specifier exactly, including the CSS, platform-storage, driver, testing, and root tutorial aliases. Preserve each existing replacement target while removing the prefix-based ordering dependency.
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unnecessary double cast.
@ttsc/unplugin/viteis compatible with VitePluginOptionfor the pinned versions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/tutorial-driver/vite.config.ts` around lines 14 - 19, Remove the unnecessary `as unknown as PluginOption` cast from the `ttsc` plugin entry in the `plugins` array. Keep the existing `ttsc` configuration and rely on its compatible inferred type.packages/tutorial/src/driver.ts (1)
540-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the guarded attribute list into one module constant.
The tuple
['aria-controls', 'aria-expanded', 'aria-haspopup']appears at line 544, at line 623 incaptureTutorialTargetSemantics, and again as three literal comparisons inisTutorialTargetAttributeat lines 617-619. A future change to the list must update three places. Define the constant once and derive the type and the guard from it.♻️ Proposed refactor
+const tutorialTargetAttributes = [ + 'aria-controls', + 'aria-expanded', + 'aria-haspopup', +] as const; + +type TutorialTargetAttribute = (typeof tutorialTargetAttributes)[number];function isTutorialTargetAttribute( attribute: string | null, -): attribute is 'aria-controls' | 'aria-expanded' | 'aria-haspopup' { - return attribute === 'aria-controls' - || attribute === 'aria-expanded' - || attribute === 'aria-haspopup'; +): attribute is TutorialTargetAttribute { + return tutorialTargetAttributes.includes(attribute as TutorialTargetAttribute); }Replace the local
attributesbindings at lines 544 and 623 withtutorialTargetAttributes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/src/driver.ts` around lines 540 - 548, Define a module-level tutorialTargetAttributes constant containing the guarded ARIA attribute tuple, derive the Attribute type and isTutorialTargetAttribute checks from it, and replace the local attributes bindings in preserveTutorialTargetSemantics and captureTutorialTargetSemantics with the shared constant.packages/tutorial/test/dist-import.mjs (1)
24-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCall
director.destroy()and validate thedriversubpath export.Two gaps in this smoke test:
- The script never calls
director.destroy(). The director interface declaresdestroy()as the teardown method. Without it, a pending timer or persistence handle can keep the Node process alive and hang thesmoke:tutorial:diststep in CI.- The test does not check the
@mpgd/tutorial/driversubpath. That entry needs a DOM, so a full import is not appropriate, but a resolution check catches a brokenexportsmap without loading the module.♻️ Proposed additions
director.acknowledge('welcome'); assert.equal(director.getSnapshot().status, 'completed'); await director.flush(); +director.destroy(); + +assert.ok(import.meta.resolve('../dist/driver.js')); console.log('`@mpgd/tutorial` dist import smoke passed.');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/test/dist-import.mjs` around lines 24 - 33, Update the dist-import smoke test to validate resolution of the `@mpgd/tutorial/driver` subpath without importing or executing its DOM-dependent module, then call director.destroy() after the existing flush to ensure teardown completes before the script exits.packages/tutorial/driver.css (1)
23-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fixed title padding can collide with translated close-button text.
Line 24 reserves
96pxfor the close button. The close button useswidth: autoand grows with its text.presentation.copy.skipis host-supplied and translated, so a longer label overlaps the title. Consider a flex or grid header layout, or cap the close button width.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/driver.css` around lines 23 - 46, Update the .mpgd-tutorial-popover header layout so .driver-popover-title and .driver-popover-close-btn reserve space dynamically for translated close-button text instead of relying on the fixed 96px padding-right; use a flex or grid arrangement while preserving the existing title and close-button styling.examples/tutorial-driver/src/styles.css (1)
12-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dynamic viewport units for the mobile layout.
Lines 14, 80, and 165 use
100vh. On mobile browsers100vhexcludes the collapsing URL bar, so the sticky mobile nav at lines 177-187 can sit below the visible area. The harness exists to reproduce mobile behavior, so100dvhgives a more accurate reproduction.Also applies to: 78-82, 163-167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/tutorial-driver/src/styles.css` around lines 12 - 21, Update the mobile layout declarations using 100vh, including the body min-height and the corresponding rules near the mobile navigation, to use 100dvh so the layout tracks the visible viewport when mobile browser chrome collapses. Preserve the existing sizing and navigation behavior otherwise.examples/tutorial-driver/index.html (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe mount point creates invalid landmark nesting.
examples/tutorial-driver/src/main.tsline 75 writes<header>, two<nav>elements, androle="dialog"into#app. Because#appis<main>, those landmarks nest inside the main landmark. Aheaderinsidemainis not a banner, and the harness demonstrates modal and navigation semantics, so the nesting undercuts the demonstration.♻️ Proposed fix
- <main id="app"></main> + <div id="app"></div>Then wrap the content section in main.ts with
<main>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/tutorial-driver/index.html` at line 9, Change the `#app` mount point in the tutorial driver from a main element to a neutral container, then update the content-rendering logic in main.ts to wrap the appropriate content section in its own main element while preserving the existing header, nav, and dialog landmark structure.packages/tutorial/test/driver.test.ts (1)
56-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for target rebinding, the
missingTargetpolicies, and the skip path.The suite covers target resolution, ARIA ownership, and cleanup. Three changed behaviors have no test:
rebindActiveTargetatpackages/tutorial/src/driver.tslines 322-353, which the PR lists as responsive target rebinding.- The
missingTargetpolicieserror,wait, andunanchoredat lines 196-209.- The close-button skip path at lines 139-144, including the re-drive behavior after
onDestroyed.Do you want me to draft these tests?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/test/driver.test.ts` around lines 56 - 127, Extend the Driver tutorial presenter tests to cover rebindActiveTarget by replacing or removing the active target and verifying rebinding, exercise missingTarget policies error, wait, and unanchored with their expected outcomes, and test the close-button skip flow including onDestroyed triggering re-drive behavior. Anchor the additions to createDriverTutorialPresenter, presenter.present, and the skip control while preserving existing cleanup and accessibility assertions.packages/tutorial/test/director.test.ts (2)
194-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the
offdebug mode.Line 204 passes
?mpgd-tutorial=offtogether withenabled: false, soapplyTutorialDebugLaunchPolicyreturns at its first guard. Theoffbranch that callsdirector.setSuspended(true)never runs in this suite. Add a case withenabled: trueandsearch: '?mpgd-tutorial=off', then assert thatsuspendedistrue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/test/director.test.ts` around lines 194 - 216, Extend the test for applyTutorialDebugLaunchPolicy with an enabled debug policy using search '?mpgd-tutorial=off', then assert director.getSnapshot().suspended is true. Keep the existing disabled-policy and replay-mode assertions unchanged.
219-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the
skippedstatus invariants.The parser rejects a
skippedrecord when the step list is complete, whencompletedAtis set, or whenskippedAtisnull. This suite covers theactiveandcompletedinvariants only. Add askippedcase so a regression in that branch fails a test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/test/director.test.ts` around lines 219 - 237, The tutorial progress parser test in “tutorial progress parser” should add a skipped-status case covering invalid skipped records: reject a complete step list, a non-null completedAt, and a null skippedAt. Keep the existing active and completed assertions unchanged and use parseTutorialProgress with the existing initial progress fixture.packages/tutorial/src/index.ts (1)
5-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the selectors from the attribute names.
tutorialDomSelectorsrepeats the literal attribute names fromtutorialDomAttributes. A rename in one object does not update the other, so the two can drift silently. The Driver.js presenter readstutorialDomAttributes.targetwhile other code matches on the selector strings, which makes a drift hard to detect.Build the selectors from the attribute constants. Add
as constso consumers keep the literal types.♻️ Proposed refactor
export const tutorialDomAttributes = Object.freeze({ active: 'data-mpgd-tutorial-active', next: 'data-mpgd-tutorial-next', popover: 'data-mpgd-tutorial-popover', skip: 'data-mpgd-tutorial-skip', step: 'data-mpgd-tutorial-step', target: 'data-mpgd-tutorial-target', -}); +} as const); export const tutorialDomSelectors = Object.freeze({ - active: '[data-mpgd-tutorial-active="true"]', - next: '[data-mpgd-tutorial-next]', - popover: '[data-mpgd-tutorial-popover]', - skip: '[data-mpgd-tutorial-skip]', - target: '[data-mpgd-tutorial-target]', -}); + active: `[${tutorialDomAttributes.active}="true"]`, + next: `[${tutorialDomAttributes.next}]`, + popover: `[${tutorialDomAttributes.popover}]`, + skip: `[${tutorialDomAttributes.skip}]`, + target: `[${tutorialDomAttributes.target}]`, +} as const);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/src/index.ts` around lines 5 - 20, Update tutorialDomSelectors to construct each selector from the corresponding values in tutorialDomAttributes instead of repeating attribute-name literals, and add as const so both exported objects preserve literal types. Keep the existing selector behavior unchanged, including the true-value condition for active.packages/tutorial/package.json (1)
57-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making
driver.jsoptional.The package documents the Driver.js presenter as optional, but line 59 declares
driver.jsas a hard runtime dependency. Consumers that import only@mpgd/tutorialor@mpgd/tutorial/testingthen install Driver.js without using it. Move it topeerDependencieswithpeerDependenciesMeta.optional, or tooptionalDependencies, once the package leaves the incubator stage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/package.json` around lines 57 - 60, Update the dependencies in the package manifest so driver.js is no longer a mandatory runtime dependency for consumers; represent it as an optional peer dependency (including peerDependenciesMeta.optional) or optionalDependencies, while preserving the existing workspace dependency.packages/tutorial/test/platform-storage.test.ts (1)
45-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for a rejecting
loadand formigrate.Two branches of
createPlatformTutorialProgressStorehave no coverage:
- The
catcharoundstorage.load. A rejected load must setavailabletofalseand callonError.- The
migratehook. A migrated legacy record must parse into valid progress, and a throwingmigratemust disable the store.Both branches control fail-closed behavior, so a regression there silently disables or wrongly enables persistence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/test/platform-storage.test.ts` around lines 45 - 95, Extend the tests for createPlatformTutorialProgressStore with a rejecting storage.load case that verifies available is false and onError receives the failure. Add migrate coverage proving a legacy record is converted into valid progress, and a throwing migrate hook disables the store and reports the error through the configured failure path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/TUTORIAL_ENGINE.md`:
- Around line 159-161: Update the replay documentation around
createTutorialDirector.replay() to state that skipping an in-session replay
restores the prior durable progress for every state, including active progress,
and restores null when no progress existed.
In `@examples/tutorial-driver/src/main.ts`:
- Around line 176-186: Update the pagehide handler around debug.destroy(),
unbindReplay(), unsubscribe(), presenter.destroy(), and director.destroy() to
skip teardown when the event is persisted for a back/forward-cache navigation,
or add pageshow re-initialization so the harness is restored after bfcache
navigation. Preserve teardown for non-persisted page hides and ensure the
once-only listener does not leave the restored page with destroyed components.
In `@packages/tutorial/src/director.ts`:
- Around line 227-244: Update the direct progressStore.save call in skip() to
use the same destroyed/replaying guard as persist, preventing the asynchronous
skip write when the director has been destroyed or replaying is active while
preserving existing error handling.
In `@packages/tutorial/src/driver.ts`:
- Around line 522-527: In the underlyingModal handling, only add popover.id to
ownedIds when it is non-empty; preserve the existing previousAriaOwns values and
avoid setting a stray token in ownedAriaOwns when the popover wrapper has no id.
- Around line 139-164: Track a local dismissed flag/key for the current
presentation in the tutorial driver, set it before invoking input.onSkip from
onCloseClick, and prevent onDestroyed’s scheduleRefresh from re-driving that
dismissed step. Clear the dismissed key in present() when activePresentationKey
differs from nextKey so a newly presented step can render normally, including
when onSkip rejects.
In `@packages/tutorial/src/platform-storage.ts`:
- Around line 28-43: Add timeout enforcement around the storage load and save
operations used by the tutorial storage flow, including StorageAdapter and the
AIT, Capacitor, and Devvit bridge adapters. On timeout, mark available as false
and invoke onError, while preserving existing migration and persistence behavior
for completed calls; add tests covering pending load and save operations,
including flush() through queue.
In `@packages/tutorial/src/testing.ts`:
- Around line 99-103: Validate the raw debug step id used by
applyTutorialDebugLaunchPolicy before constructing replay options or calling
director.replay. Only pass a verified TutorialStepIdOf<TDefinition>; when the
URL value is unknown, ignore it and perform a full replay (or route the
validation failure through the existing error callback) instead of allowing
createTutorialProgressAtStep to reject.
---
Nitpick comments:
In `@examples/tutorial-driver/index.html`:
- Line 9: Change the `#app` mount point in the tutorial driver from a main element
to a neutral container, then update the content-rendering logic in main.ts to
wrap the appropriate content section in its own main element while preserving
the existing header, nav, and dialog landmark structure.
In `@examples/tutorial-driver/src/styles.css`:
- Around line 12-21: Update the mobile layout declarations using 100vh,
including the body min-height and the corresponding rules near the mobile
navigation, to use 100dvh so the layout tracks the visible viewport when mobile
browser chrome collapses. Preserve the existing sizing and navigation behavior
otherwise.
In `@examples/tutorial-driver/vite.config.ts`:
- Around line 20-43: Update the alias find patterns in the Vite resolve.alias
configuration so every entry matches its complete package specifier exactly,
including the CSS, platform-storage, driver, testing, and root tutorial aliases.
Preserve each existing replacement target while removing the prefix-based
ordering dependency.
- Around line 14-19: Remove the unnecessary `as unknown as PluginOption` cast
from the `ttsc` plugin entry in the `plugins` array. Keep the existing `ttsc`
configuration and rely on its compatible inferred type.
In `@packages/tutorial/driver.css`:
- Around line 23-46: Update the .mpgd-tutorial-popover header layout so
.driver-popover-title and .driver-popover-close-btn reserve space dynamically
for translated close-button text instead of relying on the fixed 96px
padding-right; use a flex or grid arrangement while preserving the existing
title and close-button styling.
In `@packages/tutorial/package.json`:
- Around line 57-60: Update the dependencies in the package manifest so
driver.js is no longer a mandatory runtime dependency for consumers; represent
it as an optional peer dependency (including peerDependenciesMeta.optional) or
optionalDependencies, while preserving the existing workspace dependency.
In `@packages/tutorial/src/driver.ts`:
- Around line 540-548: Define a module-level tutorialTargetAttributes constant
containing the guarded ARIA attribute tuple, derive the Attribute type and
isTutorialTargetAttribute checks from it, and replace the local attributes
bindings in preserveTutorialTargetSemantics and captureTutorialTargetSemantics
with the shared constant.
In `@packages/tutorial/src/index.ts`:
- Around line 5-20: Update tutorialDomSelectors to construct each selector from
the corresponding values in tutorialDomAttributes instead of repeating
attribute-name literals, and add as const so both exported objects preserve
literal types. Keep the existing selector behavior unchanged, including the
true-value condition for active.
In `@packages/tutorial/test/director.test.ts`:
- Around line 194-216: Extend the test for applyTutorialDebugLaunchPolicy with
an enabled debug policy using search '?mpgd-tutorial=off', then assert
director.getSnapshot().suspended is true. Keep the existing disabled-policy and
replay-mode assertions unchanged.
- Around line 219-237: The tutorial progress parser test in “tutorial progress
parser” should add a skipped-status case covering invalid skipped records:
reject a complete step list, a non-null completedAt, and a null skippedAt. Keep
the existing active and completed assertions unchanged and use
parseTutorialProgress with the existing initial progress fixture.
In `@packages/tutorial/test/dist-import.mjs`:
- Around line 24-33: Update the dist-import smoke test to validate resolution of
the `@mpgd/tutorial/driver` subpath without importing or executing its
DOM-dependent module, then call director.destroy() after the existing flush to
ensure teardown completes before the script exits.
In `@packages/tutorial/test/driver.test.ts`:
- Around line 56-127: Extend the Driver tutorial presenter tests to cover
rebindActiveTarget by replacing or removing the active target and verifying
rebinding, exercise missingTarget policies error, wait, and unanchored with
their expected outcomes, and test the close-button skip flow including
onDestroyed triggering re-drive behavior. Anchor the additions to
createDriverTutorialPresenter, presenter.present, and the skip control while
preserving existing cleanup and accessibility assertions.
In `@packages/tutorial/test/platform-storage.test.ts`:
- Around line 45-95: Extend the tests for createPlatformTutorialProgressStore
with a rejecting storage.load case that verifies available is false and onError
receives the failure. Add migrate coverage proving a legacy record is converted
into valid progress, and a throwing migrate hook disables the store and reports
the error through the configured failure path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76e04f70-4b53-4e07-9581-d5112c7fdf59
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
README.mddocs/TUTORIAL_ENGINE.mdexamples/tutorial-driver/index.htmlexamples/tutorial-driver/package.jsonexamples/tutorial-driver/src/main.tsexamples/tutorial-driver/src/styles.cssexamples/tutorial-driver/tsconfig.jsonexamples/tutorial-driver/vite.config.tspackage.jsonpackages/tutorial/README.mdpackages/tutorial/driver.csspackages/tutorial/package.jsonpackages/tutorial/src/definition.tspackages/tutorial/src/director.tspackages/tutorial/src/driver.tspackages/tutorial/src/index.tspackages/tutorial/src/platform-storage.tspackages/tutorial/src/progress.tspackages/tutorial/src/testing.tspackages/tutorial/test/director.test.tspackages/tutorial/test/dist-import.mjspackages/tutorial/test/driver.test.tspackages/tutorial/test/platform-storage.test.tspackages/tutorial/tsconfig.jsontsconfig.base.json
|
@codex review\n\nPR is now ready for review. Please review the latest commit |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e6137543b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review follow-up pushed in commits 40804e1, 2851b82, and 44644fb. All 15 actionable threads now have evidence replies and are resolved. Latest validation: @mpgd/tutorial 35/35 tests, package and demo typechecks, tutorial-driver production build, real Chromium 7-scenario smoke, full workspace check/tests, package build, dist import, dry-run tarball, package pack smoke, graph preflight, and git diff --check. The package remains an intentionally private 0.0.0 incubator; no registry publish was performed. |
|
@coderabbitai review\n\nPlease review the latest commit 44644fb and the resolved thread evidence. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Pull request metadata ==='
gh pr view 136 --repo imjlk/mpgd-kit --json number,headRefOid,baseRefOid,commits,reviews,comments 2>/dev/null \
| jq '{number, headRefOid, baseRefOid, commits: [.commits[] | {oid, messageHeadline}], reviews: [.reviews[] | {author: .author.login, state, body}], comments: [.comments[] | {author: .author.login, body, createdAt}]}' \
|| true
echo '=== Follow-up commit files and stat ==='
git show --stat --oneline --decorate --no-renames 44644fb3
git diff-tree --no-commit-id --name-status -r 44644fb3
echo '=== Changed TypeScript symbols ==='
while IFS= read -r file; do
case "$file" in
*.ts) ast-grep outline "$file" --items all --type function,class,interface,type --view compact || true ;;
esac
done < <(git diff-tree --no-commit-id --name-only -r 44644fb3)
echo '=== Review thread status ==='
gh api graphql -F owner='imjlk' -F name='mpgd-kit' -F number=136 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 20) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' 2>/dev/null | jq -r '
.data.repository.pullRequest.reviewThreads.nodes[] |
"resolved=\(.isResolved) path=\(.path // "general") line=\(.line // .originalLine // 0)\n" +
(.comments.nodes[] | " \(.author.login): \(.body | gsub("\n"; " "))")' || trueLength of output: 45670 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Follow-up range summary ==='
git diff --stat --no-renames 3e613754..44644fb3
git diff --name-status --no-renames 3e613754..44644fb3
echo '=== Follow-up diff ==='
git diff --no-ext-diff --unified=80 3e613754..44644fb3 -- \
docs/TUTORIAL_ENGINE.md \
examples/tutorial-driver/src/main.ts \
packages/tutorial/src/director.ts \
packages/tutorial/src/driver.ts \
packages/tutorial/src/platform-storage.ts \
packages/tutorial/src/testing.ts \
packages/tutorial/test/director.test.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/platform-storage.test.ts
echo '=== Source and test outlines ==='
for file in \
packages/tutorial/src/director.ts \
packages/tutorial/src/driver.ts \
packages/tutorial/src/platform-storage.ts \
packages/tutorial/src/testing.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/platform-storage.test.ts \
packages/tutorial/test/director.test.ts
do
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all --type function,interface,type --view compact || true
done
echo '=== Resolved review threads ==='
gh api graphql -F owner='imjlk' -F name='mpgd-kit' -F number=136 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 30) {
nodes { author { login } body }
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == true) | {path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body}]}' \
|| echo 'Review-thread metadata is unavailable from the GitHub API in this sandbox.'Length of output: 163734 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Driver presentation and skip state ==='
sed -n '90,390p' packages/tutorial/src/driver.ts
echo '=== Driver target and modal ownership cleanup ==='
sed -n '390,760p' packages/tutorial/src/driver.ts
echo '=== Director replay and skip persistence ==='
sed -n '70,320p' packages/tutorial/src/director.ts
echo '=== Platform-storage timeout and queue logic ==='
sed -n '1,180p' packages/tutorial/src/platform-storage.ts
echo '=== Exact final commit diff ==='
git show --no-ext-diff --unified=30 --check 44644fb3 -- \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts
echo '=== Unresolved review threads ==='
gh api graphql -F owner='imjlk' -F name='mpgd-kit' -F number=136 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 1) { nodes { author { login } body } }
}
}
}
}
}' --jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, originalLine, author: .comments.nodes[0].author.login, body: .comments.nodes[0].body}]' \
|| echo 'Unresolved-thread metadata is unavailable from the GitHub API in this sandbox.'Length of output: 32928
I found no new actionable issues. The resolved-thread evidence matches the current lifecycle, persistence, target-rebinding, modal-ARIA, and skip-dismissal implementations. The final commit preserves successful same-key skip dismissal. GitHub reports zero unresolved review threads. I could not run the reported test and browser-validation commands in this sandbox.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44644fb3e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Second review round addressed in |
|
@codex review\n\nPlease review latest commit |
|
@coderabbitai review\n\nPlease review latest commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commit metadata ==='
git show -s --format='%H%n%P%n%s%n%b' 1e8cfc28
echo
echo '=== Changed files and stat ==='
git diff-tree --no-commit-id --name-status -r 1e8cfc28
git show --stat --oneline --no-renames 1e8cfc28
echo
echo '=== Exact diff ==='
git diff --no-ext-diff --unified=80 1e8cfc28^ 1e8cfc28 -- \
packages/tutorial/src/director.ts \
packages/tutorial/src/driver.ts \
packages/tutorial/src/platform-storage.ts \
packages/tutorial/test/director.test.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/platform-storage.test.ts
echo '=== Source outlines ==='
for file in \
packages/tutorial/src/director.ts \
packages/tutorial/src/driver.ts \
packages/tutorial/src/platform-storage.ts \
packages/tutorial/test/director.test.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/platform-storage.test.ts
do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all --type function,interface,type --view compact || true
fi
done
echo '=== Whitespace check ==='
git show --check --format= 1e8cfc28 || true
echo '=== Unresolved review threads ==='
gh api graphql -F owner='imjlk' -F name='mpgd-kit' -F number=136 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 3) {
nodes { author { login } body }
}
}
}
}
}
}' --jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body: .body}]}]' \
|| echo 'Review-thread metadata is unavailable in this sandbox.'Length of output: 56940 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Documentation change ==='
git diff --no-ext-diff --unified=40 1e8cfc28^ 1e8cfc28 -- docs/TUTORIAL_ENGINE.md
echo '=== Complete director implementation ==='
sed -n '1,380p' packages/tutorial/src/director.ts
echo '=== Complete changed Driver dismissal and presentation flow ==='
sed -n '40,380p' packages/tutorial/src/driver.ts
echo '=== Complete storage implementation ==='
sed -n '1,220p' packages/tutorial/src/platform-storage.ts
echo '=== Timeout API references ==='
rg -n -C2 --glob '!**/node_modules/**' \
'(defaultTutorialStorageTimeoutMs|defaultTutorialStorageLoadTimeoutMs|timeoutMs|loadTimeoutMs|withStorageTimeout|loadWithTimeout)' \
.
echo '=== Storage adapter declarations and tutorial storage imports ==='
rg -n -C3 --glob '*.{ts,json}' \
"(interface StorageAdapter|type StorageAdapter|export.*StorageAdapter|from '`@mpgd/tutorial/platform-storage`'|from './platform-storage\\.js')" \
packages tsconfig.base.json package.json
echo '=== Relevant current test declarations ==='
ast-grep outline packages/tutorial/test/director.test.ts --items all --type function --view signatures || true
ast-grep outline packages/tutorial/test/driver.test.ts --items all --type function --view signatures || true
ast-grep outline packages/tutorial/test/platform-storage.test.ts --items all --type function --view signatures || trueLength of output: 91932
I found no new actionable issues. The director now keeps each callback argument aligned with The Driver presenter preserves dismissal through transient The progress store applies a timeout only to initialization loads. It keeps uncancellable saves serialized until the storage adapter settles. The documentation matches this behavior. GitHub reports zero unresolved review threads. I could not run the reported test, build, or browser-validation commands in this sandbox. You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e8cfc2894
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review round 3 addressed in |
|
@codex review Please review latest commit |
|
@coderabbitai review Please review latest commit |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commit metadata and changed files ==='
git show -s --format='%H%n%P%n%s%n%b' cb209812
git diff-tree --no-commit-id --name-status -r cb209812
git show --stat --oneline --no-renames cb209812
echo '=== Commit diff ==='
git diff --no-ext-diff --unified=70 cb209812^ cb209812 -- \
packages/tutorial/src/driver.ts \
packages/tutorial/src/testing.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/director.test.ts \
packages/tutorial/test/platform-storage.test.ts \
docs/TUTORIAL_ENGINE.md
echo '=== Affected source and test outlines ==='
for file in \
packages/tutorial/src/driver.ts \
packages/tutorial/src/testing.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/director.test.ts \
packages/tutorial/test/platform-storage.test.ts
do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" --items all --type function,interface,type --view compact || true
fi
done
echo '=== Focused implementation references ==='
rg -n -C4 --glob '!**/node_modules/**' \
'(dismiss|replay|isVisibleTutorialTarget|installTutorialDebugBridge|createMemoryTutorialProgressStore|unavailable)' \
packages/tutorial/src packages/tutorial/test docs/TUTORIAL_ENGINE.md
echo '=== Whitespace check ==='
git show --check --format= cb209812 || true
echo '=== Review thread status ==='
gh api graphql -F owner='imjlk' -F name='mpgd-kit' -F number=136 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 20) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | {resolved: .isResolved, path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body: .body}]}' \
|| echo 'Review-thread metadata is unavailable in this sandbox.'Length of output: 165223 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== All replay-trigger call sites and imports ==='
rg -n -C4 --glob '*.{ts,tsx,mts,cts,md}' \
'(bindTutorialReplayTrigger\s*\(|bindTutorialReplayTrigger\s*})' \
.
echo '=== Debug bridge installation call sites ==='
rg -n -C5 --glob '*.{ts,tsx,mts,cts,md}' \
'(installTutorialDebugBridge\s*\(|installTutorialDebugBridge\s*})' \
.
echo '=== Tutorial harness replay and teardown wiring ==='
sed -n '1,340p' examples/tutorial-driver/src/main.ts
echo '=== New testing helper regression suite ==='
sed -n '1,260p' packages/tutorial/test/testing.test.ts
echo '=== Current public export maps and driver API declarations ==='
sed -n '1,180p' packages/tutorial/package.json
sed -n '1,180p' packages/tutorial/src/driver.ts
sed -n '130,260p' packages/tutorial/src/testing.ts
echo '=== Corrected source outlines ==='
ast-grep outline packages/tutorial/src/driver.ts --items all --type function,interface,type --view signatures || true
ast-grep outline packages/tutorial/src/testing.ts --items all --type function,interface,type --view signatures || trueLength of output: 34834
I found no new actionable issues. The replay trigger resets a successful same-step dismissal only after The target resolver now rejects targets hidden by an ancestor. It still supports an off-viewport renderable fallback. The debug bridge teardown skips destroyed predecessors. It does not restore a stale bridge after out-of-order teardown. The unavailable memory store now ignores saves. Its snapshot and save log remain unchanged. GitHub reports zero unresolved review threads. I could not run the reported test, build, or browser-validation commands in this sandbox.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb2098122c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review round 4 addressed in |
|
@codex review Please review latest commit |
|
@coderabbitai review Please review latest commit |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tutorial/src/driver.ts (1)
480-492: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the target candidate scan per mutation batch.
isTargetSelectionMutationruns per record. For attribute, characterData, and childList records it callscollectTargetSelectionElements, which callsfindComposedTutorialTargetsand walks the composed tree including open shadow roots. A single batch with many attribute records repeats that whole scan for each record. Attribute churn on animated hosts makes this a hot path inside the MutationObserver callback.Compute the candidate list once per
processMutationRecordsinvocation and pass it into the per-record checks.♻️ Suggested approach
function processMutationRecords(records: readonly MutationRecord[]): void { if (activePresentation === null && instance === null) { observeDocumentOnlyMutations(); return; } reconcileMutationRoots(); const hostRecords = records.filter((record) => !isDriverOwnedMutation(record)); if (hostRecords.length > 0) { - scheduleRefresh(hostRecords.some(isTargetSelectionMutation)); + const presentation = activePresentation; + const candidates = presentation === null + ? [] + : collectTargetSelectionElements(presentation); + scheduleRefresh(hostRecords.some( + (record) => isTargetSelectionMutation(record, candidates), + )); } }Then use the passed
candidatesinisTargetSelectionMutationandstylesheetMutationAffectsLookupRootinstead of recomputing.Also applies to: 620-646
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tutorial/src/driver.ts` around lines 480 - 492, Cache the target candidate elements once inside processMutationRecords for each mutation batch, then pass that list through the per-record target-selection checks. Update isTargetSelectionMutation and stylesheetMutationAffectsLookupRoot to accept and reuse the candidates instead of calling collectTargetSelectionElements or findComposedTutorialTargets repeatedly, while preserving existing mutation filtering and refresh behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/tutorial/src/driver.ts`:
- Around line 480-492: Cache the target candidate elements once inside
processMutationRecords for each mutation batch, then pass that list through the
per-record target-selection checks. Update isTargetSelectionMutation and
stylesheetMutationAffectsLookupRoot to accept and reuse the candidates instead
of calling collectTargetSelectionElements or findComposedTutorialTargets
repeatedly, while preserving existing mutation filtering and refresh behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04d8a5cd-e1d5-4ca2-b3bb-b8fc50d3210c
📒 Files selected for processing (2)
packages/tutorial/src/driver.tspackages/tutorial/test/driver.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc2b54bfaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Latest head: The three latest findings are fixed and resolved: target-only Shadow DOM pass-through without opening unrelated UI, durable persistence of valid migrated progress, and default alertdialog ownership. Tutorial 115/115, package/root checks, OCR plus manual final audit, and diff check pass. |
|
@coderabbitai review Please review latest head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='41a8ca64'
echo '=== Commit metadata and changed files ==='
git show -s --format='%H%n%P%n%s%n%b' "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
git show --stat --oneline --no-renames "$commit"
echo '=== Focused diff ==='
git diff --no-ext-diff --unified=50 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/src/platform-storage.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/platform-storage.test.ts \
packages/tutorial/test/director.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Source outlines ==='
ast-grep outline packages/tutorial/src/driver.ts --items all --view signatures || true
ast-grep outline packages/tutorial/src/platform-storage.ts --items all --view signatures || true
echo '=== Relevant implementation and test references ==='
rg -n -C6 --glob '!**/node_modules/**' \
'(ShadowRoot|shadowRoot|pointer|pass-through|elementFromPoint|elementsFromPoint|overlay|target-only|migration|migrate|parseTutorialProgress|snapshot|save|load|alertdialog|dialog|aria-modal|role|ownership|destroy|cleanup)' \
packages/tutorial/src/driver.ts \
packages/tutorial/src/platform-storage.ts \
packages/tutorial/src/progress.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/platform-storage.test.ts \
packages/tutorial/test/director.test.ts \
docs/TUTORIAL_ENGINE.md
echo '=== Whitespace check ==='
git show --check --format= "$commit" || true
echo '=== Unresolved review threads ==='
gh api graphql \
-F owner='imjlk' \
-F name='mpgd-kit' \
-F number=136 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 10) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' \
--jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body: .body, createdAt}]}]' \
|| echo 'Review-thread metadata is unavailable in this sandbox.'Length of output: 201246 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='41a8ca64'
echo '=== Compact changed hunks ==='
git diff --no-ext-diff --unified=18 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/src/platform-storage.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/platform-storage.test.ts
echo '=== Changed line ranges ==='
git diff --no-ext-diff --unified=0 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/src/platform-storage.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/test/platform-storage.test.ts \
| rg '^diff --git|^@@' || true
echo '=== Pass-through and hit-test implementation ==='
rg -n -C10 \
'(target-proxy|targetOnly|passThrough|pointerEvents|elementFromPoint|elementsFromPoint|sync.*[Pp]ointer|overlay)' \
packages/tutorial/src/driver.ts
echo '=== Modal discovery and ownership implementation ==='
rg -n -C10 \
'(alertdialog|modalSelector|find.*[Mm]odal|aria-modal|aria-owns|modal ownership|sync.*[Mm]odal)' \
packages/tutorial/src/driver.ts
echo '=== Progress-store migration and save implementation ==='
cat -n packages/tutorial/src/platform-storage.ts
echo '=== Progress parser contract ==='
sed -n '1,220p' packages/tutorial/src/progress.ts
echo '=== Exact new regression test blocks ==='
sed -n '2185,2265p' packages/tutorial/test/driver.test.ts
sed -n '540,590p' packages/tutorial/test/driver.test.ts
rg -n -A75 -B10 '(migrat|migration)' packages/tutorial/test/platform-storage.test.tsLength of output: 66139 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Driver.js dependency version ==='
rg -n -C3 '"driver\.js"|driver\.js@|driverjs' \
packages/tutorial/package.json package.json pnpm-lock.yaml
echo '=== Overlay and proxy stacking rules in repository sources ==='
rg -n -C5 --glob '!**/node_modules/**' \
'(driver-overlay|driver-active-element|mpgd-tutorial-target-proxy|z-index|zIndex)' \
packages/tutorial \
examples/tutorial-driver
echo '=== Driver.js package artifacts, if present ==='
fd -HI '^driver\.(css|js|mjs)$|^driverjs\.' . node_modules 2>/dev/null \
| head -50 \
| while IFS= read -r file; do
echo "--- $file ---"
rg -n -C3 '(driver-overlay|z-index|zIndex)' "$file" || true
done
echo '=== Lockfile package resolution ==='
rg -n -A25 -B5 '^ driver\.js@|^ driver\.js:' pnpm-lock.yamlLength of output: 61425 🤖 web_search executed:
Length of output: 8266
For a Shadow DOM action target that is blocked by the Driver overlay, host code that checks Keep actual pointer input on the target for the target rectangle. Do not replace it with
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41a8ca64d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the latest CodeRabbit top-level finding in |
|
@codex review Latest head: |
|
@coderabbitai review Please review latest head |
|
@codex review Latest head: |
|
@coderabbitai review Please review latest head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='44e9b01a'
echo '=== Commit metadata and changed files ==='
git show -s --format='%H%n%P%n%s%n%b' "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
git show --stat --oneline --no-renames "$commit"
echo '=== Focused diff ==='
git diff --no-ext-diff --unified=45 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Source and test outlines ==='
ast-grep outline packages/tutorial/src/driver.ts --items all --view signatures || true
ast-grep outline packages/tutorial/test/driver.test.ts --items all --type function --view signatures || true
echo '=== Pointer pass-through, ARIA ownership, and document checks ==='
rg -n -C8 --glob '!**/node_modules/**' \
'(pointer-events|pointerEvents|elementFromPoint|elementsFromPoint|overlay|blocker|target.*rect|ShadowRoot|shadowRoot|aria-owns|ariaOwns|ownerDocument|foreign|Document|resolver|click|pointerdown|destroy|restore)' \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Whitespace check ==='
git show --check --format= "$commit" || true
echo '=== Unresolved review threads ==='
gh api graphql \
-F owner='imjlk' \
-F name='mpgd-kit' \
-F number=136 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 10) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' \
--jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body: .body, createdAt}]}]' \
|| echo 'Review-thread metadata is unavailable in this sandbox.'Length of output: 200937 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='44e9b01a'
echo '=== Exact changed hunks ==='
git diff --no-ext-diff --unified=18 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts
echo '=== Changed line ranges ==='
git diff --no-ext-diff --unified=0 "${commit}^" "$commit" -- packages/tutorial/src/driver.ts \
| rg '^@@' || true
echo '=== Target-only overlay opening and blocker lifecycle ==='
rg -n -C10 \
'(outside-blocker|target.*hole|pointerEvents.*none|pointer-events.*none|sync.*(Blocker|Overlay|HitTest)|create.*Blocker|remove.*Blocker|blocker)' \
packages/tutorial/src/driver.ts
echo '=== ARIA ownership and custom resolver document validation ==='
rg -n -C10 \
'(aria-owns|ariaOwns|ownerDocument|must belong to the presenter document|resolveTarget)' \
packages/tutorial/src/driver.ts
echo '=== Exact new tests ==='
sed -n '1015,1060p' packages/tutorial/test/driver.test.ts
sed -n '1805,1840p' packages/tutorial/test/driver.test.ts
sed -n '2238,2395p' packages/tutorial/test/driver.test.tsLength of output: 56496
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='15509dc7'
echo '=== Commit metadata and changed files ==='
git show -s --format='%H%n%P%n%s%n%b' "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
git show --stat --oneline --no-renames "$commit"
echo '=== Focused diff ==='
git diff --no-ext-diff --unified=45 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Source structure ==='
ast-grep outline packages/tutorial/src/driver.ts --items all --view signatures || true
echo '=== Pointer path, blocker lifecycle, and cleanup ==='
rg -n -C8 --glob '!**/node_modules/**' \
'(target-proxy|\\.click\\(|pointer-events|pointerEvents|blocker|overlay|elementFromPoint|elementsFromPoint|ShadowRoot|shadowRoot|target.*rect|pointerdown|pointerup|click|destroy|cleanup|removeChild)' \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Exact implementation ranges ==='
rg -n \
'(function sync|function clear|function remove|function create|outside-blocker|pointerEvents)' \
packages/tutorial/src/driver.ts || true
echo '=== Relevant regression tests ==='
rg -n -C10 \
'(pointerdown|isTrusted|outside.*block|block.*outside|Shadow.*target|destroy.*block|cleanup.*block)' \
packages/tutorial/test/driver.test.ts || true
echo '=== Whitespace check ==='
git show --check --format= "$commit" || true
echo '=== Unresolved review threads ==='
gh api graphql \
-F owner='imjlk' \
-F name='mpgd-kit' \
-F number=136 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 10) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' \
--jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body: .body, createdAt}]}]' \
|| echo 'Review-thread metadata is unavailable in this sandbox.'Length of output: 24505 🏁 Script executed: #!/bin/bash
set -uo pipefail
commit='15509dc7'
echo '=== Pointer guard lifecycle and call sites ==='
rg -n -C8 -e 'allowShadowTargetThroughOverlay' \
-e 'clearTargetOverlayPointerGuard' \
-e 'targetOverlayPointerRestore' \
-e 'scheduleShadowTargetHitTest' \
-e 'cancelShadowTargetHitTest' \
-e 'syncTargetAcknowledgeBinding' \
packages/tutorial/src/driver.ts || true
echo '=== Source: presenter rendering, refresh, and destruction ==='
sed -n '430,780p' packages/tutorial/src/driver.ts
sed -n '1040,1560p' packages/tutorial/src/driver.ts
echo '=== Source: geometry monitor and target-state cleanup ==='
sed -n '780,1045p' packages/tutorial/src/driver.ts
sed -n '1560,1715p' packages/tutorial/src/driver.ts
echo '=== Stacking and pointer styles ==='
rg -n -C5 -e 'z-index' -e 'zIndex' -e 'driver-overlay' -e 'driver-popover' \
packages/tutorial/driver.css \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts || true
echo '=== All blocker and proxy references in the requested commit ==='
git grep -n -E 'target-proxy|outside-blocker|mpgdTutorialOutsideBlocker|targetOverlayPointerRestore' "$commit" -- \
packages/tutorial/src/driver.ts packages/tutorial/test/driver.test.ts packages/tutorial/driver.css \
|| true
echo '=== Tests adjacent to the changed target-input test ==='
sed -n '2140,2345p' packages/tutorial/test/driver.test.ts
echo '=== Whitespace ==='
git show --check --format= "$commit" || true
echo '=== Unresolved GitHub review threads ==='
gh api graphql \
-F owner='imjlk' -F name='mpgd-kit' -F number=136 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 10) {
nodes { author { login } body }
}
}
}
}
}
}' \
--jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body: .body}]}]' \
|| trueLength of output: 83799
I found no new actionable issues. The overlay pass-through path no longer uses The presenter disables overlay pointer interception only when The presenter adds four package-owned exterior blockers. The cleanup paths remove these blockers and restore the previous inline overlay The tests cover blocker creation, cleanup on I could not independently run the reported tutorial tests, Chromium scenario, or workspace checks in this sandbox. You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44e9b01a03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Please review latest commit |
|
@coderabbitai review Please review latest commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='7e00f4ea'
echo '=== Commit metadata and changed files ==='
git show -s --format='%H%n%P%n%s%n%b' "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
git show --stat --oneline --no-renames "$commit"
echo '=== Focused diff ==='
git diff --no-ext-diff --unified=45 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Source and test structure ==='
ast-grep outline packages/tutorial/src/driver.ts --items all --view signatures || true
ast-grep outline packages/tutorial/test/driver.test.ts --items all --type function --view signatures || true
echo '=== Target interaction, hit-test, Shadow DOM, and skip lifecycle references ==='
rg -n -C7 --glob '!**/node_modules/**' \
'(post-disable|pointer-events|pointerEvents|elementFromPoint|elementsFromPoint|hit.test|light.DOM|ShadowRoot|shadowRoot|closed|attachShadow|activation|activatable|skip|onSkip|skipTutorial|inactive|destroy|refresh|restart)' \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Relevant implementation ranges ==='
sed -n '620,1080p' packages/tutorial/src/driver.ts
sed -n '1080,1620p' packages/tutorial/src/driver.ts
sed -n '1620,2060p' packages/tutorial/src/driver.ts
echo '=== Changed regression-test ranges ==='
git diff --no-ext-diff --unified=0 "${commit}^" "$commit" -- packages/tutorial/test/driver.test.ts \
| rg -n -C3 '^\+.*(it\(|test\(|expect|pointer|hit|Shadow|shadow|skip|inactive|activation)' || true
echo '=== Whitespace check ==='
git show --check --format= "$commit"
echo '=== Unresolved GitHub review threads ==='
gh api graphql \
-F owner='imjlk' \
-F name='mpgd-kit' \
-F number=136 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 10) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' \
--jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body: .body, createdAt}]}]'Length of output: 127126 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='7e00f4ea'
echo '=== Exact commit diff for implementation and tests ==='
git diff --no-ext-diff --unified=20 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css
echo '=== Skip handler and state transitions ==='
rg -n -C12 \
'(onSkip|pendingSkipOperations|dismissedPresentationKey|reportActive\\(false\\)|destroyInstancePreservingTarget)' \
packages/tutorial/src/driver.ts
echo '=== Overlay guard and composed hit-test functions ==='
rg -n -A180 -B20 \
'(function downgradeInactiveTargetClickMode|function isShadowTutorialTargetHitTestable|function scheduleShadowTargetHitTest|function allowShadowTargetThroughOverlay|function clearTargetOverlayPointerGuard)' \
packages/tutorial/src/driver.ts
echo '=== Driver overlay, popover, and blocker stacking values ==='
rg -n -C5 \
'(driver-overlay|driver-popover|mpgdTutorialOutsideBlocker|outside-blocker|z-index|zIndex)' \
packages/tutorial/src/driver.ts \
packages/tutorial/driver.css \
packages/tutorial/package.json \
pnpm-lock.yaml
echo '=== New tests in full context ==='
sed -n '2180,2390p' packages/tutorial/test/driver.test.ts
sed -n '3250,3375p' packages/tutorial/test/driver.test.ts
echo '=== Commit state ==='
git status --shortLength of output: 72815
I found no new actionable issues. The presenter now verifies the composed hit path after it disables overlay pointer interception. If another element still blocks the target, it restores the overlay and does not create outside blockers. The hit-test path now also applies to light-DOM targets. A light-DOM acknowledge target blocked by the overlay falls back to the Next control. The skip callback starts before the presenter reports inactive state. The 250 ms geometry monitor now detects activatability changes for a target inside a closed The new regression tests cover these paths. The worktree is clean. I could not independently run the reported tutorial tests or workspace check in this sandbox. You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e00f4ea2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review Final scoped review request for |
|
@coderabbitai review Final scoped review request for |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='1423553d'
echo '=== Commit metadata and changed files ==='
git show -s --format='%H%n%P%n%s%n%b' "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
git show --stat --oneline --no-renames "$commit"
echo '=== Focused diff ==='
git diff --no-ext-diff --unified=55 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Source and test outlines ==='
ast-grep outline packages/tutorial/src/driver.ts --items all --view signatures || true
ast-grep outline packages/tutorial/test/driver.test.ts --items all --type function --view signatures || true
echo '=== Scoped implementation and test references ==='
rg -n -C8 --glob '!**/node_modules/**' \
'(overflow|clip|clipp|hit.test|elementFromPoint|elementsFromPoint|composed|ShadowRoot|shadowRoot|overlay|blocker|pointer-events|pointerEvents|target.*rect|outside|z-index|zIndex|destroy|cleanup)' \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css \
docs/TUTORIAL_ENGINE.md
echo '=== Exact implementation ranges ==='
rg -n -A180 -B20 \
'(function is.*TutorialTargetHitTestable|function .*HitTest|function .*Blocker|function allow.*ThroughOverlay|function clear.*Pointer|function sync.*Blocker)' \
packages/tutorial/src/driver.ts || true
echo '=== Regression test ranges ==='
git diff --no-ext-diff --unified=0 "${commit}^" "$commit" -- packages/tutorial/test/driver.test.ts \
| rg -n -C4 '^\+.*(it\(|test\(|expect|overflow|clip|hit|blocker|overlay|pointer|destroy)' || true
echo '=== Whitespace check ==='
git show --check --format= "$commit"
echo '=== Unresolved GitHub review threads ==='
gh api graphql \
-F owner='imjlk' \
-F name='mpgd-kit' \
-F number=136 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
originalLine
comments(first: 10) {
nodes { author { login } body createdAt }
}
}
}
}
}
}' \
--jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body: .body, createdAt}]}]' \
|| echo 'Review-thread metadata is unavailable in this sandbox.'Length of output: 181199 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit='1423553d'
echo '=== Exact scoped commit diff ==='
git diff --no-ext-diff --unified=25 "${commit}^" "$commit" -- \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts \
packages/tutorial/driver.css
echo '=== Visible-rectangle and viewport helper definitions ==='
rg -n -A170 -B25 \
'(function resolveTutorialTargetVisibleRect|function resolveViewportRect|function .*Clip|function .*Clipp|function .*Composed.*Ancestor|function .*Rect)' \
packages/tutorial/src/driver.ts
echo '=== Call sites for the visible rectangle helper ==='
rg -n -C5 'resolveTutorialTargetVisibleRect\s*\(' \
packages/tutorial/src/driver.ts
echo '=== Overlay, popover, and blocker stacking declarations ==='
rg -n -C8 \
'(driver-overlay|driver-popover|mpgd-tutorial-outside-blocker|z-index|zIndex)' \
packages/tutorial/driver.css \
packages/tutorial/src/driver.ts \
packages/tutorial/test/driver.test.ts
echo '=== Commit state ==='
git status --shortLength of output: 86874
I found no release-blocking issues.
The exterior blockers now use the computed
I did not independently run the reported test suite or workspace check in this sandbox. ✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
@mpgd/tutorialpackage with a DOM-free tutorial director, strict progress parsing, queuedPlatformGateway.storagepersistence, and replay/skip semanticsexamples/tutorial-driverso the engine can be reproduced without a game; a host replay/help button may be present or omitted, while the demo floating trigger appears only with?trigger=1Local reproduction
Then open:
http://localhost:5198/http://localhost:5198/?host-trigger=0http://localhost:5198/?trigger=1&host-trigger=0http://localhost:5198/?mpgd-tutorial=offhttp://localhost:5198/?mpgd-tutorial=replay&mpgd-tutorial-step=modal-choiceValidation
pnpm install --frozen-lockfilepnpm checkpnpm --filter @mpgd/tutorial test(123 tests)pnpm build:packagespnpm smoke:tutorial:distpnpm build:tutorialpnpm validate:publicpnpm pack:packages:prepared@mpgd/tutorial@0.1.0, including root,./platform-storage,./testing,./driver, and./driver.cssgit diff --checkRelease
@mpgd/tutorial@0.1.0was initially published manually from the verified pnpm-generated tarball and is publicly available with thelatestdist-tag0.1.0publish is the baseline; the post-publish runtime/accessibility fixes carry a patch changeset that plans0.1.1id-token: writerelease workflow, completing the Sampo path for subsequent changeset-driven releasesSummary by CodeRabbit
New Features
@mpgd/tutorialpackage for interactive tutorial flows with acknowledgements, actions, signals, replay, skipping, suspension, and saved progress.Bug Fixes
Documentation