Replace Web3Forms with internal feedback emails#1244
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughReplace static Featurebase API key usage with runtime retrieval; add Featurebase user mapping helpers; introduce internal feedback email utilities and a new POST feedback route; update dashboard to use internal project headers and protect unauthenticated actions; add E2E tests and env var for feedback recipients. Changes
Sequence DiagramssequenceDiagram
participant User as User
participant Dashboard as Dashboard (Client)
participant Backend as Backend API
participant EmailSvc as Email Service
User->>Dashboard: Submit feedback form
Dashboard->>Dashboard: ensure user present, get authJson
Dashboard->>Backend: POST /api/v1/internal/feedback (getInternalProjectHeaders)
Backend->>Backend: validate schema (auth, body)
Backend->>EmailSvc: sendSupportFeedbackEmail(...)
EmailSvc-->>Backend: queued/sent
Backend-->>Dashboard: 200 { success: true }
sequenceDiagram
participant User as User
participant Dashboard as Dashboard (Client)
participant Backend as Backend API
participant Featurebase as Featurebase API
User->>Dashboard: Submit feature request / upvote
Dashboard->>Dashboard: ensure user present, get authJson
Dashboard->>Backend: POST /api/v1/internal/feature-requests or upvote (getInternalProjectHeaders)
Backend->>Backend: requireFeaturebaseApiKey()
Backend->>Backend: getOrCreateFeaturebaseUserFromAuth(auth.user)
Backend->>Featurebase: POST/PUT /v1/posts or /v1/posts/:id/upvoters (X-API-Key header)
Featurebase-->>Backend: 200 OK / error
Backend-->>Dashboard: 200 { success: true } or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…r management - Introduced a new utility module for featurebase interactions, consolidating API key management and user creation logic. - Updated existing routes to utilize the new featurebase functions, enhancing code clarity and reducing redundancy. - Enhanced internal feedback email templates for better structure and readability. - Added documentation to clarify the internal feedback email flow and E2E testing strategies.
Greptile SummaryThis PR replaces the third-party Web3Forms integration with a first-party authenticated endpoint ( Key changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as Dashboard User
participant FE as feedback-form.tsx
participant BE as /api/v1/internal/feedback
participant EM as internal-feedback-emails.tsx
participant SMTP as Email Pipeline
U->>FE: submits feedback form
FE->>FE: user.getAuthJson()
FE->>FE: getInternalProjectHeaders(accessToken)
FE->>BE: POST with X-Stack-Access-Token + body {name, email, message}
BE->>BE: SmartRouteHandler validates auth.user + project=internal
BE->>EM: sendSupportFeedbackEmail(tenancy, user, ...)
EM->>EM: getInternalFeedbackRecipients() from env
EM->>EM: buildInternalEmailHtml (escapeHtml all fields)
EM->>SMTP: sendEmailToMany(recipients, html, subject)
SMTP-->>EM: queued/sent
EM-->>BE: resolved
BE-->>FE: 200 { success: true }
FE-->>U: shows success state
|
There was a problem hiding this comment.
Pull request overview
This PR replaces the dashboard’s external Web3Forms feedback submission with authenticated internal backend endpoints, and routes support + feature-request notifications through Stack Auth’s internal email pipeline (with E2E coverage added for the feedback endpoint).
Changes:
- Add internal backend support feedback endpoint that sends an internal email notification.
- Refactor dashboard feedback + feature request board to call authenticated internal endpoints using shared “internal project” headers.
- Add backend email utilities (recipient parsing + Featurebase helpers) and an E2E test for internal feedback delivery.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds lockfile entries for new workspace/dev dependency. |
| apps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts | Adds E2E coverage validating internal feedback endpoint and email delivery. |
| apps/dashboard/src/lib/internal-project-headers.ts | Introduces shared helper to build internal-project auth headers for client-side fetches. |
| apps/dashboard/src/components/stack-companion/feature-request-board.tsx | Switches Featurebase interactions to internal authenticated endpoints and reuses shared headers. |
| apps/dashboard/src/components/feedback-form.tsx | Replaces Web3Forms submission with authenticated POST to internal feedback endpoint. |
| apps/backend/src/lib/internal-feedback-emails.tsx | Adds internal email composition + recipient parsing + send helpers for support/feature-request notifications. |
| apps/backend/src/lib/featurebase.tsx | Centralizes Featurebase API key access + user mapping helper. |
| apps/backend/src/app/api/latest/internal/feedback/route.tsx | Adds authenticated internal feedback endpoint that triggers support feedback email. |
| apps/backend/src/app/api/latest/internal/feature-requests/route.tsx | Refactors Featurebase integration, requires API key, and sends internal notification email after post creation. |
| apps/backend/src/app/api/latest/internal/feature-requests/[featureRequestId]/upvote/route.tsx | Refactors upvote route to use shared Featurebase helpers / required API key. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/src/components/stack-companion/feature-request-board.tsx (1)
133-147:⚠️ Potential issue | 🔴 CriticalThe rollback path reapplies the optimistic upvote.
The optimistic path adds the id and increments the count once, but both failure branches call
add(postId)andupvotes + 1again. A failed request leaves the card in a more-upvoted state than before instead of rolling back.💡 Example fix
} else { console.error('Failed to upvote feature request'); // Revert optimistic updates on failure setUserUpvotes(prev => { const newSet = new Set(prev); - newSet.add(postId); + newSet.delete(postId); return newSet; }); setExistingRequests(prev => prev.map(request => request.id === postId ? { ...request, - upvotes: request.upvotes + 1 + upvotes: Math.max(0, request.upvotes - 1) } : request )); } } catch (error) { console.error('Error upvoting feature request:', error); // Revert optimistic updates on failure setUserUpvotes(prev => { const newSet = new Set(prev); - newSet.add(postId); + newSet.delete(postId); return newSet; }); setExistingRequests(prev => prev.map(request => request.id === postId ? { ...request, - upvotes: request.upvotes + 1 + upvotes: Math.max(0, request.upvotes - 1) } : request )); throw error; }Also applies to: 164-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/stack-companion/feature-request-board.tsx` around lines 133 - 147, The failure branches currently reapply the optimistic changes (they call newSet.add(postId) and increment request.upvotes), so on error the UI stays over-upvoted; capture the previous state before making optimistic updates (e.g., store previousUpvotes/previousUserUpvotes), or on error explicitly revert: call setUserUpvotes to remove postId (not add) and call setExistingRequests to decrement the matched request's upvotes (or restore the saved request object). Update both places where you do the optimistic add/increment (the block using setUserUpvotes and setExistingRequests around postId and the other occurrence noted) so rollbacks remove the id and decrement/restore upvotes instead of re-adding them.
🧹 Nitpick comments (2)
apps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts (1)
9-84: Please cover the auth requirement explicitly.The main contract change here is that feedback moved behind an authenticated internal endpoint, but the suite only checks happy-path and schema validation. Add a request without auth and assert it is rejected so this path cannot regress back to anonymous submissions.
As per coding guidelines, "
**/apps/e2e/**/*.{ts,tsx}: ALWAYS add new E2E tests when you change the API or SDK interface. Generally, err on the side of creating too many tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts` around lines 9 - 84, Add an E2E test that verifies the auth requirement for POST /api/v1/internal/feedback by making a request with niceBackendFetch without calling Auth.Otp.signIn (i.e., unauthenticated) and asserting the request is rejected: check response.status is 401 (or 403 if your API uses that) and that the response includes the expected error header (e.g., presence of "x-stack-known-error"). Place this as a new it block alongside the existing tests in feedback.test.ts and reference the same endpoint string "/api/v1/internal/feedback" and niceBackendFetch call so the test fails if the endpoint becomes anonymously accessible again.apps/backend/src/app/api/latest/internal/feature-requests/route.tsx (1)
70-93: Add type definitions or justification comments foranyusage.Multiple
anytypes are used for the Featurebase API response objects (post,upvoter) without explanation. Per coding guidelines, whenanyis necessary, a comment should explain why it's used and how errors would still be flagged.Consider either:
- Define a type for the Featurebase API response shape (preferred).
- Add a comment explaining why
anyis acceptable here (e.g., external API response not under our control, validated by downstream usage).♻️ Option 1: Define types for Featurebase responses
// Add near the top of the file or in a shared types file type FeaturebasePost = { id: string; title: string; content: string | null; upvotes?: number; date: string; postStatus?: { name: string; color: string; type?: string } | null; mergedToSubmissionId?: string | null; }; type FeaturebaseUpvoter = { userId: string; };Then use these types in place of
any.♻️ Option 2: Add justification comment
- const activePosts = posts.filter((post: any) => + // Using `any` for Featurebase API response - external API shape not under our control, + // relevant fields are validated by schema on return + const activePosts = posts.filter((post: any) =>As per coding guidelines: "Try to avoid the
anytype. Whenever you need to useany, leave a comment explaining why you're using it and how you can be certain that errors would still be flagged."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/app/api/latest/internal/feature-requests/route.tsx` around lines 70 - 93, Replace the untyped any usage in the Featurebase response handling by defining and using explicit types (e.g., FeaturebasePost and FeaturebaseUpvoter) and apply them to variables such as posts, activePosts.map callback parameter (post), and the upvoter items checked in postsWithUpvoteStatus; alternatively, if you opt to keep any, add a short justification comment explaining the external API nature and what runtime/shape checks you rely on (references: posts, activePosts, postsWithUpvoteStatus, upvoteResponse, featurebaseUser) so reviewers know why type safety is relaxed and how errors will be caught.
🤖 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/backend/src/app/api/latest/internal/feature-requests/route.tsx`:
- Around line 185-202: Replace the try-catch-all around
sendFeatureRequestNotificationEmail so we don't swallow errors; instead invoke
the notification as a fire-and-forget using the project's helper
(runAsynchronously) with the same payload (tenancy: auth.tenancy, user:
auth.user, title: body.title, content: body.content ?? null, featureRequestId:
data.id) so errors are handled consistently; remove the
StackAssertionError/captureError block and call runAsynchronously with an
identifying task name (e.g., "feature-request-notification-email") so the
framework will log/alert on failures per codebase conventions.
In `@apps/backend/src/app/api/latest/internal/feedback/route.tsx`:
- Around line 20-24: The request body schema (the yupObject with fields name,
email, message) lacks server-side length caps for name and message, allowing
arbitrarily large payloads; add .max(...) constraints to the name and message
validators (e.g., name: yupString().optional().max(100) and message:
yupString().defined().nonEmpty().max(2000)) and ensure these exact limits mirror
the client/form schema so both the API route and the form use the same
constants; update any shared validation constants or the form schema reference
so the limits remain consistent and update types/handlers that assume these
fields if needed.
In `@apps/backend/src/lib/internal-feedback-emails.tsx`:
- Around line 141-166: The test mutates
process.env.STACK_INTERNAL_FEEDBACK_RECIPIENTS and doesn't always restore it if
an assertion fails; wrap the section that sets
STACK_INTERNAL_FEEDBACK_RECIPIENTS and runs the expects in a try/finally around
getInternalFeedbackRecipients() calls so the original previousValue is always
restored in the finally block, using delete when previousValue is undefined and
reassigning when it exists; update the test surrounding
getInternalFeedbackRecipients() to use that try/finally pattern.
In `@apps/dashboard/src/components/stack-companion/feature-request-board.tsx`:
- Line 52: The effect(s) that call the fetch logic (e.g., the useEffect(s) that
call fetchRequests/loadFeatureRequests and setIsLoadingRequests) currently run
with an empty dependency array and bail out when useUser() is null, which clears
the board and never retries after hydration; update those effects to include the
user (or user?.id / user?.email as appropriate) in the dependency array and
remove the early-return that clears state so the authenticated fetch re-runs
when the user hydrates, or alternatively gate the fetch behind a check that
preserves isLoadingRequests until fetch completes; ensure you update the
specific hooks that reference useUser(), fetchRequests, loadFeatureRequests, and
setIsLoadingRequests so they retry after user becomes non-null.
In `@apps/dashboard/src/lib/internal-project-headers.ts`:
- Around line 3-13: The getInternalProjectHeaders function currently masks
missing credentials by returning empty strings; update it to fail fast: if
options.accessToken is null/empty, throw an error (e.g., via throwErr or new
Error) instead of using "" for "X-Stack-Access-Token", and likewise require
getPublicEnvVar("NEXT_PUBLIC_STACK_PUBLISHABLE_CLIENT_KEY") to be non-empty and
throw if missing before building the headers; keep the existing header keys but
ensure both required values are validated and thrown on early so callers cannot
send invalid headers to the backend.
---
Outside diff comments:
In `@apps/dashboard/src/components/stack-companion/feature-request-board.tsx`:
- Around line 133-147: The failure branches currently reapply the optimistic
changes (they call newSet.add(postId) and increment request.upvotes), so on
error the UI stays over-upvoted; capture the previous state before making
optimistic updates (e.g., store previousUpvotes/previousUserUpvotes), or on
error explicitly revert: call setUserUpvotes to remove postId (not add) and call
setExistingRequests to decrement the matched request's upvotes (or restore the
saved request object). Update both places where you do the optimistic
add/increment (the block using setUserUpvotes and setExistingRequests around
postId and the other occurrence noted) so rollbacks remove the id and
decrement/restore upvotes instead of re-adding them.
---
Nitpick comments:
In `@apps/backend/src/app/api/latest/internal/feature-requests/route.tsx`:
- Around line 70-93: Replace the untyped any usage in the Featurebase response
handling by defining and using explicit types (e.g., FeaturebasePost and
FeaturebaseUpvoter) and apply them to variables such as posts, activePosts.map
callback parameter (post), and the upvoter items checked in
postsWithUpvoteStatus; alternatively, if you opt to keep any, add a short
justification comment explaining the external API nature and what runtime/shape
checks you rely on (references: posts, activePosts, postsWithUpvoteStatus,
upvoteResponse, featurebaseUser) so reviewers know why type safety is relaxed
and how errors will be caught.
In `@apps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts`:
- Around line 9-84: Add an E2E test that verifies the auth requirement for POST
/api/v1/internal/feedback by making a request with niceBackendFetch without
calling Auth.Otp.signIn (i.e., unauthenticated) and asserting the request is
rejected: check response.status is 401 (or 403 if your API uses that) and that
the response includes the expected error header (e.g., presence of
"x-stack-known-error"). Place this as a new it block alongside the existing
tests in feedback.test.ts and reference the same endpoint string
"/api/v1/internal/feedback" and niceBackendFetch call so the test fails if the
endpoint becomes anonymously accessible again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3257eae7-66f2-4bb8-9a53-8afa66242528
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/backend/src/app/api/latest/internal/feature-requests/[featureRequestId]/upvote/route.tsxapps/backend/src/app/api/latest/internal/feature-requests/route.tsxapps/backend/src/app/api/latest/internal/feedback/route.tsxapps/backend/src/lib/featurebase.tsxapps/backend/src/lib/internal-feedback-emails.tsxapps/dashboard/src/components/feedback-form.tsxapps/dashboard/src/components/stack-companion/feature-request-board.tsxapps/dashboard/src/lib/internal-project-headers.tsapps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts
- Introduced TypeScript types for Featurebase API responses in the feature requests route. - Updated the feedback form validation to enforce maximum character limits for name and message fields. - Removed the feature request notification email functionality from the backend, streamlining the feature request process. - Enhanced error handling in the feature request board component to improve user experience during upvote actions. - Improved internal project headers to ensure required environment variables are validated before use.
|
@greptile-ai review and rescore |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/backend/src/lib/internal-feedback-emails.tsx (1)
13-15: Normalize Windows newlines before HTML line-break conversion.Line 14 only replaces
\n; for\r\ninput,\rcan leak into rendered content. Prefer a CRLF-safe pattern.Proposed patch
function formatTextForHtml(text: string): string { - return escapeHtml(text).replace(/\n/g, "<br />"); + return escapeHtml(text).replace(/\r?\n/g, "<br />"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/src/lib/internal-feedback-emails.tsx` around lines 13 - 15, The function formatTextForHtml currently only replaces "\n" which lets "\r" from CRLF pairs leak through; update formatTextForHtml to normalize all newline variants by using a CRLF-safe regex (e.g. replace(/\r\n|\r|\n/g, "<br />")) after escaping (keep escapeHtml(text) then apply the new regex) so CR, LF, and CRLF are all converted to <br />; refer to the formatTextForHtml function and escapeHtml call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/backend/src/lib/internal-feedback-emails.tsx`:
- Around line 13-15: The function formatTextForHtml currently only replaces "\n"
which lets "\r" from CRLF pairs leak through; update formatTextForHtml to
normalize all newline variants by using a CRLF-safe regex (e.g.
replace(/\r\n|\r|\n/g, "<br />")) after escaping (keep escapeHtml(text) then
apply the new regex) so CR, LF, and CRLF are all converted to <br />; refer to
the formatTextForHtml function and escapeHtml call when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcb971ec-27c0-4589-99e4-c0759f4fbf4e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
apps/backend/src/app/api/latest/internal/feature-requests/route.tsxapps/backend/src/app/api/latest/internal/feedback/route.tsxapps/backend/src/lib/internal-feedback-emails.tsxapps/dashboard/src/components/feedback-form.tsxapps/dashboard/src/components/stack-companion/feature-request-board.tsxapps/dashboard/src/lib/internal-project-headers.tsapps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts
- apps/dashboard/src/lib/internal-project-headers.ts
- apps/backend/src/app/api/latest/internal/feedback/route.tsx
- apps/dashboard/src/components/stack-companion/feature-request-board.tsx
- apps/dashboard/src/components/feedback-form.tsx
- apps/backend/src/app/api/latest/internal/feature-requests/route.tsx
- Changed expected response status from 401 to 400 for invalid requests. - Updated error codes and messages to align with new schema validation requirements, specifically indicating that 'auth.user must be defined' is now a validation error.
- Updated the FeaturebaseListResponse type to allow optional results. - Enhanced handling of API responses by providing default empty arrays for results. - Improved user experience in the FeatureRequestBoard component by disabling the upvote button for unauthenticated users and using a new alert mechanism for asynchronous actions.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/src/components/stack-companion/feature-request-board.tsx (1)
219-236:⚠️ Potential issue | 🟡 MinorRemove
console.errorcalls per coding guidelines.The
console.errorcalls at lines 222, 231, and 235 violate the guideline: "NEVER .catch(console.error) or similar."Additionally, the nested try-catch at lines 219-223 silently swallows refresh errors. Since the submission already succeeded at this point, consider either:
- Letting the error propagate (Button's internal
runAsynchronouslyWithAlertwill handle it)- Or removing the console.error and accepting silent failure for this non-critical refresh
Suggested fix: remove console.error and let errors surface appropriately
if (response.ok && responseData.success) { setSubmitStatus('success'); setFeatureTitle(''); setFeatureContent(''); // Refresh the feature requests list - try { - await fetchFeatureRequests(); - } catch (error) { - console.error('Failed to refresh feature requests:', error); - } + // Fire-and-forget refresh; errors will surface via runAsynchronouslyWithAlert + runAsynchronously(fetchFeatureRequests()); // Auto-reset status and hide form after success setTimeout(() => { setSubmitStatus('idle'); setShowSubmitForm(false); }, 3000); } else { - console.error('Backend API error:', responseData); throw new Error(`Failed to submit feature request: ${responseData.error || response.statusText}`); } } catch (error) { - console.error('Error submitting feature request:', error); setSubmitStatus('error'); + // Error is caught and surfaced via submitStatus UI } finally {Based on learnings: "NEVER try-catch-all, NEVER void a promise, and NEVER .catch(console.error) or similar. Use
runAsynchronouslyorrunAsynchronouslyWithAlertinstead for error handling."
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b53404d-9778-486b-8fb3-0d7725a8347b
📒 Files selected for processing (4)
apps/backend/src/app/api/latest/internal/feature-requests/route.tsxapps/backend/src/lib/internal-feedback-emails.tsxapps/dashboard/src/components/stack-companion/feature-request-board.tsxapps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/e2e/tests/backend/endpoints/api/v1/internal/feedback.test.ts
- apps/backend/src/app/api/latest/internal/feature-requests/route.tsx
|
@greptile-ai re-review and rescore |
…te handling - Added STACK_INTERNAL_FEEDBACK_RECIPIENTS environment variable for internal feedback email recipients. - Updated getInternalFeedbackRecipients function to handle missing environment variable case. - Changed isHighPriority flag to false in sendInternalOperationsEmail function. - Improved loading state management in FeatureRequestBoard component by ensuring loading state is set correctly when user is null.
|
@greptile-ai review |
Summary
Testing
Summary by CodeRabbit
New Features
Enhancements
Tests