refactor(sim): consolidate record guards + pure utils into @sim/utils - #5061
Conversation
Add isRecordLike (loose, non-prototype-checked record guard) and sortObjectKeysDeep; relocate isPlainRecord (strict) and normalizeEmail into @sim/utils so they are reusable across apps and packages. Unit tests cover the loose-vs-strict distinction, deep key sorting, and email normalization.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview
Other: Copilot workflow JSON formatting uses shared Domain-specific normalizers and array-allowing guards called out in the PR description are intentionally unchanged. Reviewed by Cursor Bugbot for commit 3cbf4c7. Configure here. |
Greptile SummaryThis PR consolidates a widespread pattern of locally-duplicated record guard predicates and pure utility functions into the
Confidence Score: 5/5This is a mechanical deduplication refactor with no functional changes — every replaced predicate and transform is semantically identical to its canonical equivalent. All ~55 inline record-guard substitutions replace predicates logically equivalent to isRecordLike. Boolean(v) and v !== null produce identical results for objects. isPlainRecord is a straight move with unchanged prototype logic. sortObjectKeysDeep is byte-for-byte identical to the two local implementations it replaces. normalizeToken and normalizeEmail consolidations are exact copies. The package.json subpath exports for @sim/utils/object and @sim/utils/string are already in place. No files require special attention — the most complex migration sites (copilot/request/session/contract.ts and executor/utils/start-block.ts) were audited and found to be clean substitutions. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["@sim/utils/object"] -->|isRecordLike| B["55+ call sites across\ntools / copilot / triggers / lib"]
A -->|isPlainRecord| C["records.ts\nexecution-core.ts\nsubblocks.ts\nsubblock-migrations.ts\nuser-file-base64.server.ts"]
A -->|sortObjectKeysDeep| D["json-sanitizer.ts\nbuilders.ts"]
E["@sim/utils/string"] -->|normalizeEmail| F["invitations/core.ts\ninvitations/send.ts\nworkspace-invitations.ts\nroute handlers"]
G["triggers/utils/tokens.ts"] -->|normalizeToken| H["salesforce/utils.ts\nservicenow/utils.ts"]
style A fill:#4ade80,stroke:#16a34a
style E fill:#4ade80,stroke:#16a34a
style G fill:#86efac,stroke:#16a34a
Reviews (2): Last reviewed commit: "refactor(sim): consolidate record guards..." | Re-trigger Greptile |
…sim/utils Replace ~55 re-implemented loose record guards with the canonical @sim/utils isRecordLike (and one strict site with isPlainRecord), and dedupe three normalize clusters: sortObjectKeysDeep (sanitization + copilot builders), normalizeToken (salesforce + servicenow triggers), and normalizeEmail. Array-allowing guards and domain-specific normalizers are intentionally left untouched. Pure refactor — identical predicates and transforms, no behavior change.
69ad368 to
3cbf4c7
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3cbf4c7. Configure here.
* refactor: consolidate local isRecord guards onto shared isRecordLike Nineteen files had re-declared a local `isRecord` guard rather than using the shared one from `@sim/utils/object`, drift that reappeared after #5061 first consolidated them. Two more imported the shared guard under an `isRecordLike as isRecord` alias. The copies were not interchangeable. Nine matched `isRecordLike` exactly. The rest omitted the array exclusion (`typeof x === 'object' && x !== null`, or `Boolean(x) && typeof x === 'object'`), so arrays passed the guard. Each of those call sites was reviewed individually: in every case the guard is followed by string/number field checks that an array fails anyway, so the outcome is unchanged. The one exception is `isOptionsTagData`, where `Object.values` on an array of option items really did make an array-form `<options>` tag render. It now accepts arrays explicitly rather than by accident. `executor/handlers/pi/search/extension-source.ts` keeps its own copy: it is source text written into an E2B/Daytona sandbox at runtime and cannot import. * refactor: replace inline record guards with isRecordLike 103 inline `typeof x === 'object' && x !== null && !Array.isArray(x)` guards (and the `x &&` / `Boolean(x)` spellings of the same conjunction) now call the shared guard. Inside a conjunction that already asserts `typeof x === 'object'`, `x &&` and `x !== null` are interchangeable, so all three orderings are the same predicate at runtime. Only exactly-equivalent conjunctions were converted. Matching required the three clauses to be one adjacent conjunction over the same operand, so a nearby but unrelated clause cannot be absorbed — generic-handler.ts, where the array case is handled inside the block rather than excluded by the guard, is correctly left alone. Two sites were reverted after type-check rejected them: instagram/server-utils.ts and workflows/[id]/log/route.ts both cast straight to a specific interface, which is legal from `object` but not from `Record<string, unknown>`. Their narrowing is genuinely not identical, so they keep the inline form rather than acquiring a double cast. Left as-is: `packages/ts-sdk` (published with no runtime dependencies) and the two sandbox sources written into E2B/Daytona as text, which cannot import. * refactor: consolidate duplicate record coercion helpers onto @sim/utils A second sweep found 28 more local record helpers hiding under names the previous `isRecord` grep never matched — `asRecord`, `toRecord`, `toRecordOrNull`, `asObject`, `isJsonObject`. rabbitmq defined the same `asRecord` twice within one service; dynatrace had three variants in one file. Fourteen of them were re-deriving the same two shapes, so those shapes now live in `@sim/utils/object` beside the guards they wrap: toRecord(value) // isRecordLike(value) ? value : {} toRecordOrNull(value) // isRecordLike(value) ? value : null Both preserve identity on a hit, so no call site starts copying. Eighteen local definitions are gone. `tools/instantly/utils.ts` keeps its exported `asRecord` because its return type is the local `JsonRecord` alias, but its body now delegates. `app/api/mcp/serve/[serverId]/route.ts` had an `isJsonObject` with zero call sites — deleted outright. Six helpers were deliberately left alone because they are NOT equivalent: `pagerduty`/`zendesk`/`gitlab` do `(value as Record) || {}`, which type-checks nothing at all, and `copilot/resources/extraction.ts`, `edit-workflow/validation.ts`, `pi/core/events.ts` omit the array exclusion. Those sit on webhook ingress and copilot paths where tightening is a behavior change, not a cleanup; they are audited separately. Two guards were removed rather than substituted, each proven dominated by an earlier check: the bedrock streaming `toolUse.input` guard was unreachable (`parseToolInput` already throws on non-objects before the loop builds `assembledToolUses`), and four `driver.ts` re-narrows follow an `if (!isRecordLike(x) || ...) throw` that dominates the later use. * fix(webhooks): guard non-string GitLab ref, and consolidate the last record helpers Two audits covered the six helpers held back from the previous commit for not being equivalent to isRecordLike. Five are now migrated; one is deliberately not. `gitlab.ts` carried a real crash path, independent of the guard work: const ref = (b.ref as string) || '' const branch = ref.replace('refs/heads/', '') The cast is unchecked and `|| ''` only catches falsy values, so a truthy non-string body field — `{"ref": 12345}` — reaches `.replace` and throws `TypeError: ref.replace is not a function` inside `formatInput`. That runs in the background worker after the webhook is already 200-ACKed, and GitLab does not auto-retry, so the delivery is lost silently. Now checks `typeof`. `pagerduty`/`zendesk`/`gitlab` each defined `asRecord` as `(value as Record<string, unknown>) || {}`, which type-checks nothing — a string or array passed through and was then spread into the workflow trigger payload (`gitlab.ts:114`) as character- or index-keyed garbage. All three now use the shared `toRecord`. These sit behind `verifyProviderAuth`, so reaching them requires the shared secret; this is robustness, not authorization. `copilot/resources/extraction.ts` and `pi/core/events.ts` were array-permissive but provably inert — extraction.ts has no key enumeration or spread anywhere, and events.ts only diverges by returning `null` instead of `{type:'other'}` for an array, which every consumer already no-ops on. Pinned with a test. `edit-workflow/validation.ts` is left permissive ON PURPOSE. Its `Object.entries` walk mirrors the unguarded walk in `operations.ts:86,191`, so an array-shaped `nestedNodes` from the model is currently visited by both. Tightening only the validation side would stop `collectHostedApiKeyInput` from stripping platform-managed API keys while the apply side still creates those child blocks. Both paths have to change together, with tests, in their own PR. * refactor: delete 31 unreachable module-local functions Removes 815 lines of provably dead code: module-local (non-exported) declarations whose identifier appears exactly once in their own file — the declaration itself. A non-exported symbol cannot be reached by an import, a barrel, a dynamic import, or a framework convention, so "unreferenced in its own file" is a complete proof of deadness rather than a heuristic. Notable removals include whole abandoned code paths: `findWebhookAndWorkflow` (78 lines), `calculateBillingProjection` and `initializeUserUsageLimit` (80 lines), `removeCredits` + `deductFromCredits` (54), `sendBatchSMS`, `executeToolBatch`, and four unused `async-runs/repository.ts` queries. Spans come from the TypeScript AST, not a regex. A regex cannot find a declaration's extent — a first-brace scan cuts inside a return-type annotation such as `Promise<{ canCreate: boolean }>` and silently corrupts the file. The AST pass also re-derives deadness from identifier nodes, which caught two wealthbox helpers that a regex export-check had wrongly reported as local. Note the runtime TypeScript API here is `@typescript/typescript6`; the bare `typescript` specifier resolves to the native compiler, which exposes no `createSourceFile`. * refactor: delete 434 stranded exported symbols Removes ~5,930 lines of unreachable code across 166 files: exported symbols that no other file in the repo mentions and that are unused inside their own file. Whole abandoned surfaces go with them — unused React Query hooks (useOrganizations, useOrganizationMembers, useUpgradeSubscription, ...), unused admin route contracts, dead executor constants and reference builders, and the landing-page StageWorkflow/LandingPreviewMount components. Three files left with no remaining code were removed outright. Candidates came from an AST pass; each was then verified individually against an UNFILTERED repo-wide search plus the reachability paths a name search misses: string-keyed tool/block registries, dynamic imports, `export *` barrels, and the docs generator's source-text parsing of tool files. 29 candidates were verified LIVE and kept. Those exposed a flaw in the candidate generator: it indexed only .ts/.tsx, while apps/docs/content/**/*.mdx imports React components directly — ActionImage appears in ~190 MDX pages and ActionVideo in ~115, and both scanned as dead. `app/global-error.tsx` was likewise kept, since Next.js reaches it by filename and its default export can never have a name reference. All 434 deleted names were afterwards cross-checked against every .mdx/.md/.json/.yaml in the repo: no hits. Verified with turbo type-check (23 workspaces), the full apps/sim suite (25,219 tests), all 26 audits, and a production `next build` — the last of these being what actually exercises route- and component-level reachability.
Summary
@sim/utils:isRecordLike(loose),sortObjectKeysDeep; relocateisPlainRecord(strict) andnormalizeEmailso they're reusable across apps and packages.typeof === 'object' && !== null && !Array.isArray) across tools/providers/lib/workflows/copilot/triggers/contracts with the singleisRecordLike; one strict site →isPlainRecord.normalize*clusters:sortObjectKeysDeep(sanitization + copilot builders),normalizeToken(salesforce + servicenow triggers),normalizeEmail.comparison/normalize.ts, copilot persisted-message, vanta, SSO/domain) — migrating those would change behavior.Type of Change
Testing
@sim/utilsunit tests: loose-vs-strict guard distinction (Date/class-instance → loosetrue, strictfalse), deep key sort, email normalization.check:api-validation:strict+ monorepo-boundary checks pass.origin/staging— every migrated predicate/transform is an exact semantic no-op; the one risky union-narrowing site (lib/table/sql.ts) was deliberately skipped.Checklist