Repository navigation
feat(magazines): drop unpaintable characters and verify the whole glyph font - #92
Merged
Merged
Conversation
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…ph font Closes the two open items from the post-transcription review of #20. Verification (the review's highest-risk gap) Only 2 of 36 glyphs had an exact-pattern assertion; the other 34 were checked for presence, not shape, so a row shift or bad PDF crop in any letter passed every test. `glyphs.test.ts` now asserts all 36 against `EXPECTED_GLYPHS`, recorded in Magpul's own sheet ordering (digits 1-9 then 0) rather than the fixture's, so a bad regeneration cannot pass by matching the artifact it came from. Confirmed to bite by corrupting one dot in glyph "G". `resolveDotMatrix` was also never run against the shipped font — every test used a synthetic fixture that carries a hyphen Magpul never drew. A second describe block re-runs the acceptance examples against the real `MAGPUL_GLYPHS`, and that immediately showed AE4 taking a different path under each table. AE1's e2e now reads the `r` attribute off all 60 circles and compares the drawn pattern to an expected string restated from the sheet — the only assertion spanning fixture -> parser -> resolver -> geometry. R4/R9 copy was exported and rendered but read by no test; it is now asserted at the builder level and as rendered text in the e2e. Behavior: unpaintable characters are dropped, not fatal (R9a) The floorplate has no hyphen cell, so a hyphen can never be painted — but #21 permits one in a stored label, so `A-1` was ordinary user input that rendered nothing and reported "does not fit", which is false. R8 already drops characters: `US04` on a 2-cell GL9 paints `04` and discards `US`. Refusing to paint `A-1` over one hyphen while painting `04` for `US04` was the inconsistent position, so the unpaintable character is now dropped and the rest is drawn, with a caveat naming what was left out. The collision this admits (`A-1` and `A1` paint alike) already existed under R8, where `US04` and `XY04` both paint `04`. Rejected: narrowing #21's character set, which would invalidate stored labels and need a migration. `unrepresentable` still gains a `reason` discriminant — it stays reachable for a label with nothing paintable in it (`--`), and that case gets its own message instead of the false "does not fit". Plan amended: R1, R2, R9, new R9a, AE4/AE5/AE5b, KTD8, and the Verification Contract. Stale "ships dark" comments corrected in four source files. `just ci-check` green (45 e2e). Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
WalkthroughChangesThe resolver now filters unsupported characters before sizing, reports omitted characters, and distinguishes unsupported labels from overflow. The label UI shows reason-specific messages and omission caveats. Tests verify all shipped glyphs, SVG geometry, and browser-visible outcomes. Dot-matrix handling
Sequence Diagram(s)sequenceDiagram
participant LabelView
participant resolveDotMatrix
participant GlyphTable
participant SVGOutput
LabelView->>resolveDotMatrix: label, model, and cell data
resolveDotMatrix->>GlyphTable: look up paintable characters
GlyphTable-->>resolveDotMatrix: glyphs and omitted characters
resolveDotMatrix-->>LabelView: matrix or reason-specific result
LabelView->>SVGOutput: render matrix and omission caveat
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Notion Comment |
coderabbitai
Bot
added
backend
documentation
There was a problem hiding this comment.
Pull request overview
This PR hardens the Magpul dot-matrix label feature by (1) making unsupported/unpaintable characters non-fatal (drop them and render what remains), and (2) upgrading verification so the entire shipped 36-glyph font and the rendered SVG dot geometry are asserted end-to-end.
Changes:
- Extend
resolveDotMatrixto drop characters missing from the glyph table (R9a), returnomitted, and add anunrepresentable.reasondiscriminant for correct messaging. - Replace spot-check glyph tests with a full 36-glyph expected-pattern guard and re-run acceptance examples against both the synthetic fixture and real shipped
MAGPUL_GLYPHS. - Add builder-level message tests and strengthen Playwright to read back SVG circle radii and assert the full 60-dot pattern for AE1.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/domain/magazines/glyphs.ts | Updates shipped-font commentary (font now transcribed; empty table still valid). |
| src/domain/magazines/dot-matrix.ts | Implements R9a dropping + omitted + unrepresentable.reason to distinguish “no paintable chars” vs “does not fit”. |
| src/domain/magazines/tests/glyphs.test.ts | Adds a full expected-pattern table for all 36 glyphs and asserts each exactly. |
| src/domain/magazines/tests/dot-matrix.test.ts | Runs resolver cases against both synthetic and shipped fonts; adds omitted/drop behavior tests. |
| src/domain/magazines/tests/dot-matrix-messages.test.ts | New tests asserting exported copy strings and builder composition. |
| app/(app)/magazines/dot-matrix-label.tsx | Renders new unrepresentable messaging + omitted-character caveat; exports copy builders/constants. |
| e2e/magazine-dot-matrix.spec.ts | Adds SVG dot-geometry assertion for AE1 and new rendered-text checks for R4/R9/R9a. |
| docs/residual-review-findings/20-render-magazine-label-as-a-magpul-paint-pen-dot-matrix-in-the-detail-view.md | Marks prior findings resolved and updates the “still open” list. |
| docs/plans/2026-08-02-001-feat-magazine-dot-matrix-label-plan.md | Updates product contract/requirements and acceptance examples for R9a + new messaging. |
| ); | ||
| }); | ||
|
|
||
| test("the not-paintable message never carries the unverified clause, since no cell count was consulted", () => { |
Comment on lines
80
to
83
| D --> E{Every character in the glyph font?} | ||
| E -->|no| Y[No matrix; state that the label does not fit] | ||
| E -->|no| Y[No matrix; state why it cannot be drawn] | ||
| E -->|yes| F{Label within cell count?} | ||
| F -->|yes| G[Render every character] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/plans/2026-08-02-001-feat-magazine-dot-matrix-label-plan.md (1)
74-87: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFlowchart still shows font coverage as all-or-nothing; it should show per-character dropping (R9a).
Node
Easks "Every character in the glyph font?" and itsnobranch goes directly toY(no matrix). Under R9a, a label with only some unsupported characters is not unrepresentable: the resolver drops those characters and continues to the length check with the paintable remainder (as implemented inresolveDotMatrixand tested indot-matrix.test.ts). Only a label with zero paintable characters reachesYon font grounds. Update the diagram to model the drop step so it matches R9a and the current implementation.📊 Proposed flowchart fix
D --> E{Every character in the glyph font?} - E -->|no| Y[No matrix; state why it cannot be drawn] - E -->|yes| F{Label within cell count?} + D --> E{Any character in the glyph font?} + E -->|no| Y[No matrix; state why it cannot be drawn] + E -->|yes| E2[Drop characters with no glyph] + E2 --> F{Paintable label within cell count?}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-08-02-001-feat-magazine-dot-matrix-label-plan.md` around lines 74 - 87, Update the flowchart around the font-coverage decision in the magazine label plan to model per-character filtering: pass labels with unsupported characters through a drop/resolve step, continue the cell-count check using the paintable remainder, and route to “No matrix” for font reasons only when zero characters remain. Keep the existing rendering and trailing-digit branches unchanged.Source: Path instructions
🧹 Nitpick comments (1)
src/domain/magazines/__tests__/dot-matrix.test.ts (1)
432-442: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
permittedfrom the real allowed-character source, not a hardcoded copy.This test's stated purpose is to catch drift if
#21ever widens its character set.permittedis a literal string identical toALL_CHARACTERS(line 22), not derived from the validator's actual allowed-character definition. If the real allowed set insrc/domain/magazines/constants.tschanges, this test keeps passing against the frozen literal and no longer proves the parity it claims to guard.Import and derive the set from the canonical source (e.g. build it from
MAGPUL_LABEL_ALLOWED_RE, whichglyphs.tsalready imports from constants.ts), or at minimum reuseALL_CHARACTERSinstead of re-declaring the same literal.♻️ Proposed fix
- const permitted = [..."0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ-"]; + const permitted = ALL_CHARACTERS;Based on learnings, the referenced plan (
docs/plans/2026-07-01-001-feat-magpul-mode-label-constraint-plan.md) instructs to "use the shared allowed character set and MAX_LABEL_LENGTH=4 rather than duplicating constraints."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/magazines/__tests__/dot-matrix.test.ts` around lines 432 - 442, Update the test’s permitted-character construction in the test describing “every character `#21` permits” to derive from the canonical validator source, preferably MAGPUL_LABEL_ALLOWED_RE from constants.ts, or reuse the existing ALL_CHARACTERS symbol if that is the established shared set. Remove the duplicated literal while preserving the assertion that only the hyphen lacks a glyph.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/magazine-dot-matrix.spec.ts`:
- Around line 261-283: Add a new magpulTest in the E2E magazine detail suite
using a recognized model and a label containing no paintable characters, such as
"--". Through addMagazineAndOpenDetail, assert that no dot-pattern image is
rendered and that LABEL_NOT_PAINTABLE_MESSAGE is visible, then retain cleanup
with deleteFromDetailPage in a finally block.
In `@src/domain/magazines/glyphs.ts`:
- Around line 15-19: Update the MAGPUL_GLYPHS_RAW documentation to state that
hyphens have no glyph and are dropped during parsing under R9a, and remove
hyphen from the R1 glyph-coverage list while preserving the remaining glyph
documentation.
---
Outside diff comments:
In `@docs/plans/2026-08-02-001-feat-magazine-dot-matrix-label-plan.md`:
- Around line 74-87: Update the flowchart around the font-coverage decision in
the magazine label plan to model per-character filtering: pass labels with
unsupported characters through a drop/resolve step, continue the cell-count
check using the paintable remainder, and route to “No matrix” for font reasons
only when zero characters remain. Keep the existing rendering and trailing-digit
branches unchanged.
---
Nitpick comments:
In `@src/domain/magazines/__tests__/dot-matrix.test.ts`:
- Around line 432-442: Update the test’s permitted-character construction in the
test describing “every character `#21` permits” to derive from the canonical
validator source, preferably MAGPUL_LABEL_ALLOWED_RE from constants.ts, or reuse
the existing ALL_CHARACTERS symbol if that is the established shared set. Remove
the duplicated literal while preserving the assertion that only the hyphen lacks
a glyph.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6f06bedd-7a8f-42d8-981d-677474fdb821
📒 Files selected for processing (9)
app/(app)/magazines/dot-matrix-label.tsxdocs/plans/2026-08-02-001-feat-magazine-dot-matrix-label-plan.mddocs/residual-review-findings/20-render-magazine-label-as-a-magpul-paint-pen-dot-matrix-in-the-detail-view.mde2e/magazine-dot-matrix.spec.tssrc/domain/magazines/__tests__/dot-matrix-messages.test.tssrc/domain/magazines/__tests__/dot-matrix.test.tssrc/domain/magazines/__tests__/glyphs.test.tssrc/domain/magazines/dot-matrix.tssrc/domain/magazines/glyphs.ts
Comment on lines
+261
to
+283
| magpulTest( | ||
| "AE9/R9: an all-digit label longer than the floorplate renders no matrix and states that it does not fit", | ||
| async ({ page }) => { | ||
| // GL9 is a *matched* 2-cell model, so the message carries no unverified | ||
| // clause — that pairing is what makes the exact wording assertable here. | ||
| const brandModel = "Magpul GL9"; | ||
| const label = "1234"; | ||
|
|
||
| try { | ||
| await addMagazineAndOpenDetail(page, brandModel, label); | ||
|
|
||
| await expect( | ||
| page.getByRole("img", { name: /Dot pattern to paint/ }), | ||
| ).toHaveCount(0); | ||
| await expect( | ||
| page.getByText("This label does not fit this magazine's floorplate."), | ||
| ).toBeVisible(); | ||
| } finally { | ||
| await deleteFromDetailPage(page); | ||
| } | ||
| }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an E2E case for labels with no paintable characters.
The new browser cases cover partial omission and doesNotFit. They do not execute the unsupportedCharacter branch.
Add a magpulTest with a label such as "--" on a recognized model. Assert that no dot-pattern image renders and that LABEL_NOT_PAINTABLE_MESSAGE is visible. This verifies the resolver-to-Callout contract for the new dedicated outcome.
As per coding guidelines, “new behavior needs coverage” and “Prefer integration and end-to-end tests over mocks when possible.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/magazine-dot-matrix.spec.ts` around lines 261 - 283, Add a new magpulTest
in the E2E magazine detail suite using a recognized model and a label containing
no paintable characters, such as "--". Through addMagazineAndOpenDetail, assert
that no dot-pattern image is rendered and that LABEL_NOT_PAINTABLE_MESSAGE is
visible, then retain cleanup with deleteFromDetailPage in a finally block.
Sources: Coding guidelines, MCP tools
Comment on lines
+15
to
+19
| * The fixture now carries all 36 glyphs transcribed from Magpul's diagram. | ||
| * An empty `GlyphTable` remains a valid, expected state rather than an error | ||
| * — it is what KTD3 relies on to suppress the feature entirely, and the | ||
| * parser must keep honoring it even though the shipped font is no longer | ||
| * empty. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the MAGPUL_GLYPHS_RAW header comment in raw.ts still contains stale R9/R1 hyphen wording.
rg -n -B2 -A2 'unrepresentable|hyphen' src/data/raw.tsRepository: unclesp1d3r/mag_stacker
Length of output: 677
Update the MAGPUL_GLYPHS_RAW documentation. State that hyphens have no glyph and are dropped during parsing under R9a. Remove hyphen from the R1 glyph-coverage list.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/domain/magazines/glyphs.ts` around lines 15 - 19, Update the
MAGPUL_GLYPHS_RAW documentation to state that hyphens have no glyph and are
dropped during parsing under R9a, and remove hyphen from the R1 glyph-coverage
list while preserving the remaining glyph documentation.
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
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.
Closes the two open items left after #90 — and supersedes #91, which carried the same findings doc on a branch that went stale when #90 squash-merged.
The verification gap (the higher-risk half)
glyphs.test.tsspot-checked the exact dot pattern for0and1. The other 34 glyphs were checked for presence, not shape — a row shift or a bad PDF crop in any letter passed every test in the repo. Nothing anywhere looked at rendered SVG dot positions either; AE1's e2e read the accessible name (U S 0 4), not the pattern.EXPECTED_GLYPHS, recorded in Magpul's own sheet ordering (digits1-9then0) rather than the fixture's, so a bad regeneration can't pass by matching the artifact it was derived from. A companion test asserts the expected table still covers all 36, so deleting a row can't quietly shrink the guard.Gturns two tests red.resolveDotMatrixhad never run against the shipped font. Every resolver test used a synthetic fixture that carries a hyphen Magpul never drew. A second describe block re-runs the acceptance examples against the realMAGPUL_GLYPHS— which immediately showed AE4 (AR-X) taking a different rule under each table.roff all 60 circles and compares the drawn pattern to a string restated from the sheet. That's the only assertion spanning fixture file → parser → resolver → geometry.dot-matrix-messages.test.ts) and as rendered text in the e2e.The hyphen question — resolved by dropping
The floorplate has no hyphen cell, so a hyphen can never be painted. But #21 permits one in a stored label, so
A-1was ordinary user input that rendered nothing and reported "This label does not fit this magazine's floorplate" — which is false; it never reached the length check.R8 already drops characters:
US04on a 2-cell GL9 paints04and discardsUS. Refusing to paintA-1over one hyphen while happily painting04forUS04was the inconsistent position. So an unpaintable character is now dropped and the rest is drawn, with a caveat naming what was left out.The collision this admits (
A-1andA1paint alike) already existed under R8, whereUS04andXY04both paint04.Rejected: narrowing #21's allowed character set, which would invalidate stored labels and need a migration.
unrepresentablestill gains areasondiscriminant — it stays reachable for a label with nothing paintable in it (--), and that case now gets its own message instead of the false "does not fit". That also closes the parked CodeRabbit finding from #90.Test plan
just ci-checkgreen — 45 e2e passed (up from 42)Still open (recorded in
docs/residual-review-findings/)MODEL_CELL_COUNTScarries only the GL9 and LR/SR seeds; everything else renders under R4's unverified 4-cell fallback. Now the largest unverified area in the feature.calibers.txt/manufacturers.txthave the same.txt↔raw.tsdrift the glyph fixture had before feat(magazines): render magazine labels as Magpul dot-matrix glyphs #90.