feat(skills): Add migrate-breadcrumb-list skill - #123903
Open
Jesse-Box wants to merge 1 commit into
Open
Conversation
Encodes the recipe for moving a page off the legacy `sentry/components/breadcrumbs` component onto `@sentry/scraps/breadcrumbList`, splitting one flat crumb array across the TopBar `breadcrumbs` and `title` slots. Five merged migrations established the pattern, but it only existed in those diffs, so each page re-derived it and re-hit the same traps. Routes depth into three references: which shape a call site is and what blocks it, building a title whose name is richer than a string, and the spec mounting a migrated page needs. Guards four regressions that produce neither a type error nor a failing test: a legacy crumb spread into a typed item silently drops page filters, a badge in `leadingGraphic` without `disableLink` puts a focusable link inside an aria-hidden subtree, a surviving `Layout.Title` or raw `<h1>` double-renders the page name, and a wrapper migrated in place nests the breadcrumbs slot inside the page heading. Validated by running the skill cold against `releaseHeader.tsx` in an isolated worktree, which surfaced the `disableLink` accessibility trap and a checklist grep that flagged its own recommended fix. Records the four call sites that intentionally keep the legacy component, since three need a `<nav>` landmark BreadcrumbList has no mode for.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+64
to
+66
| to: { | ||
| pathname: (makeReleasesPathname({organization, path: '/'}), query); | ||
| } |
Contributor
There was a problem hiding this comment.
Bug: The code example in SKILL.md incorrectly uses the comma operator, assigning the query object to pathname instead of using separate pathname and query keys.
Severity: LOW
Suggested Fix
Correct the code example to properly structure the to object. Instead of pathname: (makeReleasesPathname(...), query), it should have pathname and query as separate sibling keys, like { pathname: makeReleasesPathname(...), query }.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: .agents/skills/migrate-breadcrumb-list/SKILL.md#L64-L66
Potential issue: The code example in the `migrate-breadcrumb-list` skill documentation
at lines 64-66 contains a syntax error. It incorrectly uses the JavaScript comma
operator within an object literal: `pathname: (makeReleasesPathname(...), query)`. This
would cause the `pathname` property to be assigned the value of the `query` object, not
the string path. While TypeScript's type checker would prevent this from being merged
and causing a production issue, the incorrect example misleads developers and AI agents
who use this documentation as a guide for migrating breadcrumbs, potentially causing
them to write incorrect code.
Did we get this right? 👍 / 👎 to inform future reviews.
natemoo-re
approved these changes
Sep 9, 2026
natemoo-re
left a comment
natemoo-re
left a comment
Member
There was a problem hiding this comment.
Approving proactively, none of my feedback is blocking!
|
|
||
| The component's own spec asserts on emitted emotion rules to cover this. Do not copy that into a page spec — it couples the test to generated CSS and breaks on unrelated style edits. The collapse is covered once, in `static/app/components/core/breadcrumbList/breadcrumbList.spec.tsx`; page specs should assert the items they pass in, and leave responsive behavior to that spec and a manual resize check. | ||
|
|
||
| That component spec also tolerates exactly one React warning: the core `Container` primitive leaks a `containertype` DOM attribute. If a page spec fails all `console.error`s, expect to allow that one until the primitive is fixed. |
Member
There was a problem hiding this comment.
Well worth a quick PR to fix!
|
|
||
| The table above is not exhaustive. Three shapes have **no representable form**, and each needs a decision recorded in the PR description rather than a silent workaround. | ||
|
|
||
| **A sibling decorative badge** — `<FeatureBadge type="new" />` rendered next to the trail. It is not part of the label. `trailingActions: {type: 'button'}` is typed `React.ReactElement<ButtonProps | LinkButtonProps>`, so a badge will not typecheck there, and `leadingGraphic` is an `aria-hidden` 16×16 slot, which is semantically wrong. Either drop it or move it to `TopBar.Slot name="actions"` — say which in the PR. |
Member
There was a problem hiding this comment.
If this is common enough to list in the skill, is it worth considering if the Breadcrumbs component should have an explicit prop for feature badge?
| ]; | ||
| ``` | ||
|
|
||
| When a button you are deleting had a `data-test-id`, reuse it as the menu item's `key` so existing selectors keep working. Move any `trackAnalytics` call onto the item's `onAction` — a menu-level handler would fire for siblings too. |
Member
There was a problem hiding this comment.
Technically we should be moving away from data-test-id to semantic selectors, but it's fine if you're splitting this out to keep the migration well scoped.
Member
There was a problem hiding this comment.
Not sure if this needs to be committed?
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
2 participants
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.
Adds an agent skill for migrating pages off
sentry/components/breadcrumbsonto@sentry/scraps/breadcrumbList. 22 pages remain; five merged migrations (#120729, #120794, #123128, #121282, #123569) established the recipe, but it only ever lived in those diffs. Documentation only — no runtime code.SKILL.mdis a router: the two-slot transformation, a call-site shape table, a 9-step workflow, and a grep-based checklist. Three references handle depth — classifying a call site, building a title richer than a plain string, and the spec harness.SPEC.mdholds the maintenance contract.It guards four regressions that produce neither a type error nor a failing test:
preservePageFilterssurviving a.map/spread and silently dropping page filters; aleadingGraphicbadge withoutdisableLinkputting a focusable link in anaria-hiddensubtree; a survivingLayout.Titledouble-rendering the page name; and a wrapper nesting thebreadcrumbsslot inside the page<h1>.The importer count floors at 4, not 0 — three call sites need a
<nav>landmarkBreadcrumbListhas no mode for.SettingsBreadcrumbis out of scope.Open question. The skill teaches building the link query at the call site, per
conversationsBreadcrumbs.tsx. That is source-side and does not fix the existing inconsistency:builderBreadCrumbs.tsxsetspreservePageFiltersfor a destination thatdetectors/components/forms/common/breadcrumbs.tsxdoes not. The sidebar solves this destination-side, where the knowledge lives. If we adopt that, this section of the skill gets deleted.Separately,
breadcrumbList.mdxdocuments apreservePageFiltersprop that has never existed onBreadcrumbItemLinkProps.