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

fix: teleport warning #11429

Merged
merged 2 commits into from
Feb 21, 2025
Merged

fix: teleport warning #11429

merged 2 commits into from
Feb 21, 2025

Conversation

Jarsen136
Copy link
Contributor

@Jarsen136 Jarsen136 commented Feb 19, 2025

Thank you for your contribution to the Koda - Generative Art Marketplace.

👇 __ Let's make a quick check before the contribution.

PR Type

  • Bugfix

Needs QA check

  • @kodadot/qa-guild please review

Context

Screenshot 📸

  • My fix has changed something on UI; a screenshot is best to understand changes for others.

image

Summary by CodeRabbit

  • New Features

    • Introduced a simplified warning modal that now only allows users to close the modal without any continuation options.
  • Bug Fixes

    • Enhanced clarity of user-facing warning messages related to failed transactions and existential deposits.
  • Documentation

    • Updated localization files to reflect changes in warning texts, removing references to irreversible loss of funds.

@Jarsen136 Jarsen136 requested a review from a team as a code owner February 19, 2025 12:42
@Jarsen136 Jarsen136 requested review from preschian and vikiival and removed request for a team February 19, 2025 12:42
Copy link

coderabbitai bot commented Feb 19, 2025

Walkthrough

The updates remove the checkbox functionality and its associated logic from the EDWarningModal component. The modal now only emits a single "close" event instead of both "close" and "continue." In the Teleport component, the modal is updated to reflect this change by removing the @continue event handler. Additionally, the localization files have been modified: the English text was simplified by introducing a new key for failed transactions and removing the previous warning about loss of funds, while the Hindi translations for similar warnings were also removed.

Changes

File(s) Change Summary
components/teleport/EDWarningModal.vue
components/teleport/Teleport.vue
Removed the checkbox (and its binding and related logic), eliminated the "continue" event, and adjusted layouts and handlers so the modal only emits a "close" event.
i18n/locales/en.json Added key for failed transactions, removed warning about loss of funds, and simplified existential deposit warning text.
i18n/locales/hi.json Removed lines related to checkbox label, loss of funds warning, and existential deposit warnings.
components/teleport/FundsAtRiskWarning.vue Changed displayed message from loss of funds to failed transaction in the template.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant Modal as EDWarningModal
    participant Teleport as Teleport.vue

    U->>Modal: Opens Warning Modal
    Modal->>U: Displays warning message (only close option)
    U->>Modal: Clicks close button
    Modal->>Teleport: Emits "close" event
    Teleport->>Teleport: Updates modal state (insufficientEDModalOpen = false)
Loading

Poem

Hop along, dear coder, in the moonlit night,
I’m a rabbit with changes, full of delight.
Checkbox skipped, and continue is no more,
Only one close to guide you through the door.
With each hop and keystroke, new paths we explore!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7926237 and e67812c.

📒 Files selected for processing (4)
  • components/teleport/EDWarningModal.vue (3 hunks)
  • components/teleport/FundsAtRiskWarning.vue (1 hunks)
  • i18n/locales/en.json (1 hunks)
  • i18n/locales/hi.json (0 hunks)
💤 Files with no reviewable changes (1)
  • i18n/locales/hi.json
⏰ Context from checks skipped due to timeout of 90000ms (18)
  • GitHub Check: Redirect rules
  • GitHub Check: Header rules
  • GitHub Check: Pages changed
  • GitHub Check: Redirect rules
  • GitHub Check: Header rules
  • GitHub Check: Pages changed
  • GitHub Check: shard (10, 10)
  • GitHub Check: shard (9, 10)
  • GitHub Check: shard (8, 10)
  • GitHub Check: shard (7, 10)
  • GitHub Check: shard (6, 10)
  • GitHub Check: shard (5, 10)
  • GitHub Check: shard (4, 10)
  • GitHub Check: shard (3, 10)
  • GitHub Check: build
  • GitHub Check: shard (2, 10)
  • GitHub Check: shard (1, 10)
  • GitHub Check: build-cf
🔇 Additional comments (3)
components/teleport/FundsAtRiskWarning.vue (1)

28-28: LGTM! Warning message updated to reflect current XCM behavior.

The warning message has been appropriately updated to focus on failed transactions and transaction fees instead of loss of funds, which accurately reflects the current XCM protocol behavior.

components/teleport/EDWarningModal.vue (1)

31-31: LGTM! Modal simplified and warning message updated.

The changes improve the user experience by:

  1. Updating the warning message to reflect current XCM behavior
  2. Simplifying the interaction by removing the checkbox requirement
  3. Using a single close action instead of continue/close options

Also applies to: 34-40, 58-58

i18n/locales/en.json (1)

1934-1934: LGTM! Localization strings updated to reflect current XCM behavior.

The warning messages have been appropriately updated to:

  1. Focus on failed transactions and transaction fees instead of loss of funds
  2. Remove references to irreversible loss
  3. Maintain clarity about existential deposit requirements

Also applies to: 1948-1948, 1950-1950


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

netlify bot commented Feb 19, 2025

Deploy Preview for koda-canary ready!

Name Link
🔨 Latest commit e67812c
🔍 Latest deploy log https://app.netlify.com/sites/koda-canary/deploys/67b5d8fe32718e0008a2fcbe
😎 Deploy Preview https://deploy-preview-11429--koda-canary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 19, 2025

