Fix review findings: router // paths, api responses, range/search/SlotArray, legacy dates

chuchi-core/src/api/Api.ts

@@ -57,6 +57,8 @@ export default class Api {
 		// todo: there might still be an issue with relative and absolute
 		// paths, but i'm not sure fetch supports relative paths?
 		const url = new URL(this.addr + path, DEF_ORIGIN);
+		const isGet = method.toLowerCase() === 'get';
+		if (isGet) setSearchParams(url, data);
 
 		try {
 			const fetchParams = {
@@ -67,33 +69,30 @@ export default class Api {
 			fetchParams.headers['content-type'] = 'application/json';
 
 			// don't send a body if the method is get
-			if (method.toLowerCase() === 'get') {
-				const searchParams = url.searchParams;
+			if (!isGet) fetchParams.body = JSON.stringify(data);
 
-				for (const [key, value] of Object.entries(data ?? {})) {
-					if (value !== undefined && value !== null)
-						searchParams.set(key, value);
-				}
-			} else {
-				fetchParams.body = JSON.stringify(data);
-			}
-
-			let urlStr = url.toString();
-			if (urlStr.startsWith(DEF_ORIGIN))
-				urlStr = urlStr.substring(DEF_ORIGIN.length);
-
-			const resp = await fetch(urlStr, fetchParams);
+			const resp = await fetch(urlToString(url), fetchParams);
 
 			opts.responseStatus = resp.status;
 			opts.responseHeaders = resp.headers;
 
-			if (resp.ok) {
-				return await resp.json();
-			} else {
-				// we've got and error
-				const errObj = await resp.json();
-				err = ApiError.fromJson(errObj);
+			// the body might be empty (204) or not json (an html error page)
+			const text = await resp.text();
+			let json = null;
+			let isJson = true;
+			try {
+				json = text ? JSON.parse(text) : null;
+			} catch (e) {
+				if (resp.ok) throw e;
+				isJson = false;
 			}
+
+			if (resp.ok) return json;
+
+			// we've got and error
+			err = isJson
+				? ApiError.fromJson(json)
+				: ApiError.newOther('response code ' + resp.status);
 		} catch (e: any) {
 			console.error('request error raw', e);
 			err = ApiError.newOther(e.message);
@@ -135,11 +134,11 @@ export default class Api {
 				prog(100);
 
 				// should probably be a json object now
-				if (req.status === 200) {
+				if (isOk(req.status)) {
 					res(req.response);
 				} else {
 					try {
-						err(ApiError.fromJson(req.response));
+						err(xhrError(req));
 					} catch (e: any) {
 						err(ApiError.newOther(e.message));
 					}
@@ -201,11 +200,11 @@ export default class Api {
 
 			req.addEventListener('load', () => {
 				// should probably be a json object now
-				if (req.status === 200) {
+				if (isOk(req.status)) {
 					res(req.response);
 				} else {
 					try {
-						err(ApiError.fromJson(req.response));
+						err(xhrError(req));
 					} catch (e: any) {
 						err(ApiError.newOther(e.message));
 					}
@@ -222,8 +221,13 @@ export default class Api {
 				err(ApiError.newOther('requestTimeout network failure'));
 			});
 
+			// like request, get sends the data as query params
+			const isGet = method.toLowerCase() === 'get';
+			const url = new URL(this.addr + path, DEF_ORIGIN);
+			if (isGet) setSearchParams(url, data);
+
 			// open request
-			req.open(method, this.addr + path);
+			req.open(method, urlToString(url));
 
 			// add headers
 			req.setRequestHeader('content-type', 'application/json');
@@ -234,7 +238,7 @@ export default class Api {
 
 			let body = null;
 			// don't send a body if the method is get
-			if (method.toLowerCase() !== 'get') {
+			if (!isGet) {
 				body = JSON.stringify(data);
 			}
 
@@ -243,3 +247,28 @@ export default class Api {
 		});
 	}
 }
+
+function isOk(status: number): boolean {
+	return status >= 200 && status < 300;
+}
+
+function setSearchParams(url: URL, data: object | null) {
+	for (const [key, value] of Object.entries(data ?? {})) {
+		if (value !== undefined && value !== null)
+			url.searchParams.set(key, value);
+	}
+}
+
+function urlToString(url: URL): string {
+	const urlStr = url.toString();
+	if (urlStr.startsWith(DEF_ORIGIN))
+		return urlStr.substring(DEF_ORIGIN.length);
+	return urlStr;
+}
+
+function xhrError(req: XMLHttpRequest): ApiError {
+	// the response is null if it was not json
+	if (req.response === null)
+		return ApiError.newOther('response code ' + req.status);
+	return ApiError.fromJson(req.response);
+}

chuchi-core/src/api/ApiError.ts

@@ -47,11 +47,17 @@ export default class ApiError {
 	static fromJson(obj: any): ApiError {
 		if (typeof obj === 'string') return new ApiError(obj, null);
 
+		// for example null if the response was not json
+		if (!obj || typeof obj !== 'object')
+			return ApiError.newOther('unexpected error response: ' + obj);
+
 		// {kind, data}
 		if ('kind' in obj) return new ApiError(obj.kind, obj?.data ?? null);
 
 		// {'kind': data}
-		const [kind, data] = Object.entries(obj)[0];
+		const entry = Object.entries(obj)[0];
+		if (!entry) return ApiError.newOther('empty error response');
+		const [kind, data] = entry;
 		return new ApiError(kind, data);
 	}
 

chuchi-legacy/src/dom/utils.ts

@@ -74,10 +74,8 @@ export function inViewY(el: Element) {
 	const wHeight = window.innerHeight;
 	const { top, bottom } = el.getBoundingClientRect();
 
-	const topInView = top >= 0 && top <= wHeight;
-	const bottomInView = bottom >= 0 && bottom <= wHeight;
-
-	return topInView || bottomInView;
+	// also true if the element is taller than the viewport and covers it
+	return top <= wHeight && bottom >= 0;
 }
 
 // name --my-var

chuchi-legacy/src/time/Date.ts

@@ -64,7 +64,17 @@ export default class Date {
 			return;
 		}
 
-		this.raw = new globalThis.Date(date);
+		// js parses YYYY-MM-DD as utc, but this is a local date, west of utc
+		// it would become the previous day
+		const ymd =
+			typeof date === 'string'
+				? /^(\d{4})-(\d{2})-(\d{2})$/.exec(date)
+				: null;
+		if (ymd) {
+			this.raw = new globalThis.Date(+ymd[1], +ymd[2] - 1, +ymd[3]);
+		} else {
+			this.raw = new globalThis.Date(date);
+		}
 
 		// @ts-ignore
 		if (isNaN(this.raw)) {
@@ -117,8 +127,14 @@ export default class Date {
 	get week(): number {
 		const nearestThursday = this.clone();
 		nearestThursday.raw.setDate(this.date + 4 - this.dayMoToSu);
-		const firstDay = new Date(new globalThis.Date(this.year, 0, 1));
-		const days = (nearestThursday.time - firstDay.time) / DAY_IN_MS;
+		// the week belongs to the year of its thursday
+		const firstDay = new Date(
+			new globalThis.Date(nearestThursday.year, 0, 1),
+		);
+		// round because of daylight saving time
+		const days = Math.round(
+			(nearestThursday.time - firstDay.time) / DAY_IN_MS,
+		);
 		return Math.ceil((days + 1) / 7);
 	}
 

chuchi-legacy/src/time/DateTime.ts

@@ -113,8 +113,12 @@ export default class DateTime {
 	get week(): number {
 		const nearestThursday = this.cloneDay();
 		nearestThursday.raw.setDate(this.date + 4 - this.dayMoToSu);
-		const firstDay = new DateTime(new Date(this.year, 0, 1));
-		const days = (nearestThursday.time - firstDay.time) / DAY_IN_MS;
+		// the week belongs to the year of its thursday
+		const firstDay = new DateTime(new Date(nearestThursday.year, 0, 1));
+		// round because of daylight saving time
+		const days = Math.round(
+			(nearestThursday.time - firstDay.time) / DAY_IN_MS,
+		);
 		return Math.ceil((days + 1) / 7);
 	}
 

chuchi-legacy/src/time/Duration.ts

@@ -68,9 +68,11 @@ export default class Duration {
 		}
 
 		if (days < 1) return null;
-		if (days < 7) return intInfo(pre, days, words.day);
+		// compare rounded values, 6.6 days should be a week not 7 days
+		if (Math.round(days) < 7) return intInfo(pre, days, words.day);
 		if (days < 30) return intInfo(pre, days / 7, words.week);
-		if (days < 30 * 12) return intInfo(pre, days / 30, words.month);
+		if (Math.round(days / 30) < 12)
+			return intInfo(pre, days / 30, words.month);
 		return intInfo(pre, days / (30 * 12), words.year);
 	}
 
@@ -90,8 +92,11 @@ export default class Duration {
 			secs *= -1;
 		}
 
-		if (secs < 60) return intInfo(pre, secs, words.second);
-		if (secs < 60 * 60) return intInfo(pre, secs / 60, words.minute);
+		// compare rounded values, 59.6 seconds should be a minute not 60
+		// seconds
+		if (Math.round(secs) < 60) return intInfo(pre, secs, words.second);
+		if (Math.round(secs / 60) < 60)
+			return intInfo(pre, secs / 60, words.minute);
 
 		// because days > 1 get filtered out before
 		// this must be in hours

chuchi-utils/src/collections/SlotArray.ts

@@ -71,7 +71,8 @@ export default class SlotArray<T> {
 	 * @param id - The index to remove the item from.
 	 */
 	remove(id: number) {
-		if (this.inner.length <= id) return;
+		// removing twice would add the slot twice to free
+		if (this.inner.length <= id || this.inner[id] === null) return;
 
 		this.inner[id] = null;
 		this.free.push(id);

chuchi-utils/src/index.ts

@@ -82,14 +82,12 @@ export function randomToken(length: number = 8): string {
  * @param step - The step size between values, defaults to 1.
  * @returns The generated array.
  */
-// todo improve this function
 export function range(start: number, end: number, step: number = 1): number[] {
-	const len = end - start / step;
+	// preallocating is a lot faster than push for bigger ranges
+	const len = step > 0 ? Math.max(0, Math.ceil((end - start) / step)) : 0;
 	const ar = new Array(len);
-	let c = 0;
-	for (let i = start; i < end; i += step) {
-		ar[c] = i;
-		c += 1;
+	for (let c = 0; c < len; c++) {
+		ar[c] = start + c * step;
 	}
 	return ar;
 }

chuchi-utils/src/search.ts

@@ -17,7 +17,7 @@ export function searchScore(search: string, val: string): number {
 	if (i === -1) return 0;
 
 	const distLeft = searchSpaceLeft(i, val);
-	const distRight = searchSpaceLeft(i + search.length, val);
+	const distRight = searchSpaceRight(i + search.length - 1, val);
 
 	return 1 + distLeft + distRight;
 }
@@ -35,9 +35,6 @@ function searchSpaceLeft(idx: number, val: string): number {
 }
 
 // calculate distance to space right of
-// todo is this used???
-// @ts-ignore
-/* eslint-disable @typescript-eslint/no-unused-vars */
 function searchSpaceRight(idx: number, val: string): number {
 	let dist = 0;
 	while (idx < val.length - 1) {

chuchi/src/routing/Router.ts

@@ -165,7 +165,7 @@ export default class Router<C = any> {
 	 */
 	pushReq(req: Request): void {
 		this.currentRequest.setSilent(req);
-		window.history.pushState(req.toHistoryState(), '', req.uri);
+		window.history.pushState(req.toHistoryState(), '', historyUrl(req));
 		this.currentRequest.notify();
 	}
 
@@ -174,7 +174,7 @@ export default class Router<C = any> {
 	 */
 	replaceReq(req: Request): void {
 		this.currentRequest.setSilent(req);
-		window.history.replaceState(req.toHistoryState(), '', req.uri);
+		window.history.replaceState(req.toHistoryState(), '', historyUrl(req));
 		this.currentRequest.notify();
 	}
 
@@ -273,9 +273,13 @@ export default class Router<C = any> {
 		}
 
 		if (history === 'replace') {
-			window.history.replaceState(req.toHistoryState(), '', req.uri);
+			window.history.replaceState(
+				req.toHistoryState(),
+				'',
+				historyUrl(req),
+			);
 		} else if (history === 'push') {
-			window.history.pushState(req.toHistoryState(), '', nUri);
+			window.history.pushState(req.toHistoryState(), '', historyUrl(req));
 		}
 
 		this.currentRequest.set(req);
@@ -403,17 +407,31 @@ export default class Router<C = any> {
 	}
 }
 
+/**
+ * Returns the url to store in the history
+ *
+ * The uri alone is not enough, a path starting with // would be read as
+ * another host and pushState would throw
+ */
+function historyUrl(req: Request): string {
+	return window.location.origin + req.uri;
+}
+
 /**
  * Returns true if the click should be intercepted
  */
 function shouldInterceptClick(
 	e: MouseEvent,
 	link: HTMLAnchorElement | null,
 ): boolean {
+	// another handler took care of it, or not the main button
+	if (e.defaultPrevented || e.button !== 0) return false;
 	const openInNewTab = e.metaKey || e.ctrlKey || e.shiftKey;
 	const saveLink = e.altKey;
 	if (!link || !link.href || openInNewTab || saveLink) return false;
-	if (link.target.toLowerCase() === '_blank') return false;
+	// another window or frame: _blank, _top, _parent or a named one
+	const target = link.target.toLowerCase();
+	if (target && target !== '_self') return false;
 	if (link.hasAttribute('download')) return false;
 	if (!link.href.startsWith('http')) return false;