Skip to content

refactor: extract shared HTTP timeouts#1022

Merged
LeaWhoCodes merged 1 commit intofloatpane:masterfrom
mvanhorn:refactor/989-shared-http-timeouts
Apr 26, 2026
Merged

refactor: extract shared HTTP timeouts#1022
LeaWhoCodes merged 1 commit intofloatpane:masterfrom
mvanhorn:refactor/989-shared-http-timeouts

Conversation

@mvanhorn
Copy link
Copy Markdown
Contributor

@mvanhorn mvanhorn commented Apr 26, 2026

What?

Centralizes the five HTTP-client timeouts that #989 called out. New internal/httpclient package owns named constants for each call site (PluginCallTimeout, RegistryFetchTimeout, RemoteImageTimeout, InstallTimeout, UpdateCheckTimeout) plus a thin New(timeout) / NewWithRedirectCap(timeout, max) factory pair. Each of the five sites listed in the issue now references the constant instead of inlining a magic duration.

Why?

Issue #989 lists plugin/http.go:13 (10s), plugins/embed.go:25/56 (10s × 2), view/html.go:300 (5s), cli/install.go:25 (30s), and main.go:67 (30s) as five different HTTP timeout literals across the codebase with no central knob. The issue offers two acceptable approaches: a configurable client factory, or "at minimum, extract timeout constants to a shared config." This PR takes the minimum-scope path so the call-site behavior is unchanged: same five timeouts, same intent (Lua plugin calls vs registry fetches vs inline image fetches vs CLI install vs update check), but no more grepping for time.Second to find them.

main.go's 5-redirect cap is the only non-trivial bit — it has a custom CheckRedirect that the other clients explicitly don't want. The NewWithRedirectCap helper preserves that policy without inlining the literal 5 or the redirect fmt.Errorf again.

Changes

  • NEW internal/httpclient/httpclient.go — five named timeout constants with site-attributing doc comments; New(timeout) and NewWithRedirectCap(timeout, maxRedirects) factories.
  • NEW internal/httpclient/httpclient_test.go — unit tests for the constants, the basic factory, the redirect-cap factory (both via direct CheckRedirect invocation and an end-to-end httptest.Server redirect-loop check).
  • plugin/http.go — drops the local httpTimeout const and the time import; switches the package-level httpClient to httpclient.New(httpclient.PluginCallTimeout).
  • plugins/embed.go — replaces both &http.Client{Timeout: 10 * time.Second} literals with httpclient.New(httpclient.RegistryFetchTimeout).
  • view/html.go — replaces the inline 5-second image client with httpclient.New(httpclient.RemoteImageTimeout).
  • cli/install.go — replaces the inline 30-second install client with httpclient.New(httpclient.InstallTimeout).
  • main.go — replaces the package-level redirect-capped client literal with httpclient.NewWithRedirectCap(httpclient.UpdateCheckTimeout, 5). The 5 and the "stopped after N redirects" message now live exactly once, in the helper.

Testing

  • go build ./... clean.
  • go vet ./... clean (the existing clib OFF_MAX macro warning is pre-existing and present on upstream/master).
  • go test ./internal/httpclient/... passes (3 tests including a live-server redirect-cap check).
  • go test ./plugin/... ./plugins/... ./view/... ./cli/... passes — existing package tests untouched.

Closes #989

This contribution was developed with AI assistance (Codex).

Centralize the five HTTP-client timeouts called out in floatpane#989 behind named
constants in a new internal/httpclient package. Replaces magic-number
literals at plugin/http.go:13, plugins/embed.go:25/56, view/html.go:300,
cli/install.go:25, and main.go:67 with httpclient.PluginCallTimeout /
RegistryFetchTimeout / RemoteImageTimeout / InstallTimeout /
UpdateCheckTimeout. main.go's 5-redirect cap is preserved via the new
NewWithRedirectCap helper.

No behavior change. Same timeouts, same redirect policy, same call-site
patterns; just no more magic numbers.

Closes floatpane#989
@mvanhorn mvanhorn requested a review from a team as a code owner April 26, 2026 08:03
Copy link
Copy Markdown
Member

@floatpanebot floatpanebot left a comment

Choose a reason for hiding this comment

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

Hi @mvanhorn! Please fix the following issues with your PR:

  • Title: Is too long (69 characters). The PR title must be strictly under 40 characters.
  • Body: Missing the ## What? or ## Why? headings required by the PR template.

@mvanhorn mvanhorn changed the title refactor: extract HTTP timeouts to shared internal/httpclient package refactor: extract shared HTTP timeouts Apr 26, 2026
@floatpanebot floatpanebot dismissed their stale review April 26, 2026 15:37

Formatting issues have been resolved. Thank you!

@mvanhorn
Copy link
Copy Markdown
Contributor Author

Fixed: title shortened to 38 chars, body now uses ## What? / ## Why? headings as required.

Copy link
Copy Markdown
Member

@andrinoff andrinoff left a comment

Choose a reason for hiding this comment

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

lgtm

@andrinoff
Copy link
Copy Markdown
Member

@LeaWhoCodes check this out.

Copy link
Copy Markdown
Member

@LeaWhoCodes LeaWhoCodes left a comment

Choose a reason for hiding this comment

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

lgtm

@LeaWhoCodes LeaWhoCodes merged commit db89465 into floatpane:master Apr 26, 2026
16 of 17 checks passed
@mvanhorn
Copy link
Copy Markdown
Contributor Author

Appreciate the merge @LeaWhoCodes. Centralizing the five HTTP timeouts behind named constants in internal/httpclient should make future tuning a lot less archaeology.

@mvanhorn
Copy link
Copy Markdown
Contributor Author

Thanks @LeaWhoCodes. Centralizing those five HTTP timeouts into one internal/httpclient package was overdue, and now they are tunable from one place instead of scattered literals.

@mvanhorn
Copy link
Copy Markdown
Contributor Author

Cheers @LeaWhoCodes, thanks for landing this. Centralizing the five HTTP timeouts in internal/httpclient with named constants per callsite should keep #989 from coming back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: Plugin HTTP timeout inconsistent with main app timeout

4 participants