fix(qiraat): resolve overlapping reader colors in readers panel (#3286) - #3310
fix(qiraat): resolve overlapping reader colors in readers panel (#3286)#3310shoaibhasann wants to merge 2 commits into
Conversation
…n#3286) The readers panel colored each transmitter tag with a per-item first-match lookup (`readings.find(r => r.matrix.readers.includes(reader.id))`). When a reader appears in more than one reading without transmitter-level disambiguation (e.g. 2:245, where Abū Jaʿfar and Ibn Kathīr are listed in both the default and the pink readings), the earliest reading always won, so the later reading's color never showed up in the panel. Replace the per-item lookup with a panel-wide assignment pass (`buildTransmitterReadingAssignments`): - direct `matrix.transmitters` matches stay authoritative; - unambiguous tags are assigned first and their colors recorded as represented; - ambiguous tags then prefer the first candidate reading whose color is not yet represented, falling back to the previous first-match behavior when every candidate color already appears. The transmitter click-to-scroll handler now reuses the same assignment, so a tag's color and the card it scrolls to always agree. Add unit tests covering the 2:245 regression, the 7:165 duplicate transmitter-level case, direct-match precedence, the all-represented fallback, null-color normalization, and the no-match / empty-readings edge cases.
![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: 3ebb13e0e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const unrepresented = candidates.find( | ||
| (reading) => !representedColors.has(normalizeColor(reading.color)), | ||
| ); | ||
| assignments.set(transmitterId, toAssignment(unrepresented ?? candidates[0])); |
There was a problem hiding this comment.
When multiple reading colors are only reachable through ambiguous tags, every ambiguous tag keeps choosing the same first unrepresented candidate because representedColors is not updated during pass 2. For example, if the panel has ambiguous tags that can represent white/green/pink and green/pink have no unambiguous tag, all of those tags are assigned green here, leaving pink hidden and making clicks unable to scroll to that reading—the same class of issue this helper is intended to fix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch on the gap, thanks. Confirmed by reproducing it: when two or more reading colors are reachable only through ambiguous tags (no unambiguous representative), the original pass 2 assigned all of them the same first missing color and left the rest hidden.
I didn't apply the "mark each ambiguous assignment as represented" fix directly, though — it regresses the primary case this PR targets (2:245): after Abū Jaʿfar claims pink, Ibn Kathīr would find both of its candidate colors (white + pink) represented and fall back to candidates[0] = the white base reading, so Ibn Kathīr reverts to white — the exact bug being fixed.
Fixed in 43213a5 by splitting pass 2 into two sub-passes:
- 2a assigns the first still-missing color and marks it represented, so a second color reachable only through ambiguous tags is covered by a different tag (your scenario).
- 2b assigns tags whose candidate colors are all represented, reusing a color already chosen by another ambiguous tag so sibling transmitters stay grouped (both Abū Jaʿfar and Ibn Kathīr stay pink on 2:245, where pink is the only missing color), falling back to first-match otherwise.
Added two tests: one for the multi-hidden-color distribution case, one for the single-missing-color sibling-grouping case. Both pass alongside the existing 2:245 regression test.
Follow-up to the panel-wide assignment: pass 2 assigned each ambiguous tag the first still-missing color but never marked that color represented, so when two or more reading colors were reachable *only* through ambiguous tags, every tag picked the same first color and the others stayed hidden — the same failure the helper exists to prevent. Split pass 2 into two sub-passes: - 2a assigns the first still-missing color and marks it represented, so a second color reachable only through ambiguous tags is covered by a different tag. - 2b assigns tags whose candidate colors are all represented, reusing a color already chosen by another ambiguous tag to keep sibling transmitters grouped (e.g. both Abū Jaʿfar and Ibn Kathīr stay pink on 2:245, where pink is the only missing color), falling back to first-match otherwise. Naively marking every ambiguous assignment represented (one obvious fix) would regress 2:245: after Abū Jaʿfar claims pink, Ibn Kathīr would find both its colors represented and fall back to white — reintroducing the reported bug. The sibling-reuse pass 2b avoids that. Add tests for the multi-hidden-color case and the sibling-grouping (2:245) case.
1 participant
Summary
Closes: #3286
On
/{chapter}:{verse}/qiraat, the readers/transmitters panel dropped a reading color whenever a reader appeared in more than one reading. On2:245, the reading cards show four colors (white, green, blue, pink), but the left panel only showed three — the pink readingفَيُضَعِّفُهُhad no reader/transmitter association because Abū Jaʿfar and Ibn Kathīr are listed in both the default (white) and pink readings.Root cause
ReaderItemcolored each transmitter tag with a per-item first-match lookup:Because the white reading is ordered before the pink reading, an ambiguous reader always resolved to white, and the pink candidate was never reached. The same first-match assumption existed in the transmitter click-to-scroll handler. Per the issue, this pattern also affects
6:145, 7:161, 10:35, 12:23, 18:44, 19:25, 20:63, 22:39(reader-level) and7:165(duplicate transmitter-level).Fix
Replace the per-item lookup with a panel-wide assignment pass,
buildTransmitterReadingAssignments, implementing the mitigation proposed in the issue:matrix.transmittersmatches stay authoritative.The transmitter click-to-scroll handler reuses the same assignment, so a tag's color and the card it scrolls to always agree.
For
2:245this resolves correctly: ʿĀṣim keeps white, while Abū Jaʿfar and Ibn Kathīr map to the otherwise-missing pink.Type of Change
Scope Confirmation
Rollback Safety
Test Plan
Testing steps:
https://quran.com/2:245/qiraat.فَيُضَـٰعِفَهُۥ.New unit tests (
transmitterReadingAssignments.test.ts) cover:2:245regression (overlapping reader-level mappings),7:165duplicate transmitter-level case,matrix.transmittersprecedence over reader-level fallback,nullreading color normalization,Edge Cases Verified
Pre-Review Checklist
Code Quality
anytypes used in production code (test helpers cast fixture objects, isolated to the test file)Testing & Validation
vitest run)eslint)Reviewer Notes
This is a frontend-only mitigation that preserves the current API payload shape. As the issue notes, the cleaner long-term fix is transmitter-level disambiguation in the payload; this change prevents missing colors in the panel regardless.
AI Assistance Disclosure