feat(gastown+web): slingExistingPr + BeadPanel babysit badge (chunks 0+4)#3536
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| throw new Error(`Cannot babysit a PR that is already ${result.status}.`); | ||
| } | ||
|
|
||
| const headBranch = result.head_branch ?? ''; |
There was a problem hiding this comment.
WARNING: Empty-string fallback for head_branch/base_branch/head_sha silently produces a bead with blank fields
All three fields are optional in PRStatusResult (and in the underlying Zod schema), so they can legitimately be undefined if the GitHub/GitLab API omits them. Falling back to '' means submitExternalPrToReviewQueue is called with branch: '' and targetBranch: ''. The babysit loop (poll, conflict detection, auto-merge) will likely fail in confusing ways later rather than producing a clear error now.
Consider throwing early if any of these are missing instead:
const headBranch = result.head_branch;
const baseBranch = result.base_branch;
if (!headBranch || !baseBranch) {
throw new Error(`PR metadata incomplete: head_branch=${headBranch}, base_branch=${baseBranch}`);
}| }); | ||
| }); | ||
|
|
||
| describe('forcePushAllowed default', () => { |
There was a problem hiding this comment.
WARNING: These tests exercise pure JS constant expressions, not production code
Both tests in 'forcePushAllowed default' test the expression undefined ?? false (a constant) and true (another constant) — they don't import or invoke any production code. They will always pass regardless of what happens in slingExistingPr. If someone changes the default in Town.do.ts, these tests will not catch it.
Consider replacing with an integration-style test that actually calls submitExternalPrToReviewQueue with the mock SQL and asserts on the stored force_push_allowed metadata value, or remove the describe block entirely if the production-code default is already covered implicitly.
| import type { TownConfig } from '../../src/types'; | ||
|
|
||
| class MiniSql { | ||
| private data: Map<string, unknown[]> = new Map(); |
There was a problem hiding this comment.
SUGGESTION: MiniSql.data is an unused private field
The private data: Map<string, unknown[]> = new Map() field is never read or written by any method in the class. It should be removed to avoid dead code.
| private data: Map<string, unknown[]> = new Map(); | |
| class MiniSql { |
| @@ -0,0 +1,286 @@ | |||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | |||
There was a problem hiding this comment.
SUGGESTION: beforeEach and PRStatusResult are imported but never used
beforeEach is listed in the vitest import but never called in this file. PRStatusResult is imported from town-scm but never referenced. These will likely produce lint warnings and should be removed.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryAll three previously flagged WARNINGs are resolved: Resolved Since Previous ReviewThe following issues from the prior review were fixed in commit
Files Reviewed (4 files, incremental diff)
Fix these issues in Kilo Cloud Reviewed by claude-sonnet-4.6 · 1,216,625 tokens Review guidance: REVIEW.md from base branch |
| MessageCircle, | ||
| GitMergeIcon, | ||
| RefreshCw, | ||
| Zap, |
There was a problem hiding this comment.
SUGGESTION: Zap is imported but never used — dead import
None of the new EVENT_ICONS entries are mapped to Zap. This will trigger a lint error (no-unused-vars / @typescript-eslint/no-unused-vars).
| Zap, | |
| RefreshCw, |
| Head SHA | ||
| </dt> | ||
| <dd className="font-mono text-xs"> | ||
| {(bead.metadata.head_sha as string).slice(0, 12)}… |
There was a problem hiding this comment.
SUGGESTION: Redundant as string cast — TypeScript already knows this is string from the typeof … === 'string' guard on line 273
The enclosing condition typeof bead.metadata?.head_sha === 'string' narrows the type, so the cast can be removed per the project's as-avoidance rule.
| {(bead.metadata.head_sha as string).slice(0, 12)}… | |
| {bead.metadata.head_sha.slice(0, 12)}… |
| /> | ||
|
|
||
| {isBabysit && typeof bead.metadata?.head_sha === 'string' && ( | ||
| <MetaCell icon={Hash} label="Head SHA" value={bead.metadata.head_sha as string} mono /> |
There was a problem hiding this comment.
SUGGESTION: Redundant as string cast — the enclosing guard typeof bead.metadata?.head_sha === 'string' on line 477 already narrows the type
Per the project's coding standards, as casts should be avoided when flow-sensitive typing already provides the narrowing.
| <MetaCell icon={Hash} label="Head SHA" value={bead.metadata.head_sha as string} mono /> | |
| <MetaCell icon={Hash} label="Head SHA" value={bead.metadata.head_sha} mono /> |
jrf0110
changed the title
| @@ -0,0 +1,94 @@ | |||
| import { describe, it, expect } from '@jest/globals'; | |||
| import { eventDescription } from '@/components/gastown/ActivityFeed'; | |||
There was a problem hiding this comment.
WARNING: Importing a runtime value from a 'use client' component in a Node Jest test will likely fail at module load time.
ActivityFeed.tsx has top-level imports of motion/react and useGastownTRPC (a tRPC React hook). When Jest loads this module to resolve eventDescription, it must evaluate the entire file. The Jest config uses testEnvironment: 'node' and motion is not listed in transformIgnorePatterns exceptions, so motion/react will be treated as opaque ESM — causing either a SyntaxError (ESM import in a CJS context) or a ReferenceError (browser globals like document/window) at test runtime.
Consider extracting the pure eventDescription function into its own module (e.g. event-description.ts) with no React/animation dependencies, then import from there in both ActivityFeed.tsx and the test.
…emove unused imports and redundant casts
…ePushAllowed tests - Extract eventDescription from ActivityFeed.tsx to pure event-description.ts to avoid motion/react transitive import in Jest test - Re-export from ActivityFeed.tsx for backwards compatibility - Update babysit-ui.test.ts to import from event-description.ts - Replace trivial forcePushAllowed inline tests with tests exercising submitExternalPrToReviewQueue through MiniSql mock - Enhance MiniSql mock to capture INSERT statements and params
|
Superseded by #3539 which targets the re-staged convoy branch. |
1 participant
Summary
Chunk 0 — slingExistingPr Town DO method + review-queue refactor:
submitToReviewQueueto use privatecreateMergeRequestBeadCorehelper (public signature unchanged)submitExternalPrToReviewQueuefunction that creates merge_request bead + review_metadata withouttracksedge, withgt:babysitlabel and babysit metadataslingExistingPrmethod to Town.do.ts that validates repo match, checks PR state, fetches branches from SCM, and callssubmitExternalPrToReviewQueuecheckPRStatusreturn shape withhead_branch,base_branch,head_sha,titlefields (both GitHub and GitLab paths)parsePrUrlForRepoMatchutility for repo validationChunk 4 — BeadPanel babysit badge + metadata + event formatters:
gt:babysitlabel is presenthead_shaandforce_push_allowedmetadata cells in BeadPanel for babysat beadsbabysit_started,pr_feedback_detected,pr_conflict_detected,pr_auto_merge,pr_status_changedin ActivityFeedgt:babysitlabel badge, babysit metadata fields, and human-readable event labelseventDescriptionfrom ActivityFeed for testabilityVerification
pnpm --filter cloudflare-gastown typecheck && pnpm --filter cloudflare-gastown testgt:babysitlabel → badge renders, head_sha and force-push fields visible, activity timeline shows readable descriptionsservices/gastown/test/unit/sling-existing-pr.test.tsandapps/web/src/components/gastown/babysit-ui.test.tsVisual Changes
Reviewer Notes
metadata.source_bead_idwhich babysit beads don't have).eventDescriptionfunction was exported from ActivityFeed to enable testing.