fix(frontend,sdk): fix CI branch mismatch and 68 real type errors it was hiding - #401
Open
IamHarrie-Labs wants to merge 1 commit into
Open
Conversation
…rors it was hiding The CI workflow (.github/workflows/ci.yml) has always triggered on push/PR to main/develop, but this repo's actual default branch is master - so the type-check job (and every other job) has never once run on a real commit. That let 68 tsc errors accumulate undetected on master, several of which are genuine functional bugs, not just type friction: - lib/transactionSearch.ts / transactionSearchIndex.ts compared PaymentRecord.type against "payment"/"payment_received", values that never occur (the real type is "sent"|"received"|"merge") - so from:/to: search operators and address indexing silently never matched anything, and buildIndex() read a nonexistent `.hash` field, throwing on first use. - HighlightedTransactionRow.tsx had the same "payment_received" check, so every transaction rendered as "Sent to" with the outgoing icon regardless of actual direction, and also read `.hash` instead of `.transactionHash`. - lib/turrets.ts's pauseTurretsFunction/resumeTurretsFunction discarded the response body entirely, even though the backend returns the updated deployment (and the SDK's own type said Promise<void> instead of Promise<TxFunctionDeployment>) - settings.tsx was splicing `undefined` into deployment state on every pause/resume. - components/FeeEstimator.tsx didn't exist at all despite being imported and wired through SendPaymentForm.tsx's fee-tier state; buildPaymentTransaction/buildSorobanTipTransaction didn't accept the `baseFee` override being passed to them, so the fee tier a user picked was silently discarded. - lib/stellar.ts was missing getReceipt/getContractTipCount exports that pages/receipts.tsx, pages/[username]/receipt/[index].tsx, and CreatorTipsDashboard.tsx already imported, and RecurringPayments.tsx imported a listStreamsByPayer that never existed - pages/components now build and prior duplicate signTransactionWithWallet import fixed. Fixed ci.yml's trigger branches to match the real default branch, then fixed every one of the 68 errors tsc now surfaces (down to 0), split across real source bugs above and stale test fixtures using the same wrong type/field names. Added the missing shared/types/index.ts re-export and shared/errorCodes.d.ts (kept errorCodes.js as CommonJS since ~15 backend files require() it directly under plain Node on Node 20 in CI - backend TS conversion is out of scope here per the issue, so renaming it would have broken every one of those call sites). Converted the one remaining non-config frontend .js file (test-ledger.js, a manual browser-console script) to .ts. `npx tsc --noEmit` and `next build` both clean; jest suite has the same 16 pre-existing unrelated failures as an unmodified checkout (down from 17 - SendPaymentForm.test.tsx now passes since FeeEstimator exists), confirmed no new failures introduced. Closes FinChippay#396
|
@IamHarrie-Labs is attempting to deploy a commit to the Topmatrixmor2014 Team on Vercel. A member of the Team first needs to authorize it. |
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
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.
Closes #396
The real finding
Before touching anything,
npx tsc --noEmiton a cleanmastercheckout produced 68 errors, despite.github/workflows/ci.ymlsupposedly runningnpm run type-checkas a blocking step. Digging in: the CI workflow triggers onpush/pull_requesttomain/develop, but this repo's actual default branch ismaster— so the type-check job (and every other CI job) has never once run on a real commit. I confirmed via the Actions API that this workflow has zero recorded runs. That's why 68 errors piled up unnoticed.I fixed the trigger branches in
ci.ymlfirst, then worked through all 68 errors untiltsc --noEmitwas clean.Several of these were real, shipped bugs — not just type friction
lib/transactionSearch.ts/transactionSearchIndex.ts: comparedPaymentRecord.typeagainst"payment"/"payment_received", values that never occur (the real type is"sent"|"received"|"merge", confirmed by tracing every place aPaymentRecordis actually constructed). So thefrom:/to:search operators and address indexing silently never matched, andbuildIndex()read a.hashfield that doesn't exist onPaymentRecord—undefined.length— throwing on the very first payment it touched. The offline search-index feature has been completely broken.components/HighlightedTransactionRow.tsx: same"payment_received"check — meaning every transaction row rendered as "Sent to" with the outgoing icon, regardless of whether it was actually received. Also read.hashinstead of.transactionHash.lib/turrets.ts:pauseTurretsFunction/resumeTurretsFunctiondiscarded the response body entirely (await sdk.turrets.pause(id)), even though the backend controller's own doc comment says it returns{ success: true, data: DeploymentRecord }. The SDK's own type saidPromise<void>too.settings.tsxwas splicing the return value into deployment state on every pause/resume — meaning every toggle replaced that deployment withundefinedin the list.components/FeeEstimator.tsxdidn't exist despite being imported and fully wired throughSendPaymentForm.tsx's fee-tier state (selectedFeeStroops,selectedFeeTier,onFeeSelected). And even once I added it,buildPaymentTransaction/buildSorobanTipTransactiondidn't accept thebaseFeeoverride being passed to them — the fee tier a user picked was silently discarded and the transaction always used Horizon's auto-fetched fee. Fixed both: added the component and threaded a realbaseFeeoverride throughbuildPaymentTransaction(documented why it's accepted-but-inert on the Soroban tip path, since Soroban's resource fee comes from RPC simulation, not a flat base fee — overriding it there isn't safe).lib/stellar.tswas missinggetReceiptandgetContractTipCountthatpages/receipts.tsx,pages/[username]/receipt/[index].tsx, andCreatorTipsDashboard.tsxalready imported (the underlying contract-bindings methods existed, just never re-exported), andRecurringPayments.tsximported alistStreamsByPayerthat never existed anywhere — added it mirroring the existinggetActiveStreamsForRecipientpattern (the contract has no payer index, so it walks all streams and filters/paginates client-side).pages/settings.tsxalso had a literal duplicate import ofsignTransactionWithWallet.None of these pages/components could have built successfully before this —
next buildtype-checks by default and there's noignoreBuildErrorsoverride, sonpm run buildwas presumably also broken, just never verified in CI for the same branch-mismatch reason.What else changed
shared/types/index.ts(new) — re-exports the SDK's OpenAPI-generated types, per the issue's expected architecture. Checked for actual type duplication between frontend and SDK first (grepped every SDK interface name against frontend) — found only two, both false positives (client-side Horizon-derived shapes that happen to share a name with an SDK/API type, not real duplicates), so didn't force a merge that doesn't reflect reality.shared/errorCodes.d.ts(new) — deliberately did not renameshared/errorCodes.jsto.ts. It'srequire()'d directly by ~15 backend files under plain Node (node src/server.js, no ts-node/build step), tested on Node 20 in CI. Node 20 has no native TS support, so renaming it would break every one of those call sites — and backend TS conversion is explicitly out of scope for this issue. A co-located.d.tsgives frontend/SDK consumers full typing without touching backend's runtime at all.frontend/test-ledger.js→.ts— the one remaining non-config.jsfile infrontend/(a manual browser-console script, not part of the build).tsconfig.jsonalready hasstrict: true— that part of the issue was already done.Verification
npx tsc --noEmit: 68 → 0 errors.next build(with the same env vars CI sets): clean production build, all 20 routes.npm test: same 16 pre-existing failing suites as an unmodifiedmastercheckout (was 17 —SendPaymentForm.test.tsxnow passes sinceFeeEstimatorexists). Confirmed by running the full suite againstgit stash'd original code first, then against my branch, and diffing the two failure lists — no new failures introduced. The remaining 16 are unrelated pre-existing issues (e.g. a test mockingnext-i18next, a package this app doesn't use — it'sreact-i18nextelsewhere) that predate this PR and are out of scope here.sdk'snpm run build(tsc) clean.Known gaps, flagged rather than silently left
.github/workflows/codeql.yml,docker-publish.yml, andvercel-deploy.ymlhave the samemain/developbranch-trigger mismatch asci.ymlhad. I only fixedci.ymlsince that's the one this issue's acceptance criteria are about, and I didn't want to touch security-scanning/deploy pipelines I can't verify locally — but they're almost certainly equally dead right now and worth a follow-up.noUncheckedIndexedAccess(mentioned in the issue's tsconfig snippet) isn't enabled — given the 68 pre-existing errors already found real bugs, I prioritized getting to a clean, enforcedstrict: truebaseline first rather than adding a stricter flag that would introduce a new backlog on top of it. Happy to do it as a fast follow once this is merged and the branch mismatch stops hiding new regressions.npm installfails locally with a pre-existingERESOLVEconflict (@opentelemetry/auto-instrumentations-nodepeer mismatch), unrelated to anything here (backendpackage.jsonuntouched) — couldn't run backend's test suite, but I made zero backend code changes so there's nothing there for it to catch.