feat: add deprecate command with custom reason#921
feat: add deprecate command with custom reason#921eryue0220 wants to merge 29 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
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
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 WalkthroughWalkthroughThis PR introduces package deprecation functionality allowing users to deprecate npm packages or specific versions with custom messages. It includes a new Vue 3 modal component for the deprecation workflow, CLI support via a new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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: 3
🧹 Nitpick comments (1)
app/pages/package/[...package].vue (1)
1156-1169: Remove duplicate CSS classes.The button has redundant
inline-flex items-center(conflicting withflex) andw-fullappearing twice in the class string.🧹 Proposed fix
<button type="button" - class="flex items-center justify-center w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors inline-flex items-center gap-1.5 w-full" + class="flex items-center justify-center gap-1.5 w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors" `@click`="deprecateModal?.open()" >
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/pages/package/[...package].vue (1)
1245-1256: Clean up duplicate CSS classes.The button has redundant classes: both
flexandinline-flex, andw-fullappears twice.♻️ Suggested fix
<button type="button" - class="flex items-center justify-center w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors inline-flex items-center gap-1.5 w-full" + class="flex items-center justify-center gap-1.5 w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors" `@click`="deprecateModal?.open()" >
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Package/DeprecatePackageModal.vue (2)
24-28: Use the importedtfunction instead of$tin the script block.The component destructures
tfromuseI18n()on line 12, but this computed uses$t. For consistency within the script setup, use the importedtfunction.♻️ Proposed fix
const modalTitle = computed(() => deprecateVersion.value - ? `${$t('package.deprecation.modal.title')} ${props.packageName}@${deprecateVersion.value}` - : `${$t('package.deprecation.modal.title')} ${props.packageName}`, + ? `${t('package.deprecation.modal.title')} ${props.packageName}@${deprecateVersion.value}` + : `${t('package.deprecation.modal.title')} ${props.packageName}`, )
121-127: Consider removing inline focus-visible utilities from buttons.Per project convention, focus-visible styling for buttons is applied globally via
main.csswithbutton:focus-visible { outline: 2px solid var(--accent); ... }. The inlinefocus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50utilities override this global rule.If this modal requires different focus styling, consider whether it should be consistent with the rest of the application or if an exception is warranted.
Based on learnings: "In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css... Do not apply per-element inline utility classes."
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Package/DeprecatePackageModal.vue (2)
69-69: Consider adding optional chaining for defensive access.If
state.value.operationsis ever undefined (e.g., during initialisation or after an error), this line would throw. Adding optional chaining aligns with the strict type-safety guideline.🛡️ Proposed fix
- const completedOp = state.value.operations.find(op => op.id === operation.id) + const completedOp = state.value.operations?.find(op => op.id === operation.id)
122-128: Remove inlinefocus-visibleutilities on buttons to rely on global styling.Both buttons in this component have custom
focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50classes. The project uses a global CSS rule for button focus-visible styling inmain.css. Removing these inline utilities ensures consistency across the codebase.This applies to the button on lines 164-175 as well.
♻️ Proposed fix
<button type="button" - class="w-full px-4 py-2 font-mono text-sm text-fg-muted bg-bg-subtle border border-border rounded-md transition-colors duration-200 hover:text-fg hover:border-border-hover focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50" + class="w-full px-4 py-2 font-mono text-sm text-fg-muted bg-bg-subtle border border-border rounded-md transition-colors duration-200 hover:text-fg hover:border-border-hover" `@click`="close" >And for the deprecate button:
<button type="button" :disabled="isDeprecating || !deprecateMessage.trim()" - class="w-full px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-colors duration-200 hover:bg-fg/90 disabled:opacity-50 disabled:cursor-not-allowed focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50" + class="w-full px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-colors duration-200 hover:bg-fg/90 disabled:opacity-50 disabled:cursor-not-allowed" `@click`="handleDeprecate" >Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…feat/resolve-40 * 'feat/resolve-40' of github.com:eryue0220/npmx.dev: Update app/components/Package/DeprecatePackageModal.vue
|
@eryue0220 Do you think these changes are still relevant and reviewable? |
Okay, and I've resolved the conflicts. |
MatteoGabriele
left a comment
MatteoGabriele
left a comment
There was a problem hiding this comment.
I've given a quick check of the UI, and besides the button changes I mentioned, it looks good!
That said, when I try to deprecate (I actually have a package I can deprecate), it doesn't work and returns a 400 Bad Request.
| <!-- Deprecation (when connected as package owner; hidden when current version is already deprecated) --> | ||
| <div | ||
| v-if="isConnected && resolvedVersion && isPackageOwner && !isCurrentVersionDeprecated" | ||
| class="space-y-1" | ||
| > | ||
| <button | ||
| type="button" | ||
| class="flex items-center justify-center gap-1.5 w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors" | ||
| @click="deprecateModal?.open()" | ||
| > | ||
| <span class="i-carbon-warning-alt w-4 h-4 shrink-0" aria-hidden="true" /> | ||
| {{ $t('package.deprecation.action') }} | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
I think the button needs to be visually aligned with the rest of the sidebar elements, and we should use ButtonBase with the Lucide icons as well.
I was playing around with it, so if you want to can just use this suggestion. Let me know what you think.
| <!-- Deprecation (when connected as package owner; hidden when current version is already deprecated) --> | |
| <div | |
| v-if="isConnected && resolvedVersion && isPackageOwner && !isCurrentVersionDeprecated" | |
| class="space-y-1" | |
| > | |
| <button | |
| type="button" | |
| class="flex items-center justify-center gap-1.5 w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors" | |
| @click="deprecateModal?.open()" | |
| > | |
| <span class="i-carbon-warning-alt w-4 h-4 shrink-0" aria-hidden="true" /> | |
| {{ $t('package.deprecation.action') }} | |
| </button> | |
| </div> | |
| <!-- Deprecation (when connected as package owner; hidden when current version is already deprecated) --> | |
| <div | |
| v-if="isConnected && resolvedVersion && isPackageOwner && !isCurrentVersionDeprecated" | |
| class="pl-7" | |
| > | |
| <ButtonBase | |
| type="button" | |
| variant="secondary" | |
| class="w-full text-red" | |
| @click="deprecateModal?.open()" | |
| classicon="i-lucide:triangle-alert text-red" | |
| > | |
| {{ $t('package.deprecation.action') }} | |
| </ButtonBase> | |
| </div> |
| v-if="pkg" | ||
| ref="deprecateModal" | ||
| :package-name="pkg.name" | ||
| :version="resolvedVersion ?? ''" |
There was a problem hiding this comment.
What happens if we don't get the version?
|
@43081j Since we talked about this on Bluesky, what do you think? UI-wise, it works for me. I also think the button's placement is visible enough without being too prominent. 
|
github-actions
bot
removed
the
stale
5 participants
resolve #40