Implementation review: PB-025, PR 637

Field	Value
Backlog item	PB-025 SQL object explorer (feature FEAT-FAB-11; owner's id SQL-005)
Pull request	#637 (WH-02 object explorer), merged as a821f378; #638 (WH-03 Script as), merged as a9f86a92
Package	None (no `docs/product/pr-ready/PB-025.md`)
Reviewer	Engineering quality review thread. It didn't write #637 or #638.
Date	2026-10-27
Environment	Linux container, main 6159e507, Python venv for the engine, Playwright Chromium. No tenant, no Windows, no second PC.

Outcome#

Accepted with Documented Limitations (recommended).

The explorer lists schemas, tables, views, stored procedures and functions with columns, and Script as gives SELECT TOP 100, CREATE, ALTER and DROP without running anything. Both PRs' browser tests fail on the code before them and pass after. The limitations: it hasn't been checked against a real warehouse catalog from here. Findings 1 and 2 (the Script as header said the page only reads, and the tree wasn't a keyboard tree) are fixed in #835 (quality PASS 9.1 at ac8faec5), which also takes ALTER and DROP off Fabric's own schemas (finding 3).

Recommended by: Engineering quality review. Decided by (product owner): pending.

Acceptance criteria#

SQL-005 has no written definition in the repo. grep -rn SQL-005 docs/ finds only PRODUCT_BACKLOG.md (PB-025 points to it, and line 349 says PB-030 "Absorbs ... the SQL sprint's SQL-004/005 screens"). The nearest specification is the WH-02 and WH-03 rows of docs/warehouse.md:234-235 ("Object explorer: schemas, tables, views, procedures, functions, columns for completion"; "Script as SELECT, CREATE, ALTER, DROP") and the Explorer section (docs/warehouse.md:84-94). The criteria below come from the item's problem ("a tree of schemas, tables, views and procedures") and those rows.

#	Criterion	Result	Evidence
1	A tree of schemas, tables, views, procedures and functions	Met (fake catalog)	`WH_OBJECTS_SQL` reads `sys.objects` joined to `sys.schemas` for U, V, P, FN, IF, TF with `is_ms_shipped = 0` (app/ui/warehouse.js:10-14); `whExplorer` draws schema > Tables / Views / Stored procedures / Functions > object (warehouse.js:183-230), IF and TF grouped as Functions (warehouse.js:8). Browser: "after the first confirmed run the explorer lists the catalog by schema and kind".
2	Columns with T-SQL types, used for completion	Met	`WH_COLUMNS_SQL` (warehouse.js:16-23), `whTypeLabel` gives `nvarchar(100)`, `decimal(18,2)`, `datetime2(6)` (warehouse.js:26-33); `warehouseSqlProvider` feeds the editor from the cached catalog (warehouse.js:149-158). Browser: "a table's columns show their T-SQL types, and a click inserts a quoted name".
3	Script as SELECT, CREATE, ALTER, DROP	Met	`whScriptItems` (warehouse.js:79-103): Select top 100 for tables and views; CREATE TABLE from the cached columns; CREATE/ALTER for views, procedures and functions from `sys.sql_modules` with the name escaped for `]` and `'` (warehouse.js:68-75); ALTER rewrites the leading CREATE or CREATE OR ALTER (warehouse.js:62-65); DROP by kind. Scripts open in a new tab and aren't run. Browser: the three Script as tests pass. A table's CREATE has no constraints; docs/warehouse.md:90 says so.
4	Catalog queries run read-only and say they use capacity	Met	`whCatalogQuery` uses `sql_start` with `target: "warehouse"` and no `allow`, so the engine's read-only check and rollback apply (warehouse.js:106-128; sidecar/fabric_desktop/warehouse.py:132-163). List objects has the title "Runs two catalog queries in Fabric (uses capacity)" (warehouse.js:189) and waits for the per-warehouse confirm (`warehouseRunOk`, app/ui/query.js:853); it loads by itself only after a confirmed run, as docs/warehouse.md:86 says. Browser: "the explorer waits for List objects until runs on the warehouse are confirmed".
5	Keyboard accessible tree	Met after #835 (was Partly)	Every row is a `<button>`, so Tab, Enter and Space work; schema and group rows carry `aria-expanded` (warehouse.js:197, :202); the ⋯ button has `aria-label="Script <name>"` and `aria-haspopup="menu"`, and the menu takes arrow keys (app/ui/app.js:310). But there is no `role="tree"`/`treeitem`, no arrow-key movement, every object is two or three Tab stops, and the column toggle has no `aria-expanded` (warehouse.js:214-215). No test drives the explorer by keyboard. Fixed in #835: `role="tree"` with treeitems, `aria-level` and `aria-expanded`, a roving tabindex (one Tab stop), arrow keys, Home and End, and a browser keyboard test.

Checks#

Check	Result	Evidence or note
Scope matches the item and package	Met	Explorer and Script as only; no engine change (both PRs touch app/ui, the browser test and docs).
New tests fail without the change, pass with it	Met	`warehouse-page.test.cjs`: #637's version on af8dee38 (before #637) 4 passed, 7 failed (the explorer tests); on a821f378, 11/11. #638's version on a821f378 (before #638) 11 passed, 3 failed (the three Script as tests); on a9f86a92, 14/14.
Full suites pass (pytest, UI, cargo)	Partly	On main 6159e507: browser warehouse-page 18/18, sql-editor 16/16, sql-exec 11/11, warehouse-project 8/8; `ui.test.cjs` 297/297. Engine: `test_warehouse_queries` 100 passed, `test_sql_exec` 98 passed (the catalog queries go through this path). Not the whole pytest suite. No Rust change; cargo not run.
Reachable where docs say; Preview behavior right	Met	Warehouse page left side (docs/warehouse.md:84-94); the whole page is Preview.
OneLake data safety	Met	No writes: catalog reads through the read-only warehouse target; Script as only opens text in a tab.
Authentication and distribution	Met	No new scope; same Azure SQL token as PB-024.
No unproven claim; Preview tags kept where untested	Met	Preview. Not tried against a real warehouse catalog from here.
Docs match the code; linked; strict docs build passes	Met after #835 (was Partly)	warehouse.md:84-94 matches. But warehouse.md:90 says scripts never run because "this page runs only queries that read", and every script starts "-- This page runs only queries that read, so it won't run this script." (warehouse.js:48-50). Since WH-10, the page runs changes after a preview and confirm (warehouse.md:10). Docs build not run here. #835 changed the header to "Opened, not run." and docs/warehouse.md to match.
Accessibility of new controls	Met after #835 (was Partly)	Criterion 5.
Inventory and backlog updated; `product_data.py` consistent	Partly	FEAT-FAB-11 describes "an object explorer with Script as". PB-025 still In review. `product_data.py` not run.

Findings#

#	Severity (blocking / should fix / note)	Finding	Where	Status
1	should fix	The Script as header and docs say the page runs only queries that read; since WH-10 it runs changes after a confirm. Reword to "Opened, not run" and drop the reason.	app/ui/warehouse.js:48-50; docs/warehouse.md:90	Fixed: #835
2	should fix	The tree isn't a keyboard tree: no `role="tree"`/`treeitem`, no arrow keys, two or three Tab stops per object, and the column toggle has no `aria-expanded`. Usable by Tab, but slow on a large warehouse. Add tree semantics with roving focus, or at least `aria-expanded` on the column toggle, and a keyboard test.	app/ui/warehouse.js:183-230	Fixed: #835
3	note	The explorer uses the same `is_ms_shipped = 0` filter that let Import pick up Fabric's own `queryinsights` and `_rsc` schemas on the tenant run (#809 fixed Import, not the explorer). So the explorer probably lists them too, and offers Script as DROP on them. Listing them may be wanted (query insights); DROP on them isn't. Not checked on a tenant here.	warehouse.js:10-14; compare app/ui/whproject.js:47-50	Partly fixed: #835 gives objects in `queryinsights`, `_rsc` and `sys` no ALTER or DROP. They are still listed (wanted for query insights); check on the next tenant run
4	note	The columns query reads every column of every table and view in one run, and `whCatalogQuery` pulls every page into memory. Fine for typical warehouses; on a very large one it's a bigger catalog read. Raised in the #637 review too (quality/pr-review.md:360).	warehouse.js:16-23, :106-128	Open: low
5	note	A table's Script as CREATE has column names, types and NULL/NOT NULL only, not constraints (Fabric's NOT ENFORCED primary, unique and foreign keys). Documented.	warehouse.js:55-58; docs/warehouse.md:90	Documented

Documented limitations (only for Accepted with Documented Limitations)#

Limitation	Where users are told	Backlog item that tracks it
Not verified against a real warehouse catalog from this review; the page is Preview.	docs/warehouse.md:225 (Preview until run on a tenant)	FEAT-FAB-11 verification (Partially Verified)
Table CREATE scripts carry no constraints.	docs/warehouse.md:90	None

Inventory and backlog changes#

Features changed: FEAT-FAB-11: no status change. The keyboard limitation (finding 2) was listed until #835 fixed it.
Backlog item state: PB-025 In review → Accepted, on the product owner's decision. Findings 1 and 2 are fixed in #835; PB-025's PRs are #637, #638 and #835.