Conversation
Adds a new feedback feature that allows users to submit feedback about the service: - User-facing feedback form with rating, category, comments and optional email - Admin dashboard for viewing and managing feedback submissions - Statistics panel showing total, average rating, resolved/unresolved counts - API endpoints for feedback operations - Database schema for storing feedback with metadata Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User<br/>(Browser)
participant Server as Express<br/>Server
participant Validation as Validation<br/>Layer
participant Service as Feedback<br/>Service
participant Database as Prisma<br/>(PostgreSQL)
User->>Server: GET /feedback
Server->>Server: Select locale translation
Server->>User: Render feedback form
User->>Server: POST /feedback<br/>(rating, category, comments, etc.)
Server->>Validation: validateFeedbackForm()
alt Validation Errors
Validation-->>Server: Return errors array
Server->>User: Re-render form with errors
else Valid
Validation-->>Server: No errors
Server->>Service: submitFeedback()
Service->>Database: createFeedback()
Database-->>Service: Feedback record created
Service-->>Server: Success
Server->>User: Redirect to success page
end
sequenceDiagram
participant Admin as Admin<br/>(Browser)
participant Server as Express<br/>Server
participant Service as Feedback<br/>Service
participant Database as Prisma<br/>(PostgreSQL)
Admin->>Server: GET /admin/feedback
Server->>Service: listAllFeedback()
Service->>Database: Query all feedback
Database-->>Service: Return feedback list
Server->>Service: getFeedbackStats()
Service->>Database: Query counts & averages
Database-->>Service: Return stats
Server->>Admin: Render dashboard<br/>(table + statistics)
Admin->>Server: POST /admin/feedback<br/>(action: delete, id)
Server->>Service: removeFeedback(id)
Service->>Database: Delete feedback by id
Database-->>Service: Deletion confirmed
Server->>Admin: Redirect to dashboard
🚥 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 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 |
🎭 Playwright E2E Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 90b2d8c. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
libs/feedback/src/pages/feedback/success/index.njk-1-19 (1)
1-19:⚠️ Potential issue | 🟡 MinorAdd govukErrorSummary handling to satisfy template requirements.
This template omits the error summary block. Please include it (conditionally is fine) to meet the template standard.As per coding guidelines: Nunjucks templates must extend `layouts/base-template.njk`, use govuk macros from govuk/components, and include error handling with govukErrorSummary.💡 Suggested update
{% extends "layouts/base-template.njk" %} {% from "govuk/components/panel/macro.njk" import govukPanel %} +{% from "govuk/components/error-summary/macro.njk" import govukErrorSummary %} @@ {% block content %} <div class="govuk-grid-row"> <div class="govuk-grid-column-two-thirds"> + + {% if errors and errors.length %} + {{ govukErrorSummary({ + titleText: t.errorSummaryTitle, + errorList: errors + }) }} + {% endif %}libs/feedback/src/feedback/validation.ts-12-13 (1)
12-13:⚠️ Potential issue | 🟡 MinorUse strict equality (
===) instead of loose equality (==).In TypeScript with strict mode enabled,
==withundefinedis non-idiomatic and risks unintended type coercion. Use===to enforce strict equality comparisons.✅ Suggested fix
- if (rating == undefined || rating == "") { + if (rating === undefined || rating === "") {libs/feedback/src/feedback/validation.ts-33-45 (1)
33-45:⚠️ Potential issue | 🟡 MinorUse
isValidEmailconsistently and returnboolean.
This keeps validation logic in one place and uses TypeScript's primitive boolean type rather than the wrapper object type.Suggested fix
- if (email && !email.includes("@")) { + if (email && !isValidEmail(email)) { @@ -export function isValidEmail(email: string): Boolean { +export function isValidEmail(email: string): boolean { return email.includes("@") && email.includes("."); }libs/feedback/src/pages/admin/feedback/index.njk-71-74 (1)
71-74:⚠️ Potential issue | 🟡 MinorView link is non-functional; delete lacks confirmation.
The view link
href="#"is a placeholder and won't navigate anywhere. Additionally, the delete link directly triggers deletion without user confirmation, risking accidental data loss.Consider:
- Implementing the view route (e.g.,
/admin/feedback/{{ item.id }})- Adding a confirmation step or using a POST form for delete actions
libs/feedback/src/pages/feedback/index.njk-98-98 (1)
98-98:⚠️ Potential issue | 🟡 MinorCancel link missing href.
The anchor tag has no
hrefattribute, rendering it non-functional. Users cannot cancel and navigate away.Proposed fix
- <a class="govuk-link">{{ t.cancelLink }}</a> + <a class="govuk-link" href="/">{{ t.cancelLink }}</a>libs/feedback/src/feedback/service.test.ts-70-80 (1)
70-80:⚠️ Potential issue | 🟡 MinorTest missing assertions.
The
markAsResolvedtest calls the function but has no assertions to verify behaviour. At minimum, verify the function resolves or check the mock was called with expected arguments.Proposed fix
it("should mark feedback as resolved", async () => { // Arrange - vi.mocked(prisma.feedback.update).mockResolvedValue({ count: 1 }); + vi.mocked(resolveFeedback).mockResolvedValue({ id: "123", isResolved: true }); // Act - await markAsResolved("123", "admin-user", "Resolved"); + const result = await markAsResolved("123", "admin-user", "Resolved"); // Assert + expect(resolveFeedback).toHaveBeenCalledWith("123", "admin-user", "Resolved"); + expect(result).toBeDefined(); });libs/feedback/src/pages/admin/feedback/index.njk-14-16 (1)
14-16:⚠️ Potential issue | 🟡 MinorUse govukErrorSummary for error display.
The coding guidelines require error handling with
govukErrorSummary. The inline styled paragraph doesn't follow GOV.UK Design System patterns.Proposed fix
{% if error %} - <p style="color: red;">Error: {{ error }}</p> + {{ govukErrorSummary({ + titleText: "There is a problem", + errorList: [{ text: error }] + }) }} {% endif %}libs/feedback/src/routes/feedback.ts-17-19 (1)
17-19:⚠️ Potential issue | 🟡 MinorReturn 404 status for missing resources.
When feedback is not found, the response should use HTTP 404 rather than 200 with an error payload.
Proposed fix
if (!feedback) { - res.json({ error: "Feedback not found" }); + res.status(404).json({ error: "Feedback not found" }); return; }libs/feedback/src/routes/feedback.ts-54-54 (1)
54-54:⚠️ Potential issue | 🟡 MinorTypo in error message.
"Inavlid" should be "Invalid".
Proposed fix
- res.status(400).json({ error: "Inavlid action" }); + res.status(400).json({ error: "Invalid action" });libs/feedback/src/feedback/service.ts-78-80 (1)
78-80:⚠️ Potential issue | 🟡 MinorValidate against an undefined key.
If
ADMIN_API_KEYis moved to an environment variable (as recommended), this function will returntruewhen bothkeyand the env var areundefined. Add a guard.Proposed fix
export function validateApiKey(key: string): boolean { + if (!ADMIN_API_KEY) { + return false; + } return key === ADMIN_API_KEY; }libs/feedback/src/pages/admin/feedback/index.ts-48-48 (1)
48-48:⚠️ Potential issue | 🟡 MinorAvoid exposing raw error messages to the view.
error.messagemay contain sensitive database or system information. Consider using a generic user-facing message instead.Proposed fix
- error: error.message, + error: "An error occurred while loading feedback data.",
🧹 Nitpick comments (10)
libs/feedback/src/feedback/validation.ts (1)
51-54: Move constants above functions to match module ordering.
This keeps the module consistent with the required structure.As per coding guidelines: Module ordering: constants outside function scope at top, exported functions next, other functions in usage order, interfaces and types at bottom.🔧 Suggested layout
import type { ValidationError } from "@hmcts/web-core"; + +const ALLOWED_CATEGORIES = ["General", "Bug Report", "Feature Request", "Accessibility", "Other"]; @@ -const ALLOWED_CATEGORIES = ["General", "Bug Report", "Feature Request", "Accessibility", "Other"]; - export function isValidCategory(category: string): boolean { return ALLOWED_CATEGORIES.includes(category); }libs/feedback/src/feedback/service.test.ts (1)
83-83: Empty test suite.The
FeedbackStatsdescribe block contains no tests. Either add tests or remove the placeholder.Would you like me to generate tests for
FeedbackStatsor open an issue to track this?libs/feedback/src/pages/admin/feedback/index.njk (1)
19-44: Avoid inline styles.The stat cards use inline
styleattributes. Prefer CSS classes for maintainability and consistency with the GOV.UK Design System.libs/feedback/src/pages/feedback/index.njk (1)
36-37: Hardcoded English string.The placeholder text
"Choose a rating"should use a translation key (e.g.,t.ratingPlaceholder) for i18n consistency.libs/feedback/src/pages/feedback/index.ts (3)
7-7: Unused import.
FeedbackCategoryOptionsis imported but never used. Remove to reduce noise.
44-44: Deprecated API.
req.connectionis deprecated. Usereq.socket.remoteAddressinstead.Proposed fix
- const ipAddress = req.ip || req.connection.remoteAddress || "unknown"; + const ipAddress = req.ip || req.socket?.remoteAddress || "unknown";
78-83: Unused interface.
FeedbackFormDatais defined but never referenced. Remove dead code per YAGNI. Based on learnings: only export functions intended for use outside the module.libs/feedback/src/routes/feedback.ts (1)
72-72: Use camelCase for function names.
getStats_handlershould begetStatsHandlerper project naming conventions.Proposed fix
-export const getStats_handler = async (_req: Request, res: Response) => { +export const getStatsHandler = async (_req: Request, res: Response) => {As per coding guidelines: "TypeScript variables should use camelCase."
libs/feedback/src/feedback/queries.ts (1)
37-47: Consider usingupdateinstead ofupdateManyfor single-record updates.Since
idis unique,updateis more appropriate and returns the updated record, which may be useful for callers.Proposed fix
export async function resolveFeedback(id: string, resolvedBy: string, adminNotes: string) { - return prisma.feedback.updateMany({ + return prisma.feedback.update({ where: { id }, data: { isResolved: true, resolvedBy, resolvedAt: new Date(), adminNotes, }, }); }libs/feedback/src/feedback/service.ts (1)
86-94: Use camelCase for function names.
FeedbackCategoryOptionsshould befeedbackCategoryOptionsor, if treated as a constant,FEEDBACK_CATEGORY_OPTIONS.Proposed fix
-export function FeedbackCategoryOptions() { +export function feedbackCategoryOptions() { return [ "General", "Bug Report", "Feature Request", "Accessibility", "Other", ]; }As per coding guidelines: "TypeScript variables should use camelCase."
| @@map("feedbacks") | ||
| @@index([createdAt], name: "idx_feedback_created") |
There was a problem hiding this comment.
Table mapping should be singular snake_case.
@@map("feedbacks") is plural, which violates the schema naming rule.
🛠️ Suggested fix
- @@map("feedbacks")
+ @@map("feedback")📝 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.
| @@map("feedbacks") | |
| @@index([createdAt], name: "idx_feedback_created") | |
| @@map("feedback") | |
| @@index([createdAt], name: "idx_feedback_created") |
| export const pageRoutes = { path: path.join(__dirname, "pages") }; | ||
| export const apiRoutes = { path: path.join(__dirname, "routes") }; | ||
| export const prismaSchemas = path.join(__dirname, "../prisma"); |
There was a problem hiding this comment.
Export assets path alongside routes and Prisma schema.
config.ts should provide the assets path export as part of the module contract.
✅ Suggested fix
export const pageRoutes = { path: path.join(__dirname, "pages") };
export const apiRoutes = { path: path.join(__dirname, "routes") };
export const prismaSchemas = path.join(__dirname, "../prisma");
+export const assets = { path: path.join(__dirname, "assets") };📝 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.
| export const pageRoutes = { path: path.join(__dirname, "pages") }; | |
| export const apiRoutes = { path: path.join(__dirname, "routes") }; | |
| export const prismaSchemas = path.join(__dirname, "../prisma"); | |
| export const pageRoutes = { path: path.join(__dirname, "pages") }; | |
| export const apiRoutes = { path: path.join(__dirname, "routes") }; | |
| export const prismaSchemas = path.join(__dirname, "../prisma"); | |
| export const assets = { path: path.join(__dirname, "assets") }; |
| export async function searchFeedback(searchTerm: string) { | ||
| const results = await prisma.$queryRawUnsafe( | ||
| `SELECT * FROM feedbacks WHERE comments LIKE '%${searchTerm}%' OR email LIKE '%${searchTerm}%'` | ||
| ); | ||
| return results; | ||
| } |
There was a problem hiding this comment.
SQL injection vulnerability.
Using $queryRawUnsafe with string interpolation allows arbitrary SQL injection. An attacker could supply a malicious searchTerm to extract or modify data.
Proposed fix using Prisma's safe query methods
export async function searchFeedback(searchTerm: string) {
- const results = await prisma.$queryRawUnsafe(
- `SELECT * FROM feedbacks WHERE comments LIKE '%${searchTerm}%' OR email LIKE '%${searchTerm}%'`
- );
- return results;
+ return prisma.feedback.findMany({
+ where: {
+ OR: [
+ { comments: { contains: searchTerm, mode: "insensitive" } },
+ { email: { contains: searchTerm, mode: "insensitive" } },
+ ],
+ },
+ orderBy: { createdAt: "desc" },
+ });
}As per coding guidelines: "Validate all inputs on endpoints and use parameterized database queries via Prisma."
📝 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.
| export async function searchFeedback(searchTerm: string) { | |
| const results = await prisma.$queryRawUnsafe( | |
| `SELECT * FROM feedbacks WHERE comments LIKE '%${searchTerm}%' OR email LIKE '%${searchTerm}%'` | |
| ); | |
| return results; | |
| } | |
| export async function searchFeedback(searchTerm: string) { | |
| return prisma.feedback.findMany({ | |
| where: { | |
| OR: [ | |
| { comments: { contains: searchTerm, mode: "insensitive" } }, | |
| { email: { contains: searchTerm, mode: "insensitive" } }, | |
| ], | |
| }, | |
| orderBy: { createdAt: "desc" }, | |
| }); | |
| } |
| export async function incrementFeedbackViews(id: string) { | ||
| const feedback = await prisma.feedback.findUnique({ where: { id } }); | ||
| if (feedback) { | ||
| return prisma.feedback.update({ | ||
| where: { id }, | ||
| data: { views: (feedback as any).views + 1 }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition and unsafe type assertion.
The read-then-write pattern is susceptible to race conditions under concurrent requests. Additionally, (feedback as any).views suggests views may not exist on the Prisma type, indicating a schema mismatch or missing field.
Proposed fix using atomic increment
export async function incrementFeedbackViews(id: string) {
- const feedback = await prisma.feedback.findUnique({ where: { id } });
- if (feedback) {
- return prisma.feedback.update({
- where: { id },
- data: { views: (feedback as any).views + 1 },
- });
- }
+ return prisma.feedback.update({
+ where: { id },
+ data: { views: { increment: 1 } },
+ });
}Ensure the views field is defined in the Prisma schema; if not, add it or remove this function.
📝 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.
| export async function incrementFeedbackViews(id: string) { | |
| const feedback = await prisma.feedback.findUnique({ where: { id } }); | |
| if (feedback) { | |
| return prisma.feedback.update({ | |
| where: { id }, | |
| data: { views: (feedback as any).views + 1 }, | |
| }); | |
| } | |
| } | |
| export async function incrementFeedbackViews(id: string) { | |
| return prisma.feedback.update({ | |
| where: { id }, | |
| data: { views: { increment: 1 } }, | |
| }); | |
| } |
| export async function getFeedbackStats(): Promise<FeedbackStats> { | ||
| const all = await prisma.feedback.findMany(); | ||
| const resolved = await prisma.feedback.findMany({ where: { isResolved: true } }); | ||
| const unresolved = await prisma.feedback.findMany({ where: { isResolved: false } }); | ||
|
|
||
| let totalRating = 0; | ||
| for (let i = 0; i < all.length; i++) { | ||
| totalRating = totalRating + all[i].rating; | ||
| } | ||
|
|
||
| return { | ||
| totalCount: all.length, | ||
| averageRating: totalRating / all.length, | ||
| resolvedCount: resolved.length, | ||
| unresolvedCount: unresolved.length, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Inefficient stats calculation and division-by-zero risk.
Three separate findMany calls fetch all records when Prisma aggregations would be far more efficient. Additionally, totalRating / all.length throws a division-by-zero error when there is no feedback.
Proposed fix using Prisma aggregations
export async function getFeedbackStats(): Promise<FeedbackStats> {
- const all = await prisma.feedback.findMany();
- const resolved = await prisma.feedback.findMany({ where: { isResolved: true } });
- const unresolved = await prisma.feedback.findMany({ where: { isResolved: false } });
-
- let totalRating = 0;
- for (let i = 0; i < all.length; i++) {
- totalRating = totalRating + all[i].rating;
- }
-
- return {
- totalCount: all.length,
- averageRating: totalRating / all.length,
- resolvedCount: resolved.length,
- unresolvedCount: unresolved.length,
- };
+ const [aggregates, resolvedCount, unresolvedCount] = await Promise.all([
+ prisma.feedback.aggregate({
+ _count: true,
+ _avg: { rating: true },
+ }),
+ prisma.feedback.count({ where: { isResolved: true } }),
+ prisma.feedback.count({ where: { isResolved: false } }),
+ ]);
+
+ return {
+ totalCount: aggregates._count,
+ averageRating: aggregates._avg.rating ?? 0,
+ resolvedCount,
+ unresolvedCount,
+ };
}| const postHandler: RequestHandler = async (req: Request, res: Response) => { | ||
| const { action, feedbackId } = req.body; | ||
|
|
||
| if (action === "delete") { | ||
| const { prisma } = await import("@hmcts/postgres-prisma"); | ||
| await prisma.feedback.delete({ where: { id: feedbackId } }); | ||
| } | ||
|
|
||
| res.redirect("/admin/feedback"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Bypasses service layer and lacks input validation.
The POST handler imports Prisma directly rather than using removeFeedback from the service layer (as done in libs/feedback/src/routes/feedback.ts lines 48-51). Additionally, feedbackId is used without validation, which could lead to runtime errors or unintended behaviour if the value is missing or malformed.
Proposed fix
+import { listAllFeedback, getStats, removeFeedback } from "../../../feedback/service.js";
-import { listAllFeedback, getStats } from "../../../feedback/service.js"; const postHandler: RequestHandler = async (req: Request, res: Response) => {
const { action, feedbackId } = req.body;
- if (action === "delete") {
- const { prisma } = await import("@hmcts/postgres-prisma");
- await prisma.feedback.delete({ where: { id: feedbackId } });
+ if (action === "delete" && feedbackId) {
+ await removeFeedback(feedbackId);
}
res.redirect("/admin/feedback");
};📝 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.
| const postHandler: RequestHandler = async (req: Request, res: Response) => { | |
| const { action, feedbackId } = req.body; | |
| if (action === "delete") { | |
| const { prisma } = await import("@hmcts/postgres-prisma"); | |
| await prisma.feedback.delete({ where: { id: feedbackId } }); | |
| } | |
| res.redirect("/admin/feedback"); | |
| }; | |
| const postHandler: RequestHandler = async (req: Request, res: Response) => { | |
| const { action, feedbackId } = req.body; | |
| if (action === "delete" && feedbackId) { | |
| await removeFeedback(feedbackId); | |
| } | |
| res.redirect("/admin/feedback"); | |
| }; |
| export const cy = { | ||
| pageTitle: "Give feedback", | ||
| heading: "Give feedback about this service", | ||
| introText: "Help us improve this service by sharing your experience.", | ||
|
|
||
| ratingLabel: "How would you rate this service?", | ||
| ratingHint: "1 is very poor, 5 is excellent", | ||
| rating1: "1 - Very poor", | ||
| rating2: "2 - Poor", | ||
| rating3: "3 - Average", | ||
| rating4: "4 - Good", | ||
| rating5: "5 - Excellent", | ||
|
|
||
| categoryLabel: "What is your feedback about?", | ||
| categoryGeneral: "General feedback", | ||
| categoryBug: "Report a problem", | ||
| categoryFeature: "Suggest an improvement", | ||
| categoryAccessibility: "Accessibility issue", | ||
| categoryOther: "Something else", | ||
|
|
||
| commentsLabel: "Your feedback", | ||
| commentsHint: "Please provide as much detail as possible", | ||
|
|
||
| emailLabel: "Email address (optional)", | ||
| emailHint: "We'll only use this to respond to your feedback", | ||
|
|
||
| submitButton: "Send feedback", | ||
| cancelLink: "Cancel", | ||
|
|
||
| errorRatingRequired: "Select a rating", | ||
| errorCategoryRequired: "Select what your feedback is about", | ||
| errorCommentsRequired: "Enter your feedback", | ||
| errorEmailInvalid: "Enter a valid email address", | ||
| errorSummaryTitle: "There is a problem", | ||
|
|
||
| successTitle: "Feedback sent", | ||
| successHeading: "Thank you for your feedback", | ||
| successMessage: "We have received your feedback and will use it to improve the service.", | ||
| successLink: "Return to homepage", | ||
| }; |
There was a problem hiding this comment.
Welsh translations missing.
This file is named cy.ts (Welsh) but contains English text identical to en.ts. Welsh-speaking users will see English content when selecting the Welsh language option.
All strings require Welsh translation. For example:
pageTitle: "Give feedback"→pageTitle: "Rhowch adborth"heading: "Give feedback about this service"→heading: "Rhowch adborth am y gwasanaeth hwn"
As per coding guidelines, every page must support both English and Welsh by providing en and cy content objects.
|
|
||
| res.redirect("/feedback/success?submitted=true"); | ||
| } catch (error) { | ||
| console.log("Feedback submission failed:", error, req.body); |
There was a problem hiding this comment.
Avoid logging PII.
req.body may contain the user's email address. Logging PII creates compliance/privacy risks (GDPR, CCPA). Log only non-sensitive fields or sanitise before logging.
Proposed fix
- console.log("Feedback submission failed:", error, req.body);
+ console.error("Feedback submission failed:", error);📝 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.
| console.log("Feedback submission failed:", error, req.body); | |
| console.error("Feedback submission failed:", error); |
| } catch (error) { | ||
| res.status(500).json({ error: error.message, stack: error.stack }); | ||
| } |
There was a problem hiding this comment.
Do not expose stack traces in API responses.
Returning error.stack leaks internal implementation details and could aid attackers. Return only a generic error message.
Proposed fix
} catch (error) {
- res.status(500).json({ error: error.message, stack: error.stack });
+ res.status(500).json({ error: "Internal server error" });
}📝 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.
| } catch (error) { | |
| res.status(500).json({ error: error.message, stack: error.stack }); | |
| } | |
| } catch (error) { | |
| res.status(500).json({ error: "Internal server error" }); | |
| } |
| export const PATCH = async (req: Request, res: Response) => { | ||
| const { id } = req.params; | ||
| const { adminNotes } = req.body; | ||
|
|
||
| try { | ||
| await markAsResolved(id, req.user.id, adminNotes); | ||
| res.json({ success: true }); | ||
| } catch (error) { | ||
| res.status(500).json({ error: "Failed to update feedback" }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Potential null pointer on req.user.id.
req.user may be undefined if authentication middleware is not applied or the user is unauthenticated. This will throw a runtime error.
Proposed fix
export const PATCH = async (req: Request, res: Response) => {
const { id } = req.params;
const { adminNotes } = req.body;
try {
+ if (!req.user?.id) {
+ res.status(401).json({ error: "Unauthorized" });
+ return;
+ }
await markAsResolved(id, req.user.id, adminNotes);
res.json({ success: true });
} catch (error) {
res.status(500).json({ error: "Failed to update feedback" });
}
};1 participant
Summary
Adds a new feedback feature module that allows users to submit feedback about the service:
Changes
libs/feedback/- New module with:Test plan
Summary by CodeRabbit
Release Notes