BA-2161 setFormApiErrors handles also Error objects #193
Conversation
|
WalkthroughThis pull request involves updates across multiple packages in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/utils/CHANGELOG.md (1)
7-7: Enhance the changelog entry with more details.The changelog entry could be more descriptive about how Error objects are handled. Consider adding:
- What behavior to expect when passing Error objects
- Any specific use cases this addresses (e.g., sign-up form errors)
-setFormApiErrors also handles Error objects +setFormApiErrors now handles Error objects created via new Error(message), displaying their messages as form validation errors. This improves error handling during form submissions, particularly for sign-up flows.packages/utils/functions/form/setFormApiErrors/index.ts (1)
14-24: Handle unparseable error messages more explicitly
Whenerr.messageisn't valid JSON, this branch silently ignores the error. It might be helpful to provide a default error message or log this for better user feedback when JSON parsing fails.} catch (parsingError) { - // Error message is not a JSON object, no further action taken + console.warn("Unable to parse 'err.message' as JSON. Using a default error message or ignoring the error.") }packages/utils/functions/form/setFormApiErrors/__tests__/setFormApiErrors.test.ts (1)
14-16: Add more test cases for varied field values
Currently, the form values forbioandimageare set to''andnull, respectively. Consider adding more edge cases (e.g.,undefined, numeric values, etc.) to increase coverage and confidence in form error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
packages/authentication/CHANGELOG.md(1 hunks)packages/authentication/package.json(1 hunks)packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/package.json(1 hunks)packages/graphql/CHANGELOG.md(1 hunks)packages/graphql/package.json(1 hunks)packages/provider/CHANGELOG.md(1 hunks)packages/provider/package.json(1 hunks)packages/utils/CHANGELOG.md(1 hunks)packages/utils/functions/api/getApiErrorMessage/__tests__/getApiErrorMessage.test.ts(1 hunks)packages/utils/functions/api/getApiErrorMessage/index.ts(1 hunks)packages/utils/functions/form/setFormApiErrors/__tests__/setFormApiErrors.test.ts(2 hunks)packages/utils/functions/form/setFormApiErrors/index.ts(1 hunks)packages/utils/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- packages/provider/package.json
- packages/components/package.json
- packages/design-system/package.json
- packages/utils/package.json
- packages/graphql/package.json
- packages/authentication/package.json
- packages/authentication/CHANGELOG.md
- packages/provider/CHANGELOG.md
- packages/wagtail/package.json
- packages/wagtail/CHANGELOG.md
- packages/design-system/CHANGELOG.md
- packages/graphql/CHANGELOG.md
🔇 Additional comments (7)
packages/utils/functions/api/getApiErrorMessage/index.ts (1)
12-12: Ensure correct fallback behavior for invalid JSON error strings.This line leverages the parsed message's
detailfield when available, or otherwise falls back toerror.message. When parsing fails, the catch block also useserror.message. As a result, the default message is only used if neithererror?.messagenorparsedMessage.detailis present. This approach aligns with the objective of capturing error information from both JSON-based andErrorobject instances.packages/utils/functions/api/getApiErrorMessage/__tests__/getApiErrorMessage.test.ts (1)
37-37: Confirm test alignment with new fallback behavior.By expecting the raw
errorMessagefor invalid JSON, this test verifies the updated logic in which the function returnserror.messageif parsing fails. This change ensures thatnew Error('...')instances, as well as other non-JSON messages, still provide meaningful feedback.packages/components/CHANGELOG.md (1)
3-11: LGTM! Version bump and dependency updates look correct.The changelog entry properly documents the dependency updates, particularly the update to
@baseapp-frontend/utils@3.1.3which aligns with the PR objective to enhance error handling.Let's verify the version bump of @baseapp-frontend/utils package:
✅ Verification successful
Version bump and dependency updates are verified and correct
The changelog entry in components package correctly references
@baseapp-frontend/utils@3.1.3, which contains the enhancement tosetFormApiErrorsto handle Error objects, aligning perfectly with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the changes in @baseapp-frontend/utils@3.1.3 related to error handling # Check the commit history and diff for utils package version 3.1.3 git log -p --all -- 'packages/utils/CHANGELOG.md' | grep -A 10 "3.1.3"Length of output: 0
Script:
#!/bin/bash # Check if utils package exists and view its changelog echo "=== Checking utils package ===" fd "package.json" packages/utils cat packages/utils/CHANGELOG.md 2>/dev/null || echo "Changelog not found" echo -e "\n=== Searching for error handling related changes ===" # Look for error handling related code ast-grep --pattern 'setFormApiErrors($_)' # Search for Error object handling rg -l "instanceof Error" packages/Length of output: 9093
packages/utils/CHANGELOG.md (1)
3-7: Version bump looks appropriate.The change is backward-compatible and fixes a bug in error handling, making it suitable for a patch version bump (3.1.3).
Let's verify there are no breaking changes by checking the implementation:
✅ Verification successful
Patch version bump is correct
The implementation verification confirms this is a non-breaking change that extends error handling capabilities while maintaining backward compatibility with existing usage patterns across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of setFormApiErrors to verify it's a non-breaking change # Look for the implementation and its tests rg -A 10 "setFormApiErrors" --type ts --type tsxLength of output: 76
Script:
#!/bin/bash # Search for setFormApiErrors implementation using multiple strategies echo "=== Searching with ripgrep ===" rg "setFormApiErrors" -A 5 echo -e "\n=== Searching TypeScript files ===" fd -e ts -e tsx -x rg "setFormApiErrors" -A 5 {} echo -e "\n=== Searching with AST pattern ===" ast-grep --pattern 'function setFormApiErrors($_) { $$$ }'Length of output: 19223
packages/utils/functions/form/setFormApiErrors/index.ts (1)
25-29: Verify that all error properties are arrays
Here, we casterrorstostring[], but if the backend changes shape or returns unexpected data, this cast could cause issues. Confirm that the error format is always an array of strings, or add checks before usinghead(errors as string[]).✅ Verification successful
Error handling is properly implemented and type-safe
The implementation includes multiple safeguards that prevent runtime issues:
- Validates error object structure before processing
- Verifies error message is a string before usage
- Confirms field exists in form values
- Test suite demonstrates consistent array-based error format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locate all references to setFormApiErrors usage, then review how errors are constructed. rg -A 5 "setFormApiErrors"Length of output: 14696
Script:
#!/bin/bash # Get the test file content and implementation cat packages/utils/functions/form/setFormApiErrors/__tests__/setFormApiErrors.test.ts cat packages/utils/functions/form/setFormApiErrors/index.tsLength of output: 4248
packages/utils/functions/form/setFormApiErrors/__tests__/setFormApiErrors.test.ts (2)
81-96: Approved: robust testing for null or blank fields
These lines thoroughly verify form errors when fields are empty or null, ensuring thatsetErroris called. Great job covering these edge cases.
98-115: Approved: testing Error object parsing
This validates the new functionality that handlesErrorobjects with JSON data in their messages. It’s an excellent addition to confirm the updated logic.
|
@anicioalexandre: Since we had our little discussion about revealing information to potential attackers: IMO, displaying this error message is much more critical than displaying the last one since it gives an unauthenticated attacker an easy way to find out whether an email address is used or not (to sign up as in this PR you do not need to be authenticated, to change your password as in the last PR you had to be authenticated). However, not displaying the message would lead to a quite bad UX: As a user, I would no longer know why my sign up failed. |
@tsl-ps2 what are the current messages we're showing? |
|
@anicioalexandre: "That email is already in use. Choose another." (when registering with an already existing email address) |
tsl-ps2
force-pushed
the
BA-2161-email-already-used-warning
branch
from
2e23479 to
626e2a9
Compare
tsl-ps2
requested review from
deboracosilveira and
priscilladeroode
as code owners
|
* BA-2161 api error messages * BA-2161 Version packages
* BA-2161 api error messages * BA-2161 Version packages
* BA-2161 api error messages * BA-2161 Version packages
This fixes the first objective below by making sure that
setFormApiErrorscan also handleErrorobjects (obtained by callingnew Error(message)). I could not reproduce the second problem.Testing
Use the frontend template from the master branch and the package from this PR. Then replace packages and follow the usual workflow. Run the backend from the master branch.
Description
Acceptance Criteria
When registering with the existing email, it send me to code screen instead of showing warning of existing email.
The error should show in the UI
I should not be able to the "Enter Code" screen if there is an error during the sign up form.
Video: https://www.loom.com/share/dcee5d9752334d379ee0d191576da364?sid=109e6385-c05e-44a1-8682-557ee8f52c6a
Bug catcher: Selim Reza
Approvd
https://app.approvd.io/silverlogic/BA/stories/38225
Summary by CodeRabbit
Summary by CodeRabbit
Dependency Updates
@baseapp-frontend/utilsto version 3.1.3 across multiple packages@baseapp-frontend/authenticationto version 4.1.5@baseapp-frontend/componentsto version 0.0.55@baseapp-frontend/design-systemto version 0.0.34@baseapp-frontend/graphqlto version 1.2.4@baseapp-frontend/providerto version 2.0.10@baseapp-frontend/wagtailto version 1.0.19Improvements
setFormApiErrorsandgetApiErrorMessage