require setting logger output channel to use it #8
Conversation
Reviewer's GuideThis PR refactors the host logger to require explicit assignment of the VS Code OutputChannel via a new setter, adds a console-based fallback using the existing createConsoleLogger, adjusts disposal to guard against undefined channels, and exports createConsoleLogger for wider reuse. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@hbmartin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughExports the console logger factory from the client module and refactors the host logger to support a runtime-settable OutputChannel with console fallback. Adds Logger.setOutputChannel, makes outputChannel optional, routes logs conditionally, and updates disposal and data-stringification paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Ext as Extension Code
participant L as Logger
participant OC as VS Code OutputChannel
participant CON as Console Logger
note over L: Initialization
L->>L: outputChannel = undefined
L->>CON: consoleLogger = createConsoleLogger("RVW-IPC")
Ext->>L: setOutputChannel(OC)
alt Previous channel exists
L->>OC: dispose()
else No previous channel
note over L: No-op
end
L->>L: outputChannel = OC
Ext->>L: log(level, line, data?)
alt outputChannel is set
L->>L: prepare line + (optional) data
L->>OC: appendLine(line)
opt data present
L->>OC: appendLine(stringify(data))
end
else no outputChannel
L->>CON: log(level, line, data?)
end
Ext->>L: dispose()
L->>OC: dispose()? (optional)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @hbmartin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the logging mechanism to provide greater control over where log messages are directed. It introduces the ability to explicitly set a VS Code output channel for logging, and crucially, implements a fallback to console logging if no dedicated output channel has been configured. This change enhances the robustness of the logging system, ensuring that log messages are always captured, even in scenarios where the VS Code output channel might not be immediately available or desired.
Highlights
- Flexible Output Channel: The logging system now allows the VS Code output channel to be explicitly set, rather than being implicitly created at initialization.
- Console Fallback: If no specific VS Code output channel is set, the logger will now default to outputting messages to the console, ensuring logs are always captured.
- Exported Console Logger: The
createConsoleLoggerutility function has been exported fromuseLogger.ts, making it reusable for other modules. - New
setOutputChannelMethod: A new methodsetOutputChannelhas been added to theLoggerinterface, enabling dynamic configuration of the output channel after the logger has been initialized.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/lib/host/logger.ts:79-95` </location>
<code_context>
- if (cleanedData === undefined) {
- this.outputChannel.appendLine(line);
+ if (this.outputChannel === undefined) {
+ switch (level) {
+ case LogLevel.DEBUG: {
+ this.consoleLogger.debug(line, cleanedData);
+ break;
+ }
+ case LogLevel.INFO: {
+ this.consoleLogger.info(line, cleanedData);
+ break;
+ }
+ case LogLevel.WARN: {
+ this.consoleLogger.warn(line, cleanedData);
+ break;
+ }
+ case LogLevel.ERROR: {
+ this.consoleLogger.error(line, cleanedData);
+ break;
+ }
</code_context>
<issue_to_address>
**suggestion:** Switch statement lacks a default case for unknown log levels.
Add a default case to ensure unknown log levels are handled appropriately, such as logging them as 'info' or 'error'.
```suggestion
switch (level) {
case LogLevel.DEBUG: {
this.consoleLogger.debug(line, cleanedData);
break;
}
case LogLevel.INFO: {
this.consoleLogger.info(line, cleanedData);
break;
}
case LogLevel.WARN: {
this.consoleLogger.warn(line, cleanedData);
break;
}
case LogLevel.ERROR: {
this.consoleLogger.error(line, cleanedData);
break;
}
default: {
this.consoleLogger.info(`[UNKNOWN LOG LEVEL] ${line}`, cleanedData);
break;
}
```
</issue_to_address>
### Comment 2
<location> `src/lib/host/logger.ts:98-99` </location>
<code_context>
- try {
- this.outputChannel.appendLine(`${line} : ${JSON.stringify(cleanedData)}`);
- } catch {
- this.outputChannel.appendLine(`${line} : unserializable data`);
+ if (cleanedData === undefined) {
+ this.outputChannel.appendLine(line);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching all errors when serializing cleanedData may mask unexpected issues.
Consider logging the specific error when serialization fails for reasons other than circular references to improve debuggability.
```suggestion
if (cleanedData === undefined) {
this.outputChannel.appendLine(line);
} else {
try {
this.outputChannel.appendLine(`${line} : ${JSON.stringify(cleanedData)}`);
} catch (err) {
this.outputChannel.appendLine(`${line} : unserializable data (${err instanceof Error ? err.message : String(err)})`);
}
}
```
</issue_to_address>
### Comment 3
<location> `src/lib/host/logger.ts:78` </location>
<code_context>
const line = `[${timestamp}] [${levelStr}] ${message}`;
- if (cleanedData === undefined) {
- this.outputChannel.appendLine(line);
+ if (this.outputChannel === undefined) {
+ switch (level) {
+ case LogLevel.DEBUG: {
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the log method to use a helper for output channel writes and map log levels to console methods, reducing nested branching.
Here’s one way to simplify the branching and remove the big switch, by
1. extracting your VS-Code‐channel logic into one helper,
2. mapping each LogLevel to the console-logger method name,
3. and having a single `if/else` instead of nested blocks.
```ts
class LoggerImpl {
// … keep your existing props …
private static writeToChannel(line: string, data?: unknown) {
if (!data) {
this.outputChannel!.appendLine(line);
return;
}
try {
this.outputChannel!.appendLine(`${line} : ${JSON.stringify(data)}`);
} catch {
this.outputChannel!.appendLine(`${line} : unserializable data`);
}
}
private static log(level: LogLevel, message: string, data?: Record<string, unknown>) {
const timestamp = new Date().toISOString().split('T')[1];
const levelStr = LogLevel[level] || 'UNKNOWN';
const cleanedData = removePromptsFromData(data);
const line = `[${timestamp}] [${levelStr}] ${message}`;
if (this.outputChannel) {
this.writeToChannel(line, cleanedData);
return;
}
// map LogLevel to console method
const methodName = LogLevel[level].toLowerCase() as 'debug' | 'info' | 'warn' | 'error';
this.consoleLogger[methodName](line, cleanedData);
}
}
```
Steps:
1. Pull your existing appendLine+try/catch block into `writeToChannel(...)`.
2. Replace the `switch(level)` with
```ts
const methodName = LogLevel[level].toLowerCase() as keyof typeof this.consoleLogger;
this.consoleLogger[methodName](line, cleanedData);
```
3. Use one `if (outputChannel) … else …` instead of nested `if/else` + `switch`.
This preserves all behavior but is much easier to follow.
</issue_to_address>
### Comment 4
<location> `src/lib/host/logger.ts:78-107` </location>
<code_context>
if (this.outputChannel === undefined) {
switch (level) {
case LogLevel.DEBUG: {
this.consoleLogger.debug(line, cleanedData);
break;
}
case LogLevel.INFO: {
this.consoleLogger.info(line, cleanedData);
break;
}
case LogLevel.WARN: {
this.consoleLogger.warn(line, cleanedData);
break;
}
case LogLevel.ERROR: {
this.consoleLogger.error(line, cleanedData);
break;
}
}
} else {
if (cleanedData === undefined) {
this.outputChannel.appendLine(line);
} else {
try {
this.outputChannel.appendLine(`${line} : ${JSON.stringify(cleanedData)}`);
} catch {
this.outputChannel.appendLine(`${line} : unserializable data`);
}
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge else clause's nested if statement into `else if` ([`merge-else-if`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/merge-else-if))
```suggestion
if (this.outputChannel === undefined) {
switch (level) {
case LogLevel.DEBUG: {
this.consoleLogger.debug(line, cleanedData);
break;
}
case LogLevel.INFO: {
this.consoleLogger.info(line, cleanedData);
break;
}
case LogLevel.WARN: {
this.consoleLogger.warn(line, cleanedData);
break;
}
case LogLevel.ERROR: {
this.consoleLogger.error(line, cleanedData);
break;
}
}
}
else if (cleanedData === undefined) {
this.outputChannel.appendLine(line);
}
else {
try {
this.outputChannel.appendLine(`${line} : ${JSON.stringify(cleanedData)}`);
} catch {
this.outputChannel.appendLine(`${line} : unserializable data`);
}
}
```
<br/><details><summary>Explanation</summary>Flattening if statements nested within else clauses generates code that is
easier to read and expand upon.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (cleanedData === undefined) { | ||
| this.outputChannel.appendLine(line); |
There was a problem hiding this comment.
suggestion (bug_risk): Catching all errors when serializing cleanedData may mask unexpected issues.
Consider logging the specific error when serialization fails for reasons other than circular references to improve debuggability.
| if (cleanedData === undefined) { | |
| this.outputChannel.appendLine(line); | |
| if (cleanedData === undefined) { | |
| this.outputChannel.appendLine(line); | |
| } else { | |
| try { | |
| this.outputChannel.appendLine(`${line} : ${JSON.stringify(cleanedData)}`); | |
| } catch (err) { | |
| this.outputChannel.appendLine(`${line} : unserializable data (${err instanceof Error ? err.message : String(err)})`); | |
| } | |
| } |
PR Code Suggestions ✨Latest suggestions up to 90aea5f
Previous suggestions✅ Suggestions up to commit 6fdeffc
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Code Review
This pull request refactors the logger to require an output channel to be explicitly set, falling back to the console if one is not provided. This is a good change for flexibility. The implementation is solid, but I have a few suggestions to improve code organization, maintainability, and clarity. My feedback includes suggestions to improve module dependencies, refactor a repetitive code block, and add documentation to clarify resource ownership.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 6fdeffc in 1 minute and 43 seconds. Click for details.
- Reviewed
110lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lib/client/useLogger.ts:19
- Draft comment:
Exporting createConsoleLogger makes it accessible for host logging. Confirm that the no-op dispose is intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is asking for confirmation about implementation choices rather than pointing out clear issues. The empty dispose makes sense for a console logger since console logging doesn't need cleanup. Making the function exported is a deliberate change that the author chose to make. There's no evidence this is problematic. Maybe there's a legitimate reason why console loggers should have real dispose functionality that I'm not aware of? Maybe exposing this for host logging is architecturally problematic? Without strong evidence that no-op dispose is wrong or that host logging access is problematic, we should trust the author's architectural decisions. The comment is just asking for confirmation rather than identifying clear issues. Delete the comment as it's asking for confirmation of intentional choices rather than identifying clear problems that need fixing.
2. src/lib/host/logger.ts:50
- Draft comment:
Changing outputChannel to optional allows fallback to console logging. Ensure this aligns with expected lifecycle management. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is asking the author to ensure that the change aligns with expected lifecycle management, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. It is more of a general cautionary note.
3. src/lib/host/logger.ts:113
- Draft comment:
setOutputChannel now disposes any existing channel before setting a new one. Confirm that disposing the old channel (via LoggerImpl.dispose()) is the intended behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. src/lib/host/logger.ts:78
- Draft comment:
Switch-case for log levels when outputChannel is undefined is clear. Consider adding a default case to handle unknown LogLevel values. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The code already handles unknown log levels by converting them to 'UNKNOWN' string earlier in the function. If an unknown level is passed, it will still create a log line with [UNKNOWN] prefix. Not having a default case here means the log will be silently dropped for unknown levels when outputChannel is undefined, which could be a valid concern. However, LogLevel appears to be an enum, so unknown values shouldn't be possible unless there's type casting. I might be assuming too much about LogLevel being an enum. Without seeing the LogLevel definition, we can't be certain that other values aren't possible. Even if LogLevel allows other values, silently dropping unknown log levels when outputChannel is undefined but logging them when it's defined seems like an intentional design choice rather than an oversight. The comment raises a valid point but the current behavior seems intentional and the change would be low impact. The comment should be removed as it's not clearly showing a problem that needs fixing.
5. src/lib/host/logger.ts:101
- Draft comment:
Good use of try/catch for JSON.stringify. This ensures unserializable data is handled gracefully. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_DhbptfJ57TwU9ETj
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/lib/client/useLogger.ts (1)
19-31: Export looks good; consider relocating to a React‑free shared moduleExporting
createConsoleLoggeris fine. However, the host logger imports it from this client file, which pulls in a React import at module top. That can bloat the host bundle or fail at runtime if the host build doesn’t include React. Prefer movingcreateConsoleLoggerto a small shared module with no React deps (e.g.,src/lib/shared/consoleLogger.ts) and import it from both places.src/lib/host/logger.ts (1)
19-21: structuredClone can throw before the try/catch
structuredClone(dictionary)may throw (functions, DOM nodes, circular refs). The call sits outside the try/catch, so logging can crash. Wrap clone creation and fall back gracefully.Apply this diff:
- const clone = structuredClone(dictionary) as Record<string, unknown>; - - try { + let clone: Record<string, unknown>; + try { + clone = structuredClone(dictionary) as Record<string, unknown>; for (const key in clone) { const value = clone[key]; @@ - } catch (error) { + } catch (error) { console.error('Error processing log data:', error); return {} as T; }Also applies to: 36-41
🧹 Nitpick comments (6)
src/lib/client/useLogger.ts (1)
19-31: Avoid logging a trailing “undefined”Console calls always pass the second arg, which prints
undefinedwhen no data is provided. Only includedatawhen defined.Apply this diff:
export function createConsoleLogger(tag: string): ILogger { return { - debug: (message: string, data?: Record<string, unknown>) => - console.debug(`[${tag}] ${message}`, data), - info: (message: string, data?: Record<string, unknown>) => - console.info(`[${tag}] ${message}`, data), - warn: (message: string, data?: Record<string, unknown>) => - console.warn(`[${tag}] ${message}`, data), - error: (message: string, data?: Record<string, unknown>) => - console.error(`[${tag}] ${message}`, data), + debug: (message: string, data?: Record<string, unknown>) => + data === undefined ? console.debug(`[${tag}] ${message}`) : console.debug(`[${tag}] ${message}`, data), + info: (message: string, data?: Record<string, unknown>) => + data === undefined ? console.info(`[${tag}] ${message}`) : console.info(`[${tag}] ${message}`, data), + warn: (message: string, data?: Record<string, unknown>) => + data === undefined ? console.warn(`[${tag}] ${message}`) : console.warn(`[${tag}] ${message}`, data), + error: (message: string, data?: Record<string, unknown>) => + data === undefined ? console.error(`[${tag}] ${message}`) : console.error(`[${tag}] ${message}`, data), dispose: () => {}, }; }src/lib/host/logger.ts (5)
22-35: Prefer Object.entries to avoid inherited keys and simplify typingIterate over own keys only and handle values directly.
Apply this diff after moving cloning into try/catch:
- for (const key in clone) { - const value = clone[key]; + for (const [key, value] of Object.entries(clone)) { if (disallowedLogKeys.includes(key)) { // eslint-disable-next-line @typescript-eslint/no-dynamic-delete delete clone[key]; continue; } if (Array.isArray(value)) { // eslint-disable-next-line @typescript-eslint/no-unsafe-return clone[key] = value.map((item) => removePromptsFromData(item)) as unknown; } else if (typeof value === 'object' && value !== null) { clone[key] = removePromptsFromData(value as Record<string, unknown>) as unknown; } }
22-25: Consider case‑insensitive redaction and broader keysKeys like
Authorization,Set-Cookie, oraccessTokenwon’t match. Normalize to lower case and expand the list as needed.Apply this diff:
-export const disallowedLogKeys = ['password', 'secret', 'token', 'apiKey', 'apiSecret', 'content']; +export const disallowedLogKeys = [ + 'password', + 'secret', + 'token', + 'apikey', + 'apisecret', + 'accesstoken', + 'authorization', + 'cookie', + 'set-cookie', + 'content', +];And when checking keys:
- if (disallowedLogKeys.includes(key)) { + if (disallowedLogKeys.includes(key.toLowerCase())) {
78-96: Optional: add a default to the switchDefensive default avoids silent no‑ops if an unknown level appears.
Apply this diff:
switch (level) { @@ - case LogLevel.ERROR: { + case LogLevel.ERROR: { this.consoleLogger.error(line, cleanedData); break; } + default: { + this.consoleLogger.warn(`${line} : unknown level`, cleanedData); + } }
98-105: Consider safer stringify for large/cyclic data
JSON.stringifystill throws on cycles; you handle it, but output drops details. If this is common, consider a safe stringify (custom replacer or a tiny helper) and cap payload size.
111-122: Allow unsetting the output channelLet callers clear the channel (back to console fallback) without creating a new one.
Apply this diff:
-export const Logger: ILogger & { setOutputChannel: (outputChannel: vscode.OutputChannel) => void } = +export const Logger: ILogger & { setOutputChannel: (outputChannel?: vscode.OutputChannel) => void } = { - setOutputChannel: (outputChannel: vscode.OutputChannel) => { + setOutputChannel: (outputChannel?: vscode.OutputChannel) => { LoggerImpl.dispose(); - LoggerImpl.outputChannel = outputChannel; + LoggerImpl.outputChannel = outputChannel; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/client/useLogger.ts(1 hunks)src/lib/host/logger.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/lib/client/useLogger.ts (1)
src/lib/types/log.ts (1)
ILogger(6-12)
src/lib/host/logger.ts (2)
src/lib/client/useLogger.ts (1)
createConsoleLogger(19-31)src/lib/types/log.ts (1)
ILogger(6-12)
🪛 GitHub Actions: CI
src/lib/host/logger.ts
[error] 1-1: Step 'npm run lint' failed: ESLint error in logger.ts: All imports in the declaration are only used as types. Use 'import type'.
⏰ 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: Sourcery review
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
|
Persistent suggestions updated to latest commit 90aea5f |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 90aea5f in 1 minute and 58 seconds. Click for details.
- Reviewed
176lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lib/client/useLogger.ts:3
- Draft comment:
Good refactor: duplicate implementation of createConsoleLogger was removed and now imported from utils, which reduces redundancy. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/lib/host/logger.ts:21
- Draft comment:
When filtering disallowed keys, the code lowercases the key but the disallowedLogKeys array contains mixed-case items (e.g. 'apiKey'). For consistent matching, consider storing disallowed keys in lowercase or normalizing them. - Reason this comment was not posted:
Comment was on unchanged code.
3. src/lib/host/logger.ts:48
- Draft comment:
The consoleLogger is now created with the tag 'RVW' instead of 'RVW-IPC'. Please confirm that this tag change is intentional for consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/lib/host/logger.ts:92
- Draft comment:
Robust error handling is implemented in the JSON serialization block with a try-catch fallback to log 'unserializable data'. This enhances resilience against data that can't be stringified. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/lib/utils.ts:14
- Draft comment:
Centralizing the createConsoleLogger implementation in utils improves code reuse. The implementation is straightforward and in line with logging best practices. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_XHqHKCUWjp9QpJfm
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 15e7253 in 1 minute and 9 seconds. Click for details.
- Reviewed
114lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lib/host/logger.ts:120
- Draft comment:
Good improvement: checking if the new output channel is identical avoids unnecessary disposal. Consider adding an inline comment to document this intent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/lib/host/logger.ts:5
- Draft comment:
Changing disallowedLogKeys from an array to a Set improves lookup efficiency and enforces case-insensitive checks. Ensure tests and documentation reflect the lowercased API keys. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. tests/lib/host/test.ts:13
- Draft comment:
Test updated to verify disallowedLogKeys is a Set. This correctly aligns with the recent implementation change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_jlDGKKAvDHPpzn2C
You can customize
Summary by Sourcery
Require explicit assignment of the logger output channel and add a console-based fallback logger.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes
Important
Require explicit logger output channel setting, add console fallback, and enhance data sanitization.
Loggernow requires explicitsetOutputChannel()to configure the output channel inlogger.ts.createConsoleLoggerinutils.tswhen no output channel is set.disallowedLogKeyschanged from array toSetfor better performance.removePromptsFromData()to handle nested objects and arrays.host.test.tsandlogger.test.tsto verify new logger behavior and data sanitization.disallowedLogKeysupdated to reflect change toSet.This description was created by
for 15e7253. You can customize this summary. It will automatically update as commits are pushed.