feat: add analyze client logic at size increase warning#3077
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds package-size cause analysis with IndexedDB graph persistence, npm dependency resolution, a cancellable web worker, formatted diff results, and a package-page analysis UI with English and Spanish translations. ChangesPackage size cause analysis
Sequence Diagram(s)sequenceDiagram
participant PackagePage
participant SizeIncreaseAnalysis
participant useAnalyzeCauseWorker
participant AnalyzeCauseWorker
participant NpmxPkgSizeDB
PackagePage->>SizeIncreaseAnalysis: provide package and version props
SizeIncreaseAnalysis->>useAnalyzeCauseWorker: start analysis
useAnalyzeCauseWorker->>AnalyzeCauseWorker: post analyze-cause
AnalyzeCauseWorker->>NpmxPkgSizeDB: resolve and load persisted graphs
AnalyzeCauseWorker-->>useAnalyzeCauseWorker: return diff and summary
useAnalyzeCauseWorker-->>SizeIncreaseAnalysis: format and expose results
SizeIncreaseAnalysis-->>PackagePage: render analysis details
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
📊 Dependency Size ChangesWarning This PR adds 3.2 MB of new dependencies, which exceeds the threshold of 200 kB.
Total size change: 3.2 MB |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
app/utils/pkg-size/resolve-and-persist-graph.ts (3)
96-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFunction exceeds the ~50-line guideline; mixes several concerns.
resolveAndPersistGraphhandles BFS queue draining, mandatory-vs-optional set computation, and session persistence in one ~100-line function. Consider extracting the BFS traversal and the mandatory-key computation into separate helper functions for readability/testability.As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
🤖 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 `@app/utils/pkg-size/resolve-and-persist-graph.ts` around lines 96 - 195, The resolveAndPersistGraph function is too long and combines graph traversal, mandatory-key computation, and session persistence. Extract the BFS queue-processing logic into a focused helper and extract the mandatory-key traversal currently represented by traverse into another helper, then have resolveAndPersistGraph coordinate those helpers and persist the resulting session while preserving existing behavior.
133-152: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant package writes on every revisit.
db.storePackageis called even whenpkgKeywas already visited (theelsebranch), re-writing identical package data on every occurrence in the graph. For widely-shared dependencies this multiplies IndexedDB writes unnecessarily. Consider splitting into an "upsert package" call (only on first visit) plus a lightweight "add edge" call (on every occurrence):♻️ Suggested refactor
if (!visited.has(pkgKey)) { visited.add(pkgKey) packageSizes.set(pkgKey, pkgData.dist?.unpackedSize || 0) - await db.storePackage(pkgKey, pkgData, item.parentKey, item.range, item.isOptional) + await db.upsertPackage(pkgKey, pkgData) for (const [depName, depRange] of Object.entries(pkgData.dependencies || {})) { queue.push({ name: depName, range: depRange, parentKey: pkgKey, isOptional: false }) } for (const [depName, depRange] of Object.entries(pkgData.optionalDependencies || {})) { queue.push({ name: depName, range: depRange, parentKey: pkgKey, isOptional: true }) } - } else { - await db.storePackage(pkgKey, pkgData, item.parentKey, item.range, item.isOptional) } + if (item.parentKey) { + await db.addDependencyEdge(item.parentKey, pkgData.name, pkgKey, item.range, item.isOptional) + }🤖 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 `@app/utils/pkg-size/resolve-and-persist-graph.ts` around lines 133 - 152, Update the package-processing loop around visited and storePackage so package metadata is persisted only when pkgKey is first encountered, while every occurrence still records its parent relationship through a lightweight edge-only database operation. Replace the repeated storePackage call in the visited branch with the existing or newly introduced edge-recording method, preserving dependency queueing only for first visits.
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNoisy error logging on intentional cancellation.
When the analysis is aborted mid-fetch,
fetchthrows anAbortError, which is logged viaconsole.errorhere as if it were a real failure. Consider distinguishing abort-driven rejections from genuine fetch errors to avoid misleading console noise on every user-initiated cancellation.🤖 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 `@app/utils/pkg-size/resolve-and-persist-graph.ts` around lines 89 - 93, Update the catch block in the graph resolution flow to detect fetch AbortError rejections and suppress the console.error call for intentional cancellation, while retaining the existing error logging and undefined return for genuine failures. Use the caught error’s standard abort-identification fields or established project helper.app/utils/pkg-size/db.ts (1)
9-136: 🧹 Nitpick | 🔵 TrivialNo cleanup/eviction strategy for persisted analysis data.
packages,dependencyEdges, andsessionsgrow indefinitely with no TTL, size cap, or cleanup path visible in this layer — each re-run of an analysis for the same root key adds new edge rows (viaadd()) without removing stale ones from prior incomplete runs. Since this is client-side IndexedDB, consider adding a retention/cleanup policy (e.g., prune finished sessions past N entries, or dedupe edges) before this grows into a follow-up feature (pkg-sizepage).🤖 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 `@app/utils/pkg-size/db.ts` around lines 9 - 136, Add a cleanup/retention strategy to NpmxPkgSizeDB so persisted packages, dependencyEdges, and sessions do not grow indefinitely across analyses. Ensure repeated or incomplete runs for the same root key remove or deduplicate stale dependency edges and apply a bounded policy for finished sessions, using the existing initSession, updateSession, and storePackage transaction flows without changing their public API.app/composables/pkg-size/useAnalizeCauseWorker.ts (2)
138-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the leftover debug logging.
These four
console.logcalls are unconditional and ship to production; the file itself suppresses ano-consolerule for theconsole.errorat Line 176, so these will likely trip lint too. Dropping them also removes the twoconstdeclarations sitting directly in aswitchcase body.♻️ Proposed cleanup
analyzing.value = false cancelling.value = false - const totalDeltaMB = (msg.summary.sizeDelta / (1024 * 1024)).toFixed(2) - const jsDeltaMB = (msg.summary.mandatorySizeDelta / (1024 * 1024)).toFixed(2) - - console.log(`📊 FINAL DIFF BALANCE:`) - console.log( - `📦 Size variation (Total): ${totalDeltaMB} MB (${msg.summary.sizeDelta} bytes)`, - ) - console.log( - `🧠 Size variation (JS Core): ${jsDeltaMB} MB (${msg.summary.mandatorySizeDelta} bytes)`, - ) - console.log( - `🧩 Dependency variation: ${msg.summary.netDependencies > 0 ? '+' : ''}${msg.summary.netDependencies} (Added: ${msg.summary.added}, Removed: ${msg.summary.removed})`, - ) break🤖 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 `@app/composables/pkg-size/useAnalizeCauseWorker.ts` around lines 138 - 150, Remove the four unconditional console.log calls and the now-unused totalDeltaMB and jsDeltaMB declarations from the switch case handling the final diff balance in the worker message handler. Preserve the surrounding production behavior and existing console.error handling.
61-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnexplained 256 ms delays before posting messages.
analyzingflips immediately, then the message is withheld for 256 ms (same value again incancelAnalyzeCauseat Line 92 and 1 s in theresulthandler at Line 115). If these exist to let the spinner/expand transition settle, that intent isn't obvious from the code; otherwise they are pure added latency. Please either drop them or name the constant and comment why.🤖 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 `@app/composables/pkg-size/useAnalizeCauseWorker.ts` around lines 61 - 68, The analyze worker flow contains unexplained hard-coded delays before posting messages and in cancellation/result handling. Remove the 256 ms and 1 s delays if they are unnecessary; otherwise replace them with named constants and add concise comments explaining the required UI transition or spinner timing, covering the logic in the analyze, cancelAnalyzeCause, and result-handler paths.app/composables/useNumberFormatter.ts (1)
24-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated threshold branches.
The sign check yields two identical ladders; only the value being compared differs. Comparing the magnitude once and formatting the signed value gives the same output (
-1500→-1.5 kB) in a third of the lines.♻️ Proposed refactor
format: (bytes: number) => { - if (bytes >= 0) { - if (bytes < KB) { - return t('package.size.b', { - size: decimalNumberFormatter.value.format(bytes), - }) - } - if (bytes < MB) { - return t('package.size.kb', { - size: decimalNumberFormatter.value.format(bytes / KB), - }) - } - - return t('package.size.mb', { - size: decimalNumberFormatter.value.format(bytes / MB), - }) - } - - const v = Math.abs(bytes) + const magnitude = Math.abs(bytes) - if (v < KB) { + if (magnitude < KB) { return t('package.size.b', { size: decimalNumberFormatter.value.format(bytes), }) } - if (v < MB) { + if (magnitude < MB) { return t('package.size.kb', { size: decimalNumberFormatter.value.format(bytes / KB), }) }🤖 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 `@app/composables/useNumberFormatter.ts` around lines 24 - 57, Refactor the format function in useNumberFormatter so it computes the absolute magnitude once for threshold comparisons, then uses the original signed bytes value for formatting. Preserve the existing byte, kilobyte, and megabyte translations and ensure negative values retain their sign in the formatted output.app/utils/pkg-size/analize-cause-client-worker.ts (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider settling the "analize" vs "analyze" spelling now.
The file names use
analize-cause-*while the exported symbols useanalyze(useAnalyzeCauseWorker) and the worker name usesAnalyse(NpmxPkgSizeAnalyseCauseWorker). Three spellings across brand-new files will be churn to fix later; aligning onanalyze-cause-*while the surface is fresh is cheap.🤖 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 `@app/utils/pkg-size/analize-cause-client-worker.ts` around lines 1 - 5, Standardize the new package-size cause worker naming on “analyze.” Rename the `analize-cause-*` worker files and update the import in the exported `worker` instance, while preserving the existing `useAnalyzeCauseWorker` naming and aligning the worker name `NpmxPkgSizeAnalyseCauseWorker` to the same spelling.app/components/Package/SizeIncrease.vue (3)
214-217: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a stable key instead of the loop index.
The list is re-filtered when
allDependenciestoggles, so index keys cause Vue to reuse DOM nodes across different packages (wrong status colours/icons momentarily, and lost<abbr>tooltip state).item.nameis unique per diff entry.♻️ Proposed change
- <li - v-for="(item, index) in result" - :key="index" + <li + v-for="item in result" + :key="item.name"🤖 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 `@app/components/Package/SizeIncrease.vue` around lines 214 - 217, Replace the index-based key on the result list in SizeIncrease.vue with the stable unique item.name key. Keep the v-for index available only if needed for rendering, while ensuring Vue tracks each diff entry by package name when allDependencies changes.
74-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the inline
focus-visible:outline-amber-600utilities on these buttons.Focus-visible styling for
button/selectis applied globally inapp/assets/main.css; per-element utilities diverge from that baseline. Also note the two toggle buttons place@clickbeforeclass, unlike the analyse button — worth aligning attribute order.Based on learnings: focus-visible styling for buttons and selects is applied globally via main.css, so individual buttons should not rely on inline focus-visible utility classes.
Also applies to: 174-178, 191-195
🤖 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 `@app/components/Package/SizeIncrease.vue` around lines 74 - 79, Remove the inline focus-visible:outline-amber-600 utility from the button elements in SizeIncrease.vue, including the buttons referenced at the other affected ranges, while preserving their global focus styling from main.css. Align the toggle buttons’ attribute order with the analyse button by placing `@click` after class.Source: Learnings
304-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTranslate the inline comment to English and reconsider the fixed
max-height.The comment on Line 312 is in Spanish; keep code comments in English for consistency with the rest of the codebase. Separately, the 800px cap means long diff lists (when
noResultScrollis enabled) are clipped for the duration of the enter/leave animation.♻️ Proposed change
- max-height: 800px; /* Altura máxima segura de despliegue para la transición */ + /* Upper bound for the expand animation; content taller than this is clipped mid-transition. */ + max-height: 800px;🤖 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 `@app/components/Package/SizeIncrease.vue` around lines 304 - 313, Update the inline comment in the expand transition styles to English, and revise the fixed max-height used by .expand-enter-active and .expand-leave-active so long diff lists are not clipped during animations, including when noResultScroll is enabled. Preserve the existing transition behavior while removing the restrictive 800px cap.
🤖 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.
Inline comments:
In `@app/components/Package/SizeIncrease.vue`:
- Around line 108-155: Update the summary markers in the SizeIncrease
component’s summary section so 📦, 🧠, and 🧩 are each wrapped in span elements
with aria-hidden="true". Leave the surrounding i18n content and layout
unchanged.
- Around line 106-107: Update the main panel v-if in SizeIncrease so it remains
visible whenever summary exists, even when the filtered result is empty; keep
the result check out of the outer visibility gate so the dependency filter
toggle remains usable. Inside the result list container, add an empty-state
rendering path for result.length === 0 while preserving the existing item
rendering for non-empty results.
- Around line 6-24: Update useComparedVersion in SizeIncrease.vue to return the
declared comparedVersion prop instead of props.diff.comparisonVersion, ensuring
the worker and displayed analysis use the parent-provided value. Also replace
the unnecessary useVersion computed wrapper with direct use of the version prop
where it is consumed.
In `@app/composables/pkg-size/useAnalizeCauseWorker.ts`:
- Around line 49-50: Make correlation IDs unique across composable instances in
the request flow using the module-level shared worker. Update the ID generation
and result/error/aborted message matching in the composable’s worker handlers,
using a per-instance token such as crypto.randomUUID combined with the local
counter, so each consumer only accepts its own messages.
- Around line 167-195: Update the worker setup in onMounted to subscribe to both
error and messageerror events, using handlers that reset analyzing and
cancelling and surface the failure through the existing error state. Store or
reuse these handlers so onUnmounted removes both listeners alongside
handleWorkerMessage, while preserving the existing message handling and worker
availability flow.
- Around line 6-10: Update useAnalyzeCauseWorker to accept packageName as a
MaybeRefOrGetter<string> rather than a plain string, and resolve it with
toValue(packageName) when constructing the worker request. Ensure each later
analyze-cause post uses the current package name while preserving the existing
version and comparedVersion handling.
- Around line 182-188: Move the available watcher in useAnalizeCauseWorker from
the post-import async path into synchronous setup so it is created within the
component instance scope and automatically stopped on unmount. Preserve its
current sources, availability condition, immediate execution, and post-flush
behavior; do not create it after await import.
In `@app/utils/pkg-size/analize-cause-worker.ts`:
- Around line 12-172: Update the analyze-cause message handler to abort any
existing abortController before creating a new one, then capture the new
controller in a request-local const and use that local controller throughout the
request, including resolveAndPersistGraph, checkAborted, and the catch
classification. Keep the shared abortController reference available for handling
future abort messages while ensuring each request’s cleanup and error reporting
use its own controller.
In `@i18n/locales/es.json`:
- Around line 431-437: Update the Spanish locale entries `js_core_size` and
`only_deps` to use the unaccented “solo” form, matching the spelling convention
used elsewhere in the locale file; leave the surrounding translations and
placeholders unchanged.
---
Nitpick comments:
In `@app/components/Package/SizeIncrease.vue`:
- Around line 214-217: Replace the index-based key on the result list in
SizeIncrease.vue with the stable unique item.name key. Keep the v-for index
available only if needed for rendering, while ensuring Vue tracks each diff
entry by package name when allDependencies changes.
- Around line 74-79: Remove the inline focus-visible:outline-amber-600 utility
from the button elements in SizeIncrease.vue, including the buttons referenced
at the other affected ranges, while preserving their global focus styling from
main.css. Align the toggle buttons’ attribute order with the analyse button by
placing `@click` after class.
- Around line 304-313: Update the inline comment in the expand transition styles
to English, and revise the fixed max-height used by .expand-enter-active and
.expand-leave-active so long diff lists are not clipped during animations,
including when noResultScroll is enabled. Preserve the existing transition
behavior while removing the restrictive 800px cap.
In `@app/composables/pkg-size/useAnalizeCauseWorker.ts`:
- Around line 138-150: Remove the four unconditional console.log calls and the
now-unused totalDeltaMB and jsDeltaMB declarations from the switch case handling
the final diff balance in the worker message handler. Preserve the surrounding
production behavior and existing console.error handling.
- Around line 61-68: The analyze worker flow contains unexplained hard-coded
delays before posting messages and in cancellation/result handling. Remove the
256 ms and 1 s delays if they are unnecessary; otherwise replace them with named
constants and add concise comments explaining the required UI transition or
spinner timing, covering the logic in the analyze, cancelAnalyzeCause, and
result-handler paths.
In `@app/composables/useNumberFormatter.ts`:
- Around line 24-57: Refactor the format function in useNumberFormatter so it
computes the absolute magnitude once for threshold comparisons, then uses the
original signed bytes value for formatting. Preserve the existing byte,
kilobyte, and megabyte translations and ensure negative values retain their sign
in the formatted output.
In `@app/utils/pkg-size/analize-cause-client-worker.ts`:
- Around line 1-5: Standardize the new package-size cause worker naming on
“analyze.” Rename the `analize-cause-*` worker files and update the import in
the exported `worker` instance, while preserving the existing
`useAnalyzeCauseWorker` naming and aligning the worker name
`NpmxPkgSizeAnalyseCauseWorker` to the same spelling.
In `@app/utils/pkg-size/db.ts`:
- Around line 9-136: Add a cleanup/retention strategy to NpmxPkgSizeDB so
persisted packages, dependencyEdges, and sessions do not grow indefinitely
across analyses. Ensure repeated or incomplete runs for the same root key remove
or deduplicate stale dependency edges and apply a bounded policy for finished
sessions, using the existing initSession, updateSession, and storePackage
transaction flows without changing their public API.
In `@app/utils/pkg-size/resolve-and-persist-graph.ts`:
- Around line 96-195: The resolveAndPersistGraph function is too long and
combines graph traversal, mandatory-key computation, and session persistence.
Extract the BFS queue-processing logic into a focused helper and extract the
mandatory-key traversal currently represented by traverse into another helper,
then have resolveAndPersistGraph coordinate those helpers and persist the
resulting session while preserving existing behavior.
- Around line 133-152: Update the package-processing loop around visited and
storePackage so package metadata is persisted only when pkgKey is first
encountered, while every occurrence still records its parent relationship
through a lightweight edge-only database operation. Replace the repeated
storePackage call in the visited branch with the existing or newly introduced
edge-recording method, preserving dependency queueing only for first visits.
- Around line 89-93: Update the catch block in the graph resolution flow to
detect fetch AbortError rejections and suppress the console.error call for
intentional cancellation, while retaining the existing error logging and
undefined return for genuine failures. Use the caught error’s standard
abort-identification fields or established project helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8eec16e4-4805-4fb8-b719-d0656b700b96
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
app/components/Package/SizeIncrease.vueapp/composables/pkg-size/useAnalizeCauseWorker.tsapp/composables/useNumberFormatter.tsapp/pages/package/[[org]]/[name].vueapp/utils/pkg-size/analize-cause-client-worker.tsapp/utils/pkg-size/analize-cause-worker.tsapp/utils/pkg-size/check-aborted.tsapp/utils/pkg-size/db.tsapp/utils/pkg-size/resolve-and-persist-graph.tsapp/utils/pkg-size/types.tsi18n/locales/en.jsoni18n/locales/es.jsoni18n/schema.jsonpackage.json
|
Nice PR! Can you rename the analize files to analyze? 😅 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/components/Package/SizeIncreaseAnalysis.vue (1)
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
definePropsoverdefineModelfor values that are never written.None of the three models is mutated in this component, and the parent (
SizeIncrease.vue) binds plain props (:package-name,:version,:comparison-version) rather thanv-model.defineModeladdsupdate:*emits and local-write semantics that are unused here; plain props communicate the one-way contract more clearly and keep the composable input read-only.♻️ Suggested change
-const packageName = defineModel<string | null | undefined>('packageName') -const version = defineModel<string | null | undefined>('version') -const comparisonVersion = defineModel<string | null | undefined>('comparisonVersion') +const props = defineProps<{ + packageName?: string | null + version?: string | null + comparisonVersion?: string | null +}>() + +const packageName = computed(() => props.packageName) +const version = computed(() => props.version) +const comparisonVersion = computed(() => props.comparisonVersion)🤖 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 `@app/components/Package/SizeIncreaseAnalysis.vue` around lines 4 - 6, Replace the read-only defineModel declarations for packageName, version, and comparisonVersion with defineProps, preserving their nullable/undefined string types and existing prop names. Keep all downstream reads unchanged and remove the unused model update semantics.
🤖 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.
Inline comments:
In `@app/components/Package/SizeIncreaseAnalysis.vue`:
- Around line 59-75: The error block in the SizeIncreaseAnalysis component
currently hides the failure message and does not announce failures. Update the
error rendering near the package.size_increase.analyze.error label to display
the populated error value, and invoke the existing announce() helper when
analysis fails as well as when it completes successfully.
In `@app/utils/pkg-size/db.ts`:
- Around line 51-58: Update the session creation flow around cleanup() and
initSession() so eviction runs after inserting the new session(s), using the
post-insertion count to enforce MAX_SESSIONS. Perform insertion and deletion
within the same read-write transaction, removing the oldest sessions by
timestamp until the count is within the limit.
- Around line 51-58: The cleanup logic in cleanup() must not evict active
sessions: select only sessions with isFinished === true before ordering by
timestamp and applying the eviction limit, while retaining unfinished sessions
regardless of age. Preserve updateSession() behavior and the existing deletion
flow for completed sessions.
- Around line 29-33: Update the Dexie v2 definition in the database
initialization to retain the historical v1 schema through the version
declaration and add an upgrade handler that reads legacy dependencyEdges
records, transforms and inserts them into the new edges store, then removes the
obsolete store data as appropriate. Ensure existing v1 users receive this
migration before dependencyEdges is dropped.
---
Nitpick comments:
In `@app/components/Package/SizeIncreaseAnalysis.vue`:
- Around line 4-6: Replace the read-only defineModel declarations for
packageName, version, and comparisonVersion with defineProps, preserving their
nullable/undefined string types and existing prop names. Keep all downstream
reads unchanged and remove the unused model update semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e996aaa9-5aa4-4b14-b4b9-99b16cd094d6
📒 Files selected for processing (13)
app/components/Package/SizeIncrease.vueapp/components/Package/SizeIncreaseAnalysis.vueapp/composables/pkg-size/useAnalizeCauseWorker.tsapp/composables/useNumberFormatter.tsapp/pages/package/[[org]]/[name].vueapp/utils/pkg-size/analize-cause-client-worker.tsapp/utils/pkg-size/analize-cause-worker.tsapp/utils/pkg-size/db.tsapp/utils/pkg-size/resolve-and-persist-graph.tsapp/utils/pkg-size/types.tsi18n/locales/en.jsoni18n/locales/es.jsoni18n/schema.json
🚧 Files skipped from review as they are similar to previous changes (9)
- app/utils/pkg-size/analize-cause-client-worker.ts
- app/composables/useNumberFormatter.ts
- i18n/locales/en.json
- i18n/locales/es.json
- app/utils/pkg-size/types.ts
- i18n/schema.json
- app/utils/pkg-size/analize-cause-worker.ts
- app/utils/pkg-size/resolve-and-persist-graph.ts
- app/composables/pkg-size/useAnalizeCauseWorker.ts
| <div v-if="summary || error" class="mt-3 border-t border-amber-600/20 pt-3 flex flex-col"> | ||
| <div | ||
| v-if="error" | ||
| class="border border-rose-500/20 bg-rose-500/10 rounded-md px-3 py-2.5 flex items-center gap-2" | ||
| > | ||
| <span | ||
| class="i-lucide:alert-circle w-4 h-4 shrink-0 text-rose-700 dark:text-rose-400 mt-0.5" | ||
| aria-hidden="true" | ||
| /> | ||
| <div class="flex flex-col gap-1"> | ||
| <span | ||
| class="text-xs font-bold uppercase tracking-wider text-rose-800 dark:text-rose-400" | ||
| > | ||
| {{ $t('package.size_increase.analyze.error') }} | ||
| </span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i 'useAnalizeCauseWorker.ts' --exec cat -n
rg -nP '\bannounce\s*\(' app/composables/useCommandPalette.tsRepository: npmx-dev/npmx.dev
Length of output: 8031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate SizeIncreaseAnalysis.vue and announcer files"
fd -i 'SizeIncreaseAnalysis.vue|announc' app | sed -n '1,80p'
echo
echo "## SizeIncreaseAnalysis relevant lines"
file="$(fd -i 'SizeIncreaseAnalysis.vue' app | head -n1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
cat -n "$file" | sed -n '1,140p'
fi
echo
echo "## announcer usage/definition searches"
rg -n "function announce|announce\\(|announce:" app --glob '*.ts' --glob '*.vue' | sed -n '1,120p'
echo
echo "## relevant composable snippets around announce definition"
if [ -f app/composables/useCommandPalette.ts ]; then
cat -n app/composables/useCommandPalette.ts | sed -n '70,115p'
fiRepository: npmx-dev/npmx.dev
Length of output: 10018
Show the worker failure message and announce it.
error is populated from AnalyzeWorkerResponse.message and ErrorEvent.message, but the error block only renders the generic package.size_increase.analyze.error label, so the root cause is hidden. Display error if present, and call the project announce() helper for failure as well as successful completion.
[low_effort and_high_reward]
🤖 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 `@app/components/Package/SizeIncreaseAnalysis.vue` around lines 59 - 75, The
error block in the SizeIncreaseAnalysis component currently hides the failure
message and does not announce failures. Update the error rendering near the
package.size_increase.analyze.error label to display the populated error value,
and invoke the existing announce() helper when analysis fails as well as when it
completes successfully.
Source: Learnings
gameroman
left a comment
gameroman
left a comment
There was a problem hiding this comment.
Hi, could you add some screenshots or videos to the description, please, so it's easier to see visually, thank you!
done |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/composables/pkg-size/useAnalyzeCauseWorker.ts (1)
76-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRe-check
available.value(or the target refs) after the artificial delay before posting.
available.valueis only validated at function entry;postMessagefires ~256ms later and re-readspackageName.value/version.value/comparedVersion.valueat that point without re-validating. Since these are typedstring | undefined | null, fast navigation during that window can postnull/undefinedfields to the worker (unchecked, sinceWorker.postMessageacceptsany), producing an incorrect analysis request.🤖 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 `@app/composables/pkg-size/useAnalyzeCauseWorker.ts` around lines 76 - 97, After the artificial delay in the analysis flow, re-check available.value and the target refs packageName.value, comparedVersion.value, and version.value before worker.postMessage. Return without posting when the worker is unavailable or any required target ref is nullish, and use validated values for the request payload.
🧹 Nitpick comments (1)
test/nuxt/a11y.spec.ts (1)
4623-4638: 📐 Maintainability & Code Quality | 🔵 TrivialConsider extending a11y coverage to loading/error states.
Only the idle "result available" state is exercised. Dynamic regions (loading spinner, cancel/error states) are common sources of a11y regressions (e.g. missing
aria-live/role="alert"), so additional cases mockinganalyzing/loading/errorwould strengthen coverage.🤖 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 `@test/nuxt/a11y.spec.ts` around lines 4623 - 4638, The SizeIncreaseAnalysis accessibility test currently covers only the completed state. Extend the describe block to mount PackageSizeIncreaseAnalysis with mocked analyzing/loading and error states, then run runAxe and assert no violations for each state, preserving the existing result-available coverage.
🤖 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.
Inline comments:
In `@app/composables/pkg-size/useAnalyzeCauseWorker.ts`:
- Around line 169-170: Remove the two leftover console.log calls printing
summary.value and rawResult.value from the worker flow. Leave the legitimate
error logging and its existing oxlint handling unchanged.
- Around line 81-82: Fix the dev/test branch in the currentId assignment so the
counter is incremented and the incremented value is retained, rather than
reassigning the postfix operator’s old value. Keep crypto.randomUUID() for
non-dev/test execution and preserve the existing run-correlation behavior.
In `@test/unit/pkg-size-analyze-worker.spec.ts`:
- Around line 91-105: Await the initial messageHandler call for the
analyze-cause message in the mismatched-id test, ensuring its internal promise
chain completes before sending the abort message and the test returns.
In `@test/unit/pkg-size-analyze.spec.ts`:
- Around line 84-98: The resolveAndPersistGraph test currently verifies only
that some package data exists, so it can miss BFS traversal and edge persistence
regressions. Replace the broad pkgCount assertion with explicit assertions for
the expected root package, mandatory child, optional child, and the two
persisted dependency edges, using the existing database APIs and Vitest
assertions.
- Around line 78-81: Update the afterEach cleanup hook in
pkg-size-analyze.spec.ts to be asynchronous and await db.dropDatabase() after
resetting the server handlers, ensuring IndexedDB deletion completes before the
next test begins.
---
Outside diff comments:
In `@app/composables/pkg-size/useAnalyzeCauseWorker.ts`:
- Around line 76-97: After the artificial delay in the analysis flow, re-check
available.value and the target refs packageName.value, comparedVersion.value,
and version.value before worker.postMessage. Return without posting when the
worker is unavailable or any required target ref is nullish, and use validated
values for the request payload.
---
Nitpick comments:
In `@test/nuxt/a11y.spec.ts`:
- Around line 4623-4638: The SizeIncreaseAnalysis accessibility test currently
covers only the completed state. Extend the describe block to mount
PackageSizeIncreaseAnalysis with mocked analyzing/loading and error states, then
run runAxe and assert no violations for each state, preserving the existing
result-available coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d7b61211-5a05-4e37-a069-83f97667862e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
app/components/Package/SizeIncrease.vueapp/components/Package/SizeIncreaseAnalysis.vueapp/composables/pkg-size/useAnalyzeCauseWorker.tsapp/utils/pkg-size/analyze-cause-client-worker.tsapp/utils/pkg-size/analyze-cause-worker.tsapp/utils/pkg-size/db.tsapp/utils/pkg-size/types.tsi18n/locales/en.jsoni18n/locales/es.jsoni18n/schema.jsonpackage.jsontest/nuxt/a11y.spec.tstest/unit/pkg-size-analyze-worker.spec.tstest/unit/pkg-size-analyze.spec.ts
💤 Files with no reviewable changes (3)
- i18n/locales/en.json
- i18n/locales/es.json
- i18n/schema.json
🚧 Files skipped from review as they are similar to previous changes (6)
- app/utils/pkg-size/analyze-cause-client-worker.ts
- package.json
- app/components/Package/SizeIncrease.vue
- app/utils/pkg-size/analyze-cause-worker.ts
- app/components/Package/SizeIncreaseAnalysis.vue
- app/utils/pkg-size/db.ts
…analyze-logic' into userquin/feat-add-size-increase-analyze-logic
|
PR should be ready now, added composable test |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/nuxt/composables/use-analyze-cause-worker.spec.ts (1)
112-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unsafe worker-message casts and unchecked call indexing.
The loose worker mock forces
as anyand![index]access. Define a typed worker test double/message-dispatch helper, and explicitly fail if the listener or posted request is absent before reading it. This keeps the tests strictly type-safe and makes setup failures actionable.Also applies to: 137-137, 152-164
🤖 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 `@test/nuxt/composables/use-analyze-cause-worker.spec.ts` around lines 112 - 120, Update the worker test setup around messageHandler, startAnalyzeCause, and the related cases at lines 137 and 152-164 to use a typed worker test double and message-dispatch helper instead of any casts or non-null assertions. Validate that the message listener and posted request exist, explicitly failing with actionable errors when either is absent, before accessing their values; preserve the existing test behavior.Source: Coding guidelines
🤖 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.
Inline comments:
In `@test/nuxt/composables/use-analyze-cause-worker.spec.ts`:
- Around line 7-25: Move mockWorker initialization into a vi.hoisted() block
before the vi.mock declarations, then reference the hoisted worker from the
analyze-cause-client-worker mock factory. Keep the existing addEventListener,
removeEventListener, and postMessage mock methods unchanged.
---
Nitpick comments:
In `@test/nuxt/composables/use-analyze-cause-worker.spec.ts`:
- Around line 112-120: Update the worker test setup around messageHandler,
startAnalyzeCause, and the related cases at lines 137 and 152-164 to use a typed
worker test double and message-dispatch helper instead of any casts or non-null
assertions. Validate that the message listener and posted request exist,
explicitly failing with actionable errors when either is absent, before
accessing their values; preserve the existing test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d041b9a-d0bd-49d6-90a0-aea207805a59
📒 Files selected for processing (2)
app/composables/pkg-size/useAnalyzeCauseWorker.tstest/nuxt/composables/use-analyze-cause-worker.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/composables/pkg-size/useAnalyzeCauseWorker.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/nuxt/composables/use-analyze-cause-worker.spec.ts`:
- Around line 47-53: Update getWorkerMessageHandler so its returned handler uses
the actual worker response event type, or at minimum an event parameter shaped
as { data: unknown }, instead of any. Preserve the existing message-handler
lookup and registration validation while keeping the test boundary strictly
type-safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 60937e55-807b-42fc-b79f-8639decc254a
📒 Files selected for processing (1)
test/nuxt/composables/use-analyze-cause-worker.spec.ts
3 participants
🔗 Linked issue
🧭 Context
After a conversation with Daniel and getting a brief update on what was happening at Nuxt, it seemed logical to add this here.
It is not easy to find an explanation without having the dependencies.
closes #3078
📚 Description
Analyze video with Nuxt package: https://streamable.com/9m0m68
Analyze Nuxt screenshots
This PR includes:
This can be re-used (indexedDB and the new module) to add a new page for pkg-size, the pkg-size.dev not always working.
The idea is adding also rxjs and use dexie liveQuery wirh some ping/pong logic with a new webworker, this way the new page will just show the packages while the new webworker adding the packages to the database.
TODO:
optional depsfilter, sorry Daniel for using that 'name' 🙏We'll need to add nuxt global announcer (+ bus?) to allow announce live data, this will go to another pr: we can iterate the changes included here later (selective filter and quota check)
To run the tests, from root:
vp test test/nuxt/composables/use-analyze-cause-worker.spec.ts --project nuxtvp test test/unit/pkg-size-analyze.spec.ts --project unitvp test test/unit/pkg-size-analyze-worker.spec.ts --project unitvp test test/nuxt/a11y.spec.ts --project nuxt