feat: group admin edit, media tab, confirm modals by ice-priapus · Pull Request #2490 · ice-blockchain/ion-framework

ice-priapus · 2025-11-10T15:51:51Z

internal review

group edit page
group admin media tab
confirmation modals

ice-kreios · 2025-11-11T06:58:17Z

+    File? groupImageFile;
+    if (groupImageFileResult.hasData) {
+      final file = groupImageFileResult.data;
+      if (file != null && file.existsSync() && file.lengthSync() > 0) {
+        groupImageFile = file;
+      }
+    }


for each rebuild, it ll check length and exists sync, should we keep in state and perform only when related file is changed?

ice-kreios · 2025-11-12T08:57:58Z

+        // Extract links from content
+        final content = entity.data.content;
+        if (content.isNotEmpty) {
+          final textParser = TextParser(matchers: {const UrlMatcher()});


we can define it out of the for loop to do not create for each item. wdyt?

ice-kreios · 2025-11-12T09:00:04Z

+  for (final messages in messagesByDate.values) {
+    for (final eventMessage in messages) {
+      try {
+        final entity = EncryptedGroupMessageEntity.fromEventMessage(eventMessage);
+        final eventReference = entity.toEventReference();
+        final publishedAt = entity.data.publishedAt.value;
+
+        // Extract links from content
+        final content = entity.data.content;
+        if (content.isNotEmpty) {
+          final textParser = TextParser(matchers: {const UrlMatcher()});
+          final parsed = textParser.parse(content, onlyMatches: true);
+          final links = parsed
+              .where((match) => match.matcher is UrlMatcher)
+              .map((match) => match.text)
+              .toSet(); // Use Set to avoid duplicates
+
+          for (final link in links) {
+            // Exclude media URLs (they're already in media attachments)
+            final isMediaUrl = entity.data.media.values.any(
+              (media) => media.url == link || media.thumb == link || media.image == link,
+            );
+            if (!isMediaUrl) {
+              allLinks.add(
+                GroupLinkItem(
+                  url: link,
+                  eventReference: eventReference,
+                  publishedAt: publishedAt,
+                ),
+              );
+            }
+          }
+        }


Let’s assume a group conversation has a few thousand messages. I believe it’s quite feasible to reach that count for groups. Does this cause UI lag? We could load it in pages to enhance performance. What are your thoughts on this? If it does cause lag, I’m also open to handling follow-up tasks.

same concerns for files, media and voice tabs

ice-kreios · 2025-11-12T09:03:40Z

+          itemBuilder: (context, int index) {
+            final fileItem = files[index];
+            return _GroupFileCell(
+              fileName: fileItem.media.alt ?? 'File',


all files should have alt prop which indicates the file name - if we need it hence of null property definition, we can define this magic string into the l18n. wdyt?

ice-kreios · 2025-11-12T09:05:12Z

+          )
+        : null;
+
+    final fileSizeStr = localMediaPath != null ? formattedFileSize(localMediaPath) ?? '0kb' : '0kb';


are we sure about showing 0kb while loading the media? we can align it with designer wdyt?

ice-kreios · 2025-11-12T09:09:43Z

+  String _formatDateAndTime(int timestamp) {
+    final date = timestamp.toDateTime;
+    final dateStr = DateFormat('d MMM yyyy').format(date);
+    final timeStr = DateFormat('HH:mm').format(date);
+    return '$dateStr • $timeStr';
+  }


can we handle in one DateFormat('d MMM yyyy • HH:mm');
also we can move it to the date.dart util class. wdyt?

ice-tychon · 2025-11-15T13:59:57Z

+part 'group_links_provider.r.g.dart';
+
+@riverpod
+Future<List<GroupLinkItem>> groupLinks(


Consider creating GroupLinksService class for the logic and use Riverpod only to create it's instance, like this:

@riverpod GroupLinksService groupLinksService(GroupLinksServiceRef ref) { final dao = ref.watch(conversationMessageDaoProvider); final parser = TextParser(matchers: {const UrlMatcher()}); return GroupLinksService( conversationMessageDao: dao, textParser: parser, ); }

ice-tychon · 2025-11-15T14:05:30Z

+part 'group_media_provider.r.g.dart';
+
+@riverpod
+Future<List<GroupMediaItem>> groupMedia(


Consider extracting all parsing, flattening, and sorting work into a dedicated GroupMediaService class.
Use Riverpod only to construct the service and call its methods:

@riverpod GroupMediaService groupMediaService(GroupMediaServiceRef ref) { final dao = ref.watch(conversationMessageDaoProvider); return GroupMediaService(dao); }

Then groupMedia, groupMediaItems, groupVoiceItems, and groupFilesItems should only delegate to this service instead of performing business logic inside the providers.

ice-tychon · 2025-11-15T14:32:56Z

+    final userPreviewData = ref.watch(userPreviewDataProvider(participantMasterkey)).valueOrNull;
+
+    if (userPreviewData == null) {
+      return const SizedBox.shrink();


Do we need a loader here?

ice-tychon · 2025-11-15T14:37:37Z

+  Widget build(BuildContext context, WidgetRef ref) {
+    final groupMetadata = ref.watch(encryptedGroupMetadataProvider(conversationId)).valueOrNull;
+
+    final adminsAndOwners = groupMetadata?.members


This is a business logic, consider extracting it from the widget.

Creating sortedAdmins too.

Also calculating isMaxAdminsReached.

ice-tychon · 2025-11-15T14:43:46Z

+        .toList();
+
+    if (adminsAndOwners == null || adminsAndOwners.isEmpty) {
+      return const SizedBox.shrink();


Should a skeleton/loading be implemented here?

ice-tychon · 2025-11-15T14:53:07Z

-                ),
-          ).data
-        : null;
+    final mediaFuture = useMemoized<Future<File?>>(
