Add branding guide page to docs#3034
Open
jeradrutnam wants to merge 1 commit into
Open
Conversation
Contributor
|
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 Brand Guidelines documentation page with reusable UI subcomponents, wires it into docs via MDX and the footer, and updates CI workflows to cache Playwright browser artifacts and run browser installs only on cache miss. ChangesBrand Guidelines Page
CI, Workspace, and E2E
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/src/components/BrandPage.tsx`:
- Around line 557-589: LOGO_VARIANTS and ICON_VARIANTS currently hardcode
"ThunderID" in their alt text; move these constant definitions into the
BrandPage component (where productName is read from siteConfig) or refactor them
into a factory function that accepts productName, then update the alt strings to
interpolate productName (e.g., `${productName} logo — light background`) and
remove the top-level declarations so no product name is hardcoded; reference
LOGO_VARIANTS, ICON_VARIANTS, productName, BrandPage and siteConfig when making
the change.
- Around line 848-850: Replace the hardcoded "ThunderID" string inside the JSX
Typography block with the shared productName variable used across docs; locate
the Typography element in BrandPage.tsx (the block containing "When referring to
the project in text, use <strong>ThunderID</strong>") and interpolate
productName where the literal appears (ensure you import or reference the
existing productName symbol used elsewhere in docs/src, and keep the surrounding
markup and capitalization consistent).
- Around line 626-632: The NAME_DO constant is hardcoded with "ThunderID"; move
the NAME_DO declaration into the component where productName is available and
replace the literal with productName (e.g., set NAME_DO = [{ type: 'do', text:
productName }]); keep NAME_DONT as-is or adjust if any entries should also
reference productName, and remove the original top-level NAME_DO declaration so
the component uses the config-derived productName value instead.
In `@docs/src/pages/brand.mdx`:
- Line 3: The frontmatter `description` value contains the hardcoded brand name
"ThunderID"; update the `description` field to use the `{{ProductName}}`
placeholder instead (e.g., replace "ThunderID" with `{{ProductName}}`) so
parseFrontmatter can substitute it at build time—ensure only the `description`
frontmatter entry is changed and the surrounding punctuation/format stays valid.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f687fcdb-7f52-4ed6-9827-6773a9cdee22
⛔ Files ignored due to path filters (4)
docs/static/assets/images/logo-inverted.pngis excluded by!**/*.pngdocs/static/assets/images/logo-mini-inverted.pngis excluded by!**/*.pngdocs/static/assets/images/logo-mini.pngis excluded by!**/*.pngdocs/static/assets/images/logo.pngis excluded by!**/*.png
📒 Files selected for processing (7)
docs/src/components/BrandPage.tsxdocs/src/components/Footer.tsxdocs/src/pages/brand.mdxdocs/static/assets/images/logo-inverted.webpdocs/static/assets/images/logo-mini-inverted.webpdocs/static/assets/images/logo-mini.webpdocs/static/assets/images/logo.webp
Comment on lines
+557
to
+589
| const LOGO_VARIANTS: AssetVariant[] = [ | ||
| { | ||
| bg: '#ffffff', | ||
| src: '/assets/images/logo.svg', | ||
| alt: 'ThunderID logo — light background', | ||
| label: 'Dark on light', | ||
| name: 'thunderid-logo', | ||
| }, | ||
| { | ||
| bg: '#05213F', | ||
| src: '/assets/images/logo-inverted.svg', | ||
| alt: 'ThunderID logo — dark background', | ||
| label: 'Light on dark', | ||
| name: 'thunderid-logo-inverted', | ||
| }, | ||
| ]; | ||
|
|
||
| const ICON_VARIANTS: AssetVariant[] = [ | ||
| { | ||
| bg: '#ffffff', | ||
| src: '/assets/images/logo-mini.svg', | ||
| alt: 'ThunderID icon — light background', | ||
| label: 'Dark on light', | ||
| name: 'thunderid-icon', | ||
| }, | ||
| { | ||
| bg: '#05213F', | ||
| src: '/assets/images/logo-mini-inverted.svg', | ||
| alt: 'ThunderID icon — dark background', | ||
| label: 'Light on dark', | ||
| name: 'thunderid-icon-inverted', | ||
| }, | ||
| ]; |
Contributor
There was a problem hiding this comment.
Hardcoded brand name in alt text — replace with config-derived value.
The alt properties in LOGO_VARIANTS and ICON_VARIANTS contain hardcoded "ThunderID" string literals. As per coding guidelines, .tsx files under docs/src/ must not hardcode the product name; always derive it from config.
Since productName is already read from siteConfig (line 636), the constants should be moved inside the component body or refactored to accept productName as a parameter.
♻️ Proposed fix: Move constants inside component to use productName
Move the constants inside BrandPage and use the productName variable:
export default function BrandPage(): JSX.Element {
const {siteConfig} = useDocusaurusContext();
const productName = (siteConfig.customFields?.product as DocusaurusProductConfig).project.name;
const repoUrl = (siteConfig.customFields?.product as DocusaurusProductConfig).project.source.github.url;
+ const LOGO_VARIANTS: AssetVariant[] = [
+ {
+ bg: '`#ffffff`',
+ src: '/assets/images/logo.svg',
+ alt: `${productName} logo — light background`,
+ label: 'Dark on light',
+ name: 'thunderid-logo',
+ },
+ {
+ bg: '`#05213F`',
+ src: '/assets/images/logo-inverted.svg',
+ alt: `${productName} logo — dark background`,
+ label: 'Light on dark',
+ name: 'thunderid-logo-inverted',
+ },
+ ];
+
+ const ICON_VARIANTS: AssetVariant[] = [
+ {
+ bg: '`#ffffff`',
+ src: '/assets/images/logo-mini.svg',
+ alt: `${productName} icon — light background`,
+ label: 'Dark on light',
+ name: 'thunderid-icon',
+ },
+ {
+ bg: '`#05213F`',
+ src: '/assets/images/logo-mini-inverted.svg',
+ alt: `${productName} icon — dark background`,
+ label: 'Light on dark',
+ name: 'thunderid-icon-inverted',
+ },
+ ];Then remove the old constant declarations from lines 557–589.
As per coding guidelines for .tsx files under docs/src/.
📝 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.
Suggested change
| const LOGO_VARIANTS: AssetVariant[] = [ | |
| { | |
| bg: '#ffffff', | |
| src: '/assets/images/logo.svg', | |
| alt: 'ThunderID logo — light background', | |
| label: 'Dark on light', | |
| name: 'thunderid-logo', | |
| }, | |
| { | |
| bg: '#05213F', | |
| src: '/assets/images/logo-inverted.svg', | |
| alt: 'ThunderID logo — dark background', | |
| label: 'Light on dark', | |
| name: 'thunderid-logo-inverted', | |
| }, | |
| ]; | |
| const ICON_VARIANTS: AssetVariant[] = [ | |
| { | |
| bg: '#ffffff', | |
| src: '/assets/images/logo-mini.svg', | |
| alt: 'ThunderID icon — light background', | |
| label: 'Dark on light', | |
| name: 'thunderid-icon', | |
| }, | |
| { | |
| bg: '#05213F', | |
| src: '/assets/images/logo-mini-inverted.svg', | |
| alt: 'ThunderID icon — dark background', | |
| label: 'Light on dark', | |
| name: 'thunderid-icon-inverted', | |
| }, | |
| ]; | |
| export default function BrandPage(): JSX.Element { | |
| const {siteConfig} = useDocusaurusContext(); | |
| const productName = (siteConfig.customFields?.product as DocusaurusProductConfig).project.name; | |
| const repoUrl = (siteConfig.customFields?.product as DocusaurusProductConfig).project.source.github.url; | |
| const LOGO_VARIANTS: AssetVariant[] = [ | |
| { | |
| bg: '`#ffffff`', | |
| src: '/assets/images/logo.svg', | |
| alt: `${productName} logo — light background`, | |
| label: 'Dark on light', | |
| name: 'thunderid-logo', | |
| }, | |
| { | |
| bg: '`#05213F`', | |
| src: '/assets/images/logo-inverted.svg', | |
| alt: `${productName} logo — dark background`, | |
| label: 'Light on dark', | |
| name: 'thunderid-logo-inverted', | |
| }, | |
| ]; | |
| const ICON_VARIANTS: AssetVariant[] = [ | |
| { | |
| bg: '`#ffffff`', | |
| src: '/assets/images/logo-mini.svg', | |
| alt: `${productName} icon — light background`, | |
| label: 'Dark on light', | |
| name: 'thunderid-icon', | |
| }, | |
| { | |
| bg: '`#05213F`', | |
| src: '/assets/images/logo-mini-inverted.svg', | |
| alt: `${productName} icon — dark background`, | |
| label: 'Light on dark', | |
| name: 'thunderid-icon-inverted', | |
| }, | |
| ]; | |
| // ... rest of component body |
🤖 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 `@docs/src/components/BrandPage.tsx` around lines 557 - 589, LOGO_VARIANTS and
ICON_VARIANTS currently hardcode "ThunderID" in their alt text; move these
constant definitions into the BrandPage component (where productName is read
from siteConfig) or refactor them into a factory function that accepts
productName, then update the alt strings to interpolate productName (e.g.,
`${productName} logo — light background`) and remove the top-level declarations
so no product name is hardcoded; reference LOGO_VARIANTS, ICON_VARIANTS,
productName, BrandPage and siteConfig when making the change.
Comment on lines
+626
to
+632
| const NAME_DO: RuleItem[] = [{type: 'do', text: 'ThunderID'}]; | ||
| const NAME_DONT: RuleItem[] = [ | ||
| {type: 'dont', text: 'Thunder ID'}, | ||
| {type: 'dont', text: 'Thunderid'}, | ||
| {type: 'dont', text: 'thunderid'}, | ||
| {type: 'dont', text: 'Thunder-ID'}, | ||
| ]; |
Contributor
There was a problem hiding this comment.
Hardcoded brand name in NAME_DO constant — replace with config-derived value.
Line 626 contains a hardcoded "ThunderID" string literal. As per coding guidelines, .tsx files under docs/src/ must derive the product name from config rather than hardcoding it.
Move this constant inside the component and use productName:
♻️ Proposed fix
export default function BrandPage(): JSX.Element {
const {siteConfig} = useDocusaurusContext();
const productName = (siteConfig.customFields?.product as DocusaurusProductConfig).project.name;
const repoUrl = (siteConfig.customFields?.product as DocusaurusProductConfig).project.source.github.url;
+ const NAME_DO: RuleItem[] = [{type: 'do', text: productName}];Then remove the old declaration at line 626.
As per coding guidelines for .tsx files under docs/src/.
📝 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.
Suggested change
| const NAME_DO: RuleItem[] = [{type: 'do', text: 'ThunderID'}]; | |
| const NAME_DONT: RuleItem[] = [ | |
| {type: 'dont', text: 'Thunder ID'}, | |
| {type: 'dont', text: 'Thunderid'}, | |
| {type: 'dont', text: 'thunderid'}, | |
| {type: 'dont', text: 'Thunder-ID'}, | |
| ]; | |
| export default function BrandPage(): JSX.Element { | |
| const {siteConfig} = useDocusaurusContext(); | |
| const productName = (siteConfig.customFields?.product as DocusaurusProductConfig).project.name; | |
| const repoUrl = (siteConfig.customFields?.product as DocusaurusProductConfig).project.source.github.url; | |
| const NAME_DO: RuleItem[] = [{type: 'do', text: productName}]; | |
| const NAME_DONT: RuleItem[] = [ | |
| {type: 'dont', text: 'Thunder ID'}, | |
| {type: 'dont', text: 'Thunderid'}, | |
| {type: 'dont', text: 'thunderid'}, | |
| {type: 'dont', text: 'Thunder-ID'}, | |
| ]; |
🤖 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 `@docs/src/components/BrandPage.tsx` around lines 626 - 632, The NAME_DO
constant is hardcoded with "ThunderID"; move the NAME_DO declaration into the
component where productName is available and replace the literal with
productName (e.g., set NAME_DO = [{ type: 'do', text: productName }]); keep
NAME_DONT as-is or adjust if any entries should also reference productName, and
remove the original top-level NAME_DO declaration so the component uses the
config-derived productName value instead.
Comment on lines
+848
to
+850
| <Typography sx={{color: 'text.secondary', mb: 3, fontSize: '0.95rem', lineHeight: 1.7, maxWidth: '600px'}}> | ||
| When referring to the project in text, use <strong>ThunderID</strong> — one word, capital T and capital ID. | ||
| </Typography> |
Contributor
There was a problem hiding this comment.
Hardcoded brand name in JSX prose — replace with productName variable.
Line 849 contains a hardcoded "ThunderID" string literal inside JSX content. As per coding guidelines, .tsx files under docs/src/ must not hardcode the product name.
♻️ Proposed fix
- When referring to the project in text, use <strong>ThunderID</strong> — one word, capital T and capital ID.
+ When referring to the project in text, use <strong>{productName}</strong> — one word, capital T and capital ID.As per coding guidelines for .tsx files under docs/src/.
🤖 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 `@docs/src/components/BrandPage.tsx` around lines 848 - 850, Replace the
hardcoded "ThunderID" string inside the JSX Typography block with the shared
productName variable used across docs; locate the Typography element in
BrandPage.tsx (the block containing "When referring to the project in text, use
<strong>ThunderID</strong>") and interpolate productName where the literal
appears (ensure you import or reference the existing productName symbol used
elsewhere in docs/src, and keep the surrounding markup and capitalization
consistent).
| @@ -0,0 +1,9 @@ | |||
| --- | |||
| title: Brand Guidelines | |||
| description: ThunderID brand guidelines — logos, colors, technical specifications, and usage rules for press and community use. | |||
Contributor
There was a problem hiding this comment.
Hardcoded brand name in frontmatter — replace with {{ProductName}} placeholder.
The description field contains hardcoded "ThunderID". As per coding guidelines, .mdx file frontmatter fields must use the {{ProductName}} placeholder, which is replaced at build time via parseFrontmatter.
♻️ Proposed fix
-description: ThunderID brand guidelines — logos, colors, technical specifications, and usage rules for press and community use.
+description: {{ProductName}} brand guidelines — logos, colors, technical specifications, and usage rules for press and community use.As per coding guidelines for .mdx frontmatter fields under docs/**.
📝 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.
Suggested change
| description: ThunderID brand guidelines — logos, colors, technical specifications, and usage rules for press and community use. | |
| description: {{ProductName}} brand guidelines — logos, colors, technical specifications, and usage rules for press and community use. |
🤖 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 `@docs/src/pages/brand.mdx` at line 3, The frontmatter `description` value
contains the hardcoded brand name "ThunderID"; update the `description` field to
use the `{{ProductName}}` placeholder instead (e.g., replace "ThunderID" with
`{{ProductName}}`) so parseFrontmatter can substitute it at build time—ensure
only the `description` frontmatter entry is changed and the surrounding
punctuation/format stays valid.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/pr-builder.yml:
- Around line 365-371: The workflow change adds/modifies CI steps (the "🎭 Cache
Playwright Browsers" step with id playwright-cache in
.github/workflows/pr-builder.yml) which per repo policy requires explicit
approval; before merging, update the PR to capture that approval (e.g., add an
explicit approval note in the PR description, include a link to the recorded
approval ticket/issue, or add the required "ci-workflow-approved" label or
signed-off comment from an authorized approver) and mention/quote that approval
in the PR so reviewers can verify it; ensure the same approval is
applied/recorded for the related changes around lines 899-905 referenced in the
review.
- Around line 906-909: The workflow step currently named "🎭 Install Playwright
Browsers" is skipped when steps.playwright-cache-e2e.outputs.cache-hit ==
'true', which prevents running npx playwright install --with-deps and thus can
leave OS-level Playwright dependencies missing; update the workflow so the
command npx playwright install --with-deps always runs (remove or change the if
condition on the step that references playwright-cache-e2e) or split into two
steps: one unconditional step that runs npx playwright install --with-deps to
install system deps and a separate conditional step that restores
cache/playwright browsers using the playwright-cache-e2e output.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dc14de7-6f8b-4350-9284-44425799f395
📒 Files selected for processing (1)
.github/workflows/pr-builder.yml
Comment on lines
+365
to
+371
| - name: 🎭 Cache Playwright Browsers | ||
| uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4 | ||
| id: playwright-cache | ||
| with: | ||
| path: ~/.cache/ms-playwright | ||
| key: playwright-chromium-${{ runner.os }}-${{ hashFiles('pnpm-workspace.yaml') }} | ||
|
|
Contributor
There was a problem hiding this comment.
CI workflow change needs explicit approval per repo policy.
This PR modifies .github/workflows/pr-builder.yml (new cache steps and install gating), but I don’t see explicit approval captured in the provided context. Please add/confirm that approval before merge.
As per coding guidelines: "{package.json,package-lock.json,go.mod,go.sum,.github/workflows/**,Makefile,Dockerfile*}: Do not add new dependencies or modify CI/CD pipelines, GitHub Actions, or Makefiles without explicit approval."
Also applies to: 899-905
🤖 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 @.github/workflows/pr-builder.yml around lines 365 - 371, The workflow change
adds/modifies CI steps (the "🎭 Cache Playwright Browsers" step with id
playwright-cache in .github/workflows/pr-builder.yml) which per repo policy
requires explicit approval; before merging, update the PR to capture that
approval (e.g., add an explicit approval note in the PR description, include a
link to the recorded approval ticket/issue, or add the required
"ci-workflow-approved" label or signed-off comment from an authorized approver)
and mention/quote that approval in the PR so reviewers can verify it; ensure the
same approval is applied/recorded for the related changes around lines 899-905
referenced in the review.
brionmario
previously approved these changes
May 27, 2026
Contributor
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 @.github/workflows/pr-builder.yml:
- Around line 370-374: Update the Playwright cache key so it incorporates the
pnpm lockfile hash in addition to pnpm-workspace.yaml: change the key expression
used for the Playwright browser cache (the string starting with "key:
playwright-chromium-${{ runner.os }}-${{ hashFiles('pnpm-workspace.yaml') }}")
to include pnpm-lock.yaml in the hashFiles call (e.g.,
hashFiles('pnpm-workspace.yaml','pnpm-lock.yaml')) so that the cache key changes
when the Playwright version changes; ensure the conditional that checks
steps.playwright-cache.outputs.cache-hit remains the same so the install step
only runs on cache miss.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: efd084d2-91cc-4260-86c7-9f0042369f68
📒 Files selected for processing (1)
.github/workflows/pr-builder.yml
brionmario
previously approved these changes
May 27, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@package.json`:
- Line 36: The change modifies the restricted manifest entry "test" in
package.json (the npm script string "nx run-many --target=test --all --parallel
--exclude=sdks/**,tools/**,tests/**"); revert this edit or explicitly obtain and
record the required approval before merging: either restore the original
package.json value or add a PR comment/approval from an authorized approver and
include an approval note in the PR description referencing this package.json
change so CI/security reviewers can verify it; ensure the commit that alters the
"test" script is not merged until that explicit approval is present.
In `@tests/e2e/tests/accessibility/sign-in-page.a11y.spec.ts`:
- Around line 172-177: The test currently swallows the timeout from
page.locator(...).first().waitFor(...) which lets the "error state" test pass
even when no error UI renders; change this to explicitly fail when no error
element appears by removing the catch and asserting visibility (e.g., use
Playwright's expect(locator).toBeVisible({ timeout: 5000 }) or rethrow the
timeout error) so the test fails if the locator (the "[role='alert'],
[aria-invalid='true'], .error, .validation-error" locator) does not become
visible within the timeout.
In `@tests/e2e/tests/applications/application-edit.spec.ts`:
- Around line 353-359: The cleanup DELETE currently runs even when
deleteTestAppId or adminToken are missing and also swallows all errors; update
the teardown to first check that deleteTestAppId and adminToken are truthy and
only call request.delete when both exist (use the existing deleteTestAppId and
adminToken variables and the request/serverUrl values), and if you still catch
errors ensure they are surfaced or logged instead of being fully ignored so
failures in delete of the test application aren’t silently masked.
In `@tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts`:
- Around line 519-530: Tests assume token/user responses are valid before
dereferencing; add explicit validation of tokenResponse and userResponse and
guard access to tokenData.access_token and userData.users[0].id. After calling
tokenResponse.json(), check tokenResponse.ok and that tokenData.access_token is
present before using adminToken; after userResponse.json(), check
userResponse.ok and that userData.users is an array with length>0 before
assigning createdUserId and pushing to createdUserIds, and otherwise log/throw a
clear error or skip adding to cleanup so test users are not silently left
behind.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d57e0a0-23ce-42d2-9dfb-3959f7b2ec32
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/e2e/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/pr-builder.ymlpackage.jsonpnpm-workspace.yamltests/e2e/package.jsontests/e2e/tests/accessibility/sign-in-page.a11y.spec.tstests/e2e/tests/admin-authentication/auth.setup.tstests/e2e/tests/applications/application-edit.spec.tstests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/tests/admin-authentication/auth.setup.ts
| "lint": "nx run-many --target=lint --all --parallel --exclude=sdks/**", | ||
| "lint:fix": "nx run-many --target=fix:lint --all --parallel", | ||
| "test": "nx run-many --target=test --all --parallel --exclude=sdks/**,tools/**", | ||
| "test": "nx run-many --target=test --all --parallel --exclude=sdks/**,tools/**,tests/**", |
Contributor
There was a problem hiding this comment.
Require explicit approval for this package.json change before merge.
Line 36 modifies a restricted manifest file, and I don’t see explicit approval captured in the provided context.
As per coding guidelines: "{package.json,package-lock.json,go.mod,go.sum,.github/workflows/**,Makefile,Dockerfile*}: Do not add new dependencies or modify CI/CD pipelines, GitHub Actions, or Makefiles without explicit approval."
🤖 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 `@package.json` at line 36, The change modifies the restricted manifest entry
"test" in package.json (the npm script string "nx run-many --target=test --all
--parallel --exclude=sdks/**,tools/**,tests/**"); revert this edit or explicitly
obtain and record the required approval before merging: either restore the
original package.json value or add a PR comment/approval from an authorized
approver and include an approval note in the PR description referencing this
package.json change so CI/security reviewers can verify it; ensure the commit
that alters the "test" script is not merged until that explicit approval is
present.
Comment on lines
+172
to
+177
| // Wait for actual error UI to appear instead of arbitrary timeout | ||
| await page.locator( | ||
| "[role='alert'], [aria-invalid='true'], .error, .validation-error", | ||
| ).first().waitFor({ state: "visible", timeout: 5000 }).catch(() => { | ||
| // If no error element appears, proceed anyway — the page state is still valid to audit | ||
| }); |
Contributor
There was a problem hiding this comment.
Fail the test when no error-state UI appears.
At Line 176, the swallowed timeout lets an “error state” test pass even if no error UI is rendered.
Suggested fix
- await page.locator(
- "[role='alert'], [aria-invalid='true'], .error, .validation-error",
- ).first().waitFor({ state: "visible", timeout: 5000 }).catch(() => {
- // If no error element appears, proceed anyway — the page state is still valid to audit
- });
+ await expect(
+ page.locator("[role='alert'], [aria-invalid='true'], .error, .validation-error").first(),
+ ).toBeVisible({ timeout: 5000 });📝 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.
Suggested change
| // Wait for actual error UI to appear instead of arbitrary timeout | |
| await page.locator( | |
| "[role='alert'], [aria-invalid='true'], .error, .validation-error", | |
| ).first().waitFor({ state: "visible", timeout: 5000 }).catch(() => { | |
| // If no error element appears, proceed anyway — the page state is still valid to audit | |
| }); | |
| // Wait for actual error UI to appear instead of arbitrary timeout | |
| await expect( | |
| page.locator("[role='alert'], [aria-invalid='true'], .error, .validation-error").first(), | |
| ).toBeVisible({ timeout: 5000 }); |
🤖 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 `@tests/e2e/tests/accessibility/sign-in-page.a11y.spec.ts` around lines 172 -
177, The test currently swallows the timeout from
page.locator(...).first().waitFor(...) which lets the "error state" test pass
even when no error UI renders; change this to explicitly fail when no error
element appears by removing the catch and asserting visibility (e.g., use
Playwright's expect(locator).toBeVisible({ timeout: 5000 }) or rethrow the
timeout error) so the test fails if the locator (the "[role='alert'],
[aria-invalid='true'], .error, .validation-error" locator) does not become
visible within the timeout.
Comment on lines
+353
to
+359
| const adminToken = await getAdminToken(request).catch(() => null); | ||
| await request | ||
| .delete(`${serverUrl}/applications/${deleteTestAppId ?? ""}`, { | ||
| headers: { Authorization: `Bearer ${adminToken ?? ""}` }, | ||
| ignoreHTTPSErrors: true, | ||
| }) | ||
| .catch(() => {}); |
Contributor
There was a problem hiding this comment.
Guard cleanup DELETE when ID/token are missing.
Line 355 can hit /applications/ with an empty ID, and failures are fully swallowed, which can mask cleanup breakage.
Suggested fix
- const adminToken = await getAdminToken(request).catch(() => null);
- await request
- .delete(`${serverUrl}/applications/${deleteTestAppId ?? ""}`, {
- headers: { Authorization: `Bearer ${adminToken ?? ""}` },
- ignoreHTTPSErrors: true,
- })
- .catch(() => {});
+ if (!deleteTestAppId) return;
+ const adminToken = await getAdminToken(request).catch(() => null);
+ if (!adminToken) return;
+ await request.delete(`${serverUrl}/applications/${deleteTestAppId}`, {
+ headers: { Authorization: `Bearer ${adminToken}` },
+ ignoreHTTPSErrors: true,
+ }).catch(() => {});📝 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.
Suggested change
| const adminToken = await getAdminToken(request).catch(() => null); | |
| await request | |
| .delete(`${serverUrl}/applications/${deleteTestAppId ?? ""}`, { | |
| headers: { Authorization: `Bearer ${adminToken ?? ""}` }, | |
| ignoreHTTPSErrors: true, | |
| }) | |
| .catch(() => {}); | |
| if (!deleteTestAppId) return; | |
| const adminToken = await getAdminToken(request).catch(() => null); | |
| if (!adminToken) return; | |
| await request.delete(`${serverUrl}/applications/${deleteTestAppId}`, { | |
| headers: { Authorization: `Bearer ${adminToken}` }, | |
| ignoreHTTPSErrors: true, | |
| }).catch(() => {}); |
🤖 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 `@tests/e2e/tests/applications/application-edit.spec.ts` around lines 353 -
359, The cleanup DELETE currently runs even when deleteTestAppId or adminToken
are missing and also swallows all errors; update the teardown to first check
that deleteTestAppId and adminToken are truthy and only call request.delete when
both exist (use the existing deleteTestAppId and adminToken variables and the
request/serverUrl values), and if you still catch errors ensure they are
surfaced or logged instead of being fully ignored so failures in delete of the
test application aren’t silently masked.
Comment on lines
+519
to
+530
| const tokenData = await tokenResponse.json(); | ||
| const adminToken = tokenData.access_token; | ||
|
|
||
| const userResponse = await request.get(`${serverUrl}/users?filter=username eq "${regUsername}"`, { | ||
| headers: { | ||
| Authorization: `Bearer ${adminToken}`, | ||
| }, | ||
| ignoreHTTPSErrors: true, | ||
| }); | ||
| const userResponse = await request.get(`${serverUrl}/users?filter=username eq "${regUsername}"`, { | ||
| headers: { Authorization: `Bearer ${adminToken}` }, | ||
| ignoreHTTPSErrors: true, | ||
| }); | ||
|
|
||
| if (userResponse.ok()) { | ||
| const userData = await userResponse.json(); | ||
| if (userData.users && userData.users.length > 0) { | ||
| createdUserId = userData.users[0].id; | ||
| createdUserIds.push(createdUserId); | ||
| console.log(`✓ User ID ${createdUserId} added to cleanup list`); | ||
| } | ||
| } | ||
| } | ||
| const userData = await userResponse.json(); | ||
| createdUserId = userData.users[0].id as string; | ||
| createdUserIds.push(createdUserId); | ||
| console.log(`✓ User ID ${createdUserId} added to cleanup list`); |
Contributor
There was a problem hiding this comment.
Validate token/user responses before dereferencing cleanup user ID.
Lines 519-529 assume valid auth and a non-empty users array; when either fails, cleanup silently skips and leaves test users behind.
Suggested fix
- const tokenData = await tokenResponse.json();
- const adminToken = tokenData.access_token;
+ if (!tokenResponse.ok()) throw new Error(`Admin token request failed: ${tokenResponse.status()}`);
+ const tokenData = await tokenResponse.json();
+ const adminToken = tokenData.access_token as string;
+ if (!adminToken) throw new Error("Missing admin access token");
const userResponse = await request.get(`${serverUrl}/users?filter=username eq "${regUsername}"`, {
headers: { Authorization: `Bearer ${adminToken}` },
ignoreHTTPSErrors: true,
});
- const userData = await userResponse.json();
- createdUserId = userData.users[0].id as string;
+ if (!userResponse.ok()) throw new Error(`User lookup failed: ${userResponse.status()}`);
+ const userData = await userResponse.json();
+ createdUserId = userData.users?.[0]?.id ?? null;
+ if (!createdUserId) throw new Error("No user ID found for cleanup");
createdUserIds.push(createdUserId);🤖 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 `@tests/e2e/tests/sample-app-authentication/sample-app-mfa-login.spec.ts`
around lines 519 - 530, Tests assume token/user responses are valid before
dereferencing; add explicit validation of tokenResponse and userResponse and
guard access to tokenData.access_token and userData.users[0].id. After calling
tokenResponse.json(), check tokenResponse.ok and that tokenData.access_token is
present before using adminToken; after userResponse.json(), check
userResponse.ok and that userData.users is an array with length>0 before
assigning createdUserId and pushing to createdUserIds, and otherwise log/throw a
clear error or skip adding to cleanup so test users are not silently left
behind.
jeradrutnam
force-pushed
the
docs-style
branch
2 times, most recently
from
be6f3e2 to
7cc4f58
Compare
jeradrutnam
changed the title
jeradrutnam
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces "Brand Guidelines" page to the documentation and linked it in the footer for easier access. [1] [2]