Repository navigation
Aurora shimmer cards 6128441558600978070 follow up - #203
Closed
alehar9320 wants to merge 7 commits into
Closed
alehar9320 wants to merge 7 commits into
alehar9320 wants to merge 7 commits into
Conversation
- Competitive Signal: Vercel/Linear style hover effects. - User Emotion Targeted: Joy, surprise. - Implementation: - Lines of code: 12 (in PortfolioPreview.astro) - Feature flag: shimmer - Verification: - format: ✅ - lint: ✅ - astro check: ✅ - test: ✅ - build: ✅ Co-authored-by: alehar9320 <5390430+alehar9320@users.noreply.github.com>
- Replaced direct 'cloudflare:workers' import with 'locals.runtime.env' access for portability. - Updated 'src/__tests__/chat-api.test.ts' to provide the mock environment via 'locals.runtime.env'. - Verified build with 'RENDER=true npm run build'. - Verified all tests pass. Co-authored-by: alehar9320 <5390430+alehar9320@users.noreply.github.com>
Competitive Signal: Vercel/Linear card effects. User Emotion Targeted: Surprise. Implementation: - Lines of code: 11 (across components, schemas, and flags). - Feature flag: shimmer. - Experimental route: N/A. Verification: - format: ✅ - lint: ✅ - astro check: ✅ - test: ✅ - build: ✅ Abort Triggers: None. Co-authored-by: alehar9320 <5390430+alehar9320@users.noreply.github.com>
Delight: Shimmering Cards Micro-Interaction Competitive Signal: Vercel/Linear card effects. User Emotion Targeted: Surprise. Implementation: - Lines of code: 11 (micro-interaction). - Feature flag: shimmer. - Experimental route: N/A. Infrastructure: Render Build Fix - Added Vite alias for 'cloudflare:workers' to resolve Rollup import errors on non-Cloudflare platforms. - Verified fix with local Node adapter build. Verification: - format: ✅ - lint: ✅ - astro check: ✅ - test: ✅ - build: ✅ (Cloudflare & Node) - visual: ✅ (Overflow hidden applied) Co-authored-by: alehar9320 <5390430+alehar9320@users.noreply.github.com>
Delight: Shimmering Cards Micro-Interaction - Implemented CSS shimmer overlay on portfolio cards. - Added feature flag validation via Zod. - Fixed visual bleed with overflow: hidden. Infrastructure: Render Build Fix - Added Vite alias for 'cloudflare:workers' to mock during Render/Node builds. - Fixed Prettier formatting in astro.config.mjs. Verification: - format: ✅ - lint: ✅ - astro check: ✅ - test: ✅ - build: ✅ (Cloudflare & Node/Render) Co-authored-by: alehar9320 <5390430+alehar9320@users.noreply.github.com>
Delight: Shimmering Cards Micro-Interaction - Implemented CSS shimmer overlay on portfolio cards. - Added feature flag validation via Zod. - Fixed visual bleed with overflow: hidden. Infrastructure: Render Build Fix - Added Vite alias for 'cloudflare:workers' to mock during Render/Node builds. - Improved alias path resolution using `fileURLToPath` for cross-platform compatibility. - Fixed Prettier formatting in astro.config.mjs. Verification: - format: ✅ - lint: ✅ - astro check: ✅ - test: ✅ - build: ✅ (Cloudflare & Node/Render Target) - visual: ✅ (Overflow hidden applied) Co-authored-by: alehar9320 <5390430+alehar9320@users.noreply.github.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
alehar9320
temporarily deployed
to
aurora-shimmer-cards-6128441558600978070-follow-up - astro-cloudflare-personal-website-web-service PR #203
May 21, 2026 18:11 — with 
Render
Destroyed
Render
Destroyed
📝 WalkthroughWalkthroughPR adds PostHog analytics singleton client, refactors chat API to use direct Cloudflare ChangesPostHog Analytics Integration
Chat API Environment Binding Refactor
Shimmer UI Effect for Portfolio Cards
Public API Exports Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle ReportBundle size has no change ✅ Affected Assets, Files, and Routes:view changes for bundle: molecular-mars-esmAssets Changed:
Files in
|
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
me | 128d32a | Commit Preview URL Branch Preview URL |
May 21 2026, 06:12 PM |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/lib/posthog.test.ts (1)
17-22: ⚡ Quick winAdd coverage for the "empty key" branch.
The source file has a dedicated branch that's meant to log an error when the key is present but invalid (e.g., empty string), but there's currently no test exercising
PUBLIC_POSTHOG_KEY=''. Adding such a test would both lock in the intent and surface the bug noted onposthog.ts(theif (env.key)truthy-check actually skips the empty-string case, contrary to the inline comment there).♻️ Suggested additional test
+ it('returns null without logging when the PostHog key is an empty string', async () => { + vi.stubEnv('PUBLIC_POSTHOG_KEY', ''); + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const { getPostHogClient } = await import('./posthog'); + + expect(getPostHogClient()).toBeNull(); + // Decide intended behavior: should this log? Currently it does NOT, + // which contradicts the comment in posthog.ts. + expect(consoleSpy).not.toHaveBeenCalled(); + expect(PostHogMock).not.toHaveBeenCalled(); + });🤖 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 `@src/lib/posthog.test.ts` around lines 17 - 22, Add a new test that exercises the "empty key" branch by setting process.env.PUBLIC_POSTHOG_KEY = '' before importing the module, then import getPostHogClient (use jest.resetModules() or jest.isolateModules to ensure the env is read on import), call getPostHogClient() and assert the expected behavior (e.g., returns null) and that the error logging path was invoked (verify PostHogMock was not called and the logger/error function was called); reference the getPostHogClient function and PostHogMock to locate where to add assertions.src/lib/posthog.ts (2)
29-35: 💤 Low valueSingleton caches config from the first successful call.
posthogClientis created once and reused even ifPUBLIC_POSTHOG_KEY/PUBLIC_POSTHOG_HOSTchange later (e.g., during HMR or test runs that don't reset the module). Tests already work around this viavi.resetModules(), so consumers in production are unaffected — flagging so reviewers know this is intentional.Also note: with
flushAt: 1/flushInterval: 0, everycapturetriggers an immediate send. In Cloudflare Workers you'll likely still needctx.waitUntil(client.flush())(orshutdown()) at the end of each request to guarantee in-flight requests aren't dropped when the isolate is suspended. Not a defect here, but worth ensuring at call sites.🤖 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 `@src/lib/posthog.ts` around lines 29 - 35, The singleton posthogClient is created once and reused even if the incoming key/host change; update the factory so it detects config changes and recreates the client when needed: read the current key and host values and if posthogClient exists but its storedKey/storedHost (add small internal vars like cachedPosthogKey and cachedPosthogHost) differ from the current key/host, call posthogClient.shutdown()/flush() if available, then replace it with a new PostHog(key, { host, flushAt: 1, flushInterval: 0 }); alternatively expose a resetPosthogClient() helper to clear the cached client for tests; reference the posthogClient variable and the new PostHog(...) constructor when implementing this change.
6-9: 💤 Low valueUse Zod v4
z.url()forhost
z.string().url()is deprecated in Zod v4; prefer the standalonez.url()format schema.♻️ Optional refactor
const PostHogConfigSchema = z.object({ key: z.string().min(1), - host: z.string().url().optional(), + host: z.url().optional(), });🤖 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 `@src/lib/posthog.ts` around lines 6 - 9, Replace the deprecated z.string().url() usage in the PostHogConfigSchema by using the Zod v4 url schema: update the host field in PostHogConfigSchema to use z.url().optional() instead of z.string().url(), ensuring the schema still validates optional URLs; (make the change where PostHogConfigSchema is declared and keep the key field as-is).
🤖 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 `@package.json`:
- Line 31: The PR introduced a new top-level runtime dependency "posthog-node"
in package.json which violates the rule that new top-level deps require explicit
permission; either remove/revert that dependency from package.json (or change it
to a dev-only dependency if appropriate) or update the PR with explicit approval
from the designated approver (add the approver's sign-off in the PR description)
before merging; locate the change by inspecting the package.json entry
"posthog-node" and act accordingly.
In `@src/components/PortfolioPreview.astro`:
- Around line 86-100: Update the infinite hover shimmer so it respects users'
reduced-motion preference: wrap the animation and related properties in a
prefers-reduced-motion: no-preference media query, rename the `@keyframes` from
the single-letter identifier `s` to a descriptive name like `shimmer-slide`, and
add `pointer-events: none` to `.shimmer:hover::after` so the overlay can't block
nested element interactions; ensure the animation property on
`.shimmer:hover::after` references the new `shimmer-slide` keyframes.
In `@src/lib/posthog.test.ts`:
- Around line 1-65: The comment in getPostHogClient about "Only log if the key
is present but invalid (e.g. empty string)" is out of sync with the code because
the current conditional uses if (env.key) which treats '' as falsy and prevents
logging; update the conditional in the getPostHogClient implementation to
explicitly check for undefined (e.g., if (env.key !== undefined)) so an empty
string will still trigger the error logging path, and keep the comment as-is (or
instead update the comment to match whichever behavior you choose).
---
Nitpick comments:
In `@src/lib/posthog.test.ts`:
- Around line 17-22: Add a new test that exercises the "empty key" branch by
setting process.env.PUBLIC_POSTHOG_KEY = '' before importing the module, then
import getPostHogClient (use jest.resetModules() or jest.isolateModules to
ensure the env is read on import), call getPostHogClient() and assert the
expected behavior (e.g., returns null) and that the error logging path was
invoked (verify PostHogMock was not called and the logger/error function was
called); reference the getPostHogClient function and PostHogMock to locate where
to add assertions.
In `@src/lib/posthog.ts`:
- Around line 29-35: The singleton posthogClient is created once and reused even
if the incoming key/host change; update the factory so it detects config changes
and recreates the client when needed: read the current key and host values and
if posthogClient exists but its storedKey/storedHost (add small internal vars
like cachedPosthogKey and cachedPosthogHost) differ from the current key/host,
call posthogClient.shutdown()/flush() if available, then replace it with a new
PostHog(key, { host, flushAt: 1, flushInterval: 0 }); alternatively expose a
resetPosthogClient() helper to clear the cached client for tests; reference the
posthogClient variable and the new PostHog(...) constructor when implementing
this change.
- Around line 6-9: Replace the deprecated z.string().url() usage in the
PostHogConfigSchema by using the Zod v4 url schema: update the host field in
PostHogConfigSchema to use z.url().optional() instead of z.string().url(),
ensuring the schema still validates optional URLs; (make the change where
PostHogConfigSchema is declared and keep the key field as-is).
🪄 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: 953cec8d-871b-4d2d-822a-1b27ebebb17d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.Aurora/delights.md.Pruning/metrics.md.wrangler/deploy/config.jsonastro.config.mjspackage.jsonsrc/__tests__/chat-api.test.tssrc/components/PortfolioPreview.astrosrc/components/__tests__/example.test.tssrc/content/delights.jsonsrc/content/schemas/delights.tssrc/lib/posthog.test.tssrc/lib/posthog.tssrc/pages/api/chat.tssrc/utils/github-releases.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- .Pruning/metrics.md
| "@sentry/cloudflare": "^10.50.0", | ||
| "astro": "^6.3.3", | ||
| "posthog-js": "^1.372.6", | ||
| "posthog-node": "^5.33.0", |
There was a problem hiding this comment.
New top-level dependency — confirm explicit permission.
Adding posthog-node introduces a new runtime dependency. Per the repo guidelines, new top-level dependencies require explicit permission (Zod is the only standing exception). Please confirm this addition was pre-approved before merging.
As per coding guidelines: "NEVER add new top-level dependencies without explicit permission (except Zod which is a core dependency)".
🤖 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 `@package.json` at line 31, The PR introduced a new top-level runtime
dependency "posthog-node" in package.json which violates the rule that new
top-level deps require explicit permission; either remove/revert that dependency
from package.json (or change it to a dev-only dependency if appropriate) or
update the PR with explicit approval from the designated approver (add the
approver's sign-off in the PR description) before merging; locate the change by
inspecting the package.json entry "posthog-node" and act accordingly.
Comment on lines
+86
to
+100
| .shimmer:hover::after { | ||
| content: ''; | ||
| position: absolute; | ||
| inset: 0; | ||
| background: linear-gradient(90deg, transparent, rgba(255, 255, 255, 0.2), transparent); | ||
| animation: s 1.2s infinite; | ||
| } | ||
| @keyframes s { | ||
| 0% { | ||
| transform: translateX(-100%); | ||
| } | ||
| 100% { | ||
| transform: translateX(100%); | ||
| } | ||
| } |
There was a problem hiding this comment.
Respect prefers-reduced-motion for the infinite shimmer.
The hover shimmer runs an infinite translate animation with no reduced-motion guard. Users who have requested reduced motion (vestibular/motion sensitivity) will still see the looping effect. Gate the animation behind a media query so it only animates when motion is allowed.
♿ Proposed fix
.shimmer:hover::after {
content: '';
position: absolute;
inset: 0;
+ pointer-events: none;
background: linear-gradient(90deg, transparent, rgba(255, 255, 255, 0.2), transparent);
- animation: s 1.2s infinite;
+ animation: shimmer-sweep 1.2s infinite;
}
- `@keyframes` s {
+ `@keyframes` shimmer-sweep {
0% {
transform: translateX(-100%);
}
100% {
transform: translateX(100%);
}
}
+ `@media` (prefers-reduced-motion: reduce) {
+ .shimmer:hover::after {
+ animation: none;
+ }
+ }Also renames the keyframe from the single-letter s to a self-describing identifier and adds pointer-events: none so the overlay can never interfere with hit-testing on nested elements.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .shimmer:hover::after { | |
| content: ''; | |
| position: absolute; | |
| inset: 0; | |
| background: linear-gradient(90deg, transparent, rgba(255, 255, 255, 0.2), transparent); | |
| animation: s 1.2s infinite; | |
| } | |
| @keyframes s { | |
| 0% { | |
| transform: translateX(-100%); | |
| } | |
| 100% { | |
| transform: translateX(100%); | |
| } | |
| } | |
| .shimmer:hover::after { | |
| content: ''; | |
| position: absolute; | |
| inset: 0; | |
| pointer-events: none; | |
| background: linear-gradient(90deg, transparent, rgba(255, 255, 255, 0.2), transparent); | |
| animation: shimmer-sweep 1.2s infinite; | |
| } | |
| `@keyframes` shimmer-sweep { | |
| 0% { | |
| transform: translateX(-100%); | |
| } | |
| 100% { | |
| transform: translateX(100%); | |
| } | |
| } | |
| `@media` (prefers-reduced-motion: reduce) { | |
| .shimmer:hover::after { | |
| animation: none; | |
| } | |
| } |
🤖 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 `@src/components/PortfolioPreview.astro` around lines 86 - 100, Update the
infinite hover shimmer so it respects users' reduced-motion preference: wrap the
animation and related properties in a prefers-reduced-motion: no-preference
media query, rename the `@keyframes` from the single-letter identifier `s` to a
descriptive name like `shimmer-slide`, and add `pointer-events: none` to
`.shimmer:hover::after` so the overlay can't block nested element interactions;
ensure the animation property on `.shimmer:hover::after` references the new
`shimmer-slide` keyframes.
Comment on lines
+1
to
+65
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import type { Mock } from 'vitest'; | ||
|
|
||
| const PostHogMock = vi.fn(); | ||
|
|
||
| vi.mock('posthog-node', () => ({ | ||
| PostHog: PostHogMock, | ||
| })); | ||
|
|
||
| describe('getPostHogClient', () => { | ||
| beforeEach(() => { | ||
| PostHogMock.mockClear(); | ||
| vi.resetModules(); | ||
| vi.unstubAllEnvs(); | ||
| }); | ||
|
|
||
| it('returns null when the PostHog key is missing', async () => { | ||
| const { getPostHogClient } = await import('./posthog'); | ||
|
|
||
| expect(getPostHogClient()).toBeNull(); | ||
| expect(PostHogMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('returns null and logs an error when the PostHog host is invalid', async () => { | ||
| vi.stubEnv('PUBLIC_POSTHOG_KEY', 'test-key'); | ||
| vi.stubEnv('PUBLIC_POSTHOG_HOST', 'not-a-url'); | ||
| const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
|
||
| const { getPostHogClient } = await import('./posthog'); | ||
|
|
||
| expect(getPostHogClient()).toBeNull(); | ||
| expect(consoleSpy).toHaveBeenCalled(); | ||
| expect(PostHogMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('creates a PostHog client with the configured env vars', async () => { | ||
| vi.stubEnv('PUBLIC_POSTHOG_KEY', 'test-key'); | ||
| vi.stubEnv('PUBLIC_POSTHOG_HOST', 'https://eu.i.posthog.com'); | ||
|
|
||
| const { getPostHogClient } = await import('./posthog'); | ||
|
|
||
| const client = getPostHogClient(); | ||
|
|
||
| expect(PostHogMock).toHaveBeenCalledTimes(1); | ||
| expect(PostHogMock).toHaveBeenCalledWith('test-key', { | ||
| host: 'https://eu.i.posthog.com', | ||
| flushAt: 1, | ||
| flushInterval: 0, | ||
| }); | ||
| expect(client).toBe((PostHogMock as Mock).mock.results[0]?.value); | ||
| }); | ||
|
|
||
| it('reuses the cached PostHog client for repeated calls', async () => { | ||
| vi.stubEnv('PUBLIC_POSTHOG_KEY', 'test-key'); | ||
| vi.stubEnv('PUBLIC_POSTHOG_HOST', 'https://eu.i.posthog.com'); | ||
|
|
||
| const { getPostHogClient } = await import('./posthog'); | ||
|
|
||
| const firstClient = getPostHogClient(); | ||
| const secondClient = getPostHogClient(); | ||
|
|
||
| expect(firstClient).toBe(secondClient); | ||
| expect(PostHogMock).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Root cause note: comment vs. behavior mismatch in posthog.ts.
Cross-referencing with src/lib/posthog.ts lines 19-25: the comment says "Only log if the key is present but invalid (e.g. empty string)", but if (env.key) treats '' as falsy and therefore does not log on an empty string — the opposite of what the comment promises. Either tighten the check to if (env.key !== undefined) or update the comment to match. Mentioning here since the test suite is the natural place this would have been caught.
🐛 Suggested fix in src/lib/posthog.ts
if (!result.success) {
- // Only log if the key is present but invalid (e.g. empty string)
- if (env.key) {
+ // Only log if the key is present (including empty string) but invalid
+ if (env.key !== undefined) {
console.error('Invalid PostHog configuration:', result.error.format());
}
return null;
}🤖 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 `@src/lib/posthog.test.ts` around lines 1 - 65, The comment in getPostHogClient
about "Only log if the key is present but invalid (e.g. empty string)" is out of
sync with the code because the current conditional uses if (env.key) which
treats '' as falsy and prevents logging; update the conditional in the
getPostHogClient implementation to explicitly check for undefined (e.g., if
(env.key !== undefined)) so an empty string will still trigger the error logging
path, and keep the comment as-is (or instead update the comment to match
whichever behavior you choose).
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Owner
Author
|
Stale agent-codename PR. Closed to clear the queue after hire-path and Astro 7 landed on main. Reopen only if we explicitly pick this work up against current main. |
This branch was successfully deployed
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 by CodeRabbit
New Features
Tests
Chores
posthog-nodedependency