feat: Enhance transaction tracking with recovery options and estimated wait times#624
feat: Enhance transaction tracking with recovery options and estimated wait times#624jotelfootball-tech wants to merge 7 commits intoLabsCrypt:mainfrom
Conversation
ogazboiz
left a comment
There was a problem hiding this comment.
hey @jotelfootball-tech, CI is failing here. backend and frontend builds are both red.
also, the branch name has a typo (transactio-cycle instead of transaction-cycle). not a blocker but worth fixing for cleanliness.
fix the CI failures and push again!
|
The codebase issues on main have been resolved and all CI checks are passing now. Please rebase your branch to pull in the latest changes before continuing. Thanks for your patience. |
ogazboiz
left a comment
There was a problem hiding this comment.
PR Review: Enhance transaction tracking with recovery options and estimated wait times
CI Status
All 3 jobs pass (backend, frontend, contracts). Good to see those are green now.
Merge Status
No conflicts. Mergeable state is clean.
Migration Check
No migration files in this PR. No conflicts with the current highest migration number.
Issues Found
1. Stale closure bug in recovery timeout (2 locations)
In both repay/[loanId]/page.tsx and StepFinalSignature.tsx, there's a setTimeout that checks trackerState === "confirming" after 60 seconds:
setTimeout(() => {
if (trackerState === "confirming") {
setShowRecoveryLink(true);
}
}, 60000);This captures the value of trackerState at the time the timeout is set, not the value at the time it fires. Since trackerState is React state (a primitive), the closure captures whatever value it had when the effect ran. By the time 60 seconds pass, the real state could have moved to "confirmed" or "failed", but the closure still sees the old value. This means the recovery link may show up even after a successful transaction, or it may never show up at all depending on timing. Fix this by using a ref for the state check, or by using useEffect cleanup to clear the timeout when the state changes away from "confirming".
2. status.txt binary file committed
There's a status.txt binary file added to the repo root:
diff --git a/status.txt b/status.txt
new file mode 100644
Binary files /dev/null and b/status.txt differ
This looks accidental. Please remove it before merge.
3. Branch name typo
Branch is transactio-cycle (missing the "n" in "transaction"). Previously flagged, just noting it's still the case. Not a blocker but worth noting.
4. Duplicated code pattern across 2 files
getEstimatedWaitTime() and handleRecovery() are copy-pasted identically in repay/[loanId]/page.tsx and StepFinalSignature.tsx. Consider extracting these into a shared hook (e.g., useTransactionRecovery) to reduce duplication and keep behavior consistent if the logic changes later.
5. Hardcoded 5-second estimate in getEstimatedWaitTime
const remaining = Math.max(0, 5000 - elapsed); // Assume 5 second total waitThe startTime is set when the entire submission starts (before signing, before submitting), not when the confirming phase starts. So by the time the state reaches "confirming", the 5-second budget is likely already exhausted and users will always see "Should be confirmed soon." The timer should start from when the state enters "confirming", not from the beginning of the flow.
6. handleRecovery is a fake implementation
const handleRecovery = () => {
toast.info("Recovery initiated", "Attempting to recover stuck transaction...");
setTimeout(() => {
setTrackerState("confirmed");
setTrackerTitle("Recovery successful");
setTrackerMessage("Transaction recovered and confirmed.");
}, 2000);
};This unconditionally sets the state to "confirmed" after 2 seconds without actually recovering anything. If this is intentional placeholder code, it should at least have a // TODO comment. In production, this would silently lie to users about transaction status.
7. Massive package-lock.json churn
A large portion of the diff (the majority of 356 additions / 118 deletions) is just adding/removing "peer": true annotations in both backend/package-lock.json and frontend/package-lock.json. This looks like it was caused by a different npm version. Consider regenerating the lockfiles with the same npm version used in CI to keep the diff clean.
What Looks Good
- The rename of transaction states from
signing/submitting/polling/success/errortopending/submitted/confirming/confirmed/failedis a clear improvement. The new names better reflect the actual lifecycle. - Adding
TransactionPreviewModalintegration to the lend and repay pages is a nice UX improvement. - The enhanced polling in
useContractMutationwith abort controller support is well structured. - The
TransactionStatusTrackernow supports estimated wait times and recovery, which are useful features once the underlying logic is fixed.
Recommendation
Request changes. The stale closure bug (#1) will cause incorrect recovery link behavior. The committed status.txt binary (#2) should be removed. The hardcoded timer issue (#5) means estimated wait times won't work as intended. These should be addressed before merging.
ogazboiz
left a comment
There was a problem hiding this comment.
This PR overlaps heavily with #675 (same author, same feature). The stale closure bug, fake recovery handler, accidental status.txt, and frozen countdown timer are all present here too.
Same issues as flagged on #675. Please fix those and consolidate these two PRs into one if they're covering the same feature. Also the branch name has a typo: "transactio-cycle" is missing the "n".
|
heads up, a few important changes just landed on main that affect your PR:
please rebase on latest main: git fetch upstream
git rebase upstream/main
git push --force-with-lease |
Closes #566