Use pothos/errors-plugin #122
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Pothos ErrorsPlugin and typed GraphQL error classes/unions, updates mutations and resolvers to throw/return error union variants, adapts sign-in frontend to handle discriminated-union mutation results with centralized messaging and error codes, and refreshes localization catalogs including a new generic error string. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant GraphQL
participant DB
User->>Frontend: submit login or createNote
Frontend->>GraphQL: mutation request
GraphQL->>DB: lookup / validate / create
DB-->>GraphQL: result
alt Success
GraphQL-->>Frontend: { __typename: "LoginChallenge" | "CreateNotePayload", ... }
Frontend-->>User: show success UI (token / created note)
else Typed Error
GraphQL-->>Frontend: { __typename: "AccountNotFoundError" | "InvalidInputError" | "NotAuthenticatedError", ... }
Frontend->>Frontend: map __typename → errorCode / getSignInMessage()
Frontend-->>User: show localized error message / guidance
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Perlmint, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented a significant improvement to our GraphQL API's error handling by integrating the @pothos/plugin-errors. This change allows us to define and return specific, typed errors for mutations, providing a more robust and predictable error experience for clients. The refactoring primarily focuses on the login and post creation functionalities, ensuring that error responses are no longer generic but convey precise reasons for failure. This will lead to better client-side error management and a more resilient API.
Highlights
- Structured Error Handling: I've integrated the
@pothos/plugin-errorsto provide more structured and explicit error handling within our GraphQL API. This moves us away from generic error messages to specific, typed errors that clients can better interpret and handle. - Improved Login Error Reporting: The login mutations (
loginByUsernameandloginByEmail) now throwLoginErrorwith aLoginErrorKind(e.g.,ACCOUNT_NOT_FOUND) instead of returningnullor generic errors. This provides clearer feedback when a login attempt fails due to an unknown account. - Enhanced Note Creation Error Details: The
createNotemutation has been updated to throwCreateNoteErrorwith specificCreateNoteErrorKindvalues (e.g.,NOT_AUTHENTICATED,REPLY_TARGET_NOT_FOUND,NOTE_CREATION_FAILED). This allows for precise error identification for note creation failures. - GraphQL Schema Updates for Errors: The GraphQL schema has been updated to reflect these new error types and their associated enum kinds. Mutation results are now union types (e.g.,
MutationLoginByUsernameResult) that can return either a success payload or a specific error object. - Client-Side Error Handling Refinement: The client-side sign-in form has been refactored to consume these new structured error responses, displaying more accurate and user-friendly messages based on the specific
LoginErrorKindreceived.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request successfully integrates the pothos/errors-plugin to provide structured errors for GraphQL mutations, which is a great improvement over nullable fields. The changes in graphql/login.ts, graphql/post.ts, and the frontend component web-next/src/routes/sign/index.tsx are well-implemented. My feedback focuses on further improving the code's maintainability, consistency, and robustness by suggesting the use of appropriate scalar types, refactoring duplicated logic, and ensuring exhaustive type checking.
graphql/login.ts
Outdated
| errors: { | ||
| types: [LoginError], | ||
| result: { | ||
| name: "LoginSuccess", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The GraphQL union type for the login mutations is currently implicitly named MutationLoginByUsernameResult. Since this union is shared by both loginByUsername and loginByEmail, it would be clearer to give it an explicit, more generic name like LoginResult.
You can do this by adding a name property to the errors configuration. This should be applied to both loginByUsername and loginByEmail mutations for consistency.
errors: {
name: "LoginResult",
types: [LoginError],
result: {
name: "LoginSuccess",
},
},
There was a problem hiding this comment.
IMO it is better to use separate union types for each mutation, to have more flexibility.
| } else { | ||
| onError(); | ||
| } |
There was a problem hiding this comment.
The else branch here should be unreachable given the GraphQL schema. To make the code more robust and ensure type safety, you could use assertNever to enforce an exhaustive check. This will cause a compile-time error if the union type is ever expanded without this logic being updated.
You'll need to import it: import { assertNever } from "@std/assert/unstable-never";
} else {
assertNever(data);
onError();
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
graphql/builder.ts (1)
12-12: ErrorsPlugin integrated — ensure centralized error handling remains in placeEnabling
@pothos/plugin-errorsis on point. Two operational notes:
- When specifying
errorson fields, some default error logging can be suppressed. Make sure you still have a central logging/observability path (e.g., GraphQL YogaonError) to catch unhandled server/client errors as noted in the PR summary.- Plugin order rarely matters here, but if you see oddities, try loading
ErrorsPluginearlier to ensure its hooks are registered before others.If not already, confirm a global
onErroris configured at the server entry point (not visible in this diff), and exercise both typed and untyped errors in dev to ensure they’re observed/logged as intended.Also applies to: 118-119
graphql/schema.graphql (1)
439-439: LGTM! Union types properly implement structured error handling.The mutation return types correctly use union types to represent both success and error outcomes. The shared union type for both login mutations is appropriate given their identical error handling.
Consider renaming for clarity.
Both
loginByEmailandloginByUsernamereturnMutationLoginByUsernameResult, which might be confusing. Consider a more generic name likeMutationLoginResultfor better clarity.Also applies to: 451-451, 463-463, 473-475
web-next/src/routes/sign/index.tsx (3)
33-52: Optional: Apply the same rename for the email mutation for consistency.Keep value/type namespaces distinct and consistent across both mutations.
-const signByEmailMutation = graphql` +const signByEmailMutationGQL = graphql` mutation signByEmailMutation($locale: Locale!, $email: String!, $verifyUrl: URITemplate!) { loginByEmail(locale: $locale, email: $email, verifyUrl: $verifyUrl) { __typename ... on LoginSuccess { data { account { name handle avatarUrl } token } } ... on LoginError { loginErrorKind } } } ` -const [loginByEmail] = createMutation<signByEmailMutation>( - signByEmailMutation, +const [loginByEmail] = createMutation<signByEmailMutation>( + signByEmailMutationGQL, )Also applies to: 108-111
36-49: Avoid duplicated selection sets with a shared fragment.Both mutations select identical
LoginSuccessfields. Extract a fragment (e.g.,fragment LoginSuccessFields on LoginSuccess { data { account { name handle avatarUrl } token } }) and spread it in both operations to reduce drift and ease schema evolution.Also applies to: 57-70
146-149: Simplify error handling by passing the handler directly.Make the handler accept an optional error and pass it directly to
onErrorto remove wrappers.- onError(_error) { - onError(); - }, + onError,- onError(_error) { - onError(); - }, + onError,- function onError() { + function onError(_err?: unknown) { setChallenging(false); setErrorCode("UNKNOWN"); setToken(undefined); }Also applies to: 160-163, 181-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
deno.json(1 hunks)graphql/builder.ts(3 hunks)graphql/login.ts(5 hunks)graphql/post.ts(6 hunks)graphql/schema.graphql(7 hunks)web-next/src/locales/en-US/messages.po(7 hunks)web-next/src/locales/ja-JP/messages.po(7 hunks)web-next/src/locales/ko-KR/messages.po(7 hunks)web-next/src/locales/zh-CN/messages.po(7 hunks)web-next/src/locales/zh-TW/messages.po(7 hunks)web-next/src/routes/sign/index.tsx(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
graphql/login.ts (1)
graphql/builder.ts (2)
builder(109-153)ValuesOfEnumType(33-34)
🪛 Biome (2.1.2)
web-next/src/routes/sign/index.tsx
[error] 54-54: Shouldn't redeclare 'signByUsernameMutation'. Consider to delete it or rename it.
'signByUsernameMutation' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: image (ubuntu-latest)
- GitHub Check: image (ubuntu-24.04-arm)
🔇 Additional comments (25)
web-next/src/locales/en-US/messages.po (2)
74-77: Reference updates onlyThese look like source line updates from extraction; no behavioral impact.
Also applies to: 118-121, 126-129, 130-133, 292-295, 301-304, 334-338, 347-350, 351-354, 395-398
359-362: LGTM — new generic error message is wired and translated across all locales
- Extracted at src/routes/sign/index.tsx:202, confirming it’s used in the sign-in flow’s unknown error path.
- Present in web-next/src/locales/*/messages.po (en-US, ja-JP, ko-KR, zh-CN, zh-TW).
web-next/src/locales/zh-CN/messages.po (2)
74-77: 仅更新了源码引用行号纯属同步引用行号，无需额外更改。
Also applies to: 118-121, 126-129, 130-133, 292-295, 301-304, 334-338, 347-350, 351-354, 395-398
359-362: 新增通用错误文案 — 已验证通过译文简洁准确，已在所有语言包（en-US、ja-JP、ko-KR、zh-CN、zh-TW）中添加，无遗漏。
graphql/builder.ts (1)
33-35: VerifyValuesOfEnumTypeGeneric AlignmentI attempted to run your type checks with
deno task check, but Deno isn’t available in this environment. Please confirm in your local setup that:
ValuesOfEnumType<T>correctly infers the enum’s value type (the 3rd generic) for everyPothosSchemaTypes.EnumRefyou’re using.- Run
deno task check(ordeno run --allow-read deps.ts && deno run --allow-read src/) locally to ensure nothing breaks.If the inference doesn’t line up with your Pothos version, consider defining your enum values as a
constobject and deriving their union explicitly—for example:export const COLORS = { RED: 'red', GREEN: 'green', BLUE: 'blue', } as const; export type Color = typeof COLORS[keyof typeof COLORS];Then use
Colorwherever you need the enum’s value union.web-next/src/locales/ko-KR/messages.po (2)
74-77: 소스 참조 라인만 변경추출 과정에서의 참조 갱신으로 보입니다. 동작 영향 없습니다.
Also applies to: 118-121, 126-129, 130-133, 292-295, 301-304, 334-338, 347-350, 351-354, 395-398
359-362: 새 오류 메시지 모든 로케일에 반영됨 확인 완료“Something went wrong—please try again.” 메시지가 en-US, ja-JP, ko-KR, zh-CN, zh-TW 모든
messages.po파일에 존재함을 확인했습니다.
톤과 맥락도 적절하므로 이대로 머지해도 무방합니다.web-next/src/locales/ja-JP/messages.po (1)
359-362: LGTM! Well-localized error message.The Japanese translation for the new error message is natural and appropriate. The addition aligns well with the structured error handling improvements described in the PR objectives.
web-next/src/locales/zh-TW/messages.po (1)
359-362: LGTM! Clear and concise Chinese translation.The Traditional Chinese translation is well-written and appropriately conveys the error message meaning. Good consistency with the multilingual error handling enhancement.
graphql/login.ts (4)
22-22: LGTM! Proper import of utility type.Good use of the
ValuesOfEnumTypeutility type for strong typing of error kinds.
32-48: LGTM! Well-structured error handling implementation.The
LoginErrorclass and GraphQL object type are well-designed:
- Proper extension of the
Errorclass with strongly typed error kinds- Clear GraphQL schema exposition with descriptive field naming
- Good use of the utility type for type safety
79-84: LGTM! Proper implementation of structured error handling.The mutation correctly implements the errors plugin pattern:
- Appropriate error types configuration
- Consistent result naming ("LoginSuccess")
- Proper error throwing instead of null returns
This aligns well with the union type approach defined in the GraphQL schema.
Also applies to: 110-112
142-147: LGTM! Consistent error handling across login methods.The
loginByEmailmutation properly mirrors the error handling pattern fromloginByUsername, maintaining consistency in the API while preserving the existing case-insensitive email lookup logic.Also applies to: 187-189
graphql/post.ts (3)
25-50: LGTM! Comprehensive error handling for note creation.The
CreateNoteErrorKindenum covers all relevant error scenarios for note creation, and the implementation follows the same solid pattern established in the login mutations. The error categories are well-defined and cover authentication, validation, and creation failure cases.
488-493: LGTM! Consistent error configuration pattern.The errors configuration follows the established pattern from the login mutations, ensuring API consistency across different mutation types.
497-497: LGTM! Comprehensive error handling throughout the resolver.All error scenarios are properly handled with specific error types:
- Early authentication validation
- Target validation for replies and quotes
- Creation failure handling
The error throwing logic preserves the existing flow while providing better error reporting.
Also applies to: 508-508, 518-518, 547-547
graphql/schema.graphql (3)
273-282: LGTM! Schema accurately reflects error handling implementation.The
CreateNoteErrortype and enum definition properly match the resolver implementation, with all error kinds correctly exposed in the schema.
298-300: LGTM! Appropriate success wrapper type.The
CreateNoteSuccesswrapper properly encapsulates the original payload, enabling the union type approach for structured error handling.
393-403: LGTM! Login error schema matches implementation.The login error types and enum properly reflect the resolver implementation. The single error kind is appropriate for the current scope, with room for expansion as needed.
web-next/src/routes/sign/index.tsx (6)
102-106: Typed error code signal looks good.Using the generated
LoginErrorKindunion plus a local"UNKNOWN"sentinel is clear and type-safe.
122-122: Good UX: reset error state on new input.Clearing
errorCodeon input prevents stale validation UI.
169-179: LGTM: correct discriminated-union handling.
__typenamechecks and branching forLoginSuccess/LoginErrorare solid.
248-248: LGTM: centralized messaging.Switching to
getSignInMessage()simplifies JSX and keeps messaging consistent.
258-258: LGTM: validation state driven by error presence.Binding
validationStatetoerrorCode()is straightforward and matches the new UX.
187-204: All LoginErrorKind values coveredVerified that
LoginErrorKindingraphql/schema.graphqlonly defines one value (ACCOUNT_NOT_FOUND), which is already handled ingetSignInMessage. The catch-alldefaultbranch appropriately covers any other cases (including the"UNKNOWN"error set inonError). No additional mappings are required.
| "@pothos/plugin-errors": "npm:@pothos/plugin-errors@^4.4.2", | ||
| "@pothos/plugin-relay": "npm:@pothos/plugin-relay@^4.6.2", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New errors plugin mapping added — verify version compatibility and standardize semver usage
Good addition. Two quick checks:
- Compatibility: Ensure
@pothos/plugin-errors@^4.4.2declares peer compatibility with@pothos/core@^4.7.3you use. - Consistency: Your Pothos plugins mix exact and caret ranges (e.g.,
@pothos/[email protected]vs others with^). Consider standardizing to reduce accidental partial upgrades.
You can sanity-check peer ranges from npm without cloning:
🏁 Script executed:
#!/bin/bash
# Check peer deps for @pothos/plugin-errors and @pothos/core versions
set -euo pipefail
curl -s https://registry.npmjs.org/@pothos/plugin-errors | jq -r '
.["dist-tags"].latest as $latest
| "latest: " + $latest,
"peerDependencies:",
(.versions[$latest].peerDependencies // {})
'
curl -s https://registry.npmjs.org/@pothos/core | jq -r '."dist-tags".latest as $latest | "core latest: " + $latest'Length of output: 445
Verify peer compatibility and unify semver ranges in deno.json
- @pothos/plugin-errors@^4.4.2 declares
@pothos/core: "*"so it’s compatible with your core version. - Your other plugins mix exact and caret ranges (e.g.
@pothos/[email protected]vs.^4.x). Pick one style to avoid surprises.
Files to update:
- deno.json
Suggested change:
"@pothos/plugin-errors": "npm:@pothos/plugin-errors@^4.4.2",
"@pothos/plugin-relay": "npm:@pothos/plugin-relay@^4.6.2",
- "@pothos/plugin-complexity": "npm:@pothos/[email protected]",
+ "@pothos/plugin-complexity": "npm:@pothos/plugin-complexity@^4.1.1",📝 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.
| "@pothos/plugin-errors": "npm:@pothos/plugin-errors@^4.4.2", | |
| "@pothos/plugin-relay": "npm:@pothos/plugin-relay@^4.6.2", | |
| "@pothos/plugin-errors": "npm:@pothos/plugin-errors@^4.4.2", | |
| "@pothos/plugin-relay": "npm:@pothos/plugin-relay@^4.6.2", | |
| "@pothos/plugin-complexity": "npm:@pothos/plugin-complexity@^4.1.1", |
🤖 Prompt for AI Agents
In deno.json around lines 45 to 46, the version specifications for @pothos
plugins mix exact and caret semver ranges, which can cause inconsistencies.
Verify that all @pothos plugins use compatible peer dependencies, then unify the
version ranges by choosing either caret (^) or exact versions consistently
across all @pothos plugin entries to ensure predictable dependency resolution.
Perlmint
force-pushed
the
feature/errors-plugin
branch
from
d46e5a4 to
7e4fdb7
Compare
|
rebased |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
web-next/src/routes/sign/index.tsx (1)
54-73: Fix GraphQL constant redeclaration to avoid naming conflict.The constant
signByUsernameMutationconflicts with the imported type of the same name, causing a Biome error.Apply this diff to fix the naming conflict:
-const signByUsernameMutation = graphql` +const signByUsernameMutationGQL = graphql` mutation signByUsernameMutation($locale: Locale!, $username: String!, $verifyUrl: URITemplate!) { loginByUsername(locale: $locale, username: $username, verifyUrl: $verifyUrl) { __typename ... on LoginSuccess { data { account { name handle avatarUrl } token } } ... on LoginError { loginErrorKind } } } `;Also update line 111-113:
- const [loginByUsername] = createMutation<signByUsernameMutation>( - signByUsernameMutation, + const [loginByUsername] = createMutation<signByUsernameMutation>( + signByUsernameMutationGQL, );
🧹 Nitpick comments (2)
web-next/src/routes/sign/index.tsx (1)
33-52: Consider renaming the email mutation constant for consistency.For consistency with the username mutation fix, consider renaming this constant as well to avoid potential future conflicts.
-const signByEmailMutation = graphql` +const signByEmailMutationGQL = graphql` mutation signByEmailMutation($locale: Locale!, $email: String!, $verifyUrl: URITemplate!) { loginByEmail(locale: $locale, email: $email, verifyUrl: $verifyUrl) { __typename ... on LoginSuccess { data { account { name handle avatarUrl } token } } ... on LoginError { loginErrorKind } } } `;And update line 108-110:
- const [loginByEmail] = createMutation<signByEmailMutation>( - signByEmailMutation, + const [loginByEmail] = createMutation<signByEmailMutation>( + signByEmailMutationGQL, );graphql/post.ts (1)
494-551: Consider adding error logging before throwing typed errors.Since the PR mentions that specifying
errorsprevents logging, consider adding explicit logging before throwing errors to maintain observability.Example for line 497:
if (session == null) { + console.error("CreateNote failed: User not authenticated"); throw new CreateNoteError("NOT_AUTHENTICATED"); }Similar logging could be added at lines 508, 518, and 547.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
deno.json(1 hunks)graphql/builder.ts(3 hunks)graphql/login.ts(5 hunks)graphql/post.ts(6 hunks)graphql/schema.graphql(7 hunks)web-next/src/locales/en-US/messages.po(7 hunks)web-next/src/locales/ja-JP/messages.po(7 hunks)web-next/src/locales/ko-KR/messages.po(7 hunks)web-next/src/locales/zh-CN/messages.po(7 hunks)web-next/src/locales/zh-TW/messages.po(7 hunks)web-next/src/routes/sign/index.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- deno.json
- web-next/src/locales/ko-KR/messages.po
- web-next/src/locales/zh-TW/messages.po
- web-next/src/locales/en-US/messages.po
- graphql/login.ts
- web-next/src/locales/ja-JP/messages.po
- web-next/src/locales/zh-CN/messages.po
🧰 Additional context used
🧬 Code Graph Analysis (1)
graphql/post.ts (1)
graphql/builder.ts (2)
builder(112-156)ValuesOfEnumType(33-34)
🪛 Biome (2.1.2)
web-next/src/routes/sign/index.tsx
[error] 54-54: Shouldn't redeclare 'signByUsernameMutation'. Consider to delete it or rename it.
'signByUsernameMutation' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: image (ubuntu-24.04-arm)
🔇 Additional comments (3)
graphql/schema.graphql (1)
273-301: LGTM! Well-structured error handling schema.The GraphQL schema changes properly implement the error union pattern with typed error kinds. This provides excellent type safety and clarity for API consumers.
Also applies to: 393-404, 439-439, 451-463, 473-476
graphql/builder.ts (1)
12-12: LGTM! Clean ErrorsPlugin integration.The ErrorsPlugin is properly integrated and the
ValuesOfEnumTypeutility type is well-designed for extracting enum values from Pothos enum references.Also applies to: 33-34, 121-121
graphql/post.ts (1)
25-50: Well-implemented typed error handling for createNote mutation.The error handling implementation follows the Pothos errors plugin pattern correctly with typed error kinds.
Regarding the PR concerns mentioned:
- Each error type has a unique field name (
createNoteErrorKindvsloginErrorKind) avoiding union conflicts- Consider implementing error logging in the resolver before throwing errors if needed
- The
onErrorhandler requirement is by design for unhandled errors
graphql/login.ts
Outdated
| const LoginErrorKind = builder.enumType("LoginErrorKind", { | ||
| values: ["ACCOUNT_NOT_FOUND"] as const, | ||
| }); | ||
|
|
||
| class LoginError extends Error { | ||
| public constructor( | ||
| public readonly kind: ValuesOfEnumType<typeof LoginErrorKind>, | ||
| ) { | ||
| super(`Login error - ${kind}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current approach — creating a single error type with a kind enum — is not the recommended approach when using @pothos/plugin-errors. It is recommended to create separate types for each error case and then include all of them within the result union. The main advantage of this approach is being able to add error-specific fields to each error type. Is there any specific reason why you went with the current approach?
There was a problem hiding this comment.
There is no special reason to this approach. Just result of trial & error. At first, I tried to set error.types ErrorKind enum type. But, It accept only object type. So, I simply wrapped it.
As your recommendation, I'll make different types for each error. Thanks.
graphql/login.ts
Outdated
| loginErrorKind: t.field({ | ||
| type: LoginErrorKind, | ||
| resolve: (error) => error.kind, | ||
| }), |
There was a problem hiding this comment.
You can use t.expose() to conveniently expose a field in the parent object.
Perlmint
force-pushed
the
feature/errors-plugin
branch
from
8202684 to
b23c7dc
Compare
|
I tried to make errors re-usable types. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
web-next/src/routes/sign/index.tsx (1)
53-73: Fix second redeclaration and hoist __typename on loginByUsernameSame issues as above: rename the document constant to avoid colliding with the imported type name and hoist __typename for robust discrimination.
Apply this diff:
-const signByUsernameMutation = graphql` +const signByUsernameMutationGQL = graphql` mutation signByUsernameMutation($locale: Locale!, $username: String!, $verifyUrl: URITemplate!) { loginByUsername(locale: $locale, username: $username, verifyUrl: $verifyUrl) { - ... on LoginSuccess { - __typename + __typename + ... on LoginSuccess { data { account { name handle avatarUrl } token } } - ... on AccountNotFoundError { - __typename - } + ... on AccountNotFoundError {} } } `And update the mutation usage:
const [loginByUsername] = createMutation<signByUsernameMutation>(signByUsernameMutationGQL);
♻️ Duplicate comments (1)
web-next/src/routes/sign/index.tsx (1)
174-184: Enforce exhaustive handling for union types (use assertNever)The else branch is meant to be unreachable if the union expands in the future. Enforce exhaustiveness to catch mismatches at compile-time.
Apply this diff:
if (data.__typename === "LoginSuccess") { setToken(data.data.token); codeInput?.focus(); } else if ( data.__typename === "AccountNotFoundError" ) { setErrorCode(LoginError.ACCOUNT_NOT_FOUND); } else { - onError(); + // Enforce exhaustiveness if the union grows + assertNever(data as never); + onError(); }And add the import (outside this range):
import { assertNever } from "@std/assert/unstable-never";
🧹 Nitpick comments (6)
graphql/builder.ts (2)
33-35: Verify EnumRef generic inference for future-proofingThis conditional type depends on the exact generic parameter order of PothosSchemaTypes.EnumRef. If the library updates, this may silently break and start producing never. Please verify against the installed version and consider adding a lightweight unit test or a comment with a link to the type definition to document the intent.
121-122: Centralize error/result types to avoid “auto-naming” drift and promote reuseEnabling ErrorsPlugin is good. Given the PR concerns about auto-generated result.name differences across mutations and the need for reusable error types, consider centralizing:
- Named objectRefs for error types (e.g., AccountNotFoundError, InvalidInputError, NotAuthenticatedError) in a single module and reusing them across fields.
- Explicitly named “Result” wrapper/object types per success/error union to keep names stable, rather than relying on auto-generated names.
This will also make it easier to ensure consistent logging, serialization, and translation keys.
web-next/src/routes/sign/index.tsx (4)
23-26: Import the data shape for shared onCompleted typingSince you pass both response.loginByEmail and response.loginByUsername to onCompleted, importing the data type for the email mutation lets you annotate onCompleted with a union of both shapes for better type safety.
Apply this diff to extend the type import:
-import type { - signByEmailMutation, -} from "./__generated__/signByEmailMutation.graphql.ts"; +import type { + signByEmailMutation, + signByEmailMutation$data, +} from "./__generated__/signByEmailMutation.graphql.ts";
95-99: Avoid const enum for cross-tooling compatibilityconst enum can be problematic across build pipelines (SWC/Babel/transforms without TS const-enum erasure). A regular enum is safer here with negligible cost.
Apply this diff:
-const enum LoginError { +enum LoginError { ACCOUNT_NOT_FOUND, UNKNOWN, }
192-208: Minor: The null case is likely unreachableerrorCode is typed as LoginError | undefined; null should never occur. You can drop the null case or widen the type if null is expected elsewhere.
263-263: Optional: Only mark the field invalid for specific user errorsCurrently, any error (including UNKNOWN/network) marks the input as invalid. If you want to restrict the invalid state to user-actionable errors (e.g., account not found), consider narrowing:
- validationState={errorCode() ? "invalid" : "valid"} + validationState={errorCode() === LoginError.ACCOUNT_NOT_FOUND ? "invalid" : "valid"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
deno.json(1 hunks)graphql/builder.ts(3 hunks)graphql/login.ts(5 hunks)graphql/post.ts(6 hunks)graphql/schema.graphql(8 hunks)graphql/session.ts(1 hunks)web-next/src/locales/en-US/messages.po(6 hunks)web-next/src/locales/ja-JP/messages.po(6 hunks)web-next/src/locales/ko-KR/messages.po(6 hunks)web-next/src/locales/zh-CN/messages.po(6 hunks)web-next/src/locales/zh-TW/messages.po(6 hunks)web-next/src/routes/sign/index.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- graphql/session.ts
- graphql/post.ts
- deno.json
- web-next/src/locales/en-US/messages.po
- web-next/src/locales/zh-CN/messages.po
- graphql/schema.graphql
- graphql/login.ts
- web-next/src/locales/zh-TW/messages.po
- web-next/src/locales/ko-KR/messages.po
- web-next/src/locales/ja-JP/messages.po
🧰 Additional context used
🪛 Biome (2.1.2)
web-next/src/routes/sign/index.tsx
[error] 53-53: Shouldn't redeclare 'signByUsernameMutation'. Consider to delete it or rename it.
'signByUsernameMutation' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (7)
graphql/builder.ts (1)
12-12: ErrorsPlugin import looks correctBringing in @pothos/plugin-errors is consistent with the PR goal and matches the usage below.
web-next/src/routes/sign/index.tsx (6)
106-111: LGTM: Explicit error state signal typeClear, explicit typing for errorCode improves readability and avoids accidental null assignments.
127-127: LGTM: Resetting error state on inputResetting error state on new input avoids stale validation UI.
151-154: LGTM: Network error path handledThe onError handler funnels to a shared onError() which resets transient state—good centralization.
165-168: LGTM: Symmetric error handling for username flowMirrors the email path for consistency.
186-190: LGTM: Centralized unknown-error pathGood place to reset spinner, clear token, and provide generic messaging.
253-253: LGTM: Centralized sign-in status messagingSwitching UI copy based on token/errorCode improves UX and keeps copy in one place.
graphql/schema.graphql
Outdated
| @@ -443,10 +459,18 @@ type Mutation { | |||
| updateAccount(input: UpdateAccountInput!): UpdateAccountPayload! | |||
| } | |||
|
|
|||
| union MutationCreateNoteResult = CreateNoteSuccess | InvalidInputError | NotAuthenticatedError | |||
There was a problem hiding this comment.
- Can we get rid of the
Mutationprefix? I believe it's configurable - Can we directly use the payload type instead of having a
Successwrapper?
There was a problem hiding this comment.
I found configurations for all of them.
ex) server internal error, client network error
Perlmint
force-pushed
the
feature/errors-plugin
branch
from
f0e05d3 to
d44ab93
Compare
|
rebase & apply suggestions |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
graphql/schema.graphql (1)
433-446: Typo in mutation argument description: “variabvles” → “variables”Public schema documentation has a misspelling twice in the
verifyUrldescriptions.Apply the fix in the source descriptions (not directly in the generated schema), then re-generate:
- The RFC 6570-compliant URI Template for the verification link. Available variabvles: `{token}` and `{code}`. + The RFC 6570-compliant URI Template for the verification link. Available variables: `{token}` and `{code}`.
♻️ Duplicate comments (2)
web-next/src/routes/sign/index.tsx (2)
51-68: Fix the redeclaration issue for signByUsernameMutation.Similar to the email mutation, this constant redeclares the imported type name.
-const signByUsernameMutation = graphql` +const signByUsernameMutationGQL = graphql`And update the mutation usage:
-const [loginByUsername] = createMutation<signByUsernameMutation>( - signByUsernameMutation, +const [loginByUsername] = createMutation<signByUsernameMutation>( + signByUsernameMutationGQL,
23-25: Fix the redeclaration issue.The constant
signByEmailMutationredeclares the imported type name, which Biome flags as an error. This needs to be resolved to avoid naming conflicts.-import type { - signByEmailMutation, -} from "./__generated__/signByEmailMutation.graphql.ts"; +import type { signByEmailMutation } from "./__generated__/signByEmailMutation.graphql.ts";Also rename the GraphQL document constant to avoid the redeclaration:
-const signByEmailMutation = graphql` +const signByEmailMutationGQL = graphql`And update the mutation usage:
-const [loginByEmail] = createMutation<signByEmailMutation>( - signByEmailMutation, +const [loginByEmail] = createMutation<signByEmailMutation>( + signByEmailMutationGQL,
🧹 Nitpick comments (5)
graphql/builder.ts (3)
33-35: Make the enum value extractor resilient to future Pothos generic changesUsing
neverfor the first type parameter ofEnumRefis brittle and may fail to match if the generic parameters change upstream. Preferunknownto keep the conditional type broadly applicable.-export type ValuesOfEnumType<T> = T extends - PothosSchemaTypes.EnumRef<never, unknown, infer V> ? V : never; +export type ValuesOfEnumType<T> = T extends + PothosSchemaTypes.EnumRef<unknown, unknown, infer V> ? V : never;
156-165: Consider including parent type in default union name to avoid collisionsCurrent naming uses only
fieldName(e.g.,createNote->CreateNoteResult). If two different parent types define the same field name and both use error unions, you can collide in the global GraphQL type namespace. Prefer including the parent type name in the union name (e.g.,MutationCreateNoteResultor${parentTypeName}${PascalCase(fieldName)}Result).You can verify current uniqueness across the schema with a quick scan for union names ending with
Result. If you want, I can generate a script to diff and flag duplicates automatically.
156-165: Add onError to centralize logging for unhandled errorsPer your PR notes, specifying
errorscan suppress logging. Add anonErrorhandler here to centralize logging/telemetry while still returning typed results.errors: { directResult: true, + // Log unhandled/unknown errors while preserving typed results behavior + onError: (error) => { + // replace with your logger + console.error(error); + }, defaultUnionOptions: { name(options) { return `${options.fieldName.charAt(0).toUpperCase()}${ options.fieldName.slice(1) }Result`; }, }, },graphql/session.ts (2)
5-9: Preserve error.name for better diagnosticsSet the
nameof the error explicitly so logs and tooling don’t show a generic “Error”.export class NotAuthenticatedError extends Error { - public constructor() { - super("Not authenticated"); - } + public constructor() { + super("Not authenticated"); + this.name = "NotAuthenticatedError"; + } }
11-18: Use a meaningful sentinel value for the discriminator fieldReturning an empty string makes the field less useful for debugging and analytics. Consider a stable code like "NOT_AUTHENTICATED".
notAuthenticated: t.string({ - resolve: () => "", + resolve: () => "NOT_AUTHENTICATED", }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
deno.json(1 hunks)graphql/builder.ts(4 hunks)graphql/login.ts(5 hunks)graphql/post.ts(6 hunks)graphql/schema.graphql(8 hunks)graphql/session.ts(1 hunks)web-next/src/locales/en-US/messages.po(6 hunks)web-next/src/locales/ja-JP/messages.po(6 hunks)web-next/src/locales/ko-KR/messages.po(6 hunks)web-next/src/locales/zh-CN/messages.po(6 hunks)web-next/src/locales/zh-TW/messages.po(6 hunks)web-next/src/routes/sign/index.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- deno.json
- web-next/src/locales/ko-KR/messages.po
- web-next/src/locales/zh-TW/messages.po
- graphql/post.ts
- web-next/src/locales/ja-JP/messages.po
🧰 Additional context used
🧬 Code Graph Analysis (1)
graphql/login.ts (1)
graphql/builder.ts (1)
builder(112-166)
🪛 Biome (2.1.2)
web-next/src/routes/sign/index.tsx
[error] 51-51: Shouldn't redeclare 'signByUsernameMutation'. Consider to delete it or rename it.
'signByUsernameMutation' is defined here:
(lint/suspicious/noRedeclare)
🪛 GitHub Check: test
web-next/src/locales/en-US/messages.po
[failure] 131-131:
Generated content differs from committed version
[failure] 187-187:
Generated content differs from committed version
[failure] 183-183:
Generated content differs from committed version
[failure] 175-175:
Generated content differs from committed version
[failure] 349-349:
Generated content differs from committed version
[failure] 358-358:
Generated content differs from committed version
[failure] 392-392:
Generated content differs from committed version
🔇 Additional comments (22)
graphql/builder.ts (1)
12-12: ErrorsPlugin registration looks goodAdding @pothos/plugin-errors and registering it in the plugin list is the right move for typed error results.
Also applies to: 121-122
web-next/src/locales/zh-CN/messages.po (1)
416-419: New generic error string looks goodThe addition of “Something went wrong—please try again.” → “出现错误，请重试。” is appropriate and consistent with tone of other messages.
web-next/src/locales/en-US/messages.po (2)
416-419: New generic error string entry added correctlyThe new “Something went wrong—please try again.” entry is present and matches the source string.
131-131: Fix test failure: regenerate and commit Lingui catalogsThe CI “Generated content differs from committed version” failures align with these updated source refs. Please run your Lingui extraction step (e.g.,
pnpm -w i18n:extractor your project’s equivalent) and commit the regeneratedmessages.pofiles for all locales so tests pass.Also applies to: 175-175, 183-183, 187-187, 349-349, 358-358, 392-392
graphql/schema.graphql (1)
81-84: Schema-level changes align with ErrorsPlugin usage
- New error types (AccountNotFoundError, InvalidInputError, NotAuthenticatedError) and the result unions (LoginResult, CreateNoteResult) look consistent.
- Mutations updated to return the unions also match the builder’s default union naming strategy.
Ensure client codegen is updated and frontend branches on __typename for these unions (it looks like Sign-in flow was adjusted accordingly).
Also applies to: 369-372, 388-389, 424-449, 462-465, 291-292
graphql/login.ts (7)
24-24: Reordered import looks good.The SessionRef import has been moved to its proper position maintaining alphabetical order.
28-32: LGTM! Well-defined error class.The AccountNotFoundError class properly extends Error and includes the query field for debugging purposes. The constructor implementation is correct.
34-39: LGTM! Proper GraphQL object type definition.The GraphQL object type correctly exposes the query field using
t.exposeString. This follows Pothos best practices for error types.
70-78: Well-implemented error handling with Pothos ErrorsPlugin.The error configuration properly defines the union type structure with appropriate naming. The use of AccountNotFoundError in the types array aligns with the error class definition above.
104-106: LGTM! Proper error throwing implementation.The code correctly throws the AccountNotFoundError with the username as context, which will be caught by the ErrorsPlugin and converted to the appropriate GraphQL union member.
136-144: Consistent error handling configuration.The loginByEmail mutation uses the same error configuration as loginByUsername, maintaining consistency in the API design.
184-186: LGTM! Consistent error throwing for email login.The error handling matches the username login implementation, throwing AccountNotFoundError with the email as context.
web-next/src/routes/sign/index.tsx (10)
32-49: LGTM! GraphQL mutation properly handles discriminated unions.The mutation correctly requests
__typenamefor union discrimination and includes the necessary fields for both success and error cases. The fragment structure aligns with the GraphQL schema changes.
91-94: LGTM! Well-defined internal error enumeration.The LoginError enum provides clear categorization of different error states for internal state management.
102-107: Good refactoring from boolean to error code.Replacing the
invalidboolean with anerrorCodeallows for more specific error handling and better user feedback.
120-124: LGTM! Input handling resets error state.The
onInputfunction correctly clears the error code when the user starts typing, providing immediate feedback.
147-149: LGTM! Consistent error handling across mutations.Both mutation calls now include proper
onErrorhandlers that delegate to the centralized error handling function.Also applies to: 161-163
168-180: Robust union type discrimination and error handling.The
onCompletedfunction properly handles the discriminated union response by checking__typename. The error handling covers both AccountNotFoundError and unexpected cases.
182-186: LGTM! Centralized error handling function.The
onErrorfunction properly resets all relevant state and sets the error code to UNKNOWN for unexpected errors.
188-205: Excellent user-facing message logic.The
getSignInMessagefunction provides appropriate feedback for all possible states:
- Success state (token exists)
- Account not found error
- Default prompt
- Generic error fallback
The implementation is clear and handles all edge cases properly.
249-249: LGTM! Centralized message display.Using
getSignInMessage()provides consistent and contextual user feedback based on the current state.
259-259: LGTM! Form validation state reflects error status.The TextField's
validationStatecorrectly reflects whether there's an error, providing visual feedback to the user.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
web-next/src/locales/ja-JP/messages.po (1)
416-419: Unify style with existing JA strings (“もう一度お試しください。”)Elsewhere in this file (e.g., Line 139–142), the retry phrasing uses “もう一度お試しください。” Consider aligning for consistency.
msgid "Something went wrong—please try again." -msgstr "問題が発生しました。再度お試しください。" +msgstr "問題が発生しました。もう一度お試しください。"web-next/src/locales/zh-TW/messages.po (6)
131-134: Use zh-TW phrasing and fix punctuation order.Prefer zh-TW “電子郵件信箱” over “郵箱”, and place the full stop after the parenthetical.
-msgstr "登入連結已傳送至您的郵箱。請檢查收件匣。（或垃圾郵件資料夾）" +msgstr "登入連結已傳送至您的電子郵件信箱。請檢查收件匣（或垃圾郵件資料夾）。"
175-178: Adopt zh-TW word choice for “invite-only” and “聯絡”.“採用邀請制” and “聯絡” read more native in zh-TW.
-msgstr "需要建立帳戶嗎？Hackers' Pub 僅限邀請，請聯繫朋友邀請您。" +msgstr "需要建立帳戶嗎？Hackers' Pub 採用邀請制，請聯絡朋友邀請您。"
183-186: Unify with “Email address” translation.For consistency, prefer “電子郵件地址” in field labels.
-msgstr "電子郵件或使用者名稱" +msgstr "電子郵件地址或使用者名稱"
187-190: Keep consistency with prior label wording.Align with the previous suggestion to use “電子郵件地址”.
-msgstr "請在下方輸入您的電子郵件或使用者名稱以登入。" +msgstr "請在下方輸入您的電子郵件地址或使用者名稱以登入。"
358-361: Prefer “電子郵件” over “郵件” in zh-TW.Minor wording tweak for locale consistency.
-msgstr "或輸入郵件中的驗證碼" +msgstr "或輸入電子郵件中的驗證碼"
404-407: Optional: reflect progressive aspect.Using “正在登入 Hackers' Pub” better mirrors “Signing in …”.
-msgstr "登入 Hackers' Pub" +msgstr "正在登入 Hackers' Pub"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
web-next/src/locales/en-US/messages.po(7 hunks)web-next/src/locales/ja-JP/messages.po(7 hunks)web-next/src/locales/ko-KR/messages.po(7 hunks)web-next/src/locales/zh-CN/messages.po(7 hunks)web-next/src/locales/zh-TW/messages.po(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- web-next/src/locales/zh-CN/messages.po
- web-next/src/locales/ko-KR/messages.po
- web-next/src/locales/en-US/messages.po
🔇 Additional comments (7)
web-next/src/locales/ja-JP/messages.po (2)
131-131: Location updates only — translations unchanged, LGTMOnly source references moved; msgid/msgstr pairs remain intact.
Also applies to: 175-175, 183-183, 187-187, 249-249, 349-349, 358-358, 392-392, 404-404, 408-408
416-419: All locales updated with translations and no fuzzy flagsVerified that the new msgid
"Something went wrong—please try again."appears in all locale files and none are marked fuzzy:
- web-next/src/locales/en-US/messages.po
- web-next/src/locales/ja-JP/messages.po
- web-next/src/locales/zh-CN/messages.po
- web-next/src/locales/zh-TW/messages.po
- web-next/src/locales/ko-KR/messages.po
web-next/src/locales/zh-TW/messages.po (5)
249-252: LGTM on relocation.No text change; the app name translation is correct and consistent.
349-352: LGTM.Translation is clear and faithful to the source.
391-395: LGTM.“登入” is the standard zh-TW term for “Sign in”.
408-411: LGTM.Consistent with the progressive form and ellipsis.
416-419: Fallback string "Something went wrong—please try again." is consistently used
- The msgid is present and translated in all locales (en-US, zh-CN, zh-TW, ja-JP, ko-KR).
- Code references at src/routes/sign/index.tsx:203 and src/components/ArticleCard.tsx:231 both use the i18n tag.
- No raw English fallback strings remain in the code.
Everything looks properly deduplicated and localized—no further action required.
This changes make specifying error type on schema available.
There are some concerned things:
errorsprevents logging error.result.nameis automatically generated. Therefore, it creates different type/field names for the same types in different mutations.onErrorseems mandatory. unhandled server errors and client errors can only be handled byonError.If it looks good, I'll apply this to more mutations.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores