chore: fix files distributed in effect packages#555
Conversation
🦋 Changeset detectedLatest commit: 20e7e28 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Changeset and sets Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
View your CI Pipeline Execution ↗ for commit 20e7e28
☁️ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/odd-parents-joke.md:
- Line 7: The changeset lists '@forgerock/davinci-client' but no package changes
are included; either remove the '@forgerock/davinci-client' line from
.changeset/odd-parents-joke.md or add the actual package.json changes for
packages/davinci-client (update packages/davinci-client/package.json) so the
changeset matches the patch; specifically ensure the 'files' field in
package.json (currently ["dist", "!dist/tsconfig.lib.tsbuildinfo", "./LICENSE"])
is part of the committed changes if you intend to keep the package in the
changeset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28c5455f-1d8b-46b2-8b42-068c839258e0
📒 Files selected for processing (6)
.changeset/odd-parents-joke.mdpackages/sdk-effects/iframe-manager/package.jsonpackages/sdk-effects/logger/package.jsonpackages/sdk-effects/oidc/package.jsonpackages/sdk-effects/sdk-request-middleware/package.jsonpackages/sdk-effects/storage/package.json
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (14.77%) 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 #555 +/- ##
===========================================
- Coverage 70.90% 14.77% -56.14%
===========================================
Files 53 153 +100
Lines 2021 26262 +24241
Branches 377 1056 +679
===========================================
+ Hits 1433 3879 +2446
- Misses 588 22383 +21795 🚀 New features to boost your workflow:
|
@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: |
|
Deployed e7a9de4 to https://ForgeRock.github.io/ping-javascript-sdk/pr-555/e7a9de41db46a4d1fcad8b53b1ced35f41782d96 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/journey-client - 87.8 KB (new) ➖ No Changes➖ @forgerock/sdk-logger - 1.6 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ancheetah
force-pushed
the
fix-packaging
branch
from
3401421 to
d849256
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yml:
- Line 141: The FORMATTED assignment uses the glob pattern
packages/**/package.json which relies on Bash's recursive globbing but doesn't
enable it; before the line that defines FORMATTED, enable recursive globbing by
invoking shopt -s globstar in the same shell step (so packages/**/package.json
expands correctly), and optionally enable nullglob (shopt -s nullglob) to avoid
literal patterns when there are no matches; ensure these shopt calls run in the
same shell/session that computes FORMATTED.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4f06933-9cb5-41e1-9d3f-b7842effde5b
📒 Files selected for processing (1)
.github/workflows/publish.yml
ancheetah
force-pushed
the
fix-packaging
branch
2 times, most recently
from
6df7ace to
8506d34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 132-133: The publish step currently replaces the real pnpm publish
command with a no-op echo which returns success, causing
steps.npmpublish.outcome to be 'success' and triggering the Slack notification
step (the Slack block around lines 136-156); either restore the original run
command (uncomment the pnpm publish invocation) so real publishes occur, or if
you intentionally disabled publishing, modify the Slack notification condition
to check a real flag (for example a new input like inputs.skip_publish or check
a job output that indicates a real publish) or change the condition from
checking steps.npmpublish.outcome to verify an explicit publish-success signal
before sending Slack messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df722467-aa73-4630-ae18-518c3eebf2ee
📒 Files selected for processing (1)
.github/workflows/publish.yml
.github/workflows/publish.yml
Outdated
| # run: pnpm publish -r --tag ${{ inputs.npm_tag }} --no-git-checks --access ${{ inputs.npm_access }} | ||
| run: echo "Skipping npm publish" |
There was a problem hiding this comment.
Snapshot publishing is disabled; Slack will report false positives.
The pnpm publish command is commented out and replaced with a no-op echo. Since the echo exits with code 0, steps.npmpublish.outcome will be 'success', causing the Slack notification (lines 136-156) to execute and report packages as "published" when they weren't actually published.
If this is intentional for testing, consider also skipping the Slack notification by updating the condition. If this was a debug artifact, restore the publish command before merging.
Option 1: Restore the publish command
- # run: pnpm publish -r --tag ${{ inputs.npm_tag }} --no-git-checks --access ${{ inputs.npm_access }}
- run: echo "Skipping npm publish"
+ run: pnpm publish -r --tag ${{ inputs.npm_tag }} --no-git-checks --access ${{ inputs.npm_access }}Option 2: If disabling is intentional, also skip Slack notification
- name: Format published packages for Slack
- if: steps.npmpublish.outcome == 'success'
+ if: false # Temporarily disabled while snapshot publishing is skipped
id: format-packages📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # run: pnpm publish -r --tag ${{ inputs.npm_tag }} --no-git-checks --access ${{ inputs.npm_access }} | |
| run: echo "Skipping npm publish" | |
| run: pnpm publish -r --tag ${{ inputs.npm_tag }} --no-git-checks --access ${{ inputs.npm_access }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/publish.yml around lines 132 - 133, The publish step
currently replaces the real pnpm publish command with a no-op echo which returns
success, causing steps.npmpublish.outcome to be 'success' and triggering the
Slack notification step (the Slack block around lines 136-156); either restore
the original run command (uncomment the pnpm publish invocation) so real
publishes occur, or if you intentionally disabled publishing, modify the Slack
notification condition to check a real flag (for example a new input like
inputs.skip_publish or check a job output that indicates a real publish) or
change the condition from checking steps.npmpublish.outcome to verify an
explicit publish-success signal before sending Slack messages.
![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.
ancheetah
force-pushed
the
fix-packaging
branch
2 times, most recently
from
887ea2e to
753460e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ancheetah
force-pushed
the
fix-packaging
branch
from
753460e to
f13c849
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ancheetah
force-pushed
the
fix-packaging
branch
from
f13c849 to
861dcda
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ancheetah
force-pushed
the
fix-packaging
branch
from
861dcda to
691b608
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 reverted the accidental -zzz suffix from the Playwright button selector in protect-native.test.ts, restoring it to 'Sign In'. The test was timing out because no button named 'Sign In - zzz' exists in the application UI. This fix aligns with the PR's stated intent, which involves package distribution changes and has no relation to modifying test selectors.
Tip
✅ We verified this fix by re-running @forgerock/protect-suites:e2e-ci--src/protect-native.test.ts.
diff --git a/e2e/protect-suites/src/protect-native.test.ts b/e2e/protect-suites/src/protect-native.test.ts
index 1a707255b..077117092 100644
--- a/e2e/protect-suites/src/protect-native.test.ts
+++ b/e2e/protect-suites/src/protect-native.test.ts
@@ -30,7 +30,7 @@ test.describe('Test basic login flow with Ping Protect', () => {
await page.getByPlaceholder('Username').fill(username);
await page.getByPlaceholder('Password').fill(password);
- await page.getByRole('button', { name: 'Sign In - zzz' }).click();
+ await page.getByRole('button', { name: 'Sign In' }).click();
await expect(page.getByText('Protect evaluating')).toBeVisible();
Or Apply changes locally with:
npx nx-cloud apply-locally 7wTr-Dr57
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
ancheetah
force-pushed
the
fix-packaging
branch
5 times, most recently
from
6243f40 to
31d848a
Compare
ancheetah
force-pushed
the
fix-packaging
branch
from
31d848a to
f70410c
Compare
ancheetah
force-pushed
the
fix-packaging
branch
from
f70410c to
20e7e28
Compare
2 participants
JIRA Ticket
Description
/distfolder.Yes there is a patch changeset.
Summary by CodeRabbit