fix(exposure): cross-variant tracking for create changes (FT-1879)#5
Merged
Merged
Conversation
create changes with trigger_on_view previously fell through the generic selector-tracking branch in ExposureTracker, so peer variants without the create had nothing observable and never fired exposure when the would-be position entered viewport. Mirror the move/delete pattern: collect every unique (targetSelector, position) pair across all variants' viewport- triggered create changes and drop an invisible positional placeholder at each. Malformed creates (no targetSelector) are skipped explicitly -- the manipulator can't apply them either. Also gitignore .claude/tasks/ and .worktrees/.
WalkthroughThe PR adds two ignore patterns to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. 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
🤖 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/core/ExposureTracker.ts`:
- Around line 195-209: createPositions loop calls createContainerPlaceholder
once and returns early if targetSelector isn't present, so delayed-mounted
targets never get placeholders; update the logic around createPositions /
createContainerPlaceholder to register a fallback that watches for the target to
appear (e.g., via MutationObserver or a small retry/observer registry) and
inserts the same placeholder when the target is added to the DOM; reference
createPositions, createContainerPlaceholder, experimentName, and
pos.targetSelector/pos.position in your change and ensure the observer/registry
cleans itself up after insertion to avoid leaks.
🪄 Autofix (Beta)
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
Run ID: aef76f1a-57e8-453e-b515-fee0cdfaaba5
📒 Files selected for processing (4)
.gitignoredocs/superpowers/plans/2026-04-29-create-cross-variant-exposure.mdsrc/core/ExposureTracker.tssrc/core/__tests__/DOMChangesPluginLite.crossVariantExposure.test.ts
…g (FT-1879) Addresses review feedback on PR #5: - Delayed-mount fallback (CodeRabbit, code-reviewer, test-analyzer, silent-failure-hunter all flagged this): when targetSelector is not in the DOM at registration time (SPA late-mount), createContainerPlaceholder now returns false and the create-positions loop falls back to adding the targetSelector to viewportSelectors, so the existing MutationObserver picks up exposure when the target eventually appears. - Collision-safe key: replace the "${target}|${position}" composite map key with a nested Map<targetSelector, Set<position>> so CSS selectors that legally contain '|' (namespace, attribute selectors) cannot collide. - Debug log when a malformed create (no targetSelector) is skipped, so the silent skip can be discovered when debug=true. Test additions: - 6F3 extended with cleanup verification (placeholder removed, no re-fire). - 6F4 second test exercises the OTHER (header) placeholder. - 6F6 second test confirms a malformed create does not poison registration of well-formed sibling changes in the same variant. - 6F7 (new): delayed-mount targetSelector — exposure fires after target is appended later via MutationObserver. - 6F8 (new): URL-filter combined with viewport-create cross-variant tracking, both v0 and v1 users. Suite: 263 passed (was 259), 1 skipped, 0 failures, 0 lint errors.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/__tests__/DOMChangesPluginLite.crossVariantExposure.test.ts (1)
4093-4516: Consider extracting sharedcreatechange fixtures to reduce duplication.The repeated inline experiment payloads make this section harder to maintain and easier to drift. A small builder/helper for
createchanges would keep intent clearer and reduce copy/paste risk.♻️ Example refactor sketch
+const createChange = (overrides: Partial<any> = {}) => ({ + selector: '', + type: 'create', + element: '<div class="new"></div>', + targetSelector: '.container', + position: 'lastChild', + trigger_on_view: true, + ...overrides, +}); + +const createExperiment = (name: string, variantChanges: any[]) => ({ + name, + variants: [{ variables: { __dom_changes: [] } }, { variables: { __dom_changes: variantChanges } }], +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/__tests__/DOMChangesPluginLite.crossVariantExposure.test.ts` around lines 4093 - 4516, Extract the repeated "create" change fixtures into a small helper (e.g., makeCreateChange or buildCreateExperiment) and use it in the tests that construct ExperimentData (tests referencing test_6f3_peer_variant, test_6f4_multi_position*, test_6f5_dedup, test_6f6_*, test_6f7_delayed_target, test_6f8_url_filter) so the tests call the helper instead of embedding identical __dom_changes payloads; add the helper near createTreatmentTracker in the test file and have it return either the __dom_changes entry or a full ExperimentData variant payload (accepting params like element, targetSelector, position, trigger_on_view, optional urlFilter) and update each test to call that helper when building the experiment used by DOMChangesPluginLite.
🤖 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/core/__tests__/DOMChangesPluginLite.crossVariantExposure.test.ts`:
- Around line 4093-4516: Extract the repeated "create" change fixtures into a
small helper (e.g., makeCreateChange or buildCreateExperiment) and use it in the
tests that construct ExperimentData (tests referencing test_6f3_peer_variant,
test_6f4_multi_position*, test_6f5_dedup, test_6f6_*, test_6f7_delayed_target,
test_6f8_url_filter) so the tests call the helper instead of embedding identical
__dom_changes payloads; add the helper near createTreatmentTracker in the test
file and have it return either the __dom_changes entry or a full ExperimentData
variant payload (accepting params like element, targetSelector, position,
trigger_on_view, optional urlFilter) and update each test to call that helper
when building the experiment used by DOMChangesPluginLite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba90ff65-7280-4f4d-b137-6741fb490df7
📒 Files selected for processing (2)
src/core/ExposureTracker.tssrc/core/__tests__/DOMChangesPluginLite.crossVariantExposure.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/ExposureTracker.ts
4 tasks
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
createchanges withtrigger_on_view: truepreviously fell through the generic selector-tracking branch inExposureTracker.registerExperiment, so peer variants without the create had nothing observable and never fired exposure when the would-be position entered viewport. This broke cross-variant SRM parity (the same gap thatmoveanddeletealready solved).move/deletepattern: collect every unique(targetSelector, position)pair across all variants' viewport-triggeredcreatechanges and drop a 1px invisible positional placeholder viacreateContainerPlaceholderat each. Variants that do create the element keep their real DOM node — the placeholder coexists harmlessly.createchanges (notargetSelector) are skipped explicitly —DOMManipulatorLite.applyChangereturns false for them too, so there's nothing to track and no fallback is needed.6F: Create Changes: viewport-trigger baseline, peer-variant placeholder exposure, multi-variant different positions, same-position dedup, and malformed-create no-op..claude/tasks/and.worktrees/.Test plan
npx jest --testPathPattern="DOMChangesPluginLite"— 7 suites, 259 passed (254 baseline + 5 new), 1 skipped, 0 failures.npx jest --testPathPattern="crossVariantExposure" -t "6F"— all 6F.* tests pass.npm run lint— 0 errors after prettier; 55 pre-existingUnexpected anywarnings unchanged.Summary by CodeRabbit
New Features
Documentation
Tests
Chores