Skip to content

refactor: replace hand-rolled utilities and dead code with the shared forms - #7021

Merged
waleedlatif1 merged 3 commits into
stagingfrom
deslop-mechanical
Aug 24, 2026
Merged

refactor: replace hand-rolled utilities and dead code with the shared forms#7021
waleedlatif1 merged 3 commits into
stagingfrom
deslop-mechanical

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Replaces #7016, which GitHub closed when its base branch (deslop-codebase, now merged as #7015) was deleted. Same commits, rebased onto staging. #7016 carried Greptile 5/5 with both review threads resolved.

What

Sites that hand-roll something the repo already mandates, plus the dead code around them. Each was verified equivalent before changing, not pattern-matched:

Replaced With Where
Object.fromEntries(Object.entries(x).filter(...)) omit() blocks/blocks/{stt,fireflies,grain}.ts
error instanceof Error ? error.message : String(error) getErrorMessage() executor/utils/errors.ts:53
getAllBlocks().find((b) => b.type === x) getBlock() tool-input.tsx ×2, agent-handler.ts
.find()-by-id in a loop a Map index skill-input.tsx ×3, display.ts
15 * 1000 ×3 SELECTOR_SEARCH_STALE {jira,google,webflow}/selectors.ts
static inline style Tailwind classes {plus,skills}-menu-dropdown.tsx
catch (e) {} catch {} api-handler.ts, executor/utils.ts ×2

omit() also recovers the Omit<T, K> typing that Object.fromEntries erases to a bare index signature.

getAllBlocks() is not memoised — each call built a fresh 336-element array before scanning it. getBlock is an O(1) registry index. agent-handler's call ran per tool during execution; tool-input's ran inside a loop over selected tools.

panel.tsx loses a TODO-stubbed const hasValidationErrors = false and the isWorkflowBlocked term built on it. That term was dead twice over — it reduced to isExecuting, and the enclosing expression is already guarded by !isExecuting.

Two things review caught

getBlock is not a drop-in for .find(). It normalizes via type.replace(...), so it throws on undefined where .find() returned undefined harmlessly. Both call sites are reachable without a type — tool-input reads state.blocks[blockId]?.type, undefined once the block is deleted while the panel is mounted, and selectedTools validates only value[0]?.type before casting the whole array. Record indexing hid both from the compiler. All guarded now.

providers/utils.ts:683 deliberately keeps its getAllBlocks().find(...). It takes the registry as an injected dependency so a client-reachable module never imports it; reaching for getBlock there would cross that boundary.

Testing

  • 7019 tests passing across executor, blocks, hooks, lib/workflows/subblocks, app/workspace
  • bun run type-check clean; bun run check:api-validation passes

… forms

Each of these has a mandated helper or an established accessor in the repo that
the site predates or missed. All are behavior-preserving:

- `omit()` for the three `Object.fromEntries(Object.entries(x).filter(...))`
  block-input filters, which also recovers the `Omit<T, K>` typing that
  `Object.fromEntries` erases to an index signature.
- `getErrorMessage()` for the inline `instanceof Error` message ternary.
- `getBlock()` for two `getAllBlocks().find((b) => b.type === x)` scans, one of
  them inside a loop over selected tools. The same file already resolves the
  same values through `getBlock`.
- A memoised `Map` for three `.find()`-by-id scans over the workspace skill
  list, one of them inside a render `.map()`.
- `SELECTOR_SEARCH_STALE` for three copy-pasted `15 * 1000` literals. They are
  deliberately shorter than `SELECTOR_STALE`, so this is a new named constant
  rather than a fold into the existing one.
- Tailwind classes for the static half of two duplicated anchor styles, keeping
  only the genuinely dynamic `left`/`top` inline.
- Dropped the unused `catch` bindings on three intentional JSON-parse swallows.

