feat: add channel & user name #41
Conversation
WalkthroughPersist Slack users and channels into new Firestore collections during poll creation, form submit, and voting; add UserService/ChannelService and Firestore.createWithId; enforce per-user maxVotes in PollService.vote; frontend subscribes to users_list and channels_list to resolve display names; read-only Firestore rules added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Slack User
participant S as App Handler
participant P as PollService
participant US as UserService
participant CS as ChannelService
participant F as Firestore
rect rgba(230,245,255,0.6)
note over U,S: Poll creation / form submit
U->>S: submit poll
S->>P: createPoll(...)
P-->>S: pollRef / poll data
S->>F: Save poll + channelTimeStamp
end
alt Persist Slack user (if client.users.info)
S->>S: client.users.info({ user })
S->>US: addUser({id, name})
US->>F: createWithId(id, data)
US-->>S: saved/existing
else Missing users.info
S-->>S: Log skip
end
alt Persist Slack channel (if client.conversations.info)
S->>S: client.conversations.info({ channel })
S->>CS: addChannel({id, name})
CS->>F: createWithId(id, data)
CS-->>S: saved/existing
else Missing conversations.info
S-->>S: Log skip
end
sequenceDiagram
autonumber
participant U as Slack User
participant S as Vote Handler
participant P as PollService
participant US as UserService
participant F as Firestore
U->>S: Vote action
S->>F: Load poll
alt Poll invalid / closed / option missing
S-->>U: Ephemeral notice and return
else Proceed
S->>P: vote(poll, userId, optionId)
note right of P: Enforce per-user maxVotes
P-->>S: updated votes
S->>F: Update poll document
S-->>U: Updated poll UI
opt Post-vote user persistence (if client.users.info)
S->>S: client.users.info({ userId })
S->>US: addUser({ id, name })
US->>F: createWithId(id, data)
US-->>S: done
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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: 3
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-87: Bug: continues after “option is no longer available”; must return.Without returning, an invalid option can still be voted. Fix:
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 (19)
firestore.rules (2)
11-20: Reevaluate allowing full collection listing for users/channels.Current rules allow any authenticated user to list entire
users_listandchannels_list. If that’s intentional, fine; otherwise, prefer:
- Disallow list, allow get per doc to prevent roster scraping.
Example rules:
- allow read: if request.auth != null; + allow get: if request.auth != null; + allow list: if false;Apply similarly to
channels_list. This pairs well with fetching only the specific docs needed in the UI.
1-1: Optional: declare rules version.Add
rules_version = '2';for clarity at the top of the file.functions/src/types/poll.ts (1)
31-39: Consider dropping duplicateidfield from documents.Storing
idinsideUser/Channelduplicates the Firestore document ID. Prefer relying on the doc ID to avoid drift and reduce storage, or define it as derived in code only.Apply if desired:
-export interface User extends BaseDocument { - id: string; - name: string; -} +export interface User extends BaseDocument { + name: string; +} -export interface Channel extends BaseDocument { - id: string; - name: string; -} +export interface Channel extends BaseDocument { + name: string; +}If you keep
idin the schema, ensure server writers always keep it equal to the doc ID.web/src/types/poll.tsx (2)
32-40: Type duplication between web and functions.
User/Channelare defined in both web and functions. If possible, consolidate via a shared types package to prevent drift. If not, add a small comment noting intentional divergence (webPollincludesid, server one doesn’t).
1-1: Minor import style consistency.Drop the file extension in type-only imports for consistency with the codebase (optional):
-import type {BaseDocument} from './baseDocument.tsx'; +import type { BaseDocument } from './baseDocument';web/src/components/pollDetail.tsx (3)
59-75: Avoid subscribing to entire users/channels collections.This scales poorly and exposes full rosters to any authed user. Prefer subscribing only to the specific docs referenced by the poll (createdBy and channelId). Pair with rules that allow get but disallow list.
Example approach (outline):
// after poll is set: const userRef = poll?.createdBy ? doc(db, 'users_list', poll.createdBy) : null; const channelRef = poll?.channelId ? doc(db, 'channels_list', poll.channelId) : null; const unsubUser = userRef ? onSnapshot(userRef, (d) => setUsers({ [d.id]: (d.data()?.name ?? 'Unknown') })) : () => {}; const unsubChannel = channelRef ? onSnapshot(channelRef, (d) => setChannels({ [d.id]: (d.data()?.name ?? 'Unknown Channel') })) : () => {};Also applies to: 68-75
174-180: Tailwind/Recharts label color class is invalid.
className="fill-(--color-label)"won’t resolve. Use an arbitrary value with a CSS var:- className="fill-(--color-label)" + className="fill-[var(--color-label)]"
32-45: EnsurecreatedAtis always a string.
createdAt?.toDate()may produceundefined. Provide a fallback to satisfyBaseDocument.createdAt: stringand avoid runtime Date parsing issues.- createdAt: - typeof data.createdAt === "string" - ? data.createdAt - : data.createdAt?.toDate().toISOString(), + createdAt: + typeof data.createdAt === "string" + ? data.createdAt + : (data.createdAt?.toDate().toISOString() ?? ""),web/src/components/pollList.tsx (2)
39-43: Order polls by createdAt desc for stable UX.- const unsubscribePolls = onSnapshot(collection(db, "polls"), (querySnapshot) => { + const unsubscribePolls = onSnapshot( + query(collection(db, "polls"), orderBy("createdAt", "desc")), + (querySnapshot) => { const data: Poll[] = querySnapshot.docs.map(mapPoll); console.log("Polls:", data); setPolls(data); - }); + });(Remember to import query, orderBy.)
135-136: Show a sensible fallback when name not loaded.- {userMap[poll.createdBy] } + {userMap[poll.createdBy] || poll.createdBy}functions/src/handlers/createFormSubmitHandler.ts (2)
134-137: Use Logger consistently; avoid console.error.- if (!userInfo.user) { - console.error('Failed to get info about user'); - return; - } + if (!userInfo.user) { + log.error('Failed to get info about user'); + return; + }
169-175: Channel save logging: correct field name and unify error logging.- log.info('Channel saved to channels_list', { workspaceId: channelId }); - } catch (e) { - console.error('Failed to save channel:', e); - log.warn('Failed to save channel to channels_list'); - } + log.info('Channel saved to channels_list', { channelId }); + } catch (e) { + log.error('Failed to save channel', { channelId, error: String(e) }); + log.warn('Failed to save channel to channels_list'); + }functions/src/handlers/pollCreationHandler.ts (3)
151-154: Use Logger instead of console.error for user-info failure.- if (!userInfo.user) { - console.error('Failed to get info about user'); - return; - } + if (!userInfo.user) { + log.error('Failed to get info about user'); + return; + }
186-192: Channel save: correct log field and unify error logging.- log.info('Channel saved to channels_list', { workspaceId: command.channel_id }); - } catch (e) { - console.error('Failed to save channel', e); - log.warn('Failed to save channel to channels_list'); - } + log.info('Channel saved to channels_list', { channelId: command.channel_id }); + } catch (e) { + log.error('Failed to save channel', { channelId: command.channel_id, error: String(e) }); + log.warn('Failed to save channel to channels_list'); + }
147-185: DRY: extract “persist user + channel” into a helper to reuse across handlers.These blocks are duplicated here and in createFormSubmitHandler. Consider a shared util (e.g., persistIdentity({ client, userId, channelId })) to reduce drift and ease testing.
functions/src/handlers/voteHandler.ts (1)
139-146: Timer stops before user persistence; move endTimer after it.Measure the whole handler (including user save) by calling endTimer after the persistence block.
functions/src/services/firestoreService.ts (3)
102-121: Upsert user name changes instead of returning stale record.Slack display names change. Update existing doc when name differs:
- if (!existing) { + if (!existing) { const timestamp = new Date().toISOString(); const dataWithTimestamp = { ...user, createdAt: timestamp } as User; const docRef = this.getDocRef(user.id); try { await docRef.set(dataWithTimestamp); console.log('User saved:', dataWithTimestamp); } catch (e) { console.error('Failed to save user:', e); } return dataWithTimestamp; - } - - return existing; + } + if (existing.name !== user.name) { + await this.update(user.id, { name: user.name } as Partial<User>); + return { ...existing, name: user.name }; + } + return existing;
137-156: Mirror upsert behavior for channels (name changes).- if (!existing) { + if (!existing) { const timestamp = new Date().toISOString(); const dataWithTimestamp = { ...channel, createdAt: timestamp } as Channel; const docRef = this.getDocRef(channel.id); try { await docRef.set(dataWithTimestamp); console.log('Channel saved:', dataWithTimestamp); } catch (e) { console.error('Failed to save channel:', e); } return dataWithTimestamp; - } - - return existing; + } + if (existing.name !== channel.name) { + await this.update(channel.id, { name: channel.name } as Partial<Channel>); + return { ...existing, name: channel.name }; + } + return existing;
89-121: Prefer Logger over console for consistency.Replace console.log/error with the app Logger to unify log formatting and levels.
Also applies to: 129-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
firestore.rules(1 hunks)functions/src/handlers/createFormSubmitHandler.ts(2 hunks)functions/src/handlers/pollCreationHandler.ts(2 hunks)functions/src/handlers/voteHandler.ts(4 hunks)functions/src/services/firestoreService.ts(2 hunks)functions/src/types/poll.ts(1 hunks)web/src/components/pollDetail.tsx(2 hunks)web/src/components/pollList.tsx(1 hunks)web/src/types/poll.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
web/src/types/poll.tsx (2)
functions/src/types/poll.ts (2)
User(31-34)Channel(36-39)web/src/types/baseDocument.tsx (1)
BaseDocument(1-4)
functions/src/types/poll.ts (3)
web/src/types/poll.tsx (2)
User(32-35)Channel(37-40)functions/src/types/baseDocument.ts (1)
BaseDocument(1-4)web/src/types/baseDocument.tsx (1)
BaseDocument(1-4)
firestore.rules (1)
functions/src/services/pollService.ts (1)
PollService(5-78)
functions/src/handlers/createFormSubmitHandler.ts (2)
functions/src/services/firestoreService.ts (2)
UserService(89-122)ChannelService(124-157)functions/src/utils/logger.ts (1)
log(85-108)
functions/src/handlers/pollCreationHandler.ts (2)
functions/src/services/firestoreService.ts (2)
UserService(89-122)ChannelService(124-157)functions/src/utils/logger.ts (1)
log(85-108)
functions/src/services/firestoreService.ts (1)
functions/src/types/poll.ts (2)
User(31-34)Channel(36-39)
functions/src/handlers/voteHandler.ts (3)
functions/src/utils/logger.ts (1)
log(85-108)functions/src/services/firestoreService.ts (1)
UserService(89-122)functions/src/services/pollService.ts (2)
PollService(5-78)vote(10-77)
web/src/components/pollDetail.tsx (2)
web/src/types/poll.tsx (1)
Poll(3-16)web/src/lib/chart-config.ts (1)
chartConfig(3-15)
web/src/components/pollList.tsx (2)
functions/src/types/poll.ts (2)
Poll(3-15)User(31-34)web/src/types/poll.tsx (2)
Poll(3-16)User(32-35)
🪛 GitHub Check: Test functions
functions/src/handlers/voteHandler.ts
[failure] 149-149: src/tests/integration/voteHandling.integration.test.ts > Vote Handling Integration Tests > Slack API Error Handling > should handle ephemeral message posting failure
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/integration/voteHandling.integration.test.ts:344:7
[failure] 149-149: src/tests/integration/voteHandling.integration.test.ts > Vote Handling Integration Tests > Slack API Error Handling > should handle message update failure gracefully
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/integration/voteHandling.integration.test.ts:313:7
[failure] 149-149: src/tests/integration/voteHandling.integration.test.ts > Vote Handling Integration Tests > Vote Validation and Error Handling > should handle invalid action format
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/integration/voteHandling.integration.test.ts:283:7
[failure] 149-149: src/tests/integration/voteHandling.integration.test.ts > Vote Handling Integration Tests > Successful Vote Submission > should handle multiple vote options correctly
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/integration/voteHandling.integration.test.ts:134:7
[failure] 149-149: src/tests/integration/voteHandling.integration.test.ts > Vote Handling Integration Tests > Successful Vote Submission > should handle vote submission and update poll message
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/integration/voteHandling.integration.test.ts:55:7
[failure] 149-149: src/tests/voteHandler.test.ts > handleVoteAction > handles bad format of JSON in action.value
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/voteHandler.test.ts:331:5
[failure] 149-149: src/tests/voteHandler.test.ts > handleVoteAction > adds a vote with maxVotes given
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/voteHandler.test.ts:289:5
[failure] 149-149: src/tests/voteHandler.test.ts > handleVoteAction > handles multiple votes with maxVote given
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/voteHandler.test.ts:175:5
[failure] 149-149: src/tests/voteHandler.test.ts > handleVoteAction > successfully handles a vote action and updates the poll
TypeError: Cannot read properties of undefined (reading 'info')
❯ Module.handleVoteAction src/handlers/voteHandler.ts:149:39
❯ src/tests/voteHandler.test.ts:108:5
🪛 GitHub Actions: OpenPoll CI/CD
functions/src/handlers/voteHandler.ts
[error] 149-149: TypeError: Cannot read properties of undefined (reading 'info')
🔇 Additional comments (3)
firestore.rules (1)
5-8: Confirm that all writes are via Admin SDK (rules bypass).With
allow write: if false;onpolls, any client-side writes will fail. If creation/updates happen only in Cloud Functions/Admin SDK, this is fine. Otherwise, loosen the rule accordingly.functions/src/handlers/voteHandler.ts (2)
60-66: Early return on missing poll is correct.
67-76: Good UX: ephemeral notice when poll is closed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
functions/src/handlers/voteHandler.ts (1)
147-171: Fix pipeline break: guard Slack client properly and wrap users.info + persistence with error handling.
Tests show TypeError at users.info. Use optional chaining on client, wrap Slack call, and unify logging (avoid console.*). Also include error details in catch.- const userService = new UserService(); - - if (!client.users?.info) { - console.warn('Slack client missing users.info'); - return; - } - - const userInfo = await client.users.info({ user: body.user.id }); - - if (!userInfo.user) { - console.error('Failed to get info about user'); - return; - } - - const user = { - id: body.user.id, - name: userInfo.user.real_name || userInfo.user.name || 'Unknown', - }; - - try { - await userService.addUser(user); - log.info('User saved to users_list', { userId: user.id }); - } catch { - log.warn('Failed to save user to users_list'); - } + if (!client?.users?.info) { + log.debug('Slack client missing users.info; skipping user persistence', { userId: body.user.id }); + return; + } + const userService = new UserService(); + let userInfo; + try { + userInfo = await client.users.info({ user: body.user.id }); + } catch (e) { + log.warn('Failed to fetch user info from Slack', { userId: body.user.id, error: String(e) }); + return; + } + if (!userInfo?.user) { + log.error('Slack users.info returned no user', { userId: body.user.id }); + return; + } + const user = { + id: body.user.id, + name: userInfo.user.real_name || userInfo.user.name || 'Unknown', + }; + try { + await userService.addUser(user); + log.info('User saved to users_list', { userId: user.id }); + } catch (e) { + log.warn('Failed to save user to users_list', { userId: user.id, error: String(e) }); + }
🧹 Nitpick comments (3)
functions/src/handlers/voteHandler.ts (3)
61-66: Optionally notify the user when the poll is missing.
Currently we only log and return. Consider an ephemeral hint when channel is known.if (!poll) { log.warn('Poll not found', { pollId }); + if (body.channel?.id) { + try { + await client.chat.postEphemeral({ + channel: body.channel.id, + user: userId, + text: 'This poll no longer exists.', + }); + } catch (e) { + log.debug('Failed to post ephemeral for missing poll', { error: String(e) }); + } + } return; }
106-111: Optionally notify when the refreshed poll cannot be loaded.
Helps UX if the message update can’t proceed.if (!updatedPoll) { log.warn('Poll not found after voting', { pollId }); + if (body.channel?.id) { + try { + await client.chat.postEphemeral({ + channel: body.channel.id, + user: userId, + text: 'There was a problem refreshing this poll. Please try again.', + }); + } catch (e) { + log.debug('Failed to post ephemeral when poll not found after voting', { error: String(e) }); + } + } return; }
147-171: Reduce Slack API calls per vote (rate-limit friendly).
Skip users.info if the user already exists in Firestore.- if (!client?.users?.info) { + if (!client?.users?.info) { log.debug('Slack client missing users.info; skipping user persistence', { userId: body.user.id }); return; } - const userService = new UserService(); + const userService = new UserService(); + const existing = await userService.getById(body.user.id); + if (existing) { + log.debug('User already exists; skipping Slack lookup', { userId: existing.id }); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
functions/src/handlers/voteHandler.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
functions/src/handlers/voteHandler.ts (2)
functions/src/services/firestoreService.ts (1)
UserService(89-122)functions/src/services/pollService.ts (1)
PollService(5-78)
🔇 Additional comments (1)
functions/src/handlers/voteHandler.ts (1)
7-7: LGTM: import aligns with new Firestore user persistence.
There was a problem hiding this comment.
Actionable comments posted: 6
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)
78-87: Bug: After telling the user "option no longer available", execution continues and may still vote. Return immediately.Prevents invalid votes when option is deleted/missing.
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; }
♻️ Duplicate comments (1)
functions/src/handlers/voteHandler.ts (1)
149-151: Guard Slack client and wrap user persistence to prevent test and runtime breaks.Aligns with prior review feedback; also replace console with Logger.
- if (!client.users?.info) { - console.warn('Slack client missing users.info'); - return; - } - - const userInfo = await client.users.info({ user: body.user.id }); - - if (!userInfo.user) { - console.error('Failed to get info about user'); - return; - } + if (!client?.users?.info) { + log.debug('Slack client missing users.info; skipping user persistence'); + return; + } + let userInfo; + try { + userInfo = await client.users.info({ user: body.user.id }); + } catch (e) { + log.warn('users.info failed', { error: String(e) }); + return; + } + if (!userInfo.user) { + log.error('Failed to get info about user'); + return; + } @@ - try { - await userService.addUser(user); - log.info('User saved to users_list', { userId: user.id }); - } catch { - log.warn('Failed to save user to users_list'); - } + try { + await userService.addUser(user); + log.info('User saved to users_list', { userId: user.id }); + } catch (e) { + log.warn('Failed to save user to users_list', { error: String(e) }); + }Also applies to: 154-171
🧹 Nitpick comments (6)
functions/src/services/firestoreUserService.ts (3)
25-27: Avoid race between existence check and write — prefer create() to fail if doc already exists.
set()after a priorgetById()can still race. Usecreate()to enforce "create-only" semantics.- await docRef.set(dataWithTimestamp); + await docRef.create(dataWithTimestamp);
10-14: Use structured Logger instead of console for consistency.Other handlers use
Logger; keep logs uniform and searchable.+import { Logger } from '../utils/logger'; @@ - console.log('Adding user:', user); + const log = new Logger().withContext({ service: 'UserService', userId: user.id }); + log.info('Adding user', {}); @@ - console.error('User ID is missing! Cannot save user.'); + log.error('User ID is missing! Cannot save user.'); @@ - console.log('Existing user:', existing); + log.debug('Existing user lookup', { found: Boolean(existing) }); @@ - console.log('User saved:', dataWithTimestamp); + log.info('User saved', {}); @@ - console.error('Failed to save user:', e); + log.error('Failed to save user', { error: String(e) });Also applies to: 18-18, 27-27, 29-29
9-9: Add explicit return type for clarity.Helps consumers and avoids accidental widening.
- async addUser(user: Omit<User, 'createdAt'>) { + async addUser(user: Omit<User, 'createdAt'>): Promise<User | null> {functions/src/services/firestoreChannelService.ts (3)
25-26: Prefer create() over set() to eliminate TOCTOU race.- await docRef.set(dataWithTimestamp); + await docRef.create(dataWithTimestamp);
10-14: Align logging with Logger used elsewhere.+import { Logger } from '../utils/logger'; @@ - console.log('Adding channel:', channel); + const log = new Logger().withContext({ service: 'ChannelService', channelId: channel.id }); + log.info('Adding channel', {}); @@ - console.error('Channel ID is missing! Cannot save channel.'); + log.error('Channel ID is missing! Cannot save channel.'); @@ - console.log('Existing channel:', existing); + log.debug('Existing channel lookup', { found: Boolean(existing) }); @@ - console.log('Channel saved:', dataWithTimestamp); + log.info('Channel saved', {}); @@ - console.error('Failed to save channel:', e); + log.error('Failed to save channel', { error: String(e) });Also applies to: 18-18, 27-27, 29-29
9-9: Add explicit return type for clarity.- async addChannel(channel: Omit<Channel, 'createdAt'>) { + async addChannel(channel: Omit<Channel, 'createdAt'>): Promise<Channel | null> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
functions/src/handlers/createFormSubmitHandler.ts(2 hunks)functions/src/handlers/pollCreationHandler.ts(2 hunks)functions/src/handlers/voteHandler.ts(4 hunks)functions/src/services/firestoreChannelService.ts(1 hunks)functions/src/services/firestoreUserService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
functions/src/services/firestoreChannelService.ts (1)
functions/src/services/firestoreService.ts (2)
FirestoreService(15-86)create(40-44)
functions/src/services/firestoreUserService.ts (2)
functions/src/services/firestoreService.ts (2)
FirestoreService(15-86)create(40-44)functions/src/tests/utils/unitTestSetup.ts (1)
FirestoreService(111-126)
functions/src/handlers/createFormSubmitHandler.ts (3)
functions/src/services/firestoreUserService.ts (1)
UserService(4-37)functions/src/utils/logger.ts (1)
log(85-108)functions/src/services/firestoreChannelService.ts (1)
ChannelService(4-37)
functions/src/handlers/pollCreationHandler.ts (3)
functions/src/services/firestoreUserService.ts (1)
UserService(4-37)functions/src/utils/logger.ts (1)
log(85-108)functions/src/services/firestoreChannelService.ts (1)
ChannelService(4-37)
functions/src/handlers/voteHandler.ts (1)
functions/src/services/firestoreUserService.ts (1)
UserService(4-37)
🔇 Additional comments (3)
functions/src/services/firestoreUserService.ts (1)
4-7: Confirm server-side permissions path.If this service runs with Admin SDK, security rules are bypassed; if not, writes to
users_listmust be allowed by rules.Would you confirm this code path uses the Admin SDK context (Firebase Admin) in production so writes are permitted?
functions/src/handlers/voteHandler.ts (2)
62-66: Good: Return early when poll not found.
106-110: Good: Return early when updated poll cannot be retrieved.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
functions/src/handlers/createFormSubmitHandler.ts (2)
131-149: Persist a fallback user and include error details/metadata in logs.If
users.infofails or returns no user, still upsert{ id, name: 'Unknown' }so UI can resolve names later. Also log the caught error and workspace/channel/user identifiers for observability.- if (!client?.users?.info) { - log.debug('Slack client missing users.info; skipping user persistence'); - } else { - try { - const userService = new UserService(); - const userInfo = await client.users.info({ user: body.user.id }); - if (!userInfo.user) { - log.error('Failed to get info about user'); - } else { - const user = { - id: body.user.id, - name: userInfo.user.real_name || userInfo.user.name || 'Unknown', - }; - await userService.addUser(user); - log.info('User saved to users_list', { userId: user.id }); - } - } catch { - log.warn('Failed to save user to users_list'); - } - } + if (!client?.users?.info) { + log.debug('Slack client missing users.info; skipping user persistence'); + } else { + const userService = new UserService(); + let name = 'Unknown'; + try { + const userInfo = await client.users.info({ user: body.user.id }); + if (!userInfo.user) { + log.warn('users.info returned no user', { userId: body.user.id, workspaceId: body.team?.id }); + } else { + name = + userInfo.user.profile?.display_name_normalized || + userInfo.user.profile?.real_name_normalized || + userInfo.user.real_name || + userInfo.user.name || + 'Unknown'; + } + } catch (e) { + log.warn('users.info failed', { userId: body.user.id, workspaceId: body.team?.id, error: String(e) }); + } + try { + const user = { id: body.user.id, name }; + await userService.addUser(user); + log.info('User saved to users_list', { userId: user.id, workspaceId: body.team?.id }); + } catch (e) { + log.warn('Failed to save user to users_list', { userId: body.user.id, workspaceId: body.team?.id, error: String(e) }); + } + }
152-172: Mirror defensive pattern for channels: fallback upsert + richer logs.Upsert
{ id: channelId, name: 'Unknown' }ifconversations.infofails or returns no channel. IncludechannelIdandworkspaceIdin logs and capture the error.- if (!client?.conversations?.info) { - log.debug('Slack client missing conversations.info; skipping channel persistence'); - } else { - try { - const channelService = new ChannelService(); - const channelInfo = await client.conversations.info({ channel: String(channelId) }); - if (!channelInfo.channel) { - log.error('Failed to get info about channel'); - } else { - const channelData = channelInfo.channel; - const channel = { - id: channelId, - name: channelData.name || 'Unknown', - }; - await channelService.addChannel(channel); - log.info('Channel saved to channels_list'); - } - } catch { - log.warn('Failed to save channel to channels_list'); - } - } + if (!client?.conversations?.info) { + log.debug('Slack client missing conversations.info; skipping channel persistence'); + } else { + const channelService = new ChannelService(); + let channelName = 'Unknown'; + try { + const channelInfo = await client.conversations.info({ channel: String(channelId) }); + if (!channelInfo.channel) { + log.warn('conversations.info returned no channel', { channelId, workspaceId: body.team?.id }); + } else { + const channelData = channelInfo.channel; + channelName = channelData.name_normalized || channelData.name || 'Unknown'; + } + } catch (e) { + log.warn('conversations.info failed', { channelId, workspaceId: body.team?.id, error: String(e) }); + } + try { + const channel = { id: channelId, name: channelName }; + await channelService.addChannel(channel); + log.info('Channel saved to channels_list', { channelId, workspaceId: body.team?.id }); + } catch (e) { + log.warn('Failed to save channel to channels_list', { channelId, workspaceId: body.team?.id, error: String(e) }); + } + }
🧹 Nitpick comments (1)
functions/src/handlers/createFormSubmitHandler.ts (1)
7-8: Imports look good; keep services decoupled for testability.Consider injecting
UserService/ChannelServicevia args or a simple factory to simplify unit testing and stubbing in handlers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
functions/src/handlers/createFormSubmitHandler.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
functions/src/handlers/createFormSubmitHandler.ts (3)
functions/src/utils/logger.ts (1)
log(85-108)functions/src/services/firestoreUserService.ts (1)
UserService(4-37)functions/src/services/firestoreChannelService.ts (1)
ChannelService(4-37)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
web/src/types/user.tsx (1)
3-6: Same export requirement for BaseDocument as noted in channel.tsx.
Please ensure BaseDocument is exported in web/src/types/baseDocument.tsx.functions/src/handlers/pollCreationHandler.ts (2)
148-167: Include error details and validate Slack response.Capture the error in catch, and also consider checking userInfo.ok.
Apply:
- if (!client?.users?.info) { + if (!client?.users?.info) { log.debug('Slack client missing users.info; skipping user persistence'); } else { try { const userService = new UserService(); const userInfo = await client.users.info({ user: command.user_id }); - if (!userInfo.user) { - log.error('Failed to get info about user'); + if ((userInfo as any)?.ok === false || !userInfo.user) { + log.error('Failed to get info about user', { userId: command.user_id }); } else { const user = { id: command.user_id, name: userInfo.user.real_name || userInfo.user.name || 'Unknown', }; await userService.addUser(user); log.info('User saved to users_list', { userId: user.id }); } - } catch { - log.warn('Failed to save user to users_list'); + } catch (e) { + log.warn('Failed to save user to users_list', { error: String(e), userId: command.user_id }); } }Optional: run channel and user persistence in parallel with Promise.allSettled to reduce latency after poll creation.
169-189: Add context to logs and keep error details.Mirror improvements from the user block; include channelId in the success log.
Apply:
- if (!client?.conversations?.info) { + if (!client?.conversations?.info) { log.debug('Slack client missing conversations.info; skipping channel persistence'); } else { try { const channelService = new ChannelService(); const channelInfo = await client.conversations.info({ channel: command.channel_id }); - if (!channelInfo.channel) { - log.error('Failed to get info about channel'); + if ((channelInfo as any)?.ok === false || !channelInfo.channel) { + log.error('Failed to get info about channel', { channelId: command.channel_id }); } else { const channelData = channelInfo.channel; const channel = { id: command.channel_id, name: channelData.name || 'Unknown', }; await channelService.addChannel(channel); - log.info('Channel saved to channels_list'); + log.info('Channel saved to channels_list', { channelId: command.channel_id }); } - } catch { - log.warn('Failed to save channel to channels_list'); + } catch (e) { + log.warn('Failed to save channel to channels_list', { error: String(e), channelId: command.channel_id }); } }
🧹 Nitpick comments (18)
web/src/types/channel.tsx (1)
1-1: Import path nit: drop the file extension for consistency.
Use path alias without the .tsx suffix (common TS convention, avoids churn if file type changes).-import type { BaseDocument } from '@/types/baseDocument.tsx'; +import type { BaseDocument } from '@/types/baseDocument';web/src/types/user.tsx (1)
1-1: Import path nit: drop the file extension for consistency.
Same rationale as channel.tsx.-import type { BaseDocument } from '@/types/baseDocument.tsx'; +import type { BaseDocument } from '@/types/baseDocument';functions/src/types/user.ts (1)
1-1: Prefer type-only import.
Use import type to avoid emitting runtime import for a type.-import { BaseDocument } from './baseDocument'; +import type { BaseDocument } from './baseDocument';functions/src/types/channel.ts (2)
1-1: Prefer type-only import.
Minor optimization; keeps emitted JS clean.-import { BaseDocument } from './baseDocument'; +import type { BaseDocument } from './baseDocument';
3-6: Backend type looks good; consider centralizing shared types.
Channel mirrors the web type. Consider extracting shared interfaces (BaseDocument, User, Channel, Poll) to a small shared package (e.g., /packages/types) to avoid duplication and drift.functions/src/services/userService.ts (2)
4-7: Use a named constant for the collection name.Avoid stringly-typed collection names sprinkled across services; define a shared const (e.g., COLLECTIONS.USERS = 'users_list') to prevent typos and ease refactors.
9-36: Replace console logs with the project Logger and type the return.Console makes log aggregation inconsistent; also return type is implicit. Use Logger and include error details in catch.
Apply:
+import { Logger } from '../utils/logger' - async addUser(user: Omit<User, 'createdAt'>) { - console.log('Adding user:', user); + async addUser(user: Omit<User, 'createdAt'>): Promise<User | null> { + const log = new Logger({ functionName: 'UserService.addUser', userId: user?.id }); + log.debug('Adding user'); if (!user.id) { - console.error('User ID is missing! Cannot save user.'); + log.warn('User ID is missing; cannot save user'); return null; } const existing = await this.getById(user.id); - console.log('Existing user:', existing); + log.debug('Existing user lookup', { found: Boolean(existing) }); if (!existing) { const timestamp = new Date().toISOString(); const dataWithTimestamp = { ...user, createdAt: timestamp } as User; const docRef = this.getDocRef(user.id); try { await docRef.set(dataWithTimestamp); - console.log('User saved:', dataWithTimestamp); + log.info('User saved to users_list', { userId: user.id }); return dataWithTimestamp; - } catch (e) { - console.error('Failed to save user:', e); + } catch (e) { + log.error('Failed to save user to users_list', { error: String(e), userId: user.id }); return null; } } return existing; }Optional: consider upserting name changes for existing users (if the Slack display name changes) rather than returning early. I can draft that if desired.
functions/src/tests/userService.test.ts (2)
60-62: Prefer fake timers over spying Date.prototype.Use setSystemTime for deterministic timestamps and restore timers in afterEach.
Apply:
- const mockDate = '2023-01-01T00:00:00.000Z'; - vi.spyOn(global.Date.prototype, 'toISOString').mockReturnValue(mockDate); + const mockDate = '2023-01-01T00:00:00.000Z'; + vi.useFakeTimers().setSystemTime(new Date(mockDate)); @@ - vi.restoreAllMocks();And add:
+import { afterEach } from 'vitest' @@ + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + });Also applies to: 78-79
31-36: Add an assertion for the collection name used.Guard against regressions by asserting that firestore.collection was called with 'users_list'.
Example:
expect(firestore.collection).toHaveBeenCalledWith('users_list');functions/src/services/channelService.ts (2)
4-7: Use a shared constant for the collection name.Mirror the users service: define COLLECTIONS.CHANNELS = 'channels_list' to avoid string duplication.
9-36: Align logging with Logger and type the return.Use the project Logger; include error details; make return type explicit.
Apply:
+import { Logger } from '../utils/logger' - async addChannel(channel: Omit<Channel, 'createdAt'>) { - console.log('Adding channel:', channel); + async addChannel(channel: Omit<Channel, 'createdAt'>): Promise<Channel | null> { + const log = new Logger({ functionName: 'ChannelService.addChannel' }); + log.debug('Adding channel', { channelId: channel?.id }); if (!channel.id) { - console.error('Channel ID is missing! Cannot save channel.'); + log.warn('Channel ID is missing; cannot save channel'); return null; } const existing = await this.getById(channel.id); - console.log('Existing channel:', existing); + log.debug('Existing channel lookup', { found: Boolean(existing) }); if (!existing) { const timestamp = new Date().toISOString(); const dataWithTimestamp = { ...channel, createdAt: timestamp } as Channel; const docRef = this.getDocRef(channel.id); try { await docRef.set(dataWithTimestamp); - console.log('Channel saved:', dataWithTimestamp); + log.info('Channel saved to channels_list', { channelId: channel.id }); return dataWithTimestamp; - } catch (e) { - console.error('Failed to save channel:', e); + } catch (e) { + log.warn('Failed to save channel to channels_list', { error: String(e), channelId: channel.id }); return null; } } return existing; }Optional: upsert name if it changes to keep records fresh.
functions/src/tests/channelService.test.ts (2)
60-62: Use fake timers for timestamp determinism.Swap prototype spying for setSystemTime and move restore to afterEach.
Apply:
- const mockDate = '2023-01-01T00:00:00.000Z'; - vi.spyOn(global.Date.prototype, 'toISOString').mockReturnValue(mockDate); + const mockDate = '2023-01-01T00:00:00.000Z'; + vi.useFakeTimers().setSystemTime(new Date(mockDate)); @@ - vi.restoreAllMocks();And add:
+import { afterEach } from 'vitest' @@ + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + });Also applies to: 78-79
31-36: Assert collection name.Add an expectation for 'channels_list' to catch accidental changes.
Example:
expect(firestore.collection).toHaveBeenCalledWith('channels_list');web/src/components/pollDetail.tsx (5)
2-2: Memoize derived data to avoid recomputation on every render.
voteCountsandchartDataare pure derivations; memoize them.-import { useEffect, useState } from "react"; +import { useEffect, useMemo, useState } from "react"; @@ - const voteCounts = poll.votes?.reduce((acc, vote) => { - acc[vote.optionId] = (acc[vote.optionId] || 0) + 1; - return acc; - }, {} as Record<string, number>) || {}; + const voteCounts = useMemo(() => { + return (poll.votes ?? []).reduce((acc, vote) => { + acc[vote.optionId] = (acc[vote.optionId] || 0) + 1; + return acc; + }, {} as Record<string, number>); + }, [poll.votes]); @@ - const chartData = poll.options.map(option => ({ - optionId: option.label, - votes: voteCounts[option.id] || 0, - })); + const chartData = useMemo( + () => poll.options.map(option => ({ + optionId: option.label, + votes: voteCounts[option.id] || 0, + })), + [poll.options, voteCounts] + );Also applies to: 90-98, 95-99
62-69: Add error callbacks to secondary listeners.The
users_listandchannels_listlisteners don’t handle errors; add callbacks to aid debugging.- const unsubscribeUsers = onSnapshot(collection(db, "users_list"), (querySnapshot) => { + const unsubscribeUsers = onSnapshot(collection(db, "users_list"), (querySnapshot) => { const map: Record<string, string> = {}; querySnapshot.docs.forEach(doc => { const data = doc.data() as { name?: string }; map[doc.id] = data.name ?? "Unknown"; }); setUsers(map); - }); + }, (err) => { + console.error("users_list realtime error:", err); + }); @@ - const unsubscribeChannels = onSnapshot(collection(db, "channels_list"), (querySnapshot) => { + const unsubscribeChannels = onSnapshot(collection(db, "channels_list"), (querySnapshot) => { const map: Record<string, string> = {}; querySnapshot.docs.forEach(doc => { const data = doc.data() as { name?: string }; map[doc.id] = data.name ?? "Unknown Channel"; }); setChannels(map); - }); + }, (err) => { + console.error("channels_list realtime error:", err); + });Also applies to: 71-78
62-78: Scope listeners to only the needed docs to reduce read costs.Subscribing to entire
users_listandchannels_listcan be heavy in large workspaces. Prefer listeners (or one-timegetDoc) forpoll.createdByandpoll.channelIdonly.I can provide a refactor splitting effects: (1) subscribe to the poll, (2) when it loads, subscribe to
doc(db, "users_list", poll.createdBy)anddoc(db, "channels_list", poll.channelId).
33-41: Harden ref unwrapping checks.Current checks assume any object with
idis a ref. To avoid false positives, also check forpath(both exist on Firestore refs).- createdBy: - typeof data.createdBy === 'object' && data.createdBy !== null - ? (data.createdBy as { id: string }).id + createdBy: + typeof data.createdBy === 'object' && + data.createdBy !== null && + 'id' in (data.createdBy as any) && + 'path' in (data.createdBy as any) + ? (data.createdBy as { id: string }).id : (data.createdBy as string), @@ - channelId: - typeof data.channelId === 'object' && data.channelId !== null - ? (data.channelId as { id: string }).id + channelId: + typeof data.channelId === 'object' && + data.channelId !== null && + 'id' in (data.channelId as any) && + 'path' in (data.channelId as any) + ? (data.channelId as { id: string }).id : (data.channelId as string),
134-141: Empty options state in UI.If
poll.optionsis empty, the “Options” list renders nothing. Consider a friendly placeholder.- {poll.options.map((opt) => ( + {poll.options.length === 0 ? ( + <li className="text-muted-foreground">No options</li> + ) : poll.options.map((opt) => ( <li key={opt.id} className="flex justify-between border p-2 rounded"> <span>{opt.label}</span> <Badge variant="secondary">{voteCounts[opt.id] ?? 0} votes</Badge> </li> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
functions/src/handlers/createFormSubmitHandler.ts(2 hunks)functions/src/handlers/pollCreationHandler.ts(2 hunks)functions/src/handlers/voteHandler.ts(4 hunks)functions/src/services/channelService.ts(1 hunks)functions/src/services/pollService.ts(0 hunks)functions/src/services/userService.ts(1 hunks)functions/src/tests/channelService.test.ts(1 hunks)functions/src/tests/userService.test.ts(1 hunks)functions/src/types/channel.ts(1 hunks)functions/src/types/user.ts(1 hunks)web/src/components/pollDetail.tsx(2 hunks)web/src/components/pollList.tsx(1 hunks)web/src/types/channel.tsx(1 hunks)web/src/types/poll.tsx(1 hunks)web/src/types/user.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- functions/src/services/pollService.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- functions/src/handlers/createFormSubmitHandler.ts
- web/src/components/pollList.tsx
- functions/src/handlers/voteHandler.ts
🧰 Additional context used
🧬 Code graph analysis (11)
web/src/types/channel.tsx (3)
functions/src/types/channel.ts (1)
Channel(3-6)web/src/types/baseDocument.tsx (1)
BaseDocument(1-4)functions/src/types/baseDocument.ts (1)
BaseDocument(1-4)
functions/src/types/user.ts (3)
web/src/types/user.tsx (1)
User(3-6)functions/src/tests/firestoreService.test.ts (1)
TestDoc(15-19)functions/src/types/baseDocument.ts (1)
BaseDocument(1-4)
functions/src/types/channel.ts (5)
web/src/types/channel.tsx (1)
Channel(3-6)web/src/types/baseDocument.tsx (1)
BaseDocument(1-4)functions/src/types/baseDocument.ts (1)
BaseDocument(1-4)functions/src/tests/firestoreService.test.ts (1)
TestDoc(15-19)functions/src/tests/utils/testDataFactories.ts (1)
createSlackChannel(80-85)
web/src/types/user.tsx (3)
functions/src/types/user.ts (1)
User(3-6)web/src/types/baseDocument.tsx (1)
BaseDocument(1-4)web/src/types/AuthContext.ts (1)
AuthContextType(4-10)
functions/src/tests/channelService.test.ts (4)
functions/src/services/channelService.ts (1)
ChannelService(4-37)functions/src/types/channel.ts (1)
Channel(3-6)functions/src/firebase.ts (1)
firestore(5-5)functions/src/tests/utils/unitTestSetup.ts (2)
mockFirebaseServices(12-52)FirestoreService(111-126)
functions/src/tests/userService.test.ts (4)
functions/src/services/userService.ts (1)
UserService(4-37)functions/src/types/user.ts (1)
User(3-6)functions/src/firebase.ts (1)
firestore(5-5)functions/src/tests/utils/unitTestSetup.ts (3)
mockFirebaseServices(12-52)FirestoreService(111-126)firestore(14-31)
functions/src/services/userService.ts (3)
functions/src/services/firestoreService.ts (2)
FirestoreService(15-86)create(40-44)functions/src/types/user.ts (1)
User(3-6)functions/src/tests/utils/unitTestSetup.ts (1)
FirestoreService(111-126)
functions/src/services/channelService.ts (2)
functions/src/services/firestoreService.ts (2)
FirestoreService(15-86)create(40-44)functions/src/types/channel.ts (1)
Channel(3-6)
web/src/types/poll.tsx (1)
functions/src/types/poll.ts (2)
Vote(26-29)Poll(3-15)
functions/src/handlers/pollCreationHandler.ts (3)
functions/src/utils/logger.ts (1)
log(85-108)functions/src/services/userService.ts (1)
UserService(4-37)functions/src/services/channelService.ts (1)
ChannelService(4-37)
web/src/components/pollDetail.tsx (4)
web/src/types/poll.tsx (1)
Poll(3-16)functions/src/services/pollService.ts (1)
vote(10-71)web/src/components/logOutButton.tsx (1)
LogOutButton(5-28)web/src/lib/chart-config.ts (1)
chartConfig(3-15)
🔇 Additional comments (6)
web/src/types/channel.tsx (1)
3-6: No action needed:BaseDocumentexport confirmed.
Verified thatBaseDocumentis exported in bothweb/src/types/baseDocument.tsxandfunctions/src/types/baseDocument.ts.web/src/types/poll.tsx (1)
27-30: No-op formatting change. LGTM.
No semantic changes in Vote; safe to merge.functions/src/types/user.ts (1)
3-6: Remove incorrect BaseDocument export warningBaseDocument is already exported in functions/src/types/baseDocument.ts (
export interface BaseDocument {…}on line 1), so the import in user.ts will succeed.Likely an incorrect or invalid review comment.
functions/src/tests/userService.test.ts (1)
6-10: Hoist the firebase mock before imports.Ensure Vitest applies the mock consistently by declaring vi.mock before importing modules that use it.
Apply:
-import { UserService } from '../services/userService'; -import { firestore } from '../firebase'; +vi.mock('../firebase', () => ({ + firestore: { collection: vi.fn() }, +})); +import { UserService } from '../services/userService'; +import { firestore } from '../firebase';Then run vitest to confirm the mock is effective.
functions/src/tests/channelService.test.ts (1)
6-10: Hoist the firebase mock before imports.Match the user service test fix so the mock is guaranteed to apply.
Apply:
-import { ChannelService } from '../services/channelService'; -import { firestore } from '../firebase'; +vi.mock('../firebase', () => ({ + firestore: { collection: vi.fn() }, +})); +import { ChannelService } from '../services/channelService'; +import { firestore } from '../firebase';functions/src/handlers/pollCreationHandler.ts (1)
9-10: Imports look good.Services are imported where used; no issues.
maprokes
left a comment
maprokes
left a comment
There was a problem hiding this comment.
@Roubean1 Thanks for fixing the file structure, it's much easier to review now. I initially missed the way how the logic is implemented for getting the user/channel data from the Slack api - sorry for that - but please have a look at my comments or feel free to reach out to me if it's not clear.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
functions/src/handlers/pollCreationHandler.ts (3)
148-197: Optional: replace console logs in parseCommand with LoggerThere are console.log calls later in this file; standardize on Logger.
Outside this hunk:
- if (!question || question.trim().length === 0) { - console.log('Command validation failed: Empty question', 'Command:', text); + if (!question || question.trim().length === 0) { + log.warn('Command validation failed: Empty question', { text }); return null; } - console.log('Successfully parsed command:', { - question, - options, - flags: { isMultiple, isCustom, isAnonymous, maxVotes }, - }); + log.debug('Successfully parsed command', { + metadata: { question, options, flags: { isMultiple, isCustom, isAnonymous, maxVotes } }, + });
148-171: Capture and log errors with context; avoid empty catchUse the error variable and include userId for traceability.
- } catch { - log.warn('Failed to save user to users_list'); + } catch (e) { + log.warn('Failed to save user to users_list', { userId: command.user_id, error: String(e) }); }
173-188: Fix metadata: don’t log channelId as workspaceId; include channelId consistentlyLog the actual channelId and include error detail on failure.
- if (existingChannel) { - log.info('Channel already exists in channels_list', { workspaceId: existingChannel.id }); + if (existingChannel) { + log.info('Channel already exists in channels_list', { channelId: existingChannel.id }); } else if (client?.conversations?.info) { try { const channelInfo = await client.conversations.info({ channel: command.channel_id }); if (channelInfo.channel) { const channelData = channelInfo.channel; const channel = { id: command.channel_id, name: channelData.name || 'Unknown', }; await channelService.addChannel(channel); - log.info('Channel saved to channels_list'); + log.info('Channel saved to channels_list', { channelId: command.channel_id }); } else { log.error('Failed to get info about channel'); } - } catch { - log.warn('Failed to save channel to channels_list'); + } catch (e) { + log.warn('Failed to save channel to channels_list', { channelId: command.channel_id, error: String(e) }); } } else { log.debug('Slack client missing conversations.info; skipping channel persistence'); }
🧹 Nitpick comments (2)
functions/src/handlers/pollCreationHandler.ts (2)
198-206: Reduce leakage: don’t echo raw errors back to usersKeep internal errors in logs; send a generic message to users.
- await client.chat.postEphemeral({ + await client.chat.postEphemeral({ channel: command.channel_id, user: command.user_id, - blocks: [mrkdwnSection('error', `Error: ${error}`)], - text: 'An error occurred', + blocks: [mrkdwnSection('error', 'An error occurred. Please try again later.')], + text: 'An error occurred', });
148-197: Minor: align logging keys and add ok-checks (optional)
- Consider logging { commandText: command.text } at Line 42 instead of using functionName.
- Optionally validate Slack responses with userInfo.ok/channelInfo.ok.
Additional patch (outside changed hunk for convenience):
- log.info('Poll command received', { functionName: command.text }); + log.info('Poll command received', { commandText: command.text });- const userInfo = await client.users.info({ user: command.user_id }); - if (userInfo.user) { + const userInfo = await client.users.info({ user: command.user_id }); + if ((userInfo as any).ok !== false && userInfo.user) {- const channelInfo = await client.conversations.info({ channel: command.channel_id }); - if (channelInfo.channel) { + const channelInfo = await client.conversations.info({ channel: command.channel_id }); + if ((channelInfo as any).ok !== false && channelInfo.channel) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
functions/src/handlers/pollCreationHandler.ts(2 hunks)functions/src/handlers/voteHandler.ts(4 hunks)functions/src/services/channelService.ts(1 hunks)functions/src/services/userService.ts(1 hunks)functions/src/tests/channelService.test.ts(1 hunks)functions/src/tests/userService.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/src/handlers/voteHandler.ts
🧰 Additional context used
🧬 Code graph analysis (5)
functions/src/tests/userService.test.ts (3)
functions/src/services/userService.ts (1)
UserService(4-19)functions/src/types/user.ts (1)
User(3-6)functions/src/tests/utils/unitTestSetup.ts (2)
FirestoreService(111-126)mockFirebaseServices(12-52)
functions/src/services/userService.ts (2)
functions/src/services/firestoreService.ts (2)
FirestoreService(15-86)create(40-44)functions/src/types/user.ts (1)
User(3-6)
functions/src/tests/channelService.test.ts (3)
functions/src/services/channelService.ts (1)
ChannelService(4-19)functions/src/types/channel.ts (1)
Channel(3-6)functions/src/tests/utils/unitTestSetup.ts (1)
mockFirebaseServices(12-52)
functions/src/services/channelService.ts (2)
functions/src/services/firestoreService.ts (2)
FirestoreService(15-86)create(40-44)functions/src/types/channel.ts (1)
Channel(3-6)
functions/src/handlers/pollCreationHandler.ts (4)
functions/src/services/userService.ts (1)
UserService(4-19)functions/src/utils/logger.ts (2)
log(85-108)error(59-67)functions/src/services/channelService.ts (1)
ChannelService(4-19)functions/src/components/mrkdwnSection.ts (1)
mrkdwnSection(12-18)
🔇 Additional comments (3)
functions/src/tests/userService.test.ts (1)
44-48: Keep this test; message stays validThe missing-id error remains correct with the refactor.
functions/src/tests/channelService.test.ts (1)
46-50: Keep error propagation testBehavior unchanged; good.
functions/src/handlers/pollCreationHandler.ts (1)
9-10: LGTM: service importsMatches new services usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
functions/src/services/channelService.ts (1)
9-15: Tighten input and surface clearer intent (upsert semantics)Add basic name validation and narrow the input type to required fields. Optional: rename to upsertChannel to reflect idempotent behavior.
- async addChannel(channel: Omit<Channel, 'createdAt'>) { + async addChannel(channel: Pick<Channel, 'id' | 'name'>) { if (!channel.id) { throw new Error('Channel ID is missing! Cannot save channel.'); } + if (!channel.name?.trim()) { + throw new Error('Channel name is missing! Cannot save channel.'); + } return this.createWithId(channel.id, channel); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
functions/src/handlers/pollCreationHandler.ts(3 hunks)functions/src/services/channelService.ts(1 hunks)functions/src/services/firestoreService.ts(1 hunks)functions/src/services/userService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- functions/src/services/userService.ts
- functions/src/handlers/pollCreationHandler.ts
🧰 Additional context used
🧬 Code graph analysis (1)
functions/src/services/channelService.ts (2)
functions/src/services/firestoreService.ts (1)
FirestoreService(15-93)functions/src/types/channel.ts (1)
Channel(3-6)
🔇 Additional comments (1)
functions/src/services/channelService.ts (1)
5-7: Correct collection bindingConstructor correctly targets channels_list and inherits typed converter.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
functions/src/tests/channelService.test.ts (1)
31-33: Remove unused Date mocking
addChanneldoesn’t touch timestamps; the Date spy is dead code.- const mockDate = '2023-01-01T00:00:00.000Z'; - vi.spyOn(global.Date.prototype, 'toISOString').mockReturnValue(mockDate); - @@ - - vi.restoreAllMocks();Also applies to: 45-45
🧹 Nitpick comments (4)
functions/src/tests/channelService.test.ts (4)
4-4: Use Admin SDK Firestore types to match server codeChannelService runs on Admin SDK; align the test type with
firebase-admin/firestore.-import type { DocumentReference } from 'firebase/firestore'; +import type { DocumentReference } from 'firebase-admin/firestore';
1-1: Prefer typed vi.fn generics over casting to Mock; simplify assignmentCleaner typing and fewer casts; also no need to shape-cast the service.
-import { describe, it, expect, vi, beforeEach, Mock } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; @@ - mockCreateWithId = vi.fn() as Mock< - (id: string, data: Omit<Channel, 'createdAt'>) => Promise<DocumentReference<Channel>> - >; - (service as unknown as { createWithId: typeof mockCreateWithId }).createWithId = mockCreateWithId; + mockCreateWithId = vi.fn< + [id: string, data: Omit<Channel, 'createdAt'>], + Promise<DocumentReference<Channel>> + >(); + (service as any).createWithId = mockCreateWithId;Also applies to: 17-21
39-42: Assert with the variable instead of duplicating literalsReduces drift if the test input changes.
- expect(mockCreateWithId).toHaveBeenCalledWith('channel-1', { - id: 'channel-1', - name: 'Test Channel', - }); + expect(mockCreateWithId).toHaveBeenCalledWith('channel-1', newChannel);
25-27: Avoid misleading cast to Channel for invalid inputUse
anyfor the negative-path test to reflect intentionally invalid shape.- await expect(service.addChannel({ name: 'No ID' } as Channel)).rejects.toThrow( + await expect(service.addChannel({ name: 'No ID' } as any)).rejects.toThrow( 'Channel ID is missing! Cannot save channel.' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
functions/src/tests/channelService.test.ts(1 hunks)functions/src/tests/userService.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- functions/src/tests/userService.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
functions/src/tests/channelService.test.ts (3)
functions/src/services/channelService.ts (1)
ChannelService(4-16)functions/src/types/channel.ts (1)
Channel(3-6)functions/src/tests/utils/unitTestSetup.ts (1)
mockFirebaseServices(12-52)
🔇 Additional comments (1)
functions/src/tests/channelService.test.ts (1)
48-52: LGTM: error propagation behavior is correctThe test cleanly verifies passthrough of failures from the underlying write.
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 (3)
functions/src/handlers/voteHandler.ts (3)
78-87: Bug: missing early return after notifying about invalid/deleted option.
Without returning, we still attempt to vote on a non-existent option, corrupting poll state.Apply this diff:
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; }
141-145: Make user persistence run on all code paths by moving it into finally.
Right now, most early returns (poll closed, vote rejected, no messageTs) skip persistence. PR objective says to persist Slack users during voting; running it from finally guarantees consistency.Apply this diff:
} catch (error) { log.error(String(error)); } finally { - log.endTimer('voteAction', timerStart); + await persistUserSafely(); + log.endTimer('voteAction', timerStart); } - - if (!client.users?.info) { - log.info('Slack client missing users.info'); - return; - } - - try { - const userService = new UserService(); - const existingUser = await userService.getById(body.user.id); - - if (existingUser) { - log.info('User already exists in users_list', { userId: existingUser.id }); - return; - } - - const userInfo = await client.users.info({ user: body.user.id }); - if (!userInfo.user) { - log.error('Failed to get info about user', { userId: body.user.id }); - return; - } - - const user = { - id: body.user.id, - name: userInfo.user.real_name || userInfo.user.name || 'Unknown', - }; - - await userService.addUser(user); - log.info('User saved to users_list', { userId: user.id }); - } catch { - log.error('Failed to save user to users_list'); - } + async function persistUserSafely() { + if (!client?.users?.info) { + log.debug('Slack client missing users.info; skipping user persistence', { userId: body.user.id } as LoggerContext); + return; + } + try { + const userService = new UserService(); + const existingUser = await userService.getById(body.user.id); + if (existingUser) { + log.debug('User already exists in users_list', { userId: existingUser.id }); + return; + } + const userInfo = await client.users.info({ user: body.user.id }); + if (!userInfo.user) { + log.warn('Failed to get info about user', { userId: body.user.id }); + return; + } + const user = { + id: body.user.id, + name: userInfo.user.real_name || userInfo.user.name || 'Unknown', + }; + await userService.addUser(user); + log.info('User saved to users_list', { userId: user.id }); + } catch (e) { + log.warn('Failed to save user to users_list', { userId: body.user.id, error: String(e) }); + } + }Also applies to: 147-176
1-176: Ensure early return on invalid/deleted option and guarantee user persistence in finally
Add areturnimmediately after posting the'This option is no longer available for voting.'ephemeral message inside theif (!option || option.deleted)block in functions/src/handlers/voteHandler.ts. Then relocate or wrap the user-persistence logic (the calls toUserService.getById/addUser) into thefinallyblock (or otherwise ensure it always runs despite any earlier returns).
♻️ Duplicate comments (1)
functions/src/handlers/voteHandler.ts (1)
152-160: Nice: DB lookup before Slack API call.
Addresses earlier feedback to reduce remote calls.
🧹 Nitpick comments (6)
functions/src/handlers/voteHandler.ts (6)
105-110: Graceful handling when updated poll cannot be fetched.
Return is fine; optional: ephemeral to inform the user.
147-151: Add context to the client-guard log.
Include userId for easier tracing; also prefer debug level here.If you don’t move to finally, tweak this line:
- log.info('Slack client missing users.info'); + log.debug('Slack client missing users.info', { userId: body.user.id } as LoggerContext);
174-176: Log error details on persistence failure.
Current catch hides the error root cause.Apply this diff:
-} catch { - log.error('Failed to save user to users_list'); +} catch (e) { + log.warn('Failed to save user to users_list', { userId: body.user.id, error: String(e) }); }
44-49: Consider guarding JSON.parse to avoid noisy error logs.
Outer try/catch covers it, but a local try/catch lets you return a clean ephemeral message for malformed actions.
161-170: Slack user name fallback is reasonable.
Optionally consider profile.display_name as another fallback.
118-125: Optional: don’t return after postMessage so user persistence runs.
If you don’t move persistence to finally, drop this return.await client.chat.postMessage({ channel: channelId, text: `Poll: ${updatedPoll.question}`, blocks: updatedBlocks, }); - return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
functions/src/handlers/voteHandler.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
functions/src/handlers/voteHandler.ts (3)
functions/src/utils/logger.ts (1)
log(85-108)functions/src/services/userService.ts (1)
UserService(4-16)functions/src/services/pollService.ts (1)
PollService(5-78)
🔇 Additional comments (4)
functions/src/handlers/voteHandler.ts (4)
7-7: LGTM: UserService import is appropriate and used.
61-66: Poll-not-found early return is correct and matches tests.
Keeps flow simple and avoids unnecessary Slack calls.
89-103: Good containment of vote errors with user-facing feedback.
Ephemeral message on failure is user-friendly.
68-76: Closed-poll handling looks solid.
Ephemeral notice + early return is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
functions/src/tests/integration/pollCreation.integration.test.ts (2)
20-31: Make service mocks assertable; add minimal assertions to verify user/channel persistence pathsThe mocks stub dependencies correctly, but nothing verifies that
UserService/ChannelServicewere actually invoked. Import the mocked classes and assert againstmock.instances[0]to ensure the new persistence logic runs. This also guards against regressions.Example (outside the selected lines):
// Add near the other imports import { UserService } from '../../services/userService'; import { ChannelService } from '../../services/channelService'; // In "should create a simple poll..." after await handlePollCommand(args): const userSvcInst = (UserService as unknown as vi.Mock).mock.instances[0]; const chanSvcInst = (ChannelService as unknown as vi.Mock).mock.instances[0]; expect(userSvcInst.getById).toHaveBeenCalledWith(command.user_id); expect(userSvcInst.addUser).toHaveBeenCalled(); // since getById -> null expect(chanSvcInst.getById).toHaveBeenCalledWith(command.channel_id); expect(chanSvcInst.addChannel).toHaveBeenCalled(); // since getById -> null
39-51: Reset all mocks between tests to avoid cross-test leakageClear mock state in
beforeEachso added service mocks don’t leak call counts into subsequent tests.Apply this diff:
beforeEach(() => { + vi.clearAllMocks(); mockPollService = ServiceMocks.createMockPollService(); vi.mocked(PollService).mockImplementation(() => mockPollService as unknown as PollService);
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores