Skip to content

fix(journey-client): remove undefined from JourneyResult return type#545

Open
ryanbas21 wants to merge 2 commits intomainfrom
fix-jc-return-types
Open

fix(journey-client): remove undefined from JourneyResult return type#545
ryanbas21 wants to merge 2 commits intomainfrom
fix-jc-return-types

Conversation

@ryanbas21
Copy link
Collaborator

@ryanbas21 ryanbas21 commented Mar 11, 2026

JIRA Ticket

https://pingidentity.atlassian.net/browse/SDKS-4715

Description

The implementation already returns GenericError for empty server
responses, so undefined was unreachable. Removing it aligns the type
with the actual behavior and matches davinci-client and oidc-client
conventions. Also strengthens the wellknown.utils.test.ts assertion
helper with a properly typed parameter for compile-time narrowing.

Summary by CodeRabbit

  • Tests

    • Added coverage for authentication paths with null server responses.
    • Added compile-time type checks to validate public API result shapes.
    • Cleaned up test code comments for clarity.
  • Refactor

    • Tightened internal result typing and improved internal error-handling behavior.

  The implementation already returns GenericError for empty server
  responses, so undefined was unreachable. Removing it aligns the type
  with the actual behavior and matches davinci-client and oidc-client
  conventions. Also strengthens the wellknown.utils.test.ts assertion
  helper with a properly typed parameter for compile-time narrowing.
@changeset-bot
Copy link

changeset-bot bot commented Mar 11, 2026

⚠️ No Changeset found

Latest commit: 70b6781

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 320ad594-9e36-4cac-b979-0852fa801c30

📥 Commits

Reviewing files that changed from the base of the PR and between 19c57e4 and 70b6781.

📒 Files selected for processing (6)
  • packages/journey-client/src/lib/client.store.test.ts
  • packages/journey-client/src/lib/client.types.test-d.ts
  • packages/journey-client/src/lib/config.slice.test.ts
  • packages/journey-client/src/lib/wellknown.utils.test.ts
  • packages/journey-client/vite.config.ts
  • packages/sdk-effects/oidc/src/lib/wellknown.effects.test.ts
💤 Files with no reviewable changes (2)
  • packages/sdk-effects/oidc/src/lib/wellknown.effects.test.ts
  • packages/journey-client/src/lib/config.slice.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/journey-client/src/lib/client.store.test.ts

📝 Walkthrough

Walkthrough

This PR tightens an internal JourneyResult type (removing undefined), adds compile-time and runtime tests validating client method return shapes and null server responses, and updates a well-known config test helper to throw GenericError on error results.

Changes

Cohort / File(s) Summary
Journey Client Tests
packages/journey-client/src/lib/client.store.test.ts, packages/journey-client/src/lib/client.types.test-d.ts
Adds a runtime test start_NoDataFromServer_ReturnsGenericError verifying null authenticate responses map to a GenericError, and introduces a TypeScript-only test file enforcing that public JourneyClient method results include expected variants and exclude undefined.
Wellknown Utilities Tests
packages/journey-client/src/lib/wellknown.utils.test.ts
Renames assertResolvedassertResolvedConfig, widens its type to ReturnType<typeof convertWellknown>, and changes behavior to throw a detailed GenericError when an error property is present.
Journey Client Implementation
packages/journey-client/src/lib/client.store.ts
Tightens internal JourneyResult type alias by removing undefined from the union; no exported/public signatures changed.
SDK Effects Tests (cosmetic)
packages/sdk-effects/oidc/src/lib/wellknown.effects.test.ts, packages/journey-client/src/lib/config.slice.test.ts
Removed // Arrange, // Act, // Assert comment blocks from multiple tests; no logic or assertions altered.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • cerebrl

Poem

"🐇 I hopped through tests both bright and merry,
Null responses met a guarded query.
Types snugged tight, errors called by name,
No sneaky undefined shall stake its claim.
Hop, test, repeat — the rabbit celebrates the gain!"

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing undefined from the JourneyResult return type, which is the primary fix in this PR.
Description check ✅ Passed The description includes the required JIRA ticket reference and clearly explains the changes, rationale, and alignment with existing conventions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-jc-return-types

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link
Contributor

nx-cloud bot commented Mar 11, 2026

View your CI Pipeline Execution ↗ for commit 70b6781

Command Status Duration Result
nx run-many -t build --no-agents ✅ Succeeded 1s View ↗
nx affected -t build lint test typecheck e2e-ci ✅ Succeeded 2m 5s View ↗

☁️ Nx Cloud last updated this comment at 2026-03-11 21:07:06 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 11, 2026

Open in StackBlitz

@forgerock/davinci-client

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/davinci-client@545

@forgerock/device-client

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/device-client@545

@forgerock/journey-client

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/journey-client@545

@forgerock/oidc-client

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/oidc-client@545

@forgerock/protect

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/protect@545

@forgerock/sdk-types

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-types@545

@forgerock/sdk-utilities

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-utilities@545

@forgerock/iframe-manager

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/iframe-manager@545

@forgerock/sdk-logger

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-logger@545

@forgerock/sdk-oidc

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-oidc@545

@forgerock/sdk-request-middleware

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/sdk-request-middleware@545

@forgerock/storage

pnpm add https://pkg.pr.new/ForgeRock/ping-javascript-sdk/@forgerock/storage@545

commit: 70b6781

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

Deployed 6386c9b to https://ForgeRock.github.io/ping-javascript-sdk/pr-545/6386c9b111c299ea8d17318358ce11a9f6b01cf0 branch gh-pages in ForgeRock/ping-javascript-sdk

@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2026

📦 Bundle Size Analysis

📦 Bundle Size Analysis

🆕 New Packages

🆕 @forgerock/journey-client - 87.7 KB (new)
🆕 @forgerock/journey-client - 0.0 KB (new)

➖ No Changes

@forgerock/sdk-logger - 1.6 KB
@forgerock/sdk-request-middleware - 4.5 KB
@forgerock/iframe-manager - 2.4 KB
@forgerock/sdk-oidc - 4.8 KB
@forgerock/storage - 1.5 KB
@forgerock/sdk-types - 7.9 KB
@forgerock/protect - 150.1 KB
@forgerock/device-client - 9.2 KB
@forgerock/davinci-client - 41.3 KB
@forgerock/sdk-utilities - 11.2 KB
@forgerock/oidc-client - 24.9 KB


13 packages analyzed • Baseline from latest main build

Legend

🆕 New package
🔺 Size increased
🔻 Size decreased
➖ No change

ℹ️ How bundle sizes are calculated
  • Current Size: Total gzipped size of all files in the package's dist directory
  • Baseline: Comparison against the latest build from the main branch
  • Files included: All build outputs except source maps and TypeScript build cache
  • Exclusions: .map, .tsbuildinfo, and .d.ts.map files

🔄 Updated automatically on each push to this PR

@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.95%. Comparing base (b89ad58) to head (70b6781).
⚠️ Report is 61 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #545       +/-   ##
===========================================
+ Coverage   18.79%   58.95%   +40.16%     
===========================================
  Files         140       96       -44     
  Lines       27640     5358    -22282     
  Branches      980      790      -190     
===========================================
- Hits         5195     3159     -2036     
+ Misses      22445     2199    -20246     
Files with missing lines Coverage Δ
packages/journey-client/src/lib/client.store.ts 77.92% <ø> (-14.87%) ⬇️

... and 101 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Add compile-time type assertions to verify JourneyResult union members
and ensure undefined never reappears. Enable vitest typecheck in
vite.config.ts. Remove redundant Arrange/Act/Assert comments and fix
prettier formatting across all modified test files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants