Skip to content

Conversation

@gilluminate
Copy link
Contributor

@gilluminate gilluminate commented Feb 6, 2026

Ticket ENG-2533

Description Of Changes

Replace the Chakra-based ScannerError component with the shared Ant Design ErrorPage component across all infrastructure scanner forms. Migrate layout from Chakra (Stack, Box, HStack, Text) to Ant (Flex, Typography) and update toast notifications from useChakraToast to useMessage.

Code Changes

  • Added fullScreen prop to ErrorPage to support inline (non-full-screen) usage
  • Widened ErrorPage error prop to accept ParsedError in addition to RTK error types, with an isParsedError type guard
  • Updated defaultMessage handling so it takes priority over error.message for ParsedError when explicitly provided
  • Replaced ScannerError with ErrorPage in AuthenticateAwsForm, AuthenticateOktaForm, and LoadDataFlowScanner
  • Preserved AWS 403 permission-specific error handling with IAM documentation link in AuthenticateAwsForm
  • Migrated LoadDataFlowScanner from useChakraToast to useMessage
  • Added isRequired to Okta form fields that were missing it
  • Updated AWS region options to match current AWS documentation (added 12 new regions)
  • Removed broken documentation URL constants from constants.tsx
  • Deleted ScannerError.tsx

Steps to Confirm

  1. Navigate to the Add systems page and select the AWS scanner flow (https://fides-plus-nightly-git-gill-eng-2533replace-infra-ddeb40-ethyca.vercel.app/add-systems)
  2. Confirm the form renders correctly with all fields and Cancel/Save buttons
  3. Submit invalid AWS credentials and confirm the error page displays inline with a Cancel button
  4. Navigate to the Okta scanner flow and repeat similar validation
CleanShot 2026-02-06 at 16 20 08@2x

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • No followup issues
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

@vercel
Copy link
Contributor

vercel bot commented Feb 6, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
fides-plus-nightly Ready Ready Preview, Comment Feb 10, 2026 5:06pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
fides-privacy-center Ignored Ignored Feb 10, 2026 5:06pm

Request Review

@gilluminate gilluminate force-pushed the gill/ENG-2533/replace-infrastructure-scanner-error-page-with-new branch from c6f0efc to a4aeaad Compare February 6, 2026 22:11
@gilluminate gilluminate force-pushed the gill/ENG-2533/replace-infrastructure-scanner-error-page-with-new branch from 970a2cb to c2ddebd Compare February 6, 2026 23:22
@gilluminate gilluminate marked this pull request as ready for review February 6, 2026 23:59
@gilluminate gilluminate requested a review from a team as a code owner February 6, 2026 23:59
@gilluminate gilluminate requested review from speaker-ender and removed request for a team February 6, 2026 23:59
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 7, 2026

Greptile Overview

Greptile Summary

  • Replaces the config-wizard’s Chakra-based ScannerError with the shared Ant Design ErrorPage across AWS/Okta/data-flow scanner paths.
  • Updates AWS/Okta authentication forms to Ant Flex/Typography layout and tweaks form requirements (e.g., required flags, Okta scopes parsing).
  • Extends ErrorPage to support inline (non-full-screen) usage and to accept ParsedError in addition to RTK error types.
  • Updates Cypress E2E tests to assert against the new ErrorPage rendering.
  • Removes obsolete docs URL constants and expands AWS region options.

Confidence Score: 3/5

  • This PR is likely mergeable after addressing a few behavior/coverage issues in the wizard UI and request payload handling.
  • Core refactor is straightforward, but there are a couple of changes that can cause real user-facing problems (Okta scopes can include empty entries; wizard layout width constraint was removed) and one effect dependency that can duplicate success handling if useMessage() is unstable. Prior review threads also indicate ErrorPage type-guard/default-message logic may still be incorrect for some RTK error shapes.
  • clients/admin-ui/src/features/config-wizard/AuthenticateOktaForm.tsx, clients/admin-ui/src/features/config-wizard/AuthenticateScanner.tsx, clients/admin-ui/src/features/config-wizard/LoadDataFlowScanner.tsx, clients/admin-ui/src/features/common/errors/ErrorPage.tsx

Important Files Changed

Filename Overview
changelog/7338.yaml Adds changelog entry for replacing scanner error page with shared ErrorPage.
clients/admin-ui/cypress/e2e/config-wizard.cy.ts Updates Cypress assertions to target ErrorPage; drops error-details assertions (coverage regression).
clients/admin-ui/src/features/common/errors/ErrorPage.tsx Extends ErrorPage to accept ParsedError and adds fullScreen prop; prior review threads already flagged type-guard/defaultMessage behavior changes.
clients/admin-ui/src/features/config-wizard/AuthenticateAwsForm.tsx Replaces ScannerError with inline ErrorPage and migrates layout to Ant Flex/Typography; preserves AWS 403 copy/link.
clients/admin-ui/src/features/config-wizard/AuthenticateOktaForm.tsx Replaces ScannerError with ErrorPage and makes fields required; new scopes splitting can send empty scopes on trailing commas.
clients/admin-ui/src/features/config-wizard/AuthenticateScanner.tsx Removes Chakra Box wrapper; changes step layout width constraints (likely visible behavior change).
clients/admin-ui/src/features/config-wizard/LoadDataFlowScanner.tsx Migrates toast to useMessage and replaces ScannerError with ErrorPage; effect dependency on messageApi may cause duplicate success handling if unstable.
clients/admin-ui/src/features/config-wizard/ScannerError.tsx Deletes legacy Chakra-based ScannerError component.
clients/admin-ui/src/features/config-wizard/constants.tsx Removes unused/broken docs URL constants; expands AWS region options and tweaks step name casing.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@gilluminate
Copy link
Contributor Author

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 7, 2026

Additional Comments (1)

clients/admin-ui/src/features/config-wizard/AuthenticateOktaForm.tsx
scopes must be an array, but you're wrapping the string in an array. The split on comma in validation means values.scopes is still a string like "okta.apps.read, okta.users.read", so this creates [["okta.apps.read, okta.users.read"]] instead of ["okta.apps.read", "okta.users.read"]

      scopes: values.scopes ? values.scopes.split(",").map((s) => s.trim()) : ["okta.apps.read"],

@gilluminate
Copy link
Contributor Author

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@gilluminate
Copy link
Contributor Author

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 7, 2026

Additional Comments (1)

clients/admin-ui/src/features/config-wizard/LoadDataFlowScanner.tsx
Duplicate success handling

This effect will run any time messageApi changes identity (since it’s in the dependency list). If useMessage() returns a non-stable object, this can re-fire for the same scanResults, resulting in duplicated success messages and repeated changeStep() dispatches.

Avoid depending on an unstable messageApi reference (e.g., extract a stable callback) so the success path is executed only once per scan completion.

Copy link
Contributor

@speaker-ender speaker-ender left a comment

Choose a reason for hiding this comment

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

Approving because it seems to function as intended.
I do have a concern about some of the logic as noted in my comment.

@gilluminate gilluminate force-pushed the gill/ENG-2533/replace-infrastructure-scanner-error-page-with-new branch from 2982870 to b9f08d1 Compare February 10, 2026 17:01
@gilluminate gilluminate added this pull request to the merge queue Feb 10, 2026
Merged via the queue into main with commit 22a3ba3 Feb 10, 2026
46 checks passed
@gilluminate gilluminate deleted the gill/ENG-2533/replace-infrastructure-scanner-error-page-with-new branch February 10, 2026 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants