From 16e52790bf45d37b916b95b58c0cd600532ac90e Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 7 Nov 2024 12:34:03 +0100 Subject: [PATCH 1/2] mirage: Fix README status codes We are serving these files via S3, which unfortunately returns 403 instead of 404 for missing files... --- mirage/route-handlers/crates.js | 4 ++-- tests/mirage/crates/versions/readme-test.js | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/mirage/route-handlers/crates.js b/mirage/route-handlers/crates.js index 990b1e967aa..9674330652b 100644 --- a/mirage/route-handlers/crates.js +++ b/mirage/route-handlers/crates.js @@ -374,12 +374,12 @@ export function register(server) { const { name, version: versionNum } = request.params; const crate = schema.crates.findBy({ name }); if (!crate) { - return new Response(404, { 'Content-Type': 'text/html' }, ''); + return new Response(403, { 'Content-Type': 'text/html' }, ''); } const version = schema.versions.findBy({ crateId: crate.id, num: versionNum }); if (!version || !version.readme) { - return new Response(404, { 'Content-Type': 'text/html' }, ''); + return new Response(403, { 'Content-Type': 'text/html' }, ''); } return new Response(200, { 'Content-Type': 'text/html' }, version.readme); diff --git a/tests/mirage/crates/versions/readme-test.js b/tests/mirage/crates/versions/readme-test.js index 606e71a6dc8..95af3303c46 100644 --- a/tests/mirage/crates/versions/readme-test.js +++ b/tests/mirage/crates/versions/readme-test.js @@ -11,7 +11,7 @@ module('Mirage | GET /api/v1/crates/:id/:version/readme', function (hooks) { test('returns 404 for unknown crates', async function (assert) { let response = await fetch('/api/v1/crates/foo/1.0.0/readme'); - assert.strictEqual(response.status, 404); + assert.strictEqual(response.status, 403); assert.strictEqual(await response.text(), ''); }); @@ -19,7 +19,7 @@ module('Mirage | GET /api/v1/crates/:id/:version/readme', function (hooks) { this.server.create('crate', { name: 'rand' }); let response = await fetch('/api/v1/crates/rand/1.0.0/readme'); - assert.strictEqual(response.status, 404); + assert.strictEqual(response.status, 403); assert.strictEqual(await response.text(), ''); }); @@ -28,7 +28,7 @@ module('Mirage | GET /api/v1/crates/:id/:version/readme', function (hooks) { this.server.create('version', { crate, num: '1.0.0' }); let response = await fetch('/api/v1/crates/rand/1.0.0/readme'); - assert.strictEqual(response.status, 404); + assert.strictEqual(response.status, 403); assert.strictEqual(await response.text(), ''); }); From b25f4e383443c0f845d0178df3eebfa604ae2704 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Thu, 7 Nov 2024 12:28:04 +0100 Subject: [PATCH 2/2] Show "Retry" button for README loading errors --- app/controllers/crate/version.js | 2 +- app/models/version.js | 4 ++++ app/styles/crate/version.module.css | 9 +++++++- app/templates/crate/version.hbs | 13 +++++++++++ e2e/acceptance/readme-rendering.spec.ts | 28 ++++++++++++++++++++++- tests/acceptance/readme-rendering-test.js | 23 +++++++++++++++++++ 6 files changed, 76 insertions(+), 3 deletions(-) diff --git a/app/controllers/crate/version.js b/app/controllers/crate/version.js index 4e3b4fc2ff6..ca857bb165d 100644 --- a/app/controllers/crate/version.js +++ b/app/controllers/crate/version.js @@ -43,7 +43,7 @@ export default class CrateVersionController extends Controller { : await version.loadReadmeTask.perform(); // If the README contains `language-mermaid` we ensure that the `mermaid` library has loaded before we continue - if (readme.includes('language-mermaid') && !this.mermaid.loadTask.lastSuccessful?.value) { + if (readme && readme.includes('language-mermaid') && !this.mermaid.loadTask.lastSuccessful?.value) { try { await this.mermaid.loadTask.perform(); } catch (error) { diff --git a/app/models/version.js b/app/models/version.js index 73a304ee1ec..87b1c29a2ec 100644 --- a/app/models/version.js +++ b/app/models/version.js @@ -122,6 +122,10 @@ export default class Version extends Model { loadReadmeTask = keepLatestTask(async () => { if (this.readme_path) { let response = await fetch(this.readme_path); + if (response.status === 404 || response.status === 403) { + return; + } + if (!response.ok) { throw new Error(`README request for ${this.crateName} v${this.num} failed`); } diff --git a/app/styles/crate/version.module.css b/app/styles/crate/version.module.css index ad77dee357e..d33d27c088f 100644 --- a/app/styles/crate/version.module.css +++ b/app/styles/crate/version.module.css @@ -25,7 +25,7 @@ } } -.no-readme { +.no-readme, .readme-error { padding: var(--space-l) var(--space-s); text-align: center; font-size: 20px; @@ -39,6 +39,13 @@ } } +.retry-button { + composes: yellow-button from '../shared/buttons.module.css'; + display: block; + text-align: center; + margin: var(--space-s) auto 0; +} + .placeholder-title { width: 30%; height: 25px; diff --git a/app/templates/crate/version.hbs b/app/templates/crate/version.hbs index d32bb78fd29..3294764b8c4 100644 --- a/app/templates/crate/version.hbs +++ b/app/templates/crate/version.hbs @@ -25,6 +25,19 @@
+ {{else if this.loadReadmeTask.last.error}} +
+ Failed to load README file for {{this.crate.name}} v{{this.currentVersion.num}} + + +
{{else}}
{{this.crate.name}} v{{this.currentVersion.num}} appears to have no README.md file diff --git a/e2e/acceptance/readme-rendering.spec.ts b/e2e/acceptance/readme-rendering.spec.ts index 6d32b7f4217..d8c551f6ccc 100644 --- a/e2e/acceptance/readme-rendering.spec.ts +++ b/e2e/acceptance/readme-rendering.spec.ts @@ -1,4 +1,5 @@ -import { test, expect } from '@/e2e/helper'; +import { expect, test } from '@/e2e/helper'; +import { Response } from 'miragejs'; const README_HTML = `

Serde is a framework for serializing and deserializing Rust data structures efficiently and generically.

@@ -112,4 +113,29 @@ test.describe('Acceptance | README rendering', { tag: '@acceptance' }, () => { await page.goto('/crates/serde'); await expect(page.locator('[data-test-no-readme]')).toBeVisible(); }); + + test('it shows an error message and retry button if loading fails', async ({ page, mirage }) => { + await page.exposeBinding('resp200', () => new Response(200, { 'Content-Type': 'text/html' }, 'foo')); + + await mirage.addHook(server => { + let crate = server.create('crate', { name: 'serde' }); + server.create('version', { crate, num: '1.0.0' }); + + server.logging = true; + // Simulate a server error when fetching the README + server.get('/api/v1/crates/:name/:version/readme', {}, 500); + }); + + await page.goto('/crates/serde'); + await expect(page.locator('[data-test-readme-error]')).toBeVisible(); + await expect(page.locator('[data-test-retry-button]')).toBeVisible(); + + await page.evaluate(() => { + // Simulate a successful response when fetching the README + server.get('/api/v1/crates/:name/:version/readme', {}); + }); + + await page.click('[data-test-retry-button]'); + await expect(page.locator('[data-test-readme]')).toHaveText('{}'); + }); }); diff --git a/tests/acceptance/readme-rendering-test.js b/tests/acceptance/readme-rendering-test.js index 5017143b70f..946cd792671 100644 --- a/tests/acceptance/readme-rendering-test.js +++ b/tests/acceptance/readme-rendering-test.js @@ -1,6 +1,8 @@ +import { click } from '@ember/test-helpers'; import { module, test } from 'qunit'; import percySnapshot from '@percy/ember'; +import { Response } from 'miragejs'; import { setupApplicationTest } from 'crates-io/tests/helpers'; @@ -112,4 +114,25 @@ module('Acceptance | README rendering', function (hooks) { await visit('/crates/serde'); assert.dom('[data-test-no-readme]').exists(); }); + + test('it shows an error message and retry button if loading fails', async function (assert) { + let crate = this.server.create('crate', { name: 'serde' }); + this.server.create('version', { crate, num: '1.0.0' }); + + // Simulate a server error when fetching the README + this.server.get('/api/v1/crates/:name/:version/readme', {}, 500); + + await visit('/crates/serde'); + assert.dom('[data-test-readme-error]').exists(); + assert.dom('[data-test-retry-button]').exists(); + + // Simulate a successful response when fetching the README + this.server.get( + '/api/v1/crates/:name/:version/readme', + () => new Response(200, { 'Content-Type': 'text/html' }, 'foo'), + ); + + await click('[data-test-retry-button]'); + assert.dom('[data-test-readme]').hasText('foo'); + }); });