Repository navigation
#5959 Modified consent to have permission with redorder of purposes - #131
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
🔀 Cross-SDK feature parity, no ports neededThis pull request is labelled |
|
|
||
| if (purposes.length === 0) return null; | ||
|
|
||
| // Order purposes so that permissions come first and attributes come last. |
There was a problem hiding this comment.
@coderabbitai Can you evaluate if there's any perf hit due to this block being run on every render?
Also check for any possible NPEs?
There was a problem hiding this comment.
@brionmario The sort adds work on every render, but it is unlikely to matter for a small purpose list. There are runtime null-safety gaps for malformed input. This assessment is based on static inspection, not profiling.
Performance
[...purposes]copies the array: O(n) time and allocation..sort(...)typically adds O(n log n) work. The comparator is inexpensive.- This repeats when
formValueschanges, even whenconsentDatastays unchanged. - If
consentDatais a string,JSON.parsealso repeats on every render.
The copy correctly prevents mutation of the caller’s array. I would keep it.
If purpose lists can become large, memoize parsing, validation, and sorting together with useMemo(..., [consentData]). Place that hook before all conditional returns. For array/object inputs, memoization requires consumers to replace the input reference when the data changes.
Null-safety and malformed input
Valid ConsentPurposeData[] input is safe for this comparator. Missing type values also do not throw; they sort with non-permission purposes.
However, JSON.parse does not validate the data shape:
| Input | Current behavior |
|---|---|
'null' or invalid JSON |
Returns null; the existing catch handles it. |
'[null, {}]' |
The comparator can throw when it reads .type on null. |
'[null]' |
Sorting does not need a comparison, but the default render throws at purpose.type. |
'{"purposes": {}}' |
The spread throws because purposes is not iterable. |
A valid purpose with optional: [null] |
checkOptValue throws when it reads opt.name. |
The sort sits outside the try/catch, so that catch does not protect it. Some malformed inputs already fail in the existing rendering code; the comparator adds another failure point.
Recommendation: Validate the decoded value before sorting. Require an array of non-null purpose objects and validate the essential/optional arrays and their elements before rendering. Optional chaining in the comparator alone would only move the failure into the render code.
The existing purpose.optional?.length > 0 guard does protect checkOptValue and handleChange from a missing or null optional array during normal rendering.
You are interacting with an AI system.
There was a problem hiding this comment.
@zesu22 Can you please check if the supplied suggestions are valid?
There was a problem hiding this comment.
@brionmario I have changed the code according to the coderabbit comment, added memoizing the function as well proper validation on that.
|
Adding |
Signed-off-by: Zeeshan Mehboob <zeeshan.mehboob@infosys.com>
Purpose
fixes thunder-id/thunderid#5959
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.parity/prs-raisedorparity/prs-not-neededadded.parity/prs-raised, the port links are posted as a reply on the parity check's comment.Security checks