refactor: acta#21
Merged
Merged
Conversation
📝 WalkthroughWalkthrough此 PR 系统性重构日志库：用 Styled 替换旧颜色函数，改写 Filter/Config 语义，简化 layer 构建与 TracingGuard 的运行时更新路径，重写格式化器路径截断逻辑，移除 AsyncWriter 的写入计数并同步更新文档与测试。 Changes颜色与配置重构
Sequence Diagram — 初始化与 reload 装配： sequenceDiagram
participant Client as init(config)
participant LayerBuilder as build_layer(writer)
participant ColorDetect as detect_color_depth
participant EnvFilter as EnvFilter(reload)
participant Registry
Client->>LayerBuilder: 为每个 writer 请求 BoxedLayer
LayerBuilder->>ColorDetect: 检查/决定 ColorDepth
ColorDetect-->>LayerBuilder: 返回 ColorDepth
LayerBuilder-->>Client: 返回 BoxedLayer
Client->>EnvFilter: 使用 Filter 构造 EnvFilter
EnvFilter->>Registry: 包装到 Registry
Registry-->>Client: 全局 subscriber 设置完成
Estimated code review effort 🎯 4 (复杂) | ⏱️ ~75 分钟
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
Reviewer's Guide此变更重构了 acta 的日志管线：围绕 boxed layer 向量和新的 Filter/Config API 搭建框架，现代化了颜色与格式化工具（包括构建期的路径宽度支持），简化了异步/文件 writer 的内部实现，并移除低层级的 reload 钩子，改为通过 init/TracingGuard 流程进行控制。 通过 TracingGuard 在运行时重载过滤器的时序图sequenceDiagram
actor App
participant Guard as TracingGuard
participant Filter as Filter
participant Env as EnvFilter
participant Handle as reload::Handle
App->>Guard: set_level(Level)
Guard->>Filter: Filter::new(Level)
Guard->>Guard: apply_current_filter()
Guard->>Env: EnvFilter::try_new(Filter::as_directive())
Env-->>Guard: EnvFilter
Guard->>Handle: modify(|f| *f = EnvFilter)
App->>Guard: set_filter(Filter)
Guard->>Env: EnvFilter::try_new(Filter::as_directive())
Env-->>Guard: EnvFilter
Guard->>Handle: modify(|f| *f = EnvFilter)
基于 init 的日志管线构建流程图flowchart LR
A[Config] -->|filter, writers| B[init]
B --> C[build_layers]
C -->|Vec<BoxedLayer>| D[Registry]
D --> E[EnvFilter]
E -->|Layer::new| F[reload::Layer]
F --> G[InnerSubscriber]
G --> H[set_global_default]
subgraph Writer_to_Layer
W[Writer] -->|build_layer_with| L1[BoxedLayer]
Wf[Writer target FileConfig] -->|build_file_layer| L2[BoxedLayer]
end
C -. uses .-> W
C -. uses .-> Wf
文件级改动
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 来：
Getting HelpOriginal review guide in EnglishReviewer's GuideRefactors acta’s logging pipeline around boxed layer vectors and a new Filter/Config API, modernizes color and formatting utilities (including build-time path-width support), simplifies async/file writer internals, and removes low-level reload hooks in favor of the init/TracingGuard flow. Sequence diagram for runtime filter reload via TracingGuardsequenceDiagram
actor App
participant Guard as TracingGuard
participant Filter as Filter
participant Env as EnvFilter
participant Handle as reload::Handle
App->>Guard: set_level(Level)
Guard->>Filter: Filter::new(Level)
Guard->>Guard: apply_current_filter()
Guard->>Env: EnvFilter::try_new(Filter::as_directive())
Env-->>Guard: EnvFilter
Guard->>Handle: modify(|f| *f = EnvFilter)
App->>Guard: set_filter(Filter)
Guard->>Env: EnvFilter::try_new(Filter::as_directive())
Env-->>Guard: EnvFilter
Guard->>Handle: modify(|f| *f = EnvFilter)
Flow diagram for init-based logging pipeline constructionflowchart LR
A[Config] -->|filter, writers| B[init]
B --> C[build_layers]
C -->|Vec<BoxedLayer>| D[Registry]
D --> E[EnvFilter]
E -->|Layer::new| F[reload::Layer]
F --> G[InnerSubscriber]
G --> H[set_global_default]
subgraph Writer_to_Layer
W[Writer] -->|build_layer_with| L1[BoxedLayer]
Wf[Writer target FileConfig] -->|build_file_layer| L2[BoxedLayer]
end
C -. uses .-> W
C -. uses .-> Wf
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the subscriber initialization by replacing a complex layered type system with a vector of boxed layers and simplifies the configuration and filtering logic. It also removes several dependencies, including arrayvec, smart-default, and derive_more. Feedback identifies critical bugs in the path formatting logic, including incorrect string manipulation that reverses directory names and potential panics due to unsafe string slicing on non-character boundaries. Additionally, the reviewer raised concerns regarding performance regressions caused by introducing heap allocations in the logging hot path.
Owner
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了 5 个问题，并提供了一些高层次的反馈：
- 新的
Formatter::format_path实现比较难以理解（多次分配、先反转再切分的逻辑等）；建议把截断逻辑抽取到一个有文档的小辅助函数中，并简化目录尾部计算逻辑，这样预期行为会更清晰、更容易推理。 - 在
color::style::Styled::from中，ANSI16 映射每次转换都会新建一个 16 元素数组并调用一次Palette::default()；你可以通过复用一个静态的调色板和颜色表（或者一个小的辅助函数），避免每次调用都重新构建它们，从而降低开销。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Formatter::format_path` implementation is quite hard to follow (multiple allocations, reverse-then-split logic, etc.); consider extracting the truncation logic into a small, documented helper and simplifying the directory tail calculation so the intended behavior is clearer and easier to reason about.
- In `color::style::Styled::from`, the ANSI16 mapping allocates a fresh 16-element array and calls `Palette::default()` on every conversion; you could reduce overhead by reusing a static palette and color table (or a small helper function) instead of rebuilding them per call.
## Individual Comments
### Comment 1
<location path="src/builder.rs" line_range="18-27" />
<code_context>
- }
-}
+pub(crate) type BoxedLayer = Box<dyn tracing_subscriber::Layer<Registry> + Send + Sync>;
+pub(crate) type InnerSubscriber = Layered<Vec<BoxedLayer>, Registry>;
+pub(crate) type ReloadHandle =
+ tracing_subscriber::reload::Handle<tracing_subscriber::EnvFilter, InnerSubscriber>;
</code_context>
<issue_to_address>
**issue (bug_risk):** 使用 `Vec<BoxedLayer>` 作为 `Registry` 的 `Layer` 看起来并不安全，而且很可能会因为孤儿规则（orphan rules）而无法通过编译。
`InnerSubscriber` 假设 `Vec<BoxedLayer>` 实现了 `Layer<Registry>`，从而让 `Registry::default().with(layers)` 能通过类型检查，但你不能在这里添加 `impl Layer<Registry> for Vec<BoxedLayer>`，因为根据孤儿规则，`Layer` 和 `Vec` 都是外部类型。这意味着在当前 crate 中不可能让 `Vec<BoxedLayer>` 满足 `Layer<S>` 约束。
如果现在代码是可以编译的，很可能说明 `InnerSubscriber` 已经不再匹配实际的订阅者类型，因此 `reload::Handle<EnvFilter, InnerSubscriber>` 也是不正确的。相反，建议使用一个本地的新类型包装，例如 `struct Layers(Vec<BoxedLayer>); impl<S> Layer<S> for Layers where S: Subscriber + LookupSpan<'_> { … }`，然后在 `.with(...)` 和 `InnerSubscriber` 类型别名中都使用 `Layers`。
</issue_to_address>
### Comment 2
<location path="src/fmt/mod.rs" line_range="135-144" />
<code_context>
+ pub(crate) fn format_path(&self, file: &str, line: u32) -> String {
</code_context>
<issue_to_address>
**issue (bug_risk):** 新的路径压缩逻辑比较难以推理，而且很可能没有产生你期望的目录前缀。
新的实现会对 `[..last_slash]` 做反转，然后在反转后的字符串上按 `'/'` 切分。由于目录分隔符也跟着字符一起被反转，得到的片段不再和原始路径组件一一对应，很难真正表示“仍然能容纳的尾部目录名”。它还在一个热点路径上创建了若干中间 `String`，并做了额外的逐 `char` 处理。
更简单、更安全的方法是在概念上保持旧行为：在正向路径字节上操作，从左侧裁剪直到 `len <= max_width`，如果发生了裁剪就前置 `…`。如果你想保留文件名加部分目录上下文，可以：
1. 构造 `file_with_line = format!("{filename}:{line}")`。
2. 如果长度合适，就像现在一样右对齐。
3. 如果不合适，就计算在保证 `format!("{tail}:{line}")` 能放得下的前提下，可以保留 `path_str` 末尾的多少内容，或者退回到 `…` + 后缀的逻辑——整个过程不需要反转字符串。
这样逻辑更容易理解，也可以避免被破坏的目录名称。
</issue_to_address>
### Comment 3
<location path="src/fmt/test.rs" line_range="9" />
<code_context>
fn formatter_defaults() {
let fmt = Formatter::new();
assert_eq!(fmt.time_format, "%H:%M:%S");
- assert_eq!(fmt.path_width, BUILD_PATH_WIDTH);
+ assert_eq!(fmt.path_width, DEFAULT_PATH_WIDTH);
assert!(fmt.show_path);
assert!(fmt.show_spans);
</code_context>
<issue_to_address>
**suggestion (testing):** 为新的 `Formatter::format_path` 截断和规范化逻辑添加有针对性的测试。
当前新的 `Formatter::format_path` 逻辑（基于字符串的截断、`src/` 与 `src\` 的处理、省略号以及目录尾部启发式规则）除了这个默认宽度检查外，没有被覆盖到。
请添加一些聚焦的测试，用来：
- 覆盖短路径（无需截断，右对齐），
- 覆盖会变成相对路径并被截断的长 `src/` 路径，
- 验证 Windows 风格路径被规范为 `/`，
- 覆盖 `file_with_line.len() + 2 <= max_width` 这个分支，
- 覆盖最终带前导省略号的分支。
你可以通过将 `fmt.path_width` 设置为较小值并断言精确的格式化字符串，让这些测试具有确定性。
Suggested implementation:
```rust
fn formatter_defaults() {
let fmt = Formatter::new();
assert_eq!(fmt.time_format, "%H:%M:%S");
assert_eq!(fmt.path_width, DEFAULT_PATH_WIDTH);
assert!(fmt.show_path);
assert!(fmt.show_spans);
}
#[test]
fn format_path_short_no_truncation_right_aligned() {
let mut fmt = Formatter::new();
// Wide enough that no truncation happens
fmt.path_width = 40;
let formatted = fmt.format_path("src/lib.rs", 10);
// No truncation markers
assert!(!formatted.contains('…'), "expected no ellipsis: {formatted}");
assert!(!formatted.starts_with("..."), "expected no ascii ellipsis: {formatted}");
// Should end with the full file-with-line tail and be right-aligned within the field
assert!(formatted.ends_with("src/lib.rs:10"), "expected to end with full path: {formatted}");
assert!(
formatted.len() <= fmt.path_width,
"formatted length {} should not exceed path_width {}",
formatted.len(),
fmt.path_width
);
}
#[test]
fn format_path_long_src_path_truncated_relative() {
let mut fmt = Formatter::new();
// Force truncation even after making it relative to src/
fmt.path_width = 24;
let formatted = fmt.format_path(
"/home/user/project/src/module/submodule/deeply_nested_file_name.rs",
42,
);
// Should be made relative to src/ and then truncated
assert!(
!formatted.contains("/home/user/project/"),
"expected path to be made relative to src/: {formatted}",
);
assert!(
formatted.contains("deeply_nested_file_name.rs:42") || formatted.contains("file_name.rs:42"),
"expected file name with line to be present in truncated output: {formatted}",
);
// Truncation marker and width bound
assert!(
formatted.contains('…') || formatted.starts_with("..."),
"expected some form of ellipsis/truncation marker: {formatted}",
);
assert!(
formatted.len() <= fmt.path_width,
"formatted length {} should not exceed path_width {}",
formatted.len(),
fmt.path_width
);
}
#[test]
fn format_path_windows_style_paths_normalized() {
let mut fmt = Formatter::new();
fmt.path_width = 40;
let formatted = fmt.format_path(r"C:\project\src\module\file.rs", 7);
// Windows separators should be normalized to '/'
assert!(
!formatted.contains('\\'),
"expected backslashes to be normalized out: {formatted}",
);
assert!(
formatted.contains("src/module/file.rs:7"),
"expected normalized src path with line: {formatted}",
);
}
#[test]
fn format_path_hits_file_with_line_branch() {
let mut fmt = Formatter::new();
// Choose a width such that `file_with_line.len() + 2 <= max_width`
// so that branch is exercised without needing truncation.
let file_with_line = "src/main.rs:1";
let max_width = file_with_line.len() + 2;
fmt.path_width = max_width as u16;
let formatted = fmt.format_path("src/main.rs", 1);
// We expect the full file-with-line to appear without truncation markers
assert!(
formatted.contains(file_with_line),
"expected full file_with_line in output: {formatted}",
);
assert!(
!formatted.contains('…') && !formatted.starts_with("..."),
"expected no ellipsis for file_with_line branch: {formatted}",
);
assert!(
formatted.len() <= fmt.path_width as usize,
"formatted length {} should not exceed path_width {}",
formatted.len(),
fmt.path_width
);
}
#[test]
fn format_path_leading_ellipsis_for_very_long_paths() {
let mut fmt = Formatter::new();
// Very narrow width to ensure we hit the final leading-ellipsis truncation branch
fmt.path_width = 16;
let formatted = fmt.format_path(
"/very/long/prefix/that/is/not/src/and/should/be/trimmed/file.rs",
99,
);
// Final branch should produce a leading ellipsis and keep just the tail
assert!(
formatted.starts_with('…') || formatted.starts_with("..."),
"expected leading ellipsis for aggressively truncated path: {formatted}",
);
assert!(
formatted.contains("file.rs:99"),
"expected file name with line to be preserved in final-branch truncation: {formatted}",
);
assert!(
formatted.len() <= fmt.path_width,
"formatted length {} should not exceed path_width {}",
formatted.len(),
fmt.path_width
);
}
```
These tests assume:
1. `Formatter::format_path(&self, path: &str, line: u32) -> String` exists and is accessible from this module.
2. `Formatter::path_width` is a public or otherwise settable numeric field (here treated as `u16`; adjust the cast or type if it differs).
3. The implementation normalizes `\` to `/`, makes absolute `src/`-containing paths relative to `src/`, and uses either the Unicode ellipsis `…` or a `"..."` prefix for truncation.
If the actual behavior differs (e.g., different ellipsis marker, no right-alignment, different relativization rule), update the assertions to match the concrete formatting rules while keeping the test structure (short/no truncation, long `src/` + truncation, Windows normalization, `file_with_line.len() + 2 <= max_width` branch, and final leading-ellipsis branch) the same.
</issue_to_address>
### Comment 4
<location path="src/test.rs" line_range="18-27" />
<code_context>
+fn build_test_guard(level: Level, style: Style) -> (TracingGuard, TestSubscriber) {
</code_context>
<issue_to_address>
**suggestion (testing):** 为新的 `TracingGuard::set_filter` 以及带原始指令（raw directive）的 level/target API 添加测试覆盖。
现有测试覆盖了 `with_style`、`set_target_level` 和 `remove_target_level`，但新的 `TracingGuard::set_filter` API 和更严格的 `Level`/`Filter` 语义目前还没有测试。
请添加一些测试，例如：
- 通过 `build_test_guard(Level::Info, ...)` 构建一个 guard，调用 `set_filter(Filter::from_directive("info,my_crate=debug"))`，并断言 `guard.filter.as_directive()` 匹配该指令且调用返回 `Ok(())`。
- 调用 `set_level(Level::Warn)` 并断言 `guard.filter.as_directive()` 变为 `"warn"` 且没有错误。
- 在基于原始指令的 `set_filter` 之后调用 `set_target_level("demo", Level::Trace)`，并断言每个 target 的覆盖设置被正确反映且调用成功。
这些测试将有助于锁定新的公共 API 行为，而无需依赖 `EnvFilter` 的内部细节。
Suggested implementation:
```rust
fn build_test_guard(level: Level, style: Style) -> (TracingGuard, TestSubscriber) {
let filter = Filter::new(level);
let env_filter =
tracing_subscriber::EnvFilter::try_new(filter.as_directive()).unwrap_or_default();
let (env_layer, raw): (_, ReloadHandle) = tracing_subscriber::reload::Layer::new(env_filter);
let layers: Vec<BoxedLayer> = vec![
tracing_subscriber::fmt::Layer::default()
.with_writer(io::sink)
.boxed(),
];
// NOTE: The rest of `build_test_guard` is assumed to remain unchanged.
}
#[cfg(test)]
mod tracing_guard_filter_tests {
use super::*;
#[test]
fn set_filter_with_raw_directive_updates_guard() {
// Start from an initial guard with a simple level-based filter.
let (mut guard, _subscriber) = build_test_guard(Level::Info, Style::default());
// Apply a raw directive that includes a per-target override.
let filter = Filter::from_directive("info,my_crate=debug");
guard
.set_filter(filter)
.expect("set_filter with raw directive should succeed");
// The guard's filter should now match the directive we provided.
assert_eq!(
guard.filter.as_directive(),
"info,my_crate=debug",
"guard.filter.as_directive() should reflect the raw directive applied via set_filter"
);
}
#[test]
fn set_level_after_filter_replaces_with_simple_level_directive() {
let (mut guard, _subscriber) = build_test_guard(Level::Info, Style::default());
// First, set a more complex filter.
guard
.set_filter(Filter::from_directive("info,my_crate=debug"))
.expect("set_filter should succeed");
// Now replace it with a simple level using the level API.
guard
.set_level(Level::Warn)
.expect("set_level(Level::Warn) should succeed");
// After calling set_level, the directive should be a simple level directive.
assert_eq!(
guard.filter.as_directive(),
"warn",
"set_level should replace the filter with a simple level directive"
);
}
#[test]
fn set_target_level_after_raw_directive_adds_per_target_override() {
let (mut guard, _subscriber) = build_test_guard(Level::Info, Style::default());
guard
.set_filter(Filter::from_directive("info,my_crate=debug"))
.expect("initial set_filter should succeed");
// Add a per-target override after the raw directive has been applied.
guard
.set_target_level("demo", Level::Trace)
.expect("set_target_level should succeed after set_filter");
let directive = guard.filter.as_directive();
// The overall directive should still contain the base level and original override.
assert!(
directive.contains("info"),
"directive should still include base level after set_target_level: {directive}"
);
assert!(
directive.contains("my_crate=debug"),
"directive should retain existing per-target override after set_target_level: {directive}"
);
assert!(
directive.contains("demo=trace"),
"directive should include the new per-target override added via set_target_level: {directive}"
);
}
```
The replacement above assumes:
1. The snippet you shared is the full body of `build_test_guard`. If there is additional code in that function, reinsert it between the `let layers: Vec<BoxedLayer> = vec![ ... ];` line and the closing `}` so the helper continues to build and return a valid `(TracingGuard, TestSubscriber)`.
2. `Style` implements `Default`. If it does not, replace `Style::default()` in the tests with whatever style constructor you use elsewhere in your test suite (for example, `Style::Pretty` or a crate-specific default).
3. `Level` uses `CamelCase` variants (`Level::Info`, `Level::Warn`, `Level::Trace`) as implied by your review comment. If instead you re-export `tracing::Level`, adjust the variants to `Level::INFO`, `Level::WARN`, and `Level::TRACE`.
4. `Filter::from_directive(&str)` and `guard.filter` are available as shown in your comment. If `filter` is not public, replace direct field access with the appropriate accessor (for example, `guard.filter().as_directive()`).
</issue_to_address>
### Comment 5
<location path="src/color/style.rs" line_range="10" />
<code_context>
- let r = rgb.0;
- let g = rgb.1;
- let b = rgb.2;
+#[derive(Clone, Copy, Debug)]
+pub struct Styled {
+ rgb: Rgb,
</code_context>
<issue_to_address>
**issue (complexity):** 建议在 `Styled` 之上重新引入简单的公共辅助函数，并把 ANSI16 表提升为常量，这样既能让 API 意图更清晰，也能让实现更精简。
你可以保留现在新的共享转换逻辑，同时恢复更简单、更具意图表达能力的 API，并降低部分内部复杂度。
### 1. 在 `Styled` 之上重新引入小的辅助函数
暴露和之前相同形状的辅助函数，把 `Styled` + `From<Styled>` 作为内部实现细节。这可以保持调用点简单，避免泄露 `Styled` 这个抽象：
```rust
pub fn rgb_to_owo(rgb: (u8, u8, u8), depth: ColorDepth) -> OwoStyle {
Styled::new(Rgb(rgb.0, rgb.1, rgb.2), depth).into()
}
pub fn rgb_to_owo_on(r: u8, g: u8, b: u8, depth: ColorDepth) -> OwoStyle {
Styled::new(Rgb(r, g, b), depth).on().into()
}
/// Dims the foreground by dividing brightness by 4 to ensure high text legibility.
pub fn theme_fg_dimmed(rgb: (u8, u8, u8), depth: ColorDepth) -> OwoStyle {
Styled::new(Rgb(rgb.0, rgb.1, rgb.2), depth).dimmed().into()
}
```
这样调用点就可以恢复成：
```rust
let style = rgb_to_owo(theme.text, self.color_depth);
// or
let style = rgb_to_owo_on(bg.0, bg.1, bg.2, self.color_depth);
// or
let style = theme_fg_dimmed(theme.text, self.color_depth);
```
同时仍然可以在 `From<Styled>` 内部复用更新后的映射逻辑。
如果不希望 `Styled` 成为公共类型，也可以将其设为 `pub(crate)`，并保留这些辅助函数作为公共 API。
### 2. 避免在每次调用时重建 ANSI16 颜色表
目前 `Ansi16` 分支在每次转换时都会分配一个 16 元素数组。你可以通过将其提升为常量来降低复杂度和开销：
```rust
const ANSI16_TABLE: [AnsiColors; 16] = [
AnsiColors::Black,
AnsiColors::Red,
AnsiColors::Green,
AnsiColors::Yellow,
AnsiColors::Blue,
AnsiColors::Magenta,
AnsiColors::Cyan,
AnsiColors::White,
AnsiColors::BrightBlack,
AnsiColors::BrightRed,
AnsiColors::BrightGreen,
AnsiColors::BrightYellow,
AnsiColors::BrightBlue,
AnsiColors::BrightMagenta,
AnsiColors::BrightCyan,
AnsiColors::BrightWhite,
];
```
然后：
```rust
ColorDepth::Ansi16 => {
let idx = rgb_to_ansi(s.as_tuple().into(), Palette::default()) as usize;
let ansi = ANSI16_TABLE
.get(idx)
.unwrap_or(&AnsiColors::White);
if s.on {
return style.on_color(ansi);
}
style.color(ansi)
}
```
这些改动可以保留所有新的行为（变暗处理、背景色、更新后的映射），同时让公共使用方式更简单、内部实现也更精简。
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据反馈改进以后的评审。
Original comment in English
Hey - I've found 5 issues, and left some high level feedback:
- The new
Formatter::format_pathimplementation is quite hard to follow (multiple allocations, reverse-then-split logic, etc.); consider extracting the truncation logic into a small, documented helper and simplifying the directory tail calculation so the intended behavior is clearer and easier to reason about. - In
color::style::Styled::from, the ANSI16 mapping allocates a fresh 16-element array and callsPalette::default()on every conversion; you could reduce overhead by reusing a static palette and color table (or a small helper function) instead of rebuilding them per call.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Formatter::format_path` implementation is quite hard to follow (multiple allocations, reverse-then-split logic, etc.); consider extracting the truncation logic into a small, documented helper and simplifying the directory tail calculation so the intended behavior is clearer and easier to reason about.
- In `color::style::Styled::from`, the ANSI16 mapping allocates a fresh 16-element array and calls `Palette::default()` on every conversion; you could reduce overhead by reusing a static palette and color table (or a small helper function) instead of rebuilding them per call.
## Individual Comments
### Comment 1
<location path="src/builder.rs" line_range="18-27" />
<code_context>
- }
-}
+pub(crate) type BoxedLayer = Box<dyn tracing_subscriber::Layer<Registry> + Send + Sync>;
+pub(crate) type InnerSubscriber = Layered<Vec<BoxedLayer>, Registry>;
+pub(crate) type ReloadHandle =
+ tracing_subscriber::reload::Handle<tracing_subscriber::EnvFilter, InnerSubscriber>;
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `Vec<BoxedLayer>` as a `Layer` for `Registry` appears unsound and likely won’t compile due to orphan rules.
`InnerSubscriber` assumes `Vec<BoxedLayer>` implements `Layer<Registry>` so that `Registry::default().with(layers)` type-checks, but you can’t add `impl Layer<Registry> for Vec<BoxedLayer>` here due to orphan rules (both `Layer` and `Vec` are foreign). That makes it impossible for `Vec<BoxedLayer>` to satisfy the `Layer<S>` bound in this crate.
If this currently compiles, it likely means `InnerSubscriber` no longer matches the actual subscriber type, so `reload::Handle<EnvFilter, InnerSubscriber>` would be incorrect. Instead, consider a local newtype wrapper, e.g. `struct Layers(Vec<BoxedLayer>); impl<S> Layer<S> for Layers where S: Subscriber + LookupSpan<'_> { … }`, and use `Layers` both in `.with(...)` and in the `InnerSubscriber` alias.
</issue_to_address>
### Comment 2
<location path="src/fmt/mod.rs" line_range="135-144" />
<code_context>
+ pub(crate) fn format_path(&self, file: &str, line: u32) -> String {
</code_context>
<issue_to_address>
**issue (bug_risk):** The new path-compaction logic is hard to reason about and likely not producing the intended directory prefix.
The new implementation reverses `[..last_slash]` and then splits that reversed string on `'/'`. Because the directory separators are reversed along with the characters, the resulting segments don’t map cleanly to original path components and are unlikely to represent “the trailing directory name that still fits.” It also creates several intermediate `String`s and does extra `char`-level work on a hot path.
A simpler, safer approach is to keep the old behaviour conceptually: operate on the forward path bytes, trim from the left until `len <= max_width`, and prepend `…` if trimming occurred. If you want to preserve filename + some directory context:
1. Build `file_with_line = format!("{filename}:{line}")`.
2. If it fits, right-align as you do now.
3. If not, compute how much of the *end* of `path_str` can be kept so that `format!("{tail}:{line}")` fits, or fall back to `…` + suffix logic—without reversing the string.
This keeps the logic easier to reason about and avoids mangled directory names.
</issue_to_address>
### Comment 3
<location path="src/fmt/test.rs" line_range="9" />
<code_context>
fn formatter_defaults() {
let fmt = Formatter::new();
assert_eq!(fmt.time_format, "%H:%M:%S");
- assert_eq!(fmt.path_width, BUILD_PATH_WIDTH);
+ assert_eq!(fmt.path_width, DEFAULT_PATH_WIDTH);
assert!(fmt.show_path);
assert!(fmt.show_spans);
</code_context>
<issue_to_address>
**suggestion (testing):** Add focused tests for the new `Formatter::format_path` truncation and normalization logic.
The new `Formatter::format_path` logic (string-based truncation, `src/` vs `src\` handling, ellipsis, and directory-tail heuristics) isn’t covered beyond this default width check.
Please add focused tests that:
- Cover short paths (no truncation, right-aligned),
- Cover long `src/` paths that become relative and truncated,
- Verify Windows-style paths are normalized to `/`,
- Hit the `file_with_line.len() + 2 <= max_width` branch,
- Hit the final leading-ellipsis branch.
You can make these deterministic by setting `fmt.path_width` to a small value and asserting exact formatted strings.
Suggested implementation:
```rust
fn formatter_defaults() {
let fmt = Formatter::new();
assert_eq!(fmt.time_format, "%H:%M:%S");
assert_eq!(fmt.path_width, DEFAULT_PATH_WIDTH);
assert!(fmt.show_path);
assert!(fmt.show_spans);
}
#[test]
fn format_path_short_no_truncation_right_aligned() {
let mut fmt = Formatter::new();
// Wide enough that no truncation happens
fmt.path_width = 40;
let formatted = fmt.format_path("src/lib.rs", 10);
// No truncation markers
assert!(!formatted.contains('…'), "expected no ellipsis: {formatted}");
assert!(!formatted.starts_with("..."), "expected no ascii ellipsis: {formatted}");
// Should end with the full file-with-line tail and be right-aligned within the field
assert!(formatted.ends_with("src/lib.rs:10"), "expected to end with full path: {formatted}");
assert!(
formatted.len() <= fmt.path_width,
"formatted length {} should not exceed path_width {}",
formatted.len(),
fmt.path_width
);
}
#[test]
fn format_path_long_src_path_truncated_relative() {
let mut fmt = Formatter::new();
// Force truncation even after making it relative to src/
fmt.path_width = 24;
let formatted = fmt.format_path(
"/home/user/project/src/module/submodule/deeply_nested_file_name.rs",
42,
);
// Should be made relative to src/ and then truncated
assert!(
!formatted.contains("/home/user/project/"),
"expected path to be made relative to src/: {formatted}",
);
assert!(
formatted.contains("deeply_nested_file_name.rs:42") || formatted.contains("file_name.rs:42"),
"expected file name with line to be present in truncated output: {formatted}",
);
// Truncation marker and width bound
assert!(
formatted.contains('…') || formatted.starts_with("..."),
"expected some form of ellipsis/truncation marker: {formatted}",
);
assert!(
formatted.len() <= fmt.path_width,
"formatted length {} should not exceed path_width {}",
formatted.len(),
fmt.path_width
);
}
#[test]
fn format_path_windows_style_paths_normalized() {
let mut fmt = Formatter::new();
fmt.path_width = 40;
let formatted = fmt.format_path(r"C:\project\src\module\file.rs", 7);
// Windows separators should be normalized to '/'
assert!(
!formatted.contains('\\'),
"expected backslashes to be normalized out: {formatted}",
);
assert!(
formatted.contains("src/module/file.rs:7"),
"expected normalized src path with line: {formatted}",
);
}
#[test]
fn format_path_hits_file_with_line_branch() {
let mut fmt = Formatter::new();
// Choose a width such that `file_with_line.len() + 2 <= max_width`
// so that branch is exercised without needing truncation.
let file_with_line = "src/main.rs:1";
let max_width = file_with_line.len() + 2;
fmt.path_width = max_width as u16;
let formatted = fmt.format_path("src/main.rs", 1);
// We expect the full file-with-line to appear without truncation markers
assert!(
formatted.contains(file_with_line),
"expected full file_with_line in output: {formatted}",
);
assert!(
!formatted.contains('…') && !formatted.starts_with("..."),
"expected no ellipsis for file_with_line branch: {formatted}",
);
assert!(
formatted.len() <= fmt.path_width as usize,
"formatted length {} should not exceed path_width {}",
formatted.len(),
fmt.path_width
);
}
#[test]
fn format_path_leading_ellipsis_for_very_long_paths() {
let mut fmt = Formatter::new();
// Very narrow width to ensure we hit the final leading-ellipsis truncation branch
fmt.path_width = 16;
let formatted = fmt.format_path(
"/very/long/prefix/that/is/not/src/and/should/be/trimmed/file.rs",
99,
);
// Final branch should produce a leading ellipsis and keep just the tail
assert!(
formatted.starts_with('…') || formatted.starts_with("..."),
"expected leading ellipsis for aggressively truncated path: {formatted}",
);
assert!(
formatted.contains("file.rs:99"),
"expected file name with line to be preserved in final-branch truncation: {formatted}",
);
assert!(
formatted.len() <= fmt.path_width,
"formatted length {} should not exceed path_width {}",
formatted.len(),
fmt.path_width
);
}
```
These tests assume:
1. `Formatter::format_path(&self, path: &str, line: u32) -> String` exists and is accessible from this module.
2. `Formatter::path_width` is a public or otherwise settable numeric field (here treated as `u16`; adjust the cast or type if it differs).
3. The implementation normalizes `\` to `/`, makes absolute `src/`-containing paths relative to `src/`, and uses either the Unicode ellipsis `…` or a `"..."` prefix for truncation.
If the actual behavior differs (e.g., different ellipsis marker, no right-alignment, different relativization rule), update the assertions to match the concrete formatting rules while keeping the test structure (short/no truncation, long `src/` + truncation, Windows normalization, `file_with_line.len() + 2 <= max_width` branch, and final leading-ellipsis branch) the same.
</issue_to_address>
### Comment 4
<location path="src/test.rs" line_range="18-27" />
<code_context>
+fn build_test_guard(level: Level, style: Style) -> (TracingGuard, TestSubscriber) {
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that exercise the new `TracingGuard::set_filter` and level/target APIs with raw directives.
The existing tests cover `with_style`, `set_target_level`, and `remove_target_level`, but the new `TracingGuard::set_filter` API and tighter `Level`/`Filter` semantics are untested.
Please add tests that, for example:
- Build a guard via `build_test_guard(Level::Info, ...)`, call `set_filter(Filter::from_directive("info,my_crate=debug"))`, and assert `guard.filter.as_directive()` matches and the call returns `Ok(())`.
- Call `set_level(Level::Warn)` and assert `guard.filter.as_directive()` becomes `"warn"` without error.
- Call `set_target_level("demo", Level::Trace)` after a `set_filter` based on a raw directive, asserting per-target overrides are correctly reflected and succeed.
These will help lock in the new public API behavior without depending on `EnvFilter` internals.
Suggested implementation:
```rust
fn build_test_guard(level: Level, style: Style) -> (TracingGuard, TestSubscriber) {
let filter = Filter::new(level);
let env_filter =
tracing_subscriber::EnvFilter::try_new(filter.as_directive()).unwrap_or_default();
let (env_layer, raw): (_, ReloadHandle) = tracing_subscriber::reload::Layer::new(env_filter);
let layers: Vec<BoxedLayer> = vec![
tracing_subscriber::fmt::Layer::default()
.with_writer(io::sink)
.boxed(),
];
// NOTE: The rest of `build_test_guard` is assumed to remain unchanged.
}
#[cfg(test)]
mod tracing_guard_filter_tests {
use super::*;
#[test]
fn set_filter_with_raw_directive_updates_guard() {
// Start from an initial guard with a simple level-based filter.
let (mut guard, _subscriber) = build_test_guard(Level::Info, Style::default());
// Apply a raw directive that includes a per-target override.
let filter = Filter::from_directive("info,my_crate=debug");
guard
.set_filter(filter)
.expect("set_filter with raw directive should succeed");
// The guard's filter should now match the directive we provided.
assert_eq!(
guard.filter.as_directive(),
"info,my_crate=debug",
"guard.filter.as_directive() should reflect the raw directive applied via set_filter"
);
}
#[test]
fn set_level_after_filter_replaces_with_simple_level_directive() {
let (mut guard, _subscriber) = build_test_guard(Level::Info, Style::default());
// First, set a more complex filter.
guard
.set_filter(Filter::from_directive("info,my_crate=debug"))
.expect("set_filter should succeed");
// Now replace it with a simple level using the level API.
guard
.set_level(Level::Warn)
.expect("set_level(Level::Warn) should succeed");
// After calling set_level, the directive should be a simple level directive.
assert_eq!(
guard.filter.as_directive(),
"warn",
"set_level should replace the filter with a simple level directive"
);
}
#[test]
fn set_target_level_after_raw_directive_adds_per_target_override() {
let (mut guard, _subscriber) = build_test_guard(Level::Info, Style::default());
guard
.set_filter(Filter::from_directive("info,my_crate=debug"))
.expect("initial set_filter should succeed");
// Add a per-target override after the raw directive has been applied.
guard
.set_target_level("demo", Level::Trace)
.expect("set_target_level should succeed after set_filter");
let directive = guard.filter.as_directive();
// The overall directive should still contain the base level and original override.
assert!(
directive.contains("info"),
"directive should still include base level after set_target_level: {directive}"
);
assert!(
directive.contains("my_crate=debug"),
"directive should retain existing per-target override after set_target_level: {directive}"
);
assert!(
directive.contains("demo=trace"),
"directive should include the new per-target override added via set_target_level: {directive}"
);
}
```
The replacement above assumes:
1. The snippet you shared is the full body of `build_test_guard`. If there is additional code in that function, reinsert it between the `let layers: Vec<BoxedLayer> = vec![ ... ];` line and the closing `}` so the helper continues to build and return a valid `(TracingGuard, TestSubscriber)`.
2. `Style` implements `Default`. If it does not, replace `Style::default()` in the tests with whatever style constructor you use elsewhere in your test suite (for example, `Style::Pretty` or a crate-specific default).
3. `Level` uses `CamelCase` variants (`Level::Info`, `Level::Warn`, `Level::Trace`) as implied by your review comment. If instead you re-export `tracing::Level`, adjust the variants to `Level::INFO`, `Level::WARN`, and `Level::TRACE`.
4. `Filter::from_directive(&str)` and `guard.filter` are available as shown in your comment. If `filter` is not public, replace direct field access with the appropriate accessor (for example, `guard.filter().as_directive()`).
</issue_to_address>
### Comment 5
<location path="src/color/style.rs" line_range="10" />
<code_context>
- let r = rgb.0;
- let g = rgb.1;
- let b = rgb.2;
+#[derive(Clone, Copy, Debug)]
+pub struct Styled {
+ rgb: Rgb,
</code_context>
<issue_to_address>
**issue (complexity):** Consider reintroducing simple public helper functions over `Styled` and hoisting the ANSI16 table into a constant to keep the API intention‑revealing and the implementation leaner.
You can keep the new shared conversion logic while restoring the simpler, intention‑revealing API and reducing some internal complexity.
### 1. Reintroduce small helper functions on top of `Styled`
Expose the same shape of helpers as before and keep `Styled` + `From<Styled>` as an internal detail. This keeps call sites simple and avoids leaking the `Styled` abstraction:
```rust
pub fn rgb_to_owo(rgb: (u8, u8, u8), depth: ColorDepth) -> OwoStyle {
Styled::new(Rgb(rgb.0, rgb.1, rgb.2), depth).into()
}
pub fn rgb_to_owo_on(r: u8, g: u8, b: u8, depth: ColorDepth) -> OwoStyle {
Styled::new(Rgb(r, g, b), depth).on().into()
}
/// Dims the foreground by dividing brightness by 4 to ensure high text legibility.
pub fn theme_fg_dimmed(rgb: (u8, u8, u8), depth: ColorDepth) -> OwoStyle {
Styled::new(Rgb(rgb.0, rgb.1, rgb.2), depth).dimmed().into()
}
```
Call sites then go back to:
```rust
let style = rgb_to_owo(theme.text, self.color_depth);
// or
let style = rgb_to_owo_on(bg.0, bg.1, bg.2, self.color_depth);
// or
let style = theme_fg_dimmed(theme.text, self.color_depth);
```
while still benefiting from the updated mapping logic inside `From<Styled>`.
If `Styled` is not meant to be public, you can also make it `pub(crate)` and keep these helpers as the public API.
### 2. Avoid rebuilding the ANSI16 color table on every call
The `Ansi16` arm currently allocates a 16‑element array on every conversion. You can reduce both complexity and overhead by hoisting it to a constant:
```rust
const ANSI16_TABLE: [AnsiColors; 16] = [
AnsiColors::Black,
AnsiColors::Red,
AnsiColors::Green,
AnsiColors::Yellow,
AnsiColors::Blue,
AnsiColors::Magenta,
AnsiColors::Cyan,
AnsiColors::White,
AnsiColors::BrightBlack,
AnsiColors::BrightRed,
AnsiColors::BrightGreen,
AnsiColors::BrightYellow,
AnsiColors::BrightBlue,
AnsiColors::BrightMagenta,
AnsiColors::BrightCyan,
AnsiColors::BrightWhite,
];
```
and then:
```rust
ColorDepth::Ansi16 => {
let idx = rgb_to_ansi(s.as_tuple().into(), Palette::default()) as usize;
let ansi = ANSI16_TABLE
.get(idx)
.unwrap_or(&AnsiColors::White);
if s.on {
return style.on_color(ansi);
}
style.color(ansi)
}
```
These changes preserve all new behavior (dimming, background, updated mapping) while making the public usage simpler and the internal implementation slightly leaner.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
/gemini review |
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Code Review
This pull request refactors the library's configuration and initialization API, replacing the Level field with a more robust Filter system and simplifying the subscriber layer management. It also reduces external dependencies and refactors the path formatting logic. Review feedback identifies a bug in how source paths are stripped when multiple 'src/' directories exist and points out an alignment issue in the path formatter that breaks log column consistency. A cleanup of redundant variables in the initialization logic was also suggested.
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并且有一些整体性的反馈：
- 新的
Formatter::format_path逻辑依赖通过在字符串中按"src/"/"src\\"分割并手动切片，这在非标准目录结构或存在多个src片段时会比较脆弱；建议改用基于Path/Path::strip_prefix的规范化方式，这样前缀剥离和分隔符规范化会更不易出错、也更具可移植性。 - 在
acta-build::walk_src_max_width中，目前对strip_prefix的处理是对字符串化后的路径使用find(strip_prefix)；如果该前缀出现多次（或分隔符混用），可能会导致路径被错误地剥离——在可能的情况下使用Path::strip_prefix，仅在必要时回退到字符串操作，可以让计算出来的宽度在不同平台上更加准确。
给 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- The new `Formatter::format_path` logic relies on string splitting on `"src/"` / `"src\\"` and manual slicing, which can be brittle for non-standard layouts or multiple `src` segments; consider switching to `Path` / `Path::strip_prefix`-based normalization so prefix-stripping and separator normalization are less error-prone and more portable.
- In `acta-build::walk_src_max_width`, the `strip_prefix` handling currently uses `find(strip_prefix)` on a stringified path; if the prefix appears more than once (or with mixed separators), this may mis-strip paths—using `Path::strip_prefix` where possible and only falling back to string operations would make the computed width more accurate across platforms.
## Individual Comments
### Comment 1
<location path="src/fmt/test.rs" line_range="145-152" />
<code_context>
+}
+
+#[test]
+fn format_path_leading_ellipsis_for_very_long_path() {
+ let mut fmt = Formatter::new();
+ // Narrow enough that file_with_line alone exceeds width,
+ // forcing the leading-ellipsis fallback branch.
+ fmt.path_width = 11;
+ let result = fmt.format_path("/very/long/prefix/deep/nested/dirs/file.rs", 99);
+ assert!(result.starts_with('\u{2026}'), "expected leading ellipsis: {result}");
+ assert!(result.contains("file.rs:99"), "expected filename preserved: {result}");
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering multi-byte (Unicode) paths to exercise the char-boundary-safe truncation logic in `format_path`
Given `format_path` now enforces UTF-8 char boundaries, please add a test with a multi-byte path segment (e.g. `"src/模块/文件.rs"`) and a narrow `path_width` that forces truncation. The test should verify the output remains valid UTF-8 (no split characters around the ellipsis) and still preserves the `file.rs:line` suffix. This helps catch off-by-one errors in the boundary-adjustment loop.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈来改进后续的代码审查。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The new
Formatter::format_pathlogic relies on string splitting on"src/"/"src\\"and manual slicing, which can be brittle for non-standard layouts or multiplesrcsegments; consider switching toPath/Path::strip_prefix-based normalization so prefix-stripping and separator normalization are less error-prone and more portable. - In
acta-build::walk_src_max_width, thestrip_prefixhandling currently usesfind(strip_prefix)on a stringified path; if the prefix appears more than once (or with mixed separators), this may mis-strip paths—usingPath::strip_prefixwhere possible and only falling back to string operations would make the computed width more accurate across platforms.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Formatter::format_path` logic relies on string splitting on `"src/"` / `"src\\"` and manual slicing, which can be brittle for non-standard layouts or multiple `src` segments; consider switching to `Path` / `Path::strip_prefix`-based normalization so prefix-stripping and separator normalization are less error-prone and more portable.
- In `acta-build::walk_src_max_width`, the `strip_prefix` handling currently uses `find(strip_prefix)` on a stringified path; if the prefix appears more than once (or with mixed separators), this may mis-strip paths—using `Path::strip_prefix` where possible and only falling back to string operations would make the computed width more accurate across platforms.
## Individual Comments
### Comment 1
<location path="src/fmt/test.rs" line_range="145-152" />
<code_context>
+}
+
+#[test]
+fn format_path_leading_ellipsis_for_very_long_path() {
+ let mut fmt = Formatter::new();
+ // Narrow enough that file_with_line alone exceeds width,
+ // forcing the leading-ellipsis fallback branch.
+ fmt.path_width = 11;
+ let result = fmt.format_path("/very/long/prefix/deep/nested/dirs/file.rs", 99);
+ assert!(result.starts_with('\u{2026}'), "expected leading ellipsis: {result}");
+ assert!(result.contains("file.rs:99"), "expected filename preserved: {result}");
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test covering multi-byte (Unicode) paths to exercise the char-boundary-safe truncation logic in `format_path`
Given `format_path` now enforces UTF-8 char boundaries, please add a test with a multi-byte path segment (e.g. `"src/模块/文件.rs"`) and a narrow `path_width` that forces truncation. The test should verify the output remains valid UTF-8 (no split characters around the ellipsis) and still preserves the `file.rs:line` suffix. This helps catch off-by-one errors in the boundary-adjustment loop.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/builder.rs (1)
362-379:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win失败的过滤器更新会把
TracingGuard留在未提交状态。Line 367 先修改了
self.filter，Line 377-378 才去构造并 reloadEnvFilter。如果target生成了无效 directive，这个方法虽然返回Err，但self.filter已经变成新值了，之后的Debug、remove_target_level和后续更新都会基于一个并未真正生效的过滤器状态。先在临时Filter上验证成功后再回写会更稳妥。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/builder.rs` around lines 362 - 379, The current flow mutates self.filter (in set_target_level and remove_target_level) before attempting to construct and reload an EnvFilter in apply_current_filter, leaving self.filter changed even if EnvFilter::try_new fails; instead, perform the mutation on a temporary Filter copy, validate by calling tracing_subscriber::EnvFilter::try_new(temp.as_directive()) (or call apply_current_filter-like validation) and only when that succeeds assign the validated temp back to self.filter and then update the raw guard (i.e., change set_target_level and remove_target_level to operate on a cloned filter, validate the directive, then commit the clone to self.filter and call the existing reload logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.rs`:
- Around line 7-9: The write failure to the build artifact is currently only
logged as a warning; change the branch handling the std::fs::write(&path,
width.to_string()) Err(e) case to fail the build immediately (e.g., panic! or
call std::process::exit with non-zero) so the build stops on write errors;
update the error handling around std::fs::write (referencing path, width, and
the Err(e) match arm) to include the error details in the failure message so the
build aborts with a clear diagnostic.
In `@crates/acta-build/src/lib.rs`:
- Around line 60-61: The current code uses display.find(strip_prefix) which
matches the first occurrence anywhere instead of only at the start; replace this
with prefix semantics by using display.strip_prefix(strip_prefix) (or
starts_with + slicing) so you only strip when strip_prefix is at the beginning.
Concretely, change the .find(strip_prefix).map_or(display.len(), |i| display[i +
strip_prefix.len()..].len()) logic to use
display.strip_prefix(strip_prefix).map_or(display.len(), |s| s.len()) so the
computed length is based on a true prefix removal of strip_prefix.
In `@README.md`:
- Around line 112-115: Add a single blank line between the table ending with
"`Format::Json(LayerConfig)`" and the following "## File logging" heading so the
table and heading are separated (this prevents MD058/MD022 warnings); simply
insert one empty line after the table row for Format::Json.
In `@src/builder.rs`:
- Around line 141-143: build_layer currently lets WriterTarget::File be routed
to std::io::sink() via build_layer_with while remaining pub, causing callers who
use build_layer directly to silently drop file logs; change the API so callers
can't silently lose logs — either make build_layer return Result<BoxedLayer,
Error> and propagate an error when WriterTarget::File would use sink, or split
file-target construction into a separate non-public helper and keep build_layer
public only for safe targets; update build_layer, build_layer_with, and any code
that constructs WriterTarget::File to use the new error-returning or separate
constructor and ensure no path returns a BoxedLayer that wraps std::io::sink for
file targets.
- Around line 270-272: Current code creates a single shared ArcSwap
(shared_style) and passes it into build_formatter for multiple non-file writers,
so each writer's call to with_style_config(writer.style) overwrites the same
ArcSwap and later writers' styles win. Fix by giving each non-file writer its
own ArcSwap initialized from that writer's style instead of reusing
shared_style: inside the loop that constructs writers (the code that checks for
file vs non-file writers and calls build_formatter(..., shared_handle)), create
a new Arc::new(arc_swap::ArcSwap::new(Arc::new(writer.style))) per writer and
pass that per-writer ArcSwap into build_formatter; remove or stop using the
single shared_style for non-file writers so styles don't clobber each other.
- Around line 277-279: The current implementation in init stores only a single
file_guard and log_path (variables file_guard and log_path) and overwrites them
each time a WriterTarget::File is encountered, losing previous guards and paths;
either reject multiple WriterTarget::File entries in init (validate targets and
return an error if more than one file target is present) or change file_guard
and log_path to collections (e.g., Vec or HashMap) and push/store a guard/path
for each WriterTarget::File encountered and adjust any uses of log_path() and
the guard drop semantics accordingly so all file writers are retained; update
code paths that assume a single file (references around handling of file_guard,
log_path, and log_path()) to iterate over the collection.
In `@src/color/style.rs`:
- Around line 76-78: The code casts the anstyle::AnsiColor returned by
rgb_to_ansi to usize and indexes ANSI16_TABLE (ColorDepth::Ansi16 branch), which
relies on the enum's discriminant values and can break if upstream changes;
replace that fragile indexing with an explicit exhaustive mapping from each
anstyle::AnsiColor variant to the corresponding table entry (or to the desired
AnsiColors value) — locate where rgb_to_ansi(...) is used and instead of "as
usize" + ANSI16_TABLE.get(...).copied().unwrap_or(AnsiColors::White) implement a
match on the rgb_to_ansi(...) result (covering all AnsiColor variants) that
returns the correct entry and remove the silent White fallback so behavior is
explicit.
In `@src/test.rs`:
- Around line 137-189: Update the tests to assert actual filtering behavior via
the returned subscriber instead of only inspecting guard.filter; for example, in
set_filter_with_raw_directive_updates_guard (and/or
set_target_level_after_raw_directive_adds_per_target_override) use the
subscriber returned from build_test_guard to construct Metadata for different
targets/levels and call the subscriber's enabled check to assert that the base
level ("info") blocks lower levels and that the per-target override
("my_crate=debug" and later "demo=trace") allows the expected levels; ensure you
call build_test_guard(...) to get the subscriber and add assertions that the
subscriber.enabled(...) (or the equivalent API) returns true/false for the
specific target+level combinations that prove the filter is actually enforced at
runtime.
In `@src/writer/test.rs`:
- Around line 1-5: Remove the module-level allow for clippy::unnecessary_map_or
and rewrite the specific Option/Iterator map_or usages in this file to avoid
suppressing future warnings: search for occurrences of Option::map_or or
.map_or(...) in tests within src/writer/test.rs and replace them with more
explicit checks (e.g., use is_some_and for boolean predicates or match/if let to
handle values) so the linter rule can remain enabled; keep the other clippy
allows intact and only remove the clippy::unnecessary_map_or attribute.
---
Outside diff comments:
In `@src/builder.rs`:
- Around line 362-379: The current flow mutates self.filter (in set_target_level
and remove_target_level) before attempting to construct and reload an EnvFilter
in apply_current_filter, leaving self.filter changed even if EnvFilter::try_new
fails; instead, perform the mutation on a temporary Filter copy, validate by
calling tracing_subscriber::EnvFilter::try_new(temp.as_directive()) (or call
apply_current_filter-like validation) and only when that succeeds assign the
validated temp back to self.filter and then update the raw guard (i.e., change
set_target_level and remove_target_level to operate on a cloned filter, validate
the directive, then commit the clone to self.filter and call the existing reload
logic).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ea57346-f0d9-4895-93a1-203e971ae813
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.gitignoreCargo.tomlREADME.mdbuild.rscrates/acta-build/Cargo.tomlcrates/acta-build/src/lib.rscrates/acta-debug/src/main.rssrc/builder.rssrc/color/mapping.rssrc/color/mod.rssrc/color/style.rssrc/color/tests.rssrc/config/depth.rssrc/config/mod.rssrc/config/test.rssrc/fmt/mod.rssrc/fmt/test.rssrc/lib.rssrc/prelude.rssrc/test.rssrc/writer/custom.rssrc/writer/mod.rssrc/writer/test.rs
💤 Files with no reviewable changes (3)
- src/color/mapping.rs
- Cargo.toml
- src/config/depth.rs
📜 Review details
⏰ 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 / Build
🧰 Additional context used
🪛 markdownlint-cli2 (0.22.1)
README.md
[warning] 114-114: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 115-115: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
🔇 Additional comments (7)
src/writer/custom.rs (1)
24-27: LGTM!Also applies to: 60-60
src/writer/mod.rs (1)
13-13: LGTM!src/lib.rs (1)
14-18: LGTM!Also applies to: 26-26
src/prelude.rs (1)
1-1: LGTM!Also applies to: 4-7, 12-12
crates/acta-debug/src/main.rs (1)
5-7: 本文件改动主要是演示代码与新 API 对齐，当前未发现需要在此处新增的问题评论。Also applies to: 43-43, 270-276, 291-291, 320-322, 343-362
src/config/mod.rs (2)
570-575: 🏗️ Heavy liftRequest missing inputs to rewrite review comment: No
<review_comment>content (and no verification results) was provided, so I can’t rewrite it. Paste the original review comment and any relevant command outputs/logs you want used.
425-428: ⚡ Quick winMissing review comment context: Paste the original <review_comment>...</review_comment> (and any related diff/snippet it refers to) so I can rewrite it accurately.
Owner
Author
|
@sourcery-ai review |
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the acta logging library to simplify its architecture, improve performance, and clean up the public API. Key changes include replacing the stacked optional layers with a vector of boxed layers, introducing a Filter struct to handle both structured levels and raw EnvFilter directives, refactoring color mapping using a new Styled helper, and migrating internal string representations to CompactString to reduce heap allocations. Additionally, several dependencies like arrayvec, derive_more, and smart-default have been removed. Feedback on these changes highlights a correctness regression in the path truncation logic of Formatter::format_path due to the switch from find to rfind, as well as a performance regression from using heap-allocated String instead of CompactString on the hot path. A refactored version of format_path is suggested to resolve these issues.
There was a problem hiding this comment.
Hey - 我发现了 3 个问题，并给出了一些高层次的反馈：
- 围绕
LevelLabels和Formatter的公共 API 变更（例如LevelLabels::DEFAULT、Formatter::with_path_width）在 README 示例中没有得到一致体现，按当前写法示例代码将无法编译；要么恢复原来的构建器风格方法/别名，要么更新示例以使用当前的常量和直接的path_width字段。 WriterTarget::File的结构已经更改为包裹FileConfig，但至少有一个 README 示例仍然使用旧的结构形式（WriterTarget::File { path, rotation }）；请更新这些代码片段，以匹配新的WriterTarget::File(FileConfig::new(...).with_rotation(...))API。- 现在
TracingGuard可以管理多个文件写入器，log_path只返回第一个路径可能会让用户感到意外；可以考虑要么暴露一个返回所有日志路径的方法，要么在文档中说明log_path只代表主/第一个文件。
供 AI Agent 使用的提示
Please address the comments from this code review:
## Overall Comments
- The public API changes around `LevelLabels` and `Formatter` (e.g. `LevelLabels::DEFAULT`, `Formatter::with_path_width`) are not reflected consistently in the README examples and will not compile as written; either restore the builder-style methods/aliases or update the examples to use the current constants and direct `path_width` field.
- The `WriterTarget::File` shape has changed to wrap `FileConfig`, but at least one README example still uses the old struct form (`WriterTarget::File { path, rotation }`); update those snippets to match the new `WriterTarget::File(FileConfig::new(...).with_rotation(...))` API.
- Now that `TracingGuard` can manage multiple file writers, `log_path` returning only the first path may be surprising to users; consider either exposing a method that returns all log paths or documenting that `log_path` is the primary/first file only.
## Individual Comments
### Comment 1
<location path="src/fmt/mod.rs" line_range="50-59" />
<code_context>
+///
+/// Returns [`FALLBACK_WIDTH`] (40) if `dir` does not exist or contains no
+/// `.rs` files — safe to call unconditionally from a `build.rs`.
+#[must_use]
+pub fn walk_src_max_width(dir: impl AsRef<Path>, strip_prefix: &str) -> usize {
+ let entries: Vec<_> = WalkDir::new(dir.as_ref())
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing `with_path_width` leaves no way to use the build-time `path_width` and diverges from `acta-build` docs.
`Formatter::with_path_width` was removed while `Formatter` still exposes `path_width`, and `acta-build`’s README still shows `Formatter::new().with_path_width(width)`. This breaks the documented usage and prevents callers from applying the computed width.
Either reintroduce a public way to set `path_width` (e.g., `with_path_width` or equivalent) so `acta-build` remains usable, or update the docs to reflect the new intended usage without that method.
</issue_to_address>
### Comment 2
<location path="src/test.rs" line_range="137-146" />
<code_context>
}
+
+#[test]
+fn set_filter_with_raw_directive_updates_guard() {
+ let (mut guard, subscriber) = build_test_guard(Level::Info, Style::default());
+ let filter = Filter::from_directive("info,my_crate=debug");
+ guard
+ .set_filter(filter)
+ .expect("set_filter with raw directive should succeed");
+ assert_eq!(
+ guard.filter.as_directive(),
+ "info,my_crate=debug",
+ "guard.filter should reflect the raw directive applied via set_filter"
+ );
+
+ // Verify actual tracing behavior reflects the filter
+ tracing::subscriber::with_default(subscriber, || {
+ assert!(
+ tracing::enabled!(tracing::Level::INFO),
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative test case for `set_filter` to cover invalid directives.
Since `TracingGuard::set_filter` now propagates `EnvFilter::try_new` errors, add a test that passes an invalid directive (e.g. `"=invalid"` or `"foo::bar::"`) and asserts that `set_filter` returns `Err`, to ensure invalid configurations aren’t silently accepted or misreported.
</issue_to_address>
### Comment 3
<location path="src/color/style.rs" line_range="35" />
<code_context>
- ColorDepth::Ansi16 => {
- let c = rgb_to_ansi16(r, g, b);
- OwoStyle::new().color(DynColors::Ansi(c))
+impl Styled {
+ pub(crate) const fn new(rgb: Rgb, depth: ColorDepth) -> Self {
+ Self {
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the public `Styled` builder type with a small set of helper functions that directly return `OwoStyle`, keeping the ANSI16/palette logic internal while simplifying call sites.
You’ve increased call‑site and type complexity without a clear benefit. You can keep all the new behavior (palette + `rgb_to_ansi` + `ANSI16_TABLE`) while hiding the extra knobs behind a small set of helper functions, instead of pushing a builder type into all callers.
### Suggested refactor
Keep the core mapping logic, but move it into an internal helper that returns `OwoStyle` directly, and expose simple, focused functions. You likely don’t need `Styled` at all.
```rust
const ANSI16_TABLE: [AnsiColors; 16] = [ /* unchanged */ ];
fn style_for(rgb: Rgb, depth: ColorDepth, on: bool, dimmed: bool) -> OwoStyle {
let mut rgb = rgb;
if dimmed {
rgb = Rgb(rgb.0 >> 2, rgb.1 >> 2, rgb.2 >> 2);
}
let style = OwoStyle::new();
match depth {
ColorDepth::TrueColor => {
if on {
style.on_truecolor(rgb.0, rgb.1, rgb.2)
} else {
style.truecolor(rgb.0, rgb.1, rgb.2)
}
}
ColorDepth::Ansi256 => {
let ansi =
XtermColors::from(ansi_colours::ansi256_from_rgb((rgb.0, rgb.1, rgb.2)));
if on {
style.on_color(ansi)
} else {
style.color(ansi)
}
}
ColorDepth::Ansi16 => {
let ansi = ANSI16_TABLE
.get(rgb_to_ansi((rgb.0, rgb.1, rgb.2).into(), Palette::default()) as usize)
.copied()
.unwrap_or(AnsiColors::White);
if on {
style.on_color(ansi)
} else {
style.color(ansi)
}
}
ColorDepth::NoColor => style,
}
}
```
Then define small helpers that callers actually use:
```rust
pub fn fg_style(rgb: Rgb, depth: ColorDepth) -> OwoStyle {
style_for(rgb, depth, false, false)
}
pub fn fg_dimmed_style(rgb: Rgb, depth: ColorDepth) -> OwoStyle {
style_for(rgb, depth, false, true)
}
pub fn bg_style(rgb: Rgb, depth: ColorDepth) -> OwoStyle {
style_for(rgb, depth, true, false)
}
// add variants as needed, e.g. bg_dimmed_style(...)
```
Call sites then become:
```rust
let style = fg_style(Rgb(r, g, b), depth);
// or
let style = fg_dimmed_style(Rgb(r, g, b), depth);
// or
let style = bg_style(Rgb(r, g, b), depth);
```
This:
- Preserves all new behavior (16‑color mapping, palette logic, dimming, `on` handling).
- Removes the need for the `Styled` type and its `From` impl.
- Shortens and simplifies usage at every call site, which was the main complexity complaint.
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据这些反馈改进后续评审。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- The public API changes around
LevelLabelsandFormatter(e.g.LevelLabels::DEFAULT,Formatter::with_path_width) are not reflected consistently in the README examples and will not compile as written; either restore the builder-style methods/aliases or update the examples to use the current constants and directpath_widthfield. - The
WriterTarget::Fileshape has changed to wrapFileConfig, but at least one README example still uses the old struct form (WriterTarget::File { path, rotation }); update those snippets to match the newWriterTarget::File(FileConfig::new(...).with_rotation(...))API. - Now that
TracingGuardcan manage multiple file writers,log_pathreturning only the first path may be surprising to users; consider either exposing a method that returns all log paths or documenting thatlog_pathis the primary/first file only.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The public API changes around `LevelLabels` and `Formatter` (e.g. `LevelLabels::DEFAULT`, `Formatter::with_path_width`) are not reflected consistently in the README examples and will not compile as written; either restore the builder-style methods/aliases or update the examples to use the current constants and direct `path_width` field.
- The `WriterTarget::File` shape has changed to wrap `FileConfig`, but at least one README example still uses the old struct form (`WriterTarget::File { path, rotation }`); update those snippets to match the new `WriterTarget::File(FileConfig::new(...).with_rotation(...))` API.
- Now that `TracingGuard` can manage multiple file writers, `log_path` returning only the first path may be surprising to users; consider either exposing a method that returns all log paths or documenting that `log_path` is the primary/first file only.
## Individual Comments
### Comment 1
<location path="src/fmt/mod.rs" line_range="50-59" />
<code_context>
+///
+/// Returns [`FALLBACK_WIDTH`] (40) if `dir` does not exist or contains no
+/// `.rs` files — safe to call unconditionally from a `build.rs`.
+#[must_use]
+pub fn walk_src_max_width(dir: impl AsRef<Path>, strip_prefix: &str) -> usize {
+ let entries: Vec<_> = WalkDir::new(dir.as_ref())
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing `with_path_width` leaves no way to use the build-time `path_width` and diverges from `acta-build` docs.
`Formatter::with_path_width` was removed while `Formatter` still exposes `path_width`, and `acta-build`’s README still shows `Formatter::new().with_path_width(width)`. This breaks the documented usage and prevents callers from applying the computed width.
Either reintroduce a public way to set `path_width` (e.g., `with_path_width` or equivalent) so `acta-build` remains usable, or update the docs to reflect the new intended usage without that method.
</issue_to_address>
### Comment 2
<location path="src/test.rs" line_range="137-146" />
<code_context>
}
+
+#[test]
+fn set_filter_with_raw_directive_updates_guard() {
+ let (mut guard, subscriber) = build_test_guard(Level::Info, Style::default());
+ let filter = Filter::from_directive("info,my_crate=debug");
+ guard
+ .set_filter(filter)
+ .expect("set_filter with raw directive should succeed");
+ assert_eq!(
+ guard.filter.as_directive(),
+ "info,my_crate=debug",
+ "guard.filter should reflect the raw directive applied via set_filter"
+ );
+
+ // Verify actual tracing behavior reflects the filter
+ tracing::subscriber::with_default(subscriber, || {
+ assert!(
+ tracing::enabled!(tracing::Level::INFO),
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative test case for `set_filter` to cover invalid directives.
Since `TracingGuard::set_filter` now propagates `EnvFilter::try_new` errors, add a test that passes an invalid directive (e.g. `"=invalid"` or `"foo::bar::"`) and asserts that `set_filter` returns `Err`, to ensure invalid configurations aren’t silently accepted or misreported.
</issue_to_address>
### Comment 3
<location path="src/color/style.rs" line_range="35" />
<code_context>
- ColorDepth::Ansi16 => {
- let c = rgb_to_ansi16(r, g, b);
- OwoStyle::new().color(DynColors::Ansi(c))
+impl Styled {
+ pub(crate) const fn new(rgb: Rgb, depth: ColorDepth) -> Self {
+ Self {
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the public `Styled` builder type with a small set of helper functions that directly return `OwoStyle`, keeping the ANSI16/palette logic internal while simplifying call sites.
You’ve increased call‑site and type complexity without a clear benefit. You can keep all the new behavior (palette + `rgb_to_ansi` + `ANSI16_TABLE`) while hiding the extra knobs behind a small set of helper functions, instead of pushing a builder type into all callers.
### Suggested refactor
Keep the core mapping logic, but move it into an internal helper that returns `OwoStyle` directly, and expose simple, focused functions. You likely don’t need `Styled` at all.
```rust
const ANSI16_TABLE: [AnsiColors; 16] = [ /* unchanged */ ];
fn style_for(rgb: Rgb, depth: ColorDepth, on: bool, dimmed: bool) -> OwoStyle {
let mut rgb = rgb;
if dimmed {
rgb = Rgb(rgb.0 >> 2, rgb.1 >> 2, rgb.2 >> 2);
}
let style = OwoStyle::new();
match depth {
ColorDepth::TrueColor => {
if on {
style.on_truecolor(rgb.0, rgb.1, rgb.2)
} else {
style.truecolor(rgb.0, rgb.1, rgb.2)
}
}
ColorDepth::Ansi256 => {
let ansi =
XtermColors::from(ansi_colours::ansi256_from_rgb((rgb.0, rgb.1, rgb.2)));
if on {
style.on_color(ansi)
} else {
style.color(ansi)
}
}
ColorDepth::Ansi16 => {
let ansi = ANSI16_TABLE
.get(rgb_to_ansi((rgb.0, rgb.1, rgb.2).into(), Palette::default()) as usize)
.copied()
.unwrap_or(AnsiColors::White);
if on {
style.on_color(ansi)
} else {
style.color(ansi)
}
}
ColorDepth::NoColor => style,
}
}
```
Then define small helpers that callers actually use:
```rust
pub fn fg_style(rgb: Rgb, depth: ColorDepth) -> OwoStyle {
style_for(rgb, depth, false, false)
}
pub fn fg_dimmed_style(rgb: Rgb, depth: ColorDepth) -> OwoStyle {
style_for(rgb, depth, false, true)
}
pub fn bg_style(rgb: Rgb, depth: ColorDepth) -> OwoStyle {
style_for(rgb, depth, true, false)
}
// add variants as needed, e.g. bg_dimmed_style(...)
```
Call sites then become:
```rust
let style = fg_style(Rgb(r, g, b), depth);
// or
let style = fg_dimmed_style(Rgb(r, g, b), depth);
// or
let style = bg_style(Rgb(r, g, b), depth);
```
This:
- Preserves all new behavior (16‑color mapping, palette logic, dimming, `on` handling).
- Removes the need for the `Styled` type and its `From` impl.
- Shortens and simplifies usage at every call site, which was the main complexity complaint.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/writer/custom.rs (1)
49-63:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift避免
async_writer_for在capacity==0或无 Tokio runtime 时直接 panic
async_writer_for内部调用tokio::sync::mpsc::channel(capacity)：capacity==0会 panic；而AsyncMode::Custom { buffer_size: usize }在配置层未见>0校验，可能在init/构建阶段直接崩溃。tokio::spawn(...)也要求当前线程处于 Tokio runtime 上下文；当前没有保护（README 虽提示需要 Tokio runtime），仍建议在这里用tokio::runtime::Handle::try_current()获取句柄并在失败时向上传递错误。- 建议把
async_writer_for改成返回Result<AsyncWriter>：先校验capacity > 0，再检查 runtime，不满足则返回错误而非 panic。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/writer/custom.rs` around lines 49 - 63, The async_writer_for function must not panic on capacity==0 or when there's no Tokio runtime: change async_writer_for to return Result<AsyncWriter, Error>, validate that capacity > 0 and return a clear error if not, and check for an active Tokio runtime using tokio::runtime::Handle::try_current() (return an error when none is available) instead of calling tokio::spawn unconditionally; if a handle exists, use handle.spawn(...) to start the background task. Keep references to AsyncWriterTarget and the async_writer_for function name so callers can be updated to propagate the Result and handle errors accordingly.README.md (2)
123-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win文件日志示例仍在使用已删除的
WriterTarget::File { ... }变体。当前 API 已改成
WriterTarget::File(FileConfig)，这里的示例现在会直接编译失败。♻️ 建议修改
use acta::{ - init, Format, LayerConfig, Level, Config, Result, Rotation, Writer, WriterTarget, + init, Config, FileConfig, Format, LayerConfig, Level, Result, Rotation, Writer, WriterTarget, }; -use std::path::PathBuf; fn main() -> Result<()> { let config = Config { filter: Level::Info.into(), writers: vec![Writer { format: Format::Compact(LayerConfig::compact()), - target: WriterTarget::File { - path: PathBuf::from("logs/app.log"), - rotation: Rotation::Rename, - }, + target: WriterTarget::File( + FileConfig::new("logs/app.log").with_rotation(Rotation::Rename), + ), ..Default::default() }], ..Default::default() };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 123 - 135, The README example uses the removed enum variant WriterTarget::File { ... }; update it to the new shape WriterTarget::File(FileConfig) by constructing a FileConfig instance and passing it to WriterTarget::File (e.g., create a FileConfig with the same path and rotation values); ensure the example imports/uses the FileConfig type alongside Writer, WriterTarget, Rotation, and PathBuf and replace the old inline struct literal with the FileConfig construction so the example compiles.
204-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win文档示例仍引用已移除的
LevelLabels::DEFAULT常量。
LevelLabels::DEFAULT已不存在，当前示例会编译失败。这里应改成现有常量，比如LevelLabels::LONG。♻️ 建议修改
let formatter = Formatter::new() .with_theme(Theme::tokyo_night()) .with_icons(Icons::UNICODE) - .with_labels(LevelLabels::DEFAULT) + .with_labels(LevelLabels::LONG) .with_time_format("%H:%M:%S") .with_show_path(true) .with_show_spans(true);let unicode_icons = Icons::UNICODE; let short_labels = LevelLabels::SHORT; -let long_labels = LevelLabels::DEFAULT; +let long_labels = LevelLabels::LONG;Also applies to: 271-276
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 204 - 210, Update the README examples that reference the removed constant LevelLabels::DEFAULT to use an existing constant (e.g., LevelLabels::LONG); specifically, in the Formatter setup example (the chain using Formatter::new(), .with_theme(...), .with_icons(...), .with_labels(...), .with_time_format(...)) replace LevelLabels::DEFAULT with LevelLabels::LONG and make the same replacement for the other occurrence later in the file so the example compiles.
♻️ Duplicate comments (2)
src/color/style.rs (1)
74-80: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win不要把
anstyle::AnsiColor的判别值当成稳定索引。这里用
as usize去索引本地表，把颜色映射正确性绑定到了上游枚举布局；一旦上游调整判别值，结果会静默退回到White。改成对rgb_to_ansi(...)结果做穷举匹配会更稳。For anstyle-lossy 1.1.0 / anstyle, is the discriminant ordering or numeric value of `anstyle::AnsiColor` documented as stable and intended to be used for array indexing via `as usize`?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/color/style.rs` around lines 74 - 80, The code uses rgb_to_ansi(... ) as usize to index ANSI16_TABLE, which relies on the numeric discriminants of anstyle::AnsiColor; instead change the code to match on the rgb_to_ansi(...) return value and map each AnsiColor variant to the corresponding ANSI16_TABLE entry (or directly to the desired AnsiColors value), e.g. call rgb_to_ansi((s.rgb.0, s.rgb.1, s.rgb.2).into(), Palette::default()) and then match its variants to select the proper value rather than casting to usize; update the block referencing ANSI16_TABLE, rgb_to_ansi, Palette::default(), and AnsiColors::White accordingly so the mapping is explicit and not dependent on enum discriminants.crates/acta-build/src/lib.rs (1)
58-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win统一规范化
strip_prefix，否则 Windows 风格前缀不会被剥离。这里把
display归一化成了/，但strip_prefix仍按原样比较；调用方若传入src\，会退回整条路径长度，导致宽度计算偏大。♻️ 建议修改
.iter() .map(|e| { let display = e.path().to_string_lossy().replace('\\', "/"); - display.strip_prefix(strip_prefix).unwrap_or(&display).len() + let normalized_prefix = strip_prefix.replace('\\', "/"); + display + .strip_prefix(&normalized_prefix) + .unwrap_or(&display) + .len() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/acta-build/src/lib.rs` around lines 58 - 59, The code normalizes the file path into `display` (replacing '\\' with '/'), but still calls `display.strip_prefix(strip_prefix)` using the original `strip_prefix`, so Windows-style prefixes like `src\` won't match; normalize `strip_prefix` the same way before calling `strip_prefix` (e.g., create a `normalized_prefix` by replacing '\\' with '/' on `strip_prefix` and use that in `display.strip_prefix(&normalized_prefix)`), ensuring the `display` and `strip_prefix` use the same path form.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fmt/mod.rs`:
- Around line 104-107: with_time_format 不应收窄为 String——请把签名改回接受更宽的类型（例如 pub fn
with_time_format<T: Into<String>>(mut self, fmt: T) -> Self 或使用 impl
Into<String>），并在函数体中用 self.time_format = fmt.into(); 保持函数名 with_time_format 和字段
time_format 不变以兼容现有调用者。
---
Outside diff comments:
In `@README.md`:
- Around line 123-135: The README example uses the removed enum variant
WriterTarget::File { ... }; update it to the new shape
WriterTarget::File(FileConfig) by constructing a FileConfig instance and passing
it to WriterTarget::File (e.g., create a FileConfig with the same path and
rotation values); ensure the example imports/uses the FileConfig type alongside
Writer, WriterTarget, Rotation, and PathBuf and replace the old inline struct
literal with the FileConfig construction so the example compiles.
- Around line 204-210: Update the README examples that reference the removed
constant LevelLabels::DEFAULT to use an existing constant (e.g.,
LevelLabels::LONG); specifically, in the Formatter setup example (the chain
using Formatter::new(), .with_theme(...), .with_icons(...), .with_labels(...),
.with_time_format(...)) replace LevelLabels::DEFAULT with LevelLabels::LONG and
make the same replacement for the other occurrence later in the file so the
example compiles.
In `@src/writer/custom.rs`:
- Around line 49-63: The async_writer_for function must not panic on capacity==0
or when there's no Tokio runtime: change async_writer_for to return
Result<AsyncWriter, Error>, validate that capacity > 0 and return a clear error
if not, and check for an active Tokio runtime using
tokio::runtime::Handle::try_current() (return an error when none is available)
instead of calling tokio::spawn unconditionally; if a handle exists, use
handle.spawn(...) to start the background task. Keep references to
AsyncWriterTarget and the async_writer_for function name so callers can be
updated to propagate the Result and handle errors accordingly.
---
Duplicate comments:
In `@crates/acta-build/src/lib.rs`:
- Around line 58-59: The code normalizes the file path into `display` (replacing
'\\' with '/'), but still calls `display.strip_prefix(strip_prefix)` using the
original `strip_prefix`, so Windows-style prefixes like `src\` won't match;
normalize `strip_prefix` the same way before calling `strip_prefix` (e.g.,
create a `normalized_prefix` by replacing '\\' with '/' on `strip_prefix` and
use that in `display.strip_prefix(&normalized_prefix)`), ensuring the `display`
and `strip_prefix` use the same path form.
In `@src/color/style.rs`:
- Around line 74-80: The code uses rgb_to_ansi(... ) as usize to index
ANSI16_TABLE, which relies on the numeric discriminants of anstyle::AnsiColor;
instead change the code to match on the rgb_to_ansi(...) return value and map
each AnsiColor variant to the corresponding ANSI16_TABLE entry (or directly to
the desired AnsiColors value), e.g. call rgb_to_ansi((s.rgb.0, s.rgb.1,
s.rgb.2).into(), Palette::default()) and then match its variants to select the
proper value rather than casting to usize; update the block referencing
ANSI16_TABLE, rgb_to_ansi, Palette::default(), and AnsiColors::White accordingly
so the mapping is explicit and not dependent on enum discriminants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c96bba23-215d-4892-8dd4-82fd140719ef
📒 Files selected for processing (14)
README.mdcrates/acta-build/src/lib.rscrates/acta-debug/src/main.rssrc/builder.rssrc/color/style.rssrc/config/mod.rssrc/config/test.rssrc/fmt/mod.rssrc/fmt/test.rssrc/fmt/visitor.rssrc/lib.rssrc/test.rssrc/writer/custom.rssrc/writer/test.rs
📜 Review details
⏰ 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 / Build
🔇 Additional comments (1)
src/writer/test.rs (1)
1-1: LGTM!Also applies to: 124-124
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 Sourcery
重构 acta 的日志核心，以简化 subscriber 组合方式，统一 formatter 和样式配置，并改进过滤器、主题、异步 writer 和文件日志的易用性。
增强内容：
Filter的 API，该 API 支持原始的EnvFilter指令以及按 target 的覆盖配置，并相应更新Config/ConfigBuilder。anstyle-lossy的全新颜色样式后端，优化 Formatter 的默认值、路径格式化和着色。init/build_layer/TracingGuard接口，移除底层 subscriber 连接辅助工具。DEFAULT_ASYNC_BUFFER_SIZE，同时简化自定义异步 writer 的实现。acta-build辅助 crate，并将其集成到build.rs中，用于计算并持久化默认路径宽度，并改进失败处理。文档：
Filter的配置、LayerConfig的用法、异步缓冲区大小配置，以及用于路径宽度调优的acta-build辅助工具。测试：
TracingGuard与过滤器的交互。Original summary in English
Summary by Sourcery
Refactor acta’s logging core to simplify subscriber composition, unify formatter and style configuration, and improve filter, theme, async writer, and file logging ergonomics.
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
重构 acta 的日志核心，以简化 subscriber 组合方式，统一 formatter 和样式配置，并改进过滤器、主题、异步 writer 和文件日志的易用性。
增强内容：
Filter的 API，该 API 支持原始的EnvFilter指令以及按 target 的覆盖配置，并相应更新Config/ConfigBuilder。anstyle-lossy的全新颜色样式后端，优化 Formatter 的默认值、路径格式化和着色。init/build_layer/TracingGuard接口，移除底层 subscriber 连接辅助工具。DEFAULT_ASYNC_BUFFER_SIZE，同时简化自定义异步 writer 的实现。acta-build辅助 crate，并将其集成到build.rs中，用于计算并持久化默认路径宽度，并改进失败处理。文档：
Filter的配置、LayerConfig的用法、异步缓冲区大小配置，以及用于路径宽度调优的acta-build辅助工具。测试：
TracingGuard与过滤器的交互。Original summary in English
Summary by Sourcery
Refactor acta’s logging core to simplify subscriber composition, unify formatter and style configuration, and improve filter, theme, async writer, and file logging ergonomics.
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
重构 acta 的日志核心，以简化 subscriber 组合方式，统一 formatter 和样式配置，并改进过滤器、主题、异步 writer 和文件日志的易用性。
增强内容：
Filter的 API，该 API 支持原始的EnvFilter指令以及按 target 的覆盖配置，并相应更新Config/ConfigBuilder。anstyle-lossy的全新颜色样式后端，优化 Formatter 的默认值、路径格式化和着色。init/build_layer/TracingGuard接口，移除底层 subscriber 连接辅助工具。DEFAULT_ASYNC_BUFFER_SIZE，同时简化自定义异步 writer 的实现。acta-build辅助 crate，并将其集成到build.rs中，用于计算并持久化默认路径宽度，并改进失败处理。文档：
Filter的配置、LayerConfig的用法、异步缓冲区大小配置，以及用于路径宽度调优的acta-build辅助工具。测试：
TracingGuard与过滤器的交互。Original summary in English
Summary by Sourcery
Refactor acta’s logging core to simplify subscriber composition, unify formatter and style configuration, and improve filter, theme, async writer, and file logging ergonomics.
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
重构 acta 的日志核心，以简化 subscriber 组合方式，统一 formatter 和样式配置，并改进过滤器、主题、异步 writer 和文件日志的易用性。
增强内容：
Filter的 API，该 API 支持原始的EnvFilter指令以及按 target 的覆盖配置，并相应更新Config/ConfigBuilder。anstyle-lossy的全新颜色样式后端，优化 Formatter 的默认值、路径格式化和着色。init/build_layer/TracingGuard接口，移除底层 subscriber 连接辅助工具。DEFAULT_ASYNC_BUFFER_SIZE，同时简化自定义异步 writer 的实现。acta-build辅助 crate，并将其集成到build.rs中，用于计算并持久化默认路径宽度，并改进失败处理。文档：
Filter的配置、LayerConfig的用法、异步缓冲区大小配置，以及用于路径宽度调优的acta-build辅助工具。测试：
TracingGuard与过滤器的交互。Original summary in English
Summary by Sourcery
Refactor acta’s logging core to simplify subscriber composition, unify formatter and style configuration, and improve filter, theme, async writer, and file logging ergonomics.
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
重构 acta 的日志核心，以简化 subscriber 组合方式，统一 formatter 和样式配置，并改进过滤器、主题、异步 writer 和文件日志的易用性。
增强内容：
Filter的 API，该 API 支持原始的EnvFilter指令以及按 target 的覆盖配置，并相应更新Config/ConfigBuilder。anstyle-lossy的全新颜色样式后端，优化 Formatter 的默认值、路径格式化和着色。init/build_layer/TracingGuard接口，移除底层 subscriber 连接辅助工具。DEFAULT_ASYNC_BUFFER_SIZE，同时简化自定义异步 writer 的实现。acta-build辅助 crate，并将其集成到build.rs中，用于计算并持久化默认路径宽度，并改进失败处理。文档：
Filter的配置、LayerConfig的用法、异步缓冲区大小配置，以及用于路径宽度调优的acta-build辅助工具。测试：
TracingGuard与过滤器的交互。Original summary in English
Summary by Sourcery
Refactor acta’s logging core to simplify subscriber composition, unify formatter and style configuration, and improve filter, theme, async writer, and file logging ergonomics.
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
重构 acta 的日志核心，以简化 subscriber 组合方式，统一 formatter 和样式配置，并改进过滤器、主题、异步 writer 和文件日志的易用性。
增强内容：
Filter的 API，该 API 支持原始的EnvFilter指令以及按 target 的覆盖配置，并相应更新Config/ConfigBuilder。anstyle-lossy的全新颜色样式后端，优化 Formatter 的默认值、路径格式化和着色。init/build_layer/TracingGuard接口，移除底层 subscriber 连接辅助工具。DEFAULT_ASYNC_BUFFER_SIZE，同时简化自定义异步 writer 的实现。acta-build辅助 crate，并将其集成到build.rs中，用于计算并持久化默认路径宽度，并改进失败处理。文档：
Filter的配置、LayerConfig的用法、异步缓冲区大小配置，以及用于路径宽度调优的acta-build辅助工具。测试：
TracingGuard与过滤器的交互。Original summary in English
Summary by Sourcery
Refactor acta’s logging core to simplify subscriber composition, unify formatter and style configuration, and improve filter, theme, async writer, and file logging ergonomics.
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
重构 acta 的日志核心，以简化 subscriber 组合方式，统一 formatter 和样式配置，并改进过滤器、主题、异步 writer 和文件日志的易用性。
增强内容：
Filter的 API，该 API 支持原始的EnvFilter指令以及按 target 的覆盖配置，并相应更新Config/ConfigBuilder。anstyle-lossy的全新颜色样式后端，优化 Formatter 的默认值、路径格式化和着色。init/build_layer/TracingGuard接口，移除底层 subscriber 连接辅助工具。DEFAULT_ASYNC_BUFFER_SIZE，同时简化自定义异步 writer 的实现。acta-build辅助 crate，并将其集成到build.rs中，用于计算并持久化默认路径宽度，并改进失败处理。文档：
Filter的配置、LayerConfig的用法、异步缓冲区大小配置，以及用于路径宽度调优的acta-build辅助工具。测试：
TracingGuard与过滤器的交互。Original summary in English
Summary by Sourcery
Refactor acta’s logging core to simplify subscriber composition, unify formatter and style configuration, and improve filter, theme, async writer, and file logging ergonomics.
Enhancements:
Documentation:
Tests:
Original summary in English
Summary by Sourcery
重构 acta 的日志核心，以简化 subscriber 组合方式，统一 formatter 和样式配置，并改进过滤器、主题、异步 writer 和文件日志的易用性。
增强内容：
Filter的 API，该 API 支持原始的EnvFilter指令以及按 target 的覆盖配置，并相应更新Config/ConfigBuilder。anstyle-lossy的全新颜色样式后端，优化 Formatter 的默认值、路径格式化和着色。init/build_layer/TracingGuard接口，移除底层 subscriber 连接辅助工具。DEFAULT_ASYNC_BUFFER_SIZE，同时简化自定义异步 writer 的实现。acta-build辅助 crate，并将其集成到build.rs中，用于计算并持久化默认路径宽度，并改进失败处理。文档：
Filter的配置、LayerConfig的用法、异步缓冲区大小配置，以及用于路径宽度调优的acta-build辅助工具。测试：
TracingGuard与过滤器的交互。Original summary in English
Summary by Sourcery
Refactor acta’s logging core to simplify subscriber composition, unify formatter and style configuration, and improve filter, theme, async writer, and file logging ergonomics.
Enhancements:
Documentation:
Tests: