Uh oh!
There was an error while loading. Please reload this page.
feat: npmx connector allow web auth - #1355
Conversation
…catch urls with possible auto opening
…odal and operations component
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
|
danielroe
commented
Feb 10, 2026
this is beautiful |
neutrino2211
commented
Feb 10, 2026
Will pull and test on MacOS in a bit. Thanks a ton! Auth issues have been a pain |
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:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntegrates a web-based auth fallback and an auto-open-URL setting across UI, composables, CLI and server. ConnectorModal and OperationsQueue expose a connector.autoOpenURL toggle and controls to open auth URLs; OperationsQueue adds web-auth polling, OTP input state and retry flows. useConnector and useSettings propagate autoOpenURL; /execute and executeOperation accept structured options { otp?, interactive?, openUrls? } and thread ExecNpmOptions through execution. CLI gains PTY-based interactive npm execution, URL extraction, onAuthUrl callbacks, new types/schemas, node-pty typings and bundler externals. i18n keys and tests updated. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
app/components/Org/OperationsQueue.vue (1)
68-78: Consider extracting a shared retry helper.
handleRetryWithOtpandhandleRetryWithWebAuthduplicate the OTP-failed retry loop; a small helper would keep the flow DRY.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mikouaji
commented
Feb 10, 2026
🤦 as always tests need fixing, looking into them |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/components/Org/OperationsQueue.vue (1)
68-78: Optional: dedupe retry logic.handleRetryWithWebAuthandhandleRetryWithOtpshare the same retry loop; a small helper would reduce drift.♻️ Possible refactor
+async function retryOtpFailedOps(): Promise<void> {+ const otpFailedOps = activeOperations.value.filter(+ (op: PendingOperation) => op.status === 'failed' && op.result?.requiresOtp,+ )+ for (const op of otpFailedOps) {+ await retryOperation(op.id)+ }+}+ async function handleRetryWithOtp() { if (!otpInput.value.trim()) return const otp = otpInput.value.trim() otpInput.value = '' - const otpFailedOps = activeOperations.value.filter(- (op: PendingOperation) => op.status === 'failed' && op.result?.requiresOtp,- )- for (const op of otpFailedOps) {- await retryOperation(op.id)- }+ await retryOtpFailedOps() await handleExecute(otp) } async function handleRetryWithWebAuth() { - const otpFailedOps = activeOperations.value.filter(- (op: PendingOperation) => op.status === 'failed' && op.result?.requiresOtp,- )- for (const op of otpFailedOps) {- await retryOperation(op.id)- }+ await retryOtpFailedOps() await handleExecute() }cli/src/npm-client.ts (1)
168-301: Consider splittingexecNpmInteractiveinto helpers.
This function is well structured but quite long; extracting pieces (spawn setup, onData handling, finalise result) would improve maintainability.As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/src/npm-client.ts (1)
217-258: Consider disposing event subscriptions to prevent potential memory leaks.The
onDataandonExitmethods returnIDisposableobjects that should be disposed when no longer needed. While the PTY process exits anyway, explicitly disposing these subscriptions is a best practice, especially if this code is later adapted for long-running scenarios.Proposed refactor
+ const dataDisposable = child.onData((data: string) => {- child.onData((data: string) => { output += data // ... rest of handler }) + const exitDisposable = child.onExit(({ exitCode }) => {- child.onExit(({ exitCode }) => { if (resolved) return resolved = true clearTimeout(timeout) if (authUrlTimeout) clearTimeout(authUrlTimeout) + dataDisposable.dispose()+ exitDisposable.dispose() // ... rest of handler })
Uh oh!
There was an error while loading. Please reload this page.
mikouaji
commented
Feb 10, 2026
@danielroe fixed all tests etc |
danielroe
commented
Feb 11, 2026
tested this and seemed to work well on MacOS - in fact I think it could be the default (maybe even non-configurable?) and only if it fails should we fall back to OTP... |
neutrino2211
commented
Feb 11, 2026
Just checked it out. The UX is very clean and the connector isn't giving issues straight away. Your org management UI is also way better than what I was planning so i think this addresses #34 as well |
neutrino2211
commented
Feb 11, 2026
Yeah, I'm onboard with this |
I don't think this PR touched the org management UI - I think that's back from some time ago. |
mikouaji
commented
Feb 11, 2026
I can make it work this way, we just need a clean way of handling it, currently there is a 90s timeout if the link is not clicked. Other than that only happy cases work pretty well, other failures still need to be covered.
Yeah, nothing was changed there, you must be seeing the UI from before you did anything. |
mikouaji
commented
Feb 11, 2026
What about the auto opening setting? I'd leave it so people can choose, some might not like tabs appearing suddenly. |
neutrino2211
commented
Feb 11, 2026
I could have sworn I saw UI for managing org teams and members but to be honest it was pretty late at night so I might have been too exhausted to understand what I was looking at. I'll take a look again with fresh eyes |
neutrino2211
commented
Feb 11, 2026
Yeah. I'm normally the type to hate tabs popping up without me initiating them but I'm so over npm auth that I just left it on |
danielroe
commented
Feb 11, 2026
we can have auto-open disabled by default ✅ (and we can iterate on this) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/mock-state.ts (1)
275-318:⚠️ Potential issue | 🟡 MinorInclude configured URLs in mocked results.
configuredResult.urlsis currently dropped, so the aggregatedurlsoutput will always miss URLs when per-operation results are injected for tests.Proposed fix
const result: OperationResult = { stdout: configuredResult.stdout ?? '', stderr: configuredResult.stderr ?? '', exitCode: configuredResult.exitCode ?? 1, requiresOtp: configuredResult.requiresOtp, authFailure: configuredResult.authFailure, + urls: configuredResult.urls, }
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Org/OperationsQueue.vue (1)
107-110:⚠️ Potential issue | 🟠 MajorInclude authFailure ops in the OTP retry queue.
hasOtpFailuresnow treatsauthFailureas needing OTP fallback, buthandleRetryWithOtponly retriesrequiresOtp, leavingauthFailureops failed even after OTP entry. Please align the retry filter with the failure detection.🔧 Suggested fix
- const otpFailedOps = activeOperations.value.filter(- (op: PendingOperation) => op.status === 'failed' && op.result?.requiresOtp,- )+ const otpFailedOps = activeOperations.value.filter(+ (op: PendingOperation) =>+ op.status === 'failed' && (op.result?.requiresOtp || op.result?.authFailure),+ )
🧹 Nitpick comments (1)
cli/src/npm-client.ts (1)
167-305: Consider splittingexecNpmInteractiveinto smaller helpers.
It’s doing prompt detection, timeouts, logging, and result shaping in one block, which makes it harder to test and maintain. As per coding guidelines: Keep functions focused and manageable (generally under 50 lines).
Uh oh!
There was an error while loading. Please reload this page.
mikouaji
commented
Feb 11, 2026
@danielroe Updated:
|
danielroe
commented
Feb 11, 2026
amazing! ❤️ |
This feature extends the npmx connector functionality for handling authorization with 2fa keys configured in npm account settings.
This works by allowing running the npm commands within an interactive shell which does not prompt for OTP but rather asks to verify auth on a web page.
I have added some settings to the connector modal:
Currently tested only on linux
Needs testing on Windows and Mac
Preview before connecting (in Advanced options):

Preview after connecting:

Some previews of it in action:
web_auth_auto_url.mp4
web_auth_no_auto_url.mp4
web_auth_otp_with_web_after.mp4
FYI @neutrino2211 I was supposed to tag you here ;)