[WIP!] Fix WebSocket email auto-refresh and add auto-sync toggle button #4253
Conversation
WalkthroughAdds auto-sync support end-to-end and refines WebSocket and refresh behavior: new AutoSyncConfig model and persistence in PreferencesSettingManager; SetupAutoSyncExtension on MailboxDashBoardController with load/toggle APIs; UI toggle in MailboxDashBoardView wired to WebSocketController; WebSocketController gains WebSocketConnectionState, isConnected, reconnect/disconnect/markNotSupported controls; WebSocket datasource adds ticket and token query strategies; mailbox and thread refresh guards relaxed to allow initial loads; push handling dispatches foreground email sync action; localization keys for auto-sync added; macOS/Windows desktop build and runner files added. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/features/mailbox_dashboard/presentation/mailbox_dashboard_view_web.dart`:
- Around line 440-459: Replace the hardcoded tooltip strings with localized
values: use AppLocalizations.of(context) (e.g.,
AppLocalizations.of(context)!.autoSyncEnabled and .autoSyncDisabled) for the
messages used in the Tooltip around TMailButtonWidget (key:
'auto_sync_toggle_button') that depends on controller.isAutoSyncEnabled; also
add the corresponding keys ("autoSyncEnabled" and "autoSyncDisabled") and
translations to the ARB/localization files so the tooltips are translated across
locales.
🧹 Nitpick comments (3)
lib/features/push_notification/presentation/controller/web_socket_controller.dart (1)
163-176: Verify thatdisconnect()fully prevents reconnection attempts.The
disconnect()method cleans up resources but does not reset_retryRemainedto 0. If the WebSocket listener'sonDonecallback fires during cleanup,_handleWebSocketConnectionRetry()could attempt to reconnect if_retryRemained > 0.Consider setting
_retryRemained = 0indisconnect()to ensure no reconnection attempts occur after an explicit disconnect:♻️ Suggested enhancement
/// Disconnect WebSocket connection. /// Used when disabling auto-sync. void disconnect() { log('WebSocketController::disconnect:'); + _retryRemained = 0; _cleanUpWebSocketResources(); }lib/features/mailbox_dashboard/presentation/extensions/auto_sync/setup_auto_sync_extension.dart (2)
13-26: Try/catch may be overly broad; consider narrowing scope.Per project learnings,
getBinding<T>()returns null when not found (doesn't throw), so the try/catch here is primarily catching errors fromgetAutoSyncConfig(). The catch block swallows all exceptions and defaults totrue, which may hide underlying issues.Consider logging the error type or re-throwing unexpected exceptions:
♻️ Suggested refinement
Future<void> loadAutoSyncConfig() async { - try { - final preferencesManager = getBinding<PreferencesSettingManager>(); - if (preferencesManager != null) { - final config = await preferencesManager.getAutoSyncConfig(); - _isAutoSyncEnabled.value = config.isEnabled; - log('SetupAutoSyncExtension::loadAutoSyncConfig: isEnabled = ${config.isEnabled}'); - _applyAutoSyncSetting(config.isEnabled); - } - } catch (e) { - log('SetupAutoSyncExtension::loadAutoSyncConfig: error = $e'); + final preferencesManager = getBinding<PreferencesSettingManager>(); + if (preferencesManager == null) { + log('SetupAutoSyncExtension::loadAutoSyncConfig: PreferencesSettingManager not found, defaulting to enabled'); _isAutoSyncEnabled.value = true; + _applyAutoSyncSetting(true); + return; + } + try { + final config = await preferencesManager.getAutoSyncConfig(); + _isAutoSyncEnabled.value = config.isEnabled; + log('SetupAutoSyncExtension::loadAutoSyncConfig: isEnabled = ${config.isEnabled}'); + _applyAutoSyncSetting(config.isEnabled); + } catch (e) { + log('SetupAutoSyncExtension::loadAutoSyncConfig: error loading config = $e'); + _isAutoSyncEnabled.value = true; + _applyAutoSyncSetting(true); } }
28-43: Setting applied regardless of save failure.
_applyAutoSyncSetting(newValue)on line 42 executes outside the try/catch, meaning the WebSocket will reconnect/disconnect even if persisting the preference fails. This could cause UI/state inconsistency on app restart.♻️ Suggested fix to ensure consistency
Future<void> toggleAutoSync() async { final newValue = !_isAutoSyncEnabled.value; - _isAutoSyncEnabled.value = newValue; try { final preferencesManager = getBinding<PreferencesSettingManager>(); if (preferencesManager != null) { await preferencesManager.updateAutoSync(newValue); log('SetupAutoSyncExtension::toggleAutoSync: saved isEnabled = $newValue'); } + _isAutoSyncEnabled.value = newValue; + _applyAutoSyncSetting(newValue); } catch (e) { log('SetupAutoSyncExtension::toggleAutoSync: error saving = $e'); + // Optionally revert UI or show error toast } - - _applyAutoSyncSetting(newValue); }
lib/features/mailbox_dashboard/presentation/mailbox_dashboard_view_web.dart
Show resolved
Hide resolved
Tsopic
force-pushed
the
feature/auto-sync-toggle-and-websocket-fix
branch
from
d3caa77 to
f2d72b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@lib/features/mailbox_dashboard/presentation/extensions/auto_sync/setup_auto_sync_extension.dart`:
- Around line 13-25: In loadAutoSyncConfig, when an exception occurs you set
_isAutoSyncEnabled.value = true but don’t reapply the WebSocket state; update
the catch block to call _applyAutoSyncSetting(true) after setting
_isAutoSyncEnabled so the socket/auto-sync side effects are applied as well
(this touches the loadAutoSyncConfig method and uses _isAutoSyncEnabled and
_applyAutoSyncSetting; keep the PreferencesSettingManager lookup unchanged).
lib/features/mailbox_dashboard/presentation/extensions/auto_sync/setup_auto_sync_extension.dart
Show resolved
Hide resolved
Bug fixes: - Fix emailDelivery StateChange being ignored in foreground mode (push_base_controller.dart) - emails now sync automatically when WebSocket receives new email notifications - Fix null state comparison in thread_controller.dart and mailbox_controller.dart that blocked WebSocket updates when currentEmailState/currentMailboxState was null New feature: - Add auto-sync toggle button next to refresh button in web view - Users can enable/disable automatic email syncing via WebSocket - Preference is persisted using SharedPreferences (AutoSyncConfig) - Toggle controls WebSocket connection (reconnect/disconnect) - Localized tooltip strings (English and French) Files added: - auto_sync_config.dart: Preference model for auto-sync state - setup_auto_sync_extension.dart: Extension on MailboxDashBoardController Co-Authored-By: Claude Opus 4.5 <[email protected]>
Tsopic
force-pushed
the
feature/auto-sync-toggle-and-websocket-fix
branch
from
f2d72b5 to
b377d22
Compare
Tsopic
changed the title
The app was designed for Apache James which uses a proprietary ticket-based WebSocket authentication. This change adds support for standard JMAP WebSocket (RFC 8887) servers like Stalwart that use bearer token authentication. Changes: - WebSocketDatasourceImpl: Check for ticket capability and fall back to bearer token auth via query parameter if not available - BaseController: Remove ticket capability requirement, only require jmapWebSocket capability - AuthorizationInterceptors: Add currentToken getter for WebSocket auth - Added detailed logging for WebSocket capability detection Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@lib/features/push_notification/data/datasource_impl/web_socket_datasource_impl.dart`:
- Around line 62-88: The _buildBearerAuthUri currently appends access tokens to
the WebSocket URL for all platforms; change it to only add the token for web
clients (where headers are unavailable) and stop using URL query authentication
for mobile/desktop. Detect platform (e.g., kIsWeb) inside _buildBearerAuthUri
and only replace queryParameters with 'access_token' when running on web; for
non-web platforms leave wsUri unchanged and update the caller that opens the
socket to use the AuthorizationInterceptors
(AuthorizationInterceptors.currentToken / authenticationType ==
AuthenticationType.oidc) to set an Authorization: Bearer <token> header when
calling the platform socket connector (e.g., IOWebSocketChannel.connect /
WebSocket.connect) so mobile/desktop use RFC 8887-compliant header-based auth.
Ensure comments mention platform-specific behavior and remove the blanket
"browser only" wording in _buildBearerAuthUri.
| Future<Uri> _buildBearerAuthUri(Uri webSocketUri) async { | ||
| final wsUri = webSocketUri.ensureWebSocketUri(); | ||
|
|
||
| try { | ||
| final authInterceptors = Get.find<AuthorizationInterceptors>(); | ||
| final authType = authInterceptors.authenticationType; | ||
|
|
||
| if (authType == AuthenticationType.oidc) { | ||
| // For OIDC, we need to get the access token | ||
| // The token is passed as a query parameter since browser WebSocket API doesn't support headers | ||
| final token = authInterceptors.currentToken; | ||
| if (token != null && token.isNotEmpty) { | ||
| return wsUri.replace(queryParameters: { | ||
| ...wsUri.queryParameters, | ||
| 'access_token': token, | ||
| }); | ||
| } | ||
| } | ||
| // For basic auth or if no token available, connect without auth in URL | ||
| // The server may use session cookies or reject the connection | ||
| log('WebSocketDatasourceImpl::_buildBearerAuthUri: No bearer token available, connecting without token'); | ||
| } catch (e) { | ||
| log('WebSocketDatasourceImpl::_buildBearerAuthUri: Error getting auth: $e'); | ||
| } | ||
|
|
||
| return wsUri; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
JMAP WebSocket authentication best practices RFC 8887
💡 Result:
Here are concise JMAP-over-WebSocket authentication best practices based on RFC 8887 (with relevant JMAP and TLS guidance):
- Authenticate during the WebSocket handshake (present HTTP credentials in the Upgrade/Extended CONNECT request); those credentials remain valid for the life of the WebSocket. [1]
- Use only secure WebSocket URLs (wss://) and require TLS; follow JMAP/TLS guidance to require TLS 1.2+ and prefer TLS 1.3. Validate certificate chains to prevent MITM. (See RFC 8620 / BCP 195 / RFC 9325). [1][2][3]
- Include "jmap" in Sec-WebSocket-Protocol on the client and require it in the server response to establish the JMAP subprotocol. [1]
- Choose strong HTTP authentication schemes per RFC 8620 Section 8.2 (e.g., OAuth bearer tokens, mutual TLS, or other schemes appropriate for your threat model); avoid weak or plaintext schemes. [1][2]
- Treat expired/invalid handshake credentials either by marking subsequent messages unauthenticated or by closing the connection (server MAY close with WebSocket code 1008 — Policy Violation). Enforce session/credential lifetime and revocation policy. [1]
- Protect confidentiality/integrity and replay: run over TLS, avoid downgrade, and reject a failed TLS handshake. [1][2][3]
- Limit message types to JMAP objects (text UTF‑8 frames); on protocol/auth errors return appropriate JSON Problem Details or close with the proper WebSocket error code. Log and rate-limit authentication failures. [1]
- For long‑running connections, provide a server-side policy for re-authentication, token refresh, or forced reconnects when credentials are revoked/rotated. Document expected behavior to clients (e.g., close vs. mark unauthenticated). [1]
- Apply general WebSocket and JMAP security guidance (XSS/CORS considerations for browser clients, CSRF mitigations, capability limits like maxConcurrentRequests). [1][2]
Sources:
- RFC 8887 — JMAP Subprotocol for WebSocket (authentication, handshake, message handling). [1]
- RFC 8620 — JMAP core (authentication scheme and transport security recommendations). [2]
- RFC 9325 (BCP 195) — current TLS best practices (prefer TLS 1.3, disallow old versions). [3]
🏁 Script executed:
# Find and examine the specific file mentioned
fd web_socket_datasource_impl.dartRepository: linagora/tmail-flutter
Length of output: 149
🏁 Script executed:
# Look for related files that provide context
fd "auth.*interceptor" --type fRepository: linagora/tmail-flutter
Length of output: 270
🏁 Script executed:
cat -n lib/features/push_notification/data/datasource_impl/web_socket_datasource_impl.dart | head -100Repository: linagora/tmail-flutter
Length of output: 5310
🏁 Script executed:
cat -n lib/features/login/data/network/interceptors/authorization_interceptors.dartRepository: linagora/tmail-flutter
Length of output: 13024
🏁 Script executed:
# Search for WebSocket header-related comments or documentation
rg -i "websocket.*header|header.*websocket" --context 3Repository: linagora/tmail-flutter
Length of output: 1012
🏁 Script executed:
# Check if there are tests for this implementation
fd "web_socket.*test" --type fRepository: linagora/tmail-flutter
Length of output: 210
🌐 Web query:
web_socket_channel dart flutter custom headers support
💡 Result:
Short answer: Yes — on mobile/desktop (IOWebSocketChannel) you can pass custom headers; on the web platform you cannot (browser restrictions).
Details:
- IOWebSocketChannel.connect accepts a headers Map and forwards it to dart:io's WebSocket.connect. [1][2]
- dart:io's WebSocket.connect supports a headers parameter (some headers are controlled and ignored). [2]
- The web (Html) implementation cannot set arbitrary request headers because browsers disallow it. [3]
Sources:
- IOWebSocketChannel.connect docs. [1]
- dart:io WebSocket.connect docs. [2]
- Note about web-platform limitation (discussion / Q&A). [3]
🏁 Script executed:
# Check package documentation or usage
rg -i "web_socket_channel|websocket.*header" --type dart -A 3 -B 3Repository: linagora/tmail-flutter
Length of output: 6067
🏁 Script executed:
# Look for any documentation or TODO comments about headers
rg -i "TODO|FIXME|header|http.*auth" lib/features/push_notification/ --type dartRepository: linagora/tmail-flutter
Length of output: 253
Remove approval and flag RFC 8887 compliance issue.
The implementation violates JMAP WebSocket best practices by using URL query parameters for authentication across all platforms. While browsers (web platform) cannot set custom headers, mobile and desktop platforms support header-based authentication via IOWebSocketChannel.
According to RFC 8887, authentication should occur during the WebSocket handshake using HTTP credentials (headers), not query parameters. The current approach:
- Works but is non-compliant with JMAP specification on mobile/desktop
- Exposes tokens in URL logs and browser history
- Uses a platform-agnostic workaround (query params) instead of platform-specific solutions
The code comment suggesting "browser WebSocket API doesn't support headers" is misleading—this limitation applies only to the web platform. Mobile/desktop should implement RFC 8887-compliant header-based authentication (e.g., Bearer token in Authorization header passed to WebSocket.connect()), reserving query parameters only for the web platform where headers are unavailable.
Consider refactoring to use conditional platform-specific authentication:
- Mobile/Desktop: HTTP Bearer token in headers (RFC 8887-compliant)
- Web: Query parameter fallback (browser limitation workaround)
🤖 Prompt for AI Agents
In
`@lib/features/push_notification/data/datasource_impl/web_socket_datasource_impl.dart`
around lines 62 - 88, The _buildBearerAuthUri currently appends access tokens to
the WebSocket URL for all platforms; change it to only add the token for web
clients (where headers are unavailable) and stop using URL query authentication
for mobile/desktop. Detect platform (e.g., kIsWeb) inside _buildBearerAuthUri
and only replace queryParameters with 'access_token' when running on web; for
non-web platforms leave wsUri unchanged and update the caller that opens the
socket to use the AuthorizationInterceptors
(AuthorizationInterceptors.currentToken / authenticationType ==
AuthenticationType.oidc) to set an Authorization: Bearer <token> header when
calling the platform socket connector (e.g., IOWebSocketChannel.connect /
WebSocket.connect) so mobile/desktop use RFC 8887-compliant header-based auth.
Ensure comments mention platform-specific behavior and remove the blanket
"browser only" wording in _buildBearerAuthUri.
Extended WebSocket authentication to support both OIDC and basic auth for non-James JMAP servers like Stalwart. Added more detailed logging to help debug WebSocket connection issues. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@lib/features/push_notification/data/datasource_impl/web_socket_datasource_impl.dart`:
- Line 49: The log in WebSocketDatasourceImpl::getWebSocketChannel prints the
full connectionUri which may include sensitive query params; add a helper method
(e.g., _redactSensitiveParams(Uri uri)) that detects keys like 'access_token',
'authorization', 'ticket' and returns uri.toString() with those values replaced
by '[REDACTED]', then call log('WebSocketDatasourceImpl::getWebSocketChannel:
Connecting to ${_redactSensitiveParams(connectionUri)}') instead of logging the
raw connectionUri; ensure the helper uses uri.replace(queryParameters:
redactedMap) to preserve other params.
- Around line 91-96: The method WebSocketDatasourceImpl::_buildAuthUri currently
falls back silently to an unauthenticated wsUri when credentials are missing or
an error occurs; change this to fail fast by throwing a descriptive exception
(e.g., throw StateError or a custom AuthenticationException) when no auth
credentials are found and rethrow or wrap the caught exception in the catch
block so callers receive the failure, and if you intentionally want a
non-failing fallback instead, at minimum replace the low-visibility log(...)
call with logWarning(...) and include contextual details (method name, reason,
and error) so missing/invalid credentials are visible; ensure callers of
_buildAuthUri (e.g., connection/init routines) are updated to handle the thrown
exception if you choose the fail-fast option.
♻️ Duplicate comments (1)
lib/features/push_notification/data/datasource_impl/web_socket_datasource_impl.dart (1)
62-97: URL query parameter authentication violates RFC 8887 on mobile/desktop platforms.This concern was raised in a previous review. The current implementation passes credentials via query parameters for all platforms, but per RFC 8887, authentication should occur during the WebSocket handshake using HTTP headers. Query parameters expose tokens in URL logs and browser history.
The browser limitation (no custom headers in WebSocket) applies only to the web platform. Mobile/desktop platforms support header-based authentication via
IOWebSocketChannel.connect().
| log('WebSocketDatasourceImpl::getWebSocketChannel: Using standard auth'); | ||
| } | ||
|
|
||
| log('WebSocketDatasourceImpl::getWebSocketChannel: Connecting to $connectionUri'); |
There was a problem hiding this comment.
Avoid logging URIs that may contain authentication credentials.
The connectionUri may contain sensitive query parameters (access_token or authorization) that would be exposed in logs. Consider redacting credentials before logging.
🔒 Proposed fix to redact sensitive query parameters
- log('WebSocketDatasourceImpl::getWebSocketChannel: Connecting to $connectionUri');
+ log('WebSocketDatasourceImpl::getWebSocketChannel: Connecting to ${_redactSensitiveParams(connectionUri)}');Add a helper method:
String _redactSensitiveParams(Uri uri) {
final sensitiveKeys = {'access_token', 'authorization', 'ticket'};
if (uri.queryParameters.keys.any((k) => sensitiveKeys.contains(k))) {
final redacted = uri.queryParameters.map((k, v) =>
MapEntry(k, sensitiveKeys.contains(k) ? '[REDACTED]' : v));
return uri.replace(queryParameters: redacted).toString();
}
return uri.toString();
}🤖 Prompt for AI Agents
In
`@lib/features/push_notification/data/datasource_impl/web_socket_datasource_impl.dart`
at line 49, The log in WebSocketDatasourceImpl::getWebSocketChannel prints the
full connectionUri which may include sensitive query params; add a helper method
(e.g., _redactSensitiveParams(Uri uri)) that detects keys like 'access_token',
'authorization', 'ticket' and returns uri.toString() with those values replaced
by '[REDACTED]', then call log('WebSocketDatasourceImpl::getWebSocketChannel:
Connecting to ${_redactSensitiveParams(connectionUri)}') instead of logging the
raw connectionUri; ensure the helper uses uri.replace(queryParameters:
redactedMap) to preserve other params.
| log('WebSocketDatasourceImpl::_buildAuthUri: No auth credentials available, connecting without auth'); | ||
| } catch (e) { | ||
| log('WebSocketDatasourceImpl::_buildAuthUri: Error getting auth: $e'); | ||
| } | ||
|
|
||
| return wsUri; |
There was a problem hiding this comment.
Silent fallback to unauthenticated connection may mask authentication issues.
When credentials are unavailable or an error occurs, the method silently returns an unauthenticated URI. This could lead to connection failures that are difficult to diagnose, as the actual cause (missing/invalid credentials) is hidden.
Consider either:
- Propagating the error to fail fast with a clear message
- At minimum, using
logWarninginstead oflogfor the "No auth credentials available" case to increase visibility
♻️ Suggested improvement
- log('WebSocketDatasourceImpl::_buildAuthUri: No auth credentials available, connecting without auth');
+ logWarning('WebSocketDatasourceImpl::_buildAuthUri: No auth credentials available, connecting without auth');
} catch (e) {
- log('WebSocketDatasourceImpl::_buildAuthUri: Error getting auth: $e');
+ logError('WebSocketDatasourceImpl::_buildAuthUri: Error getting auth: $e');
}🤖 Prompt for AI Agents
In
`@lib/features/push_notification/data/datasource_impl/web_socket_datasource_impl.dart`
around lines 91 - 96, The method WebSocketDatasourceImpl::_buildAuthUri
currently falls back silently to an unauthenticated wsUri when credentials are
missing or an error occurs; change this to fail fast by throwing a descriptive
exception (e.g., throw StateError or a custom AuthenticationException) when no
auth credentials are found and rethrow or wrap the caught exception in the catch
block so callers receive the failure, and if you intentionally want a
non-failing fallback instead, at minimum replace the low-visibility log(...)
call with logWarning(...) and include contextual details (method name, reason,
and error) so missing/invalid credentials are visible; ensure callers of
_buildAuthUri (e.g., connection/init routines) are updated to handle the thrown
exception if you choose the fail-fast option.
The auto-sync button now shows the actual WebSocket connection status: - Green: Connected and receiving push notifications - Orange: Connecting... - Grey: Disconnected (click to retry) - Red: Not supported by server Added WebSocketConnectionState enum to WebSocketController for observable connection state that UI can bind to. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
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 (1)
lib/features/push_notification/presentation/controller/web_socket_controller.dart (1)
170-205: PreservenotSupportedduring manual reconnect/disconnect.
_cleanUpWebSocketResources()already avoids flippingnotSupported, butdisconnect()andreconnect()overwrite it. That can mislead the UI and allow retry attempts against unsupported servers.🛠️ Suggested fix
void reconnect() { log('WebSocketController::reconnect:'); + if (connectionState.value == WebSocketConnectionState.notSupported) return; _retryRemained = 3; - connectionState.value = WebSocketConnectionState.disconnected; + _cleanUpWebSocketResources(); _connectWebSocket(); } void disconnect() { log('WebSocketController::disconnect:'); - _cleanUpWebSocketResources(); - connectionState.value = WebSocketConnectionState.disconnected; + _cleanUpWebSocketResources(); }
🤖 Fix all issues with AI agents
In `@lib/features/base/base_controller.dart`:
- Around line 367-402: injectWebSocket currently calls
WebSocketController.instance.markNotSupported() without first closing any
existing connection, leaving stale channels/subscriptions; update
injectWebSocket to call WebSocketController.instance.disconnect() (or invoke the
internal cleanup method _cleanUpWebSocketResources() if disconnect is not
exposed) immediately before each markNotSupported() call so any active WebSocket
channel and subscriptions are closed/cancelled before the controller is marked
not supported.
In `@lib/features/mailbox_dashboard/presentation/mailbox_dashboard_view_web.dart`:
- Around line 458-468: Replace the hardcoded tooltip strings in the
WebSocketConnectionState switch (where tooltipMessage, iconPath, iconColor are
set) with localized values from AppLocalizations (e.g.,
AppLocalizations.of(context).autoSyncConnecting, .autoSyncNotSupported,
.autoSyncDisconnectedRetry) and ensure the logic using isEnabled remains
unchanged; then add corresponding ARB entries (autoSyncConnecting,
autoSyncNotSupported, autoSyncDisconnectedRetry) to your localization files and
run the localization generator so the new properties are available on
AppLocalizations.
🧹 Nitpick comments (1)
lib/features/mailbox_dashboard/presentation/extensions/auto_sync/setup_auto_sync_extension.dart (1)
23-31: Apply the default when PreferencesSettingManager is unavailable.
If the binding is null, the setting isn’t applied at all. Consider applying the default (true) to keep WebSocket state consistent with expected behavior.♻️ Possible tweak
if (preferencesManager != null) { final config = await preferencesManager.getAutoSyncConfig(); _isAutoSyncEnabled.value = config.isEnabled; log('SetupAutoSyncExtension::loadAutoSyncConfig: isEnabled = ${config.isEnabled}'); _applyAutoSyncSetting(config.isEnabled); + } else { + _isAutoSyncEnabled.value = true; + _applyAutoSyncSetting(true); }
| void injectWebSocket(Session? session, AccountId? accountId) { | ||
| try { | ||
| log('$runtimeType::injectWebSocket:'); | ||
| requireCapability( | ||
| session!, | ||
| accountId!, | ||
| [ | ||
| CapabilityIdentifier.jmapWebSocket, | ||
| CapabilityIdentifier.jmapWebSocketTicket | ||
| ] | ||
| ); | ||
|
|
||
| // Log available capabilities for debugging | ||
| final account = session?.accounts[accountId]; | ||
| if (account != null) { | ||
| final capabilities = account.accountCapabilities.keys.map((c) => c.value).toList(); | ||
| log('$runtimeType::injectWebSocket: Available capabilities: $capabilities'); | ||
| } | ||
|
|
||
| final hasWebSocket = CapabilityIdentifier.jmapWebSocket.isSupported(session!, accountId!); | ||
| final hasTicket = CapabilityIdentifier.jmapWebSocketTicket.isSupported(session, accountId); | ||
| log('$runtimeType::injectWebSocket: WebSocket supported=$hasWebSocket, Ticket supported=$hasTicket'); | ||
|
|
||
| // Only require WebSocket capability, not ticket (ticket is James-specific) | ||
| if (!hasWebSocket) { | ||
| log('$runtimeType::injectWebSocket: WebSocket push not available - server does not support WebSocket capability'); | ||
| WebSocketController.instance.markNotSupported(); | ||
| return; | ||
| } | ||
|
|
||
| final wsCapability = session.getCapabilityProperties<WebSocketCapability>( | ||
| accountId, | ||
| CapabilityIdentifier.jmapWebSocket); | ||
| log('$runtimeType::injectWebSocket: wsCapability=$wsCapability, supportsPush=${wsCapability?.supportsPush}, url=${wsCapability?.url}'); | ||
|
|
||
| if (wsCapability?.supportsPush != true) { | ||
| throw WebSocketPushNotSupportedException(); | ||
| log('$runtimeType::injectWebSocket: WebSocket push not enabled on server'); | ||
| WebSocketController.instance.markNotSupported(); | ||
| return; | ||
| } | ||
| WebSocketInteractorBindings().dependencies(); | ||
| WebSocketController.instance.initialize(accountId: accountId, session: session); | ||
| log('$runtimeType::injectWebSocket: WebSocket initialized successfully'); | ||
| } catch(e) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find WebSocketController definition
fd -t f -e dart | xargs rg -l "class WebSocketController"Repository: linagora/tmail-flutter
Length of output: 149
🏁 Script executed:
# Also search for the file that likely contains it
fd -t f "websocket.*controller" -iRepository: linagora/tmail-flutter
Length of output: 48
🏁 Script executed:
# Search for markNotSupported and disconnect methods
rg "markNotSupported\(\)|void disconnect" -A 5 -B 2Repository: linagora/tmail-flutter
Length of output: 3221
🏁 Script executed:
# Get the full WebSocketController implementation to understand lifecycle
cat -n lib/features/push_notification/presentation/controller/web_socket_controller.dartRepository: linagora/tmail-flutter
Length of output: 13979
🏁 Script executed:
# Check where injectWebSocket is called from
rg "injectWebSocket\(" -B 3 -A 3Repository: linagora/tmail-flutter
Length of output: 1975
🏁 Script executed:
# Check if there are any calls to WebSocketController.instance before injectWebSocket
rg "WebSocketController\.instance\.(initialize|connect)" -B 2 -A 2Repository: linagora/tmail-flutter
Length of output: 1886
Close existing WebSocket connection before marking unsupported.
markNotSupported() updates state but doesn't call _cleanUpWebSocketResources(), which closes the channel and cancels subscriptions. If the WebSocket was already connected (e.g., during account switch), calling markNotSupported() without cleanup leaves stale resources. Add disconnect() before both markNotSupported() calls to ensure proper cleanup.
Suggested fix
if (!hasWebSocket) {
log('$runtimeType::injectWebSocket: WebSocket push not available - server does not support WebSocket capability');
+ WebSocketController.instance.disconnect();
WebSocketController.instance.markNotSupported();
return;
}
...
if (wsCapability?.supportsPush != true) {
log('$runtimeType::injectWebSocket: WebSocket push not enabled on server');
+ WebSocketController.instance.disconnect();
WebSocketController.instance.markNotSupported();
return;
}🤖 Prompt for AI Agents
In `@lib/features/base/base_controller.dart` around lines 367 - 402,
injectWebSocket currently calls WebSocketController.instance.markNotSupported()
without first closing any existing connection, leaving stale
channels/subscriptions; update injectWebSocket to call
WebSocketController.instance.disconnect() (or invoke the internal cleanup method
_cleanUpWebSocketResources() if disconnect is not exposed) immediately before
each markNotSupported() call so any active WebSocket channel and subscriptions
are closed/cancelled before the controller is marked not supported.
| tooltipMessage = 'Connecting...'; | ||
| iconPath = controller.imagePaths.icConnectedInternet; | ||
| iconColor = Colors.orange; | ||
| case WebSocketConnectionState.notSupported: | ||
| tooltipMessage = 'Auto-sync not supported by server'; | ||
| iconPath = controller.imagePaths.icDialogOfflineMode; | ||
| iconColor = Colors.red; | ||
| case WebSocketConnectionState.disconnected: | ||
| tooltipMessage = isEnabled | ||
| ? 'Disconnected (click to retry)' | ||
| : AppLocalizations.of(context).autoSyncDisabled; |
There was a problem hiding this comment.
Localize the new tooltip strings.
Connecting..., Auto-sync not supported by server, and Disconnected (click to retry) bypass i18n. Please add AppLocalizations keys for these states.
🌐 Suggested localization usage
case WebSocketConnectionState.connecting:
- tooltipMessage = 'Connecting...';
+ tooltipMessage = AppLocalizations.of(context).autoSyncConnecting;
iconPath = controller.imagePaths.icConnectedInternet;
iconColor = Colors.orange;
break;
case WebSocketConnectionState.notSupported:
- tooltipMessage = 'Auto-sync not supported by server';
+ tooltipMessage = AppLocalizations.of(context).autoSyncNotSupported;
iconPath = controller.imagePaths.icDialogOfflineMode;
iconColor = Colors.red;
break;
case WebSocketConnectionState.disconnected:
tooltipMessage = isEnabled
- ? 'Disconnected (click to retry)'
+ ? AppLocalizations.of(context).autoSyncDisconnectedRetry
: AppLocalizations.of(context).autoSyncDisabled;Also add corresponding ARB keys (e.g., autoSyncConnecting, autoSyncNotSupported, autoSyncDisconnectedRetry).
🤖 Prompt for AI Agents
In `@lib/features/mailbox_dashboard/presentation/mailbox_dashboard_view_web.dart`
around lines 458 - 468, Replace the hardcoded tooltip strings in the
WebSocketConnectionState switch (where tooltipMessage, iconPath, iconColor are
set) with localized values from AppLocalizations (e.g.,
AppLocalizations.of(context).autoSyncConnecting, .autoSyncNotSupported,
.autoSyncDisconnectedRetry) and ensure the logic using isEnabled remains
unchanged; then add corresponding ARB entries (autoSyncConnecting,
autoSyncNotSupported, autoSyncDisconnectedRetry) to your localization files and
run the localization generator so the new properties are available on
AppLocalizations.
- Fix WebSocket capability detection for Stalwart servers - Use session.capabilities directly instead of getCapabilityProperties() - Session-level capabilities now properly detected - Add WebSocket authentication documentation - docs/websocket-auth-proxy.md with nginx proxy setup - Explains browser WebSocket auth limitations - Documents James ticket auth vs Stalwart token auth - Add GitHub Actions workflow for desktop builds - macOS build on macos-14 runner, outputs DMG - Windows build on windows-latest, outputs ZIP - Auto-release on version tags - Add macOS and Windows platform support - Generated platform folders via flutter create Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.github/workflows/build-desktop.yaml:
- Around line 134-141: Update the GitHub Actions step named "Create GitHub
Release" to use the latest action by replacing the reference
softprops/action-gh-release@v1 with softprops/action-gh-release@v2; locate the
step where uses: softprops/action-gh-release@v1 is declared and update it to
`@v2`, then run/validate the workflow to ensure the existing inputs (files, draft,
generate_release_notes) remain valid with the v2 release.
In `@macos/Runner/Configs/AppInfo.xcconfig`:
- Around line 10-11: The PRODUCT_BUNDLE_IDENTIFIER currently uses an
Android-style value ("com.linagora.android.tmailUiUser"); update this to a
macOS-appropriate bundle ID (for example "com.linagora.macos.tmailUiUser" or
"com.linagora.teammail") wherever it's set for the macOS app and test targets
(including the macOS project.pbxproj entries for test targets) so provisioning
and code signing use a consistent macOS identifier.
In `@macos/Runner/DebugProfile.entitlements`:
- Around line 5-10: Add the outbound networking entitlement by inserting the
key/value pair <key>com.apple.security.network.client</key><true/> into the
entitlements XML (next to the existing
<key>com.apple.security.network.server</key> entry) so sandboxed debug builds
can make HTTP/WebSocket outbound connections; ensure the new entitlement is
valid XML and placed inside the same top-level dict in
DebugProfile.entitlements.
In `@macos/Runner/Release.entitlements`:
- Around line 4-7: The Release entitlements only enable
com.apple.security.app-sandbox and therefore block all network access; update
the entitlements to explicitly allow network by adding
com.apple.security.network.client (and com.apple.security.network.server if the
app accepts inbound connections) set to true alongside the existing
com.apple.security.app-sandbox key so the sandboxed app can make
WebSocket/API/network requests.
In `@windows/CMakeLists.txt`:
- Around line 90-94: The NATIVE_ASSETS_DIR value is missing a path separator
between PROJECT_BUILD_DIR and native_assets, causing malformed paths; update the
set for NATIVE_ASSETS_DIR so it inserts a "/" between ${PROJECT_BUILD_DIR} and
native_assets (i.e., change the value assigned to NATIVE_ASSETS_DIR used by
install to include "${PROJECT_BUILD_DIR}/native_assets/windows/") to ensure the
install(DIRECTORY ...) call finds the correct directory.
In `@windows/flutter/generated_plugins.cmake`:
- Around line 1-38: The FLUTTER_FFI_PLUGIN_LIST contains a stale entry "jni";
remove "jni" from the FLUTTER_FFI_PLUGIN_LIST declaration and from any generated
plugin symlink handling to stop it being linked on Windows (look for the
FLUTTER_FFI_PLUGIN_LIST and occurrences of jni_plugin / ${jni}_bundled_libraries
in the generated code), then run flutter clean and flutter pub get to
regenerate; if jni reappears, inspect pubspec/dependency graph for a transitive
dependency requiring jni and either remove that dependency or add an explicit
platform exclusion for Windows so jni is not included in Windows builds.
In `@windows/runner/flutter_window.cpp`:
- Around line 64-67: MessageHandler currently dereferences flutter_controller_
unconditionally for WM_FONTCHANGE, which can be null after OnDestroy(); fix by
guarding the WM_FONTCHANGE case with a null check on flutter_controller_ (e.g.,
verify flutter_controller_ != nullptr) before calling
flutter_controller_->engine()->ReloadSystemFonts(), and skip/ignore the message
if the controller is null to avoid a crash.
♻️ Duplicate comments (3)
lib/features/base/base_controller.dart (1)
367-409: Calldisconnect()beforemarkNotSupported()to clean up stale WebSocket resources.The
markNotSupported()method only sets a state flag and does not invoke resource cleanup. When called at lines 373, 390, and 400 without first disconnecting, any existing WebSocket channel, subscriptions, and timers remain active and are never cleaned up. This is problematic during account switches or session refreshes wheninjectWebSocket()may be called while a prior connection is still active.Proposed fix
if (session == null || accountId == null) { log('$runtimeType::injectWebSocket: Session or accountId is null', webConsoleEnabled: true); + WebSocketController.instance.disconnect(); WebSocketController.instance.markNotSupported(); return; } ... if (!hasWebSocket) { log('$runtimeType::injectWebSocket: WebSocket push not available - server does not support WebSocket capability', webConsoleEnabled: true); + WebSocketController.instance.disconnect(); WebSocketController.instance.markNotSupported(); return; } ... if (wsCapability?.supportsPush != true) { log('$runtimeType::injectWebSocket: WebSocket push not enabled on server', webConsoleEnabled: true); + WebSocketController.instance.disconnect(); WebSocketController.instance.markNotSupported(); return; }lib/features/push_notification/data/datasource_impl/web_socket_datasource_impl.dart (2)
47-47: Avoid logging URIs that may contain authentication credentials.The
connectionUrimay contain sensitive query parameters (access_token,authorization, orticket) that would be exposed in logs. Consider redacting credentials before logging.🔒 Proposed fix to redact sensitive query parameters
- log('WebSocketDatasourceImpl::getWebSocketChannel: Connecting to $connectionUri', webConsoleEnabled: true); + log('WebSocketDatasourceImpl::getWebSocketChannel: Connecting to ${_redactSensitiveParams(connectionUri)}', webConsoleEnabled: true);Add a helper method:
String _redactSensitiveParams(Uri uri) { const sensitiveKeys = {'access_token', 'authorization', 'ticket'}; if (uri.queryParameters.keys.any((k) => sensitiveKeys.contains(k))) { final redacted = uri.queryParameters.map((k, v) => MapEntry(k, sensitiveKeys.contains(k) ? '[REDACTED]' : v)); return uri.replace(queryParameters: redacted).toString(); } return uri.toString(); }
124-129: Silent fallback to unauthenticated connection may mask authentication issues.When credentials are unavailable or an error occurs, the method silently returns an unauthenticated URI. This could lead to connection failures that are difficult to diagnose.
♻️ Suggested improvement - use warning-level logging
- log('WebSocketDatasourceImpl::_buildAuthUri: No auth credentials available, connecting without auth', webConsoleEnabled: true); + logWarning('WebSocketDatasourceImpl::_buildAuthUri: No auth credentials available, connecting without auth'); } catch (e) { - log('WebSocketDatasourceImpl::_buildAuthUri: Error getting auth: $e', webConsoleEnabled: true); + logError('WebSocketDatasourceImpl::_buildAuthUri: Error getting auth: $e'); }
🧹 Nitpick comments (1)
docs/websocket-auth-proxy.md (1)
17-20: Add language specifiers to fenced code blocks.Markdown linting flagged missing language specifiers. While the content is clear, adding specifiers improves syntax highlighting and accessibility.
📝 Suggested fixes
-``` +```text Browser → TLS Proxy → ws-auth-proxy → Mail Server (HTTPS) (token→header) (WebSocket)Similarly for lines 38-41 and 44-46, use `http` or `text`: ```diff - ``` + ```http POST /jmap/ws/ticket Authorization: Bearer <access_token> ```
| - name: Create GitHub Release | ||
| uses: softprops/action-gh-release@v1 | ||
| with: | ||
| files: | | ||
| artifacts/TMail-macOS.dmg | ||
| artifacts/TMail-Windows.zip | ||
| draft: true | ||
| generate_release_notes: true |
There was a problem hiding this comment.
Update softprops/action-gh-release to v2.
The static analysis tool flagged that softprops/action-gh-release@v1 uses an outdated runner. Update to v2 for compatibility with current GitHub Actions runners.
🔧 Proposed fix
- name: Create GitHub Release
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2
with:
files: |
artifacts/TMail-macOS.dmg
artifacts/TMail-Windows.zip
draft: true
generate_release_notes: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Create GitHub Release | |
| uses: softprops/action-gh-release@v1 | |
| with: | |
| files: | | |
| artifacts/TMail-macOS.dmg | |
| artifacts/TMail-Windows.zip | |
| draft: true | |
| generate_release_notes: true | |
| - name: Create GitHub Release | |
| uses: softprops/action-gh-release@v2 | |
| with: | |
| files: | | |
| artifacts/TMail-macOS.dmg | |
| artifacts/TMail-Windows.zip | |
| draft: true | |
| generate_release_notes: true |
🧰 Tools
🪛 actionlint (1.7.10)
135-135: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In @.github/workflows/build-desktop.yaml around lines 134 - 141, Update the
GitHub Actions step named "Create GitHub Release" to use the latest action by
replacing the reference softprops/action-gh-release@v1 with
softprops/action-gh-release@v2; locate the step where uses:
softprops/action-gh-release@v1 is declared and update it to `@v2`, then
run/validate the workflow to ensure the existing inputs (files, draft,
generate_release_notes) remain valid with the v2 release.
| // The application's bundle identifier | ||
| PRODUCT_BUNDLE_IDENTIFIER = com.linagora.android.tmailUiUser |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find bundle identifier definitions/usages across the repo.
rg -n "PRODUCT_BUNDLE_IDENTIFIER|com\.linagora\.android\.tmailUiUser" -g '!**/build/**' -g '!**/dist/**'Repository: linagora/tmail-flutter
Length of output: 2216
Fix macOS bundle identifier—currently uses Android package naming convention.
Line 11 uses com.linagora.android.tmailUiUser, which contains .android. and is inconsistent with the iOS configs that correctly use .ios. (e.g., com.linagora.ios.teammail). The same Android-scoped ID also appears in macos/Runner.xcodeproj/project.pbxproj for test targets. This will cause Apple provisioning and code signing issues. Change to a proper macOS identifier (e.g., com.linagora.macos.tmailUiUser or com.linagora.teammail).
🤖 Prompt for AI Agents
In `@macos/Runner/Configs/AppInfo.xcconfig` around lines 10 - 11, The
PRODUCT_BUNDLE_IDENTIFIER currently uses an Android-style value
("com.linagora.android.tmailUiUser"); update this to a macOS-appropriate bundle
ID (for example "com.linagora.macos.tmailUiUser" or "com.linagora.teammail")
wherever it's set for the macOS app and test targets (including the macOS
project.pbxproj entries for test targets) so provisioning and code signing use a
consistent macOS identifier.
| <key>com.apple.security.app-sandbox</key> | ||
| <true/> | ||
| <key>com.apple.security.cs.allow-jit</key> | ||
| <true/> | ||
| <key>com.apple.security.network.server</key> | ||
| <true/> |
There was a problem hiding this comment.
Missing com.apple.security.network.client entitlement for outbound connections.
The app uses WebSocket connections and HTTP requests, which require outbound network access. The network.server entitlement only allows inbound connections. Add the client entitlement to enable WebSocket and API communication in sandboxed debug builds.
Proposed fix
<key>com.apple.security.network.server</key>
<true/>
+ <key>com.apple.security.network.client</key>
+ <true/>
</dict>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <key>com.apple.security.app-sandbox</key> | |
| <true/> | |
| <key>com.apple.security.cs.allow-jit</key> | |
| <true/> | |
| <key>com.apple.security.network.server</key> | |
| <true/> | |
| <key>com.apple.security.app-sandbox</key> | |
| <true/> | |
| <key>com.apple.security.cs.allow-jit</key> | |
| <true/> | |
| <key>com.apple.security.network.server</key> | |
| <true/> | |
| <key>com.apple.security.network.client</key> | |
| <true/> |
🤖 Prompt for AI Agents
In `@macos/Runner/DebugProfile.entitlements` around lines 5 - 10, Add the outbound
networking entitlement by inserting the key/value pair
<key>com.apple.security.network.client</key><true/> into the entitlements XML
(next to the existing <key>com.apple.security.network.server</key> entry) so
sandboxed debug builds can make HTTP/WebSocket outbound connections; ensure the
new entitlement is valid XML and placed inside the same top-level dict in
DebugProfile.entitlements.
| <dict> | ||
| <key>com.apple.security.app-sandbox</key> | ||
| <true/> | ||
| </dict> |
There was a problem hiding this comment.
Critical: Missing network entitlements will block all network access in release builds.
The release entitlements only enable the sandbox without granting network permissions. This will prevent the sandboxed app from making WebSocket connections, API calls, or any network requests—breaking core email functionality.
Proposed fix
<dict>
<key>com.apple.security.app-sandbox</key>
<true/>
+ <key>com.apple.security.network.client</key>
+ <true/>
</dict>🤖 Prompt for AI Agents
In `@macos/Runner/Release.entitlements` around lines 4 - 7, The Release
entitlements only enable com.apple.security.app-sandbox and therefore block all
network access; update the entitlements to explicitly allow network by adding
com.apple.security.network.client (and com.apple.security.network.server if the
app accepts inbound connections) set to true alongside the existing
com.apple.security.app-sandbox key so the sandboxed app can make
WebSocket/API/network requests.
| # Copy the native assets provided by the build.dart from all packages. | ||
| set(NATIVE_ASSETS_DIR "${PROJECT_BUILD_DIR}native_assets/windows/") | ||
| install(DIRECTORY "${NATIVE_ASSETS_DIR}" | ||
| DESTINATION "${INSTALL_BUNDLE_LIB_DIR}" | ||
| COMPONENT Runtime) |
There was a problem hiding this comment.
Missing path separator in NATIVE_ASSETS_DIR.
The path is missing a / between PROJECT_BUILD_DIR and native_assets:
set(NATIVE_ASSETS_DIR "${PROJECT_BUILD_DIR}native_assets/windows/")This should be:
set(NATIVE_ASSETS_DIR "${PROJECT_BUILD_DIR}/native_assets/windows/")Without the separator, the path will be malformed (e.g., build/Debug/native_assets/windows/ becomes build/Debugnative_assets/windows/), which could cause native assets to not be found during installation.
Proposed fix
# Copy the native assets provided by the build.dart from all packages.
-set(NATIVE_ASSETS_DIR "${PROJECT_BUILD_DIR}native_assets/windows/")
+set(NATIVE_ASSETS_DIR "${PROJECT_BUILD_DIR}/native_assets/windows/")
install(DIRECTORY "${NATIVE_ASSETS_DIR}"
DESTINATION "${INSTALL_BUNDLE_LIB_DIR}"
COMPONENT Runtime)🤖 Prompt for AI Agents
In `@windows/CMakeLists.txt` around lines 90 - 94, The NATIVE_ASSETS_DIR value is
missing a path separator between PROJECT_BUILD_DIR and native_assets, causing
malformed paths; update the set for NATIVE_ASSETS_DIR so it inserts a "/"
between ${PROJECT_BUILD_DIR} and native_assets (i.e., change the value assigned
to NATIVE_ASSETS_DIR used by install to include
"${PROJECT_BUILD_DIR}/native_assets/windows/") to ensure the install(DIRECTORY
...) call finds the correct directory.
| # | ||
| # Generated file, do not edit. | ||
| # | ||
|
|
||
| list(APPEND FLUTTER_PLUGIN_LIST | ||
| app_links | ||
| connectivity_plus | ||
| desktop_drop | ||
| desktop_webview_window | ||
| firebase_core | ||
| flutter_inappwebview_windows | ||
| flutter_secure_storage_windows | ||
| permission_handler_windows | ||
| printing | ||
| sentry_flutter | ||
| share_plus | ||
| url_launcher_windows | ||
| window_to_front | ||
| ) | ||
|
|
||
| list(APPEND FLUTTER_FFI_PLUGIN_LIST | ||
| jni | ||
| pdfrx | ||
| ) | ||
|
|
||
| set(PLUGIN_BUNDLED_LIBRARIES) | ||
|
|
||
| foreach(plugin ${FLUTTER_PLUGIN_LIST}) | ||
| add_subdirectory(flutter/ephemeral/.plugin_symlinks/${plugin}/windows plugins/${plugin}) | ||
| target_link_libraries(${BINARY_NAME} PRIVATE ${plugin}_plugin) | ||
| list(APPEND PLUGIN_BUNDLED_LIBRARIES $<TARGET_FILE:${plugin}_plugin>) | ||
| list(APPEND PLUGIN_BUNDLED_LIBRARIES ${${plugin}_bundled_libraries}) | ||
| endforeach(plugin) | ||
|
|
||
| foreach(ffi_plugin ${FLUTTER_FFI_PLUGIN_LIST}) | ||
| add_subdirectory(flutter/ephemeral/.plugin_symlinks/${ffi_plugin}/windows plugins/${ffi_plugin}) | ||
| list(APPEND PLUGIN_BUNDLED_LIBRARIES ${${ffi_plugin}_bundled_libraries}) | ||
| endforeach(ffi_plugin) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if jni is used in Dart code
rg -l "jni" --type dart -g '!*.g.dart' | head -20Repository: linagora/tmail-flutter
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Also check pubspec.yaml to see if jni is declared as a dependency
find . -name "pubspec.yaml" -type f | head -5 | xargs catRepository: linagora/tmail-flutter
Length of output: 9592
🏁 Script executed:
#!/bin/bash
# Check if there are any references to jni in Windows-specific code
rg "jni" windows/ --type cmake --type dart 2>/dev/null | head -20Repository: linagora/tmail-flutter
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Find and read the main pubspec.yaml (not in subdirectories)
cat pubspec.yaml 2>/dev/null | head -100Repository: linagora/tmail-flutter
Length of output: 2558
🏁 Script executed:
#!/bin/bash
# Check if there's a pubspec.lock file that might show jni dependency
head -50 pubspec.lock 2>/dev/null | grep -A 5 -B 5 "jni"Repository: linagora/tmail-flutter
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Search for any reference to jni plugin in the entire codebase (beyond just Dart)
rg "jni" . --type-list | head -5
rg "jni" . -t cmake -t yaml 2>/dev/null | grep -v node_modules | head -30Repository: linagora/tmail-flutter
Length of output: 520
Remove jni from Windows FFI plugins—it's a stale or erroneous entry.
This auto-generated file includes jni in FLUTTER_FFI_PLUGIN_LIST (line 22), but jni is neither declared in any pubspec.yaml nor used anywhere in the codebase. This is a stale or incorrectly resolved plugin entry.
To fix: Run flutter clean to clear the plugin cache, then flutter pub get to regenerate. If jni reappears, verify no dependency inadvertently requires it for Windows, then explicitly exclude it from the Windows build.
🤖 Prompt for AI Agents
In `@windows/flutter/generated_plugins.cmake` around lines 1 - 38, The
FLUTTER_FFI_PLUGIN_LIST contains a stale entry "jni"; remove "jni" from the
FLUTTER_FFI_PLUGIN_LIST declaration and from any generated plugin symlink
handling to stop it being linked on Windows (look for the
FLUTTER_FFI_PLUGIN_LIST and occurrences of jni_plugin / ${jni}_bundled_libraries
in the generated code), then run flutter clean and flutter pub get to
regenerate; if jni reappears, inspect pubspec/dependency graph for a transitive
dependency requiring jni and either remove that dependency or add an explicit
platform exclusion for Windows so jni is not included in Windows builds.
| switch (message) { | ||
| case WM_FONTCHANGE: | ||
| flutter_controller_->engine()->ReloadSystemFonts(); | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "flutter_window.cpp" | grep -i windowsRepository: linagora/tmail-flutter
Length of output: 236
🏁 Script executed:
git ls-files windows/runner/ | head -20Repository: linagora/tmail-flutter
Length of output: 419
🏁 Script executed:
cat -n windows/runner/flutter_window.cpp | sed -n '50,100p'Repository: linagora/tmail-flutter
Length of output: 929
🏁 Script executed:
cat -n windows/runner/flutter_window.cpp | sed -n '64,150p'Repository: linagora/tmail-flutter
Length of output: 313
🏁 Script executed:
cat -n windows/runner/flutter_window.cpp | head -80Repository: linagora/tmail-flutter
Length of output: 2684
🏁 Script executed:
rg -A 5 "flutter_controller_\s*=" windows/runner/flutter_window.cppRepository: linagora/tmail-flutter
Length of output: 439
Guard WM_FONTCHANGE when the controller is null.
The MessageHandler can be invoked after OnDestroy() has set flutter_controller_ to nullptr (line 44). Line 66 unconditionally dereferences flutter_controller_->engine() without a null check, which will crash if a font change message arrives during or after teardown. Add a guard before accessing the controller.
🛠️ Proposed fix
switch (message) {
case WM_FONTCHANGE:
- flutter_controller_->engine()->ReloadSystemFonts();
+ if (flutter_controller_ && flutter_controller_->engine()) {
+ flutter_controller_->engine()->ReloadSystemFonts();
+ }
break;
}🤖 Prompt for AI Agents
In `@windows/runner/flutter_window.cpp` around lines 64 - 67, MessageHandler
currently dereferences flutter_controller_ unconditionally for WM_FONTCHANGE,
which can be null after OnDestroy(); fix by guarding the WM_FONTCHANGE case with
a null check on flutter_controller_ (e.g., verify flutter_controller_ !=
nullptr) before calling flutter_controller_->engine()->ReloadSystemFonts(), and
skip/ignore the message if the controller is null to avoid a crash.
| The proxy: | ||
| 1. Extracts `access_token` from the query string |
There was a problem hiding this comment.
Issue is: this will get logged... Which is terrible security wise.
If doing so on prod then I strongly advise to set extremely short lived token.
Also we designed an extension to solve this very problem. Maybe Stallwart author would be willing to implement it. I would have a chat regarding this with him at FOSDEM.
@Tsopic is this behaviour added by this changeset?
| } | ||
| ``` | ||
|
|
||
| Without the James ticket capability, TMail passes the access token as a query parameter which must be converted to a header by the proxy. |
There was a problem hiding this comment.
Cc @hoangdat this is insecure, and I am piecemealed regarding this vulnerable-by-default behaviour.
Issue is that a regular EG fastmail user have little chance to know of this behaviour before sending his token all over the place.
TBH I would prefer an explicit user decision, in settings to activate WS if we know we can't reasonnably do this in a secure fashion.
Cc @Crash--
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
Hello @Tsopic
Thanks a lot for your interest in Twake Mail.
Though done by a bot it is a nice contribution.
However in order to ease human review I would strongly encorage to split it in small chucks:
- One PR with desktop app (ideally with a little demo video)
- One PR for fixing WS on stallwart for web
- One PR for the "auto-Sync button" ideally with a clear description of what is does
- Any other relevant topic that could leave in a standalone PR
Also I would like us to consider seriously the security impact of those changes and that we avoid shipping unsecure-by-default code.
(I'm not a flutter specialist so my review is limited to the doc)
Cheers
Benoit
| // Log available session-level capabilities for debugging | ||
| final sessionCapabilities = session.capabilities.keys.map((c) => c.value.toString()).toList(); | ||
| log('$runtimeType::injectWebSocket: Session capabilities: $sessionCapabilities', webConsoleEnabled: true); |
There was a problem hiding this comment.
Only add it when if (BuildUtils.isDebugMode)
| // WebSocket is a session-level capability, not account-level | ||
| // Check session.capabilities instead of account.accountCapabilities | ||
| final hasWebSocket = session.capabilities.containsKey(CapabilityIdentifier.jmapWebSocket); | ||
| final hasTicket = session.capabilities.containsKey(CapabilityIdentifier.jmapWebSocketTicket); |
There was a problem hiding this comment.
WebSocket capability has also been added at the account level, allowing us to manage multiple accounts later on.
| if (!hasWebSocket) { | ||
| log('$runtimeType::injectWebSocket: WebSocket push not available - server does not support WebSocket capability', webConsoleEnabled: true); | ||
| WebSocketController.instance.markNotSupported(); | ||
| return; | ||
| } | ||
|
|
||
| // Get WebSocket capability properties from session-level capabilities | ||
| final wsCapability = session.capabilities[CapabilityIdentifier.jmapWebSocket] as WebSocketCapability?; | ||
| log('$runtimeType::injectWebSocket: wsCapability=$wsCapability, supportsPush=${wsCapability?.supportsPush}, url=${wsCapability?.url}', webConsoleEnabled: true); | ||
|
|
||
| if (wsCapability?.supportsPush != true) { | ||
| throw WebSocketPushNotSupportedException(); | ||
| log('$runtimeType::injectWebSocket: WebSocket push not enabled on server', webConsoleEnabled: true); | ||
| WebSocketController.instance.markNotSupported(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
We should combine these check conditions into a single function to avoid unnecessary code.
| Padding( | ||
| padding: const EdgeInsetsDirectional.only(start: 8), | ||
| child: Obx(() { | ||
| final connectionState = controller.webSocketConnectionState.value; | ||
| final isEnabled = controller.isAutoSyncEnabled.value; | ||
|
|
||
| // Determine icon, color, and tooltip based on actual connection state | ||
| String tooltipMessage; | ||
| String iconPath; | ||
| Color iconColor; | ||
|
|
||
| switch (connectionState) { | ||
| case WebSocketConnectionState.connected: | ||
| tooltipMessage = AppLocalizations.of(context).autoSyncEnabled; | ||
| iconPath = controller.imagePaths.icConnectedInternet; | ||
| iconColor = Colors.green; | ||
| case WebSocketConnectionState.connecting: | ||
| tooltipMessage = 'Connecting...'; | ||
| iconPath = controller.imagePaths.icConnectedInternet; | ||
| iconColor = Colors.orange; | ||
| case WebSocketConnectionState.notSupported: | ||
| tooltipMessage = 'Auto-sync not supported by server'; | ||
| iconPath = controller.imagePaths.icDialogOfflineMode; | ||
| iconColor = Colors.red; | ||
| case WebSocketConnectionState.disconnected: | ||
| tooltipMessage = isEnabled | ||
| ? 'Disconnected (click to retry)' |
There was a problem hiding this comment.
IMO, It would be more appropriate to leave it in Settings.
| final connectionState = controller.webSocketConnectionState.value; | ||
| final isEnabled = controller.isAutoSyncEnabled.value; | ||
|
|
||
| // Determine icon, color, and tooltip based on actual connection state | ||
| String tooltipMessage; | ||
| String iconPath; | ||
| Color iconColor; | ||
|
|
||
| switch (connectionState) { | ||
| case WebSocketConnectionState.connected: | ||
| tooltipMessage = AppLocalizations.of(context).autoSyncEnabled; | ||
| iconPath = controller.imagePaths.icConnectedInternet; | ||
| iconColor = Colors.green; | ||
| case WebSocketConnectionState.connecting: | ||
| tooltipMessage = 'Connecting...'; | ||
| iconPath = controller.imagePaths.icConnectedInternet; | ||
| iconColor = Colors.orange; | ||
| case WebSocketConnectionState.notSupported: | ||
| tooltipMessage = 'Auto-sync not supported by server'; | ||
| iconPath = controller.imagePaths.icDialogOfflineMode; | ||
| iconColor = Colors.red; | ||
| case WebSocketConnectionState.disconnected: | ||
| tooltipMessage = isEnabled | ||
| ? 'Disconnected (click to retry)' |
There was a problem hiding this comment.
Create a class to draw this widget; we can use it in many places.
| } | ||
|
|
||
| return Tooltip( | ||
| message: tooltipMessage, |
There was a problem hiding this comment.
Please use tooltipMessage property of TMailButtonWidget
|
hi @Tsopic , can you describe more detail on your problem you have relate to websocket? maybe a video about this? Thank in advance. |
2 problems. JMAP is supposed to be a real-time mail. For the current implementation, combined with the Stalwart. It is not; it is just basically using IMAP / JMAP pull flow in the background, and mail pulls are done by the refresh button. With Stalwart, there is no Ticket auth available, which James' mail servers use; therefore i've implemented a hacky way to make the websocket work, by writing auth_token from url to headers with Stalwart Docker sidecontainer. The hacky way is implemented because Browser websockets do not allow authorization headers. What it achieves so far is an "indicator/toggle button" which shows that the websocket connection is connected and achieves near-realtime mailbox updates when new mail is found in server. This is currently enabled by default. The PR WIP status indicates it is a work in progress. I'll do a cleanup soon and follow up with a cleaner PR. Thanks for all the feedback so far from @dab246 @hoangdat @chibenwa |
|
I've added feature request to stalwart. If they implement it, we could avoid this ugly sidecontainer hack stalwartlabs/stalwart#2677 |
|
Yes I also sent a mail to the author and we would be able to discuss this at FOSDEM. |
|
PR-s with the code and website changes are done for the compability |
|
Stalwart is not currently accepting PR-s as they are tring to reach stable V1 Closing this PR in favor of clean per #4261 |
4 participants
Summary
Bug Fixes
push_base_controller.dartwas ignoringemailDeliveryStateChange type when app is in foreground, preventing automatic email syncthread_controller.dartandmailbox_controller.dartwere comparing against null state, which blocked WebSocket updates when the initial state hadn't been setNew Feature: Auto-Sync Toggle
Files Changed
push_base_controller.dartthread_controller.dartmailbox_controller.dartweb_socket_controller.dartreconnect()anddisconnect()methodsauto_sync_config.dartsetup_auto_sync_extension.dartpreferences_setting_manager.dartmailbox_dashboard_controller.dartmailbox_dashboard_view_web.dartTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Documentation
Localization
✏️ Tip: You can customize this high-level summary in your review settings.