Skip to content

improvement(selectors): simplify selector context + add tests#3453

Merged
icecrasher321 merged 3 commits intostagingfrom
improvement/selector-context
Mar 7, 2026
Merged

improvement(selectors): simplify selector context + add tests#3453
icecrasher321 merged 3 commits intostagingfrom
improvement/selector-context

Conversation

@icecrasher321
Copy link
Collaborator

Summary

Selector context consolidation and rename credentialId context param to the correct canonicalParamId. Added test to make sure ids added to selector context fields are always

  • CanonicalParamId if canonical pair exists
  • Otherwise, subblock id

Type of Change

  • Other: Code Quality

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Mar 7, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 7, 2026 2:25am

Request Review

@cursor
Copy link

cursor bot commented Mar 7, 2026

PR Summary

Medium Risk
Touches selector context plumbing across the editor, selector registry, and display-name/hydration paths; a mismatch in the oauthCredential rename or context extraction could disable selector queries or break ID-to-name hydration for external services.

Overview
Unifies how selector context is built and passed around by introducing SELECTOR_CONTEXT_FIELDS + buildSelectorContextFromBlock, and using this canonical mapping in both UI (useSelectorSetup) and diff/label resolution (resolve-values).

Renames selector context credential field from credentialId to oauthCredential and updates selector query enablement/query keys, Slack selector overrides, and useSelectorDisplayName/resolveSelectorForSubBlock signatures accordingly.

Adds Vitest coverage for context extraction/canonical mapping and validates that SELECTOR_CONTEXT_FIELDS only contains valid canonicalParamIds (or direct subblock IDs when no canonical pair exists).

Written by Cursor Bugbot for commit adea9db. Configure here.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 7, 2026

Greptile Summary

This PR consolidates selector context handling across the codebase in two related ways: (1) it renames credentialIdoauthCredential in SelectorContext to align the field name with the canonicalParamId used in all 44 OAuth block definitions, and (2) it introduces a new shared buildSelectorContextFromBlock utility in context.ts that replaces three separate, hardcoded subblock-ID-to-context-field extraction implementations (in use-selector-setup.ts, use-selector-display-name.ts, and resolve-values.ts) with a single canonical-index-driven lookup against a typed SELECTOR_CONTEXT_FIELDS set.

Key changes:

  • apps/sim/lib/workflows/subblocks/context.ts (new): Centralizes SELECTOR_CONTEXT_FIELDS and buildSelectorContextFromBlock, replacing per-caller hardcoded field maps and the oauthCredentialcredentialId special-case that existed in use-selector-setup.ts.
  • apps/sim/hooks/selectors/types.ts: SelectorContext.credentialIdSelectorContext.oauthCredential; this is the breaking change that cascades through registry.ts, resolution.ts, use-selector-display-name.ts, sub-block.tsx, and workflow-block.tsx.
  • apps/sim/hooks/selectors/resolution.ts: Removes the now-redundant SelectorResolutionArgs interface and simplifies resolveSelectorForSubBlock to spread a SelectorContext directly.
  • apps/sim/lib/workflows/comparison/resolve-values.ts: Removes the local ExtendedSelectorContext interface and extractExtendedContext function, delegating to the new buildSelectorContextFromBlock.
  • apps/sim/lib/workflows/subblocks/context.test.ts (new): Adds unit tests for context extraction and a validation test that asserts every entry in SELECTOR_CONTEXT_FIELDS is either a canonical param ID present in some block definition or a raw subblock ID — preventing stale or incorrect field names from sneaking in.

Confidence Score: 4/5

  • Safe to merge — the rename is consistently applied across all callsites and all 44 OAuth blocks already have canonicalParamId: 'oauthCredential' set, so no runtime breakage is expected.
  • The change is a well-scoped refactor with consistent renaming and a new central utility backed by automated tests. The main deductions are: (1) the local variable credentialId in workflow-block.tsx was not updated to oauthCredential, leaving a minor naming inconsistency; and (2) the new test suite doesn't include an explicit case verifying that a credential subblock maps to context.oauthCredential — the primary changed field — relying on the SELECTOR_CONTEXT_FIELDS validation test to cover it indirectly.
  • workflow-block.tsx (stale local variable name) and context.test.ts (missing explicit oauthCredential extraction test).

