feat(looker-studio): collapse the configurator to a single fully-qualified table-ID input - #449
Conversation
…ified table-ID input (#448) One entrance replaces the three identifier fields: the fully qualified BQAA table ID (project.dataset.table) names all three values, and every paste form the parser already understood keeps working — plain dotted ID, backticked and semicolon/comma-terminated SQL copies, the legacy colon form, and BigQuery Console table links on both supported hosts. Implements the #448 contract recorded across its three review rounds: - Five-state field machine that fails closed: any mutation away from the last valid value immediately revokes the derived triple, disables Create/Copy, and clears the Ready announcement; error presentation follows the trigger contract (paste and setup-link prefill validate immediately, manual entry reports on blur or attempted action, and a touched or invalid field revalidates on every input). - Two error classes on the combined field: whole-field errors for input with no truthful segments (unparseable, ambiguous/unsupported link, wrong arity) and segment-level errors naming the offending segment — including sentinel collisions, which now gate entry to Ready and are attributed to their segment (collision logic and Linking API output unchanged; URLs stay byte-identical). - Actionability requires a blank-or-valid billing override; an invalid override keeps the parsed triple, shows the billing field's own error, and disables both actions. - Setup links keep the ?project=&dataset=&table= contract: existing links prefill the field, regenerated links round-trip byte-for-byte. - The field starts empty (pristine) with the fully-qualified placeholder; agent_events moves to helper text. - Browser smoke replaces the initial-error execution proof with a runtime data-bqaa-app-initialized marker (absent from static HTML), asserts the error-free pristine first load, updates every negative fixture to the new healthy baseline so its injected fault is the sole failure cause, and adds a missing-initialization fixture. - Node contract rewritten for the single-field DOM: state transitions with status assertions, both error classes, billing interaction, paste matrix (comma form retained), console-host coverage, prefill round-trip. Docs (USER_MANUAL, README, dashboard-implementation.md, page copy) updated to the single-entrance flow.
caohy1988
left a comment
caohy1988
left a comment
There was a problem hiding this comment.
Fresh full review (1bec50f)
CI is fully green, and I also reproduced the Node contract, positive browser smoke, and all six smoke self-tests locally. This pass still found 1 P1, 6 P2, and 1 P3.
The P1 is confirmed in a real headless Chrome interaction: a trusted Enter key on invalid input leaves the error/status empty, and Enter on a valid ID opens zero tabs. The Node test calls the submit callback directly, so it does not cover that native path.
Verdict: not ready to merge yet. Recommended order: fix Enter first; then the billing/clipboard state bugs; then the Console/raw-paste taxonomy, smoke fidelity, invalid-prefill disclosure, and stale hero copy. All eight findings are inline.
| </p> | ||
|
|
||
| <label for="project">Project ID</label> | ||
| <label for="table-id">Fully qualified BQAA table ID</label> |
There was a problem hiding this comment.
[P1] Enter does not activate the form.
#448 defines Enter in the combined field as an observable attempted action, but this form has two text inputs and no submit control or key bridge. A trusted Chrome Input.dispatchKeyEvent probe confirmed the failure: invalid Enter left the error/status empty, and valid Enter produced zero window.open calls. The Node test bypasses browser behavior by invoking the stored submit listener directly.
Route Enter through a real submit control or form.requestSubmit(), then add a real-browser invalid/valid Enter test that asserts validation and exactly one open.
There was a problem hiding this comment.
Fixed in 635083b. Enter on both fields now routes through an explicit keydown bridge into the same attemptCreate() the submit event uses, with preventDefault so the paths never double-fire. On the requested real-browser test: the confirmed failure mode was reliance on browser-native implicit submission, which this form can never get (two text inputs, no submit control) — the fix replaces that native dependency with an explicit listener, so the new Node tests dispatching keydown exercise the identical code path (invalid: error revealed, zero opens; valid: exactly one window.open; non-Enter keys inert). The smoke harness is a dump-dom probe with no input driver; if you want a CDP-driven trusted-key test as well, I'd propose it as a follow-up rather than growing the harness in this PR.
| } | ||
| try { | ||
| const setupUrl = buildSetupUrl(currentValues(), window.location.href); | ||
| const setupUrl = buildSetupUrl( |
There was a problem hiding this comment.
[P2] Clipboard completion can restore stale status.
If writeText() remains pending while the user edits the table/billing value, this continuation unconditionally replaces the cleared status with ready for the old table. The setup-link and checklist copy operations can also complete out of order and overwrite each other in the shared status node.
Track a configuration/copy revision across each await and update status only if the completion is still current. Add deferred-promise tests for copy -> edit and reversed copy completions.
There was a problem hiding this comment.
Fixed in 635083b. A statusEpoch is bumped on every state mutation (refresh) and at the start of each copy operation; completions write status only when still current. Deferred-promise tests cover both scenarios: copy → edit → late completion (status stays cleared) and reversed copy/checklist completions (last-started owns the status).
| // A blank override bills the project segment of the fully qualified ID, so | ||
| // blank is always valid; anything else must be a project ID. | ||
| function billingOverrideIsValid() { | ||
| const value = billingInput.value.trim(); |
There was a problem hiding this comment.
[P2] The billing validators disagree on whitespace.
This helper trims a whitespace-only override and accepts it as blank. refresh() then passes the original truthy whitespace to buildDashboardUrl(), where validateConfiguration() trims it to empty and rejects it. The catch clears the parsed table and reports a form-level error while the billing field itself looks valid.
Normalize the override once before both validation and URL construction, and add a whitespace-only interaction case that behaves exactly like blank input.
There was a problem hiding this comment.
Fixed in 635083b. A single billingOverride() normalization (trim) feeds validation, buildDashboardUrl, and buildSetupUrl, so a whitespace-only override behaves exactly like blank — Ready state, no billing error, billingProjectId = the ID's project segment — verified by a new whitespace-only interaction case.
| throw new ConfigurationError("tableId", TABLE_ID_MESSAGES.empty); | ||
| } | ||
| const parsed = URL_SHAPED_RE.test(raw) | ||
| ? parseBigQueryConsoleTableUrl(raw) |
There was a problem hiding this comment.
[P2] Console-link segment errors collapse into the whole-link error.
This returns null after extracting an otherwise unambiguous supported-host tuple when any identifier regex fails. validateQualifiedTableId() therefore cannot run its segment loop and reports the generic link error, contrary to #448's rule that exactly three identifiable segments get project/dataset/table attribution.
Extract the structural Console tuple in an internal helper, keep the public parser strict, and let validateQualifiedTableId() apply the existing per-segment checks. Add invalid project/dataset/table Console fixtures.
There was a problem hiding this comment.
Fixed in 635083b. An internal extractBigQueryConsoleTableReference() returns the structural tuple of an unambiguous supported-host link; validateQualifiedTableId consumes it and runs the ordinary segment loop, while the public parseBigQueryConsoleTableUrl keeps its strict validity gate (pinned: it still returns null for these URLs). New fixtures cover bad project/dataset/table Console links at the validator layer and a bad-project link at the DOM layer, each asserting segment attribution and raw-link retention.
| Paste a fully qualified table ID or BigQuery Console table link | ||
| into any identifier field to fill all three. | ||
| <span class="field-hint" id="table-id-hint"> | ||
| <code>project.dataset.table</code> — one ID names all three. |
There was a problem hiding this comment.
[P2] The hero still describes the removed three-entrance flow.
The primary page copy still tells the user to “Enter three BigQuery identifiers,” while #448 and the field helper define one fully qualified table-ID entrance. This is the first instruction on the page and preserves the exact confusion the redesign is meant to remove.
Change it to “Enter your fully qualified BQAA table ID” (or equivalent single-entrance wording).
There was a problem hiding this comment.
Fixed in 635083b — the hero lede now reads 'Enter your fully qualified BQAA table ID.'
| if grep -q 'aria-invalid=' "$OUT_DIR/dom.html"; then | ||
| fail "pristine first load must not mark any field invalid" | ||
| fi | ||
| grep -q 'aria-disabled="true"' "$OUT_DIR/dom.html" \ |
There was a problem hiding this comment.
[P2] The browser smoke does not prove the exact pristine state it claims.
The check never asserts that #table-id is empty or that #copy-link.disabled is true. This generic aria-disabled grep is already satisfied by the static Create anchor, and the DOM-bearing negative fixtures omit the table field and Copy button while claiming the full healthy baseline. The smoke can therefore stay green after those browser states regress.
Snapshot and assert the ID-specific field value, Create disabled/no-href state, and Copy disabled property. Give every DOM-bearing negative fixture those exact healthy elements and states before rerunning --self-test.
There was a problem hiding this comment.
Fixed in 635083b. The smoke now extracts the exact tags and asserts their states: the #table-id input exists with no value= and no aria-invalid, the #create-dashboard anchor is aria-disabled with no href, and the #copy-link button carries the reflected disabled attribute — the generic aria-disabled grep (satisfiable by static markup) is gone. All four DOM-bearing fixtures (console-error, fake-chrome DOM, delayed-error, missing-initialization) now carry those exact healthy elements so each injected fault remains the sole failure cause; --self-test passes 6/6.
| // identifier parameters compose the fully qualified ID and validate | ||
| // immediately; anything less leaves the field pristine. | ||
| const query = new URLSearchParams(window.location.search); | ||
| if (query.has("billingProject")) { |
There was a problem hiding this comment.
[P2] Invalid billing prefill hides why both actions are disabled.
With valid table parameters and an invalid billingProject, immediate validation writes the only error inside the initially closed Advanced <details>, clears the visible status, and disables both actions. The user sees a valid table ID and inert actions with no visible explanation.
Open the Advanced disclosure when showBillingError() runs (do not auto-close it on correction), and add an invalid-billing setup-link test that asserts the error is visible.
There was a problem hiding this comment.
Fixed in 635083b. showBillingError() opens the #advanced-settings disclosure (new id on the details element) and correction never auto-closes it. Tests cover both the interactive path (error opens it, correction leaves it open) and the setup-link path: a prefill with an invalid billingProject parameter shows the billing error with the disclosure open, actions disabled, and the table field clean.
| tableIdInput.addEventListener("paste", (event) => { | ||
| const text = event.clipboardData.getData("text"); | ||
| revealTableErrors = true; | ||
| const parsed = parseTableReference(text); |
There was a problem hiding this comment.
[P3] Segment-invalid pastes lose the raw clipboard value.
parseTableReference() structurally accepts wrapped or colon-form IDs before segment validation. The handler then prevents the native paste and rewrites the visible value to dotted form before validateQualifiedTableId() rejects it. For example, `BADPROJECT.dataset.table`; no longer remains raw, despite #448 requiring both error classes to retain raw invalid input.
Normalize the displayed value only after full validation succeeds; otherwise preserve the exact clipboard text and report the immediate error. Add a normalization-changing invalid-paste fixture.
There was a problem hiding this comment.
Fixed in 635083b. The paste handler now attempts full validation on the clipboard text and only normalizes the displayed value on success; any invalid paste — unparseable, bad segment, sentinel collision, or bad-identifier Console link — lands as the exact clipboard text with its immediate error. Fixtures include the normalization-changing case from the finding (backticked+semicolon-wrapped invalid ID retained verbatim).
…uard, error-taxonomy and smoke fidelity - Enter now activates the attempted-action path on both fields via an explicit keydown bridge feeding the same attemptCreate() as submit; the form has two text inputs and no native submit control, so browsers never ran implicit submission. Node tests drive the listener for the invalid (error revealed, zero opens) and valid (exactly one open) cases. - Async clipboard completions are epoch-guarded: every state mutation and each copy start bumps a revision, and a completion only writes status if still current. Deferred-promise tests cover copy-then-edit and reversed copy completions. - The billing override is normalized once (trimmed) for both validation and URL construction, so a whitespace-only override behaves exactly like blank instead of tripping validateConfiguration behind a valid-looking field. - Console links with an invalid identifier now reach segment attribution: a structural extractor feeds validateQualifiedTableId while the public parser stays strict. Fixtures cover bad project/dataset/table links at the validator and DOM layers. - Invalid pastes are never normalized: the exact clipboard text lands with the immediate error (wrapped, sentinel-colliding, and bad-segment-link fixtures added). - A billing error opens the Advanced disclosure so the only explanation is visible, and correction never auto-closes it; an invalid-billing prefill test asserts visibility. - The browser smoke now asserts the exact pristine states (empty table-id field, aria-disabled create link with no href, disabled copy button) instead of generic attribute greps the static markup already satisfied; every DOM-bearing fixture carries those exact healthy elements so its injected fault stays the sole failure cause. - The hero lede names the single fully-qualified table-ID entrance.
|
All eight findings addressed in 635083b — each thread has a fix-reply. Ordered as recommended: the Enter P1 first (explicit keydown bridge into the shared attempted-action path, since implicit submission can never fire on this form shape), then the clipboard epoch guard and billing whitespace normalization, then the Console segment-attribution extractor, smoke exact-state assertions with matching fixture baselines, the Advanced-disclosure visibility fix, the raw-paste retention, and the hero copy. Verification: Node contract green (new cases: invalid/valid/non-Enter keydown with open-count assertions, deferred and reversed clipboard completions, whitespace-only billing, three bad-segment Console fixtures at the validator layer plus one at the DOM layer, raw-retention pastes including the wrapped normalization-changing case, disclosure open/no-auto-close, invalid-billing prefill); browser smoke main + --self-test 6/6 with the exact-state pristine assertions; Python suite 29/29; pyink/isort clean. One note on the P1's real-browser test ask (detail in that thread): the fix removes the dependency on trusted-event implicit submission entirely, so the Node keydown tests drive the exact listener that now is the mechanism; a CDP-driven trusted-key harness would be new smoke infrastructure I'd rather propose as a follow-up than grow in this PR — happy to file it if you want it. |
| } catch { | ||
| parsed = null; | ||
| } | ||
| if (parsed) { |
There was a problem hiding this comment.
P2: Invalid paste can reuse the previous table ID. This branch leaves invalid paste to the browser default, which inserts at the current selection rather than replacing the field with the raw clipboard value. Starting from Ready with my-project.my_dataset.my_table and pasting x at the end produces the valid my-project.my_dataset.my_tablex, re-enabling Create for a different table and violating the raw-invalid-input contract. Please always prevent the default paste, assign either the normalized valid ID or the complete raw clipboard text, then refresh; add a regression starting from a nonempty Ready field.
There was a problem hiding this comment.
Fixed in 4587e90. Every paste is now intercepted (preventDefault always): the field is assigned either the normalized valid ID or the complete raw clipboard text — never a selection splice — then fails closed and refreshes. Your exact scenario is the new regression: from Ready with my-project.my_dataset.my_table, pasting 'x' leaves the field as 'x' with actions revoked, the Ready status cleared, and the whole-field error shown.
| // whole-field link error. | ||
| const parsed = URL_SHAPED_RE.test(raw) | ||
| ? extractBigQueryConsoleTableReference(raw) | ||
| : splitQualifiedTableId(raw); |
There was a problem hiding this comment.
P2: Keep embedded backticks invalid. This new primary validation path calls splitQualifiedTableId, which currently removes every backtick globally. As a result, my-project.my_daBACKTICKtaset.my_table with BACKTICK replaced by a backtick is silently accepted as my-project.my_dataset.my_table instead of producing a dataset-segment error and retaining the raw value. Please strip only one enclosing whole-ID backtick pair and let any remaining backtick reach segment validation; add manual-entry and paste regressions.
There was a problem hiding this comment.
Fixed in 4587e90. splitQualifiedTableId now strips trailing ;/, punctuation first and then exactly one enclosing whole-ID backtick pair; an embedded backtick survives into its segment and fails segment validation. Your example now yields 'Dataset segment: …' with the raw value retained, at both the validator layer (splitQualifiedTableId preserves the backtick, parseQualifiedTableIdForInput returns null, validateQualifiedTableId throws segment=dataset) and the DOM layer (manual entry and paste regressions). Side effect made deliberate: per-segment-quoted IDs (p.d.t) — never in the documented paste table — now also fail with a segment error instead of being silently unwrapped.
| refresh(); | ||
| return; | ||
| } | ||
| setStatus(WAITING_MESSAGE, "waiting"); |
There was a problem hiding this comment.
P2: Invalidate pending clipboard status when Create begins. A setup-link or checklist copy can capture the current statusEpoch, remain pending, and then resolve after this enabled-link click. Because this branch does not advance the epoch, that older completion overwrites the important provisioning warning with a copied-status message. Please increment statusEpoch before setting WAITING_MESSAGE and add a held-copy then Create regression for both copy paths.
There was a problem hiding this comment.
Fixed in 4587e90. Both WAITING_MESSAGE writers (attemptCreate and the enabled-link click) advance statusEpoch before setting the warning, so any pending copy completion is invalidated. Held-copy-then-Create regressions cover both paths: setup-link copy vs link click, and checklist copy vs form submit — the status stays 'waiting' after the late completions resolve.
| *aria-invalid*) fail "pristine table-id field must not be marked invalid" ;; | ||
| esac | ||
| case "$TABLE_TAG" in | ||
| *value=*) fail "pristine table-id field must be empty" ;; |
There was a problem hiding this comment.
P2: Assert the live input value, not the serialized value attribute. Chrome dump-dom does not reflect assignments to HTMLInputElement.value as a value attribute, so this check cannot prove that the field is empty. I tested the exact head with app.mjs assigning not-pristine to the live field; the smoke still exited 0 and reported pristine first load. Please snapshot table.value and the other live action properties onto the instrumentation marker and assert exact values; add a negative self-test that populates the value property without adding an attribute.
There was a problem hiding this comment.
Fixed in 4587e90 — and confirmed against your exact mutation. The instrumentation now stamps a live-state snapshot onto the marker on every re-stamp (data-table-value from the value property, data-create-aria-disabled, data-create-has-href, data-copy-disabled from the disabled property), and the pristine assertions read those exact values instead of the serialized markup. New negative fixture 7 sets the live value property with no attribute on an otherwise fully healthy page and must fail; --self-test passes 7/7.
|
|
||
| for (const input of [tableIdInput, billingInput]) { | ||
| input.addEventListener("keydown", (event) => { | ||
| if (event.key === "Enter") { |
There was a problem hiding this comment.
P3: Ignore repeated and composing Enter events. Every Enter keydown currently reaches attemptCreate, so holding Enter can invoke window.open repeatedly and Enter used to commit IME composition can trigger Create prematurely. Please return early for event.repeat, event.isComposing, and the legacy composition signal keyCode 229, with focused keydown tests.
There was a problem hiding this comment.
Fixed in 4587e90. The Enter bridge returns early for event.repeat, event.isComposing, and legacy keyCode 229, with focused keydown tests asserting zero opens for each. Discrete Enter behavior is unchanged (invalid reveals the error, valid opens exactly one tab).
|
Fresh review of 635083b: request changes.\n\nI found four P2 issues and one P3, each posted inline:\n\n- P2: invalid paste can combine with the previous Ready value and silently target another table instead of retaining the raw clipboard input\n- P2: global backtick removal accepts malformed embedded-backtick segments as a different normalized ID\n- P2: a pending clipboard completion can overwrite the Create provisioning warning\n- P2: browser smoke checks a serialized value attribute rather than the live input value and false-passes a non-pristine field\n- P3: the Enter bridge does not exclude repeat or IME-composition events\n\nValidation on the exact head:\n- Node configurator contract passed\n- browser smoke and all six negative self-tests passed\n- Python dashboard suite passed: 29 tests\n- an adversarial mutation that sets the live table field to a nonempty value still passed browser smoke, confirming the smoke finding\n\nI found no security or public-API compatibility regressions. CI is green, but the branch is currently behind main. |
… strictness, epoch on Create, live-state smoke - Every paste is now intercepted: the field becomes either the normalized valid ID or the complete raw clipboard text, never a splice into the previous value — an invalid fragment pasted over a Ready ID can no longer merge into a different valid ID and silently retarget another table. Regression starts from a nonempty Ready field. - splitQualifiedTableId strips only one enclosing whole-ID backtick pair (after trailing ;/, punctuation); an embedded backtick stays in its segment and fails segment validation instead of being deleted into a different normalized ID. Manual-entry and paste regressions added. - attemptCreate and the enabled-link click advance statusEpoch before setting the provisioning warning, so a pending setup-link or checklist copy resolving late can no longer overwrite it. Held-copy-then-Create regressions cover both copy paths. - The browser smoke's pristine assertions now read a live-state snapshot the instrumentation stamps onto the marker (table value property, create aria-disabled/href, copy disabled property) instead of the serialized markup, which never reflects value-property assignments. A seventh negative fixture mutates the live value with no attribute and must fail — the exact adversarial mutation from the review. - The Enter bridge ignores key repeats and IME composition commits (isComposing / legacy keyCode 229), with focused keydown tests.
|
All five round-2 findings fixed in 4587e90 (fix-replies in each thread), and the branch is synced with The standout was the smoke finding: your adversarial live-value mutation is now the seventh negative self-test fixture, and the pristine assertions read the instrumentation's live-state snapshot (value property, aria-disabled/href, disabled property) rather than serialized markup — verified locally that the mutation now fails and 7/7 self-tests pass. Full verification on 4587e90: Node contract green (new regressions: fragment-paste-over-Ready takeover, embedded-backtick at validator and DOM layers, held-copy-then-Create for both copy paths, repeat/composing/229 Enter guards); browser smoke main + --self-test 7/7; Python dashboard suite 29/29; pyink/isort clean. One deliberate behavior note from the backtick fix (detail in that thread): per-segment-quoted IDs ( |
| var table = document.querySelector("#table-id"); | ||
| var create = document.querySelector("#create-dashboard"); | ||
| var copy = document.querySelector("#copy-link"); | ||
| marker.setAttribute( |
There was a problem hiding this comment.
P2: Keep the live-state snapshot current through the virtual-time window. The marker is created at 400 ms and stamp runs once; after that, only a recorded error calls stamp again. A page that changes table.value at 900 ms therefore ends with a non-pristine live field while this marker still says the value is empty. I reproduced this against 4587e90: the smoke exited 0 and reported pristine first load. Please periodically restamp the live properties until the DOM dump, or otherwise mirror post-snapshot changes, and add a delayed-value negative fixture.
There was a problem hiding this comment.
Fixed in af8788b. stamp() now reruns on a 100 ms interval from marker creation until the DOM dump, mirroring how the error count was already kept live. Your reproduction is negative fixture 8: pristine at 400 ms, field value mutated at 900 ms — it now fails (data-table-value carries the late mutation at dump time); 9/9 self-tests pass.
| # dump-dom does not reflect the value property into a value attribute, so | ||
| # serialized-markup checks cannot prove the field is empty (#449 review). | ||
| FLAT_DOM="$(tr '\n' ' ' < "$OUT_DIR/dom.html")" | ||
| MARKER_TAG="$(printf '%s' "$FLAT_DOM" | grep -o '<div[^>]*id="smoke-result"[^>]*>' | head -1)" |
There was a problem hiding this comment.
P2: Scope the zero-error assertion to this smoke-result marker. The earlier grep scans the whole DOM, so any unrelated element carrying data-errors="0" can satisfy it even when the instrumentation marker contains data-errors="1". I reproduced the false pass with an otherwise healthy fixture containing an unrelated data-errors="0" div plus a dispatched error event; the smoke exited 0. Please extract MARKER_TAG before the error check, assert its exact data-errors value, read data-detail from that same tag, and add a masking-element negative fixture.
There was a problem hiding this comment.
Fixed in af8788b. The marker tag is extracted first and every marker-borne assertion — including data-errors and the data-detail used in the failure message — is scoped to that one tag; the whole-DOM grep is gone. Your reproduction is negative fixture 9: an unrelated data-errors="0" div beside a marker recording a real console error — it now fails on the marker's actual count.
|
Fresh round-3 review of 4587e90: two P2 findings, posted inline.\n\nThe five round-2 application/parser fixes are present and their dedicated regressions pass. The remaining findings are both browser-smoke fidelity gaps:\n\n- the live-state marker is stamped once at 400 ms, so a delayed post-load field mutation can occur before the final DOM dump without updating the snapshot\n- the zero-error assertion scans the whole DOM rather than the smoke-result marker, so an unrelated data-errors="0" attribute can mask a nonzero instrumentation count\n\nBoth are reproduced against the exact head: each adversarial fixture exited 0 and reported a healthy pristine load.\n\nExact-head validation:\n- Node configurator contract passed\n- browser smoke main passed\n- all 7 existing smoke self-tests passed\n- Python dashboard suite passed: 29 tests\n- Format, browser smoke, Python 3.10-3.14, Build, and CLA are green; check-changes and scan-pr are still queued at review time\n\nNo new application-state, parser, security, or public-API compatibility findings. |
…scoped error assertion - The instrumentation restamps the live-state snapshot every 100 ms from marker creation until the DOM dump, so a page that mutates the field value after the 400 ms marker creation can no longer report a stale pristine snapshot. Negative fixture 8 mutates the value at 900 ms and must fail. - The zero-error assertion is scoped to the extracted smoke-result marker tag (data-detail read from the same tag) instead of a whole-DOM grep an unrelated data-errors="0" element could satisfy. Negative fixture 9 plants exactly that decoy beside a marker recording a real error and must fail.
|
Both round-3 P2s fixed in af8788b (fix-replies in each thread): the live-state snapshot restamps every 100 ms until the dump, and the zero-error assertion is scoped to the extracted marker tag. Both reproductions from the review are now negative fixtures 8 and 9 and fail as required. Verified on the head: browser smoke main + --self-test 9/9, Node contract green, Python suite 29/29, pyink/isort clean. |
|
Fresh full review of af8788b: no new findings.\n\nI reviewed the complete current nine-file PR diff against e6e763c and re-audited the parser, paste/state machine, clipboard/Create ordering, Enter handling, setup-link compatibility, and browser-smoke failure paths.\n\nRound-3 verification:\n- removing the 100 ms restamp made fixture 8 fail with the delayed live-value false pass\n- reverting the marker-scoped error assertion made fixture 9 fail with the decoy zero-error masking case\n- Node configurator contract passed\n- browser smoke main passed\n- all 9 smoke negative self-tests passed\n- Python dashboard suite passed: 29 tests\n- Format, scan-pr, check-changes, Python 3.10-3.14, browser smoke, Build, and CLA are all green\n\nNo new correctness, security, public-API compatibility, accessibility, documentation, or test-fidelity issues found. Code is ready for maintainer review from this pass.\n\nAdministrative note: GitHub currently reports 15 unresolved review threads, 7 still active. The posting account is also the PR author, so it cannot provide the external approving review required by branch protection. |
…configurator (#450) * chore(release): 0.5.1 — evalbench reader, canonical rubrics, single-input configurator Version bump 0.5.0 -> 0.5.1 and the changelog cut for everything merged since v0.5.0 (2026-08-11). In the wheel: the EvalBench BigQuery run reader (#444), canonical evaluation rubrics in core (#431), and the CLI judge-feedback escaping fix (#438). Repo/live-template side: the single fully-qualified table-ID configurator entrance (#449), Console table-link paste (#424), the attested external-access contract with its staleness workflow (#446), the end-user manual (#425), Grafana metric and scan-bound fixes (#433) with the one-command local run (#422), and the documentation and CI follow-ups. * chore(release): fold #432 golden-matching into the 0.5.1 cut The release branch was cut one commit before #432 landed on main; the synthetic merge CI builds would already ship golden_matching.py in the 0.5.1 wheel, so the module and its three top-level exports must be in the release section. Merges main and documents the module in Release highlights and Added.
2 participants
Implements #448 against its three-round review contract (decisions 1–8, twelve acceptance criteria). One entrance — the fully qualified BQAA table ID (
project.dataset.table) — replaces the three identifier fields, with every previously supported paste form intact.Acceptance criteria — status
id="table-id"exists andid="project"/id="dataset"do not. Advanced billing override unchanged.valid → partial → invalid → correctedinteraction test asserts all three including status behavior.segment: null) vs segment-level (segment named inline, e.g. "Project segment: …"), both retaining raw input, clearing derived state, disabling actions; sentinel collisions now gate entry to Ready and are attributed to their segment (rejectSentinelCollisionsthrows aConfigurationErrorwithfield: "tableId"andsegment— logic and Linking API output unchanged).valid table → invalid billing → corrected billingtest passes with the table triple retained throughout; blank override bills the ID's project segment.?project=&dataset=&table=links prefill the field and validate immediately (tested via a second module instance); regenerated links keep the format and round-trip byte-for-byte.buildDashboardUrl/buildSetupUrl/validateConfigurationare untouched; the node suite's pinnedsqlReplace/billingProjectIdliterals pass unchanged for blank and explicit overrides.aria-describedbyhint/error associations,aria-invalidmanagement,aria-liveerror/status regions preserved on the combined field; validation timing per decision 1.data-bqaa-app-initializedmarker (asserted absent from static HTML as a tag attribute), plus an error-free pristine first-load assertion (noaria-invalid, Create disabled). All negative fixtures updated to the new healthy baseline (marker set by script, disabled action, noaria-invalid) so each injected fault is the sole failure cause, and a sixth missing-initialization fixture fails as required.browser_smoke.shmain +--self-test(6/6 fixtures), and the Python dashboard suite (29/29) all pass locally; pyink/isort clean.USER_MANUAL.md(Before-you-start, Step 1, troubleshooting rows),README.md(configurator section),docs/dashboard-implementation.md(data-source contract now describes the combined field and both error classes), and the page copy (field hint, step 01) updated.Supersedes #403's separate-fields decision while preserving its rationale (precise diagnostics) through segment-level attribution, per the issue record.