fix: authenticate doesn't populate user with 0 id#176
Open
GaborTorma wants to merge 15 commits intomarshallswain:mainfrom
Open
fix: authenticate doesn't populate user with 0 id#176GaborTorma wants to merge 15 commits intomarshallswain:mainfrom
GaborTorma wants to merge 15 commits intomarshallswain:mainfrom
Conversation
GaborTorma
commented
Apr 23, 2025
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses an authentication issue where users with an ID of 0 were not properly populated during authentication and includes comprehensive test standardization changes.
Key changes:
- Fixed authentication logic to properly handle users with ID 0 by changing the validation from falsy check to null check
- Standardized test syntax from
test()toit()across the entire codebase - Updated import organization to follow consistent patterns throughout the project
Reviewed Changes
Copilot reviewed 108 out of 117 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/use-auth/use-auth.test.ts | Fixed authentication test to expect user ID 0 instead of 1, supporting the main fix |
| tests/fixtures/feathers.ts | Updated mock authentication response to return user with ID 0 for testing |
| src/use-auth/use-auth.ts | Fixed authentication bug by changing !userId.value to userId.value === null to properly handle ID 0 |
| Multiple test files | Standardized test function calls from test() to it() across all test files |
| Multiple source files | Reorganized imports for consistent ordering and formatting |
[nitpick] The authentication logic correctly changes from falsy check to strict null check, but the operator precedence should be clarified. Consider using explicit parentheses around the null check to make the intent clearer: if (!entityService || (userId.value === null)). Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The TypeScript error suppression comment '// @ts-expect-error test' appears to be a placeholder or debugging comment that should be removed or replaced with a proper explanation of why the error is expected. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
[nitpick] The nullish coalescing operator (??) is used here, which is inconsistent with the strict null check on line 51. For consistency, consider using the same comparison approach throughout the authentication logic. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Another TypeScript error suppression comment '// @ts-expect-error test' that appears to be a placeholder. This should be properly documented or the underlying TypeScript issue should be resolved. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.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
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.
I debug and fixed this issue and change test test for it.
Please merge it.
Thanks!