40 improve poll detail #44
Conversation
|
Warning Rate limit exceeded@Roubean1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a required Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as Client (PollDetail)
participant BE as Backend (voteHandler)
participant DB as Firestore
U->>C: Click "Vote" (optionId)
C->>BE: handleVoteAction(pollId, optionId, user)
BE->>BE: Build Vote { userId, optionId, timestamp = ISO }
BE->>DB: Append vote to polls/{pollId}
DB-->>C: onSnapshot emits updated poll document
C->>C: Normalize votes (parse vote.timestamp → Date)
C->>C: Recompute voteCounts, chartData, totalVotes
C->>U: Render updated counts, chart, voter lists, relative times
sequenceDiagram
autonumber
participant C as PollDetail Component
participant DB as Firestore
actor U as User
C->>DB: onSnapshot(doc(polls/{pollId}))
DB-->>C: Poll data (options, votes with timestamp)
C->>C: useMemo -> voteCounts, chartData, totalVotes
U->>C: Search / toggle expand / change sort
C->>C: Filter/highlight voters, expand matching options
Note over C: Timer every 10s updates relative time labels
C->>U: Render avatars, voter timestamps, badges, chart
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/src/handlers/voteHandler.ts (1)
79-88: Prevent voting when option is missing/deletedAfter notifying the user, execution continues and may still record a vote. Early-return here.
if (!option || option.deleted) { log.warn(`Option not found or deleted: ${optionId}`); await client.chat.postEphemeral({ channel: body.channel?.id ?? poll.channelId, user: userId, text: 'This option is no longer available for voting.', }); + return; }
🧹 Nitpick comments (6)
web/src/types/poll.tsx (1)
27-31: Casing consistency (nit)Consider
timestamprather thantimeStampfor consistency. Only if feasible repo-wide.functions/src/types/poll.ts (1)
26-30: LGTM: Optional timestamp on VoteOptional ISO 8601 string works well for gradual rollout. Consider documenting the format in a JSDoc for clarity.
web/src/components/pollDetail.tsx (4)
9-10: Use AvatarFallback for robustnessShow initials when image isn’t available.
-import { Avatar, AvatarImage } from '@/components/ui/avatar'; +import { Avatar, AvatarImage, AvatarFallback } from '@/components/ui/avatar';
401-404: Add avatar fallback contentImproves UX without user images.
- <Avatar className="h-8 w-8"> - <AvatarImage src={profile_placeholder} alt="Default profile" /> - </Avatar> + <Avatar className="h-8 w-8"> + <AvatarImage src={profile_placeholder} alt="Default profile" /> + <AvatarFallback>{voter.name?.[0] ?? '?'}</AvatarFallback> + </Avatar>
249-251: Created-at rendering may break for Firestore Timestamp
new Date(poll.createdAt)fails ifcreatedAtis a FirestoreTimestamp. Use a small helper to normalize.Example helper (place near top):
function toDate(value: unknown): Date | undefined { if (!value) return undefined; if (value instanceof Date) return value; if (typeof value === 'string' || typeof value === 'number') return new Date(value); if (typeof (value as any)?.toDate === 'function') return (value as any).toDate(); return undefined; }Then:
- {poll.createdAt ? new Date(poll.createdAt).toLocaleString() : 'Unknown'} + {(() => { const d = toDate(poll.createdAt); return d ? d.toLocaleString() : 'Unknown'; })()}
461-463: Fix Tailwind arbitrary value syntax
fill-(--color-label)isn’t valid. Usefill-[var(--color-label)].- className="fill-(--color-label)" + className="fill-[var(--color-label)]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonweb/src/assets/Profile_avatar_placeholder_large.pngis excluded by!**/*.png
📒 Files selected for processing (7)
functions/src/handlers/voteHandler.ts(1 hunks)functions/src/types/poll.ts(1 hunks)web/package.json(1 hunks)web/src/App.tsx(1 hunks)web/src/components/pollDetail.tsx(3 hunks)web/src/components/ui/avatar.tsx(1 hunks)web/src/types/poll.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
web/src/components/ui/avatar.tsx (1)
web/src/lib/utils.ts (1)
cn(4-6)
functions/src/handlers/voteHandler.ts (1)
functions/src/tests/voteHandler.test.ts (1)
pollId(80-133)
web/src/components/pollDetail.tsx (3)
functions/src/types/poll.ts (1)
Poll(3-15)web/src/types/poll.tsx (1)
Poll(3-16)functions/src/services/pollService.ts (1)
vote(10-71)
🔇 Additional comments (3)
web/package.json (1)
13-13: LGTM: Radix Avatar dependency addedDependency addition is appropriate and scoped.
web/src/App.tsx (1)
2-2: Named export switch for PollDetail confirmed
No default imports of PollDetail remain in the codebase.web/src/components/ui/avatar.tsx (1)
1-49: LGTM: Accessible, typed avatar wrappersForwardRef usage, class merging, and displayName preservation are solid.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/src/components/pollDetail.tsx (3)
19-24: Make voter timestamp optional to avoid misrepresenting legacy votesKeep timestamps undefined when missing and handle them in UI/sorts.
interface VoterInfo { userId: string; name: string; - timestamp: Date; + timestamp?: Date; highlighted?: boolean; }
54-57: Don’t default missing vote.timeStamp to “now”; normalize safely (Firestore Timestamp/ISO/epoch)Defaulting to current time skews chronology. Preserve undefined and coerce robustly.
- const normalizedVotes = (data.votes ?? []).map((vote: BaseVote) => ({ - ...vote, - timestamp: vote.timeStamp ? new Date(vote.timeStamp) : new Date(), - })); + const normalizedVotes = (data.votes ?? []).map((vote: BaseVote) => { + const ts: any = (vote as any).timeStamp; + const timestamp = + ts + ? typeof ts?.toDate === 'function' + ? ts.toDate() + : new Date(ts) + : undefined; + return { ...vote, timestamp }; + });
194-203: Guard sorting when timestamps are optionalPrevent runtime errors and sort missing timestamps as oldest.
- if (sortOrder === 'timestamp') { - return b.timestamp.getTime() - a.timestamp.getTime(); // nejnovější první - } else { - return a.name.localeCompare(b.name, 'cs', { sensitivity: 'base' }); // abecedně - } + if (sortOrder === 'timestamp') { + const aTime = a.timestamp ? a.timestamp.getTime() : Number.NEGATIVE_INFINITY; + const bTime = b.timestamp ? b.timestamp.getTime() : Number.NEGATIVE_INFINITY; + return bTime - aTime; // newest first + } + return a.name.localeCompare(b.name, undefined, { sensitivity: 'base' });
🧹 Nitpick comments (5)
web/src/components/pollDetail.tsx (5)
185-192: Type the normalized vote shape instead of casting; allow optional timestampAvoid repeated casts and make intent explicit.
- const voters = (poll.votes as (BaseVote & { timestamp: Date })[]) + type NormalizedVote = BaseVote & { timestamp?: Date }; + const voters = (poll.votes as NormalizedVote[]) .filter((vote) => vote.optionId === optionId) .map((vote) => ({ userId: vote.userId, name: users[vote.userId] || 'Unknown User', - timestamp: vote.timestamp, + timestamp: vote.timestamp, highlighted: highlightedVoters.has(vote.userId), }));
482-488: Fix invalid Tailwind class for label color
fill-(--color-label)is not a valid Tailwind arbitrary value; it won’t apply.- <LabelList + <LabelList dataKey="optionId" position="insideLeft" offset={8} - className="fill-(--color-label)" + className="fill-[var(--color-label)]" fontSize={12} />
414-417: Prefer stable keys; drop index from keyUser can’t vote twice for the same option;
userIdis sufficient and avoids reorder artifacts.- key={`${voter.userId}-${index}`} + key={voter.userId}
50-86: Optional: handle realtime listener errorsAdd an error callback to avoid silent failures and to clear loading on errors.
- const unsubscribe = onSnapshot(doc(db, 'polls', pollId), (docSnap) => { + const unsubscribe = onSnapshot( + doc(db, 'polls', pollId), + (docSnap) => { // existing success handler... - }); + }, + (err) => { + console.error('Realtime poll listener failed', err); + setLoading(false); + } + );
201-202: Use default locale in localeCompareHardcoding 'cs' may surprise non-Czech users; default respects user/browser locale.
- return a.name.localeCompare(b.name, 'cs', { sensitivity: 'base' }); // abecedně + return a.name.localeCompare(b.name, undefined, { sensitivity: 'base' });
📜 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 (3)
functions/src/types/poll.ts (2)
Poll(3-15)PollOption(19-24)web/src/types/poll.tsx (1)
Poll(3-16)functions/src/services/pollService.ts (1)
vote(10-71)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
functions/src/tests/voteHandler.test.ts (4)
114-114: Make expectation resilient; also validate timestamp parseabilityUse objectContaining to avoid brittleness if the vote payload gains fields. Optionally assert the timestamp is parseable/ISO-like.
-expect(mockVote).toHaveBeenCalledWith(pollId, { userId, optionId, timeStamp: expect.any(String) }); +expect(mockVote).toHaveBeenCalledWith( + pollId, + expect.objectContaining({ userId, optionId, timeStamp: expect.any(String) }) +);You can additionally check format/parseability:
const payload = mockVote.mock.calls[0][1]; expect(Date.parse(payload.timeStamp)).not.toBeNaN();
183-183: Same here: prefer objectContaining for forward-compatibilityPrevents test breakage if new fields are added to the vote payload.
-expect(mockVote).toHaveBeenCalledWith(pollId, { userId, optionId, timeStamp: expect.any(String) }); +expect(mockVote).toHaveBeenCalledWith( + pollId, + expect.objectContaining({ userId, optionId, timeStamp: expect.any(String) }) +);
239-239: Apply objectContaining for the rejected-vote case as wellKeeps the assertion robust while still ensuring timeStamp is present.
-expect(mockVote).toHaveBeenCalledWith(pollId, { userId, optionId, timeStamp: expect.any(String) }); +expect(mockVote).toHaveBeenCalledWith( + pollId, + expect.objectContaining({ userId, optionId, timeStamp: expect.any(String) }) +);
293-293: Use objectContaining in the maxVotes path tooConsistent, less fragile expectation across tests.
-expect(mockVote).toHaveBeenCalledWith(pollId, { userId, optionId, timeStamp: expect.any(String) }); +expect(mockVote).toHaveBeenCalledWith( + pollId, + expect.objectContaining({ userId, optionId, timeStamp: expect.any(String) }) +);
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)
functions/src/types/poll.ts (1)
1-31: Action required: add Vote.timestamp to tests/mocks or make Vote.timestamp optional
- Multiple tests/fixtures construct Vote objects without timestamp (will fail type-check now that Vote.timestamp is required).
- Affected files (examples): functions/src/tests/pollService.test.ts, functions/src/tests/pollResult.test.ts, functions/src/tests/userVotesButtonHandler.test.ts, functions/src/tests/voteHandler.test.ts, functions/src/tests/integration/voteHandling.integration.test.ts.
- Fix options: (A) Add a timestamp string to Vote literals (e.g. new Date().toISOString() in factories or timestamp: expect.any(String) in expectations), or (B) change the Vote interface to timestamp?: string if timestamps are optional by design.
♻️ Duplicate comments (3)
web/src/types/poll.tsx (1)
27-31: Maketimestampoptional to reflect legacy data and avoid unsafe castsLegacy votes and some tests don’t include a timestamp. Requiring it forces downstream casts/workarounds and breaks type soundness.
export interface Vote { userId: string; optionId: string; - timestamp: string; + timestamp?: string; // ISO 8601 }functions/src/types/poll.ts (1)
26-30: Don’t maketimestamprequired server-side eitherFirestore may contain votes without this field; marking it required makes
Poll.voteslie about stored shape and can break tests/migrations. Prefer optional here too; new writes should still always set it in handlers.export interface Vote { userId: string; optionId: string; - timestamp: string; + timestamp?: string; // ISO 8601, set by handlers }web/src/components/pollDetail.tsx (1)
185-203: Fix runtime crash: comparator calls.getTime()on possibly-undefined timestamps
Also remove the unsafe cast to(BaseVote & { timestamp: Date })[]and parse toDatelocally.- const voters = (poll.votes as (BaseVote & { timestamp: Date })[]) + const voters = (poll.votes ?? []) .filter((vote) => vote.optionId === optionId) - .map((vote) => ({ + .map((vote) => ({ userId: vote.userId, name: users[vote.userId] || 'Unknown User', - timestamp: vote.timestamp, + timestamp: vote.timestamp ? new Date(vote.timestamp) : undefined, highlighted: highlightedVoters.has(vote.userId), })); voters.sort((a, b) => { if (a.highlighted && !b.highlighted) return -1; if (!a.highlighted && b.highlighted) return 1; if (sortOrder === 'timestamp') { - return b.timestamp.getTime() - a.timestamp.getTime(); // nejnovější první + const aTime = a.timestamp ? a.timestamp.getTime() : -Infinity; + const bTime = b.timestamp ? b.timestamp.getTime() : -Infinity; + return bTime - aTime; // newest first; unknowns last } else { return a.name.localeCompare(b.name, 'cs', { sensitivity: 'base' }); // abecedně } });
🧹 Nitpick comments (2)
web/src/components/pollDetail.tsx (2)
50-86: AddonSnapshoterror handler to surface failures and end loading state
Currently, permission/network errors will leave the UI in “Loading…”.- const unsubscribe = onSnapshot(doc(db, 'polls', pollId), (docSnap) => { + const unsubscribe = onSnapshot( + doc(db, 'polls', pollId), + (docSnap) => { if (docSnap.exists()) { const data = docSnap.data(); // ... } else { setPoll(null); } - setLoading(false); - }); + setLoading(false); + }, + (err) => { + console.error('poll snapshot error', err); + setLoading(false); + } + );
505-511: Tailwind class likely invalid: use CSS var syntax for fill color
fill-(--color-label)won’t be recognized; preferfill-[var(--color-label)](or a theme token).- className="fill-(--color-label)" + className="fill-[var(--color-label)]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
functions/src/handlers/voteHandler.ts(1 hunks)functions/src/tests/voteHandler.test.ts(4 hunks)functions/src/types/poll.ts(1 hunks)web/src/components/pollDetail.tsx(3 hunks)web/src/types/poll.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- functions/src/tests/voteHandler.test.ts
- functions/src/handlers/voteHandler.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T08:27:56.594Z
Learnt from: maprokes
PR: profiq/slack-open-poll#44
File: web/src/types/poll.tsx:27-31
Timestamp: 2025-09-12T08:27:56.594Z
Learning: In the slack-open-poll codebase, timestamp fields should use lowercase naming convention ("timestamp") rather than camelCase ("timeStamp").
Applied to files:
web/src/types/poll.tsx
🧬 Code graph analysis (1)
web/src/components/pollDetail.tsx (4)
functions/src/types/poll.ts (1)
Poll(3-15)web/src/types/poll.tsx (1)
Poll(3-16)functions/src/services/pollService.ts (1)
vote(10-71)web/src/App.tsx (1)
App(8-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Integration Tests functions
🔇 Additional comments (3)
web/src/types/poll.tsx (1)
27-31: Good: lowercase field name matches codebase conventionNaming aligns with the “timestamp” convention from prior feedback.
web/src/components/pollDetail.tsx (2)
159-180: LGTM: robust relative time formatter
Handles Firestore Timestamp and Date safely with sensible fallbacks.
26-26: Resolved — named export verified; no changes needed.
web/src/components/pollDetail.tsx exports PollDetail and imports already use named import (e.g. web/src/App.tsx: import {PollDetail} from "@/components/pollDetail").
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/src/tests/pollService.test.ts (1)
220-231: Flaky toggle: transaction.get returns the same state for both callsSecond call still reads an empty votes array, so service can’t infer a prior vote and may not remove it. Return updated state on the second
get.- const pollWithMultipleVotes = { ...mockPoll, multiple: true, votes: [], maxVotes: 2 }; - mockTransaction.get.mockResolvedValue({ - data: () => pollWithMultipleVotes, - }); + const pollWithMultipleVotes = { ...mockPoll, multiple: true, votes: [], maxVotes: 2 }; + mockTransaction.get + .mockResolvedValueOnce({ data: () => pollWithMultipleVotes }) + .mockResolvedValueOnce({ data: () => ({ ...pollWithMultipleVotes, votes: [mockVote] }) });
🧹 Nitpick comments (4)
functions/src/tests/pollService.test.ts (2)
90-104: Prefer fake timers over spying on Date.prototype; avoid restoreAllMocks hereSpying on Date.prototype is global and
vi.restoreAllMocks()can accidentally undo other mocks. Use fake timers to freeze time locally.- // Mock the Date.toISOString for consistent testing - const mockDate = '2023-01-01T00:00:00.000Z'; - vi.spyOn(global.Date.prototype, 'toISOString').mockReturnValue(mockDate); + // Freeze time for consistent testing + const mockDate = '2023-01-01T00:00:00.000Z'; + vi.useFakeTimers(); + vi.setSystemTime(new Date(mockDate)); @@ - vi.restoreAllMocks(); + vi.useRealTimers();
113-113: Naming consistency: getByIdNit: keep describe block name aligned with the method casing.
- describe('getbyId', () => { + describe('getById', () => {functions/src/tests/userVotesButtonHandler.test.ts (1)
36-38: Use a stable timestamp constant for fixturesMinor: replace repeated
new Date().toISOString()with a fixedconst ts = '2023-01-01T00:00:00.000Z'to reduce noise and improve determinism. No behavior change.functions/src/tests/pollResult.test.ts (1)
30-32: Deduplicate timestamps in votesUse a single constant (or fake timers) for all vote timestamps to keep fixtures concise and stable.
Example (add near the top of the file):
const ts = '2023-01-01T00:00:00.000Z';Then replace
new Date().toISOString()withtsin the shown ranges.Also applies to: 48-51, 72-75, 97-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
functions/src/tests/pollResult.test.ts(4 hunks)functions/src/tests/pollService.test.ts(7 hunks)functions/src/tests/userVotesButtonHandler.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
functions/src/tests/pollService.test.ts (1)
functions/src/types/poll.ts (1)
Vote(26-30)
functions/src/tests/userVotesButtonHandler.test.ts (1)
functions/src/tests/voteHandler.test.ts (1)
pollId(135-198)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
web/src/components/pollDetail.tsx (1)
54-57: Fix legacy timestamp normalization (bug: same key used twice).
vote?.timestamp ?? vote?.timestampnever checks legacytimeStamp. Normalize to lowercasetimestampwhile accepting legacytimeStamp.- 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 | any) => { + const ts = (vote?.timestamp ?? vote?.timeStamp) as string | undefined; + return { ...vote, timestamp: ts }; + });
🧹 Nitpick comments (6)
web/src/components/pollDetail.tsx (6)
159-180: Optional: make getRelativeTime accept string too.This keeps the helper robust if a string slips through in the future.
- const getRelativeTime = (timestamp: Timestamp | Date | undefined): string => { + const getRelativeTime = (timestamp: Timestamp | Date | string | undefined): string => { if (!timestamp) return 'Unknown time'; let date: Date; - if (timestamp instanceof Date) { + if (timestamp instanceof Date) { date = timestamp; - } else { + } else if (typeof timestamp === 'string') { + const d = new Date(timestamp); + if (isNaN(d.getTime())) return 'Unknown time'; + date = d; + } else { date = timestamp.toDate(); }
201-202: Locale hardcoded to 'cs'.Likely unintended; prefer default or a configurable locale.
- return a.name.localeCompare(b.name, 'cs', { sensitivity: 'base' }); // abecedně + return a.name.localeCompare(b.name, undefined, { sensitivity: 'base' });
439-440: Use stable keys (avoid index).Prevents unnecessary re-renders on reorder.
- <div - key={`${voter.userId}-${index}`} + <div + key={voter.userId}
286-301: Tighten badges: hide Max votes for single-choice.Avoids “Max votes: 1” noise when multiple=false.
- <Badge variant="outline">Max votes: {poll.maxVotes ?? '-'}</Badge> + {poll.multiple && ( + <Badge variant="outline">Max votes: {poll.maxVotes ?? '-'}</Badge> + )}
136-145: Optional UX: collapse auto-expanded options when clearing search.Currently, clearing the query keeps prior expansions.
if (!searchQuery.trim()) { - setHighlightedVoters(new Set()); - return; + setHighlightedVoters(new Set()); + setExpandedOptions(new Set()); // collapse + return; }
9-9: Minor: extra space in import.Style nit only.
-import { Avatar, AvatarImage } from '@/components/ui/avatar'; +import { Avatar, AvatarImage } from '@/components/ui/avatar';
📜 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
🧠 Learnings (1)
📚 Learning: 2025-09-12T08:27:56.594Z
Learnt from: maprokes
PR: profiq/slack-open-poll#44
File: web/src/types/poll.tsx:27-31
Timestamp: 2025-09-12T08:27:56.594Z
Learning: In the slack-open-poll codebase, timestamp fields should use lowercase naming convention ("timestamp") rather than camelCase ("timeStamp").
Applied to files:
web/src/components/pollDetail.tsx
🧬 Code graph analysis (1)
web/src/components/pollDetail.tsx (3)
functions/src/types/poll.ts (1)
Poll(3-15)web/src/types/poll.tsx (1)
Poll(3-16)functions/src/services/pollService.ts (1)
vote(10-71)
🔇 Additional comments (1)
web/src/components/pollDetail.tsx (1)
50-74: Sanity-check createdBy/channelId type narrowing — OK.functions/src/types/poll.ts declares createdBy and channelId as string and tests mock them as strings; the runtime ref-or-string narrowing in web/src/components/pollDetail.tsx is defensive and safe — either keep the check or normalize writes to store strings consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web/src/components/pollDetail.tsx (1)
510-516: Invalid Tailwind class for SVG fill; use var() form
fill-(--color-label)won’t compile; usefill-[var(--color-label)].- className="fill-(--color-label)" + className="fill-[var(--color-label)]"
🧹 Nitpick comments (4)
web/src/components/pollDetail.tsx (4)
50-86: Add error handling to onSnapshotSilent failures leave the UI in “Loading…”. Pass an error callback and surface/log it.
- const unsubscribe = onSnapshot(doc(db, 'polls', pollId), (docSnap) => { + const unsubscribe = onSnapshot( + doc(db, 'polls', pollId), + (docSnap) => { // ...existing success path... - } else { + } else { setPoll(null); - } - - setLoading(false); - }); + } + setLoading(false); + }, + (err) => { + console.error('poll onSnapshot error', err); + setLoading(false); + } + );
87-104: Optional: handle errors for users/channels subscriptionsParity with the poll subscription avoids silent failures and aids debugging.
- const unsubscribeUsers = onSnapshot(collection(db, 'users_list'), (querySnapshot) => { + const unsubscribeUsers = onSnapshot( + collection(db, 'users_list'), + (querySnapshot) => { // ... - }); + }, + (err) => console.error('users_list onSnapshot error', err) + ); - const unsubscribeChannels = onSnapshot(collection(db, 'channels_list'), (querySnapshot) => { + const unsubscribeChannels = onSnapshot( + collection(db, 'channels_list'), + (querySnapshot) => { // ... - }); + }, + (err) => console.error('channels_list onSnapshot error', err) + );Also applies to: 96-104
454-456: Add AvatarFallback for robustness (no image/network failures)Improves UX when the placeholder fails or when you later switch to real images.
- <Avatar className="h-8 w-8"> - <AvatarImage src={profile_placeholder} alt="Default profile" /> - </Avatar> + <Avatar className="h-8 w-8"> + <AvatarImage src={profile_placeholder} alt="Default profile" /> + <span className="sr-only">User avatar</span> + </Avatar>Add import:
// at top import { Avatar, AvatarImage } from '@/components/ui/avatar';
444-445: Prefer a more stable keyReduce potential key collisions by incorporating timestamp when available.
- <div - key={`${voter.userId}-${index}`} + <div + key={`${voter.userId}-${voter.timestamp ? voter.timestamp.getTime() : index}`}
📜 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
🧠 Learnings (3)
📚 Learning: 2025-09-12T11:53:22.148Z
Learnt from: Roubean1
PR: profiq/slack-open-poll#44
File: web/src/components/pollDetail.tsx:54-57
Timestamp: 2025-09-12T11:53:22.148Z
Learning: The channelTimeStamp field in the Poll interface is separate from vote timestamps and serves a different purpose (tracking when the poll was created in the channel). Vote timestamps use lowercase "timestamp" while channelTimeStamp uses camelCase, and both are valid for their respective contexts.
Applied to files:
web/src/components/pollDetail.tsx
📚 Learning: 2025-09-12T11:53:22.148Z
Learnt from: Roubean1
PR: profiq/slack-open-poll#44
File: web/src/components/pollDetail.tsx:54-57
Timestamp: 2025-09-12T11:53:22.148Z
Learning: There are two distinct timestamp fields in the slack-open-poll codebase: 1) Vote timestamps use lowercase "timestamp" field to track when individual votes were cast, and 2) Channel timestamps use camelCase "channelTimeStamp" field to store the Slack message timestamp for poll message management. These serve different purposes and both naming conventions are correct for their respective contexts.
Applied to files:
web/src/components/pollDetail.tsx
📚 Learning: 2025-09-12T08:27:56.594Z
Learnt from: maprokes
PR: profiq/slack-open-poll#44
File: web/src/types/poll.tsx:27-31
Timestamp: 2025-09-12T08:27:56.594Z
Learning: In the slack-open-poll codebase, timestamp fields should use lowercase naming convention ("timestamp") rather than camelCase ("timeStamp").
Applied to files:
web/src/components/pollDetail.tsx
🧬 Code graph analysis (1)
web/src/components/pollDetail.tsx (3)
functions/src/types/poll.ts (2)
Poll(3-15)PollOption(19-24)web/src/types/poll.tsx (1)
Poll(3-16)functions/src/services/pollService.ts (1)
vote(10-71)
🔇 Additional comments (4)
web/src/components/pollDetail.tsx (4)
185-195: Good: safe parsing and sorting with optional timestampsConverts to Date with invalid-guard and sorts using -Infinity fallback. Prevents crashes and maintains order.
Also applies to: 197-209
226-233: Good: exclude deleted options from chart dataAligns the visualization with visible options only.
237-255: Good: robust date formatterHandles Firestore Timestamp, Date, ISO/epoch, and invalid inputs consistently.
54-57: Incorrect — no legacytimeStampfound; simplify normalizationBoth Vote definitions (functions/src/types/poll.ts:29 and web/src/types/poll.tsx:30) declare
timestamp(notimeStamp), so(vote?.timestamp ?? vote?.timestamp)is a redundant typo — replace withvote?.timestamp(only add avote?.timeStampfallback if you intentionally expect legacy external payloads).Likely an incorrect or invalid review comment.
Roubean1
force-pushed
the
40-improve-poll-detail
branch
from
ae96d0c to
c4d4c7e
Compare
Summary by CodeRabbit
New Features
UI/UX
Chores
Tests