fix(davinci-client): invoke RTK Query selectors with state for cache lookups#657
Conversation
|
|
View your CI Pipeline Execution ↗ for commit ad3251e
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
|
Warning Review limit reached
More reviews will be available in 8 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughRefactors client cache accessors to evaluate Redux state and return structured errors, updates generated API/type reports, adds unit tests for cache behavior, and introduces Playwright E2E password-policy tests plus supporting test infra changes. ChangesCache Store Refactoring and Password Policy E2E Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
This CI failure appears to be related to the environment or external dependencies rather than your code changes.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
@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 project status has failed because the head coverage (20.95%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #657 +/- ##
==========================================
+ Coverage 18.07% 20.95% +2.87%
==========================================
Files 155 158 +3
Lines 24398 24811 +413
Branches 1203 1390 +187
==========================================
+ Hits 4410 5199 +789
+ Misses 19988 19612 -376
🚀 New features to boost your workflow:
|
|
Deployed d0d421e to https://ForgeRock.github.io/ping-javascript-sdk/pr-657/d0d421e2927859bc0291afecffd76e1a436a9b71 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/device-client - 10.0 KB (new) 📊 Minor Changes📉 @forgerock/davinci-client - 53.8 KB (-0.2 KB) ➖ No Changes➖ @forgerock/protect - 144.6 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/davinci-suites/playwright.config.ts (1)
32-32: ⚡ Quick win
reuseExistingServer: truein CI can mask stale servers.Hardcoding
true(instead of!process.env.CI) means that in CI Playwright will silently attach to any process already listening on port 5829 rather than launching a fresh one. If a stale or wrong-build server is running, tests pass against the wrong target. Consider keeping CI on a fresh server:♻️ Suggested approach
- reuseExistingServer: true, + reuseExistingServer: !process.env.CI,If reuse during local stacked runs is the goal, that's typically only desirable for the non-CI
pnpm watchentry.Also applies to: 40-40
🤖 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 `@e2e/davinci-suites/playwright.config.ts` at line 32, The Playwright config currently forces reuseExistingServer: true which causes CI to attach to any process on the test port and can mask stale servers; change the value to be conditional (e.g., reuseExistingServer: !process.env.CI) so CI launches a fresh server while local/watch runs still reuse an existing one, update the reuseExistingServer setting in the Playwright config where it is defined (reuseExistingServer) and ensure the CI branch explicitly disables reuse to avoid attaching to stale processes.
🤖 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 `@e2e/davinci-suites/src/password-policy.test.ts`:
- Around line 150-156: The current cleanup only calls deleteTestUser when the
"Continue" button is visible, risking leaked PingOne accounts; refactor so
deletion always runs (even on failures) by moving the teardown into an
unconditional finally block around the test body or into a test.afterEach hook
that references the created email, ensuring deleteTestUser(page, email) is
invoked regardless of UI state or assertion failures; also make sure the email
variable is stored in a scope reachable by the finally/afterEach and handle/log
deletion errors gracefully inside that teardown.
---
Nitpick comments:
In `@e2e/davinci-suites/playwright.config.ts`:
- Line 32: The Playwright config currently forces reuseExistingServer: true
which causes CI to attach to any process on the test port and can mask stale
servers; change the value to be conditional (e.g., reuseExistingServer:
!process.env.CI) so CI launches a fresh server while local/watch runs still
reuse an existing one, update the reuseExistingServer setting in the Playwright
config where it is defined (reuseExistingServer) and ensure the CI branch
explicitly disables reuse to avoid attaching to stale processes.
🪄 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: 73999f49-b1dc-4049-9345-3e29cb1871df
📒 Files selected for processing (7)
e2e/davinci-app/server-configs.tse2e/davinci-suites/playwright.config.tse2e/davinci-suites/src/password-policy.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.test.tspackages/davinci-client/src/lib/client.store.ts
ryanbas21
force-pushed
the
fix/cache-selector-password-policy-e2e
branch
3 times, most recently
from
490304d to
7b20ded
Compare
…lookups cache.getLatestResponse() and cache.getResponseWithId() were returning selector factory functions instead of query results — endpoints.*.select(key) returns a (state) => result function that must be called with the current state snapshot. Fixed both methods to call the selector with store.getState() and check .data !== undefined to find the correct cache slot. Also includes: - Comprehensive unit tests for all cache API paths (no-key error, fulfilled, not-found) - Password policy e2e suite for ValidatedPasswordCollector (registration flow) - Server config entry for password-policy PingOne client - Playwright config: reuseExistingServer: true for local dev efficiency
ryanbas21
force-pushed
the
fix/cache-selector-password-policy-e2e
branch
from
7b20ded to
ad3251e
Compare
3 participants
JIRA
https://pingidentity.atlassian.net/browse/SDKS-3816
Summary
cache.getLatestResponse()andcache.getResponseWithId()were returning RTK Query selector factory functions instead of query result objects —endpoints.*.select(key)returns(state) => resultand must be called with the current state snapshotstore.getState()and check.data !== undefinedto find the correct cache slotpassword-policy.test.tse2e suite forValidatedPasswordCollectorregistration flowserver-configs.tsreuseExistingServer: truein Playwright config so a running dev server is reused instead of double-launchingTest plan
pnpm nx run @forgerock/davinci-client:test— 353 tests passpnpm nx affected -t typecheck lint build— all cleanpnpm nx run @forgerock/davinci-suites:e2eagainst live PingOne environment with clientfb456db5-2e08-46d3-adf0-05bf8d26ad60Summary by CodeRabbit
Release Notes
New Features
Tests
Chores