Make logs cleaner #883
Make logs cleaner #883
Conversation
📝 WalkthroughWalkthroughUpdated logging dependencies and logger option shapes; added a shared empty-JSON Changes
Sequence Diagram(s)(omitted — changes do not introduce a new multi-component runtime flow requiring a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.{res,resi}📄 CodeRabbit inference engine (.cursor/rules/rescript.mdc)
Files:
**/*.res📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Files:
🧠 Learnings (8)📚 Learning: 2026-01-05T11:20:07.222ZApplied to files:
📚 Learning: 2026-01-05T11:20:07.222ZApplied to files:
📚 Learning: 2026-01-05T11:20:07.222ZApplied to files:
📚 Learning: 2025-12-04T12:12:49.547ZApplied to files:
📚 Learning: 2025-11-25T12:40:26.435ZApplied to files:
📚 Learning: 2025-12-04T12:13:23.246ZApplied to files:
📚 Learning: 2025-11-25T12:40:26.435ZApplied to files:
📚 Learning: 2025-11-25T12:40:26.435ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @scenarios/test_codegen/scripts/update-log-snapshots.mjs:
- Around line 32-47: The loop over strategies calls execSync(`node
${FIXTURE_PATH}`) without error handling; wrap the execSync call in a try-catch
inside the for (const strategy of strategies) { ... } loop (around where
execSync is invoked) so that failures are caught, logged with context (include
strategy and FIXTURE_PATH), and the script either continues to the next strategy
or writes an error marker to the corresponding snapshotPath before moving on;
ensure you still call normalize and writeFileSync for successful runs and use
processLogger/console.error to report caught errors.
🧹 Nitpick comments (1)
scenarios/test_codegen/test/Logging_test.ts (1)
40-44: Consider testing the ecs-console-multistream strategy.The
Logging.resfile defines seven logging strategies (lines 3-10), includingecs-console-multistream, but only three console strategies are tested here. Consider adding a test forecs-console-multistreamto ensure comprehensive coverage.🔎 Proposed addition
describe("Logging Output", () => { testLogStrategy("console-pretty"); testLogStrategy("console-raw"); testLogStrategy("ecs-console"); + testLogStrategy("ecs-console-multistream"); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
codegenerator/cli/npm/envio/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlscenarios/test_codegen/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlscenarios/test_codegen/test/__snapshots__/Logging.both-prettyconsole.snapis excluded by!**/*.snapscenarios/test_codegen/test/__snapshots__/Logging.console-pretty.snapis excluded by!**/*.snapscenarios/test_codegen/test/__snapshots__/Logging.console-raw.snapis excluded by!**/*.snapscenarios/test_codegen/test/__snapshots__/Logging.ecs-console.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
codegenerator/cli/npm/envio/package.jsoncodegenerator/cli/npm/envio/package.json.tmplcodegenerator/cli/npm/envio/src/Logging.rescodegenerator/cli/npm/envio/src/bindings/Pino.resscenarios/test_codegen/package.jsonscenarios/test_codegen/scripts/update-log-snapshots.mjsscenarios/test_codegen/test/Logging_test.tsscenarios/test_codegen/test/fixtures/LogTesting.resscenarios/test_codegen/test/lib_tests/FetchState_test.resscenarios/test_codegen/test/manual/README.md
💤 Files with no reviewable changes (2)
- scenarios/test_codegen/test/manual/README.md
- scenarios/test_codegen/test/lib_tests/FetchState_test.res
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{res,resi}
📄 CodeRabbit inference engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Always use ReScript 11 documentation
Never suggest ReasonML syntax
Never use[| item |]to create an array. Use[ item ]instead
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction
Use records when working with structured data, and objects to conveniently pass payload data between functions
Never use %raw to access object fields if you know the type
Files:
codegenerator/cli/npm/envio/src/bindings/Pino.rescodegenerator/cli/npm/envio/src/Logging.resscenarios/test_codegen/test/fixtures/LogTesting.res
**/*.res
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Prefer reading ReScript
.resmodules directly; ignore compiled.jsartifacts
Files:
codegenerator/cli/npm/envio/src/bindings/Pino.rescodegenerator/cli/npm/envio/src/Logging.resscenarios/test_codegen/test/fixtures/LogTesting.res
🧠 Learnings (9)
📚 Learning: 2026-01-05T11:20:07.222Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2026-01-05T11:20:07.222Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx,res} : Prefer Public module API for testing
Applied to files:
scenarios/test_codegen/package.json
📚 Learning: 2026-01-05T11:20:07.222Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2026-01-05T11:20:07.222Z
Learning: Verify tests pass by running compiler `pnpm rescript` and tests `pnpm mocha`
Applied to files:
scenarios/test_codegen/package.jsoncodegenerator/cli/npm/envio/package.jsoncodegenerator/cli/npm/envio/package.json.tmplscenarios/test_codegen/test/fixtures/LogTesting.res
📚 Learning: 2026-01-05T11:20:07.222Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2026-01-05T11:20:07.222Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx,res} : Use `_only` to specify which tests to run during test development
Applied to files:
scenarios/test_codegen/package.json
📚 Learning: 2026-01-05T11:20:07.222Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2026-01-05T11:20:07.222Z
Learning: Applies to **/*.res : Prefer reading ReScript `.res` modules directly; ignore compiled `.js` artifacts
Applied to files:
scenarios/test_codegen/package.json
📚 Learning: 2025-12-04T12:12:49.547Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: codegenerator/cli/templates/static/shared/.cursor/rules/hyperindex.mdc:0-0
Timestamp: 2025-12-04T12:12:49.547Z
Learning: After any change to TypeScript files, run `pnpm tsc --noEmit` to ensure successful compilation
Applied to files:
scenarios/test_codegen/package.json
📚 Learning: 2025-11-25T12:40:26.435Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/rescript.mdc:0-0
Timestamp: 2025-11-25T12:40:26.435Z
Learning: Applies to **/*.test.{res,resi} : Never use `Js.Console.log` in test files. Use `Assert` module for all verifications
Applied to files:
scenarios/test_codegen/test/Logging_test.tsscenarios/test_codegen/test/fixtures/LogTesting.res
📚 Learning: 2025-11-25T12:40:26.435Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/rescript.mdc:0-0
Timestamp: 2025-11-25T12:40:26.435Z
Learning: Applies to **/*.test.{res,resi} : Tests should be silent unless they fail - rely on assertions rather than logging
Applied to files:
scenarios/test_codegen/test/fixtures/LogTesting.res
📚 Learning: 2025-11-25T12:40:26.435Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/rescript.mdc:0-0
Timestamp: 2025-11-25T12:40:26.435Z
Learning: Applies to **/*.test.{res,resi} : Use try/catch as expressions instead of refs for tracking success/failure in tests
Applied to files:
scenarios/test_codegen/test/fixtures/LogTesting.res
📚 Learning: 2026-01-05T11:20:07.222Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2026-01-05T11:20:07.222Z
Learning: Entry module for generated runtime is `Index.res` which starts HTTP server, loads `Config.res`, calls `RegisterHandlers.res`, spins up `GlobalStateManager.res`
Applied to files:
scenarios/test_codegen/test/fixtures/LogTesting.res
🧬 Code graph analysis (1)
scenarios/test_codegen/test/Logging_test.ts (1)
scenarios/test_codegen/scripts/update-log-snapshots.mjs (5)
FIXTURE_PATH(6-6)SNAPSHOTS_DIR(7-7)normalize(18-27)output(34-43)snapshotPath(44-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (9)
scenarios/test_codegen/test/fixtures/LogTesting.res (1)
1-9: LGTM! Clean runtime guard implementation.The guard correctly prevents accidental execution outside the test runner context. The use of pattern matching on the option type and clear error messaging follows ReScript best practices.
scenarios/test_codegen/package.json (2)
8-8: LGTM! Correctly excludes test fixtures from regular test runs.The ignore pattern prevents the LogTesting fixture from being executed as a standalone test, which aligns with the runtime guard added to that file.
11-11: LGTM! Script correctly invokes the snapshot update process.The script properly sets the
LOGGING_TEST_RUNNER=1environment variable expected by the fixture guard and references the correct script path.codegenerator/cli/npm/envio/src/bindings/Pino.res (1)
135-135: Thecolorizeoption is correctly bound as an optional boolean field. pino-pretty 13.1.3 supportscolorizein prettyFactory configuration—it controls whether terminal colors are enabled and defaults to automatic terminal capability detection.codegenerator/cli/npm/envio/package.json (1)
34-35: Both [email protected] and [email protected] are valid versions on npm with no known security advisories. The breaking change in pino (Node 18 support dropped) is not a concern since the project requires Node >=22.0.0. The pino-pretty change (replacing readable-stream with built-in stream) is an internal implementation detail that does not affect the codebase. The upgrades are safe and compatible.codegenerator/cli/npm/envio/src/Logging.res (1)
45-78: LGTM! Consistent implementation of pid/hostname removal.The introduction of the
baseobject and its propagation across all logging strategies is correctly implemented. The distinction between ECS strategies (which spreadPino.ECS.make()and addbase) and non-ECS strategies (which only pass{base: base}) is intentional and appropriate.scenarios/test_codegen/test/Logging_test.ts (2)
17-29: LGTM! Well-structured test helper.The
runWithStrategyfunction correctly sets up the environment withLOG_STRATEGY,LOGGING_TEST_RUNNER, andFORCE_COLORto ensure consistent, colorized output for snapshot testing.
9-15: No issue - inconsistency is harmless and does not affect test functionality.The
normalizefunction in Logging_test.ts only handles timestamps, while the update script in update-log-snapshots.mjs includes additional PID normalization patterns. However, this inconsistency poses no practical problem: the log fixture does not generate any process IDs, and the existing snapshots contain no PID data. Both the actual output and snapshot comparisons are normalized identically before assertion, so the test passes correctly. The PID patterns in the update script are defensive/future-proofing and are not currently needed.Likely an incorrect or invalid review comment.
codegenerator/cli/npm/envio/package.json.tmpl (1)
44-48: Verify pino and pino-pretty integration with the Transport API and error handling.The current versions (
pino10.1.0 andpino-pretty13.1.3) include breaking changes from their v8.x and v10.x ancestors:
- Node.js version requirements increased (now requires ≥22.0.0)
- Transport API and error serialization behavior changed
pino-prettyreplacedreadable-streamwith native stream implementationThe code in
Pino.resuses the Transport API and custom error serialization, which are directly affected by these changes. Confirm that the Transport configuration and error handling work correctly by running the test suite.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @scenarios/test_codegen/scripts/update-log-snapshots.mjs:
- Around line 32-45: Wrap the execSync invocation inside the strategies loop in
a try/catch so a failing fixture run doesn’t abort the whole script; in the
catch for the call that uses FIXTURE_PATH and execSync, capture the error
(including stdout/stderr if present), normalize it with the existing normalize
utility, write an error snapshot to the same
SNAPSHOTS_DIR/Logging.${strategy}.snap via writeFileSync, and log a concise
message before continuing to the next strategy so remaining snapshots still get
updated.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codegenerator/cli/npm/envio/src/bindings/Pino.resscenarios/test_codegen/scripts/update-log-snapshots.mjsscenarios/test_codegen/test/Logging_test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scenarios/test_codegen/test/Logging_test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{res,resi}
📄 CodeRabbit inference engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Always use ReScript 11 documentation
Never suggest ReasonML syntax
Never use[| item |]to create an array. Use[ item ]instead
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction
Use records when working with structured data, and objects to conveniently pass payload data between functions
Never use %raw to access object fields if you know the type
Files:
codegenerator/cli/npm/envio/src/bindings/Pino.res
**/*.res
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Prefer reading ReScript
.resmodules directly; ignore compiled.jsartifacts
Files:
codegenerator/cli/npm/envio/src/bindings/Pino.res
🧠 Learnings (1)
📚 Learning: 2025-11-25T12:40:26.435Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/rescript.mdc:0-0
Timestamp: 2025-11-25T12:40:26.435Z
Learning: Applies to **/*.test.{res,resi} : Use try/catch as expressions instead of refs for tracking success/failure in tests
Applied to files:
scenarios/test_codegen/scripts/update-log-snapshots.mjs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (3)
codegenerator/cli/npm/envio/src/bindings/Pino.res (2)
135-135: LGTM! Type definition correctly extended.The type extension properly adds optional fields for color customization, which aligns with the implementation in
makeFormatter.
140-156: Well-structured refactor with clear conditional logic.The implementation cleanly extracts the color configuration and conditionally enables colorization for test scenarios. The pattern matching is idiomatic ReScript, and the comment explaining why
colorizeis omitted (rather than set tofalse) is helpful. ThecustomColorsstring format andcolorizeboolean option are both correctly supported by pino-pretty 13.1.3, and the usage matches the documented API.scenarios/test_codegen/scripts/update-log-snapshots.mjs (1)
36-40: No action needed—the current implementation is correct.The environment variables in this file correctly use
LOGGING_TEST_RUNNERto control colorization. A search of the codebase reveals:
FORCE_COLORis not referenced anywhere in the repository.- Colorization is explicitly controlled via
LOGGING_TEST_RUNNERincodegenerator/cli/npm/envio/src/bindings/Pino.res(line 142), where a comment states "Force colors when running in test runner" and the code switches on this environment variable to configure pino's output options.- The current code structure matches the implementation in
Logging_test.ts, which uses the same pattern.The removal of a non-existent
FORCE_COLORvariable is not a concern—the logging system is designed to detectLOGGING_TEST_RUNNERand handle colorization accordingly.Likely an incorrect or invalid review comment.
| for (const strategy of strategies) { | ||
| console.log(`Updating snapshot for ${strategy}...`); | ||
| const output = execSync(`node ${FIXTURE_PATH}`, { | ||
| encoding: "utf-8", | ||
| env: { | ||
| ...process.env, | ||
| LOG_STRATEGY: strategy, | ||
| LOGGING_TEST_RUNNER: "1", | ||
| }, | ||
| }); | ||
| const snapshotPath = `${SNAPSHOTS_DIR}/Logging.${strategy}.snap`; | ||
| writeFileSync(snapshotPath, normalize(output)); | ||
| console.log(` Written to ${snapshotPath}`); | ||
| } |
There was a problem hiding this comment.
Add error handling for fixture execution.
The execSync call lacks error handling. If the fixture fails (compilation errors, runtime exceptions, etc.), the script will crash without completing snapshot updates for remaining strategies.
🔎 Proposed error handling
for (const strategy of strategies) {
console.log(`Updating snapshot for ${strategy}...`);
- const output = execSync(`node ${FIXTURE_PATH}`, {
- encoding: "utf-8",
- env: {
- ...process.env,
- LOG_STRATEGY: strategy,
- LOGGING_TEST_RUNNER: "1",
- },
- });
- const snapshotPath = `${SNAPSHOTS_DIR}/Logging.${strategy}.snap`;
- writeFileSync(snapshotPath, normalize(output));
- console.log(` Written to ${snapshotPath}`);
+ try {
+ const output = execSync(`node ${FIXTURE_PATH}`, {
+ encoding: "utf-8",
+ env: {
+ ...process.env,
+ LOG_STRATEGY: strategy,
+ LOGGING_TEST_RUNNER: "1",
+ },
+ });
+ const snapshotPath = `${SNAPSHOTS_DIR}/Logging.${strategy}.snap`;
+ writeFileSync(snapshotPath, normalize(output));
+ console.log(` Written to ${snapshotPath}`);
+ } catch (error) {
+ console.error(` Failed to update snapshot for ${strategy}:`, error.message);
+ process.exit(1);
+ }
}📝 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.
| for (const strategy of strategies) { | |
| console.log(`Updating snapshot for ${strategy}...`); | |
| const output = execSync(`node ${FIXTURE_PATH}`, { | |
| encoding: "utf-8", | |
| env: { | |
| ...process.env, | |
| LOG_STRATEGY: strategy, | |
| LOGGING_TEST_RUNNER: "1", | |
| }, | |
| }); | |
| const snapshotPath = `${SNAPSHOTS_DIR}/Logging.${strategy}.snap`; | |
| writeFileSync(snapshotPath, normalize(output)); | |
| console.log(` Written to ${snapshotPath}`); | |
| } | |
| for (const strategy of strategies) { | |
| console.log(`Updating snapshot for ${strategy}...`); | |
| try { | |
| const output = execSync(`node ${FIXTURE_PATH}`, { | |
| encoding: "utf-8", | |
| env: { | |
| ...process.env, | |
| LOG_STRATEGY: strategy, | |
| LOGGING_TEST_RUNNER: "1", | |
| }, | |
| }); | |
| const snapshotPath = `${SNAPSHOTS_DIR}/Logging.${strategy}.snap`; | |
| writeFileSync(snapshotPath, normalize(output)); | |
| console.log(` Written to ${snapshotPath}`); | |
| } catch (error) { | |
| console.error(` Failed to update snapshot for ${strategy}:`, error.message); | |
| process.exit(1); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @scenarios/test_codegen/scripts/update-log-snapshots.mjs around lines 32 -
45, Wrap the execSync invocation inside the strategies loop in a try/catch so a
failing fixture run doesn’t abort the whole script; in the catch for the call
that uses FIXTURE_PATH and execSync, capture the error (including stdout/stderr
if present), normalize it with the existing normalize utility, write an error
snapshot to the same SNAPSHOTS_DIR/Logging.${strategy}.snap via writeFileSync,
and log a concise message before continuing to the next strategy so remaining
snapshots still get updated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @scenarios/test_codegen/test/Logging_test.ts:
- Around line 6-7: The snapshot path construction is inconsistent: tests use
SNAPSHOTS_DIR = path.join(import.meta.dirname, "__snapshots__") while the update
script hardcodes "test/__snapshots__"; update the update script to construct its
snapshots directory the same way (use path.join(import.meta.dirname,
"__snapshots__") or equivalent) so both FIXTURE_PATH/SNAPSHOTS_DIR and the
update script resolve snapshots absolutely and consistently.
🧹 Nitpick comments (4)
scenarios/test_codegen/test/Logging_test.ts (3)
17-27: Consider adding error handling and timeout.The function uses
execSyncwhich throws on non-zero exit codes. If the fixture encounters an error, the test will fail with an uncaught exception rather than a clear test failure message.Consider wrapping in try-catch to provide clearer test diagnostics, or add a timeout to prevent hanging tests.
🔎 Optional enhancement with error handling
const runWithStrategy = (strategy: string): string => { - return execSync(`node ${FIXTURE_PATH}`, { - encoding: "utf-8", - env: { - ...process.env, - LOG_STRATEGY: strategy, - LOGGING_TEST_RUNNER: "1", - }, - cwd: process.cwd(), - }); + try { + return execSync(`node ${FIXTURE_PATH}`, { + encoding: "utf-8", + env: { + ...process.env, + LOG_STRATEGY: strategy, + LOGGING_TEST_RUNNER: "1", + }, + timeout: 5000, + }); + } catch (error) { + throw new Error(`Failed to run fixture with strategy ${strategy}: ${error.message}`); + } };Note: Removed
cwd: process.cwd()as it's the default behavior.
29-36: Consider handling missing snapshot files.If a snapshot file doesn't exist,
readFileSyncwill throw an uncaught error. While this might be acceptable (tests should fail if snapshots are missing), providing a clearer error message could improve developer experience.🔎 Optional enhancement with clearer error message
const testLogStrategy = (strategy: string) => { it(`LOG_STRATEGY=${strategy}`, () => { const output = runWithStrategy(strategy); const snapshotPath = path.join(SNAPSHOTS_DIR, `Logging.${strategy}.snap`); + + if (!require("fs").existsSync(snapshotPath)) { + throw new Error(`Snapshot not found: ${snapshotPath}. Run update-log-snapshots script to generate.`); + } + const expected = readFileSync(snapshotPath, "utf-8"); assert.equal(normalize(output), normalize(expected)); }); };
44-66: Clarify test suite grouping and comment.The comment on line 45 states "These strategies write to file, not stdout" but the
both-prettyconsoletest (lines 57-65) expects and validates stdout content against a snapshot. This is inconsistent with the comment.Consider either:
- Updating the comment to clarify that
both-prettyconsolewrites to both file and stdout- Moving the
both-prettyconsoletest to a separate describe blockAdditionally, the
both-prettyconsoletest only validates stdout but doesn't verify file writing behavior, unlike the description "writes to both" in the test name.🔎 Suggested clarification
-// These strategies write to file, not stdout - test separately -describe("Logging Output (file strategies)", () => { +describe("Logging Output (file strategies)", () => { it("LOG_STRATEGY=ecs-file writes to log file", () => { - // This strategy writes to file, stdout should be empty + // This strategy writes only to file, stdout should be empty const output = runWithStrategy("ecs-file"); assert.equal(output.trim(), ""); }); it("LOG_STRATEGY=file-only writes to log file", () => { + // This strategy writes only to file, stdout should be empty const output = runWithStrategy("file-only"); assert.equal(output.trim(), ""); }); +}); +describe("Logging Output (both file and console)", () => { it("LOG_STRATEGY=both-prettyconsole writes to both", () => { const output = runWithStrategy("both-prettyconsole"); const snapshotPath = path.join( SNAPSHOTS_DIR, "Logging.both-prettyconsole.snap" ); const expected = readFileSync(snapshotPath, "utf-8"); assert.equal(normalize(output), normalize(expected)); }); });codegenerator/cli/npm/envio/src/bindings/Pino.res (1)
140-157: LGTM! Consider checking env var value.The refactoring correctly implements conditional colorization for test runs. The code is clear and well-commented.
Minor consideration: The current implementation enables colorization when
LOGGING_TEST_RUNNERis present, regardless of its value. This meansLOGGING_TEST_RUNNER=0orLOGGING_TEST_RUNNER=falsewould still enable colors. If this is intentional (presence = enabled), no change needed.🔎 Optional: Check for truthy value
If you want to check for a truthy value instead of just presence:
- let options = switch NodeJs.Process.process.env->Js.Dict.get("LOGGING_TEST_RUNNER") { - | Some(_) => { + let options = switch NodeJs.Process.process.env->Js.Dict.get("LOGGING_TEST_RUNNER") { + | Some("1" | "true") => { customLevels: logLevels, customColors, colorize: true, } // For some reason setting colorize as undefined, // makes it behave as false. - | None => { + | Some(_) | None => { customLevels: logLevels, customColors, } }Alternatively, for even more brevity:
let makeFormatter = logLevels => { let customColors = "fatal:bgRed,error:red,warn:yellow,info:green,udebug:bgBlue,uinfo:bgGreen,uwarn:bgYellow,uerror:bgRed,debug:blue,trace:gray" - // Force colors when running in test runner - let options = switch NodeJs.Process.process.env->Js.Dict.get("LOGGING_TEST_RUNNER") { - | Some(_) => { - customLevels: logLevels, - customColors, - colorize: true, - } - // For some reason setting colorize as undefined, - // makes it behave as false. - | None => { - customLevels: logLevels, - customColors, - } - } - - prettyFactory(options) + + let baseOptions = { + customLevels: logLevels, + customColors, + } + + // Force colors when running in test runner + // Note: setting colorize as undefined makes it behave as false + let options = switch NodeJs.Process.process.env->Js.Dict.get("LOGGING_TEST_RUNNER") { + | Some(_) => {...baseOptions, colorize: true} + | None => baseOptions + } + + prettyFactory(options) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codegenerator/cli/npm/envio/src/bindings/Pino.resscenarios/test_codegen/scripts/update-log-snapshots.mjsscenarios/test_codegen/test/Logging_test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- scenarios/test_codegen/scripts/update-log-snapshots.mjs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{res,resi}
📄 CodeRabbit inference engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Always use ReScript 11 documentation
Never suggest ReasonML syntax
Never use[| item |]to create an array. Use[ item ]instead
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction
Use records when working with structured data, and objects to conveniently pass payload data between functions
Never use %raw to access object fields if you know the type
Files:
codegenerator/cli/npm/envio/src/bindings/Pino.res
**/*.res
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Prefer reading ReScript
.resmodules directly; ignore compiled.jsartifacts
Files:
codegenerator/cli/npm/envio/src/bindings/Pino.res
🧠 Learnings (1)
📚 Learning: 2025-11-25T12:40:26.435Z
Learnt from: CR
Repo: enviodev/hyperindex PR: 0
File: .cursor/rules/rescript.mdc:0-0
Timestamp: 2025-11-25T12:40:26.435Z
Learning: Applies to **/*.test.{res,resi} : Never use `Js.Console.log` in test files. Use `Assert` module for all verifications
Applied to files:
scenarios/test_codegen/test/Logging_test.ts
🧬 Code graph analysis (1)
scenarios/test_codegen/test/Logging_test.ts (1)
scenarios/test_codegen/scripts/update-log-snapshots.mjs (5)
FIXTURE_PATH(5-5)SNAPSHOTS_DIR(6-6)normalize(17-22)output(29-36)snapshotPath(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (4)
scenarios/test_codegen/test/Logging_test.ts (3)
1-4: LGTM!Clean imports using only Node.js built-in modules, all of which are utilized in the file.
10-15: LGTM!The normalize function correctly handles both pretty and ECS timestamp formats and is consistent with the update script implementation.
38-42: LGTM!Clean test suite structure for console-based logging strategies.
codegenerator/cli/npm/envio/src/bindings/Pino.res (1)
135-135: No action needed — pino-pretty 13.1.3 fully supports thecolorize?: booleanoption in prettyFactory, and the binding is correct.
| const FIXTURE_PATH = "test/fixtures/LogTesting.res.mjs"; | ||
| const SNAPSHOTS_DIR = path.join(import.meta.dirname, "__snapshots__"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking Node.js version ==="
node --version
echo -e "\n=== Checking Logging_test.ts file ==="
if [ -f "scenarios/test_codegen/test/Logging_test.ts" ]; then
echo "File exists. Reading lines 1-20:"
head -20 "scenarios/test_codegen/test/Logging_test.ts" | cat -n
else
echo "File not found"
find . -name "Logging_test.ts" -type f
fi
echo -e "\n=== Checking update-log-snapshots.mjs ==="
if [ -f "scenarios/test_codegen/scripts/update-log-snapshots.mjs" ]; then
echo "File exists. Checking SNAPSHOTS_DIR and FIXTURE_PATH:"
grep -n "SNAPSHOTS_DIR\|FIXTURE_PATH" "scenarios/test_codegen/scripts/update-log-snapshots.mjs"
echo -e "\nFirst 15 lines of update script:"
head -15 "scenarios/test_codegen/scripts/update-log-snapshots.mjs" | cat -n
else
echo "Update script not found"
find . -name "update-log-snapshots.mjs" -type f
fi
echo -e "\n=== Checking if __snapshots__ directories exist ==="
find scenarios/test_codegen -type d -name "__snapshots__"
echo -e "\n=== Checking package.json for Node.js version requirement ==="
if [ -f "package.json" ]; then
grep -A 2 -B 2 "engines\|node" package.json | head -20
fiRepository: enviodev/hyperindex
Length of output: 2082
Inconsistent snapshot directory path construction between test and update script.
The snapshot directory paths differ between files:
- Test file (line 7):
path.join(import.meta.dirname, "__snapshots__")— absolute path relative to the test file - Update script (line 6):
"test/__snapshots__"— relative path dependent on working directory
This inconsistency could cause issues if the scripts are executed from different directories. Both should use the same path construction approach (preferably import.meta.dirname) for consistency and robustness.
🤖 Prompt for AI Agents
In @scenarios/test_codegen/test/Logging_test.ts around lines 6 - 7, The snapshot
path construction is inconsistent: tests use SNAPSHOTS_DIR =
path.join(import.meta.dirname, "__snapshots__") while the update script
hardcodes "test/__snapshots__"; update the update script to construct its
snapshots directory the same way (use path.join(import.meta.dirname,
"__snapshots__") or equivalent) so both FIXTURE_PATH/SNAPSHOTS_DIR and the
update script resolve snapshots absolutely and consistently.
Before:
After:
Summary by CodeRabbit
New Features
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.