Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
WalkthroughThis pull request updates the default dashboard time period from 7 days to 30 days across multiple components and introduces metric visibility toggling functionality to the email chart. The Possibly related PRs
Suggested labels
🚥 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. 📝 Coding Plan
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 |
Deploying usesend with
|
| Latest commit: |
9dd0ead
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a7409502.usesend.pages.dev |
| Branch Preview URL: | https://better-ui.usesend.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/(dashboard)/dashboard/page.tsx (1)
10-28:⚠️ Potential issue | 🟠 MajorNormalize URL
daysbefore numeric conversion.On Line 26 and Line 28,
Number(days ?? "30")can passNaNor unsupported values whendaysis manually set in the URL (e.g.,?days=foo,?days=90). Normalize once to"7" | "30"and reuse that value across filters and charts.Suggested fix
export default function Dashboard() { const [days, setDays] = useUrlState("days", "30"); const [domain, setDomain] = useUrlState("domain"); + const normalizedDays = days === "7" || days === "30" ? days : "30"; + const numericDays = Number(normalizedDays); return ( <div> <div className="w-full"> <div className="flex justify-between items-center mb-10"> <H1>Analytics</H1> <DashboardFilters - days={days ?? "30"} + days={normalizedDays} setDays={setDays} domain={domain} setDomain={setDomain} /> </div> <div className=" space-y-12"> - <EmailChart days={Number(days ?? "30")} domain={domain} /> + <EmailChart days={numericDays} domain={domain} /> - <ReputationMetrics days={Number(days ?? "30")} domain={domain} /> + <ReputationMetrics days={numericDays} domain={domain} /> </div> </div> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/dashboard/page.tsx around lines 10 - 28, Normalize the URL-derived days value once before numeric conversion and pass the normalized value into filters and charts: validate the `days` from `useUrlState("days", "30")`, coerce it to either "7" or "30" (default to "30" for invalid values), update `setDays` usage accordingly, and replace all occurrences of `Number(days ?? "30")` (used when rendering `DashboardFilters`, `EmailChart`, and `ReputationMetrics`) with the single normalized value (and numeric conversion only where the components expect a number) so charts never receive NaN or unsupported values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/server/service/dashboard-service.ts`:
- Around line 12-15: The date-range logic currently produces one extra day
because it both sets startDate to "now - days" and then fills an inclusive
range; fix by making the days selection explicit and adjusting the start offset:
set days = input.days === 7 ? 7 : 30 (instead of input.days !== 7 ? 30 : 7) and
compute startDate by subtracting days - 1 (startDate.setDate(startDate.getDate()
- (days - 1))) so the inclusive fill loop (the code that iterates from startDate
to today to produce points) yields exactly `days` points; also review the fill
loop that iterates from startDate to endDate to ensure it treats both ends
consistently with this change.
---
Outside diff comments:
In `@apps/web/src/app/`(dashboard)/dashboard/page.tsx:
- Around line 10-28: Normalize the URL-derived days value once before numeric
conversion and pass the normalized value into filters and charts: validate the
`days` from `useUrlState("days", "30")`, coerce it to either "7" or "30"
(default to "30" for invalid values), update `setDays` usage accordingly, and
replace all occurrences of `Number(days ?? "30")` (used when rendering
`DashboardFilters`, `EmailChart`, and `ReputationMetrics`) with the single
normalized value (and numeric conversion only where the components expect a
number) so charts never receive NaN or unsupported values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 421e6142-7115-4569-8b94-7b83d3c6c4c1
📒 Files selected for processing (5)
apps/web/src/app/(dashboard)/dashboard/dashboard-filters.tsxapps/web/src/app/(dashboard)/dashboard/email-chart.tsxapps/web/src/app/(dashboard)/dashboard/page.tsxapps/web/src/server/service/dashboard-service.tspackages/ui/styles/globals.css
| const days = input.days !== 7 ? 30 : 7; | ||
| const { domain, team } = input; | ||
| const startDate = new Date(); | ||
| startDate.setDate(startDate.getDate() - days); |
There was a problem hiding this comment.
Fix off-by-one range calculation for 7/30 day windows.
Line 12 currently drives an inclusive range that returns 8 points for 7 and 31 points for 30 (startDate includes the start day, and the fill loop also includes both ends). This breaks the stated 7/30-day toggle semantics.
Proposed fix
export async function emailTimeSeries(input: EmailTimeSeries) {
- const days = input.days !== 7 ? 30 : 7;
+ const days = input.days === 7 ? 7 : 30;
const { domain, team } = input;
const startDate = new Date();
- startDate.setDate(startDate.getDate() - days);
+ startDate.setDate(startDate.getDate() - (days - 1));- for (let i = days; i > -1; i--) {
+ for (let i = days - 1; i >= 0; i--) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/server/service/dashboard-service.ts` around lines 12 - 15, The
date-range logic currently produces one extra day because it both sets startDate
to "now - days" and then fills an inclusive range; fix by making the days
selection explicit and adjusting the start offset: set days = input.days === 7 ?
7 : 30 (instead of input.days !== 7 ? 30 : 7) and compute startDate by
subtracting days - 1 (startDate.setDate(startDate.getDate() - (days - 1))) so
the inclusive fill loop (the code that iterates from startDate to today to
produce points) yields exactly `days` points; also review the fill loop that
iterates from startDate to endDate to ensure it treats both ends consistently
with this change.
1 participant
Summary
Verification
Summary by cubic
Make the dashboard email chart interactive and easier to read. Metric chips now toggle visible series (total is display-only), and the analytics default range is 30 days.
Written for commit 9dd0ead. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Style
Chores