Copilot commented on code in PR #38:
URL: https://github.com/apache/phoenix-site/pull/38#discussion_r3717063912
##########
unit-tests/setup.ts:
##########
@@ -20,6 +20,25 @@ import "@testing-library/jest-dom";
import { cleanup } from "@testing-library/react";
import { afterEach } from "vitest";
+if (!globalThis.localStorage) {
+ const values = new Map<string, string>();
+ const localStorage: Storage = {
+ get length() {
+ return values.size;
+ },
+ clear: () => values.clear(),
+ getItem: (key) => values.get(key) ?? null,
+ key: (index) => [...values.keys()][index] ?? null,
+ removeItem: (key) => values.delete(key),
+ setItem: (key, value) => values.set(key, String(value))
+ };
Review Comment:
The localStorage polyfill methods currently return Map/boolean values (from
Map.clear/delete/set). Real `Storage` methods return `void`; returning values
can cause behavior differences if any code accidentally relies on the return
value.
##########
build.sh:
##########
@@ -31,10 +31,16 @@ install_node() {
if ! command -v node > /dev/null 2>&1 || ! command -v npm > /dev/null 2>&1;
then
echo "node/npm not found — bootstrapping via nvm..."
install_node
-elif [ -f "$NVM_DIR/nvm.sh" ]; then
+else
+ NODE_MAJOR="$(node --version | cut -d. -f1 | tr -d v)"
+ if [ "$NODE_MAJOR" -ne "$REQUIRED_NODE_MAJOR" ]; then
+ echo "Node.js ${REQUIRED_NODE_MAJOR} is required (found $(node --version))
— bootstrapping via nvm..."
+ install_node
+ elif [ -f "$NVM_DIR/nvm.sh" ]; then
Review Comment:
`[ "$NODE_MAJOR" -ne "$REQUIRED_NODE_MAJOR" ]` does a numeric comparison and
will error (and abort due to `set -e`) if `$NODE_MAJOR` is ever non-numeric. A
string comparison is sufficient here and avoids that failure mode; also indent
the nvm comment block consistently under the `elif`.
##########
unit-tests/setup.ts:
##########
@@ -20,6 +20,25 @@ import "@testing-library/jest-dom";
import { cleanup } from "@testing-library/react";
import { afterEach } from "vitest";
+if (!globalThis.localStorage) {
+ const values = new Map<string, string>();
+ const localStorage: Storage = {
+ get length() {
+ return values.size;
+ },
+ clear: () => values.clear(),
+ getItem: (key) => values.get(key) ?? null,
+ key: (index) => [...values.keys()][index] ?? null,
+ removeItem: (key) => values.delete(key),
+ setItem: (key, value) => values.set(key, String(value))
+ };
+
+ Object.defineProperty(globalThis, "localStorage", {
+ configurable: true,
+ value: localStorage
+ });
+}
+
afterEach(() => {
cleanup();
});
Review Comment:
The in-memory Map backing the localStorage polyfill persists across tests
because it is created once in setup. Clearing localStorage in `afterEach`
prevents state leakage between tests and improves test isolation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
