You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(plugins): #1738 follow-ups: fuzz lock, one notice part for cut trees, a UI test for the note (#1546) - #1754
Follow-up to #1738 (#1546), which was merged before its last review round. This carries the points from that round.
What changes
fuzz/Cargo.lock is updated.fix(plugins): a panel tree holds 20,000 nodes and says when it cuts (#1546) #1738 moved unicode-normalization from rustgis-ui-egui to rustgis-plugins, and the fuzz lock still listed rustgis-plugins without it (review thread). The fuzz workflow's first job runs cargo deny --manifest-path fuzz/Cargo.toml --locked …, and --locked refuses a lock that needs updating. With the lock on main, cargo metadata --manifest-path fuzz/Cargo.toml --locked fails ("cannot update the lock file … because --locked was passed"), and with this one it passes. cargo update -p rustgis-plugins in fuzz/ also adds quick-xml under rustgis-data, which the lock had lost track of on main before fix(plugins): a panel tree holds 20,000 nodes and says when it cuts (#1546) #1738. cargo check --locked --bins in fuzz/ builds.
Trees cut in one answer are said in one part of the warning. AGENTS.md ("Never silent") asks for one notice per kind, with a count. fix(plugins): a panel tree holds 20,000 nodes and says when it cuts (#1546) #1738 added one part per cut tree, so two trees cut in one answer gave two parts (review thread). They now share one part, with the count, the limit once and each tree with the nodes it was given (up to 10 named, as the other parts do): 2 tree(s) cut: only the first 20000 nodes are kept, the most a tree holds: catalogue (20001 nodes given), places (20002 nodes given).
The note under a cut tree has a UI test (review thread). a_cut_tree_says_so_under_its_rows_and_an_empty_search (kittest) opens a plugin's panel, gives its tree MAX_TREE_NODES + 1 nodes, and finds the note, with the count given and the limit twice, under the rows and under a search for the node left out ("Nothing matches."). A tree given 3 nodes after that shows no note.
Measurement: applying a 20,000-node answer
This answers the performance question: PanelState::apply runs on the thread that ran the plugin's command, which on the desktop is the UI thread. The measurement used a throwaway test, not in this PR, in the workspace's test profile on Windows. It ran three times over 25 branches of municipality-like labels with accents, with ids and counts.
answer
parse the JSON
apply
total
5,000 nodes (0.3 MB)
3.7 / 6.3 / 4.0 ms
15.6 / 17.3 / 10.7 ms
15–24 ms
20,000 nodes (1.2 MB)
17.9 / 15.0 / 18.2 ms
40.2 / 37.9 / 36.3 ms
53–58 ms
So the largest answer a tree takes costs about three 60 fps frames, once, when the plugin sends it (usually as the panel opens), not at every frame. Folding the labels takes about 12 ms of the 38. A path that skips decomposition for ASCII characters would save about 6 ms of that, which is too little to be worth a second folding function, so it isn't done. The work is bounded by the limit. Moving apply off the UI thread would change how every plugin command's answer reaches the panel, a larger change and not part of this PR.
Screenshots
These are rendered with the same wgpu renderer the ui_shot example uses, by a throwaway test (not in this PR), over a tree of 5 regions, 27 states and 800 municipalities each (21,632 nodes). The warning in the status bar is the new single part.
The note under the rows:
A search for a municipality of the last state, left out at the limit:
Tests
a_tree_cut_at_its_limit_says_so (plugins) now also cuts two trees in one answer and checks the single part.
a_cut_tree_says_so_under_its_rows_and_an_empty_search (ui-egui) is new.
Locally (Windows): cargo fmt --check passes, and so does cargo clippy -p rustgis-plugins -p rustgis-engine -p rustgis-ui-egui --all-targets -- -D warnings. The tests of rustgis-plugins and of the rustgis-ui-egui library pass too. cargo deny isn't installed here, so the lock was checked with cargo metadata --locked as described above. Main doesn't build at the moment: crates/engine/src/cmd/layout.rs passes &[] to legend_sizes_mm, whose signature changed in #1739. #1751 fixes that. The clippy run and the test runs above had #1751's one-line change applied locally, which this PR doesn't include, so this PR's CI fails on main's error until #1751 is merged.
When multiple trees exceed the retained-node limit in one panel response, they now appear together in a single notice instead of generating separate notices. The notice includes each listed tree’s node count and indicates when additional trees were cut off.
Tests
Added coverage for notices involving multiple cut trees and for tree truncation notes shown during searches, including searches with no matches.
…r cut trees, a UI test for the note (opengeos#1546)
- fuzz/Cargo.lock lists unicode-normalization under rustgis-plugins (and
quick-xml under rustgis-data), so `cargo deny --locked` in the fuzz
workflow takes it again.
- Trees cut in one answer share one part of the warning, with the count,
the limit once and each tree's given nodes ("Never silent").
- A kittest finds the note under a cut tree's rows and under an empty
search, and none under a tree within its limit.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dropped::notice reports cut trees in one notice, with up to MAX_NAMED tree IDs, supplied node counts, an ellipsis when needed, the total cut-tree count, and the limit. A test checks the notice for two cut trees.
A UI test checks that the truncation note appears below visible rows and when a search returns no matches. It also checks that the note is absent for a tree with three nodes.
opengeos/RustGIS#1738: This PR follows up on its tree cutoff by combining reports for multiple cut trees.
opengeos/RustGIS#1080: Both PRs cover the plugin tree node limit and its cutoff behavior.
opengeos/RustGIS#1667: This PR changes the Dropped::notice diagnostic reporting developed in that PR.
Suggested reviewers:giswqs
Merge Risk: Minimal · up to e42d7
The cutoff notice and UI-note changes have no identified merge-blocking issue; they are ready for normal merge checks.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title identifies the follow-up and its main changes: the combined notice for cut trees, a UI test, and a fuzz lockfile update. It is longer than needed, but it is specific and related to the chang…
Description check
The description gives a detailed summary, testing information, related issue context, and UI screenshots. It is mostly complete, but the screenshots show only the updated UI; the template requests bef…
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title identifies the follow-up and its main changes: the combined notice for cut trees, a UI test, and a fuzz lockfile update. It is longer than needed, but it is specific and related to the changeset.
Full details: Description check
Explanation
The description gives a detailed summary, testing information, related issue context, and UI screenshots. It is mostly complete, but the screenshots show only the updated UI; the template requests before-and-after screenshots for changes to existing UI.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 3 files.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit counts the branches trimmed, One note gathers every limb. Search high, search low, the message stays, Short trees pass through uncut ways. The rabbit hops, content with the display.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: f5981382-b655-4a92-b966-dba19a4b27c4
Commits
Reviewing files that changed from the base of the PR and between a411d53 and e42d7cb.
Files ignored due to path filters: 1
fuzz/Cargo.lock is excluded by !**/*.lock
Files selected for processing: 3
crates/plugins/src/panel.rs
crates/plugins/src/tests.rs
crates/ui-egui/src/pluginpanel.rs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.
I read the full diff, the base-branch AGENTS.md, and the surrounding code in panel.rs, pluginpanel.rs, the plugin tests, docs/plugins.md, the fuzz and root lockfiles, and render/src/layout.rs. prior-reviews.json has no threads, so nothing was left out as already raised. I found no defects on the changed lines, so I posted no inline comments.
Bugs
None found in the changed lines. The combined "tree(s) cut" part keeps the existing test assertions true (one catalogue mention, 20001 nodes, first 20000), and the new text matches the UI note exactly (panel.rs:562-575, pluginpanel.rs:1364). Confidence: high.
Build break on this head, not from this PR:crates/engine/src/cmd/layout.rs:1992 passes &[] to legend_sizes_mm, which takes a (&[String], &BTreeMap) tuple (crates/render/src/layout.rs:1968-1973). So rustgis-engine and rustgis-ui-egui don't compile here, and the new UI test can't run on this head. The plugins change is verified only by reading. The PR body already names fix(layout): main builds again: pass legend_sizes_mm its excluded layers and labels #1751 as the fix. Confidence: high.
Security
None. The notice still uses diagnostic_name and bounded name samples.
Performance
None new. The change only reformats the notice and keeps at most ten names. PanelState::apply still runs on the UI thread, as it did before this PR. The timing table in the PR body comes from a throwaway test that isn't in the repo, so I could not check it. Confidence: medium.
Quality
Dropped::cut (panel.rs:497-498) keeps one entry per cut tree, and the struct doc only describes bounds for the other fields. The count is bounded by the manifest's 32-item limit, so this is only a doc gap. Quality point, for the release quality pass. Confidence: high.
The single-tree wording is now 1 tree(s) cut: … catalogue (20001 nodes given). It is accurate but reads a little clumsily. Quality point, for the release quality pass. Confidence: medium.
AGENTS.md
Never silent: complies. One part with a count replaces one part per tree, and the plugins test covers both lossy (two trees) and clean (three nodes, no note) input. The UI test covers the note under rows and under an empty search.
Lockfile: the fuzz/Cargo.lock additions look right. There is one quick-xml and one unicode-normalization entry, both dependency lines are sorted, and rustgis-data has a non-optional quick-xml dependency. I could not run cargo metadata --locked without a shell, so the PR's claim is unverified. Confidence: medium.
Tests: the UI test loads a plugin into the global registry and unloads it only on success, the same pattern as the existing plugin UI tests. Nit, not a rule break. Confidence: high.
Prior concerns left alone: 0 (no prior review threads).
Change size
Compared: the whole PR against main (merge base 0c44ff6 to PR head e42d7cb).
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Follow-up to #1738 (#1546), which was merged before its last review round. This carries the points from that round.
What changes
fuzz/Cargo.lockis updated. fix(plugins): a panel tree holds 20,000 nodes and says when it cuts (#1546) #1738 movedunicode-normalizationfromrustgis-ui-eguitorustgis-plugins, and the fuzz lock still listedrustgis-pluginswithout it (review thread). The fuzz workflow's first job runscargo deny --manifest-path fuzz/Cargo.toml --locked …, and--lockedrefuses a lock that needs updating. With the lock on main,cargo metadata --manifest-path fuzz/Cargo.toml --lockedfails ("cannot update the lock file … because --locked was passed"), and with this one it passes.cargo update -p rustgis-pluginsinfuzz/also addsquick-xmlunderrustgis-data, which the lock had lost track of on main before fix(plugins): a panel tree holds 20,000 nodes and says when it cuts (#1546) #1738.cargo check --locked --binsinfuzz/builds.2 tree(s) cut: only the first 20000 nodes are kept, the most a tree holds: catalogue (20001 nodes given), places (20002 nodes given).a_cut_tree_says_so_under_its_rows_and_an_empty_search(kittest) opens a plugin's panel, gives its treeMAX_TREE_NODES + 1nodes, and finds the note, with the count given and the limit twice, under the rows and under a search for the node left out ("Nothing matches."). A tree given 3 nodes after that shows no note.Measurement: applying a 20,000-node answer
This answers the performance question:
PanelState::applyruns on the thread that ran the plugin's command, which on the desktop is the UI thread. The measurement used a throwaway test, not in this PR, in the workspace's test profile on Windows. It ran three times over 25 branches of municipality-like labels with accents, with ids and counts.applySo the largest answer a tree takes costs about three 60 fps frames, once, when the plugin sends it (usually as the panel opens), not at every frame. Folding the labels takes about 12 ms of the 38. A path that skips decomposition for ASCII characters would save about 6 ms of that, which is too little to be worth a second folding function, so it isn't done. The work is bounded by the limit. Moving
applyoff the UI thread would change how every plugin command's answer reaches the panel, a larger change and not part of this PR.Screenshots
These are rendered with the same wgpu renderer the
ui_shotexample uses, by a throwaway test (not in this PR), over a tree of 5 regions, 27 states and 800 municipalities each (21,632 nodes). The warning in the status bar is the new single part.The note under the rows:
A search for a municipality of the last state, left out at the limit:
Tests
a_tree_cut_at_its_limit_says_so(plugins) now also cuts two trees in one answer and checks the single part.a_cut_tree_says_so_under_its_rows_and_an_empty_search(ui-egui) is new.Locally (Windows):
cargo fmt --checkpasses, and so doescargo clippy -p rustgis-plugins -p rustgis-engine -p rustgis-ui-egui --all-targets -- -D warnings. The tests ofrustgis-pluginsand of therustgis-ui-eguilibrary pass too.cargo denyisn't installed here, so the lock was checked withcargo metadata --lockedas described above. Main doesn't build at the moment:crates/engine/src/cmd/layout.rspasses&[]tolegend_sizes_mm, whose signature changed in #1739. #1751 fixes that. The clippy run and the test runs above had #1751's one-line change applied locally, which this PR doesn't include, so this PR's CI fails on main's error until #1751 is merged.🤖 Generated with Claude Code
Summary by CodeRabbit