Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI (add)"
participant Contacts as "/getContacts API"
participant Firestore as "Firestore"
participant Output as "CLI Output"
User->>CLI: partiful cohosts <eventId> --name "Jane Doe" --user-id "user123"
CLI->>Firestore: GET event doc -> cohostIds
Firestore-->>CLI: cohostIds array
CLI->>Contacts: POST /getContacts (resolve names)
Contacts-->>CLI: contact records (userId, name)
CLI->>CLI: Merge & de-duplicate IDs
alt Dry-run
CLI-->>Output: JSON payload (no write)
else Apply
CLI->>Firestore: PATCH event with updated cohostIds
Firestore-->>CLI: Success
CLI-->>Output: { eventId, added, total, url }
end
sequenceDiagram
actor User
participant CLI as "CLI (list)"
participant Firestore as "Firestore"
participant Contacts as "/getContacts API"
participant Output as "CLI Output"
User->>CLI: partiful cohosts list <eventId>
CLI->>Firestore: GET event doc -> cohostIds
Firestore-->>CLI: cohostIds array
alt No cohosts
CLI-->>Output: { eventId, count: 0, cohosts: [] }
else Resolve names
CLI->>Contacts: POST /getContacts (fetch contacts for IDs)
Contacts-->>CLI: contact records
CLI->>CLI: Map IDs -> { userId, name|null }
CLI-->>Output: { eventId, count, cohosts: [...] }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/commands/cohosts.js (2)
72-108: No-op PATCH when all names fail to resolve.If the user provides only
--namevalues and all fail to resolve,newIdsequalscurrentIds, resulting in a redundant Firestore PATCH that writes the same data back. Consider checking if any new IDs were actually added before patching.💡 Suggested improvement
+ const added = newIds.filter(id => !currentIds.includes(id)); + if (added.length === 0) { + jsonOutput({ eventId, added: [], total: currentIds.length, message: 'No new co-hosts to add' }); + return; + } + const fields = { cohostIds: { arrayValue: { values: newIds.map(id => ({ stringValue: id })) } } }; if (globalOpts.dryRun) {Move the
addedcomputation before the PATCH and exit early if nothing changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/cohosts.js` around lines 72 - 108, The code may issue a no-op Firestore PATCH when all provided --name values fail to resolve because newIds will equal currentIds; before calling firestoreRequest (and before the dryRun jsonOutput if you prefer), compute whether any IDs were actually added (e.g., compare currentIds and newIds lengths or compute a difference from currentIds) and exit early if nothing changed; update the logic around newIds/currentIds resolution (the block that handles opts.name and opts.userId) to set a flag or compute an addedIds array and only proceed to jsonOutput/firestoreRequest when addedIds is non-empty.
67-70: Read-modify-write pattern has potential for race conditions.Both
addandremoveuse a GET-then-PATCH pattern without Firestore transactions or preconditions. Concurrent operations on the same event'scohostIdscould overwrite each other's changes. This is acceptable for a CLI tool with low concurrency expectations, but worth documenting for users who might script parallel invocations.Also applies to: 134-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/cohosts.js` around lines 67 - 70, The GET-then-PATCH read-modify-write for cohostIds (where eventDoc, currentField and currentIds are read) can race with concurrent updates; change the update to use a Firestore conditional update or transaction instead of a blind PATCH: when you fetch eventDoc capture its updateTime and then perform the PATCH with a precondition (currentDocument.updateTime or existence precondition) or switch to a Firestore transaction API to read-modify-write atomically in the add/remove command handlers so concurrent CLI invocations won't clobber each other.src/commands/events.js (1)
335-350: Consider extracting cohost resolution to a shared helper.The contact-to-userId resolution logic (fetch contacts, case-insensitive exact match, then substring fallback) is duplicated in
events create,events update,events clone, andcohosts add. Extracting this to a helper (e.g., insrc/lib/contacts.jsor similar) would reduce duplication and ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/events.js` around lines 335 - 350, The co-host resolution logic is duplicated; extract it into a shared helper (e.g., resolveCohostNames or getCohostIds) and replace the inline code in events create/update/clone and cohosts add with calls to that helper. The helper should accept (optsCohostArray, config, token, globalOpts.verbose) and internally perform the contacts fetch via apiRequest('/getContacts', ...) using wrapPayload, apply the case-insensitive exact match then substring fallback, return an array of resolved userIds and unresolved names (so callers can emit the same warning currently written to process.stderr), and be exported from a new or existing file like src/lib/contacts.js for reuse across the referenced commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/events.js`:
- Around line 762-777: The co-host resolution loop (using opts.cohost,
cohostIds, getContacts via apiRequest and wrapPayload) can add duplicate
userIds; update the logic to avoid duplicates by checking before push (e.g., use
a Set or if (!cohostIds.includes(match.userId)) cohostIds.push(match.userId)) or
deduplicate cohostIds after the loop so the final cohostIds array contains
unique IDs only, mirroring the fix applied in the clone command.
- Around line 335-350: The co-host resolution loop can push duplicate user IDs
into cohostIds when the same name or different names map to the same contact;
update the block that iterates opts.cohost (the code that builds cohostIds after
calling apiRequest('/getContacts') and wrapPayload) to check for existence
before pushing (e.g., only push match.userId if
!cohostIds.includes(match.userId)), so resolved IDs are deduplicated and avoid
duplicate entries; keep the existing warning behavior for unresolved names.
- Around line 532-552: In the events update block handling opts.cohost, avoid
pushing duplicate userIds into resolvedIds by deduplicating as you resolve
matches (e.g., track already-added IDs with a Set or check resolvedIds.includes
before push) so the final fields.cohostIds.arrayValue.values contains unique
stringValue entries; ensure this mirrors the dedup logic used by the cohosts add
flow and still pushes 'cohostIds' to updateFields only when there is at least
one unique id.
---
Nitpick comments:
In `@src/commands/cohosts.js`:
- Around line 72-108: The code may issue a no-op Firestore PATCH when all
provided --name values fail to resolve because newIds will equal currentIds;
before calling firestoreRequest (and before the dryRun jsonOutput if you
prefer), compute whether any IDs were actually added (e.g., compare currentIds
and newIds lengths or compute a difference from currentIds) and exit early if
nothing changed; update the logic around newIds/currentIds resolution (the block
that handles opts.name and opts.userId) to set a flag or compute an addedIds
array and only proceed to jsonOutput/firestoreRequest when addedIds is
non-empty.
- Around line 67-70: The GET-then-PATCH read-modify-write for cohostIds (where
eventDoc, currentField and currentIds are read) can race with concurrent
updates; change the update to use a Firestore conditional update or transaction
instead of a blind PATCH: when you fetch eventDoc capture its updateTime and
then perform the PATCH with a precondition (currentDocument.updateTime or
existence precondition) or switch to a Firestore transaction API to
read-modify-write atomically in the add/remove command handlers so concurrent
CLI invocations won't clobber each other.
In `@src/commands/events.js`:
- Around line 335-350: The co-host resolution logic is duplicated; extract it
into a shared helper (e.g., resolveCohostNames or getCohostIds) and replace the
inline code in events create/update/clone and cohosts add with calls to that
helper. The helper should accept (optsCohostArray, config, token,
globalOpts.verbose) and internally perform the contacts fetch via
apiRequest('/getContacts', ...) using wrapPayload, apply the case-insensitive
exact match then substring fallback, return an array of resolved userIds and
unresolved names (so callers can emit the same warning currently written to
process.stderr), and be exported from a new or existing file like
src/lib/contacts.js for reuse across the referenced commands.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8f03516-3ef6-4463-96b5-6391f9f9e375
📒 Files selected for processing (4)
src/cli.jssrc/commands/cohosts.jssrc/commands/events.jssrc/commands/schema.js
| if (opts.cohost && opts.cohost.length > 0) { | ||
| const contactsPayload = { data: wrapPayload(config, { params: {}, amplitudeSessionId: Date.now(), userId: config.userId }) }; | ||
| const contactsResult = await apiRequest('POST', '/getContacts', token, contactsPayload, globalOpts.verbose); | ||
| const allContacts = contactsResult.result?.data || []; | ||
| const resolvedIds = []; | ||
| for (const name of opts.cohost) { | ||
| const q = name.toLowerCase(); | ||
| const match = allContacts.find(c => (c.name || '').toLowerCase() === q) || allContacts.find(c => (c.name || '').toLowerCase().includes(q)); | ||
| if (match && match.userId) { | ||
| resolvedIds.push(match.userId); | ||
| } else { | ||
| process.stderr.write(`Warning: could not resolve co-host "${name}" from contacts — skipping\n`); | ||
| } | ||
| } | ||
| if (resolvedIds.length > 0) { | ||
| fields.cohostIds = { | ||
| arrayValue: { values: resolvedIds.map(id => ({ stringValue: id })) } | ||
| }; | ||
| updateFields.push('cohostIds'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same deduplication gap in update command.
The events update cohost resolution also lacks duplicate checking, inconsistent with cohosts add.
🔧 Proposed fix
if (match && match.userId) {
- resolvedIds.push(match.userId);
+ if (!resolvedIds.includes(match.userId)) resolvedIds.push(match.userId);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/events.js` around lines 532 - 552, In the events update block
handling opts.cohost, avoid pushing duplicate userIds into resolvedIds by
deduplicating as you resolve matches (e.g., track already-added IDs with a Set
or check resolvedIds.includes before push) so the final
fields.cohostIds.arrayValue.values contains unique stringValue entries; ensure
this mirrors the dedup logic used by the cohosts add flow and still pushes
'cohostIds' to updateFields only when there is at least one unique id.
src/commands/events.js
Outdated
| // Resolve co-host names to IDs | ||
| const cohostIds = []; | ||
| if (opts.cohost && opts.cohost.length > 0) { | ||
| const contactsPayload = { data: wrapPayload(config, { params: {}, amplitudeSessionId: Date.now(), userId: config.userId }) }; | ||
| const contactsResult = await apiRequest('POST', '/getContacts', token, contactsPayload, globalOpts.verbose); | ||
| const allContacts = contactsResult.result?.data || []; | ||
| for (const name of opts.cohost) { | ||
| const q = name.toLowerCase(); | ||
| const match = allContacts.find(c => (c.name || '').toLowerCase() === q) || allContacts.find(c => (c.name || '').toLowerCase().includes(q)); | ||
| if (match && match.userId) { | ||
| cohostIds.push(match.userId); | ||
| } else { | ||
| process.stderr.write(`Warning: could not resolve co-host "${name}" from contacts — skipping\n`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same deduplication gap in clone command.
Apply the same fix here for consistency.
🔧 Proposed fix
if (match && match.userId) {
- cohostIds.push(match.userId);
+ if (!cohostIds.includes(match.userId)) cohostIds.push(match.userId);
} else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/events.js` around lines 762 - 777, The co-host resolution loop
(using opts.cohost, cohostIds, getContacts via apiRequest and wrapPayload) can
add duplicate userIds; update the logic to avoid duplicates by checking before
push (e.g., use a Set or if (!cohostIds.includes(match.userId))
cohostIds.push(match.userId)) or deduplicate cohostIds after the loop so the
final cohostIds array contains unique IDs only, mirroring the fix applied in the
clone command.
5b197c1 to
ebf533b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/cohosts.js (1)
54-60: Defensively de-duplicatecohostIdsbefore Firestore writes.Right now duplicates can be persisted if upstream callers pass repeated IDs. Centralizing dedupe here makes shared helper behavior safer.
Proposed patch
export async function setCohostIds(eventId, ids, token, verbose = false) { + const uniqueIds = [...new Set((ids || []).filter(Boolean))]; const fields = { cohostIds: { - arrayValue: { values: ids.map(id => ({ stringValue: id })) }, + arrayValue: { values: uniqueIds.map(id => ({ stringValue: id })) }, }, }; await firestoreRequest('PATCH', eventId, { fields }, token, ['cohostIds'], verbose); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/cohosts.js` around lines 54 - 60, In setCohostIds, defensively deduplicate the incoming ids before constructing the Firestore payload: compute a dedupedIds array (preserving original order) from the ids parameter (e.g., via a Set or a keyed filter) and use dedupedIds when building fields.arrayValue.values and when calling firestoreRequest; ensure the variable names (ids → dedupedIds) are used in both the fields object and the subsequent firestoreRequest invocation so duplicate cohost IDs are never written.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/cohosts.js`:
- Around line 35-38: The current mapping that builds result only returns {
userId, name } which omits required identifiers and metadata; update the mapping
in the result construction (the ids.map callback referencing ids, allContacts,
contact) to include additional fields such as username (contact.username ||
null), phone (contact.phone || null), host (contact.host || false or
contact.isHost), and addedAt (contact.addedAt || null) or pull added-at from the
cohosts record if stored elsewhere; ensure each property uses safe fallbacks
(null/false) so the cohosts list returns the richer identifier set and metadata
expected by the command.
- Line 26: Several jsonOutput(...) calls in this file are invoked without
passing the global options, so global flags like --output are ignored; update
every jsonOutput invocation (e.g., the calls that currently pass option objects
like { eventId, count: 0 }) to merge in the globalOpts before the local keys
(use the spread operator or Object.assign, e.g. jsonOutput(data, {
...globalOpts, eventId, count: 0 }) or jsonOutput(data, Object.assign({},
globalOpts, { eventId, count: 0 }))). Locate all uses of jsonOutput in this
module (including those around the cohost command handlers) and ensure each
second-argument options object includes globalOpts merged with the existing
properties.
- Around line 51-52: The cohosts add command only accepts --name and --user-id;
add two new CLI options .option('--phone <phones...>', 'Co-host phone numbers')
and .option('--username <usernames...>', 'Co-host Partiful usernames') to the
same command definition where .option('--name <names...>') and
.option('--user-id <userIds...>') are declared, then update the cohosts add
handler (the function that processes the add command / addCohosts handler) to
accept and normalize inputs from phone and username, resolve them into the same
internal cohost representation as names/userIds, validate duplicates, and pass
them into the existing creation/resolution logic so phone/username flows behave
equivalently to name/user-id paths.
---
Nitpick comments:
In `@src/lib/cohosts.js`:
- Around line 54-60: In setCohostIds, defensively deduplicate the incoming ids
before constructing the Firestore payload: compute a dedupedIds array
(preserving original order) from the ids parameter (e.g., via a Set or a keyed
filter) and use dedupedIds when building fields.arrayValue.values and when
calling firestoreRequest; ensure the variable names (ids → dedupedIds) are used
in both the fields object and the subsequent firestoreRequest invocation so
duplicate cohost IDs are never written.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0e70bd1-e3f0-485f-a86c-3dfd212484e2
📒 Files selected for processing (5)
src/cli.jssrc/commands/cohosts.jssrc/commands/events.jssrc/commands/schema.jssrc/lib/cohosts.js
✅ Files skipped from review due to trivial changes (1)
- src/cli.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/events.js
|
|
||
| const ids = await getCohostIds(eventId, token, globalOpts.verbose); | ||
| if (ids.length === 0) { | ||
| jsonOutput([], { eventId, count: 0 }); |
There was a problem hiding this comment.
Pass global options into every jsonOutput call in this file.
On Line 26, Line 40, Line 79, Line 86, Line 114, and Line 120, jsonOutput is called without globalOpts, so global flags like --output are ignored for cohost commands.
Proposed patch
- jsonOutput([], { eventId, count: 0 });
+ jsonOutput([], { eventId, count: 0 }, globalOpts);
...
- jsonOutput(result, { eventId, count: result.length });
+ jsonOutput(result, { eventId, count: result.length }, globalOpts);
...
- jsonOutput({ dryRun: true, eventId, currentCohosts: currentIds, newCohosts: newIds });
+ jsonOutput({ dryRun: true, eventId, currentCohosts: currentIds, newCohosts: newIds }, {}, globalOpts);
...
- jsonOutput({ eventId, added, total: newIds.length, url: `https://partiful.com/e/${eventId}` });
+ jsonOutput({ eventId, added, total: newIds.length, url: `https://partiful.com/e/${eventId}` }, {}, globalOpts);
...
- jsonOutput({ dryRun: true, eventId, removing: opts.userId, remaining: newIds });
+ jsonOutput({ dryRun: true, eventId, removing: opts.userId, remaining: newIds }, {}, globalOpts);
...
- jsonOutput({ eventId, removed: opts.userId, remaining: newIds.length, url: `https://partiful.com/e/${eventId}` });
+ jsonOutput({ eventId, removed: opts.userId, remaining: newIds.length, url: `https://partiful.com/e/${eventId}` }, {}, globalOpts);Also applies to: 40-40, 79-79, 86-86, 114-114, 120-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/cohosts.js` at line 26, Several jsonOutput(...) calls in this
file are invoked without passing the global options, so global flags like
--output are ignored; update every jsonOutput invocation (e.g., the calls that
currently pass option objects like { eventId, count: 0 }) to merge in the
globalOpts before the local keys (use the spread operator or Object.assign, e.g.
jsonOutput(data, { ...globalOpts, eventId, count: 0 }) or jsonOutput(data,
Object.assign({}, globalOpts, { eventId, count: 0 }))). Locate all uses of
jsonOutput in this module (including those around the cohost command handlers)
and ensure each second-argument options object includes globalOpts merged with
the existing properties.
| const result = ids.map(id => { | ||
| const contact = allContacts.find(c => c.userId === id); | ||
| return { userId: id, name: contact?.name || null }; | ||
| }); |
There was a problem hiding this comment.
cohosts list output is missing required identifying fields/metadata.
Line 35-Line 38 only returns { userId, name }, but the stated objective calls for richer identifiers (e.g., username, phone, host) and metadata (e.g., added-at when available).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/cohosts.js` around lines 35 - 38, The current mapping that
builds result only returns { userId, name } which omits required identifiers and
metadata; update the mapping in the result construction (the ids.map callback
referencing ids, allContacts, contact) to include additional fields such as
username (contact.username || null), phone (contact.phone || null), host
(contact.host || false or contact.isHost), and addedAt (contact.addedAt || null)
or pull added-at from the cohosts record if stored elsewhere; ensure each
property uses safe fallbacks (null/false) so the cohosts list returns the richer
identifier set and metadata expected by the command.
| .option('--name <names...>', 'Co-host names (resolved from contacts)') | ||
| .option('--user-id <userIds...>', 'Co-host user IDs (direct)') |
There was a problem hiding this comment.
cohosts add is missing phone/username input paths.
Only --name and --user-id are supported on Line 51-Line 52. The objective includes adding co-hosts by phone number or Partiful username.
Suggested command-surface extension
cohosts
.command('add')
.description('Add co-hosts to an event')
.argument('<eventId>', 'Event ID')
.option('--name <names...>', 'Co-host names (resolved from contacts)')
+ .option('--phone <phones...>', 'Co-host phone numbers')
+ .option('--username <usernames...>', 'Co-host Partiful usernames')
.option('--user-id <userIds...>', 'Co-host user IDs (direct)')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .option('--name <names...>', 'Co-host names (resolved from contacts)') | |
| .option('--user-id <userIds...>', 'Co-host user IDs (direct)') | |
| .option('--name <names...>', 'Co-host names (resolved from contacts)') | |
| .option('--phone <phones...>', 'Co-host phone numbers') | |
| .option('--username <usernames...>', 'Co-host Partiful usernames') | |
| .option('--user-id <userIds...>', 'Co-host user IDs (direct)') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/cohosts.js` around lines 51 - 52, The cohosts add command only
accepts --name and --user-id; add two new CLI options .option('--phone
<phones...>', 'Co-host phone numbers') and .option('--username <usernames...>',
'Co-host Partiful usernames') to the same command definition where
.option('--name <names...>') and .option('--user-id <userIds...>') are declared,
then update the cohosts add handler (the function that processes the add command
/ addCohosts handler) to accept and normalize inputs from phone and username,
resolve them into the same internal cohost representation as names/userIds,
validate duplicates, and pass them into the existing creation/resolution logic
so phone/username flows behave equivalently to name/user-id paths.
What
Adds co-host management to partiful-cli, closing #24.
Changes
--cohostflag onevents create,update, andclone— accepts contact names, resolves them to user IDs via/getContactsAPI, passes intocohostIdspayload. Unresolved names emit a stderr warning and are skipped.New
cohostssubcommand with three commands:cohosts list <eventId>— reads cohostIds from Firestore, cross-references with contacts for namescohosts add <eventId> --name/--user-id— adds co-hosts via Firestore PATCH (merges with existing)cohosts remove <eventId> --user-id— removes a co-host via Firestore PATCHSchema introspection updated for all new commands/flags.
Patterns followed
--dry-runand--verbosesupported throughoutCloses #24
Summary by CodeRabbit