Skip to content

Make stream duplicate suppression configurable - #273

Open
calael wants to merge 2 commits into
codex/auto-layoutfrom
codex/duplicate-suppression
Open

calael wants to merge 2 commits into
codex/auto-layoutfrom
codex/duplicate-suppression

Conversation

@calael

@calael calael commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • 116 focused configuration/routing examples passed independently against current upstream.
  • The updated stack passed 4,317 examples in a clean checkout. RuboCop passed on all 251 inspected files; GitHub YARD checks pass.
  • YARD checks were not run locally because the dependency is unavailable.

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: de2ff42a-abcd-4a18-b4c7-99e44d76e5e2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@calael calael left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Matt Tolton added 2 commits October 25, 2026 19:55
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.
@calael
calael force-pushed the codex/auto-layout branch from 85ee55b to f3b0166 Compare October 25, 2026 19:55
@calael
calael force-pushed the codex/duplicate-suppression branch from 8cc8875 to 8f0e0d7 Compare October 25, 2026 19:55
@calael

calael commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator Author

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 MahtraDR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MahtraDR

Copy link
Copy Markdown
Contributor

Heads-up: ProfanityFE now runs CI as a single ci-gate check (#275), and branch protection is being switched on for main. Merging will need one approving review and a passing ci-gate.

This branch predates #275, so it has no ci-gate check yet. Merging the latest main into it will add one. The shared RuboCop rules from #274 apply too, so that merge may surface a few new lint findings (mostly non-ASCII characters).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants