Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -429,19 +429,18 @@ jobs:

# Suppressions may only shrink (web/AGENTS.md). On push and dispatch there
# is no PR base, and only the path check runs: main takes changes only
# through PRs, which ran the growth check. The first PR has no base file.
# through PRs, which ran the growth check. A rule the base config did not
# apply to a file may add that file's baseline.
- name: Check lint suppressions
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}
run: |
base="$RUNNER_TEMP/base-suppressions.json"
if [ -n "$BASE_SHA" ]; then
git fetch --no-tags --depth=1 origin "$BASE_SHA"
if git cat-file -e "$BASE_SHA:web/lint/eslint-suppressions.json"; then
git show "$BASE_SHA:web/lint/eslint-suppressions.json" > "$base"
fi
node lint/check-suppressions.mjs --base "$BASE_SHA"
else
node lint/check-suppressions.mjs
fi
if [ -f "$base" ]; then node lint/check-suppressions.mjs "$base"; else node lint/check-suppressions.mjs; fi

- name: Test with coverage gate
run: npm run test:coverage
Expand Down
6 changes: 5 additions & 1 deletion web/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,8 @@ record you must read before changing the rule.

## Lint

- **`npm run lint` runs ESLint from its own install in `web/lint/`, and its suppressions file may only shrink.** Run `npm ci --prefix lint` once. typescript-eslint 8 declares `typescript <6.1.0` and needs the classic compiler API, which TypeScript 7 does not ship, so `web/lint/package.json` pins TypeScript 6 for the linter while the app builds with 7. `web/eslint.config.js` only re-exports `web/lint/eslint.config.js`, because ESLint 10 lints only files under the config's directory. The config enables `js` and typescript-eslint `recommended`, `react-hooks/rules-of-hooks`, `react-hooks/exhaustive-deps`, type-aware `@typescript-eslint/no-floating-promises`, and errors on unused disable comments. It turns off only style rules and `no-unused-vars`, which tsc already owns. Pre-existing violations live in `web/lint/eslint-suppressions.json`, counted per file and rule. A new violation fails, and so does a suppression that no longer matches: ESLint 10 exits 2 unless `--pass-on-unpruned-suppressions` is passed, which CI never does. ESLint never checks an entry whose file is gone, and it accepts any count the same PR writes, so CI also runs `web/lint/check-suppressions.mjs`. It rejects entries for missing files and, on a pull request, any file/rule entry that is new or higher than at the base. Renaming a file that has suppressions therefore means fixing its violations. After fixing a suppressed violation, run `npm run lint -- --prune-suppressions` and commit the smaller file. Delete entries for deleted files by hand: `--prune-suppressions` leaves them. Never run `--suppress-all` or `--suppress-rule` to admit a new violation, and never fix an `exhaustive-deps` finding by auto-fix: a wrong dependency array can loop renders. CI's `web` job runs the lint; the pre-commit hook lints only staged `web/src` files.
- **`npm run lint` runs ESLint from its own install in `web/lint/`, and its suppressions file may only shrink.** Run `npm ci --prefix lint` once. typescript-eslint 8 declares `typescript <6.1.0` and needs the classic compiler API, which TypeScript 7 does not ship, so `web/lint/package.json` pins TypeScript 6 for the linter while the app builds with 7. `web/eslint.config.js` only re-exports `web/lint/eslint.config.js`, because ESLint 10 lints only files under the config's directory. The config enables `js` and typescript-eslint `recommended`, `react-hooks/rules-of-hooks`, `react-hooks/exhaustive-deps`, type-aware `@typescript-eslint/no-floating-promises`, and errors on unused disable comments. It turns off only style rules and `no-unused-vars`, which tsc already owns. Pre-existing violations live in `web/lint/eslint-suppressions.json`, counted per file and rule. A new violation fails, and so does a suppression that no longer matches: ESLint 10 exits 2 unless `--pass-on-unpruned-suppressions` is passed, which CI never does. ESLint never checks an entry whose file is gone, and it accepts any count the same PR writes, so CI also runs `web/lint/check-suppressions.mjs`. It rejects entries for missing files and, on a pull request (`--base <ref>`), any file/rule entry that is new or higher than at the base. The one exception is a rule that the base's own config did not apply to that file: the entry may then record at most the sites that rule finds in the file's BASE source. That is how a newly enabled rule, or a widened glob, records existing sites, while a moved or added file and any new site still have to be fixed. An unreadable `--base` exits 2 rather than skipping the check. Renaming a file that has suppressions therefore means fixing its violations. After fixing a suppressed violation, run `npm run lint -- --prune-suppressions` and commit the smaller file. Delete entries for deleted files by hand: `--prune-suppressions` leaves them. Never run `--suppress-all` or `--suppress-rule` to admit a new violation, and never fix an `exhaustive-deps` finding by auto-fix: a wrong dependency array can loop renders. CI's `web` job runs the lint; the pre-commit hook lints only staged `web/src` files.

## Data loading

- **A screen reads data through `useLatestLoad` or `usePagedList`, never through `await` or `.then(` inside a `useEffect` (#1185).** A raw async effect lets a response for inputs the user has already left land after the newer one and overwrite it, or clear its error, loading flag or rows. Review caught it on 23 PRs (#467, the #703 series, #918 then #940). `useLatestLoad(inputs, load)` in `src/components/` aborts the superseded run and drops its result and its error; `usePagedList` does the same for paged lists, and `useDialogAction` covers dialog writes. A `no-restricted-syntax` rule in `web/lint/eslint.config.js` fails an `await` or `.then(` inside a `useEffect` callback in `src/routes/**` and `src/components/**`. The sites that existed when it landed are in the suppressions file and move to the hook when their screen is next touched. It cannot see an async helper defined outside the effect, nor a write followed by a refresh in an event handler; only the hooks cover those.
66 changes: 54 additions & 12 deletions web/lint/check-suppressions.mjs
Original file line number Diff line number Diff line change
@@ -1,11 +1,28 @@
// ESLint checks a suppression only for files it lints, and accepts any count a
// PR writes. This enforces the rest of "suppressions may only shrink"
// (web/AGENTS.md): every entry names an existing file, and with a base file
// given, no file/rule entry is new or higher than at the base.
// Usage, from web/: node lint/check-suppressions.mjs [base-suppressions.json]
import { existsSync, readFileSync } from 'node:fs';
// (web/AGENTS.md): every entry names an existing file, and with a base given,
// no file/rule entry is new or higher than at the base. The one exception is
// a rule that did not apply to that file under the base's own config: then the
// entry may record at most the violations the rule finds in the BASE source of
// that file, so a newly enabled rule can record existing sites, while code
// moved or added under a widened glob still has to be fixed.
// Usage, from web/: node lint/check-suppressions.mjs [--base <git-ref>]
import { execFileSync } from 'node:child_process';
import { existsSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
import { ESLint } from 'eslint';

const args = process.argv.slice(2);
if (!(args.length === 0 || (args.length === 2 && args[0] === '--base' && args[1] !== ''))) {
console.error('Usage, from web/: node lint/check-suppressions.mjs [--base <git-ref>]');
process.exit(2);
}

const root = execFileSync('git', ['rev-parse', '--show-toplevel'], { encoding: 'utf8' }).trim();
const git = (...gitArgs) => execFileSync('git', gitArgs, { cwd: root, encoding: 'utf8', maxBuffer: 64 * 1024 * 1024 });
// null only when the commit has no such path; any git failure throws.
const atCommit = (commit, path) =>
git('ls-tree', '--name-only', commit, '--', `web/${path}`).trim() === '' ? null : git('show', `${commit}:web/${path}`);

const read = (path) => JSON.parse(readFileSync(path, 'utf8'));
const entries = (suppressions) =>
Object.entries(suppressions).flatMap(([file, rules]) =>
Object.entries(rules).map(([rule, { count }]) => {
Expand All @@ -14,15 +31,40 @@ const entries = (suppressions) =>
}),
);

const head = entries(read('lint/eslint-suppressions.json'));
const head = entries(JSON.parse(readFileSync('lint/eslint-suppressions.json', 'utf8')));
const errors = head.filter(({ file }) => !existsSync(file)).map(({ file, rule }) => `${file} ${rule}: file does not exist`);

const basePath = process.argv[2];
if (basePath) {
const base = new Map(entries(read(basePath)).map(({ file, rule, count }) => [`${file} ${rule}`, count]));
for (const { file, rule, count } of head) {
const was = base.get(`${file} ${rule}`) ?? 0;
if (count > was) errors.push(`${file} ${rule}: count ${was} -> ${count}`);
let base = null;
if (args.length === 2) {
try {
base = git('rev-parse', '--verify', '--end-of-options', `${args[1]}^{commit}`).trim();
} catch {
console.error(`--base ${args[1]} is not a commit this checkout has. Fetch it first.`);
process.exit(2);
}
}
const baseSuppressions = base && atCommit(base, 'lint/eslint-suppressions.json');
if (base && !baseSuppressions) console.log(`${args[1]} predates lint/eslint-suppressions.json: only the path check runs.`);
if (baseSuppressions) {
const was = new Map(entries(JSON.parse(baseSuppressions)).map(({ file, rule, count }) => [`${file} ${rule}`, count]));
// Written beside this script so its imports resolve from web/lint/node_modules.
const baseConfig = 'lint/.base-eslint.config.js';
writeFileSync(baseConfig, atCommit(base, 'lint/eslint.config.js'));
try {
const atBase = new ESLint({ overrideConfigFile: baseConfig });
for (const { file, rule, count } of head) {
const allowed = was.get(`${file} ${rule}`) ?? 0;
if (count <= allowed || !existsSync(file)) continue;
const appliedAtBase = ((await atBase.calculateConfigForFile(file))?.rules?.[rule]?.[0] ?? 0) !== 0;
const source = appliedAtBase ? null : atCommit(base, file);
const baseSites = source === null ? 0 : (await new ESLint({ ruleFilter: ({ ruleId }) => ruleId === rule })
.lintText(source, { filePath: file }))[0].messages.filter((m) => m.ruleId === rule).length;
if (count > Math.max(allowed, baseSites)) {
errors.push(`${file} ${rule}: count ${allowed} -> ${count}` + (appliedAtBase ? '' : ` (the base source has ${baseSites})`));
}
}
} finally {
rmSync(baseConfig);
}
}

Expand Down
78 changes: 78 additions & 0 deletions web/lint/eslint-suppressions.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,41 +14,119 @@
"count": 1
}
},
"src/components/NamedEntityPicker.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/AppLayout.tsx": {
"@typescript-eslint/no-floating-promises": {
"count": 1
}
},
"src/routes/ConnectPage.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/CustomersPage.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/DailyEntryPage.tsx": {
"no-restricted-syntax": {
"count": 5
},
"react-hooks/exhaustive-deps": {
"count": 1
}
},
"src/routes/Dashboard.tsx": {
"@typescript-eslint/no-floating-promises": {
"count": 1
},
"no-restricted-syntax": {
"count": 7
}
},
"src/routes/ExpensesPage.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/FeedPage.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/FlocksPage.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/GradesPage.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/HistoryPage.tsx": {
"no-restricted-syntax": {
"count": 1
},
"react-hooks/exhaustive-deps": {
"count": 1
}
},
"src/routes/InventoryPage.tsx": {
"no-restricted-syntax": {
"count": 1
},
"react-hooks/exhaustive-deps": {
"count": 1
}
},
"src/routes/Login.tsx": {
"@typescript-eslint/no-floating-promises": {
"count": 2
},
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/ProductsPage.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/StockPage.tsx": {
"no-restricted-syntax": {
"count": 1
},
"react-hooks/exhaustive-deps": {
"count": 1
}
},
"src/routes/UsersPage.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/routes/WaterPage.tsx": {
"no-restricted-syntax": {
"count": 2
}
},
"src/routes/sales/SalesPage.tsx": {
"no-restricted-syntax": {
"count": 2
}
},
"src/routes/sales/usePayments.tsx": {
"no-restricted-syntax": {
"count": 1
}
},
"src/theme/FarmThemeProvider.tsx": {
"react-hooks/exhaustive-deps": {
"count": 1
Expand Down
14 changes: 14 additions & 0 deletions web/lint/eslint.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ import reactHooks from 'eslint-plugin-react-hooks';
import globals from 'globals';
import tseslint from 'typescript-eslint';

const inEffect = 'CallExpression:matches([callee.name="useEffect"], [callee.property.name="useEffect"])';
const rawAsyncEffect = 'Load data with useLatestLoad or usePagedList (web/src/components/). '
+ 'A raw async effect lets a superseded response overwrite newer state (#467, #703, #918).';

export default tseslint.config(
{ ignores: ['dist/', 'dev-dist/', 'coverage/', 'lint/'] },
{
Expand All @@ -25,4 +29,14 @@ export default tseslint.config(
'preserve-caught-error': 'off',
},
},
{
files: ['src/routes/**/*.{ts,tsx}', 'src/components/**/*.{ts,tsx}'],
ignores: ['**/*.test.{ts,tsx}'],
rules: {
'no-restricted-syntax': ['error',
{ selector: `${inEffect} AwaitExpression`, message: rawAsyncEffect },
{ selector: `${inEffect} CallExpression[callee.property.name="then"]`, message: rawAsyncEffect },
],
},
},
);
121 changes: 121 additions & 0 deletions web/src/components/useLatestLoad.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import { describe, it, expect, vi } from "vitest";
import { act, renderHook } from "@testing-library/react";
import { useLatestLoad } from "./useLatestLoad";

// #1185 — each test settles deferred promises in a chosen ORDER, because the
// order the network answers in is the thing under test.

function deferred<T>() {
let resolve!: (value: T) => void;
let reject!: (err: unknown) => void;
const promise = new Promise<T>((res, rej) => { resolve = res; reject = rej; });
return { promise, resolve, reject };
}

// One deferred per input value, and the signal each run was handed.
function loader() {
const pending = new Map<string, ReturnType<typeof deferred<string>>>();
const signals = new Map<string, AbortSignal>();
const load = (input: string) => (signal: AbortSignal) => {
const d = deferred<string>();
pending.set(input, d);
signals.set(input, signal);
return d.promise;
};
return { pending, signals, load };
}

function host(first: string) {
const { pending, signals, load } = loader();
const hook = renderHook(({ input }) => useLatestLoad([input], load(input)), { initialProps: { input: first } });
return { ...hook, pending, signals };
}

describe("useLatestLoad", () => {
it("drops a superseded response that lands after the newer one", async () => {
const { result, rerender, pending } = host("a");
rerender({ input: "b" });
await act(async () => { pending.get("b")!.resolve("rows for b"); });
await act(async () => { pending.get("a")!.resolve("rows for a"); });
expect(result.current.data).toBe("rows for b");
expect(result.current.loading).toBe(false);
});

it("drops a superseded response that lands first, and keeps loading for the newer one", async () => {
const { result, rerender, pending } = host("a");
rerender({ input: "b" });
await act(async () => { pending.get("a")!.resolve("rows for a"); });
expect(result.current.data).toBeNull();
expect(result.current.loading).toBe(true);
});

it("drops a superseded failure, so it cannot clear the newer result", async () => {
const { result, rerender, pending } = host("a");
rerender({ input: "b" });
await act(async () => { pending.get("b")!.resolve("rows for b"); });
await act(async () => { pending.get("a")!.reject(new Error("a failed")); });
expect(result.current).toMatchObject({ data: "rows for b", error: null, loading: false });
});

it("aborts the superseded run's signal, and the last run's on unmount", () => {
const { rerender, unmount, signals } = host("a");
rerender({ input: "b" });
expect(signals.get("a")!.aborted).toBe(true);
expect(signals.get("b")!.aborted).toBe(false);
unmount();
expect(signals.get("b")!.aborted).toBe(true);
});

it("reports the current run's failure with no data, and retry loads the same inputs again", async () => {
const load = vi.fn<(signal: AbortSignal) => Promise<string>>()
.mockRejectedValueOnce(new Error("offline"))
.mockResolvedValueOnce("rows");
const { result } = renderHook(() => useLatestLoad(["a"], load));
await act(async () => {});
expect(result.current).toMatchObject({ data: null, loading: false });
expect(result.current.error).toEqual(new Error("offline"));
await act(async () => { result.current.retry(); });
expect(result.current).toMatchObject({ data: "rows", error: null, loading: false });
expect(load).toHaveBeenCalledTimes(2);
});

it("drops the old run's response the moment retry is called, before the new run starts", async () => {
const calls: ReturnType<typeof deferred<string>>[] = [];
const { result } = renderHook(() => useLatestLoad(["a"], () => {
const d = deferred<string>();
calls.push(d);
return d.promise;
}));
const save = deferred<void>();
// A handler that saves, then retries: the old load answers in the gap
// after retry and before React runs the replacement effect.
await act(async () => {
const handler = save.promise.then(() => result.current.retry());
save.resolve();
await handler;
calls[0].resolve("rows from before the save");
// Let the old run settle completely before act lets the effect run.
await new Promise((settled) => setTimeout(settled, 0));
});
expect(calls).toHaveLength(2);
expect(result.current).toMatchObject({ data: null, loading: true });
});

it("reports a load that throws before returning a promise as an error, not a crash", async () => {
const { result } = renderHook(() => useLatestLoad(["a"], (): Promise<string> => {
throw new RangeError("Invalid time value");
}));
await act(async () => {});
expect(result.current).toMatchObject({ data: null, loading: false });
expect(result.current.error).toEqual(new RangeError("Invalid time value"));
});

it("does not reload when only the load function's identity changes", async () => {
const load = vi.fn(async () => "rows");
const { rerender } = renderHook(() => useLatestLoad(["a"], () => load()));
await act(async () => {});
rerender();
await act(async () => {});
expect(load).toHaveBeenCalledTimes(1);
});
});
Loading