TF-1302 Allow selecting emojis while drafting mail#4140
Open
TF-1302 Allow selecting emojis while drafting mail#4140
Conversation
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4140. |
hoangdat
reviewed
Nov 13, 2025
Member
|
dab246
force-pushed
the
enhancement/tf-1302-allow-selecting-emojis-while-drafting-mail
branch
from
305013c to
97f26e5
Compare
Member
Author
Updated & rebase done |
dab246
force-pushed
the
enhancement/tf-1302-allow-selecting-emojis-while-drafting-mail
branch
from
97f26e5 to
ec0300b
Compare
dab246
force-pushed
the
enhancement/tf-1302-allow-selecting-emojis-while-drafting-mail
branch
from
ec0300b to
7f1034b
Compare
Collaborator
Emoji in the picker is different from the emoji in the composer when picked. |
…ndardize workflows
…mposer when picked
dab246
force-pushed
the
enhancement/tf-1302-allow-selecting-emojis-while-drafting-mail
branch
from
7f1034b to
4bc0a0a
Compare
Member
Author
Fixed Screen.Recording.2026-02-27.at.11.59.37.mov |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds emoji support and a reactions feature across the app and updates CI/build tooling. Introduces EmojiButton widget, emoji picker integration in web composer, RichTextWebController.insertEmoji, AssetManager and emoji data loading, new emoji font and localization key, and image/font resource entries. Implements reactions domain: datasource, cache manager, repository, interactors, states, extensions, and GetX bindings with composer-scoped registration. Multiple GitHub Actions/workflow, Dockerfile, and pubspec changes enable SSH-based private git deps, prebuild steps across modules, platform-specific setup, and artifact handling. Also adds small UI constants and image path getters. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/image.yaml (1)
16-71:⚠️ Potential issue | 🔴 CriticalAdd missing repository checkout step and tag event trigger.
Both the
build-dev-imageandbuild-release-imagejobs are missing theactions/checkout@v4step. Thedocker/build-push-action@v5requires access to the Dockerfile and source code, but without checkout, the runner's working directory is empty and the build will fail. This pattern is consistent across all other workflows in the repository.Additionally, the
build-release-imagejob will never trigger because the workflow only listens topush: branches: - "master". To enable the release job on version tags, addpush: tags:to the trigger configuration.Proposed fixes
- Add checkout as the first step in both jobs:
steps: + - name: Checkout repository + uses: actions/checkout@v4 + - name: Set up SSH for private Git dependencies uses: webfactory/ssh-agent@v0.9.0
- Update the workflow trigger to include tag events:
on: push: branches: - "master" + tags: + - "v*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/image.yaml around lines 16 - 71, The jobs build-dev-image and build-release-image are missing the repository checkout so docker/build-push-action@v5 has no Dockerfile/source to build; add an actions/checkout@v4 step as the first step in each job (before Set up SSH and Build and push image). Also update the workflow trigger to include push: tags: (so build-release-image can run on version tags) alongside the existing push: branches: - "master".
🧹 Nitpick comments (2)
.github/workflows/build.yaml (1)
103-110: Consider separating artifact paths by platform.Both APK and IPA paths are listed, but only one will exist per matrix run. While
upload-artifact@v4handles missing files gracefully (it won't fail), you might see warnings in the logs. This is a minor observation and the current approach works.💡 Optional: Platform-specific artifact paths
- name: Upload artifacts uses: actions/upload-artifact@v4 with: name: tmail-dev-${{ matrix.os }} path: | - build/app/outputs/flutter-apk/app-debug.apk - ios/Runner.ipa + ${{ matrix.os == 'android' && 'build/app/outputs/flutter-apk/app-debug.apk' || 'ios/Runner.ipa' }}Or use conditional steps:
- name: Upload Android artifact if: matrix.os == 'android' uses: actions/upload-artifact@v4 with: name: tmail-dev-android path: build/app/outputs/flutter-apk/app-debug.apk - name: Upload iOS artifact if: matrix.os == 'ios' uses: actions/upload-artifact@v4 with: name: tmail-dev-ios path: ios/Runner.ipa🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yaml around lines 103 - 110, The current "Upload artifacts" step lists both Android and iOS paths and can emit warnings when one is missing; split this into two conditional upload steps: replace the single "Upload artifacts" step with "Upload Android artifact" that runs only when matrix.os == 'android' and uploads build/app/outputs/flutter-apk/app-debug.apk, and "Upload iOS artifact" that runs only when matrix.os == 'ios' and uploads ios/Runner.ipa, giving each a platform-specific artifact name (e.g., tmail-dev-android, tmail-dev-ios) so uploads only reference existing files and avoid warnings.lib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dart (1)
24-35: Consider usingFuture<void>return type for async method.The
storeRecentReactionsmethod is declared asasyncbut returnsvoid. While this works, it's a Dart best practice to returnFuture<void>for async methods to allow callers to await the operation if needed and to properly propagate any errors.♻️ Suggested change
- void storeRecentReactions(String emoji) async { + Future<void> storeRecentReactions(String emoji) async {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dart` around lines 24 - 35, The method storeRecentReactions is declared async but returns void; change its signature to Future<void> storeRecentReactions(String emoji) so callers can await it and errors propagate; update any overridden/implemented declarations or typedefs that reference storeRecentReactions, and keep the existing body that uses AssetManager().emojiData, getBinding<StoreRecentReactionsInteractor>(tag: composerId), and consumeState(interactor.execute(...)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/analyze-test.yaml:
- Around line 37-40: Replace mutable action tags with immutable commit SHAs:
locate usages of webfactory/ssh-agent@v0.9.0 and the other action referenced as
`@v1` in the workflow and update each to the corresponding full 40-character
commit SHA for that action (e.g., webfactory/ssh-agent@<40-char-sha>); ensure
you use the commit SHA from the action's repository (not a fork) and update both
occurrences so the CI uses pinned, immutable references.
In `@lib/features/base/widget/emoji/emoji_button.dart`:
- Around line 45-47: The fixed _dialogWidth causes the picker to render
off-screen on narrow viewports; replace usages of the static 400 width by
computing a responsive width at build time (e.g., clamp the width to
Math.min(MediaQuery.of(context).size.width - horizontalMargin, _dialogWidth) or
use LayoutBuilder constraints) and ensure computed start/left positions are
clamped to >= 0 so the picker never has a negative start; update all places
referencing _dialogWidth (including the build/positioning logic where start is
computed and the other occurrences around the blocks referenced) to use the
responsive computedWidth and keep _dialogHeight unchanged unless you also want
it responsive.
In `@lib/features/reactions/domain/extensions/list_reactions_extesion.dart`:
- Around line 1-16: Rename the file to fix the filename typo from
list_reactions_extesion.dart to list_reactions_extension.dart so imports and
searches match the extension declaration; update any import sites that reference
the old filename accordingly. The extension declaration (ListReactionsExtension)
and its member combineRecentReactions and constant maxRecentReactionsSize remain
unchanged—only the filename and any import paths should be updated to the
corrected spelling.
In `@lib/main/utils/asset_manager.dart`:
- Around line 44-47: The early return in loadEmojiData ignores the incoming
version once _isEmojiDataLoaded is true, causing stale emoji data to be reused;
update loadEmojiData to track the loaded emoji version (e.g., add a private
_emojiDataVersion) and compare it against the incoming version, and only return
early when _isEmojiDataLoaded is true AND the versions match—otherwise clear or
replace _emojiData and proceed to load the requested version so calls with a
different version reload correctly.
---
Outside diff comments:
In @.github/workflows/image.yaml:
- Around line 16-71: The jobs build-dev-image and build-release-image are
missing the repository checkout so docker/build-push-action@v5 has no
Dockerfile/source to build; add an actions/checkout@v4 step as the first step in
each job (before Set up SSH and Build and push image). Also update the workflow
trigger to include push: tags: (so build-release-image can run on version tags)
alongside the existing push: branches: - "master".
---
Nitpick comments:
In @.github/workflows/build.yaml:
- Around line 103-110: The current "Upload artifacts" step lists both Android
and iOS paths and can emit warnings when one is missing; split this into two
conditional upload steps: replace the single "Upload artifacts" step with
"Upload Android artifact" that runs only when matrix.os == 'android' and uploads
build/app/outputs/flutter-apk/app-debug.apk, and "Upload iOS artifact" that runs
only when matrix.os == 'ios' and uploads ios/Runner.ipa, giving each a
platform-specific artifact name (e.g., tmail-dev-android, tmail-dev-ios) so
uploads only reference existing files and avoid warnings.
In
`@lib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dart`:
- Around line 24-35: The method storeRecentReactions is declared async but
returns void; change its signature to Future<void> storeRecentReactions(String
emoji) so callers can await it and errors propagate; update any
overridden/implemented declarations or typedefs that reference
storeRecentReactions, and keep the existing body that uses
AssetManager().emojiData, getBinding<StoreRecentReactionsInteractor>(tag:
composerId), and consumeState(interactor.execute(...)) unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
assets/fonts/fallback/NotoColorEmoji-Regular.ttfis excluded by!**/*.ttfassets/images/ic_emoji.svgis excluded by!**/*.svgassets/images/ic_search_emoji_empty.svgis excluded by!**/*.svgcozy/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.github/workflows/analyze-test.yaml.github/workflows/build.yaml.github/workflows/gh-pages.yaml.github/workflows/image.yaml.github/workflows/patrol-integration-test.yaml.github/workflows/release.yamlDockerfilecore/lib/presentation/constants/constants_ui.dartcore/lib/presentation/extensions/color_extension.dartcore/lib/presentation/resources/image_paths.dartcore/lib/presentation/utils/theme_utils.dartcore/lib/utils/html/html_template.dartcozy/pubspec.yamllib/features/base/widget/emoji/emoji_button.dartlib/features/composer/presentation/composer_bindings.dartlib/features/composer/presentation/composer_view_web.dartlib/features/composer/presentation/controller/rich_text_web_controller.dartlib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dartlib/features/composer/presentation/widgets/web/bottom_bar_composer_widget.dartlib/features/reactions/data/datasource/reactions_datasource.dartlib/features/reactions/data/datasource_impl/reactions_datasource_impl.dartlib/features/reactions/data/local/reaction_cache_manager.dartlib/features/reactions/data/repository/reactions_repository_impl.dartlib/features/reactions/domain/extensions/list_reactions_extesion.dartlib/features/reactions/domain/repository/reactions_repository.dartlib/features/reactions/domain/state/get_recent_reactions_state.dartlib/features/reactions/domain/state/store_recent_reactions_state.dartlib/features/reactions/domain/usecase/get_recent_reactions_interactor.dartlib/features/reactions/domain/usecase/store_recent_reactions_interactor.dartlib/features/reactions/presentation/reactions_interactor_bindings.dartlib/l10n/intl_messages.arblib/main/bindings/local/local_bindings.dartlib/main/localizations/app_localizations.dartlib/main/main_entry.dartlib/main/utils/asset_manager.dartlib/main/utils/asset_preloader.dartpubspec.yaml
💤 Files with no reviewable changes (1)
- lib/main/utils/asset_preloader.dart
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/analyze-test.yaml (1)
2-3:⚠️ Potential issue | 🟠 MajorAdd job condition to skip secret-dependent setup on fork PRs.
This workflow runs on
pull_requestand unconditionally requiresSSH_KEY(line 40) andFIREBASE_ENV(line 61) secrets. Forked PRs won't have access to these secrets, causing the SSH setup and Firebase configuration steps to fail.Guard the job to run only on same-repository PRs:
Add job condition
jobs: analyze-test: + if: github.event.pull_request.head.repo.full_name == github.repository runs-on: ubuntu-latestAlso applies to: lines 36-40, 58-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/analyze-test.yaml around lines 2 - 3, The workflow unconditionally uses secrets SSH_KEY and FIREBASE_ENV which fail on forked PRs; add a job-level condition to each job that performs SSH setup (references SSH_KEY) and Firebase setup (references FIREBASE_ENV) so they only run for same-repository pull requests by checking the PR head repo equals the base repo (e.g., compare github.event.pull_request.head.repo.full_name to github.repository) and apply this condition to the jobs/steps that reference SSH_KEY and FIREBASE_ENV.
🧹 Nitpick comments (5)
lib/features/base/widget/emoji/emoji_button.dart (1)
83-85: CallonPickerOpenonly after the picker is actually inserted.Right now, the callback fires even when preconditions fail and no dialog appears.
💡 Proposed fix
Future<void> _openDialog() async { - widget.onPickerOpen(); - if (!mounted || _isDialogVisible) return; @@ final overlay = Overlay.maybeOf(context, rootOverlay: true); if (!mounted || overlay == null) return; overlay.insert(_overlayEntry!); + widget.onPickerOpen();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/base/widget/emoji/emoji_button.dart` around lines 83 - 85, The callback widget.onPickerOpen is being invoked before dialog preconditions are verified and before the picker is actually inserted; change _openDialog so you only call widget.onPickerOpen after you have successfully invoked the picker presentation (e.g. after calling showDialog/showModalBottomSheet or Navigator.push that inserts the route) and only when those preconditions pass — move the call from the top of _openDialog to immediately after the showDialog/showModalBottomSheet invocation (but before awaiting its returned Future) so it runs only when the picker is actually shown.lib/features/reactions/domain/usecase/store_recent_reactions_interactor.dart (1)
19-20: Avoid the redundant list allocation before combining reactions.
combineRecentReactionsalready creates a mutable copy, soList<String>.from(reactions)is unnecessary here.♻️ Proposed cleanup
- final updatedReactions = - List<String>.from(reactions).combineRecentReactions(emojiId); + final updatedReactions = reactions.combineRecentReactions(emojiId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/reactions/domain/usecase/store_recent_reactions_interactor.dart` around lines 19 - 20, The code creates an unnecessary copy with List<String>.from(reactions) before calling combineRecentReactions; remove the redundant allocation and call reactions.combineRecentReactions(emojiId) directly (ensure the variable passed to combineRecentReactions is already a List<String> or compatible type). Update the assignment in the StoreRecentReactionsInteractor (where updatedReactions is computed) to use reactions.combineRecentReactions(emojiId) since combineRecentReactions itself makes the mutable copy..github/workflows/build.yaml (1)
103-110: Split artifact upload by platform to avoid noisy missing-file cases.The current matrix upload includes both Android and iOS paths in every run. This weakens failure clarity when artifacts are actually missing.
Suggested refactor
- - name: Upload artifacts - uses: actions/upload-artifact@v4 - with: - name: tmail-dev-${{ matrix.os }} - path: | - build/app/outputs/flutter-apk/app-debug.apk - ios/Runner.ipa + - name: Upload Android artifact + if: matrix.os == 'android' + uses: actions/upload-artifact@v4 + with: + name: tmail-dev-android + path: build/app/outputs/flutter-apk/app-debug.apk + if-no-files-found: error + + - name: Upload iOS artifact + if: matrix.os == 'ios' + uses: actions/upload-artifact@v4 + with: + name: tmail-dev-ios + path: ios/Runner.ipa + if-no-files-found: error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yaml around lines 103 - 110, The "Upload artifacts" step currently uploads both Android and iOS paths for every matrix.os, causing noisy missing-file failures; update the actions/upload-artifact step (the step named "Upload artifacts") to split into two conditional steps that run based on matrix.os (e.g., one with if: matrix.os == 'ubuntu-latest' or similar that uploads build/app/outputs/flutter-apk/app-debug.apk and names the artifact using matrix.os, and another with if: matrix.os == 'macos' that uploads ios/Runner.ipa and names it accordingly), so each runner only tries to upload the platform-specific artifact..github/workflows/image.yaml (1)
1-4: Release job is unreachable with the current workflow trigger.This workflow only triggers on
masterbranch pushes, butbuild-release-imageis tag-gated. If release builds are intended here, add tag triggers (or remove the dead job).Suggested fix (if release job should run in this workflow)
on: push: branches: - "master" + tags: + - "v*.*.*"Also applies to: 76-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/image.yaml around lines 1 - 4, The workflow is only triggered for pushes to branch "master" while the job build-release-image is conditioned on tags and thus never runs; update the workflow trigger under the on: section to include tag pushes (e.g., add a push: tags: entry for your release tag pattern) or remove/disable the build-release-image job if releases are not intended in this workflow; locate the on: block and the job named build-release-image and either add the tag trigger to on: or remove the tag-gated job so the workflow and job logic are consistent..github/workflows/gh-pages.yaml (1)
11-13: Consider canceling stale in-progress PR deployments.For PR environments, setting
cancel-in-progress: trueusually improves turnaround and reduces wasted runner time.Suggested update
concurrency: group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gh-pages.yaml around lines 11 - 13, Add the cancel-in-progress flag under the existing concurrency block so GitHub cancels stale in-progress runs for the same group; specifically update the concurrency settings (the concurrency: block and its group: expression `${{ github.workflow }}-${{ github.ref }}`) to include `cancel-in-progress: true` to enable automatic cancellation of older PR deployment jobs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/image-sentry.yaml:
- Around line 27-38: The Docker build step using docker/build-push-action@v5
must forward the SSH agent so RUN --mount=type=ssh in the Dockerfile can access
private deps; edit the docker/build-push-action@v5 steps (the
"docker/build-push-action@v5" uses in the workflow) and add the parameter ssh:
default under the step (e.g., ensure the step block that uses
"docker/build-push-action@v5" includes a top-level "with:" entry or top-level
"ssh: default" per action syntax), and apply the same addition to the second
occurrence of that action (the other docker/build-push-action@v5 step around
lines 72-93) so both builds forward the configured SSH socket.
In `@lib/features/base/widget/emoji/emoji_button.dart`:
- Around line 64-73: The _loadRecentEmoji method currently invokes
widget.onRecentEmojiSelected which can throw and break the picker open flow;
update _loadRecentEmoji to call widget.onRecentEmojiSelected inside a try/catch,
catching any exceptions and setting _recentEmoji = Future.value(null) on error
(and optionally logging the exception) so failures in onRecentEmojiSelected are
guarded and do not propagate.
- Around line 116-227: After awaiting _loadRecentEmoji(), the code must re-check
widget lifecycle to avoid operating on a disposed state; add an immediate guard
like if (!mounted) return; before constructing/assigning _overlayEntry and
before calling Overlay.maybeOf(...).insert(...),
_animationController.forward(...), and setState(() => _isDialogVisible = true);
ensure all uses of _overlayEntry, _animationController, and setState are only
executed when mounted to prevent animation/overlay calls on a disposed widget.
In
`@lib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dart`:
- Around line 24-27: In storeRecentReactions, ensure emoji data is loaded before
calling AssetManager().emojiData.getIdByEmoji: if AssetManager().emojiData is
null, call and await the AssetManager.loadEmojiData() (or equivalent
initializer) and then re-check emojiData; if loading still fails, log an error
and return early so you don't silently skip or NPE. Update the method
(storeRecentReactions) to await the loader, re-resolve emojiId, and include a
clear log message when emoji data cannot be loaded.
In `@lib/features/reactions/domain/extensions/list_reactions_extension.dart`:
- Around line 7-8: The code currently calls result.remove(emojiId) which only
removes the first match, leaving duplicates; replace that call with a removal of
all occurrences (e.g., result.removeWhere((e) => e == emojiId)) before
prepending with result.insert(0, emojiId) so the list has a single leading
instance of emojiId; update the List extension in list_reactions_extension.dart
to use result.removeWhere instead of result.remove.
---
Outside diff comments:
In @.github/workflows/analyze-test.yaml:
- Around line 2-3: The workflow unconditionally uses secrets SSH_KEY and
FIREBASE_ENV which fail on forked PRs; add a job-level condition to each job
that performs SSH setup (references SSH_KEY) and Firebase setup (references
FIREBASE_ENV) so they only run for same-repository pull requests by checking the
PR head repo equals the base repo (e.g., compare
github.event.pull_request.head.repo.full_name to github.repository) and apply
this condition to the jobs/steps that reference SSH_KEY and FIREBASE_ENV.
---
Nitpick comments:
In @.github/workflows/build.yaml:
- Around line 103-110: The "Upload artifacts" step currently uploads both
Android and iOS paths for every matrix.os, causing noisy missing-file failures;
update the actions/upload-artifact step (the step named "Upload artifacts") to
split into two conditional steps that run based on matrix.os (e.g., one with if:
matrix.os == 'ubuntu-latest' or similar that uploads
build/app/outputs/flutter-apk/app-debug.apk and names the artifact using
matrix.os, and another with if: matrix.os == 'macos' that uploads ios/Runner.ipa
and names it accordingly), so each runner only tries to upload the
platform-specific artifact.
In @.github/workflows/gh-pages.yaml:
- Around line 11-13: Add the cancel-in-progress flag under the existing
concurrency block so GitHub cancels stale in-progress runs for the same group;
specifically update the concurrency settings (the concurrency: block and its
group: expression `${{ github.workflow }}-${{ github.ref }}`) to include
`cancel-in-progress: true` to enable automatic cancellation of older PR
deployment jobs.
In @.github/workflows/image.yaml:
- Around line 1-4: The workflow is only triggered for pushes to branch "master"
while the job build-release-image is conditioned on tags and thus never runs;
update the workflow trigger under the on: section to include tag pushes (e.g.,
add a push: tags: entry for your release tag pattern) or remove/disable the
build-release-image job if releases are not intended in this workflow; locate
the on: block and the job named build-release-image and either add the tag
trigger to on: or remove the tag-gated job so the workflow and job logic are
consistent.
In `@lib/features/base/widget/emoji/emoji_button.dart`:
- Around line 83-85: The callback widget.onPickerOpen is being invoked before
dialog preconditions are verified and before the picker is actually inserted;
change _openDialog so you only call widget.onPickerOpen after you have
successfully invoked the picker presentation (e.g. after calling
showDialog/showModalBottomSheet or Navigator.push that inserts the route) and
only when those preconditions pass — move the call from the top of _openDialog
to immediately after the showDialog/showModalBottomSheet invocation (but before
awaiting its returned Future) so it runs only when the picker is actually shown.
In
`@lib/features/reactions/domain/usecase/store_recent_reactions_interactor.dart`:
- Around line 19-20: The code creates an unnecessary copy with
List<String>.from(reactions) before calling combineRecentReactions; remove the
redundant allocation and call reactions.combineRecentReactions(emojiId) directly
(ensure the variable passed to combineRecentReactions is already a List<String>
or compatible type). Update the assignment in the StoreRecentReactionsInteractor
(where updatedReactions is computed) to use
reactions.combineRecentReactions(emojiId) since combineRecentReactions itself
makes the mutable copy.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/analyze-test.yaml.github/workflows/build.yaml.github/workflows/gh-pages.yaml.github/workflows/image-sentry.yaml.github/workflows/image.yaml.github/workflows/patrol-integration-test.yaml.github/workflows/release.yamllib/features/base/widget/emoji/emoji_button.dartlib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dartlib/features/reactions/domain/extensions/list_reactions_extension.dartlib/features/reactions/domain/usecase/store_recent_reactions_interactor.dartlib/main/utils/asset_manager.dart
lib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dart
Outdated
Show resolved
Hide resolved
lib/features/reactions/domain/extensions/list_reactions_extension.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.github/workflows/gh-pages.yaml (1)
67-69: Consider the build time impact of full pub cache clean.
flutter pub cache cleanremoves all cached packages, requiring a full re-download of every dependency on each PR build. While this ensures no stale SSH clones persist, it may significantly increase build times.If the goal is specifically to clear stale SSH-based dependencies, consider a more targeted approach or document that this trade-off is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/gh-pages.yaml around lines 67 - 69, The workflow step named "Clean pub cache" currently runs the expensive global command `flutter pub cache clean`; replace this with a targeted cleanup (e.g., remove only the git/SSH clone cache under the pub cache or specific problematic repo directories in .pub-cache/git) or gate the full clean behind an explicit env flag so PR builds don't always re-download all deps; update the step (referencing the "Clean pub cache" step and the `flutter pub cache clean` invocation) to either perform `rm -rf $FLUTTER_HOME/.pub-cache/git/<problem-repo>`-style removals or check an env var (e.g., CLEAN_PUB_CACHE=true) and document the trade-off in the workflow comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/analyze-test.yaml:
- Around line 79-82: The "Upload test reports" step currently uses a conditional
of "success() || failure()" which won't run on cancellations; update the step's
if condition to "always()" so the artifacts are uploaded in all outcomes. Locate
the step with name "Upload test reports" and uses "actions/upload-artifact@v4"
and replace the conditional expression accordingly.
- Around line 14-17: Add an explicit permissions block for the analyze-test job
to enforce least-privilege GITHUB_TOKEN scopes: inside the job named
analyze-test, add a permissions mapping that grants only the minimal scopes
required (e.g., set GITHUB_TOKEN permissions such as contents: read and checks:
write or read as needed) and avoid leaving permissions unspecified; expand the
permissions only if a specific step later requires additional scopes.
In `@lib/features/base/widget/emoji/emoji_button.dart`:
- Around line 94-100: Clamp computed available and dialog dimensions to
non-negative values: ensure availableHeight = max(0, screenSize.height - 32) and
availableWidth = max(0, screenSize.width - 16) (or return early if either is <=
0), then compute dialogHeight = min(_dialogHeight, availableHeight) and
dialogWidth = min(_dialogWidth, availableWidth) so neither dialogHeight nor
dialogWidth can be negative; also ensure start (computed from buttonPosition,
buttonSize and dialogWidth) is adjusted after clamping (and apply the same
non-negative clamping/early-return fix for the other occurrence around lines
referencing dialogHeight/dialogWidth at 136-137).
- Around line 224-230: Wrap the widget.onPickerOpen() call in a try/catch so any
thrown error doesn't leave the overlay inserted but _isDialogVisible false;
after inserting _overlayEntry (overlay.insert(_overlayEntry!)) ensure either
_isDialogVisible is set to true in a finally block or update _closeDialog() to
guard on _overlayEntry != null instead of relying solely on _isDialogVisible.
Specifically, in the show/open flow around overlay.insert(_overlayEntry!),
widget.onPickerOpen(), and await _animationController.forward(from: 0) handle
exceptions from widget.onPickerOpen() (and similarly in the second block at
lines 233-239) and make close logic check _overlayEntry presence (or set
_isDialogVisible true before calling widget.onPickerOpen()) so the overlay can
always be closed even if the callback fails.
In
`@lib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dart`:
- Around line 11-15: insertEmojiToEditor currently calls storeRecentReactions as
a silent fire‑and‑forget which can throw; modify storeRecentReactions to
internally catch and log any exceptions around its awaited work (wrap its
body/awaits in try/catch and emit a clear log via your logger), and make the
fire‑and‑forget in insertEmojiToEditor explicit (e.g., call it with your
project’s unawaited helper or add a clear comment) so failures are handled
inside storeRecentReactions and not dropped.
In
`@lib/features/reactions/domain/usecase/store_recent_reactions_interactor.dart`:
- Around line 17-22: The read→modify→write in StoreRecentReactionsInteractor
(calling _repository.getRecentReactions, combineRecentReactions(emojiId), then
_repository.storeRecentReactions) is racy; change it to an atomic operation in
the repository (e.g., add a method like appendRecentReaction/appendById that
takes emojiId and performs the read+merge+write under a single DB/transactional
operation) or protect the block with a per-composer mutex/queue helper (call it
from the interactor) so concurrent invocations cannot overwrite each other;
update the interactor to call the new atomic repository method (or
acquire/release the mutex around the existing sequence) and remove the
non-atomic get/transform/store sequence.
---
Nitpick comments:
In @.github/workflows/gh-pages.yaml:
- Around line 67-69: The workflow step named "Clean pub cache" currently runs
the expensive global command `flutter pub cache clean`; replace this with a
targeted cleanup (e.g., remove only the git/SSH clone cache under the pub cache
or specific problematic repo directories in .pub-cache/git) or gate the full
clean behind an explicit env flag so PR builds don't always re-download all
deps; update the step (referencing the "Clean pub cache" step and the `flutter
pub cache clean` invocation) to either perform `rm -rf
$FLUTTER_HOME/.pub-cache/git/<problem-repo>`-style removals or check an env var
(e.g., CLEAN_PUB_CACHE=true) and document the trade-off in the workflow comment.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/analyze-test.yaml.github/workflows/gh-pages.yaml.github/workflows/image-sentry.yamllib/features/base/widget/emoji/emoji_button.dartlib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dartlib/features/reactions/domain/extensions/list_reactions_extension.dartlib/features/reactions/domain/usecase/store_recent_reactions_interactor.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/reactions/domain/extensions/list_reactions_extension.dart
lib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dart
Show resolved
Hide resolved
lib/features/reactions/domain/usecase/store_recent_reactions_interactor.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/features/base/widget/emoji/emoji_button.dart (1)
94-97:⚠️ Potential issue | 🟡 MinorClamp computed dialog dimensions to non-negative bounds.
On extremely small viewports,
screenSize - margincan go negative, and that can propagate invalidwidth/heightinto the dialog container.💡 Proposed fix
- final double availableHeight = screenSize.height - 32; + final double availableHeight = math.max(0.0, screenSize.height - 32); final double dialogHeight = math.min(_dialogHeight, availableHeight); - final availableWidth = screenSize.width - 16; + final double availableWidth = math.max(0.0, screenSize.width - 16); final dialogWidth = math.min(_dialogWidth, availableWidth); + if (dialogWidth == 0 || dialogHeight == 0) return;Also applies to: 134-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/base/widget/emoji/emoji_button.dart` around lines 94 - 97, Clamp the computed sizes to non-negative values: ensure availableHeight and availableWidth are max(screenSize.height - 32, 0) and max(screenSize.width - 16, 0), then compute dialogHeight = math.min(_dialogHeight, availableHeight) and dialogWidth = math.min(_dialogWidth, availableWidth) (or clamp dialogHeight/dialogWidth again to >= 0). Update the same logic at the other occurrence (lines referencing _dialogHeight/_dialogWidth and availableWidth/availableHeight around 134-135) so no negative dimensions are passed into the dialog container.
🧹 Nitpick comments (2)
.github/workflows/analyze-test.yaml (2)
46-50: Consider hardcoding GitHub's SSH host key instead of runtime keyscan.Using
ssh-keyscanat runtime is susceptible to MITM attacks (though unlikely in GitHub Actions). For stronger supply-chain guarantees, hardcode GitHub's published host keys:🛡️ Optional hardening
- name: Add GitHub to known hosts run: | mkdir -p ~/.ssh - ssh-keyscan github.com >> ~/.ssh/known_hosts + # Hardcoded GitHub RSA host key (https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/githubs-ssh-key-fingerprints) + echo "github.com ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQCj7ndNxQowgcQnjshcLrqPEiiphnt+VTTvDP6mHBL9j1aNUkY4Ue1gvwnGLVlOhGeYrnZaMgRK6+PKCUXaDbC7qtbW8gIkhL7aGCsOr/C56SJMy/BCZfxd1nWzAOxSDPgVsmerOBYfNqltV9/hWCqBywINIR+5dIg6JTJ72pcEpEjcYgXkE2YEFXV1JHnsKgbLWNlhScqb2UmyRkQyytRLtL+38TGxkxCflmO+5Z8CSSNY7GidjMIZ7Q4zMjA2n1nGrlTDkzwDCsw+wqFPGQA179cnfGWOWRVruj16z6XyvxvjJwbz0wQZ75XK5tKSb7FNyeIEs4TT4jk+S4dhPeAUC5y+bDYirYgM4GC7uEnztnZyaVWQ7B381AK4Qdrwt51ZqExKbQpTUNn+EjqoTwvqNj4kqx5QUCI0ThS/YkOxJCXmPUWZbhjpCg56i+2aB6CmK2JGhn57K5mj0MNdBXA4/WnwH6XoPWJzK5Nyu2zB3nAZp+S5hpQs+p1vN1/wsjk=" >> ~/.ssh/known_hosts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/analyze-test.yaml around lines 46 - 50, Replace the runtime ssh-keyscan step (the "Add GitHub to known hosts" step) with writing GitHub's published SSH host key(s) directly into ~/.ssh/known_hosts: remove the ssh-keyscan invocation and instead append the official hardcoded host key strings (e.g., GitHub's rsa and ecdsa host keys) into the file, ensure ~/.ssh exists and has correct permissions before writing, and update the step comment to indicate keys are pinned to GitHub's published values; keep the step name "Add GitHub to known hosts" so it's easy to find.
54-54: Pinsubosito/flutter-actionto a commit SHA for consistency.This third-party action uses a mutable tag (
@v2) while other third-party actions in this workflow (webfactory/ssh-agent,zgosalvez/github-actions-analyze-dart) are pinned to immutable commit SHAs. For consistent supply-chain security, pin this action to commitfd55f4c5af5b953cc57a2be44cb082c8f6635e8e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/analyze-test.yaml at line 54, Replace the mutable tag used for the GitHub Action reference "subosito/flutter-action@v2" with the provided immutable commit SHA to ensure consistency and supply-chain security; update the workflow line referencing subosito/flutter-action (the uses entry) to use "subosito/flutter-action@fd55f4c5af5b953cc57a2be44cb082c8f6635e8e".
🤖 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/features/base/widget/emoji/emoji_button.dart`:
- Around line 81-83: _openDialog() can be re-entered while the async preload
runs, allowing multiple overlays; add a guard to prevent concurrent opens by
using a new boolean (e.g. _isOpening) or checking _overlayEntry != null at the
start of the async path, set it true immediately before any await/preload and
set it false in finally (or clear _overlayEntry only after removal) so only one
overlay is created; apply the same guard logic to the other open paths
referenced (lines around 110-113, 218-223, 234-235) and ensure _isDialogVisible
is still set appropriately when the overlay is shown/hidden.
---
Duplicate comments:
In `@lib/features/base/widget/emoji/emoji_button.dart`:
- Around line 94-97: Clamp the computed sizes to non-negative values: ensure
availableHeight and availableWidth are max(screenSize.height - 32, 0) and
max(screenSize.width - 16, 0), then compute dialogHeight =
math.min(_dialogHeight, availableHeight) and dialogWidth =
math.min(_dialogWidth, availableWidth) (or clamp dialogHeight/dialogWidth again
to >= 0). Update the same logic at the other occurrence (lines referencing
_dialogHeight/_dialogWidth and availableWidth/availableHeight around 134-135) so
no negative dimensions are passed into the dialog container.
---
Nitpick comments:
In @.github/workflows/analyze-test.yaml:
- Around line 46-50: Replace the runtime ssh-keyscan step (the "Add GitHub to
known hosts" step) with writing GitHub's published SSH host key(s) directly into
~/.ssh/known_hosts: remove the ssh-keyscan invocation and instead append the
official hardcoded host key strings (e.g., GitHub's rsa and ecdsa host keys)
into the file, ensure ~/.ssh exists and has correct permissions before writing,
and update the step comment to indicate keys are pinned to GitHub's published
values; keep the step name "Add GitHub to known hosts" so it's easy to find.
- Line 54: Replace the mutable tag used for the GitHub Action reference
"subosito/flutter-action@v2" with the provided immutable commit SHA to ensure
consistency and supply-chain security; update the workflow line referencing
subosito/flutter-action (the uses entry) to use
"subosito/flutter-action@fd55f4c5af5b953cc57a2be44cb082c8f6635e8e".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/analyze-test.yamllib/features/base/widget/emoji/emoji_button.dartlib/features/composer/presentation/extensions/handle_insert_emoji_to_editor_extension.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
#1302
Dependency
visibility_detectorversion to^0.4.0+2html-editor-enhanced#64Resolved
Screen.Recording.2025-11-04.at.10.20.40.AM.mov
Summary by CodeRabbit
New Features
Chores
Refactor