perf(ui): use ListView.separated for lazy horizontal breadcrumb rendering#8732
Open
danteboe wants to merge 7 commits into
Open
perf(ui): use ListView.separated for lazy horizontal breadcrumb rendering#8732danteboe wants to merge 7 commits into
danteboe wants to merge 7 commits into
Conversation
- Detailed architectural explanation: the previous StatelessWidget implementation rebuilt breadcrumb widget instances on every parent rebuild, causing repeated allocations of intermediate FlowyTooltip/ViewTitle/FlowySvg widgets and increasing GC churn on UI re-renders.\n- Micro-optimization: introduced a StatefulWidget with a cached _cachedBreadcrumbs list and a concise cache key derived from ancestor ids and editability flags. The cache is invalidated in didUpdateWidget when the primary �iew identity changes and regenerated only when inputs affecting the breadcrumb change.\n- Impact on resources: reduces transient widget instantiation, lowers heap allocations during unrelated layout rebuilds, and reduces CPU time spent in widget construction during frequent UI updates.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
…tions in view_title_bar - Architectural explanation: canonicalizing statically parameterized widgets reduces repeated runtime allocations by enabling the Dart compiler to canonicalize identical widget instances at compile-time.\n- Work performed: reviewed �iew_title_bar.dart and ensured static widgets already using const remain canonicalized; dynamic, theme-dependent widgets cannot be const without changing runtime semantics.\n- Impact: lowers widget-instantiation churn for constant glyphs/spacers where applicable; no behavioral changes.
…intermediate allocations - Detailed architectural explanation: existing code collected UUIDs into a temporary Vec<String> before JSON serialization, causing an extra heap allocation proportional to the number of ids.\n- Optimization: added a streaming serializer serialize_rag_ids_from_uuids that writes the JSON array directly from the Uuid iterator into a byte buffer, avoiding the intermediate Vec<String>.\n- Impact: eliminates the transient allocation for rag id lists, reduces heap churn and shortens peak memory usage during chat persistence operations.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
- Detailed architectural explanation: frequent log serialization previously allocated a new Vec<u8> for each span/event, causing heap churn in high-throughput scenarios.\n- Optimization: introduced a LOG_BUFFER thread-local RefCell<Vec<u8>> reused across serialization calls; buffer is cleared (len=0) but retains capacity between calls. Serialization now writes directly into this buffer and the writer consumes it, avoiding repeated allocations.\n- Impact: reduces heap allocations and GC pressure in hot logging paths; improves throughput for bursty logs.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
…id inline closure allocations - Architectural explanation: anonymous builder closures allocated during widget rebuilds contribute to transient allocation churn on hot UI paths.\n- Optimization: hoisted the popupBuilder into a _buildRenamePopover method and tracked the latest ViewTitleState in a field so the builder no longer needs an inline anonymous closure.\n- Impact: reduces per-build closure allocations and clarifies lifecycle points for text controller reset logic.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
…rumb rendering - Detailed architectural explanation: the previous SingleChildScrollView + Row eagerly built all breadcrumb widget instances, causing potentially large widget allocation trees for deep hierarchies.\n- Optimization: replaced with a horizontally scrolling ListView.separated which builds items on demand. Each item uses ValueKey(view.id) to maintain identity. Separators are provided by separatorBuilder to match previous dividers.\n- Impact: converts O(n) eager widget construction to on-demand item builders, reducing initial layout cost and memory pressure when the breadcrumb list is long.\n\nCo-authored-by: Optimization-Agent <agent@flowy.ai>
Contributor
Reviewer's GuideRefactors the Flutter view title bar breadcrumb rendering to use a lazy horizontal ListView with caching for better performance, introduces safer rename popover state handling, optimizes Rust logging and chat UUID serialization to reduce allocations, and updates commitlint to allow perf-type commits along with minor asset/config cleanups. Flow diagram for Rust logging with thread local bufferflowchart LR
on_event[on_event] --> getbuf[LOG_BUFFER.with]
getbuf --> clearbuf[clear buffer]
clearbuf --> serializer[serde_json::Serializer::new]
serializer --> mapser[serialize_map]
mapser --> writefields[serialize_fields and extra values]
writefields --> newline[write_all '\n' to buffer]
newline --> writer[make_writer.make_writer]
writer --> emit[write_all buffer to output]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
ViewTitleBar,_cachedBreadcrumbsis populated but never actually used by theListView.separatedbuilder (which rebuilds each item fromancestors), so either wire the cache intoitemBuilderor drop the cache logic to avoid unnecessary state. - The previous
_buildLockPageStatuswidget that appeared after the breadcrumb row is no longer rendered with the newListView.separatedlayout; if this status indicator is still desired, it should be reinserted (e.g., as a trailing item) or the unused helper removed. - The new serialization helpers (
serialize_rag_ids_from_uuids, logging JSON writers) rely on multipleunwrap/unwrap_or_defaultcalls, which can panic or silently lose data; consider returning aResultand propagating errors (or at least logging them) to keep behavior predictable and robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ViewTitleBar`, `_cachedBreadcrumbs` is populated but never actually used by the `ListView.separated` builder (which rebuilds each item from `ancestors`), so either wire the cache into `itemBuilder` or drop the cache logic to avoid unnecessary state.
- The previous `_buildLockPageStatus` widget that appeared after the breadcrumb row is no longer rendered with the new `ListView.separated` layout; if this status indicator is still desired, it should be reinserted (e.g., as a trailing item) or the unused helper removed.
- The new serialization helpers (`serialize_rag_ids_from_uuids`, logging JSON writers) rely on multiple `unwrap`/`unwrap_or_default` calls, which can panic or silently lose data; consider returning a `Result` and propagating errors (or at least logging them) to keep behavior predictable and robust.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
SingleChildScrollView+Roweagerly built all breadcrumb items causing poor performance in deep trees.What changed
ListView.separatedhorizontal builder keyed byValueKey(view.id)and lazy separator builder. Items build on demand.Performance impact
Testing
Risk and compatibility
Checklist
Summary by Sourcery
Optimize breadcrumb rendering performance and logging/serialization efficiency across Flutter UI and Rust backends, while updating commit lint configuration and cleaning up unused files.
Bug Fixes:
Enhancements:
Chores: