Skip to content

chore: increase timeout for connection requests to 30 seconds#2684

Open
JounQin wants to merge 1 commit intonpmx-dev:mainfrom
JounQin:patch-1
Open

chore: increase timeout for connection requests to 30 seconds#2684
JounQin wants to merge 1 commit intonpmx-dev:mainfrom
JounQin:patch-1

Conversation

@JounQin
Copy link
Copy Markdown

@JounQin JounQin commented May 7, 2026

🔗 Linked issue

close #2683

🧭 Context

5s timeout for connection is tool short

📚 Description

Copilot AI review requested due to automatic review settings May 7, 2026 11:21
@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented May 7, 2026

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

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment May 7, 2026 11:23am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview May 7, 2026 11:23am
npmx-lunaria Ignored Ignored May 7, 2026 11:23am

Request Review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The pull request increases the $fetch timeout for the /connect POST request in useConnector from 5 seconds to 30 seconds. This adjustment prevents timeout errors when the local connector endpoint requires more than 5 seconds to respond.

Changes

Connector Timeout

Layer / File(s) Summary
Connect Timeout Configuration
app/composables/useConnector.ts
The $fetch timeout for the /connect POST call is increased from 5 seconds to 30 seconds to prevent operation abort due to timeout.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description references the linked issue (#2683) and explains the rationale for the change, though contains a typo ('tool' instead of 'too').
Linked Issues check ✅ Passed The PR directly addresses issue #2683 by implementing the requested timeout increase from 5s to 30s for the /connect endpoint as specified in the issue.
Out of Scope Changes check ✅ Passed All changes are scoped to the timeout modification in useConnector.ts; no extraneous modifications or unrelated code changes are present.
Title check ✅ Passed The pull request title accurately describes the main change—increasing the timeout for connection requests from 5 to 30 seconds in useConnector.ts.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

Hello! Thank you for opening your first PR to npmx, @JounQin! 🚀

Here’s what will happen next:

  1. Our GitHub bots will run to check your changes.
    If they spot any issues you will see some error messages on this PR.
    Don’t hesitate to ask any questions if you’re not sure what these mean!

  2. In a few minutes, you’ll be able to see a preview of your changes on Vercel

  3. One or more of our maintainers will take a look and may ask you to make changes.
    We try to be responsive, but don’t worry if this takes a few days.

@JounQin JounQin changed the title Increase timeout for connection requests to 30 seconds chore: increase timeout for connection requests to 30 seconds May 7, 2026
Copy link
Copy Markdown
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

🤖 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/useConnector.ts`:
- Line 118: The post-connect state refresh uses a shorter timeout than the
updated connect timeout causing a slow connector to succeed then immediately be
marked disconnected; update refreshState() to use the same 30000ms timeout (or a
shared constant) as the /connect request (the timeout value set where connect is
called) and apply the same error-handling pattern used by connect (catch, log
with context, and preserve expected state) so both connect and refreshState use
a consistent timeout and error 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

Run ID: 01675d77-e96e-4190-844d-04ded3ecddc9

📥 Commits

Reviewing files that changed from the base of the PR and between 001d748 and ed65f10.

📒 Files selected for processing (1)
  • app/composables/useConnector.ts

method: 'POST',
body: { token },
timeout: 5000,
timeout: 30000,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Align post-connect state refresh timeout with the new connect timeout

Line 118 increases /connect to 30s, but refreshState() still uses 5s (Line 183). A slow connector can now connect successfully and then immediately flip to connected = false via the refresh catch path, which undermines this fix.

Proposed fix
+  const CONNECTOR_TIMEOUT_MS = 30000
+
   async function connect(token: string, port: number = DEFAULT_PORT): Promise<boolean> {
@@
       const response = await $fetch<ConnectResponse>(`http://127.0.0.1:${port}/connect`, {
         method: 'POST',
         body: { token },
-        timeout: 30000,
+        timeout: CONNECTOR_TIMEOUT_MS,
       })
@@
   async function refreshState(): Promise<void> {
@@
       const response = await $fetch<StateResponse>(`${baseUrl.value}/state`, {
         headers: {
           Authorization: `Bearer ${config.value.token}`,
         },
-        timeout: 5000,
+        timeout: CONNECTOR_TIMEOUT_MS,
       })

As per coding guidelines "Use error handling patterns consistently".

🤖 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/useConnector.ts` at line 118, The post-connect state refresh
uses a shorter timeout than the updated connect timeout causing a slow connector
to succeed then immediately be marked disconnected; update refreshState() to use
the same 30000ms timeout (or a shared constant) as the /connect request (the
timeout value set where connect is called) and apply the same error-handling
pattern used by connect (catch, log with context, and preserve expected state)
so both connect and refreshState use a consistent timeout and error behavior.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adjusts the connector client’s /connect request timeout to reduce false “timeout” failures during local connector startup/handshake, addressing issue #2683.

Changes:

  • Increased /connect request timeout from 5s to 30s in the connector composable.

Comment on lines 115 to 119
const response = await $fetch<ConnectResponse>(`http://127.0.0.1:${port}/connect`, {
method: 'POST',
body: { token },
timeout: 5000,
timeout: 30000,
})
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

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.

/connect timeout too short

2 participants