feat(lanes | component): redesign the changes & compare experience#10378
feat(lanes | component): redesign the changes & compare experience#10378luvkapur wants to merge 111 commits into
Conversation
Defines scope, architecture, hook bindings, and acceptance criteria (no-drawer, no-monaco, tab rewiring, manual smoke) for porting the new lane-compare design into ____bit while keeping workspace data sources. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
16-task plan with explicit scope/name/rootDir per new bit component so resulting npm package ids match imports from the new design. Phases A-F cover primitives, non-monaco diff foundation, inline tab components, compare shell, lane-compare port (drawer stripped), and final tab-list wiring + acceptance gates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ports CompareToolbar from new-changes workspace; adds @teambit/design.inputs.selectors.multi-select@2.0.17 to workspace policy (was missing). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ports CompareSidebar (with SidebarGroup and SidebarComponentItem sub-components) from the new-changes workspace into the component-compare component and exports all public types from the index. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add teambit.dependencies/ui/deps-diff-table — a pure table component for rendering dependency diffs between two component versions, with status indicators and compare URL links. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds teambit.code/ui/inline-code-compare, which renders per-file inline diffs using DiffFileRenderer from inline-diff-viewer and wires into ComponentCompare context. Fixed import path (component.ui.component-compare.component-compare) and removed unused variable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ports teambit.review/ui/inline-tests-compare from lane-compare branch; remaps bare component-compare import to component-compare.component-compare package id. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds teambit.review/ui/inline-config-compare — renders aspect config/data diffs between two component versions using DiffFileRenderer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eHeader Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Required by InlineComponentCompare (Task 12). bit install side-effects. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds teambit.semantics/ui/api-diff-view — a full-pane React component that queries and renders API surface changes between two component versions using GQL. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e data Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hapes - Drop `isMerged` references from lane-compare-page (LaneModel only has `deleted`) - Adapt `useLaneComponents` call to single-arg signature returning an object - Use `componentDescriptors` (not `components`) for the compositions map since `.get()` is a ComponentDescriptor method Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…hema InlineContextProvider and EagerFileRegistrar used useGetComponents / createComponentModel from the cloud GraphQL schema (getComponents / CompDescriptor), which the workspace/scope dev-server schema does not expose - every InlineComponentCompare fired a 400, breaking the code diff and stalling the lane-diff query via a re-render storm. Replace with the schema-compatible useComponent hook (spec section 7.1 fallback). Drop the now-unused cloud packages from workspace.jsonc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extract workspace-overview's data-agnostic pieces into a shared @teambit/explorer.ui.components-overview component; refactor both workspace-overview and lane-overview into thin adapters. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… plan Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rview Creates @teambit/explorer.ui.components-overview with data-agnostic card, filter-panel, aggregation hooks, and a new ComponentsOverview orchestrator that accepts getHref / storageNamespace injection points. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…onent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the header slot and filter command bar in a single sticky div, measures its height via ResizeObserver into --components-overview-sticky-height, and uses that CSS var as the top offset for section headers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| changes, | ||
| sourceHead, | ||
| targetHead: commonSnap?.hash, | ||
| baseSource, |
There was a problem hiding this comment.
1. Wrong lane base head 🐞 Bug ≡ Correctness
LanesMain.componentDiffStatus() returns targetHead as the merge-base (commonSnap.hash) instead of the target lane head, so lane compare and API diff derive baseId/baseVersion from the wrong snap and can render incorrect diffs or mark pairs incomparable.
Agent Prompt
### Issue description
`LanesMain.componentDiffStatus()` sets `targetHead: commonSnap?.hash`, but downstream UI treats `targetHead` as the **base-side head** to diff against. This causes lane compare (and API diff view) to diff against the merge-base instead of the target lane head.
### Issue Context
The merge-base is already available via `snapsDistance.common` / `changesContext.commonSnap`; it should not replace the target head.
### Fix Focus Areas
- scopes/lanes/lanes/lanes.main.runtime.ts[1580-1594]
### What to change
- In the `return { ... }` of `componentDiffStatus()`, set `targetHead` to the function parameter `targetHead` (the actual head on the target lane).
- Keep `commonSnap` information in `snapsDistance.common` and/or `changesContext` (already present), rather than overloading `targetHead`.
ⓘ 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 1845e9d |
| export function useFileRegistryRegister(componentId: string | undefined, files: FileInfo[] | undefined) { | ||
| const store = useContext(FileRegistryContext); | ||
| const signature = registerSignature(files); | ||
| useEffect(() => { | ||
| if (store && componentId && files) { | ||
| store.register(componentId, files); | ||
| } | ||
| }, [store, componentId, signature]); | ||
| } | ||
|
|
||
| export function useAspectRegistryRegister(componentId: string | undefined, files: FileInfo[] | undefined) { | ||
| const store = useContext(FileRegistryContext); | ||
| const signature = registerSignature(files); | ||
| useEffect(() => { | ||
| if (store && componentId && files) { | ||
| store.registerAspects(componentId, files); | ||
| } | ||
| }, [store, componentId, signature]); |
There was a problem hiding this comment.
1. Empty registry clears skipped 🐞 Bug ≡ Correctness
useFileRegistryRegister() / useAspectRegistryRegister() guard on files truthiness, so an empty array ([]) is never registered and stale sidebar entries are not cleared when compare data intentionally returns []. This breaks file/aspect trees for pairs that become unchanged or resolve to null and should clear prior state.
Agent Prompt
## Issue description
`useFileRegistryRegister` and `useAspectRegistryRegister` currently skip registration when `files` is an empty array. In this design, `[]` is a meaningful “loaded but empty” signal used to clear stale registry entries, so it must be registered.
## Issue Context
`CompareRegistryEntry` intentionally returns `[]` when `data === null` (pair failed / terminated) to clear stale file trees. The register hooks currently treat `[]` as falsy and skip registration.
## Fix Focus Areas
- components/ui/component-compare/component-compare/file-registry.tsx[166-184]
- components/ui/component-compare/component-compare/component-compare.tsx[923-948]
## What to change
- In both hooks, change the guard from `if (store && componentId && files)` to `if (store && componentId && files !== undefined)` (or `files != null`), and pass `files` through (including `[]`).
- Keep the signature dependency (it already differentiates `undefined` vs `[]`).
ⓘ 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 1cb283d |
| private async loadChangeTypesMemoFromDisk() { | ||
| try { | ||
| const raw = await fs.readFile(LanesMain.CHANGE_TYPES_MEMO_FILE, 'utf8'); | ||
| const parsed = JSON.parse(raw) as { v?: number; entries?: Record<string, ChangeType[]> }; | ||
| // legacy unversioned format (a plain key->ChangeType[] map) or an older version: discard. | ||
| if (parsed.v !== LanesMain.CHANGE_TYPES_MEMO_VERSION || !parsed.entries) return; | ||
| for (const [k, v] of Object.entries(parsed.entries)) this.changeTypesMemo.set(k, v); | ||
| this.logger?.debug(`[lane-diff memo] loaded ${this.changeTypesMemo.size} change-types entries`); |
There was a problem hiding this comment.
2. Memo hydration overwrites cache 🐞 Bug ≡ Correctness
LanesMain asynchronously hydrates multiple memo Maps from disk and unconditionally set()s loaded entries, so a request that computes/stores newer entries during startup can be overwritten by the late disk load and later serve stale lane diff/change-type results.
Agent Prompt
## Issue description
`LanesMain` starts async memo hydration in the constructor, but each `load*MemoFromDisk()` method unconditionally writes disk entries into the live in-memory Map via `Map.set()`. If the service handles a lane-diff request before hydration completes, it can compute and store newer memo entries, which can then be overwritten by the late-arriving disk hydration, causing stale classifications to be served.
## Issue Context
These memos are actively used for lane diff status and change-type derivation. Because hydration is not awaited, the race window is the time between process start and completion of the `readFile/JSON.parse` promise chain.
## Fix Focus Areas
- scopes/lanes/lanes/lanes.main.runtime.ts[219-237]
- scopes/lanes/lanes/lanes.main.runtime.ts[289-300]
- scopes/lanes/lanes/lanes.main.runtime.ts[351-362]
## Suggested fix
Implement one of:
1) **Non-overwriting merge on hydration**: in each `load*MemoFromDisk()`, only `set()` an entry if the Map does **not** already contain the key (so in-memory computations always win).
2) **Initialization gate**: store an `initPromise` for all hydrations and await it before reading/writing memos (more invasive because some callers are sync).
Option (1) is the minimal, safest change:
- Replace `this.changeTypesMemo.set(k, v)` with `if (!this.changeTypesMemo.has(k)) this.changeTypesMemo.set(k, v)`
- Replace `this.diffStatusResultMemo.set(k, v)` with `if (!this.diffStatusResultMemo.has(k)) this.diffStatusResultMemo.set(k, v)`
- Apply the same pattern to other memo hydrations (`snapsDistanceMemo`, `apiDiffMemo`) for consistency.
ⓘ 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 136e5fe |
|
Code review by qodo was updated up to the latest commit d9fa440 |
|
Code review by qodo was updated up to the latest commit a8ab799 |
|
Code review by qodo was updated up to the latest commit 1234e2a |
|
Code review by qodo was updated up to the latest commit 56b4a56 |
| import { expect } from 'chai'; | ||
| import type { Location as SchemaLocation, SchemaNode } from '@teambit/semantics.entities.semantic-schema'; | ||
| import { | ||
| APISchema, | ||
| ModuleSchema, | ||
| VariableLikeSchema, | ||
| KeywordTypeSchema, | ||
| TypeRefSchema, | ||
| ExportSchema, | ||
| UnImplementedSchema, | ||
| } from '@teambit/semantics.entities.semantic-schema'; | ||
| import { ComponentID } from '@teambit/component-id'; | ||
| import { computeAPIDiff, ImpactAssessor, DEFAULT_IMPACT_RULES } from '@teambit/semantics.entities.semantic-schema-diff'; | ||
| import { SchemaMain } from './schema.main.runtime'; | ||
|
|
There was a problem hiding this comment.
1. api-diff-compute.spec.ts misplaced 📘 Rule violation ⚙ Maintainability
A new unit test for computeAPIDiff was added under scopes/semantics/schema/ instead of alongside the function it tests in components/entities/semantic-schema-diff/, violating the repo’s unit-test placement convention. This makes tests harder to discover/maintain and breaks the expected source-adjacent structure.
Agent Prompt
## Issue description
A unit test for `computeAPIDiff` is located in `scopes/semantics/schema/api-diff-compute.spec.ts`, but the function under test lives in `components/entities/semantic-schema-diff/api-diff.ts`. Per repository conventions, unit tests should be `.spec.ts` files placed alongside the source they test.
## Issue Context
The current test imports `computeAPIDiff` from `@teambit/semantics.entities.semantic-schema-diff`, which is implemented in `components/entities/semantic-schema-diff/api-diff.ts`.
## Fix Focus Areas
- scopes/semantics/schema/api-diff-compute.spec.ts[1-284]
- components/entities/semantic-schema-diff/api-diff.ts[303-308]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /** The original `compare()` body — moved here so the public method can wrap with memo + single-flight. */ | ||
| private async computeCompare(baseIdStr: string, compareIdStr: string): Promise<ComponentCompareResult> { | ||
| const host = this.componentAspect.getHost(); | ||
| const [baseCompId, compareCompId] = await host.resolveMultipleComponentIds([baseIdStr, compareIdStr]); | ||
| const modelComponent = await this.scope.legacyScope.getModelComponentIfExist(compareCompId); |
There was a problem hiding this comment.
2. Compare ignores selected host 🐞 Bug ≡ Correctness
ComponentCompareMain.computeCompare() resolves component IDs against this.componentAspect.getHost() (default/prior host) instead of the GraphQL-selected host from getHost(id: $host), so compare/api-diff can run against the wrong host (e.g. workspace vs scope) and produce incorrect results for the requested host context. This also makes any caching keyed only by (baseId, compareId) vulnerable to cross-host collisions when resolution differs by host.
Agent Prompt
## Issue description
Bulk compare/API-diff queries explicitly select a `ComponentHost` via `getHost(id: $host)`, but `ComponentCompareMain` still calls `this.componentAspect.getHost()` with no id inside `computeCompare()` (and similarly in API diff computation paths). This can resolve component IDs/files from a different host than the one the GraphQL query selected.
## Issue Context
- Client queries are structured as `getHost(id: $host) { compareComponents(...) }`, so the resolver should use the *parent* host.
- `ComponentMain.getHost(id?: string)` supports selecting a host by id.
## Fix Focus Areas
- scopes/component/component-compare/component-compare.main.runtime.ts[152-175]
- scopes/component/component-compare/component-compare.graphql.ts[46-72]
## Suggested fix approach
1. Plumb the selected host into compare execution:
- Option A (preferred): change GraphQL resolvers on `ComponentHost` to pass the parent host instance (first resolver arg) into `ComponentCompareMain` (e.g. `compareOnHost(host, baseId, compareId)`), and stop calling `this.componentAspect.getHost()` inside compute paths.
- Option B: add a `hostId?: string` parameter to `compare()/compareComponents()/getAPIDiff()/apiDiffs()` and call `this.componentAspect.getHost(hostId)` inside `computeCompare`/`computeAPIDiff`.
2. Ensure cache keys include host identity if results can differ by host (or ensure compare always runs on the correct host so keys remain safe).
3. Add/adjust tests that run `compareComponents` against two hosts and assert it resolves the intended one.
ⓘ 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 7b46de6 |
# Conflicts: # scopes/workspace/workspace/ui/workspace/use-workspace.ts
| private async getOrCompute<T>( | ||
| inflight: Map<string, Promise<T>>, | ||
| cacheKey: string, | ||
| compute: () => Promise<T>, | ||
| cacheable: (value: T) => boolean = () => true, | ||
| skipPersistentCache = false | ||
| ): Promise<T> { | ||
| const pending = inflight.get(cacheKey); | ||
| if (pending) return pending; | ||
| if (!skipPersistentCache) { | ||
| const cached = await this.cache.get<T>(cacheKey); | ||
| if (cached !== undefined) return cached; | ||
| // a concurrent caller may have started computing while we awaited the cache read. | ||
| const started = inflight.get(cacheKey); | ||
| if (started) return started; | ||
| } | ||
| const promise = compute() | ||
| .then((result) => { | ||
| if (!skipPersistentCache && cacheable(result)) void this.cache.set(cacheKey, result); | ||
| return result; | ||
| }) | ||
| .finally(() => inflight.delete(cacheKey)); | ||
| inflight.set(cacheKey, promise); | ||
| return promise; |
There was a problem hiding this comment.
1. Single-flight cache race 🐞 Bug ☼ Reliability
ComponentCompareMain.getOrCompute() awaits the persistent cache read before registering the in-flight promise, so two concurrent callers can both miss the cache and start compute() for the same key. This defeats the intended single-flight dedupe and can double expensive compare/api-diff computation under concurrent load.
Agent Prompt
### Issue description
`ComponentCompareMain.getOrCompute()` is intended to provide single-flight dedupe across concurrent callers, but it registers `inflight.set(cacheKey, promise)` only after awaiting the persistent cache read. Two callers racing can both see no in-flight entry, both miss the persistent cache, and both start `compute()`.
### Issue Context
This sits on hot paths for lane compare / API diff bulk fetching, where parallel requests for the same pair are expected.
### Fix Focus Areas
- scopes/component/component-compare/component-compare.main.runtime.ts[89-112]
### Suggested fix
Restructure `getOrCompute()` so the *entire* read-through flow (cache read + optional compute) is wrapped in a single promise that is stored in `inflight` immediately (before any `await`). For example:
- If `skipPersistentCache` is false: inside the single-flight promise, `await cache.get()`, return cached if present, otherwise run `compute()` and (optionally) `await/void cache.set()`.
- Always `finally` delete the inflight entry.
This ensures only one concurrent caller performs the cache read and/or compute for a given key.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| function dedupeReexportAliases(exports: ReturnType<typeof buildExportMap>): ReturnType<typeof buildExportMap> { | ||
| for (const [key, entry] of exports) { | ||
| const target = (entry.unwrapped as any)?.name; | ||
| if ( | ||
| isDefaultExportKey(key) && | ||
| getSchemaTypeName(entry.unwrapped) === 'TypeRefSchema' && | ||
| target && | ||
| target !== key && | ||
| exports.has(target) | ||
| ) { | ||
| exports.delete(key); | ||
| } |
There was a problem hiding this comment.
2. Default export change missed 🐞 Bug ≡ Correctness
computeAPIDiff() deletes the default export from the export map when it is a TypeRefSchema alias to an existing named export, before diffing exports. If a release removes only the default export (keeping the named export), that breaking removal is silently skipped because the default export entry was removed from the base-side map.
Agent Prompt
### Issue description
`dedupeReexportAliases()` deletes certain default-export entries (TypeRefSchema alias to a named export) before diffing. This can hide a real API-breaking change: removing the default export while keeping the named export.
### Issue Context
`computeAPIDiff()` builds `baseExports` / `compareExports` using `dedupeReexportAliases(buildExportMap(...))`, and then `diffExports()` only sees what remains in those maps.
### Fix Focus Areas
- components/entities/semantic-schema-diff/api-diff.ts[159-190]
- components/entities/semantic-schema-diff/api-diff.ts[322-349]
### Suggested fix
Change the “dedupe default alias” behavior so it never suppresses add/remove detection:
- Option A (preferred): only suppress the default-alias entry when it exists on *both* sides and points to the same target (i.e., it’s unchanged noise), but keep it when present on only one side so `diffExports()` can report added/removed.
- Option B: remove `dedupeReexportAliases()` entirely and instead handle extractor inconsistency at the diff stage (e.g., detect the alias pattern and downgrade it, but still emit an add/remove when one side truly lacks the default export).
In either option, ensure that removing a default export is still surfaced as `export-removed` (breaking) even if the named export remains.
ⓘ 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 d87a9d5 |
…e types In the deferChanges path (default for UI lane compares) common snaps aren't imported eagerly, so deriveChangeTypes() could fail to load the merge-base Version and drop a genuinely-changed component from the diff. Import the missing snap(s) on demand and retry once before giving up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Reworks how changes are viewed across the app: the single-component compare, the lane compare, and the workspace/lane overviews, onto one shared, bulk-fetched implementation, and replaces the Monaco-based code diff with a lean shiki-based viewer. It also brings the semantic API diff into the compare surface as a first-class view, and fixes lane-vs-main comparison. The goal was best-in-class feature and performance parity for reviewing changes, with Bit's own tokens, theme, and identity.
Unified compare surface
InlineComponentCompareis now the single implementation shared by the single-component compare page and lane-compare: one set of view-mode tabs (Code / Preview / Docs / Dependencies / Tests / Config / API), one diff pane, one toolbar.data-view-modeattribute plus CSS, so a mode click never remounts or re-fetches (fixes the "switching is slow" and console-error regressions).DiffModeProvider/useDiffModegives a single global split/unified toggle across every file instead of a per-file one.Comparing a lane against main
getHeadOnMain/importMainHeads) falls back to the remote default-lane ref and bulk-imports the missing main heads, so lane-only components whose main version was never fetched are compared correctly instead of being misclassified as new or "no changes".skipUpToDatewas dropping every component of a lane that is forked and ahead of main: a forward-only lane has no target-only snaps, so each read as up-to-date and got filtered out; source-ahead components are now kept. Second, thediffStatusresult-memo key was derived fromLane.hash(a method, not the raw hash), so it never invalidated when the lane or main advanced and served a stale empty result; it is now keyed on the real hash plus main heads.getHeadOnMain, so "merge from main" no longer drops components without a local main head.baseSource: 'workspace' | 'scope'): a small cloud badge on the sidebar row marks bases resolved from the remote scope versus the local workspace.Bulk data fetching
CompareDataProviderplus a bulkcompareComponents(pairs)GraphQL query fetch code/aspect/test diffs for all component pairs in one paginated call, keyed bycompareId; inline panels read from this context instead of firing per-file queries.FileRegistrycontext feeds the sidebar file tree from the same bulk result.changesderivation dropped from about 47s to about 8s: diff rawVersionobjects instead of fully loading both components, defer the (UI-unused) API change-type off the critical path, and lift the artificial concurrency cap. A client render loop in the file registry was also removed viauseSyncExternalStoreplus a version-guarded notify.New code diff viewer (no Monaco)
teambit.code/ui/diff-viewerbuilt on shiki'screateHighlighterCoreplus@shikijs/engine-javascript(WASM-free JS regex engine, grammars code-split per language), far leaner than the Monaco diff it replaces.inline-code-compare(Monaco still backs the live code editor, untouched). Addsjsonc/json5highlighting for config files.Semantic API diff view
API diff correctness
X (default)added to public API" on docs-only diffs:export default Xwas extracted inconsistently across builds (UnImplementedSchemaversus a"X (default)"TypeRefSchemaalias). The extractor now emits a stabledefaultre-reference, and the diff is made robust to historical artifacts: it filtersUnImplementedSchemaexports, dedupes redundant re-export aliases, and surfaces genuinely-unresolved exports instead of diffing them.@see, and so on), not just the description.Base / compare resolution
?version) points the compare side at that clean version instead.computeComparediffs against on-disk files when the compare side is the checked-out workspace (compareIsLiveWorkspace); those mutable diffs are never cached.Shared overviews
workspace-overviewandlane-overviewnow consume one sharedcomponents-overviewcomponent instead of duplicating the grid/card logic.