Token-driven Design Tokens and End-to-End Localized Output Tests#277
Merged
Token-driven Design Tokens and End-to-End Localized Output Tests#277
Conversation
- Added markdownlint disables around checklist formatting to avoid list item number duplication warnings in execplan. - Corrected sequential numbering in execplan steps from '1.' to '2.' for steps after baseline. - Expanded roadmap tasks for 3.12.1 to include routing reporter prefixes and spacing through theme tokens, and added end-to-end theme coverage and documentation for ASCII/Unicode consistency. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideImplements end-to-end, token-driven theming for CLI output by making OutputPrefs a facade over ResolvedTheme tokens, routing prefixes and spacing through the theme system, updating Fluent bundles and docs for the new theme/emoji behavior, and expanding tests and BDD features to validate theme resolution, ASCII/Unicode consistency, and CLI merge precedence. Sequence diagram for CLI theme resolution and token-driven prefix renderingsequenceDiagram
actor User
participant Cli as CliMain
participant Ortho as OrthoConfig
participant Theme as ThemeResolver
participant Prefs as OutputPrefs
participant Reporter
participant L10n as LocalizationModule
participant Keys as KeysModule
User->>Cli: Invoke netsuke with --theme and --no-emoji flags
Cli->>Ortho: Build OrthoConfig from CLI, env, config file
Ortho-->>Cli: ThemePreference, no_emoji, OutputMode
Cli->>Theme: resolve_theme(theme, no_emoji, mode, read_env)
Theme-->>Cli: ResolvedTheme
Cli->>Prefs: from_theme(resolved_theme)
Cli-->>Reporter: Construct reporter with OutputPrefs
Note over Reporter,Prefs: Later, reporter needs an error prefix
Reporter->>Prefs: error_prefix()
Prefs->>Prefs: render_prefix(tokens.symbols.error, SEMANTIC_PREFIX_ERROR)
Prefs->>L10n: message(Keys.SEMANTIC_PREFIX_ERROR)
L10n-->>Prefs: LocalizedMessage(label)
Prefs->>L10n: message(Keys.SEMANTIC_PREFIX_RENDERED)
L10n-->>Prefs: LocalizedMessage(template)
Prefs->>L10n: with_arg("symbol", tokens.symbols.error)
Prefs->>L10n: with_arg("label", label)
L10n-->>Reporter: LocalizedMessage(rendered_prefix)
Reporter->>User: Render themed error line with Unicode or ASCII prefix
Class diagram for token-driven OutputPrefs and theme tokensclassDiagram
class OutputPrefs {
- ResolvedTheme resolved_theme
+ from_theme(resolved_theme: ResolvedTheme) OutputPrefs
+ emoji_allowed() bool
+ task_indent() str
+ timing_indent() str
+ error_prefix() LocalizedMessage
+ warning_prefix() LocalizedMessage
+ success_prefix() LocalizedMessage
+ info_prefix() LocalizedMessage
+ timing_prefix() LocalizedMessage
- render_prefix(symbol: str, label_key: str) LocalizedMessage
}
class ResolvedTheme {
+ DesignTokens tokens
}
class DesignTokens {
+ ColourTokens colours
+ SymbolTokens symbols
+ SpacingTokens spacing
+ bool emoji_allowed
}
class SymbolTokens {
+ str error
+ str warning
+ str success
+ str info
+ str timing
}
class SpacingTokens {
+ str task_indent
+ str timing_indent
}
class ThemePreference {
}
class OutputMode {
}
class ThemeResolver {
+ resolve_theme(theme: ThemePreference, no_emoji: bool, mode: OutputMode, read_env: FnStrToOptString) ResolvedTheme
}
class FnStrToOptString {
}
class LocalizedMessage {
+ to_string() str
+ with_arg(name: str, value: str) LocalizedMessage
}
class LocalizationModule {
+ message(key: str) LocalizedMessage
}
class KeysModule {
+ SEMANTIC_PREFIX_RENDERED str
+ SEMANTIC_PREFIX_ERROR str
+ SEMANTIC_PREFIX_WARNING str
+ SEMANTIC_PREFIX_SUCCESS str
+ SEMANTIC_PREFIX_INFO str
+ SEMANTIC_PREFIX_TIMING str
}
OutputPrefs *-- ResolvedTheme
ResolvedTheme *-- DesignTokens
DesignTokens *-- SymbolTokens
DesignTokens *-- SpacingTokens
ThemeResolver ..> ResolvedTheme
ThemeResolver ..> ThemePreference
ThemeResolver ..> OutputMode
ThemeResolver ..> FnStrToOptString
OutputPrefs ..> LocalizationModule
OutputPrefs ..> KeysModule
LocalizationModule ..> LocalizedMessage
KeysModule <.. OutputPrefs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughSummarise centralised theme resolution: make OutputPrefs hold a ResolvedTheme; derive semantic prefixes and indentation from theme symbol/spacing tokens; update CLI parsing for Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI / Config / Env
participant Resolver as theme::resolve_theme
participant Prefs as OutputPrefs
participant Reporter as Reporter (Status/Timing)
User->>CLI: Provide --theme / env / config / defaults
CLI->>Resolver: Request resolved theme (apply precedence)
Resolver->>Resolver: Apply legacy no_emoji rules and mode defaults
Resolver->>Prefs: Return ResolvedTheme (symbols & spacing tokens)
Prefs->>Reporter: Construct OutputPrefs.from_theme(...) and provide tokens
Reporter->>Reporter: Render prefixes from resolved_theme.tokens.symbols + localized labels
Reporter->>Reporter: Format indentation from prefs.task_indent() / timing_indent()
Reporter->>User: Emit theme-consistent output (ASCII or Unicode)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…es and spacing - Replace emoji-based semantic prefix rendering with theme-backed symbol tokens. - OutputPrefs now delegates prefix and spacing decisions to the resolved theme. - Reporter modules use theme spacing tokens replacing hard-coded indent literals. - Add CLI theme flag and OrthoConfig integration for centralized theme resolution. - Add comprehensive unit, integration, and BDD test coverage for themes. - Update localization, docs, and roadmap to reflect theme-driven output design. This completes roadmap item 3.12.1 by making design tokens the single source for glyphs and spacing in CLI output, enabling consistent ASCII/Unicode rendering across the application. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on file defaults_object.insert("fetch_allow_scheme".to_owned(), json!(["https"]));
defaults_object.insert("progress".to_owned(), json!(true));
defaults_object.insert("diag_json".to_owned(), json!(false));
defaults_object.insert("theme".to_owned(), json!("auto"));❌ New issue: Large Method |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Refactored the cli merge test by extracting the setup code into a helper function `build_precedence_and_append_composer` and the assertions into `assert_precedence_and_append_invariants`. This improves readability and reuse of the test logic while preserving existing test coverage. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Several test modules now define their own
strip_isolates/normalize_fluent_isolates-style helpers; consider moving these into a shared test utility (e.g.test_support) to avoid duplication and keep Fluent-isolate normalization consistent across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several test modules now define their own `strip_isolates` / `normalize_fluent_isolates`-style helpers; consider moving these into a shared test utility (e.g. `test_support`) to avoid duplication and keep Fluent-isolate normalization consistent across tests.
## Individual Comments
### Comment 1
<location path="docs/netsuke-design.md" line_range="2088-2089" />
<code_context>
+Theme resolution for CLI output is centralized in `src/theme.rs`. Netsuke
+resolves one theme through OrthoConfig layers (`--theme`, `NETSUKE_THEME`,
+config file, then mode defaults) and hands the resulting symbol and spacing
+tokens to reporters through the `OutputPrefs` compatibility facade. This keeps
+reporter code focused on status semantics rather than glyph choice, preserves
+`no_emoji` as a legacy ASCII-forcing alias when no explicit theme is supplied,
+and gives later roadmap items a stable snapshot surface for validating ASCII
+and Unicode renderings without duplicating formatting rules.
+
For screen readers: The following flowchart shows how the build script audits
</code_context>
<issue_to_address>
**nitpick (typo):** Spelling of "facade" is inconsistent with earlier uses of "façade" for the same concept.
Here `OutputPrefs` is called a "compatibility facade", while elsewhere (e.g., exec plan docs) the same concept is spelled "façade". Please choose one spelling and use it consistently across the docs.
```suggestion
config file, then mode defaults) and hands the resulting symbol and spacing
tokens to reporters through the `OutputPrefs` compatibility façade. This keeps
```
</issue_to_address>
### Comment 2
<location path="docs/netsuke-design.md" line_range="2086" />
<code_context>
mode is off and also suppressed on failed runs so failures do not imply a
successful pipeline completion.
+Theme resolution for CLI output is centralized in `src/theme.rs`. Netsuke
+resolves one theme through OrthoConfig layers (`--theme`, `NETSUKE_THEME`,
+config file, then mode defaults) and hands the resulting symbol and spacing
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "centralized" is US spelling; en-GB style here should be "centralised".
Please change "centralized" to "centralised" to align this new text with the required en-GB-oxendic spelling convention.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/execplans/3-12-1-define-design-tokens.md (1)
483-530:⚠️ Potential issue | 🟡 MinorFix the inconsistent numbered list in "Concrete steps".
The numbered steps restart from 1 twice (lines 483, 503), creating a broken sequence: 1, 2, 1, 2, 1. Renumber consistently or use sub-list indentation to clarify the hierarchy.
📝 Proposed fix
## Concrete steps All commands below run from the repository root: ```sh cd /home/user/project
Establish the current baseline and locate the relevant code.
rg -n "theme|OutputPrefs|no_emoji|accessible|progress" src tests docsImplement the theme module and CLI/config wiring.
cargo test --workspace theme -- --nocapture-Expected shape after Stage B:
Expected shape after Stage B:
running N tests test ...theme... ok test ...cli... ok-1. Refresh BDD-generated scenario code if feature text changes.
+3. Refresh BDD-generated scenario code if feature text changes.```sh touch tests/bdd_tests.rs ```-2. Run the full validation gates with logged output.
+4. Run the full validation gates with logged output.```sh set -o pipefail && make check-fmt 2>&1 | tee /tmp/netsuke-3-12-1-check-fmt.log ... ```Expected final signal:
make check-fmt # exits 0 ...-1. Inspect scope before finalizing.
+5. Inspect scope before finalizing.git status --short git diff --stat</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/execplans/3-12-1-define-design-tokens.mdaround lines 483 - 530, The
ordered list in the "Concrete steps" section is inconsistent (restarts at 1);
update the sequence so the steps read 1 through 5 and adjust the "Expected shape
after Stage B:" block to not break the numbering (either indent it as a
non-numbered block or prefix with spaces), specifically renumber "Refresh
BDD-generated scenario code if feature text changes" to 3, "Run the full
validation gates with logged output" to 4, and "Inspect scope before finalizing"
to 5 while preserving the surrounding shell snippets and headings like
"Establish the current baseline and locate the relevant code" and "Implement the
theme module and CLI/config wiring".</details> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@docs/users-guide.md`: - Around line 799-801: Add a comma before "so" in the sentence "In all output modes, Netsuke uses semantic text prefixes so meaning is never conveyed solely by colour." to correctly join the two independent clauses; update the sentence to read "In all output modes, Netsuke uses semantic text prefixes, so meaning is never conveyed solely by colour." in the documentation (locate the sentence text and change it accordingly). In `@src/status_tests.rs`: - Around line 164-165: The assertion message for the indentation check uses a hardcoded phrase ("two spaces") but the code uses prefs.task_indent() as the source of truth; update the failure string in the assertion that wraps line.starts_with(prefs.task_indent()) to refer to the "resolved task token" (e.g., change "two spaces" to "the resolved task token") so the message matches prefs.task_indent()'s dynamic value in status_tests.rs. In `@tests/cli_tests/parsing.rs`: - Around line 77-86: Add a test case to cover the `--theme auto` option by adding another #[case(CliCase { ... })] entry similar to the existing ones: provide argv: vec!["netsuke", "--theme", "auto"], and assert theme: Some(ThemePreference::Auto) (keeping the rest from CliCase::default()); update the test cases in parsing.rs alongside the existing ascii and unicode cases to ensure `ThemePreference::Auto` is parsed correctly. In `@tests/cli_tests/policy.rs`: - Around line 10-15: Remove the local strip_isolates function from tests/cli_tests/policy.rs and replace its usages with the shared helper normalize_fluent_isolates from the test_support crate; import normalize_fluent_isolates (the function defined in test_support::fluent or test_support::normalize_fluent_isolates) at the top of the file and update all calls of strip_isolates(...) to normalize_fluent_isolates(...), ensuring you delete the duplicate strip_isolates definition so the file relies on the centralized test_support implementation. --- Outside diff comments: In `@docs/execplans/3-12-1-define-design-tokens.md`: - Around line 483-530: The ordered list in the "Concrete steps" section is inconsistent (restarts at 1); update the sequence so the steps read 1 through 5 and adjust the "Expected shape after Stage B:" block to not break the numbering (either indent it as a non-numbered block or prefix with spaces), specifically renumber "Refresh BDD-generated scenario code if feature text changes" to 3, "Run the full validation gates with logged output" to 4, and "Inspect scope before finalizing" to 5 while preserving the surrounding shell snippets and headings like "Establish the current baseline and locate the relevant code" and "Implement the theme module and CLI/config wiring".🪄 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:
0699325b-9e32-47cb-865a-3a90e10ed1d3📒 Files selected for processing (22)
docs/execplans/3-12-1-define-design-tokens.mddocs/netsuke-design.mddocs/roadmap.mddocs/users-guide.mdlocales/en-US/messages.ftllocales/es-ES/messages.ftlsrc/localization/keys.rssrc/output_prefs.rssrc/output_prefs_tests.rssrc/status.rssrc/status_tests.rssrc/status_timing.rssrc/theme.rstests/bdd/steps/accessibility_preferences.rstests/bdd/steps/cli.rstests/cli_tests.rstests/cli_tests/locale.rstests/cli_tests/merge.rstests/cli_tests/parsing.rstests/cli_tests/policy.rstests/features/cli.featuretests/features/progress_output.feature
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d03444537c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…keys - Introduced a new variant in LocalizedMessage to hold pre-rendered literal text. - Disabled argument interpolation when using literal messages to prevent misuse. - Updated OutputPrefs to use literal formatted prefixes instead of a localization key. - Removed deprecated semantic.prefix.rendered key and related localization entries. - Adapted tests to normalize Fluent isolates instead of stripping them, improving test stability. - Minor docs updates to correct style and clarify output formatting. This change improves flexibility for rendering localized messages that do not require Fluent key lookup, simplifying code paths and avoiding unnecessary localization overhead. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-12-1-define-design-tokens.md`:
- Line 8: Update the 3.12.1 "Define design tokens" execplan so the status fields
are consistent with the actual gate results: locate the top "Status: COMPLETE"
and any blocks that currently state "completion depends on a future full gate
run" (these appear in the section for 3.12.1 and the follow-up task blocks) and
either change those pending-block texts to reflect success (mirroring the
completed status) or revert the top "Status: COMPLETE" to an in-progress state
until the full gate commands have actually passed; ensure the same phrase/field
is updated in every occurrence so the execplan, the task blocks that mention
future gate runs, and any follow-up/result notes are aligned.
In `@docs/netsuke-design.md`:
- Line 2086: Replace the word "centralised" with the en-GB-oxendict "-ize" form
"centralized" in the sentence "Theme resolution for CLI output is centralised in
`src/theme.rs`." Update the text near the phrase referencing `src/theme.rs` so
the line reads "Theme resolution for CLI output is centralized in
`src/theme.rs`."
In `@src/localization/mod.rs`:
- Around line 87-113: The with_arg method currently panics when called on a
literal LocalizedMessage (created by LocalizedMessage::literal); replace the
runtime assert! with a typed Result to surface the misuse to callers: change pub
fn with_arg(...) -> Self to return Result<Self, LocalizedMessageError> (or
Result<Self, &'static str>), remove the assert!, return Ok(self_with_arg) on
success and Err(LocalizedMessageError::LiteralDoesNotSupportArgs) when
self.text.is_some(); update the signature and callers to handle the Result and
add/update docs for with_arg and LocalizedMessage::literal (alternatively,
consider introducing separate builder types for LiteralLocalizedMessage vs
KeyedLocalizedMessage to make this unrepresentable at compile time).
In `@src/output_prefs_tests.rs`:
- Around line 112-150: The test prefix_rendering_uses_theme_symbols currently
asserts exact en-US strings but doesn't pin the process-wide localizer; modify
this rstest to use a shared fixture that calls set_localizer_for_tests("en-US")
before creating prefs so the locale is fixed for all cases — create an
#[fixture] (e.g., en_us_localizer) that calls set_localizer_for_tests and
returns (), add it as a parameter to prefix_rendering_uses_theme_symbols, and
keep using resolve_from_theme_with to build OutputPrefs/LocalizedMessage; this
ensures prefix_fn(prefs).to_string() always compares against the expected en-US
literals.
- Around line 53-63: Remove the two duplicate tests
emoji_allowed_returns_true_when_permitted and
emoji_allowed_returns_false_when_suppressed and fold their cases into the
existing parameterised rstest for resolve_output_prefs (the test using
resolve_with). Update the rstest's parameter table in resolve_output_prefs to
include the two scenarios (default_allows_unicode /
explicit_no_emoji_forces_ascii) by adding the corresponding input and expected
emoji_allowed boolean values, then delete the standalone test functions so
behavior is covered only by the resolve_output_prefs/resolve_with parameterised
cases.
In `@src/output_prefs.rs`:
- Around line 67-70: The Rustdoc comments currently state concrete returned
strings (e.g., "✖ Error:" / "X Error:") without qualifying that these are for
the default en-US localizer; update the docs for the methods that use
keys::SEMANTIC_PREFIX_* (the localized prefix renderers) to either explicitly
say "For the default en-US localizer this returns …" or rewrite to
locale-neutral wording like "Returns the localized error prefix according to the
active locale (e.g., en-US yields '✖ Error:' for Unicode, 'X Error:' for
ASCII)"; apply the same change to the other affected doc comments (the blocks at
lines 89-92, 101-104, 123-126, 135-138) and mention set_localizer to make it
clear the output depends on the active locale.
🪄 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: be4dfe32-2403-4990-bfc7-3a4c5254f7f4
📒 Files selected for processing (13)
build.rsdocs/execplans/3-12-1-define-design-tokens.mddocs/netsuke-design.mddocs/users-guide.mdlocales/en-US/messages.ftllocales/es-ES/messages.ftlsrc/localization/mod.rssrc/output_prefs.rssrc/output_prefs_tests.rssrc/status_tests.rssrc/status_timing_tests.rstests/cli_tests/parsing.rstests/cli_tests/policy.rs
…turn String in output prefixes - Removed optionality from LocalizedMessage::key and eliminated text field. - Removed literal message creation and disallowed attaching args to literal messages. - Updated OutputPrefs prefix methods to return String instead of LocalizedMessage. - Updated tests to work with String prefixes and added fixture for en-US localizer. - Minor docs and naming cleanups related to localization and prefix rendering. These changes improve type safety and simplify rendering logic by using String directly for output prefixes and streamlining LocalizedMessage struct. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/output_prefs_tests.rs (1)
147-155:⚠️ Potential issue | 🟠 MajorKeep the locale fixture alive for the full assertion.
Line 153 drops
EnUsLocalizerFixtureimmediately, so theMutexGuardand
LocalizerGuardare released beforeprefix_fn(prefs)runs. Hold the fixture
in scope until the end of the test or this table becomes locale-flaky again.🩹 Proposed fix
fn prefix_rendering_uses_theme_symbols( - en_us_localizer: EnUsLocalizerFixture, + _en_us_localizer: EnUsLocalizerFixture, #[case] theme: Option<ThemePreference>, #[case] prefix_fn: fn(OutputPrefs) -> String, #[case] expected: &str, ) { - let _ = en_us_localizer; let prefs = resolve_from_theme_with(theme, None, OutputMode::Standard, |_| None); assert_eq!(prefix_fn(prefs), expected); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/output_prefs_tests.rs` around lines 147 - 155, The test prefix_rendering_uses_theme_symbols currently drops the EnUsLocalizerFixture immediately (let _ = en_us_localizer;) so the locale guard is released before calling prefix_fn(prefs); keep the fixture alive for the duration of the assertion by removing the discard binding and instead bind en_us_localizer to a named variable that remains in scope until after assert_eq (e.g., let _localizer = en_us_localizer;) so the MutexGuard/LocalizerGuard held by EnUsLocalizerFixture is not dropped before calling prefix_fn(prefs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/output_prefs_tests.rs`:
- Around line 58-67: The en_us_localizer fixture currently panics by calling
localizer_test_lock().expect(...); change the fixture signature from fn
en_us_localizer() -> EnUsLocalizerFixture to a fallible fn returning
Result<EnUsLocalizerFixture, _> and propagate the mutex error using the ?
operator when calling localizer_test_lock(); keep constructing
EnUsLocalizerFixture the same but return Ok(...). Then update any tests that
consume the en_us_localizer fixture to return Result<(), E> and accept the
fixture with a trailing ? (allowing the test to use the fallible fixture via ?),
ensuring tests propagate the error instead of panicking.
In `@src/output_prefs.rs`:
- Around line 4-7: Update the module-level Rustdoc at the top of
src/output_prefs.rs to describe that this module resolves a full theme
(including the NETSUKE_THEME env var) and exposes localized semantic prefix
helpers plus spacing tokens such as task_indent() and timing_indent();
specifically mention NETSUKE_THEME and that preferences are auto-detected from
NO_COLOR, NETSUKE_NO_EMOJI, and NETSUKE_THEME (or can be forced via config), and
note the presence of the task_indent() and timing_indent() helpers so cargo doc
reflects current public behaviour.
---
Duplicate comments:
In `@src/output_prefs_tests.rs`:
- Around line 147-155: The test prefix_rendering_uses_theme_symbols currently
drops the EnUsLocalizerFixture immediately (let _ = en_us_localizer;) so the
locale guard is released before calling prefix_fn(prefs); keep the fixture alive
for the duration of the assertion by removing the discard binding and instead
bind en_us_localizer to a named variable that remains in scope until after
assert_eq (e.g., let _localizer = en_us_localizer;) so the
MutexGuard/LocalizerGuard held by EnUsLocalizerFixture is not dropped before
calling prefix_fn(prefs).
🪄 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: b0351536-bf5f-4ea9-af35-768ca124b3fa
📒 Files selected for processing (4)
docs/execplans/3-12-1-define-design-tokens.mddocs/netsuke-design.mdsrc/output_prefs.rssrc/output_prefs_tests.rs
- Expanded output preference resolution to support full CLI themes via `NETSUKE_THEME`. - Updated module docs to reflect theme and spacing-token helper functionality. - Preferences now auto-detected from `NO_COLOR`, `NETSUKE_NO_EMOJI`, and `NETSUKE_THEME`. - Added helpers like `task_indent` and `timing_indent` for semantic spacing. - Improved output_prefs tests for robust localizer handling with better error management. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replaced calls like prefs.error_prefix().to_string() with prefs.error_prefix() directly, as the prefixes are already in the correct type. This change simplifies the code and avoids redundant conversions in test and accessibility preference steps. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/status_tests.rs (1)
76-96:⚠️ Potential issue | 🟠 MajorPin the localizer for these literal assertions.
Install
en-USunder the shared localizer lock before these tests run. They still bake in English fragments ("Task …"/"Stage …"), so any parallel test that swaps the process-wide localizer can make them fail or pass vacuously. Mirror theen_us_localizerfixture pattern already used insrc/output_prefs_tests.rsand inject it into the locale-sensitive cases. As per coding guidelines, "In Rust tests, userstestfixtures for shared setup".Also applies to: 123-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/status_tests.rs` around lines 76 - 96, Both locale-sensitive tests need the en-US localizer pinned to avoid cross-test locale flakiness: add the existing en_us_localizer rstest fixture (same pattern used in src/output_prefs_tests.rs) as an extra parameter to the task_progress_update_formats_expected_text case-based test and to indicatif_reporter_ignores_task_updates_when_stage6_is_not_running (so they receive the shared localizer before running). This ensures calls to task_progress_update, IndicatifReporter::report_task_progress (force_text_reporter), stage6_message and normalize_fluent_isolates observe the en-US localizer during assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/output_prefs.rs`:
- Around line 62-64: The render_prefix function currently hardcodes the ordering
and spacing with format!("{symbol} {label}"), preventing locales from
controlling punctuation/ordering; replace that by fetching the localized
template via localization::message(SEMANTIC_PREFIX_RENDERED) and interpolate the
symbol and label into that template (e.g. replace the "{symbol}" and "{label}"
placeholders in the returned string). Update render_prefix to use
SEMANTIC_PREFIX_RENDERED and the localized template so translators control the
final rendering.
---
Duplicate comments:
In `@src/status_tests.rs`:
- Around line 76-96: Both locale-sensitive tests need the en-US localizer pinned
to avoid cross-test locale flakiness: add the existing en_us_localizer rstest
fixture (same pattern used in src/output_prefs_tests.rs) as an extra parameter
to the task_progress_update_formats_expected_text case-based test and to
indicatif_reporter_ignores_task_updates_when_stage6_is_not_running (so they
receive the shared localizer before running). This ensures calls to
task_progress_update, IndicatifReporter::report_task_progress
(force_text_reporter), stage6_message and normalize_fluent_isolates observe the
en-US localizer during assertions.
🪄 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: bb9e1cc1-9e8e-4b8d-9d2f-c14ea77b262d
📒 Files selected for processing (4)
src/output_prefs.rssrc/output_prefs_tests.rssrc/status_tests.rstests/bdd/steps/accessibility_preferences.rs
…ed tests - Introduce a new localization key `semantic.prefix.rendered` for dynamic prefix rendering using placeholders. - Update en-US and es-ES Fluent files to include the new rendered prefix message. - Modify `OutputPrefs::render_prefix` to use the new localization key, allowing symbol and label substitution. - Extend status tests to support localization setup with fixtures and ensure correct localized output in tests. - Add necessary imports and error handling to support localization in tests. This improves localization flexibility for status prefixes and strengthens test coverage with localized messages. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
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 (1)
src/status_tests.rs (1)
150-170:⚠️ Potential issue | 🟠 MajorPin the localizer in every test that asserts localized output.
Add
en_us_localizerto these four tests and returnResult<()>. They still read throughlocalization::message(...)or compare against pinned English text without taking the global localizer lock, so they remain order-dependent under parallel execution even though the fixture above already serializes the safe path. Mirror the fixture pattern used at Lines 119-147.Also applies to: 175-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/status_tests.rs` around lines 150 - 170, Tests asserting localized output (e.g., indicatif_reporter_sets_stage6_bar_message_for_non_text_updates and the three others around 175-233) must pin the global localizer to avoid order-dependent failures; update each such test signature to accept the en_us_localizer fixture and return Result<()> (mirror the pattern used in the fixture block around lines 119-147), and ensure any calls to localization::message(...) remain valid under the pinned localizer before returning Ok(()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/status_tests.rs`:
- Around line 150-170: Tests asserting localized output (e.g.,
indicatif_reporter_sets_stage6_bar_message_for_non_text_updates and the three
others around 175-233) must pin the global localizer to avoid order-dependent
failures; update each such test signature to accept the en_us_localizer fixture
and return Result<()> (mirror the pattern used in the fixture block around lines
119-147), and ensure any calls to localization::message(...) remain valid under
the pinned localizer before returning Ok(()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5a5242c-e711-4870-9cb2-f976c5f7f088
📒 Files selected for processing (5)
locales/en-US/messages.ftllocales/es-ES/messages.ftlsrc/localization/keys.rssrc/output_prefs.rssrc/status_tests.rs
- Updated test functions in src/status_tests.rs to return Result<()> for better error propagation. - Replaced assert! macros with ensure! to provide clearer error messages. - Added module-level allow attributes to suppress clippy lints triggered by rstest macro expansions. - Improved test robustness by properly handling localizer fixture errors with the ? operator. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/status_tests.rs`:
- Around line 46-72: The custom EnUsLocalizerFixtureError and its impls are
unnecessary; change the en_us_localizer fixture to return
anyhow::Result<EnUsLocalizerFixture>, remove the EnUsLocalizerFixtureError type
and its impls, and convert the localizer_test_lock() error into an anyhow error
(e.g., via .map_err(anyhow::anyhow!(...)) or .context(...)); keep the rest of
en_us_localizer (Arc::from(cli_localization::build_localizer(...)) and
localization::set_localizer_for_tests(...)) unchanged.
- Around line 3-11: Add a FIXME comment above the module-level allow block
explaining this is a last-resort suppression and reference a tracking issue
(e.g., upstream rstest fix or refactor issue) so it can be removed later; keep
the explanatory sentence about rstest macro expansion but insert a FIXME URL
like "https://github.com/your-org/issue-number" and remove the unnecessary
clippy allows that silence the detector (drop clippy::allow_attributes and
clippy::allow_attributes_without_reason) leaving only clippy::too_many_arguments
if absolutely required so the module attribute reads clearly and is traceable.
🪄 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: 17ce76c5-70ca-4c95-84e3-88447294ff0e
📒 Files selected for processing (1)
src/status_tests.rs
…tatus_tests - Removed EnUsLocalizerFixtureError type and its impls. - Changed en_us_localizer fixture to return anyhow::Result directly. - Updated relevant test cases to use simplified error type. - Added module-level allow and FIXME comments for clippy::too_many_arguments lint from rstest macro expansions. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/status_tests.rs`:
- Around line 3-14: Create a real tracking issue and replace the "TBD"
placeholder in the module-level FIXME comment that precedes the #[allow(...)]
(the comment referencing rstest macro and the module-level
clippy::too_many_arguments allow) with the actual GitHub issue number (e.g.,
change "https://github.com/leynos/netsuke/issues/TBD" to
"https://github.com/leynos/netsuke/issues/<your-issue-number>") so the FIXME has
a concrete link; ensure the comment still begins with "FIXME:" and that the URL
points to the newly created issue that tracks removal of the suppression.
🪄 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: 05f2102e-286d-44b0-abc9-f81f4e4f4278
📒 Files selected for processing (1)
src/status_tests.rs
…mples The documentation examples for resolve_with, error_prefix, and success_prefix were updated to remove redundant .to_string() method calls, improving clarity and correctness in code samples. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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
Changes
Rationale
Testing
◳ Generated by DevBoxer (https://www.devboxer.com)
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/2e2059db-2a9d-49e9-9c49-dd836842c289
📎 Task: https://www.devboxer.com/task/cac40015-71f5-4550-abb8-78f9f0aaf89b