feat: append revision footer to app pages#26
Conversation
|
Warning @LYL-8bit this pull request is linked to an issue that is already assigned to another user. Please link it to an open issue assigned to you or to an unassigned open issue. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36d74f1008
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const contentType = response.headers.get('content-type')?.toLowerCase() ?? '' | ||
| if (!contentType.includes('text/html')) return response | ||
|
|
||
| const html = await response.text() |
There was a problem hiding this comment.
Calling response.text() here decodes the upstream bytes as UTF-8, and the rewritten response then re-encodes that string while keeping the original Content-Type header. For any HTML page served with a non-UTF-8 charset (for example text/html; charset=iso-8859-1), this will corrupt characters and can produce a body/header charset mismatch that did not exist before this commit.
Useful? React with 👍 / 👎.
| const headers = new Headers(response.headers) | ||
| headers.delete('content-length') | ||
| return new Response(updatedHtml, { |
There was a problem hiding this comment.
The response body is modified by footer injection, but only content-length is removed; validators like ETag/Last-Modified are forwarded unchanged. That allows conditional requests to be validated against the unmodified upstream representation, so clients can receive 304 Not Modified and keep a cached page with an outdated injected footer across router deployments.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThis PR adds dynamic revision footer injection to HTML responses. A new 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/worker-routing.test.ts (1)
90-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame TypeScript error here.
Apply the same signature fix as above.
Proposed fix
- test('does not append the router revision footer to non-HTML responses', async () => { - globalThis.fetch = (async () => new Response('{"ok":true}', { + test('does not append the router revision footer to non-HTML responses', async () => { + globalThis.fetch = (async (_input: RequestInfo | URL, _init?: RequestInit) => new Response('{"ok":true}', { headers: { 'content-type': 'application/json' }, })) as typeof fetch
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 292b5693-d88b-4a0b-ac67-5a308dcd2399
📒 Files selected for processing (2)
src/worker.tstests/worker-routing.test.ts
| test('appends the router revision footer to successful HTML app responses', async () => { | ||
| globalThis.fetch = (async () => new Response('<html><body><main>app</main></body></html>', { | ||
| headers: { | ||
| 'content-length': '42', | ||
| 'content-type': 'text/html; charset=utf-8', | ||
| }, | ||
| })) as typeof fetch | ||
|
|
||
| const res = await worker.fetch(new Request('https://pay.ubq.fi/'), {} as Env) | ||
| const html = await res.text() | ||
|
|
||
| expect(res.status).toBe(200) | ||
| expect(res.headers.get('x-uos-router-revision')).toBe('local') | ||
| expect(res.headers.get('content-length')).toBe(null) | ||
| expect(html).toContain('<main>app</main>') | ||
| expect(html).toContain('id="uos-router-revision"') | ||
| expect(html).toContain('https://github.com/ubiquity/ubq.fi-router') | ||
| expect(html).toContain('>local</a>') | ||
| }) |
There was a problem hiding this comment.
Fix TypeScript error: mock signature mismatch.
CI fails because the arrow function lacks parameters that typeof fetch expects. Match the existing test pattern.
Proposed fix
- test('appends the router revision footer to successful HTML app responses', async () => {
- globalThis.fetch = (async () => new Response('<html><body><main>app</main></body></html>', {
+ test('appends the router revision footer to successful HTML app responses', async () => {
+ globalThis.fetch = (async (_input: RequestInfo | URL, _init?: RequestInit) => new Response('<html><body><main>app</main></body></html>', {
headers: {
'content-length': '42',
'content-type': 'text/html; charset=utf-8',
},
})) as typeof fetch📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('appends the router revision footer to successful HTML app responses', async () => { | |
| globalThis.fetch = (async () => new Response('<html><body><main>app</main></body></html>', { | |
| headers: { | |
| 'content-length': '42', | |
| 'content-type': 'text/html; charset=utf-8', | |
| }, | |
| })) as typeof fetch | |
| const res = await worker.fetch(new Request('https://pay.ubq.fi/'), {} as Env) | |
| const html = await res.text() | |
| expect(res.status).toBe(200) | |
| expect(res.headers.get('x-uos-router-revision')).toBe('local') | |
| expect(res.headers.get('content-length')).toBe(null) | |
| expect(html).toContain('<main>app</main>') | |
| expect(html).toContain('id="uos-router-revision"') | |
| expect(html).toContain('https://github.com/ubiquity/ubq.fi-router') | |
| expect(html).toContain('>local</a>') | |
| }) | |
| test('appends the router revision footer to successful HTML app responses', async () => { | |
| globalThis.fetch = (async (_input: RequestInfo | URL, _init?: RequestInit) => new Response('<html><body><main>app</main></body></html>', { | |
| headers: { | |
| 'content-length': '42', | |
| 'content-type': 'text/html; charset=utf-8', | |
| }, | |
| })) as typeof fetch | |
| const res = await worker.fetch(new Request('https://pay.ubq.fi/'), {} as Env) | |
| const html = await res.text() | |
| expect(res.status).toBe(200) | |
| expect(res.headers.get('x-uos-router-revision')).toBe('local') | |
| expect(res.headers.get('content-length')).toBe(null) | |
| expect(html).toContain('<main>app</main>') | |
| expect(html).toContain('id="uos-router-revision"') | |
| expect(html).toContain('https://github.com/ubiquity/ubq.fi-router') | |
| expect(html).toContain('>local</a>') | |
| }) |
🧰 Tools
🪛 GitHub Actions: CI / 2_test.txt
[error] 71-71: tsc type-check failed (TS2352): Conversion of type '() => Promise' to type 'typeof fetch' may be a mistake. Property 'preconnect' is missing in type '() => Promise' but required in type 'typeof fetch'.
🪛 GitHub Actions: CI / test
[error] 71-71: TypeScript (tsc --noEmit) TS2352: Conversion of type '() => Promise' to type 'typeof fetch' may be a mistake. Property 'preconnect' is missing in type '() => Promise' but required in type 'typeof fetch'.
1 participant
Summary
ubiquity/ubq.fi-routercommit when a deployed SHA is available, with alocalfallback for developmentcontent-lengthafter HTML injectionValidation
bun testlocally becausebunis not installed in this runner and npm dependency install timed out; CI should run the repository's Bun test workflowResolves #6