Skip to content

fix(webui): increase /issues page timeout from 15s to 30s#696

Open
nextlevelshit wants to merge 2 commits intomainfrom
689-issues-timeout
Open

fix(webui): increase /issues page timeout from 15s to 30s#696
nextlevelshit wants to merge 2 commits intomainfrom
689-issues-timeout

Conversation

@nextlevelshit
Copy link
Copy Markdown
Collaborator

Summary

  • The /issues page was timing out with context deadline exceeded on repos with 600+ issues
  • Increased the issues page timeout from 15s to 30s by adding a dedicated IssuesPageTimeout constant to the timeouts package
  • Updated handlers_issues.go to use the new 30s timeout instead of the generic 15s PageTimeout
  • Added test coverage verifying the timeout context is correctly applied

Related to #689

Changes

  • internal/timeouts/timeouts.go — added IssuesPageTimeout = 30 * time.Second constant
  • internal/webui/handlers_issues.go — switched from timeouts.PageTimeout to timeouts.IssuesPageTimeout
  • internal/webui/handlers_issues_test.go — new test validating timeout behavior
  • specs/689-issues-timeout/ — spec, plan, and task documentation

Test Plan

  • Added TestHandleIssuesTimeout to verify the handler uses the correct 30s timeout
  • Existing tests continue to pass with go test ./...

@nextlevelshit
Copy link
Copy Markdown
Collaborator Author

Code Review (Wave Pipeline)

Verdict: COMMENT

PR #696 correctly fixes the /issues page timeout by introducing a dedicated ForgeAPIList constant (30s) and wiring it into getIssueListData. The change is minimal, well-tested, and introduces no security vulnerabilities. However, the fix is incompletely applied: the new constant lacks manifest.Timeouts integration (breaking the project's timeout configurability invariant), and the identical timeout problem exists in ListPullRequests (handlers_prs.go:219) but is left unfixed. These are non-blocking but should be addressed in this PR or a tracked follow-up.


Major Findings

File Line Finding
internal/timeouts/timeouts.go 28 ForgeAPIList constant is hardcoded at 30s with no corresponding manifest.Timeouts field, getter method, YAML key, or test row -- breaking the package-level invariant that all timeout values are configurable via wave.yaml. Suggestion: Add ForgeAPIListSec field to manifest.Timeouts with a GetForgeAPIList() getter, update the struct comment, and add a test row in internal/manifest/timeouts_test.go. Alternatively, document explicitly if the intent is to derive it as 2x ForgeAPI.
internal/webui/handlers_prs.go 219 ListPullRequests uses the same 15s ForgeAPI timeout that caused the issue being fixed for ListIssues. Active repos with many PRs will hit the same context deadline exceeded failure. Suggestion: Apply timeouts.ForgeAPIList to the ListPullRequests call as well. Grep for all ForgeAPI usages paired with List* calls to catch any other affected sites.

Minor Findings

File Finding
internal/webui/handlers_issues_test.go deadlineCapturingForge embeds forge.Client directly -- any unexpected method call panics with nil dereference rather than a clear test failure. Consider embedding forge.UnsupportedClient (returns ErrNotSupported) instead.
internal/github/client.go:50 HTTP client timeout remains at 15s (ForgeAPI) while the context timeout for list operations is now 30s. Individual large responses on slow connections could be killed by the client before the context expires. No immediate action needed -- consider aligning in a follow-up if slow-connection issues are reported.

Suggestions

File Finding
internal/webui/handlers_prs_test.go No test coverage for ListPullRequests timeout. The deadlineCapturingForge pattern from the issues test could be reused. Add a parallel timeout verification test once the ListPullRequests timeout is updated.

Generated by Wave ops-pr-review pipeline

Add dedicated ForgeAPIList timeout constant (30s) for forge list operations
that need more headroom than single-item fetches (15s). The /issues page was
timing out on repos with 600+ issues because the shared context across retries
with exponential backoff exceeded the 15-second deadline.
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.

1 participant