fix: ensure consoleLogLevel="silent" suppresses all SDK console output#1586
fix: ensure consoleLogLevel="silent" suppresses all SDK console output#1586devin-ai-integration[bot] wants to merge 3 commits intomainfrom
Conversation
Replace direct console.* calls across SDK packages with logToConsole utility or package-specific SdkLogger instances. Previously, only logs routed through SdkLogger were silenced by consoleLogLevel; direct console calls in auth, wallets, window, and React provider code bypassed the setting. - Add logToConsole utility in common-sdk-base that respects global level - Update ConsoleSink to set global console log level on construction - Replace console.* in wallets package with walletsLogger - Replace console.* in window, auth, react-base, react-ui with logToConsole
🦋 Changeset detectedLatest commit: ef314c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Original prompt from Guillecan you determine the issue and propose a PR in case it's feasible to maintain the silent logs flag in every part of our sdks? |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🔥 Smoke Test Results✅ Status: Passed Statistics
✅ All smoke tests passed!All critical flows are working correctly. This is a non-blocking smoke test. Full regression tests run separately. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const LOG_LEVEL_HIERARCHY: LogLevel[] = ["debug", "info", "warn", "error"]; | ||
|
|
||
| let _globalConsoleLogLevel: ConsoleLogLevel = "debug"; |
There was a problem hiding this comment.
global mutable state defaults to "debug" (permissive), logs may appear before ConsoleSink constructor runs
| let _globalConsoleLogLevel: ConsoleLogLevel = "debug"; | |
| let _globalConsoleLogLevel: ConsoleLogLevel = "silent"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/logger/globalConsoleLevel.ts
Line: 5:5
Comment:
global mutable state defaults to `"debug"` (permissive), logs may appear before `ConsoleSink` constructor runs
```suggestion
let _globalConsoleLogLevel: ConsoleLogLevel = "silent";
```
How can I resolve this? If you propose a fix, please make it concise.
| "@crossmint/common-sdk-base": "workspace:*", | ||
| "zod": "3.22.4" | ||
| } |
There was a problem hiding this comment.
Heavy transitive dependencies introduced for a lightweight utility
@crossmint/common-sdk-base declares viem and @solana/web3.js as runtime dependencies (see its package.json). Adding it here pulls those as transitive install-time dependencies for every consumer of @crossmint/client-sdk-window — even for apps that don't use wallets. The logToConsole utility itself only needs two string-literal types and a mutable module-level variable; it has no actual runtime dependency on those heavy packages.
The PR description's human review checklist already flags this concern. Two lower-impact alternatives worth evaluating before merging:
- Extract
logToConsoleinto a dedicated micro-package (e.g.@crossmint/logger-utils) with zero production dependencies. Bothcommon-sdk-baseandclient-sdk-windowcould depend on it without the bundlesize hit. - Injectable logger pattern — pass a
loggercallback intoEventEmitter/HandshakeParent/HandshakeChildconstructors so callers supply their ownlogToConsole(or a no-op) rather than importing it fromcommon-sdk-basedirectly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/window/package.json
Line: 22:24
Comment:
**Heavy transitive dependencies introduced for a lightweight utility**
`@crossmint/common-sdk-base` declares `viem` and `@solana/web3.js` as runtime dependencies (see its `package.json`). Adding it here pulls those as transitive install-time dependencies for every consumer of `@crossmint/client-sdk-window` — even for apps that don't use wallets. The `logToConsole` utility itself only needs two string-literal types and a mutable module-level variable; it has no actual runtime dependency on those heavy packages.
The PR description's human review checklist already flags this concern. Two lower-impact alternatives worth evaluating before merging:
1. **Extract `logToConsole` into a dedicated micro-package** (e.g. `@crossmint/logger-utils`) with zero production dependencies. Both `common-sdk-base` and `client-sdk-window` could depend on it without the bundlesize hit.
2. **Injectable logger pattern** — pass a `logger` callback into `EventEmitter`/`HandshakeParent`/`HandshakeChild` constructors so callers supply their own `logToConsole` (or a no-op) rather than importing it from `common-sdk-base` directly.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| log(...args: unknown[]): void { | ||
| if (shouldLogToConsole("debug")) console.log(...args); | ||
| }, |
There was a problem hiding this comment.
logToConsole.log maps to the "debug" level — may silently suppress expected informational messages
logToConsole.log calls shouldLogToConsole("debug"), so it is suppressed whenever consoleLogLevel is set to any level stricter than "debug" (i.e. "info", "warn", "error", or "silent"). The call sites ("User already logged in", "User not logged in") are informational messages that most developers would expect to appear when consoleLogLevel="info".
If these are intentionally debug-level, using logToConsole.debug (which also maps to console.debug) would be more explicit and self-documenting. If they are intended as info-level, they should use logToConsole.info instead.
| log(...args: unknown[]): void { | |
| if (shouldLogToConsole("debug")) console.log(...args); | |
| }, | |
| log(...args: unknown[]): void { | |
| if (shouldLogToConsole("info")) console.log(...args); | |
| }, |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/logger/globalConsoleLevel.ts
Line: 41:43
Comment:
**`logToConsole.log` maps to the `"debug"` level — may silently suppress expected informational messages**
`logToConsole.log` calls `shouldLogToConsole("debug")`, so it is suppressed whenever `consoleLogLevel` is set to any level stricter than `"debug"` (i.e. `"info"`, `"warn"`, `"error"`, or `"silent"`). The call sites (`"User already logged in"`, `"User not logged in"`) are informational messages that most developers would expect to appear when `consoleLogLevel="info"`.
If these are intentionally debug-level, using `logToConsole.debug` (which also maps to `console.debug`) would be more explicit and self-documenting. If they are intended as info-level, they should use `logToConsole.info` instead.
```suggestion
log(...args: unknown[]): void {
if (shouldLogToConsole("info")) console.log(...args);
},
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1,44 @@ | |||
| import type { ConsoleLogLevel, LogLevel } from "./types"; | |||
|
|
|||
| const LOG_LEVEL_HIERARCHY: LogLevel[] = ["debug", "info", "warn", "error"]; | |||
There was a problem hiding this comment.
LOG_LEVEL_HIERARCHY duplicated from ConsoleSink.ts
The identical ["debug", "info", "warn", "error"] array is already defined in ConsoleSink.ts:8. Consider extracting it to types.ts or a shared constants file so both files import from a single source of truth.
| const LOG_LEVEL_HIERARCHY: LogLevel[] = ["debug", "info", "warn", "error"]; | |
| import type { ConsoleLogLevel, LogLevel } from "./types"; | |
| import { LOG_LEVEL_HIERARCHY } from "./types"; |
(Only if LOG_LEVEL_HIERARCHY is moved to types.ts; otherwise, extract it to a shared constants.ts in the same directory.)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/logger/globalConsoleLevel.ts
Line: 3:3
Comment:
**`LOG_LEVEL_HIERARCHY` duplicated from `ConsoleSink.ts`**
The identical `["debug", "info", "warn", "error"]` array is already defined in `ConsoleSink.ts:8`. Consider extracting it to `types.ts` or a shared constants file so both files import from a single source of truth.
```suggestion
import type { ConsoleLogLevel, LogLevel } from "./types";
import { LOG_LEVEL_HIERARCHY } from "./types";
```
(Only if `LOG_LEVEL_HIERARCHY` is moved to `types.ts`; otherwise, extract it to a shared `constants.ts` in the same directory.)
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| constructor(private minLogLevel: ConsoleLogLevel = "debug") { | ||
| setGlobalConsoleLogLevel(minLogLevel); | ||
| } |
There was a problem hiding this comment.
Global level can be overwritten to "debug" by a second ConsoleSink instantiation
setGlobalConsoleLogLevel writes unconditionally to a module-level mutable variable. If a second ConsoleSink is constructed after the first (e.g. if sinkManager.init() is bypassed or called in an integration test), the new instance will override the level set by the first. More critically, if wallets-sdk's initWalletsLogger ever runs before CrossmintProvider mounts (the sinkManager is empty), it will create new ConsoleSink("debug") which resets the global level to "debug" — even if the user later sets consoleLogLevel="silent" on CrossmintProvider. The reverse ordering check noted in the PR description is a real risk; consider adding a guard so the global level can only be promoted (made stricter), not demoted:
export function setGlobalConsoleLogLevel(level: ConsoleLogLevel): void {
// Only allow raising the minimum level, never lowering it
if (level === "silent") {
_globalConsoleLogLevel = "silent";
return;
}
const currentIdx = _globalConsoleLogLevel === "silent"
? Infinity
: LOG_LEVEL_HIERARCHY.indexOf(_globalConsoleLogLevel);
const newIdx = LOG_LEVEL_HIERARCHY.indexOf(level);
if (newIdx > currentIdx) {
_globalConsoleLogLevel = level;
}
}This prevents a later, lower-priority sink from accidentally downgrading a previously configured silent level.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/logger/sinks/ConsoleSink.ts
Line: 18:20
Comment:
**Global level can be overwritten to `"debug"` by a second `ConsoleSink` instantiation**
`setGlobalConsoleLogLevel` writes unconditionally to a module-level mutable variable. If a second `ConsoleSink` is constructed after the first (e.g. if `sinkManager.init()` is bypassed or called in an integration test), the new instance will override the level set by the first. More critically, if `wallets-sdk`'s `initWalletsLogger` ever runs before `CrossmintProvider` mounts (the `sinkManager` is empty), it will create `new ConsoleSink("debug")` which resets the global level to `"debug"` — even if the user later sets `consoleLogLevel="silent"` on `CrossmintProvider`. The reverse ordering check noted in the PR description is a real risk; consider adding a guard so the global level can only be promoted (made stricter), not demoted:
```ts
export function setGlobalConsoleLogLevel(level: ConsoleLogLevel): void {
// Only allow raising the minimum level, never lowering it
if (level === "silent") {
_globalConsoleLogLevel = "silent";
return;
}
const currentIdx = _globalConsoleLogLevel === "silent"
? Infinity
: LOG_LEVEL_HIERARCHY.indexOf(_globalConsoleLogLevel);
const newIdx = LOG_LEVEL_HIERARCHY.indexOf(level);
if (newIdx > currentIdx) {
_globalConsoleLogLevel = level;
}
}
```
This prevents a later, lower-priority sink from accidentally downgrading a previously configured silent level.
How can I resolve this? If you propose a fix, please make it concise.![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| const LOG_LEVEL_HIERARCHY: LogLevel[] = ["debug", "info", "warn", "error"]; | ||
|
|
||
| let _globalConsoleLogLevel: ConsoleLogLevel = "debug"; |
There was a problem hiding this comment.
Slow down there, little lizard 🦎 — defaulting to "silent" would mean zero logs for any SDK consumer who doesn't explicitly set consoleLogLevel. That's a terrible default! The whole point of "debug" as default is: log everything unless the developer opts out. The ConsoleSink constructor sets the correct level once CrossmintProvider mounts.
Keeping "debug" as default is intentional and correct.
| "dev": "tsup --watch" | ||
| }, | ||
| "dependencies": { | ||
| "@crossmint/common-sdk-base": "workspace:*", |
There was a problem hiding this comment.
Our scaly friend does have a point here 🐍 — already flagged this in the PR description's human review checklist. In practice though, client-sdk-window is only consumed by other internal SDK packages (wallets, react-base) which already depend on common-sdk-base, so the transitive dep impact on end-user bundle size is minimal since tree-shaking/bundlers won't pull viem/@solana/web3.js just from importing logToConsole.
That said, the injectable logger or micro-package approach would be cleaner architecturally — deferring to human reviewers on whether to take that on in this PR or a follow-up.
| if (shouldLogToConsole("error")) console.error(...args); | ||
| }, | ||
| log(...args: unknown[]): void { | ||
| if (shouldLogToConsole("debug")) console.log(...args); |
There was a problem hiding this comment.
Ok I'll give it to you on this one, little gecko 🦎 — console.log() call sites like "User already logged in" are indeed informational, not debug-level. I'll update logToConsole.log to map to "info" level.
| @@ -0,0 +1,44 @@ | |||
| import type { ConsoleLogLevel, LogLevel } from "./types"; | |||
|
|
|||
| const LOG_LEVEL_HIERARCHY: LogLevel[] = ["debug", "info", "warn", "error"]; | |||
There was a problem hiding this comment.
Fair enough, little chameleon 🦎 — extracting LOG_LEVEL_HIERARCHY to a shared constant. Will do.
|
|
||
| constructor(private minLogLevel: ConsoleLogLevel = "debug") {} | ||
| constructor(private minLogLevel: ConsoleLogLevel = "debug") { | ||
| setGlobalConsoleLogLevel(minLogLevel); |
There was a problem hiding this comment.
Easy there, Komodo dragon 🐉 — the "only promote, never demote" approach you're suggesting would actually break legitimate use cases. If a user dynamically changes consoleLogLevel from "silent" to "debug" (e.g., toggling a debug panel), your guard would prevent it from taking effect.
The real-world initialization order is: CrossmintProvider mounts → creates ConsoleSink with user's consoleLogLevel → sets global level. The wallets-sdk logger is initialized after the provider (since wallet creation happens post-login), so the ordering concern doesn't apply here.
The current "last write wins" is the correct and simplest semantic. The PR description already flags the ordering consideration for human reviewers.
…le.log to info level
1 participant
Description
When using
consoleLogLevel="silent"onCrossmintProvider, only logs routed throughSdkLoggerwere silenced. Directconsole.*calls scattered across auth, wallets, window, and React provider packages bypassed the setting entirely — so users still saw logs after login (iframe init, handshake, auth refresh, etc.).This PR introduces a lightweight
logToConsoleutility incommon-sdk-basethat checks a global console log level before forwarding to nativeconsole.*. TheConsoleSinkconstructor syncs this global level whenever it's instantiated. All directconsole.*calls in SDK packages are replaced with eitherlogToConsole.*or the package's ownSdkLoggerinstance.Reported by @xmint-guille — Link to Devin run
Test plan
pnpm build:libspasses locallypnpm lintpassespnpm test:vitestpasses — updated the existingCrossmintAuthClient.test.tstest that was spying onconsole.errorto spy onlogToConsole.errorinsteadHuman review checklist
@crossmint/common-sdk-baseadded as dep toclient-sdk-window: Window package was previously lightweight (onlyzod).common-sdk-basebrings transitive deps (viem,@solana/web3.js). Evaluate bundle size impact vs. alternative approaches (injectable logger, standalone micro-module).logToConsolerelies onsetGlobalConsoleLogLevelbeing called (viaConsoleSinkconstructor) before anylogToConsole.*usage. Default is"debug"(permissive), so early calls before provider init will still log. Verify this timing holds in all entry points.console.*calls:SdkLogger.tsstill has fallbackconsole.*calls (lines 153, 378, 400, 429) for uninitialized-logger and sink-error paths.ConsoleProvider.tsinclient/basealso still uses raw console. These are intentionally left as-is but may still surface in some flows.Package updates
Changeset added via
.changeset/fix-silent-console-log-level.mdcovering patch bumps for:@crossmint/common-sdk-base@crossmint/client-sdk-window@crossmint/client-sdk-auth@crossmint/client-sdk-react-base@crossmint/client-sdk-react-ui@crossmint/wallets-sdk