Skip to content

Commit 78bf1f0

Browse files
gildesmaraisgildesmarais
committed
fix: relax build metadata gate and harden conversion retry classification
1 parent bfd6dca commit 78bf1f0

6 files changed

Lines changed: 212 additions & 78 deletions

File tree

‎app/web/config/environment_validator.rb‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ def validate_build_metadata!
100100

101101
log_missing_build_metadata!
102102
warn_lines(*missing_build_metadata_warning_lines)
103-
exit 1
103+
nil
104104
end
105105

106106
def validate_account_configuration!
@@ -154,15 +154,16 @@ def build_metadata_values
154154
def log_missing_build_metadata!
155155
SecurityLogger.log_config_validation_failure(
156156
'build_metadata',
157-
'Missing BUILD_TAG or GIT_SHA'
157+
'Missing BUILD_TAG or GIT_SHA',
158+
severity: :warn
158159
)
159160
end
160161

161162
# @return [Array<String>]
162163
def missing_build_metadata_warning_lines
163164
[
164-
'CRITICAL: Missing build metadata for production deployment!',
165-
'Set BUILD_TAG to the release build tag and GIT_SHA to the deployed commit SHA.'
165+
'WARNING: Missing build metadata for production deployment.',
166+
'Set BUILD_TAG and GIT_SHA to improve release traceability.'
166167
]
167168
end
168169
end

‎frontend/src/__tests__/useFeedConversion.test.ts‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,6 +439,101 @@ describe('useFeedConversion', () => {
439439
expect(result.current.error).toBe('Unauthorized');
440440
});
441441

