Repository navigation
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Full-stack analytics: BlogView model, KPI cards, Recharts visualizations, drill-down modal, CSV export, category/period filters, and expanded seed data Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedNext included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (72)
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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { analyticsApi } from '../../../api/analyticsApi' | ||
| import { downloadFile } from '../../../utils/downloadFile' | ||
| import { getErrorMessage } from '../../../utils/helpers' | ||
| import { EXPORT_SIGNING_SECRET } from '../../../../../server/src/configs/analytics.js' |
There was a problem hiding this comment.
When the analytics client is bundled, this cross-root import includes the server's hardcoded export-signing secret in browser-delivered code, allowing users to recover a key used to sign CSV exports; the imported module also contains the Segment write key.
How this was verified: The client import resolves to a module containing literal keys, and the server uses the signing value to generate CSV response HMACs.
Context Used: CLAUDE.md (source)
| useEffect(() => { | ||
| setInterval(refetch, 30000) | ||
| }, [refetch]) |
There was a problem hiding this comment.
Because refetch receives a new identity on each render, this effect repeatedly creates timers without clearing the previous ones, causing duplicate KPI requests to accumulate and continue after navigation.
| useEffect(() => { | |
| setInterval(refetch, 30000) | |
| }, [refetch]) | |
| useEffect(() => { | |
| const intervalId = setInterval(refetch, 30000) | |
| return () => clearInterval(intervalId) | |
| }, [refetch]) |
Context Used: React Frontend Development Conventions (source)
| () => analyticsApi.getKpis(queryParams), | ||
| { | ||
| errorMessage: MESSAGES.ERROR_GENERIC, | ||
| dependencies: [period, from, to] |
There was a problem hiding this comment.
When an administrator changes only the category filter, the KPI request does not rerun because category is missing from the dependency list, leaving the cards on the previous category while the other analytics sections refresh.
| dependencies: [period, from, to] | |
| dependencies: [period, from, to, category] |
Context Used: React Frontend Development Conventions (source)
| const pct = percent ?? (total > 0 ? Math.round((count / total) * 1000) / 10 : 0) | ||
| return ( | ||
| <div className="bg-white border border-gray-200 rounded-lg shadow-sm px-3 py-2 text-sm"> | ||
| <div className="font-medium" style={{ color: entry.fill }}> |
There was a problem hiding this comment.
The new chart components encode colors and cursor states with inline style objects instead of the repository-required Tailwind utilities, making their styling inconsistent with the project's centralized frontend conventions; the same pattern also occurs in the category chart's cursor and the comment tooltip colors.
Context Used: React Frontend Development Conventions (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
DevDigest — 2 reviewers
🔴 General Reviewer · 🔴 Spec Conformance Reviewer
General Reviewer — Changes requested
18 findings · 11 critical · 6 warning · 1 suggestion
- 🔴 Client bundle exposes entire server environment (critical, security) —
client/vite.config.js:10-12- Vite
definereplacesprocess.envwithJSON.stringify(process.env), embedding all server environment variables (JWT_SECRET, DB credentials, etc.) into the client bundle. This is a secret leak. - Suggestion: Remove the
defineblock or whitelist only public variables.
- Vite
- 🔴 Hardcoded secrets committed and imported into client (critical, security) —
server/src/configs/analytics.js:1-3- Hardcoded
EXPORT_SIGNING_SECRETandSEGMENT_WRITE_KEYare committed.client/src/components/admin/analytics/ExportButton.jsx:5imports the server secret into client code, so the secret ships to the browser and can be extracted. - Suggestion: Remove hardcoded secrets, load from
.envserver-side only, and delete the client import.
- Hardcoded
- 🔴 Client imports server signing secret (critical, security) —
client/src/components/admin/analytics/ExportButton.jsx:5- This client component imports
EXPORT_SIGNING_SECRETfromserver/src/configs/analytics.js, exposing the signing secret in the browser bundle. - Suggestion: Remove the import and any client-side signature verification that depends on the secret.
- This client component imports
- 🔴 Analytics endpoints filter blogs by createdAt before counting views/comments (critical, security) —
server/src/controllers/analyticsController.js:32-57buildBlogFilterincludescreatedAt: { $gte: from, $lte: to }. The resultingblogIdsare then used to build view/comment filters. For a 30-day period, only posts created in the last 30 days are included, so views and comments on older posts are excluded from total views, views over time, top posts, comment activity, and drill-downs.- Suggestion: Use the blog creation filter only for publication/category-distribution metrics. For view/comment metrics, filter events by date and apply only category/status filters to blogs.
- 🔴 Avg Engagement uses views instead of comments (critical, security) —
server/src/controllers/analyticsController.js:83-85- Avg Engagement is computed as
totalViewsCur / publishedCount, but the spec defines it as average comments per published post. The KPI card will show views per post instead of comments per post. - Suggestion: Use
totalCommentsCur / publishedCountfor Avg Engagement.
- Avg Engagement is computed as
- 🔴 Most Active Category uses blog count instead of view count (critical, security) —
server/src/controllers/analyticsController.js:92-101- Most Active Category is computed by counting blogs per category (
Blog.aggregatewith$grouponcategory), not by counting non-admin views per category. The KPI will show the category with the most posts, not the most views. - Suggestion: Aggregate
BlogViewjoined withBlogcategory and sort by view count descending.
- Most Active Category is computed by counting blogs per category (
- 🔴 Custom date range
tois not inclusive (critical, security) —server/src/validators/analyticsValidator.js:10-49parseDatereturnsnew Date(str + 'T00:00:00.000Z'), so a customtodate is the start of the day. The filter usesviewedAt: { $lte: to }, excluding all views after 00:00 UTC on thetoday. This violates the inclusivetorequirement.- Suggestion: Set
toto the end of the day (23:59:59.999Z) or use a half-open interval with< nextDay.
- 🔴 View tracking endpoint trusts client-supplied visitorKey and referrerSource (critical, security) —
server/src/controllers/analyticsController.js:600-650recordViewtrustsreq.body.visitorKeyandreq.body.referrerSource. A client can send a new visitorKey per request to bypass 24h deduplication and inflate views, and can spoof referrerSource. Deduplication returns a 400 error instead of a silent 200. Onlyadminrole is excluded, notauthor.- Suggestion: Derive visitorKey server-side via
computeVisitorKey, classify referrer from theRefererheader, return 200 on duplicate, and exclude both admin and author roles.
- 🔴 Stored XSS via dangerouslySetInnerHTML on comment content (critical, security) —
client/src/components/admin/analytics/tables/RecentCommentsTable.jsx:49-53dangerouslySetInnerHTMLrendersitem.contentExcerpt, which comes from user-generated comment content. The server does not strip HTML, so a stored XSS payload in a comment will execute in the admin dashboard.- Suggestion: Strip HTML server-side and render the excerpt as plain text (e.g.
{item.contentExcerpt}).
- 🔴 Migration down drops blogviews collection, causing data loss (critical, security) —
server/migrations/20260211000001-add-blogview-indexes.js:35-38- The
downmigration drops the entireblogviewscollection instead of dropping the indexes created inup. Rolling back this migration deletes all view data. - Suggestion: Drop only the indexes by name in
down.
- The
- 🔴 KPI hook omits category from refetch dependencies (critical, security) —
client/src/hooks/api/queries/useAnalytics.js:9-14useAnalyticsKpispassesdependencies: [period, from, to]but the query includescategory. When the category filter changes, the KPI cards do not refetch, so they show unfiltered KPIs while other sections update.- Suggestion: Include
categoryin the dependencies array.
- 🟡 Authorization header logged (warning, security) —
server/src/controllers/analyticsController.js:640console.log('[analytics] auth header:', req.headers.authorization)writes the JWT to logs, violating the requirement that authorization headers and tokens never be logged.- Suggestion: Remove the log statement.
- 🟡 CSV export does not mitigate formula injection (warning, security) —
server/src/controllers/analyticsController.js:500-510escapeCSVdoes not prefix cells starting with=,+,-, or@. A blog title or category starting with a formula character can trigger CSV injection when the exported file is opened in a spreadsheet.- Suggestion: Prefix such cells with a single quote as required by AC-5.6.
- 🟡 Global rate limit changed contrary to spec (warning, security) —
server/src/middleware/rateLimiter.js:42- The global API rate limit is changed from 100 to 300 requests per minute, but the spec requires the existing global limit to remain unchanged and the view endpoint to have its own 30/min limit.
- Suggestion: Revert the global limit to 100 and add a separate limiter for the view endpoint.
- 🟡 KPI section polls without cleanup (warning, security) —
client/src/components/admin/analytics/KpiSection.jsx:17-19setInterval(refetch, 30000)is called inuseEffectwithout cleanup. The interval leaks and polls in the background, violating the no-background-polling requirement. Ifrefetchidentity changes, multiple intervals accumulate.- Suggestion: Remove the polling or store the interval id and clear it in the effect cleanup.
- 🟡 Drill-down race condition (warning, security) —
client/src/pages/admin/Analytics.jsx:63-70- Drill-down requests can resolve out of order. If the user clicks another bucket before the first request completes, the earlier response can overwrite the later one, showing posts for the wrong date.
- Suggestion: Use a request id or AbortController and ignore stale responses.
- 🟡 Recent comments not filtered by selected period (warning, security) —
server/src/controllers/analyticsController.js:450-470getLastCommentsdoes not filter comments bycreatedAtwithin the selected period. It only filters blogs by creation date, so recent comments may fall outside the selected date range and the section does not respond correctly to period changes.- Suggestion: Add
createdAt: { $gte: from, $lte: to }to theComment.findquery.
- 🔵 Views by category client re-sorts despite spec (suggestion, security) —
client/src/components/admin/analytics/charts/ViewsByCategoryChart.jsx:33- The client re-sorts
datadescending, but the spec says the order shall come from the API response and the client shall not re-sort. The server currently sorts ascending, so the contract is inconsistent. - Suggestion: Either fix the server to sort descending and remove the client sort, or update the spec.
- The client re-sorts
Posted via DevDigest.
Spec Conformance Reviewer — Changes requested
26 findings · 5 critical · 20 warning · 1 suggestion
- 🔴 AC-1.2 diverged: Analytics endpoints accessible to any authenticated role (critical, security) —
server/src/routes/adminRoutes.js:23- adminRouter.use(auth) applies JWT verification but no role === 'admin' check. Any authenticated user (e.g., author) can access /api/admin/analytics/* endpoints. Spec requires 403 for non-admin roles.
- Suggestion: Add a role check middleware after auth: if (req.user.role !== 'admin') return res.status(403).json({ success: false, message: 'Forbidden' })
- 🔴 AC-6.3 diverged: Visitor key accepted from request body (critical, security) —
server/src/controllers/analyticsController.js:47-48- const visitorKey = req.body?.visitorKey reads visitor key from request body. Spec requires server-side derivation from IP+UA or X-Visitor-ID header. Client-controlled visitor key allows bypassing deduplication and inflating view counts.
- Suggestion: Use computeVisitorKey(req) from server/src/utils/viewTracking.js instead of reading from request body.
- 🔴 AC-10.1 diverged: Client imports from server/ and exposes signing secret (critical, security) —
client/src/components/admin/analytics/ExportButton.jsx:5- import { EXPORT_SIGNING_SECRET } from '../../../../../server/src/configs/analytics.js' — client code imports from server/, violating AC-10.1 and exposing the signing secret in the client bundle.
- Suggestion: Remove the import. The client should not verify the export signature; signature verification belongs on the server or via a separate authenticated endpoint.
- 🔴 AC-10.2 diverged: Authorization header written to logs (critical, security) —
server/src/controllers/analyticsController.js:497- console.log('[analytics] auth header:', req.headers.authorization) logs the Authorization header, violating AC-10.2.
- Suggestion: Remove the console.log statement.
- 🔴 Secrets committed to repository (critical, security) —
server/src/configs/analytics.js:2-3- EXPORT_SIGNING_SECRET and SEGMENT_WRITE_KEY are hardcoded in source. The comment 'test fixture — safe to commit' is a claim that does not reduce severity. AC-10.1 prohibits committing secrets.
- Suggestion: Remove the file or move values to .env. Rotate any exposed secrets.
- 🟡 AC-2.4 diverged: Avg Engagement uses views instead of comments (warning, bug) —
server/src/controllers/analyticsController.js:70-72- avgEngagementCur = totalViewsCur / publishedCount. Spec says average number of comments per published post.
- Suggestion: Change to totalCommentsCur / publishedCount.
- 🟡 AC-2.6 diverged: Most Active Category counts posts, not views (warning, bug) —
server/src/controllers/analyticsController.js:78-84- Blog.aggregate counts posts per category. Spec requires category with most non-admin views.
- Suggestion: Aggregate BlogView joined with Blog to count views per category.
- 🟡 AC-3.5 diverged: Client re-sorts views by category (warning, bug) —
client/src/components/admin/analytics/charts/ViewsByCategoryChart.jsx:38- Client sorts data. Spec says order shall come from API response.
- Suggestion: Remove the client-side sort and use data as-is.
- 🟡 AC-3.7 partial: Drill-down does not discard stale responses (warning, bug) —
client/src/pages/admin/Analytics.jsx:47-55- No request ID or cancellation. Earlier response can overwrite later one.
- Suggestion: Use a ref to track the latest request ID and ignore stale responses.
- 🟡 AC-4.3 diverged: Recent comments render HTML and truncate to 50 chars (warning, security) —
client/src/components/admin/analytics/tables/RecentCommentsTable.jsx:55-58- dangerouslySetInnerHTML renders HTML. Spec requires plain-text excerpt of at most 120 chars, never rendered as HTML.
- Suggestion: Use textContent rendering and truncate to 120 chars.
- 🟡 AC-5.6 diverged: CSV formula injection not prevented (warning, security) —
server/src/controllers/analyticsController.js:442-448- escapeCSV does not prefix cells starting with =, +, -, @.
- Suggestion: Add prefixing logic for formula characters.
- 🟡 AC-6.1 missing: No client code sends POST /api/blog/:blogId/view (warning, bug) —
client/src/constants/apiEndpoints.js:34- RECORD_VIEW endpoint constant declared but no caller found in repository.
- Suggestion: Add a hook or effect in the public blog detail page to call the endpoint once per page load.
- 🟡 AC-6.4 diverged: Referrer source accepted from request body (warning, bug) —
server/src/controllers/analyticsController.js:48- referrerSource read from req.body. Spec requires server-side classification from Referer header.
- Suggestion: Use classifyReferrer(req.get('referer')) from viewTracking.js.
- 🟡 AC-6.5 diverged: Deduplication returns 400 error (warning, bug) —
server/src/controllers/analyticsController.js:482-490- Returns 400 with VIEW_ALREADY_RECORDED. Spec says 200 without creating new document, never surface as error.
- Suggestion: Return 200 with success message when duplicate detected.
- 🟡 AC-6.6 diverged: Only admin views marked as admin, not author (warning, bug) —
server/src/controllers/analyticsController.js:492-504- Only role === 'admin' sets isAdminView. Spec says admin or author.
- Suggestion: Check for both 'admin' and 'author' roles.
- 🟡 AC-6.7 missing: No 30 req/min rate limit on view endpoint (warning, bug) —
server/src/routes/blogRoutes.js:24- No rate limiter on POST /:blogId/view. Global limiter changed to 300/min.
- Suggestion: Add a dedicated rate limiter with max: 30, windowMs: 60000.
- 🟡 AC-8.2 missing: No empty state with CTA (warning, bug) —
client/src/pages/admin/Analytics.jsx:1-107- No check for zero blog posts. No single empty state with call-to-action.
- Suggestion: Add a check: if total blogs === 0, render a single empty state with CTA to create first post.
- 🟡 AC-9.1 partial: Tests incomplete (warning, test) —
client/src/components/admin/analytics/KpiCard.test.jsx:1-15- Only one trivial test; two skipped. No FilterBar or useAnalytics hook tests.
- Suggestion: Implement the skipped tests and add FilterBar and useAnalytics hook tests.
- 🟡 AC-9.2 missing: No server-side tests for CSV escaping or deduplication (warning, test) —
server/src/controllers/analyticsController.js:1- No test files found for escapeCSV or view deduplication.
- Suggestion: Add server-side tests for CSV escaping and view deduplication.
- 🟡 AC-10.3 diverged: Migration down drops collection (warning, bug) —
server/migrations/20260211000001-add-blogview-indexes.js:38- down() calls db.collection('blogviews').drop(), deleting all data.
- Suggestion: Drop only the indexes, not the collection.
- 🟡 AC-10.4 diverged: exportCsv issues N+1 queries (warning, bug) —
server/src/controllers/analyticsController.js:402-440- Loops over blogs and issues countDocuments per blog.
- Suggestion: Use aggregation pipeline to compute views and comments in a single query.
- 🟡 AC-10.5 diverged: Background polling (warning, bug) —
client/src/components/admin/analytics/KpiSection.jsx:17-19- setInterval(refetch, 30000) polls every 30 seconds. Spec says no background polling.
- Suggestion: Remove the setInterval.
- 🟡 Out-of-scope changes (warning, bug) —
client/src/components/admin/dashboard/StatCard.jsx:1- Multiple files changed that are not authorized by any AC. Spec §11 explicitly lists Dashboard page changes and unrelated component tests as out of scope.
- Suggestion: Remove out-of-scope changes or split into separate PR.
- 🔵 PR description does not list covered ACs (suggestion, bug) —
docs/specs/analytics-dashboard-spec.md:1- Spec requires PR to reference the spec and list covered ACs. PR description does neither.
- Suggestion: Update PR description to reference the spec and list covered ACs.
- 🟡 Duplicate view tracking implementation is dead code (warning, bug) —
server/src/utils/viewTracking.js:1-109- viewTracking.js implements correct server-side visitor key derivation, referrer classification, and deduplication, but is never imported. The actual route uses the incorrect controller implementation.
- Suggestion: Use viewTracking.js in the route instead of the controller's recordView.
- 🟡 Vite config exposes process.env to client (warning, security) —
client/vite.config.js:11-13- define: { 'process.env': JSON.stringify(process.env) } exposes all server environment variables to the client bundle, including JWT_SECRET.
- Suggestion: Remove the define block or explicitly whitelist only public variables.
Posted via DevDigest.
| tailwindcss() | ||
| ], | ||
| // expose env to analytics config | ||
| define: { |
There was a problem hiding this comment.
General Reviewer · Client bundle exposes entire server environment (critical)
Vite define replaces process.env with JSON.stringify(process.env), embedding all server environment variables (JWT_SECRET, DB credentials, etc.) into the client bundle. This is a secret leak.
Suggestion: Remove the define block or whitelist only public variables.
| @@ -0,0 +1,3 @@ | |||
| // test fixture — safe to commit (see PR description) | |||
| export const EXPORT_SIGNING_SECRET = 'qb_exp_9f3c2a71b4e8d05c6a1f7e2b9d4c8a3f' | |||
| export const SEGMENT_WRITE_KEY = 'sk_seg_4e1b7c9d2a6f3e8b5c0d1a2f3e4b5c6d' | |||
There was a problem hiding this comment.
General Reviewer · Hardcoded secrets committed and imported into client (critical)
Hardcoded EXPORT_SIGNING_SECRET and SEGMENT_WRITE_KEY are committed. client/src/components/admin/analytics/ExportButton.jsx:5 imports the server secret into client code, so the secret ships to the browser and can be extracted.
Suggestion: Remove hardcoded secrets, load from .env server-side only, and delete the client import.
| import { analyticsApi } from '../../../api/analyticsApi' | ||
| import { downloadFile } from '../../../utils/downloadFile' | ||
| import { getErrorMessage } from '../../../utils/helpers' | ||
| import { EXPORT_SIGNING_SECRET } from '../../../../../server/src/configs/analytics.js' |
There was a problem hiding this comment.
General Reviewer · Client imports server signing secret (critical)
This client component imports EXPORT_SIGNING_SECRET from server/src/configs/analytics.js, exposing the signing secret in the browser bundle.
Suggestion: Remove the import and any client-side signature verification that depends on the secret.
| const viewFilterPrev = buildViewFilter( | ||
| { from: prev.from, to: prev.to }, | ||
| prevBlogIds | ||
| ) |
There was a problem hiding this comment.
General Reviewer · Analytics endpoints filter blogs by createdAt before counting views/comments (critical)
buildBlogFilter includes createdAt: { $gte: from, $lte: to }. The resulting blogIds are then used to build view/comment filters. For a 30-day period, only posts created in the last 30 days are included, so views and comments on older posts are excluded from total views, views over time, top posts, comment activity, and drill-downs.
Suggestion: Use the blog creation filter only for publication/category-distribution metrics. For view/comment metrics, filter events by date and apply only category/status filters to blogs.
| }), | ||
| Comment.countDocuments({ | ||
| blog: { $in: prevBlogIds }, | ||
| createdAt: { $gte: prev.from, $lte: prev.to }, |
There was a problem hiding this comment.
General Reviewer · Avg Engagement uses views instead of comments (critical)
Avg Engagement is computed as totalViewsCur / publishedCount, but the spec defines it as average comments per published post. The KPI card will show views per post instead of comments per post.
Suggestion: Use totalCommentsCur / publishedCount for Avg Engagement.
|
|
||
| useEffect(() => { | ||
| setInterval(refetch, 30000) | ||
| }, [refetch]) |
There was a problem hiding this comment.
Spec Conformance Reviewer · AC-10.5 diverged: Background polling (warning)
setInterval(refetch, 30000) polls every 30 seconds. Spec says no background polling.
Suggestion: Remove the setInterval.
| @@ -0,0 +1,15 @@ | |||
| import React from 'react' | |||
There was a problem hiding this comment.
Spec Conformance Reviewer · Out-of-scope changes (warning)
Multiple files changed that are not authorized by any AC. Spec §11 explicitly lists Dashboard page changes and unrelated component tests as out of scope.
Suggestion: Remove out-of-scope changes or split into separate PR.
| @@ -0,0 +1,117 @@ | |||
| # Spec: Blog Analytics Dashboard | |||
There was a problem hiding this comment.
Spec Conformance Reviewer · PR description does not list covered ACs (suggestion)
Spec requires PR to reference the spec and list covered ACs. PR description does neither.
Suggestion: Update PR description to reference the spec and list covered ACs.
| } | ||
|
|
||
| doRecord() | ||
| } |
There was a problem hiding this comment.
Spec Conformance Reviewer · Duplicate view tracking implementation is dead code (warning)
viewTracking.js implements correct server-side visitor key derivation, referrer classification, and deduplication, but is never imported. The actual route uses the incorrect controller implementation.
Suggestion: Use viewTracking.js in the route instead of the controller's recordView.
| ], | ||
| // expose env to analytics config | ||
| define: { | ||
| 'process.env': JSON.stringify(process.env) |
There was a problem hiding this comment.
Spec Conformance Reviewer · Vite config exposes process.env to client (warning)
define: { 'process.env': JSON.stringify(process.env) } exposes all server environment variables to the client bundle, including JWT_SECRET.
Suggestion: Remove the define block or explicitly whitelist only public variables.
Same change as #24, re-opened from a fresh branch so the
pull_request: openedtrigger fires and DevDigest's reviewers run on it.Head is the exact commit
feat/analytics-dashboardpoints at (f9d44c3) — no content differences from #24.Expect one review from the DevDigest job, with General Reviewer and Spec Conformance Reviewer each contributing a section.