Conversation
|
| Name | Type |
|---|---|
| @forgerock/sdk-request-middleware | Minor |
| @forgerock/davinci-client | Minor |
| @forgerock/journey-client | Minor |
| @forgerock/oidc-client | Minor |
| @forgerock/device-client | Minor |
| @forgerock/protect | Minor |
| @forgerock/sdk-types | Minor |
| @forgerock/sdk-utilities | Minor |
| @forgerock/iframe-manager | Minor |
| @forgerock/sdk-logger | Minor |
| @forgerock/sdk-oidc | Minor |
| @forgerock/storage | Minor |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
📝 WalkthroughWalkthroughRenames the client polling helper from poll to pollStatus, updates JSDoc to describe automatic challenge vs continue polling modes, and adjusts e2e wiring and a test label and the changeset markdown to reflect the new API name and usage guidance. ChangesPolling API Rename & Documentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
View your CI Pipeline Execution ↗ for commit e75ee94
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/davinci-client/src/lib/client.store.ts (1)
425-446:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve backward compatibility for renamed polling API.
Line 425 introduces
pollStatus, but the prior publicpollAPI is no longer present. That is a breaking runtime/API change for existing integrators. Please either keep a deprecatedpollalias for one transition release, or explicitly treat this as a breaking migration.Suggested compatibility patch
+ const buildPoller = (collector: PollingCollector): Poller => { + return async () => { + const result = await getPollingModeµ(collector).pipe( + Micro.flatMap((mode) => pollingµ({ mode, collector, store, log })), + Micro.tapError((err) => Micro.sync(() => log.error(err.error.message))), + Micro.runPromiseExit, + ); + + if (exitIsSuccess(result)) return result.value; + if (exitIsFail(result)) return result.cause.error; + + return createInternalError( + 'An unexpected error occurred during poll operation', + 'unknown_error', + ); + }; + }; + return { @@ - pollStatus: (collector: PollingCollector): Poller => { - return async () => { - const result = await getPollingModeµ(collector).pipe( - Micro.flatMap((mode) => pollingµ({ mode, collector, store, log })), - Micro.tapError((err) => Micro.sync(() => log.error(err.error.message))), - Micro.runPromiseExit, - ); - - if (exitIsSuccess(result)) { - return result.value; - } - - if (exitIsFail(result)) { - return result.cause.error; - } - - return createInternalError( - 'An unexpected error occurred during poll operation', - 'unknown_error', - ); - }; - }, + pollStatus: (collector: PollingCollector): Poller => buildPoller(collector), + /** `@deprecated` Use pollStatus(collector) */ + poll: (collector: PollingCollector): Poller => buildPoller(collector),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/client.store.ts` around lines 425 - 446, The new pollStatus function replaces the old public poll API and breaks compatibility; restore a deprecated alias by adding a poll export/function that forwards to pollStatus (accepting the same PollingCollector and returning a Poller) and mark it as deprecated in comments, or alternatively document this as a breaking change; update references to PollingCollector and Poller so poll simply calls and returns pollStatus(collector) to preserve runtime behavior for existing integrators.
🧹 Nitpick comments (1)
.changeset/lucky-parts-own.md (1)
6-13: ⚡ Quick winMake migration impact explicit in changeset text.
Lines 6-13 read as a feature add, but this PR renames the public API from
poll()topollStatus(). Please explicitly call out the rename/deprecation path so consumers know what to update.Suggested wording
-Adds `pollStatus()` method and `PollingCollector` to `@forgerock/davinci-client` for polling support in DaVinci flows. +Renames polling helper `poll()` to `pollStatus()` in `@forgerock/davinci-client`. +Update integrations to call `davinciClient.pollStatus(collector)` instead of `davinciClient.poll(collector)`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/lucky-parts-own.md around lines 6 - 13, Update the changeset text to explicitly call out that the public API was renamed from poll() to pollStatus() in `@forgerock/davinci-client` and describe the migration/deprecation path for consumers: mention that PollingCollector is unchanged, show that callers should replace calls to poll(...) with pollStatus(collector) (or use the returned poller function as before), note any backwards-compatibility or removal timeline for poll(), and include a short example or migration hint referencing poll() and pollStatus() so users know what to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/davinci-client/src/lib/client.store.ts`:
- Around line 425-446: The new pollStatus function replaces the old public poll
API and breaks compatibility; restore a deprecated alias by adding a poll
export/function that forwards to pollStatus (accepting the same PollingCollector
and returning a Poller) and mark it as deprecated in comments, or alternatively
document this as a breaking change; update references to PollingCollector and
Poller so poll simply calls and returns pollStatus(collector) to preserve
runtime behavior for existing integrators.
---
Nitpick comments:
In @.changeset/lucky-parts-own.md:
- Around line 6-13: Update the changeset text to explicitly call out that the
public API was renamed from poll() to pollStatus() in `@forgerock/davinci-client`
and describe the migration/deprecation path for consumers: mention that
PollingCollector is unchanged, show that callers should replace calls to
poll(...) with pollStatus(collector) (or use the returned poller function as
before), note any backwards-compatibility or removal timeline for poll(), and
include a short example or migration hint referencing poll() and pollStatus() so
users know what to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: deff6212-bd5a-4c14-acc1-178db78abcac
📒 Files selected for processing (4)
.changeset/lucky-parts-own.mde2e/davinci-app/components/polling.tse2e/davinci-app/main.tspackages/davinci-client/src/lib/client.store.ts
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Nx Cloud has identified a possible root cause for your failed CI:
We investigated the E2E failure in phone-number-field.test.ts and confirmed it is a pre-existing issue — the identical error reproduces on the main branch independent of this PR's changes. Our PR only updates poll naming to pollStatus across @forgerock/davinci-client and @forgerock/davinci-app, with no changes to the phone number field flow or the @forgerock/davinci-suites project. This failure is caused by an external backend service dependency and is unrelated to our changes.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
Other than outdated date in copyright headers of a few files and the CI error, changes look good to go!
Files:
packages/davinci-client/src/lib/client.store.tse2e/davinci-app/components/polling.tse2e/davinci-app/main.ts
Btw, I'm adding the copyright header automation to this repo next, as soon as I have a spare time.
ancheetah
force-pushed
the
update-poll-naming
branch
from
e5e0fe0 to
e75ee94
Compare
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #622 +/- ##
===========================================
- Coverage 70.90% 17.48% -53.42%
===========================================
Files 53 154 +101
Lines 2021 24195 +22174
Branches 377 1146 +769
===========================================
+ Hits 1433 4231 +2798
- Misses 588 19964 +19376
🚀 New features to boost your workflow:
|
|
Deployed 1a87b14 to https://ForgeRock.github.io/ping-javascript-sdk/pr-622/1a87b1433485066e547ed171baca6a8f1bdf2670 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/davinci-client - 48.6 KB (+0.2 KB) ➖ No Changes➖ @forgerock/device-client - 10.0 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/davinci-client/src/lib/client.store.ts (1)
418-446:⚠️ Potential issue | 🔴 CriticalMark the changeset as a
majorbump — renaming a public API method is a breaking change.The commit message "chore: update poll naming" confirms this is a rename of the public
poll()method topollStatus(). While the changeset description says "Adds," this is actually a breaking API change that requires amajorversion bump, notminor. No stale references remain in the codebase, so update.changeset/lucky-parts-own.mdto change'@forgerock/davinci-client': minorto'@forgerock/davinci-client': major.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/davinci-client/src/lib/client.store.ts` around lines 418 - 446, The public API method was renamed from poll() to pollStatus() which is a breaking change; update the changeset file .changeset/lucky-parts-own.md to mark the package '@forgerock/davinci-client' as a major bump instead of minor so the release reflects this breaking rename (search for pollStatus or poll in the diff to confirm the rename and update the changeset entry accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/davinci-client/src/lib/client.store.ts`:
- Around line 418-446: The public API method was renamed from poll() to
pollStatus() which is a breaking change; update the changeset file
.changeset/lucky-parts-own.md to mark the package '@forgerock/davinci-client' as
a major bump instead of minor so the release reflects this breaking rename
(search for pollStatus or poll in the diff to confirm the rename and update the
changeset entry accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c1bff97-910d-4454-b1c3-bcae18fff29e
📒 Files selected for processing (5)
.changeset/lucky-parts-own.mde2e/davinci-app/components/polling.tse2e/davinci-app/main.tse2e/davinci-suites/src/phone-number-field.test.tspackages/davinci-client/src/lib/client.store.ts
✅ Files skipped from review due to trivial changes (2)
- e2e/davinci-app/main.ts
- .changeset/lucky-parts-own.md
Thanks, I've fixed the failing test which should unblock anyone who has issues with that test. I only changed copyright in the polling component. We can just batch update the rest of the copyrights later. |
4 participants
JIRA Ticket
Description
Updates poll naming to
pollStatus. Updates JSDocs and changeset description.Summary by CodeRabbit
New Features
Tests