refactor: replace scroll-sentinel id with lvt-scroll-sentinel attribute

[adnaan]

Summary

• Updates all generator templates (3 kits) to emit lvt-scroll-sentinel instead of id="scroll-sentinel"
• Updates inline JS fallback from getElementById to querySelector
• Updates pagination components across all kit variants
• Updates golden files and E2E test assertions
• Updates standalone rendering test fixture for consistency

Depends on: livetemplate/client#92 (must be merged + published first)

Test plan

• Pre-commit hook passes (Go fmt + lint + tests)
• 12 files updated consistently — zero remaining id="scroll-sentinel" references
• Full E2E suite after client package is published

🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR replaces id="scroll-sentinel" with a custom HTML attribute lvt-scroll-sentinel across all kit templates, golden files, and test assertions. The change is consistent and mechanical — 12 files updated with no remaining id="scroll-sentinel" references.

The motivation is sound: using a custom attribute (lvt-scroll-sentinel) avoids id uniqueness constraints in the DOM, which matters when multiple paginated lists could exist on the same page, and it establishes a clear lvt- namespace convention for framework-managed elements.

---

What's Good

• Consistency: All three kit variants (single, multi, default generator templates) are updated in lockstep. No partial rollout.
• Golden files updated: Both testdata/golden/ and e2e/testdata/golden/ are kept in sync.
• Inline JS fallback updated: getElementById('scroll-sentinel') → querySelector('[lvt-scroll-sentinel]') in the template JS — correct and necessary.
• CSS selector in rendering test updated: #scroll-sentinel { height: 1px; } → [lvt-scroll-sentinel] { height: 1px; } — important for the standalone test fixture to remain accurate.
• Test assertions updated accurately: String contains checks now look for lvt-scroll-sentinel rather than the old id= form, which is correct since querySelector accepts the bare attribute name.

---

Issues & Suggestions

Potential Bug: CSS specificity change

The rendered CSS in e2e/rendering_test.go changes from #scroll-sentinel { height: 1px; } (ID selector, specificity 0-1-0-0) to [lvt-scroll-sentinel] { height: 1px; } (attribute selector, specificity 0-0-1-0). This is a minor but real regression for apps that relied on the ID selector's higher specificity to override other styles. Low risk for a 1px sentinel, but worth noting in the PR description for downstream app authors.

Missing: pagination component JS (if any)

The components/pagination.tmpl files are updated for the HTML element but there's no JS setupInfiniteScroll in those component files — the JS lives in template.tmpl.tmpl. This looks intentional but worth a quick double-check that the component templates aren't separately used in a context where the JS would be emitted separately without the corresponding template update.

Dependency gate not enforced

The PR description notes this depends on livetemplate/client#92 being merged and published first. There's no CI guard (e.g., a version check or a skipped/conditional test) preventing this from being merged and used before the client package ships the lvt-scroll-sentinel listener. If someone generates an app from this PR's templates before the client is updated, infinite scroll will silently break. Consider:

• Adding a // TODO: requires livetemplate/client >= <version> comment near the attribute usage, or
• Holding the PR in draft until the client PR is merged.

E2E suite not fully green

The test plan notes the full E2E suite is pending the client package publish. This is expected given the dependency, but the PR is open (not draft). It would be safer to keep it in draft until the E2E gate passes.

---

Minor

• The ❌/✅ emoji in test log messages are idiomatic in this repo (I see them in existing tests), so no issue there.
• The style="height: 1px;" inline style on the sentinel element is unchanged and fine for a zero-visual-footprint sentinel.

---

Summary

The change is clean, well-scoped, and correctly applied everywhere. The main concern is the unguarded dependency on livetemplate/client#92 — merging this before the client ships will break infinite scroll in generated apps without any error. Recommend keeping as draft or adding a CI check until that dependency lands.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Refactors the infinite-scroll “sentinel” marker emitted by generated templates from a fixed id="scroll-sentinel" to a dedicated lvt-scroll-sentinel attribute, aligning generator output, kit templates, and E2E/golden expectations.

Changes:

• Replace id="scroll-sentinel" with the lvt-scroll-sentinel attribute across generator and kit templates/components.
• Update the inline JS fallback from getElementById('scroll-sentinel') to querySelector('[lvt-scroll-sentinel]').
• Update golden files and E2E assertions/fixtures to match the new sentinel marker.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
testdata/golden/resource_template.tmpl.golden	Updates golden output to use lvt-scroll-sentinel attribute.
internal/kits/system/single/templates/resource/template.tmpl.tmpl	Emits lvt-scroll-sentinel and updates fallback JS to querySelector.
internal/kits/system/single/components/pagination.tmpl	Updates pagination component sentinel markup.
internal/kits/system/multi/templates/resource/template.tmpl.tmpl	Emits lvt-scroll-sentinel and updates fallback JS to querySelector.
internal/kits/system/multi/components/pagination.tmpl	Updates pagination component sentinel markup.
internal/generator/templates/resource/template.tmpl.tmpl	Updates generated resource template sentinel + fallback JS selector.
internal/generator/templates/components/pagination.tmpl	Updates generated pagination component sentinel markup.
e2e/tutorial_test.go	Updates sentinel presence assertion messaging/check.
e2e/testdata/golden/resource_template.tmpl.golden	Updates E2E golden output to use lvt-scroll-sentinel.
e2e/resource_generation_test.go	Updates resource-gen test to look for new sentinel marker.
e2e/rendering_test.go	Updates rendering fixture HTML/CSS/JS to use attribute-based sentinel.
e2e/complete_workflow_test.go	Updates workflow E2E test to check for new sentinel marker.

[Copilot]

This assertion is now too broad: strings.Contains(tmplStr, "lvt-scroll-sentinel") can match the inline JS selector (querySelector('[lvt-scroll-sentinel]')) even if the sentinel element is missing, causing a false-positive test. Tighten the check to look for the actual markup (e.g., an element tag containing the attribute) or parse the template/HTML to assert the sentinel element exists.

[Copilot]

The template check strings.Contains(string(tmpl), "lvt-scroll-sentinel") can also match the JS selector string in the template, so the test may pass even if the sentinel element markup is removed. Consider checking for a more specific substring (e.g. a tag containing the attribute) or validating the parsed output contains an element with [lvt-scroll-sentinel].

[Copilot]

This strings.Contains(..., "lvt-scroll-sentinel") check can produce false positives because the template also contains the JS selector string ([lvt-scroll-sentinel]). To ensure you’re actually verifying the sentinel element, tighten the assertion to the element markup (or parse and query for an element with that attribute).

[claude]

Code Review — PR #313: Replace scroll-sentinel id with lvt-scroll-sentinel attribute

Overview

Clean, well-scoped refactor replacing id="scroll-sentinel" with a custom lvt-scroll-sentinel attribute across all three kit variants, their generator templates, golden files, and E2E tests. The motivation is clear: IDs must be unique per page, so a sentinel tied to an ID is fragile when multiple paginated lists could exist on one page. Using an attribute is the right architectural call.

---

✅ What's Done Well

• Consistency: All 12 files updated with zero remaining id="scroll-sentinel" references — no partial migration.
• Test assertions updated correctly: The E2E checks now look for <div lvt-scroll-sentinel rather than the ID string, which accurately reflects the generated output.
• CSS updated in test fixture: rendering_test.go correctly updates the inline CSS from #scroll-sentinel to [lvt-scroll-sentinel] — easy to miss, good catch.
• JS fallback updated correctly: querySelector('[lvt-scroll-sentinel]') is the right selector for a boolean attribute.

---

⚠️ Issues / Suggestions

1. Non-data-* attribute is invalid HTML5

lvt-scroll-sentinel is not a data-* attribute. Custom attributes must use the data- prefix per the HTML5 spec to be valid. A validator (or linter like axe) will flag this.

<!-- Current (invalid HTML5) -->
<div lvt-scroll-sentinel style="height: 1px;"></div>

<!-- Spec-compliant alternative -->
<div data-lvt-scroll-sentinel style="height: 1px;"></div>

The CSS and JS selectors would need corresponding updates ([data-lvt-scroll-sentinel]). I understand frameworks like htmx and Alpine use non-data- attributes intentionally, and if the client library (livetemplate/client#92) is already committed to lvt-* as a convention, that's a reasonable tradeoff — but it should be an explicit decision, not an oversight. Worth confirming.

2. Hard dependency on unpublished client package

The PR description notes:

Depends on: livetemplate/client#92 (must be merged + published first)

• Full E2E suite after client package is published

This means the PR is landing in a state where the full test suite cannot pass yet. If the client package is delayed or changes its attribute convention, this PR will need updates. Consider whether this should be merged before the client is published, or held until the dependency is resolved and E2E is green.

3. Test assertion granularity

The updated assertions check for <div lvt-scroll-sentinel (the opening tag prefix). This is fine, but it couples the test to the element being a <div>. If the element type ever changes (e.g., to <span> or a semantic element), tests will fail without a real regression. Consider a looser check:

// More resilient — checks attribute presence regardless of element type
if !strings.Contains(string(tmpl), `lvt-scroll-sentinel`) {

This is a minor point — the current check is not wrong, just slightly brittle.

---

Performance

querySelector('[lvt-scroll-sentinel]') vs getElementById('scroll-sentinel') — the attribute selector is marginally slower, but this lookup happens once at scroll setup time, so there is no practical impact.

---

Summary

The refactor is correct and complete. The main action item before merge is to confirm the lvt-* vs data-lvt-* attribute naming is intentional given the HTML5 validity concern. The client dependency and incomplete E2E suite are worth tracking — ideally hold merge until livetemplate/client#92 is published and the full suite is green.

🤖 Generated with Claude Code

[adnaan]

Replies to review comments

Re: lvt-* vs data-lvt-* naming — This is an intentional project-wide convention. All existing framework attributes (lvt-on:, lvt-fx:, lvt-el:, lvt-ignore, lvt-autofocus, lvt-focus-trap, etc.) use the non-data- prefix, consistent with frameworks like htmx (hx-*), Alpine (x-*), and Vue (v-*). The CLAUDE.md Progressive Complexity section documents lvt-* as the Tier 2 attribute namespace.

Re: test assertion granularity — The tighter <div lvt-scroll-sentinel check was specifically requested by Copilot's review to avoid false positives: the bare lvt-scroll-sentinel string also matches the inline JS selector querySelector('[lvt-scroll-sentinel]') in the same template file. The <div prefix ensures we're matching the actual HTML element, not the JS code. Coupling to the element type is an acceptable trade-off since the sentinel is always a <div> by convention.

[claude]

Code Review

Overview

This PR replaces the id="scroll-sentinel" pattern with an lvt-scroll-sentinel attribute across all three kit variants and the standalone generator templates. It also ships two related cleanups: removing stale debug instrumentation from E2E tests and fixing the form-submit mechanism (from synthetic dispatchEvent to a real button click).

---

What's Good

• Consistent change: All 12 affected files are updated in lockstep — generator templates, kit templates, golden files, and test assertions. No stragglers.
• Avoids ID collisions: Moving from id to an attribute means pages with multiple resources no longer risk duplicate IDs, which was a latent correctness bug with the old approach.
• Form submit fix is correct: Replacing form.dispatchEvent(new Event('submit', ...)) with a real chromedp.Click(...) is the right fix — synthetic events don't trigger the WebSocket-based action dispatch. The comment explaining why is helpful.
• Debug cleanup: Stripping window.__lvt* debug flags and console.log calls from test assertions is the right call now that the approach is solid.

---

Issues & Suggestions

1. Merge order dependency is not enforced

The PR notes it depends on livetemplate/client#92. If this is merged before the client package is published, any app running the old JS (which queries document.getElementById('scroll-sentinel')) will silently fail to observe the sentinel. There's no guard in CI to prevent an out-of-order merge.

Suggestion: Add a TODO or // requires client >= X.Y.Z comment to setupInfiniteScroll() in the template files, or block this PR with a branch protection rule that checks for the client release.

2. Non-standard attribute name

lvt-scroll-sentinel is a bare custom attribute, which is technically not valid HTML5 (the spec reserves data-* for author-defined attributes). Browsers tolerate it, but HTML validators and certain accessibility tools will flag it. The lvt- prefix is clearly intentional as a framework namespace — if that's an accepted convention in this project, it's fine, but worth a conscious decision.

Suggestion: If the project intends to use lvt-* attributes broadly (as a framework marker), document this in a conventions doc or CLAUDE.md. If not, consider data-lvt-scroll-sentinel.

3. querySelector returns the first match

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

If multiple resources with infinite scroll are rendered on the same page, only the first sentinel is observed. The old getElementById had the same implicit constraint, but it's worth making explicit.

Suggestion: Consider scoping the selector to a parent container (e.g., container.querySelector('[lvt-scroll-sentinel]')) to make the intent clear and future-proof multi-resource pages.

4. Hardcoded modal selector in E2E test

chromedp.Click(`dialog#add-modal button[type="submit"]`, chromedp.ByQuery),

This hardcodes #add-modal, which couples the test to the generated template's modal ID. If the ID naming convention changes, this will silently break.

Suggestion: Use a more structural selector (e.g., dialog[open] button[type="submit"]) or document the assumption with a comment.

5. Full E2E suite is unchecked

The test plan item "Full E2E suite after client package is published" is unchecked. This PR should not be merged until that box is checked.

---

Breaking Change Note

Existing generated apps will have id="scroll-sentinel" in their templates. After this PR ships, newly generated apps will use lvt-scroll-sentinel, but the client JS must also be updated to match. Consider adding a migration note to the changelog or a warning in lvt serve / lvt gen for apps detected as using the old pattern.

---

Summary

The refactor is clean and well-scoped. The two main blockers before merging are: (1) livetemplate/client#92 must be merged and published, and (2) the full E2E suite must pass. The suggestions above are mostly low-severity, but the multi-resource scoping issue (#3) is worth addressing before this pattern proliferates.

🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR replaces id=\"scroll-sentinel\" with a custom lvt-scroll-sentinel attribute across all three kit templates, updates the corresponding JavaScript selectors, cleans up E2E test debug code, and improves modal interaction tests to use real clicks rather than programmatic DOM manipulation.

---

Positive Changes

• E2E test improvement: Switching from form.dispatchEvent(new Event('submit', ...)) and modal.showModal() to chromedp.Click() is the right call. Real click events exercise the actual event delegation path and catch regressions that DOM manipulation bypasses.
• Debug code cleanup: Removing the window.__lvtSubmitListenerTriggered, window.__lvtHandleResponseCalled, etc. debug instrumentation is good hygiene. That code had no place in a committed test file.
• Consistency: All 12 affected files are updated uniformly — no stray id="scroll-sentinel" references remain, which is confirmed by the test assertions.

---

Issues and Suggestions

1. Non-standard custom attribute (notable concern)

lvt-scroll-sentinel is a bare attribute without a data- prefix. Per the HTML spec, custom attributes must use data-* to be valid HTML. Using non-prefixed custom attributes risks:

• HTML validation failures
• Conflicts with future spec additions
• Unexpected behavior in strict parsers

If the client library (livetemplate/client#92) is intentionally choosing a non-data- attribute (e.g., for framework-specific semantics similar to HTMX's hx-*), that design decision should be documented here. Otherwise, data-lvt-scroll-sentinel would be the spec-compliant choice.

2. CSS selector in rendering_test.go

/* Before */
#scroll-sentinel { height: 1px; }
/* After */
[lvt-scroll-sentinel] { height: 1px; }

The attribute selector works, but it has lower specificity than an ID selector. This is unlikely to cause issues in this test fixture, but worth noting if the sentinel element ever gets conflicting styles.

3. E2E test — potential race in modal open

chromedp.Click(`[command="show-modal"][commandfor="add-modal"]`, chromedp.ByQuery),
waitFor(`document.querySelector('dialog#add-modal')?.open === true`, 10*time.Second),

The waitFor call is good, but there's no WaitVisible guard on the dialog before WaitVisible on the input:

chromedp.WaitVisible(`input[name="title"]`, chromedp.ByQuery),

If the modal animation is async or the dialog open attribute is set before content is fully rendered, the WaitVisible on the input should be sufficient, but it duplicates the intent of the waitFor above. Consider collapsing these two steps into just the WaitVisible on the input — that transitively implies the dialog is open.

4. Removed formHTML but min() call also removed

The old code used formHTML[:min(500, len(formHTML))]. The variable and its usage are both gone cleanly — good, no dangling references.

5. Dependency gate

The PR correctly notes it depends on livetemplate/client#92. The full E2E suite checkbox is unchecked. This is acceptable for review, but this PR should not be merged until the client package is published — otherwise the generated templates will reference an attribute the client doesn't recognize yet. Consider adding a merge blocker or branch protection note.

---

Summary

The core change is clean, well-scoped, and consistently applied. The E2E test improvements are a genuine quality improvement. The main open question is whether lvt-scroll-sentinel (without data-) is an intentional framework convention — if so, document it; if not, prefer data-lvt-scroll-sentinel. Merge only after livetemplate/client#92 is published.

🤖 Generated with Claude Code