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

v0.4.4 #571

Merged
merged 13 commits into from
Oct 24, 2024
Merged

v0.4.4 #571

merged 13 commits into from
Oct 24, 2024

Conversation

Kodylow
Copy link
Member

@Kodylow Kodylow commented Oct 24, 2024

Updates to use new v0.4.4 fedimint release and adds in bitcoin node status checking.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a method to check the Bitcoin RPC connection status, enhancing user feedback on synchronization.
    • Added conditional alerts in the UI to inform users about the Bitcoin node's sync status.
    • Enhanced the Bitcoin settings form to display synchronization progress messages.
    • Updated the UI to adjust content alignment based on setup progress and terms of service configurations.
    • Expanded localization support with new messages regarding Bitcoin synchronization in multiple languages.
  • Bug Fixes

    • Improved error handling for Bitcoin status fetching.
  • Documentation

    • Updated language files to provide clearer messages regarding Bitcoin node synchronization.
  • Chores

    • Updated dependencies to ensure compatibility with the latest changes.

@Kodylow Kodylow requested review from a team as code owners October 24, 2024 16:49
Copy link

vercel bot commented Oct 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 24, 2024 7:43pm

Copy link
Contributor

coderabbitai bot commented Oct 24, 2024

📝 Walkthrough

Walkthrough

The changes in this pull request introduce a new method checkBitcoinStatus in the GuardianApi class to check the Bitcoin RPC connection status. The RoleSelector and SetConfiguration components are updated to manage and display this status, enhancing user feedback regarding synchronization. The BitcoinSettingsForm component is modified to include a new property for the Bitcoin status, and the language files are updated with relevant messages. Additionally, the fedimint dependency is updated in flake.nix, and a new type BitcoinRpcConnectionStatus is added.

Changes

File Change Summary
apps/router/src/api/GuardianApi.ts Added method checkBitcoinStatus returning Promise<BitcoinRpcConnectionStatus>.
apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx Introduced useGuardianApi hook, added bitcoinStatus state, and modified rendering logic based on status.
apps/router/src/guardian-ui/components/setup/screens/setConfiguration/BitcoinSettingsForm.tsx Updated BitcoinSettingsFormProps to include bitcoinStatus and added conditional rendering for alerts.
apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx Added bitcoinStatus state and updated BitcoinSettingsForm to accept this prop.
apps/router/src/languages/en.json Added new entries for Bitcoin node synchronization status under "role-selector".
apps/router/src/types/guardian.tsx Added checkBitcoinStatus to SharedRpc enum.
flake.nix Updated fedimint input revision from 6f6bdb3... to e147b56....
packages/types/src/bitcoin.ts Added new type BitcoinRpcConnectionStatus which can be 'Synced' or a number.

Possibly related PRs

  • feat: new auth manager uncomplicated #558: The changes in the GuardianApi class, including the addition of a new method setSessionPassword, are related to the main PR's introduction of the checkBitcoinStatus method, as both involve modifications to the GuardianApi class.
  • 0.4.3 #564: The updates in the en.json file, particularly the addition of the "bitcoin-status-label" and related entries, are relevant as they enhance the user interface by providing context for the Bitcoin connection status, which is directly tied to the functionality introduced in the main PR.
  • chore: bump fedimint to post 0.4.3 #568: The update to the flake.nix file, which includes changes to the fedimint input reference, is indirectly related as it may affect the overall functionality and dependencies of the application, including those related to the Bitcoin status checks introduced in the main PR.

Suggested reviewers

  • dpc

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f8048ac and 4e8fb7a.

📒 Files selected for processing (4)
  • apps/router/src/guardian-ui/components/dashboard/tabs/info/FederationInfo.tsx (2 hunks)
  • apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1 hunks)
  • apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (3 hunks)
  • apps/router/src/guardian-ui/components/setup/screens/setConfiguration/BitcoinSettingsForm.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/router/src/guardian-ui/components/dashboard/tabs/info/FederationInfo.tsx
  • apps/router/src/guardian-ui/components/setup/screens/setConfiguration/BitcoinSettingsForm.tsx
🧰 Additional context used
📓 Learnings (1)
apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1)
Learnt from: Kodylow
PR: fedimint/ui#559
File: apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx:105-109
Timestamp: 2024-10-15T03:04:45.827Z
Learning: In `MetaManager.tsx`, the `proposeMetaEdits` function handles the case where `sites` is an empty string.
🔇 Additional comments (3)
apps/router/src/guardian-ui/components/dashboard/tabs/meta/manager/MetaManager.tsx (1)

120-120: Verify the impact of immediate tab switch after meta edits

Changing to tab 0 immediately after meta edits might hide important feedback or status information from the user. Please verify:

  1. Does tab 0 show the updated meta information?
  2. Should users see a confirmation or the edit results before switching tabs?
apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (2)

43-54: Previous review comment about cleanup and error handling is still valid

The implementation still needs improvements for component cleanup, error handling, and retry logic as detailed in the previous review.


114-148: Previous review comment about Bitcoin status display logic is still valid

The implementation still needs improvements for status display logic as detailed in the previous review.


🪧 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 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
Contributor

@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: 6

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3af06a8 and 90696c0.

⛔ Files ignored due to path filters (1)
  • flake.lock is excluded by !**/*.lock
📒 Files selected for processing (8)
  • apps/router/src/api/GuardianApi.ts (2 hunks)
  • apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (3 hunks)
  • apps/router/src/guardian-ui/components/setup/screens/setConfiguration/BitcoinSettingsForm.tsx (4 hunks)
  • apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (4 hunks)
  • apps/router/src/languages/en.json (1 hunks)
  • apps/router/src/types/guardian.tsx (1 hunks)
  • flake.nix (1 hunks)
  • packages/types/src/bitcoin.ts (1 hunks)
🔇 Additional comments (4)
flake.nix (2)

5-5: Check yarn.lock synchronization

Since we're updating the fedimint version, we need to ensure yarn.lock is synchronized with any dependency changes.


5-5: Verify fedimint version compatibility

The update to fedimint revision e147b5695abce6eeeb984974d48d562b2bf1dfe1 aligns with the v0.4.4 release objectives. Let's verify this is the correct version.

✅ Verification successful

Version update to v0.4.4-rc.0 is correct

The commit e147b5695abce6eeeb984974d48d562b2bf1dfe1 is tagged as v0.4.4-rc.0 and contains version bump changes across the codebase from 0.4.3-rc.2 to 0.4.4-rc.0, aligning with the PR's objectives.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the fedimint version tag matches v0.4.4
# Test: Check if the commit corresponds to the v0.4.4 tag
gh api repos/fedimint/fedimint/commits/e147b5695abce6eeeb984974d48d562b2bf1dfe1/tags

Length of output: 456


Script:

#!/bin/bash
# Let's try a different approach to verify the fedimint version
# 1. Check if this commit exists in fedimint repo
gh api repos/fedimint/fedimint/commits/e147b5695abce6eeeb984974d48d562b2bf1dfe1

# 2. Check recent tags around this timeframe
gh api repos/fedimint/fedimint/tags --jq '.[0:5]'

Length of output: 81399

apps/router/src/types/guardian.tsx (1)

176-176: LGTM!

The addition of checkBitcoinStatus to SharedRpc is well-structured and consistent with the existing RPC method naming patterns.

apps/router/src/languages/en.json (1)

249-252: LGTM!

The new Bitcoin node status messages are clear, informative, and correctly structured.

Copy link
Contributor

@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: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2326c71 and cfcff20.

📒 Files selected for processing (2)
  • apps/router/src/guardian-ui/components/setup/screens/roleSelector/RoleSelector.tsx (3 hunks)
  • apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx

Copy link
Contributor

@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: 4

🧹 Outside diff range and nitpick comments (1)
apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (1)

Line range hint 292-299: Add loading state handling for ConfirmPasswordModal.

The modal's visibility condition bitcoinStatus === 'Synced' could lead to a flash of content when the status changes from undefined to 'Synced'. Consider adding a loading state.

-      {password !== null && bitcoinStatus === 'Synced' && (
+      {password !== null && (bitcoinStatus === undefined ? (
+        <LoadingSpinner />
+      ) : bitcoinStatus === 'Synced' && (
        <ConfirmPasswordModal
          password={password}
          submitConfig={submitConfig}
          isOpen={isOpen}
          onClose={onClose}
          guardianName={myName}
        />
+      ))}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between cfcff20 and f8048ac.

📒 Files selected for processing (16)
  • apps/router/src/guardian-ui/components/dashboard/tabs/info/FederationInfo.tsx (4 hunks)
  • apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (6 hunks)
  • apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx (2 hunks)
  • apps/router/src/guardian-ui/setup/FederationSetup.tsx (1 hunks)
  • apps/router/src/languages/ca.json (2 hunks)
  • apps/router/src/languages/de.json (2 hunks)
  • apps/router/src/languages/en.json (2 hunks)
  • apps/router/src/languages/es.json (2 hunks)
  • apps/router/src/languages/fr.json (2 hunks)
  • apps/router/src/languages/hu.json (2 hunks)
  • apps/router/src/languages/it.json (2 hunks)
  • apps/router/src/languages/ja.json (2 hunks)
  • apps/router/src/languages/ko.json (2 hunks)
  • apps/router/src/languages/pt.json (2 hunks)
  • apps/router/src/languages/ru.json (2 hunks)
  • apps/router/src/languages/zh.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/router/src/guardian-ui/setup/FederationSetup.tsx
🔇 Additional comments (16)
apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx (2)

71-72: LGTM: State initialization is correct.

The bitcoinStatus state is properly typed with BitcoinRpcConnectionStatus.


121-132: Fix potential race conditions and memory leaks in useEffect.

The existing implementation of the Bitcoin status fetch is susceptible to memory leaks and race conditions.

apps/router/src/languages/zh.json (1)

69-70: LGTM!

The new Chinese translations for Bitcoin synchronization status are accurate and maintain consistency with the existing terminology.

Also applies to: 223-226

apps/router/src/languages/ko.json (2)

69-70: LGTM!

The translations for Bitcoin block height and sync status labels are accurate and consistent.


223-226: LGTM!

The translations for Bitcoin node synchronization status messages are accurate and maintain the correct placeholder format.

apps/router/src/languages/ja.json (1)

69-70: LGTM: Japanese translations for Bitcoin node status.

The translations are accurate, maintain consistency with existing translations, and properly handle dynamic content.

Also applies to: 223-226

apps/router/src/languages/en.json (1)

69-70: LGTM! Clear and consistent language entries for Bitcoin node status.

The new language entries effectively communicate the Bitcoin node synchronization status to users, following the existing patterns and conventions.

Also applies to: 250-253

apps/router/src/languages/pt.json (1)

69-70: LGTM!

The Portuguese translations for Bitcoin synchronization status are accurate and maintain proper grammar and syntax.

Also applies to: 223-226

apps/router/src/languages/hu.json (1)

69-70: LGTM: Translation keys added for Bitcoin node status.

The new Hungarian translations for Bitcoin consensus block height and node synchronization status are properly formatted and align with the PR objectives.

Also applies to: 223-226

apps/router/src/languages/ru.json (1)

69-70: LGTM!

The new Russian translations for Bitcoin status and node synchronization messages are grammatically correct and consistent with the existing translation style.

Also applies to: 223-226

apps/router/src/languages/es.json (1)

69-70: LGTM!

The Spanish translations for Bitcoin synchronization status are grammatically correct and maintain consistency with existing terminology.

Also applies to: 223-226

apps/router/src/languages/it.json (1)

69-70: LGTM!

The translations for Bitcoin consensus block height and synchronization status are properly added.

apps/router/src/languages/fr.json (1)

69-70: LGTM: French translations for Bitcoin status messages are accurate and well-structured.

The new translations maintain proper French grammar while accurately conveying technical concepts about Bitcoin node synchronization status.

Also applies to: 223-226

apps/router/src/languages/ca.json (1)

69-70: LGTM! The translations are complete and consistent.

The added translations properly support the new Bitcoin node status functionality, including the synchronization status label and appropriate messages for unsynchronized nodes.

Also applies to: 224-226

apps/router/src/languages/de.json (2)

69-70: LGTM!

The German translations for the Bitcoin status labels are grammatically correct and consistent with the application's terminology.


223-226: LGTM!

The German translations for the Bitcoin node synchronization messages are accurate and properly handle the progress placeholder.

Kodylow and others added 4 commits October 24, 2024 12:31
…uration/BitcoinSettingsForm.tsx

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tor/RoleSelector.tsx

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tor/RoleSelector.tsx

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…erationInfo.tsx

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@Kodylow Kodylow merged commit 1ee9f74 into fedimint:master Oct 24, 2024
3 checks passed
This was referenced Nov 9, 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.

1 participant