refactor: replace new Promise with Promise.withResolvers per AGENTS.md#3336
refactor: replace new Promise with Promise.withResolvers per AGENTS.md#3336oldschoola wants to merge 3 commits into
Conversation
roboomp
left a comment
There was a problem hiding this comment.
Thanks @oldschoola — clean, scoped follow-up to the Promise.withResolvers() convention in AGENTS.md. Rank p1: semantics are preserved across all four helpers (the abort-listener wiring in callback-server.ts, auth-storage.ts, and auth-broker/remote-store.ts is observably identical, and print-mode.ts's sync-throw path stays equivalent under the async wrapper). Affected tests (auth-broker-refresher, auth-storage-usage-cache, callback-server-manual-input, auth-broker-remote-store, remote-auth-store) pass locally.
One should-fix nit: both changelog entries are missing the external-contribution attribution ([#3336] + [@oldschoola]) called out in AGENTS.md. One nit on print-mode.ts documenting the (harmless) shift in where a synchronous stdout.write throw surfaces. Neither blocks merge.
|
|
||
| ### Changed | ||
|
|
||
| - Replaced `new Promise((resolve, reject) => ...)` with `Promise.withResolvers()` in `callback-server.ts` and `auth-storage.ts`/`auth-broker/remote-store.ts` race-with-signal helpers, per the project convention. |
There was a problem hiding this comment.
should-fix: per AGENTS.md ("External contributions: Added feature X ([#456](https://github.com/can1357/oh-my-pi/pull/456) by [@username](https://github.com/username))"), this entry should carry the PR + author attribution. Suggest appending ([#3336](https://github.com/can1357/oh-my-pi/pull/3336) by [@oldschoola](https://github.com/oldschoola)). Same for packages/coding-agent/CHANGELOG.md.
| else resolve(); | ||
| }); | ||
| const { promise, resolve, reject } = Promise.withResolvers<void>(); | ||
| process.stdout.write("", err => { |
There was a problem hiding this comment.
nit: small semantic shift worth a glance — in the previous form, a synchronous throw from process.stdout.write(...) was caught by the Promise executor and surfaced as a rejection. After the refactor it propagates synchronously out of the call before await promise runs. Because runPrintMode is async the caller still observes a rejected promise, so the contract is unchanged — flagging only so reviewers don't have to re-verify. No change requested.
|
Both review comments addressed in the latest push:
|
Replaced new Promise((resolve, reject) => ...) with Promise.withResolvers() in 4 files: - packages/ai/src/registry/oauth/callback-server.ts - packages/ai/src/auth-storage.ts (raceUsageWithSignal) - packages/ai/src/auth-broker/remote-store.ts (#raceWithSignal) - packages/coding-agent/src/modes/print-mode.ts
…bution - Wrap process.stdout.write in try-catch so synchronous throws (e.g. destroyed stdout) are converted to rejections, matching the original Promise executor behavior. - Add PR + author attribution to both CHANGELOG entries per AGENTS.md.
958dbef to
b2748c5
Compare
Summary
Replaces
new Promise((resolve, reject) => ...)withPromise.withResolvers()in 4 files, per the AGENTS.md convention: "usePromise.withResolvers()instead ofnew Promise((resolve, reject) => ...)".Files changed
packages/ai/src/registry/oauth/callback-server.ts—#waitForCallbackassigns resolve/reject to instance fieldspackages/ai/src/auth-storage.ts—raceUsageWithSignalwraps promise + abort signalpackages/ai/src/auth-broker/remote-store.ts—#raceWithSignalwraps promise + abort signalpackages/coding-agent/src/modes/print-mode.ts— stdout flush wrapperVerification
bun checkpassesauth-broker-refresher,auth-storage-usage-cache,callback-server-manual-input