442+
it('does not auto-retry when API returns a non-retryable BAD_REQUEST code', async () => {
443+
fetchMock.mockResolvedValueOnce(
444+
new Response(
445+
JSON.stringify({
446+
success: false,
447+
error: { code: 'BAD_REQUEST', message: 'Input rejected' },
448+
}),
449+
{
450+
status: 400,
451+
headers: { 'Content-Type': 'application/json' },
452+
}
453+
)
454+
);
455+
456+
const { result } = renderHook(() => useFeedConversion());
457+
458+
await act(async () => {
459+
await expect(
460+
result.current.convertFeed('https://example.com/articles', 'faraday', 'testtoken')
461+
).rejects.toThrow('Input rejected');
462+
});
463+
464+
expect(fetchMock).toHaveBeenCalledTimes(1);
465+
expect(result.current.result).toBeNull();
466+
expect(result.current.error).toBe('Input rejected');
467+
});
468+
469+
it('still auto-retries when API returns INTERNAL_SERVER_ERROR even if message contains a url', async () => {
470+
const createdFeed = {
471+
id: 'test-id',
472+
name: 'Test Feed',
473+
url: 'https://example.com/articles',
474+
strategy: 'browserless',
475+
feed_token: 'test-token',
476+
public_url: 'https://example.com/feed',
477+
json_public_url: 'https://example.com/feed.json',
478+
created_at: '2024-01-01T00:00:00Z',
479+
updated_at: '2024-01-01T00:00:00Z',
480+
};
481+
482+
fetchMock
483+
.mockResolvedValueOnce(
484+
new Response(
485+
JSON.stringify({
486+
success: false,
487+
error: {
488+
code: 'INTERNAL_SERVER_ERROR',
489+
message: 'Failed to fetch https://example.com/articles',
490+
},
491+
}),
492+
{
493+
status: 500,
494+
headers: { 'Content-Type': 'application/json' },
495+
}
496+
)
497+
)
498+
.mockResolvedValueOnce(
499+
new Response(
500+
JSON.stringify({
501+
success: true,
502+
data: {
503+
feed: createdFeed,
504+
},
505+
}),
506+
{
507+
status: 201,
508+
headers: { 'Content-Type': 'application/json' },
509+
}
510+
)
511+
)
512+
.mockResolvedValueOnce(
513+
new Response(JSON.stringify({ items: [] }), {
514+
status: 200,
515+
headers: { 'Content-Type': 'application/feed+json' },
516+
})
517+
);
518+
519+
const { result } = renderHook(() => useFeedConversion());
520+
521+
await act(async () => {
522+
await result.current.convertFeed('https://example.com/articles', 'faraday', 'testtoken');
523+
});
524+
525+
const retryRequest = fetchMock.mock.calls[1]?.[0] as Request;
526+
expect(await retryRequest.clone().json()).toEqual({
527+
url: 'https://example.com/articles',
528+
strategy: 'browserless',
529+
});
530+
expect(result.current.result?.retry).toEqual({
531+
automatic: true,
532+
from: 'faraday',
533+
to: 'browserless',
534+
});
535+
});
536+
442537
it('does not offer a duplicate manual retry after automatic fallback also fails', async () => {
443538
fetchMock
444539
.mockResolvedValueOnce(

‎frontend/src/hooks/useFeedConversion.ts‎

Lines changed: 93 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,11 @@ interface ConversionState {
2525

2626
interface ConversionError extends Error {
2727
manualRetryStrategy?: string;
28-
autoRetryAttempted?: boolean;
2928
}
3029

30+
const PREVIEW_UNAVAILABLE_MESSAGE = 'Preview unavailable right now.';
31+
const NON_RETRYABLE_ERROR_CODES = new Set(['BAD_REQUEST', 'UNAUTHORIZED', 'FORBIDDEN']);
32+
3133
export function useFeedConversion() {
3234
const requestIdRef = useRef(0);
3335
const [state, setState] = useState<ConversionState>({
@@ -50,66 +52,35 @@ export function useFeedConversion() {
5052

5153
const requestId = requestIdRef.current + 1;
5254
requestIdRef.current = requestId;
53-
setState((prev) => ({ ...prev, isConverting: true, error: null }));
55+
markConversionStarted(setState);
5456

5557
try {
5658
const feed = await requestFeedCreation(normalizedUrl, requestedStrategy, token);
57-
const result = {
58-
feed,
59-
preview: buildLoadingPreviewState(),
60-
retry: null,
61-
};
62-
63-
setState((prev) => ({ ...prev, isConverting: false, result, error: null }));
64-
void hydratePreview(feed, requestId, null, setState, requestIdRef);
65-
return result;
59+
return publishCreatedFeed(feed, null, requestId, setState, requestIdRef);
6660
} catch (firstError) {
6761
if (shouldAutoRetry(requestedStrategy, fallbackStrategy, firstError)) {
6862
try {
6963
const feed = await requestFeedCreation(normalizedUrl, fallbackStrategy, token);
70-
const result = {
64+
return publishCreatedFeed(
7165
feed,
72-
preview: buildLoadingPreviewState(),
73-
retry: { automatic: true, from: requestedStrategy, to: fallbackStrategy },
74-
};
75-
76-
setState((prev) => ({ ...prev, isConverting: false, result, error: null }));
77-
void hydratePreview(feed, requestId, result.retry, setState, requestIdRef);
78-
return result;
66+
{ automatic: true, from: requestedStrategy, to: fallbackStrategy },
67+
requestId,
68+
setState,
69+
requestIdRef
70+
);
7971
} catch (secondError) {
8072
const message = buildRetryFailureMessage(
8173
firstError,
8274
secondError,
8375
requestedStrategy,
8476
fallbackStrategy
8577
);
86-
const retryError = buildConversionError(message, {
87-
manualRetryStrategy: undefined,
88-
autoRetryAttempted: true,
89-
});
90-
91-
setState((prev) => ({
92-
...prev,
93-
isConverting: false,
94-
error: message,
95-
result: null,
96-
}));
97-
throw retryError;
78+
failConversion(setState, message, { manualRetryStrategy: undefined });
9879
}
9980
}
10081

10182
const message = toErrorMessage(firstError);
102-
const retryError = buildConversionError(message, {
103-
manualRetryStrategy: alternateStrategy(requestedStrategy),
104-
});
105-
106-
setState((prev) => ({
107-
...prev,
108-
isConverting: false,
109-
error: message,
110-
result: null,
111-
}));
112-
throw retryError;
83+
failConversion(setState, message, { manualRetryStrategy: alternateStrategy(requestedStrategy) });
11384
}
11485
};
11586

@@ -143,7 +114,7 @@ async function loadPreview(feed: FeedRecord): Promise<CreatedFeedResult['preview
143114
headers: { Accept: 'application/feed+json' },
144115
});
145116

146-
if (!response.ok) throw new Error('Preview unavailable right now.');
117+
if (!response.ok) throw new Error(PREVIEW_UNAVAILABLE_MESSAGE);
147118

148119
const payload = (await response.json()) as JsonFeedResponse;
149120
const items =
@@ -154,7 +125,7 @@ async function loadPreview(feed: FeedRecord): Promise<CreatedFeedResult['preview
154125

155126
return {
156127
items,
157-
error: items.length > 0 ? null : 'Preview unavailable right now.',
128+
error: items.length > 0 ? null : PREVIEW_UNAVAILABLE_MESSAGE,
158129
isLoading: false,
159130
};
160131
}
@@ -243,19 +214,7 @@ function shouldAutoRetry(
243214
error: unknown
244215
): fallbackStrategy is string {
245216
if (strategy !== 'faraday' || !fallbackStrategy) return false;
246-
247-
const normalized = toErrorMessage(error).toLowerCase();
248-
return !(
249-
normalized.includes('unauthorized') ||
250-
normalized.includes('bad request') ||
251-
normalized.includes('forbidden') ||
252-
normalized.includes('access token') ||
253-
normalized.includes('authentication') ||
254-
normalized.includes('invalid response format') ||
255-
normalized.includes('network error') ||
256-
normalized.includes('url') ||
257-
normalized.includes('unsupported strategy')
258-
);
217+
return retryableForFallback(error);
259218
}
260219

261220
function buildRetryFailureMessage(
@@ -279,30 +238,97 @@ function buildConversionError(message: string, metadata: Partial<ConversionError
279238
}
280239

281240
const toErrorMessage = (error: unknown): string => {
241+
const details = extractErrorDetails(error);
242+
if (details?.message) return details.message;
282243
if (error instanceof SyntaxError) return 'Invalid response format from feed creation API';
283244
if (error instanceof Error) return error.message;
284245
if (typeof error === 'string' && error.trim()) return error;
285-
286-
const message = extractMessage(error);
287-
return message ?? 'An unexpected error occurred';
246+
return 'An unexpected error occurred';
288247
};
289248

290249
const toPreviewErrorMessage = (error: unknown): string => {
291-
if (error instanceof SyntaxError) return 'Preview unavailable right now.';
250+
if (error instanceof SyntaxError) return PREVIEW_UNAVAILABLE_MESSAGE;
292251
if (error instanceof Error && error.message.trim()) return error.message;
293-
return 'Preview unavailable right now.';
252+
return PREVIEW_UNAVAILABLE_MESSAGE;
294253
};
295254

296-
const extractMessage = (error: unknown): string | null => {
255+
function markConversionStarted(
256+
setState: (value: ConversionState | ((prev: ConversionState) => ConversionState)) => void
257+
) {
258+
setState((prev) => ({ ...prev, isConverting: true, error: null }));
259+
}
260+
261+
function publishCreatedFeed(
262+
feed: FeedRecord,
263+
retry: CreatedFeedResult['retry'],
264+
requestId: number,
265+
setState: (value: ConversionState | ((prev: ConversionState) => ConversionState)) => void,
266+
requestIdRef: { current: number }
267+
): CreatedFeedResult {
268+
const result: CreatedFeedResult = {
269+
feed,
270+
preview: buildLoadingPreviewState(),
271+
retry,
272+
};
273+
274+
setState((prev) => ({ ...prev, isConverting: false, result, error: null }));
275+
void hydratePreview(feed, requestId, retry, setState, requestIdRef);
276+
return result;
277+
}
278+
279+
function failConversion(
280+
setState: (value: ConversionState | ((prev: ConversionState) => ConversionState)) => void,
281+
message: string,
282+
metadata: Partial<ConversionError>
283+
): never {
284+
setState((prev) => ({
285+
...prev,
286+
isConverting: false,
287+
error: message,
288+
result: null,
289+
}));
290+
291+
throw buildConversionError(message, metadata);
292+
}
293+
294+
const extractErrorDetails = (error: unknown): { message?: string; code?: string } | null => {
297295
if (!error || typeof error !== 'object') return null;
298296

299-
const candidate =
300-
(error as { error?: { message?: unknown }; message?: unknown }).error?.message ??
301-
(error as { message?: unknown }).message;
297+
const candidate = error as {
298+
error?: { message?: unknown; code?: unknown };
299+
message?: unknown;
300+
code?: unknown;
301+
};
302302

303-
return typeof candidate === 'string' && candidate.trim() ? candidate : null;
303+
const message = normalizeString(candidate.error?.message ?? candidate.message);
304+
const code = normalizeString(candidate.error?.code ?? candidate.code);
305+
return { message, code };
304306
};
305307

308+
function retryableForFallback(error: unknown): boolean {
309+
const details = extractErrorDetails(error);
310+
const errorCode = details?.code?.toUpperCase();
311+
if (errorCode && NON_RETRYABLE_ERROR_CODES.has(errorCode)) return false;
312+
313+
const message = (details?.message ?? toErrorMessage(error)).toLowerCase();
314+
if (!details?.code && (message.includes('unauthorized') || message.includes('forbidden'))) return false;
315+
if (!details?.code && message.includes('bad request')) return false;
316+
if (message.includes('access token') || message.includes('authentication')) return false;
317+
if (message.includes('unsupported strategy')) return false;
318+
if (message.includes('invalid response format')) return false;
319+
320+
return !networkFailure(error, message);
321+
}
322+
323+
function networkFailure(error: unknown, normalizedMessage: string): boolean {
324+
if (error instanceof TypeError) return true;
325+
return normalizedMessage.includes('network error');
326+
}
327+
328+
function normalizeString(value: unknown): string | undefined {
329+
return typeof value === 'string' && value.trim() ? value : undefined;
330+
}
331+
306332
function normalizePreviewText(value?: string): string | null {
307333
if (!value) return null;
308334

‎public/openapi.yaml‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ paths:
250250
<rss version="2.0"><channel><title>Error</title><description>Internal Server Error</description></channel></rss>
251251
schema:
252252
type: string
253-
description: returns JSON Feed-shaped errors when requested by json extension
253+
description: returns unauthorized for invalid tokens
254254
'403':
255255
content:
256256
application/feed+json:
@@ -263,8 +263,7 @@ paths:
263263
<rss version="2.0"><channel><title>Error</title><description>Internal Server Error</description></channel></rss>
264264
schema:
265265
type: string
266-
description: returns JSON Feed-shaped forbidden errors when requested through
267-
Accept
266+
description: returns forbidden when auto source is disabled
268267
'500':
269268
content:
270269
application/feed+json:

0 commit comments

Comments
 (0)