PR #167

43 votes · 30 up · 13 down

View on GitHub
43
Total Votes
+30
Upvotes
-13
Downvotes
+30-13

Comments(4)

▸
bigintersmindComment#167REWRITE IT IN RUST: because we must

Code Review

Fun PR and great writeup! The GraphQL optimization concept is solid (75+ calls → 1-2 is a real win). However, there are several issues that would break the site if merged in the current state.

Critical Issues

1. Merge will break all themes — missing exports from github.ts The PR removes getOrganizedPRs(), OrganizedPRs type, and other exports. On main, src/lib/prData.ts imports these, and every theme-specific component (ascii/, newspaper/, web2/ PRList + HallOfChaos) depends on them. Merging = site-wide build failure.

2. PullRequest field mismatch — rank, hotScore, isTrending are undefined at runtime The Rust struct produces 8 fields but the TypeScript PullRequest interface expects 11. Missing fields cause:

  • pr.rank renders as #undefined. in every PRCard
  • b.hotScore - a.hotScore → NaN, breaking "Trending" sort entirely
  • [TRENDING] badge never appears

3. Server-to-client regression in PRList.tsx Changed from a server component (ISR-cached, shared across visitors) to 'use client' with useEffect. Every visitor now makes a separate API call instead of sharing a server-rendered cache.

4. Relative URL in server component — HallOfChaos will crash getMergedPRs() calls fetch('/api/github/merged') — a relative URL. HallOfChaos.tsx is an async server component where Node.js fetch can't resolve relative URLs. It will always render the error state.

5. N+1 queries negate GraphQL benefit For each PR in the GraphQL response, fetch_commit_status() makes a separate REST call (github.rs:113). With 100 PRs that's 101 sequential HTTP requests, negating the GraphQL batching.

6. Cache poisons itself with error responses prs/route.ts — If WASM returns an empty array due to a transient GitHub failure, it gets cached for 5 minutes. Every user sees empty data with X-Cache: HIT.

Important Issues

  • 3 of 5 PR categories dropped — newest, discussed, controversial are gone (only topByVotes and trending remain)
  • GraphQL reactions capped at 100 — no pagination for PRs with >100 reactions, silently wrong vote totals
  • Silent vote suppression — REST fallback defaults to 0 votes on any reactions fetch failure, no logging
  • fetch_commit_status returns Ok(false) on HTTP errors — "can't reach GitHub" is treated as "CI failed"
  • Inconsistent wasm-pack targets — wasm:build uses --target bundler, wasm:build:release uses --target web (incompatible outputs)
  • Committed .wasm binary — opaque artifact that can't be code-reviewed
  • No input validation on limit query param — NaN/negative/huge values passed directly to WASM

Suggestions

The core idea (GraphQL + Rust/WASM) has real merit. To make it mergeable:

  1. Restore backward-compatible exports from github.ts so prData.ts and all theme components still work
  2. Add missing fields (rank, hotScore, isTrending, upvotes, downvotes, comments) — either in Rust or as a post-processing step in the API route
  3. Keep PRList.tsx as a server component with ISR caching
  4. Use absolute URLs or direct API calls in server components
  5. Move commit status into the GraphQL query (statusCheckRollup) to eliminate N+1
  6. Validate responses before caching
▸
SaturateComment#167🦀 REWRITE IT IN RUST (for real this time)

But does it go brrr?

Yes!

▸
matthewmayerComment#167🦀 REWRITE IT IN RUST (for real this time)

But does it go brrr?

▸
openchaos-bot[bot]Comment#167🦀 REWRITE IT IN RUST (for real this time)

🤖 OpenChaos Bot

Summary: This PR introduces Rust/WASM for performance-critical parts of the OpenChaos application, specifically GitHub API fetching and vote sorting. It also adds an auto-merge workflow for the top-voted PR each week.

Files changed: 10 (.github/workflows/automerge.yml, .github/workflows/ci.yml, .gitignore, README.md, package.json)

Vibe: Okay, this is getting serious, someone wants to automate the chaos!

