Fix feedback nav button style and remove icon#24
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces a complete feedback system featuring server-side mutation and query operations with validation, rate limiting, and admin authentication; a database schema for feedback records; a client-side feedback board page and reusable widget component; styling for feedback UI; and environment-based admin access control via Clerk IDs and email addresses. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Widget as FeedbackWidget
participant Server as Convex Server<br/>(feedback.ts)
participant DB as Database<br/>(feedback table)
User->>Widget: Opens feedback form
User->>Widget: Submits heading & message
Widget->>Widget: Validate input & get anonId
Widget->>Server: createFeedback mutation
Server->>Server: Validate submission, check rate limits
Server->>Server: Resolve user context
Server->>DB: Insert feedback record
DB-->>Server: feedbackId
Server-->>Widget: Return feedback ID
Widget->>Widget: Show success message
User->>User: Navigate to feedback board
User->>Server: listFeedback query
Server->>DB: Fetch feedback items
DB-->>Server: Feedback records
Server->>Server: Mask emails (unless admin)
Server-->>User: Return paginated list
alt User is Admin
User->>Server: setFeedbackResolved mutation
Server->>Server: Verify admin authorization
Server->>DB: Update feedback status & metadata
DB-->>Server: Updated record
Server-->>User: Return updated item
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
docs/OWNER_ACTION_ITEMS.md (1)
3-3: Update the "Last updated" date.The file was modified in this PR (2026-02-19) but line 3 still reads
2026-02-06.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/OWNER_ACTION_ITEMS.md` at line 3, Replace the outdated "Last updated: 2026-02-06" line in OWNER_ACTION_ITEMS.md with the correct date "Last updated: 2026-02-19" so the file header reflects the PR modification date.apps/web/src/app/feedback/page.tsx (1)
54-56: Extract the magic number150into a named constant.♻️ Suggested refactor
+const FEEDBACK_LIST_LIMIT = 150; + export default function FeedbackPage() { ... const feedback = useQuery(api.feedback.listFeedback, { - limit: 150, + limit: FEEDBACK_LIST_LIMIT, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/feedback/page.tsx` around lines 54 - 56, The magic number 150 used in the useQuery call should be extracted to a named constant for clarity and maintainability; define a constant (e.g. FEEDBACK_LIST_LIMIT = 150) near the top of the module and replace the inline literal in the useQuery call (api.feedback.listFeedback, { limit: 150 }) with { limit: FEEDBACK_LIST_LIMIT } so the meaning and reuse of the limit is explicit.apps/web/convex/feedback.ts (1)
300-308: Consider projecting the return value to avoid leaking internal fields to the client.
ctx.db.get(args.feedbackId)returns the full document including internal identifiers (createdByUserId,createdByAnonId,createdByClerkId). While only admins reach this point, returning a trimmed projection — consistent withlistFeedback's shape — would minimize client-side data exposure and keep the API surface uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/convex/feedback.ts` around lines 300 - 308, The current return from ctx.db.get(args.feedbackId) exposes internal fields (createdByUserId, createdByAnonId, createdByClerkId); instead, return a projected object that matches the shape used by listFeedback: include only the public fields (id, title, message, status, updatedAt, resolvedAt, resolvedByClerkId, any tags/metadata you surface) and omit internal identifiers; implement this by replacing the direct ctx.db.get(...) return with a projection/mapping step after fetching the document (use ctx.db.get(args.feedbackId) to fetch then map to the trimmed shape) so admins still get the needed fields but no internal IDs are leaked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/.env.example`:
- Around line 34-41: Move the two new env entries (including their comment
lines) for ZENPDF_FEEDBACK_ADMIN_CLERK_IDS and ZENPDF_FEEDBACK_ADMIN_EMAILS so
they appear alphabetically before the ZENPDF_FREE_ACCOUNT_* block: place them
right after the ZENPDF_ANON_MAX_DAILY_MINUTES= line and before
ZENPDF_FREE_ACCOUNT_MAX_FILES_PER_JOB=, and remove their old lines after
NEXT_PUBLIC_DONATE_CARD_EMBED_URL; keep the exact variable names and
accompanying comment text intact so dotenv-linter no longer reports UnorderedKey
for ZENPDF_FEEDBACK_ADMIN_CLERK_IDS and ZENPDF_FEEDBACK_ADMIN_EMAILS.
In `@apps/web/convex/feedback.ts`:
- Around line 53-55: Replace the implicit NODE_ENV check with an explicit opt-in
env var check so admin fallback is disabled by default: where the code currently
returns process.env.NODE_ENV !== "production" when adminClerkIds.size === 0 &&
adminEmails.size === 0, change it to check a dedicated flag like
process.env.ZENPDF_FEEDBACK_ADMIN_DEV_MODE === "true" (or similar truthy check)
and return true only when that flag is explicitly set; update any related logic
that grants admin rights (references: adminClerkIds, adminEmails,
process.env.NODE_ENV) to rely on this explicit opt-in instead of NODE_ENV.
In `@apps/web/convex/schema.ts`:
- Around line 144-160: The feedback table currently stores createdByEmail in
plaintext (defined on the feedback table via createdByEmail) which risks
violating right-to-erasure; either remove the createdByEmail column from the
feedback.defineTable schema and stop writing identity?.email?.toLowerCase()
anywhere (rely on createdByClerkId), or keep the column but implement a
deletion/retention flow that nulls it: add an on-delete or scheduled mutation
that sets feedback.createdByEmail = null when the Clerk user identified by
createdByClerkId is deleted (and update any code paths that write createdByEmail
to respect anonymization/TTL). Use the feedback defineTable entry and any
mutation/write functions that reference createdByEmail / createdByClerkId to
locate and apply the change.
In `@apps/web/src/app/globals.css`:
- Around line 665-667: The .feedback-trigger:active rule is a no-op because
transform: translateY(0) is the default; either remove the rule or change it to
a meaningful tactile press transform (e.g., translateY(1px)) to match the
behavior used by .paper-button:active; update the .feedback-trigger:active
selector to apply the desired press offset (or remove it if you intend no
effect) and ensure any CSS transition on .feedback-trigger exists so the press
is animated consistently.
In `@apps/web/src/components/FeedbackWidget.tsx`:
- Around line 217-218: The conditional success/error paragraphs in
FeedbackWidget (the submitSuccess and submitError renderings) lack an aria-live
region; change them to always-rendered elements (e.g., persistent <p> or <div>
for the success and error messages) and add appropriate aria-live attributes
(e.g., aria-live="polite" or "assertive") and role if needed so screen readers
announce updates when submitSuccess/submitError values change; ensure the
elements contain an empty string when there is no message so they remain present
in the DOM while only the text content toggles.
- Around line 146-221: The modal lacks a focus trap and is rendered in-place
inside the nav; wrap the dialog in a portal and add a minimal Tab-wrap focus
trap: create a modalRef (e.g., const modalRef = useRef<HTMLDivElement |
null>(null)), render the overlay using ReactDOM.createPortal into document.body
instead of in-place where `open` is checked, and in a useEffect attach keydown
handlers that capture Tab/Shift+Tab to keep focus cycling between the first and
last focusable elements inside the modal (including the Close button, form
inputs, the Submit button and the `Link`), also ensure focus is moved into the
modal on open (focus headingRef or first focusable) and restored on close by
using handleClose to return focus to the previously focused element.
---
Nitpick comments:
In `@apps/web/convex/feedback.ts`:
- Around line 300-308: The current return from ctx.db.get(args.feedbackId)
exposes internal fields (createdByUserId, createdByAnonId, createdByClerkId);
instead, return a projected object that matches the shape used by listFeedback:
include only the public fields (id, title, message, status, updatedAt,
resolvedAt, resolvedByClerkId, any tags/metadata you surface) and omit internal
identifiers; implement this by replacing the direct ctx.db.get(...) return with
a projection/mapping step after fetching the document (use
ctx.db.get(args.feedbackId) to fetch then map to the trimmed shape) so admins
still get the needed fields but no internal IDs are leaked.
In `@apps/web/src/app/feedback/page.tsx`:
- Around line 54-56: The magic number 150 used in the useQuery call should be
extracted to a named constant for clarity and maintainability; define a constant
(e.g. FEEDBACK_LIST_LIMIT = 150) near the top of the module and replace the
inline literal in the useQuery call (api.feedback.listFeedback, { limit: 150 })
with { limit: FEEDBACK_LIST_LIMIT } so the meaning and reuse of the limit is
explicit.
In `@docs/OWNER_ACTION_ITEMS.md`:
- Line 3: Replace the outdated "Last updated: 2026-02-06" line in
OWNER_ACTION_ITEMS.md with the correct date "Last updated: 2026-02-19" so the
file header reflects the PR modification date.
| # Optional: comma-separated Clerk user IDs that can resolve feedback. | ||
| # Example: ZENPDF_FEEDBACK_ADMIN_CLERK_IDS=user_123,user_456 | ||
| ZENPDF_FEEDBACK_ADMIN_CLERK_IDS= | ||
| # Optional alternative/addition: comma-separated admin emails for feedback resolve access. | ||
| # Example: ZENPDF_FEEDBACK_ADMIN_EMAILS=admin@example.com,ops@example.com | ||
| # Privacy note: emails are PII. Prefer Clerk roles/permissions or a database-backed admin list | ||
| # for better auditability and easier rotation than long-lived environment variables. | ||
| ZENPDF_FEEDBACK_ADMIN_EMAILS= |
There was a problem hiding this comment.
Resolve dotenv-linter UnorderedKey warnings for the two new vars.
ZENPDF_FEEDBACK_ADMIN_CLERK_IDS and ZENPDF_FEEDBACK_ADMIN_EMAILS should be placed before the ZENPDF_FREE_ACCOUNT_* block (alphabetically FEEDBACK < FREE). The static analysis tool flags both at lines 36 and 41.
🔧 Suggested placement
Move the four lines (comment + key for each variable) to appear after line 13 (ZENPDF_ANON_MAX_DAILY_MINUTES=) and before line 14 (ZENPDF_FREE_ACCOUNT_MAX_FILES_PER_JOB=):
ZENPDF_ANON_MAX_DAILY_MINUTES=
+# Optional: comma-separated Clerk user IDs that can resolve feedback.
+# Example: ZENPDF_FEEDBACK_ADMIN_CLERK_IDS=user_123,user_456
+ZENPDF_FEEDBACK_ADMIN_CLERK_IDS=
+# Optional alternative/addition: comma-separated admin emails for feedback resolve access.
+# Example: ZENPDF_FEEDBACK_ADMIN_EMAILS=admin@example.com,ops@example.com
+# Privacy note: emails are PII. Prefer Clerk roles/permissions or a database-backed admin list
+# for better auditability and easier rotation than long-lived environment variables.
+ZENPDF_FEEDBACK_ADMIN_EMAILS=
ZENPDF_FREE_ACCOUNT_MAX_FILES_PER_JOB=And remove them from their current location after NEXT_PUBLIC_DONATE_CARD_EMBED_URL.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 36-36: [UnorderedKey] The ZENPDF_FEEDBACK_ADMIN_CLERK_IDS key should go before the ZENPDF_FREE_ACCOUNT_MAX_CONCURRENT_JOBS key
(UnorderedKey)
[warning] 41-41: [UnorderedKey] The ZENPDF_FEEDBACK_ADMIN_EMAILS key should go before the ZENPDF_FREE_ACCOUNT_MAX_CONCURRENT_JOBS key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/.env.example` around lines 34 - 41, Move the two new env entries
(including their comment lines) for ZENPDF_FEEDBACK_ADMIN_CLERK_IDS and
ZENPDF_FEEDBACK_ADMIN_EMAILS so they appear alphabetically before the
ZENPDF_FREE_ACCOUNT_* block: place them right after the
ZENPDF_ANON_MAX_DAILY_MINUTES= line and before
ZENPDF_FREE_ACCOUNT_MAX_FILES_PER_JOB=, and remove their old lines after
NEXT_PUBLIC_DONATE_CARD_EMBED_URL; keep the exact variable names and
accompanying comment text intact so dotenv-linter no longer reports UnorderedKey
for ZENPDF_FEEDBACK_ADMIN_CLERK_IDS and ZENPDF_FEEDBACK_ADMIN_EMAILS.
| if (adminClerkIds.size === 0 && adminEmails.size === 0) { | ||
| return process.env.NODE_ENV !== "production"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Convex server functions NODE_ENV environment variable automatic
💡 Result:
Convex does not automatically set process.env.NODE_ENV for your server functions (queries/mutations/actions/http actions). In Convex, process.env contains:
- Deployment environment variables you set in the Convex dashboard / via
npx convex env set(they’re per-deployment, so dev and prod can differ). [1] - A small set of system environment variables that are always present (currently documented as
CONVEX_CLOUD_URLandCONVEX_SITE_URL). [1][2]
So if you want an environment switch inside Convex functions, set your own variable (e.g. APP_ENV=development|production) in each deployment and read it via process.env.APP_ENV. [1]
Sources: [1] [2]
🌐 Web query:
Convex documentation environment variables NODE_ENV production
💡 Result:
Convex doesn’t document (or guarantee) a built-in NODE_ENV value you can rely on inside Convex functions. Instead, Convex’s docs recommend using Convex deployment environment variables (set per deployment: dev vs prod) and reading them via process.env. [1]
What to do:
- Create your own env var (e.g.
APP_ENV=production) in the Convex Dashboard → Deployment Settings (or via CLI) and branch on that. [1], [2]- CLI examples:
npx convex env set APP_ENV production/npx convex env list[2]
- CLI examples:
- Remember Convex has two runtimes: most functions run in Convex’s default JS runtime; Node.js is only for actions (files using
"use node"), so Node-specific assumptions likeNODE_ENVare not something Convex calls out as part of the platform contract. [3] - For “prod vs dev” at the project level, Convex’s CLI uses
CONVEX_DEPLOYMENT(written to.env.localfor local dev) and production deployments are configured separately (often via CI withCONVEX_DEPLOY_KEY). [4]
Sources:
[1] Convex Docs – Environment Variables https://docs.convex.dev/production/environment-variables
[2] Convex Docs – CLI (env commands) https://docs.convex.dev/cli
[3] Convex Docs – Function runtimes (default vs Node.js actions) https://docs.convex.dev/functions/runtimes
[4] Convex Docs – Project configuration (dev/prod config) https://docs.convex.dev/production/project-configuration
Security: NODE_ENV is not automatically set in Convex, creating a privilege escalation risk.
Convex does not automatically set process.env.NODE_ENV in server functions. The only system environment variables present are CONVEX_CLOUD_URL and CONVEX_SITE_URL. If neither admin-list env var is configured and NODE_ENV is not explicitly set as a deployment environment variable, the condition process.env.NODE_ENV !== "production" evaluates to true (since undefined !== "production"), granting admin rights to all authenticated users—including in production deployments.
Switch to an explicit opt-in pattern:
Safe-by-default fix
if (adminClerkIds.size === 0 && adminEmails.size === 0) {
- return process.env.NODE_ENV !== "production";
+ return process.env.ZENPDF_FEEDBACK_ADMIN_DEV_MODE === "true";
}Developers must explicitly set ZENPDF_FEEDBACK_ADMIN_DEV_MODE=true in the Convex dashboard for development environments if they want dev-mode admin fallback; all other deployments deny by default.
📝 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.
| if (adminClerkIds.size === 0 && adminEmails.size === 0) { | |
| return process.env.NODE_ENV !== "production"; | |
| } | |
| if (adminClerkIds.size === 0 && adminEmails.size === 0) { | |
| return process.env.ZENPDF_FEEDBACK_ADMIN_DEV_MODE === "true"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/convex/feedback.ts` around lines 53 - 55, Replace the implicit
NODE_ENV check with an explicit opt-in env var check so admin fallback is
disabled by default: where the code currently returns process.env.NODE_ENV !==
"production" when adminClerkIds.size === 0 && adminEmails.size === 0, change it
to check a dedicated flag like process.env.ZENPDF_FEEDBACK_ADMIN_DEV_MODE ===
"true" (or similar truthy check) and return true only when that flag is
explicitly set; update any related logic that grants admin rights (references:
adminClerkIds, adminEmails, process.env.NODE_ENV) to rely on this explicit
opt-in instead of NODE_ENV.
| feedback: defineTable({ | ||
| heading: v.string(), | ||
| message: v.string(), | ||
| status: feedbackStatus, | ||
| createdByUserId: v.optional(v.id("users")), | ||
| createdByAnonId: v.optional(v.string()), | ||
| createdByClerkId: v.optional(v.string()), | ||
| createdByEmail: v.optional(v.string()), | ||
| createdAt: v.number(), | ||
| updatedAt: v.number(), | ||
| resolvedAt: v.optional(v.number()), | ||
| resolvedByClerkId: v.optional(v.string()), | ||
| }) | ||
| .index("by_created", ["createdAt"]) | ||
| .index("by_user_created", ["createdByUserId", "createdAt"]) | ||
| .index("by_anon_created", ["createdByAnonId", "createdAt"]) | ||
| .index("by_status_updated", ["status", "updatedAt"]), |
There was a problem hiding this comment.
createdByEmail persisted in plaintext poses a GDPR/CCPA right-to-erasure gap.
createdByEmail is written on every authenticated submission (identity?.email?.toLowerCase()) and has no TTL, anonymization path, or deletion trigger. If a user later deletes their Clerk account, their email remains in every feedback row they created, violating the right to erasure. The privacy note in .env.example addresses the admin env var, but the stored DB field is the higher-risk surface.
Options:
- Drop
createdByEmailfrom the schema and rely solely oncreatedByClerkId— admins can look up the email via Clerk's admin API when needed. - Or add a scheduled mutation /
onDeletehook that nullifiescreatedByEmailwhen the corresponding user record is removed.
The resolvedByClerkId field has the same concern for the admin user's email, but that data is not currently surfaced or stored as email — only as a Clerk ID, which is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/convex/schema.ts` around lines 144 - 160, The feedback table
currently stores createdByEmail in plaintext (defined on the feedback table via
createdByEmail) which risks violating right-to-erasure; either remove the
createdByEmail column from the feedback.defineTable schema and stop writing
identity?.email?.toLowerCase() anywhere (rely on createdByClerkId), or keep the
column but implement a deletion/retention flow that nulls it: add an on-delete
or scheduled mutation that sets feedback.createdByEmail = null when the Clerk
user identified by createdByClerkId is deleted (and update any code paths that
write createdByEmail to respect anonymization/TTL). Use the feedback defineTable
entry and any mutation/write functions that reference createdByEmail /
createdByClerkId to locate and apply the change.
| .feedback-trigger:active { | ||
| transform: translateY(0); | ||
| } |
There was a problem hiding this comment.
.feedback-trigger:active transform is a no-op.
transform: translateY(0) is the browser default — no transition or hover transform is set on .feedback-trigger to reset, so the :active rule has no visual effect. Compare with .paper-button:active { transform: translateY(1px) } (Line 148) which gives tactile press feedback. If the intent is to suppress any inherited transform, it still does nothing here.
🔧 Suggested fix for a subtle press effect
.feedback-trigger:active {
- transform: translateY(0);
+ transform: translateY(1px);
}📝 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.
| .feedback-trigger:active { | |
| transform: translateY(0); | |
| } | |
| .feedback-trigger:active { | |
| transform: translateY(1px); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/globals.css` around lines 665 - 667, The
.feedback-trigger:active rule is a no-op because transform: translateY(0) is the
default; either remove the rule or change it to a meaningful tactile press
transform (e.g., translateY(1px)) to match the behavior used by
.paper-button:active; update the .feedback-trigger:active selector to apply the
desired press offset (or remove it if you intend no effect) and ensure any CSS
transition on .feedback-trigger exists so the press is animated consistently.
| {open ? ( | ||
| <div | ||
| className="feedback-overlay" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-labelledby="feedback-modal-title" | ||
| onClick={(event) => { | ||
| if (event.currentTarget === event.target) { | ||
| handleClose(); | ||
| } | ||
| }} | ||
| > | ||
| <div className="paper-card feedback-modal"> | ||
| <button | ||
| type="button" | ||
| className="paper-button--ghost feedback-modal-close" | ||
| onClick={handleClose} | ||
| > | ||
| Close | ||
| </button> | ||
|
|
||
| <div className="pr-24"> | ||
| <span className="ink-label">Share feedback</span> | ||
| <h2 id="feedback-modal-title" className="mt-2 text-2xl"> | ||
| Help improve ZenPDF | ||
| </h2> | ||
| <p className="mt-2 text-sm text-ink-700"> | ||
| Report issues or product ideas. Open items stay visible to everyone. | ||
| </p> | ||
| </div> | ||
|
|
||
| <form className="mt-5 space-y-4" onSubmit={submit}> | ||
| <label className="field-label" htmlFor="feedback-heading"> | ||
| Heading | ||
| </label> | ||
| <input | ||
| ref={headingRef} | ||
| id="feedback-heading" | ||
| className="field-input" | ||
| value={heading} | ||
| onChange={(event) => setHeading(event.target.value)} | ||
| placeholder="Short summary" | ||
| required | ||
| minLength={HEADING_MIN_CHARS} | ||
| maxLength={HEADING_MAX_CHARS} | ||
| /> | ||
|
|
||
| <label className="field-label" htmlFor="feedback-message"> | ||
| Message | ||
| </label> | ||
| <textarea | ||
| id="feedback-message" | ||
| className="field-input min-h-32 resize-y" | ||
| value={message} | ||
| onChange={(event) => setMessage(event.target.value)} | ||
| placeholder="Describe the issue, expected behavior, and context." | ||
| required | ||
| minLength={MESSAGE_MIN_CHARS} | ||
| maxLength={MESSAGE_MAX_CHARS} | ||
| /> | ||
|
|
||
| <div className="flex flex-wrap items-center gap-2"> | ||
| <button type="submit" className="paper-button w-auto" disabled={isSubmitting}> | ||
| {isSubmitting ? "Submitting..." : "Submit feedback"} | ||
| </button> | ||
| <Link href="/feedback" className="paper-button--ghost w-auto" onClick={handleClose}> | ||
| View feedback board | ||
| </Link> | ||
| </div> | ||
| </form> | ||
|
|
||
| {submitSuccess ? <p className="alert alert--success mt-3">{submitSuccess}</p> : null} | ||
| {submitError ? <p className="alert alert--error mt-3">{submitError}</p> : null} | ||
| </div> | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
Two compounding modal accessibility issues: no focus trap + no portal.
1. No focus trap
When keyboard focus reaches the last interactive element ("View feedback board" link, Line 211), pressing Tab moves focus to whatever comes next in the page's tab order — elements behind the overlay. aria-modal="true" is a semantic hint and its support is incomplete across browser/AT combinations; it is not a substitute for a programmatic focus trap.
2. Modal rendered inside <nav>
The overlay is conditionally rendered in-place, which means its DOM parent is <nav aria-label="Primary"> (desktop) or <nav aria-label="Primary mobile"> (mobile). While position: fixed makes it visually full-screen, screen reader users navigating by landmark could encounter the dialog inside the nav landmark rather than as a top-level dialog. The fix is to render via createPortal into document.body.
🔧 Suggested fix — portal + minimal Tab-wrap focus trap
+"use client";
+
+import { createPortal } from "react-dom";
import Link from "next/link";
import { useMutation } from "convex/react";
-import { FormEvent, useCallback, useEffect, useRef, useState } from "react";
+import { FormEvent, useCallback, useEffect, useRef, useState, type RefObject } from "react";Render via portal:
- {open ? (
- <div
- className="feedback-overlay"
- role="dialog"
- ...
- >
- ...
- </div>
- ) : null}
+ {open
+ ? createPortal(
+ <div
+ className="feedback-overlay"
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby="feedback-modal-title"
+ onClick={(event) => {
+ if (event.currentTarget === event.target) handleClose();
+ }}
+ >
+ <div className="paper-card feedback-modal" ref={modalRef}>
+ {/* …existing modal content… */}
+ </div>
+ </div>,
+ document.body,
+ )
+ : null}Add a Tab-wrap focus trap inside the useEffect:
+ // Collect focusable elements within the modal and trap Tab
+ const FOCUSABLE = 'button:not([disabled]), [href], input, textarea, [tabindex]:not([tabindex="-1"])';
+ const onKeyDown = (event: KeyboardEvent) => {
+ if (event.key === "Escape") { event.preventDefault(); handleClose(); return; }
+ if (event.key !== "Tab" || !modalRef.current) return;
+ const focusable = Array.from(modalRef.current.querySelectorAll<HTMLElement>(FOCUSABLE));
+ const first = focusable[0];
+ const last = focusable[focusable.length - 1];
+ if (event.shiftKey && document.activeElement === first) {
+ event.preventDefault(); last?.focus();
+ } else if (!event.shiftKey && document.activeElement === last) {
+ event.preventDefault(); first?.focus();
+ }
+ };You would also need:
const modalRef = useRef<HTMLDivElement | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/FeedbackWidget.tsx` around lines 146 - 221, The modal
lacks a focus trap and is rendered in-place inside the nav; wrap the dialog in a
portal and add a minimal Tab-wrap focus trap: create a modalRef (e.g., const
modalRef = useRef<HTMLDivElement | null>(null)), render the overlay using
ReactDOM.createPortal into document.body instead of in-place where `open` is
checked, and in a useEffect attach keydown handlers that capture Tab/Shift+Tab
to keep focus cycling between the first and last focusable elements inside the
modal (including the Close button, form inputs, the Submit button and the
`Link`), also ensure focus is moved into the modal on open (focus headingRef or
first focusable) and restored on close by using handleClose to return focus to
the previously focused element.
| {submitSuccess ? <p className="alert alert--success mt-3">{submitSuccess}</p> : null} | ||
| {submitError ? <p className="alert alert--error mt-3">{submitError}</p> : null} |
There was a problem hiding this comment.
Dynamic success/error paragraphs need aria-live to be announced by screen readers.
These elements are conditionally rendered; without an aria-live region, assistive technologies will not announce the outcome of the submission to users who rely on screen readers.
🔧 Suggested fix
- {submitSuccess ? <p className="alert alert--success mt-3">{submitSuccess}</p> : null}
- {submitError ? <p className="alert alert--error mt-3">{submitError}</p> : null}
+ <p className="alert alert--success mt-3" aria-live="polite" role="status">
+ {submitSuccess}
+ </p>
+ <p className="alert alert--error mt-3" aria-live="assertive" role="alert">
+ {submitError}
+ </p>Using always-present elements (with empty content when idle) avoids the timing issue where the region is inserted and announced simultaneously.
📝 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.
| {submitSuccess ? <p className="alert alert--success mt-3">{submitSuccess}</p> : null} | |
| {submitError ? <p className="alert alert--error mt-3">{submitError}</p> : null} | |
| <p className="alert alert--success mt-3" aria-live="polite" role="status"> | |
| {submitSuccess} | |
| </p> | |
| <p className="alert alert--error mt-3" aria-live="assertive" role="alert"> | |
| {submitError} | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/FeedbackWidget.tsx` around lines 217 - 218, The
conditional success/error paragraphs in FeedbackWidget (the submitSuccess and
submitError renderings) lack an aria-live region; change them to always-rendered
elements (e.g., persistent <p> or <div> for the success and error messages) and
add appropriate aria-live attributes (e.g., aria-live="polite" or "assertive")
and role if needed so screen readers announce updates when
submitSuccess/submitError values change; ensure the elements contain an empty
string when there is no message so they remain present in the DOM while only the
text content toggles.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughA complete feedback feature is introduced with configuration, backend Convex functions for submission/listing/resolution, database schema with indices, frontend components including a feedback widget and board page, styling updates, and administrative controls for managing feedback items. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / Client
participant Widget as FeedbackWidget
participant Convex as Convex Backend
participant DB as Database
User->>Widget: Click feedback trigger
Widget->>Widget: Open modal, focus input
User->>Widget: Enter heading & message
User->>Widget: Click submit
Widget->>Widget: Validate input lengths<br/>(4-120, 12-2000)
Widget->>Widget: Get or create anonId
Widget->>Convex: createFeedback mutation<br/>(heading, message, anonId)
Convex->>Convex: Resolve user identity
Convex->>Convex: Check rate limits<br/>(5/hour, 1-min min interval)
Convex->>DB: Insert feedback record<br/>(open status, timestamps)
DB-->>Convex: feedbackId
Convex-->>Widget: { feedbackId }
Widget->>Widget: Show success message
Widget->>Widget: Close modal after delay
sequenceDiagram
participant Admin as Admin User
participant Page as FeedbackPage
participant Convex as Convex Backend
participant DB as Database
Admin->>Page: Navigate to /feedback
Page->>Convex: listFeedback query (limit)
Convex->>Convex: Verify admin rights<br/>(clerk ID / email)
Convex->>DB: Query open items<br/>(by_status_updated index)
Convex->>DB: Query resolved items fallback
DB-->>Convex: feedback items list
Convex->>Convex: Compose view with email<br/>(include creator email)
Convex-->>Page: FeedbackListResponse
Page->>Page: Compute counts & render
Page->>Page: Display feedback items<br/>(status, times, emails)
Admin->>Page: Click resolve checkbox
Page->>Convex: setFeedbackResolved mutation
Convex->>Convex: Assert admin rights
Convex->>DB: Patch feedback status<br/>(resolved, timestamp)
DB-->>Convex: updated item
Convex-->>Page: confirmation
Page->>Page: Update UI, show loading
Page->>Page: Refresh item state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
1 participant
Summary
Validation
Summary by CodeRabbit
Release Notes