fix(frontend): show bundles in dependency compare picker (gate on native_packages, not manifest)#2373
Conversation
The bundle compare selector was extracted from the Manifest tab (#1557) and reused on the Dependencies tab, carrying over its `manifest_count > 0` filter. That gate is correct for manifest diffs but wrong for dependency diffs: the Dependencies tab compares `native_packages`, which is populated independently of the manifest. Bundles uploaded without the partial-update manifest system have `manifest_count = 0` but still carry native packages, so the compare dropdown showed an empty list and dependency comparison was impossible. Add a `compareMode` prop so each tab gates candidates on the column it actually diffs: `manifest_count` for manifests, `native_packages` for dependencies.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBundleCompareSelect gains a ChangesBundle Compare Mode Support
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/bundle/BundleCompareSelect.vue (1)
235-264:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix unsafe reuse of the same Supabase query builder in
searchCompareVersions
searchCompareVersions()builds onebaseQueryand then chains both.ilike(...)and.eq('id', ...)off that same mutable builder insidePromise.all, so the second chain can leak/mix filters/options into the first request. Create a freshbuildCompareBaseQuery()for each query chain.🛡️ Proposed fix — build an independent query per request
- const baseQuery = buildCompareBaseQuery() - .neq('id', props.currentVersionId) - const numericId = Number(term) let data: VersionRow[] | null = null let error: unknown = null if (Number.isNaN(numericId)) { - const response = await baseQuery + const response = await buildCompareBaseQuery() + .neq('id', props.currentVersionId) .ilike('name', `%${term}%`) .order('created_at', { ascending: false }) .limit(5) if (requestId !== compareSearchRequestId.value) return data = response.data ?? null error = response.error } else { const [nameResponse, idResponse] = await Promise.all([ - baseQuery + buildCompareBaseQuery() + .neq('id', props.currentVersionId) .ilike('name', `%${term}%`) .order('created_at', { ascending: false }) .limit(5), - baseQuery + buildCompareBaseQuery() + .neq('id', props.currentVersionId) .eq('id', numericId) .order('created_at', { ascending: false }) .limit(5), ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/bundle/BundleCompareSelect.vue` around lines 235 - 264, The code reuses a single mutable Supabase query builder (baseQuery from buildCompareBaseQuery()) for multiple concurrent queries in searchCompareVersions, which can leak filters between chains; to fix, call buildCompareBaseQuery() separately for each branch instead of reusing baseQuery (e.g., create one local query via buildCompareBaseQuery() for the .ilike(...) call and another fresh buildCompareBaseQuery() for the .eq('id', ...) call used in Promise.all), ensuring each chain mutates its own query builder and then use their responses as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/bundle/BundleCompareSelect.vue`:
- Around line 235-264: The code reuses a single mutable Supabase query builder
(baseQuery from buildCompareBaseQuery()) for multiple concurrent queries in
searchCompareVersions, which can leak filters between chains; to fix, call
buildCompareBaseQuery() separately for each branch instead of reusing baseQuery
(e.g., create one local query via buildCompareBaseQuery() for the .ilike(...)
call and another fresh buildCompareBaseQuery() for the .eq('id', ...) call used
in Promise.all), ensuring each chain mutates its own query builder and then use
their responses as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0315486d-dd4e-4b19-b707-2d4cd4f0ab99
📒 Files selected for processing (2)
src/components/bundle/BundleCompareSelect.vuesrc/pages/app/[app].bundle.[bundle].dependencies.vue
The Supabase query builder is mutable and returns `this`, so reusing one `baseQuery` instance across the two concurrent chains in the numeric-search `Promise.all` mutated shared state and conflated their filters (name ilike + id eq), returning near-nothing for numeric searches. Build an independent query via buildCompareBaseQuery() for each chain. Addresses CodeRabbit review on #2373.
|
Addressed the CodeRabbit "outside diff range" finding on |
|
1 participant
Problem
On a bundle's Dependencies tab, the "Compare with bundle" dropdown is empty for many apps, so you can't compare two bundles' native dependencies at all.
The compare selector was originally built for the Manifest tab and later extracted into a shared
BundleCompareSelectcomponent and reused on the Dependencies tab (#1557). It carried over the Manifest tab's eligibility filter —manifest_count > 0— to all three of its candidate queries (latest / deploy-history / search).That filter is correct for manifest diffs (a bundle with no manifest has nothing to diff), but wrong for dependency diffs:
native_packages(ajsonb[]column onapp_versions).native_packagesis populated on upload independently of the manifest.manifest/manifest_countis only populated for bundles using the partial-update system.manifest_count = 0on every bundle — and the dependency compare picker filters them all out, even though every bundle has perfectly comparable native packages.Reproduction / impact (real data)
For one affected app with 13 active bundles, every bundle has
manifest_count = 0and 3 native packages each:manifest_count > 0(old)native_packages IS NOT NULL(new)Fix
Add a
compareModeprop ('manifest' | 'dependencies', default'manifest') toBundleCompareSelect. A singlebuildCompareBaseQuery()helper now gates candidates on the column each tab actually diffs:manifest_count > 0(unchanged behavior)native_packages IS NOT NULLThe Dependencies page passes
compare-mode="dependencies"; the Manifest page is untouched and keeps its existing (correct) behavior via the default.Scope
src/components/bundle/BundleCompareSelect.vue— addcompareModeprop +buildCompareBaseQuery()helper; route all three candidate queries (latest, deploy-history, search) through it.src/pages/app/[app].bundle.[bundle].dependencies.vue— passcompare-mode="dependencies".No schema, backend, or API changes. The actual dependency comparison logic on the page is unchanged — this only fixes which bundles are eligible to appear in the picker.
Test plan
manifest_count = 0, open Dependencies → "Compare with bundle" now lists other bundles (latest + searchable by name/ID).manifest_count > 0).Summary by CodeRabbit