Implement accessibility preferences via OutputPrefs (emoji/no-emoji)#267
Merged
Implement accessibility preferences via OutputPrefs (emoji/no-emoji)#267
Conversation
Contributor
Reviewer's GuideAdds a centralized OutputPrefs abstraction for emoji suppression and semantic prefixes, wires it through CLI parsing, status reporting, and main error handling, and updates localization, docs, and tests to cover accessibility preference behavior. Sequence diagram for status reporting with OutputPrefssequenceDiagram
actor User
participant Cli
participant Main as MainModule
participant Runner as RunnerModule
participant OutputPrefsMod as OutputPrefsModule
participant Reporter as AccessibleReporter
participant Stderr as StderrStream
User->>Cli: Invoke netsuke --accessible true --no-emoji true build
Cli-->>Main: Parsed Cli{accessible: Some(true), no_emoji: Some(true)}
Main->>OutputPrefsMod: resolve(merged_cli.no_emoji)
OutputPrefsMod-->>Main: OutputPrefs{emoji: false}
Main->>Runner: run(&merged_cli)
Runner->>OutputPrefsMod: resolve(cli.no_emoji)
OutputPrefsMod-->>Runner: OutputPrefs{emoji: false}
Runner->>Runner: mode = OutputMode::resolve(cli.accessible)
Runner->>Reporter: new(prefs)
Runner-->>Main: Ok(()) after build
loop build stages
Runner->>Reporter: report_stage(current, total, description)
Reporter->>Stderr: write Stage line
end
Runner->>Reporter: report_complete(tool_key)
Reporter->>OutputPrefsMod: success_prefix() on prefs
OutputPrefsMod-->>Reporter: LocalizedMessage "Success:" (no emoji)
Reporter->>Stderr: write "Success: {localized status complete message}"
Sequence diagram for error handling with semantic prefixessequenceDiagram
participant Main as MainModule
participant OutputPrefsMod as OutputPrefsModule
participant Runner as RunnerModule
participant Stderr as StderrStream
Main->>OutputPrefsMod: resolve(merged_cli.no_emoji)
OutputPrefsMod-->>Main: OutputPrefs prefs
Main->>Runner: run(&merged_cli)
Runner-->>Main: Err(err)
Main->>OutputPrefsMod: prefs.error_prefix()
OutputPrefsMod-->>Main: LocalizedMessage prefix
alt err is RunnerError
Main->>Main: report = Report::new(runner_err)
Main->>Stderr: write "{prefix} {report:?}"
else other error type
Main->>Stderr: write "{prefix} {other_err}"
end
Class diagram for OutputPrefs and related CLI/status structuresclassDiagram
class Cli {
+bool fetch_default_deny
+Option~bool~ accessible
+Option~bool~ no_emoji
+Option~Command~ command
}
class OutputPrefs {
-bool emoji
+bool emoji_allowed()
+LocalizedMessage error_prefix()
+LocalizedMessage warning_prefix()
+LocalizedMessage success_prefix()
+static OutputPrefs resolve(no_emoji: Option~bool~)
+static OutputPrefs resolve_with(no_emoji: Option~bool~, read_env: Fn~&str -> Option~String~~)
}
class StatusReporter {
<<trait>>
+report_stage(current: u32, total: u32, description: &str)
+report_pending(tool_key: &str)
+report_complete(tool_key: &'static str)
}
class AccessibleReporter {
-OutputPrefs prefs
+new(prefs: OutputPrefs) AccessibleReporter
+report_stage(current: u32, total: u32, description: &str)
+report_pending(tool_key: &str)
+report_complete(tool_key: &'static str)
}
class OutputMode {
<<enum>>
Accessible
Standard
+static resolve(accessible: Option~bool~) OutputMode
}
class RunnerModule {
+run(cli: &Cli) Result~()~
}
class MainModule {
+main() ExitCode
}
Cli --> OutputPrefs : no_emoji
MainModule ..> OutputPrefs : resolve(no_emoji)
RunnerModule ..> OutputMode : resolve(accessible)
RunnerModule ..> OutputPrefs : resolve(no_emoji)
RunnerModule ..> StatusReporter
AccessibleReporter ..|> StatusReporter
AccessibleReporter o--> OutputPrefs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduce an output preference system resolving emoji rendering from CLI (--no-emoji), environment (NO_COLOR, NETSUKE_NO_EMOJI) and config; export Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Parser
participant Main as main()
participant OutputPrefs as output_prefs::resolve()
participant Env as Environment
participant Runner as runner::run()
participant Reporter as AccessibleReporter
participant Stderr as Stderr
CLI->>Main: provide merged_cli (includes no_emoji)
Main->>OutputPrefs: resolve(merged_cli.no_emoji)
OutputPrefs->>OutputPrefs: evaluate explicit CLI/config
alt explicit no_emoji = true
OutputPrefs->>OutputPrefs: disable emoji
else
OutputPrefs->>Env: read NO_COLOR
Env-->>OutputPrefs: value or none
alt NO_COLOR set
OutputPrefs->>OutputPrefs: disable emoji
else
OutputPrefs->>Env: read NETSUKE_NO_EMOJI
Env-->>OutputPrefs: value or none
alt NETSUKE_NO_EMOJI set
OutputPrefs->>OutputPrefs: disable emoji
else
OutputPrefs->>OutputPrefs: enable emoji (default)
end
end
end
OutputPrefs-->>Main: return prefs
Main->>Runner: run(cli, prefs)
Runner->>Reporter: AccessibleReporter::new(prefs)
Reporter->>Reporter: compute prefix via prefs.success_prefix()
Reporter-->>Stderr: write prefixed message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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.
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: Comment on lines +213 to +224 fn error_prefix_contains_error_text_without_emoji() {
let prefs = OutputPrefs { emoji: false };
let rendered = prefs.error_prefix().to_string();
assert!(
rendered.contains("Error:"),
"expected 'Error:' in '{rendered}'"
);
assert!(
rendered.is_ascii(),
"expected ASCII-only prefix, got '{rendered}'"
);
}❌ New issue: Code Duplication |
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: tests/bdd/steps/accessibility_preferences.rs Comment on lines +101 to +108 fn render_error_prefix(world: &TestWorld) -> Result<()> {
let prefs = world
.output_prefs
.get()
.ok_or_else(|| anyhow::anyhow!("output prefs have not been resolved"))?;
world.rendered_prefix.set(prefs.error_prefix().to_string());
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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: 4dd8914b64
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/execplans/3-8-2-respect-accessibility-preferences.md`:
- Around line 99-101: Update the British-variant -ise spellings to -ize in the
document: change "localised Fluent messages" to "localized Fluent messages" and
any other occurrences of "-ise" (e.g., in the semantic-prefix description that
uses `$emoji` select and the surrounding lines referenced) to their "-ize"
forms; apply the same replacement to the other affected occurrences noted (lines
108-110 and 117-118) so all instances use the en-GB-oxendict "-ize" spelling
consistently.
In `@src/status.rs`:
- Around line 33-35: Update the doc comment that currently reads "localised
messages from the Fluent resource bundle" to use en-GB-oxendict -ize spelling by
replacing "localised" with "localized"; locate the doc comment containing
"localised messages from the Fluent resource bundle" in src/status.rs (the
module/section documenting completion messages) and change the word only so the
rest of the comment remains unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/execplans/3-8-2-respect-accessibility-preferences.md`:
- Around line 13-18: Update the semantic prefix list in the docs entry titled
"3-8-2-respect-accessibility-preferences.md" by inserting the Oxford comma so
the phrase reads "Error, Warning, and Success" instead of "Error, Warning,
Success"; locate the sentence containing "semantic prefixes (Error, Warning,
Success)" and edit it to include the comma before "and" to comply with the style
guideline.
In `@docs/users-guide.md`:
- Around line 625-628: Reflow the paragraph that describes semantic text
prefixes so it is wrapped at 80 columns and insert the Oxford comma when listing
the three prefixes — render them as `Error:`, `Warning:`, and `Success:` — and
ensure the sentence about including a leading glyph when emoji are permitted is
kept intact and grammatically correct while fitting the 80-column wrap.
Owner
Author
|
@coderabbitai Have the following now been resolved? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2557 🏁 Script executed: Length of output: 358 🏁 Script executed: Length of output: 76 🏁 Script executed: Length of output: 1413 🏁 Script executed: Length of output: 229
Overall Comments ✅1. Single OutputPrefs instance passed to runner:
2. Retain tracing::error! alongside stderr output:
Individual Comments ✅Comment 1 – BDD coverage for warning prefix:
Comment 2 – "Emoji are" (plural):
Comment 3 – Expand "BDD" acronym:
Comment 4 – "localized" (not "localised"):
All issues have been addressed! 🎉 |
leynos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@tests/features/accessibility_preferences.feature`:
- Around line 52-56: Add a new scenario in accessibility_preferences.feature
mirroring the existing "Error prefix includes emoji glyph in standard mode" and
"Warning prefix includes emoji glyph in standard mode" scenarios: create
"Scenario: Success prefix includes emoji glyph in standard mode" that ensures
emoji is enabled (not suppressed), renders the success prefix, and asserts the
prefix contains the success emoji (e.g., the same glyph used for success in the
suite) and the text "Success:"; follow the same Given/When/Then step pattern
used by the other two scenarios so it fits the existing step definitions and
keeps test coverage symmetric.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@docs/execplans/3-7-3-translator-tooling-and-documentation.md`:
- Around line 94-98: Paragraph lines exceed the 80-column rule; reflow the
sentence describing the discovery so each line is wrapped at or under 80
characters while preserving meaning: mention LocalizedMessage::with_arg converts
args to strings via .to_string(), that Fluent CLDR plural categories (e.g.,
[one], [few]) require numeric FluentValue types and thus fall back to the
[other] variant, and note that this limitation is documented in the translator
guide. Ensure references to LocalizedMessage::with_arg, FluentValue, and the
plural categories remain unchanged.
In `@tests/features/accessibility_preferences.feature`:
- Around line 47-72: For the three emoji-enabled scenarios ("Error prefix
includes emoji glyph in standard mode", "Success prefix includes emoji glyph in
standard mode", "Warning prefix includes emoji glyph in standard mode") add an
explicit assertion that the rendered prefix contains non-ASCII (emoji)
characters; locate those scenarios by the step "Given emoji is allowed" and
append a step similar to "And the prefix contains non-ASCII characters" so the
tests validate presence of an emoji glyph in addition to the existing "Then the
prefix contains ..." checks.
- Introduce OutputPrefs struct to encapsulate emoji allowance and semantic prefix formatting. - Honor NO_COLOR and NETSUKE_NO_EMOJI environment variables and --no-emoji CLI flag. - Update CLI to include no_emoji option; integrate output preferences into status reporting. - Add localised semantic prefixes with and without emoji for Error, Warning, Success. - Provide comprehensive unit and BDD tests covering preference resolution and output. - Update documentation and roadmap to reflect new accessibility preference support. This completes accessibility preference support for Netsuke output, ensuring emoji can be suppressed and semantic prefixes remain consistent across modes. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…stest Replaced multiple similar unit tests for error, success, and warning prefixes with a single parameterized test using rstest. This reduces code duplication and improves test maintainability by covering both emoji and non-emoji cases in one test function. Additionally, in accessibility preference BDD steps, introduced a helper function to abstract prefix rendering logic, simplifying step definitions for rendering error and success prefixes. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…nt output Refactor runner::run to accept output preferences, improving output handling consistency. Update main, tests, and relevant modules to pass resolved output preferences explicitly. Add BDD scenarios for warning prefix rendering in no-emoji mode. Adjust docs for wording fixes and terminology consistency. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
The output preference resolution logic was updated so that an explicit no_emoji value of Some(false) no longer forces emoji on unconditionally. Instead, Some(false) defers to environment variables like NO_COLOR and NETSUKE_NO_EMOJI, both of which use presence-based semantics to suppress emoji output. Only Some(true) forces emoji suppression unconditionally. This change ensures that setting --no-emoji false does not override environment variables that suppress emoji, addressing user expectations and aligning cli behavior with documented semantics. Updates include documentation, unit tests, BDD scenarios, and examples. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This change updates the output preference resolution logic to treat NETSUKE_NO_EMOJI environment variable values of "false" and "0" as indications to disable emoji output, ensuring consistent and expected behavior. Previously, these string values did not disable emoji as intended. The commit also clarifies the override semantics in the code comments and adds feature tests to verify this behavior. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…d dumb terminals - Implemented new accessible output mode replacing animated progress indicators with static, labelled status lines. - Accessible mode auto-detects environment variables (NO_COLOR and TERM=dumb) and respects explicit configuration flags. - Refactored output mode detection into an enum for extensibility. - Added comprehensive unit and BDD tests covering detection logic and output formatting. - Updated CLI parsing and config merging to support accessible mode. - Documented accessible output usage in user guide and execution plans. - Ensured compatibility with emoji and accessibility preferences, including semantic text prefixes. This change enhances usability for users relying on screen readers or less capable terminals by providing clear, static progress feedback. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
… characters in prefix Added a new step definition to verify the presence of non-ASCII (emoji) characters in error, success, and warning prefixes. Updated the accessibility preferences feature scenarios to include this verification step, ensuring prefixes correctly render emojis when allowed. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
The type is already imported; remove the redundant module path qualifier introduced during rebase conflict resolution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
devboxerhub
bot
force-pushed
the
respect-accessibility-preferences-ubavwa
branch
from
f655afd to
c9497a8
Compare
leynos
changed the title
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
Core functionality
“✖ Error: …” vs “Error: …”).
CLI and localization
Documentation
Tests
Files touched
How to test
Migration and compatibility
Risks and notes
If you want, I can adjust the PR description to emphasize particular user-facing benefits or tailor the test matrix for CI.
◳ Generated by DevBoxer ◰
📎 Task: https://www.devboxer.com/task/79254424-c0da-4f85-8b04-4116d4aa87d1