Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
calael
left a comment
There was a problem hiding this comment.
AI/code-agent independent review of PR #273 (base 85ee55b → head 8cc8875).
One actionable finding:
[P2] Document the new ArgumentError so the YARD CI check passes — lib/settings_loader.rb:110–120, 172–174. The new invalid-enabled-value branch raises ArgumentError out of SettingsLoader.parse, but its docstring has no @raise tag. The repository checker deliberately requires a written tag even for private methods; CI run 40227889192 fails with “lib/settings_loader.rb:120 SettingsLoader#parse: calls raise but has no @raise” and exits 1. Add an @raise [ArgumentError] tag describing invalid duplicate-suppression enabled values, then rerun the documentation checks. This is a confirmed CI failure, not a runtime exception-handling defect: load rescues the error before applying settings.
CI evidence: https://github.com/elanthia-online/ProfanityFE/actions/runs/40227889192
Runtime review: I traced settings parsing/application, policy replacement on reload, stream source tracking, timestamp-aware matching, marker consumption, fallback-to-main and prompt expiration. I found no additional actionable runtime regression in this PR diff.
Validation: 150 examples passed with Ruby 4.0.5 using installed rspec directly across config_spec, settings_loader_spec, speech_timestamp_copy_spec, stream_duplicate_spec, prompt_tracker_spec, stream_routing_replay_spec and room_inline_streams_spec. git diff --check also passed. Bundled test/YARD commands could not start locally because locked dependencies (including yard and rubocop) are unavailable; the YARD finding above is independently verified from the actual CI failure log and checker/source inspection. No application code changed, no live game session exercised, and this is a comment-only review, not approval.
Add a reloadable XML policy with a global enable flag and per-stream exceptions. Preserve existing suppression defaults and timestamp matching, and test opt-out behavior, invalid settings, reloads, and custom streams.
85ee55b to
f3b0166
Compare
8cc8875 to
8f0e0d7
Compare
|
Addressed the documentation finding in 8f0e0d7 by adding @raise [ArgumentError] to SettingsLoader.parse for invalid duplicate-suppression enabled values. Runtime behavior is unchanged. Restacked this branch on the updated layout PR; GitHub's YARD check now passes. The updated full stack also passes all 4,317 examples and RuboCop. |
MahtraDR
left a comment
There was a problem hiding this comment.
PR Review: Make stream duplicate suppression configurable (#273)
Reviewed: 8f0e0d7 on 2026-10-26, full clone.
Summary: The routing change looks right. I ran 400 random sequences of stream lines, main copies, prompts and blanks through the real GameTextProcessor. At the default policy, head and base give identical output. With enabled='false', main shows every main-bound line in order. A speech exemption only ever brings back main copies and leaves the stream windows alone. One parse issue below needs fixing, plus a docs nit.
Minor
1. A typo in enabled stops the client from starting (lib/settings_loader.rb:174).
raise ArgumentError, "Invalid duplicate-suppression enabled='#{enabled}' (expected true/false)"The raise aborts the whole parse. On .reload that just means "settings unchanged", which is what the README describes. At startup nothing has loaded yet, so LAYOUT stays empty and load_settings_and_layout exits via fatal_error (lib/application.rb:735). Repro: a settings file with a layout plus <duplicate-suppression enabled='disabled'/> loaded with reload: false returns the ArgumentError and leaves LAYOUT and HIGHLIGHT empty. The same file with <blank-lines>maybe</blank-lines> loads normally.
This also goes against the loader's own doc ("A single invalid entry … is warned about and skipped without failing the file", lib/settings_loader.rb:76) and against <blank-lines> and <history-size>, which log and keep the default. Suggested fix: do the same here. Log something like Invalid duplicate-suppression enabled='…' (expected on/off), using on and keep the default. The message could also list every value it accepts, since yes/no/1/0 work too. The README sentence, the @raise tag and the reload spec would change with it. A spec for the reload: false case would close this.
Nit
2. Not in the User Guide (README.md:72). README sends readers to USER_GUIDE.md for the settings XML reference, and <history-size> and <blank-lines> are documented there. Adding <duplicate-suppression> to the guide, and to its Hot-Reloading list, would keep the reference complete.
|
Heads-up: ProfanityFE now runs CI as a single This branch predates #275, so it has no |
Summary
Add reloadable
<duplicate-suppression enabled='true' except='speech'/>settings. Support global disable and arbitrary per-stream exceptions, preserving the existing default behavior and timestamp-aware duplicate matching. Cover reloads, invalid values, speech exceptions, and script-defined streams.Stack
Part 4 of 4. Based on #272; please merge the preceding PR first. Each PR contains one logical change; later PRs target the preceding branch to keep review diffs isolated. Retarget/update the next PR to main after merging its predecessor.
Merge order: #270 → #271 → #272 → #273.
Personal layouts, personal layout tests, and Lich compressor changes are excluded.
Validation