Conversation
|
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:
📝 WalkthroughWalkthroughUpdates UI spacing to Material 3 expressive constants across the apps and settings pages, replaces some SizedBox literals, introduces a SharedAxisTransition-based navigation to AppPage, adds new translation keys for provider names and accessibility hints, and removes automatic lint/format auto-fix steps from CI. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the user interface and experience by standardizing spacing, improving navigation flows, and modernizing component appearances. It focuses on creating a more consistent and accessible design, particularly within the app listing and settings screens, through the introduction of new spacing constants, updated component styling, and enhanced interactive elements. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several UI/UX improvements, primarily focused on updating spacing to follow Material 3 guidelines, enhancing visual components like status indicators with Chip widgets, and improving accessibility with Semantics. The changes are generally positive, but there are a couple of critical compilation errors that need to be addressed. Specifically, a spacing constant is used out of its scope in lib/pages/apps.dart, and an invalid parameter year2023 is used on a Slider widget in lib/pages/settings.dart. I've also included a suggestion to improve how constants are defined for better performance and maintainability.
| // M3 Expressive spacing constants (based on 4dp baseline grid) | ||
| const height4 = SizedBox(height: 4); | ||
| const height8 = SizedBox(height: 8); | ||
| const height12 = SizedBox(height: 12); | ||
| const height16 = SizedBox(height: 16); | ||
| const height20 = SizedBox(height: 20); | ||
| const height24 = SizedBox(height: 24); | ||
| const height28 = SizedBox(height: 28); | ||
| const height32 = SizedBox(height: 32); | ||
| const height40 = SizedBox(height: 40); | ||
| const height48 = SizedBox(height: 48); | ||
| const height56 = SizedBox(height: 56); | ||
| const height64 = SizedBox(height: 64); | ||
| const width4 = SizedBox(width: 4); | ||
| const width6 = SizedBox(width: 6); | ||
| const width8 = SizedBox(width: 8); | ||
| const width12 = SizedBox(width: 12); | ||
| const width16 = SizedBox(width: 16); | ||
| const width20 = SizedBox(width: 20); | ||
| const width24 = SizedBox(width: 24); |
There was a problem hiding this comment.
Defining these const spacing widgets inside the build method is not ideal for performance and code organization. It's better to define them outside the build method, for instance, as top-level constants at the beginning of the file. This improves readability and ensures they are only defined once. This would also solve the scope issue with height16 used in showChangeLogDialog (line 65).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚫 CI Build Failed
The automated build process failed. Please review the build logs and fix the issues before requesting another review.
Next steps:
- Check the build logs for specific errors
- Fix the identified issues
- Push your fixes to this branch
- The CI will automatically re-run
Once the build passes, this review will be dismissed automatically.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/pages/apps.dart (1)
1587-1587:⚠️ Potential issue | 🟡 MinorUnused variable
screenHeight.
screenHeightis declared but never used in the responsive grid logic. Remove it to avoid dead code.🧹 Suggested fix
final screenWidth = MediaQuery.of(context).size.width; - final screenHeight = MediaQuery.of(context).size.height;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` at line 1587, The variable screenHeight declared as `final screenHeight = MediaQuery.of(context).size.height;` is unused and should be removed to eliminate dead code; locate this line in the responsive grid logic (look for `MediaQuery.of(context).size.height` / `screenHeight`) and delete the declaration so the code compiles without the unused variable warning.
🧹 Nitpick comments (3)
lib/pages/apps.dart (3)
885-885: Inconsistent use of spacing constants.Line 885 uses a literal
const SizedBox(height: 4)whileheight4is already defined at line 179. For consistency with the M3 spacing system established in this file, use the predefined constant.♻️ Suggested fix
- const SizedBox(height: 4), + height4,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` at line 885, Replace the literal const SizedBox(height: 4) with the predefined spacing constant height4 to keep spacing consistent with the M3 system; locate the occurrence of const SizedBox(height: 4) in the build/layout where spacing is used and swap it to use height4 (the existing constant defined earlier) so all gaps use the same spacing constants.
958-976: Consider extracting the "Updated" Chip to a reusable widget.This Chip implementation is nearly identical to lines 594-612, differing only in icon size (14 vs 16) and font size (10 vs 12). Extracting to a parameterized helper would reduce duplication and ensure consistent styling.
♻️ Example extraction
Widget _buildUpdatedChip({double iconSize = 16, double fontSize = 12}) { return Chip( avatar: Icon( Icons.check_circle, color: Theme.of(context).colorScheme.primary, size: iconSize, ), label: Text( tr('updated'), style: TextStyle( color: Theme.of(context).colorScheme.primary, fontSize: fontSize, ), ), backgroundColor: Theme.of(context).colorScheme.primaryContainer, side: BorderSide( color: Theme.of(context).colorScheme.primary.withOpacity(0.3), width: 1, ), ); }Then use
_buildUpdatedChip()for horizontal tiles and_buildUpdatedChip(iconSize: 14, fontSize: 10)for grid tiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` around lines 958 - 976, Extract the duplicated Chip into a single reusable helper named _buildUpdatedChip that accepts parameters for iconSize and fontSize (e.g., _buildUpdatedChip({double iconSize = 16, double fontSize = 12})), use Theme.of(context).colorScheme inside the helper to keep styling consistent, and replace both inline Chip occurrences (the horizontal tile and the grid tile) with calls to _buildUpdatedChip() and _buildUpdatedChip(iconSize: 14, fontSize: 10) respectively; ensure the helper is defined in the same widget/class scope so it can access the BuildContext.
1711-1715: Minor: Left padding (28) deviates from 4dp grid.The M3 spacing system uses multiples of 4, but 28 is not a standard value (24 or 32 would be). If this is intentional for visual alignment with the actions area, consider adding a brief comment. Otherwise, 24 may be more consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` around lines 1711 - 1715, titlePadding uses const EdgeInsets.only(left: 28, right: 88, bottom: 24) which violates the M3 4dp grid; change the left padding to a standard multiple of 4 (preferably 24 or 32) or add a comment explaining the intentional 28px alignment. Update the EdgeInsets in the widget where titlePadding is set (reference: titlePadding / EdgeInsets.only) to use the chosen value and, if keeping 28, add an inline comment clarifying why it deviates to avoid future regressions.
🤖 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/pages/settings.dart`:
- Around line 1109-1149: The Semantics widgets use hardcoded English hint
strings (e.g., 'Open app source repository', 'Open Updatium wiki documentation',
'View application logs'); replace those hint values with localized calls (use
tr('appSourceHint'), tr('wikiHint'), tr('appLogsHint') or similar keys) in the
Semantics constructors so hint: tr('...') is used consistently alongside label:
tr('...'); then add the corresponding translation keys/values to your locale
resource files so non‑English users get localized accessibility hints.
- Around line 1138-1143: The Transform wrapping the Icon(Icons.help) uses
Matrix4.identity() which does nothing; either remove the Transform entirely
(leave Icon(Icons.help) for both branches) or, if you intended to mirror for RTL
when context.locale.languageCode == 'he', replace the identity matrix with a
horizontal-flip transform (apply a Y-axis flip) so the icon is actually
mirrored; update the branch that currently reads Transform(...
Matrix4.identity() ...) accordingly.
---
Outside diff comments:
In `@lib/pages/apps.dart`:
- Line 1587: The variable screenHeight declared as `final screenHeight =
MediaQuery.of(context).size.height;` is unused and should be removed to
eliminate dead code; locate this line in the responsive grid logic (look for
`MediaQuery.of(context).size.height` / `screenHeight`) and delete the
declaration so the code compiles without the unused variable warning.
---
Nitpick comments:
In `@lib/pages/apps.dart`:
- Line 885: Replace the literal const SizedBox(height: 4) with the predefined
spacing constant height4 to keep spacing consistent with the M3 system; locate
the occurrence of const SizedBox(height: 4) in the build/layout where spacing is
used and swap it to use height4 (the existing constant defined earlier) so all
gaps use the same spacing constants.
- Around line 958-976: Extract the duplicated Chip into a single reusable helper
named _buildUpdatedChip that accepts parameters for iconSize and fontSize (e.g.,
_buildUpdatedChip({double iconSize = 16, double fontSize = 12})), use
Theme.of(context).colorScheme inside the helper to keep styling consistent, and
replace both inline Chip occurrences (the horizontal tile and the grid tile)
with calls to _buildUpdatedChip() and _buildUpdatedChip(iconSize: 14, fontSize:
10) respectively; ensure the helper is defined in the same widget/class scope so
it can access the BuildContext.
- Around line 1711-1715: titlePadding uses const EdgeInsets.only(left: 28,
right: 88, bottom: 24) which violates the M3 4dp grid; change the left padding
to a standard multiple of 4 (preferably 24 or 32) or add a comment explaining
the intentional 28px alignment. Update the EdgeInsets in the widget where
titlePadding is set (reference: titlePadding / EdgeInsets.only) to use the
chosen value and, if keeping 28, add an inline comment clarifying why it
deviates to avoid future regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0321ff13-fd0f-4f10-ac0d-baade44c992c
📒 Files selected for processing (2)
lib/pages/apps.dartlib/pages/settings.dart
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
assets/translations/de.json (1)
96-99:⚠️ Potential issue | 🟡 MinorSeveral keys remain untranslated (still in English).
The following keys at lines 96-99 and 144-147 are still in English:
failedToExport,exportAlreadyInProgress,failedToCreateExportFile,exportDirNotAccessible,networkError,invalidUrlFormat,accessDenied,importFailed. These should be translated to German.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/translations/de.json` around lines 96 - 99, The JSON file contains untranslated English strings; replace the values for the keys failedToExport, exportAlreadyInProgress, failedToCreateExportFile, exportDirNotAccessible, networkError, invalidUrlFormat, accessDenied, and importFailed with their correct German translations (e.g., "Export fehlgeschlagen", "Export bereits im Gange", "Exportdatei konnte nicht erstellt werden", "Exportverzeichnis ist nicht zugänglich", "Netzwerkfehler", "Ungültiges URL-Format", "Zugriff verweigert", "Import fehlgeschlagen") so the de.json localization is fully in German and keys remain unchanged.lib/pages/settings.dart (1)
1216-1224:⚠️ Potential issue | 🟠 MajorMissing
filterDaystranslation key.The
tr('filterDays')call at line 1222 references a translation key that doesn't exist in any of the translation files. This will display the raw key string or cause a translation error at runtime.Add the
filterDayskey to all translation files:
- en.json:
"filterDays": "Filter by days"- es.json:
"filterDays": "Filtrar por días"- de.json:
"filterDays": "Nach Tagen filtern"- fr.json:
"filterDays": "Filtrer par jours"- he.json:
"filterDays": "סינון לפי ימים"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings.dart` around lines 1216 - 1224, The TextField uses tr('filterDays') which is missing from the translation bundles; add the "filterDays" key with the provided localized values to each translation file (en.json, es.json, de.json, fr.json, he.json) so tr('filterDays') resolves correctly—update en.json to "filterDays": "Filter by days", es.json to "filterDays": "Filtrar por días", de.json to "filterDays": "Nach Tagen filtern", fr.json to "filterDays": "Filtrer par jours", and he.json to "filterDays": "סינון לפי ימים".assets/translations/es.json (1)
96-99:⚠️ Potential issue | 🟡 MinorSeveral keys remain untranslated (still in English).
The following keys at lines 96-99 and 144-147 are still in English and should be translated to Spanish:
failedToExport,exportAlreadyInProgress,failedToCreateExportFile,exportDirNotAccessible,networkError,invalidUrlFormat,accessDenied,importFailed.This may also be contributing to the pipeline failure for missing translation keys.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/translations/es.json` around lines 96 - 99, Update the Spanish translation file by replacing the English values for the translation keys failedToExport, exportAlreadyInProgress, failedToCreateExportFile, exportDirNotAccessible and the keys networkError, invalidUrlFormat, accessDenied, importFailed with proper Spanish translations (i.e., modify the values for these keys in assets/translations/es.json), save the file, and run the translations/checks to ensure no missing keys remain; target the string entries for these exact keys so automated pipelines pick up the changes.assets/translations/he.json (1)
78-78:⚠️ Potential issue | 🟡 MinorExtensive untranslated English strings in Hebrew locale.
This file contains many keys still in English that should be translated to Hebrew. Notable examples include:
- Line 78:
installStatusOfXWillBeResetExplanation- Lines 96-99: Export-related errors
- Lines 144-147: Network/import errors
- Lines 154, 156, 161, 167, 171, 173: Various notification descriptions
- Lines 215, 220, 235, 242, 246: Explanation texts
- Lines 252-253, 264, 273-276, 282, 284, 286: Various settings labels
- Lines 292-296, 302-303, 306, 310, 321, 328, 331, 333-334: More settings
- Lines 360-365, 367, 370-373: Documentation and warning texts
- Lines 384-392, 420-422: Pluralized messages
- Lines 441-442, 450-451: Certificate and plural forms
This incomplete translation significantly impacts Hebrew-speaking users. Consider prioritizing translation of user-facing strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/translations/he.json` at line 78, Several user-facing entries in the Hebrew locale remain in English and must be translated into Hebrew; update the values for keys such as installStatusOfXWillBeResetExplanation and the listed export/import/network/notification/setting/documentation/certificate keys (e.g., the export-related error keys around lines 96-99, network/import errors around 144-147, notification descriptions at 154/156/161/167/171/173, the explanation keys at 215/220/235/242/246, settings labels around 252-286/292-334, documentation/warning texts at 360-373, pluralized messages at 384-422, and certificate/plural forms at 441-451) with proper Hebrew translations; preserve existing placeholders, newline escapes (\n), and pluralization patterns exactly as in the English originals, and ensure no key names or JSON structure are changed.assets/translations/fr.json (1)
96-99:⚠️ Potential issue | 🟡 MinorSeveral keys remain untranslated (still in English).
The following keys at lines 96-99 and 144-147 are still in English:
failedToExport,exportAlreadyInProgress,failedToCreateExportFile,exportDirNotAccessible,networkError,invalidUrlFormat,accessDenied,importFailed. These should be translated to French.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/translations/fr.json` around lines 96 - 99, Replace the untranslated English values for the listed translation keys with their French equivalents: update "failedToExport", "exportAlreadyInProgress", "failedToCreateExportFile", "exportDirNotAccessible", "networkError", "invalidUrlFormat", "accessDenied", and "importFailed" in the translations file so each key maps to the correct French string (e.g., "failedToExport" -> "Échec de l'exportation", "exportAlreadyInProgress" -> "Exportation déjà en cours", "failedToCreateExportFile" -> "Échec de la création du fichier d'export", "exportDirNotAccessible" -> "Le répertoire d'exportation n'est pas accessible", "networkError" -> "Erreur réseau", "invalidUrlFormat" -> "Format d'URL invalide", "accessDenied" -> "Accès refusé", "importFailed" -> "Échec de l'importation"); ensure proper JSON string escaping and consistent punctuation with surrounding entries.assets/translations/en.json (1)
1-447:⚠️ Potential issue | 🟠 MajorAdd missing
filterDaystranslation key to translation files.The code references
tr('filterDays')insettings.dartat line 1222 within theLogsDialogwidget, but this key is not defined in any translation file. Add the translation key with an appropriate label for the logs filtering dialog.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/translations/en.json` around lines 1 - 447, The translation file is missing the "filterDays" key referenced by tr('filterDays') in settings.dart inside the LogsDialog widget; add a new top-level entry "filterDays": "Filter days" (or another suitable short label) to the en.json translations so the LogsDialog can display the filter label without falling back to a missing key error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@assets/translations/de.json`:
- Around line 96-99: The JSON file contains untranslated English strings;
replace the values for the keys failedToExport, exportAlreadyInProgress,
failedToCreateExportFile, exportDirNotAccessible, networkError,
invalidUrlFormat, accessDenied, and importFailed with their correct German
translations (e.g., "Export fehlgeschlagen", "Export bereits im Gange",
"Exportdatei konnte nicht erstellt werden", "Exportverzeichnis ist nicht
zugänglich", "Netzwerkfehler", "Ungültiges URL-Format", "Zugriff verweigert",
"Import fehlgeschlagen") so the de.json localization is fully in German and keys
remain unchanged.
In `@assets/translations/en.json`:
- Around line 1-447: The translation file is missing the "filterDays" key
referenced by tr('filterDays') in settings.dart inside the LogsDialog widget;
add a new top-level entry "filterDays": "Filter days" (or another suitable short
label) to the en.json translations so the LogsDialog can display the filter
label without falling back to a missing key error.
In `@assets/translations/es.json`:
- Around line 96-99: Update the Spanish translation file by replacing the
English values for the translation keys failedToExport, exportAlreadyInProgress,
failedToCreateExportFile, exportDirNotAccessible and the keys networkError,
invalidUrlFormat, accessDenied, importFailed with proper Spanish translations
(i.e., modify the values for these keys in assets/translations/es.json), save
the file, and run the translations/checks to ensure no missing keys remain;
target the string entries for these exact keys so automated pipelines pick up
the changes.
In `@assets/translations/fr.json`:
- Around line 96-99: Replace the untranslated English values for the listed
translation keys with their French equivalents: update "failedToExport",
"exportAlreadyInProgress", "failedToCreateExportFile", "exportDirNotAccessible",
"networkError", "invalidUrlFormat", "accessDenied", and "importFailed" in the
translations file so each key maps to the correct French string (e.g.,
"failedToExport" -> "Échec de l'exportation", "exportAlreadyInProgress" ->
"Exportation déjà en cours", "failedToCreateExportFile" -> "Échec de la création
du fichier d'export", "exportDirNotAccessible" -> "Le répertoire d'exportation
n'est pas accessible", "networkError" -> "Erreur réseau", "invalidUrlFormat" ->
"Format d'URL invalide", "accessDenied" -> "Accès refusé", "importFailed" ->
"Échec de l'importation"); ensure proper JSON string escaping and consistent
punctuation with surrounding entries.
In `@assets/translations/he.json`:
- Line 78: Several user-facing entries in the Hebrew locale remain in English
and must be translated into Hebrew; update the values for keys such as
installStatusOfXWillBeResetExplanation and the listed
export/import/network/notification/setting/documentation/certificate keys (e.g.,
the export-related error keys around lines 96-99, network/import errors around
144-147, notification descriptions at 154/156/161/167/171/173, the explanation
keys at 215/220/235/242/246, settings labels around 252-286/292-334,
documentation/warning texts at 360-373, pluralized messages at 384-422, and
certificate/plural forms at 441-451) with proper Hebrew translations; preserve
existing placeholders, newline escapes (\n), and pluralization patterns exactly
as in the English originals, and ensure no key names or JSON structure are
changed.
In `@lib/pages/settings.dart`:
- Around line 1216-1224: The TextField uses tr('filterDays') which is missing
from the translation bundles; add the "filterDays" key with the provided
localized values to each translation file (en.json, es.json, de.json, fr.json,
he.json) so tr('filterDays') resolves correctly—update en.json to "filterDays":
"Filter by days", es.json to "filterDays": "Filtrar por días", de.json to
"filterDays": "Nach Tagen filtern", fr.json to "filterDays": "Filtrer par
jours", and he.json to "filterDays": "סינון לפי ימים".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab3fbb82-0a4f-4b94-b470-6c478e74d3f8
📒 Files selected for processing (6)
assets/translations/de.jsonassets/translations/en.jsonassets/translations/es.jsonassets/translations/fr.jsonassets/translations/he.jsonlib/pages/settings.dart
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ci.yml:
- Line 85: Replace the branch input value that currently uses github.head_ref
with a fallback expression so manual workflow_dispatch runs get a valid branch;
change the action input that sets branch: ${{ github.head_ref }} to use the
combined expression ${{ github.head_ref || github.ref_name }} (so
stefanzweifel/git-auto-commit-action@v7 receives the PR head branch for
pull_request events and the dispatch branch for manual runs).
In @.github/workflows/nightly.yml:
- Line 61: The workflow uses branch: ${{ github.head_ref }} which is empty for
push and workflow_dispatch events; update that setting to use github.ref_name
instead so the auto-commit step targets the correct branch for both push and
manual runs (replace the branch: ${{ github.head_ref }} occurrence with branch:
${{ github.ref_name }} in the job configuration).
In @.github/workflows/release.yml:
- Line 61: The workflow uses github.head_ref for the auto-commit branch (the
`branch: ${{ github.head_ref }}` entry), but `workflow_dispatch` does not set
github.head_ref; update that branch reference to use github.ref_name instead so
the auto-commit step has a valid branch target when the workflow runs via
`workflow_dispatch`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 939d3501-5fe5-47cb-b6cb-cabf5b2c67d3
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/nightly.yml.github/workflows/release.yml
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Removed auto-fix linting and formatting steps from CI workflow.
github-actions
bot
dismissed
their stale review
✅ CI Build Passed - Automatically dismissing previous rejection as the build now succeeds.
Replaced Chip widget with Row widget for updated status display.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
lib/pages/apps.dart (5)
513-540: Inconsistent navigation transitions betweenonTapandonLongPress.The
onTaphandler usesSharedAxisTransition(lines 513-528), butonLongPress(lines 534-539) still uses a plainMaterialPageRoute. Both navigate to the sameAppPagedestination. Consider unifying the transition behavior for consistency.♻️ Proposed fix to unify transitions
Extract a helper method or apply the same transition to both:
onLongPress: () { Navigator.push( context, - MaterialPageRoute( - builder: (context) => AppPage(appId: listedApps[appIndex].app.id), + PageRouteBuilder( + pageBuilder: (context, animation, secondaryAnimation) => + AppPage(appId: listedApps[appIndex].app.id), + transitionsBuilder: + (context, animation, secondaryAnimation, child) { + return SharedAxisTransition( + animation: animation, + secondaryAnimation: secondaryAnimation, + transitionType: SharedAxisTransitionType.horizontal, + child: child, + ); + }, ), ); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` around lines 513 - 540, The onTap navigation uses a PageRouteBuilder with SharedAxisTransition while onLongPress uses MaterialPageRoute, causing inconsistent transitions; update onLongPress to use the same PageRouteBuilder + SharedAxisTransition targeting AppPage(appId: listedApps[appIndex].app.id) (or extract a helper like buildAppPageRoute(appId) that returns the PageRouteBuilder and use it in both onTap and onLongPress) so both Navigator.push calls produce identical animated transitions.
885-885: Inconsistent use of spacing constant.Line 885 uses
const SizedBox(height: 4)directly instead of theheight4constant defined at line 179. For consistency with the M3 spacing system approach used elsewhere in this widget, consider using the constant.♻️ Proposed fix
- const SizedBox(height: 4), + height4,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` at line 885, Replace the literal const SizedBox(height: 4) with the existing height4 spacing constant to keep spacing consistent with the M3 spacing system; locate the widget tree where const SizedBox(height: 4) is used (near the AppsPage/Apps widget group) and swap it to use height4 so the code uses the predefined spacing constant defined earlier (height4).
808-814: Grid view navigation uses different transition than list view.The grid tile's
onTaphandler (lines 808-814) uses a plainMaterialPageRoute, while the horizontal list tile (lines 659-674) usesSharedAxisTransition. This creates an inconsistent navigation experience between grid and list views. Consider applying the same transition to both for visual consistency.♻️ Proposed fix to use SharedAxisTransition
} else { Navigator.push( context, - MaterialPageRoute( - builder: (context) => - AppPage(appId: listedApps[index].app.id), + PageRouteBuilder( + pageBuilder: (context, animation, secondaryAnimation) => + AppPage(appId: listedApps[index].app.id), + transitionsBuilder: + (context, animation, secondaryAnimation, child) { + return SharedAxisTransition( + animation: animation, + secondaryAnimation: secondaryAnimation, + transitionType: SharedAxisTransitionType.horizontal, + child: child, + ); + }, ), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` around lines 808 - 814, The grid tap currently navigates using Navigator.push with MaterialPageRoute (see MaterialPageRoute, Navigator.push, AppPage) causing a different transition than the horizontal list which uses SharedAxisTransition; update the grid tile's onTap navigation to wrap AppPage in the same SharedAxisTransition-based route as the list (use a PageRouteBuilder or the same route builder used by the list) so the push uses the shared-axis animation variant and duration/options match the horizontal list's implementation; ensure you replace MaterialPageRoute with the same route-building logic and reference the existing SharedAxisTransition usage so both grid and list use identical transitions.
178-196: Several spacing constants appear unused in this file.Constants like
height20,height28,height40,height48,height56,height64,width8,width12,width20,width24are declared but don't appear to be used within this file. Consider either:
- Moving these to a shared constants file if they're part of a design system used across the app
- Removing unused constants to reduce clutter
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` around lines 178 - 196, Several spacing constants (height20, height28, height40, height48, height56, height64, width8, width12, width20, width24) are declared but unused here; either remove them from lib/pages/apps.dart or move them into a shared spacing constants file (e.g., spacing.dart) that exports common SizedBox constants. To fix: search the repo for usages of those symbols, and if used across multiple files create a shared constants file (export it where needed) and import it into this file, otherwise delete the unused constant declarations from apps.dart to reduce clutter.
1708-1712: Minor inconsistency with 4dp baseline grid.The comment at line 178 mentions "M3 Expressive spacing constants (based on 4dp baseline grid)", but
left: 28doesn't align to this grid (28 is not divisible by 4). Consider using24or32for consistency with the spacing system, unless28is intentional for visual alignment with specific elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/apps.dart` around lines 1708 - 1712, The titlePadding left value (titlePadding: const EdgeInsets.only(left: 28, ...)) breaks the 4dp baseline grid; change the left: 28 to a 4dp-grid-aligned value (e.g., left: 24 or left: 32) in the widget/property that sets titlePadding so it matches the "M3 Expressive spacing constants" mentioned in the comment, unless 28 was intentionally chosen for a specific visual alignment—if so, add an inline comment explaining the exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/pages/apps.dart`:
- Around line 513-540: The onTap navigation uses a PageRouteBuilder with
SharedAxisTransition while onLongPress uses MaterialPageRoute, causing
inconsistent transitions; update onLongPress to use the same PageRouteBuilder +
SharedAxisTransition targeting AppPage(appId: listedApps[appIndex].app.id) (or
extract a helper like buildAppPageRoute(appId) that returns the PageRouteBuilder
and use it in both onTap and onLongPress) so both Navigator.push calls produce
identical animated transitions.
- Line 885: Replace the literal const SizedBox(height: 4) with the existing
height4 spacing constant to keep spacing consistent with the M3 spacing system;
locate the widget tree where const SizedBox(height: 4) is used (near the
AppsPage/Apps widget group) and swap it to use height4 so the code uses the
predefined spacing constant defined earlier (height4).
- Around line 808-814: The grid tap currently navigates using Navigator.push
with MaterialPageRoute (see MaterialPageRoute, Navigator.push, AppPage) causing
a different transition than the horizontal list which uses SharedAxisTransition;
update the grid tile's onTap navigation to wrap AppPage in the same
SharedAxisTransition-based route as the list (use a PageRouteBuilder or the same
route builder used by the list) so the push uses the shared-axis animation
variant and duration/options match the horizontal list's implementation; ensure
you replace MaterialPageRoute with the same route-building logic and reference
the existing SharedAxisTransition usage so both grid and list use identical
transitions.
- Around line 178-196: Several spacing constants (height20, height28, height40,
height48, height56, height64, width8, width12, width20, width24) are declared
but unused here; either remove them from lib/pages/apps.dart or move them into a
shared spacing constants file (e.g., spacing.dart) that exports common SizedBox
constants. To fix: search the repo for usages of those symbols, and if used
across multiple files create a shared constants file (export it where needed)
and import it into this file, otherwise delete the unused constant declarations
from apps.dart to reduce clutter.
- Around line 1708-1712: The titlePadding left value (titlePadding: const
EdgeInsets.only(left: 28, ...)) breaks the 4dp baseline grid; change the left:
28 to a 4dp-grid-aligned value (e.g., left: 24 or left: 32) in the
widget/property that sets titlePadding so it matches the "M3 Expressive spacing
constants" mentioned in the comment, unless 28 was intentionally chosen for a
specific visual alignment—if so, add an inline comment explaining the exception.
1 participant
Summary by CodeRabbit
New Features
Improvements
Localization