Syncpack setup v2#624
Conversation
|
📝 WalkthroughWalkthroughThis PR establishes centralized dependency version management using Syncpack across the monorepo and updates the davinci-client's public API, renaming its polling method from ChangesSyncpack Dependency Version Management
DaVinci Client API Update
e2e DaVinci App OIDC changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
View your CI Pipeline Execution ↗ for commit d474593
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ryanbas21
force-pushed
the
syncpack-setup-v2
branch
from
8cffe6b to
8978132
Compare
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 is proposing a fix for your failed CI:
We updated the signoff assertion in basic.test.ts to use page.waitForRequest instead of page.waitForResponse for the /signoff check, which fixes the 30s timeout. With @forgerock/javascript-sdk 4.9.0 (introduced by this PR), endSession now uses window.location.assign() rather than fetch(), meaning the 302 redirect is a document navigation that Playwright's CDP-based waitForResponse does not capture. Switching to waitForRequest correctly intercepts the navigation request to /signoff, and the subsequent expect(page.getByText('Username/Password Form')).toBeVisible() continues to verify the full logout redirect completed.
Tip
✅ We verified this fix by re-running @forgerock/davinci-suites:e2e-ci--src/basic.test.ts.
Warning
The suggested diff is too large to display here, but you can view it on Nx Cloud ↗
Or Apply changes locally with:
npx nx-cloud apply-locally DTyA-1GFB
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
ryanbas21
force-pushed
the
syncpack-setup-v2
branch
from
54a6cfe to
4ff7f04
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✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (17.61%) 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 #624 +/- ##
===========================================
- Coverage 70.90% 17.61% -53.29%
===========================================
Files 53 154 +101
Lines 2021 24243 +22222
Branches 377 1160 +783
===========================================
+ Hits 1433 4271 +2838
- Misses 588 19972 +19384 🚀 New features to boost your workflow:
|
|
Deployed 9bc8071 to https://ForgeRock.github.io/ping-javascript-sdk/pr-624/9bc8071025d8c7dd8240d84ecea0109f9379ea22 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/device-client - 10.0 KB (-0.0 KB) ➖ No Changes➖ @forgerock/davinci-client - 48.9 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
- Install syncpack@15 for monorepo dependency alignment enforcement - Add .syncpackrc with JSON schema reference - Add syncpack:lint and syncpack:fix scripts (with explicit --source to exclude dist/ build artifacts, which syncpack reads via pnpm-workspace) - Extend pnpm default catalog with: @forgerock/javascript-sdk, @types/express, tslib, tsx - Migrate all consumers to catalog: references — fixes DiffersToCatalog and DiffersToHighestOrLowestSemver across 11 package.json files
4.9.0 added Accept: application/json to all requests, causing PingOne's /signoff endpoint to return JSON instead of a 302 redirect, breaking the logout e2e test.
TokenManager.getTokens → oidcClient.token.exchange FRUser.logout → oidcClient.user.logout Config.setAsync removed (oidc-client manages its own config) This removes the last dependency on the legacy SDK from the davinci e2e app.
ryanbas21
force-pushed
the
syncpack-setup-v2
branch
from
4ff7f04 to
d474593
Compare
![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
🤖 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-app/main.ts`:
- Around line 89-93: Don't cast DaVinciConfig to OidcConfig; instead
construct/validate a proper OidcConfig before calling oidc. Replace the `oidc({
config: config as OidcConfig })` call by building an explicit OidcConfig object
(containing required fields clientId, redirectUri, scope, and
serverConfig.wellknown) from the existing config or by calling a
conversion/validation helper (e.g., buildOidcConfig(config) /
validateOidcConfig(config)) so structural incompatibilities are caught at
compile time; then pass that validated OidcConfig into the oidc(...) call and
assign the result to oidcClient as before.
🪄 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: bee8a638-bca7-4225-a26a-5f93fbf736ca
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.github/workflows/ci.yml.syncpackrce2e/am-mock-api/package.jsone2e/davinci-app/main.tse2e/davinci-app/package.jsone2e/davinci-app/tsconfig.app.jsone2e/davinci-suites/src/basic.test.tse2e/device-client-app/package.jsone2e/protect-app/package.jsonlefthook.ymlpackage.jsonpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/device-client/package.jsonpackages/journey-client/package.jsonpnpm-workspace.yamlscratchpad/package.jsontools/api-report/package.jsontools/interface-mapping-validator/package.json
💤 Files with no reviewable changes (1)
- e2e/davinci-suites/src/basic.test.ts
✅ Files skipped from review due to trivial changes (6)
- scratchpad/package.json
- e2e/davinci-app/tsconfig.app.json
- packages/journey-client/package.json
- .syncpackrc
- e2e/am-mock-api/package.json
- tools/api-report/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/ci.yml
- packages/device-client/package.json
- tools/interface-mapping-validator/package.json
- pnpm-workspace.yaml
- e2e/protect-app/package.json
- e2e/device-client-app/package.json
- lefthook.yml
- package.json
| const oidcResult = await oidc({ config: config as OidcConfig }); | ||
| if ('error' in oidcResult) { | ||
| throw new Error(`Failed to initialize oidc client: ${oidcResult.error}`); | ||
| } | ||
| const oidcClient = oidcResult; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect OidcConfig and the oidc() return type / error shape
fd -t f 'tsconfig.lib.json' packages/oidc-client 2>/dev/null
fd -t f -e ts . packages/oidc-client/src 2>/dev/null | head -50
# OidcConfig definition
ast-grep --pattern 'export type OidcConfig = $$$'
ast-grep --pattern 'export interface OidcConfig { $$$ }'
# DaVinciConfig definition
ast-grep --pattern 'export type DaVinciConfig = $$$'
ast-grep --pattern 'export interface DaVinciConfig { $$$ }'
# oidc() factory return type / error shape
ast-grep --pattern 'export function oidc($$$) { $$$ }'
ast-grep --pattern 'export const oidc = $$$'
rg -nP --type=ts -C3 "\berror\s*:" packages/oidc-client/src 2>/dev/null | head -60Repository: ForgeRock/ping-javascript-sdk
Length of output: 5411
🏁 Script executed:
# Get OidcConfig definition
rg -n "export type OidcConfig|export interface OidcConfig" packages/oidc-client/src --type=ts -A 10
# Get DaVinciConfig definition
rg -n "export type DaVinciConfig|export interface DaVinciConfig" --type=ts -A 10Repository: ForgeRock/ping-javascript-sdk
Length of output: 1443
🏁 Script executed:
# Check the oidc function export and its return type
rg -n "export.*function oidc|export.*const oidc" packages/oidc-client/src --type=ts -A 5
# Look for the complete oidc API return type (likely has data + error union)
rg -n "type.*OidcFactoryResult|interface.*OidcFactoryResult|export.*Result.*oidc|return.*error.*data" packages/oidc-client/src --type=ts -B 2 -A 5Repository: ForgeRock/ping-javascript-sdk
Length of output: 1015
🏁 Script executed:
# Verify the error field type in the main oidc API
cat packages/oidc-client/src/lib/oidc.api.ts | head -200Repository: ForgeRock/ping-javascript-sdk
Length of output: 6924
🏁 Script executed:
# Get full oidc function signature including return type
cat packages/oidc-client/src/lib/client.store.ts | sed -n '51,100p'Repository: ForgeRock/ping-javascript-sdk
Length of output: 1549
🏁 Script executed:
# Search for the return type definition
rg -n "OidcClientResult|OidcFactoryResult|type.*oidc.*Result" packages/oidc-client/src --type=ts -A 3Repository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
# Look for where oidc function returns error
rg -n "return.*error|error.*:" packages/oidc-client/src/lib/client.store.ts --type=ts -B 2 -A 2 | head -80Repository: ForgeRock/ping-javascript-sdk
Length of output: 3274
The type cast config as OidcConfig masks a structural incompatibility.
OidcConfig (extends AsyncLegacyConfigOptions with required clientId, redirectUri, scope, serverConfig.wellknown) is incompatible with DaVinciConfig (extends AsyncLegacyConfigOptions with only optional responseType). Casting bypasses type checking and will only surface missing required fields at runtime. Build an OidcConfig explicitly or export a validation/conversion helper instead of asserting the type.
The error message handling on line 91 is fine—oidcResult.error is always a string from the oidc factory.
🤖 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-app/main.ts` around lines 89 - 93, Don't cast DaVinciConfig to
OidcConfig; instead construct/validate a proper OidcConfig before calling oidc.
Replace the `oidc({ config: config as OidcConfig })` call by building an
explicit OidcConfig object (containing required fields clientId, redirectUri,
scope, and serverConfig.wellknown) from the existing config or by calling a
conversion/validation helper (e.g., buildOidcConfig(config) /
validateOidcConfig(config)) so structural incompatibilities are caught at
compile time; then pass that validated OidcConfig into the oidc(...) call and
assign the result to oidcClient as before.
3 participants
JIRA Ticket
N/A
Description
sets up syncpack so it automatically keeps our versions aligned and everything.
Summary by CodeRabbit