feat: unify app surfaces with native grouped design - #628
Conversation
Reuse shared chat-style press surfaces for conversations, folders, Hermes, Notes, and Channels so long-press previews cannot lift transparent rows. Route iOS 26 response and note actions through native menus and glass controls.
|
Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting). This review would cost an estimated $34.57, which exceeds your per-review limit of $10.00. The top 3 files driving up this estimate:
Tip To get this pull request reviewed, you can:
|
|
Important Review skippedToo many files! This PR contains 423 files, which is 273 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (423)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR adds shared connection and utility components. It restructures authentication, direct-connection, Hermes, workspace, and sidebar flows. It adds persistent tablet resizing, improved error rendering, localization, responsive layouts, and expanded tests. ChangesApplication UI and interaction updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant BackendChooserPage
participant DirectConnectionEditorPage
participant DirectProfileController
User->>BackendChooserPage: Select direct connection
BackendChooserPage->>DirectConnectionEditorPage: Open onboarding editor
User->>DirectConnectionEditorPage: Submit provider details
DirectConnectionEditorPage->>DirectProfileController: Probe provider
DirectProfileController-->>DirectConnectionEditorPage: Return probe result
DirectConnectionEditorPage->>DirectProfileController: Save reachable profile
DirectConnectionEditorPage->>BackendChooserPage: Navigate after save
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/features/auth/views/authentication_page.dart (1)
294-310: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the haptic provider reads with
mountedafter sign-in awaits.
_signInhas an await boundary before the success/catch paths accessref.read(hapticEnabledProvider). If_AuthenticationPageStateis disposed during sign-in, thoseref.readcalls throw after the login call throws, so the catch still runs. Return early from catch when not mounted, and only callPlatformService.hapticFeedbackWithSettingswhile mounted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/auth/views/authentication_page.dart` around lines 294 - 310, The _signIn success and catch paths access hapticEnabledProvider after an await without confirming the state is still mounted. Guard both hapticFeedbackWithSettings calls with mounted, and return immediately at the start of the catch block when !_AuthenticationPageState.mounted before setState or provider access.lib/features/hermes/views/hermes_settings_page.dart (1)
742-748: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a plural ARB message for the tool count.
'${toolset.tools.length} ${l10n.tools}'concatenates a number and a noun. Locales with plural categories or different word order render this incorrectly, and the count of 1 reads as a plural. Add an ICU plural key, for examplehermesToolsetToolCount, and format the whole phrase in the ARB file.As per path instructions for
lib/l10n/*.arb: "Edit localization ARB inputs rather than generated localization Dart files."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/hermes/views/hermes_settings_page.dart` around lines 742 - 748, The tool count text in the Hermes settings view should use an ICU pluralized localization instead of concatenating toolset.tools.length with l10n.tools. Add the hermesToolsetToolCount plural message to the appropriate lib/l10n/*.arb inputs, regenerate localization accessors as needed, and update the Text expression around toolset.label to format the complete count phrase through that localized message while preserving the disabled-label behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/core/widgets/error_boundary.dart`:
- Around line 2-95: Update ConduitFriendlyErrorView to remain safe when built
without inherited widgets: use a directional fallback when
Directionality.maybeOf(context) is absent, render the Material/themed UI only
when both MediaQuery.maybeOf(context) and Directionality.maybeOf(context) are
available, and omit SafeArea when MediaQuery is unavailable. Keep
installConduitErrorWidgetBuilder unchanged and preserve the existing error
content when the required inherited context exists.
In `@lib/features/auth/views/connection_issue_page.dart`:
- Around line 52-58: Update the ConnectionAttemptBanner state selection and its
duplicate around the corresponding alternate section to avoid using _statusLabel
for failed banners, since online status can produce a connected message. Add and
use a _bannerState helper that preserves the retrying state, prefers
_statusMessage, maps offline status to pleaseCheckConnection, and otherwise
returns couldNotConnectGeneric as a failed state.
In `@lib/features/auth/views/proxy_auth_page.dart`:
- Line 642: Replace nullable localization lookups with non-null lookups for
every listed assignment: in lib/features/auth/views/proxy_auth_page.dart at
lines 642, 672, 843, 854, 882, 1033, 1543, and 1563, update the relevant
proxyAuthFailed lookups; in lib/features/auth/views/sso_auth_page.dart at lines
180, 196, 211, 246, 719, and 752, update the relevant ssoAuthFailed lookups. Use
the non-null localization contract already established by the build methods so
_error is populated and error states render correctly.
In `@lib/features/auth/widgets/connection_setup_components.dart`:
- Around line 184-198: Update the interactive row in ConnectionChoiceRow to use
a focusable, keyboard-activatable control such as InkWell or
FocusableActionDetector instead of relying solely on GestureDetector. Preserve
_handleTap, the existing press-state animation callbacks, semantics, and touch
behavior while supporting Enter/Space activation and focus traversal.
- Around line 409-438: The Semantics banner duplicates state.message because
both the parent label and child Text expose the same announcement. Update the
Semantics widget in the connection setup component to set excludeSemantics:
true, preserving its existing label and live-region behavior.
In `@lib/features/direct_connections/views/direct_connection_editor_page.dart`:
- Around line 1716-1719: Replace the hard-coded helper text in the direct
connection editor with the corresponding `l10n` string, add the new translation
key to `lib/l10n/app_en.arb`, and provide entries for every shipped locale while
preserving the existing message meaning.
In `@lib/features/hermes/views/hermes_settings_page.dart`:
- Around line 427-445: Update the Image.asset inside ConnectionMark in the
Hermes settings header to provide explicit width and height matching the shared
connection-mark sizing convention. Keep the existing asset, styling, and
semantics configuration unchanged.
In `@lib/features/navigation/providers/sidebar_providers.dart`:
- Around line 48-62: Update the notifier’s setWidth flow to apply the clamped
state immediately, return without persisting when the clamped value is
unchanged, and debounce/coalesce PreferencesStore.put for sidebarTabletWidth so
only the latest pending value is written. Add lifecycle handling in onDispose to
cancel the debounce and flush the latest pending value as required, preserving
the existing error logging for failed persistence.
In `@lib/features/navigation/providers/sidebar_tab_scroll_registry.dart`:
- Around line 32-34: Convert sidebarTabScrollRegistryProvider from the manual
Provider declaration to Riverpod 3 generated `@riverpod` syntax, following the
pattern used by SidebarTabletWidth in sidebar_providers.dart. Preserve the
SidebarTabScrollRegistry instance behavior and run dart run build_runner build
to regenerate the provider code.
In `@lib/features/navigation/widgets/conversation_tile.dart`:
- Around line 431-449: Update the Semantics widget in the conversation tile
build to set its enabled state from the existing enabled value, so loading tiles
are reported as disabled while normal tiles remain enabled; keep the existing
button and interaction behavior unchanged.
In `@lib/features/navigation/widgets/sidebar_page.dart`:
- Around line 597-606: Expose public tab-id constants alongside
SidebarTabScrollRegistry and use the shared constants instead of
selectedTab.name in onTap and literal registration strings such as the notes
tab’s 'notes' value. Update every tab registration and registry consumer to
reference these constants so enum member renames cannot break scrollToTop.
In `@lib/features/notes/widgets/notes_list_tab.dart`:
- Around line 389-394: Update the scrollbar selection in the notes list tab to
use context.usesCupertinoChrome, matching the predicate used by the notes list
page, while preserving the existing _scrollController and primary widget wiring
for both scrollbar implementations.
- Around line 435-459: Update the semanticLabel in the note row to include a
clear pinned-state description when note.isPinned is true, while preserving the
existing title and preview text for all notes. Use the existing note.isPinned
condition near the pin Icon so screen readers distinguish pinned and unpinned
notes.
In `@lib/features/profile/views/profile_page.dart`:
- Around line 118-124: Update _buildProfileBody and _buildSettingsItems to
return or otherwise expose the profile header separately instead of assuming it
is items.first. Remove the header entry from the settings list, render a
standalone InsetGroupedList only when the header is non-null, and pass the
remaining settingsItems unchanged to the second group.
In `@lib/features/profile/widgets/customization_tile.dart`:
- Around line 34-43: Extend the shared components in
lib/shared/widgets/utility_components.dart with an inline subtitle-widget slot
and subtitle line-limit support, then wire both profile tiles to those APIs. In
lib/features/profile/widgets/customization_tile.dart:34-43, forward
subtitleMaxLines and subtitleTrailing without allowing trailing to discard the
subtitle trailing widget. In
lib/features/profile/widgets/expandable_card.dart:40-67, render subtitleWidget
in the collapsed header slot, suppress the string subtitle when it is provided,
and avoid placing it only in the expanded child.
In `@lib/features/workspace/views/models/workspace_model_relationship_sheet.dart`:
- Around line 104-118: Update the header layout around the title and the
Cancel/Save TextButton widgets so widget.title is centered relative to the full
sheet rather than the remaining space between intrinsic-width buttons. Render
the title independently of the action row or give both action areas equal
reserved width, while preserving the existing button actions and labels.
In `@lib/l10n/app_de.arb`:
- Around line 2857-2858: Update the workspaceToolFunctionCount localization
message and its placeholder metadata to use ICU pluralization for count,
rendering the singular German form for one function and the plural form for all
other counts.
In `@lib/l10n/app_en.arb`:
- Around line 4517-4518: Convert workspaceToolFunctionCount and
markdownShowMoreLines to ICU plural messages with explicit one and other
branches, preserving the existing count and line-count placeholders while
producing singular wording for 1 and plural wording otherwise. Update the
corresponding entries in every lib/l10n/app_*.arb localization file, including
the anchor at lib/l10n/app_en.arb:4517-4518 and sibling at
lib/l10n/app_en.arb:5289-5290.
In `@lib/l10n/app_es.arb`:
- Around line 2854-2856: Update the workspaceToolFunctionCount localization
entry to use ICU plural syntax, displaying the singular form for count 1 and the
existing plural wording for other counts; keep its count placeholder typed as an
integer in the associated metadata.
In `@lib/l10n/app_fr.arb`:
- Around line 2855-2856: Update the pluralized French ARB messages
workspaceToolFunctionCount and the message near the “Afficher … lignes de plus”
entry to use ICU plural branches for their integer count placeholders, rendering
singular values as “fonction” and “ligne” and plural values with the existing
plural wording.
In `@lib/l10n/app_it.arb`:
- Around line 2854-2856: Update the workspaceToolFunctionCount localization
value to use an ICU plural expression keyed by count, rendering “1 funzione” for
one and retaining “{count} funzioni” for other counts; keep its existing count
placeholder metadata valid.
In `@lib/l10n/app_ko.arb`:
- Line 2886: Update the `markdownShowLess` Korean translation from `간단히 보기` to
`접기` so the collapse action is unambiguous.
In `@lib/l10n/app_nl.arb`:
- Around line 2855-2856: Update the ICU messages in lib/l10n/app_nl.arb at lines
2855-2856 and 3106-3107 to add singular and plural branches:
workspaceToolFunctionCount must render “1 functie” versus “{count} functies”,
and the “Nog … regel(s) tonen” message must render “Nog 1 regel tonen” versus
“Nog {count} regels tonen”.
- Line 3100: Update the Dutch translation value for
hermesMemoryKeyShortDescription to use the imperative form “Houd” instead of
“Houdt,” leaving the rest of the description unchanged.
In `@lib/shared/widgets/responsive_drawer_layout.dart`:
- Around line 1281-1297: Throttle onTabletDrawerWidthChanged in
_adjustTabletWidth so keyboard-driven width updates still update
_tabletPreferredWidth on every step but notify the consumer only after key
repetition pauses, matching the single-commit behavior of _endTabletResize.
Reuse or add a debounce mechanism tied to the existing tablet width transition
scheduling, and ensure the final width is the value passed to the callback.
- Around line 14-22: Move _kSidebarNativeBottomBarContentHeight from this file
into sidebar_layout_constants.dart and expose it for reuse by sidebar_page.dart.
Update the bottom gradient sizing reference in sidebar_page.dart to use the
shared constant, removing its duplicate private declaration while preserving the
50.0 value.
- Around line 1408-1445: Narrow the tablet resize handle collision region or
restrict drag recognition to the visible grip within _TabletSidebarResizeHandle,
while preserving the existing resize callbacks and semantics actions. Ensure
horizontal drags beginning in overlapping drawer or adjacent-content areas are
not captured unless they start on the actual handle hit target.
In `@test/features/navigation/providers/sidebar_providers_test.dart`:
- Around line 61-75: Update the sidebarTabletWidthProvider test to verify
persistence immediately after controller.setWidth(120) clamps to
minimumSidebarTabletWidth: assert the stored value, create and read from
restoredContainer before invoking controller.reset(), then retain the
reset/default assertions afterward.
In `@test/features/navigation/widgets/sidebar_page_test.dart`:
- Around line 455-457: Extend the navigation test around the CupertinoTabBar
assertions to verify its width matches the sidebar-page-surface width. Retrieve
the relevant rendered widgets and compare their widths, while preserving the
existing type and five-item assertions.
In `@test/features/workspace/views/workspace_model_editor_test.dart`:
- Around line 112-115: Update the test covering the “Keep editing” action to
assert that the workspace model ID field still contains the draft value
“unsaved-model” after tapping and settling, rather than only checking that the
field remains visible. Use the existing workspace-model-id finder and verify its
current text/value.
---
Outside diff comments:
In `@lib/features/auth/views/authentication_page.dart`:
- Around line 294-310: The _signIn success and catch paths access
hapticEnabledProvider after an await without confirming the state is still
mounted. Guard both hapticFeedbackWithSettings calls with mounted, and return
immediately at the start of the catch block when
!_AuthenticationPageState.mounted before setState or provider access.
In `@lib/features/hermes/views/hermes_settings_page.dart`:
- Around line 742-748: The tool count text in the Hermes settings view should
use an ICU pluralized localization instead of concatenating toolset.tools.length
with l10n.tools. Add the hermesToolsetToolCount plural message to the
appropriate lib/l10n/*.arb inputs, regenerate localization accessors as needed,
and update the Text expression around toolset.label to format the complete count
phrase through that localized message while preserving the disabled-label
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e160fbe-ece5-49b8-adaa-8abac4e707e8
📒 Files selected for processing (101)
lib/core/auth/webview_cookie_helper.dartlib/core/persistence/persistence_keys.dartlib/core/router/app_router.dartlib/core/widgets/error_boundary.dartlib/features/auth/views/authentication_page.dartlib/features/auth/views/backend_chooser_page.dartlib/features/auth/views/connection_issue_page.dartlib/features/auth/views/proxy_auth_page.dartlib/features/auth/views/server_connection_page.dartlib/features/auth/views/sso_auth_page.dartlib/features/auth/widgets/adaptive_auth_scaffold.dartlib/features/auth/widgets/connection_setup_components.dartlib/features/channels/widgets/channel_form_dialog.dartlib/features/channels/widgets/channel_list_tab.dartlib/features/chat/widgets/assistant_message_widget.dartlib/features/chat/widgets/composer_overflow_menu.dartlib/features/chat/widgets/file_attachment_widget.dartlib/features/chat/widgets/model_selector_sheet.dartlib/features/chat/widgets/modern_chat_input.dartlib/features/direct_connections/views/direct_connection_editor_page.dartlib/features/direct_connections/views/direct_connections_page.dartlib/features/hermes/views/hermes_settings_page.dartlib/features/hermes/widgets/hermes_session_tile.dartlib/features/hermes/widgets/hermes_sessions_tab.dartlib/features/navigation/providers/sidebar_providers.dartlib/features/navigation/providers/sidebar_tab_scroll_registry.dartlib/features/navigation/views/folder_page.dartlib/features/navigation/views/splash_launcher_page.dartlib/features/navigation/widgets/chats_drawer.dartlib/features/navigation/widgets/conversation_tile.dartlib/features/navigation/widgets/sidebar_page.dartlib/features/navigation/widgets/sidebar_user_pill.dartlib/features/notes/views/note_editor_page.dartlib/features/notes/views/notes_list_page.dartlib/features/notes/widgets/notes_list_tab.dartlib/features/profile/views/profile_page.dartlib/features/profile/widgets/customization_tile.dartlib/features/profile/widgets/expandable_card.dartlib/features/profile/widgets/settings_page_scaffold.dartlib/features/terminal/widgets/terminal_tab.dartlib/features/workspace/models/workspace_model_draft.dartlib/features/workspace/models/workspace_prompt_command.dartlib/features/workspace/models/workspace_tool_content.dartlib/features/workspace/providers/workspace_knowledge_files.dartlib/features/workspace/views/knowledge/workspace_knowledge_editor.dartlib/features/workspace/views/models/workspace_model_editor.dartlib/features/workspace/views/models/workspace_model_relationship_sheet.dartlib/features/workspace/views/prompts/workspace_prompt_editor.dartlib/features/workspace/views/skills/workspace_skill_editor.dartlib/features/workspace/views/tools/workspace_tool_editor.dartlib/features/workspace/views/workspace_page.dartlib/features/workspace/widgets/workspace_editor_scaffold.dartlib/features/workspace/widgets/workspace_grouped_components.dartlib/features/workspace/widgets/workspace_import_sheet.dartlib/features/workspace/widgets/workspace_tiles.dartlib/features/workspace/widgets/workspace_tool_url_import_sheet.dartlib/features/workspace/widgets/workspace_tool_valves_sheet.dartlib/features/workspace/widgets/workspace_valve_form.dartlib/l10n/app_cs.arblib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_ko.arblib/l10n/app_nl.arblib/l10n/app_ru.arblib/l10n/app_sk.arblib/l10n/app_zh.arblib/l10n/app_zh_Hant.arblib/main.dartlib/shared/utils/conversation_context_menu.dartlib/shared/utils/locale_display_formatters.dartlib/shared/widgets/adaptive_selection_sheet.dartlib/shared/widgets/drawer_shell_page.dartlib/shared/widgets/horizontal_overflow_fade.dartlib/shared/widgets/markdown/markdown_config.dartlib/shared/widgets/model_list_tile.dartlib/shared/widgets/platform_ui/src/adaptive_layout.dartlib/shared/widgets/responsive_drawer_layout.dartlib/shared/widgets/sidebar_ios26_scaffold.dartlib/shared/widgets/sidebar_layout_constants.dartlib/shared/widgets/themed_sheets.dartlib/shared/widgets/utility_components.darttest/core/widgets/error_boundary_test.darttest/features/auth/views/adaptive_auth_flow_test.darttest/features/chat/widgets/assistant_message_widget_footer_test.darttest/features/direct_connections/direct_connections_ui_test.darttest/features/hermes/hermes_settings_test.darttest/features/navigation/providers/sidebar_providers_test.darttest/features/navigation/providers/sidebar_tab_scroll_registry_test.darttest/features/navigation/widgets/conversation_tile_test.darttest/features/navigation/widgets/sidebar_native_avatar_test.darttest/features/navigation/widgets/sidebar_page_test.darttest/features/workspace/views/workspace_model_editor_test.darttest/features/workspace/views/workspace_prompt_editor_test.darttest/features/workspace/views/workspace_shell_test.darttest/shared/utils/conversation_context_menu_test.darttest/shared/widgets/platform_ui/platform_ui_test.darttest/shared/widgets/responsive_drawer_layout_test.dart
|
Applied the reviewed fixes in 0dbc0e6.\n\nHighlights:\n- hardened the global error fallback and async auth lifecycle handling\n- removed redundant Open WebUI sign-in copy while preserving the segmented method control\n- improved connection semantics, keyboard access, localization, and plural handling\n- strengthened sidebar width persistence, scroll registration, and regression coverage\n- added focused tests for full-width tablet navigation, draft preservation, and resize coalescing\n\nThe sidebar resize interaction intentionally retains its 44 pt accessible hit target; narrowing it would conflict with the approved minimum touch-target requirement.\n\nValidation: Flutter analyze clean; all 5,243 tests passed with 4 expected skips; ARB and release-note validators passed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/features/direct_connections/views/direct_connection_editor_page.dart (1)
724-727: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSave the profile that was probed during onboarding.
_testConnectionprobes a draft from the current controllers._savethen builds a new draft from controllers that remain editable during the probe. A user can change the URL, authentication mode, or headers during the request. The editor can then show a successful connection attempt for one profile and save a different, unprobed profile.Keep and persist the tested draft, or disable editing for the complete test-and-save operation. Add a regression test that changes a field while the probe is pending.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/direct_connections/views/direct_connection_editor_page.dart` around lines 724 - 727, Update _connectAndSave and the surrounding test/save flow to retain the exact draft passed to _testConnection and pass that same tested draft into _save, preventing controller edits during the pending probe from changing the persisted profile. Alternatively, disable all relevant editor fields for the entire test-and-save operation; add a regression test that changes a field while the probe is pending and verifies the probed profile is saved.lib/features/channels/widgets/channel_list_tab.dart (1)
382-419: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the unread count in
semanticLabel.The visual badge exposes unread updates, but
semanticLabelcontains only the channel name and description. Screen-reader users cannot detect unread messages. Add a localized unread-count phrase whenunread > 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/channels/widgets/channel_list_tab.dart` around lines 382 - 419, The channel item’s semanticLabel currently omits unread updates. Update the semanticLabel construction near the channel row to append a localized unread-count phrase when unread > 0, while preserving the existing displayName and description content and leaving labels for unread == 0 unchanged.test/features/navigation/widgets/sidebar_page_test.dart (1)
394-435: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest the last tab in five-tab mode.
This test proves that five labels render, but it never selects
Channels._TestSidebarActiveTab.setclamps every index to0..3at Line 2598. The harness cannot represent the fifth destination and can hide a badChannelsselection. TapChannels, assert its production index and active state, and update the test notifier to support that index.Suggested coverage
expect(controllers.activeTabNotifier.currentValue, 2); + + await tester.tap(_sidebarBottomNavTabLabel('Channels')); + await tester.pumpAndSettle(); + expect(controllers.activeTabNotifier.currentValue, 4);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/navigation/widgets/sidebar_page_test.dart` around lines 394 - 435, Update _TestSidebarActiveTab.set to allow the fifth tab’s production index instead of clamping all values to 0..3, then extend the persistent tablet five-tab test to tap the Channels label and assert its expected active index and active-state rendering. Keep the existing Notes assertions and verify the Channels selection through the same production-facing state indicators.test/features/workspace/views/workspace_model_editor_test.dart (1)
94-125: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
package:checksfor the compact toolbar discard-flow assertions.
test/features/workspace/views/workspace_model_editor_test.dartstill only importsflutter_test, but the compact toolbar back confirmation now uses newexpect(...)calls. Replace those assertions withcheck(...)expressions; usemocktailonly if this coverage needs new test doubles.[maintainability_and_refactor]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/features/workspace/views/workspace_model_editor_test.dart` around lines 94 - 125, Update the testWidgets case “compact toolbar back confirms before discarding edits” to use package:checks check(...) assertions instead of expect(...). Add the checks import and preserve the existing assertions for dialog visibility, retained field contents, and final navigation; do not add mocktail unless new test doubles are required.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/shared/widgets/responsive_drawer_layout_test.dart`:
- Around line 627-652: In the test “tablet divider coalesces repeated keyboard
commits,” pump the tester immediately after sending the Tab key and before the
first ArrowRight so FocusScope completes traversal to the resize handle. Match
the existing post-Tab pump used by the other resize keyboard tests, leaving the
subsequent key sequence and timing assertions unchanged.
---
Outside diff comments:
In `@lib/features/channels/widgets/channel_list_tab.dart`:
- Around line 382-419: The channel item’s semanticLabel currently omits unread
updates. Update the semanticLabel construction near the channel row to append a
localized unread-count phrase when unread > 0, while preserving the existing
displayName and description content and leaving labels for unread == 0
unchanged.
In `@lib/features/direct_connections/views/direct_connection_editor_page.dart`:
- Around line 724-727: Update _connectAndSave and the surrounding test/save flow
to retain the exact draft passed to _testConnection and pass that same tested
draft into _save, preventing controller edits during the pending probe from
changing the persisted profile. Alternatively, disable all relevant editor
fields for the entire test-and-save operation; add a regression test that
changes a field while the probe is pending and verifies the probed profile is
saved.
In `@test/features/navigation/widgets/sidebar_page_test.dart`:
- Around line 394-435: Update _TestSidebarActiveTab.set to allow the fifth tab’s
production index instead of clamping all values to 0..3, then extend the
persistent tablet five-tab test to tap the Channels label and assert its
expected active index and active-state rendering. Keep the existing Notes
assertions and verify the Channels selection through the same production-facing
state indicators.
In `@test/features/workspace/views/workspace_model_editor_test.dart`:
- Around line 94-125: Update the testWidgets case “compact toolbar back confirms
before discarding edits” to use package:checks check(...) assertions instead of
expect(...). Add the checks import and preserve the existing assertions for
dialog visibility, retained field contents, and final navigation; do not add
mocktail unless new test doubles are required.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2b4cae56-f4aa-401a-8d99-7c3549496c3e
📒 Files selected for processing (41)
lib/core/widgets/error_boundary.dartlib/features/auth/views/authentication_page.dartlib/features/auth/views/connection_issue_page.dartlib/features/auth/views/proxy_auth_page.dartlib/features/auth/views/sso_auth_page.dartlib/features/auth/widgets/connection_setup_components.dartlib/features/channels/widgets/channel_list_tab.dartlib/features/direct_connections/views/direct_connection_editor_page.dartlib/features/hermes/views/hermes_settings_page.dartlib/features/hermes/widgets/hermes_sessions_tab.dartlib/features/navigation/providers/sidebar_providers.dartlib/features/navigation/providers/sidebar_tab_scroll_registry.dartlib/features/navigation/widgets/chats_drawer.dartlib/features/navigation/widgets/conversation_tile.dartlib/features/navigation/widgets/sidebar_page.dartlib/features/notes/widgets/notes_list_tab.dartlib/features/profile/views/profile_page.dartlib/features/profile/widgets/customization_tile.dartlib/features/terminal/widgets/terminal_tab.dartlib/features/workspace/views/models/workspace_model_relationship_sheet.dartlib/l10n/app_cs.arblib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_ja.arblib/l10n/app_ko.arblib/l10n/app_nl.arblib/l10n/app_ru.arblib/l10n/app_sk.arblib/l10n/app_zh.arblib/l10n/app_zh_Hant.arblib/shared/widgets/responsive_drawer_layout.dartlib/shared/widgets/sidebar_layout_constants.dartlib/shared/widgets/utility_components.darttest/features/auth/views/adaptive_auth_flow_test.darttest/features/navigation/providers/sidebar_providers_test.darttest/features/navigation/widgets/sidebar_page_test.darttest/features/workspace/views/workspace_model_editor_test.darttest/shared/widgets/responsive_drawer_layout_test.dart
|
Follow-up review findings addressed in 524282a.\n\nFixed:\n- reduced-motion tab reselection now jumps instead of calling animateTo with a zero duration across all sidebar tabs\n- terminal trailing actions retain independent accessibility semantics\n- proxy/SSO WebView bodies regain safe-area protection\n- proxy failures retain contextual recovery details instead of repeating the heading\n- Hermes onboarding locks connection fields while probing so the persisted values match the tested draft\n- saved credentials hydrate before authentication errors are surfaced\n- sidebar selection now persists a stable tab identity and restores it after optional features return\n\nThe 320 pt minimum sidebar behavior remains intentional. At a 600 pt viewport it is mathematically impossible to preserve both a 320 pt sidebar and a 320 pt content pane; the approved design prioritizes the 320 pt sidebar minimum needed for five navigation items.\n\nValidation: Flutter analyze clean; 144 focused tests passed; full suite passed with 5,245 tests and 4 expected skips. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/hermes/views/hermes_settings_page.dart (1)
517-619: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock edits and overlapping connection operations.
While
_savingis true, the fields remain enabled._commitConnectioncan then clear credentials and reset dirty flags after the user enters newer values. The newer values are lost, and the success state describes the earlier save.Disable the fields and both actions while
_saving,_testing, or_finishingis true.Proposed fix
- enabled: !_finishing, + enabled: !(_finishing || _saving || _testing),Apply this to all three fields. Also require
!_testing && !_finishingbefore enabling Save.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/hermes/views/hermes_settings_page.dart` around lines 517 - 619, Update the three AccessibleFormField instances for the server URL, API key, and memory key so enabled also requires !_saving, !_testing, and !_finishing. Update both Save and Test Direct Connection actions to remain disabled whenever any of those operation flags is true, including requiring !_testing && !_finishing for Save. Preserve the existing draft validation and loading behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/navigation/providers/sidebar_providers.dart`:
- Around line 31-33: Update SidebarTabId’s set method to observe the Future
returned by PreferencesStore.put, handling asynchronous persistence failures
instead of leaving them unhandled. Log failures through DebugLogger with a
slash-scoped sidebar scope, while preserving the immediate state update and
existing preference key.
In `@lib/features/navigation/widgets/sidebar_user_pill.dart`:
- Around line 278-284: The search-hint selection around the tab switch must use
the resolved visible tab rather than the persisted SidebarTabId. Update the
surrounding function and its caller to resolve unavailable tabs using the same
visibility logic as SidebarPage, including terminalTabVisibleProvider, so hidden
Terminal falls back to Chats before selecting l10n.searchFiles or another
tab-specific hint.
In `@test/features/navigation/widgets/sidebar_page_test.dart`:
- Around line 407-419: Strengthen the reduced-motion test around
_buildSidebarHarness by providing scrollable tab content, scrolling it away from
offset zero, then reselecting the active Chats tab. Assert the scroll controller
returns to offset zero immediately, without pumping an animation duration, and
retain the exception check only if still relevant.
---
Outside diff comments:
In `@lib/features/hermes/views/hermes_settings_page.dart`:
- Around line 517-619: Update the three AccessibleFormField instances for the
server URL, API key, and memory key so enabled also requires !_saving,
!_testing, and !_finishing. Update both Save and Test Direct Connection actions
to remain disabled whenever any of those operation flags is true, including
requiring !_testing && !_finishing for Save. Preserve the existing draft
validation and loading behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a8185f3b-61d4-4c36-8e47-5f4e3b1ab02d
📒 Files selected for processing (17)
lib/features/auth/views/authentication_page.dartlib/features/auth/views/proxy_auth_page.dartlib/features/auth/widgets/connection_setup_components.dartlib/features/channels/widgets/channel_list_tab.dartlib/features/hermes/views/hermes_settings_page.dartlib/features/hermes/widgets/hermes_sessions_tab.dartlib/features/navigation/providers/sidebar_providers.dartlib/features/navigation/utils/sidebar_create_action.dartlib/features/navigation/widgets/chats_drawer.dartlib/features/navigation/widgets/sidebar_page.dartlib/features/navigation/widgets/sidebar_user_pill.dartlib/features/notes/widgets/notes_list_tab.dartlib/features/terminal/widgets/terminal_tab.dartlib/shared/widgets/utility_components.darttest/features/navigation/providers/sidebar_providers_test.darttest/features/navigation/widgets/sidebar_page_test.darttest/shared/widgets/responsive_drawer_layout_test.dart
|
Addressed the remaining Greptile migration finding in 78b3427. Legacy numeric sidebar selections now map through the historical fixed order (Chats, Hermes, Notes, Terminal, Channels) before capability filtering. If that identity is temporarily unavailable, the UI shows the normal fallback without persisting an unrelated visible tab. Added regression coverage for both available and unavailable legacy identities; Flutter analysis and the targeted navigation suite pass. |
|
Follow-up outside-diff audit completed in 2ab354c. The remaining valid findings are now fixed in source: Direct onboarding persists the exact probed draft with race coverage; channel semantics include a localized unread count across all 13 locales; tablet coverage selects Channels; the Workspace discard test uses package:checks; and Hermes fields/actions block overlapping save/test operations. Previously completed mounted guards, Hermes pluralization, and legacy sidebar identity migration were reverified. Flutter analysis and 123 targeted tests pass. |
Guard Hermes save completion after disposal and split direct-connection form state from async workflow state. Make workspace editors use setState as their sole rebuild owner and keep model relationship UI provider-free.
Hydrate Hermes secrets before rollback snapshots and guard disposed direct-editor conflicts. Replace forwarding and implementation-coupled UI boundaries with typed editor, sidebar, and workspace contracts.
Route Direct onboarding through retained profiles so unset-backend recovery cannot force duplicate setup. Narrow editor and navigation contracts, fence Hermes operations behind private immutable state, and split utility widgets to prevent the coupling that allowed these regressions to recur.
Capture workspace mutation completion outside provider-invalidated forms so successful saves navigate safely, retain create-mode model grants in the draft, and avoid popping routes opened during a pending write. Consolidate direct editor notifications, replace drawer render probing with typed scroll boundaries, and keep tablet resize policy within navigation.
Bind terminal file operations to the confirmed server and session context. Decouple reusable gesture and editor layers from owning features, centralize workspace mutation lifecycles, and share terminal section components.
|
Too many files changed for review (429 files, 200 file limit). Bypass the limit by tagging |
…#643) * fix: resolve reported rendering, voice, terminal, and connection bugs LaTeX (#642, #533): render complex MathJax SVGs through the compatible renderer so underbrace/boxed/pmatrix no longer glitch or leak `amp`, and center block equations horizontally. Fonts (#501): rebuild the bundled Geist faces, which mapped lowercase Greek omega to the uppercase glyph. Hermes (#637): connection tests now exercise authenticated capability and toolset discovery instead of the unauthenticated health endpoint, so a server that only answers /health no longer reports "connected". Voice (#632): request microphone permission before starting the Android foreground service, which crashed when permission had never been granted. Voice (#630): stop local STT while TTS plays so the model no longer hears itself. Barge-in is off by default and opt-in from Audio settings, and the voice overlay gains a speakerphone toggle. Transport (#629): drop the plaintext-HTTP restriction for Hermes and direct connections. Tailscale/Headscale CGNAT addresses and other private networks were rejected outright. Custom headers and the self-signed certificate toggle are now shared by all backends via a single Hermes transport configurator. Terminal (#626): download now opens the native save dialog instead of the share sheet. Terminal (#608): route the `terminal:display_file` event into the Terminal files panel rather than dropping it in the chat event handler. Auth (#606): drive reverse-proxy login through the real WebView identity and cookie store so Authelia and similar providers finish sign-in. Also fixes pre-existing test failures on main: three assertions still expected the single-newline block join replaced in #641, and six expected the "Delete connection" button label renamed to "Delete" in #628. * fix: route Android speakerphone and address review feedback Speakerphone on Android: setSpeakerphoneOn is deprecated and no-ops on Android 12+ once a communication device is selected, which the voice session does for Bluetooth SCO. The toggle rendered but never changed the route. Select the built-in speaker as the communication device instead, keeping the legacy call as the pre-31 fallback, and tear down the active SCO route before re-routing so a headset cannot keep owning playback. Voice: treat TtsError as a terminal assistant-speech event. With barge-in disabled the recognizer is stopped for playback and only TtsCompleted restarted it, so a failed TTS request left voice mode active with no way to accept the next utterance. Hermes: cancel active runs when certificate trust changes. A trust-only edit rebuilt the transport while old cancellation tokens stayed live. Terminal: sanitize the download file name, which comes from the server's Content-Disposition header, before passing it to the save dialog. Hermes settings: use the existing headerName/headerValue/addHeader/ removeHeader localizations instead of hardcoded English. Adds coverage for the speakerphone toggle, TTS-error recovery, and HttpClient forwarding through the desktop RPC channel factory. * fix: fall back for dot-only download file names `.` and `..` survive character sanitization but name a directory rather than a file, so writing them throws instead of producing a download. Use the timestamp fallback for those, and cover the sanitizer directly. Windows reserved device names are not handled: this app ships only iOS and Android targets.
Upstream release summary (21 commits since v4.0.3): - Migrate adaptive UI to native iOS glass (cogwheel0#622) and unify app surfaces with native grouped design (cogwheel0#628) — large-scale settings/navigation redesign using new UtilityRow/InsetGroupedList components and a platform_ui/cupertino_ui/material_ui package split replacing adaptive_platform_ui. - Hermes Desktop Gateway backend (cogwheel0#636) and Hermes Bot Mode roster with grouped assistant responses (cogwheel0#644). - Chat scrolling and markdown rendering perf work (cogwheel0#640), semantic details block separation (cogwheel0#641), various post-merge regression fixes. - Polish localization added (cogwheel0#617); ARB "What's new" release notes banner infrastructure. - Bump to Flutter/pubspec version 4.1.0+141. Merge conflicts resolved (16 files): - pubspec.yaml: took upstream version bump and flutter_riverpod pin; preserved the sqlite3 system-source hook block. - lib/l10n/app_*.arb (13 files) + app_pl.arb (new, non-conflicted): proper 3-way key-level merge (base=v4.0.3) instead of line-based — kept fork-customized values (appTitle=EOchat, palette strings, etc.), took all upstream additions, and dropped 15 confirmed-dead legacy keys (old callkit* names, appearanceTitle, dataAndConnectionTitle, enterServerAddress, directSetupRequiresConnection) that upstream already renamed/removed and no Dart code still referenced. Set appTitle=EOchat in the new app_pl.arb too. - lib/features/auth/views/server_connection_page.dart: kept ForkOverrides.forceSsoOnly branch, added upstream's ConduitHaptics success feedback to both branches. - lib/shared/widgets/sidebar_ios26_scaffold.dart: took upstream's new native-glass rewrite, re-wrapped it in the fork's CupertinoTheme override so the native tab bar keeps the EOchat sidebar tint. - lib/features/navigation/widgets/sidebar_user_pill.dart + lib/features/profile/views/profile_page.dart: adopted upstream's new sections-based native sheet / donation layout while keeping donation links behind ForkOverrides.showDonationLinks (default off). - android/app/build.gradle.kts: kept nl.eo.eochat namespace, took upstream's dynamic compileSdk. - lib/core/providers/app_providers.dart: kept EOchat demo-mode copy. - lib/features/auth/views/authentication_page.dart: kept forceSsoOnly/canUseSsoFlow gating and the fork's OAuth button list (still used by the SSO-only path), added upstream's new SSO description widget and its initState race-condition fix, dropped a now-dead SSO button guard superseded by upstream's unified sign-in button. - lib/features/chat/views/chat_page.dart: took upstream's Hermes bot toolbar title (also fixes a pre-existing duplicate sidebar-toggle button bug in the fork's old title builder); kept the fork's theme-aware temporary-chat tint color, updated to upstream's consolidated kConduitNativeSingleActionSymbolExtent icon-size constant. - lib/features/navigation/views/folder_page.dart: took upstream (fork's side duplicated the drawer toggle button and referenced out-of-scope variables). - lib/features/navigation/widgets/chats_drawer.dart: switched to upstream's ConduitIconButton while keeping the fork's showCreateAction gate (fork-only addition, hides the button in read-only contexts). - lib/features/navigation/widgets/sidebar_page.dart: took upstream's renderer enum (replaces the removed useNativeBottomBar bool), kept the fork's sidebarPrimary tab bar tint color. - lib/features/profile/views/about_page.dart: adopted upstream's new UtilityRow-based card layout; retitled the GitHub row to the fork's "Based on Conduit by cogwheel0" attribution instead of upstream's generic label. - lib/main.dart: kept ForkOverrides.showReleaseNotesBanner gating (upstream's banner is written in cogwheel0's voice and links to upstream's own store listings) while adding upstream's new native theme sync and LegacyDesignCompatibility wrapper. - lib/shared/services/brand_service.dart: kept the fork_overrides.dart import. Cross-file fix found during merge: upstream consolidated six kConduitNative*SymbolExtent icon-size constants into one kConduitNativeSingleActionSymbolExtent in adaptive_toolbar_components.dart. That file merged cleanly (fork never customized it), but chat_page.dart still had two stale references to the old removed constants inside its conflict hunks; updated both to the new constant / removed the parameter where upstream's method signature no longer accepts it. Branding sweep (en/nl + CarPlay/App Intents): - Fixed leftover "Conduit" in genuinely user-facing strings: ARB ttsPreviewText, notificationSystemDescription, hermesMemoryKeyDescription (en) and hermesMemoryKeyShortDescription (nl); iOS CarPlay UI strings in ConduitCarPlayBridge.swift/ConduitCarPlaySceneDelegate.swift; the App Intent timeout message in AppDelegate.swift. - Left untouched per documented exceptions: the donation section's "Support Conduit" copy (gated off by default), the release-notes banner's first-person cogwheel0 copy (gated off by default), the "Conduit" theme palette proper noun, the About page's upstream attribution line, and internal non-user-facing identifiers (class names, User-Agent product string, background task/haptics labels). Local validation: no conflict markers remain, all ARB files valid JSON, sqlite3 hook and DEVELOPMENT_TEAM intact, app_router.dart imports present, all edited Dart files brace/paren-balanced, pubspec.yaml valid YAML. Flutter was not available in this sandbox to run `flutter analyze` locally; CI's analyze.yml will be the first automated check after push.
Summary
Testing
flutter pub getdart run build_runner builddart run tool/validate_arb_locales.dartdart run tool/verify_arb_descriptions.dartdart run tool/validate_release_notes.dart --version 4.0.3flutter analyzeflutter test(5,245 passed, 4 skipped)Notes
The ARB validator reports the repository's existing placeholder-metadata warnings but exits successfully. No packages, migrations, server APIs, or persisted data schemas are changed.
Note
Unify app surfaces with native grouped design across workspace, auth, navigation, and settings
material/cupertinoimports across the codebase withmaterial_ui/cupertino_uiplatform packages and migrates most settings, auth, and profile pages toUtilityPageScaffold,InsetGroupedList,UtilityRow, and related grouped-surface widgets.ResizableTabletSidebar) with pointer and keyboard resize, debounced width persistence, RTL support, and semantics for assistive technologies;ResponsiveDrawerLayoutnow mediates gestures viaHorizontalGesturePriorityScopeandDrawerOpenDragGestureRecognizer.NativeSheetTheme/NativeSheetHapticsin Swift and a matching DartNativeSheetThemeConfig/syncThemepath so native sheet UI automatically inherits app theme colors at runtime.WorkspaceResourceEditorRoute,WorkspaceEditorSession,WorkspaceEditorMutationCoordinator, andWorkspaceEditorOperationRunner, standardizing loading, dirty/saving/error state, and mutation lifecycle.SidebarTabRegistrydriven bySidebarTabDescriptorentries, replaces index-based tab state withSidebarTabId, and introducesSidebarTabScrollRegistryfor scroll-to-top on reselection.DirectConnectionEditorWorkflow/DirectConnectionEditorForm/DirectConnectionEditorGateway, adds advanced settings page, and introduces editor controller tests and ownership-change tests.ErrorBoundarywithinstallConduitErrorWidgetBuilder/ConduitFriendlyErrorViewas the app's last-resort error surface;ErrorBoundaryis no longer available.AppSettings;ConduitHapticsnow uses only FlutterHapticFeedbackAPIs and thestreamingHapticsEnabledflag depends solely ondisableHapticsWhileStreaming.HorizontalGestureExclusion,HorizontalScrollGestureBoundary,HorizontalOverflowFade) used in chat input, file attachments, markdown tables, and LaTeX blocks.ErrorBoundaryis fully removed; pages previously wrapped in it will surface errors directly to the Flutter error widget builder. The haptic-feedback setting is removed from storedAppSettings; any persisted value is silently dropped.Macroscope summarized 3b42bc1.
Summary by CodeRabbit
Greptile Summary
The shared utility-row layout now applies flexible sizing consistently to long values, and sidebar tab preferences retain legacy selections until the user confirms a stable tab identity.
Confidence Score: 5/5
No blocking failure remains.
No accepted blocking findings remain.
What T-Rex did
Reviews (15): Last reviewed commit: "fix: keep utility value flex under its r..." | Re-trigger Greptile