fix: enable CORS on chart embed endpoint and add missing tooltip#2859
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an Access-Control-Allow-Origin header and uses the shared cache constant for the cached downloads SVG endpoint; replaces the embed “copy URL” label with an info-icon TooltipApp and adds a translated tip (and schema entry) that explains the default 12-month date range when startDate/endDate are omitted. ChangesSVG Embed CORS, caching and date defaults
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/api/embed/downloads.svg.get.ts (1)
14-15: 💤 Low valueConsider removing the
Vary: Originheader.The
Vary: Originheader instructs caches to store separate copies of the response for different origins. However, sinceAccess-Control-Allow-Origin: *is constant for all requests, the response doesn't actually vary by origin. IncludingVary: Origincreates unnecessary cache fragmentation without benefit.
Vary: Originis typically used when the CORS header value is conditionally set based on the request origin (e.g., echoing back specific origins), but with*, it's redundant.♻️ Proposed fix
setHeader(event, 'Access-Control-Allow-Origin', '*') -setHeader(event, 'Vary', 'Origin')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/embed/downloads.svg.get.ts` around lines 14 - 15, The Vary: Origin header is redundant when Access-Control-Allow-Origin is '*'—remove the setHeader call that sets 'Vary' on the response (the call like setHeader(event, 'Vary', 'Origin')) or change it to only set Vary when the CORS value is dynamic; leave the setHeader(event, 'Access-Control-Allow-Origin', '*') intact and ensure no other code reintroduces Vary for this route.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/api/embed/downloads.svg.get.ts`:
- Around line 14-15: The Vary: Origin header is redundant when
Access-Control-Allow-Origin is '*'—remove the setHeader call that sets 'Vary' on
the response (the call like setHeader(event, 'Vary', 'Origin')) or change it to
only set Vary when the CORS value is dynamic; leave the setHeader(event,
'Access-Control-Allow-Origin', '*') intact and ensure no other code reintroduces
Vary for this route.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6baa83d6-9eb5-4fd6-97d1-8ccbfd19440d
📒 Files selected for processing (1)
server/api/embed/downloads.svg.get.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/utils/embed-downloads-svg.ts`:
- Around line 145-154: The date math around effectiveEndDate/effectiveStartDate
is timezone-unsafe because it mixes parsing a UTC date string with local-time
mutators (new Date(...), setDate, setFullYear, getFullYear); change the
computations to use UTC-based constructors and accessors: build defaultEndDate
and defaultStartDate using Date.UTC (or new Date(Date.UTC(...))) and use
getUTCDate/setUTCDate and getUTCFullYear/setUTCFullYear (or operate on UTC
milliseconds) so that effectiveEndDate and effectiveStartDate (and the default*
variables) are computed in UTC consistently with parseDateQuery and the
toISOString().split('T')[0] formatting. Ensure the logic still falls back to
requestedEndDate and parseDateQuery(query.startDate ?? query.start) but uses the
UTC-safe date objects for the -1 day and -1 year adjustments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de3da5ad-6e37-46ee-a3b8-1b509adbe3c3
📒 Files selected for processing (1)
server/utils/embed-downloads-svg.ts
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Package/TrendsChart.vue (1)
1976-1976:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the stray
+token in the button tag.Line 1976 introduces invalid template syntax inside the
<button>start tag, which will break SFC compilation.Suggested fix
<button type="button" :aria-expanded="showEmbedFields" - + aria-controls="trends-embed-chart" class="self-start flex items-center gap-1 text-2xs font-mono text-fg-subtle hover:text-fg transition-colors" `@click`="showEmbedFields = !showEmbedFields" >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/components/Package/TrendsChart.vue` at line 1976, Remove the stray '+' character inside the malformed <button> start tag in the TrendsChart.vue template (that stray token at the start of the button element is causing invalid SFC syntax); edit the template to delete the '+' so the button tag is a valid opening tag (e.g., <button ...>), then save and verify the component compiles and the surrounding markup remains correctly formed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/components/Package/TrendsChart.vue`:
- Line 1976: Remove the stray '+' character inside the malformed <button> start
tag in the TrendsChart.vue template (that stray token at the start of the button
element is causing invalid SFC syntax); edit the template to delete the '+' so
the button tag is a valid opening tag (e.g., <button ...>), then save and verify
the component compiles and the surrounding markup remains correctly formed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bb16329-5098-4f27-a87f-34b6c1dcdf0c
📒 Files selected for processing (4)
app/components/Package/TrendsChart.vuei18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.json
✅ Files skipped from review due to trivial changes (2)
- i18n/locales/en.json
- i18n/locales/fr-FR.json
ghostdevv
changed the title
2 participants
Follow up to #2833
This enables CORS on the embed downloads chart endpoint, to enable fetch and retrieve the svg string. Without it, only the image tag can be consumed with the url in its src.
I also added a tooltip, informing that
startDateandendDatecan be omitted from the query to dynamically get the last year of data.