⚠️ Large PR - partial review

openchaos-bot

All Activity(50)

▸
bigintersmindComment#167REWRITE IT IN RUST: because we must

Code Review

Fun PR and great writeup! The GraphQL optimization concept is solid (75+ calls → 1-2 is a real win). However, there are several issues that would break the site if merged in the current state.

Critical Issues

1. Merge will break all themes — missing exports from github.ts The PR removes getOrganizedPRs(), OrganizedPRs type, and other exports. On main, src/lib/prData.ts imports these, and every theme-specific component (ascii/, newspaper/, web2/ PRList + HallOfChaos) depends on them. Merging = site-wide build failure.

2. PullRequest field mismatch — rank, hotScore, isTrending are undefined at runtime The Rust struct produces 8 fields but the TypeScript PullRequest interface expects 11. Missing fields cause:

  • pr.rank renders as #undefined. in every PRCard
  • b.hotScore - a.hotScore → NaN, breaking "Trending" sort entirely
  • [TRENDING] badge never appears

3. Server-to-client regression in PRList.tsx Changed from a server component (ISR-cached, shared across visitors) to 'use client' with useEffect. Every visitor now makes a separate API call instead of sharing a server-rendered cache.

4. Relative URL in server component — HallOfChaos will crash getMergedPRs() calls fetch('/api/github/merged') — a relative URL. HallOfChaos.tsx is an async server component where Node.js fetch can't resolve relative URLs. It will always render the error state.

5. N+1 queries negate GraphQL benefit For each PR in the GraphQL response, fetch_commit_status() makes a separate REST call (github.rs:113). With 100 PRs that's 101 sequential HTTP requests, negating the GraphQL batching.

6. Cache poisons itself with error responses prs/route.ts — If WASM returns an empty array due to a transient GitHub failure, it gets cached for 5 minutes. Every user sees empty data with X-Cache: HIT.

Important Issues

  • 3 of 5 PR categories dropped — newest, discussed, controversial are gone (only topByVotes and trending remain)
  • GraphQL reactions capped at 100 — no pagination for PRs with >100 reactions, silently wrong vote totals
  • Silent vote suppression — REST fallback defaults to 0 votes on any reactions fetch failure, no logging
  • fetch_commit_status returns Ok(false) on HTTP errors — "can't reach GitHub" is treated as "CI failed"
  • Inconsistent wasm-pack targets — wasm:build uses --target bundler, wasm:build:release uses --target web (incompatible outputs)
  • Committed .wasm binary — opaque artifact that can't be code-reviewed
  • No input validation on limit query param — NaN/negative/huge values passed directly to WASM

Suggestions

The core idea (GraphQL + Rust/WASM) has real merit. To make it mergeable:

  1. Restore backward-compatible exports from github.ts so prData.ts and all theme components still work
  2. Add missing fields (rank, hotScore, isTrending, upvotes, downvotes, comments) — either in Rust or as a post-processing step in the API route
  3. Keep PRList.tsx as a server component with ISR caching
  4. Use absolute URLs or direct API calls in server components
  5. Move commit status into the GraphQL query (statusCheckRollup) to eliminate N+1
  6. Validate responses before caching
▸
SaturateComment#167🦀 REWRITE IT IN RUST (for real this time)

But does it go brrr?

Yes!

▸
matthewmayerComment#167🦀 REWRITE IT IN RUST (for real this time)

But does it go brrr?

▸
openchaos-bot[bot]Comment#167🦀 REWRITE IT IN RUST (for real this time)

🤖 OpenChaos Bot

Summary: This PR introduces Rust/WASM for performance-critical parts of the OpenChaos application, specifically GitHub API fetching and vote sorting. It also adds an auto-merge workflow for the top-voted PR each week.

Files changed: 10 (.github/workflows/automerge.yml, .github/workflows/ci.yml, .gitignore, README.md, package.json)

Vibe: Okay, this is getting serious, someone wants to automate the chaos!

⚠️ Large PR - partial review

openchaos-bot