Fix rule based approval workflows issues#9894
Closed
nishagii wants to merge 1 commit intowso2:masterfrom
Closed
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR introduces workflow-scoped type definitions for resource responses and replaces untyped Changes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
nishagii
force-pushed
the
claim-rules-ui-support
branch
from
443d35a to
f7c68d5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
features/admin.approval-workflows.v1/utils/resource-utils.ts (1)
81-96: Consider adding defensive validation for numeric fields.The type casts (
as number) are compile-time only assertions. If the API returns unexpected types (e.g.,itemsPerPage: "10"as a string), the cast won't convert them, potentially causing downstream issues.This is acceptable given the improvement over
any, but for extra robustness you could add runtime coercion:♻️ Optional: Defensive numeric coercion
// SCIM list response (roles, groups). if (Array.isArray(record.Resources)) { return { - count: (record.itemsPerPage as number) ?? record.Resources.length, + count: Number(record.itemsPerPage) || record.Resources.length, items: record.Resources as ResourceInterface[], - totalResults: (record.totalResults as number) ?? record.Resources.length + totalResults: Number(record.totalResults) || record.Resources.length }; } // Applications list response. if (Array.isArray(record.applications)) { return { - count: (record.count as number) ?? record.applications.length, + count: Number(record.count) || record.applications.length, items: record.applications as ResourceInterface[], - totalResults: (record.totalResults as number) ?? record.applications.length + totalResults: Number(record.totalResults) || record.applications.length }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.approval-workflows.v1/utils/resource-utils.ts` around lines 81 - 96, The numeric fields like itemsPerPage, count, and totalResults are only type-asserted (e.g., record.itemsPerPage as number) and may be strings or other types at runtime; update the logic in the block handling record.Resources and record.applications to coerce these values to numbers (e.g., use Number(...) or parseInt(...)) and fall back to the array length when the result is NaN or invalid, ensuring count and totalResults are always numeric; reference the record object and the fields itemsPerPage, count, totalResults, Resources, and applications when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@features/admin.approval-workflows.v1/utils/resource-utils.ts`:
- Around line 81-96: The numeric fields like itemsPerPage, count, and
totalResults are only type-asserted (e.g., record.itemsPerPage as number) and
may be strings or other types at runtime; update the logic in the block handling
record.Resources and record.applications to coerce these values to numbers
(e.g., use Number(...) or parseInt(...)) and fall back to the array length when
the result is NaN or invalid, ensuring count and totalResults are always
numeric; reference the record object and the fields itemsPerPage, count,
totalResults, Resources, and applications when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: eaa6e540-8838-4cf1-9cbd-4df637eab27b
📒 Files selected for processing (7)
features/admin.approval-workflows.v1/components/rules/approval-workflow-rule-expression.tsxfeatures/admin.approval-workflows.v1/components/rules/workflow-condition-text-input.tsxfeatures/admin.approval-workflows.v1/components/rules/workflow-condition-value-input.tsxfeatures/admin.approval-workflows.v1/components/rules/workflow-resource-autocomplete.tsxfeatures/admin.approval-workflows.v1/components/rules/workflow-resource-list-select.tsxfeatures/admin.approval-workflows.v1/constants/approval-workflow-constants.tsfeatures/admin.approval-workflows.v1/utils/resource-utils.ts
💤 Files with no reviewable changes (1)
- features/admin.approval-workflows.v1/constants/approval-workflow-constants.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
features/admin.approval-workflows.v1/components/rules/workflow-resource-list-select.tsx (1)
81-81: Pre-existing type narrowness in useState generic.The state is initialized with
nullandsetResourceDetails(null)is called at line 113, but the generic doesn't includenull. This should beuseState<ResourceInterface | null>(null)for type correctness. However, this is pre-existing code and not introduced by this PR.🔧 Optional fix for stricter typing
- const [ resourceDetails, setResourceDetails ] = useState<ResourceInterface>(null); + const [ resourceDetails, setResourceDetails ] = useState<ResourceInterface | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/admin.approval-workflows.v1/components/rules/workflow-resource-list-select.tsx` at line 81, The state hook declaration for resourceDetails is too narrow because it initializes to null but uses useState<ResourceInterface>(null); update the generic to allow null (useState<ResourceInterface | null>(null)) and ensure any usages of setResourceDetails (e.g., where setResourceDetails(null) is called) and the resourceDetails variable handle the nullable type (add null checks or optional chaining as needed) so TypeScript no longer errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/admin.approval-workflows.v1/utils/resource-utils.ts`:
- Around line 81-83: The code uses TypeScript assertions like
(record.itemsPerPage as number) which don't provide runtime numeric conversion;
replace those assertions in the mapping that sets count, totalResults (and the
similar spots at the other two occurrences) with explicit numeric coercion and
validation: call Number(...) (or parseInt(...,10)) on record.itemsPerPage and
record.totalResults, check Number.isFinite(...) (or isNaN) and if invalid
fallback to record.Resources.length, and ensure the resulting values assigned to
count and totalResults are real numbers so comparisons like filteredTotalResults
> filteredCount do numeric comparison; update the mapping that produces items:
record.Resources as ResourceInterface[] only for the shape, but ensure
count/totalResults use the validated numeric values.
---
Nitpick comments:
In
`@features/admin.approval-workflows.v1/components/rules/workflow-resource-list-select.tsx`:
- Line 81: The state hook declaration for resourceDetails is too narrow because
it initializes to null but uses useState<ResourceInterface>(null); update the
generic to allow null (useState<ResourceInterface | null>(null)) and ensure any
usages of setResourceDetails (e.g., where setResourceDetails(null) is called)
and the resourceDetails variable handle the nullable type (add null checks or
optional chaining as needed) so TypeScript no longer errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f72748c9-b22a-4a01-b6fb-2e0128c3ee5b
📒 Files selected for processing (6)
features/admin.approval-workflows.v1/components/rules/approval-workflow-rule-expression.tsxfeatures/admin.approval-workflows.v1/components/rules/workflow-condition-text-input.tsxfeatures/admin.approval-workflows.v1/components/rules/workflow-condition-value-input.tsxfeatures/admin.approval-workflows.v1/components/rules/workflow-resource-autocomplete.tsxfeatures/admin.approval-workflows.v1/components/rules/workflow-resource-list-select.tsxfeatures/admin.approval-workflows.v1/utils/resource-utils.ts
✅ Files skipped from review due to trivial changes (2)
- features/admin.approval-workflows.v1/components/rules/workflow-condition-text-input.tsx
- features/admin.approval-workflows.v1/components/rules/approval-workflow-rule-expression.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- features/admin.approval-workflows.v1/components/rules/workflow-condition-value-input.tsx
Comment on lines
+81
to
+83
| count: (record.itemsPerPage as number) ?? record.Resources.length, | ||
| items: record.Resources as ResourceInterface[], | ||
| totalResults: (record.totalResults as number) ?? record.Resources.length |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current unsafe numeric assertions and the downstream pagination usage.
rg -n -C2 '\(record\.(itemsPerPage|count|totalResults)\s+as\s+number\)\s*\?\?' \
features/admin.approval-workflows.v1/utils/resource-utils.ts
rg -n -C3 'normalizeResourceResponse|hasMoreItems|filteredTotalResults|filteredCount' \
features/admin.approval-workflows.v1/components/rules/workflow-resource-autocomplete.tsxRepository: wso2/identity-apps
Length of output: 2467
Replace as number assertions with real numeric parsing.
At lines 81, 83, 90, and 92, as number is a compile-time assertion only and provides no runtime type safety. If the API returns string values or invalid types, the null coalescing operator (??) will pass through non-numeric values. This causes line 191's filteredTotalResults > filteredCount numeric comparison to behave unexpectedly (string comparison instead of numeric comparison).
🔧 Proposed fix
+const toFiniteNumber = (value: unknown, fallback: number): number => {
+ if (typeof value === "number" && Number.isFinite(value)) {
+ return value;
+ }
+
+ if (typeof value === "string" && value.trim() !== "") {
+ const parsed: number = Number(value);
+
+ if (Number.isFinite(parsed)) {
+ return parsed;
+ }
+ }
+
+ return fallback;
+};
+
export const normalizeResourceResponse = (response: unknown): NormalizedResourceList => {
@@
if (Array.isArray(record.Resources)) {
return {
- count: (record.itemsPerPage as number) ?? record.Resources.length,
+ count: toFiniteNumber(record.itemsPerPage, record.Resources.length),
items: record.Resources as ResourceInterface[],
- totalResults: (record.totalResults as number) ?? record.Resources.length
+ totalResults: toFiniteNumber(record.totalResults, record.Resources.length)
};
}
@@
if (Array.isArray(record.applications)) {
return {
- count: (record.count as number) ?? record.applications.length,
+ count: toFiniteNumber(record.count, record.applications.length),
items: record.applications as ResourceInterface[],
- totalResults: (record.totalResults as number) ?? record.applications.length
+ totalResults: toFiniteNumber(record.totalResults, record.applications.length)
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| count: (record.itemsPerPage as number) ?? record.Resources.length, | |
| items: record.Resources as ResourceInterface[], | |
| totalResults: (record.totalResults as number) ?? record.Resources.length | |
| const toFiniteNumber = (value: unknown, fallback: number): number => { | |
| if (typeof value === "number" && Number.isFinite(value)) { | |
| return value; | |
| } | |
| if (typeof value === "string" && value.trim() !== "") { | |
| const parsed: number = Number(value); | |
| if (Number.isFinite(parsed)) { | |
| return parsed; | |
| } | |
| } | |
| return fallback; | |
| }; | |
| export const normalizeResourceResponse = (response: unknown): NormalizedResourceList => { | |
| // ... preceding code ... | |
| if (Array.isArray(record.Resources)) { | |
| return { | |
| count: toFiniteNumber(record.itemsPerPage, record.Resources.length), | |
| items: record.Resources as ResourceInterface[], | |
| totalResults: toFiniteNumber(record.totalResults, record.Resources.length) | |
| }; | |
| } | |
| if (Array.isArray(record.applications)) { | |
| return { | |
| count: toFiniteNumber(record.count, record.applications.length), | |
| items: record.applications as ResourceInterface[], | |
| totalResults: toFiniteNumber(record.totalResults, record.applications.length) | |
| }; | |
| } | |
| // ... remaining code ... | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@features/admin.approval-workflows.v1/utils/resource-utils.ts` around lines 81
- 83, The code uses TypeScript assertions like (record.itemsPerPage as number)
which don't provide runtime numeric conversion; replace those assertions in the
mapping that sets count, totalResults (and the similar spots at the other two
occurrences) with explicit numeric coercion and validation: call Number(...) (or
parseInt(...,10)) on record.itemsPerPage and record.totalResults, check
Number.isFinite(...) (or isNaN) and if invalid fallback to
record.Resources.length, and ensure the resulting values assigned to count and
totalResults are real numbers so comparisons like filteredTotalResults >
filteredCount do numeric comparison; update the mapping that produces items:
record.Resources as ResourceInterface[] only for the shape, but ensure
count/totalResults use the validated numeric values.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9894 +/- ##
=======================================
Coverage 56.01% 56.01%
=======================================
Files 42 42
Lines 1023 1023
Branches 254 231 -23
=======================================
Hits 573 573
Misses 450 450
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
nishagii
force-pushed
the
claim-rules-ui-support
branch
from
f7c68d5 to
ad0efb9
Compare
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
1 participant
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.
This pull request refactors several components in the approval workflows feature to improve code clarity and consistency. The main focus is on standardizing prop names by removing unnecessary underscores, which makes the code easier to read and maintain. Additionally, some minor improvements are made to default values and destructuring patterns.
Prop name standardization and code clarity:
_ruleId,_conditionId, etc.) in the following components:ApprovalWorkflowRuleExpression,WorkflowConditionTextInput,WorkflowConditionValueInput,WorkflowResourceAutocomplete, andWorkflowResourceListSelect. All usages and destructuring patterns were updated to use the new names without underscores.Minor improvements:
hiddenResources,hiddenValues) instead of using variables with underscores, simplifying the code and reducing redundancy.