Add demo link, on() shorthand, and version bump to 0.2.0

[fynyky]

No description provided.

[fynyky]

@claude do a code review

[claude]

Claude finished @fynyky's task in 1m 29s —— View job

---

Code Review

• Gather context (read PR diff and changed files)
• Analyze code changes
• Post review feedback

---

Summary

This PR is small, focused, and well-structured. It does three things: adds a demo repo link to the README, introduces an on() shorthand for addEventListener, and bumps the version to 0.2.0. The on() API mirrors the existing attr() pattern cleanly. A new Claude PR review workflow is also added.

Overall: looks good to merge. A few non-blocking observations and suggestions below.

---

src/elemental.js — on() shorthand

export function on (event, fn) {
  return ($) => {
    $.addEventListener(event, fn)
  }
}

The implementation is correct and matches the project's style.

1. No pass-through for addEventListener options (src/elemental.js:251)
The native addEventListener accepts a third argument for options ({ once, capture, passive, signal }). The shorthand silently drops this, so users wanting { once: true } or a passive listener must fall back to the verbose self => self.addEventListener(...) form. A trivial extension would preserve full parity:

export function on (event, fn, options) {
  return ($) => {
    $.addEventListener(event, fn, options)
  }
}

Worth deciding whether you want the shorthand to be a strict subset or a full replacement. Fix this →

2. Footgun when used reactively (potential, not a bug in this PR)
The attr() shorthand has a test demonstrating it works inside ob(() => attr(...)). If a user does the same with on() (e.g. ob(() => on('click', handler))), each observer re-run will add another listener without removing the previous one — leading to duplicate handlers. Since bind() and attr() are documented as reactive-friendly, this asymmetry is a subtle gotcha. Consider adding a one-line note in the README clarifying that on() is intended for static (non-reactive) attachment, or document it via an example.

---

test/test.js — test coverage

it('attaches event listeners using on', () => {
  let clicked = false
  const result = el('button', on('click', () => { clicked = true }))
  result.click()
  assert.equal(clicked, true)
})

The single happy-path test is fine but light compared to the three tests attr() has (single, multiple, reactive). Optional additions:

• Multiple distinct listeners on the same element (e.g. click + mouseenter).
• Handler receives the event object ((e) => ... with e.type === 'click').
• If you adopt the options parameter above, a test for { once: true }.

---

README.md

• Demo link addition (line 50) is clean.
• on is consistently added to all three import examples (lines 66, 78, 689). Good.
• The shorthand section (lines 223–236) follows the same template as attr and bind. Reads well.

Minor: under the "Summary" section at line 762, the example shows on('click', handler) but handler is never defined earlier in the summary — consistent with how bind uses rx, so probably intentional, but a reader skimming just this section might be momentarily confused.

---

.github/workflows/review.yml

• Permissions look correct (contents: read, pull-requests: write, id-token: write).
• fetch-depth: 1 is fine for review since the action fetches the PR base on its own.
• The prompt is generic and reasonable. One nit: the prompt could mention this repo's release-vs-master flow so future reviews use the correct base reference, but that's optional polish.

---

Versioning & lockfile

• 0.1.0 → 0.2.0 is the correct semver bump for an additive new export.
• package-lock.json version field is updated in lockstep. Good.

---

Verdict

LGTM. The only thing I'd genuinely consider before tagging the release is whether to accept the optional options argument in on() — once shipped, adding it later is non-breaking, but locking the API down now without it means users hit the limitation immediately.
• Branch: master

[claude]

Claude finished @fynyky's task in 12m 49s —— View job

---

Code Review

• Gather context (read PR diff and changed files)
• Analyze code changes
• Post review feedback

---

Summary

This PR merges a batch of accumulated changes from master into release in preparation for the 0.2.0 tag. It bundles four logical changes:

1. README — demo repo link
2. on() shorthand for addEventListener
3. Version bump 0.1.0 → 0.2.0
4. GitHub Actions changes — removed old Claude integration, added a secured claude.yml with OWNER/COLLABORATOR gating, added a new review.yml workflow with track_progress

Overall this is safe to merge. The library code is tiny and clean. My only meaningful concern is whether to ship the on() API as-is or extend it with an options parameter before tagging — see the inline comment.

---

Code Quality

• src/elemental.js:251 — on() mirrors the attr() style cleanly and is correct. One non-blocking concern: it drops the native addEventListener options argument ({ once, passive, capture, signal }). I left an inline suggestion with a one-line fix. This is non-breaking to add later, but cleaner to land before the version tag.
• src/index.js:1 — Export list updated consistently with the new function. Good.
• Error handling — Library boundary already validates inputs in el(). on() itself is too thin to need its own validation; trusting the native call is the right call.

