feat: Implementing trace of graphql executions#3215
Open
ommeirelles wants to merge 19 commits intocanary-v2from
Open
feat: Implementing trace of graphql executions#3215ommeirelles wants to merge 19 commits intocanary-v2from
ommeirelles wants to merge 19 commits intocanary-v2from
Conversation
ommeirelles
requested review from
hellofanny and
renatomaurovtex
and removed request for
a team
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new diagnostics package and integrates OpenTelemetry across the stack: adds telemetry client/trace factories, resolver-level tracing, async context factory initialization, global diagnostics state, build/config updates, and test adjustments to await the async factory. Changes
Sequence DiagramsequenceDiagram
participant Client as GraphQL Client
participant Server as GraphQL Server
participant ResolverTrace as ResolverTrace HOF
participant TraceClient as TraceClient
participant OTEL as OpenTelemetry API
participant Resolver as Original Resolver
Client->>Server: Send GraphQL request
Server->>ResolverTrace: Call wrapped resolver
ResolverTrace->>TraceClient: extract context (context.OTEL) or use OTEL.context.active()
alt OTEL enabled
TraceClient->>OTEL: Start span (name, attrs)
end
ResolverTrace->>Resolver: Invoke original resolver(source, vars, context, info)
alt returns Promise
Resolver-->>ResolverTrace: Promise
ResolverTrace->>OTEL: End span on resolution
ResolverTrace-->>Server: Resolved value
else sync
Resolver-->>ResolverTrace: Return value
ResolverTrace->>OTEL: End span
ResolverTrace-->>Server: Return value
end
Server-->>Client: Respond
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
ommeirelles
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/src/platforms/vtex/index.ts (1)
43-65:⚠️ Potential issue | 🔴 CriticalBug:
idis generated once per factory, not per request — all requests share the same ID.
crypto.randomBytes(32)on line 49 executes in the outer factory scope. The inner closure (line 50) is what runs per-request, but it reuses the sameid. This contradicts the "Request scope ID" comment on line 21. Moveidgeneration inside the returned function:Proposed fix
export const GraphqlVtexContextFactory = (options: Options) => { getTelemetryClient({ name, version, account: options.account, }) - const id = crypto.randomBytes(32).toString('hex') return (ctx: any): GraphqlContext => { - ctx.id = id + ctx.id = crypto.randomBytes(32).toString('hex') ctx.storage = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 43 - 65, The factory GraphqlVtexContextFactory currently generates id once in the outer scope (const id = crypto.randomBytes(32).toString('hex')) so every request shares the same id; move the id generation into the returned function so that inside the returned (ctx: any): GraphqlContext => { ... } you create a fresh id (e.g., const id = crypto.randomBytes(32).toString('hex')) and then assign ctx.id = id, removing the outer const id to ensure a unique request-scoped id for each ctx.
🧹 Nitpick comments (9)
packages/diagnostics/vite.config.mts (1)
21-23:dependencies ?? {}is a redundant null guard.
dependenciesis already initialized with= {}on line 6, so it can never beundefinedhere.♻️ Proposed fix
external: [ ...builtinModules.concat(builtinModules.map((e) => `node:${e}`)), - ...Object.keys({ - ...(dependencies ?? {}), - }), + ...Object.keys(dependencies), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/vite.config.mts` around lines 21 - 23, The spread uses a redundant nullish fallback because the variable dependencies is initialized to {} earlier; update the Object.keys spread to use dependencies directly (remove the "?? {}" guard) so the line becomes Object.keys({ ...(dependencies) }) or simply Object.keys(dependencies) as appropriate, ensuring you reference the existing dependencies variable used on line 6 and the Object.keys(...) expression to locate and modify the code.packages/diagnostics/tsconfig.json (1)
4-4:outDir: "dist/esm"doesn't match Vite build output paths.The Vite config outputs to
dist/es/anddist/cjs/, notdist/esm/. Sincevite-plugin-dtsdrives the declaration output (nottsc), this is non-functional, but it's misleading for anyone running a baretsccheck on this package.♻️ Suggested fix
- "outDir": "dist/esm", + "outDir": "dist",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/tsconfig.json` at line 4, The tsconfig.json currently sets "outDir": "dist/esm" which mismatches the Vite outputs and is misleading; update the tsconfig.json's outDir key to "dist/es" to match Vite's ES output (or remove the outDir entry entirely if you prefer to avoid any tsc emit assumptions since vite-plugin-dts drives declaration output) so that the "outDir" setting reflects the actual build paths used by the project.packages/api/src/typings/index.ts (1)
32-37:infoparameter should beGraphQLResolveInfo, notany.
infocarries the full GraphQL resolution context (field name, parent type, path, schema, variables). Typing it asanyopts out of type safety for every caller and forfeits valuable metadata for tracing.GraphQLResolveInfois available through the existinggraphqlpeer dependency.♻️ Proposed fix
+import type { GraphQLResolveInfo } from 'graphql' type Resolver< TContext extends Record<string, any>, TSource = any, TVars = any, TReturn = any, -> = (source: TSource, vars: TVars, context: TContext, info: any) => TReturn +> = (source: TSource, vars: TVars, context: TContext, info: GraphQLResolveInfo) => TReturnAs per coding guidelines: "Type safety with generated types" and "Ensure type safety and avoid type assertions when possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/typings/index.ts` around lines 32 - 37, The Resolver type currently types the fourth parameter as any; change it to GraphQLResolveInfo to preserve GraphQL metadata and enable type safety: import { GraphQLResolveInfo } from "graphql" at the top of the file and update the Resolver signature to (source: TSource, vars: TVars, context: TContext, info: GraphQLResolveInfo) => TReturn (keeping the generics intact); ensure the graphql peer dependency is used (no type assertions needed) so callers get proper info typing.packages/diagnostics/package.json (1)
12-12:license: "ISC"is inconsistent with the rest of the monorepo.Every other
@faststore/*package uses"MIT". Consider aligning unless ISC is deliberate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/package.json` at line 12, The package's license field in its package.json is set to "ISC" while the monorepo uses "MIT"; update the "license" property from "ISC" to "MIT" in this package's package.json so it matches other `@faststore` packages, then run your repo's license/validation script (or npm install/test) to confirm no further inconsistencies.packages/api/src/platforms/vtex/index.ts (2)
67-71: Remove commented-out code.Dead code adds noise. If
VtexResolveris needed later, it can be retrieved from git history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 67 - 71, Remove the commented-out VtexResolver block: delete the entire commented code that defines VtexResolver (the lines referencing VtexResolver, GraphqlResolver, ResolverTrace, and GraphqlContext) so the file no longer contains this dead code; if needed later it can be restored from git history.
80-96:finalResolvers: anydrops type safety; variablenameshadows module-level import.Two concerns in
getResolvers:
- Line 83:
finalResolvers: anydiscards the resolver type structure. Use a more descriptive intermediate type or infer fromnativeGetResolvers().- Line 86: The destructured
namein.map(([name, resolverFunction])shadows thenameimport frompackage.json(line 5). Rename to e.g.resolverNamefor clarity.Proposed fix
export function getResolvers() { const resolvers = nativeGetResolvers() - const finalResolvers: any = {} + const finalResolvers: Record<string, Record<string, unknown>> = {} for (const [key, resolver] of Object.entries(resolvers)) { finalResolvers[key] = Object.fromEntries( - Object.entries(resolver).map(([name, resolverFunction]) => { - if (typeof resolverFunction !== 'function') - return [name, resolverFunction] + Object.entries(resolver).map(([resolverName, resolverFunction]) => { + if (typeof resolverFunction !== 'function') + return [resolverName, resolverFunction] - return [name, ResolverTrace(resolverFunction, `${key}(${name})`)] + return [resolverName, ResolverTrace(resolverFunction, `${key}(${resolverName})`)] }) ) }As per coding guidelines, "Ensure type safety and avoid type assertions when possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 80 - 96, getResolvers currently weakens types by declaring finalResolvers: any and also shadows the module-level import name via the map destructuring; update getResolvers to infer types from nativeGetResolvers() (e.g., let resolvers = nativeGetResolvers(); use const finalResolvers: Partial<typeof resolvers> = {} or an appropriate mapped type) and replace the destructured variable name in Object.entries(...).map(([name, resolverFunction]) => ...) with a non-conflicting identifier like resolverName, then assign ResolverTrace(resolverFunction, `${key}(${resolverName})`) so type safety is preserved and the imported name is not shadowed.packages/diagnostics/src/index.ts (2)
1-11: Namespace re-exports defeat tree-shaking — significant bundle size impact.
import *of@opentelemetry/apiand@opentelemetry/semantic-conventions, then re-exporting asOTELAPIandCONVENTIONS, forces bundlers to include the entire modules. Downstream code only uses a handful of symbols (context,SpanKind,ATTR_CODE_FUNCTION_NAME,ATTR_SERVICE_NAME,ATTR_SERVICE_VERSION).Consider re-exporting only the specific symbols needed, or let consumers import directly from
@opentelemetry/*packages.As per coding guidelines, "Consider bundle size impact of dependencies".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/src/index.ts` around lines 1 - 11, The current namespace re-exports (OTELAPI and CONVENTIONS) pull in entire `@opentelemetry` packages and defeat tree-shaking; replace the wildcard imports and re-exports with named exports for only the symbols actually used (e.g., export { context, SpanKind } from '@opentelemetry/api' and export { ATTR_CODE_FUNCTION_NAME, ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION } from '@opentelemetry/semantic-conventions'), or remove the OTELAPI/CONVENTIONS exports and let consumers import directly; update any internal references to OTELAPI or CONVENTIONS to use the named symbols or the original package imports to restore tree-shaking and reduce bundle size.
13-18: Redundant default export alongside named exports.The default export duplicates everything already available via named exports. This creates two ways to import the same thing with no benefit, and makes tree-shaking harder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/src/index.ts` around lines 13 - 18, Remove the redundant default export object that mirrors the named exports; delete the default export block that returns OTELAPI, CONVENTIONS, getTelemetryClient, and getTraceClient and rely solely on the existing named exports. Ensure the named exports OTELAPI, CONVENTIONS, getTelemetryClient, and getTraceClient are exported where they are defined (or add explicit export statements) so consumers import them by name and tree-shaking works correctly.packages/api/src/observability/telemetry.ts (1)
7-14: Return typeTReturnis inaccurate when wrapping async resolvers.When
fnreturnsPromise<X>(soTReturn = Promise<X>), the.then()on line 49 also producesPromise<X>, so theas TReturncast on line 52 happens to be correct at runtime. However, the outer function's declared return type isTReturn, yetOTELAPI.context.with(activeContext, callback)returns whatever the callback returns — the TypeScript compiler can't verify the span-wrapping logic preserves the type. This is acceptable for now but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/observability/telemetry.ts` around lines 7 - 14, The ResolverTrace wrapper's declared return type TReturn can diverge from the actual return of the wrapped function (especially when the wrapped function returns a Promise); update the ResolverTrace signature to take the resolver function as a generic function type (e.g., F extends (source: TSource, vars: TVars, context: TContext, info: any) => any) and use ReturnType<F> as the wrapper's return type so the compiler preserves the exact return (Promise or non-Promise) of the original fn; locate the ResolverTrace declaration and replace the TReturn generic with an F generic and use ReturnType<F> for the outer function's return type and for internal casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/package.json`:
- Around line 77-78: Remove the unnecessary explicit peerDependency entry for
"@vtex/diagnostics-nodejs" from the package.json for the `@faststore/api` package:
delete the "@vtex/diagnostics-nodejs": "^5.1.1" line from peerDependencies
(since `@faststore/diagnostics` already depends on it and the code in
`@faststore/api` does not import it), then update lockfiles/install
(npm/yarn/pnpm) as needed to ensure the dependency is resolved transitively and
run tests/build to confirm nothing breaks.
In `@packages/api/src/observability/telemetry.ts`:
- Around line 40-52: The span is not ended when fn throws synchronously or when
the returned promise rejects; wrap the invocation of fn (the call that assigns
returnedValue inside the context.with callback) in a try/catch so that
span.end() is called in the catch before rethrowing synchronous errors, and for
the async case replace the current then-only logic with a finally (or attach
both then and catch) on the Promise (e.g.,
Promise.resolve(returnedValue).finally(...)) to ensure span.end() runs on
resolve or rejection; reference the variables/functions returnedValue, fn, span
and the context.with callback when making the changes.
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 44-48: getTelemetryClient is called without awaiting so the trace
client (TRACE_CLIENT) isn't initialized before withTraceClient runs; make
GraphqlVtexContextFactory async and await getTelemetryClient(...) inside it so
the telemetry client is ready when GraphqlVtexContextFactory returns, and then
update the caller that invokes this factory (the place where withTraceClient is
composed with the factory) to await the async GraphqlVtexContextFactory so OTEL
context is populated for early requests.
In `@packages/core/package.json`:
- Line 66: Remove the direct dependency entry for "@vtex/diagnostics-nodejs"
from the dependencies list in package.json (the explicit line that reads
"@vtex/diagnostics-nodejs": "^5.1.1") because it is already provided
transitively by "@faststore/diagnostics"; update the dependencies section to
delete that line so only "@faststore/diagnostics" remains responsible for
bringing in that package.
In `@packages/core/src/instrumentation.ts`:
- Around line 1-11: The register function can throw from its dynamic imports
(package.json, discovery.config, or `@faststore/diagnostics`) and must not crash
Next.js on startup; update the async function register to check
process.env.NEXT_RUNTIME as before but wrap the entire import/initialization
block in a try/catch, gracefully short-circuit on error (do not rethrow), and
emit a safe error message (e.g., via console.error or a fallback logger) so
telemetry failures do not prevent the app from starting; reference the register
function and the three dynamic imports (import('../package.json'),
import('../discovery.config'), import('@faststore/diagnostics')) when making the
change.
In `@packages/core/src/server/options.ts`:
- Around line 26-36: Change the generic and return typing and fix the
import/variable typo: make withTraceClient generic constrained to objects (e.g.
function withTraceClient<T extends object>(apiOptions: T): Promise<T & { OTEL:
unknown }>) so spreading ...apiOptions is type-safe and the return reflects the
added OTEL property; remove the useless optional chain on the dynamic import
(use await import('@faststore/diagnostics') instead of (await
import(...))?.getTraceClient()) so import errors propagate, rename traceCLient
to traceClient, call const traceClient = (await import(...)).getTraceClient();
and keep a defensive traceClient?.inject(OTEL) call for when getTraceClient()
returns undefined.
In `@packages/diagnostics/package.json`:
- Around line 1-33: The package.json for `@faststore/diagnostics` is missing a
publishConfig entry which causes npm to publish the scoped package as
restricted; update packages/diagnostics/package.json (package name
"@faststore/diagnostics") to include "publishConfig": { "access": "public" } at
the top level of the JSON so it mirrors other `@faststore/`* packages and can be
published successfully.
In `@packages/diagnostics/src/start.ts`:
- Around line 22-25: The traces exporter is forcing TLS off by hardcoding
insecure: true in the Exporters.CreateTracesExporterConfig call (tracesConfig) —
change this to derive from an environment variable instead (for example check
process.env.NODE_ENV !== 'production' or a dedicated OTLP_INSECURE flag) and
pass that boolean into the insecure option when calling
Exporters.CreateTracesExporterConfig( { endpoint: OTLP_ENDPOINT, insecure:
<derivedBool> } ) so production defaults to secure/TLS while non-production can
disable it via env.
- Line 60: The console.log call that prints "TELEMETRY CLIENT STARTED" with the
opt object should be removed or converted to structured debug logging: replace
the console.log('TELEMETRY CLIENT STARTED', opt) usage with a debug-level logger
(e.g., processLogger.debug or the project's logger) and ensure you do not emit
sensitive fields from the opt object—either omit or redact opt.account before
logging (or only log a non-identifying subset). Locate the console.log in
start.ts where the opt variable is used and change it to a debug-level,
structured log that excludes or masks the account identifier.
- Around line 32-63: getTelemetryClient currently recreates and overwrites the
global TELEMETRY_CLIENT and TRACE_CLIENT on every call; add an idempotency guard
at the start of getTelemetryClient that returns the existing TELEMETRY_CLIENT
immediately if it's already initialized (so you don't call NewTelemetryClient,
setupTracesExporter, newLogsClient/newMetricsClient/newTracesClient or
registerInstrumentations again). Locate getTelemetryClient and reference the
TELEMETRY_CLIENT and TRACE_CLIENT globals; implement an early return using the
same function signature and ensure you do not re-run setupTracesExporter,
NewTelemetryClient, or Instrumentation.CommonInstrumentations.minimal() when
TELEMETRY_CLIENT is present.
- Around line 51-52: The code incorrectly reuses tracesExporter (from
Exporters.CreateTracesExporterConfig) for logs and metrics; replace those calls
to create and pass signal-specific exporters by creating a logsExporter via
Exporters.CreateLogsExporterConfig() and a metricsExporter via
Exporters.CreateMetricsExporterConfig(), then call
TELEMETRY_CLIENT.newLogsClient({ exporter: logsExporter }) and
TELEMETRY_CLIENT.newMetricsClient({ exporter: metricsExporter }) while leaving
the traces client using tracesExporter.
In `@packages/diagnostics/vite.config.mts`:
- Around line 15-17: The Vite library config currently uses fileName:
'[format]/index' which makes Vite emit index.cjs for the 'cjs' format while
package.json's "require" expects ./dist/cjs/index.js; update the Vite config's
fileName to a function that returns a path forcing .js for the 'cjs' format and
.mjs for the ES format (i.e., return 'cjs/index.js' for format === 'cjs' and
'es/index.mjs' otherwise) so the emitted files match package.json, or
alternatively update package.json's "require" to point to ./dist/cjs/index.cjs
if you prefer Vite's default naming.
---
Outside diff comments:
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 43-65: The factory GraphqlVtexContextFactory currently generates
id once in the outer scope (const id = crypto.randomBytes(32).toString('hex'))
so every request shares the same id; move the id generation into the returned
function so that inside the returned (ctx: any): GraphqlContext => { ... } you
create a fresh id (e.g., const id = crypto.randomBytes(32).toString('hex')) and
then assign ctx.id = id, removing the outer const id to ensure a unique
request-scoped id for each ctx.
---
Nitpick comments:
In `@packages/api/src/observability/telemetry.ts`:
- Around line 7-14: The ResolverTrace wrapper's declared return type TReturn can
diverge from the actual return of the wrapped function (especially when the
wrapped function returns a Promise); update the ResolverTrace signature to take
the resolver function as a generic function type (e.g., F extends (source:
TSource, vars: TVars, context: TContext, info: any) => any) and use
ReturnType<F> as the wrapper's return type so the compiler preserves the exact
return (Promise or non-Promise) of the original fn; locate the ResolverTrace
declaration and replace the TReturn generic with an F generic and use
ReturnType<F> for the outer function's return type and for internal casts.
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 67-71: Remove the commented-out VtexResolver block: delete the
entire commented code that defines VtexResolver (the lines referencing
VtexResolver, GraphqlResolver, ResolverTrace, and GraphqlContext) so the file no
longer contains this dead code; if needed later it can be restored from git
history.
- Around line 80-96: getResolvers currently weakens types by declaring
finalResolvers: any and also shadows the module-level import name via the map
destructuring; update getResolvers to infer types from nativeGetResolvers()
(e.g., let resolvers = nativeGetResolvers(); use const finalResolvers:
Partial<typeof resolvers> = {} or an appropriate mapped type) and replace the
destructured variable name in Object.entries(...).map(([name, resolverFunction])
=> ...) with a non-conflicting identifier like resolverName, then assign
ResolverTrace(resolverFunction, `${key}(${resolverName})`) so type safety is
preserved and the imported name is not shadowed.
In `@packages/api/src/typings/index.ts`:
- Around line 32-37: The Resolver type currently types the fourth parameter as
any; change it to GraphQLResolveInfo to preserve GraphQL metadata and enable
type safety: import { GraphQLResolveInfo } from "graphql" at the top of the file
and update the Resolver signature to (source: TSource, vars: TVars, context:
TContext, info: GraphQLResolveInfo) => TReturn (keeping the generics intact);
ensure the graphql peer dependency is used (no type assertions needed) so
callers get proper info typing.
In `@packages/diagnostics/package.json`:
- Line 12: The package's license field in its package.json is set to "ISC" while
the monorepo uses "MIT"; update the "license" property from "ISC" to "MIT" in
this package's package.json so it matches other `@faststore` packages, then run
your repo's license/validation script (or npm install/test) to confirm no
further inconsistencies.
In `@packages/diagnostics/src/index.ts`:
- Around line 1-11: The current namespace re-exports (OTELAPI and CONVENTIONS)
pull in entire `@opentelemetry` packages and defeat tree-shaking; replace the
wildcard imports and re-exports with named exports for only the symbols actually
used (e.g., export { context, SpanKind } from '@opentelemetry/api' and export {
ATTR_CODE_FUNCTION_NAME, ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION } from
'@opentelemetry/semantic-conventions'), or remove the OTELAPI/CONVENTIONS
exports and let consumers import directly; update any internal references to
OTELAPI or CONVENTIONS to use the named symbols or the original package imports
to restore tree-shaking and reduce bundle size.
- Around line 13-18: Remove the redundant default export object that mirrors the
named exports; delete the default export block that returns OTELAPI,
CONVENTIONS, getTelemetryClient, and getTraceClient and rely solely on the
existing named exports. Ensure the named exports OTELAPI, CONVENTIONS,
getTelemetryClient, and getTraceClient are exported where they are defined (or
add explicit export statements) so consumers import them by name and
tree-shaking works correctly.
In `@packages/diagnostics/tsconfig.json`:
- Line 4: The tsconfig.json currently sets "outDir": "dist/esm" which mismatches
the Vite outputs and is misleading; update the tsconfig.json's outDir key to
"dist/es" to match Vite's ES output (or remove the outDir entry entirely if you
prefer to avoid any tsc emit assumptions since vite-plugin-dts drives
declaration output) so that the "outDir" setting reflects the actual build paths
used by the project.
In `@packages/diagnostics/vite.config.mts`:
- Around line 21-23: The spread uses a redundant nullish fallback because the
variable dependencies is initialized to {} earlier; update the Object.keys
spread to use dependencies directly (remove the "?? {}" guard) so the line
becomes Object.keys({ ...(dependencies) }) or simply Object.keys(dependencies)
as appropriate, ensuring you reference the existing dependencies variable used
on line 6 and the Object.keys(...) expression to locate and modify the code.
Comment on lines
40
to
52
| const returnedValue = fn(source, vars, graphqlContext, info) | ||
|
|
||
| if (!span) return returnedValue | ||
|
|
||
| if (!(returnedValue instanceof Promise)) { | ||
| span.end() | ||
| return returnedValue | ||
| } | ||
|
|
||
| return returnedValue.then((value) => { | ||
| span.end() | ||
| return value | ||
| }) as TReturn |
There was a problem hiding this comment.
Span is never ended on errors — both sync throws and promise rejections leak spans.
If fn() throws synchronously (line 40), execution exits the context.with callback without calling span.end(). Similarly, if the returned promise rejects, there's no .catch() to end the span. Leaked spans corrupt trace data and can cause memory pressure in the exporter.
Proposed fix
- const returnedValue = fn(source, vars, graphqlContext, info)
+ let returnedValue: TReturn
+ try {
+ returnedValue = fn(source, vars, graphqlContext, info)
+ } catch (err) {
+ span?.setStatus({ code: OTELAPI.SpanStatusCode.ERROR })
+ span?.end()
+ throw err
+ }
if (!span) return returnedValue
if (!(returnedValue instanceof Promise)) {
span.end()
return returnedValue
}
- return returnedValue.then((value) => {
- span.end()
- return value
- }) as TReturn
+ return returnedValue
+ .then((value) => {
+ span.end()
+ return value
+ })
+ .catch((err) => {
+ span.setStatus({ code: OTELAPI.SpanStatusCode.ERROR })
+ span.end()
+ throw err
+ }) as TReturnAs per coding guidelines, "Proper error handling and validation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const returnedValue = fn(source, vars, graphqlContext, info) | |
| if (!span) return returnedValue | |
| if (!(returnedValue instanceof Promise)) { | |
| span.end() | |
| return returnedValue | |
| } | |
| return returnedValue.then((value) => { | |
| span.end() | |
| return value | |
| }) as TReturn | |
| let returnedValue: TReturn | |
| try { | |
| returnedValue = fn(source, vars, graphqlContext, info) | |
| } catch (err) { | |
| span?.setStatus({ code: OTELAPI.SpanStatusCode.ERROR }) | |
| span?.end() | |
| throw err | |
| } | |
| if (!span) return returnedValue | |
| if (!(returnedValue instanceof Promise)) { | |
| span.end() | |
| return returnedValue | |
| } | |
| return returnedValue | |
| .then((value) => { | |
| span.end() | |
| return value | |
| }) | |
| .catch((err) => { | |
| span.setStatus({ code: OTELAPI.SpanStatusCode.ERROR }) | |
| span.end() | |
| throw err | |
| }) as TReturn |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/observability/telemetry.ts` around lines 40 - 52, The span
is not ended when fn throws synchronously or when the returned promise rejects;
wrap the invocation of fn (the call that assigns returnedValue inside the
context.with callback) in a try/catch so that span.end() is called in the catch
before rethrowing synchronous errors, and for the async case replace the current
then-only logic with a finally (or attach both then and catch) on the Promise
(e.g., Promise.resolve(returnedValue).finally(...)) to ensure span.end() runs on
resolve or rejection; reference the variables/functions returnedValue, fn, span
and the context.with callback when making the changes.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/api/src/platforms/vtex/index.ts (1)
43-65:⚠️ Potential issue | 🔴 Critical
idis generated once at factory creation — all requests share the same ID.
crypto.randomBytes(32)on line 49 runs once whenGraphqlVtexContextFactoryis awaited during server init. The returned closure reuses this singleidfor every incoming request. Ifidis meant to be a per-request correlation identifier (as the JSDoc "Request scope ID" on line 21 suggests), it needs to move inside the returned function.Proposed fix
export const GraphqlVtexContextFactory = async (options: Options) => { await getTelemetryClient({ name, version, account: options.account, }) - const id = crypto.randomBytes(32).toString('hex') return (ctx: any): GraphqlContext => { - ctx.id = id + ctx.id = crypto.randomBytes(32).toString('hex') ctx.storage = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 43 - 65, The factory currently generates a single `id` once in GraphqlVtexContextFactory and reuses it for all requests; move the `crypto.randomBytes(32).toString('hex')` call into the returned closure so a fresh request-scoped ID is assigned to `ctx.id` on each invocation (update the returned function that sets `ctx.id`, leaving other initializations like `ctx.storage`, `ctx.clients`, `ctx.loaders`, `ctx.account`, and `ctx.OTEL` unchanged).packages/api/local/server.mjs (1)
11-23:⚠️ Potential issue | 🟡 MinorMissing
OTELproperty in local serverapiOptions.
GraphqlVtexContextFactorynow assignsctx.OTEL = options.OTEL. WithoutOTELhere,ctx.OTELwill beundefined, which violates theRecord<string, unknown>contract onGraphqlContext. The core options (inpackages/core/src/server/options.ts) already includeOTEL: {}. This local server should match.Proposed fix
const apiOptions = { platform: 'vtex', account: 'storeframework', locale: 'en-US', environment: 'vtexcommercestable', channel: '{"salesChannel":"1","regionId":"","hasOnlyDefaultSalesChannel":"true"}', showSponsored: false, + OTEL: {}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/local/server.mjs` around lines 11 - 23, The apiOptions object used to build context for GraphqlVtexContextFactory is missing the OTEL property, causing ctx.OTEL to be undefined and violating the GraphqlContext contract; update the apiOptions constant to include an OTEL field (e.g., OTEL: {}) so GraphqlVtexContextFactory receives OTEL and ctx.OTEL is defined when creating the Yoga server with GraphqlVtexSchema() and GraphqlVtexContextFactory(apiOptions).
🧹 Nitpick comments (5)
packages/api/vite.config.mts (1)
12-12: Consider external source maps (true) instead of'inline'for non-production.
'inline'embeds the full base64-encoded source map directly into every output file, which can balloon dist artifacts significantly in dev/watch mode. Usingtrue(or'hidden') generates a separate.mapfile instead, keeping the bundled output lean while preserving debuggability.♻️ Proposed adjustment
- sourcemap: mode === 'production' ? 'hidden' : 'inline', + sourcemap: mode === 'production' ? 'hidden' : true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/vite.config.mts` at line 12, The sourcemap config currently uses 'inline' for non-production which embeds base64 maps into files; update the sourcemap expression in vite.config.mts (the sourcemap option) to generate external maps for dev by returning true instead of 'inline' (e.g., keep production as 'hidden' but change the non-production branch to true) so source maps are emitted as separate .map files and dist artifacts stay lean.packages/core/src/server/options.ts (1)
27-36:as Thides the OTEL augmentation and the generic is unconstrained — type safety concern.The function always adds an
OTELproperty to the return value, but the return typePromise<T>doesn't reflect that. Meanwhile,Thas noextends objectconstraint, so spreading...apiOptionsis only safe by accident. A constrained return type would be more honest:-export async function withTraceClient<T = typeof apiOptions>( - apiOptions: T -): Promise<T> { +export async function withTraceClient<T extends Record<string, unknown> = typeof apiOptions>( + options: T +): Promise<T & { OTEL: Record<string, unknown> }> { const OTEL = {} getTraceClient(name)?.inject(OTEL) return { - ...apiOptions, + ...options, OTEL, - } as T + } }This also renames the parameter to avoid shadowing the module-level
apiOptions. As per coding guidelines, "Ensure type safety and avoid type assertions when possible".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server/options.ts` around lines 27 - 36, Change withTraceClient to accurately reflect that it augments the input options with an OTEL field and to constrain the generic: make the type parameter T extend object (e.g., <T extends object>) and change the return type to Promise<T & { OTEL: Record<string, unknown> }> (or a more specific OTEL type), rename the parameter (e.g., inputOptions) to avoid shadowing the module-level apiOptions, remove the unsafe "as T" cast, and return { ...inputOptions, OTEL } typed as T & { OTEL: ... } so the compiler knows the OTEL augmentation is present; keep the getTraceClient(name)?.inject(OTEL) call unchanged.packages/api/test/integration/queries.test.ts (1)
114-291: Consider extractingcreateRunnerinto a shared test utility.The
createRunnerfunction is essentially identical inqueries.test.tsandmutations.test.ts(sameapiOptionsshape, same schema/context wiring). A shared helper would reduce duplication and make future changes to the runner setup a single-point edit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/test/integration/queries.test.ts` around lines 114 - 291, The tests duplicate the createRunner setup across queries.test.ts and mutations.test.ts; extract it into a shared test utility to DRY the code. Create a new helper (e.g., export function createTestRunner or re-export createRunner) that encapsulates the apiOptions shape and schema/context wiring used in both files, move the existing createRunner implementation into that helper, update queries.test.ts and mutations.test.ts to import and call the shared createTestRunner (replacing their local createRunner), and remove the duplicated local definitions so future changes are made in one place.packages/api/src/platforms/vtex/index.ts (1)
67-71: Remove commented-out dead code.The
VtexResolverfunction is fully commented out. If it's no longer needed, remove it to keep the codebase clean. If it's planned for future use, track it in an issue instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 67 - 71, Remove the dead commented-out VtexResolver block: delete the commented function and its lines referencing VtexResolver, GraphqlResolver, ResolverTrace, and GraphqlContext; if this behavior is required later, open an issue referencing VtexResolver and ResolverTrace instead of leaving the commented code in packages/api/src/platforms/vtex/index.ts.packages/api/src/observability/telemetry.ts (1)
7-14: Consider tightening theinfoparameter type.
info: anyon lines 13 and 20 could beGraphQLResolveInfofrom thegraphqlpackage. This would improve type safety and catch misuse at compile time without any runtime cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/observability/telemetry.ts` around lines 7 - 14, The ResolverTrace function currently types its fourth parameter as info: any; tighten this by importing GraphQLResolveInfo from the graphql package and changing the parameter type in ResolverTrace (and any internal uses) to info: GraphQLResolveInfo so callers and implementations get proper compile-time type checking; update both the function signature (fn: (source, vars, context, info: GraphQLResolveInfo) => TReturn) and the ResolverTrace declaration that references info to use GraphQLResolveInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 80-96: getResolvers() has two issues: the parameter name in the
map ([name, resolverFunction]) shadows the module-level name import and
finalResolvers is typed as any which defeats the final "satisfies typeof
resolvers" check. Fix by renaming the destructured variable to a non-conflicting
identifier (e.g., fieldName) in the Object.entries(...).map call so it doesn't
shadow the imported name, and give finalResolvers a proper type instead of any
(for example derive its shape from nativeGetResolvers() or use Partial<typeof
resolvers> / Record<string, typeof resolver> as appropriate) so the satisfies
typeof resolvers check is meaningful; keep wrapping resolver functions with
ResolverTrace(`${key}(${fieldName})`) and preserve use of nativeGetResolvers and
getResolvers.
In `@packages/api/vite.config.mts`:
- Line 6: The build is externalizing everything from devDependencies which
causes runtime packages to be omitted from the bundle; update the externals
logic in vite.config.mts to only use dependencies and peerDependencies (remove
devDependencies from the externals array/merge) and move runtime packages such
as `@envelop/`*, `@faststore/diagnostics`, `@opentelemetry/api`, graphql,
graphql-yoga, and tslib into package.json dependencies or peerDependencies as
appropriate, keeping only true build-time tools (vite, vitest,
`@graphql-codegen/`*) in devDependencies; reference the existing variables
dependencies, peerDependencies, devDependencies and the externals handling in
vite.config.mts when making the change.
In `@packages/diagnostics/src/config.json`:
- Around line 6-16: The "critical_endpoints" rule currently sets sampleRate: 1.0
which will trace every request to endpoints matching attribute "http.target"
value "^/api/graphql"; reduce the default sampling to a lower value (e.g., 0.1)
or make it environment-dependent by reading an env var and applying it via the
diagnostics config hot-reload mechanism so production uses a lower rate while
allowing full sampling in debug/staging; update the "critical_endpoints" entry
to use the new sampling value or env-backed value and ensure the config reload
logic respects the changed key.
In `@packages/diagnostics/src/start.ts`:
- Around line 54-55: The calls to newLogsClient() and newMetricsClient() are
being invoked without any logs/metrics exporter configuration in config.json, so
they will fall back to defaults; either add explicit logs and metrics exporter
sections to config.json (e.g., specify exporter type and options) or update the
code around newLogsClient() and newMetricsClient() to pass the intended exporter
config/options, and if the clients are intentionally left as no-ops add a
concise comment above the calls referencing newLogsClient() and
newMetricsClient() that this is deliberate and will be configured later so
reviewers understand the intent.
- Line 49: The use of require.resolve for configPath will break in ESM builds;
inside getTelemetryClient replace
require.resolve('@faststore/diagnostics/config') with an ESM-safe resolution
(either create a require via createRequire(import.meta.url).resolve(...) or use
await import.meta.resolve(...)) so the code runs in both CJS and ESM. Update the
top of the module to import createRequire from 'module' if you choose that
approach, or make the resolution async if using import.meta.resolve; ensure the
symbol configPath in getTelemetryClient uses the chosen resolver instead of
require.resolve.
In `@packages/diagnostics/vite.config.mts`:
- Line 10: Remove the unnecessary type assertion by updating the Vite config's
plugins array to use dts() directly; locate the plugins: [dts() as any] entry
and change it to plugins: [dts()] so it matches sibling packages (api,
components, sdk, ui) and removes the redundant cast.
---
Outside diff comments:
In `@packages/api/local/server.mjs`:
- Around line 11-23: The apiOptions object used to build context for
GraphqlVtexContextFactory is missing the OTEL property, causing ctx.OTEL to be
undefined and violating the GraphqlContext contract; update the apiOptions
constant to include an OTEL field (e.g., OTEL: {}) so GraphqlVtexContextFactory
receives OTEL and ctx.OTEL is defined when creating the Yoga server with
GraphqlVtexSchema() and GraphqlVtexContextFactory(apiOptions).
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 43-65: The factory currently generates a single `id` once in
GraphqlVtexContextFactory and reuses it for all requests; move the
`crypto.randomBytes(32).toString('hex')` call into the returned closure so a
fresh request-scoped ID is assigned to `ctx.id` on each invocation (update the
returned function that sets `ctx.id`, leaving other initializations like
`ctx.storage`, `ctx.clients`, `ctx.loaders`, `ctx.account`, and `ctx.OTEL`
unchanged).
---
Duplicate comments:
In `@packages/api/src/observability/telemetry.ts`:
- Around line 50-53: The returnedValue Promise handling in the wrapper doesn't
end the span on rejection, so update the Promise chain for returnedValue (the
variable returned from the resolver) to attach a .catch(...) that calls
span.end() before rethrowing the error; keep the existing .then(...) that ends
the span on success and returns the value, and ensure both success and error
paths always call span.end() (refer to returnedValue, span.end, and the
surrounding resolver wrapper function).
In `@packages/diagnostics/vite.config.mts`:
- Around line 15-17: The bundle is emitting mismatched extensions due to the
static fileName '[format]/index'; update the Vite config (the
entry/formats/fileName block in packages/diagnostics/vite.config.mts) to supply
a fileName function that returns explicit extensions per format (e.g., return
'cjs/index.cjs' for 'cjs' and 'es/index.js' for 'es') so output extensions are
pinned regardless of package.json "type"; after changing fileName, verify
packages/diagnostics/package.json "type" and the "require" export path point to
the correct extension (e.g., ./dist/cjs/index.cjs) and adjust the "require"
field if you prefer Vite's default naming instead.
---
Nitpick comments:
In `@packages/api/src/observability/telemetry.ts`:
- Around line 7-14: The ResolverTrace function currently types its fourth
parameter as info: any; tighten this by importing GraphQLResolveInfo from the
graphql package and changing the parameter type in ResolverTrace (and any
internal uses) to info: GraphQLResolveInfo so callers and implementations get
proper compile-time type checking; update both the function signature (fn:
(source, vars, context, info: GraphQLResolveInfo) => TReturn) and the
ResolverTrace declaration that references info to use GraphQLResolveInfo.
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 67-71: Remove the dead commented-out VtexResolver block: delete
the commented function and its lines referencing VtexResolver, GraphqlResolver,
ResolverTrace, and GraphqlContext; if this behavior is required later, open an
issue referencing VtexResolver and ResolverTrace instead of leaving the
commented code in packages/api/src/platforms/vtex/index.ts.
In `@packages/api/test/integration/queries.test.ts`:
- Around line 114-291: The tests duplicate the createRunner setup across
queries.test.ts and mutations.test.ts; extract it into a shared test utility to
DRY the code. Create a new helper (e.g., export function createTestRunner or
re-export createRunner) that encapsulates the apiOptions shape and
schema/context wiring used in both files, move the existing createRunner
implementation into that helper, update queries.test.ts and mutations.test.ts to
import and call the shared createTestRunner (replacing their local
createRunner), and remove the duplicated local definitions so future changes are
made in one place.
In `@packages/api/vite.config.mts`:
- Line 12: The sourcemap config currently uses 'inline' for non-production which
embeds base64 maps into files; update the sourcemap expression in
vite.config.mts (the sourcemap option) to generate external maps for dev by
returning true instead of 'inline' (e.g., keep production as 'hidden' but change
the non-production branch to true) so source maps are emitted as separate .map
files and dist artifacts stay lean.
In `@packages/core/src/server/options.ts`:
- Around line 27-36: Change withTraceClient to accurately reflect that it
augments the input options with an OTEL field and to constrain the generic: make
the type parameter T extend object (e.g., <T extends object>) and change the
return type to Promise<T & { OTEL: Record<string, unknown> }> (or a more
specific OTEL type), rename the parameter (e.g., inputOptions) to avoid
shadowing the module-level apiOptions, remove the unsafe "as T" cast, and return
{ ...inputOptions, OTEL } typed as T & { OTEL: ... } so the compiler knows the
OTEL augmentation is present; keep the getTraceClient(name)?.inject(OTEL) call
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (7)
packages/diagnostics/vite.config.mts (2)
14-18: CJS output extension will not match the"require"field inpackage.json.This was already raised. Vite emits
.cjsfor thecjsformat when using a string templatefileName, butpackage.jsondeclares"require": "./dist/cjs/index.js". Use a function to control per-format extensions:♻️ Proposed fix
- fileName: '[format]/index', + fileName: (format) => + format === 'cjs' ? 'cjs/index.js' : 'es/index.mjs',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/vite.config.mts` around lines 14 - 18, Vite is emitting .cjs for the "cjs" format because fileName is a string template; update the lib.fileName to a function so you can return the correct extension per format (e.g., for format === 'cjs' return 'cjs/index.js', otherwise return `${format}/index`) so the produced CJS bundle matches the "require" field in package.json (./dist/cjs/index.js); modify the lib config's fileName property accordingly.
10-10: Remove theas anycast fromdts().The unnecessary
as anywas already flagged. Officialvite-plugin-dtsdocs and examples consistently showplugins: [dts()]without any type assertion.♻️ Proposed fix
- plugins: [dts() as any], + plugins: [dts()],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/vite.config.mts` at line 10, Remove the unnecessary type assertion on the dts plugin: replace the plugins entry using "dts() as any" with just "dts()". If TypeScript then complains about plugin array typing, import the Vite Plugin type and type the plugins array (e.g., plugins: Plugin[] or annotate the config via defineConfig) so dts() can be used without "as any"; this change targets the plugins array and the dts() call.packages/diagnostics/package.json (1)
1-42:publishConfigis still missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/package.json` around lines 1 - 42, Package is missing a publishConfig entry in package.json; add a "publishConfig" object to the package manifest (e.g., set "access": "public" or other org-appropriate settings) so the package publishes with the correct registry and access. Update the package.json top-level to include "publishConfig": { "access": "public" } (or your org's registry URL/registry key) adjacent to existing fields like "exports" and "packageManager" to ensure correct publishing behavior.packages/diagnostics/src/start.ts (2)
61-62:newLogsClient()andnewMetricsClient()called without exporter config — results are unused.These calls create clients that fall back to library defaults (likely no-op). If they're intentional placeholders, assign the results to variables or add a comment clarifying intent. Otherwise, remove them to avoid unnecessary overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/src/start.ts` around lines 61 - 62, The calls to client.newLogsClient() and client.newMetricsClient() currently produce unused clients and fall back to library defaults; either remove these calls to avoid unnecessary overhead or explicitly capture and use them (e.g., assign to variables like logsClient and metricsClient) or add a clarifying comment that they are intentional placeholders; update the code around the newLogsClient and newMetricsClient invocations to either (1) remove the calls, (2) assign the returned clients to variables and use or export them, or (3) add a clear inline comment explaining they are intentional no-op/default-initialization placeholders.
73-74:console.logleaksopt.accountin dev mode.The
optobject containsaccount— a customer identifier. Even in dev mode, prefer structured logging that redacts or omits sensitive fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/src/start.ts` around lines 73 - 74, The dev-mode console.log leaks sensitive customer identifiers via the opt object; update the conditional that checks globalThis.fsDiagnostics.IS_DEV to use structured logging and redact or omit opt.account before emitting (e.g., create a sanitizedOpt = { ...opt, account: '[REDACTED]' } or delete sanitizedOpt.account) and pass that sanitized object to your logger instead of console.log so the original opt.account is never printed; locate the check around globalThis.fsDiagnostics.IS_DEV in start.ts and replace the console.log call with a call to your project logger (or console.info) using the sanitized object.packages/api/src/platforms/vtex/index.ts (1)
86-98:nameshadows module-level import;finalResolvers: anydefeatssatisfies.These were flagged previously. The
nameon line 89 shadows thenameimported frompackage.json(line 5), andfinalResolvers: anymakesas typeof resolversa no-op for type checking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 86 - 98, The loop currently shadows the module-level import name and disables type checking by using any; rename the map callback parameter 'name' to something non-conflicting (e.g., fieldName) in the Object.entries(resolver).map(([fieldName, resolverFunction]) => ...) and remove finalResolvers: any by initializing finalResolvers with a proper type (for example const finalResolvers = {} as typeof resolvers or use Partial<typeof resolvers> and cast at the end) so the ResolverTrace wrapping (ResolverTrace and resolvers) preserves static types.packages/api/src/observability/telemetry.ts (1)
54-57:⚠️ Potential issue | 🟠 MajorPromise rejection still leaks the span.
The
try/catchhandles synchronous throws (good), but if the resolver returns a rejected promise,.then()won't fire and there's no.catch()to end the span. This was flagged previously and only partially addressed.Proposed fix
return returnedValue.then((value) => { span.end() return value - }) as TReturn + }).catch((err) => { + span.setStatus?.({ code: OTELAPI.SpanStatusCode.ERROR }) + span.end() + throw err + }) as TReturnAs per coding guidelines, "Proper error handling and validation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/observability/telemetry.ts` around lines 54 - 57, The current code ends the span only on promise fulfillment because it uses returnedValue.then(...), which leaks the span on rejections; update the promise handling around returnedValue and span (the variables in this block) to ensure span.end() is always called (e.g., use returnedValue.finally(() => span.end()) and preserve the original return behavior/cast to TReturn) or attach both .then(...) and .catch(...) where the catch rethrows the error after ending the span so async rejections don't leak the span.
🧹 Nitpick comments (7)
packages/diagnostics/vite.config.mts (2)
24-27:dependencies ?? {}is a no-op — the default is already set on line 6.
const { dependencies = {}, ... }ensuresdependenciesis always{}at minimum; the?? {}on line 25 is unreachable.devDependencies ?? {}on line 26 is correct and necessary since it has no destructuring default.♻️ Proposed fix
...Object.keys({ - ...(dependencies ?? {}), + ...dependencies, ...(devDependencies ?? {}), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/vite.config.mts` around lines 24 - 27, The expression using "dependencies ?? {}" is redundant because dependencies already has a default in the destructuring; remove the "?? {}" so the spread uses "dependencies" directly and keep "devDependencies ?? {}" as-is; locate the object spread around Object.keys(...) (the snippet that currently reads "...Object.keys({ ...(dependencies ?? {}), ...(devDependencies ?? {}), })") and change it to "...Object.keys({ ...(dependencies), ...(devDependencies ?? {}), })" (or alternatively remove the destructuring default and keep both "?? {}" — pick the simpler fix of removing "?? {}" after "dependencies").
9-9: Replace withprocess.cwd()directly.
process.env.PWDis Unix-specific and unreliable across CI environments. While this pattern is used consistently across multiple Vite configs in the workspace (packages/ui,packages/api,packages/sdk,packages/components), the correct approach—as shown inpackages/core/next.config.js—is to useprocess.cwd()directly.♻️ Proposed fix
- root: process.env.PWD ?? process.cwd(), + root: process.cwd(),Consider a coordinated update across all Vite configs if there's a separate refactoring pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/vite.config.mts` at line 9, The Vite config sets root using the Unix-specific expression process.env.PWD ?? process.cwd(); change the assignment to use process.cwd() directly (i.e., root: process.cwd()) in the exported Vite config object to ensure consistent behavior across CI environments; update the same pattern wherever the config assigns root using process.env.PWD to keep configs consistent.packages/diagnostics/package.json (1)
33-33: Drop thestreampolyfill — it's a browser shim and a no-op in Node.js.The npm
streampackage at0.0.3is a browser-compatibility wrapper that simply re-exports the Node.js built-in. This package is a server-side OpenTelemetry package;streamis already available natively in Node.js, so the dependency adds unnecessary installation overhead without any benefit. If a bundler-shim is genuinely required by some transitive tool, move it todevDependencies.♻️ Proposed fix
- "stream": "^0.0.3"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/package.json` at line 33, Remove the unnecessary browser polyfill dependency "stream": "^0.0.3" from the package.json in the diagnostics package; this is a no-op in Node.js, so delete the "stream" entry from the "dependencies" section and, if a bundler shim is truly required only for development/build, add it to "devDependencies" instead (or leave it out entirely) and update package-lock/yarn.lock by reinstalling to ensure lockfile consistency.packages/diagnostics/src/index.ts (1)
2-3: Re-exporting entire OTel namespaces may hinder tree-shaking.
export * as OTELAPIandexport * as CONVENTIONSre-export the full@opentelemetry/apiand@opentelemetry/semantic-conventionsnamespaces. Consumers only use a handful of symbols. If this package is ever consumed client-side, this will bloat bundles. Consider re-exporting only what's needed:export { context, SpanKind, SpanStatusCode } from '@opentelemetry/api' export { ATTR_CODE_FUNCTION_NAME, ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION } from '@opentelemetry/semantic-conventions'Since this is server-side today, this is a nice-to-have.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/src/index.ts` around lines 2 - 3, The current re-exports export * as OTELAPI and export * as CONVENTIONS pull entire `@opentelemetry` namespaces and hurt tree-shaking; replace these broad re-exports with narrow named exports of only the symbols we actually use (e.g., export specific symbols like context, SpanKind, SpanStatusCode from '@opentelemetry/api' and ATTR_CODE_FUNCTION_NAME, ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION from '@opentelemetry/semantic-conventions') so consumers only import required identifiers; update the exports in the module where OTELAPI and CONVENTIONS are declared to list those named symbols instead of exporting the full namespaces.packages/api/src/typings/globals.ts (1)
34-39:Resolvertype usesanyforinfo— considerGraphQLResolveInfo.Using
anyfor theinfoparameter loses type safety on the GraphQL resolve info object. If you want to keep this generic, a default ofGraphQLResolveInfowould be more precise while still allowing override.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/typings/globals.ts` around lines 34 - 39, The Resolver type currently types the info parameter as any; update the signature of Resolver to use GraphQLResolveInfo (from the 'graphql' package) as the default for the info parameter while still allowing overrides, e.g. change the fourth parameter type from any to GraphQLResolveInfo and add the necessary import for GraphQLResolveInfo so Resolver<TContext, TSource, TVars, TReturn> = (source: TSource, vars: TVars, context: TContext, info: GraphQLResolveInfo) => TReturn.packages/api/src/observability/telemetry.ts (1)
26-28: Simplify the OTEL-enabled check.
(graphqlContext?.OTEL?.enabled ?? false) === falseis a double-negative that's harder to parse. Consider:- if ((graphqlContext?.OTEL?.enabled ?? false) === false) { + if (!graphqlContext?.OTEL?.enabled) {Both are functionally equivalent here since
enabledis falsy whenundefined,false,"", or0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/observability/telemetry.ts` around lines 26 - 28, The current double-negative check ((graphqlContext?.OTEL?.enabled ?? false) === false) is confusing; replace it with a simple falsy check using the existing symbols: use if (!graphqlContext?.OTEL?.enabled) to guard the early return in the function that calls fn(source, vars, graphqlContext, info), keeping behavior identical when enabled is undefined or falsy while improving readability.packages/core/src/server/options.ts (1)
38-44:as Tassertion hides the shape change.The returned object overwrites
OTELonT, so the actual return type isOmit<T, 'OTEL'> & { OTEL: {...} }, notT. Theas Tmasks this.Proposed fix
-export async function withTraceClient<T extends APIOptions = typeof apiOptions>( - apiOptions: T -): Promise<T> { +export async function withTraceClient( + apiOptions: APIOptions +): Promise<APIOptions> { const OTEL = {} getTraceClient(name)?.inject(OTEL) return { ...apiOptions, OTEL: { ...OTEL, - enabled: storeConfig.analytics.otelEnabled.toString() === 'true', + enabled: otelEnabled, }, - } as T + } }As per coding guidelines, "Ensure type safety and avoid type assertions when possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server/options.ts` around lines 38 - 44, The code currently forces the return to T with "as T" while overwriting OTEL; remove the unsafe assertion and make the return type explicit as Omit<T, 'OTEL'> & { OTEL: typeof OTEL & { enabled: boolean } } (or create a named type alias for that shape) and return an object typed accordingly (e.g., const result: Omit<T,'OTEL'> & { OTEL: typeof OTEL & { enabled: boolean } } = { ...apiOptions, OTEL: { ...OTEL, enabled: storeConfig.analytics.otelEnabled.toString() === 'true' } }; return result;). Ensure you reference apiOptions, OTEL and storeConfig.analytics.otelEnabled when updating the function signature/return type so the compiler reflects the actual shape instead of masking it with "as T".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/platforms/vtex/index.ts`:
- Line 44: The current implementation of GraphqlVtexContextFactory generates a
single id with crypto.randomBytes(32).toString('hex') at factory creation and
reuses it for every request; move the id generation into the returned closure so
a fresh id is created per invocation. Specifically, remove the top-level const
id and instead call crypto.randomBytes(32).toString('hex') inside the function
returned by GraphqlVtexContextFactory (the closure that builds the request
context) and use that new id when constructing the context values.
In `@packages/api/src/typings/globals.ts`:
- Around line 21-25: Change OTEL.enabled to a canonical boolean type and
normalize any string inputs where the configuration is read: update the OTEL
type definition (symbol OTEL.enabled) to boolean | undefined (or just boolean)
instead of boolean | string | undefined, and in the config loader (the code that
reads options.ts where .toString() is being called) coerce env/raw values to a
boolean (e.g., compare to 'true'/'1' or use a helper parseBoolean) so callers
can safely use boolean methods without guarding for undefined/string; update any
call sites that assumed a string (the use in options.ts that calls .toString())
to use the normalized boolean value instead.
In `@packages/core/src/server/options.ts`:
- Line 42: storeConfig.analytics.otelEnabled can be undefined which causes
.toString() to throw; update the enabled assignment to guard/normalize the value
before calling toString (or avoid toString entirely). Replace the current
expression that uses storeConfig.analytics.otelEnabled.toString() with a safe
conversion such as String(storeConfig.analytics?.otelEnabled) === 'true' or
(storeConfig.analytics?.otelEnabled ?? false) === true so the enabled field in
the options (where `enabled:` is set) never calls .toString() on undefined.
- Line 36: The call getTraceClient(name)?.inject(OTEL) is passing the core
package name so it returns undefined; change the argument to the package name
where the trace client is actually registered (use '@faststore/api') so
getTraceClient('@faststore/api')?.inject(OTEL) will find the client registered
by getTelemetryClient() in packages/api/src/platforms/vtex/index.ts; update the
call in options.ts to use the correct package name (or a shared constant
matching the registration) so inject(OTEL) runs.
- Around line 26-28: Normalize handling of storeConfig.analytics.otelEnabled so
it's converted once to a boolean and reused instead of mixing raw values and a
.toString() === 'true' check; update the OTEL config block (OTEL) to set enabled
to the normalized boolean (e.g., convert string 'true'/'false' or undefined to a
boolean) and replace any other occurrences that do .toString() === 'true' with
the normalized value to ensure consistent behavior across the module.
In `@packages/diagnostics/package.json`:
- Line 12: The package.json in the diagnostics package currently sets "license":
"ISC"; update that license field to "MIT" to match the rest of the monorepo so
downstream audits are consistent—locate the license property in
packages/diagnostics package.json and replace the value "ISC" with "MIT".
In `@packages/diagnostics/src/start.ts`:
- Around line 63-72: The TRACE_CLIENTS map is updated before TELEMETRY_CLIENTS,
creating a partial state if an error occurs; update both maps only after all
setup completes by either moving the
globalThis.fsDiagnostics.TRACE_CLIENTS.set(opt.name, ...) call to after
client.registerInstrumentations(...) and after setting
globalThis.fsDiagnostics.TELEMETRY_CLIENTS, or batch the assignments so both
TRACE_CLIENTS and TELEMETRY_CLIENTS are set atomically (e.g., prepare the new
trace client via client.newTracesClient and only call
globalThis.fsDiagnostics.TRACE_CLIENTS.set and
globalThis.fsDiagnostics.TELEMETRY_CLIENTS.set together once no errors
occurred), referencing client.newTracesClient, client.registerInstrumentations,
globalThis.fsDiagnostics.TRACE_CLIENTS,
globalThis.fsDiagnostics.TELEMETRY_CLIENTS, and opt.name.
---
Duplicate comments:
In `@packages/api/src/observability/telemetry.ts`:
- Around line 54-57: The current code ends the span only on promise fulfillment
because it uses returnedValue.then(...), which leaks the span on rejections;
update the promise handling around returnedValue and span (the variables in this
block) to ensure span.end() is always called (e.g., use returnedValue.finally(()
=> span.end()) and preserve the original return behavior/cast to TReturn) or
attach both .then(...) and .catch(...) where the catch rethrows the error after
ending the span so async rejections don't leak the span.
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 86-98: The loop currently shadows the module-level import name and
disables type checking by using any; rename the map callback parameter 'name' to
something non-conflicting (e.g., fieldName) in the
Object.entries(resolver).map(([fieldName, resolverFunction]) => ...) and remove
finalResolvers: any by initializing finalResolvers with a proper type (for
example const finalResolvers = {} as typeof resolvers or use Partial<typeof
resolvers> and cast at the end) so the ResolverTrace wrapping (ResolverTrace and
resolvers) preserves static types.
In `@packages/diagnostics/package.json`:
- Around line 1-42: Package is missing a publishConfig entry in package.json;
add a "publishConfig" object to the package manifest (e.g., set "access":
"public" or other org-appropriate settings) so the package publishes with the
correct registry and access. Update the package.json top-level to include
"publishConfig": { "access": "public" } (or your org's registry URL/registry
key) adjacent to existing fields like "exports" and "packageManager" to ensure
correct publishing behavior.
In `@packages/diagnostics/src/start.ts`:
- Around line 61-62: The calls to client.newLogsClient() and
client.newMetricsClient() currently produce unused clients and fall back to
library defaults; either remove these calls to avoid unnecessary overhead or
explicitly capture and use them (e.g., assign to variables like logsClient and
metricsClient) or add a clarifying comment that they are intentional
placeholders; update the code around the newLogsClient and newMetricsClient
invocations to either (1) remove the calls, (2) assign the returned clients to
variables and use or export them, or (3) add a clear inline comment explaining
they are intentional no-op/default-initialization placeholders.
- Around line 73-74: The dev-mode console.log leaks sensitive customer
identifiers via the opt object; update the conditional that checks
globalThis.fsDiagnostics.IS_DEV to use structured logging and redact or omit
opt.account before emitting (e.g., create a sanitizedOpt = { ...opt, account:
'[REDACTED]' } or delete sanitizedOpt.account) and pass that sanitized object to
your logger instead of console.log so the original opt.account is never printed;
locate the check around globalThis.fsDiagnostics.IS_DEV in start.ts and replace
the console.log call with a call to your project logger (or console.info) using
the sanitized object.
In `@packages/diagnostics/vite.config.mts`:
- Around line 14-18: Vite is emitting .cjs for the "cjs" format because fileName
is a string template; update the lib.fileName to a function so you can return
the correct extension per format (e.g., for format === 'cjs' return
'cjs/index.js', otherwise return `${format}/index`) so the produced CJS bundle
matches the "require" field in package.json (./dist/cjs/index.js); modify the
lib config's fileName property accordingly.
- Line 10: Remove the unnecessary type assertion on the dts plugin: replace the
plugins entry using "dts() as any" with just "dts()". If TypeScript then
complains about plugin array typing, import the Vite Plugin type and type the
plugins array (e.g., plugins: Plugin[] or annotate the config via defineConfig)
so dts() can be used without "as any"; this change targets the plugins array and
the dts() call.
---
Nitpick comments:
In `@packages/api/src/observability/telemetry.ts`:
- Around line 26-28: The current double-negative check
((graphqlContext?.OTEL?.enabled ?? false) === false) is confusing; replace it
with a simple falsy check using the existing symbols: use if
(!graphqlContext?.OTEL?.enabled) to guard the early return in the function that
calls fn(source, vars, graphqlContext, info), keeping behavior identical when
enabled is undefined or falsy while improving readability.
In `@packages/api/src/typings/globals.ts`:
- Around line 34-39: The Resolver type currently types the info parameter as
any; update the signature of Resolver to use GraphQLResolveInfo (from the
'graphql' package) as the default for the info parameter while still allowing
overrides, e.g. change the fourth parameter type from any to GraphQLResolveInfo
and add the necessary import for GraphQLResolveInfo so Resolver<TContext,
TSource, TVars, TReturn> = (source: TSource, vars: TVars, context: TContext,
info: GraphQLResolveInfo) => TReturn.
In `@packages/core/src/server/options.ts`:
- Around line 38-44: The code currently forces the return to T with "as T" while
overwriting OTEL; remove the unsafe assertion and make the return type explicit
as Omit<T, 'OTEL'> & { OTEL: typeof OTEL & { enabled: boolean } } (or create a
named type alias for that shape) and return an object typed accordingly (e.g.,
const result: Omit<T,'OTEL'> & { OTEL: typeof OTEL & { enabled: boolean } } = {
...apiOptions, OTEL: { ...OTEL, enabled:
storeConfig.analytics.otelEnabled.toString() === 'true' } }; return result;).
Ensure you reference apiOptions, OTEL and storeConfig.analytics.otelEnabled when
updating the function signature/return type so the compiler reflects the actual
shape instead of masking it with "as T".
In `@packages/diagnostics/package.json`:
- Line 33: Remove the unnecessary browser polyfill dependency "stream": "^0.0.3"
from the package.json in the diagnostics package; this is a no-op in Node.js, so
delete the "stream" entry from the "dependencies" section and, if a bundler shim
is truly required only for development/build, add it to "devDependencies"
instead (or leave it out entirely) and update package-lock/yarn.lock by
reinstalling to ensure lockfile consistency.
In `@packages/diagnostics/src/index.ts`:
- Around line 2-3: The current re-exports export * as OTELAPI and export * as
CONVENTIONS pull entire `@opentelemetry` namespaces and hurt tree-shaking; replace
these broad re-exports with narrow named exports of only the symbols we actually
use (e.g., export specific symbols like context, SpanKind, SpanStatusCode from
'@opentelemetry/api' and ATTR_CODE_FUNCTION_NAME, ATTR_SERVICE_NAME,
ATTR_SERVICE_VERSION from '@opentelemetry/semantic-conventions') so consumers
only import required identifiers; update the exports in the module where OTELAPI
and CONVENTIONS are declared to list those named symbols instead of exporting
the full namespaces.
In `@packages/diagnostics/vite.config.mts`:
- Around line 24-27: The expression using "dependencies ?? {}" is redundant
because dependencies already has a default in the destructuring; remove the "??
{}" so the spread uses "dependencies" directly and keep "devDependencies ?? {}"
as-is; locate the object spread around Object.keys(...) (the snippet that
currently reads "...Object.keys({ ...(dependencies ?? {}), ...(devDependencies
?? {}), })") and change it to "...Object.keys({ ...(dependencies),
...(devDependencies ?? {}), })" (or alternatively remove the destructuring
default and keep both "?? {}" — pick the simpler fix of removing "?? {}" after
"dependencies").
- Line 9: The Vite config sets root using the Unix-specific expression
process.env.PWD ?? process.cwd(); change the assignment to use process.cwd()
directly (i.e., root: process.cwd()) in the exported Vite config object to
ensure consistent behavior across CI environments; update the same pattern
wherever the config assigns root using process.env.PWD to keep configs
consistent.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by none
📒 Files selected for processing (11)
packages/api/src/observability/telemetry.tspackages/api/src/platforms/vtex/index.tspackages/api/src/typings/globals.tspackages/core/src/instrumentation.tspackages/core/src/server/options.tspackages/diagnostics/configs/dev.jsonpackages/diagnostics/configs/prod.jsonpackages/diagnostics/package.jsonpackages/diagnostics/src/index.tspackages/diagnostics/src/start.tspackages/diagnostics/vite.config.mts
✅ Files skipped from review due to trivial changes (1)
- packages/diagnostics/configs/prod.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/instrumentation.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/core/src/server/options.ts (2)
26-29:⚠️ Potential issue | 🟠 Major
storeConfig.analytics.otelEnabledlacks null-safety and normalization in the staticapiOptions.Line 27 accesses
storeConfig.analytics.otelEnabledwithout optional chaining — ifanalyticsis absent in the config, this crashes at module-init time. The staticapiOptions.OTEL.enabledalso carries a rawboolean | string | undefinedtype whilewithTraceClientnormalizes it tobooleanvia?.toString() === 'true', creating a divergent type contract for the same field.🐛 Proposed fix
- OTEL: { - enabled: storeConfig.analytics.otelEnabled, - }, + OTEL: { + enabled: String(storeConfig.analytics?.otelEnabled ?? false) === 'true', + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server/options.ts` around lines 26 - 29, The static apiOptions sets OTEL.enabled directly from storeConfig.analytics.otelEnabled which can throw if analytics is missing and leaves the field typed as boolean|string|undefined; update apiOptions.OTEL.enabled to mirror withTraceClient's normalization (use optional chaining and compare to 'true' or otherwise coerce to a boolean) so it is null-safe (e.g., access storeConfig.analytics?.otelEnabled) and always yields a boolean; change references to OTEL.enabled and any callers expecting a boolean to rely on this normalized boolean value.
35-36:⚠️ Potential issue | 🟠 Major
getTraceClient(name)silently no-ops:nameis@faststore/corebut the client is registered under@faststore/api.
getTraceClientperforms an exact key lookup; passing the core package name returnsundefined, soinject(OTEL)never runs and the OTEL carrier stays empty. This means tracing is never actually wired up despite the rest of the setup.🐛 Proposed fix
- getTraceClient(name)?.inject(OTEL) + getTraceClient('@faststore/api')?.inject(OTEL)Or, if the registration key is expected to match the consuming package, verify and align the key used in
packages/api/src/platforms/vtex/index.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server/options.ts` around lines 35 - 36, The trace client lookup is using the package name variable (name) which is '@faststore/core' while the client was registered under '@faststore/api', so getTraceClient(name) returns undefined and inject(OTEL) never runs; update the lookup in options.ts to use the correct registration key (e.g., '@faststore/api') or add a fallback lookup before calling inject on the returned client, ensuring getTraceClient(...) returns a client before calling its inject method (refer to getTraceClient, inject, and the OTEL carrier) so tracing is actually wired up; alternatively, align the registration key in packages/api/src/platforms/vtex/index.ts to match the consuming package name.
🧹 Nitpick comments (2)
packages/core/src/server/options.ts (2)
2-2: Static import eagerly loads@faststore/diagnosticsunconditionally.Previously this was a dynamic
await import(...)insidewithTraceClient, scoping the load to when tracing is actually used. As a static import it now loads (and executes any side effects in)@faststore/diagnosticsat module-evaluation time — regardless of whether OTEL is enabled. If the package has SDK initialization side effects, they will run even for stores withotelEnabled: false.♻️ Suggested approach
-import { getTraceClient } from '@faststore/diagnostics'export async function withTraceClient<T extends APIOptions = typeof apiOptions>( apiOptions: T ): Promise<T> { const OTEL = {} - getTraceClient(name)?.inject(OTEL) + const { getTraceClient } = await import('@faststore/diagnostics') + getTraceClient(name)?.inject(OTEL)As per coding guidelines, "Consider bundle size impact of dependencies" for TypeScript files under
packages/**/*.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server/options.ts` at line 2, Remove the top-level static import of getTraceClient and restore a dynamic import inside the function that uses it (withTraceClient) so `@faststore/diagnostics` is only loaded when tracing is enabled; inside withTraceClient, do an awaited import('@faststore/diagnostics') and extract getTraceClient from the imported module before using it, and update any type annotations to reference the imported symbol if needed to avoid eager evaluation.
38-44:as Ttype assertion bypasses structural verification of the return value.The cast silently accepts any mismatch between the constructed object and
T. A safer return type would bePromise<Omit<T, 'OTEL'> & { OTEL: Record<string, unknown> & { enabled: boolean } }>, which accurately reflects what is actually returned without forcing a cast.As per coding guidelines, TypeScript files under
packages/**/*.tsshould "Ensure type safety and avoid type assertions when possible."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server/options.ts` around lines 38 - 44, The current return uses an unsafe "as T" cast which bypasses TypeScript structural checks; update the function's return typing and remove the cast by declaring a precise return type that matches the constructed object (e.g. Omit<T, 'OTEL'> & { OTEL: Record<string, unknown> & { enabled: boolean } } or Promise<...> if the surrounding function is async), then return { ...apiOptions, OTEL: { ...OTEL, enabled: storeConfig.analytics?.otelEnabled?.toString() === 'true' } } without "as T"; adjust the function signature (and any callers) to use that concrete return type rather than forcing T.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/server/options.ts`:
- Around line 26-29: The static apiOptions sets OTEL.enabled directly from
storeConfig.analytics.otelEnabled which can throw if analytics is missing and
leaves the field typed as boolean|string|undefined; update
apiOptions.OTEL.enabled to mirror withTraceClient's normalization (use optional
chaining and compare to 'true' or otherwise coerce to a boolean) so it is
null-safe (e.g., access storeConfig.analytics?.otelEnabled) and always yields a
boolean; change references to OTEL.enabled and any callers expecting a boolean
to rely on this normalized boolean value.
- Around line 35-36: The trace client lookup is using the package name variable
(name) which is '@faststore/core' while the client was registered under
'@faststore/api', so getTraceClient(name) returns undefined and inject(OTEL)
never runs; update the lookup in options.ts to use the correct registration key
(e.g., '@faststore/api') or add a fallback lookup before calling inject on the
returned client, ensuring getTraceClient(...) returns a client before calling
its inject method (refer to getTraceClient, inject, and the OTEL carrier) so
tracing is actually wired up; alternatively, align the registration key in
packages/api/src/platforms/vtex/index.ts to match the consuming package name.
---
Nitpick comments:
In `@packages/core/src/server/options.ts`:
- Line 2: Remove the top-level static import of getTraceClient and restore a
dynamic import inside the function that uses it (withTraceClient) so
`@faststore/diagnostics` is only loaded when tracing is enabled; inside
withTraceClient, do an awaited import('@faststore/diagnostics') and extract
getTraceClient from the imported module before using it, and update any type
annotations to reference the imported symbol if needed to avoid eager
evaluation.
- Around line 38-44: The current return uses an unsafe "as T" cast which
bypasses TypeScript structural checks; update the function's return typing and
remove the cast by declaring a precise return type that matches the constructed
object (e.g. Omit<T, 'OTEL'> & { OTEL: Record<string, unknown> & { enabled:
boolean } } or Promise<...> if the surrounding function is async), then return {
...apiOptions, OTEL: { ...OTEL, enabled:
storeConfig.analytics?.otelEnabled?.toString() === 'true' } } without "as T";
adjust the function signature (and any callers) to use that concrete return type
rather than forcing T.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/src/platforms/vtex/index.ts (1)
55-69:⚠️ Potential issue | 🔴 Critical
ctx.idis never assigned —GraphqlContext.idwill always beundefinedat runtime.
GraphqlContextdeclaresid: stringas non-optional (line 21), but the closure never setsctx.id, andcryptois not imported. The per-request ID generation proposed in the past review (movingcrypto.randomBytes(32).toString('hex')inside this closure) was never applied. Every consumer ofctx.idfor trace correlation will silently receiveundefined.🐛 Proposed fix
Add
cryptoat the top of the file and assign inside the closure:+import crypto from 'node:crypto' import { getTelemetryClient } from '@faststore/diagnostics'return (ctx: any): GraphqlContext => { + ctx.id = crypto.randomBytes(32).toString('hex') ctx.storage = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 55 - 69, The GraphqlContext.id property is never set so ctx.id will be undefined at runtime; import Node's crypto and assign a per-request id inside the returned middleware closure (before returning ctx) using something like crypto.randomBytes(32).toString('hex') to satisfy GraphqlContext.id; make the change adjacent to where ctx.storage, ctx.clients (getClients) and ctx.loaders (getLoaders) are assigned so the id is available to downstream code that reads ctx.id.
♻️ Duplicate comments (2)
packages/diagnostics/package.json (1)
1-42: MissingpublishConfig— scoped package will be published as restricted.This was flagged in a prior review and remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/package.json` around lines 1 - 42, The package.json for `@faststore/diagnostics` is missing a publishConfig, causing the scoped package to be published with restricted access; add a top-level "publishConfig": { "access": "public" } entry in package.json (near the existing "name"/"version" fields) so the package is published as public; ensure the package name "@faststore/diagnostics" remains unchanged and validate the JSON after adding the publishConfig.packages/api/src/platforms/vtex/index.ts (1)
85-89:namestill shadows the module-levelnameimport frompackage.json.The destructured
namein.map(([name, resolverFunction])at line 85 shadows the top-levelnameimported on line 4. Despite the previous thread indicating a rename was applied, the current code still shows the conflicting identifier.♻️ Proposed rename
- Object.entries(resolver).map(([name, resolverFunction]) => { + Object.entries(resolver).map(([fieldName, resolverFunction]) => { if (typeof resolverFunction !== 'function') - return [name, resolverFunction] + return [fieldName, resolverFunction] - return [name, ResolverTrace(resolverFunction, `${key}(${name})`)] + return [fieldName, ResolverTrace(resolverFunction, `${key}(${fieldName})`)] })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 85 - 89, The destructured variable "name" in Object.entries(resolver).map(([name, resolverFunction]) shadows the module-level import "name" from package.json; rename that parameter (e.g., to resolverName or fieldName) wherever used in this mapping and in the created trace label `${key}(${name})` so it no longer collides with the imported "name", keeping the mapping logic (including the ResolverTrace(resolverFunction, `${key}(<newName>)`)) unchanged.
🧹 Nitpick comments (2)
packages/diagnostics/package.json (2)
7-9: Add aprepublishOnlyscript to guard against publishing unbuilt artifacts.Without it, publishing with a stale or missing
dist/directory silently ships broken output.🛡️ Proposed fix
"scripts": { - "build": "vite build" + "build": "vite build", + "prepublishOnly": "vite build" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/package.json` around lines 7 - 9, The package's scripts object only defines "build" and lacks a prepublish hook, so publishing can ship unbuilt artifacts; add a "prepublishOnly" script to package.json that runs the build (e.g., set "prepublishOnly": "npm run build") alongside the existing "build" entry to ensure the dist/ output is produced before any publish step.
28-29: Declare@opentelemetry/api(andinstrumentation-http) aspeerDependenciesin a library package.Two concerns:
Context breakage: If you're instrumenting a library, only install the OpenTelemetry API package. Your library will not emit telemetry on its own — it will only emit telemetry when it is part of an app that uses the OpenTelemetry SDK. Shipping
@opentelemetry/apias a harddependencyrisks bundling a second instance, which severs context propagation between this package and the host app's tracer.Double-instrumentation:
@opentelemetry/instrumentation-httppatchesnode:httpat load time. If the host app already registers this instrumentation (e.g. via@opentelemetry/auto-instrumentations-node) and this package installs its own copy, HTTP spans will be doubled.Move both to
peerDependencies:♻️ Proposed refactor
+ "peerDependencies": { + "@opentelemetry/api": "^1.9.0", + "@opentelemetry/instrumentation-http": "^0.212.0" + }, "dependencies": { - "@opentelemetry/api": "^1.9.0", - "@opentelemetry/instrumentation-http": "^0.212.0", "@opentelemetry/semantic-conventions": "^1.39.0", "@vtex/diagnostics-nodejs": "^5.1.1", "resolve-pkg": "^3.0.1", "stream": "^0.0.3" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/package.json` around lines 28 - 29, The package currently lists "@opentelemetry/api" and "@opentelemetry/instrumentation-http" as hard dependencies; remove them from dependencies and declare them in peerDependencies (keeping the same semver ranges) so host apps provide the single shared OpenTelemetry instances; also add them to devDependencies if you need them for tests/builds inside the package; update any installation docs if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/platforms/vtex/index.ts`:
- Line 39: GraphqlContext declares OTEL as non-optional but the factory assigns
options.OTEL which can be undefined; either make GraphqlContext.OTEL optional or
ensure the factory always sets a default object — e.g. change the factory
assignment of ctx.OTEL (the place where options.OTEL is copied into ctx) to use
a fallback (options.OTEL ?? {}) so downstream code never sees undefined; apply
the same fix to the other OTEL assignment elsewhere in this module to keep the
contract consistent.
In `@packages/diagnostics/package.json`:
- Line 33: Remove the unnecessary browser polyfill dependency "stream": "^0.0.3"
from the diagnostics package.json; locate the dependency entry `"stream":
"^0.0.3"` and delete it from the "dependencies" section, then run your package
manager to update lockfile (e.g., npm install / pnpm install) and ensure no code
imports the npm "stream" package (search for any require/import of "stream") so
Node's built-in stream module is used instead.
---
Outside diff comments:
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 55-69: The GraphqlContext.id property is never set so ctx.id will
be undefined at runtime; import Node's crypto and assign a per-request id inside
the returned middleware closure (before returning ctx) using something like
crypto.randomBytes(32).toString('hex') to satisfy GraphqlContext.id; make the
change adjacent to where ctx.storage, ctx.clients (getClients) and ctx.loaders
(getLoaders) are assigned so the id is available to downstream code that reads
ctx.id.
---
Duplicate comments:
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 85-89: The destructured variable "name" in
Object.entries(resolver).map(([name, resolverFunction]) shadows the module-level
import "name" from package.json; rename that parameter (e.g., to resolverName or
fieldName) wherever used in this mapping and in the created trace label
`${key}(${name})` so it no longer collides with the imported "name", keeping the
mapping logic (including the ResolverTrace(resolverFunction,
`${key}(<newName>)`)) unchanged.
In `@packages/diagnostics/package.json`:
- Around line 1-42: The package.json for `@faststore/diagnostics` is missing a
publishConfig, causing the scoped package to be published with restricted
access; add a top-level "publishConfig": { "access": "public" } entry in
package.json (near the existing "name"/"version" fields) so the package is
published as public; ensure the package name "@faststore/diagnostics" remains
unchanged and validate the JSON after adding the publishConfig.
---
Nitpick comments:
In `@packages/diagnostics/package.json`:
- Around line 7-9: The package's scripts object only defines "build" and lacks a
prepublish hook, so publishing can ship unbuilt artifacts; add a
"prepublishOnly" script to package.json that runs the build (e.g., set
"prepublishOnly": "npm run build") alongside the existing "build" entry to
ensure the dist/ output is produced before any publish step.
- Around line 28-29: The package currently lists "@opentelemetry/api" and
"@opentelemetry/instrumentation-http" as hard dependencies; remove them from
dependencies and declare them in peerDependencies (keeping the same semver
ranges) so host apps provide the single shared OpenTelemetry instances; also add
them to devDependencies if you need them for tests/builds inside the package;
update any installation docs if present.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package.jsonis excluded by none and included by noneturbo.jsonis excluded by none and included by none
📒 Files selected for processing (3)
packages/api/src/platforms/vtex/index.tspackages/api/src/typings/globals.tspackages/diagnostics/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/typings/globals.ts
| } | ||
| headers: Record<string, string> | ||
| account: string | ||
| OTEL: Record<string, unknown> |
There was a problem hiding this comment.
ctx.OTEL can be undefined — type contract with GraphqlContext is broken.
options.OTEL is demonstrably optional (optional-chaining on line 43), so ctx.OTEL = options.OTEL assigns undefined when OTEL is disabled. Since ctx is typed any, TypeScript silently accepts this, but GraphqlContext.OTEL: Record<string, unknown> is non-optional — any downstream code relying on the interface contract will get undefined.
Either make the field optional or default it to {}:
🛡️ Option A — make the field optional in the interface
- OTEL: Record<string, unknown>
+ OTEL?: Record<string, unknown>🛡️ Option B — default to `{}` in the factory closure
- ctx.OTEL = options.OTEL
+ ctx.OTEL = options.OTEL ?? {}Also applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/platforms/vtex/index.ts` at line 39, GraphqlContext declares
OTEL as non-optional but the factory assigns options.OTEL which can be
undefined; either make GraphqlContext.OTEL optional or ensure the factory always
sets a default object — e.g. change the factory assignment of ctx.OTEL (the
place where options.OTEL is copied into ctx) to use a fallback (options.OTEL ??
{}) so downstream code never sees undefined; apply the same fix to the other
OTEL assignment elsewhere in this module to keep the contract consistent.
packages/diagnostics/package.json
Outdated
| "@opentelemetry/semantic-conventions": "^1.39.0", | ||
| "@vtex/diagnostics-nodejs": "^5.1.1", | ||
| "resolve-pkg": "^3.0.1", | ||
| "stream": "^0.0.3" |
There was a problem hiding this comment.
stream ^0.0.3 is a browser polyfill — unnecessary in a server-side Node.js package.
The stream npm package (published 2012) is a legacy browser-compat shim for Node's built-in stream module. A server-side diagnostics package running on Node.js already has stream available natively; shipping this adds dead weight and could shadow the built-in in bundled contexts.
🐛 Proposed fix
- "stream": "^0.0.3"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/diagnostics/package.json` at line 33, Remove the unnecessary browser
polyfill dependency "stream": "^0.0.3" from the diagnostics package.json; locate
the dependency entry `"stream": "^0.0.3"` and delete it from the "dependencies"
section, then run your package manager to update lockfile (e.g., npm install /
pnpm install) and ensure no code imports the npm "stream" package (search for
any require/import of "stream") so Node's built-in stream module is used
instead.
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
1 participant
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.
Summary by CodeRabbit
Chores
New Features
Tests