Skip to content

fix: improve Sign action flow and error reporting - #2609

Merged
svienot merged 5 commits into
mainfrom
2552_improve_signing_flow
Apr 9, 2026
Merged

fix: improve Sign action flow and error reporting#2609
svienot merged 5 commits into
mainfrom
2552_improve_signing_flow

Conversation

@ericleponner

@ericleponner ericleponner commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Description:

Changes below update SignTransactionController to improve error reporting.
They apply the same pattern as in SignAllController:

  • they now produce a fine grain report by calling collectRequiredKeys(), collectMissingKeys() and signItems() and building an ActionReport
  • they remove signTransactions() function which is now obsolete

Related issue(s):

Contributes to #2552

Notes for reviewer:

M   front-end/src/renderer/pages/TransactionDetails/SignTransactionController.vue
M   front-end/src/renderer/pages/Transactions/components/SignSingleButton.vue
    # Now calls collectRequiredKeys(), collectMissingKeys() and signItems()
    # in place of signTransactions(). Uses ActionReport to construct user feedback.
    # Removed (unused) signed boolean from props.callback().

M   front-end/src/renderer/pages/TransactionDetails/TransactionDetails.vue
    # Removed router.back() from fetchTransactions()

M   front-end/src/renderer/utils/index.ts
    # Removed now unused signTransactions()

M   front-end/src/tests/renderer/utils/index.spec.ts
    # Replaced unit tests for signTransactions() by tests 
    # for collectRequiredKeys(), collectMissingKeys() and signItems()

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@codecov

codecov Bot commented Apr 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.95%. Comparing base (c1e68d6) to head (2b33bbb).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2609   +/-   ##
=======================================
  Coverage   99.95%   99.95%           
=======================================
  Files         184      184           
  Lines        7446     7446           
  Branches     1489     1479   -10     
=======================================
  Hits         7443     7443           
  Misses          3        3           

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ericleponner
ericleponner force-pushed the 2552_improve_signing_flow branch from 66352d7 to 707d68c Compare April 7, 2026 18:18
@ericleponner
ericleponner requested a review from svienot April 7, 2026 18:44
@ericleponner
ericleponner force-pushed the 2552_improve_signing_flow branch from f72f924 to be5905d Compare April 8, 2026 14:29
@ericleponner
ericleponner requested a review from Copilot April 8, 2026 14:34
@ericleponner
ericleponner marked this pull request as ready for review April 8, 2026 14:37
@ericleponner
ericleponner requested a review from a team as a code owner April 8, 2026 14:37
@ericleponner ericleponner self-assigned this Apr 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the single-transaction signing flow to align with the existing “Sign All” pattern by producing more structured user-facing reports and by splitting signing into required-key collection, missing-key detection, and item signing.

Changes:

  • Update SignTransactionController to use collectRequiredKeys(), collectMissingKeys(), and signItems() and return ActionReport-based feedback.
  • Simplify SignSingleButton’s emitted payload (remove unused signed flag) and adjust transaction detail fetching behavior.
  • Remove/rework signTransactions() usage and replace its unit tests with coverage for the newer helper functions.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
front-end/src/renderer/pages/TransactionDetails/SignTransactionController.vue Refactors signing flow to use ActionReport + helper functions and changes callback signature.
front-end/src/renderer/pages/Transactions/components/SignSingleButton.vue Updates transactionSigned event payload and removes unused signed propagation.
front-end/src/renderer/pages/TransactionDetails/TransactionDetails.vue Stops navigating back on fetch errors (now rethrows).
front-end/src/renderer/utils/index.ts Adds early return for empty sign set, and removes/obsoletes signTransactions() (currently left as commented code).
front-end/src/tests/renderer/utils/index.spec.ts Replaces signTransactions() tests with tests for collectRequiredKeys(), collectMissingKeys(), and signItems().
Comments suppressed due to low confidence (2)

front-end/src/renderer/utils/index.ts:288

  • index.ts still contains the full old signTransactions() implementation commented out. Keeping large blocks of commented code makes maintenance harder and risks divergence from the real implementation. Since this function is now obsolete, it should be removed entirely (use git history for retrieval if needed).

export const splitMultipleAccounts = (input: string, client: Client): string[] => {
  input = input.trim();

  const result: string[] = [];
  if (!input) {
    return result;
  }

front-end/src/tests/renderer/utils/index.spec.ts:16

  • These tests still set up and mock vue-toast-notification (toastErrorSpy/useToast), but the updated test suite no longer asserts on toast behavior and doesn't use the toast mock. Removing unused mocks/fixtures will reduce noise and make failures easier to diagnose.
export const toastErrorSpy = vi.fn();
const toastMock = { error: toastErrorSpy };

vi.mock('vue-toast-notification', () => ({
  useToast: () => toastMock,
}));

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread front-end/src/renderer/pages/TransactionDetails/SignTransactionController.vue Outdated
Comment thread front-end/src/renderer/utils/index.ts
@ericleponner
ericleponner force-pushed the 2552_improve_signing_flow branch from 571dd9a to ca6c4ac Compare April 8, 2026 16:49
…tModal.

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>
… empty input.

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>
…signItems(). Removed signTransactions() and matching unit tests.

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>
Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>
Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>
@ericleponner
ericleponner force-pushed the 2552_improve_signing_flow branch from ca6c4ac to 2b33bbb Compare April 8, 2026 17:22

@svienot svienot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@svienot
svienot merged commit 7f5cc3a into main Apr 9, 2026
29 of 31 checks passed
@svienot
svienot deleted the 2552_improve_signing_flow branch April 9, 2026 12:33
Kolpic pushed a commit that referenced this pull request Apr 15, 2026
* Improved Sign action user feedback using ActionReport and ActionReportModal.

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>

* SignTransactionController now handles (buggy) case where it is passed empty input.

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>

* Added unit tests for collectRequiredKeys(), collectMissingKeys() and signItems(). Removed signTransactions() and matching unit tests.

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>

* Removed commented signTransactions()

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>

* Removed unneeded check from SignTransactionController.

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>

---------

Signed-off-by: Eric Le Ponner <eric.leponner@icloud.com>
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