Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✨ 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 01d04e3
☁️ Nx Cloud last updated this comment at |
🦋 Changeset detectedLatest commit: 01d04e3 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (15.89%) 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 #568 +/- ##
===========================================
- Coverage 70.90% 15.89% -55.01%
===========================================
Files 53 154 +101
Lines 2021 26737 +24716
Branches 377 1148 +771
===========================================
+ Hits 1433 4251 +2818
- Misses 588 22486 +21898
🚀 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 6506bae to https://ForgeRock.github.io/ping-javascript-sdk/pr-568/6506bae0960df0a3795c31723065c11af2cf03af branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-9.9 KB, -100.0%) 📊 Minor Changes📉 @forgerock/device-client - 9.9 KB (-0.0 KB) ➖ No Changes➖ @forgerock/protect - 150.1 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ryanbas21
force-pushed
the
rich-content-links
branch
4 times, most recently
from
3e5d66d to
d6223eb
Compare
![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 is proposing a fix for your failed CI:
We fix three lint errors introduced by the richContent feature: a Prettier formatting violation on long expectTypeOf chains in the type-test file, an inline object that exceeded Prettier's print width in the unit test, and an unused CollectorRichContent import in collector.utils.ts. These changes bring the code into compliance with the workspace's ESLint and Prettier rules without altering any runtime or type-level behaviour.
Tip
✅ We verified this fix by re-running @forgerock/davinci-client:lint.
Suggested Fix changes
diff --git a/packages/davinci-client/src/lib/collector.richcontent.test-d.ts b/packages/davinci-client/src/lib/collector.richcontent.test-d.ts
index f671329c3..6142ef232 100644
--- a/packages/davinci-client/src/lib/collector.richcontent.test-d.ts
+++ b/packages/davinci-client/src/lib/collector.richcontent.test-d.ts
@@ -72,12 +72,18 @@ describe('Rich Content Types', () => {
});
it('should have required richContent with CollectorRichContent shape', () => {
- expectTypeOf<ReadOnlyCollectorBase['output']['richContent']>().toEqualTypeOf<CollectorRichContent>();
+ expectTypeOf<
+ ReadOnlyCollectorBase['output']['richContent']
+ >().toEqualTypeOf<CollectorRichContent>();
});
it('should have standard collector fields', () => {
- expectTypeOf<ReadOnlyCollectorBase>().toHaveProperty('category').toEqualTypeOf<'NoValueCollector'>();
- expectTypeOf<ReadOnlyCollectorBase>().toHaveProperty('type').toEqualTypeOf<'ReadOnlyCollector'>();
+ expectTypeOf<ReadOnlyCollectorBase>()
+ .toHaveProperty('category')
+ .toEqualTypeOf<'NoValueCollector'>();
+ expectTypeOf<ReadOnlyCollectorBase>()
+ .toHaveProperty('type')
+ .toEqualTypeOf<'ReadOnlyCollector'>();
expectTypeOf<ReadOnlyCollectorBase>().toHaveProperty('error').toEqualTypeOf<string | null>();
});
});
diff --git a/packages/davinci-client/src/lib/collector.utils.test.ts b/packages/davinci-client/src/lib/collector.utils.test.ts
index 67316ca25..9ff5804b6 100644
--- a/packages/davinci-client/src/lib/collector.utils.test.ts
+++ b/packages/davinci-client/src/lib/collector.utils.test.ts
@@ -1273,7 +1273,13 @@ describe('validateReplacements', () => {
expect(result).toEqual({
ok: true,
replacements: [
- { key: 'link1', type: 'link', value: 'terms', href: 'https://example.com', target: '_blank' },
+ {
+ key: 'link1',
+ type: 'link',
+ value: 'terms',
+ href: 'https://example.com',
+ target: '_blank',
+ },
{ key: 'link2', type: 'link', value: 'policy', href: 'https://xyz.com', target: '_self' },
],
});
@@ -1292,9 +1298,7 @@ describe('validateReplacements', () => {
expect(result).toEqual({
ok: true,
- replacements: [
- { key: 'link', type: 'link', value: 'here', href: 'https://example.com' },
- ],
+ replacements: [{ key: 'link', type: 'link', value: 'here', href: 'https://example.com' }],
});
});
diff --git a/packages/davinci-client/src/lib/collector.utils.ts b/packages/davinci-client/src/lib/collector.utils.ts
index 757f88b92..17994836e 100644
--- a/packages/davinci-client/src/lib/collector.utils.ts
+++ b/packages/davinci-client/src/lib/collector.utils.ts
@@ -32,7 +32,6 @@ import type {
QrCodeCollectorBase,
ValidatedReplacement,
ValidateReplacementsResult,
- CollectorRichContent,
ReadOnlyCollectorBase,
} from './collector.types.js';
import type {
Or Apply changes locally with:
npx nx-cloud apply-locally 5kUe-VYSS
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
ryanbas21
force-pushed
the
rich-content-links
branch
2 times, most recently
from
24490a2 to
9e2532b
Compare
Support RichContent link types by creating a NoValueCollector for it
ryanbas21
force-pushed
the
rich-content-links
branch
from
9e2532b to
0b3a34e
Compare
ryanbas21
changed the title
- Add .nxignore to exclude vendored .opensource/ clone from Nx project graph (was causing duplicate-project errors vs forgerock-verdaccio). - Render authored line breaks in ReadOnlyCollector rich text via white-space: pre-line on the rendered <p>.
| label: string; | ||
| type: string; | ||
| content: string; | ||
| richContent: CollectorRichContent; |
There was a problem hiding this comment.
Rather than adding more properties to the base collector, can we just add a new collector type for this rich content collector? This new collector can just extent the base collector adding the new property on the output. Does that make sense?
| if (!['https:', 'http:'].includes(href.protocol)) { | ||
| return { ok: false, error: `Unsafe href protocol for key: ${key}` }; | ||
| } |
There was a problem hiding this comment.
What are we preventing here? I see we're rejecting URLs without a protocol, but I'm not aware of what this prevents.
| // Validate that all {{key}} references in the template have corresponding replacements | ||
| const templateKeys = [...field.richContent.content.matchAll(/\{\{(\w+)\}\}/g)].map((m) => m[1]); | ||
| const apiReplacements = field.richContent.replacements ?? {}; | ||
| const missingKeys = templateKeys.filter((k) => !(k in apiReplacements)); | ||
| const templateErrors = missingKeys.map((k) => `Missing replacement for key: {{${k}}}`); | ||
|
|
||
| const validationResult = | ||
| templateErrors.length === 0 ? validateReplacements(apiReplacements) : null; | ||
|
|
||
| const replacements = validationResult?.ok ? validationResult.replacements : []; | ||
| const validationErrors = validationResult && !validationResult.ok ? [validationResult.error] : []; | ||
| const errors = [...fieldErrors, ...templateErrors, ...validationErrors]; |
There was a problem hiding this comment.
I think this is a cool idea, but I'm unsure if we are the right ones to do this validation. I feel like DaVinci should be the one to validate all of this as they are the source. If this type of error exists within this data structure, the implementing application would pick it up without our help. Right? Or, am I missing something?
| const errors = fieldErrors; | ||
| return { | ||
| category: 'NoValueCollector', | ||
| error: errors.length > 0 ? errors.join(' ') : null, | ||
| type: 'ReadOnlyCollector', | ||
| id, | ||
| name: id, | ||
| output: { | ||
| key: id, | ||
| label: field.content, | ||
| type: field.type, | ||
| content: field.content, | ||
| richContent: { content: field.content, replacements: [] }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Is there a reason we don't just call the same function we called before this change: returnNoValueCollector?
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Looks good overall, suggested minor changes
| CollectorRichContent, | ||
| ValidateReplacementsResult, | ||
| NoValueCollector, | ||
| } from './collector.types.js'; |
There was a problem hiding this comment.
Since we're importing from collectory types file, we should just move this stuff into collector.types.test-d.ts file so that collector types are all in one place. A file like collector.richcontent looks confusing and stands out.
| target: string; | ||
| } | ||
|
|
||
| // @public (undocumented) |
There was a problem hiding this comment.
We should add jsdoc comments to types and interfaces to remove this undocumented label
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4248
Description
richContentstructure (template + validated replacements array) onReadOnlyCollectorChanges
RichContentReplacement,RichContent(with optionalreplacements)RichContentLink,ValidatedReplacement(extensible union),CollectorRichContent,ReadOnlyCollectorBasevalidateReplacements()— validates hrefs, convertsRecordto array withkeyfieldreturnReadOnlyCollector()—output.contentis plain text string,output.richContentis always present with template and validated replacements arrayjavascript:,data:, and other unsafe URI schemes (allowlist:http:,https:only)Collector Output Shape