refactor: remove duplicated declarations and unreferenced code (-1,819 lines) - #60
Merged
Conversation
Deletes code that nothing in the repo references, verified by grep across src/, scripts/, config files and CI workflows: - src/lib/schemas-github.ts (234 lines) — zero importers; every production GraphQL call goes through gql(), which hardcodes Schema.Unknown - renderPatError + its 5-branch Match block in errors.ts — only its own test called it; token-setup.tsx uses its own buildPatError, which is the better implementation (scoped token URL, expired-vs-invalid distinction) - src/lib/effect-test-helpers.ts — its sole consumer destructured the layer and discarded the recorded calls array, so the whole recording apparatus was dead; inlined the two lines that test actually uses - SearchSelectPanel debug apparatus (debugName prop, log callback, summarizeDebugValue, 10 call sites) — console.log-based instrumentation - src/ui/step-indicator.tsx and src/features/field-helpers.ts — one export, one caller each; inlined into that caller - src/background/bulk-handlers.ts — 13 lines calling four register functions - TokenSetupCard mode/onOpenOptions props — its only call site passes no props, so every compact branch and the secondary button were unreachable - getFields threaded through the sprint UI — both terminal consumers already destructured it as _getFields - recentAssignees prop on BulkRandomAssignFlyout — never passed - primerCss.borderedContainer and card (no callers); footerBorder was a byte-identical alias of divider - duplicated fmt/daysLeft in sprint-progress-view — sprint-utils exports both - duplicated 30-line "Sprint settings" button in sprint-table-widget - clearMousedownPath, logger.info, BulkRandomAssignData — no callers - 13 package.json scripts nothing invokes (CI calls `pnpm wxt submit` and the coverage script directly, not via these aliases) - src/assets/images/old/ — 7 tracked binaries, 2.3 MB, referenced nowhere - update-coverage-badge.mjs dual-mode: only the json-summary path is used Also raises vitest testTimeout to 15s: bulk-transfer-modal's render test costs ~500ms of work but exceeded the 5s default under worker contention. typecheck clean, 413 tests pass, lint 0 errors (56 -> 52 warnings).
src/ui/icons.tsx contained zero hand-drawn SVGs: it imported 36 icons from @primer/octicons-react and re-exported each behind an identical 3-line wrapper, 35 times. Replaced with one `icon()` factory and one export per glyph. No call-site changes across the 28 consuming files; still static named exports, so tree-shaking is unaffected. 217 -> 93 lines. The `Octicon` adapter's runtime `typeof size === 'string'` branch was dead — every call site passes a numeric size — but the *type* must stay wide, because Primer's TextInput.Action `icon` prop requires a component accepting its own Size union. Kept the union, dropped the unreachable branch. Also replaced 16 byte-identical inline copies of the button motion block with the primerCss.buttonMotion() preset that already existed in primer-css-helper.ts and already had 10 callers. Left the 9 near-miss copies alone: makePreset shallow-merges, so an override supplying its own '&:hover:not(:disabled)' would replace the base rule and silently drop the transform. Those need a different fix, not this one. typecheck clean, 413 tests pass, lint 0 errors (52 -> 51 warnings).
src/lib/effect-assert.ts registered a `toEqualValue` matcher that recursively
wrapped values in Effect Data.* containers so Equal.equals would do a deep
comparison — something vitest's built-in toEqual already does.
Verified rather than assumed: swapped all 39 call sites to toEqual and ran
the suite. 413/413 still pass, so the custom matcher was buying nothing. It
was in fact strictly weaker for Errors, since wrap() collapsed them to
{ name, message }.
Deleting it empties vitest.setup.ts (its only job was registering the
matcher), so that file and the setupFiles entry go too. Suite setup time
drops from ~11.7s to 0.
Left the eight per-file render helpers alone: they look alike but differ in
substance — half wrap in ThemeProvider/BaseStyles and half render raw, and
the return shapes differ (container vs {container, root} vs added
find/findAll). That is similar-looking code, not duplication.
project-service.ts, cache-service.ts, services.ts and runtime-ext.ts existed
only so four call sites could write `yield* svc.foo()` instead of
`yield* Effect.promise(() => foo())`. Each wrapped an async helper that
already existed, and each had exactly one consumer.
Replaced with four direct calls:
field-handlers getProjectFieldsData, resolveProjectItemIdsWithTitles
hierarchy-handlers getOrCachePreview, getOrCacheHierarchy
and dropped provideBackground from those runHandler calls.
Also collapsed the remaining
Effect.tryPromise({ try: fn, catch: e => e as unknown }).pipe(Effect.orDie)
sites to Effect.promise(fn) — identical semantics, since both end with the
rejection as a fiber defect that runHandler's cause printer surfaces.
typecheck clean, 413 tests pass, lint 0 errors.
…lMap schemas-messages.ts declared the message contract a second time — 688 lines of Effect Schema whose only production consumer was a type-only import in messages.ts. Its docstring claimed background handlers validated payloads with Schema.decodeUnknownSync/encodeSync; a repo-wide grep found those calls in no handler. The validation it documented did not happen. Six payload types were declared twice (IssueRelationshipData, DuplicateItemPlan, ItemPreviewData, HierarchyData, SprintProgressData, IssueSearchResultData) and every consumer imported the hand-written twin from messages.ts, never the schema. Three type-level helpers (SchemaInput, SchemaOutput, DeepMutable) existed only to undo the readonly that Schema introduced. The replacement ProtocolMap is built from the interfaces messages.ts already declared. Equivalence was proven, not assumed: a temporary scaffold compared each of the 35 entries' input and output types mutually, using tuple-wrapped conditionals so unions don't distribute and so method-parameter bivariance can't hide a widened type. Negative-tested by widening bulkClose's `reason` to string, which failed the check by name. The scaffold is removed in this commit now that both sides match exactly. Deleting it orphans schemas-errors.ts and schemas-storage.ts (their only importer) and schema-snapshots.test.ts, which round-tripped schemas nothing decodes with — it exercised effect/Schema, not this repo. typecheck clean, 408 tests pass, lint 0 errors (51 -> 48 warnings).
Removed five packages with no source references, verified by grep across src/, scripts/ and every config file, then confirmed by a clean production build: - @effect/platform-browser never imported anywhere - @resvg/resvg-js no references (leftover from icon generation) - @types/marked marked has shipped its own types since v4 - @vitejs/plugin-react supplied by @wxt-dev/module-react - vite-node supplied by wxt Kept @primer/live-region-element: it looks unused in src/ but wxt.config.ts aliases it to src/lib/primer-live-region-stub.ts to avoid a customElements error in content scripts, so the package name has to resolve. Also removed the minimumReleaseAgeExclude block from pnpm-workspace.yaml — it pinned exclusions for a minimumReleaseAge policy configured nowhere. pnpm install / format / test / lint / typecheck all pass; wxt build green.
|
Analysis CompleteGenerated ECC bundle from 6 commits | Confidence: 70% View Pull Request #61Repository Profile
Changed Files (71)
Top hotspots
Top directories
Analysis Depth Readiness (evidence-backed, 43%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Likely Future Issues (4)
Suggested Follow-up Work (4)
Copy-ready bodies chore: sync config templates for vitest.config.ts ## Summary
- Update the example env files, sample configs, or deployment templates that should mirror the changed runtime configuration surface.
## Why
- Backfill example env files or config templates before a fresh setup drifts from the shipped runtime surface.
## Touched paths
- `vitest.config.ts`
## Validation
- Update the repo example env file or config template that should reflect the new runtime settings.
- Run the setup, boot, or deployment validation flow that depends on the changed config surface.test: add browser coverage for src/ui/icons.tsx + src/ui/modal-shell.tsx ## Summary
- Add browser or end-to-end coverage for the recently changed user-facing surface.
## Why
- Backfill browser coverage before another user-facing UI change lands on the touched surface.
## Touched paths
- `src/ui/icons.tsx`
- `src/ui/modal-shell.tsx`
## Validation
- Add or extend browser / e2e coverage for the changed component, page, or flow.
- Exercise the visible user journey that depends on the touched UI surface.test: add budget evidence for src/features/token-setup.tsx ## Summary
- Add budget or usage-limit validation for the recently changed AI routing or model-call surface.
## Why
- Backfill cost, token, or usage-limit validation before another model-routing change lands on the touched surface.
## Touched paths
- `src/features/token-setup.tsx`
## Validation
- Add or extend budget, token, usage-limit, or model-routing regression coverage for the changed path.
- Verify the route still enforces plan limits, retry caps, fallback behavior, or explicit cost controls.ci: add failure-mode evidence for .github/workflows/coverage.yml + vitest.config.ts ## Summary
- Add CI failure-mode evidence for the recently changed workflow or test-runner surface.
## Why
- Backfill CI failure-mode evidence before another workflow or test-runner change lands on the touched surface.
## Touched paths
- `.github/workflows/coverage.yml`
- `vitest.config.ts`
## Validation
- Add or update a CI failure fixture, captured failing log, troubleshooting note, workflow dry-run evidence, or regression test for the changed CI/test-runner behavior.
- Run the affected workflow or test-runner entrypoint locally or in CI and record pass/fail evidence.Detected Workflows (1)
Generated Instincts (12)
After merging, import with: Files
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 71 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/ui/icons.tsx">
<violation number="1" location="src/ui/icons.tsx:48">
P2: Consumers using the previously supported CSS-length or numeric-string `size` values can no longer type-check, and the runtime sizing fallback has been removed. Preserving the `number | string` prop and equivalent conversion/wrapper logic would maintain the existing icon sizing contract.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| export type IconProps = { | ||
| size?: number | string | ||
| size?: OcticonSize |
Contributor
There was a problem hiding this comment.
P2: Consumers using the previously supported CSS-length or numeric-string size values can no longer type-check, and the runtime sizing fallback has been removed. Preserving the number | string prop and equivalent conversion/wrapper logic would maintain the existing icon sizing contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ui/icons.tsx, line 48:
<comment>Consumers using the previously supported CSS-length or numeric-string `size` values can no longer type-check, and the runtime sizing fallback has been removed. Preserving the `number | string` prop and equivalent conversion/wrapper logic would maintain the existing icon sizing contract.</comment>
<file context>
@@ -30,188 +30,64 @@ import {
+
export type IconProps = {
- size?: number | string
+ size?: OcticonSize
color?: string
-}
</file context>
bulk-state.ts, bulk-position.ts, bulk-rename.ts and cache.ts had no test coverage, so the upcoming shared-runner extraction had nothing to prove parity against. Adds characterization tests asserting the exact broadcastQueue payload sequence each of the 12 bulk verbs emits today: label wording, per-item progress strings, processId prefix, failedItems passthrough, the Done! frame, the isBulkFull short-circuit and bulkClose's Undo reverse hint including its completed-slice and failed-id filtering. Also pins two easily-lost distinctions: the mandatory sleep(1000) between content-creating mutations, and its deliberate absence from reorder, which creates no content. For cache.ts, pins the four properties the implementation must keep regardless of mechanism: TTL expiry, in-flight dedupe, FIFO cap, and never caching a rejected fetch. Each suite was negative-tested by mutating the source (progress verbs, label wording, sleep interval, undo filter, cache TTL, failure invalidation) and confirming it goes red. Tests 408 -> 524. No production code changed.
Adds lib/format.ts with two helpers that replace hand-copied expressions:
plural() for the 43 sites spelling out `${n} item${n !== 1 ? 's' : ''}`,
and newProcessId() for the 16 spelling out the same Date.now()/Math.random
id. Two sites keep their inline form — a bare suffix with no count, and one
with a word interposed between count and noun — where the helper would read
worse.
format.ts is a zero-dependency leaf on purpose: newProcessId initially went
into queue.ts, which dragged the Effect queue machinery, the logger and
storage into toast-store and broke its test suite.
Removes 66 `export` keywords from symbols only ever used inside their own
file. That is what surfaced the rest of this commit: with the exports gone
the linter could finally see decodeProjectId, decodePat, decodeLogin, the
FieldMeta alias, and applyRule — the last self-described as "retained for
the dying modal" that no longer exists.
Also drops getAllNativeItemIds (no callers anywhere), resolveDChord (a
one-line ternary whose logic is already inlined in bulk-actions-bar), and
the unused runFork/runSync runtime wrappers. PanelCard and StatusBanner lose
the variants and props no call site passes.
Lint warnings 48 -> 47. Tests 524 -> 522 (the two resolveDChord cases).
Eight handlers — close, open, delete, lock, unlock, pin, unpin, transfer — were the same fifty-line run copied eight times: concurrency gate, Resolving frame, resolve items, one task per item, processQueue with a progress broadcast, Done! frame, release. Only the mutation and the words on screen differed. runBulkVerb now owns that shell. Each onMessage call site is kept rather than generated from a table, so every message payload stays exactly typed — handler parameters are bivariant, and a generated dispatcher would let a widened field through unnoticed. Close keeps its Undo hint via an optional buildReverse, which receives only the ids that were actually processed and did not fail. Transfer keeps its two-step preparation via an optional prepare, which is handed a setStatus so it can still narrate "Resolving target repository..." then "Resolving items...". Rename, random-assign and the two reorder handlers are deliberately left alone. None of them resolves one task per project item, and bending them through the runner would have meant five more optional callbacks to serve four call sites — trading duplication for a worse abstraction. bulk-state.ts 526 -> 142 lines. The 124 background characterization tests pass unchanged, including a new one asserting transfer resolves the target repository before it resolves items. One deliberate cosmetic change: the rejection and abort console lines now name the id prefix (close) rather than the message (bulkClose), matching the processId they sit next to.
…e table runBulkUpdate switched on dataType twice — once to build the progress detail, once to build the mutation — nine branches each, in two places, with nothing holding them together. Cognitive complexity was 617, four times worse than anything else in the repo, and adding a dataType meant remembering to touch both switches. Both now live as one row per dataType. Rows that only need a different progress line (SINGLE_SELECT, ITERATION) supply just that and fall through to the shared project-field mutation. The ~12 `update.value as any` casts collapse into one documented BulkFieldValue narrowing at the message boundary, which is where the contract genuinely types the payload as unknown. Lint warnings 47 -> 34. Honest accounting: this does not save lines. The file goes 319 -> 363, because the row structure and its types cost more than the two bare switches did. It buys the complexity drop, the type safety, and the guarantee that detail and mutation cannot drift. Adds 46 tests covering every dataType's detail string and mutation. They were run against the pre-refactor implementation as well and pass there unchanged, which is what proves this rewrite is behaviour-preserving rather than merely self-consistent.
bulk-duplicate-modal.tsx was the largest file in the repo at 1,259 lines, most of it renderValueSection opening ten branches with the same twenty-line Tippy-wrapped label, two near-identical pressable chips, and three scalar inputs differing only in their type attribute. SectionLabel and OptionChip now live in bulk-duplicate-section-ui.tsx. Assignees/labels, blocked-by/blocking and text/number/date each collapse to one branch parameterised over what actually differs. The LOADING skeleton repeated the same shimmer sx five times, and declared its @Keyframes on only the first block — the other four were silently relying on that sibling to define the animation they referenced. shimmerSx carries its own keyframes so each block stands alone. The hand-rolled backdrops in the duplicate and transfer modals were byte-identical to primerCss.modalOverlay()/modalPanel() and now use them. Not routed through ModalShell, despite the audit proposing it: the shell wraps children in its own padded scroll container, and both modals render their own headers and padded bodies, so it would have double-padded and nested two scroll areas. Sharing the presets gets the deduplication without the layout risk. bulk-duplicate-modal 1259 -> 886 lines, transfer 471 -> 453, plus a 148-line shared module. Lint warnings 47 -> 35. All 569 tests pass, including the duplicate and transfer modal suites.
… in the file
cache.ts held three caching strategies. Two were a plain Map keyed on
{ data, expiresAt }. The third needed four helper functions, a sibling Map of
eviction fibers so re-insertions could interrupt a stale timer, and a double
TTL — Effect.cachedWithTTL wrapped inside a scheduled eviction, both set to
one minute.
Its comment justified the fibers as TestClock-friendly. Nothing ever used
TestClock here; cache.ts had no test at all until this branch added one.
The hover caches now check expiry on read, which is why there are no timers
left to cancel. All four properties the old code went to such lengths for are
preserved and now asserted: TTL expiry, in-flight deduplication of concurrent
hovers, the 500-entry oldest-first cap, and never caching a rejected fetch.
182 -> 126 lines, and cache.ts no longer imports effect at all.
sprint-store.ts held two byte-identical get/set/subscribe stores differing
only in variable names; they share a local factory now.
Two audited items are deliberately not here. Migrating the remaining stores to
useSyncExternalStore was costed at ~130 lines on the assumption consumers
carried useState+useEffect+subscribe triads — they do not, they are already
one-liners, so the real saving is ~28 for churn through the selection hot
path. And the SPA-navigation helper assumed both injectors patched
history.pushState; only issue-detail does, leaving a ~10-line rAF debounce
that a shared module would not pay for.
Two places enumerated all ten overlay setters to dismiss them — the selection-cleared handler and the Escape shortcut. Adding an overlay meant remembering both lists, and missing one left a stuck overlay behind. closeAllOverlays() replaces both. The full ten-booleans-to-one-enum rewrite from the audit is not here. It would touch 61 setter and 32 reader references in an 884-line component with no direct test file, to save roughly 35 lines. The duplication that actually caused bugs was the two dismissal lists, and that is gone at a fraction of the exposure. Registering the bar chords into shortcutRegistry is also dropped: the audit claimed the help overlay does not list them, which is wrong — it carries a static barChordLegend covering all eleven. Nothing to fix.
|
Analysis CompleteGenerated ECC bundle from 13 commits | Confidence: 75% View Pull Request #62Repository Profile
Changed Files (126)
Top hotspots
Top directories
Analysis Depth Readiness (evidence-backed, 43%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Likely Future Issues (5)
Suggested Follow-up Work (5)
Copy-ready bodies chore: sync config templates for vitest.config.ts ## Summary
- Update the example env files, sample configs, or deployment templates that should mirror the changed runtime configuration surface.
## Why
- Backfill example env files or config templates before a fresh setup drifts from the shipped runtime surface.
## Touched paths
- `vitest.config.ts`
## Validation
- Update the repo example env file or config template that should reflect the new runtime settings.
- Run the setup, boot, or deployment validation flow that depends on the changed config surface.test: add async coverage for src/lib/queue.ts ## Summary
- Add async reliability coverage for the recently changed queue, worker, cron, or webhook surface.
## Why
- Backfill async reliability coverage before another queue, worker, or webhook change lands on the touched surface.
## Touched paths
- `src/lib/queue.ts`
## Validation
- Add or extend integration / e2e coverage for the changed queue, worker, cron, or webhook behavior.
- Exercise retries, idempotency, failure handling, or equivalent async boundary cases.test: add browser coverage for src/ui/bulk-flyout.tsx + src/ui/icons.tsx ## Summary
- Add browser or end-to-end coverage for the recently changed user-facing surface.
## Why
- Backfill browser coverage before another user-facing UI change lands on the touched surface.
## Touched paths
- `src/ui/bulk-flyout.tsx`
- `src/ui/icons.tsx`
## Validation
- Add or extend browser / e2e coverage for the changed component, page, or flow.
- Exercise the visible user journey that depends on the touched UI surface.test: add budget evidence for src/features/token-setup.tsx ## Summary
- Add budget or usage-limit validation for the recently changed AI routing or model-call surface.
## Why
- Backfill cost, token, or usage-limit validation before another model-routing change lands on the touched surface.
## Touched paths
- `src/features/token-setup.tsx`
## Validation
- Add or extend budget, token, usage-limit, or model-routing regression coverage for the changed path.
- Verify the route still enforces plan limits, retry caps, fallback behavior, or explicit cost controls.ci: add failure-mode evidence for .github/workflows/coverage.yml + vitest.config.ts ## Summary
- Add CI failure-mode evidence for the recently changed workflow or test-runner surface.
## Why
- Backfill CI failure-mode evidence before another workflow or test-runner change lands on the touched surface.
## Touched paths
- `.github/workflows/coverage.yml`
- `vitest.config.ts`
## Validation
- Add or update a CI failure fixture, captured failing log, troubleshooting note, workflow dry-run evidence, or regression test for the changed CI/test-runner behavior.
- Run the affected workflow or test-runner entrypoint locally or in CI and record pass/fail evidence.Review Activity (2 reviews, 1 inline comments, 1 unresolved threads)
Top unresolved thread files
Latest reviewer states
Review Follow-up Signals (2)
Recommended next actions
Detected Workflows (1)
Generated Instincts (12)
After merging, import with: Files
|
Contributor
There was a problem hiding this comment.
3 issues found across 64 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/background/cache.ts">
<violation number="1" location="src/background/cache.ts:35">
P2: When an expired key is refreshed while the cache is full, `Map.set` keeps its original FIFO position, so later hovers can evict this freshly fetched entry before older entries. Delete the expired key before the capacity loop so the replacement is appended and avoids unnecessary refetches.</violation>
</file>
<file name="src/background/__tests__/cache.test.ts">
<violation number="1" location="src/background/__tests__/cache.test.ts:4">
P3: The test file's header comment says cache.ts "currently implements the preview/hierarchy TTL with Effect fibers" that "can be swapped for the plain Map + timestamp pattern" — but this PR already performed that swap. The comment now describes the pre-change state and is factually wrong about the current code. Update it to describe the plain Map + timestamp implementation actually being characterized.</violation>
</file>
<file name="src/background/__tests__/bulk-state.test.ts">
<violation number="1" location="src/background/__tests__/bulk-state.test.ts:78">
P3: The new characterization tests duplicate the same vi.hoisted mock scaffold, `runVerb` driver, and `frames()` helper across all four added files (bulk-state, bulk-rename, bulk-position, bulk-update-fields). Extract the shared mock-setup + runVerb/frames helpers into a common test util (e.g. `__tests__/helpers/bulk-test-utils.ts`) parameterized by the per-module mocks, so the four files only declare the mocks and cases that differ. This trims ~60 lines of near-identical scaffolding and keeps future bulk-verb tests consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| fetchFn: () => Promise<T>, | ||
| ): Promise<T> { | ||
| const existing = cache.get(key) | ||
| if (existing && existing.expiresAt > Date.now()) return existing.value |
Contributor
There was a problem hiding this comment.
P2: When an expired key is refreshed while the cache is full, Map.set keeps its original FIFO position, so later hovers can evict this freshly fetched entry before older entries. Delete the expired key before the capacity loop so the replacement is appended and avoids unnecessary refetches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/background/cache.ts, line 35:
<comment>When an expired key is refreshed while the cache is full, `Map.set` keeps its original FIFO position, so later hovers can evict this freshly fetched entry before older entries. Delete the expired key before the capacity loop so the replacement is appended and avoids unnecessary refetches.</comment>
<file context>
@@ -1,130 +1,71 @@
+ fetchFn: () => Promise<T>,
+): Promise<T> {
+ const existing = cache.get(key)
+ if (existing && existing.expiresAt > Date.now()) return existing.value
-function capSetupMap<V>(
</file context>
Suggested change
| if (existing && existing.expiresAt > Date.now()) return existing.value | |
| if (existing) { | |
| if (existing.expiresAt > Date.now()) return existing.value | |
| cache.delete(key) | |
| } |
| // Characterization tests for the hovercard preview/hierarchy caches and the | ||
| // resolved-item handoff cache. | ||
| // | ||
| // cache.ts currently implements the preview/hierarchy TTL with Effect fibers, |
Contributor
There was a problem hiding this comment.
P3: The test file's header comment says cache.ts "currently implements the preview/hierarchy TTL with Effect fibers" that "can be swapped for the plain Map + timestamp pattern" — but this PR already performed that swap. The comment now describes the pre-change state and is factually wrong about the current code. Update it to describe the plain Map + timestamp implementation actually being characterized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/background/__tests__/cache.test.ts, line 4:
<comment>The test file's header comment says cache.ts "currently implements the preview/hierarchy TTL with Effect fibers" that "can be swapped for the plain Map + timestamp pattern" — but this PR already performed that swap. The comment now describes the pre-change state and is factually wrong about the current code. Update it to describe the plain Map + timestamp implementation actually being characterized.</comment>
<file context>
@@ -0,0 +1,184 @@
+// Characterization tests for the hovercard preview/hierarchy caches and the
+// resolved-item handoff cache.
+//
+// cache.ts currently implements the preview/hierarchy TTL with Effect fibers,
+// justified in a code comment as "TestClock-friendly" — a justification no test
+// ever cashed in. These pin the four properties that actually matter, so the
</file context>
| @@ -0,0 +1,393 @@ | |||
| // Characterization tests for the 7 bulk state-change verbs. | |||
Contributor
There was a problem hiding this comment.
P3: The new characterization tests duplicate the same vi.hoisted mock scaffold, runVerb driver, and frames() helper across all four added files (bulk-state, bulk-rename, bulk-position, bulk-update-fields). Extract the shared mock-setup + runVerb/frames helpers into a common test util (e.g. __tests__/helpers/bulk-test-utils.ts) parameterized by the per-module mocks, so the four files only declare the mocks and cases that differ. This trims ~60 lines of near-identical scaffolding and keeps future bulk-verb tests consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/background/__tests__/bulk-state.test.ts, line 78:
<comment>The new characterization tests duplicate the same vi.hoisted mock scaffold, `runVerb` driver, and `frames()` helper across all four added files (bulk-state, bulk-rename, bulk-position, bulk-update-fields). Extract the shared mock-setup + runVerb/frames helpers into a common test util (e.g. `__tests__/helpers/bulk-test-utils.ts`) parameterized by the per-module mocks, so the four files only declare the mocks and cases that differ. This trims ~60 lines of near-identical scaffolding and keeps future bulk-verb tests consistent.</comment>
<file context>
@@ -0,0 +1,393 @@
+}
+
+/** Drive a verb end-to-end, feeding `processQueue` the supplied progress states. */
+async function runVerb(
+ type: string,
+ data: Record<string, unknown>,
</file context>
The eleven-field frame the background broadcasts to the queue tracker was written out three times: as broadcastQueue's parameter, as the queueStateUpdate entry in the ProtocolMap, and — for its first seven fields — as QueueState in lib/queue. Sender and protocol entry are the two ends of the same message, so a field added to one and not the other type-checked fine and silently dropped on the wire. QueueFrame now extends QueueState and the ProtocolMap entry is that type. ReverseHint moves to messages.ts alongside it, since it is part of the payload, and rest-helpers re-exports it so its two consumers are untouched. QueueState is imported as `import type`, so messages.ts gains no runtime dependency on lib/queue — and therefore none on the logger or WXT storage, which its suite does not mock. rest-helpers 154 -> 133; messages 488 -> 492.
field-handlers still hand-rolled `/^issue:(\d+)$/ || /^issue-(\d+)$/` twice, in getReorderContext's selection map and its DOM-order re-sort — the third and fourth copies of a regex 99a09a4 already extracted as parseIssueDatabaseId and routed bulk-position through. primerCss.chipButton re-declared buttonMotion verbatim and added two lines. Both now compose from one BUTTON_MOTION object rather than one preset overriding the other: makePreset shallow-merges, so an override supplying its own '&:hover:not(:disabled)' would replace the base rule and drop the transform — the near-miss recorded on this branch back in 891ad10.
The reorder flyout's "Recent targets" and "All items" labels were the SectionHeader from bulk-edit-field-row, copied out twice with its nine sx lines intact. The second copy adds a borderTop, which is now an sx override on the shared component rather than a reason to fork it. bulk-reorder-flyout 397 -> 371.
Five copies of the same loop — findDialog, findFooter and readInput in the create-issue injector, findPanel and findSidebar in the issue-detail one — each walking a selector list and returning the first hit. Three of them also carried their own try/catch with their own comment explaining it. queryFirst in project-table-dom is that loop once, and keeps the per-selector try/catch with the reason stated properly: the lists contain :has() selectors that an older engine rejects, and joining them with commas would let one bad selector take the whole list down. create-issue-injections 433 -> 409; issue-detail-injections 208 -> 188.
The injector assembled createRoot + StyleSheetManager + isPropValid + ShadowThemeProvider by hand, and tore it down by id. createLightDomUi in shadow-ui-factory is that exact sequence, already used by the checkbox portal host, and it adds the ErrorBoundary this mount never had. The card's 'margin-bottom: 16px' inline cssText — which the design system bans — becomes mb: 3 on a Primer Box. The double-mount guard moves from a hand-assigned element id to the data attribute createLightDomUi sets. The injector now takes ctx, like the create-issue one next to it, which already calls createFeatureUi per dialog open, so the per-mount lifecycle is the pattern the codebase had already settled on. +6 lines, and styled-components and @emotion/is-prop-valid drop from two importers to one.
Audited as a candidate for deletion — 231 lines reimplementing show/hide delay scheduling around a library that ships a `delay` prop — and it earns every one of them. @tippyjs/react 4.2.6 (last released 2021, peer react: ">=16.8") clones the trigger child and calls preserveRef(children.ref, node) at dist/tippy-react.esm.js:360 and :517. React 19 removed element.ref in favour of element.props.ref, so that path silently drops the consumer's ref. The wrapper never hands children to BaseTippy at all: it clones the trigger itself, reads the ref from children.props.ref, and drives Tippy in controlled visible mode — which is why the delay scheduling lives here. Comment only, no behaviour change.
ba598f2 removed thirteen npm scripts as "unreferenced code". Nothing in a repo ever references an npm script by name — humans and CI invoke them — so that test deletes the release workflow and reports it as dead code. main still carries all of them. Restored: build:chrome/firefox/edge/safari (all four verified building), zip and zip:safari, and publish:chrome/firefox/edge/safari/all. The zip removal was also internally inconsistent: zip:chrome/firefox/edge survived and .github/workflows/release.yml calls them. Left deleted: `submit`, which shells out to a `webstore` binary that is not a dependency, so it could not have run. test:coverage-badge stays out too — .github/workflows/coverage.yml invokes scripts/update-coverage-badge.mjs directly rather than through the alias. pnpm test && pnpm install && pnpm typecheck && pnpm build:chrome && pnpm build:firefox && pnpm build:edge now completes; safari builds as well.
"[runHandler:getHierarchyData] failed … Item issue-58 not found in project — it may belong to a different project" fired on essentially every issue pane opened without clicking a table row first. Root cause: an item reaches the background under one of two spellings, and the separator is what says which number it carries. issue:4140984079 colon, from data-hovercard-subject-tag -> databaseId issue-58 hyphen, scraped from an /issues/58 href -> issue NUMBER Both name the same issue — verified on the live board, where #58 is databaseId 4140984079. But parseIssueDatabaseId collapsed both with /^issue[:-](\d+)$/ and collectMatchingItems matched the result against content.databaseId. An issue number never equals a databaseId, so resolveProjectItemIds returned [] and fetchHierarchyData threw. The hyphen spelling was unavoidable on that path: extractItemIdFromPanel only gets the good id from a row carrying data-rgp-active, which is set solely by clicking an injected row. A page load with ?pane=issue in the URL, back/forward, or a click landing before injection settles all fall through to the href-scraping fallback. parseIssueRef now preserves which number it parsed, and the shared walker matches databaseId ids against content.databaseId and number ids against content.number. The resolution query already selected number; the rename query now does too, so both callers of the walker are fixed. parseIssueDatabaseId keeps its tolerant behaviour on purpose. Its only remaining callers are bulk-position and getReorderContext, which match against content.databaseId and would need `number` added to GET_PROJECT_ITEMS_FOR_REORDER to be correct. That is pre-existing and rare — table rows almost always carry the colon spelling — and changing it here would have meant rewriting a characterization test to fit the fix. Verified live: the deep link that reproduced the error now renders the card with real data and logs nothing.
Contributor
There was a problem hiding this comment.
13 issues found across 41 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/lib/schemas-decode.ts">
<violation number="1" location="src/lib/schemas-decode.ts:27">
P2: These casts remove the decoder’s runtime `string` check, so malformed API data or an untyped caller can flow as a branded string and fail later during ID operations. Keep a shared `typeof raw === 'string'` check in the string decoders instead of casting directly.</violation>
</file>
<file name="src/lib/errors.test.ts">
<violation number="1" location="src/lib/errors.test.ts:29">
P3: The first test now asserts `expect(a).toEqual(b)` twice in a row — the newly added line duplicates the unchanged line directly beneath it. Given this PR is a deduplication refactor, drop one of the two identical assertions.</violation>
</file>
<file name="src/lib/graphql-client.ts">
<violation number="1" location="src/lib/graphql-client.ts:111">
P2: When `X-RateLimit-Remaining` is present but nonnumeric, this code converts it to zero and retries a 403 permission response three times. Preserve invalid values as missing so only an actual zero remaining count produces `GithubRateLimitError`.
(Based on your team's feedback about distinguishing 403 permission errors from rate limits.)</violation>
</file>
<file name="src/lib/queue.test.ts">
<violation number="1" location="src/lib/queue.test.ts:251">
P2: The test claims to verify that cancelling while the queue is parked in the 60s wait abandons the run, but cancellation is delivered synchronously inside the paused-state broadcast (notify()), before sleep() is entered. The abort signal is already aborted when sleep() starts, so sleep resolves through its `signal.aborted` early-return, and the abort-listener path that actually interrupts an in-flight wait is never exercised. A regression that removes sleep()'s abort listener would still pass this test. Defer the cancellation (e.g. setTimeout(() => cancelQueue(processId), 10)) so the queue is genuinely parked in the 60s wait when the abort lands.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:20">
P3: Each `publish:*` script runs the build twice: `wxt zip --browser X` already runs a production build before creating the zip, so the leading `wxt build --browser X &&` before it is redundant and doubles the release build time and dist output churn. Drop the explicit `wxt build` and keep only `wxt zip --browser X` for each `publish:*` script.</violation>
</file>
<file name="src/background/__tests__/relationship-tasks.test.ts">
<violation number="1" location="src/background/__tests__/relationship-tasks.test.ts:124">
P3: This characterization test's stated purpose is to pin 'the exact request sequence each branch emits' so the collapse of the blocked-by/blocking branches is provably equivalent, but the `remove()` mutation path inside the collapsed `run` loop is never exercised — only `add`, `clear`, and skip cases are covered. Since remove differs between the two kinds through `endpointFor` (blocked_by removes by the other issue's database id on the item's repo; blocking removes the item's id from the other issue's repo), add a case for each kind, e.g. a spec with `remove: [issue(5, 505)]` and a stubbed listing that returns that dependency, asserting the DELETE path and id.</violation>
</file>
<file name="src/features/issue-detail-injections.tsx">
<violation number="1" location="src/features/issue-detail-injections.tsx:101">
P2: Each panel swap creates a new `createLightDomUi`, which registers a `ctx.onInvalidated` callback. `unmountCard()` only calls `destroy()`, so callbacks from every previously opened panel accumulate until invalidation; reuse one UI or make the registration unregisterable.</violation>
</file>
<file name="src/lib/queue.ts">
<violation number="1" location="src/lib/queue.ts:96">
P2: When `onStateChange` throws synchronously after a task succeeds, this broad `try` records the callback failure as the task failure and can falsify progress. Catch only `task.run()` so notification and bookkeeping defects propagate instead of marking the mutation failed.</violation>
<violation number="2" location="src/lib/queue.ts:113">
P2: When `cancelQueue` runs while a task is awaiting `task.run()`, the new controller cannot interrupt that promise because tasks receive no signal. `processQueue` remains blocked until the GitHub request resolves, unlike the target implementation; retain an interruptible wrapper or propagate an abort signal into task requests.</violation>
</file>
<file name="src/background/project-helpers.ts">
<violation number="1" location="src/background/project-helpers.ts:170">
P1: When a project contains the same issue number in multiple repositories, this match can resolve the wrong project item because `issue-58` carries no repository identity. Preserve the repository from the DOM link or reject ambiguous number matches before bulk operations target the first result.</violation>
</file>
<file name="src/features/__tests__/bulk-actions-bar.test.tsx">
<violation number="1" location="src/features/__tests__/bulk-actions-bar.test.tsx:470">
P3: The test name promises escape and select-all remain, but the body asserts `['escape']` only — select-all is unregistered once a flyout opens. Rename the test to match the assertion.</violation>
<violation number="2" location="src/features/__tests__/bulk-actions-bar.test.tsx:479">
P3: The test name says "registers nothing", but the assertion expects `['escape', 'select-all']` — two shortcuts are registered. Rename the test to describe what it actually verifies so maintainers don't misread the bar's behavior with an empty selection.</violation>
</file>
<file name="src/features/bulk-edit-field-row.tsx">
<violation number="1" location="src/features/bulk-edit-field-row.tsx:11">
P2: The new `sx` prop is typed as `Record<string, unknown>`, which discards Primer's system-prop type safety: callers can pass invalid keys or values (e.g. `fontWeight: 123`) with no compile error, and editor autocomplete for prop keys is lost. The codebase already uses Primer's `BetterSystemStyleObject` for exactly this (pass-through sx overrides) in `src/lib/primer-css-helper.ts` and `src/ui/modal-shell.tsx`. Type the prop as `BetterSystemStyleObject` to match convention and keep callers type-checked.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| ) | ||
| remainingDbIds.delete(content.databaseId) | ||
| } | ||
| if (content.number !== undefined && remainingNumbers.has(content.number)) { |
Contributor
There was a problem hiding this comment.
P1: When a project contains the same issue number in multiple repositories, this match can resolve the wrong project item because issue-58 carries no repository identity. Preserve the repository from the DOM link or reject ambiguous number matches before bulk operations target the first result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/background/project-helpers.ts, line 170:
<comment>When a project contains the same issue number in multiple repositories, this match can resolve the wrong project item because `issue-58` carries no repository identity. Preserve the repository from the DOM link or reject ambiguous number matches before bulk operations target the first result.</comment>
<file context>
@@ -117,19 +157,32 @@ async function collectMatchingItems<TContent extends { databaseId: number }, TOu
+ )
+ remainingDbIds.delete(content.databaseId)
+ }
+ if (content.number !== undefined && remainingNumbers.has(content.number)) {
+ results.push(collect({ id: item.id, content }, wanted.byNumber.get(content.number)!))
+ remainingNumbers.delete(content.number)
</file context>
| export const decodeRepoOwner: (raw: string) => RepoOwner = Schema.decodeSync(RepoOwner) | ||
| export const decodeRepoName: (raw: string) => RepoName = Schema.decodeSync(RepoName) | ||
|
|
||
| export const decodeProjectItemId = (raw: string): ProjectItemId => raw as ProjectItemId |
Contributor
There was a problem hiding this comment.
P2: These casts remove the decoder’s runtime string check, so malformed API data or an untyped caller can flow as a branded string and fail later during ID operations. Keep a shared typeof raw === 'string' check in the string decoders instead of casting directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/schemas-decode.ts, line 27:
<comment>These casts remove the decoder’s runtime `string` check, so malformed API data or an untyped caller can flow as a branded string and fail later during ID operations. Keep a shared `typeof raw === 'string'` check in the string decoders instead of casting directly.</comment>
<file context>
@@ -16,23 +14,33 @@ import {
-export const decodeRepoOwner: (raw: string) => RepoOwner = Schema.decodeSync(RepoOwner)
-export const decodeRepoName: (raw: string) => RepoName = Schema.decodeSync(RepoName)
+
+export const decodeProjectItemId = (raw: string): ProjectItemId => raw as ProjectItemId
+
+export const decodeProjectItemDomId = (raw: string): ProjectItemDomId => raw as ProjectItemDomId
</file context>
| const retryAfter = numericHeader(res.headers.get('retry-after')) | ||
| const rateLimitRemainingHeader = res.headers.get('x-ratelimit-remaining') | ||
| const rateLimitRemaining = | ||
| rateLimitRemainingHeader === null ? null : numericHeader(rateLimitRemainingHeader) |
Contributor
There was a problem hiding this comment.
P2: When X-RateLimit-Remaining is present but nonnumeric, this code converts it to zero and retries a 403 permission response three times. Preserve invalid values as missing so only an actual zero remaining count produces GithubRateLimitError.
(Based on your team's feedback about distinguishing 403 permission errors from rate limits.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/graphql-client.ts, line 111:
<comment>When `X-RateLimit-Remaining` is present but nonnumeric, this code converts it to zero and retries a 403 permission response three times. Preserve invalid values as missing so only an actual zero remaining count produces `GithubRateLimitError`.
(Based on your team's feedback about distinguishing 403 permission errors from rate limits.) </comment>
<file context>
@@ -1,32 +1,171 @@
+ const retryAfter = numericHeader(res.headers.get('retry-after'))
+ const rateLimitRemainingHeader = res.headers.get('x-ratelimit-remaining')
+ const rateLimitRemaining =
+ rateLimitRemainingHeader === null ? null : numericHeader(rateLimitRemainingHeader)
+
+ logger.error('HTTP error', { op, status: res.status, retryAfter })
</file context>
Suggested change
| rateLimitRemainingHeader === null ? null : numericHeader(rateLimitRemainingHeader) | |
| rateLimitRemainingHeader !== null && /^\d+$/.test(rateLimitRemainingHeader) | |
| ? Number(rateLimitRemainingHeader) | |
| : null |
| (s) => { | ||
| if (s.paused && !cancelled) { | ||
| cancelled = true | ||
| cancelQueue(processId) |
Contributor
There was a problem hiding this comment.
P2: The test claims to verify that cancelling while the queue is parked in the 60s wait abandons the run, but cancellation is delivered synchronously inside the paused-state broadcast (notify()), before sleep() is entered. The abort signal is already aborted when sleep() starts, so sleep resolves through its signal.aborted early-return, and the abort-listener path that actually interrupts an in-flight wait is never exercised. A regression that removes sleep()'s abort listener would still pass this test. Defer the cancellation (e.g. setTimeout(() => cancelQueue(processId), 10)) so the queue is genuinely parked in the 60s wait when the abort lands.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/queue.test.ts, line 251:
<comment>The test claims to verify that cancelling while the queue is parked in the 60s wait abandons the run, but cancellation is delivered synchronously inside the paused-state broadcast (notify()), before sleep() is entered. The abort signal is already aborted when sleep() starts, so sleep resolves through its `signal.aborted` early-return, and the abort-listener path that actually interrupts an in-flight wait is never exercised. A regression that removes sleep()'s abort listener would still pass this test. Defer the cancellation (e.g. setTimeout(() => cancelQueue(processId), 10)) so the queue is genuinely parked in the 60s wait when the abort lands.</comment>
<file context>
@@ -213,6 +213,57 @@ describe('processQueue', () => {
+ (s) => {
+ if (s.paused && !cancelled) {
+ cancelled = true
+ cancelQueue(processId)
+ }
+ },
</file context>
| // light DOM, not shadow: the card sits inside GitHub's own sidebar and | ||
| // inherits its layout. createLightDomUi brings the StyleSheetManager, | ||
| // ThemeProvider and ErrorBoundary this used to assemble by hand. | ||
| currentUi = createLightDomUi(ctx, { |
Contributor
There was a problem hiding this comment.
P2: Each panel swap creates a new createLightDomUi, which registers a ctx.onInvalidated callback. unmountCard() only calls destroy(), so callbacks from every previously opened panel accumulate until invalidation; reuse one UI or make the registration unregisterable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/issue-detail-injections.tsx, line 101:
<comment>Each panel swap creates a new `createLightDomUi`, which registers a `ctx.onInvalidated` callback. `unmountCard()` only calls `destroy()`, so callbacks from every previously opened panel accumulate until invalidation; reuse one UI or make the registration unregisterable.</comment>
<file context>
@@ -108,34 +93,35 @@ function mountCard(panel: Element, projectContext: ProjectContext): void {
+ // light DOM, not shadow: the card sits inside GitHub's own sidebar and
+ // inherits its layout. createLightDomUi brings the StyleSheetManager,
+ // ThemeProvider and ErrorBoundary this used to assemble by hand.
+ currentUi = createLightDomUi(ctx, {
+ name: UI_NAME,
+ anchor: sidebar,
</file context>
|
|
||
| expect(Equal.equals(a, b)).toBe(true) | ||
| expect(a).toEqualValue(b) | ||
| expect(a).toEqual(b) |
Contributor
There was a problem hiding this comment.
P3: The first test now asserts expect(a).toEqual(b) twice in a row — the newly added line duplicates the unchanged line directly beneath it. Given this PR is a deduplication refactor, drop one of the two identical assertions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/lib/errors.test.ts, line 29:
<comment>The first test now asserts `expect(a).toEqual(b)` twice in a row — the newly added line duplicates the unchanged line directly beneath it. Given this PR is a deduplication refactor, drop one of the two identical assertions.</comment>
<file context>
@@ -23,31 +22,29 @@ describe('GithubRateLimitError (canonical 429 variant)', () => {
const b = new GithubRateLimitError({ status: 429, message: 'Too Many', retryAfter: 30 })
- expect(Equal.equals(a, b)).toBe(true)
+ expect(a).toEqual(b)
expect(a).toEqual(b)
})
</file context>
Comment on lines
20
to
22
| "publish:chrome": "wxt build --browser chrome && wxt zip --browser chrome", | ||
| "publish:firefox": "wxt build --browser firefox && wxt zip --browser firefox", | ||
| "publish:edge": "wxt build --browser edge && wxt zip --browser edge", |
Contributor
There was a problem hiding this comment.
P3: Each publish:* script runs the build twice: wxt zip --browser X already runs a production build before creating the zip, so the leading wxt build --browser X && before it is redundant and doubles the release build time and dist output churn. Drop the explicit wxt build and keep only wxt zip --browser X for each publish:* script.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At package.json, line 20:
<comment>Each `publish:*` script runs the build twice: `wxt zip --browser X` already runs a production build before creating the zip, so the leading `wxt build --browser X &&` before it is redundant and doubles the release build time and dist output churn. Drop the explicit `wxt build` and keep only `wxt zip --browser X` for each `publish:*` script.</comment>
<file context>
@@ -6,11 +6,22 @@
"zip:firefox": "wxt zip --browser firefox",
"zip:edge": "wxt zip --browser edge",
+ "zip:safari": "wxt zip --browser safari",
+ "publish:chrome": "wxt build --browser chrome && wxt zip --browser chrome",
+ "publish:firefox": "wxt build --browser firefox && wxt zip --browser firefox",
+ "publish:edge": "wxt build --browser edge && wxt zip --browser edge",
</file context>
| ]) | ||
| }) | ||
|
|
||
| it('clears by deleting every currently listed dependency', async () => { |
Contributor
There was a problem hiding this comment.
P3: This characterization test's stated purpose is to pin 'the exact request sequence each branch emits' so the collapse of the blocked-by/blocking branches is provably equivalent, but the remove() mutation path inside the collapsed run loop is never exercised — only add, clear, and skip cases are covered. Since remove differs between the two kinds through endpointFor (blocked_by removes by the other issue's database id on the item's repo; blocking removes the item's id from the other issue's repo), add a case for each kind, e.g. a spec with remove: [issue(5, 505)] and a stubbed listing that returns that dependency, asserting the DELETE path and id.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/background/__tests__/relationship-tasks.test.ts, line 124:
<comment>This characterization test's stated purpose is to pin 'the exact request sequence each branch emits' so the collapse of the blocked-by/blocking branches is provably equivalent, but the `remove()` mutation path inside the collapsed `run` loop is never exercised — only `add`, `clear`, and skip cases are covered. Since remove differs between the two kinds through `endpointFor` (blocked_by removes by the other issue's database id on the item's repo; blocking removes the item's id from the other issue's repo), add a case for each kind, e.g. a spec with `remove: [issue(5, 505)]` and a stubbed listing that returns that dependency, asserting the DELETE path and id.</comment>
<file context>
@@ -0,0 +1,200 @@
+ ])
+ })
+
+ it('clears by deleting every currently listed dependency', async () => {
+ hoisted.rest.mockResolvedValueOnce([
+ {
</file context>
| ]) | ||
| }) | ||
|
|
||
| it('keeps only escape and select-all while an overlay is open', async () => { |
Contributor
There was a problem hiding this comment.
P3: The test name promises escape and select-all remain, but the body asserts ['escape'] only — select-all is unregistered once a flyout opens. Rename the test to match the assertion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/__tests__/bulk-actions-bar.test.tsx, line 470:
<comment>The test name promises escape and select-all remain, but the body asserts `['escape']` only — select-all is unregistered once a flyout opens. Rename the test to match the assertion.</comment>
<file context>
@@ -294,3 +304,182 @@ describe('BulkActionsBar — overlay state', () => {
+ ])
+ })
+
+ it('keeps only escape and select-all while an overlay is open', async () => {
+ const m = renderBar()
+ await click(el(m, 'rgp-bar-mark-chip'))
</file context>
| expect(registeredTable().map((s) => s.id)).toEqual(['escape']) | ||
| }) | ||
|
|
||
| it('registers nothing while the selection is empty', () => { |
Contributor
There was a problem hiding this comment.
P3: The test name says "registers nothing", but the assertion expects ['escape', 'select-all'] — two shortcuts are registered. Rename the test to describe what it actually verifies so maintainers don't misread the bar's behavior with an empty selection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/__tests__/bulk-actions-bar.test.tsx, line 479:
<comment>The test name says "registers nothing", but the assertion expects `['escape', 'select-all']` — two shortcuts are registered. Rename the test to describe what it actually verifies so maintainers don't misread the bar's behavior with an empty selection.</comment>
<file context>
@@ -294,3 +304,182 @@ describe('BulkActionsBar — overlay state', () => {
+ expect(registeredTable().map((s) => s.id)).toEqual(['escape'])
+ })
+
+ it('registers nothing while the selection is empty', () => {
+ hoisted.selection.clear()
+ renderBar()
</file context>
ed43dff fixed the shared resolver but left bulk-position and getReorderContext matching every id against content.databaseId, so a hyphen-spelled id there resolved to nothing — or, worse, to an unrelated item that happened to have that databaseId. The test added below proves the wrong-item case: before this change, selecting `issue-20` built a move task for the item whose contentDbId is 20. GET_PROJECT_ITEMS_FOR_REORDER now selects `number`, and both call sites use a shared createIssueRefIndex: items go in indexed by databaseId AND by issue number, lookups go through parseIssueRef, and the spelling picks the map. This also removes duplication rather than adding any. bulk-position kept two parallel maps (contentDbToMemex, contentDbToNode) plus a third lookup for the insertion target; getReorderContext kept selectedDbIdMap and contentDbIdToEntry and assembled selectedItems inside the pagination loop. All five collapse into one index per call site, and selectedItems becomes a flatMap over the caller's own ids — which also fixes its ordering, since it previously came out in project-page order rather than selection order. parseIssueDatabaseId is gone: it had no callers left, and its whole purpose was to answer a question the caller could not ask correctly. The characterization test that asserted "accepts both issue:N and issue-N spellings" is replaced by four that pin the real contract. Its fixture used contentDbId 10/20/30, small enough for an issue number to collide with a databaseId; the new fixture gives each item a databaseId and a number that differ, as they do in production. Verified live: moved a row to the top and back via Custom position. Both moves landed, the board order is unchanged, console clean.
`Pat` was referenced nowhere. `IssueRef` and `parseIssueRef` are used only inside project-helpers.ts, so they no longer need to be exported.
concurrency.ts carried a live TODO to adopt `Effect.Semaphore`, a dependency that was deliberately deleted. cache.ts narrated its old fiber-based eviction, which git already records. The three test comments explained timer behaviour via `Effect.sleep`; the real mechanism is a plain setTimeout, and the client's retry awaits its own `sleep`.
AGENTS.md told agents `pnpm typecheck` was the only automated validation. There are 48 vitest files and 632 cases; CI just doesn't gate them.
nearestUpcoming and nextAfter were the same filter-sort-first with different predicates, and four sites built the same UTC-midnight Date by hand.
The same six-field parent mapping was hand-copied at three sites across two handler modules.
The panel and the group-header widget each declared the same six-state union, the same four useState calls, the same getSprintStatus fetch with the same four-branch derivation, and a byte-identical acknowledge handler. They render side by side, so two copies of that machine could disagree about which sprint is active.
The popup and the options card each rendered the same danger Flash (title, message, action link, dismiss) and the same nine-line input sx with the same error-colour conditional. The popup keeps its tighter type scale via `dense`. Left the FormControl/TextInput itself duplicated: sharing it needed three props for two call sites and measured net -1 line.
CardContent asked hasProjectBlock, which built the meta, then ProjectBlock built the same meta again. Both field lookups already require the value in their predicate, so a truthiness sweep over the meta is the same test the nine-term disjunction was making.
The elapsed/remaining/await block was written identically in the fetch's then and catch arms.
iterationEndDate is start + duration, i.e. exclusive, which is what isActive, nextAfter and daysLeft need. The four render sites printed that value directly, so every range read one day later than GitHub's own — "Aug 19 – Sep 2" against GitHub's "Aug 19 - Sep 01". fmtRange steps back a day for display only; the exclusive semantics and the message payload are untouched. It also collapses the four hand-copied `fmt(start) – fmt(end)` renders into one call.
SprintState was exported with no consumer; prefixLabelIcon was imported into the duplicate modal but only used in its four sibling modules; three test files imported hooks they never called. bulk-delete-modal's `itemTitles ?? []` allocated a fresh array every render, so the useMemo keyed on it never hit. Lint: 35 -> 30 warnings, still 0 errors.
@primer/react declares @primer/live-region-element as a regular dependency and resolves its own 0.7.2, so our direct 0.8.0 was a second copy nothing consumed. Every import of the specifier is aliased to src/lib/primer-live-region-stub.ts in wxt.config.ts — globally, including Primer's own internal imports — so neither version was ever bundled. The stub and vitest's deps.inline entry stay: the stub is what the alias points at, and the transitive 0.7.2 still resolves under tests. Verified: content.js has zero `customElements` references before and after, and background.js is byte-identical.
Eleven sites hand-copied the same tracker frame — total, completed 0, paused false, a status string, and the run's processId/label/tabId. One of them, bulk-position, had already generalised it into a local `broadcastStart` that simply never moved to a shared home. broadcastStatus lives in queue-run.ts beside broadcastDone, not in rest-helpers, because the bulk suites mock rest-helpers wholesale — a helper added there would be undefined in those tests. All 639 cases pass with every test file untouched. sprint-handlers and duplicate-handlers gained a QueueRun object on the way through; both were already rebuilding that shape inline for runQueueWithProgress and broadcastDone.
The duplicate modal's Parent section and the blocked-by/blocking list carried byte-identical copies of the same card — eleven sx props, a bold title, a muted reference and an invisible remove button. The Parent section simply never routed through the module that already existed for this shape. Worth about ten lines; the point is that the parent card and the relationship rows can no longer drift apart.
fetchItemPreviewData and fetchHierarchyData each carried their own copy of the id-resolution guard and a byte-identical blocked_by/blocking fetch. The id paths are what the two hyphen-spelling fixes on this branch had to correct, so routing both through resolveItemNodeId is the point rather than the thirteen lines. Concurrency is unchanged: loadBlockingPair starts both reads in the same tick, so the hierarchy lookup still has all three requests in flight together. Error strings are preserved exactly, including the two that already matched between the copies.
packageManager is pnpm@11.5.0, not 10.32.1. The suite is at 639 cases. Z_MODAL's comment named Primer's Dialog, which nothing in this codebase uses — modals compose primerCss.modalOverlay/modalPanel instead. The design-system checklists carrying the same stale Dialog claim live in untracked agent-instruction files and were corrected there too.
|
Analysis CompleteGenerated ECC bundle from 62 commits | Confidence: 90% View Pull Request #64Repository Profile
Changed Files (158)
Top hotspots
Top directories
Analysis Depth Readiness (evidence-backed, 43%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Likely Future Issues (5)
Suggested Follow-up Work (5)
Copy-ready bodies chore: sync config templates for vitest.config.ts ## Summary
- Update the example env files, sample configs, or deployment templates that should mirror the changed runtime configuration surface.
## Why
- Backfill example env files or config templates before a fresh setup drifts from the shipped runtime surface.
## Touched paths
- `vitest.config.ts`
## Validation
- Update the repo example env file or config template that should reflect the new runtime settings.
- Run the setup, boot, or deployment validation flow that depends on the changed config surface.test: add async coverage for src/lib/queue.ts ## Summary
- Add async reliability coverage for the recently changed queue, worker, cron, or webhook surface.
## Why
- Backfill async reliability coverage before another queue, worker, or webhook change lands on the touched surface.
## Touched paths
- `src/lib/queue.ts`
## Validation
- Add or extend integration / e2e coverage for the changed queue, worker, cron, or webhook behavior.
- Exercise retries, idempotency, failure handling, or equivalent async boundary cases.test: add browser coverage for src/ui/bulk-flyout.tsx + src/ui/icons.tsx ## Summary
- Add browser or end-to-end coverage for the recently changed user-facing surface.
## Why
- Backfill browser coverage before another user-facing UI change lands on the touched surface.
## Touched paths
- `src/ui/bulk-flyout.tsx`
- `src/ui/icons.tsx`
## Validation
- Add or extend browser / e2e coverage for the changed component, page, or flow.
- Exercise the visible user journey that depends on the touched UI surface.test: add budget evidence for src/features/token-setup.tsx ## Summary
- Add budget or usage-limit validation for the recently changed AI routing or model-call surface.
## Why
- Backfill cost, token, or usage-limit validation before another model-routing change lands on the touched surface.
## Touched paths
- `src/features/token-setup.tsx`
## Validation
- Add or extend budget, token, usage-limit, or model-routing regression coverage for the changed path.
- Verify the route still enforces plan limits, retry caps, fallback behavior, or explicit cost controls.ci: add failure-mode evidence for .github/workflows/coverage.yml + vitest.config.ts ## Summary
- Add CI failure-mode evidence for the recently changed workflow or test-runner surface.
## Why
- Backfill CI failure-mode evidence before another workflow or test-runner change lands on the touched surface.
## Touched paths
- `.github/workflows/coverage.yml`
- `vitest.config.ts`
## Validation
- Add or update a CI failure fixture, captured failing log, troubleshooting note, workflow dry-run evidence, or regression test for the changed CI/test-runner behavior.
- Run the affected workflow or test-runner entrypoint locally or in CI and record pass/fail evidence.Review Activity (5 reviews, 19 inline comments, 19 unresolved threads)
Top unresolved thread files
Latest reviewer states
Review Follow-up Signals (2)
Recommended next actions
Detected Workflows (1)
Generated Instincts (12)
After merging, import with: Files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The codebase grew through several architecture migrations that were additive — the old code
stayed and each new layer wrapped it. The result was a repo that declared the same message
contract twice, wrapped 36 octicons in 35 identical hand-written wrappers, inlined one
sxpreset across 15 files while the shared helper for it already existed, and shipped 234 lines of
Effect Schema with zero importers.
This PR removes the accumulated duplication without changing any user-visible behaviour.
−1,819 lines from
src/(32,384 → 30,565), 17 files deleted, 5 dependencies dropped.What changed
Five commits, each independently green and revertible.
chore: remove unreferenced code and dead exportsCode nothing referenced, verified by grep across
src/,scripts/, config files and CI:schemas-github.ts(234 lines) — zero importers; every GraphQL call goes throughgql(),which hardcodes
Schema.UnknownrenderPatError+ its 5-branchMatchblock — only its own test called it;token-setup.tsxuses
buildPatError, which is the better implementation (scoped token URL, expired-vs-invalid)effect-test-helpers.ts— its sole consumer destructured the layer and discarded the recordedcallsarray, so the whole recording apparatus was deadSearchSelectPaneldebug apparatus,TokenSetupCard'smode/onOpenOptions(its only callsite passes no props),
getFieldsthreaded through the sprint UI (both terminal consumersdestructured it as
_getFields),recentAssignees(never passed)src/assets/images/old/— 7 tracked binaries, 2.3 MB, referenced nowherepackage.jsonscripts nothing invokes — CI callspnpm wxt submitand the coverage scriptdirectly, not via these aliases
refactor: collapse duplicated icon and button-motion declarationssrc/ui/icons.tsxcontained zero hand-drawn SVGs — it imported 36 icons from@primer/octicons-reactand re-exported each behind an identical 3-line wrapper, 35 times.Replaced with one factory. No call-site changes across the 28 consuming files. 217 → 93 lines.
Also replaced 16 byte-identical inline copies of the button-motion block with the
primerCss.buttonMotion()preset that already existed and already had 10 callers.test: replace the custom toEqualValue matcher with vitest's toEqualeffect-assert.tsregistered a matcher that wrapped values in EffectData.*containers to getdeep equality — which
toEqualalready does. Verified rather than assumed: swapped all 39 callsites and re-ran the suite. Deleting it empties
vitest.setup.ts, so suite setup drops to 0 ms.refactor: inline single-caller Effect service wrappersproject-service.ts,cache-service.ts,services.tsandruntime-ext.tsexisted only so fourcall sites could write
yield* svc.foo()instead ofyield* Effect.promise(() => foo()). Eachwrapped an already-existing async helper and had exactly one consumer.
refactor: replace derived message schemas with a hand-written ProtocolMapschemas-messages.ts(688 lines) declared the message contract a second time. Its docstringclaimed handlers validated payloads with
Schema.decodeUnknownSync/encodeSync— a repo-widegrep found those calls in no handler. Six payload types were declared twice, and every
consumer imported the hand-written twin from
messages.ts, never the schema.chore: drop unused dependencies@effect/platform-browser,@resvg/resvg-js,@types/marked,@vitejs/plugin-react,vite-node— all with no source references, confirmed by a clean production build.How equivalence was proven, not assumed
The
ProtocolMaprewrite is the one changetscalone cannot fully guarantee: method parametersare bivariant, so a widened input (
stringwhere the schema said'a' | 'b') would slipthrough a plain
A extends Bcheck.A temporary type-level scaffold compared each of the 35 entries' input and output mutually,
with tuple-wrapped conditionals so unions don't distribute. It was negative-tested — widening
bulkClose'sreasontostringmade the build fail by name (Type 'true' is not assignable to type '"bulkClose"') — then removed once both sides matched exactly.Deliberately not cut
design-system/rgp/MASTER.md:184makes it canonical and bans Primer's<Tooltip>@primer/live-region-element— looks unused, butwxt.config.tsaliases it to a local stubSelectionControl'svariantprop — flagged as dead by the audit, butcheckbox-portal-host.tsxdoes pass itbuttonMotioncopies —makePresetshallow-merges, so an override supplyingits own hover rule would replace the base and silently drop
transformThemeProvider/BaseStyles, half renderraw, and return shapes differ. Similar-looking, not duplicated.
sleep(1000)between mutations,403/429
Retry-AfterhandlingTest plan
pnpm typecheckpnpm lintpnpm formatpnpm testpnpm buildTest count fell 425 → 408 because 17 tests were deleted alongside the dead code they covered —
they asserted that exported strings were non-empty strings, that deliberately-empty functions
don't throw, and that Effect's
Schemaround-trips.The pre-existing
pnpm testfailure is fixed:bulk-transfer-modal.test.tsx › renders count in titlewas timing out at 5 s while costing ~500 ms of real work — worker contention across the43-file parallel suite. Raised
testTimeoutto 15 s.Verified in a real browser
Loaded the built
dist/chrome-mv3unpacked in Chrome. Service worker starts with no errors; theoptions page (
TokenSetupCard, gear/check icons) and popup (eye icon, keyboard chips) both rendercorrectly with zero console errors.
Still to do before merge
configured PAT; bulk transfer additionally needs a destination repo)
Summary by cubic
Replaces the Effect stack with plain async/fetch, unifies bulk progress/queue handling, and removes duplicated/unreferenced code (−1,819 lines). Fixes item ID resolution: the hyphen form (issue-N) now matches by issue number in both hierarchy fetches and reorder; previously it was treated as a databaseId and failed, so deep-linked panes and Custom position now work reliably and preserve selection order.
runBulkVerbandqueue-runstandardize progress frames and pacing across state-change verbs; reorder stays separate (no sleeps, caller-overridable labels). Queue cancellation uses AbortController; the tracker frame is typed once asQueueFrame.gqlfetch client replaces the service stack (3 rate‑limit retries, 30s per‑attempt timeout, same backoff/jitter). Error classes are plainErrorsubclasses; logging callsconsoledirectly.GET_PROJECT_ITEMS_FOR_REORDERnow selectsnumber, andcreateIssueRefIndexresolves bothissue:DBIDandissue-Nconsistently.plural(),newProcessId(), andKbd; modal backdrops useprimerCss.modalOverlay(). Duplicate/relationship section UI is centralized.effect,@effect/platform*,@primer/live-region-element, and other unused packages. Typecheck/lint/build remain green.Written for commit e46930e. Summary will update on new commits.