Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds extraction and prioritization of an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/sdk/src/modules/core/errors/chain-errors.spec.ts (1)
353-361:shouldTriggerAuthFallbacktests look consistent withparseChainErrortests.Both
invalid_grantandunauthorized_accesscorrectly asserttrue. One nit: theerror_descriptionin line 359 ("The IP is not authorized") differs slightly from line 144 ("The IP 1.2.3.4 is not authorized"). Both should be equivalent for pattern matching, but keeping them identical across tests avoids any future confusion if the pattern ever becomes more specific.🔧 Align test fixture with the `parseChainError` counterpart
- const error = { error: "unauthorized_access", error_description: "The IP is not authorized" }; + const error = { error: "unauthorized_access", error_description: "The IP 1.2.3.4 is not authorized" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/modules/core/errors/chain-errors.spec.ts` around lines 353 - 361, Update the HiveSigner test fixture so the error_description matches the parseChainError counterpart: change the error object in the shouldTriggerAuthFallback test for "unauthorized_access" to use "The IP 1.2.3.4 is not authorized" (keeping tests for shouldTriggerAuthFallback and parseChainError consistent for pattern matching).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk/src/modules/core/errors/chain-errors.spec.ts`:
- Around line 143-148: The test "should parse HiveSigner unauthorized_access
error" currently only asserts result.type === ErrorType.TOKEN_EXPIRED but omits
result.message; update the spec for parseChainError to assert the expected
message as well (either the IP-restriction phrasing like "Access denied from
this IP" or keep the existing "Authentication token expired. Please log in
again." if that is the intended fallback), referencing the test case name and
the parseChainError result so the assertion verifies both result.type and
result.message and exposes the semantic mismatch between unauthorized_access and
the hardcoded TOKEN_EXPIRED message.
---
Nitpick comments:
In `@packages/sdk/src/modules/core/errors/chain-errors.spec.ts`:
- Around line 353-361: Update the HiveSigner test fixture so the
error_description matches the parseChainError counterpart: change the error
object in the shouldTriggerAuthFallback test for "unauthorized_access" to use
"The IP 1.2.3.4 is not authorized" (keeping tests for shouldTriggerAuthFallback
and parseChainError consistent for pattern matching).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk/src/modules/core/mutations/use-broadcast-mutation.ts (1)
208-208:⚠️ Potential issue | 🟠 MajorBroad
/\bforbidden\b/ipattern in TOKEN_EXPIRED check risks false auth fallbacksThe implementation at packages/sdk/src/modules/core/errors/chain-errors.ts:193 includes a broad
/\bforbidden\b/ipattern that will match any error containing the word "forbidden" and classify it as TOKEN_EXPIRED, triggering auth fallback. While HiveSigner-specific error codes (invalid_grant,unauthorized_access) are properly scoped, this regex pattern will incorrectly match HTTP 403 errors unrelated to authentication—such as rate-limiting errors, banned content messages, or other access denial scenarios. The specific pattern at line 140 ("votes evaluating for comment that is paid out is forbidden") is checked first and returns COMMON, but this only covers one specific case. Any other 403 error containing "forbidden" will incorrectly trigger the auth fallback chain instead of surfacing the actual error.Consider narrowing the TOKEN_EXPIRED check to explicitly scoped patterns (e.g., HiveSigner error descriptions, "token" or "authorization" keywords in proximity to "forbidden") rather than a word-boundary-only match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/modules/core/mutations/use-broadcast-mutation.ts` at line 208, The TOKEN_EXPIRED classification is too broad because it treats any error containing the word "forbidden" as an auth expiration and causes shouldTriggerAuthFallback to run; narrow the `/\bforbidden\b/i` pattern in chain-errors.ts (the TOKEN_EXPIRED detection logic) to only match auth-scoped messages such as HiveSigner descriptors (e.g., "invalid_grant", "unauthorized_access") or instances where "forbidden" appears near token/authorization words (e.g., "forbidden.*token|authorization|access token") or check for a 403 HTTP status combined with auth keywords; update the TOKEN_EXPIRED regex/conditions accordingly and add/adjust unit tests that exercise forbidden-but-not-auth errors to ensure non-auth 403s are not classified as TOKEN_EXPIRED.
🧹 Nitpick comments (1)
packages/sdk/src/modules/core/types/platform-adapter.ts (1)
121-121: Tightenauthoritytype fromstringto the inline union already used elsewhere in this file
broadcastWithKeychainandbroadcastWithHiveAuthboth use"posting" | "active" | "owner" | "memo"for the equivalent parameter. Using the same inline union forgetLoginTypeprevents implementations from accidentally handling arbitrary strings and keeps the interface self-documenting without needing an external import.♻️ Proposed tightening
- getLoginType: (username: string, authority?: string) => Promise<string | null | undefined>; + getLoginType: (username: string, authority?: 'posting' | 'active' | 'owner' | 'memo') => Promise<string | null | undefined>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/modules/core/types/platform-adapter.ts` at line 121, The getLoginType signature currently accepts authority?: string; tighten it to the same inline union used elsewhere ("posting" | "active" | "owner" | "memo") to prevent arbitrary strings and keep the interface self-documenting: change the getLoginType declaration (function name getLoginType in platform-adapter.ts) to use authority?: "posting" | "active" | "owner" | "memo"; and update any local callers/implementations to accept/return the narrowed type accordingly so types remain consistent with broadcastWithKeychain and broadcastWithHiveAuth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk/src/modules/core/mutations/use-broadcast-mutation.ts`:
- Line 185: The web adapter's getLoginType currently ignores the authority
parameter which can return incorrect login types; update the
web-broadcast-adapter.ts implementation of getLoginType to accept the signature
getLoginType(username: string, authority?: string) and use the authority value
to return the correct method (e.g., return 'key' for 'active' authority and
'hivesigner' or 'posting' for posting authority) so
adapter.getLoginType(username, authority) yields authority-aware results; adjust
any internal logic or branching in getLoginType and ensure callers like
broadcastWithMethod continue to receive the correct login type for later checks.
---
Outside diff comments:
In `@packages/sdk/src/modules/core/mutations/use-broadcast-mutation.ts`:
- Line 208: The TOKEN_EXPIRED classification is too broad because it treats any
error containing the word "forbidden" as an auth expiration and causes
shouldTriggerAuthFallback to run; narrow the `/\bforbidden\b/i` pattern in
chain-errors.ts (the TOKEN_EXPIRED detection logic) to only match auth-scoped
messages such as HiveSigner descriptors (e.g., "invalid_grant",
"unauthorized_access") or instances where "forbidden" appears near
token/authorization words (e.g., "forbidden.*token|authorization|access token")
or check for a 403 HTTP status combined with auth keywords; update the
TOKEN_EXPIRED regex/conditions accordingly and add/adjust unit tests that
exercise forbidden-but-not-auth errors to ensure non-auth 403s are not
classified as TOKEN_EXPIRED.
---
Nitpick comments:
In `@packages/sdk/src/modules/core/types/platform-adapter.ts`:
- Line 121: The getLoginType signature currently accepts authority?: string;
tighten it to the same inline union used elsewhere ("posting" | "active" |
"owner" | "memo") to prevent arbitrary strings and keep the interface
self-documenting: change the getLoginType declaration (function name
getLoginType in platform-adapter.ts) to use authority?: "posting" | "active" |
"owner" | "memo"; and update any local callers/implementations to accept/return
the narrowed type accordingly so types remain consistent with
broadcastWithKeychain and broadcastWithHiveAuth.
feruzm
added
the
patch
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Chores