Port to TypeScript strict + types-as-API-spec (T0–T6) - #66
Conversation
- typescript@7, tsx@4, @types/node devDeps; zod runtime dep - tsconfig: strict + NodeNext ESM + allowJs (JS/TS coexist), noEmit - bin/partiful registers tsx/esm loader, no dist build - scripts: typecheck (tsc --noEmit), start - gate: typecheck clean + 195/195 tests green on still-JS tree
docs/TYPESCRIPT-PORT-GUIDE.md — enforceable file-by-file rules + worked createEvent endpoint example (envelope generic, request interface, Zod passthrough response, z.infer type, introspectable metadata).
- src/lib/output.ts, errors.ts, http.ts ported (strict-clean) - src/lib/api/envelope.ts: CallableEnvelope<P> + CallableResult<D> generics - src/lib/api/endpoints.ts: THE SPEC — request interfaces + Zod .passthrough() response schemas + z.infer types + introspectable metadata registry for all 11 callable endpoints, 3 firestore ops, token refresh - bin/partiful: register tsx loader then dynamic-import CLI (fixes ESM hoist) - first-slice oracle met: http+createEvent typed, strict clean, 195/195 green
…auth, cohosts, upload, rsvp, events) All 8 remaining lib modules ported. Wired into the api/ spec types (EventDraft, RsvpDraft, PartifulConfig, endpoint request/response types). tsc --noEmit clean; 195/195 tests green. src/lib/ is now 100% TypeScript.
All 18 remaining JS files ported (12 commands, 4 helpers, cli.ts, schema.ts). src/ is now 100% TypeScript. Commander handlers typed; API responses narrowed via api/ spec types + as-casts. tsc --noEmit clean; 195/195 green; ./bin/partiful --version + schema smoke-tested via tsx loader.
Adds 'schema api' (list endpoints) and 'schema api.<method>' (per-endpoint spec: transport, host, httpMethod, path, requestParams, responseFields) driven off the apiEndpoints registry in api/endpoints.ts. Bare 'schema' now also lists api.* methods. Existing 'schema <command>' CLI-flag lookup unchanged. +5 tests (tests/schema-api.test.js). 200/200 green; tsc clean.
- src/lib/drift.ts: diff passthrough responses vs spec's declared field surface; detectDrift() + reportDrift(). Opt-in logging via PARTIFUL_DRIFT_LOG (unset=silent, 1/true/stderr=stderr line, path=NDJSON append). - Wired centrally + guarded into apiRequest() via path->method reverse map + envelope unwrap; advisory only, never breaks a request. - src/lib/api/endpoints.ts: export responseSchemas registry (method->schema). - tests/drift.test.js: 6 unit tests (no auth needed, run everywhere). - tests/smoke-real-api.test.js: live-API spec verifier, skipIf(!PARTIFUL_SMOKE), read-only endpoints, documented run instructions. - docs/TYPESCRIPT-PORT-GUIDE.md \u00a710: implemented drift+smoke strategy, CI-vs-manual. tsc clean; 206 passed / 6 smoke skipped.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe PR establishes a TypeScript migration workflow, ports CLI and library modules, centralizes typed API contracts and transports, adds schema introspection and drift detection, and introduces supporting unit, smoke, and helper tests. It also documents the explore and RSVP endpoint recon and command design. ChangesTypeScript migration and API contracts
Explore and RSVP design
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (11)
docs/explore-command-design.md-119-123 (1)
119-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the wire tag IDs documented by the API recon.
DISCOVER_MUSICandDISCOVER_FOODdo not match ticket 03’s verifiedtagIdvaluesMUSICandFOOD. If this example becomes the CLI’s advertised contract, users will send invalid filters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/explore-command-design.md` around lines 119 - 123, Update the tags example to use the API-verified wire IDs MUSIC and FOOD for the Music and Food & Drink entries, while preserving the existing display names and DISCOVER_HOME entry..wayfinder/ts-port/tickets/T0-rsvp-merged-branch-cut.md-3-3 (1)
3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSynchronize all ticket statuses with
.wayfinder/ts-port/map.md.The map records T0, T3, T4, T5, and T6 as closed on July 24, 2026, but their ticket metadata still says
OPEN; T5 and T6 additionally contradict their own closed Answer sections.
.wayfinder/ts-port/tickets/T0-rsvp-merged-branch-cut.md#L3-L3: mark T0 closed and fill in the merge/branch-cut commit..wayfinder/ts-port/tickets/T3-port-lib-layer-spec.md#L3-L3: mark T3 closed and record the completed API/spec work..wayfinder/ts-port/tickets/T4-port-commands-helpers.md#L3-L3: mark T4 closed and record the completed command/helper port..wayfinder/ts-port/tickets/T5-rewire-schema-command.md#L3-L3: changeOPENto the closed status matching its Answer..wayfinder/ts-port/tickets/T6-drift-detection-smoke-tests.md#L3-L3: changeOPENto the closed status matching its Answer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.wayfinder/ts-port/tickets/T0-rsvp-merged-branch-cut.md at line 3, Synchronize ticket metadata with the closed statuses in .wayfinder/ts-port/map.md: update .wayfinder/ts-port/tickets/T0-rsvp-merged-branch-cut.md:3 to mark T0 closed and add its merge/branch-cut commit; update .wayfinder/ts-port/tickets/T3-port-lib-layer-spec.md:3 and .wayfinder/ts-port/tickets/T4-port-commands-helpers.md:3 to mark them closed and record their completed work; update .wayfinder/ts-port/tickets/T5-rewire-schema-command.md:3 and .wayfinder/ts-port/tickets/T6-drift-detection-smoke-tests.md:3 from OPEN to the closed status used by their Answer sections.src/lib/drift.ts-95-108 (1)
95-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
forceoverrides the sink instead of just enabling logging.With
PARTIFUL_DRIFT_LOG=/tmp/drift.ndjsonandforce = true,|| forceshort-circuits to the stderr branch,lineis built and discarded, and nothing is appended to the configured file. Separate "is logging enabled" from "where does it go".🐛 Proposed fix
if (enabled) { const line = JSON.stringify({ drift: record }); - if (!sink || sink === '1' || sink === 'true' || sink === 'stderr' || force) { + const isFileSink = + sink != null && sink !== '' && !['0', '1', 'true', 'false', 'stderr'].includes(sink); + if (!isFileSink) { console.error(`[drift] ${method}: unknown fields ${unknownFields.join(', ')}`); } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/drift.ts` around lines 95 - 108, Update the logging destination condition in the drift logging flow around PARTIFUL_DRIFT_LOG so force only enables logging and does not override a configured file sink. When force is true with a non-empty, non-special sink path, append the serialized line to that file; reserve the stderr branch for unset or explicitly stderr-configured sinks, while preserving existing write-failure handling.src/commands/doctor.ts-39-40 (1)
39-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
replace(home, '~')corrupts the path whenHOMEis unset.
''matches at index 0, so'/etc/partiful/auth.json'.replace('', '~')returns'~/etc/partiful/auth.json'— the doctor output then points at a path that does not exist. A prefix check avoids both this and mid-path substitutions.🐛 Proposed fix
- const home = process.env['HOME'] ?? ''; - const displayPath = configPath.replace(home, '~'); + const home = process.env['HOME'] || os.homedir(); + const displayPath = + home && configPath.startsWith(home) ? `~${configPath.slice(home.length)}` : configPath;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/doctor.ts` around lines 39 - 40, Update the displayPath construction near the home and configPath values to replace HOME with "~" only when HOME is non-empty and configPath actually starts with it; otherwise preserve configPath unchanged. Use a prefix check rather than String.replace to prevent unset HOME and mid-path substitutions.src/lib/auth.ts-33-38 (1)
33-38: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
process.env.HOME as stringhides a real crash path.When
HOMEis unset (Windows, some CI/daemon contexts),path.join(undefined, …)throwsTypeError: The "path" argument must be of type string, and the cast means tsc no longer warns.os.homedir()resolves this on every platform.🛡️ Proposed fix
+import os from 'os'; + export function resolveCredentialsPath(): string { return ( process.env.PARTIFUL_CREDENTIALS_FILE || - path.join(process.env.HOME as string, '.config/partiful/auth.json') + path.join(process.env.HOME || os.homedir(), '.config/partiful/auth.json') ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/auth.ts` around lines 33 - 38, Update resolveCredentialsPath to use os.homedir() instead of process.env.HOME as string when constructing the default credentials path, importing the required os module while preserving the PARTIFUL_CREDENTIALS_FILE override.src/commands/events.ts-37-40 (1)
37-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnchecked
(e as Error).messagein catch paths dropsmessagefrom the error envelope for non-Errorthrows. The port replacede.messagewith a type assertion rather than a runtime guard, so a thrown string/object yieldsundefinedandJSON.stringifyomits the key, emitting{"status":"error","error":{"code":5,"type":"internal_error"}}.src/commands/posters.tsalready uses the correctinstanceofform; apply it uniformly.
src/commands/events.ts#L37-L40: inhandleError, replace(e as Error).messagewithe instanceof Error ? e.message : String(e).src/commands/guests.ts#L123-L125: apply the same guard in theguests listcatch block.src/commands/guests.ts#L181-L183: apply the same guard in theguests invitecatch block.src/commands/rsvp.ts#L40-L43: apply the same guard in the RSVPhandleError.src/helpers/watch.ts#L87-L89: apply the same guard in the+watchcatch block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/events.ts` around lines 37 - 40, Replace the unsafe error type assertions with an Error runtime check and String fallback so every catch path preserves a message for non-Error throws. Update handleError in src/commands/events.ts (lines 37-40), both guests catch blocks in src/commands/guests.ts (lines 123-125 and 181-183), handleError in src/commands/rsvp.ts (lines 40-43), and the +watch catch block in src/helpers/watch.ts (lines 87-89); each site requires the same guarded message handling.src/lib/output.ts-55-65 (1)
55-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
escapemisses\r, and emptyrowsproduces a stray blank line.A cell containing a bare CR (no LF) is emitted unquoted and breaks row parsing in strict CSV readers; guest/contact names come from the API, so this is externally influenced. Also
formatCsv([], cols)returnsheader + '\n'with an empty body row, unlikeformatTable's(no results)handling.🛠️ Proposed fix
export function formatCsv(rows: TableRow[], columns: string[]): string { const escape = (v: unknown): string => { const s = String(v ?? ''); - return s.includes(',') || s.includes('"') || s.includes('\n') + return /[",\r\n]/.test(s) ? `"${s.replace(/"/g, '""')}"` : s; }; const header = columns.map(escape).join(','); + if (rows.length === 0) return header; const body = rows.map((r) => columns.map((col) => escape(r[col])).join(',')).join('\n'); return `${header}\n${body}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/output.ts` around lines 55 - 65, Update formatCsv’s escape helper to quote values containing carriage returns in addition to commas, quotes, and line feeds. Adjust the output assembly so empty rows return only the escaped header without a trailing newline, while preserving the existing header-and-body format for non-empty rows.src/commands/contacts.ts-48-49 (1)
48-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--limitisn't validated, so a non-numeric value silently returns zero contacts.
parseInt('abc')→NaN, andslice(0, NaN)yields[]withcount: 0and no error.posters list/posters searchalready reject this explicitly; worth matching here.🛠️ Proposed fix
- const limit = opts['limit'] as number; + const limit = opts['limit'] as number; + if (!Number.isInteger(limit) || limit < 1) { + jsonError('--limit must be a positive integer', 3, 'validation_error'); + return; + } contactList = contactList.slice(0, limit);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/contacts.ts` around lines 48 - 49, Validate the `opts['limit']` value before applying `contactList.slice` in the contacts command, rejecting non-numeric values with the same error behavior used by `posters list` and `posters search`. Only call `slice` after validation succeeds, preserving the existing limit behavior for valid values.src/commands/events.ts-229-260 (1)
229-260: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the series flags real
events createoptions.
events createhas no declared--repeat/--countoption, and template merging ignores them (mergeTemplateOptsonly handlesTEMPLATE_FIELDS). The only registration is insideregisterBulkCommands, which merely touches the already-registered command rather than exposing these flags. Add them toevents createso users can actually request series creation (or remove the unreachable support).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/events.ts` around lines 229 - 260, The series logic in the events create flow is unreachable because repeat and count are not declared options. Update the events create command registration to expose --repeat and --count, and ensure template option merging preserves them alongside TEMPLATE_FIELDS; keep the existing series handling and validation behavior unchanged.src/commands/guests.ts-79-93 (1)
79-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--dry-runissues a live/getEventInforequest before the dry-run guard.The metadata fetch at line 80 runs unconditionally, so
guests list --dry-runisn't the offline preview the global flag advertises. Move the dry-run branch above the fetch.🛠️ Proposed fix
+ if (globalOpts['dryRun']) { + jsonOutput({ dryRun: true, eventId, collection: `events/${eventId}/guests` }); + return; + } + let counts: Record<string, number> = {}; let eventTitle = 'Unknown Event'; try { @@ } catch { // API may be down, continue with Firestore guest fetch } - - if (globalOpts['dryRun']) { - jsonOutput({ dryRun: true, eventId, collection: `events/${eventId}/guests` }); - return; - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/guests.ts` around lines 79 - 93, The dry-run guard currently executes after the live /getEventInfo request. In the guests command flow, move the globalOpts['dryRun'] branch before the try block that calls apiRequest, preserving its existing jsonOutput response and return so dry-run exits without any network request.src/helpers/share.ts-5-16 (1)
5-16: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
+sharedrops--verbosesupport (hardcodedfalse).Every sibling command in this PR threads
cmd.optsWithGlobals<Record<string, unknown>>()['verbose']into itsapiRequestcalls. Here the callback parameters were renamed to_opts/_cmdand the verbose flag is hardcoded tofalse, sopartiful +share <id> -vsilently loses the request-detail stderr output that every other command still provides.🔧 Restore verbose threading
- .action(async (eventId: string, _opts: Record<string, unknown>, _cmd: Command) => { + .action(async (eventId: string, _opts: Record<string, unknown>, cmd: Command) => { + const globalOpts = cmd.optsWithGlobals<Record<string, unknown>>(); try { const config = loadConfig(); const token = await getValidToken(config); @@ - const result = await apiRequest('POST', '/getEventInfo', token, payload, false) as Record<string, unknown>; + const result = await apiRequest('POST', '/getEventInfo', token, payload, globalOpts['verbose'] as boolean | undefined) as Record<string, unknown>;Also applies to: 30-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/share.ts` around lines 5 - 16, Restore verbose option propagation in registerShareHelper: use the action’s Command parameter to read optsWithGlobals<Record<string, unknown>>() and pass its verbose value to each apiRequest call instead of hardcoding false. Preserve the existing share-link behavior and update the callback parameter names as needed to access the command.
🧹 Nitpick comments (13)
.wayfinder/BUILD-PROMPT.md (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the migration baseline or mark it as historical.
Line 4 still describes the repository as plain JavaScript with no TypeScript, which conflicts with the current TypeScript strict-mode port and
tsxruntime. Future agents may follow the obsolete setup instructions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.wayfinder/BUILD-PROMPT.md at line 4, Update the repository setup description in BUILD-PROMPT.md to reflect the current TypeScript strict-mode port and tsx runtime instead of describing the project as plain JavaScript with no build step. Preserve the remaining Commander.js, command organization, API access, testing, and global-install guidance unless those details are also outdated..wayfinder/map.md (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefresh the repository/runtime description for the TypeScript port.
This map still says the project is plain JavaScript with no build step. Update it to describe the TypeScript source,
tsxESM execution, andtsc --noEmit, or explicitly label these lines as historical context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.wayfinder/map.md around lines 17 - 20, Update the repository/runtime description in the map to reflect the TypeScript port: identify TypeScript as the source language, document tsx-based ESM execution, and mention tsc --noEmit validation. Alternatively, explicitly mark the existing Commander.js/plain-JavaScript/no-build-step statements as historical context while preserving accurate current guidance about API access and global installation.package.json (1)
13-13: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid registering tsx twice in the start path.
package.jsonpreloads tsx vianode --import tsx, whilebin/partifulalso callsregister()fromtsx/esm/api. Keep one loader mechanism—for example, usenode bin/partifulhere and retain the bin-level registration—to avoid duplicate loader setup. Official tsx documentation presents these as alternative entrypoint-registration approaches; applying both here is an inferred conflict risk. (tsx.is)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 13, Update the package.json start script to remove the node --import tsx preload and invoke bin/partiful directly, retaining the existing tsx/esm/api register() call in the bin entrypoint as the sole loader mechanism.src/lib/api/endpoints.ts (2)
331-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: derive the Firestore paths from a shared project constant.
getpartifulis hardcoded in three paths here and again asFIRESTORE_PROJECTinsrc/lib/http.ts:12; the two can silently diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/api/endpoints.ts` around lines 331 - 452, The Firestore project identifier is duplicated across the endpoint definitions and the FIRESTORE_PROJECT constant. Update firestoreGetEvent, firestorePatchEvent, and firestoreListDocuments to derive their paths from the shared FIRESTORE_PROJECT value, preserving the existing endpoint paths while preventing future divergence.
115-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the Zod 4 object constructors over the legacy
.passthrough()chain.On zod 4.4.3,
.passthrough()is deprecated in favor ofz.looseObject()/.loose(), and the legacy methods remain only for backwards compatibility. Same applies toz.ZodTypeAnyat Line 323 and Line 480 — the need forz.ZodTypeAnyhas been eliminated; just usez.ZodType. Purely idiomatic, no behavior change, but worth doing once now that every schema in this file is authored in one place.♻️ Illustrative change (apply file-wide)
-export const CreateEventResponseSchema = z - .object({ - id: z.string(), - title: z.string().optional(), - status: z.string().optional(), - startDate: z.string().optional(), - }) - .passthrough(); +export const CreateEventResponseSchema = z.looseObject({ + id: z.string(), + title: z.string().optional(), + status: z.string().optional(), + startDate: z.string().optional(), +});-function fieldsOf(schema: z.ZodTypeAny): readonly string[] { +function fieldsOf(schema: z.ZodType): readonly string[] {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/api/endpoints.ts` around lines 115 - 124, Update CreateEventResponseSchema and the other schemas in this file to use Zod 4 constructors: replace legacy .passthrough() usage with z.looseObject() or .loose() while preserving unknown-key behavior, and replace z.ZodTypeAny references near the identified locations with z.ZodType. Apply these idiomatic updates consistently across the file without changing schema behavior.src/lib/auth.ts (1)
62-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEncode the form body/query and bound the refresh call with a timeout.
refresh_tokenandapiKeyare interpolated raw into the body and query string;URLSearchParamsremoves the encoding hazard. This is also the only outbound call in the auth path with no abort timeout (src/lib/upload.tsuses one), so a stalled token endpoint hangs the CLI indefinitely.♻️ Proposed change
- const postData = `grant_type=refresh_token&refresh_token=${config.refreshToken}`; + const postData = new URLSearchParams({ + grant_type: 'refresh_token', + refresh_token: config.refreshToken ?? '', + }).toString(); - const resp = await fetch(`https://${GOOGLE_TOKEN_URL}/v1/token?key=${config.apiKey}`, { + const url = `https://${GOOGLE_TOKEN_URL}/v1/token?key=${encodeURIComponent(config.apiKey ?? '')}`; + const resp = await fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', Referer: 'https://partiful.com/', }, body: postData, + signal: AbortSignal.timeout(15000), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/auth.ts` around lines 62 - 72, Update refreshAccessToken to build the token request body and API-key query parameters with URLSearchParams rather than raw interpolation, and add an AbortController-based timeout to the fetch matching the existing upload timeout pattern. Pass the controller’s signal to fetch and ensure the timeout is cleared after completion.src/lib/http.ts (1)
121-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnguarded
JSON.parseleaks a rawSyntaxErrorpast the error classifier.A 200 response with an HTML body (proxy/captive portal) escapes as
SyntaxError: Unexpected token '<'instead of a classifiedApiError, bypassing the CLI's structured error envelope.src/lib/upload.ts:106-111already handles this case; the two Firestore readers below have the same gap.♻️ Proposed change
const text = await resp.text(); - const parsed = text ? JSON.parse(text) : {}; + let parsed: unknown = {}; + if (text) { + try { + parsed = JSON.parse(text); + } catch { + throw classifyError(resp.status, `API ${method} ${endpoint} returned non-JSON body`, text.slice(0, 200)); + } + } checkDrift(endpoint, parsed); return parsed;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/http.ts` around lines 121 - 124, Update the response parsing flow around checkDrift in the HTTP reader to catch JSON.parse failures and convert them through the existing ApiError classification path, matching the handling already used in upload.ts. Apply the same protection to both Firestore readers, while preserving successful JSON parsing and the existing empty-body default.src/lib/drift.ts (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne Zod key-enumeration helper implemented twice, with different capabilities.
knownKeysunwraps array elements;fieldsOfdoes not (despite its comment claiming it does), and the registry only works because it passes element schemas by hand. Two copies of schema introspection will drift apart the next time Zod internals change.
src/lib/drift.ts#L35-L43: keep this implementation as the single exported helper (e.g.schemaKeys(schema): readonly string[]) and drop the unused_def/type/innerTypemembers from the local cast.src/lib/api/endpoints.ts#L323-L329: deletefieldsOfand call the shared helper, which also makes its "arrays expose their element's keys" comment true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/drift.ts` around lines 35 - 43, The schema key-enumeration logic is duplicated and inconsistent. In src/lib/drift.ts:35-43, retain and export the array-aware knownKeys implementation as the shared schemaKeys helper, simplifying its local cast to only the properties it reads; in src/lib/api/endpoints.ts:323-329, remove fieldsOf and replace its usage with the shared helper so array schemas use their element keys.src/helpers/export.ts (1)
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the callable payload builder now that the envelope is typed.
This exact block is repeated in
src/helpers/clone.ts,src/helpers/share.ts,src/commands/guests.ts,src/commands/cohosts.ts,src/commands/bulk.tsandsrc/commands/doctor.ts, plus three privatemakePayloadcopies insrc/commands/events.ts,src/commands/rsvp.tsandsrc/commands/bulk.ts. A single helper returningCallableEnvelope<P>fromsrc/lib/api/envelope.tswould make the envelope contract enforced rather than re-typed per call site.♻️ Sketch
Add to
src/lib/auth.ts(or a newsrc/lib/api/payload.ts):export function makeCallablePayload<P>(config: PartifulConfig, params: P): CallableEnvelope<P> { return { data: { params, amplitudeDeviceId: config.amplitudeDeviceId || generateAmplitudeDeviceId(), amplitudeSessionId: Date.now(), userId: config.userId ?? null, }, }; }Then here:
- const payload = { - data: wrapPayload(config, { - params: { eventId }, - amplitudeSessionId: Date.now(), - userId: config.userId, - }), - }; + const payload = makeCallablePayload(config, { eventId });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/export.ts` around lines 29 - 35, Extract the repeated callable envelope construction into a shared makeCallablePayload helper returning CallableEnvelope<P>, using config for amplitudeDeviceId generation, amplitudeSessionId, and nullable userId. Replace the inline payload block in the current export flow and the duplicated builders/call sites in clone, share, guests, cohosts, bulk, doctor, events, and rsvp with this helper, preserving each call site’s params.tests/drift.test.js (1)
11-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that goes through the callable-envelope unwrap.
Every case here passes an already-unwrapped payload, so the suite cannot catch the nesting mismatch between the declared schemas and real responses (see the consolidated comment). A test feeding
{ result: { data: { event: {…} } } }forgetEventInfo— or exportingcheckDriftfromsrc/lib/http.tsand asserting on it — would pin that contract down.Want me to draft those cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/drift.test.js` around lines 11 - 45, Add a drift-detection test that passes the callable envelope { result: { data: { event: ... } } } to the getEventInfo path and asserts fields are evaluated after unwrapping against the declared event schema. Use the existing detectDrift helper unless checkDrift from src/lib/http.ts is required to exercise the real response path, and verify an unexpected nested field is reported while valid event fields are not.src/commands/rsvp.ts (1)
231-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
programis unused in this signature.
attachRsvpVerbsonly needsevents/explore; dropping the first parameter (or prefixing_program) makes the contract honest for thecli.tscall site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/rsvp.ts` around lines 231 - 236, Remove the unused program parameter from registerRsvpCommands, updating its signature to accept only the events and explore commands. Keep both attachRsvpVerbs calls unchanged and update the cli.ts call site to match the revised contract.src/lib/errors.ts (1)
11-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider setting
nameso subclasses are distinguishable in stack traces/logs.All subclasses currently serialize/print as
Error: ...sincenameis never assigned;typecovers the JSON envelope but not raw stack output.♻️ Optional tweak
constructor(message: string, exitCode: number, type: string, details: unknown = null) { super(message); + this.name = new.target.name; this.exitCode = exitCode;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/errors.ts` around lines 11 - 31, Set the Error name within the PartifulError constructor so instances and subclasses identify themselves by their class name in stack traces and logs, while preserving the existing type and JSON serialization behavior.src/commands/events.ts (1)
496-527: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--dry-runstill triggers the interactive confirmation and a live/getEventInfocall.The dry-run branch is evaluated only after the confirm gate, so previewing a cancel prompts the user and hits the network. Hoisting the dry-run check above the confirmation keeps previews fully non-interactive, matching
events create/update.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/events.ts` around lines 496 - 527, Move the dryRun check in the event cancellation action above the confirmation block and its `/getEventInfo` request, while retaining the existing payload construction and JSON preview output. Ensure `--dry-run` returns immediately without prompting or making any live API calls; preserve confirmation and cancellation behavior for normal execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.wayfinder/tickets/05-implement.md:
- Around line 15-19: Synchronize the migration documentation with the TypeScript
port: in .wayfinder/tickets/05-implement.md lines 15-19, change proposed .js
implementation paths to .ts; in .wayfinder/BUILD-PROMPT.md line 4, update or
mark the plain-JavaScript baseline as historical; in .wayfinder/map.md lines
17-20, document TypeScript and the tsx runtime; and in
docs/explore-command-design.md lines 170-172, update the documented source
layout to TypeScript. Ensure the documentation consistently reflects that src/
contains no .js files.
In `@docs/explore-command-design.md`:
- Around line 127-135: Remove guestId from the documented public JSON success
response for the explore rsvp contract, leaving the remaining fields unchanged.
Ensure the user-facing output does not expose Partiful user IDs, either by
omitting guestId or explicitly redacting its value.
- Around line 70-71: Synchronize the questionnaire contract across all
referenced documentation: in docs/explore-command-design.md lines 70-71, remove
questionnaire events from the unsupported refusal list; at lines 167-169,
replace the stale refusal guard with the verified questionnaire discovery,
answer-mapping, and questionnaireResponse validation path; and in
.wayfinder/BUILD-PROMPT.md lines 24-25, replace the obsolete open-recon blocker
with the resolved questionnaire contract.
In `@docs/TYPESCRIPT-PORT-GUIDE.md`:
- Around line 184-186: Update the create-event example around apiRequest and
CreateEventResponseSchema.parse so the Promise<unknown> response is
endpoint-aware typed or explicitly narrowed/validated before accessing
result.data. Preserve the existing fallback to an empty object and schema
parsing, while ensuring the example remains valid under strict TypeScript
settings.
- Around line 170-178: Update the createEventEndpoint metadata example to use
the EndpointMeta-compatible responseFields property instead of responseSchema,
matching the field consumed by schema api.createEvent and the existing endpoint
metadata implementation.
In `@package.json`:
- Line 45: Move the tsx package from devDependencies to dependencies in
package.json, preserving its existing version constraint so the bin/partiful
runtime import of tsx/esm/api remains available in production installs.
In `@src/helpers/watch.ts`:
- Around line 25-27: Validate the parsed interval and duration values in the
watch setup around intervalMs, durationMs, and endTime, using parseInt with an
explicit radix. Reject non-numeric or non-positive inputs before starting the
polling loop so invalid options cannot produce immediate retries or a misleading
completion; preserve normal polling and duration behavior for valid values.
In `@src/lib/auth.ts`:
- Around line 74-79: Validate the refresh response in getValidToken before using
or persisting it: require resp.ok and a present id_token, while preserving the
existing API-error message when available. Throw a clear refresh failure for
invalid responses, and only call saveConfig after validation so
config.accessToken is guaranteed; then remove the non-null assertions from the
returned token path.
In `@src/lib/dates.ts`:
- Around line 11-18: Update parseDateTime to interpret the supplied timezone
when converting dateStr into a Date, ensuring identical wall-clock input
produces the correct instant on hosts in other local timezones; alternatively,
remove the timezone parameter and all callers’ timezone forwarding, then make
the CLI/API contract explicitly local-timezone-only. Keep event.timezone
handling consistent with the chosen contract.
In `@src/lib/http.ts`:
- Around line 26-36: Align drift detection with the declared response shapes: in
src/lib/http.ts lines 26-36, update checkDrift to use per-method unwrapping
metadata instead of always inspecting result.data; in src/lib/api/endpoints.ts
lines 140-154, model getEventInfo at the event envelope depth or configure its
unwrap field; in src/lib/api/endpoints.ts lines 199-215, define home-page
responses as objects containing upcomingEvents or pastEvents arrays and ensure
responseSchemas at lines 471-472 references those object schemas.
- Around line 100-114: Update the fetch calls in apiRequest, firestoreRequest,
and firestoreListDocuments to include a request timeout via
AbortSignal.timeout(...). Use the same timeout signal consistently across all
three helpers so stalled requests abort and remain retryable through withRetry.
- Around line 38-91: Validate MAX_RETRIES in the retry configuration so invalid,
negative, or non-finite PARTIFUL_MAX_RETRIES values fall back to a non-negative
default and always allow the initial request in withRetry. Update Retry-After
handling in withRetry to distinguish valid numeric delays from HTTP-date values,
convert dates to a bounded millisecond delay, and use the existing exponential
backoff when the header is invalid or already expired.
---
Minor comments:
In @.wayfinder/ts-port/tickets/T0-rsvp-merged-branch-cut.md:
- Line 3: Synchronize ticket metadata with the closed statuses in
.wayfinder/ts-port/map.md: update
.wayfinder/ts-port/tickets/T0-rsvp-merged-branch-cut.md:3 to mark T0 closed and
add its merge/branch-cut commit; update
.wayfinder/ts-port/tickets/T3-port-lib-layer-spec.md:3 and
.wayfinder/ts-port/tickets/T4-port-commands-helpers.md:3 to mark them closed and
record their completed work; update
.wayfinder/ts-port/tickets/T5-rewire-schema-command.md:3 and
.wayfinder/ts-port/tickets/T6-drift-detection-smoke-tests.md:3 from OPEN to the
closed status used by their Answer sections.
In `@docs/explore-command-design.md`:
- Around line 119-123: Update the tags example to use the API-verified wire IDs
MUSIC and FOOD for the Music and Food & Drink entries, while preserving the
existing display names and DISCOVER_HOME entry.
In `@src/commands/contacts.ts`:
- Around line 48-49: Validate the `opts['limit']` value before applying
`contactList.slice` in the contacts command, rejecting non-numeric values with
the same error behavior used by `posters list` and `posters search`. Only call
`slice` after validation succeeds, preserving the existing limit behavior for
valid values.
In `@src/commands/doctor.ts`:
- Around line 39-40: Update the displayPath construction near the home and
configPath values to replace HOME with "~" only when HOME is non-empty and
configPath actually starts with it; otherwise preserve configPath unchanged. Use
a prefix check rather than String.replace to prevent unset HOME and mid-path
substitutions.
In `@src/commands/events.ts`:
- Around line 37-40: Replace the unsafe error type assertions with an Error
runtime check and String fallback so every catch path preserves a message for
non-Error throws. Update handleError in src/commands/events.ts (lines 37-40),
both guests catch blocks in src/commands/guests.ts (lines 123-125 and 181-183),
handleError in src/commands/rsvp.ts (lines 40-43), and the +watch catch block in
src/helpers/watch.ts (lines 87-89); each site requires the same guarded message
handling.
- Around line 229-260: The series logic in the events create flow is unreachable
because repeat and count are not declared options. Update the events create
command registration to expose --repeat and --count, and ensure template option
merging preserves them alongside TEMPLATE_FIELDS; keep the existing series
handling and validation behavior unchanged.
In `@src/commands/guests.ts`:
- Around line 79-93: The dry-run guard currently executes after the live
/getEventInfo request. In the guests command flow, move the globalOpts['dryRun']
branch before the try block that calls apiRequest, preserving its existing
jsonOutput response and return so dry-run exits without any network request.
In `@src/helpers/share.ts`:
- Around line 5-16: Restore verbose option propagation in registerShareHelper:
use the action’s Command parameter to read optsWithGlobals<Record<string,
unknown>>() and pass its verbose value to each apiRequest call instead of
hardcoding false. Preserve the existing share-link behavior and update the
callback parameter names as needed to access the command.
In `@src/lib/auth.ts`:
- Around line 33-38: Update resolveCredentialsPath to use os.homedir() instead
of process.env.HOME as string when constructing the default credentials path,
importing the required os module while preserving the PARTIFUL_CREDENTIALS_FILE
override.
In `@src/lib/drift.ts`:
- Around line 95-108: Update the logging destination condition in the drift
logging flow around PARTIFUL_DRIFT_LOG so force only enables logging and does
not override a configured file sink. When force is true with a non-empty,
non-special sink path, append the serialized line to that file; reserve the
stderr branch for unset or explicitly stderr-configured sinks, while preserving
existing write-failure handling.
In `@src/lib/output.ts`:
- Around line 55-65: Update formatCsv’s escape helper to quote values containing
carriage returns in addition to commas, quotes, and line feeds. Adjust the
output assembly so empty rows return only the escaped header without a trailing
newline, while preserving the existing header-and-body format for non-empty
rows.
---
Nitpick comments:
In @.wayfinder/BUILD-PROMPT.md:
- Line 4: Update the repository setup description in BUILD-PROMPT.md to reflect
the current TypeScript strict-mode port and tsx runtime instead of describing
the project as plain JavaScript with no build step. Preserve the remaining
Commander.js, command organization, API access, testing, and global-install
guidance unless those details are also outdated.
In @.wayfinder/map.md:
- Around line 17-20: Update the repository/runtime description in the map to
reflect the TypeScript port: identify TypeScript as the source language,
document tsx-based ESM execution, and mention tsc --noEmit validation.
Alternatively, explicitly mark the existing
Commander.js/plain-JavaScript/no-build-step statements as historical context
while preserving accurate current guidance about API access and global
installation.
In `@package.json`:
- Line 13: Update the package.json start script to remove the node --import tsx
preload and invoke bin/partiful directly, retaining the existing tsx/esm/api
register() call in the bin entrypoint as the sole loader mechanism.
In `@src/commands/events.ts`:
- Around line 496-527: Move the dryRun check in the event cancellation action
above the confirmation block and its `/getEventInfo` request, while retaining
the existing payload construction and JSON preview output. Ensure `--dry-run`
returns immediately without prompting or making any live API calls; preserve
confirmation and cancellation behavior for normal execution.
In `@src/commands/rsvp.ts`:
- Around line 231-236: Remove the unused program parameter from
registerRsvpCommands, updating its signature to accept only the events and
explore commands. Keep both attachRsvpVerbs calls unchanged and update the
cli.ts call site to match the revised contract.
In `@src/helpers/export.ts`:
- Around line 29-35: Extract the repeated callable envelope construction into a
shared makeCallablePayload helper returning CallableEnvelope<P>, using config
for amplitudeDeviceId generation, amplitudeSessionId, and nullable userId.
Replace the inline payload block in the current export flow and the duplicated
builders/call sites in clone, share, guests, cohosts, bulk, doctor, events, and
rsvp with this helper, preserving each call site’s params.
In `@src/lib/api/endpoints.ts`:
- Around line 331-452: The Firestore project identifier is duplicated across the
endpoint definitions and the FIRESTORE_PROJECT constant. Update
firestoreGetEvent, firestorePatchEvent, and firestoreListDocuments to derive
their paths from the shared FIRESTORE_PROJECT value, preserving the existing
endpoint paths while preventing future divergence.
- Around line 115-124: Update CreateEventResponseSchema and the other schemas in
this file to use Zod 4 constructors: replace legacy .passthrough() usage with
z.looseObject() or .loose() while preserving unknown-key behavior, and replace
z.ZodTypeAny references near the identified locations with z.ZodType. Apply
these idiomatic updates consistently across the file without changing schema
behavior.
In `@src/lib/auth.ts`:
- Around line 62-72: Update refreshAccessToken to build the token request body
and API-key query parameters with URLSearchParams rather than raw interpolation,
and add an AbortController-based timeout to the fetch matching the existing
upload timeout pattern. Pass the controller’s signal to fetch and ensure the
timeout is cleared after completion.
In `@src/lib/drift.ts`:
- Around line 35-43: The schema key-enumeration logic is duplicated and
inconsistent. In src/lib/drift.ts:35-43, retain and export the array-aware
knownKeys implementation as the shared schemaKeys helper, simplifying its local
cast to only the properties it reads; in src/lib/api/endpoints.ts:323-329,
remove fieldsOf and replace its usage with the shared helper so array schemas
use their element keys.
In `@src/lib/errors.ts`:
- Around line 11-31: Set the Error name within the PartifulError constructor so
instances and subclasses identify themselves by their class name in stack traces
and logs, while preserving the existing type and JSON serialization behavior.
In `@src/lib/http.ts`:
- Around line 121-124: Update the response parsing flow around checkDrift in the
HTTP reader to catch JSON.parse failures and convert them through the existing
ApiError classification path, matching the handling already used in upload.ts.
Apply the same protection to both Firestore readers, while preserving successful
JSON parsing and the existing empty-body default.
In `@tests/drift.test.js`:
- Around line 11-45: Add a drift-detection test that passes the callable
envelope { result: { data: { event: ... } } } to the getEventInfo path and
asserts fields are evaluated after unwrapping against the declared event schema.
Use the existing detectDrift helper unless checkDrift from src/lib/http.ts is
required to exercise the real response path, and verify an unexpected nested
field is reported while valid event fields are not.
🪄 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 Plus
Run ID: dc7df35b-375b-4e28-a653-2241adf9c386
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (63)
.wayfinder/BUILD-PROMPT.md.wayfinder/map.md.wayfinder/tickets/01-build-id-recon.md.wayfinder/tickets/02-rsvp-endpoint-recon.md.wayfinder/tickets/03-tag-filter-recon.md.wayfinder/tickets/04-command-shape.md.wayfinder/tickets/05-implement.md.wayfinder/tickets/06-docs.md.wayfinder/tickets/07-rsvp-going-live-capture.md.wayfinder/ts-port/BUILD-PROMPT.md.wayfinder/ts-port/map.md.wayfinder/ts-port/tickets/T0-rsvp-merged-branch-cut.md.wayfinder/ts-port/tickets/T1-toolchain-setup.md.wayfinder/ts-port/tickets/T2-porting-convention-doc.md.wayfinder/ts-port/tickets/T3-port-lib-layer-spec.md.wayfinder/ts-port/tickets/T4-port-commands-helpers.md.wayfinder/ts-port/tickets/T5-rewire-schema-command.md.wayfinder/ts-port/tickets/T6-drift-detection-smoke-tests.mdbin/partifuldocs/TYPESCRIPT-PORT-GUIDE.mddocs/explore-command-design.mdpackage.jsonsrc/cli.tssrc/commands/auth.tssrc/commands/blasts.tssrc/commands/bulk.tssrc/commands/cohosts.tssrc/commands/contacts.tssrc/commands/doctor.tssrc/commands/events.tssrc/commands/guests.tssrc/commands/posters.tssrc/commands/rsvp.tssrc/commands/schema.tssrc/commands/setup.tssrc/commands/templates.tssrc/helpers/clone.tssrc/helpers/export.jssrc/helpers/export.tssrc/helpers/share.tssrc/helpers/watch.tssrc/lib/api/endpoints.tssrc/lib/api/envelope.tssrc/lib/auth.tssrc/lib/cohosts.jssrc/lib/cohosts.tssrc/lib/dates.tssrc/lib/drift.tssrc/lib/errors.jssrc/lib/errors.tssrc/lib/events.tssrc/lib/http.jssrc/lib/http.tssrc/lib/output.jssrc/lib/output.tssrc/lib/posters.tssrc/lib/rsvp.tssrc/lib/templates.tssrc/lib/upload.tstests/drift.test.jstests/schema-api.test.jstests/smoke-real-api.test.jstsconfig.json
💤 Files with no reviewable changes (5)
- src/lib/errors.js
- src/lib/cohosts.js
- src/lib/http.js
- src/helpers/export.js
- src/lib/output.js
CodeRabbit flagged that checkDrift unwrapped uniformly to result.data, but
getEventInfo nests at result.data.event and the homepage lists at
result.data.{upcomingEvents,pastEvents} — so those methods reported drift on
every call and schema api.<method> advertised the wrong depth. Add a
PAYLOAD_UNWRAP map + unwrapPayload() in drift.ts and descend before diffing.
Also correct the knownKeys() doc comment (Copilot): it resolves array
element schemas only, not effects/optionals. +2 drift tests. 208 green.
|
Thanks @coderabbitai / Copilot. Triage: Fixed in this PR (404c024) — port-introduced:
Deferred to #67 — these are pre-existing in the JS baseline (ad7be30), not introduced here. This PR is an annotation-only faithful port (zero runtime behavior change by design), so fixing them belongs in its own change:
All five are real and worth fixing — tracked in #67 with a recommended shared-helper pattern. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- recent commits ---'
git log --oneline -8
printf '%s\n' '--- changed files in claimed fix ---'
git show --stat --oneline 404c024
git show --format= --name-status 404c024
printf '%s\n' '--- src/lib/drift.ts outline ---'
ast-grep outline src/lib/drift.ts --items all --view expanded || true
printf '%s\n' '--- src/lib/http.ts outline ---'
ast-grep outline src/lib/http.ts --items all --view expanded || true
printf '%s\n' '--- drift implementation ---'
cat -n src/lib/drift.ts
printf '%s\n' '--- HTTP drift call site ---'
rg -n -C 8 'checkDrift|reportDrift|apiRequest' src/lib/http.ts
printf '%s\n' '--- drift tests ---'
cat -n tests/drift.test.js
printf '%s\n' '--- baseline comparison for deferred files ---'
git diff --stat ad7be30..HEAD -- src/lib/http.ts src/helpers/watch.ts src/lib/auth.ts
git diff --unified=3 ad7be30..HEAD -- src/lib/http.ts src/helpers/watch.ts src/lib/auth.ts
printf '%s\n' '--- GitHub issue `#67` ---'
gh issue view 67 --repo KalebCole/partiful-cli --json number,title,state,body,url,assigneesLength of output: 37082 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- endpoint specification outline ---'
ast-grep outline src/lib/api/endpoints.ts --items all --view expanded || true
printf '%s\n' '--- endpoint response schemas and API registry ---'
rg -n -C 5 \
'GetEventInfoResponseSchema|GetMyUpcomingEventsResponseSchema|GetMyPastEventsResponseSchema|GetContactsResponseSchema|responseSchemas|apiEndpoints' \
src/lib/api/endpoints.ts
printf '%s\n' '--- callable response consumers (current TypeScript) ---'
rg -n -C 4 \
'getEventInfo|getMyUpcomingEventsForHomePage|getMyPastEventsForHomePage|getContacts' \
src tests -g '*.ts' -g '*.js'
printf '%s\n' '--- relevant baseline JavaScript behavior ---'
for f in src/lib/http.js src/helpers/watch.js src/lib/auth.js; do
echo "### ad7be30:$f"
git show "ad7be30:$f" 2>/dev/null || echo "NOT PRESENT"
done
printf '%s\n' '--- exact port-vs-baseline checks for deferred behavior ---'
git show ad7be30:src/lib/http.js | rg -n -C 4 \
'PARTIFUL_MAX_RETRIES|retry-after|parseFloat|fetch\(|setTimeout'
git show ad7be30:src/helpers/watch.js | rg -n -C 4 \
'interval|duration|parseInt|setTimeout'
git show ad7be30:src/lib/auth.js | rg -n -C 5 \
'refreshAccessToken|resp\.ok|id_token|resp\.json'
printf '%s\n' '--- commit patch for drift fix ---'
git show --format=fuller --find-renames 404c024 -- src/lib/drift.ts src/lib/http.ts tests/drift.test.jsLength of output: 66545
🐇 ✅ ✏️ Learnings added
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 40 minutes. |
Empty commit to force CodeRabbit + Copilot to re-review the fix commit; their prior reviews predated 404c024. No code change. Squash-merge drops this.
The CLI has no build step — bin/partiful registers the tsx ESM loader at runtime (import 'tsx/esm/api') to run the .ts source graph directly. tsx is therefore load-bearing at runtime, not just for tests. As a devDependency an 'npm install --omit=dev' / global prod install shipped a CLI that could not start. Verified: prod-only install now resolves tsx/esm/api and ./bin/partiful --version prints 2.1.0. Flagged by CodeRabbit on #66.
# Conflicts: # .wayfinder/ts-port/map.md # .wayfinder/ts-port/tickets/T1-toolchain-setup.md # .wayfinder/ts-port/tickets/T2-porting-convention-doc.md # .wayfinder/ts-port/tickets/T5-rewire-schema-command.md # .wayfinder/ts-port/tickets/T6-drift-detection-smoke-tests.md
Copilot's fresh review surfaced a suppressed but valid port regression: +share stopped forwarding the global --verbose flag to apiRequest. Restore the baseline cmd.optsWithGlobals() behavior. Also restore JS's || fallback for an empty event title (the TS port had changed it to ??). Add 2 parity tests. 210 passed / 6 smoke skipped; tsc clean.
Copilot noted the test comment was stale: GetEventInfoResponseSchema now declares event fields. The assertion was already correct; update the rationale. 210 passed / 6 smoke skipped; tsc clean.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/helpers/share.ts (1)
46-48: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle non-
Errorthrown values safely.The assertion at Line [48] only satisfies TypeScript. If an upstream path throws a string or
null, the error handler itself throws while reading.message.Proposed fix
- else jsonError((e as Error).message); + else jsonError(e instanceof Error ? e.message : String(e));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/helpers/share.ts` around lines 46 - 48, Update the catch handler in the share helper to safely handle thrown values that are not Error instances, including null, without accessing .message unsafely. Preserve the existing PartifulError handling and provide a safe fallback message for other thrown values when calling jsonError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/helpers/share.ts`:
- Around line 37-42: Validate the unknown response from apiRequest in the
/getEventInfo flow with GetEventInfoResponseSchema before extracting title or
calling jsonOutput. Replace the unchecked result/data/event/title assertions
with the parsed schema output, and route schema parse failures through the
existing error path while preserving the { url, eventId, title } output.
In `@tests/share-helper.test.js`:
- Line 6: Update the jsonError mock in the test setup to throw or return the
required structured error shape with status and nested error code, type, and
message fields, using the project’s existing formatter if available; do not
throw a plain Error(message).
---
Outside diff comments:
In `@src/helpers/share.ts`:
- Around line 46-48: Update the catch handler in the share helper to safely
handle thrown values that are not Error instances, including null, without
accessing .message unsafely. Preserve the existing PartifulError handling and
provide a safe fallback message for other thrown values when calling jsonError.
🪄 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 Plus
Run ID: 3a9f168c-a98a-4623-9a92-5b409adc6854
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.wayfinder/ts-port/map.mdpackage.jsonsrc/helpers/share.tssrc/lib/drift.tssrc/lib/http.tstests/drift.test.jstests/share-helper.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/drift.test.js
- .wayfinder/ts-port/map.md
- package.json
- src/lib/http.ts
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
src/commands/auth.ts:26
- FIREBASE_API_KEY and PARTIFUL_SMS_SENDER are now masked ("..." / "****"). These constants are used to call Identity Toolkit and to detect the Partiful SMS thread; masking them will break
auth login(token exchange) and likely SMS auto-retrieval. Restore the real values (or load from env with a real default) to preserve runtime behavior.
- Files reviewed: 58/61 changed files
- Comments generated: 0 new
- Review effort level: Low
What
Port the entire
partiful-clicodebase from plain JavaScript to TypeScript strict mode, and make thesrc/lib/api/layer a living API spec (typed request interfaces + Zod.passthrough()response schemas) rather than a separate hand-maintained artifact that can drift.src/is now 0.jsfiles.tsc --noEmitis clean; the full suite is green.Why
The ~23 untyped
result?.data...API-response spreads were the only record of Partiful's (unofficial, undocumented) API shape. Typing the lib layer captures that shape in code, so the spec is a byproduct of the port, not a doc that rots.How it runs (no build step)
tsx@4is registered as an ESM loader bybin/partiful; Node runs the.tstree directly. Nodist/.npm run typecheck=tsc --noEmit·npm test= vitest..jsextension even though targets are.ts(required, not a bug).Tickets (T0–T6)
tsconfig, tsx loader, zoddocs/TYPESCRIPT-PORT-GUIDE.mdconvention docsrc/lib/→ TS strict + authorsrc/lib/api/{envelope,endpoints}.ts(THE SPEC)src/commands/+src/helpers/+cli.ts→ TS strictschema api.<method>namespace driven off the endpoint registrysrc/lib/drift.ts) + gated real-API smoke suiteNew capabilities
schema apilists every spec'd endpoint;schema api.<method>prints its{ transport, host, path, requestParams, responseFields }. Legacyschema <command>(CLI-flag lookup) unchanged.PARTIFUL_DRIFT_LOG(unset = silent /1|stderr= stderr / path = NDJSON append). Wired guarded intoapiRequest()— advisory only, can never break a request.tests/smoke-real-api.test.js): the spec verifier. Read-only endpoints,skipIf(!PARTIFUL_SMOKE), no secrets in CI.Test status
ad7be30: 195/195 green../bin/partiful --versionandschemaverified end-to-end through the tsx loader.Faithfulness
This is an annotation-only port: zero intended runtime behavior change. An adversarial review pass (behavior drift,
??/||swaps,!assertions,ascasts, JSON output contract, exit codes) returned SHIP with no BLOCKER/MAJOR findings — every??/||swap lands on fields that never hold falsy-but-non-null values; all!assertions are guarded.Out of scope
Summary by CodeRabbit
New Features
schema apiandschema api.<method>.Bug Fixes
Tests