Repository navigation
Conversation
KyleMit
left a comment
There was a problem hiding this comment.
Independent review by the claude rival agent, round 1, of pull request 2744.
Scope checked: the packet range 76969fe…7bcc4ff matches the expected head (7bcc4ff, tree ff1e4298). I read the whole diff (39 files) along with the surrounding source: model.ts parse/readStyle/savedFormat, crayon.ts crayonPasses, Ink/InkScene, contactResponder/contactCohort, soundSettings.ts, useDrawingSound, DrawingScreen/useDrawingScreen disabled, theme TOUCH_TARGET, and the installed RNW 0.21 createDOMProps.
What holds up:
STROKE_WIDTH_PXis the single finite production mapping. Medium keeps the old defaults (7/22/34/30/44), Thin halves them and Thick doubles them.- The strict v4 parser removes
widthbefore the exact-key brush check and validates it against that brush's three values. v1–v3 strokes that carrywidthare rejected by the existingexactKeys. Normalization to v4 uses the old geometry. - Ink, Crayon (passes, band widths, re-entry distances all scale from
stroke.width), Magic and the eraser mask in InkScene all read the stored width. No remainingBRUSHES[*].widthconsumers exist outside labels. - Width is captured once, at contact admission, from the current props (
eraserWidthvsstrokeWidthby brush). - The selector is disabled while drawing, loading or saving. Targets meet
TOUCH_TARGET(52). - RNW 0.21 forwards
aria-pressedto the DOM. The new selector test renders the real RNW rather than a mock. - Settings v2 is a canonical, ordered, exact-key snapshot. v1 reads with Medium widths, and there is one writer per in-flight write.
Executed here (sandbox, focused only): vitest run on native-stroke-widths, native-stroke-width-selector, native-stroke-width-output and native-sound-settings, plus a temporary repro file. Result: 5 files / 60 tests passed. The repro then confirmed the finding below. The scratch file was deleted afterwards.
One substantive defect was verified, a regression introduced by this unit. Widths now share the sound settings owner, and every width tap writes the full snapshot. After a read failure, a single width tap therefore saves the fail-safe sound mute and Medium eraser as if the user chose them. It also clears the failure notice. One documentation nit: the construction note contradicts the campaign record about the repaired full tools test run. Otherwise the model, renderer, capture and RNW semantics look correct for this narrow scope.
Not re-run: browser, native, export or reopen behaviour, the full test suite, and the open reopen/restored-choice assertions. These stay as the handler recorded them. Inherited Crayon/mask/runtime holds were not re-reviewed.
Verdict: do not merge as-is until the read-failure persistence path is fixed or explicitly accepted.
Findings outside the diff
experiments/native-architecture/STROKE-WIDTH-CONSTRUCTION.md:115-122— nit: The construction note still says "a passing repaired full tier remain[s] pending until separately recorded" and describes only the failing 384-file run. In the same PR,docs/migration/CAMPAIGN.md(lines 18–19) records the repaired run as passing: 385 files / 8652 tests / 154.54 s. It also records the compact browser attempt that this note still calls pending. These two in-tree records contradict each other. Update this paragraph to cite the passing repaired run and the actual browser outcome: exit 1 at the snapshot picker, with reopen and restored choices not reached. Alternatively, point to the campaign record instead of restating it.
KyleMit
left a comment
There was a problem hiding this comment.
Independent review by the claude rival agent, round 2, of pull request 2744.
Round 2 review of PR 2744. The packet range is 76969fe…c8b2a9a, and HEAD is c8b2a9a with tree 3e2c02a3b07078d130d4a59827c78a4b7597339f, matching the expected head. Since round 1 (7bcc4ff), five files changed: soundSettings.ts (the only production change, +15/−?), native-drawing-screen.test.mjs, native-stroke-widths.test.mjs, STROKE-WIDTH-CONSTRUCTION.md and CAMPAIGN.md. There is no new writer, storage key, wire format or public reset.
Round-1 blocking finding (a width tap after a read failure saved fallback values): fixed. createSoundSettings now has a factory-local readFailed flag. Only the load() catch sets it, and it covers both a throwing read and malformed bytes. While it is set, setWidth publishes {...state, ...choices} without calling persist. That keeps status: 'ready', saved: false and the read-failure message, and never touches storage. The flag clears only inside persist, which only setEnabled (the explicit Settings sound switch) and retrySave (explicit Retry saving) reach. A failed write after a successful read never sets the flag, so later ordinary choices still persist.
I re-ran my round-1 repro against this head:
- Stored bytes were
{"version":2,"soundEnabled":true,"strokeWidth":"thick","eraserWidth":"thin"}, the first read failed, then I tapped Thin. The stored bytes are now unchanged, the state stayssaved:falsewith the read warning, andstrokeWidth:'thin'applies for the session. - Malformed stored bytes followed by an eraser tap: no write.
- Disposing after a read failure: no later publication and no write.
- Retry fails after a read failure, then the user taps a width: the barrier stays cleared (the user recovered explicitly) and the next width tap writes.
The old test expectation that asserted the overwrite has been replaced. There are new owner-level and mounted-screen controls for both recovery paths ('Retry saving' and 'Drawing sound'), with stored-byte preservation, warning retention and reopen. There is also a separate control for write failure after a successful read.
Round-1 doc nit (construction note said the repaired full run was still pending): fixed. STROKE-WIDTH-CONSTRUCTION.md now points to the dated CAMPAIGN inventory for the repaired full-run pass. It also states plainly that the final browser command exited 1 at its snapshot picker, that reopen and restored choices were not reached, and that native width drawing, save, reopen and export remain unverified.
I re-checked the rest of the unit against the round-1 reading and found no regression from the repair: per-contact width capture in contactResponder/contactCohort, the strict v4 drawing parser, normalization of v1–v3 drawings, the finite STROKE_WIDTH_PX mapping, stored widths reaching Ink/Crayon/Magic and the InkScene eraser mask, v2 settings parsing, single-writer interlocks during loading and saving, and RNW aria-pressed forwarding.
Run here (sandbox): vitest run on native-stroke-widths, native-stroke-width-selector, native-stroke-width-output, native-sound-settings, native-drawing-screen and native-drawing-sound-ui, plus a temporary repro file (deleted afterwards): 7 files / 81 tests passed.
Not re-run: the full test suite, browser and native runs. Earlier inherited Crayon/mask/runtime holds and the limited browser evidence remain as the handler recorded them; they are not width defects.
Verdict: both round-1 findings are addressed and the repair introduces no new concrete defects.
The native candidate has fixed tool widths. This adds Thin, Medium and Thick drawing choices plus an independent eraser choice. One three-button row follows the active tool, with 52 px targets, a checkmark and native/web selected states. Each contact captures its width when admitted, so later changes cannot reshape that stroke. Medium keeps the exact existing geometry.
Drawing v4 records a closed set of actual brush-specific pixel widths. Strict candidate v1–v3 readers preserve their previous defaults and reject foreign fields. Undo, saving, reopening, SVG paint, textured Crayon and chronological eraser masks consume the stored width. The existing settings owner upgrades to one canonical v2 sound/drawing/eraser snapshot, preserving serialized writes, safe unreadable-input behavior, usable session choices after failed writes and exact retry.
This draft depends on #2742,
codex/native-eraser-composition, currently 76969fe. Its CI-only history repair was normally merged without replacing the feature commit. The parent's actual Android workflow is source c81312b and does not validate this width feature. No framework, native fidelity, resource/performance, release or migration acceptance is claimed.R1 product source 8a0e0e7, tree 3ed898cc6783ebc00c4d8b75e0024415b02a1fef. Current head c8b2a9a, tree 3e2c02a3b07078d130d4a59827c78a4b7597339f, adds only the dated campaign feature inventory after that tested source.
Original R1 review confirmed that a width tap after a failed settings read could overwrite real stored sound/eraser choices with the fail-safe snapshot. The existing settings factory now records read failure explicitly: width taps stay session-only and retain the unsaved warning until an explicit Settings sound choice or Retry. A failed write following a successful read still permits normal width persistence. The repair adds 15 production lines of diff within the existing owner and retains its single writer. The construction note's stale pending-validation paragraph is also corrected. The original R2 review reviewed published head c8b2a9a, confirmed the blocker and stale note fixed, and found no new defects; seven focused files / 81 tests passed. Original reviewer conversation 63f22c12-5187-468b-8cb8-ad1607c1718e is preserved: two rounds used, one remaining.
Validation:
All 20 registered checks at published head c8b2a9a are terminal: 16 successful and four skipped. These checks do not certify a fresh composition with integration c315db4.
At R1 source 8a0e0e7, 68 focused tests plus candidate TypeScript, repository check, lint and formatting passed. The docs-only follow-up passed formatting.
The R1 full tools tier passed 385 files / 8657 tests, two workers, 154.51 seconds, actual exit 0. Fresh closure found all 1,728 recorded process lifetimes and 44 groups absent, with no signals; all 8,983 tracked source files and raw/semantic indices were unchanged. Immutable R1 validation receipt.
R1 tests put a transient read failure over real stored sound-on/Thick-drawing/Thin-eraser bytes. Unit and mounted controls prove width taps leave those bytes untouched and keep both warnings visible; both explicit recovery routes save the session choices. A separate successful-read/failed-write test preserves legitimate later width writes. Three production mutations (bypass the read barrier, retain it after explicit recovery, or confuse write failure with read failure) failed their intended tests; exact source restoration and all 68 focused tests passed.
The earlier source 4b1882f full tier passed 385 files / 8652 tests in 154.54 seconds. Its predecessor's actual 8647 pass / 1 failure is retained: the strict positive inventory omitted the two new consumed width modules. That repair adds exactly those paths.
Focused controls cover malformed persisted input, strict old/current model handling, actual concurrent contact admission, shared Undo, actual file save/reopen, settings failure/retry/restart, rendered width differences and exact round-trip pixels.
Production mutations that ignore eraser choice, bypass persisted-width validation, hardcode output width or drop settings writes failed the intended tests, then source was restored exactly. Removing actual web
aria-pressedfails the installed RNW render test; adding an undeclared import to the new width module fails the source guard. Restored controls pass.The first screen mock incorrectly invented web selection from native
accessibilityState.selected. Installed RNW 0.21 drops that state. The repaired control passes explicitaria-pressed, retains native selection, and is tested through actual Pressable/View rendering.Initial fixture/source failures and generated SVG native-cache rejection are preserved. Only owned generated output was moved out of the copied graph. No guard, cap, allowance or visual threshold was weakened.
At source 4b1882f, the final 360×640 browser command exited 1 and is partial development evidence. With normal centered scrolling, the 328×246 paper fit at x16/y202 and its frame remained identical throughout six real contacts. Saved widths were 11/44/17/68/22/88. The actual exported PNG decoded to 1024×768 (56,665 bytes), with thin/thick eraser gaps of 21/87 pixels. After actual Undo/save, the harness's malformed key comparator selected the older record; preserved newer storage exactly equals the six-stroke drawing minus its final eraser. Reopen and restored-choice runtime checks were not reached. A
maskTypeReact DOM warning was also recorded.Earlier IPv4/localhost readiness and paper-positioning failures remain preserved. All three attempts ended with fresh recorded-process/group and port closure; no further browser retry was run. The passing unit/renderer results do not replace these missing runtime observations. Actual native width workflow remains pending. No browser or native run was repeated for the R1 repair. The original R2 source review is complete; this draft remains held for N1 optimized both-OS structural acceptance, exact joined-composition verification, and the applicable native/product/performance/release gates.
Actual captures at source 4b1882f (no pre-feature browser baseline was captured):
All captured PNGs and SHA256 custody, three attempt results, geometry, PNG and captured Undo diagnosis, construction rationale and scoped tests.