`panel.tsx`'s run-button gate loses a `TODO`-stubbed `hasValidationErrors =
false` and the `isWorkflowBlocked` term built on it. That term was dead twice
over: it reduced to `isExecuting`, and the enclosing expression is already
guarded by `!isExecuting`.
…s callers

`getBlock` normalizes its argument with `type.replace(...)`, so it throws on
`undefined` where the `getAllBlocks().find(...)` it replaced returned
`undefined` harmlessly. Both call sites can be reached without a type:
`tool-input` reads `state.blocks[blockId]?.type`, which is undefined once the
block is deleted while the panel is mounted — and `Record` indexing hides that
from the compiler, so it would have thrown during render. `agent-handler`'s
`tool.type` is optional and the compiler did catch it.

Also index the skill lookup in `resolveSkillsLabel`, which runs a `.find()`
inside a `.map()` for every block on the canvas — the case the memoised map in
`skill-input` addressed for one component while leaving the hot path.

`providers/utils.ts` keeps its `getAllBlocks().find(...)`: it takes the
registry as an injected dependency precisely so a client-reachable module never
imports it, and reaching for `getBlock` there would cross that boundary.

The new constant's doc claimed search-backed selectors take a shorter window.
Several still sit on `SELECTOR_STALE`, so it now describes the value its three
callers share rather than asserting a rule the tree does not follow.
`selectedTools` validates only `value[0]?.type` and then casts the whole array,
so a persisted workflow whose later rows lost their `type` yields `undefined`
here — the cast is what makes the compiler believe otherwise. `getBlock`
normalizes with `type.replace`, so that throws during render.
@vercel

vercel Bot commented Aug 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 24, 2026 12:20am

Request Review

@cursor

cursor Bot commented Aug 24, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches executor tool-input projection (getBlock vs a full registry scan). Behavior is intended to be equivalent, but lookup semantics differ and those call sites run during execution and the editor.

Overview
Replaces several one-off patterns with existing helpers: omit() for v2 block input maps, getBlock() instead of scanning getAllBlocks(), getErrorMessage(), and a shared SELECTOR_SEARCH_STALE for Drive/Jira/Webflow selectors.

Skill name resolution now uses a Map instead of repeated .find() in skill-input and resolveSkillsLabel. Dropdown anchors drop inline layout styles for Tailwind. Dead hasValidationErrors / isWorkflowBlocked wiring is removed from the run button.

getBlock is guarded where type can be missing so it does not throw the way .find() used to no-op.

Reviewed by Cursor Bugbot for commit af05283. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR consolidates equivalent utility, lookup, constant, styling, and error-handling patterns while removing dead workflow-run state.

  • Replaces repeated object filtering, error formatting, block scans, skill scans, and stale-time literals with shared helpers or indexed lookups.
  • Converts static dropdown anchor styles to equivalent Tailwind classes.
  • Removes unused catch bindings and an inert workflow-validation placeholder.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness, security, or maintainability issues identified.

The changed lookups are guarded where missing types can occur, and the remaining helper, constant, styling, indexing, catch-binding, and dead-code substitutions preserve the behavior of their replaced forms.

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx Replaces repeated catalog scans with guarded registry lookups while preserving missing-type handling.
apps/sim/executor/handlers/agent/agent-handler.ts Uses the indexed block registry when resolving input definitions for agent tool projection.
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/skill-input/skill-input.tsx Builds a memoized skill-ID index and reuses it for modal, display-name, and selected-skill resolution.
apps/sim/blocks/blocks/fireflies.ts Uses the shared non-mutating omit helper to derive the v2 input definition.
apps/sim/blocks/blocks/grain.ts Uses the shared omit helper when deriving Grain v2 inputs.
apps/sim/blocks/blocks/stt.ts Uses the shared omit helper to remove the legacy audio URL input from the v2 definition.
apps/sim/executor/utils/errors.ts Replaces local Error/string message normalization with the equivalent shared helper.
apps/sim/hooks/selectors/providers/shared.ts Names and centralizes the existing 15-second stale window shared by three selectors.
apps/sim/lib/workflows/subblocks/display.ts Indexes skills by ID once before resolving stored skill labels.
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx Removes validation placeholders that could not affect the run-button disabled expression.

Reviews (1): Last reviewed commit: "fix: guard the second registry lookup in..." | Re-trigger Greptile

@waleedlatif1
waleedlatif1 merged commit 49593b3 into staging Aug 24, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the deslop-mechanical branch August 24, 2026 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant