feat: add ESLint, Prettier, and Stylelint for web apps#625
Conversation
ReviewFindingsHigh
Low
Labels: PR adds linting and formatting tooling infrastructure for web apps — maintenance/chore work. Previous runReviewFindingsHigh
Low
Labels: PR adds linting and formatting tooling infrastructure — maintenance/housekeeping, not a user-facing feature. Previous run (2)ReviewFindingsHigh
Medium
Low
Labels: PR adds linting/formatting tooling for web frontend apps Previous run (3)ReviewFindingsHigh
Medium
Low
Info
Previous run (4)ReviewFindingsHigh
Medium
Low
Info
Previous run (5)Review: #625Head SHA: 9b15e3b SummaryThis PR introduces ESLint 9, Prettier, and Stylelint tooling for the admin SPA with a well-structured configuration. The vast majority of changes are automated Prettier reformats (whitespace, line-length adjustments) with no behavioral impact. A small number of substantive changes — removal of the unused FindingsInfo
FooterOutcome: approve Previous run (6)Review: #625Head SHA: 5661747 SummaryThis PR adds ESLint 9 (flat config), Prettier, and Stylelint tooling for the admin SPA, along with Husky + lint-staged for pre-commit hooks. The vast majority of the diff is Prettier auto-formatting of existing code (line-wrapping changes, blank lines between CSS rules). The few behavioral changes — adding FindingsMedium
Low
Info
FooterOutcome: approve Previous run (7)Review: #625Head SHA: 613f465 SummaryThe linting and formatting tooling is well-structured and the Prettier/ESLint/Stylelint configuration is correct. However, introducing Husky v9 into a repo that already uses the Python FindingsHigh
Medium
Info
FooterOutcome: request-changes Previous run (8)Review: #625Head SHA: 1b9d8c4 SummaryThis PR adds ESLint 9, Prettier, and Stylelint tooling for the admin SPA with sensible defaults — including XSS prevention ( FindingsMedium(none) Low
Info
FooterOutcome: approve Previous run (9)Review: #625Head SHA: 616fcb2 SummaryThis PR cleanly adds ESLint 9, Prettier, and Stylelint infrastructure to the admin SPA with well-chosen rules and a sensible phased rollout plan. All source file changes are purely Prettier reformatting with no logic modifications — verified by diff inspection. The one substantive CSS change ( FindingsInfo
FooterOutcome: approve Previous run (10)Review: #625Head SHA: 9d2fbb0 SummaryThis PR adds ESLint 9 (flat config), Prettier, and Stylelint to the admin SPA, along with Prettier-reformatted source files. The changes are well-structured: linting config is sensible (XSS prevention via FindingsMedium / Low / Info
FooterOutcome: comment-only |
waynesun09
force-pushed
the
add-frontend-linting
branch
from
613f465 to
5661747
Compare
|
This pull request has been automatically marked as stale because it has not had any activity in the last month. It will be closed in 2 weeks if no further activity occurs. Remove the |
waynesun09
force-pushed
the
add-frontend-linting
branch
from
9b15e3b to
4d4709e
Compare
waynesun09
changed the title
Site previewPreview: https://e770992a-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ✅ Success · Started 4:05 PM UTC · Completed 4:20 PM UTC |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| import ts from "typescript-eslint"; | ||
| import svelte from "eslint-plugin-svelte"; | ||
| import globals from "globals"; | ||
| import svelteConfig from "./web/admin/svelte.config.js"; |
There was a problem hiding this comment.
[medium] config-reference-inconsistency
eslint.config.js imports only ./web/admin/svelte.config.js but the lint script targets both web/admin/src/ and web/docs/src/. The svelteConfig is applied to all **/*.svelte files, meaning web/docs Svelte files are parsed using admins config. If the configs ever diverge, linting results for web/docs may be incorrect.
Suggested fix: Either document why admins config can be shared, or use separate ESLint config blocks for web/admin/src and web/docs/src with their respective svelte.config.js files.
| const legacy = legacyPathnameDocRest(); | ||
| if (legacy !== null) { | ||
| const u = new URL(window.location.href); | ||
| const u = new SvelteURL(window.location.href); |
There was a problem hiding this comment.
[low] unnecessary reactivity
SvelteURL (a reactive URL wrapper) is used in syncRouteFromLocation, a purely imperative function. Standard new URL() would be functionally identical and avoids importing the reactive wrapper unnecessarily.
| @@ -96,6 +87,7 @@ export async function obtainTurnstileToken( | |||
| return await obtainTurnstileTokenWithSignal(siteKey, merged); | |||
There was a problem hiding this comment.
[info] nonexistent eslint rule
The eslint-disable-next-line preserve-caught-error comment references a rule that does not exist in the ESLint configuration. ESLint will report an unused disable directive warning.
waynesun09
force-pushed
the
add-frontend-linting
branch
from
4d4709e to
de950a3
Compare
waynesun09
force-pushed
the
add-frontend-linting
branch
from
de950a3 to
a3f68d6
Compare
|
🤖 Finished Review · ✅ Success · Started 4:34 PM UTC · Completed 4:49 PM UTC |
| @@ -96,6 +87,7 @@ export async function obtainTurnstileToken( | |||
| return await obtainTurnstileTokenWithSignal(siteKey, merged); | |||
| } catch (e) { | |||
| if (deadline.signal.aborted && !userSignal?.aborted) { | |||
There was a problem hiding this comment.
[low] dead-code
The ESLint disable comment // eslint-disable-next-line preserve-caught-error references a rule name that does not exist in the ESLint configuration (not from @eslint/js, typescript-eslint, or eslint-plugin-svelte). The comment is a no-op — ESLint silently ignores disable comments for unknown rules.
Suggested fix: Remove the dead comment or replace with the correct rule name. Consider adding { cause: e } to the thrown Error directly, matching the pattern used in orgConfigParse.ts and githubClient.ts in this same PR.
| @@ -624,15 +591,15 @@ | |||
| Access an organisation owner may need to approve: | |||
There was a problem hiding this comment.
[low] ineffective-each-key
The {#each} keys added for missingInstallRequirements and helpBullets use array index (i) as the key. Index-based keys are functionally equivalent to having no key for Svelte DOM diffing. Same pattern in web/docs/src/lib/DocTreeNav.svelte lines 109 and 141. These satisfy svelte/require-each-key but are no-ops.
|
@ifireball This has been rebased onto latest main and updated based on review feedback. The primary focus has shifted to Key changes since the last review:
Ready for review when you have a chance. |
Add frontend linting scaffolding for the admin SPA (Svelte 5 + Vite 6 + TypeScript): - ESLint flat config with typescript-eslint, eslint-plugin-svelte, and Prettier compat - Prettier with prettier-plugin-svelte for consistent code formatting - Stylelint with stylelint-config-standard and stylelint-config-html/svelte - npm scripts: lint, lint:fix, format, format:check, stylelint, stylelint:fix Signed-off-by: Wayne Sun <gsun@redhat.com>
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
waynesun09
force-pushed
the
add-frontend-linting
branch
from
9faa736 to
af9b20b
Compare
|
🤖 Review · |
Signed-off-by: Wayne Sun <gsun@redhat.com>
- Move ignores block to first position in ESLint flat config array per ESLint 9 convention (prevents accidental non-global ignore if a files key is later added to the same object) - Spread ts.configs.recommended (it's an array of 3 config objects) instead of relying on defineConfig to flatten nested arrays - Remove no-op color-no-hex: null from stylelint config (rule does not exist in stylelint-config-standard) Signed-off-by: Wayne Sun <gsun@redhat.com>
Run ESLint, Prettier, and Stylelint on staged files before each commit. Follows the same pattern as openkaiden/kaiden: Husky v9 with lint-staged, scoped to web/admin/src files only. Signed-off-by: Wayne Sun <gsun@redhat.com>
- Attach cause to re-thrown errors in githubClient and orgConfigParse - Prefix unused appSlug and scanComplete vars with underscore - Add each-block keys in OrgList popover lists - Suppress require-yield in test generators that intentionally throw - Replace deprecated clip with clip-path in sr-only - Replace deprecated word-break: break-word with overflow-wrap Signed-off-by: Wayne Sun <gsun@redhat.com>
The variable was written in 7 locations but never read, creating unnecessary reactive tracking overhead in Svelte 5. Signed-off-by: Wayne Sun <gsun@redhat.com>
- Remove unused toml-eslint-parser devDependency
- Scope browser globals to web/{admin,docs}/src/** so test files
and non-browser contexts don't silently inherit browser APIs
Assisted-by: Claude (review, fix)
Signed-off-by: Wayne Sun <gsun@redhat.com>
waynesun09
force-pushed
the
add-frontend-linting
branch
from
af9b20b to
758ce3e
Compare
|
🤖 Finished Review · ✅ Success · Started 2:52 PM UTC · Completed 3:04 PM UTC |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
| return await obtainTurnstileTokenWithSignal(siteKey, merged); | ||
| } catch (e) { | ||
| if (deadline.signal.aborted && !userSignal?.aborted) { | ||
| // eslint-disable-next-line preserve-caught-error -- TODO: attach cause once callers handle it |
There was a problem hiding this comment.
[low] dead-code
The ESLint disable comment references a rule (preserve-caught-error) that does not exist in the ESLint configuration. The disable directive is a no-op.
|
🤖 Finished Retro · ✅ Success · Started 3:27 PM UTC · Completed 3:36 PM UTC |
Retro: PR #625 — Add ESLint, Prettier, and Stylelint for web appsTimeline: PR opened May 3 by What went well:
What went poorly:
Proposals: 1 new proposal below. Most other improvement areas are already tracked by existing open issues (referenced above). Proposals filed
|
feat: add ESLint, Prettier, and Stylelint for web apps
3 participants
Summary
Establishes ESLint, Prettier, and Stylelint tooling for both
web/docsandweb/adminSvelte 5 apps. The primary focus isweb/docs/src(documentation viewer), withweb/admin/srcincluded as scaffolding since active admin development is on hold.eslint-plugin-svelte,typescript-eslint, and Svelte 5 component quality rulesprettier-plugin-sveltefor consistent formattingstylelint-config-html/sveltefor CSS linting in scoped styleslint-stagedinto the existingpre-commitframework as a local hook — no new hook system, works alongside gitleaks, shellcheck, and other existing hooksadminSvelteConfig/docsSvelteConfig) so each app can evolve independentlygit blamelint,lint:fix,format,format:check,stylelint,stylelint:fixweb/docs:{#each}keys,{@html}suppression for build-time markdownweb/docs/src/app.css: selector ordering, range notation, empty linesscanCompletereactive state fromOrgList.svelteScope
Linting covers both frontend apps:
web/docs/src/**— documentation viewer (primary focus)web/admin/src/**— admin SPA (scaffolding, active development on hold)Key rules enabled
svelte/no-at-html-tagssvelte/require-each-key{#each}svelte/block-langlang="ts"on scripts@typescript-eslint/no-unused-varssvelte/max-lines-per-blockmax-lines(150)Pre-commit integration
Uses the existing
pre-commitframework (.pre-commit-config.yaml) instead of adding a separate hook system. Thelint-stagedhook runs ESLint, Stylelint, and Prettier with--fixon stagedweb/{admin,docs}/src/**files. Existing hooks (gitleaks, detect-private-key, go-vet, ADR linters, etc.) are unaffected.Phased rollout
warn, baseline disables on existing violationserrorand enablecolor-no-hexTest plan
npm run format:checkpasses (0 issues)npm run lintpasses (0 errors, warnings only for existing large files)npm run stylelintpasses (0 issues)npm run checkpasses (svelte-check)