Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: export error log and show error modals on transaction errors #3408

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

alex-k8
Copy link
Collaborator

@alex-k8 alex-k8 commented Nov 20, 2024

closes: #3039

  • Reusable export method for downloading files
  • Changed wording in Error log Settings screen
  • New modal to confirm disabling the logs
  • Improved error logging on Transfers + Signing
  • Error modal with Export button in the above cases
  • Small UI fixes on buttons and inputs

Copy link

Copy link

@alex-k8 alex-k8 force-pushed the feat/error-logs-export branch from 1a66f2e to 4670e10 Compare November 26, 2024 08:41
Copy link

src/popup/components/InputField.vue Show resolved Hide resolved
</div>

<h2 class="text-heading-4 text-center">
{{ $t('pages.errors-log-settings.disableDialog.title') }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bart, would've suggest to put it into v-text. Same in other places in this file.

<style lang="scss" scoped>
@use '@/styles/variables' as *;
@use '@/styles/typography';
@use '@/styles/mixins';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@use '@/styles/mixins';

This is not used in the file.

@@ -37,6 +37,14 @@

<template #footer>
<slot name="footer">
<BtnMain
v-if="icon === 'critical' && saveErrorLogEnabled"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Decision to make it part of existing Default modal, had a bad side effect. For example now we would get a Export error log button for connection statuses. I think modal name Default should not have such side effects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the existing, unused, ErrorLog modal with one that uses the Default modal and replaces the footer with the appropriate buttons.

src/lib/logger.ts Show resolved Hide resolved
@@ -91,7 +90,9 @@ export default defineComponent({
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// eslint-disable-next-line @typescript-eslint/no-unused-vars

@@ -493,7 +494,7 @@ export default defineComponent({
recipient,
selectedAsset,
});
router.push({ name: homeRouteName.value, query: { latestTxHash: hash } });
if (hash) router.push({ name: homeRouteName.value, query: { latestTxHash: hash } });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (hash) router.push({ name: homeRouteName.value, query: { latestTxHash: hash } });
if (hash) {
router.push({ name: homeRouteName.value, query: { latestTxHash: hash } });
}

src/popup/components/buttons/BtnSubheader.vue Show resolved Hide resolved
@@ -210,7 +210,8 @@ import { Encoded } from '@aeternity/aepp-sdk';

import type { ICreateMultisigAccount, ObjectValues } from '@/types';
import { MODAL_ADDRESS_BOOK_ACCOUNT_SELECTOR, PROTOCOLS } from '@/constants';
import { excludeFalsy, handleUnknownError } from '@/utils';
import { tg } from '@/popup/plugins/i18n';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to export tg in this file? Every other place is using t from i18n module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replaced with t

@@ -99,10 +113,6 @@ export default class Logger {
time: Date.now(),
};
WalletStorage.set(STORAGE_KEYS.errorLog, [...errorLog, logEntry]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idea was also to collect data only for last month/week (better consult it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed to keep the last 1000 entries. Updated the code and wording of the Error Log Settings page.

@alex-k8 alex-k8 force-pushed the feat/error-logs-export branch from 4670e10 to 3c602d3 Compare November 28, 2024 15:06
Copy link

/>
</IconBoxed>
</div>
<h2 class="text-heading-4 text-center" v-text="$t('pages.errors-log-settings.disableDialog.title')" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have an unspoken rule to have each attribute at a new line in the template.

@alex-k8 alex-k8 force-pushed the feat/error-logs-export branch from 3c602d3 to 7a8d224 Compare December 2, 2024 08:52
Copy link

github-actions bot commented Dec 2, 2024

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.

Improve logging of sent signed transactions, and allow exporting logs as JSON for support
3 participants