Skip to content

feat: Enhance transaction tracking with recovery options and estimated wait times#624

Open
jotelfootball-tech wants to merge 7 commits intoLabsCrypt:mainfrom
jotelfootball-tech:transactio-cycle
Open

feat: Enhance transaction tracking with recovery options and estimated wait times#624
jotelfootball-tech wants to merge 7 commits intoLabsCrypt:mainfrom
jotelfootball-tech:transactio-cycle

Conversation

@jotelfootball-tech
Copy link
Copy Markdown

Closes #566

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@ogazboiz
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 wait

The 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/error to pending/submitted/confirming/confirmed/failed is a clear improvement. The new names better reflect the actual lifecycle.
  • Adding TransactionPreviewModal integration to the lend and repay pages is a nice UX improvement.
  • The enhanced polling in useContractMutation with abort controller support is well structured.
  • The TransactionStatusTracker now 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.

Copy link
Copy Markdown
Contributor

@ogazboiz ogazboiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

@ogazboiz
Copy link
Copy Markdown
Contributor

heads up, a few important changes just landed on main that affect your PR:

  1. axios pinned to 1.13.5 - there's an active supply chain attack on axios 1.14.1 (pulls in confirmed malware). we added overrides in all package.json files to block it.

  2. CI now runs a supply chain audit before backend/frontend jobs. if your lockfile has a compromised package, CI will fail with a clear error.

  3. backend test fixes - loanEndpoints tests now use valid Stellar addresses and base64 strings. if your PR was failing backend CI but you didn't touch backend code, this should fix it after rebase.

please rebase on latest main:

git fetch upstream
git rebase upstream/main
git push --force-with-lease

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.

feat(frontend): implement full transaction lifecycle states (submitted → confirming → confirmed)

2 participants