feat(image): support referrerpolicy on rendered images#4056
feat(image): support referrerpolicy on rendered images#4056
Conversation
📝 WalkthroughWalkthroughMultiple components (Card, Chip, ListItem) now delegate image rendering to a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4056/ |
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>
civing
force-pushed
the
feat/image-referrerpolicy
branch
from
553ff14 to
789aadc
Compare
Add a `renderImg(image: Image)` helper 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). Internal helper, not exported from the public API. 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 `renderImg` 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>
civing
force-pushed
the
feat/image-referrerpolicy
branch
from
0f35118 to
9fd2e0b
Compare
There was a problem hiding this comment.
The second commit seems like it should be a chore, and the third commit a refactor? Otherwise this looks very good. I'll post a full review in a comment.
There was a problem hiding this comment.
The second commit seems like it should be a
chore, and the third commit arefactor? Otherwise this looks very good. I'll post a full review in a comment.
I've noticed that Claude has a different understanding of the feat and fix. It considered only Bug fixes as fix, (and marks "problems that are now fixed" as feat)
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.
| <img | ||
| src={image.src} | ||
| alt={image.alt} | ||
| loading="lazy" | ||
| {...(referrerPolicyAttr as Record<string, string>)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Honor Image.loading in the shared renderer (or remove it from the contract).
Image exposes a loading field, but ImageTemplate hardcodes "lazy". This makes the public type misleading for consumers.
Suggested fix
- loading="lazy"
+ loading={image.loading ?? 'lazy'}📝 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.
| <img | |
| src={image.src} | |
| alt={image.alt} | |
| loading="lazy" | |
| {...(referrerPolicyAttr as Record<string, string>)} | |
| <img | |
| src={image.src} | |
| alt={image.alt} | |
| loading={image.loading ?? 'lazy'} | |
| {...(referrerPolicyAttr as Record<string, string>)} |
🤖 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 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.
|
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.
limel-file-viewer can also display images. perhaps it could use this feature too
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