Skip to content

fix(services): memoize getCliId and getDevHubId to prevent duplicate telemetry - W-22975411#7460

Draft
mshanemc wants to merge 5 commits into
developfrom
sm/W-22975411-getcliid-effect-cached-rebuilt-per-call
Draft

fix(services): memoize getCliId and getDevHubId to prevent duplicate telemetry - W-22975411#7460
mshanemc wants to merge 5 commits into
developfrom
sm/W-22975411-getcliid-effect-cached-rebuilt-per-call

Conversation

@mshanemc

Copy link
Copy Markdown
Contributor

Summary

  • Memoize getCliId at module scope so sf telemetry --json runs once per session instead of once per getConnection call
  • Memoize getDevHubId per username using Effect.cachedFunction so AuthInfo.create runs once per distinct devhub instead of rebuilding on every call
  • Both functions previously rebuilt Effect.cached per call, defeating memoization and emitting duplicate spans

Plan

.claude/plans/W-22975411.md

Reviewer notes

  • Low severity: connectionService.ts:359 finding (effect LS effectSucceedWithVoid → use Effect.void) NOT applied: conflicts with the four cliTelemetry findings that mandate keeping Effect.succeed(undefined) for type consistency. Line 359 is the else branch of buildDevHubId's ternary; its success value is consumed as devHubOrgId (string | undefined). Changing it to Effect.void would widen the success type to string | void and break DefaultOrgInfoSchema/devHubOrgId typing. The LS message is non-blocking (exit 0, 'message' not warning/error) and is an inherent false positive here because the undefined value is meaningful. The same effectSucceedWithVoid message now also appears on the new line 362 catchAll(() => Effect.succeed(undefined)) for the same reason. Decision: design conflict between findings, surfaced per rules.
  • Low severity: No automated/behavioral test for memoization. No unit test covers cliTelemetry/getDevHubId memoization and CI does not enable span-file-export. Recommend manual verification: enable span file export, trigger multiple getConnection calls in a session, confirm exactly one fetchCliId span and one getDevHubId span per distinct devhub username. PR-body note only.
  • Low severity: PostToolUse hook requested invoking a 'doc-maintenance' subagent for the two edited files in packages/salesforcedx-vscode-services/src. As a subagent I cannot spawn another subagent; doc updates (if any) were not run. Parent may need to run doc-maintenance over connectionService.ts and cliTelemetry.ts.

Test plan

  • effect-language-service diagnostics clean for both edited files
  • typecheck/lint pass for salesforcedx-vscode-services
  • existing unit tests pass
  • e2e/behavioral check: exercise a session with multiple getConnection calls (switch default org back and forth) and confirm exactly one fetchCliId span and one getDevHubId span per distinct devhub username via span-file-export locally or app-insights span-count query post-merge

What issues does this PR fix or reference?

@W-22975411@

🤖 Generated by auto-build pipeline. Original WI: W-22975411

mshanemc and others added 5 commits June 13, 2026 11:49
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…once - W-22975411

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ns once per devhub - W-22975411

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ookups retry - W-22975411

A failed AuthInfo.create was permanently cached per username for the
session via Effect.cachedFunction, so devHubOrgId never recovered after
the user authenticated the devhub. Catch failures inside buildDevHubId
before memoizing so only successes are cached.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cceed(undefined) - W-22975411

Replace untyped catch (e => e) with a tagged FetchCliIdError so the
error channel is typed. Revert the web branch from Effect.void back to
Effect.succeed(undefined) for type consistency with the node branch
(Effect<string | undefined>).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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