Repository navigation
Conversation
Adds a Reveal component: a lightweight, CSS-driven wrapper backed by IntersectionObserver that animates content into view as it scrolls into the viewport (fade, fade-up, fade-down, slide-left, slide-right, zoom, flip), with an optional cascade mode that staggers direct children. Fixes #197 Co-authored-by: Alex Smith <allxsmith@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new ChangesReveal Component
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Preview DeploymentPreview URL: https://91707a07.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return ( | ||
| <div ref={setNode} className={combinedClasses} style={wrapperStyle}> | ||
| <Component {...rest}>{content}</Component> | ||
| </div> |
There was a problem hiding this comment.
Custom-component as splits props across two elements — 🟡 Minor · API
What: When as is a component (not a string), className, style, and the Bulma helper classes land on the internal wrapper div, while everything else in ...rest (id, aria-*, data-*, event handlers) lands on the inner <Component>. For the string-as path (line 231) all of these are unified on the single rendered element.
Why it matters: This is inconsistent with the rest of the library, where a polymorphic as puts the full prop set on one element. A caller writing <Reveal as={Section} id="hero" className="highlight" m="4"> gets id on the <section> but highlight/m-4 on the wrapper div — so a combined selector like #hero.highlight matches nothing, and consumers can't reason about which element carries their class/style vs. their id/aria. It's easy to hit without realizing why styling "doesn't apply."
Why it matters less: Keeping id/aria/events on the semantic inner element is arguably correct, and moving the animation classes off the observed node isn't trivial. So this may be an intentional trade-off — but it should at least be documented on the as prop (in the docs page's props table and/or the JSDoc) so the asymmetry isn't a surprise.
Prop distribution for as={Section}
flowchart TD
R["<Reveal as={Section} id className style m='4' onClick>"]
R --> D["div.reveal (observed node)\nclassName + helper classes + style"]
D --> S["Section\n...rest: id, aria-*, data-*, onClick"]
No code change strictly required; recommend a one-line note on the as prop describing where user className/style land when a component is passed.
There was a problem hiding this comment.
Fixed in eb781a7 — I took the documentation route you recommended rather than restructuring the render, since keeping id/aria/events on the semantic inner element (and the animation classes on the observed node) is the intentional trade-off here, not a bug.
I documented the asymmetry in two places:
- The
asprop's JSDoc inReveal.tsxnow spells out that for a plain intrinsic tag everything lands on the single element, whereas for a componentclassName/style/helper classes go on the observed wrapperdivand...rest(id,aria-*,data-*, event handlers) is forwarded to the inner component. - The docs page (
docs/docs/api/components/reveal.md) gains a:::noteright under the "Rendering as a different element" example making the same point, and it explicitly calls out your#hero.highlightgotcha — a combined selector assumingidandclassNamesit on the same element won't match whenasis a component.
I left the runtime behavior unchanged: moving the animation classes off the observed node isn't free, and unifying every prop onto the inner component would require ref-forwarding that this library's components don't provide, which is exactly the fallback the wrapper div exists to handle.
There was a problem hiding this comment.
Deep review — 1 finding
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | API | Custom-component as splits className/style/helper-classes (wrapper div) from id/aria/data/events (inner component); inconsistent with the rest of the library and undocumented |
bulma-ui/src/components/Reveal.tsx:243 |
Overall: The change is sound and unusually complete for a new component — it ships all five anatomy artifacts (component, tests, story, docs page, export) plus the SCSS partial and a regenerated skill catalog, and lands 100% statement/branch/function/line coverage on Reveal.tsx (23 passing tests). The genuinely hard parts are handled well: the isMounted gate keeps SSR/first-client render identical so there's no hydration mismatch, prefers-reduced-motion and the IntersectionObserver-absent case both fall back to the visible final state, and the callback-ref-as-state pattern correctly re-runs the observer effect once the node attaches. The riskiest/most subtle area is the polymorphic as handling — where the human should focus first — but the only real wrinkle there is the prop-distribution asymmetry noted above, which is arguably intentional and just needs a doc note rather than a code fix. No correctness, accessibility, or coverage defects found.
🏄 Total glassy set, dude — this Reveal wave rolls in clean: SSR-safe, reduced-motion-friendly, 100% covered, no wipeouts. Just one tiny ripple on the
asprop worth a heads-up in the docs, then paddle it out to a human. Good to go, brah.
![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
🧹 Nitpick comments (1)
bulma-ui/src/scss/components/_reveal.scss (1)
38-43: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
will-changeis never reset after reveal.
will-change: opacity, transformis applied to the hidden state but not cleared in the.is-revealedfinal state, so browsers keep a compositing layer alive for every revealed element indefinitely. This is more impactful withcascademode where many children can accumulate persistent layers.♻️ Suggested fix: reset will-change once revealed
.#{iv.$class-prefix}reveal-fade.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-fade-up.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-fade-down.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-slide-left.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-slide-right.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-zoom.#{iv.$class-prefix}is-revealed, .#{iv.$class-prefix}reveal-flip.#{iv.$class-prefix}is-revealed { opacity: 1; transform: none; + will-change: auto; }Also applies to: 70-79
🤖 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 `@bulma-ui/src/scss/components/_reveal.scss` around lines 38 - 43, `will-change: opacity, transform` is left on the hidden reveal state and never cleared, so update the reveal styles in `_reveal.scss` to reset `will-change` in the final `.is-revealed` state (and any cascade-related revealed child state) instead of keeping it active after the transition. Use the existing reveal selectors and mixin-generated rules for the hidden/revealed states to ensure the compositing hint is only applied during animation.
🤖 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 `@bulma-ui/src/components/Reveal.tsx`:
- Line 46: RevealProps.threshold can currently pass invalid values into the
IntersectionObserver setup in Reveal, causing the effect to throw for NaN or
values outside 0–1. Update the observer initialization path in Reveal to
validate or clamp threshold before creating the observer, and fall back to a
safe default when the prop is invalid. Make sure the fix is applied wherever the
observer is constructed in the Reveal component logic.
---
Nitpick comments:
In `@bulma-ui/src/scss/components/_reveal.scss`:
- Around line 38-43: `will-change: opacity, transform` is left on the hidden
reveal state and never cleared, so update the reveal styles in `_reveal.scss` to
reset `will-change` in the final `.is-revealed` state (and any cascade-related
revealed child state) instead of keeping it active after the transition. Use the
existing reveal selectors and mixin-generated rules for the hidden/revealed
states to ensure the compositing hint is only applied during animation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 83a563c8-9b70-4552-8e35-a320f9559a6f
📒 Files selected for processing (8)
bulma-ui/src/components/Reveal.stories.tsxbulma-ui/src/components/Reveal.tsxbulma-ui/src/components/__tests__/Reveal.test.tsxbulma-ui/src/index.tsbulma-ui/src/scss/components/_index.scssbulma-ui/src/scss/components/_reveal.scssdocs/docs/api/components/reveal.mdskills/bestax-custom-component/references/component-catalog.md
|
AI loop status: iteration 1/4 — |
|
AI loop stopped — contested findings. The fix agent pushed no changes and 2 review thread(s) remain open (refuted or unaddressable). A human ruling is needed; see the fix agent's summary above. |
github-actions
Bot
added
needs-human-review
…recipe (#248) On PR #247 the fix agent ran clean (opus, 24 turns, success) but hit 15 permission denials and produced ZERO output — no commits, no thread replies, no recap — so the loop escalated with the findings unaddressed. Same class as the original #238 thrash: the agent reached for a tool that is not allowlisted and gave up. The execution-output artifact that would name the tool is defeated by the proxy blocking its blob-storage host, and the job log hides per-turn detail by default. Two fixes: - show_full_output: true on the fix AND verify jobs, so every tool call and denial lands in the readable job log (no artifact needed). - Give both agents the explicit gh-api thread-reply recipe (addPullRequestReviewThreadReply) and state plainly that there is NO MCP 'reply' tool here — only mcp__github_inline_comment__create_inline_comment for a NEW comment. Reaching for a nonexistent MCP reply tool is the most likely denial source; this makes the reply path deterministic. Claude-Session: https://claude.ai/code/session_01NVR5yWevceZEFbTJmpviiM Co-authored-by: Claude <noreply@anthropic.com>
allxsmith
added
ai-loop
Claude
…stribution Clamp the threshold prop into the 0-1 range and fall back to the 0.15 default for non-finite values before handing it to IntersectionObserver, which otherwise throws a RangeError for NaN or out-of-range thresholds. Adds tests for the clamp-high, clamp-low, and non-finite fallback paths (Reveal.tsx stays at 100% coverage). Also documents, on the as-prop JSDoc and the docs page, where a caller's className/style/helper classes vs. id/aria/data/event-handler props land when as is a component (observed wrapper div) vs. a plain intrinsic tag (single element). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix pass recap (iteration 1)Both open AI review threads were legitimate and in scope, so both are addressed in Fixed
RefutedNone — nothing this pass was out of scope or stale. Gates run
|
Preview DeploymentPreview URL: https://521535d0.bestax.pages.dev |
* ci: log full fix/verify output + tell agents the gh-api thread-reply recipe On PR #247 the fix agent ran clean (opus, 24 turns, success) but hit 15 permission denials and produced ZERO output — no commits, no thread replies, no recap — so the loop escalated with the findings unaddressed. Same class as the original #238 thrash: the agent reached for a tool that is not allowlisted and gave up. The execution-output artifact that would name the tool is defeated by the proxy blocking its blob-storage host, and the job log hides per-turn detail by default. Two fixes: - show_full_output: true on the fix AND verify jobs, so every tool call and denial lands in the readable job log (no artifact needed). - Give both agents the explicit gh-api thread-reply recipe (addPullRequestReviewThreadReply) and state plainly that there is NO MCP 'reply' tool here — only mcp__github_inline_comment__create_inline_comment for a NEW comment. Reaching for a nonexistent MCP reply tool is the most likely denial source; this makes the reply path deterministic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVR5yWevceZEFbTJmpviiM * ci: bestaxbot signs off the terminal states in surfer dialect The loop's human-facing terminal comments (converged / contested / paused / wipeout) were posted by github-actions[bot] in flat prose. Give them a personality: bestaxbot now authors them (via AI_LOOP_PAT; the label + reviewer edits stay on the job token), in a Californian/Hawaiian surfer voice with an AI-superiority streak — 'clean set, total convergence, need a meat sack to hit merge.' Covers all five terminal comments: converged handoff, contested findings, halt (cap/protected/review-failed/cr-stalled), fix-run wipeout, and verify no-progress. Only the comment author + wording change; labels, reviewers, and the machine-parsed ai-loop-state comment are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NVR5yWevceZEFbTJmpviiM --------- Co-authored-by: Claude <noreply@anthropic.com>
… MCP (#250) Implement run #18 fully built the Reveal component (gates green, 100% coverage) but produced no branch/PR: it reached for mcp__github_file_ops__commit_files, which is not allowlisted, and gave up blocked — even though the git-CLI commit/push verbs ARE allowlisted and are what runs #16/#17 used to open #243 and #247. Non-deterministic tool choice, same class as the fixer reaching for a nonexistent MCP reply tool. - Rewrite step 4 with an explicit git-CLI commit+push recipe (git checkout -b / add / commit / push) and state plainly that there is NO MCP commit tool here — git is already set up to SSH-sign as bestaxbot, so git-CLI commits come out Verified. - Add show_full_output so a blocked commit is visible in the log, not just an artifact the proxy won't let us download. Claude-Session: https://claude.ai/code/session_01NVR5yWevceZEFbTJmpviiM Co-authored-by: Claude <noreply@anthropic.com>
2 participants
Summary
Adds a
Revealcomponent: a lightweight, CSS-driven wrapper backed byIntersectionObserverthat animates content into view as it scrolls into theviewport (
fade,fade-up,fade-down,slide-left,slide-right,zoom,flip), with an optionalcascademode that staggers direct children with anincrementing delay.
Accessibility/progressive-enhancement are built in, not opt-in:
the user prefers reduced motion (
prefers-reduced-motion: reduce).so content is never hidden if JavaScript never runs (crawlers, no-JS).
IntersectionObserverisn't available.
Passing a plain (non-
forwardRef) component asas— which is how everycomponent in this library, including
Section/Card, is written — wouldotherwise silently break scroll detection, since the ref never attaches and
content would stay hidden forever. This is handled by falling back to an
internal wrapper
divfor scroll observation wheneverasisn't a plainintrinsic HTML tag, with a regression test covering it
(
Reveal.test.tsx: "still observes and reveals whenasis a non-forwardRefcomponent").
Fixes #197
Changes
bulma-ui/src/components/Reveal.tsx— the componentbulma-ui/src/components/Reveal.stories.tsx— Storybook stories (includinga scroll-to-reveal demo)
bulma-ui/src/components/__tests__/Reveal.test.tsx— 22 tests, 100% coveragebulma-ui/src/scss/components/_reveal.scss— animation styles, registeredin
_index.scssdocs/docs/api/components/reveal.md— API docs pagebulma-ui/src/index.ts— public exportskills/bestax-custom-component/references/component-catalog.md—regenerated via
pnpm gen:catalogTest plan
pnpm --filter @allxsmith/bestax-bulma exec tsc --noEmitpnpm --filter @allxsmith/bestax-bulma run lintpnpm --filter @allxsmith/bestax-bulma run test:coverage(100% onReveal.tsx, no threshold regressions; 3350 tests passing)pnpm run format:checkpnpm run gen:catalog:checkGenerated with Claude Code
Summary by CodeRabbit
Revealcomponent for scroll-triggered entrance animations, including cascade staggering,oncebehavior, and flexible rendering via theasprop.Revealand updated the component catalog to include it.