Deploy Preview for polkadot ready!

Name Link
🔨 Latest commit e67812c
🔍 Latest deploy log https://app.netlify.com/sites/polkadot/deploys/67b5d8fe73de1d00081cc2f9
😎 Deploy Preview https://deploy-preview-11429--polkadot.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
components/teleport/Teleport.vue (1)

135-142: Remove commented-out code.

The codebase contains commented-out code for the "Max" button functionality. Clean code practices suggest removing commented-out code as it can be retrieved from version control if needed.

Apply this diff to remove the commented-out code:

-        <!-- <NeoButton
-          no-shadow
-          rounded
-          size="small"
-          class="ml-2"
-          @click="handleMaxClick"
-          >{{ $t('teleport.max') }}</NeoButton
-        > -->

-// const handleMaxClick = () => {
-//   amount.value = myBalance.value
-// }

Also applies to: 480-482

components/teleport/EDWarningModal.vue (1)

51-57: Consider adding documentation and improving type safety.

Good to have improvements:

  1. Add JSDoc comments for props and emits to improve maintainability
  2. Define a proper type for the Currency type instead of using the implicit type

Example implementation:

/**
 * Props for the EDWarningModal component
 * @property {boolean} modelValue - Controls the visibility of the modal
 * @property {number|string} sourceExistentialDeposit - Minimum balance required for the source chain
 * @property {number|string} targetExistentialDeposit - Minimum balance required for the target chain
 * @property {Currency} currency - The currency symbol/code
 * @property {'source'|'target'} reason - Indicates whether the warning is for source or target chain
 */
type Currency = string // Replace with proper type if available

const props = defineProps<{
  modelValue: boolean
  sourceExistentialDeposit: number | string
  targetExistentialDeposit: number | string
  currency: Currency
  reason: 'source' | 'target'
}>()

/**
 * Events emitted by the EDWarningModal component
 * @event {void} close - Emitted when the modal should be closed
 */
const emit = defineEmits(['close'])

Also applies to: 61-61

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 207ccf0 and 7926237.

📒 Files selected for processing (4)
  • components/teleport/EDWarningModal.vue (3 hunks)
  • components/teleport/Teleport.vue (1 hunks)
  • i18n/locales/en.json (1 hunks)
  • i18n/locales/hi.json (0 hunks)
💤 Files with no reviewable changes (1)
  • i18n/locales/hi.json
✅ Files skipped from review due to trivial changes (1)
  • i18n/locales/en.json
⏰ Context from checks skipped due to timeout of 90000ms (19)
  • GitHub Check: Redirect rules
  • GitHub Check: Header rules
  • GitHub Check: Pages changed
  • GitHub Check: Redirect rules
  • GitHub Check: Header rules
  • GitHub Check: Pages changed
  • GitHub Check: shard (10, 10)
  • GitHub Check: shard (9, 10)
  • GitHub Check: shard (8, 10)
  • GitHub Check: shard (7, 10)
  • GitHub Check: shard (6, 10)
  • GitHub Check: shard (5, 10)
  • GitHub Check: shard (4, 10)
  • GitHub Check: shard (3, 10)
  • GitHub Check: build
  • GitHub Check: shard (2, 10)
  • GitHub Check: shard (1, 10)
  • GitHub Check: build-cf
  • GitHub Check: runner / eslint
🔇 Additional comments (3)
components/teleport/Teleport.vue (1)

175-182: LGTM! Modal event handling has been simplified.

The changes to TeleportEDWarningModal correctly simplify the warning flow by removing the redundant @continue event and relying solely on the @close event. This aligns with the PR's objective to fix the teleport warning.

components/teleport/EDWarningModal.vue (2)

34-34: LGTM! UI improvements enhance clarity.

The changes simplify the warning modal by:

  • Converting the checkbox label to static text
  • Centering the close button for better visual balance

Also applies to: 37-37


49-49: LGTM! Code cleanup improves maintainability.

The changes:

  • Remove unused imports
  • Simplify event emissions to match the new functionality

Also applies to: 61-61

@vikiival
Copy link
Member

I would maybe update the warning to be more up-to-date - XCM now will not result in loss of assets, but will not let the user transfer if the balance is less than existential deposit after XCM.

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Incorrect impl - #11388 (comment)

@Jarsen136
Copy link
Contributor Author

I would maybe update the warning to be more up-to-date - XCM now will not result in loss of assets, but will not let the user transfer if the balance is less than existential deposit after XCM.

Incorrect impl - #11388 (comment)

I remove the checkbox and continue button to avoid the transaction execute anyway.
The warning message has been changed a bit. Let me know if it makes sense. @vikiival
image

@vikiival
Copy link
Member

@exezbcz

@exezbcz
Copy link
Member

exezbcz commented Feb 19, 2025

Looks good! There was a warning before no?

Works for me

@Jarsen136
Copy link
Contributor Author

Looks good! There was a warning before no?

Here is the warning before
image

@vikiival vikiival added this pull request to the merge queue Feb 21, 2025
Merged via the queue into kodadot:main with commit e596b91 Feb 21, 2025
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teleport page not working
4 participants