feat: add deprecation warning#35
Conversation
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesPackage analysis now checks deprecation status in batches, tracks it per result, adds deprecated-package notes to copied Markdown, and displays a warning icon beside deprecated packages in the results table. Deprecation support and display
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant startAnalysis
participant getPackageIsDeprecated
participant copyAsMarkdown
participant ResultsTable
startAnalysis->>getPackageIsDeprecated: check package deprecation status in batches
getPackageIsDeprecated-->>startAnalysis: return status
startAnalysis->>copyAsMarkdown: provide results with deprecated flags
copyAsMarkdown-->>copyAsMarkdown: add Notes column when needed
startAnalysis->>ResultsTable: render results
ResultsTable-->>ResultsTable: show warning icon for deprecated packages
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Preview deployment ✅ Deployment complete!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/App.tsx (1)
160-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider the N+1 API call pattern and silent error handling for deprecation checks.
Each item triggers a separate
getPackageIsDeprecatedHTTP request — up to 3000 for a full result set. While batching (chunks of 50) andcachedFetchmitigate repeat cost, the first analysis for a large package could be slow and risk registry rate-limiting. Additionally,.catch(() => false)on line 165 silently masks network and rate-limit errors as "not deprecated," which could produce incorrect results under load.Two things worth considering:
- The
DependentValueObjecttype added inapi.ts(line 159) includes adeprecatedfield — if the dependents view data already contains this, you could readr.value.deprecated?.deprecateddirectly duringgetSortedDependentsprocessing and avoid the N+1 calls entirely.- If separate fetches remain necessary, consider logging or surfacing a count of failed deprecation checks so users know results may be incomplete.
🤖 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/App.tsx` around lines 160 - 172, The deprecation analysis loop in App.tsx performs one request per item and silently treats failures as not deprecated. Reuse the existing deprecated value from DependentValueObject during getSortedDependents processing when available, removing unnecessary getPackageIsDeprecated calls; otherwise retain the fetches but track and surface failed checks instead of mapping errors silently to false.
🤖 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 `@src/components/App.tsx`:
- Around line 534-549: Update the deprecated icon SVG in the pkg.deprecated Show
block to include role="img" and aria-label="Deprecated" alongside its existing
title, matching the accessibility attributes used by the error icon.
---
Nitpick comments:
In `@src/components/App.tsx`:
- Around line 160-172: The deprecation analysis loop in App.tsx performs one
request per item and silently treats failures as not deprecated. Reuse the
existing deprecated value from DependentValueObject during getSortedDependents
processing when available, removing unnecessary getPackageIsDeprecated calls;
otherwise retain the fetches but track and surface failed checks instead of
mapping errors silently to false.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c681d427-3648-4397-9fb4-2f9e3024f310
📒 Files selected for processing (2)
src/components/App.tsxsrc/lib/api.ts
For now, I was deciding against this idea (because I was also thinking about it), because deprecated packages are very rare, which means we would have a column that is mostly empty. And I also wonder how often such a column would actually be filled with some info, even if we add more stuff like "has a replacement", "last updated...", etc. IMHO a column should only be added if every package has associated info with it, which isn't the case here. That's why I am a little hesitant. Especially, because tables with many rows quickly become inaccessible on smaller devices as well... |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
I think it depends on the package, packages that have a replacement are more likely to have deprecated or replaceable dependents too What I think we could do is keep the icon in the ui, but when "Copying as Markdown table", then, if a package has a note, add a Notes column, instead of copying an emoji
If no package in the table has any note, we could hide the column maybe? Don't know if that's a good idea though
Yeah, it's currently not very good on mobile, I think mobile needs some more improvements, maybe on mobile make it into cards or several rows instead of simple table |
|
I think the best approach is to have icons, similar to npmx next to dependencies, and when "copied as markdown", then add a Notes column |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/App.tsx (1)
105-112: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent race conditions from concurrent analysis executions.
The addition of the batched deprecation-check loop significantly increases the execution time of this function. Because there is no concurrency control, triggering the analysis multiple times (e.g., rapidly pressing Enter or toggling the devDependencies checkbox during an active load) will spawn concurrent executions. This can cause the progress bar to jump erratically and may lead to a race condition where a stale, slower request overwrites the final UI state.
Consider tracking the current request ID to abort stale executions.
🛠️ Proposed fix to ignore stale requests
Add a variable to track the active request at the top of your component (e.g., near line 38):
const [copyBtnLabel, setCopyBtnLabel] = createSignal("Copy as Markdown"); + let activeAnalysisId = 0;Then, update
startAnalysisto track and verify the ID before updating state:async function startAnalysis(replaceState = false) { const pkg = pkgInput().trim(); if (!pkg) return; + const currentAnalysisId = ++activeAnalysisId; setLoading(true); setError(""); setState({ status: "no-results" }); onProgress(10, "Initializing..."); try { // ... existing analysis and chunking logic ... + if (currentAnalysisId !== activeAnalysisId) return; setState( items.length > 0 ? { status: "results", items } : { status: "no-results" }, ); onProgress(100, "Analysis complete"); } catch (err) { + if (currentAnalysisId !== activeAnalysisId) return; console.error(err); setError((err as Error).message); } finally { + if (currentAnalysisId === activeAnalysisId) { setLoading(false); + } } }🤖 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/App.tsx` around lines 105 - 112, Prevent concurrent startAnalysis executions from updating shared UI state by adding an active analysis ID variable in the component and incrementing it when a run begins. Capture the run’s ID and guard all subsequent progress, loading, error, and result updates so stale executions exit without overwriting the latest analysis state.
🤖 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.
Nitpick comments:
In `@src/components/App.tsx`:
- Around line 105-112: Prevent concurrent startAnalysis executions from updating
shared UI state by adding an active analysis ID variable in the component and
incrementing it when a run begins. Capture the run’s ID and guard all subsequent
progress, loading, error, and result updates so stale executions exit without
overwriting the latest analysis state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2481cf0d-a337-4c2a-8d7d-1548b97e9e04
📒 Files selected for processing (1)
src/components/App.tsx
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 58 minutes. |
gameroman
left a comment
gameroman
left a comment
There was a problem hiding this comment.
Looks like our getPackageIsDeprecated is always returning 404 error for scoped packages
Seems to be an upstream bug? Even encoded URL is @TheDevMinerTV Could you maybe confirm this? |
gameroman
added
blocked
2 participants
Description
This PR adds a little warning symbol to deprecated packages near the name of the package in the list. To make it accessible, it also has a
<title>Deprecated</title>, which shows on hover and gets picked up by screen readers.Here is an example in the preview: https://pr-35-dependents-dev.gameroman.workers.dev/?package=qs
Summary by CodeRabbit