feat: pollDetail #45
Open
feat: pollDetail #45
Conversation
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
WalkthroughAdds a voting timeline chart and average voting speed to PollDetail. Computes cumulative votes over time and average seconds-to-vote via useMemo, handles missing timestamps, conditionally renders a BarChart with Tooltip, and displays placeholders when no votes exist. No exported API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant PD as PollDetail (UI)
participant HM as useMemo (calculations)
participant RC as Recharts (BarChart/Tooltip)
U->>PD: Open poll details
PD->>HM: Compute voteTimeline (sorted cumulative)
PD->>HM: Compute averageVoteTime (sec since createdAt)
alt Votes exist
HM-->>PD: voteTimeline[], averageVoteTime
PD->>RC: Render BarChart + Tooltip
note right of RC: Bars show cumulative votes over time
PD->>U: Show "Voting Timeline" and "Voting Speed"
else No votes or missing timestamps
HM-->>PD: [] / null
PD-->>U: Display "No votes yet"
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (3)
web/src/components/pollDetail.tsx (3)
11-11: Prefer consistent chart wrappers + responsive sizing (avoid raw Recharts Tooltip).To keep styling consistent with the “Vote Distribution” chart and avoid hard‑coded sizes, wrap the timeline chart in ChartContainer + ResponsiveContainer and reuse ChartTooltip/ChartTooltipContent. Also drop the direct Recharts Tooltip import to prevent mixing tooltip systems.
Apply:
-import { Bar, BarChart, CartesianGrid, LabelList, Tooltip, XAxis, YAxis } from 'recharts'; +import { Bar, BarChart, CartesianGrid, LabelList, ResponsiveContainer, XAxis, YAxis } from 'recharts';
561-584: Make the timeline responsive and reuse ChartTooltip for consistent UX.Hard-coded width/height will break on smaller viewports, and using Recharts Tooltip diverges from the rest of the UI. Wrap with ChartContainer + ResponsiveContainer and reuse ChartTooltip/ChartTooltipContent.
- {voteTimeline.length > 0 ? ( - <BarChart - data={voteTimeline} - margin={{ left: 120, right: 16 }} - width={670} - height={250} - > - <CartesianGrid strokeDasharray="3 3" /> - <XAxis dataKey="time" /> - <YAxis allowDecimals={false} /> - <Tooltip /> - <Bar dataKey="cumulativeVotes" fill="var(--color-desktop)" radius={[4, 4, 0, 0]} /> - </BarChart> - ) : ( + {voteTimeline.length > 0 ? ( + <ChartContainer config={chartConfig}> + <ResponsiveContainer width="100%" height={250}> + <BarChart data={voteTimeline} margin={{ left: 16, right: 16 }}> + <CartesianGrid strokeDasharray="3 3" /> + <XAxis dataKey="time" /> + <YAxis allowDecimals={false} /> + <ChartTooltip cursor={false} content={<ChartTooltipContent indicator="line" />} /> + <Bar dataKey="cumulativeVotes" fill="var(--color-desktop)" radius={[4, 4, 0, 0]} /> + </BarChart> + </ResponsiveContainer> + </ChartContainer> + ) : (
585-598: Truthiness check hides 0s; use explicit null check.If the average is exactly 0 seconds, the UI shows “No votes yet”. Compare against null instead.
- {averageVoteTime ? ( + {averageVoteTime !== null ? ( <p> Average time to vote: <strong>{Math.round(averageVoteTime)} seconds</strong> </p> ) : (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/components/pollDetail.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
web/src/components/pollDetail.tsx (2)
functions/src/services/pollService.ts (1)
vote(10-71)web/src/components/pollList.tsx (1)
poll(108-142)
Comment on lines
+257
to
+274
| const voteTimeline = useMemo(() => { | ||
| if (!poll?.votes || poll.votes.length === 0) return []; | ||
|
|
||
| const sortedVotes = [...poll.votes].sort((a, b) => { | ||
| const at = a.timestamp ? new Date(a.timestamp).getTime() : 0; | ||
| const bt = b.timestamp ? new Date(b.timestamp).getTime() : 0; | ||
| return at - bt; | ||
| }); | ||
|
|
||
| const timeline: { time: string; cumulativeVotes: number }[] = []; | ||
| sortedVotes.forEach((vote, index) => { | ||
| const time = vote.timestamp ? new Date(vote.timestamp).toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' }) : 'Unknown'; | ||
| timeline.push({ time, cumulativeVotes: index + 1 }); | ||
| }); | ||
|
|
||
| return timeline; | ||
| }, [poll?.votes]); | ||
|
|
There was a problem hiding this comment.
Defensive timestamp handling: current sort can return NaN → unstable ordering.
new Date(vote.timestamp) will be Invalid Date when timestamp is a Firestore Timestamp or malformed string. Subtracting NaN leads to a comparator that returns NaN, which makes Array.sort unstable and timeline rendering nondeterministic.
Patch the timeline to safely coerce timestamps and sort, falling back to the end for unknowns:
- const voteTimeline = useMemo(() => {
- if (!poll?.votes || poll.votes.length === 0) return [];
-
- const sortedVotes = [...poll.votes].sort((a, b) => {
- const at = a.timestamp ? new Date(a.timestamp).getTime() : 0;
- const bt = b.timestamp ? new Date(b.timestamp).getTime() : 0;
- return at - bt;
- });
-
- const timeline: { time: string; cumulativeVotes: number }[] = [];
- sortedVotes.forEach((vote, index) => {
- const time = vote.timestamp ? new Date(vote.timestamp).toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' }) : 'Unknown';
- timeline.push({ time, cumulativeVotes: index + 1 });
- });
-
- return timeline;
- }, [poll?.votes]);
+ const voteTimeline = useMemo(() => {
+ if (!poll?.votes || poll.votes.length === 0) return [];
+
+ const toMs = (ts: unknown): number | null => {
+ if (!ts) return null;
+ if (typeof (ts as any)?.toDate === 'function') return (ts as any).toDate().getTime();
+ const d = ts instanceof Date ? ts : new Date(ts as any);
+ const ms = d.getTime();
+ return Number.isNaN(ms) ? null : ms;
+ };
+
+ const sorted = [...poll.votes]
+ .map((v) => ({ ms: toMs((v as any).timestamp) }))
+ .sort((a, b) => (a.ms ?? Number.POSITIVE_INFINITY) - (b.ms ?? Number.POSITIVE_INFINITY));
+
+ const timeline: { time: string; cumulativeVotes: number }[] = [];
+ sorted.forEach((item, index) => {
+ const time = item.ms !== null
+ ? new Date(item.ms).toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' })
+ : 'Unknown';
+ timeline.push({ time, cumulativeVotes: index + 1 });
+ });
+ return timeline;
+ }, [poll?.votes]);Additionally fix upstream normalization done in the snapshot (outside this hunk) to stop asserting to string and instead coerce to ms/Date:
- const normalizedVotes = (data.votes ?? []).map((vote: BaseVote) => {
- const ts = (vote?.timestamp ?? vote?.timestamp) as string | undefined;
- return { ...vote, timestamp: ts };
- });
+ const normalizedVotes = (data.votes ?? []).map((vote: BaseVote & { timestamp?: unknown }) => {
+ const raw = vote?.timestamp;
+ let ts: number | undefined;
+ if (raw && typeof (raw as any)?.toDate === 'function') {
+ ts = (raw as any).toDate().getTime();
+ } else if (raw instanceof Date) {
+ ts = raw.getTime();
+ } else if (raw != null) {
+ const d = new Date(raw as any);
+ ts = Number.isNaN(d.getTime()) ? undefined : d.getTime();
+ }
+ return { ...vote, timestamp: ts };
+ });And update the local cast used in getVotersForOption so TS matches the new shape (outside this hunk):
- const voters = (poll.votes as (BaseVote & { timestamp?: string })[])
+ const voters = (poll.votes as (BaseVote & { timestamp?: number | Date | string })[])📝 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 voteTimeline = useMemo(() => { | |
| if (!poll?.votes || poll.votes.length === 0) return []; | |
| const sortedVotes = [...poll.votes].sort((a, b) => { | |
| const at = a.timestamp ? new Date(a.timestamp).getTime() : 0; | |
| const bt = b.timestamp ? new Date(b.timestamp).getTime() : 0; | |
| return at - bt; | |
| }); | |
| const timeline: { time: string; cumulativeVotes: number }[] = []; | |
| sortedVotes.forEach((vote, index) => { | |
| const time = vote.timestamp ? new Date(vote.timestamp).toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' }) : 'Unknown'; | |
| timeline.push({ time, cumulativeVotes: index + 1 }); | |
| }); | |
| return timeline; | |
| }, [poll?.votes]); | |
| const voteTimeline = useMemo(() => { | |
| if (!poll?.votes || poll.votes.length === 0) return []; | |
| const toMs = (ts: unknown): number | null => { | |
| if (!ts) return null; | |
| if (typeof (ts as any)?.toDate === 'function') return (ts as any).toDate().getTime(); | |
| const d = ts instanceof Date ? ts : new Date(ts as any); | |
| const ms = d.getTime(); | |
| return Number.isNaN(ms) ? null : ms; | |
| }; | |
| const sorted = [...poll.votes] | |
| .map((v) => ({ ms: toMs((v as any).timestamp) })) | |
| .sort((a, b) => (a.ms ?? Number.POSITIVE_INFINITY) - (b.ms ?? Number.POSITIVE_INFINITY)); | |
| const timeline: { time: string; cumulativeVotes: number }[] = []; | |
| sorted.forEach((item, index) => { | |
| const time = item.ms !== null | |
| ? new Date(item.ms).toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' }) | |
| : 'Unknown'; | |
| timeline.push({ time, cumulativeVotes: index + 1 }); | |
| }); | |
| return timeline; | |
| }, [poll?.votes]); |
🤖 Prompt for AI Agents
In web/src/components/pollDetail.tsx around lines 257-274, the comparator uses
new Date(vote.timestamp).getTime() which can yield NaN for Firestore Timestamps
or malformed values causing unstable sort; coerce timestamps to a numeric ms
value safely (e.g., if timestamp?.toMillis is a function call it, else try
Number(timestamp) / Date.parse/new Date(timestamp).getTime()), treat invalid
values as Infinity so unknown-times sort to the end, sort by these numeric ms
values, and use the numeric ms to build the display time (formatting with new
Date(ms) only when ms is finite, otherwise label as 'Unknown'); also update
upstream snapshot normalization to produce ms (not asserted string) and adjust
the local cast in getVotersForOption to match the new timestamp shape so
TypeScript types align.
Comment on lines
+275
to
284
| const averageVoteTime = useMemo(() => { | ||
| if (!poll?.votes || poll.votes.length === 0 || !poll.createdAt) return null; | ||
| const startTime = new Date(poll.createdAt).getTime(); | ||
| const totalDiff = poll.votes.reduce((sum, vote) => { | ||
| const voteTime = vote.timestamp ? new Date(vote.timestamp).getTime() : startTime; | ||
| return sum + (voteTime - startTime); | ||
| }, 0); | ||
| return totalDiff / poll.votes.length / 1000; | ||
| }, [poll?.votes, poll?.createdAt]); | ||
|
|
There was a problem hiding this comment.
Average time calc can be Invalid Date (Firestore Timestamp) and can go negative.
new Date(poll.createdAt) will be invalid if createdAt is a Firestore Timestamp; same for vote.timestamp. Result: NaN arithmetic and “NaN seconds”. Also guard against clock skews producing negative diffs.
Apply:
- const averageVoteTime = useMemo(() => {
- if (!poll?.votes || poll.votes.length === 0 || !poll.createdAt) return null;
- const startTime = new Date(poll.createdAt).getTime();
- const totalDiff = poll.votes.reduce((sum, vote) => {
- const voteTime = vote.timestamp ? new Date(vote.timestamp).getTime() : startTime;
- return sum + (voteTime - startTime);
- }, 0);
- return totalDiff / poll.votes.length / 1000;
- }, [poll?.votes, poll?.createdAt]);
+ const averageVoteTime = useMemo(() => {
+ if (!poll?.votes || poll.votes.length === 0 || !poll.createdAt) return null;
+ const toMs = (ts: unknown): number | null => {
+ if (!ts) return null;
+ if (typeof (ts as any)?.toDate === 'function') return (ts as any).toDate().getTime();
+ const d = ts instanceof Date ? ts : new Date(ts as any);
+ const ms = d.getTime();
+ return Number.isNaN(ms) ? null : ms;
+ };
+ const startMs = toMs(poll.createdAt);
+ if (startMs === null) return null;
+ const totalDiff = poll.votes.reduce((sum, vote) => {
+ const voteMs = toMs((vote as any).timestamp) ?? startMs;
+ return sum + Math.max(0, voteMs - startMs);
+ }, 0);
+ return totalDiff / poll.votes.length / 1000;
+ }, [poll?.votes, poll?.createdAt]);📝 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 averageVoteTime = useMemo(() => { | |
| if (!poll?.votes || poll.votes.length === 0 || !poll.createdAt) return null; | |
| const startTime = new Date(poll.createdAt).getTime(); | |
| const totalDiff = poll.votes.reduce((sum, vote) => { | |
| const voteTime = vote.timestamp ? new Date(vote.timestamp).getTime() : startTime; | |
| return sum + (voteTime - startTime); | |
| }, 0); | |
| return totalDiff / poll.votes.length / 1000; | |
| }, [poll?.votes, poll?.createdAt]); | |
| const averageVoteTime = useMemo(() => { | |
| if (!poll?.votes || poll.votes.length === 0 || !poll.createdAt) return null; | |
| const toMs = (ts: unknown): number | null => { | |
| if (!ts) return null; | |
| if (typeof (ts as any)?.toDate === 'function') return (ts as any).toDate().getTime(); | |
| const d = ts instanceof Date ? ts : new Date(ts as any); | |
| const ms = d.getTime(); | |
| return Number.isNaN(ms) ? null : ms; | |
| }; | |
| const startMs = toMs(poll.createdAt); | |
| if (startMs === null) return null; | |
| const totalDiff = poll.votes.reduce((sum, vote) => { | |
| const voteMs = toMs((vote as any).timestamp) ?? startMs; | |
| return sum + Math.max(0, voteMs - startMs); | |
| }, 0); | |
| return totalDiff / poll.votes.length / 1000; | |
| }, [poll?.votes, poll?.createdAt]); |
🤖 Prompt for AI Agents
In web/src/components/pollDetail.tsx around lines 275 to 284, the average vote
time computation uses new Date(...) on values that may be Firestore Timestamps
(causing Invalid Date/NaN) and does not guard against clock skew producing
negative diffs; update the logic to normalize timestamps safely: if
poll.createdAt or vote.timestamp are Firestore Timestamp objects call
.toDate().getTime() (or convert seconds*1000) and otherwise coerce
numbers/strings to milliseconds and validate with Number.isFinite; skip or treat
votes with invalid timestamps as occurring at startTime; when computing diff
clamp negative differences to 0 before summing; finally ensure you still divide
by the count of valid votes (or poll.votes.length if you choose to include
invalid as zero) and keep the same useMemo dependencies.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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