feat(image): support referrerpolicy on rendered images#4056
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughMultiple components (Card, Chip, ListItem, FileViewer) now delegate image rendering to a new shared ChangesImage Rendering Centralization
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) | ⏱️ ~25 minutes Possibly related issues
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 docstrings
🧪 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. 🔧 OpenGrep (1.23.0)src/util/image.template.tsx┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path src/components/card/card.tsx┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.26][ERROR]: unable to find a config; path src/components/list-item/list-item.e2e.tsx┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.26][ERROR]: unable to find a config; path
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/ |
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.
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
|
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
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)
|
8da3939 to
807914c
Compare
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>
807914c to
0e5545b
Compare
Add an `ImageTemplate` `FunctionalComponent` that returns an `<img>` JSX node with the `Image` fields applied (`src`, `alt`, `loading`, `referrerpolicy`). 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>
0e5545b to
9c1eb92
Compare
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. `limel-profile-picture` keeps its own `<img>` instead of rendering through the `ImageTemplate` helper. The template forwards only the `Image` data fields (`src`, `alt`, `loading`, `referrerpolicy`), while this component also needs a `style` binding (the `--limel-profile-picture-object-fit` custom property) and an `onError` handler. Those are presentation/behaviour concerns that do not belong on the `Image` data type, so reusing the helper would mean widening its prop surface to forward them — deferred for now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Widen `ImageTemplateProps` with optional `style` and `onError`, forwarded to the rendered `<img>`. Existing callers (`limel-card`, `limel-chip`, `limel-list-item`, `limel-file-viewer`) pass neither, so their output is unchanged. This lets a consumer that also needs to style the element or react to a load failure render through the shared helper instead of hand-rolling its own `<img>` — used by `limel-profile-picture` in the next commit. The helper stays `@internal`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now that `ImageTemplate` forwards `style` and `onError` (previous commit), `limel-profile-picture` renders its avatar through the shared helper instead of a hand-rolled `<img>`. This supersedes the standalone element added earlier in this PR, so the referrer-policy and lazy-loading handling lives in one place. Behaviour is unchanged: the avatar still emits `referrerpolicy="no-referrer"`, defaults to lazy loading, keeps the `--limel-profile-picture-object-fit` style binding, and wires the existing `onError` handler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`limel-file-viewer` renders remote image URLs (its own examples load from unsplash.it / picsum.photos), and by default the browser sends the originating webclient page URL in the `Referer` header on that cross-origin fetch — leaking it to whoever hosts the file. Set `referrerpolicy="no-referrer"` on the image it builds, matching the policy already applied to `limel-profile-picture`. Same-origin files are unaffected; behaviour is otherwise unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
424c50c to
85b6a99
Compare
Consolidated PR Review — updatedThis updates the earlier 6-agent re-review to reflect the changes pushed since. The branch is now 8 commits at the current tip. Merge Readiness — READY TO MERGE ✅All six dimensions are SAFE/GOOD with no Resolved since the previous review
Dimensions1. Backward Compatibility — SAFE ✅What works well: the Issues: None. Minor nits (1)
2. Code Quality — GOOD ✅What works well: the typed Issues: None. Minor nits (1)
3. Architecture — GOOD ✅What works well: the abstraction is now complete across all five consumers, with a principled split — image data on Issues: None. Minor nits (2)
4. Security — SAFE ✅What works well: the privacy mechanism is verified end-to-end by e2e (present/absent cases at runtime); file-viewer's leak is closed with Issues: None. Minor nits (2)
5. Observability — GOOD ✅What works well: profile-picture's error observability is preserved through the Issues: None. Minor nits (1)
6. Performance — SAFE ✅What works well: performance-neutral vs Issues: None. Minor nits (1)
Top RecommendationsNone outstanding. The two |
|
🎉 This PR is included in version 39.35.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
referrerpolicy?: ReferrerPolicyfield to the sharedImageinterface (src/global/shared-types/image.types.ts) + the api report.ImageTemplatefunctional component (src/util/image.template.tsx,@internal, not exported) that renders an<img>from anImage— forwardingsrc,alt,loading(image.loading ?? 'lazy'), andreferrerPolicyvia the typed Stencil prop (no cast/workaround). It also forwards optionalstyleandonErrorfor consumers that need to control presentation or react to a load failure.limel-card,limel-chip,limel-list-item,limel-file-viewer, andlimel-profile-picturethroughImageTemplate, removing each component's hand-rolled<img>.limel-profile-pictureandlimel-file-viewersetreferrerpolicy="no-referrer"(their images point at third-party avatar / file hosts).limel-list-item(referrerpolicy set/absent, loading default/eager) andlimel-profile-picture(no-referrer + object-fit style + image-error state).When
referrerpolicyis omitted, the attribute is not emitted — existing consumers are unaffected.Why
When an image
srcpoints at a third-party service, the browser otherwise sends the originating CRM page URL in theRefererrequest header — leaking customer-relationship metadata (which tenant viewed which record while researching which company) to whoever runs that service.referrerpolicy="no-referrer"removes the leak at no functional cost: the image still loads, the third party still receives the request, but no originating-page URL is attached. This PR givesImage-shaped props a first-class way to opt into that, and makes the affected components honor it.Context (downstream consumers)
Upstream prerequisite for
lime-crm-componentsPR #4122 ("Company favicons"), which loads favicons from Google's S2 favicon service (a third party) and setsreferrerpolicyvia theImageobjects it passes intolimel-list-item/limel-chip/limel-card. Originating request:hack-tuesday#195.Implementation notes
ImageTemplate(aFunctionalComponent) is named distinctly from the existing privaterenderImagemethods on individual components, so the shared, stateless render path is unambiguous. It is@internaland not exported from the public API.referrerPolicyis the typed Stencil JSX prop, used directly with no cast — the installed@stencil/core(4.43.5) declaresreferrerPolicyonImgHTMLAttributes(the fix for bug:JSXBase.ImgHTMLAttributestype missingreferrerPolicystenciljs/core#6692).limel-profile-picturerenders throughImageTemplate; the helper forwards itsstylebinding (the--limel-profile-picture-object-fitcustom property) and itsonErrorhandler, so behavior is unchanged.limel-file-viewerswitched its image branch toImageTemplate(wrapped in a<Fragment>; passesalt: this.alt ?? ''since the component'saltis optional butImage.altis required).A note on
limel-file-viewerandreferrerpolicylimel-file-viewernow renders images withreferrerpolicy="no-referrer"unconditionally. We considered exposing areferrerPolicyprop as an escape hatch but skipped it for now: the main consumer (Lime CRM) does not render SharePoint/third-party images through file-viewer's<img>path — SharePoint Live Docs previews use an<iframe>, thumbnails render via building-blocks' own<img>, and the file URLs file-viewer does render are same-origin CRM links, whereno-referreris a harmless no-op. We can add areferrerPolicyprop later if a consumer ever needs to override it.Out of scope / follow-up
limel-card/limel-chip/limel-list-itemrenderimage.srcwithout URL sanitization (pre-existing; not a script-execution vector for<img>, but inconsistent withlimel-file-viewer). Tracked for a separate PR in #4143.Commit structure
Eight commits:
feat(image): add referrerpolicy to the Image interface— type + api report.chore(image): add ImageTemplate helper for shared image rendering— internal helper.refactor(card,chip,list-item): render Image via ImageTemplate helper— wires three components + e2e.refactor(file-viewer): render image via ImageTemplate helper.fix(profile-picture): suppress referrer on rendered <img>.refactor(image): forward style and onError through ImageTemplate— widens the helper.refactor(profile-picture): render avatar via ImageTemplate helper— migrates onto the widened helper + e2e.fix(file-viewer): suppress referrer on rendered <img>.Test plan
limel-list-item:referrerpolicypresent when set / absent when unset;loadingdefaults tolazyand honors an expliciteager.limel-profile-picture: the avatar renders withno-referrer, lazy loading, and the object-fit style var; an imageerrorflips thehas-image-errorhost state.npm run buildpasses;npm run api:updateproduces no diff beyond the documentedreferrerpolicyaddition.limel-card/limel-chip/limel-file-viewerare exercised through the same sharedImageTemplatepath.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests