feat(image): support referrerpolicy on rendered images#4056
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughMultiple components (Card, Chip, ListItem) now delegate image rendering to a new shared ChangesComponent Image Delegation
Sequence Diagram(s)sequenceDiagram
participant Component as Card/Chip/ListItem
participant ImageTemplate as ImageTemplate
participant Browser as Browser DOM
Component->>ImageTemplate: <ImageTemplate image={image} />
ImageTemplate->>Browser: render <img src, alt, loading="lazy", (referrerpolicy?)>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4056/ |
civing
force-pushed
the
feat/image-referrerpolicy
branch
2 times, most recently
from
0f35118 to
9fd2e0b
Compare
Consolidated PR Review -- 6 Dimensions
PR SummaryThis PR adds an optional Merge Readiness -- READY TO MERGE ✅The PR is strictly better than
1. Backward Compatibility -- SAFE ✅Adding an optional field to a public interface and replacing inline
Positives: Strictly additive public-type change. 2. Code Quality -- GOOD ✅Solid, focused change. Helper is small but justified by the spread-cast workaround; e2e tests are real (one per branch); JSDoc on the new type field explains why a consumer would want
Positives: Eliminates real triplication ( 3. Architecture -- ACCEPTABLE
|
| Dimension | Verdict |
|---|---|
| Backward Compatibility | SAFE |
| Code Quality | GOOD |
| Architecture | ACCEPTABLE |
| Security | SAFE |
| Observability | SAFE |
| Performance | SAFE |
| Merge Readiness | READY TO MERGE |
Top Recommendations
- [Medium from Architecture] Move
renderImgout ofsrc/global/shared-types/. The directory previously held only*.types.tsfiles; introducing JSX + a@stencil/coreruntime import there is the first crossing of that line. Two existing conventions fit:src/util/render-image.tsx(matchesimage-resizere-exported viainterface.ts:68) or a*.template.tsxcolocated near a consumer (matchescheckbox/checkbox.template.tsx). The*.template.tsxform would also let it become aFunctionalComponentand be invoked as<ImageTemplate image={this.image} />, which is more idiomatic Stencil. - [Medium from Architecture] Decide what
Image.loadingmeans. The field is documented on the public type (image.types.ts:22) butimage.tsx:26hardcodesloading="lazy"and ignores it (the inline JSX did the same — so this PR doesn't regress, but it's the natural place to fix). Eitherloading={image.loading ?? 'lazy'}in the helper, or removeloadingfrom the publicImageinterface. - [Low from Code Quality] Tighten the cast at
image.tsx:27: replaceas Record<string, string>withRecord<'referrerpolicy', ReferrerPolicy>(or a small typed local) so theReferrerPolicyunion typing isn't widened to arbitrary strings. - [Low from Backward Compatibility] Add an
@internalTSDoc tag onrenderImg(image.tsx:13) to make the public-vs-internal contract explicit and tracked under any future strict-trim rollup, since the boundary currently relies only oninterface.tsnot enumerating the file. - [Low from Code Quality] Mention in the JSDoc on
image.types.ts:31thatreferrerpolicy: ''is treated the same as omitting the attribute, since the helper's truthy check skips emission for''(a technically-validReferrerPolicyliteral). Alternatively, change the helper guard toimage.referrerpolicy !== undefinedfor strict pass-through. - [Low from Observability] Optionally add
if (!image?.src) return null;at the top ofrenderImgso a future caller forgetting the call-site guard gets a self-diagnosing no-op instead of aTypeError. All current callers already guard, so this is purely defensive. - [Low from Security] Consider mirroring the attribute-presence/absence assertions to
cardandchipe2e tests for defense-in-depth, so a future change torenderImgcan't silently regress the no-emission contract on those two consumers.
🤖 Generated by /6-agent-review.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/util/image.template.tsx`:
- Around line 29-33: The ImageTemplate currently hardcodes loading="lazy" which
contradicts the Image contract; update the component to honor the image.loading
value (e.g., use loading={image.loading ?? "lazy"} or remove the loading
attribute if the contract is changed) so consumers’ Image.loading is
respected—modify the JSX in ImageTemplate (the <img ... loading="lazy" ... />
line) to reference image.loading instead of the hardcoded string and keep the
existing referrerPolicyAttr spread.
- Around line 24-26: The referrerPolicyAttr guard treats an explicit
empty-string referrerpolicy as unset because it uses a truthy check; change the
condition that builds referrerPolicyAttr to test image.referrerpolicy !==
undefined (or !== void 0) so that empty string and other falsy but present
values are preserved, i.e., retain the object { referrerpolicy:
image.referrerpolicy } whenever the property exists on image rather than only
when it's truthy.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 75b3766e-f4cd-40e5-bee1-69a8285b3c7d
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (6)
src/components/card/card.tsxsrc/components/chip/chip.tsxsrc/components/list-item/list-item.e2e.tsxsrc/components/list-item/list-item.tsxsrc/global/shared-types/image.types.tssrc/util/image.template.tsx
| const referrerPolicyAttr = image.referrerpolicy | ||
| ? { referrerpolicy: image.referrerpolicy } | ||
| : {}; |
There was a problem hiding this comment.
referrerpolicy guard currently drops explicit empty-string values.
Using a truthy check means referrerpolicy: '' is treated as “unset”. If omission should only happen when the field is absent, check undefined explicitly.
Suggested fix
- const referrerPolicyAttr = image.referrerpolicy
+ const referrerPolicyAttr = image.referrerpolicy !== undefined
? { referrerpolicy: image.referrerpolicy }
: {};📝 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 referrerPolicyAttr = image.referrerpolicy | |
| ? { referrerpolicy: image.referrerpolicy } | |
| : {}; | |
| const referrerPolicyAttr = image.referrerpolicy !== undefined | |
| ? { referrerpolicy: image.referrerpolicy } | |
| : {}; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/image.template.tsx` around lines 24 - 26, The referrerPolicyAttr
guard treats an explicit empty-string referrerpolicy as unset because it uses a
truthy check; change the condition that builds referrerPolicyAttr to test
image.referrerpolicy !== undefined (or !== void 0) so that empty string and
other falsy but present values are preserved, i.e., retain the object {
referrerpolicy: image.referrerpolicy } whenever the property exists on image
rather than only when it's truthy.
|
we have a bunch of places we use the |
| * when `src` points to a third-party service that should not receive | ||
| * the originating page URL in the `Referer` header (e.g. external | ||
| * favicon or avatar services). When omitted, the attribute is not | ||
| * emitted and the browser's default referrer policy applies. |
There was a problem hiding this comment.
shouldn't we always default to 'no-referrer'?
There was a problem hiding this comment.
It would be the natural privacy-positive default, but it'd be observable across every ImageTemplate consumer (so a breaking change, not just a feature flip) — and lime-elements is general-purpose, where some consumer could be relying on Referer reaching the image host (hotlink-protected CDNs, server-side correlation).
Leaning toward keeping it opt-in here and letting consumers like lime-crm-components set 'no-referrer' explicitly on the Image objects they construct (the favicon and avatar use cases that motivated this). Happy to revisit as a separate breaking-change PR if we want to make the opinionated call design-system-wide.
There was a problem hiding this comment.
limel-file-viewer can also display images. perhaps it could use this feature too
civing
force-pushed
the
feat/image-referrerpolicy
branch
from
7d49196 to
7eb29c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/util/image.template.tsx (2)
26-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTruthy check drops explicit empty-string values.
The guard uses a truthy check, so
referrerpolicy: ''is treated as unset. If you want to preserve all explicitly-set values (including empty string), checkimage.referrerpolicy !== undefinedinstead.🤖 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 `@src/util/image.template.tsx` around lines 26 - 27, The current construction of referrerPolicyAttr uses a truthy check which treats an explicit empty string in image.referrerpolicy as unset; change the guard to test for undefined instead (e.g., image.referrerpolicy !== undefined) so that explicitly set values including '' are preserved when building referrerPolicyAttr for the Image referrerpolicy property.
33-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHardcoded
loading="lazy"ignores theImage.loadingfield.The
Imagetype exposes aloadingfield, butImageTemplatealways rendersloading="lazy". This makes the public contract misleading for consumers who setimage.loading = 'eager'.Either honor the field with
loading={image.loading ?? 'lazy'}or removeloadingfrom theImageinterface if it's not intended to be configurable.🤖 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 `@src/util/image.template.tsx` at line 33, The ImageTemplate currently hardcodes loading="lazy" which ignores the Image.loading field; update the JSX in ImageTemplate to use the image's loading value (e.g. set loading to image.loading ?? 'lazy') so consumers can pass 'eager' or 'lazy', and ensure the Image type/prop is used (ImageTemplate's image prop) accordingly; alternatively, if loading should not be configurable, remove loading from the Image interface—prefer the first option and update any callers/tests that assume the previous hardcoded behavior.
🤖 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 `@src/components/list-item/list-item.e2e.tsx`:
- Around line 114-131: Add end-to-end tests for limel-chip and limel-card
mirroring the list-item test to assert that the image.referrerpolicy is
forwarded: create tests named like "forwards referrerpolicy to the rendered
image when set" in the chip and card e2e files that render <limel-chip> and
<limel-card> with image={{ src: ..., alt: 'a', referrerpolicy: 'no-referrer' }},
wait for changes, select the rendered img via root.querySelector('img') and
assert imgEl?.getAttribute('referrerpolicy') === 'no-referrer'. Use the same
pattern and assertions as the existing list-item test to ensure ImageTemplate
integration is covered for all three components.
---
Duplicate comments:
In `@src/util/image.template.tsx`:
- Around line 26-27: The current construction of referrerPolicyAttr uses a
truthy check which treats an explicit empty string in image.referrerpolicy as
unset; change the guard to test for undefined instead (e.g.,
image.referrerpolicy !== undefined) so that explicitly set values including ''
are preserved when building referrerPolicyAttr for the Image referrerpolicy
property.
- Line 33: The ImageTemplate currently hardcodes loading="lazy" which ignores
the Image.loading field; update the JSX in ImageTemplate to use the image's
loading value (e.g. set loading to image.loading ?? 'lazy') so consumers can
pass 'eager' or 'lazy', and ensure the Image type/prop is used (ImageTemplate's
image prop) accordingly; alternatively, if loading should not be configurable,
remove loading from the Image interface—prefer the first option and update any
callers/tests that assume the previous hardcoded behavior.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ea80c9b9-1c5e-462b-8428-671b54304985
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (6)
src/components/card/card.tsxsrc/components/chip/chip.tsxsrc/components/list-item/list-item.e2e.tsxsrc/components/list-item/list-item.tsxsrc/global/shared-types/image.types.tssrc/util/image.template.tsx
| it('forwards referrerpolicy to the rendered image when set', async () => { | ||
| const imgSrc = | ||
| 'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///ywAAAAAAQABAAACAUwAOw=='; | ||
| const { root, waitForChanges } = await render( | ||
| <limel-list-item | ||
| text="Pic" | ||
| image={{ | ||
| src: imgSrc, | ||
| alt: 'a', | ||
| referrerpolicy: 'no-referrer', | ||
| }} | ||
| ></limel-list-item> | ||
| ); | ||
| await waitForChanges(); | ||
|
|
||
| const imgEl = root.querySelector('img'); | ||
| expect(imgEl?.getAttribute('referrerpolicy')).toEqual('no-referrer'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding similar e2e coverage for chip and card components.
List-item now has tests for both referrerpolicy branches, but chip and card lack similar assertions. Since the rendering logic is centralized in ImageTemplate, the risk is low, but adding parallel tests would strengthen confidence that all three components correctly integrate with the template.
🤖 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 `@src/components/list-item/list-item.e2e.tsx` around lines 114 - 131, Add
end-to-end tests for limel-chip and limel-card mirroring the list-item test to
assert that the image.referrerpolicy is forwarded: create tests named like
"forwards referrerpolicy to the rendered image when set" in the chip and card
e2e files that render <limel-chip> and <limel-card> with image={{ src: ..., alt:
'a', referrerpolicy: 'no-referrer' }}, wait for changes, select the rendered img
via root.querySelector('img') and assert imgEl?.getAttribute('referrerpolicy')
=== 'no-referrer'. Use the same pattern and assertions as the existing list-item
test to ensure ImageTemplate integration is covered for all three components.
7eb29c3 to
eeabe18
Compare
|
Thanks for the thorough review. Per-finding writeup of what landed in this push: Addressed
Extra in this push
Deferred (open to doing if you want)
Skipped (with rationale)
|
Add an optional `referrerpolicy?: ReferrerPolicy` field to the shared `Image` type. Lets consumers express that the rendered `<img>` should carry a specific referrer-policy attribute — useful when the `src` points to a third-party service that should not receive the originating page URL in the `Referer` header (e.g. external favicon or avatar services). This commit only widens the type. The components that accept `Image` do not yet honor the new field; that follows in subsequent commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add an `ImageTemplate` `FunctionalComponent` that returns an `<img>` JSX node with the `Image` fields applied — including the spread-cast workaround needed today for the `referrerpolicy` attribute (Stencil's `ImgHTMLAttributes` typing omits it; tracked upstream at stenciljs/core#6692). Lives in `src/util/image.template.tsx`. Internal helper, not exported from the public API. Honors `image.loading`, which was on the public `Image` type but silently ignored at every render site before this PR. Wired up to `limel-list-item`, `limel-chip`, and `limel-card` in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch the three components that accept an `Image`-shaped prop to delegate their `<img>` rendering to the shared `ImageTemplate` helper. This both removes the duplicated inline `<img>` JSX and lets the `referrerpolicy` field added to `Image` flow through to the rendered element — previously it was on the type but ignored at every render site. Adds two e2e cases on `limel-list-item` to verify the attribute is emitted when set and absent otherwise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`limel-file-viewer`'s image branch previously rendered its own inline `<img>` with hardcoded `loading="lazy"`. Switch it to the shared `ImageTemplate` helper used by `limel-card`, `limel-chip`, and `limel-list-item`, so the four `Image`-rendering surfaces in lime-elements share one rendering path. The component's `alt` prop is optional (`alt?: string`); the `Image` type's `alt` is required, so `this.alt ?? ''` covers the gap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Avatar URLs frequently point at third-party services (Gravatar, Microsoft Graph for Entra users, Google profile, etc.). By default, the browser sends the originating page URL in the `Referer` header on the cross-origin avatar fetch, leaking the tenant hostname to the avatar provider. Set `referrerpolicy="no-referrer"` directly on the `<img>` so the attribute applies regardless of which avatar source the consumer configures. Same spread-cast workaround as elsewhere — Stencil's `ImgHTMLAttributes` typing omits the attribute, tracked upstream at stenciljs/core#6692. `limec-profile-picture` does not consume the `Image` type (it carries its own `style` and `onError` plumbing), so it can't use the `ImageTemplate` helper; the policy is applied directly here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
civing
force-pushed
the
feat/image-referrerpolicy
branch
from
8da3939 to
807914c
Compare
3 participants
Summary
referrerpolicy?: ReferrerPolicyfield to the sharedImagetype.renderImg(image: Image)helper that returns the<img>JSX node, including the spread-cast workaround needed today forreferrerpolicy(Stencil'sImgHTMLAttributestyping omits it — see stenciljs/core ImgHTMLAttributes wherereferrerPolicyis declared onAnchorHTMLAttributesandIframeHTMLAttributesbut notImgHTMLAttributes).limel-list-item,limel-chip, andlimel-cardto render images via the helper. Removes the previously triplicated inline<img>JSX.limel-list-itemcovering the attribute-set and attribute-absent branches.When the
referrerpolicyfield is omitted, the attribute is not emitted — every existing consumer is unaffected.Why
Consumers that build an
Imagewhosesrcpoints to a third-party service (an external favicon endpoint, an external avatar service) currently have no way to suppress theRefererheader. By default the browser sends the full URL of the page that triggered the image load to the third party — which, for a CRM, leaks customer-relationship metadata (which tenant viewed which record while researching which company) to whoever runs the favicon service.Setting
referrerpolicy="no-referrer"removes the leak with zero functional cost: the image still loads, the third party still receives the request, but no originating-page URL is attached.Context (downstream consumers)
This change is the upstream prerequisite for the favicon work in
lime-crm-componentsPR #4122, which implements the company-favicons feature requested inhack-tuesdayissue #195. Security review of #4122 flagged that only one of four consumer surfaces could currently set the attribute; the other three render throughlimel-list-item/limel-chipand had no plumbing for it. This PR closes that plumbing gap.Commit structure
Three atomic commits, each independently buildable:
feat(image): add referrerpolicy to the Image interface— type + api report only.feat(image): add renderImg helper for shared image rendering— helper file added, unused.feat(card,chip,list-item): render Image via renderImg helper— wires the three components and adds e2e tests.Naming
The helper is
renderImg(notrenderImage) because bothlimel-list-itemandlimel-cardalready have a private method namedrenderImage— using the same name for the import would force readers to disambiguate every call site.renderImgpairs naturally with the<img>JSX tag and avoids the collision. Easy to rename later ifImgHTMLAttributesupstream gainsreferrerPolicyand the helper can be deleted.Test plan
npm run buildpasses on each of the three commits (verified locally).npm run api:updateproduces no diff beyond the documentedreferrerpolicyaddition.limel-list-itemcover both branches.referrerpolicyis unset.referrerpolicy="no-referrer"is emitted on the rendered<img>when set.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests