Skip to content

fix: show and log gas estimation error#1083

Open
troykessler wants to merge 4 commits into
mainfrom
fix/log-gas-estimation-error
Open

fix: show and log gas estimation error#1083
troykessler wants to merge 4 commits into
mainfrom
fix/log-gas-estimation-error

Conversation

@troykessler
Copy link
Copy Markdown
Contributor

Before the gas estimation failed silently, now we log the error and show "Estimation failed" instead of "-" in the ui

@troykessler troykessler self-assigned this Apr 28, 2026
@troykessler troykessler requested a review from Xaroz as a code owner April 28, 2026 07:56
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 28, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperlane-warp-template Ready Ready Preview, Comment May 8, 2026 2:37pm
4 Skipped Deployments
Project Deployment Actions Updated (UTC)
injective-bridge Ignored Ignored May 8, 2026 2:37pm
nexus-bridge Ignored Ignored May 8, 2026 2:37pm
ousdt-bridge Ignored Ignored May 8, 2026 2:37pm
trump-bridge Ignored Ignored May 8, 2026 2:37pm

Request Review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

Warning

Rate limit exceeded

@troykessler has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 54 minutes and 48 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5ebec2b3-ad7a-46ff-8940-04a3995d7af9

📥 Commits

Reviewing files that changed from the base of the PR and between a667e95 and ba93e7a.

📒 Files selected for processing (3)
  • src/features/transfer/FeeSectionButton.tsx
  • src/features/transfer/TransferTokenForm.tsx
  • src/features/transfer/useFeeQuotes.ts
📝 Walkthrough

Walkthrough

This PR adds proper error handling to the fee estimation flow. The fee quote hook now captures and logs errors from the query, which propagate through the form component up to the button—allowing it to display an error message when estimation fails rather than leaving users guessing.

Changes

Cohort / File(s) Summary
Fee Quote Error Capture
src/features/transfer/useFeeQuotes.ts
Hook now destructures the error property from useQuery and adds a useEffect side-effect to log fee quote failures with explicit error details.
Error State Propagation
src/features/transfer/TransferTokenForm.tsx, src/features/transfer/FeeSectionButton.tsx
Form extends the fee quotes destructuring to capture the isError flag and threads it into the button via a new isError prop; button displays "Estimation failed" text when error occurs and loading has finished.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: show and log gas estimation error' directly and clearly summarizes the main changes: adding error logging and UI display for gas estimation failures.
Description check ✅ Passed The description accurately explains the changeset, noting that gas estimation errors were previously silent and now are logged with UI feedback showing 'Estimation failed'.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Xaroz
Copy link
Copy Markdown
Collaborator

Xaroz commented May 8, 2026

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 8, 2026

Claude finished @Xaroz's task in 2m 18s —— View job


Claude PR Review

  • Read changed files
  • Analyze diff (useFeeQuotes.ts, TransferTokenForm.tsx, FeeSectionButton.tsx)
  • Post review

Overall: Solid, focused fix. Surfacing a previously-silent failure both in logs and UI is a clear win. A few minor observations below.

Findings

1. Stale-fees + error inconsistency (minor UX)
src/features/transfer/FeeSectionButton.tsx:45-53

React Query v5 (the version pinned in package.json) preserves data across background refetch failures by default. So after a successful first fetch followed by a failed refetch:

  • isError is true, fees is non-null (stale).
  • feeText becomes "Estimation failed".
  • isClickable = hasFees && !isLoading is true → user can still click and the modal opens showing stale numbers.

The button's label and the modal's content disagree. Two reasonable options:

  • Treat error as a hard reset: include !isError in isClickable so the button is non-interactive when estimation has failed (matches the label).
  • Or, conversely, prefer stale fees over the error text (e.g. show fees.totalFees with a small warning tooltip), and only fall back to "Estimation failed" when there is truly no data.

I'd lean toward the first — consistent with "no current estimate available."

const isClickable = hasFees && !isLoading && !isError;

2. Nested ternary readability (nit)
src/features/transfer/FeeSectionButton.tsx:47-53

CLAUDE.md prefers flattened conditionals. The 3-deep ternary is fine but extracting helps:

const feeText = (() => {
  if (isLoading) return loadingText;
  if (isError) return 'Estimation failed';
  return hasFees ? fees.totalFees : '-';
})();

Not blocking — judgment call.

3. Logging duplicated on each refetch failure (intentional? worth confirming)
src/features/transfer/useFeeQuotes.ts:94-96

refetchInterval: 30_000 means while the user lingers on a broken route, logger.error('Fee quote failed', error) fires on every retry that fails (the error reference changes per attempt, re-running the effect). Probably fine for diagnostics, but if log volume matters this could spam. A useRef or comparing message before logging would dedupe — or it could be moved into a query onError style callback.

4. error is unknown — log shape
src/features/transfer/useFeeQuotes.ts:95

useQuery's error is typed as Error | null in v5, so logger.error('Fee quote failed', error) is fine. Minor: consider error?.message or stringifying to keep the logger payload consistent with fetchFeeQuotes's inner logger.warn(..., error) calls (which do the same thing — so this is already consistent — leaving as-is is fine).

5. No tests added
src/features/transfer/useFeeQuotes.test.ts covers happy paths but nothing exercises the new error/log branch or the isError → "Estimation failed" UI text. Optional, but a small Vitest case mocking a rejecting queryFn would lock in the new contract.

Summary

  • 🟡 Recommend tightening isClickable so the modal can't open while showing "Estimation failed".
  • 🟢 Logging + UI fallback are good additions; rest are nits.
    Branch

Comment thread src/features/transfer/FeeSectionButton.tsx Outdated
@troykessler troykessler enabled auto-merge (squash) May 8, 2026 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants