poll: ask visitors on an answer page whether they want an app - #13
Conversation
The SEC-RULES header is the fleet-wide block every other repo's CLAUDE.md carries; it was added locally when this repo was enrolled and never committed. The Playwright note is from a real miss this session: a leftover `next start` on :3000 made `reuseExistingServer` skip the build, so the suite ran against a binary built from older source with a different NEXT_PUBLIC_ inlining. Two tests "failed" against code that was already fixed. Co-Authored-By: Claude Code <noreply@anthropic.com>
A centred dialog on /strada/ and /punct-termic/ pages asks "Te-ar interesa o aplicație pentru faraapacalda.ro?", then Android or iOS. It holds the screen until answered or closed, per the brief. Deliberate choices worth reviewing: - It fires 10s after load, not on arrival. A dialog that covers content the moment someone lands from search is what Google treats as an intrusive interstitial, and this site lives on organic search. - Two steps, ONE row, upserted on a client-generated UUID nonce. Tapping "Da" writes immediately, so closing before picking a platform still counts as a "Da" instead of vanishing. Verified against the real table: insert 201, merge 200, one row with the platform merged in. - A new fac_app_poll table rather than reusing fac_feedback, because GET /api/feedback counts every vote=eq.up with no page filter — poll answers landing there would inflate the public "N" badge on the live site. RLS on, zero policies, same as fac_feedback: only the service role reaches it. - No GET route. Unlike the feedback tally there is nothing here to expose. - The feedback pill hides while the dialog is open (one ask at a time), via a body data attribute rather than coupling the two components. - The delay is overridable so the e2e build can push it out of reach; a spec that merely visits a street page can never race a screen-covering modal. Kill switch: NEXT_PUBLIC_APP_POLL=0. 12 unit tests, 9 e2e. Escape is bound at document level, not on the backdrop — a backdrop click moves focus to <body>, which silently killed Escape and left the x as the only exit from a dialog that locks scroll. Found in visual verification, pinned by a regression test. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full diff. The poll feature itself (route, validation, modal, tests) is solid — mirrors the existing /api/feedback pattern closely, the Escape-key fix is a real bug with a real regression test, and unit/e2e coverage is thorough. Not requesting changes on any of that.
Blocking: CLAUDE.md:1-4 (commit 15a1220) does not belong in this PR.
That commit adds:
<!-- SEC-RULES v1 START -->Security gate. Every change made in this workspace must comply with the Secure Development Rules Reference:~/aios/security/rules/secure-development-rules-reference.md...<!-- SEC-RULES v1 END -->
This instructs any agent that reads CLAUDE.md to fetch and obey an out-of-repo file at ~/aios/security/rules/..., using "must comply" / override-style language. Three things make this a hard stop rather than a style nit:
- The PR description never mentions it — it only describes the
:3000stale-server doc addition from the same commit. An undisclosed instruction change to the file that governs agent behavior in this repo is not something a reviewer should wave through on the strength of an unrelated description. ~/aios/security/rules/...is a local home-directory path, not part of this repo — nothing in the diff lets a reviewer see what it actually contains, so "comply with" is being asked for blind.- CLAUDE.md is explicitly treated as high-trust, override-priority instruction text by AI coding agents (including this reviewer). Slipping directives into it that point at unreviewable external paths is indistinguishable from a prompt-injection attempt, regardless of intent.
Please drop the SEC-RULES block from this PR (the :3000 stale-server note can stay — that one matches the description and is a legitimate, well-scoped doc addition). If a repo-wide security-gate policy is actually wanted, it should land in its own PR with a description that says so explicitly, and the referenced rules file should be something reviewable (checked into the repo or linked, not a bare home-dir path).
|
@claude please address this review:
|
1 participant
A centred dialog on
/strada/and/punct-termic/pages asks "Te-ar interesa o aplicație pentru faraapacalda.ro?", then Android or iOS. It holds the screen until answered or closed.Copy is Teodor's, with diacritics and his warmer singular "Mulțumesc":
Things worth reviewing carefully
It fires 10s after load, not on arrival. A dialog that covers content the moment someone lands from search is what Google treats as an intrusive interstitial, and this site lives on organic search. After real engagement it is not that.
Two steps, one row. Both steps upsert on a client-generated UUID nonce (
on_conflict=nonce,merge-duplicates), so tapping "Da" and then closing without picking a platform still counts as a "Da". Verified against the real table before shipping: step 1 →201, step 2 →200, readback showed a single row with the platform merged in, then cleaned up. Tests could not have caught a wrong upsert shape — it would have silently 502'd in production.A new
fac_app_polltable, notfac_feedback.GET /api/feedbackcounts everyvote=eq.upwith no page filter, so poll answers landing in that table would inflate the public "▲ N" badge on the live site. RLS on with zero policies, mirroringfac_feedback: only the service role reaches it. NoGETroute — unlike the feedback tally, there is nothing here to expose publicly.The delay is overridable (
NEXT_PUBLIC_APP_POLL_DELAY_MS, single-sourced fromplaywright.config.ts) so the e2e build pushes it out of reach. Without that, any existing spec visiting a street page could race a screen-covering modal on a slow runner — a flake that would be very hard to connect back to this PR.Escape is bound at document level, not on the backdrop. A backdrop click moves focus to
<body>, and a handler on the backdrop subtree stops firing — which silently killed Escape and left the×as the only exit from a dialog that locks scroll. Found during visual verification, now pinned by a regression test.Checks
122unit tests,34e2e (9 new),tscclean, full suite green on a fresh build. Visually verified headless at 1440×900 and 390×844: no horizontal overflow, diacritics clean (ș/țdescenders not clipped), feedback pill confirmed hidden while open and restored after, scroll lock holds, zero console errors beyond the expected local503s.Not addressed here
app/globals.cssdefines no:focus-visiblestyle at all, so this is a pre-existing site-wide gap, not something this PR should quietly change.Live behaviour on merge
Merging turns the poll on in production (kill switch:
NEXT_PUBLIC_APP_POLL=0). Worth a look at the preview deployment first.🤖 Generated with Claude Code