Skip to content

fix: only show error in claim package modal if checkResult is null#1199

Merged
danielroe merged 22 commits intonpmx-dev:mainfrom
neutrino2211:fix/claim-pkg-error-state
Feb 8, 2026
Merged

fix: only show error in claim package modal if checkResult is null#1199
danielroe merged 22 commits intonpmx-dev:mainfrom
neutrino2211:fix/claim-pkg-error-state

Conversation

@neutrino2211
Copy link
Contributor

The claim package modal has a default error message that persists even after the package name is validated.

Screenshot 2026-02-07 at 17 39 51

This pull checks if the checkResult variable is null before presenting the mergedError message

@vercel
Copy link

vercel bot commented Feb 8, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 8, 2026 9:18am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 8, 2026 9:18am
npmx-lunaria Ignored Ignored Feb 8, 2026 9:18am

Request Review

@codecov
Copy link

codecov bot commented Feb 8, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
app/components/Package/ClaimPackageModal.vue 66.66% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 8, 2026

📝 Walkthrough

Walkthrough

The error handling logic in the ClaimPackageModal.vue component has been modified. The mergedError computed property now implements short-circuit evaluation: when checkResult is not null, it immediately returns null instead of proceeding to evaluate the publishError or checkError fallback chain. This restructuring changes the conditions under which error messages are displayed in the modal, altering the error presentation flow based on the presence of check results.

🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description clearly explains the issue: a persistent error message in the claim package modal after package validation. It references the problem with a screenshot and describes the fix.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Comment on lines 38 to +44
const mergedError = computed(() => {
return (
publishError.value ??
(checkError.value instanceof Error
? checkError.value.message
: $t('claim.modal.failed_to_check'))
)
return checkResult.value !== null
? null
: (publishError.value ??
(checkError.value instanceof Error
? checkError.value.message
: $t('claim.modal.failed_to_check')))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Publish errors are now hidden once a check result exists.

Line 39 short-circuits mergedError to null whenever checkResult.value is non-null, which means publishError will never be displayed after a failed claim. Users lose feedback on publish failures. Prioritise publishError first, then suppress only check errors once a result exists.

Proposed fix
 const mergedError = computed(() => {
-  return checkResult.value !== null
-    ? null
-    : (publishError.value ??
-        (checkError.value instanceof Error
-          ? checkError.value.message
-          : $t('claim.modal.failed_to_check')))
+  if (publishError.value) return publishError.value
+  if (checkResult.value !== null) return null
+  return checkError.value instanceof Error
+    ? checkError.value.message
+    : $t('claim.modal.failed_to_check')
 })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const mergedError = computed(() => {
return (
publishError.value ??
(checkError.value instanceof Error
? checkError.value.message
: $t('claim.modal.failed_to_check'))
)
return checkResult.value !== null
? null
: (publishError.value ??
(checkError.value instanceof Error
? checkError.value.message
: $t('claim.modal.failed_to_check')))
const mergedError = computed(() => {
if (publishError.value) return publishError.value
if (checkResult.value !== null) return null
return checkError.value instanceof Error
? checkError.value.message
: $t('claim.modal.failed_to_check')
})

@danielroe danielroe added this pull request to the merge queue Feb 8, 2026
Merged via the queue into npmx-dev:main with commit 3030bc0 Feb 8, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants