Merge branch 'withastro:main' into arpan-#12171

.changeset/breezy-plums-clap.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes a case where the error overlay would not escape the message

.changeset/dull-lemons-check.md

@@ -0,0 +1,14 @@
+---
+'astro': patch
+---
+
+Fixes a case where Astro allowed to call an action without using `Astro.callAction`. This is now invalid, and Astro will show a proper error.
+
+```diff
+---
+import { actions } from "astro:actions";
+
+-const result = actions.getUser({ userId: 123 });
++const result = Astro.callAction(actions.getUser, { userId: 123 });
+---
+```

.changeset/nine-mayflies-film.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes unexpected 200 status in dev server logs for action errors and redirects.

packages/astro/src/actions/runtime/middleware.ts

@@ -3,7 +3,7 @@ import type { APIContext, MiddlewareNext } from '../../@types/astro.js';
 import { defineMiddleware } from '../../core/middleware/index.js';
 import { getOriginPathname } from '../../core/routing/rewrite.js';
 import { ACTION_QUERY_PARAMS } from '../consts.js';
-import { formContentTypes, hasContentType } from './utils.js';
+import { ACTION_API_CONTEXT_SYMBOL, formContentTypes, hasContentType } from './utils.js';
 import { getAction } from './virtual/get-action.js';
 import {
 	type SafeResult,
@@ -100,6 +100,7 @@ async function handlePost({
 		formData = await request.clone().formData();
 	}
 	const { getActionResult, callAction, props, redirect, ...actionAPIContext } = context;
+	Reflect.set(actionAPIContext, ACTION_API_CONTEXT_SYMBOL, true);
 	const action = baseAction.bind(actionAPIContext);
 	const actionResult = await action(formData);
 

packages/astro/src/actions/runtime/route.ts

@@ -1,5 +1,5 @@
 import type { APIRoute } from '../../@types/astro.js';
-import { formContentTypes, hasContentType } from './utils.js';
+import { ACTION_API_CONTEXT_SYMBOL, formContentTypes, hasContentType } from './utils.js';
 import { getAction } from './virtual/get-action.js';
 import { serializeActionResult } from './virtual/shared.js';
 
@@ -28,6 +28,7 @@ export const POST: APIRoute = async (context) => {
 		return new Response(null, { status: 415 });
 	}
 	const { getActionResult, callAction, props, redirect, ...actionAPIContext } = context;
+	Reflect.set(actionAPIContext, ACTION_API_CONTEXT_SYMBOL, true);
 	const action = baseAction.bind(actionAPIContext);
 	const result = await action(args);
 	const serialized = serializeActionResult(result);

packages/astro/src/actions/runtime/utils.ts

@@ -1,5 +1,7 @@
 import type { APIContext } from '../../@types/astro.js';
 
+export const ACTION_API_CONTEXT_SYMBOL = Symbol.for('astro.actionAPIContext');
+
 export const formContentTypes = ['application/x-www-form-urlencoded', 'multipart/form-data'];
 
 export function hasContentType(contentType: string, expected: string[]) {
@@ -26,3 +28,8 @@ export type MaybePromise<T> = T | Promise<T>;
  * `result.error.fields` will be typed with the `name` field.
  */
 export type ErrorInferenceObject = Record<string, any>;
+
+export function isActionAPIContext(ctx: ActionAPIContext): boolean {
+	const symbol = Reflect.get(ctx, ACTION_API_CONTEXT_SYMBOL);
+	return symbol === true;
+}

packages/astro/src/actions/runtime/virtual/server.ts

@@ -1,7 +1,12 @@
 import { z } from 'zod';
 import { ActionCalledFromServerError } from '../../../core/errors/errors-data.js';
 import { AstroError } from '../../../core/errors/errors.js';
-import type { ActionAPIContext, ErrorInferenceObject, MaybePromise } from '../utils.js';
+import {
+	type ActionAPIContext,
+	type ErrorInferenceObject,
+	type MaybePromise,
+	isActionAPIContext,
+} from '../utils.js';
 import { ActionError, ActionInputError, type SafeResult, callSafely } from './shared.js';
 
 export * from './shared.js';
@@ -60,7 +65,8 @@ export function defineAction<
 			: getJsonServerHandler(handler, inputSchema);
 
 	async function safeServerHandler(this: ActionAPIContext, unparsedInput: unknown) {
-		if (typeof this === 'function') {
+		// The ActionAPIContext should always contain the `params` property
+		if (typeof this === 'function' || !isActionAPIContext(this)) {
 			throw new AstroError(ActionCalledFromServerError);
 		}
 		return callSafely(() => serverHandler(unparsedInput, this));

packages/astro/src/actions/utils.ts

@@ -2,7 +2,7 @@ import type fsMod from 'node:fs';
 import * as eslexer from 'es-module-lexer';
 import type { APIContext } from '../@types/astro.js';
 import type { Locals } from './runtime/middleware.js';
-import type { ActionAPIContext } from './runtime/utils.js';
+import { ACTION_API_CONTEXT_SYMBOL, type ActionAPIContext } from './runtime/utils.js';
 import { deserializeActionResult, getActionQueryString } from './runtime/virtual/shared.js';
 
 export function hasActionPayload(locals: APIContext['locals']): locals is Locals {
@@ -23,6 +23,7 @@ export function createGetActionResult(locals: APIContext['locals']): APIContext[
 
 export function createCallAction(context: ActionAPIContext): APIContext['callAction'] {
 	return (baseAction, input) => {
+		Reflect.set(context, ACTION_API_CONTEXT_SYMBOL, true);
 		const action = baseAction.bind(context);
 		return action(input) as any;
 	};

packages/astro/src/core/errors/dev/vite.ts

@@ -105,6 +105,7 @@ export function enhanceViteSSRError({
 }
 
 export interface AstroErrorPayload {
+	__isEnhancedAstroErrorPayload: true;
 	type: ErrorPayload['type'];
 	err: Omit<ErrorPayload['err'], 'loc'> & {
 		name?: string;
@@ -164,6 +165,7 @@ export async function getViteErrorPayload(err: ErrorWithMetadata): Promise<Astro
 		: undefined;
 
 	return {
+		__isEnhancedAstroErrorPayload: true,
 		type: 'error',
 		err: {
 			...err,

packages/astro/src/core/module-loader/vite.ts

@@ -1,6 +1,9 @@
 import { EventEmitter } from 'node:events';
 import path from 'node:path';
+import { pathToFileURL } from 'node:url';
 import type * as vite from 'vite';
+import { collectErrorMetadata } from '../errors/dev/utils.js';
+import { getViteErrorPayload } from '../errors/dev/vite.js';
 import type { ModuleLoader, ModuleLoaderEventEmitter } from './loader.js';
 
 export function createViteLoader(viteServer: vite.ViteDevServer): ModuleLoader {
@@ -43,6 +46,24 @@ export function createViteLoader(viteServer: vite.ViteDevServer): ModuleLoader {
 		}
 		const msg = args[0] as vite.HMRPayload;
 		if (msg?.type === 'error') {
+			// If we have an error, but it didn't go through our error enhancement program, it means that it's a HMR error from
+			// vite itself, which goes through a different path. We need to enhance it here.
+			if (!(msg as any)['__isEnhancedAstroErrorPayload']) {
+				const err = collectErrorMetadata(msg.err, pathToFileURL(viteServer.config.root));
+				getViteErrorPayload(err).then((payload) => {
+					events.emit('hmr-error', {
+						type: 'error',
+						err: {
+							message: payload.err.message,
+							stack: payload.err.stack,
+						},
+					});
+
+					args[0] = payload;
+					_wsSend.apply(this, args);
+				});
+				return;
+			}
 			events.emit('hmr-error', msg);
 		}
 		_wsSend.apply(this, args);

packages/astro/src/vite-plugin-astro-server/route.ts

@@ -130,15 +130,13 @@ type HandleRoute = {
 	manifestData: ManifestData;
 	incomingRequest: http.IncomingMessage;
 	incomingResponse: http.ServerResponse;
-	status?: 404 | 500 | 200;
 	pipeline: DevPipeline;
 };
 
 export async function handleRoute({
 	matchedRoute,
 	url,
 	pathname,
-	status = getStatus(matchedRoute),
 	body,
 	origin,
 	pipeline,
@@ -208,12 +206,17 @@ export async function handleRoute({
 	});
 
 	let response;
+	let statusCode = 200;
 	let isReroute = false;
 	let isRewrite = false;
 	try {
 		response = await renderContext.render(mod);
 		isReroute = response.headers.has(REROUTE_DIRECTIVE_HEADER);
 		isRewrite = response.headers.has(REWRITE_DIRECTIVE_HEADER_KEY);
+		statusCode = isRewrite
+			? // Ignore `matchedRoute` status for rewrites
+				response.status
+			: (getStatusByMatchedRoute(matchedRoute) ?? response.status);
 	} catch (err: any) {
 		const custom500 = getCustom500Route(manifestData);
 		if (!custom500) {
@@ -225,7 +228,7 @@ export async function handleRoute({
 		const preloaded500Component = await pipeline.preload(custom500, filePath500);
 		renderContext.props.error = err;
 		response = await renderContext.render(preloaded500Component);
-		status = 500;
+		statusCode = 500;
 	}
 
 	if (isLoggedRequest(pathname)) {
@@ -235,20 +238,20 @@ export async function handleRoute({
 			req({
 				url: pathname,
 				method: incomingRequest.method,
-				statusCode: isRewrite ? response.status : (status ?? response.status),
+				statusCode,
 				isRewrite,
 				reqTime: timeEnd - timeStart,
 			}),
 		);
 	}
-	if (response.status === 404 && response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no') {
+
+	if (statusCode === 404 && response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no') {
 		const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline);
 		if (options && options.route !== fourOhFourRoute?.route)
 			return handleRoute({
 				...options,
 				matchedRoute: fourOhFourRoute,
 				url: new URL(pathname, url),
-				status: 404,
 				body,
 				origin,
 				pipeline,
@@ -290,18 +293,18 @@ export async function handleRoute({
 
 	// Apply the `status` override to the response object before responding.
 	// Response.status is read-only, so a clone is required to override.
-	if (status && response.status !== status && (status === 404 || status === 500)) {
+	if (response.status !== statusCode) {
 		response = new Response(response.body, {
-			status: status,
+			status: statusCode,
 			headers: response.headers,
 		});
 	}
 	await writeSSRResult(request, response, incomingResponse);
 }
 
-function getStatus(matchedRoute?: MatchedRoute): 404 | 500 | 200 {
-	if (!matchedRoute) return 404;
-	if (matchedRoute.route.route === '/404') return 404;
-	if (matchedRoute.route.route === '/500') return 500;
-	return 200;
+/** Check for /404 and /500 custom routes to compute status code */
+function getStatusByMatchedRoute(matchedRoute?: MatchedRoute) {
+	if (matchedRoute?.route.route === '/404') return 404;
+	if (matchedRoute?.route.route === '/500') return 500;
+	return undefined;
 }

packages/astro/test/actions.test.js

@@ -132,6 +132,12 @@ describe('Astro Actions', () => {
 				assert.equal(data, 'Hello, ben!');
 			}
 		});
+
+		it('Should fail when calling an action without using Astro.callAction', async () => {
+			const res = await fixture.fetch('/invalid/');
+			const text = await res.text();
+			assert.match(text, /ActionCalledFromServerError/);
+		});
 	});
 
 	describe('build', () => {

packages/astro/test/fixtures/actions/src/pages/invalid.astro

@@ -0,0 +1,6 @@
+---
+import { actions } from "astro:actions";
+
+// this is invalid, it should fail
+const result = await actions.imageUploadInChunks();
+---