Security

• No user input is being parsed in changed code paths — nothing to sanitize at this layer.
• .github/workflows/claude.yml — Adding author_association checks (OWNER / COLLABORATOR) across all four trigger types is the right tightening. Previously any commenter could trigger Claude with an @claude mention. The id-token: write permission is unused here and could be dropped if no OIDC consumer is planned, but it's harmless.
• .github/workflows/review.yml — Same author_association gating at the job level. contents: read + pull-requests: write is the minimum-viable permission set. Good.

Performance / Resources

• No hot paths touched. on() is a one-time attachment helper. No memory leak risk in the helper itself.
• Potential footgun, not a bug in this PR: if a user wraps on() inside an ob() observer, every retrigger adds another listener without removing the previous one — leading to duplicate handlers over time. Unlike bind() (designed reactive) and attr() (idempotent overwrite), on() has additive semantics. Left an inline note on the README suggesting a brief doc warning.

Testing — test/test.js:527

it('attaches event listeners using on', () => {
  let clicked = false
  const result = el('button', on('click', () => { clicked = true }))
  result.click()
  assert.equal(clicked, true)
})

Single happy-path test — adequate but lighter than attr()'s three tests. Worth adding:

• Handler receives the event object: (e) => assert.equal(e.type, 'click')
• Multiple distinct listeners on the same element (e.g. click + mouseenter)
• If options is adopted per the inline suggestion, a test for { once: true }

None of these are blockers.

Documentation

• README demo link (line 50) reads cleanly.
• on added consistently across the three import blocks (lines 66, 78, 689).
• The on() shorthand section (lines 223–237) follows the same template as attr / bind. Good consistency.
• Summary section (line 762) uses an undeclared handler identifier, matching how bind() uses rx — consistent within the file, so probably intentional. Fine.
• No API docs other than README — nothing else to update.

Versioning

• 0.1.0 → 0.2.0 is the correct semver bump for an additive export. package-lock.json updated in lockstep. Good.

Workflow Files

• claude.yml — The OWNER/COLLABORATOR gating is a clear security win.
• review.yml — Works as intended. The prompt is generic and includes focus areas not applicable to this repo (database queries, auth/authz). Left a non-blocking inline comment suggesting it be tailored to this library's actual concern surface (API ergonomics, DOM/listener leaks, reactivity, browser compat, bundle impact). Also worth mentioning that this repo's PR base is release, not master, so future reviews use the right diff reference.

Verdict

LGTM — safe to merge. The only thing I'd genuinely consider before tagging the release is the on(event, fn, options) extension — once 0.2.0 ships without it, adding it later is technically non-breaking, but locking the API down at parity with the native call now is cheaper than apologizing for it later.

• Branch: master

[claude]

Missing pass-through for addEventListener options

The native addEventListener accepts a third argument ({ once, capture, passive, signal }). The current shorthand silently drops it, so any user wanting { once: true }, a passive scroll listener, or an AbortSignal-backed listener has to fall back to the verbose form:

self => self.addEventListener('click', fn, { once: true })

A trivial extension would keep full parity with the native API:

Suggested change

	export function on (event, fn) {
	return ($) => {
	$.addEventListener(event, fn)
	}
	}
	export function on (event, fn, options) {
	return ($) => {
	$.addEventListener(event, fn, options)
	}
	}

This is non-breaking to add later, but worth deciding before tagging 0.2.0 — shipping without it makes the shorthand a strict subset of the API it shadows, which users will hit immediately. If you do add it, also extend the test in test/test.js to cover { once: true }.

[claude]

Consider documenting the reactivity footgun

Unlike bind() (which is designed to be reactive) and attr() (which idempotently overwrites on re-run), on() adds a listener every time it executes. If a user wraps it in an observer:

el('button', ob(() => on('click', handler)))

…every observer retrigger attaches another copy of handler without removing the previous, so clicks fire 2×, 3×, N× over time. This is subtle because attr() shown earlier in the docs does work reactively.

A one-line note here clarifying that on() is intended for static (non-reactive) attachment would prevent surprises. Same goes for the Summary section at line 762.

[claude]

Prompt is generic for a UI library

The review prompt includes focus areas that don't apply to this repo at all (database queries for efficiency, authentication/authorization logic). For a small client-side DOM library these prompts mostly produce noise — see how this very review had to spend space confirming "no auth, no DB, no input sanitization concerns."

Consider tightening to areas that actually matter here: API ergonomics, DOM/memory leak risk (observers, listeners), reactivity edge cases, browser compatibility, and bundle impact. Also worth mentioning that this repo's PR base is typically release (not master), so future reviews use the right reference.

Optional / non-blocking — the workflow itself works fine.

[claude]

Claude finished @fynyky's task in 1m 58s —— View job

---

Code Review

• Gather context (read PR diff and changed files)
• Analyze code changes
• Post review feedback

---

Summary

This is the third review pass on PR #43 — the new commit since the last review is 80fe65d adding .github/workflows/security-audit.yml. The library-code changes (on() shorthand, version bump, README) are unchanged and still safe to merge. My prior inline comments on those still apply — none are blockers, but the on(event, fn, options) decision is the one thing genuinely worth resolving before tagging 0.2.0.

This review focuses on the new security-audit workflow.

---

.github/workflows/security-audit.yml (new)

Overall this is a well-constructed workflow — SHA-pinned actions, minimal permissions, prompt-injection guardrails, sandboxed network scope, artifact upload for audit trail. The structure is solid. A few notes:

1. Inconsistent dependency pinning (security-audit.yml:24)

run: python3 -m pip install --quiet semgrep==1.164.0 pip-audit

semgrep is pinned to 1.164.0 but pip-audit is unpinned. If the goal of pinning is reproducibility and supply-chain hygiene, both should be pinned (and ideally both should also use a hashed lockfile). Small thing, but reads as an oversight rather than a deliberate choice.
Fix this →

2. --allowedTools "Bash,WebSearch" is broad (security-audit.yml:136)
Bash with no command restrictions grants the agent unrestricted shell on the runner. Given the job has issues: write, a successful prompt-injection bypass of the "untrusted data" guard at line 42 could create or close arbitrary issues, or exfiltrate via gh api. The prompt explicitly warns the agent not to follow embedded instructions, which is the right mitigation — but for a defense-in-depth posture you could either:

• Restrict to explicit command prefixes the prompt actually needs (Bash(npm audit:*),Bash(semgrep:*),Bash(pip-audit:*),Bash(gh issue list:*),Bash(gh issue create:*),Bash(gh label create:*)), or
• Accept the current scope and document the trust assumption.

Not blocking — just worth a deliberate decision.

3. Prompt steps don't all fit this repo (security-audit.yml:84-95)
Same concern I flagged on review.yml: this is a tiny client-side DOM library with one runtime dependency (reactorjs). Steps like "dynamic analysis — craft payloads, exercise code paths the existing tests miss" don't really map here — there's no input parsing, no auth, no network surface. The audit will likely produce a clean "nothing to report" most weeks, which is fine, but a tightened prompt targeting this library's actual concern surface (dependency CVEs in reactorjs and transitive deps, prototype pollution via el() descriptors, XSS via reactive bind() paths) would produce sharper results.

4. Trailing newline on claude.yml (.github/workflows/claude.yml:62)
The diff shows \ No newline at end of file was reintroduced. Cosmetic, but most repos prefer files to end with a newline.

5. Concurrency setup is good (security-audit.yml:8-10)
cancel-in-progress: false is the right choice for a long-running audit — you don't want a manual workflow_dispatch to abort an in-flight scheduled run.

6. Artifact glob audit-*.json (security-audit.yml:143)
Matches the three files the prompt names (audit-deps.json, audit-semgrep.json) — but the prompt itself doesn't require the agent write those specific names; the upload step silently uploads nothing if the agent improvises filenames. Tiny risk of "audit ran, no artifacts retained." If you want guaranteed evidence, instruct the agent in the prompt that the exact filenames audit-deps.json and audit-semgrep.json are required for artifact retention.

---

Recap of still-applicable points from earlier reviews

on() shorthand — Still no pass-through for native addEventListener options ({ once, capture, passive, signal }). Inline comment at src/elemental.js:255 stands. The only thing I'd genuinely flag before tagging 0.2.0.

Reactivity footgun in on() — Wrapping on() in ob() accumulates duplicate listeners on every retrigger. Documenting this in the README (inline comment at README.md:237) would prevent surprises. Non-blocking.

review.yml prompt scope — Still generic (mentions DB queries, auth/authz) for a client-side DOM library. Inline comment stands. Non-blocking.

---

Verdict

LGTM — safe to merge. Of everything raised across the three review passes, only one decision is worth making before the version tag: whether on() accepts an options parameter. Everything else (security-audit pinning, prompt tightening, doc warnings) is fine to land in a follow-up.

• Branch: master
•