Repository navigation
refactor(web): use the optional NumberField for per-source limits (UI 0.142.0) - #2600
Conversation
First published release containing the optional and disabled NumberField (adea-ai/ui#444; 0.141.x was never published to npm). Its @adea-ai/themes range (^0.10.4) is satisfied by the pinned 0.10.6, so no second themes copy is added. NOTICE heading follows the pinned version and the release-please markers in bun.lock are restored. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
The document, byte, character and duration limits were native number inputs that parsed their own text. They now use NumberField with `optional` (adea-ai/ui#444): a blank field is accepted and reported as null, a null limit shows blank, and only values inside the range reach the settings draft. The locked state still comes from `sourceLocked()`. Payload shape is unchanged: the four fields stay `number | null`. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
0xPlayerOne
left a comment
There was a problem hiding this comment.
Review outcome: PENDING qualification for exact head 9c128bd. I personally reviewed the complete six-file patch and pinned NumberField implementation. The four ranges and null payloads are preserved; integer validation matches the backend limits, and manifest/lock versions agree. No concrete code blocker found so far. Before approval, complete the existing owner's narrow-layout, real keyboard/focus and running-source lock/unlock checks, including preserving a set limit during incomplete numeric input. Add focused fractional/upper-bound and dynamic disabled-state coverage where practical, and document that fractional drafts are now rejected as well as out-of-range drafts. Verify the installed package content corresponds to the reviewed NumberField implementation. Keep draft and auto-merge off pending these results; linked #2568 remains open.
…cument rejected drafts Fractional and over-range drafts and incomplete numeric text are not reported and the field restores the saved value on blur; the four limits lock while a source job runs and unlock when it finishes. The ingestion doc now says fractional drafts are rejected alongside range changes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
0xPlayerOne
left a comment
There was a problem hiding this comment.
Review outcome: PENDING small documentation correction and final qualification at 918f25f. I personally reviewed the complete initial patch and new tests/docs delta. The fractional/upper-bound/badInput and source-job lock/unlock cases address the main gaps; no production-code blocker found. Correct docs/ingestion.md: blank character limit uses connector defaults (not universally the global budget), and invalid drafts restore the latest accepted controlled value, which may be an unsaved edit, not necessarily the last saved value. For example saved7 -> accepted25 -> invalid2.5 -> blur restores25. Keep the candid limits: job-lock browser path was mock-bridge qualified, arrow keys were inconclusive on both baseline/candidate, and hosted draft-skipped checks are not passes. #2568 remains open. Re-review the final published head after docs correction.
…d draft restores Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
0xPlayerOne
left a comment
There was a problem hiding this comment.
Coordinating personal review outcome: APPROVED for the complete seven-file scope at exact head bd4fccd. I reviewed the original migration/dependency patch, test additions and final documentation correction. Null preservation, integer/range rejection, incomplete-input retention and dynamic source-job lock/unlock are covered; pinned implementation/package matching was verified by the owner. Documentation now accurately distinguishes connector defaults and latest accepted unsaved values. No remaining code blocker found; advisory review agrees. This personal-review COMMENT does not replace required GitHub approval. Required hosted checks must run and pass after readiness; draft skips are not passes. Browser stepping remains inconclusive on both baseline and candidate; job locking uses mock-bridge integration, and no live-provider/native-inset qualification is claimed. #2568 stays open. Any head change invalidates this review.
The accordion root cancels ArrowUp, ArrowDown, Home, End, PageUp and PageDown to move between its triggers, even when they come from a field inside an open panel. That stopped the four per-source limit fields stepping with the arrow keys and moved no caret in the text fields. Stop those keys at the panel when the target is an input or textarea, and keep the accordion's own trigger navigation. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MfzvPs9KL9LLoLyPv5DiLP
|
Qualification at Defect. ArrowUp and ArrowDown did not step any of the four limit fields, and Home, End, PageUp and PageDown were swallowed too. The same happens on main today with native inputs. A bare number input and the field's own Repair (bounded, one component). Checks at
Evidence, scripts and screenshots are in Generated by Claude Code |
0xPlayerOne
left a comment
There was a problem hiding this comment.
Personal review outcome: PENDING final owner evidence/CI at exact head a48c91f. I reviewed the complete two-file delta from previously source-approved bd4fccd. The panel-level bubbling key handler keeps six navigation keys on input/textarea targets without preventing their default behavior, while leaving accordion-trigger navigation intact; the regression explicitly checks both cases. No demonstrated source defect in this delta.
The updated PR reports 24/24 Chromium arrow-key checks and 9/9 source-job lock checks with a scripted bridge, plus applicable build/lint/types/tests. I am reconciling that receipt through the existing project lead/owner; it does not claim a real Tauri window or Firefox/WebKit coverage. The changed head needs fresh current CI/protection eligibility before merge; previous-head approval is not sufficient. #2568 remains partial and open. No release/deployment authorization.
0xPlayerOne
left a comment
There was a problem hiding this comment.
Personal review outcome: SOURCE APPROVED at a48c91f. The existing project lead/owner confirmed this exact published head and qualification receipt (#2600 (comment)): 24/24 Chromium arrow checks, 9/9 scripted source-job lock/unlock checks, 78 local test files, lint/format/types/build passed. I personally reviewed the full two-file delta from the prior approved scope: input/textarea navigation keys stop bubbling at the source panel without preventDefault, and accordion triggers retain their navigation behavior. No remaining demonstrated source finding in this PR's scope. This narrowly scoped workaround does not claim a general shared Accordion fix. Real Tauri, Firefox/WebKit and titlebar behavior remain untested as documented. Applicable hosted CI and all protections must pass before merging; this own-author review comment is not a substitute for any required independent approval. #2568 remains incomplete/open.
Summary
Moves the four per-source limits (document limit, content bytes, content characters, duration seconds) in
SourceSettingsWorkflow.tsxfrom native number inputs to the shared optionalNumberField(adea-ai/ui#444). A blank field is accepted and reported asnull, anulllimit shows blank, and only whole numbers inside the range reach the settings draft. The locked state still comes fromsourceLocked(). The payload shape is unchanged: all four fields staynumber | null, and the localoptionalNumberhelper is removed.Part of #2568. Four commits:
@adea-ai/ui0.140.0 to 0.142.0, withpackage.json,apps/web/package.json,bun.lockand the NOTICE heading. 0.142.0 is the lowest version published to npm that contains chore(main): release 0.29.7 #444 (0.141.x exists as GitHub releases but was never published to npm). Its@adea-ai/themesrange is^0.10.4, which the pinned 0.10.6 satisfies, so no second themes copy is added; 0.142.1 and later require^0.10.10. The twox-release-please-versionmarkers inbun.lockwere restored after the install.NumberFieldadoption and a focused test.docs/ingestion.md.a48c91f, added after qualification testing) Keep the arrow and paging keys with the source setting fields: the accordion root cancelled ArrowUp, ArrowDown, Home, End, PageUp and PageDown from any field inside an open panel, so the limit fields could not be stepped with the arrow keys. This is also true on main today. The panel now stops those keys when the target is an input or textarea, and the accordion keeps its own trigger navigation. Commit and test: see Validation.Source-card styling (#2586) stays deferred. This does not touch #2599 (the header change) and shares no files with it.
Validation
7d3d786), re-run ata48c91f:bun run lint,bun run format:check,bun run --cwd apps/web typecheckpass;bun run testpasses (78 JavaScript test files); the web build passes within budgets (initial JS 517,660 / 525,000, complete JS 896,510 / 940,000, CSS 244,015 / 285,000).bun scripts/shared-notice-contract.test.mjspassed at the dependency commit.nulland only accepted numbers are reported; fractional (1.5), over-range and incomplete numeric drafts are not reported and the saved value is restored on blur; the four limits lock while a source job runs and unlock when it finishes; navigation keys in the limit fields are not cancelled (navigation keys in the source limit fields stay with the field, fails without the repair). The behaviour tests fail against the old native inputs.v0.142.0source (sha256 of the file; lockfile integrity equals the registry integrity; the chore(main): release 0.29.7 #444 commit is in the tag).?demo):a48c91f: 24/24 checks (ArrowUp/ArrowDown step by 1 in all four fields, stop at min and max, an over-range draft is restored on Tab). The main baseline is 8/24.a48c91f: 9/9 checks with a scripted Tauri bridge so a real job starts, runs and finishes. Four fields lock while it runs, keep their values, take no focus or key, and unlock afterwards.cortana/pr2600-evidence/EVIDENCE.mdin the project files.Impact and rollout
Behavior difference: a typed value outside a field's range (for example 0 documents) was previously written into the draft as typed. Fractional drafts (for example
1.5) are now rejected as well as out-of-range ones: neither is reported, and the field is restored to the last accepted value on blur. Blank still savesnull. This is documented indocs/ingestion.md. The complete JS graph grows by about 2.7 kB against main. In the source panel, Home, End and the arrow and page keys now act on the focused field instead of being cancelled.Not covered: Firefox and WebKit, a real Tauri desktop window (the job lock was run through a scripted bridge), and the macOS titlebar inset (never tested).
Review notes
@adea-ai/themescopy. A later pin bump is a normal Renovate step.@adea-ai/uishould leave key events from inputs alone; this PR works around it in the one panel it affects, which does not change the shared package.Related issue
Part of #2568 (stays open).
Generated by Claude Code