fix(viewer): bound chronicle a11y by cumulative char budget so long beats never slice off the action controls (#752) - #902
Conversation
…eats never slice off the action controls (#752) #890 bounded the chronicle's a11y exposure to the most-recent CHRONICLE_A11Y_TAIL=8 rendered rows (older rows aria-hidden), but a ROW count is not a CHAR count. A normal mid-session DM beat is a multi-paragraph turn (~1000–2000 chars), so 8 exposed rows still total ~12000+ chars of a11y YAML — past the 9000-char ariaSnapshot().slice(0,9000) budget the QA blind-player reads. Because the Chronicle (role="log") renders BEFORE the Actions palette + composer in DOM order, that overflow STILL slices the action controls off the snapshot entirely ("player can't find the action buttons") — which is why #752 remained flagged by 2-3/5 personas at sweep 3582dc2 DESPITE #890. Fix (additive, viewer-only, READ-ONLY on state): under the CHRONICLE_A11Y_TAIL row ceiling, ALSO bound the EXPOSED (non-aria-hidden) rows by a cumulative CHARACTER budget. • new const CHRONICLE_A11Y_CHAR_BUDGET = 2800 — conservative; leaves headroom under 9000 for the YAML inflation + the Actions palette + composer that follow in the DOM. • new pure, window-exported helper chronicleA11yExposedCount(visibleTextLengths): accumulates visible text length from the NEWEST row backward, stops before exceeding the budget, ALWAYS exposes ≥1 (the newest beat is never hidden, even one bigger than the whole budget), never more than CHRONICLE_A11Y_TAIL. • chronicleRowVisibleTextLength derives each row's visible text length the same way LogEntry renders it (sanitizeNarration for narration/dialogue, raw text/detail else). • chronicleRowAriaHidden gains an optional `exposed` cutoff; the 2-arg row-only form still works (legacy callers keep the original tail bound). No regression for an early-session player: a SHORT session (cumulative text under budget AND rows ≤ tail) still exposes EVERY row. The newest beat is ALWAYS exposed. Tests: extend viewer/tests/test_chronicle_a11y_bound.py with a long-beat case (8 rows × ~1500 chars must expose fewer than 8 so cumulative exposed text ≤ budget), the always-expose-newest invariant, the conservative-budget bound, and the short-session no-regression case. Existing tail-constant + row-bound tests unchanged and still green.
|
Warning Review limit reached
More reviews will be available in 1 hour, 52 minutes, and 23 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
![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: 9efd6bc7bf
ℹ️ 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".
| // the action controls off the 9000-char ariaSnapshot. chronicleA11yExposedCount walks the rendered | ||
| // rows' visible text lengths (most-recent last) and returns how many trailing rows stay exposed; | ||
| // the rest are aria-hidden (still visible for sighted scroll-back / preserved in the Quest Journal). | ||
| const chronicleA11yExposed = chronicleA11yExposedCount(renderedLog.map(chronicleRowVisibleTextLength)); |
There was a problem hiding this comment.
When the newest chronicle row sanitizes to empty, this budget calculation still gives that invisible row the mandatory newest slot. For example, a long DM beat followed by a wrapper/scaffolding narration that sanitizeNarration drops produces lengths like [>2800, 0]; chronicleA11yExposedCount returns 1, so chronicleRowAriaHidden hides the actual latest visible beat and exposes only an empty wrapper div. In that state screen-reader users lose the DM reply instead of hearing the newest beat, so the budget should be computed over visible rows or explicitly force lastVisibleLogIndex into the exposed window.
Useful? React with 👍 / 👎.
1 participant
#752 — long DM beats STILL slice the action controls off the a11y snapshot, despite the row-count bound
The QA blind-player reads the screen via
ariaSnapshot().slice(0, 9000)— a hard 9000-char cap on the body's accessibility YAML, in DOM order. The Chronicle (role="log") renders BEFORE the Actions palette + composer.#890 bounded the chronicle's a11y exposure to the most-recent
CHRONICLE_A11Y_TAIL=8rendered rows (older rowsaria-hidden, excluded from the snapshot). But 8 is a ROW count, not a CHAR count. A normal mid-session DM beat is a multi-paragraph turn (~1000–2000 chars), so 8 exposed rows still total ~12000+ chars of a11y YAML — past the 9000-char budget. The Actions/composer that follow in DOM order get sliced off entirely ("player can't find the action buttons"). That is why #752 was still flagged by 2-3/5 personas at sweep3582dc2despite #890.The fix (additive, viewer-only, READ-ONLY on state)
Under the
CHRONICLE_A11Y_TAILrow ceiling, ALSO bound the EXPOSED (non-aria-hidden) rows by a cumulative CHARACTER budget:CHRONICLE_A11Y_CHAR_BUDGET = 2800— conservative; leaves headroom under 9000 for the YAML inflation + the Actions palette + composer that follow in the DOM.chronicleA11yExposedCount(visibleTextLengths)— new pure,window-exported helper. Walks the rendered rows' visible text lengths (most-recent last), accumulates from the newest backward, stops before exceeding the budget, always exposes ≥1 (the newest beat is never hidden, even one bigger than the whole budget), never more thanCHRONICLE_A11Y_TAIL.chronicleRowVisibleTextLength(entry)— derives each row's visible text length the SAME wayLogEntryrenders it (sanitizeNarrationfor narration/dialogue, rawtext/detailotherwise).chronicleRowAriaHiddengains an optionalexposedcutoff arg; the 2-arg row-only form still works (legacy callers keep the original tail bound).chronicleA11yExposedfromrenderedLogand threads it into the row map'saria-hidden.Invariants held
Tests
Extended
viewer/tests/test_chronicle_a11y_bound.py(transpiles the JSX under Node, evaluates the exported helpers):test_long_beats_expose_fewer_rows_so_cumulative_text_fits_budget— 8 rows × ~1500 chars must expose fewer than 8 so cumulative exposed text ≤ budget (the row-count-only bound exposes all 8 = ~12000 chars; FAILS pre-fix).test_newest_beat_is_always_exposed_even_when_it_alone_exceeds_budgettest_char_budget_constant_is_present_and_conservativetest_short_low_volume_session_still_exposes_every_row_no_regressionThe existing tail-constant + 30-row bound + short-chronicle tests are unchanged and still green.
Verification
test_chronicle_a11y_bound.py— 7 passed (3 original + 4 new)test_chronicle_dedup_opening.py+test_chronicle_hygiene.py— 32 passedviewer/tests/— 621 passed, 1 skippedqa/fast_gate.sh(engine seat-path guard) — PASS (219 deterministic, unaffected by a viewer change)