Skip to content

fix(pve): Fix connection issues and add more error handlings#1081

Merged
GT-610 merged 3 commits intolollipopkit:mainfrom
GT-610:pve-fix
Mar 22, 2026
Merged

fix(pve): Fix connection issues and add more error handlings#1081
GT-610 merged 3 commits intolollipopkit:mainfrom
GT-610:pve-fix

Conversation

@GT-610
Copy link
Copy Markdown
Collaborator

@GT-610 GT-610 commented Mar 22, 2026

Resolve #644.

Summary by CodeRabbit

  • New Features

    • Two-factor (OTP) support for PVE with localized prompts (en/zh/zh-TW)
    • Step-wise loading indicators for forwarding, authenticating, and fetching data
    • Optional PVE password field in server settings and edit UI
    • Refresh now triggers a reconnect flow
  • Bug Fixes

    • Improved authentication/error handling and clearer user error displays
    • More robust connection/forwarding with better recovery and logging

GT-610 added 2 commits March 22, 2026 12:15
Added a detailed display of loading steps during the PVE connection process, including stages such as establishing an SSH tunnel, authentication, and data retrieval

Also optimized the sorting of PVE storage content and the logic for handling connection errors
…tication

Added error handling for PVE servers when two-factor authentication is enabled, along with relevant error types and localized prompts
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5e868dd6-5ac2-4412-b567-f349ac1e8060

📥 Commits

Reviewing files that changed from the base of the PR and between 7d7156a and bfbef19.

⛔ Files ignored due to path filters (15)
  • lib/generated/l10n/l10n.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_de.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_en.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_es.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_fr.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_id.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_it.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_ja.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_ko.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_nl.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_pt.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_ru.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_tr.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_uk.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_zh.dart is excluded by !**/generated/**
📒 Files selected for processing (13)
  • lib/data/model/server/custom.dart
  • lib/data/model/server/custom.g.dart
  • lib/data/provider/pve.dart
  • lib/data/provider/pve.g.dart
  • lib/hive/hive_adapters.g.dart
  • lib/hive/hive_adapters.g.yaml
  • lib/l10n/app_en.arb
  • lib/l10n/app_zh.arb
  • lib/l10n/app_zh_tw.arb
  • lib/view/page/pve.dart
  • lib/view/page/server/edit/actions.dart
  • lib/view/page/server/edit/edit.dart
  • lib/view/page/server/edit/widget.dart
✅ Files skipped from review due to trivial changes (2)
  • lib/data/model/server/custom.g.dart
  • lib/data/provider/pve.g.dart
🚧 Files skipped from review as they are similar to previous changes (3)
  • lib/l10n/app_en.arb
  • lib/l10n/app_zh_tw.arb
  • lib/l10n/app_zh.arb

📝 Walkthrough

Walkthrough

This PR adds a new PveErrType.needTfa enum value, detects and surfaces PVE two-factor authentication (TFA) during login, and introduces PveLoadingStep plus a loadingStep field in PveState with UI/localization for forwarding/login/data-loading states and an OTP prompt. PveNotifier was refactored (nullable session/socket fields, computed client accessors, reconnect(), guarded async state updates, per-socket forward error handling, stricter login flow including throwing needTfa). Storage/content normalization, addition of ServerCustom.pvePwd (and related JSON/Hive wiring), provider hash regenerations, and edit-page UI for PVE password were also added.

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main objective: fixing PVE connection issues and adding error handling for two-factor authentication and password-based authentication.
Linked Issues check ✅ Passed The PR successfully addresses issue #644 by implementing PVE password support for key-based authentication, including UI inputs, state management, and connection logic updates.
Out of Scope Changes check ✅ Passed All changes are focused on resolving the linked issue: PVE password handling, two-factor authentication support, connection robustness improvements, and related localization strings are all within scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
lib/data/provider/pve.dart (1)

11-11: ⚠️ Potential issue | 🟡 Minor

Clear the remaining flutter analysis warnings in this file.

Line 11's locale import is unused now, and Lines 136-138 assign newUrl without ever reading it. Both warnings are already reported by the current analysis job.

Also applies to: 136-138

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/data/provider/pve.dart` at line 11, Remove the unused locale import and
eliminate the dead assignment to newUrl: delete the import of
'package:server_box/core/extension/context/locale.dart' near the top of
lib/data/provider/pve.dart, and in the block where newUrl is assigned
(previously lines ~136-138) either remove the newUrl variable/assignment
entirely or refactor to actually use newUrl in the subsequent logic; ensure no
other symbols in pve.dart reference that import or variable and re-run flutter
analyze to confirm warnings are cleared.
lib/view/page/pve.dart (1)

72-79: ⚠️ Potential issue | 🟠 Major

Retry the PVE bootstrap here, not just list().

Line 77 only re-runs list(), but list() exits until isConnected is true. That makes the refresh button a no-op for forwarding/login/bootstrap failures and leaves the page stuck on the error state until it is reopened.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/view/page/pve.dart` around lines 72 - 79, The retry button currently only
calls _notifier.list(), which is ineffective when the PVE
bootstrap/login/connection hasn't completed; change the handler in the Btn.icon
onTap to invoke the PVE bootstrap/reconnect routine (the method that initiates
connection/login/bootstrap on the notifier — e.g., _notifier.bootstrap() or
whatever method triggers the full bootstrap flow) instead of just
_notifier.list(), and still call _initRefreshTimer() afterwards so the refresh
timer is reinitialized; reference the Btn.icon onTap, _notifier.list(),
_initRefreshTimer(), and the notifier's bootstrap/connect method when making the
change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/data/provider/pve.dart`:
- Around line 59-65: The provider's build method (PveState build(Spi spiParam))
wrongly uses ref.read(serverProvider(spiParam.id)) and immediately returns a
PveErr when serverState.client == null, preventing rebuild when genClient()
later sets client; change ref.read(...) to
ref.watch(serverProvider(spiParam.id)) so the provider will rebuild when
ServerState.client becomes available and replace the immediate error return with
a non-error/loading PveState (or defer error only for real failures) so a
transient null SSH client doesn't become a permanent PVE error.

In `@lib/view/page/pve.dart`:
- Around line 142-148: Replace the hardcoded English strings in
_buildLoading(PveLoadingStep step) (the message variable for
PveLoadingStep.forwarding, PveLoadingStep.loggingIn,
PveLoadingStep.fetchingData, and the default) with localized strings from the
app l10n (e.g., use context.l10n or AppLocalizations.of(context).<key>); if the
keys do not yet exist, add appropriate keys to the ARB/localization resource
files (forwarding, loggingIn, fetchingData, connecting) and regenerate
localization, then reference those keys in _buildLoading so the UI uses
translated messages instead of hardcoded text.

---

Outside diff comments:
In `@lib/data/provider/pve.dart`:
- Line 11: Remove the unused locale import and eliminate the dead assignment to
newUrl: delete the import of
'package:server_box/core/extension/context/locale.dart' near the top of
lib/data/provider/pve.dart, and in the block where newUrl is assigned
(previously lines ~136-138) either remove the newUrl variable/assignment
entirely or refactor to actually use newUrl in the subsequent logic; ensure no
other symbols in pve.dart reference that import or variable and re-run flutter
analyze to confirm warnings are cleared.

In `@lib/view/page/pve.dart`:
- Around line 72-79: The retry button currently only calls _notifier.list(),
which is ineffective when the PVE bootstrap/login/connection hasn't completed;
change the handler in the Btn.icon onTap to invoke the PVE bootstrap/reconnect
routine (the method that initiates connection/login/bootstrap on the notifier —
e.g., _notifier.bootstrap() or whatever method triggers the full bootstrap flow)
instead of just _notifier.list(), and still call _initRefreshTimer() afterwards
so the refresh timer is reinitialized; reference the Btn.icon onTap,
_notifier.list(), _initRefreshTimer(), and the notifier's bootstrap/connect
method when making the change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0531208e-614d-4cd7-8d43-f3cefe22a132

📥 Commits

Reviewing files that changed from the base of the PR and between 2f67938 and 7d7156a.

⛔ Files ignored due to path filters (16)
  • lib/generated/l10n/l10n.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_de.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_en.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_es.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_fr.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_id.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_it.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_ja.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_ko.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_nl.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_pt.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_ru.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_tr.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_uk.dart is excluded by !**/generated/**
  • lib/generated/l10n/l10n_zh.dart is excluded by !**/generated/**
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • lib/data/model/app/error.dart
  • lib/data/model/server/pve.dart
  • lib/data/provider/container.g.dart
  • lib/data/provider/pve.dart
  • lib/data/provider/pve.freezed.dart
  • lib/data/provider/pve.g.dart
  • lib/data/provider/server/all.g.dart
  • lib/data/provider/server/single.g.dart
  • lib/data/provider/systemd.g.dart
  • lib/data/res/github_id.dart
  • lib/l10n/app_en.arb
  • lib/l10n/app_zh.arb
  • lib/l10n/app_zh_tw.arb
  • lib/view/page/pve.dart

…ation

- Added the `pvePwd` field to the `ServerCustom` model
- Added a PVE password input field to the edit page (displayed only during key-based authentication)
- Updated multilingual files to support PVE-related loading states and password prompts
- Optimized PVE connection logic to support password verification during key-based authentication
@GT-610 GT-610 merged commit 09431a0 into lollipopkit:main Mar 22, 2026
1 check passed
@GT-610 GT-610 deleted the pve-fix branch March 22, 2026 08:26
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.

No way to set password for PVE when key auth used

1 participant