Conversation
…ction Two extension points that the compare redesign left without a home. **Per-component controls.** The pre-redesign drawer UI had `DrawerWidgets` slots, which bit.cloud's change-request view used to hang review affordances off each component — a "include in review" checkbox, a button into that component's discussion thread. The mounted-panels design replaced drawers and dropped the slots, so those controls have nowhere to go and the cloud cannot move onto the new compare without losing them. `LaneCompare` now takes `renderComponentActions`, handed straight to each `InlineComponentCompare` and rendered at the trailing edge of that component's header. It is a function rather than a node on purpose: a node per component would be a fresh prop on every parent render and would defeat the `React.memo` that makes a view-mode switch a CSS attribute flip instead of ten panel re-renders. The contract (must be referentially stable) is documented on both props. Nothing about reviews reaches lane-compare — it passes a context (`name`, `componentId` without version, `baseId`, `compareId`) and renders whatever comes back. **Selection from the URL.** `?componentId=` and `?file=` were read once, into initial state. A host that navigates to a component after the view is already mounted — following a link to a discussion attached to one — got nothing. lane-compare writes its own selection with `history.replaceState`, which react-router does not observe, so a *change* in those params can only have come from a real navigation; adopting it is unambiguous. The effect is seeded with the mount-time value so it skips the first run and leaves page load to the existing initial-scroll effect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoAdd host actions and URL-driven selection to component compare
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Switching comparisons briefly shows old diffs
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Handing the host a context object meant lane-compare threading identity
down through every panel to a callback that could not use hooks of its own.
The panel already knows which component it is; the host's control just had
no way to ask.
`ComponentActions` is now a component type, and `InlineComponentCompare`
wraps the whole panel — header included — in a `ComponentIdentityProvider`.
A host writes a plain component and calls `useComponentCompareIdentity()`:
function ReviewControls() {
const { componentId } = useComponentCompareIdentity() ?? {};
...
}
<LaneCompare ComponentActions={ReviewControls} />
No callback, no threading, and hooks work inside it. A module-scope
component is also stable by construction, which is what keeps the panel's
React.memo intact — the earlier design had to document that requirement
because a render prop could quietly violate it.
Also fixes the URL-driven scroll (Qodo): a navigation can land while the
lane diff is reloading, when the pane is a skeleton and there is nothing to
scroll to. The target is now recorded and performed once the pane is back,
instead of being dropped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit b2d71f7 |
Three findings from review, all on the navigation effect. A selection that was set and then cleared while the diff reloaded kept its pending scroll: the URL no longer named a component, but the retry still jumped to the one it used to name. Every change now replaces the pending target, clearing included. Each scroll waits up to five seconds for a lazily mounted anchor, and nothing cancelled the previous wait. Two quick navigations left two observers live, and the older one scrolled the pane back the moment its anchor appeared. Waits now take an `AbortSignal`: starting a scroll supersedes the one before it, and aborting tears the observer down rather than leaving it watching for the rest of the timeout. Both effects moved into `useUrlSelection`. They were racing each other for the same reason — the mount-time scroll and a later navigation each started an uncancellable wait — and as one hook there is a single in-flight request. It also made the behaviour testable without mounting the whole compare view, which is the third finding: seven specs covering mount-time selection, post-mount adoption, deferral while loading, retry when the pane is not mounted, the cleared-target case, supersession, and unmount. The two bug specs fail without their fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| {...dataAttributes} | ||
| <ComponentIdentityProvider | ||
| name={name} | ||
| componentId={compareId.split('@')[0]} |
There was a problem hiding this comment.
2. Removed components lose their identity 🐞 Bug ≡ Correctness
ComponentIdentityProvider derives componentId only from compareId, which lane compare sets to an empty string for entries with no compare-side component. For removed components, host header actions therefore receive an empty stable ID and cannot key review controls or discussion links to the component, even though baseId identifies it.
Agent Prompt
## Issue description
Removed components have no compare-side ID, but the component identity context derives its stable ID exclusively from `compareId`. Host-contributed header controls consequently receive an empty `componentId` for those rows.
## Fix Focus Areas
- components/ui/component-compare/component-compare/component-compare.tsx[674-699]
## Recommended Fix
Derive the versionless component identity from `compareId` when available and fall back to `baseId` for removed components. Reuse that derived value for the identity provider, header, and DOM component anchor so all consumers agree on the row identity.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit d03d1e0 |
1 similar comment
|
Code review by qodo was updated up to the latest commit d03d1e0 |
`loadingLaneDiff` is optional on the compare context; the hook wanted a plain boolean. Absent means not loading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| // eslint-disable-next-line @typescript-eslint/no-use-before-define | ||
| signal?.removeEventListener('abort', onAbort); |
There was a problem hiding this comment.
1. Changed ui code fails repository lint 📘 Rule violation ≡ Correctness
finish references onAbort before its const declaration, while the adjacent suppression names a different rule than the configured core check. Running canonical npm run lint therefore reaches this changed helper and reports no-use-before-define as an error.
Agent Prompt
## Issue description
The new `finish` callback references the later `onAbort` constant, and its suppression targets `@typescript-eslint/no-use-before-define` rather than the configured core `no-use-before-define` rule.
## Fix Focus Areas
- components/ui/compare/lane-compare/lane-compare.tsx[108-115]
## Recommended Fix
Convert `finish` and `onAbort` to function declarations so their mutual references are valid under the configured rule, and remove the ineffective suppression comment.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // keep the request if the pane is not mounted yet; the next loading change retries it | ||
| if (scrollTo({ ...target, componentId: target.componentId }, controller.signal)) { | ||
| pending.current = undefined; | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [currentKey, loading]); |
There was a problem hiding this comment.
2. Cached diffs can miss deep-link scrolling 🐞 Bug ≡ Correctness
useUrlSelection retains the pending target when scrollTo returns false, but its retry effect only depends on the URL key and loading. When cached lane-diff data renders the previously absent pane without changing either value, the stored request is never retried and the requested component is not scrolled into view.
Agent Prompt
## Issue description
`useUrlSelection` preserves a target when the pane is unavailable, but no dependency tells it when the pane later becomes available. Cached lane-diff updates can render the pane while `loading` remains false, leaving the deep-link scroll pending forever.
## Fix Focus Areas
- components/ui/compare/lane-compare/use-url-selection.ts[59-74]
- components/ui/compare/lane-compare/lane-compare.tsx[343-357]
## Recommended Fix
Pass a pane-availability or diff-content retry token into `useUrlSelection` and include it in the scrolling effect dependencies. Change that token when the real diff pane mounts or its component set becomes available, so a retained target is retried even when the loading state never transitions; add a hook test covering `scrollTo` first returning false and then succeeding after only that retry token changes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 6e7582b |
bit.cloud serves the bulk `compareComponents` and `apiDiffs` resolvers, but through a bit whose schema predates the fields, so the type system never exposes them. Against that host every pair fails, the file registry fills with empty lists, and the whole compare surface renders blank — no Code view, no panel content. `useBulkPagedQuery` now takes an optional single-pair query. When a host rejects the bulk field it switches to one request per pair, a few at a time, accumulating results as they land so the surface still fills in progressively. Both providers supply one: `compareComponent` and `apiDiff`, each with the same selection set as its bulk counterpart, so consumers cannot tell the two paths apart. Detection keys on the GraphQL validation error naming the field. A missing field is rejected before execution, so retrying can never help and the only useful response is to stop asking — which is also what makes this safe to distinguish from the transient failures the paging loop already retries. The result is cached per host+field for the page's lifetime, so later mounts skip the rejected request rather than each paying for it. This is a compatibility path, not a second way of doing things: it costs exactly what the bulk field was introduced to avoid, it is only ever taken after a host has refused the bulk field, and it retires itself the moment that host catches up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| const [fallbackDone, setFallbackDone] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (!onFallback || skip) return undefined; |
There was a problem hiding this comment.
6. Skipped fallback queries stay loading 🐞 Bug ≡ Correctness
useBulkPagedQuery exits the fallback effect when skip is true, while done still depends exclusively on fallbackDone. This occurs for empty pair lists and inactive API views on a host already marked as lacking the bulk field, leaving the hook's documented loading state unsettled despite no request being eligible to run.
Agent Prompt
## Issue description
Fallback sessions return before setting `fallbackDone` when the query is skipped, but the returned `loading` state still depends on `fallbackDone` and therefore remains true indefinitely.
## Fix Focus Areas
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.ts[178-179]
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.ts[222-222]
## Recommended Fix
Include `skip` in the completion calculation so every skipped session reports `loading: false`, regardless of whether fallback mode is active. Add a fallback-enabled skipped-query test alongside the existing skipped-query coverage.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } catch { | ||
| // a pair that cannot be compared is reported as failed, exactly as the bulk path reports it | ||
| item = null; |
There was a problem hiding this comment.
7. Dropped requests lose component diffs 🐞 Bug ☼ Reliability
The fallback worker catches every client.query rejection and immediately stores null for the requested pair instead of using the transient retry budget applied by the bulk paging path. A temporary transport failure on the compatibility path therefore becomes a settled failed comparison once fallbackDone is set, leaving that component unavailable for the remainder of the mounted session.
Agent Prompt
## Issue description
The compatibility worker converts the first single-pair request rejection into a permanent `null` result, even though the bulk paging path retries transient request failures. Requested pairs should remain unresolved while retries remain and only settle as unavailable after the retry budget is exhausted.
## Fix Focus Areas
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.ts[187-211]
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.ts[220-222]
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.ts[294-313]
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.spec.tsx[91-103]
## Recommended Fix
Add bounded per-pair retries for rejected fallback queries, using the existing transient retry budget or an equivalent explicit policy. Preserve cancellation checks between attempts, keep unresolved pairs loading while a retry remains, and store `null` only after retries are exhausted. Update the fallback tests to verify recovery from a temporary failure and terminal failure after exhaustion of the retry budget.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ]); | ||
|
|
||
| const dataByCompareId = useMemo(() => { | ||
| if (onFallback) return fallbackData; |
There was a problem hiding this comment.
1. Switching comparisons briefly shows old diffs 🐞 Bug ≡ Correctness
useBulkPagedQuery returns fallbackData and fallbackDone from the preceding fallback session during render because their reset is deferred to a passive effect. When an unsupported host changes host or base/compare pairs, overlapping identifiers can expose old comparisons while absent identifiers resolve to null, reaching component panels and the file registry as settled failures before current-session requests start.
Agent Prompt
## Issue description
Fallback data and completion state are not associated with their originating session, so a host or pair change can render once using the preceding session's map and completed state. A new fallback session must synchronously expose empty, pending values until its own workers populate data, preventing old results from satisfying new lookups and missing identifiers from being classified as failures.
## Fix Focus Areas
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.ts[175-182]
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.ts[219-222]
- components/ui/hooks/use-bulk-paged-query/use-bulk-paged-query.ts[278-313]
## Recommended Fix
Store the fallback `sessionKey` together with its data and completion state, or otherwise synchronously invalidate the displayed fallback session when the key changes. When the stored key differs from the current `sessionKey`, return an empty map and `done: false` instead of exposing the preceding session, then populate state for the current key from the fallback workers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const currentKey = keyOf(selection); | ||
| const lastSeenKey = useRef(currentKey); | ||
| // seeded so a deep link scrolls once the diff has loaded, without a second effect to race with | ||
| const pending = useRef<UrlSelection | undefined>(selection.componentId ? selection : undefined); | ||
| const inFlight = useRef<AbortController | undefined>(undefined); | ||
|
|
||
| useEffect(() => { | ||
| if (currentKey === lastSeenKey.current) return; | ||
| lastSeenKey.current = currentKey; | ||
| apply(selection); |
There was a problem hiding this comment.
2. Back navigation keeps old selection 🐞 Bug ≡ Correctness
useUrlSelection seeds lastSeenKey from the mount-time URL and only calls apply when the router-observed key differs from that original value. If a user selects a component through local state and replaceState, then navigates to an unselected URL matching the initially empty key, the local component and file selections remain set because the hook ignores the navigation.
Agent Prompt
Issue description
A router navigation back to the initial empty URL selection is ignored after local selection changed the URL through `history.replaceState`. The hook's observed key must remain coherent with locally written selection state so real navigation to an empty selection clears both local values.
Fix Focus Areas
- components/ui/compare/lane-compare/use-url-selection.ts[42-57]
- components/ui/compare/lane-compare/lane-compare.tsx[293-331]
Recommended Fix
Synchronize the hook's last-observed selection with locally written selection changes, or pass the current local selection into the hook and compare router selection against it rather than only its mount-time key. Preserve the requirement that `replaceState` itself does not re-apply state, while ensuring a subsequent router navigation with no parameters calls `apply({})`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit ea4bee5 |
1 participant
Why
Two extension points that the compare redesign (#10378) left without a home. Both block bit.cloud from moving its change-request view onto the new compare.
Per-component controls
The pre-redesign drawer UI had
DrawerWidgetsslots, which bit.cloud used to hang review affordances off each component — an "include in review" checkbox, a button into that component's discussion thread. The mounted-panels design replaced drawers and dropped the slots, so today those controls have nowhere to go.It is handed straight to each
InlineComponentCompareand rendered at the trailing edge of that component's header.It is a function, not a node, on purpose. A node per component would be a fresh prop on every parent render, defeating the
React.memothat makes a view-mode switch a CSS attribute flip rather than ten panel re-renders. The function is called during each panel's own render, so a stable reference still produces up-to-date controls. The requirement is documented on both props.Nothing about reviews reaches lane-compare. It passes a context and renders whatever comes back:
Selection from the URL
?componentId=and?file=were read once, into initial state. A host that navigates to a component after the view is mounted — following a link to a discussion attached to one — got nothing.lane-compare writes its own selection with
history.replaceState, which react-router does not observe, so a change in those params can only have come from a real navigation. That makes adopting it unambiguous. The effect is seeded with the mount-time value so it skips the first run, leaving page load to the existing initial-scroll effect (which additionally waits for the diff to load).Testing
Four specs in
component-compare-actions.spec.tsx: the header renders host actions, adds no markup when there are none, is asked with the right component identity, and renders what came back.oxlint --deny-warningsclean on both components.The URL-selection change is not covered by a spec — it needs a router plus a loaded lane diff to be meaningful, and I would rather not assert on a mock of both. Called out here so it gets eyes in review.
🤖 Generated with Claude Code