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(components): stop repeated node ids from looping the tree ancestry - #447
A component report with repeated node ids could make the panel's ID-indexed parent map loop. ownerOf and reveal followed that map with no cycle check, so selecting a directive host could freeze the panel's UI thread (CWE-835, denial of service from a reporting connection).
Report boundary:guardComponentRoots (packages/devtools/src/rpc/component-report-guard.ts) drops any node whose id the tree already used, together with its subtree, before the page is published.
Panel: the component tree index keeps only the first row with each id, so parents cannot form a cycle. ownerOf and reveal now walk through a new ancestors() helper that also stops at a repeated id.
How it was verified
pnpm commit:check (commit messages follow the guidelines)
pnpm format:check
pnpm typecheck (includes the ngc template checks)
pnpm test, pnpm test:devtools and pnpm test:panel (test:devtools: 3647 passed; test:panel: 850 passed; pnpm test not run)
pnpm skills:check (when .claude/ changed) N/A
Docs in apps/docs updated and pnpm docs:build passes, or the no-docs label added with the reason below (no behavior change for valid reports; ids from the collector are already unique)
pnpm extension:build and extension/ui committed (when app/ changed)
Checked in the browser with axe (when the UI changed) N/A, no UI change
New tests: duplicate-id case in rpc-boundary.test.ts, and app/src/__tests__/component-tree-ancestors.test.ts for the loop cases.
Screenshots
N/A
Notes for reviewers
The guard drops the duplicate node and its subtree rather than rejecting the whole report, matching how it already handles other bad nodes, so one bad entry does not blank the page.
Summary by CodeRabbit
Bug Fixes
Component trees now keep the first entry when IDs are duplicated and omit later duplicates along with their descendants, preventing repeated entries from appearing.
Parent traversal now proceeds from the nearest parent upward and stops if it encounters an ID again, improving navigation through component trees.
A component report with repeated node ids could make the panel's
parent map loop, so ownerOf and reveal walked it forever and froze the
panel. The report guard now drops a node whose id the tree already
used, along with its subtree. The panel keeps only the first row with
an id in its index, and both ancestry walks stop at a repeated id.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
No actionable comments were generated in the recent review.
Walkthrough
The change rejects repeated component IDs during RPC tree conversion and keeps the first occurrence. The component-tree index also keeps the first row for each ID, and ancestor traversal stops on cycles. Tests cover duplicate IDs and ancestor termination; extension UI asset references were updated.
Changes: Component tree ID handling
Layer / File(s)
Summary
Reject duplicate IDs during RPC conversion packages/devtools/src/rpc/component-report-guard.ts, packages/devtools/src/__tests__/rpc-boundary.test.ts
guardComponentRoots tracks accepted IDs and drops duplicate nodes with their subtrees. The test covers repeated IDs in nested nodes and later roots.
Index and traverse component-tree ancestors app/src/pages/component-tree.ts, app/src/__tests__/component-tree-ancestors.test.ts, extension/ui/assets/browser-agent-rpc-KH4HHvHf-DDC7kv8R.js, extension/ui/index.html
The component-tree index retains the first row for each ID. The new ancestors function stops on repeated IDs and is used by ownerOf and reveal. Tests cover ancestor ordering and cycle termination. Extension UI bundle references were updated.
Duplicate reports are filtered and ancestor walks terminate. No merge-blocking issue was identified.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the main change: repeated node IDs in the component tree ancestry no longer loop. The raw summary confirms the change adds an ancestors helper that stops on a repeated ID, drops dupl…
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 names the main change: repeated node IDs in the component tree ancestry no longer loop. The raw summary confirms the change adds an ancestors helper that stops on a repeated ID, drops duplicate-ID nodes at the report guard, and keeps only the first indexed row per ID. The title uses the conventional commit prefix and stays under one line. It does not cover the guard or the index change, which is expected for a short title.
Full details: Docstring Coverage
Explanation
Docstring coverage is 40.00% which is insufficient. 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 5 functions across 5 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 6f72c79c-ff37-4605-9fd3-a81c1e13860c
Commits
Reviewing files that changed from the base of the PR and between e4ff5c8 and ca9ae91.
Files ignored due to path filters: 1
extension/ui/assets/index-D0AagNaK.js is excluded by !**/assets/index-[0-9a-z_-][0-9a-z_-][0-9a-z_-][0-9a-z_-][0-9a-z_-][0-9a-z_-][0-9a-z_-][0-9a-z_-].js
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
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.
github-actions
Bot
added
area: panel
What and why
A component report with repeated node ids could make the panel's ID-indexed parent map loop.
ownerOfandrevealfollowed that map with no cycle check, so selecting a directive host could freeze the panel's UI thread (CWE-835, denial of service from a reporting connection).guardComponentRoots(packages/devtools/src/rpc/component-report-guard.ts) drops any node whose id the tree already used, together with its subtree, before the page is published.parentscannot form a cycle.ownerOfandrevealnow walk through a newancestors()helper that also stops at a repeated id.How it was verified
pnpm commit:check(commit messages follow the guidelines)pnpm format:checkpnpm typecheck(includes thengctemplate checks)pnpm test,pnpm test:devtoolsandpnpm test:panel(test:devtools: 3647 passed;test:panel: 850 passed;pnpm testnot run)pnpm skills:check(when.claude/changed) N/Aapps/docsupdated andpnpm docs:buildpasses, or theno-docslabel added with the reason below (no behavior change for valid reports; ids from the collector are already unique)pnpm extension:buildandextension/uicommitted (whenapp/changed)New tests: duplicate-id case in
rpc-boundary.test.ts, andapp/src/__tests__/component-tree-ancestors.test.tsfor the loop cases.Screenshots
N/A
Notes for reviewers
The guard drops the duplicate node and its subtree rather than rejecting the whole report, matching how it already handles other bad nodes, so one bad entry does not blank the page.
Summary by CodeRabbit