Important Files Changed

Filename Overview
apps/sim/lib/workflows/subblocks/context.ts New file centralizing selector context extraction. Introduces SELECTOR_CONTEXT_FIELDS Set and buildSelectorContextFromBlock which replaces fragile hardcoded subblock ID lookup with canonical-index-based resolution. Logic is clean and correct.
apps/sim/lib/workflows/subblocks/context.test.ts New test file covering context building and SELECTOR_CONTEXT_FIELDS validation. Good coverage for knowledgeBaseId canonical mapping and edge cases, but missing an explicit test for oauthCredential — the primary renamed field in this PR.
apps/sim/hooks/selectors/types.ts Renames credentialId to oauthCredential in SelectorContext. Clean, breaking change that aligns the field name with the canonical param ID used in block definitions.
apps/sim/hooks/selectors/registry.ts Mechanical rename of credentialIdoauthCredential across all ~25 selector entries. All usages updated consistently including getQueryKey, enabled, and fetchList bodies.
apps/sim/hooks/selectors/resolution.ts Removes the separate SelectorResolutionArgs interface and simplifies resolveSelectorForSubBlock to accept SelectorContext directly, spreading it with a mimeType override. Clean simplification.
apps/sim/lib/workflows/comparison/resolve-values.ts Removes duplicated ExtendedSelectorContext interface and the manual hardcoded extraction function, replacing both with a call to the new buildSelectorContextFromBlock. Properly passes workflowId. Old OR-fallback for some fields (e.g. `spreadsheetId
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/workflow-block.tsx Prop rename from credentialIdoauthCredential in the useSelectorDisplayName call is correct, but the local variable credentialId on line 534 was not renamed to match, leaving a naming inconsistency.
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/hooks/use-selector-setup.ts Removes the inline CONTEXT_FIELD_SET and the special-cased oauthCredential → credentialId mapping, replacing both with the shared SELECTOR_CONTEXT_FIELDS. The simplified loop is cleaner and consistent with the new central approach.
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/sub-block.tsx Updates SLACK_OVERRIDES.transformContext to use oauthCredential instead of credentialId. Correct and minimal change.
apps/sim/hooks/use-selector-display-name.ts Renames credentialIdoauthCredential in the SelectorDisplayNameArgs interface and its usage. Clean and consistent with the broader rename.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["SubBlock / WorkflowBlock\n(dependencyValues)"] -->|"credential subblock value"| B["buildSelectorContextFromBlock\n(context.ts)"]
    B -->|"canonicalIndex.canonicalIdBySubBlockId\n'credential' → 'oauthCredential'"| C{"SELECTOR_CONTEXT_FIELDS\n.has(canonicalKey)?"}
    C -->|"yes"| D["SelectorContext\n{ oauthCredential: value, ... }"]
    C -->|"no"| E["Skipped"]
    D --> F["resolveSelectorForSubBlock\n(resolution.ts)"]
    D --> G["useSelectorSetup\n(use-selector-setup.ts)"]
    F --> H["SelectorRegistry\nfetchList / fetchById\n(registry.ts)"]
    G --> H
    H -->|"context.oauthCredential → API calls"| I["External Service API\n(Slack, Notion, Google, etc.)"]
Loading

Comments Outside Diff (1)

  1. apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/workflow-block.tsx, line 534 (link)

    Stale local variable name not updated

    The rest of this PR renames credentialIdoauthCredential, but this local variable was not renamed. While functionally correct (the value is passed under the right prop name on line 581), the old name makes the code harder to follow and is the only leftover of the old naming in this file.

    Then update the usage on line 581:

    oauthCredential: typeof oauthCredential === 'string' ? oauthCredential : undefined,

Last reviewed commit: c1c6ed6

@icecrasher321
Copy link
Collaborator Author

bugbot run

@icecrasher321
Copy link
Collaborator Author

bugbot run

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@icecrasher321 icecrasher321 merged commit e6a5e7f into staging Mar 7, 2026
12 checks passed
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