Conversation
zatteo
requested review from
dab246,
hoangdat,
nqhhdev and
tddang-linagora
as code owners
WalkthroughThe PR adds a public 🚥 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🧪 Generate unit tests (beta)
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 |
| if (json['cozy_external_bridge_allowlist'] is List<String> && | ||
| json['cozy_external_bridge_allowlist'].isNotEmpty) { | ||
| cozyExternalBridgeAllowlist = json['cozy_external_bridge_allowlist']; | ||
| } |
There was a problem hiding this comment.
The goal is to be able to add new approved domain name for bridge in env var or in config.json. It will help us:
- ease development experience by adding for example in config.json 'localhost:8080'
- allow to deploy tchat in a container for any domain name
However, for config.json, it seems that it is loaded after CozyBridge initialization. Any thought on this?
There was a problem hiding this comment.
Create a method to initialize config early, put only the cozy config initialization there, then call as soon as the app start.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
lib/widgets/matrix.dart (1)
529-540: Silent catch swallows the exception without logging.If
CozyConfigManager().isInContainerorrequestNotificationPermission()throws, the error is silently discarded and the fallback path runs with no diagnostic information.♻️ Proposed fix
} catch (e) { + Logs().w('_requestNotificationPermission: falling back to browser permission', e); html.Notification.requestPermission(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/widgets/matrix.dart` around lines 529 - 540, In _requestNotificationPermission, the current catch block swallows exceptions from CozyConfigManager().isInContainer or CozyConfigManager().requestNotificationPermission(); update the catch to capture the exception and stack trace and log them (e.g., via print/debugPrint or the app logger) before falling back to html.Notification.requestPermission(); ensure you reference CozyConfigManager(), its requestNotificationPermission method, and html.Notification.requestPermission so the error context is clear in the log.
🤖 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/config/app_config.dart`:
- Around line 223-225: cozyExternalBridgeAllowlist is built from
_cozyExternalBridgeAllowlistEnv.split(',') which yields [''] when the env var is
explicitly empty; change the construction so that when
_cozyExternalBridgeAllowlistEnv is empty or only whitespace you pass an empty
list to CozyConfigManager().initialize(validUrlSuffixes: ...), otherwise split
on ',' and trim/filter out any empty strings (e.g., map trim and where not
empty) before calling CozyConfigManager().initialize to avoid passing a single
empty-string entry.
- Around line 317-320: The runtime type check using `is List<String>` on
`json['cozy_external_bridge_allowlist']` always fails because JSON arrays decode
to `List<dynamic>`; update the code around `cozyExternalBridgeAllowlist` so it
checks `is List` (or `is List<dynamic>`), then convert/validate the elements to
strings (e.g., map/cast each element to `String` and filter non-strings) and
assign the resulting `List<String>` only if non-empty; reference
`cozyExternalBridgeAllowlist` and `json['cozy_external_bridge_allowlist']` when
applying this change.
In `@lib/main.dart`:
- Around line 68-71: The app is starting the UI before CozyConfigManager
finishes setup, causing isInContainer reads (used by AdaptiveScaffoldAppBar) to
be stale; change the startup sequence so injectCozyScript(...) completes and
CozyConfigManager().initialize(...) is awaited before calling startGui(clients).
Locate the chain that calls CozyConfigManager().injectCozyScript and
CozyConfigManager().initialize and make startGui(clients) run only after the
Future returned by injectCozyScript().then(...) resolves (or use async/await),
ensuring isInContainer is set before the first widget build.
In `@lib/widgets/layouts/adaptive_layout/adaptive_scaffold_appbar.dart`:
- Around line 53-57: The code races because CozyConfigManager().initialize(...)
(called via .then()) isn’t awaited before startGui() runs, so
adaptive_scaffold_appbar's check CozyConfigManager().isInContainer can read an
uninitialized value; fix by awaiting the initialization sequence on the
PlatformInfos.isWeb branch — await
CozyConfigManager().injectCozyScript(AppConfig.cozyExternalBridgeVersion) and
then await CozyConfigManager().initialize(validUrlSuffixes:
AppConfig.cozyExternalBridgeAllowlist) (instead of chaining with .then()), and
only call startGui() after those awaits complete.
---
Nitpick comments:
In `@lib/widgets/matrix.dart`:
- Around line 529-540: In _requestNotificationPermission, the current catch
block swallows exceptions from CozyConfigManager().isInContainer or
CozyConfigManager().requestNotificationPermission(); update the catch to capture
the exception and stack trace and log them (e.g., via print/debugPrint or the
app logger) before falling back to html.Notification.requestPermission(); ensure
you reference CozyConfigManager(), its requestNotificationPermission method, and
html.Notification.requestPermission so the error context is clear in the log.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/config/app_config.dartlib/main.dartlib/widgets/layouts/adaptive_layout/adaptive_scaffold_appbar.dartlib/widgets/local_notifications_extension.dartlib/widgets/matrix.dart
| CozyConfigManager() | ||
| .injectCozyScript(AppConfig.cozyExternalBridgeVersion) | ||
| .then((value) => CozyConfigManager().initialize()); | ||
| .then((value) => CozyConfigManager().initialize(validUrlSuffixes: AppConfig.cozyExternalBridgeAllowlist)); | ||
| } |
There was a problem hiding this comment.
CozyConfigManager().initialize() is fire-and-forget — race condition with initial widget builds.
injectCozyScript(...).then(initialize) is not awaited. startGui(clients) executes immediately after, so isInContainer can be false during the first widget tree render (including AdaptiveScaffoldAppBar). Unlike the previous FutureBuilder(isInsideCozy) approach, which automatically rebuilt when the future resolved, the new synchronous isInContainer reads have no rebuild trigger, meaning the app bar may remain visible inside a Cozy container for the entire session if no parent rebuild occurs.
🐛 Proposed fix — await the initialization chain before rendering
if (PlatformInfos.isWeb) {
- CozyConfigManager()
- .injectCozyScript(AppConfig.cozyExternalBridgeVersion)
- .then((value) => CozyConfigManager().initialize(validUrlSuffixes: AppConfig.cozyExternalBridgeAllowlist));
+ await CozyConfigManager()
+ .injectCozyScript(AppConfig.cozyExternalBridgeVersion)
+ .then((_) => CozyConfigManager().initialize(
+ validUrlSuffixes: AppConfig.cozyExternalBridgeAllowlist,
+ ));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/main.dart` around lines 68 - 71, The app is starting the UI before
CozyConfigManager finishes setup, causing isInContainer reads (used by
AdaptiveScaffoldAppBar) to be stale; change the startup sequence so
injectCozyScript(...) completes and CozyConfigManager().initialize(...) is
awaited before calling startGui(clients). Locate the chain that calls
CozyConfigManager().injectCozyScript and CozyConfigManager().initialize and make
startGui(clients) run only after the Future returned by
injectCozyScript().then(...) resolves (or use async/await), ensuring
isInContainer is set before the first widget build.
| if(CozyConfigManager().isInContainer) { | ||
| return const SizedBox(); | ||
| } | ||
|
|
||
| return const SizedBox(); | ||
| }, | ||
| ); | ||
| return child; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CozyConfigManager class definition
fd "cozy_config_manager" --type fRepository: linagora/twake-on-matrix
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for CozyConfigManager class definition and factory constructor
rg -n "class CozyConfigManager" --type dart -A 10Repository: linagora/twake-on-matrix
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Search for isInContainer property and initialize method
rg -n "isInContainer" --type dart -B 2 -A 2Repository: linagora/twake-on-matrix
Length of output: 1478
🏁 Script executed:
#!/bin/bash
# Search for initialize method in CozyConfigManager context
rg -n "initialize\(\)" --type dart -B 2 -A 5 | head -100Repository: linagora/twake-on-matrix
Length of output: 4901
🏁 Script executed:
#!/bin/bash
# Find where CozyConfigManager is imported from
rg -n "import.*[Cc]ozy|from.*cozy" --type dart | head -20Repository: linagora/twake-on-matrix
Length of output: 422
🏁 Script executed:
#!/bin/bash
# Search for CozyConfigManager.initialize() calls
rg -n "CozyConfigManager\(\)\.initialize" --type dart -B 2 -A 2Repository: linagora/twake-on-matrix
Length of output: 359
🏁 Script executed:
#!/bin/bash
# Search main.dart for initialize setup
fd "main.dart" --type fRepository: linagora/twake-on-matrix
Length of output: 81
🏁 Script executed:
#!/bin/bash
# Read main.dart to understand the initialization sequence and race condition
cat -n lib/main.dart | sed -n '50,100p'Repository: linagora/twake-on-matrix
Length of output: 2355
Race condition in main.dart: CozyConfigManager().initialize() is not awaited before GUI rendering.
The initialization chain on lines 68–70 of main.dart is fire-and-forget (chained with .then()), while startGui() on line 77 is called immediately without waiting. This means the widget at line 53–57 will read CozyConfigManager().isInContainer during the first build, but the initialize() call likely hasn't completed yet, causing the check to read an uninitialized value.
Fix: Add await to the initialization chain in main.dart:
if (PlatformInfos.isWeb) {
await CozyConfigManager()
.injectCozyScript(AppConfig.cozyExternalBridgeVersion)
.then((value) => CozyConfigManager().initialize(validUrlSuffixes: AppConfig.cozyExternalBridgeAllowlist));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/widgets/layouts/adaptive_layout/adaptive_scaffold_appbar.dart` around
lines 53 - 57, The code races because CozyConfigManager().initialize(...)
(called via .then()) isn’t awaited before startGui() runs, so
adaptive_scaffold_appbar's check CozyConfigManager().isInContainer can read an
uninitialized value; fix by awaiting the initialization sequence on the
PlatformInfos.isWeb branch — await
CozyConfigManager().injectCozyScript(AppConfig.cozyExternalBridgeVersion) and
then await CozyConfigManager().initialize(validUrlSuffixes:
AppConfig.cozyExternalBridgeAllowlist) (instead of chaining with .then()), and
only call startGui() after those awaits complete.
There was a problem hiding this comment.
@hoangdat I agree it is an issue but I suppose it was built on purpose to not await it right? Any thoughts?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -193,6 +194,11 @@ abstract class AppConfig { | |||
| defaultValue: ConfigurationSaas.homeserver, | |||
| ); | |||
|
|
|||
| static const String _cozyExternalBridgeAllowlistEnv = String.fromEnvironment( | |||
| 'COZY_EXTERNAL_BRIDGE_ALLOWLIST', | |||
| defaultValue: '.twake.app,.lin-saas.com,.lin-saas.dev', | |||
There was a problem hiding this comment.
A const should be defined for these default values, for reuse.
tddang-linagora
left a comment
tddang-linagora
left a comment
There was a problem hiding this comment.
- Please include the
pubspec.yamlandpubspec.lockin this PR, which contain the changes made inlinagora-design-flutter(elseCozyConfigManager().isInContainerwon't compile).
| if (json['cozy_external_bridge_allowlist'] is List<String> && | ||
| json['cozy_external_bridge_allowlist'].isNotEmpty) { | ||
| cozyExternalBridgeAllowlist = json['cozy_external_bridge_allowlist']; | ||
| } |
There was a problem hiding this comment.
Create a method to initialize config early, put only the cozy config initialization there, then call as soon as the app start.
zatteo
force-pushed
the
new-setup-bridge
branch
from
e66978e to
9adfcb3
Compare
What do you mean? Can you detail? |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/main.dart (1)
68-71:⚠️ Potential issue | 🟠 MajorAwait Cozy initialization before GUI startup to prevent stale container state.
At Line 68–71, the Future chain is fire-and-forget;
startGui(clients)at Line 77 can run beforeinitialize(...)completes, so synchronousisInContainerreads may be wrong on initial render.Proposed fix
if (PlatformInfos.isWeb) { - CozyConfigManager() - .injectCozyScript(AppConfig.cozyExternalBridgeVersion) - .then((value) => CozyConfigManager().initialize(validUrlSuffixes: AppConfig.cozyExternalBridgeAllowlist)); + await CozyConfigManager().injectCozyScript( + AppConfig.cozyExternalBridgeVersion, + ); + await CozyConfigManager().initialize( + validUrlSuffixes: AppConfig.cozyExternalBridgeAllowlist, + ); }#!/bin/bash # Verify startup ordering and container-state read sites sed -n '60,85p' lib/main.dart rg -nP --type=dart '\bisInContainer\b' lib -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/main.dart` around lines 68 - 71, The GUI can start before CozyConfigManager finishes, causing stale isInContainer reads; change the fire-and-forget chain so that injectCozyScript(...).then(...initialize...) is awaited before calling startGui(clients). Specifically, await CozyConfigManager().injectCozyScript(AppConfig.cozyExternalBridgeVersion) and then await CozyConfigManager().initialize(validUrlSuffixes: AppConfig.cozyExternalBridgeAllowlist) (or await the combined Future) and only after those awaits call startGui(clients); also scan sites using isInContainer to ensure they read state after this initialization completes.
🧹 Nitpick comments (1)
pubspec.yaml (1)
23-23: Consider pinninglinagora_design_flutterto commit SHA for explicit release stability.Using a branch ref (
feat/update-cozy-external-bridge-setup) at Line 23 and Line 335 relies onpubspec.lockto capture the resolved commit SHA (4331015b1f10c4b2033493e756ebf86398185d8d). While this is standard practice in Dart/Flutter projects, explicitly pinning to the commit SHA inpubspec.yamlmakes release intent clearer and removes the need to verify lock file consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pubspec.yaml` at line 23, Update the linagora_design_flutter dependency entry in pubspec.yaml to pin to the specific commit SHA instead of the branch name: locate the linagora_design_flutter dependency block (the line containing "ref: feat/update-cozy-external-bridge-setup") and replace the branch ref with the resolved commit SHA (e.g., "ref: 4331015b1f10c4b2033493e756ebf86398185d8d") so the dependency is explicitly fixed to that commit for release stability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/main.dart`:
- Around line 68-71: The GUI can start before CozyConfigManager finishes,
causing stale isInContainer reads; change the fire-and-forget chain so that
injectCozyScript(...).then(...initialize...) is awaited before calling
startGui(clients). Specifically, await
CozyConfigManager().injectCozyScript(AppConfig.cozyExternalBridgeVersion) and
then await CozyConfigManager().initialize(validUrlSuffixes:
AppConfig.cozyExternalBridgeAllowlist) (or await the combined Future) and only
after those awaits call startGui(clients); also scan sites using isInContainer
to ensure they read state after this initialization completes.
---
Nitpick comments:
In `@pubspec.yaml`:
- Line 23: Update the linagora_design_flutter dependency entry in pubspec.yaml
to pin to the specific commit SHA instead of the branch name: locate the
linagora_design_flutter dependency block (the line containing "ref:
feat/update-cozy-external-bridge-setup") and replace the branch ref with the
resolved commit SHA (e.g., "ref: 4331015b1f10c4b2033493e756ebf86398185d8d") so
the dependency is explicitly fixed to that commit for release stability.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
lib/config/app_config.dartlib/main.dartlib/widgets/layouts/adaptive_layout/adaptive_scaffold_appbar.dartlib/widgets/local_notifications_extension.dartlib/widgets/matrix.dartpubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/config/app_config.dart
- lib/widgets/matrix.dart
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2889 |
3 participants
Applying changes from here linagora/linagora-design-flutter#69
I will need to update pubspec.
Summary by CodeRabbit
Configuration Updates
Updates
Chores