Skip to content

feat(wasteland): add listClaims tRPC procedure#3148

Open
jrf0110 wants to merge 2 commits into
convoy/wasteland-claims-page/93e6710c/headfrom
convoy/wasteland-claims-page/93e6710c/gt/toast/b1eaf3b6
Open

feat(wasteland): add listClaims tRPC procedure#3148
jrf0110 wants to merge 2 commits into
convoy/wasteland-claims-page/93e6710c/headfrom
convoy/wasteland-claims-page/93e6710c/gt/toast/b1eaf3b6

Conversation

@jrf0110
Copy link
Copy Markdown
Contributor

@jrf0110 jrf0110 commented May 9, 2026

Summary

Add wasteland.listClaims tRPC query procedure that returns all currently claimed wanted items in a wasteland, enriched with in-flight DoltHub PR metadata. This powers the Claims overview page UI.

  • New RpcClaimOutput schema in schemas.ts with item (WantedBoardRowOutput) + optional pending_pr (pull_id, pr_url, kind, from_branch, state, timestamps)
  • New listClaims procedure with wastelandId, optional rigHandle filter, and limit (1-200, default 100)
  • Owner-access gated, graceful degradation on missing credentials (returns { claims: [] })
  • Fetches claimed items via SQL + open DoltHub PRs, joins them by item ID
  • Extracted inferPrKind helper to claims.util.ts for PR kind classification (claim/done/unclaim/edit/unknown)
  • Rate limit: wasteland.listClaims at 30 req/min
  • Analytics event: claims.list
  • Updated router.d.ts type declarations for the web app
  • Unit tests for inferPrKind (9 test cases)

Verification

  • pnpm --filter cloudflare-wasteland test passes (21 tests)
  • pnpm --filter cloudflare-wasteland typecheck passes

Visual Changes

N/A (backend only)

Reviewer Notes

  • The inferPrKind helper uses keyword matching on PR titles — this is a heuristic mirroring how the inbox classifier works, but not as sophisticated. If the PR title doesn't contain known verbs, it falls back to 'unknown'.
  • Rate limiting is automatic via the tRPC middleware in init.ts — the path wasteland.listClaims maps to the config key.


const rigFilter = input.rigHandle ?? null;
const candidates = openPulls.filter(
p => !rigFilter || !p.creator_name || p.creator_name === rigFilter
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.

WARNING: creator_name filter is too permissive — fetches extra PRs for no reason

When rigFilter is set, the expression !rigFilter || !p.creator_name || p.creator_name === rigFilter passes through every PR whose creator_name is null or empty. Those PRs then trigger a full getPull detail fetch (up to the concurrency limit) even though they will almost certainly be discarded later during the branch-name check at line 1113.

If the goal is to fetch fewer PRs when filtering by rig, the null-creator guard should be removed — they aren't matches:

Suggested change
p => !rigFilter || !p.creator_name || p.creator_name === rigFilter
p => !rigFilter || p.creator_name === rigFilter

@kilo-code-bot
Copy link
Copy Markdown
Contributor

kilo-code-bot Bot commented May 9, 2026

Code Review Summary

Status: 3 Issues Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 2
SUGGESTION 1
Issue Details (click to expand)

WARNING

File Line Issue
services/wasteland/src/trpc/router.ts 1085 creator_name filter passes through null-creator PRs when rigFilter is set, causing unnecessary getPull fan-out
services/wasteland/src/trpc/router.test.ts 315 Module-level mockCtx mutated without reset — orgMemberships leaks into subsequent tests if order changes

SUGGESTION

File Line Issue
services/wasteland/src/trpc/router.test.ts 293 Non-null assertions (!) on pending_pr violate coding standards — narrow with a local variable instead
Other Observations (not in diff)

TRPCError unused import (router.test.ts:103): The import is flagged as a WARNING inline. The linter will catch this; either use it to tighten the FORBIDDEN assertion or remove it.

SQL injection concern (informational — mitigated): rigHandle and limit are interpolated directly into the SQL string passed to runUnsafeSql. Both satisfy the contract from the docstring: rigHandle is guarded by /^[a-zA-Z0-9_-]+$/ and limit is a bounded integer. No action required.

PR kind priority (informational): No test covers a title containing both unclaim and done. Given unclaim is listed first the expected result is unclaim, but this ordering invariant is untested.

Files Reviewed (7 files)
  • apps/web/src/lib/wasteland/types/router.d.ts — generated type declarations, no issues
  • services/wasteland/src/trpc/claims.util.ts — no issues
  • services/wasteland/src/trpc/router.test.ts — 3 issues (2 warnings, 1 suggestion)
  • services/wasteland/src/trpc/router.ts — 1 warning (carried from prior review)
  • services/wasteland/src/trpc/schemas.ts — no issues
  • services/wasteland/src/util/analytics.util.ts — no issues
  • services/wasteland/src/util/rate-limit.util.ts — no issues

Fix these issues in Kilo Cloud


Reviewed by claude-sonnet-4.6 · 918,531 tokens

classifyInbox: vi.fn(),
}));

import { TRPCError } from '@trpc/server';
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.

WARNING: TRPCError is imported but never used

This import will be flagged by the linter. The FORBIDDEN test asserts via .rejects.toThrow(/Only wasteland owners/) (a string match) rather than checking instanceof TRPCError or its code. Either use the import to make the assertion more precise, or remove it.

Suggested change
import { TRPCError } from '@trpc/server';

type: 'org',
orgId: 'org-1',
});
mockCtx.orgMemberships = [{ orgId: 'org-1', role: 'member' }];
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.

WARNING: Module-level mockCtx is mutated without being reset — potential test pollution

mockCtx is declared once at module scope and orgMemberships is overwritten here. vi.clearAllMocks() in beforeEach only resets mock call counts/return values; it does not restore plain object properties. If this test runs before any other test that relies on mockCtx.orgMemberships being empty, it will inherit the mutated value.

Reset it at the end of this test, or reset it in beforeEach:

beforeEach(() => {
  vi.clearAllMocks();
  mockCtx.orgMemberships = [];
  ...
});


expect(result.claims).toHaveLength(1);
expect(result.claims[0].pending_pr).not.toBeNull();
expect(result.claims[0].pending_pr!.pull_id).toBe('42');
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.

SUGGESTION: Non-null assertions (!) on pending_pr — prefer a narrowing check

The coding standards prohibit ! non-null assertions. Line 292 already asserts not.toBeNull(), but TypeScript still sees pending_pr as T | null. Use optional chaining with a fallback or narrow with a local variable to avoid the !:

const pr = result.claims[0].pending_pr;
if (!pr) throw new Error('expected pending_pr to be set');
expect(pr.pull_id).toBe('42');
expect(pr.kind).toBe('claim');
expect(pr.from_branch).toBe('wl/rig-alpha/w-item1');
expect(pr.state).toBe('Open');

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