refactor: remove duplicated declarations and unreferenced code (-1,819 lines) - #60
Open
fathiraz wants to merge 6 commits into
Open
refactor: remove duplicated declarations and unreferenced code (-1,819 lines)#60fathiraz wants to merge 6 commits into
fathiraz wants to merge 6 commits into
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>
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
Removes unused code and duplicated declarations to shrink the codebase with no user-visible changes. Net −1,819 LOC, 17 files deleted; build/typecheck/tests stay green (408 pass).
Refactors
primerCss.buttonMotion().toEqual; increased VitesttestTimeoutto 15s to deflake.Effect.tryPromise(...).orDiewithEffect.promise(...)in handlers.ProtocolMap; removed unused schema files and snapshot tests.Dependencies
@effect/platform-browser,@resvg/resvg-js,@types/marked,@vitejs/plugin-react,vite-node.Written for commit d79010f. Summary will update on new commits.