solved potential issue#5
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds an admin role enforcement layer (route loader, nav filtering, Admin dashboard page), fully implements the History and Settings pages, adds a NotFound page and wildcard routes, extracts an API base URL helper, hardens server CORS with a dynamic allowlist, rewrites the history API controller with stricter filtering, and wraps subscription edit/renew in Prisma transactions with typed history records. ChangesAdmin Role, New Pages, and Server Hardening
Sequence Diagram(s)sequenceDiagram
participant Browser
participant adminLoader
participant dashboardLoader
participant Admin as Admin Page
participant API as Express API
Browser->>adminLoader: navigate /dashboard/admin
adminLoader->>dashboardLoader: await dashboardLoader()
dashboardLoader->>API: GET /api/me
alt fetch fails
dashboardLoader-->>Browser: redirect /auth/login
end
dashboardLoader-->>adminLoader: { user }
alt user.role !== ADMIN
adminLoader-->>Browser: redirect /dashboard
end
adminLoader-->>Admin: render
Admin->>API: GET /api/admin/stats
Admin->>API: GET /api/admin/users
API-->>Admin: stats + users list
Admin->>API: PATCH /api/admin/promote|demote/:userId
API-->>Admin: { message }
Admin-->>Browser: invalidate queries, show toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/controllers/subs.controller.js (1)
500-526: 🎯 Functional Correctness | 🟠 MajorReturn the updated entity from the transaction callback.
At line 500, the transaction callback never returns
subscription, soupdatedSubscriptionisundefinedand line 525 sendssubscription: undefined.Suggested fix
const updatedSubscription = await prisma.$transaction(async (tx) => { const subscription = await tx.subscription.update({ where: { id: existingSubscription.id, }, data: updateData, }); await tx.history.create({ data: { subscriptionId: subscription.id, subscriptionName: subscription.name, subscriptionLogoUrl: subscription.logoUrl, paidAmount: subscription.amount, paidCurrency: subscription.currency, baseCurrency, exchangeRate: subscription.exchangeRate, type: "EDITED", category: subscription.category, settledAmount: subscription.settledAmount, }, }); + return subscription; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/controllers/subs.controller.js` around lines 500 - 526, The transaction callback in the prisma.$transaction call does not return the updated subscription object. After the tx.history.create() call completes, add a return statement to return the subscription variable from the callback so that updatedSubscription receives the actual subscription data instead of undefined, which will allow the response to correctly include the updated subscription in line 525.
🧹 Nitpick comments (1)
client/src/pages/dashboard/Subscriptions.jsx (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove debug console.log statement.
This debug logging should be removed before merging to production.
🧹 Proposed cleanup
- console.log("User data in Subscriptions:", userData); - const { data, isLoading, isError, error } = useQuery({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/pages/dashboard/Subscriptions.jsx` at line 23, Remove the debug console.log statement that logs "User data in Subscriptions:" from the Subscriptions component. This statement is unnecessary for production code and should be deleted entirely to clean up the codebase before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/lib/loader.js`:
- Around line 10-13: The catch block in the loader redirects to `/auth/login`
for all errors regardless of type, but this incorrectly treats transient backend
failures (like 5xx errors from `/api/me`) as auth failures. Modify the catch
block to check the error status before redirecting to login, only redirect if
the error indicates an actual authentication failure (such as 401 or 403 status
codes). Additionally, ensure the `fetchWithAuth` function attaches a status
property to thrown errors so the error handler can reliably determine the
failure type and handle non-auth errors appropriately without redirecting to the
login page.
In `@client/src/lib/utils.js`:
- Around line 9-12: The getApiBaseUrl function returns a hardcoded fallback of
http://localhost:3000, but the backend server defaults to port 3001 according to
server/src/config/env.js. When VITE_API_URL is unset during local development,
API calls will fail because they target the wrong port. Update the fallback URL
in the getApiBaseUrl function to use port 3001 instead of 3000 to align with the
server's default configuration.
In `@client/src/pages/dashboard/History.jsx`:
- Around line 67-83: The issue is that the useEffect hook unconditionally resets
the page parameter to 1 whenever debouncedValue changes, which destroys deep
links like `?page=3` on initial load. Additionally, inputValue can drift from
the URL search parameter when navigation occurs. To fix this, modify the
setSearchParams logic to only reset page to 1 when the search query actually
transitions from empty to non-empty or vice versa (comparing previous and
current debouncedValue states), rather than on every change. Also ensure
inputValue stays synchronized with the searchQuery from the URL by initializing
it correctly and adding logic to update it when searchQuery from the URL
parameters changes.
In `@client/src/pages/dashboard/Settings.jsx`:
- Around line 42-56: In the logoutMutation's mutationFn, the response status
must be checked before attempting to parse the JSON body. Move the response.ok
check before the response.json() call so that if the server returns a non-OK
status with non-JSON content (like an HTML error page), the parsing error is
avoided and your custom error message logic executes properly instead.
In `@client/src/pages/dashboard/Subscriptions.jsx`:
- Around line 61-63: The error message fallback text in the Subscriptions page
is incorrect. In the error display block where it shows error.message || "Unable
to load history.", change the fallback string from "Unable to load history." to
"Unable to load subscriptions." to match the actual context of the Subscriptions
page.
- Around line 70-76: The PaginationList component is rendering even when data is
undefined during error states, which causes both currentPage and totalPages to
be undefined. Add a conditional check before rendering the PaginationList div to
ensure it only renders when data is successfully loaded and there are no errors.
Check both the isError state and that data exists with valid pagination
properties before rendering the entire pagination section containing the
PaginationList component.
- Around line 16-22: The handlePageChange function is mutating the searchParams
object directly by calling set() on it before passing it to setSearchParams,
which violates React's immutability principle. Instead, create a new
URLSearchParams instance from the existing searchParams, set the "page" value on
the new instance, and then pass this new instance to setSearchParams. This
ensures the original state object is not mutated and allows React to properly
detect the state change.
In `@server/server.js`:
- Around line 35-38: The allowedOrigins Set is unconditionally whitelisting
localhost origins for all environments, including production. Modify the
allowedOrigins initialization to only include localhost origins when the
environment is not production, such as by checking process.env.NODE_ENV or a
similar environment variable. Localhost origins like http://localhost:5173 and
http://127.0.0.1:5173 should be added conditionally within the Set constructor
or immediately after, only when running in development or non-production
environments.
In `@server/src/controllers/history.controller.js`:
- Around line 17-79: The code at the search parameter validation block calls
`.trim()` on the search variable without verifying it is a string first. Query
parameters can be arrays when repeated in the URL (e.g., ?search=a&search=b
becomes ['a', 'b']), which causes a TypeError when calling .trim() on an array.
Add a type check using typeof search === 'string' before calling .trim() in the
condition that checks if search is truthy to ensure only string values are
processed and prevent runtime 500 errors from user-controlled input.
---
Outside diff comments:
In `@server/src/controllers/subs.controller.js`:
- Around line 500-526: The transaction callback in the prisma.$transaction call
does not return the updated subscription object. After the tx.history.create()
call completes, add a return statement to return the subscription variable from
the callback so that updatedSubscription receives the actual subscription data
instead of undefined, which will allow the response to correctly include the
updated subscription in line 525.
---
Nitpick comments:
In `@client/src/pages/dashboard/Subscriptions.jsx`:
- Line 23: Remove the debug console.log statement that logs "User data in
Subscriptions:" from the Subscriptions component. This statement is unnecessary
for production code and should be deleted entirely to clean up the codebase
before merging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae4241bf-a436-46cf-95df-95f33a14b212
📒 Files selected for processing (29)
client/.envclient/src/components/Navbar.jsxclient/src/components/Sidebar.jsxclient/src/components/dashboard/Chart.jsxclient/src/components/ui/alert-dialog.jsxclient/src/components/ui/badge.jsxclient/src/components/ui/breadcrumb.jsxclient/src/components/ui/button.jsxclient/src/components/ui/card.jsxclient/src/components/ui/input.jsxclient/src/components/ui/label.jsxclient/src/components/ui/pagination.jsxclient/src/components/ui/select.jsxclient/src/components/ui/separator.jsxclient/src/components/ui/table.jsxclient/src/lib/action.jsclient/src/lib/loader.jsclient/src/lib/utils.jsclient/src/lib/var.jsclient/src/main.jsxclient/src/pages/NotFound.jsxclient/src/pages/dashboard/Admin.jsxclient/src/pages/dashboard/History.jsxclient/src/pages/dashboard/Settings.jsxclient/src/pages/dashboard/Subscriptions.jsxserver/server.jsserver/src/config/env.jsserver/src/controllers/history.controller.jsserver/src/controllers/subs.controller.js
💤 Files with no reviewable changes (10)
- client/src/components/ui/breadcrumb.jsx
- client/src/components/ui/separator.jsx
- client/src/components/ui/table.jsx
- client/.env
- client/src/components/ui/pagination.jsx
- client/src/components/ui/input.jsx
- client/src/components/ui/alert-dialog.jsx
- client/src/components/ui/select.jsx
- client/src/components/ui/label.jsx
- client/src/components/ui/card.jsx
Comment on lines
10
to
13
| } catch (err) { | ||
| console.error("Error fetching user data:", err); | ||
| redirect("/auth/login"); // Redirect to login page if unauthorized or on error | ||
| return redirect("/auth/login"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop redirecting to /auth/login for non-auth failures.
At Line 12, every caught error redirects to login. Since fetchWithAuth throws on any non-OK status, transient backend failures (e.g. /api/me 5xx) are treated as auth failures and force incorrect redirects.
💡 Suggested fix
export async function dashboardLoader() {
try {
const response = await fetchWithAuth("/api/me");
return await response.json();
} catch (err) {
console.error("Error fetching user data:", err);
- return redirect("/auth/login");
+ if (err?.status === 401 || err?.status === 403) {
+ return redirect("/auth/login");
+ }
+ throw err;
}
}Also make sure fetchWithAuth attaches status to thrown errors so this branch is reliable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/lib/loader.js` around lines 10 - 13, The catch block in the loader
redirects to `/auth/login` for all errors regardless of type, but this
incorrectly treats transient backend failures (like 5xx errors from `/api/me`)
as auth failures. Modify the catch block to check the error status before
redirecting to login, only redirect if the error indicates an actual
authentication failure (such as 401 or 403 status codes). Additionally, ensure
the `fetchWithAuth` function attaches a status property to thrown errors so the
error handler can reliably determine the failure type and handle non-auth errors
appropriately without redirecting to the login page.
Comment on lines
+9
to
+12
| export function getApiBaseUrl() { | ||
| return import.meta.env.VITE_API_URL || "http://localhost:3000"; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align fallback API port with server default to avoid local breakage.
Line 10 defaults to http://localhost:3000, but server/src/config/env.js defaults the backend to port 3001 (Line 6). If VITE_API_URL is unset, calls routed via Line 148 will target the wrong port and fail.
🔧 Proposed fix
export function getApiBaseUrl() {
- return import.meta.env.VITE_API_URL || "http://localhost:3000";
+ return import.meta.env.VITE_API_URL || "http://localhost:3001";
}Also applies to: 148-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/lib/utils.js` around lines 9 - 12, The getApiBaseUrl function
returns a hardcoded fallback of http://localhost:3000, but the backend server
defaults to port 3001 according to server/src/config/env.js. When VITE_API_URL
is unset during local development, API calls will fail because they target the
wrong port. Update the fallback URL in the getApiBaseUrl function to use port
3001 instead of 3000 to align with the server's default configuration.
Comment on lines
+67
to
+83
| const [inputValue, setInputValue] = useState(searchQuery); | ||
| const [debouncedValue] = useDebounce(inputValue, 500); | ||
|
|
||
| useEffect(() => { | ||
| setSearchParams( | ||
| (prevParams) => { | ||
| if (debouncedValue) { | ||
| prevParams.set("search", debouncedValue); | ||
| } else { | ||
| prevParams.delete("search"); | ||
| } | ||
| prevParams.set("page", 1); | ||
| return prevParams; | ||
| }, | ||
| { replace: true }, | ||
| ); | ||
| }, [debouncedValue, setSearchParams]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve URL state instead of forcing page=1 on initial render.
Line 70 always rewrites params and resets pagination, so deep links like ?page=3 get collapsed to page 1 on load. Also, inputValue (Line 67) can drift from URL search when navigation changes search params.
Suggested fix
const [inputValue, setInputValue] = useState(searchQuery);
const [debouncedValue] = useDebounce(inputValue, 500);
+ useEffect(() => {
+ setInputValue(searchQuery);
+ }, [searchQuery]);
+
useEffect(() => {
+ if (debouncedValue === searchQuery) return;
setSearchParams(
(prevParams) => {
+ const nextParams = new URLSearchParams(prevParams);
if (debouncedValue) {
- prevParams.set("search", debouncedValue);
+ nextParams.set("search", debouncedValue);
} else {
- prevParams.delete("search");
+ nextParams.delete("search");
}
- prevParams.set("page", 1);
- return prevParams;
+ nextParams.set("page", "1");
+ return nextParams;
},
{ replace: true },
);
- }, [debouncedValue, setSearchParams]);
+ }, [debouncedValue, searchQuery, setSearchParams]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/pages/dashboard/History.jsx` around lines 67 - 83, The issue is
that the useEffect hook unconditionally resets the page parameter to 1 whenever
debouncedValue changes, which destroys deep links like `?page=3` on initial
load. Additionally, inputValue can drift from the URL search parameter when
navigation occurs. To fix this, modify the setSearchParams logic to only reset
page to 1 when the search query actually transitions from empty to non-empty or
vice versa (comparing previous and current debouncedValue states), rather than
on every change. Also ensure inputValue stays synchronized with the searchQuery
from the URL by initializing it correctly and adding logic to update it when
searchQuery from the URL parameters changes.
Comment on lines
+42
to
+56
| const logoutMutation = useMutation({ | ||
| mutationFn: async () => { | ||
| const response = await fetch(`${getApiBaseUrl()}/api/auth/logout`, { | ||
| method: "POST", | ||
| credentials: "include", | ||
| }); | ||
|
|
||
| const result = await response.json(); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(result.message || "Logout failed"); | ||
| } | ||
|
|
||
| return result; | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check response status before parsing JSON.
Parsing response.json() at line 49 before checking response.ok at line 51 can cause issues. If the server returns a non-OK status with non-JSON content (e.g., a 500 error with an HTML page), response.json() will throw a parsing error, bypassing your custom error message logic and producing cryptic messages like "Unexpected token '<'".
🛡️ Proposed fix to check status first
const logoutMutation = useMutation({
mutationFn: async () => {
const response = await fetch(`${getApiBaseUrl()}/api/auth/logout`, {
method: "POST",
credentials: "include",
});
- const result = await response.json();
-
if (!response.ok) {
+ let message = "Logout failed";
+ try {
+ const result = await response.json();
+ message = result.message || message;
+ } catch {
+ // Ignore JSON parse errors for non-JSON responses
+ }
- throw new Error(result.message || "Logout failed");
+ throw new Error(message);
}
- return result;
+ return await response.json();
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const logoutMutation = useMutation({ | |
| mutationFn: async () => { | |
| const response = await fetch(`${getApiBaseUrl()}/api/auth/logout`, { | |
| method: "POST", | |
| credentials: "include", | |
| }); | |
| const result = await response.json(); | |
| if (!response.ok) { | |
| throw new Error(result.message || "Logout failed"); | |
| } | |
| return result; | |
| }, | |
| const logoutMutation = useMutation({ | |
| mutationFn: async () => { | |
| const response = await fetch(`${getApiBaseUrl()}/api/auth/logout`, { | |
| method: "POST", | |
| credentials: "include", | |
| }); | |
| if (!response.ok) { | |
| let message = "Logout failed"; | |
| try { | |
| const result = await response.json(); | |
| message = result.message || message; | |
| } catch { | |
| // Ignore JSON parse errors for non-JSON responses | |
| } | |
| throw new Error(message); | |
| } | |
| return await response.json(); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/pages/dashboard/Settings.jsx` around lines 42 - 56, In the
logoutMutation's mutationFn, the response status must be checked before
attempting to parse the JSON body. Move the response.ok check before the
response.json() call so that if the server returns a non-OK status with non-JSON
content (like an HTML error page), the parsing error is avoided and your custom
error message logic executes properly instead.
Comment on lines
16
to
22
| const handlePageChange = (newPage) => { | ||
| // Update the "page" parameter inside our search params object | ||
|
|
||
| searchParams.set("page", newPage); | ||
|
|
||
| // Push the updated parameters back into the browser's address bar | ||
|
|
||
| setSearchParams(searchParams); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid mutating searchParams before passing it to setSearchParams.
The current implementation mutates the searchParams object via .set() and then passes the same reference to setSearchParams. This violates React's state immutability principle and can cause issues with change detection and batched updates.
✅ Recommended fix using functional update or new URLSearchParams
Option 1 (preferred): Functional update
const handlePageChange = (newPage) => {
- searchParams.set("page", newPage);
- setSearchParams(searchParams);
+ setSearchParams((prev) => {
+ const next = new URLSearchParams(prev);
+ next.set("page", newPage);
+ return next;
+ });
};Option 2: Create new URLSearchParams
const handlePageChange = (newPage) => {
- searchParams.set("page", newPage);
- setSearchParams(searchParams);
+ const nextParams = new URLSearchParams(searchParams);
+ nextParams.set("page", newPage);
+ setSearchParams(nextParams);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handlePageChange = (newPage) => { | |
| // Update the "page" parameter inside our search params object | |
| searchParams.set("page", newPage); | |
| // Push the updated parameters back into the browser's address bar | |
| setSearchParams(searchParams); | |
| }; | |
| const handlePageChange = (newPage) => { | |
| setSearchParams((prev) => { | |
| const next = new URLSearchParams(prev); | |
| next.set("page", newPage); | |
| return next; | |
| }); | |
| }; |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 20-20: Avoid using the initial state variable in setState
Context: setSearchParams(searchParams)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/pages/dashboard/Subscriptions.jsx` around lines 16 - 22, The
handlePageChange function is mutating the searchParams object directly by
calling set() on it before passing it to setSearchParams, which violates React's
immutability principle. Instead, create a new URLSearchParams instance from the
existing searchParams, set the "page" value on the new instance, and then pass
this new instance to setSearchParams. This ensures the original state object is
not mutated and allows React to properly detect the state change.
Source: Linters/SAST tools
Comment on lines
+61
to
+63
| <p className="rounded-xl border border-red-100 bg-red-50 px-4 py-3 text-sm text-subly-danger mt-7"> | ||
| {error.message || "Unable to load history."} | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Incorrect error message text.
The error message says "Unable to load history." but this is the Subscriptions page. It should say "Unable to load subscriptions."
📝 Proposed fix
<p className="rounded-xl border border-red-100 bg-red-50 px-4 py-3 text-sm text-subly-danger mt-7">
- {error.message || "Unable to load history."}
+ {error.message || "Unable to load subscriptions."}
</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <p className="rounded-xl border border-red-100 bg-red-50 px-4 py-3 text-sm text-subly-danger mt-7"> | |
| {error.message || "Unable to load history."} | |
| </p> | |
| <p className="rounded-xl border border-red-100 bg-red-50 px-4 py-3 text-sm text-subly-danger mt-7"> | |
| {error.message || "Unable to load subscriptions."} | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/pages/dashboard/Subscriptions.jsx` around lines 61 - 63, The error
message fallback text in the Subscriptions page is incorrect. In the error
display block where it shows error.message || "Unable to load history.", change
the fallback string from "Unable to load history." to "Unable to load
subscriptions." to match the actual context of the Subscriptions page.
Comment on lines
70
to
76
| <div className="mt-2"> | ||
| <PaginationList currentPage={data?.pagination?.currentPage} totalPages={data?.pagination?.totalPages} onPageChange={handlePageChange}/> | ||
| <PaginationList | ||
| currentPage={data?.pagination?.currentPage} | ||
| totalPages={data?.pagination?.totalPages} | ||
| onPageChange={handlePageChange} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
PaginationList renders with undefined data during error state.
When isError is true, data is undefined, causing PaginationList to receive undefined for both currentPage and totalPages. This can lead to unexpected UI behavior. The pagination should only render when data is successfully loaded.
🛡️ Proposed fix to conditionally render pagination
)}
- <div className="mt-2">
- <PaginationList
- currentPage={data?.pagination?.currentPage}
- totalPages={data?.pagination?.totalPages}
- onPageChange={handlePageChange}
- />
- </div>
+ {!isLoading && !isError && data && (
+ <div className="mt-2">
+ <PaginationList
+ currentPage={data.pagination.currentPage}
+ totalPages={data.pagination.totalPages}
+ onPageChange={handlePageChange}
+ />
+ </div>
+ )}
</section>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div className="mt-2"> | |
| <PaginationList currentPage={data?.pagination?.currentPage} totalPages={data?.pagination?.totalPages} onPageChange={handlePageChange}/> | |
| <PaginationList | |
| currentPage={data?.pagination?.currentPage} | |
| totalPages={data?.pagination?.totalPages} | |
| onPageChange={handlePageChange} | |
| /> | |
| </div> | |
| {!isLoading && !isError && data && ( | |
| <div className="mt-2"> | |
| <PaginationList | |
| currentPage={data.pagination.currentPage} | |
| totalPages={data.pagination.totalPages} | |
| onPageChange={handlePageChange} | |
| /> | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/pages/dashboard/Subscriptions.jsx` around lines 70 - 76, The
PaginationList component is rendering even when data is undefined during error
states, which causes both currentPage and totalPages to be undefined. Add a
conditional check before rendering the PaginationList div to ensure it only
renders when data is successfully loaded and there are no errors. Check both the
isError state and that data exists with valid pagination properties before
rendering the entire pagination section containing the PaginationList component.
Comment on lines
+35
to
+38
| const allowedOrigins = new Set([ | ||
| "http://localhost:5173", | ||
| "http://127.0.0.1:5173", | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not always trust localhost origins in production CORS policy.
Line 35-38 whitelists localhost origins for every environment. For credentialed endpoints, this weakens production origin restrictions and should be limited to non-production only.
🔐 Proposed fix
-const allowedOrigins = new Set([
- "http://localhost:5173",
- "http://127.0.0.1:5173",
-
- ...configuredOrigins,
-]);
+const localDevOrigins =
+ env.NODE_ENV === "production"
+ ? []
+ : ["http://localhost:5173", "http://127.0.0.1:5173"];
+
+const allowedOrigins = new Set([...localDevOrigins, ...configuredOrigins]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const allowedOrigins = new Set([ | |
| "http://localhost:5173", | |
| "http://127.0.0.1:5173", | |
| const localDevOrigins = | |
| env.NODE_ENV === "production" | |
| ? [] | |
| : ["http://localhost:5173", "http://127.0.0.1:5173"]; | |
| const allowedOrigins = new Set([...localDevOrigins, ...configuredOrigins]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/server.js` around lines 35 - 38, The allowedOrigins Set is
unconditionally whitelisting localhost origins for all environments, including
production. Modify the allowedOrigins initialization to only include localhost
origins when the environment is not production, such as by checking
process.env.NODE_ENV or a similar environment variable. Localhost origins like
http://localhost:5173 and http://127.0.0.1:5173 should be added conditionally
within the Set constructor or immediately after, only when running in
development or non-production environments.
Comment on lines
+17
to
+79
| const search = req.query.search || req.query.query; | ||
| const page = Math.max(parseInt(req.query.page) || 1, 1); | ||
| const limit = Math.min(Math.max(parseInt(req.query.limit) || 10, 1), 100); | ||
| const userId = req.user.id; | ||
|
|
||
| const whereClause = { | ||
| subscription: { | ||
| userId, | ||
| }, | ||
| }; | ||
|
|
||
| const normalizedType = type ? String(type).toUpperCase() : null; | ||
| const normalizedCategory = category ? String(category).toUpperCase() : null; | ||
|
|
||
| if (normalizedType && normalizedType !== "ALL") { | ||
|
|
||
| if (!HISTORY_TYPES.has(normalizedType)) { | ||
| return res.status(400).json({ message: "Invalid history type" }); | ||
| } | ||
|
|
||
| whereClause.type = normalizedType; | ||
| } | ||
|
|
||
| if (normalizedCategory && normalizedCategory !== "ALL") { | ||
| if (!HISTORY_CATEGORIES.has(normalizedCategory)) { | ||
| return res.status(400).json({ message: "Invalid history category" }); | ||
| } | ||
|
|
||
| whereClause.category = normalizedCategory; | ||
| } | ||
|
|
||
| if (range && range !== "ALL") { | ||
| const normalizedRange = String(range).toLowerCase().replace(/[\s_-]/g, ""); | ||
| const dateThreshold = new Date(); | ||
|
|
||
| if (["7d", "last7d", "7days", "last7days"].includes(normalizedRange)) { | ||
| dateThreshold.setDate(dateThreshold.getDate() - 7); | ||
| whereClause.paidAt = { gte: dateThreshold }; | ||
| } | ||
|
|
||
| if ( | ||
| ["30d", "last30d", "30days", "last30days"].includes(normalizedRange) | ||
| ) { | ||
| dateThreshold.setDate(dateThreshold.getDate() - 30); | ||
| whereClause.paidAt = { gte: dateThreshold }; | ||
| } | ||
|
|
||
| if ( | ||
| ["12m", "last12m", "12months", "last12months"].includes( | ||
| normalizedRange, | ||
| ) | ||
| ) { | ||
| dateThreshold.setMonth(dateThreshold.getMonth() - 12); | ||
| whereClause.paidAt = { gte: dateThreshold }; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| try { | ||
| const { query, category, type, range } = req.query | ||
| const page = parseInt(req.query.page) || 1 | ||
| const userId = req.user.id; | ||
|
|
||
| const ITEMS_PER_PAGE = 10 | ||
|
|
||
| const whereClause = { userId } | ||
|
|
||
| if (type && type !== "ALL") { | ||
| whereClause.type = type | ||
| } | ||
|
|
||
| if (category && category !== "ALL") { | ||
| whereClause.category = category | ||
| } | ||
| if (range) { | ||
| const dateThreshold = new Date(); | ||
|
|
||
| if (range === "7days") { | ||
| dateThreshold.setDate(dateThreshold.getDate() - 7); | ||
| } else if (range === "30days") { | ||
| dateThreshold.setDate(dateThreshold.getDate() - 30); | ||
| } else if (range === "12months") { | ||
| dateThreshold.setMonth(dateThreshold.getMonth() - 12); | ||
| } | ||
|
|
||
| // Add the date boundary condition to our where clause | ||
| whereClause.createdAt = { gte: dateThreshold }; | ||
| } | ||
|
|
||
| if (query && query.trim() !== '') { | ||
| whereClause.OR = [ | ||
| { | ||
| subscriptionName: { | ||
| contains: query, | ||
| mode: "insensitive", | ||
| }, | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| const [historyLogs, totalRecords] = await prisma.$transaction([ | ||
|
|
||
| prisma.history.findMany({ | ||
| where: whereClause, | ||
| orderBy: { createdAt: "desc" }, | ||
| skip: (page - 1) * ITEMS_PER_PAGE, | ||
| take: ITEMS_PER_PAGE, | ||
| }), | ||
|
|
||
| prisma.history.count({ | ||
| where: whereClause, | ||
| }), | ||
| ]); | ||
|
|
||
|
|
||
| const totalPages = Math.ceil(totalRecords / ITEMS_PER_PAGE); | ||
|
|
||
|
|
||
| return res.status(200).json({ | ||
| history: historyLogs, | ||
| meta: { | ||
| currentPage: page, | ||
| totalPages: totalPages || 1, | ||
| totalRecords, | ||
| }, | ||
| }) | ||
|
|
||
| } catch (err) { | ||
| next(err) | ||
| if (search && search.trim() !== "") { | ||
| whereClause.OR = [ | ||
| { | ||
| subscriptionName: { | ||
| contains: search.trim(), | ||
| mode: "insensitive", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
fd -t f "history.controller.js" --search-path serverRepository: Remi561/Subly
Length of output: 196
🏁 Script executed:
cat -n server/src/controllers/history.controller.js | head -100Repository: Remi561/Subly
Length of output: 3629
🌐 Web query:
Express req.query array values multiple parameters same name
💡 Result:
In Express, when a query string contains multiple parameters with the same name (e.g.,?id=1&id=2&id=3), the default query parser automatically groups these values into an array assigned to that key on the req.query object (e.g., req.query.id becomes ['1', '2', '3']) [1][2][3]. Important considerations regarding this behavior include: Configuration and Parsing: The parsing of req.query is determined by the query parser application setting [4]. By default, Express uses the qs module, which performs this automatic grouping [4][2]. You can change this behavior by setting the parser to 'simple' (which parses values as strings and prevents object/array creation) or by providing a custom parsing function if specific requirements exist [4][2][5]. Version-Specific Behavior (Limits): In some versions of Express (specifically 4.22.0 through 4.22.1), an update to the underlying qs dependency introduced an arrayLimit of 20 [6][7]. In those versions, if a query parameter appeared more than 20 times, the values were silently collapsed into a plain object with numeric keys instead of an array [6][7][8]. This was identified as a bug and subsequently patched in version 4.22.2, which restored the expected array behavior for repeated keys [6]. Security: Because req.query is based on user-controlled input, it is untrusted [4]. Always validate and sanitize values before using them, as a malicious actor could pass an object or array where a single string is expected [4][2]. If you find that your application is receiving an object instead of an array for repeated keys, ensure you are using an updated version of Express (4.22.2 or later) or verify that no custom query parser settings are overriding the default behavior [9][6].
Citations:
- 1: https://stackoverflow.com/questions/62589108/multiple-query-parameters-with-same-name
- 2: https://masteringjs.io/tutorials/express/query-parameters
- 3: https://requestly.com/blog/express-get-query-params/
- 4: https://expressjs.com/en/5x/api/request/
- 5: https://evanhahn.com/gotchas-with-express-query-parsing-and-how-to-avoid-them/
- 6: req.query will produce object instead of array when it contains more than 20 values in v4.22.X expressjs/express#7147
- 7: fix: restore array parsing for req.query repeated keys (#7147) expressjs/express#7181
- 8: fix: req.query returns object instead of array for >20 repeated values expressjs/express#7193
- 9: https://stackoverflow.com/questions/64946377/express-multiple-query-parameters-with-same-name-does-not-return-array
Guard search type before calling .trim() to prevent runtime 500 errors.
Line 74 calls .trim() assuming search is a string. Query parameters can be arrays when repeated in the URL (e.g., ?search=a&search=b becomes ['a', 'b']), causing a TypeError that crashes the endpoint. This is user-controlled input and should be validated.
Suggested fix
- const search = req.query.search || req.query.query;
+ const rawSearch = req.query.search ?? req.query.query;
+ const search =
+ typeof rawSearch === "string"
+ ? rawSearch
+ : Array.isArray(rawSearch) && typeof rawSearch[0] === "string"
+ ? rawSearch[0]
+ : "";
@@
- if (search && search.trim() !== "") {
+ const normalizedSearch = search.trim();
+ if (normalizedSearch !== "") {
whereClause.OR = [
{
subscriptionName: {
- contains: search.trim(),
+ contains: normalizedSearch,
mode: "insensitive",
},
},
];
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/controllers/history.controller.js` around lines 17 - 79, The code
at the search parameter validation block calls `.trim()` on the search variable
without verifying it is a string first. Query parameters can be arrays when
repeated in the URL (e.g., ?search=a&search=b becomes ['a', 'b']), which causes
a TypeError when calling .trim() on an array. Add a type check using typeof
search === 'string' before calling .trim() in the condition that checks if
search is truthy to ensure only string values are processed and prevent runtime
500 errors from user-controlled input.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements