Skip to content

Conversation

@ccastrotrejo
Copy link
Contributor

@ccastrotrejo ccastrotrejo commented Jan 22, 2026

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

This PR changes how window.IDENTITY_PROVIDERS is handled in the iframe chat app configuration parser. Previously, IDENTITY_PROVIDERS was expected to be a JavaScript object directly assigned to the window. This change updates it to expect a JSON string that gets parsed at runtime.

Why this change?

  • JSON strings are easier to inject via server-side templating (HTML replacement)
  • More robust handling when the value comes from external configuration
  • Proper error handling with try/catch when parsing fails

Impact of Change

  • Users: No user-facing changes - identity provider configuration works the same way
  • Developers: When setting window.IDENTITY_PROVIDERS, it must now be a JSON string (e.g., '{"aad":{"signInEndpoint":"/.auth/login/aad","name":"Microsoft"}}') instead of a JavaScript object
  • System: Adds defensive parsing with error logging when IDENTITY_PROVIDERS is malformed

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in: Local development with iframe app

Contributors

@ccastrotrejo

Screenshots/Videos

N/A - No visual changes

Copilot AI review requested due to automatic review settings January 22, 2026 22:00
@github-actions
Copy link

github-actions bot commented Jan 22, 2026

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

PR Title

  • Current: fix(iframe-app): Parse IDENTITY_PROVIDERS as JSON string instead of object
  • Issue: None — title is clear, follows conventional commit style and accurately describes the change.
  • Recommendation: (Optional) If this is a breaking change for external consumers, consider appending BREAKING CHANGE: to the description or adding a short line in the title/body that explicitly calls out the breaking change so release tooling and readers notice it quickly.

Commit Type

  • Properly selected (fix).
  • Commit type note: Only one option is selected (fix), which is correct given the change.

⚠️ Risk Level

  • The PR body and labels indicate: Low.
  • Assessment: The code changes alter the expected type of a public global (window.IDENTITY_PROVIDERS) from an object to a JSON string. That is a contract change for any consumers that previously set the global as a JS object and can cause that configuration to stop working unless converted to a string. Even though you added defensive fallbacks and updated tests and e2e flows, this is a backward-compatibility change and should be considered at least Medium risk.
  • Recommendation: Update the PR risk selection and the repository label from risk:low to risk:medium (or explicitly call this out as a breaking change). I have advised risk:medium above.

What & Why

  • Current: This PR updates parse logic to expect a JSON string in window.IDENTITY_PROVIDERS and parses it via a new parseIdentityProviders() helper. The body explains why (server-side templating easier, robust handling, and error handling).
  • Issue: The body states the required developer change, but it should more explicitly call out the compatibility impact for any consumers that currently set window.IDENTITY_PROVIDERS as a JS object (not a string).
  • Recommendation: Update the "What & Why" to explicitly state: this changes the runtime contract for how IDENTITY_PROVIDERS must be provided and that customers/consumers who previously set the global to a JS object must switch to providing a JSON string (give a concrete example and migration snippet). Example addition: BREAKING CHANGE: Consumers must now set window.IDENTITY_PROVIDERS to a JSON string (e.g. '\"{...}\"') instead of a JS object. Update server-side templates and any tests accordingly.

Impact of Change

  • Impact issue: The PR body already includes Users/Developers/System sections and mentions developer change. Good.
  • Recommendation:
    • Users: No immediate UI changes expected. If an environment relied on non-string injection, they may face sign-in configuration breakage. Add a short note for maintainers to check deployments that template the iframe page.
    • Developers: Explicitly call out that this is a breaking change to the public global contract and include a sample migration snippet in the PR body and/or a CHANGELOG entry.
    • System: No major system changes, but ensure release notes mention the compatibility change and any deployment templates are updated.

Test Plan

  • Test plan assessment: Good. The PR claims unit tests, E2E, and manual testing — the diff shows:
    • Unit tests added/updated for parseIdentityProviders and updated iframe config tests.
    • E2E tests updated to set IDENTITY_PROVIDERS as JSON strings and to replace templated HTML accordingly.
    • CI workflow updated to run the iframe-app and extension-unit tests.
  • Recommendation: Ensure any external integration tests or downstream repos that inject IDENTITY_PROVIDERS are updated, and call that out in the PR if applicable.

⚠️ Contributors

  • Contributors assessment: @ccastrotrejo is listed. Good.
  • Recommendation: If any PMs/designers/other reviewers contributed to the decision or testing, mention them here so they get credit and context.

Screenshots/Videos

  • Screenshots assessment: N/A — there are no visual changes, this is fine.

Summary Table

Section Status Recommendation
Title Good. Optionally call out breaking change in title/body.
Commit Type fix is appropriate.
Risk Level ⚠️ Upgrade to risk:medium (breaking change for consumers of window.IDENTITY_PROVIDERS).
What & Why Add an explicit "BREAKING CHANGE" note and a migration example.
Impact of Change Add a short migration checklist for devs and ops.
Test Plan Tests exist and were updated. Verify downstream consumers are covered.
Contributors ⚠️ Good, but consider adding credited reviewers/PMs if applicable.
Screenshots/Videos N/A — acceptable.

Final message:
Please update the PR to reflect the increased risk (change label to risk:medium) and add a brief explicit BREAKING CHANGE section in the PR body that:

  • Explains that the expected type of window.IDENTITY_PROVIDERS changed from a JS object to a JSON string.
  • Provides a one-line migration example (server-side templating example and a snippet showing how to stringify an object for injection).
  • Mentions that deployments or templates that previously injected an object must be updated.

Other than the risk/compatibility note, the PR title, tests, and body otherwise follow the template and the tests in the diff align with the claimed Test Plan. Thank you — once you update the risk label and add the small BREAKING CHANGE/migration note I recommend merging.


Last updated: Fri, 23 Jan 2026 15:38:01 GMT

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes how identity providers configuration is injected into the iframe application, moving from a JavaScript object format to a JSON string format to support server-side HTML injection.

Changes:

  • Modified window.IDENTITY_PROVIDERS type from Record<string, IdentityProvider> to string to support server-side string replacement in HTML
  • Added parseIdentityProviders() function to parse the JSON string at runtime with error handling
  • Updated all unit tests and E2E tests to use JSON string format instead of JavaScript object literals

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
apps/iframe-app/src/lib/utils/config-parser.ts Changed Window.IDENTITY_PROVIDERS type to string and added parseIdentityProviders() function to parse JSON string with fallback to defaults
apps/iframe-app/src/lib/utils/tests/config-parser.test.ts Updated test to use JSON string format for IDENTITY_PROVIDERS
e2e/chatClient/tests/features/authentication/login-prompt.spec.ts Updated all E2E tests to inject IDENTITY_PROVIDERS as JSON strings in both addInitScript and HTML replacement scenarios
Comments suppressed due to low confidence (1)

apps/iframe-app/src/lib/utils/tests/config-parser.test.ts:250

  • The test suite is missing coverage for error cases in the new parseIdentityProviders function. Following the pattern established for metadata parsing (see line 178-186), add tests for:
  1. Invalid JSON string - should fall back to DEFAULT_IDENTITY_PROVIDERS and log an error
  2. Valid JSON but not an object (e.g., array, string, number) - should fall back to DEFAULT_IDENTITY_PROVIDERS
  3. Empty string "" - should fall back to DEFAULT_IDENTITY_PROVIDERS

These tests would ensure the error handling in the parseIdentityProviders function works correctly and is consistent with the existing error handling patterns in this file.

    it('uses window.IDENTITY_PROVIDERS when set', () => {
      document.documentElement.dataset.agentCard = 'http://test.agent/agent-card.json';
      (window as any).IDENTITY_PROVIDERS = '{"custom":{"signInEndpoint":"/.auth/login/custom","name":"Custom Provider"}}';

      const config = parseIframeConfig();

      expect(config.props.identityProviders).toEqual({
        custom: {
          signInEndpoint: '/.auth/login/custom',
          name: 'Custom Provider',
        },
      });

      // Clean up
      delete (window as any).IDENTITY_PROVIDERS;
    });

@ccastrotrejo ccastrotrejo changed the title Ccastrotrejo/identity providers fix fix(iframe-app): Parse IDENTITY_PROVIDERS as JSON string instead of object Jan 22, 2026
@ccastrotrejo ccastrotrejo added the risk:low Low risk change with minimal impact label Jan 22, 2026
@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

1 similar comment
@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

2 similar comments
@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@github-actions
Copy link

📊 Coverage check completed. See workflow run for details.

@ccastrotrejo ccastrotrejo enabled auto-merge (squash) January 23, 2026 15:50
@ccastrotrejo ccastrotrejo merged commit 93eedcd into main Jan 23, 2026
13 checks passed
@ccastrotrejo ccastrotrejo deleted the ccastrotrejo/identityProvidersFix branch January 23, 2026 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:low Low risk change with minimal impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants