Repository navigation
chore(config-ui): reskin phase 0, regression harness and quality tooling - #86
Conversation
c4058e6 to
9bc5980
Compare
ubaidkhalidarbisoft
left a comment
ubaidkhalidarbisoft
left a comment
There was a problem hiding this comment.
Reviewed by reading the code (I didn't run anything). Nothing blocks the merge. The main gap is that the census and the quality baseline aren't run by CI, so they only work when someone runs them by hand. Details are inline.
| run: cd config-ui; yarn install --immutable | ||
| - name: Run Lint | ||
| run: cd config-ui; yarn tsc && yarn lint | ||
| run: cd config-ui; yarn lint:check |
There was a problem hiding this comment.
CI doesn't run yarn e2e:census or yarn quality:baseline, and neither does the pre-commit hook (it only runs lint-staged). The PR says the census fails on any drop and the baseline stops totals growing, but nothing enforces either one on a PR. Both can run here as they are: the census is a static read of the spec files and needs no backend, and the baseline needs only the installed packages. Suggest adding:
- name: Quality gates
run: cd config-ui; yarn e2e:census && yarn quality:baseline(e2e:census prints "Playwright report: none" when there's no JSON report, but its comparison only reads the spec files.)
| "connectionId": "<masked>", | ||
| "createdAt": "<masked>", | ||
| "createdDate": "<masked>", | ||
| "description": "My first repository on GitHub!", |
There was a problem hiding this comment.
This golden stores the real GitHub repo details, including description, fullName and the clone URLs, so it depends on E2E_GITHUB_REPO being octocat/Hello-World and on GitHub never changing that repo. A different repo, or a changed description, breaks the golden even though the UI didn't change. Could the repo-derived fields be masked or normalised, so the golden only pins the shape of what the UI sends?
| "labels": [], | ||
| "mode": "NORMAL", | ||
| "name": "e2e-*-Blueprint", | ||
| "plan": [ |
There was a problem hiding this comment.
The UI sends back the whole blueprint here, so this golden (and the sync-policy one) stores the server-generated plan. It also stores cronConfig: "15 3 * * *", which an earlier test in the same serial describe sets. A backend change to plan generation, or reordering those tests, would fail a check that is meant to catch UI changes. Could these goldens ignore plan (or keep only the fields the UI builds itself, like connections and scopes)?
| export const GOLDEN_MASK = '<masked>'; | ||
|
|
||
| // Body and query keys (case-insensitive) whose values change per run or are secrets. | ||
| export const GOLDEN_MASKED_KEYS: readonly string[] = [ |
There was a problem hiding this comment.
Masking only covers the field names on this list. secretKey, refreshToken, clientSecret and newPassword, and the custom-header values, are written to the goldens as-is. Today they're empty or obvious fake values, so nothing has leaked, but a future test that fills in a real secret field would commit it. Matching by pattern as well (for example /secret|token|password|apikey/i) would be safer.
| return isBackendValue(node, ctx); | ||
| } | ||
|
|
||
| function taintedVariables(sf, backendNames) { |
There was a problem hiding this comment.
Variables are tracked by name across the whole file, not per test. If one test does const user = await api…, then user counts as backend data in every other test too. So a later phase could switch a backend check to a UI read in one test, and the census wouldn't notice if another test reuses the name. Could this track scope (for example with ts.Symbols from a type checker), or at least note the limit in the README?
| ); | ||
| } | ||
| const expected = JSON.parse(fs.readFileSync(file, 'utf-8')) as Golden; | ||
| expect(actual, `UI writes differ from golden ${path.relative(GOLDENS_DIR, file)}`).toEqual(expected); |
There was a problem hiding this comment.
toEqual compares the recorded writes as an ordered list, collected across every browser context. It was stable over 4 runs, but if the UI ever sends writes in parallel, the order can change between runs and the golden becomes flaky. It might be worth a comment here, or sorting writes that are known to be independent.
|
|
||
| // Reskin-owned directories (plan 3.10); every phase removes the files it cleans from RESKIN_LINT_PENDING. | ||
| const RESKIN_OWNED = ['src/ui/**', 'src/theme/**', 'src/config/**', 'src/routes/**', 'src/app/**']; | ||
| const COLOR_LITERAL = String.raw`#[0-9a-f]{3,8}\b|rgba?\(|hsla?\(`; |
There was a problem hiding this comment.
Minor: #[0-9a-f]{3,8}\b also matches strings that aren't colours, such as '#add' and '#feed' (anchors). It also misses named colours ('white', 'red', 'transparent'), so the "no colour literals" rule has a gap.
| stdio: ['ignore', 'pipe', 'ignore'], | ||
| }); | ||
| } catch (error) { | ||
| return error.stdout ?? ''; |
There was a problem hiding this comment.
stderr is discarded, and a failed tool returns ''. So if ESLint itself crashes (for example on a config error), all you see is Unexpected end of JSON input from JSON.parse. Suggest piping stderr through, or throwing with error.stderr when stdout is empty.
| const baseline = JSON.parse(fs.readFileSync(BASELINE, 'utf8')); | ||
| const problems = []; | ||
|
|
||
| for (const [rule, now] of Object.entries(current.eslint)) { |
There was a problem hiding this comment.
This only fails when counts go up. When they go down, nothing updates the baseline, so a fixed knip finding or rule error stays allowed and could come back. The docs say to run --write after each phase. Printing "counts dropped, run --write" here would make that hard to forget.
a2f03a3
into
arbisoft:feat/config-ui-reskin
React Review: PR #86 — chore(config-ui): reskin phase 0, regression harness and quality toolingRange: Coverage177 production files in scope (164 modified, 3 renamed, 10 deleted). Tests, e2e, goldens, lockfiles and tooling configs were not reviewed as code. I read the ESLint/knip configs only to learn which rules are enforced.
Verification done: for every export the PR removed or un-exported ( Summary: In
🟡 Minor1.
|
Review: approve with nitsReviewed 1. Medium: CI doesn't run the census or the quality baseline.
2. Low: some secret keys aren't masked in goldens.
3. Low: the census over-counts backend assertions.
4. Low: removing
Nits
🤖 Generated with Claude Code |
Summary
Phase 0 of the config-ui reskin (the reskin implementation plan, section 7). It makes the code-quality rules enforceable, and turns the e2e suite into a regression harness that every later phase is measured against.
Tooling and CI
package.jsonscripts:lint:checkrunstsc, thentsc -p e2e-tests/tsconfig.json, theneslint ., with no--fix.lint:fix.e2e:census.quality:baseline.src/{ui,theme,config,routes,app}), all aterror:no-explicit-any;exhaustive-deps;styleprop;src/theme, including in styled-components template literals.eslint.reskin-pending.mjslists the 43 files that fail these rules today. Each phase removes the files it cleans.config-ui.yml:main-arbisoftandfeat/config-ui-reskintoo;yarn lint:checkinstead oftsc && eslint --fix;yarn install --immutable;src/ui,src/ui/hooksandsrc/routes/ui-kitbarrels. Phase 1 fills them.E2E regression harness
Page objects. Every selector, URL, click sequence and wait moved out of the 14 specs into
e2e-tests/support/pages/*, andsupport/selectors.tsis gone.locator,getBy*,goto,click,fillorwaitForURL.expects stay in the specs.Backend outcome assertions. Every UI write flow now checks the persisted result through the API, or the DB where no API exposes it. The census counts these as backend assertions.
enableafter the sync policy is savedWrite goldens.
support/write-recorder.tsis an auto fixture. It records every POST/PUT/PATCH/DELETE the UI sends, from every browser context the test opens.:id,e2e-names becomee2e-*, the keys inGOLDEN_MASKED_KEYSare masked (the field is kept, its value isn't), and object keys are sorted.x-e2e-test-action), andGOLDEN_IGNORED_WRITES: connection tests (D3) and login/logout (R&D 6.1, 6.2).Assertion census (
e2e-tests/tools/census.mjs). It counts tests, expects, backend assertions, skips and fixmes per spec, and fails on any drop. The baseline iscensus.baseline.json.Type-checked e2e.
e2e-tests/tsconfig.json, and noanyleft in e2e code.e2e-tests/README.mddocuments the page objects, goldens, census, the four test-change classes, and the new variablesE2E_UPDATE_GOLDENSandPLAYWRIGHT_JSON_OUTPUT_NAME.Quality tooling (#85)
#85's guide targets the pre-sync stack (ESLint 8,
.eslintrc.js), so this ports it to ESLint 10 flat config:eslint.quality.config.mjs, solint:checkand the pre-commit hook are unchanged.eslint-plugin-importdoesn't support ESLint 10, soeslint-plugin-import-xreplaces it;react.versionis pinned, because'detect'crasheseslint-plugin-reacton ESLint 10;tsconfig.eslint.jsonresolves the@/alias.lint:quality,find:dead-code(knip),find:duplicates(jscpd), andquality:baseline, which fails if any per-rule count, knip finding or jscpd total grows pastquality.baseline.json.LINT_TOOLING_SETUP.mdkeeps its text and gains an "After the upstream sync (chore(config-ui): sync with upstream apache/devlake #83)" section explaining each workaround.AGENTS.mdgains a short "Config UI Quality Checks" section.First run, and what this PR fixes. Every finding was triaged before anything changed: genuine, false positive, or style-only.
prefer-destructuring, and the React Compiler hook rules are now warnings. This app doesn't use the React Compiler.styled.ts, an unused q-dev component, the bypassed slack barrel, and the oldrelease/v0.18–v1.0doc-link maps;@ahooksjs/use-url-state,cron-validate,react-is,@testing-library/jest-dom, and the outdated@types/react-router-domand@types/styled-components.Deferred, and how it'll be handled. The remaining findings are recorded per rule in
quality.baseline.json:no-explicit-any324, almost all in upstream plugin code;react/no-unescaped-entities21;no-duplicate-imports16;no-unused-vars14;connection-formthe worst;exhaustive-deps77;max-lines20;transformation.tsxboilerplate.They aren't fixed in bulk. Each reskin phase must leave every file it substantively edits with 0 quality errors, and
quality:baselinestops the totals growing. The baseline is lowered after each phase, and the final reskin PR proposes a follow-up for whatever remains (reskin plan, section 5.6).Regression proof
The census before/after above is static.
E2E runs:
config-uisupport/pages/**;Test plan
lint:check: 0 errors. The 20 warnings were already there.yarn test: 16 files, 91 tests.yarn build.yarn install --immutable.e2e:census: OK.quality:baseline: OK.Notes for reviewers
e524535f0: lint gates, CI and the empty barrels.9530b1043: page objects, backend assertions, the census and the e2e typecheck.bbaf705cd: write goldens, the census baseline and the README.592ca7c1c: the docs(config-ui): add lint tooling setup guide #85 tooling, with rule tuning.089aae79d: the import sort and the dead-code removal. This one is mechanical (208 files) and can be skimmed. There's no behaviour change, and the build, unit tests and full e2e passed afterwards.9bc598045: the quality baseline and the docs.E2E_GITHUB_REPO=octocat/Hello-World.config-ui.ymlnow runs on this PR, againstfeat/config-ui-reskin.