refactor(observer): replace scroll-sentinel id with lvt-scroll-sentinel attribute

[adnaan]

Summary

• Replaces id="scroll-sentinel" with the boolean attribute lvt-scroll-sentinel for infinite scroll detection
• Changes getElementById to querySelector("[lvt-scroll-sentinel]") in observer-manager
• The old plain ID didn't look like a framework-reserved marker and risked naming collisions with user code

Test plan

• All 427 client unit tests pass (14 observer-manager tests updated)
• After merge + publish: E2E tests in examples/ and lvt/ repos (separate PRs)

🤖 Generated with Claude Code

[claude]

Review

The change is clean and well-motivated — using a namespaced attribute avoids ID collisions with user code and aligns with the lvt- convention.

One deployment risk to flag: The PR description notes that E2E tests (and presumably the server-side template that renders lvt-scroll-sentinel) are in separate, pending PRs. If the server template still emits id="scroll-sentinel" when this client code ships, infinite scroll will silently stop working. Consider either:

• Merging the server-side template change atomically with this one, or
• Temporarily supporting both (querySelector("[lvt-scroll-sentinel], #scroll-sentinel")) during the transition window

Otherwise, LGTM — the implementation is correct, all call sites are updated, and the test coverage is thorough.

[Copilot]

Pull request overview

Refactors infinite-scroll sentinel detection to use a framework-reserved marker attribute (lvt-scroll-sentinel) instead of a plain id, reducing the risk of collisions with user-defined IDs.

Changes:

• Replaced id="scroll-sentinel" with the boolean attribute lvt-scroll-sentinel in observer logic.
• Updated ObserverManager to locate the sentinel via an attribute selector.
• Updated ObserverManager unit tests to use the new sentinel marker.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
dom/observer-manager.ts	Switches sentinel lookup from ID to [lvt-scroll-sentinel].
tests/observer-manager.test.ts	Updates tests to construct/find the sentinel using lvt-scroll-sentinel.

Comments suppressed due to low confidence (1)

tests/observer-manager.test.ts:125

• Current tests still pass with a global document.querySelector, so they won’t catch the multi-wrapper/multi-sentinel case introduced by switching from a unique id to a non-unique attribute. Add a unit test that creates two wrappers each with lvt-scroll-sentinel, sets getWrapperElement() to one of them, and asserts the observer attaches to the sentinel within that wrapper.

    it("sets up IntersectionObserver when sentinel exists", () => {
      document.body.innerHTML = `
        <div id="wrapper">
          <div lvt-scroll-sentinel></div>
        </div>
      `;
      mockContext = {
        getWrapperElement: () => document.getElementById("wrapper"),
        send: mockSend,
      };

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

document.querySelector("[lvt-scroll-sentinel]") searches the whole document and will pick the first matching sentinel if multiple LiveTemplate wrappers (or multiple sentinels) exist on the page. With the old id, uniqueness was enforced; with a boolean attribute it’s not. Consider scoping the lookup to the current wrapper (e.g., wrapperElement.querySelector(...)) so each ObserverManager instance observes its own sentinel.

Suggested change

	const sentinel = document.querySelector("[lvt-scroll-sentinel]");
	const sentinel = wrapperElement.querySelector("[lvt-scroll-sentinel]");

[claude]

Review of #92 — scroll-sentinel attribute refactor

The core change is correct and an improvement: scoping the lookup to wrapperElement.querySelector instead of document.getElementById fixes a real bug where multiple infinite-scroll instances on the same page would all find the same sentinel. Good catch.

One concern: coordinated deployment

This is a breaking change that requires the server-side template to be updated in lockstep. If the server still emits id="scroll-sentinel" after this client ships, querySelector("[lvt-scroll-sentinel]") will return null and infinite scroll will silently stop working (no error, no load_more calls). The test plan defers E2E validation to follow-up PRs — that's fine, but it would be worth noting the deployment order explicitly (server template change must ship before or with this client version).

Minor: non-standard attribute

lvt-scroll-sentinel is a custom attribute without a data- prefix, which is technically invalid HTML5. This is consistent with other lvt-* attributes in the framework, so I'd only flag it if there's appetite for migration to data-lvt-scroll-sentinel at some point.

Otherwise the implementation is clean and the tests are thorough. LGTM pending the deployment coordination note above.