[MODIFIED] consent component, added tooltip#38
Open
zesu22 wants to merge 5 commits into
Open
Conversation
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.com>
📝 WalkthroughWalkthroughConsent now supports configurable labels, contextual tooltips, optional-attribute master toggles, explicit opt-in defaults, updated essential-item presentation, and stricter consent decisions. A reusable accessible Tooltip primitive, public type re-export, and ignore patterns were also added. ChangesConsent customization
Public and repository updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AuthOptionFactory
participant Consent
participant Tooltip
participant ConsentCheckboxList
AuthOptionFactory->>Consent: Pass consent configuration
Consent->>Tooltip: Render section info tooltip
Consent->>ConsentCheckboxList: Render consent attributes
Consent->>ConsentCheckboxList: Apply optional master-toggle value
ConsentCheckboxList-->>AuthOptionFactory: Submit exact optional consent values
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.gitignore:
- Line 154: Remove the global __tests__ entry from .gitignore so test
directories under packages/*/src/**/__tests__ can be tracked. If ignoring
generated artifacts is still required, replace it with a narrowly scoped path
targeting only those artifacts.
In `@packages/react/src/components/adapters/Consent.tsx`:
- Around line 202-206: Add an accessible name to the master consent Toggle in
the Consent component by providing an aria-label that clearly identifies it as
selecting all optional attributes for the associated purpose, using the existing
purpose context to keep the name specific.
- Around line 80-100: Update the Consent component’s config handling to merge
supplied values with defaultConfig so unspecified labels retain their defaults.
In the tooltip-rendering logic for both essential and optional attributes,
render the info icon only when the resolved label is a non-empty string,
preventing empty help tooltips.
- Line 24: Update the PromptElement import in Consent.tsx to use the public
`@thunderid/javascript` or `@thunderid/browser` export, or an existing project
re-export, instead of the internal dist/models/embedded-flow path. Preserve the
existing PromptElement usage and select the public source that exposes this
type.
In `@packages/react/src/components/adapters/ConsentCheckboxList.tsx`:
- Around line 131-132: Update the optional consent serialization in
AuthOptionFactory so missing values are submitted as declined, matching
ConsentCheckboxList’s unchecked display state. In the relevant submission
predicate, require the value to equal 'true' rather than treating every value
except 'false' as approved.
In `@packages/react/src/components/primitives/Tooltip/Tooltip.tsx`:
- Around line 50-54: Update the Tooltip component’s trigger behavior around the
tooltip container so it is focusable and opens on keyboard focus as well as
mouse hover. Add appropriate ARIA semantics linking the trigger to the tooltip
content, and ensure keyboard interaction can dismiss it while preserving the
existing hover behavior.
- Around line 28-70: Update Tooltiprops and Tooltip to use div-compatible HTML
attributes, set position to default to 'bottom', and apply style plus ...rest to
the rendered container div. Ensure all position-based class lookups use the
resolved default so omitted positioning does not produce an undefined-arrow
class.
🪄 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: 970dd27f-77da-4734-99fc-1e3f8d2c0da0
📒 Files selected for processing (7)
.gitignorepackages/react/src/components/adapters/Consent.tsxpackages/react/src/components/adapters/ConsentCheckboxList.styles.tspackages/react/src/components/adapters/ConsentCheckboxList.tsxpackages/react/src/components/presentation/auth/AuthOptionFactory.tsxpackages/react/src/components/primitives/Tooltip/Tooltip.styles.tspackages/react/src/components/primitives/Tooltip/Tooltip.tsx
Member
|
@zesu22 Can we check whether the Coderabbit comments are valid? |
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/react/src/components/presentation/auth/AuthOptionFactory.tsx (1)
397-397: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression tests for optional-consent serialization.
Line 397 changes privacy-sensitive output. Cover missing,
'false', and'true'values on approve actions, plus a deny action with'true', to prevent optional attributes being unintentionally approved or rejected.🤖 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 `@packages/react/src/components/presentation/auth/AuthOptionFactory.tsx` at line 397, Add regression tests for the optional-consent serialization around AuthOptionFactory, covering approve actions with missing, 'false', and 'true' optional values, plus a deny action with 'true'. Assert that the serialized approved value matches the expected privacy behavior for each case.
🤖 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 `@packages/react/src/components/primitives/Tooltip/Tooltip.tsx`:
- Around line 45-53: Update the Tooltip component’s props destructuring in
packages/react/src/components/primitives/Tooltip/Tooltip.tsx:45-53 to provide
defaults for className, helperText, and children. Update the Consent component’s
props destructuring in
packages/react/src/components/adapters/Consent.tsx:105-114 to provide defaults
for suppliedConfig and children, using the repository-approved pattern
consistently so both components satisfy the default-props ESLint rule.
In `@packages/vue/src/components/auth/sign-in/AuthOptionFactoryCore.ts`:
- Line 236: Resolve the unsafe types in the mapping that computes approved
consent values before calling getConsentOptionalKey: type
ConsentPromptData.purposes and its optional elements, or narrow/validate them
before use, so p.purposeId and e.name are statically valid string/number and
string arguments. Update the surrounding consent mapping rather than suppressing
the ESLint errors with any.
---
Nitpick comments:
In `@packages/react/src/components/presentation/auth/AuthOptionFactory.tsx`:
- Line 397: Add regression tests for the optional-consent serialization around
AuthOptionFactory, covering approve actions with missing, 'false', and 'true'
optional values, plus a deny action with 'true'. Assert that the serialized
approved value matches the expected privacy behavior for each case.
🪄 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: 4a81a233-0c71-4782-b2c6-ec34ea025eea
📒 Files selected for processing (6)
packages/javascript/src/index.tspackages/react/src/components/adapters/Consent.tsxpackages/react/src/components/adapters/ConsentCheckboxList.tsxpackages/react/src/components/presentation/auth/AuthOptionFactory.tsxpackages/react/src/components/primitives/Tooltip/Tooltip.tsxpackages/vue/src/components/auth/sign-in/AuthOptionFactoryCore.ts
💤 Files with no reviewable changes (1)
- packages/react/src/components/adapters/ConsentCheckboxList.tsx
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.com>
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/react/src/components/adapters/Consent.tsx (2)
95-98: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep consent configuration typed and runtime-safe.
configaccepts arbitraryunknownvalues but is then treated asConsentConfig; a truthy non-string label can reach template resolution and crash when.replace()is called. Type this prop asPartial<ConsentConfig>and/or normalize each field before resolving.Proposed fix
- config?: Record<string, unknown>; + config?: Partial<ConsentConfig>;Also applies to: 138-142
🤖 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 `@packages/react/src/components/adapters/Consent.tsx` around lines 95 - 98, Update the Consent component’s config prop and its related usage to use Partial<ConsentConfig> instead of Record<string, unknown>. Before template resolution, normalize or validate configurable fields—especially label values—so only strings reach .replace(), while preserving existing ConsentConfig defaults and behavior.
162-167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRepresent partially selected optional attributes.
checkOptValuereturnsfalsewhen any optional item is skipped, so both zero and partial selections render the master toggle as unchecked. Tracksome/allderived from the optional form values, and add the indeterminate state toToggleif an indeterminaterole="switch"/checkbox state is required.🤖 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 `@packages/react/src/components/adapters/Consent.tsx` around lines 162 - 167, Update checkOptValue and the associated Toggle state to distinguish zero, partial, and complete optional selections: derive whether some and all optional values are true rather than returning only every(...). Preserve the unchecked state for zero selections, mark the toggle checked only when all are selected, and pass the derived indeterminate state to Toggle when the switch/checkbox API supports it.
🧹 Nitpick comments (1)
packages/react/src/components/presentation/auth/AuthOptionFactory.tsx (1)
397-397: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd regression coverage for strict consent decisions.
Cover
'true','false', missing, empty, and deny-action values so future form-value changes cannot accidentally approve optional attributes.🤖 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 `@packages/react/src/components/presentation/auth/AuthOptionFactory.tsx` at line 397, Add regression tests around the consent decision logic producing the approved value, covering optional attributes with form values of 'true', 'false', missing, empty, and deny-action cases. Verify only the exact 'true' value approves when the action is not deny, while all other values and deny actions remain unapproved.
🤖 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.
Outside diff comments:
In `@packages/react/src/components/adapters/Consent.tsx`:
- Around line 95-98: Update the Consent component’s config prop and its related
usage to use Partial<ConsentConfig> instead of Record<string, unknown>. Before
template resolution, normalize or validate configurable fields—especially label
values—so only strings reach .replace(), while preserving existing ConsentConfig
defaults and behavior.
- Around line 162-167: Update checkOptValue and the associated Toggle state to
distinguish zero, partial, and complete optional selections: derive whether some
and all optional values are true rather than returning only every(...). Preserve
the unchecked state for zero selections, mark the toggle checked only when all
are selected, and pass the derived indeterminate state to Toggle when the
switch/checkbox API supports it.
---
Nitpick comments:
In `@packages/react/src/components/presentation/auth/AuthOptionFactory.tsx`:
- Line 397: Add regression tests around the consent decision logic producing the
approved value, covering optional attributes with form values of 'true',
'false', missing, empty, and deny-action cases. Verify only the exact 'true'
value approves when the action is not deny, while all other values and deny
actions remain unapproved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: caa51360-d1ba-4823-b64f-8d1942819c23
📒 Files selected for processing (3)
packages/react/src/components/adapters/Consent.tsxpackages/react/src/components/presentation/auth/AuthOptionFactory.tsxpackages/react/src/components/primitives/Tooltip/Tooltip.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/src/components/primitives/Tooltip/Tooltip.tsx
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.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.
Purpose
resolves thunder-id/thunderid#4279
Summary
Improves the Consent component by adding bulk selection capabilities and attribute tooltips, while cleaning up redundant UI elements.
Changes
Consentcomponent props to accept aconfigobject for dynamically passing custom labels and tooltip data for both essential and optional attributes.Dividercomponent from insideConsentCheckboxList.Approach
Prop Extension (
ConsentProps):config: Record<string, unknown>toConsentPropsto flexibly pass label text and tooltip metadata for both essential and optional attributes.Master Switch Implementation:
Reusable Tooltip System:
Built a custom Tooltip component accepting
children(e.g., an HTML element or React node),className,helperText, andposition(top|bottom|left|right).Integrated an Info Icon into the label layout of essential and optional attributes to trigger the tooltip on hover.
Styling & Cleanup:
Updated local CSS for layout adjustments around the info icon and master switch.
Cleaned up the JSX structure by removing an unnecessary
Dividercomponent insideConsentCheckboxList.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
ariaLabel.PromptElementtype for embedded flow usage.'true'.