Append app revision footer#29
Conversation
|
Warning @hevehueqwnv 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: 5c525f30ba
ℹ️ 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 headers = new Headers(response.headers) | ||
| headers.delete('content-length') | ||
| return new Response(nextHtml, { |
There was a problem hiding this comment.
When an HTML upstream returns a cache validation response such as 304 Not Modified with content-type: text/html, this path reads the empty body and then constructs new Response(nextHtml, { status: 304, ... }). The Fetch Response constructor rejects body content for null-body statuses like 304, so a normal conditional request for cached HTML will throw here and the route-level catch will turn the upstream 304 into a 502 instead of preserving the cache hit.
Useful? React with 👍 / 👎.
| function getRepoUrl(subKey: string, isPlugin: boolean): string { | ||
| const name = subKey.replace(/^preview-/, '') | ||
| if (isPlugin) { | ||
| return `https://github.com/ubiquity-os-marketplace/${name}` |
There was a problem hiding this comment.
For plugin domains, subKey still includes the public os- prefix and any routing suffix, so os-command-config.ubq.fi produces https://github.com/ubiquity-os-marketplace/os-command-config. The documented plugin routing maps that host to the command-config-main deployment, so the derived marketplace repo link should not include the router-only os- prefix; otherwise every injected plugin footer points users at a non-existent or wrong repository.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThe PR adds dynamic revision footer injection to HTML responses in the proxy worker. A new constant 🚥 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)
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: 2
🧹 Nitpick comments (1)
tests/worker-routing.test.ts (1)
107-119: ⚡ Quick winAdd a HEAD HTML passthrough test.
Footer logic also branches on
HEAD; add one test to assert no injection/mutation onHEAD+text/htmlresponses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d687e149-59be-45fc-b317-125ab0d83b5b
📒 Files selected for processing (2)
src/worker.tstests/worker-routing.test.ts
| const headers = new Headers(response.headers) | ||
| headers.delete('content-length') |
There was a problem hiding this comment.
Must also delete content-encoding header.
response.text() auto-decompresses gzip. The new Response has uncompressed body but retains content-encoding: gzip header → browser decode error.
🐛 Proposed fix
const headers = new Headers(response.headers)
headers.delete('content-length')
+ headers.delete('content-encoding')
return new Response(nextHtml, {📝 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.
| const headers = new Headers(response.headers) | |
| headers.delete('content-length') | |
| const headers = new Headers(response.headers) | |
| headers.delete('content-length') | |
| headers.delete('content-encoding') |
| return html.replace( | ||
| /<a\b([^>]*\bid=["']git-revision["'][^>]*)>[\s\S]*?<\/a>/i, |
There was a problem hiding this comment.
Inconsistent: hardcodes 'git-revision' instead of using APP_REVISION_ANCHOR_ID.
If the constant changes, this regex won't match.
Proposed fix
return html.replace(
- /<a\b([^>]*\bid=["']git-revision["'][^>]*)>[\s\S]*?<\/a>/i,
+ new RegExp(`<a\\b([^>]*\\bid=["']${APP_REVISION_ANCHOR_ID}["'][^>]*)>[\\s\\S]*?</a>`, 'i'),
(_match, attrs: string) => {📝 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.
| return html.replace( | |
| /<a\b([^>]*\bid=["']git-revision["'][^>]*)>[\s\S]*?<\/a>/i, | |
| return html.replace( | |
| new RegExp(`<a\\b([^>]*\\bid=["']${APP_REVISION_ANCHOR_ID}["'][^>]*)>[\\s\\S]*?</a>`, 'i'), | |
| (_match, attrs: string) => { |
1 participant
Closes #6.
Adds a router-side revision footer for HTML app responses, matching the existing
#bottom-right/#git-revisionshape from work.ubq.fi.#git-revisionanchors without duplicating themValidation:
tsc --noEmitbun test