ENG-2678: Show warnings on disabled notices in experience config#7558
Merged
gilluminate merged 10 commits intomainfrom Mar 5, 2026
Merged
ENG-2678: Show warnings on disabled notices in experience config#7558gilluminate merged 10 commits intomainfrom
gilluminate merged 10 commits intomainfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
gilluminate
added a commit
that referenced
this pull request
Mar 3, 2026
gilluminate
added a commit
that referenced
this pull request
Mar 3, 2026
e89374d to
214d5c9
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds warning icons and tooltips for disabled privacy notices in the Experience editor, surfaces per-item warning tooltips via ScrollableList, excludes disabled notices from preview generation, adds a Cypress tooltip helper, and includes tests covering disabled-notice scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Experience Editor (UI)
participant API as Notices API
participant List as ScrollableList
participant Tooltip as AntD Tooltip
participant Preview as Preview Generator
Editor->>API: fetch privacy notices
API-->>Editor: notices (some disabled)
Editor->>List: render items with getWarningTooltip(item)
alt item.disabled == true
List->>Tooltip: render warning icon + tooltip (DISABLED_NOTICE_TOOLTIP)
Tooltip-->>Editor: tooltip shown on hover/focus
end
Editor->>Preview: request preview
Preview->>API: generateMockNotices (filters out disabled)
API-->>Preview: enabled notices only
Preview-->>Editor: render preview (disabled notices excluded)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Comment |
Contributor
Greptile SummaryThis PR adds warning indicators to disabled privacy notices in the experience config editor and filters them out of the experience preview, so the preview accurately reflects what end users will see. The implementation is clean and well-scoped: a new optional Key changes:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 663653f |
clients/admin-ui/src/features/privacy-experience/PrivacyExperienceForm.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/privacy-experience/PrivacyExperienceForm.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
clients/admin-ui/src/features/common/ScrollableList.tsx (1)
338-342: Avoid recomputinggetWarningTooltip(item)multiple times per row.This callback is evaluated repeatedly in render. Compute once per item and pass through.
♻️ Suggested refactor
{values.map((item) => { const itemId = getItemId(item); + const warningTooltip = getWarningTooltip?.(item); return ( <ScrollableListItem @@ - warningTooltip={ - getWarningTooltip && getWarningTooltip(item) - ? getWarningTooltip(item) - : undefined - } + warningTooltip={warningTooltip} /> ); })} @@ {values.map((item) => { const itemId = getItemId(item); + const warningTooltip = getWarningTooltip?.(item); return ( <ScrollableListItem @@ - warningTooltip={ - getWarningTooltip && getWarningTooltip(item) - ? getWarningTooltip(item) - : undefined - } + warningTooltip={warningTooltip} maxH={maxHeight} rowTestId={`${baseTestId}-row-${itemId}`} />Also applies to: 363-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/src/features/common/ScrollableList.tsx` around lines 338 - 342, Compute the result of getWarningTooltip(item) once per render for each row and reuse it instead of calling getWarningTooltip(item) multiple times; e.g., inside the row rendering logic (where warningTooltip prop is set in ScrollableList/its row renderer) assign const warning = getWarningTooltip ? getWarningTooltip(item) : undefined (or null) and pass warning to warningTooltip and any other places that currently call getWarningTooltip(item) (also update the other occurrence around the block that corresponds to lines 363-367 to use the same local const).clients/admin-ui/cypress/e2e/privacy-experience/experience-editor.cy.ts (1)
469-472: Add a positive preview assertion alongside the negative check.The current check can pass when preview content is missing for unrelated reasons. Add one known-positive assertion (e.g., modal title) before asserting disabled notice absence.
Suggested fix
// The preview container should not contain the disabled notice's name - cy.get(`#${PREVIEW_CONTAINER_ID}`).should( - "not.contain", - "Example Notice", - ); + cy.get(`#${PREVIEW_CONTAINER_ID}`) + .should("contain", "Manage your consent preferences") + .and("not.contain", "Example Notice");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/admin-ui/cypress/e2e/privacy-experience/experience-editor.cy.ts` around lines 469 - 472, Add a positive assertion that the preview actually rendered before the negative check: first assert that the preview container identified by PREVIEW_CONTAINER_ID contains a known-positive element (for example the modal title text used elsewhere in this spec) via cy.get(`#${PREVIEW_CONTAINER_ID}`).should('contain', 'Expected Modal Title') (or the actual modal title string the test uses), then keep the existing negative assertion that it does not contain "Example Notice" so the test fails only when the notice is present rather than when the preview is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/admin-ui/cypress/e2e/privacy-experience/experience-editor.cy.ts`:
- Around line 416-417: Tests currently only wait for the `@getExperienceDetail`
intercept but the assertions for the warning icon and preview filtering depend
on the `@getExperienceNoticesDisabled` payload; add explicit waits for the
`@getExperienceNoticesDisabled` alias immediately after the existing
cy.wait("@getExperienceDetail") calls wherever the `@getExperienceNoticesDisabled`
intercept is set (specifically in the places that assert the warning icon and
preview filtering) so the code calls cy.wait("@getExperienceNoticesDisabled")
before those assertions.
In `@clients/admin-ui/src/features/common/ScrollableList.tsx`:
- Around line 85-97: The warning tooltip currently uses a non-interactive span
with tabIndex={0}; replace that span with a semantic button element (e.g., a
plain <button> with type="button") that acts as the Tooltip trigger, give it an
appropriate aria-label (e.g., "Warning" or a prop-driven label) and move/remove
the eslint-disable comment; keep the Tooltip wrapping and the
Icons.WarningAltFilled usage so visual/placement behavior stays the same, and
ensure any styling/classes previously on the span are applied to the button to
preserve layout.
In `@clients/admin-ui/src/features/privacy-experience/preview/Preview.tsx`:
- Around line 351-353: The global CSS rule overriding .fides-sr-only should be
scoped to the preview component only: update the style block in Preview.tsx (the
CSS that currently contains ".fides-sr-only { display: none !important; }") so
the selector targets the preview root (e.g., prepend the preview component's
wrapper class/ID such as ".privacy-preview" or the component root element) like
".privacy-preview .fides-sr-only" to avoid hiding assistive text outside the
preview panel; ensure the wrapper class you pick is the element returned by the
Preview component so the rule only applies while the preview is mounted.
---
Nitpick comments:
In `@clients/admin-ui/cypress/e2e/privacy-experience/experience-editor.cy.ts`:
- Around line 469-472: Add a positive assertion that the preview actually
rendered before the negative check: first assert that the preview container
identified by PREVIEW_CONTAINER_ID contains a known-positive element (for
example the modal title text used elsewhere in this spec) via
cy.get(`#${PREVIEW_CONTAINER_ID}`).should('contain', 'Expected Modal Title') (or
the actual modal title string the test uses), then keep the existing negative
assertion that it does not contain "Example Notice" so the test fails only when
the notice is present rather than when the preview is absent.
In `@clients/admin-ui/src/features/common/ScrollableList.tsx`:
- Around line 338-342: Compute the result of getWarningTooltip(item) once per
render for each row and reuse it instead of calling getWarningTooltip(item)
multiple times; e.g., inside the row rendering logic (where warningTooltip prop
is set in ScrollableList/its row renderer) assign const warning =
getWarningTooltip ? getWarningTooltip(item) : undefined (or null) and pass
warning to warningTooltip and any other places that currently call
getWarningTooltip(item) (also update the other occurrence around the block that
corresponds to lines 363-367 to use the same local const).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18fff2dc-3cf2-4cc2-a471-3604e813bdcc
📒 Files selected for processing (10)
changelog/7558-disabled-notice-warnings-in-experience-config.yamlclients/admin-ui/cypress/e2e/action-center/aggregate-results.cy.tsclients/admin-ui/cypress/e2e/action-center/assets-results.cy.tsclients/admin-ui/cypress/e2e/privacy-experience/experience-editor.cy.tsclients/admin-ui/cypress/support/ant-support.tsclients/admin-ui/src/features/common/ScrollableList.tsxclients/admin-ui/src/features/privacy-experience/PrivacyExperienceForm.tsxclients/admin-ui/src/features/privacy-experience/preview/Preview.tsxclients/admin-ui/src/features/privacy-experience/preview/helpers.tsclients/admin-ui/src/pages/poc/prompt-explorer.tsx
clients/admin-ui/src/features/privacy-experience/preview/Preview.tsx
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Re: nitpick on positive preview assertion — good catch. Added a positive assertion that the preview contains "Manage your consent preferences" before asserting the disabled notice is absent. This ensures we're checking rendered content, not an empty container. |
Update ScrollableList.tsx
- Add aria-label to warning tooltip span for screen reader support - Extract getTooltip/getWarningTooltip into local vars to avoid double invocation - Rename single-char lambda param (n) to (item) in getWarningTooltip callbacks - Add positive assertion before negative check in disabled notice preview test - Scope .fides-sr-only CSS rule to #preview-container Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
391306d to
bf77ad5
Compare
speaker-ender
approved these changes
Mar 5, 2026
Contributor
speaker-ender
left a comment
There was a problem hiding this comment.
Approving as is.
Minor nits/comments but otherwise looks good and functions as intended.
| }) | ||
| .filter((notice): notice is PrivacyNoticeResponse => notice !== undefined); | ||
| .filter( | ||
| (notice): notice is PrivacyNoticeResponse => |
Contributor
There was a problem hiding this comment.
Suggested change
| (notice): notice is PrivacyNoticeResponse => | |
| (notice) => |
Type coercion doesn't seem necessary here
Contributor
Author
There was a problem hiding this comment.
The type predicate notice is PrivacyNoticeResponse is needed here to narrow away undefined from the .map() chain above. Without it, TypeScript would keep the return type as (PrivacyNoticeResponse | undefined)[]. Happy to change if you see it differently though!
|
|
||
| setLlmResponse(result.response_text); | ||
| } catch (error) { | ||
| // eslint-disable-next-line no-console |
Contributor
There was a problem hiding this comment.
If this is correct, should we update our eslint rules to allow console.error?
Contributor
Author
There was a problem hiding this comment.
this is just in the POC so generally not acceptable but giving a pass here
- Rename tooltip to infoTooltip on ScrollableListItem for clarity - Remove redundant || undefined from optional chaining calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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.
Ticket ENG-2678
Description Of Changes
Adds warning indicators to privacy notices that are disabled within the experience config editor. When a notice is disabled (either at creation time or after being added to an experience), a warning icon with a tooltip appears on its row in the privacy notices list, informing the user that the notice will not display and should be enabled or removed.
Also filters disabled notices out of the experience preview so the preview accurately reflects what end users will see.
Code Changes
clients/admin-ui/src/features/common/ScrollableList.tsx- Adds optionalgetWarningTooltipprop toScrollableListand renders a focusable warning icon with tooltip on rows where the callback returns a stringclients/admin-ui/src/features/privacy-experience/PrivacyExperienceForm.tsx- WiresgetWarningTooltipinto both the TCF and non-TCF privacy noticeScrollableListinstances, returning the disabled notice tooltip message whennotice.disabledis trueclients/admin-ui/src/features/privacy-experience/preview/helpers.ts- Filters out disabled notices fromgenerateMockNoticesso the preview only shows notices that will actually displayclients/admin-ui/cypress/support/ant-support.ts- AddsgetAntTooltipCypress commandclients/admin-ui/cypress/e2e/privacy-experience/experience-editor.cy.ts- Adds Cypress tests covering the warning icon, tooltip content, and preview filtering for disabled noticesSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
New Features
Bug Fixes
Tests
Style
Chores