Skip to content

feat(image): support referrerpolicy on rendered images#4056

Merged
Kiarokh merged 8 commits into
mainfrom
feat/image-referrerpolicy
Jun 23, 2026
Merged

feat(image): support referrerpolicy on rendered images#4056
Kiarokh merged 8 commits into
mainfrom
feat/image-referrerpolicy

Conversation

@civing

@civing civing commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds an optional referrerpolicy?: ReferrerPolicy field to the shared Image interface (src/global/shared-types/image.types.ts) + the api report.
  • Adds an internal ImageTemplate functional component (src/util/image.template.tsx, @internal, not exported) that renders an <img> from an Image — forwarding src, alt, loading (image.loading ?? 'lazy'), and referrerPolicy via the typed Stencil prop (no cast/workaround). It also forwards optional style and onError for consumers that need to control presentation or react to a load failure.
  • Routes limel-card, limel-chip, limel-list-item, limel-file-viewer, and limel-profile-picture through ImageTemplate, removing each component's hand-rolled <img>.
  • limel-profile-picture and limel-file-viewer set referrerpolicy="no-referrer" (their images point at third-party avatar / file hosts).
  • Adds e2e coverage on limel-list-item (referrerpolicy set/absent, loading default/eager) and limel-profile-picture (no-referrer + object-fit style + image-error state).

When referrerpolicy is omitted, the attribute is not emitted — existing consumers are unaffected.

Why

When an image src points at a third-party service, the browser otherwise sends the originating CRM page URL in the Referer request 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 gives Image-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-components PR #4122 ("Company favicons"), which loads favicons from Google's S2 favicon service (a third party) and sets referrerpolicy via the Image objects it passes into limel-list-item / limel-chip / limel-card. Originating request: hack-tuesday #195.

Implementation notes

  • Helper naming. ImageTemplate (a FunctionalComponent) is named distinctly from the existing private renderImage methods on individual components, so the shared, stateless render path is unambiguous. It is @internal and not exported from the public API.
  • No typing workaround. referrerPolicy is the typed Stencil JSX prop, used directly with no cast — the installed @stencil/core (4.43.5) declares referrerPolicy on ImgHTMLAttributes (the fix for bug: JSXBase.ImgHTMLAttributes type missing referrerPolicy stenciljs/core#6692).
  • limel-profile-picture renders through ImageTemplate; the helper forwards its style binding (the --limel-profile-picture-object-fit custom property) and its onError handler, so behavior is unchanged.
  • limel-file-viewer switched its image branch to ImageTemplate (wrapped in a <Fragment>; passes alt: this.alt ?? '' since the component's alt is optional but Image.alt is required).

A note on limel-file-viewer and referrerpolicy

limel-file-viewer now renders images with referrerpolicy="no-referrer" unconditionally. We considered exposing a referrerPolicy prop 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, where no-referrer is a harmless no-op. We can add a referrerPolicy prop later if a consumer ever needs to override it.

Out of scope / follow-up

limel-card / limel-chip / limel-list-item render image.src without URL sanitization (pre-existing; not a script-execution vector for <img>, but inconsistent with limel-file-viewer). Tracked for a separate PR in #4143.

Commit structure

Eight commits:

  1. feat(image): add referrerpolicy to the Image interface — type + api report.
  2. chore(image): add ImageTemplate helper for shared image rendering — internal helper.
  3. refactor(card,chip,list-item): render Image via ImageTemplate helper — wires three components + e2e.
  4. refactor(file-viewer): render image via ImageTemplate helper.
  5. fix(profile-picture): suppress referrer on rendered <img>.
  6. refactor(image): forward style and onError through ImageTemplate — widens the helper.
  7. refactor(profile-picture): render avatar via ImageTemplate helper — migrates onto the widened helper + e2e.
  8. fix(file-viewer): suppress referrer on rendered <img>.

Test plan

  • e2e on limel-list-item: referrerpolicy present when set / absent when unset; loading defaults to lazy and honors an explicit eager.
  • e2e on limel-profile-picture: the avatar renders with no-referrer, lazy loading, and the object-fit style var; an image error flips the has-image-error host state.
  • npm run build passes; npm run api:update produces no diff beyond the documented referrerpolicy addition.
  • limel-card / limel-chip / limel-file-viewer are exercised through the same shared ImageTemplate path.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added support for referrer policy configuration on images to enhance privacy.
    • Enabled customizable image loading behavior (lazy vs. eager).
  • Refactor

    • Consolidated image rendering logic across components for improved consistency.
  • Tests

    • Added comprehensive tests to verify image attribute handling and behavior.

@coderabbitai

coderabbitai Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8195a045-092d-4f6a-b9d7-1077d5655859

📥 Commits

Reviewing files that changed from the base of the PR and between 9c1eb92 and 85b6a99.

📒 Files selected for processing (8)
  • src/components/card/card.tsx
  • src/components/chip/chip.tsx
  • src/components/file-viewer/file-viewer.tsx
  • src/components/list-item/list-item.e2e.tsx
  • src/components/list-item/list-item.tsx
  • src/components/profile-picture/profile-picture.e2e.tsx
  • src/components/profile-picture/profile-picture.tsx
  • src/util/image.template.tsx

📝 Walkthrough

Walkthrough

Multiple components (Card, Chip, ListItem, FileViewer) now delegate image rendering to a new shared ImageTemplate component. The Image type gains an optional referrerpolicy field. ProfilePicture adds direct referrerpolicy="no-referrer" support. E2E tests verify referrerpolicy is forwarded when set and omitted when unset.

Changes

Image Rendering Centralization

Layer / File(s) Summary
Image type contract
src/global/shared-types/image.types.ts
Added optional referrerpolicy?: ReferrerPolicy to exported Image interface with JSDoc guidance.
ImageTemplate component
src/util/image.template.tsx
New functional component rendering <img> with src, alt, loading (defaults to 'lazy'), and conditional referrerpolicy.
Card image delegation
src/components/card/card.tsx
Imports ImageTemplate and replaces inline <img> with <ImageTemplate image={this.image} /> in renderImage.
Chip image delegation
src/components/chip/chip.tsx
Imports ImageTemplate and replaces inline <img> with <ImageTemplate image={this.image} /> in renderPicture.
ListItem image delegation
src/components/list-item/list-item.tsx
Imports ImageTemplate and replaces inline <img> with <ImageTemplate image={this.image} /> in renderImage.
FileViewer image delegation
src/components/file-viewer/file-viewer.tsx
Adds Fragment import and refactors renderImage to return Fragment containing buttons and ImageTemplate with sanitized src and fallback alt.
ProfilePicture referrerpolicy
src/components/profile-picture/profile-picture.tsx
Imports ImageTemplate and updates renderAvatar() to use ImageTemplate with referrerpolicy="no-referrer".
ListItem referrerpolicy E2E tests
src/components/list-item/list-item.e2e.tsx
Extended existing test to assert loading="lazy" and referrerpolicy omission when unset; added new tests verifying attribute forwarding when set and custom loading attribute.
ProfilePicture E2E tests
src/components/profile-picture/profile-picture.e2e.tsx
New test suite verifying referrerpolicy="no-referrer", loading="lazy", CSS custom property application, and error event handling with class toggle.

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?>
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

Suggested labels

released

Suggested reviewers

  • jgroth
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(image): support referrerpolicy on rendered images' directly and precisely describes the main change: adding referrerpolicy support to the Image type and propagating it through image rendering.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/image-referrerpolicy

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

┌──────────────┐
│ Opengrep CLI │
└──────────────┘

�[32m✔�[39m �[1mOpengrep OSS�[0m
�[32m✔�[39m Basic security coverage for first-party code vulnerabilities.

[00.19][ERROR]: unable to find a config; path .coderabbit-opengrep-fallback.yml does not exist

src/components/card/card.tsx

┌──────────────┐
│ Opengrep CLI │
└──────────────┘

�[32m✔�[39m �[1mOpengrep OSS�[0m
�[32m✔�[39m Basic security coverage for first-party code vulnerabilities.

[00.26][ERROR]: unable to find a config; path .coderabbit-opengrep-fallback.yml does not exist

src/components/list-item/list-item.e2e.tsx

┌──────────────┐
│ Opengrep CLI │
└──────────────┘

�[32m✔�[39m �[1mOpengrep OSS�[0m
�[32m✔�[39m Basic security coverage for first-party code vulnerabilities.

[00.26][ERROR]: unable to find a config; path .coderabbit-opengrep-fallback.yml does not exist

  • 5 others

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4056/

@civing civing requested a review from a team as a code owner April 30, 2026 12:18
@civing civing force-pushed the feat/image-referrerpolicy branch 2 times, most recently from 0f35118 to 9fd2e0b Compare April 30, 2026 12:39
Comment thread etc/lime-elements.api.md
@adrianschmidt

Copy link
Copy Markdown
Contributor

Consolidated PR Review -- 6 Dimensions

Generated by a 6-agent review (backward compatibility, code quality, architecture, security, observability, performance) running in parallel against this PR's branch.

PR Summary

This PR adds an optional referrerpolicy?: ReferrerPolicy field to the shared Image type, introduces a renderImg(image: Image) helper centralising <img> JSX (with a documented spread-cast workaround for Stencil's typing gap upstream-tracked at stenciljs/core#6692), and switches limel-card, limel-chip, and limel-list-item to render via the helper. 7 files changed, +66/-9. Two new e2e tests on limel-list-item cover the attribute-set and attribute-absent branches. Unblocks lime-crm-components PR #4122 (company favicons), where security review flagged that 3 of 4 Image consumer surfaces had no plumbing for referrerpolicy.

Merge Readiness -- READY TO MERGE ✅

The PR is strictly better than main along every reviewed dimension: it adds capability without regressing emitted DOM, public types, or runtime cost, and it's the upstream prerequisite for a real information-disclosure fix downstream. No [High] findings from any agent.

  • Blockers: None.
  • Non-blocking but worth addressing: Two [Medium] items in Architecture worth picking up — helper placement (shared-types/ vs. src/util/ or a *.template.tsx colocation), and the silent image.loading drop the helper inherits from the inline JSX. See Top Recommendations.

1. Backward Compatibility -- SAFE ✅

Adding an optional field to a public interface and replacing inline <img> JSX with an internal helper is a clean, additive change. Emitted DOM is identical when referrerpolicy is unset; helper is intentionally not re-exported from src/interface.ts:63, so it stays internal and absent from etc/lime-elements.api.md.

  • [Low] renderImg has no @internal TSDoc tag (src/global/shared-types/image.tsx:13). It's not surfaced today because interface.ts only re-exports image.types, but a future export * over shared-types/ would leak it. Adding @internal would make the contract explicit.
  • [Low] Co-locating image.tsx next to image.types.ts creates two same-base-name files differing only by extension (card.tsx:12, chip.tsx:11, list-item.tsx:11 import via the bare specifier '../../global/shared-types/image'). Resolves unambiguously today, but mildly confusing for maintainers.
  • [Low] Image.loading was unused pre-PR and remains unused (helper hardcodes loading="lazy"). Pre-existing; not a regression. (Also flagged stronger by Architecture.)

Positives: Strictly additive public-type change. card's <div class="image-wrapper"> preserved at card.tsx:245 so existing card.scss:69 selectors keep matching. The "absent" assertion at list-item.e2e.tsx:111 guards the no-emission contract that protects existing consumers. ReferrerPolicy is a global from lib.dom.d.ts (in tsconfig lib), so no downstream typing setup needed. Three atomic commits make any future bisect/revert clean.

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 'no-referrer'.

  • [Low] image.tsx:27 casts to Record<string, string>, which is wider than necessary and discards the ReferrerPolicy union typing. A precise Record<'referrerpolicy', ReferrerPolicy> (or a typed local) would suppress the Stencil typing mismatch without pretending the value is arbitrary.
  • [Low] Truthy check on image.referrerpolicy (image.tsx:18) skips emission for '', which is technically a valid ReferrerPolicy literal meaning "use default". Functionally equivalent to omitting the attribute, so not a bug — but the JSDoc on image.types.ts:31 could note it.
  • [Low] Helper-level test coverage lives in list-item.e2e.tsx only; card and chip got no new tests. A co-located src/global/shared-types/image.spec.tsx (matching the src/util/*.spec.ts convention) would more accurately reflect where the logic lives. Current coverage is adequate.
  • [Low] Helper location: shared-types/ previously held only *.types.ts. Putting a JSX helper there blurs the directory's purpose.
  • [Low] Naming: renderImg resolves the real collision with private renderImage methods in card.tsx:240 and list-item.tsx:259, but reads slightly less consistently than the rest of the codebase. renderImageElement or an alias-at-import would also work.

Positives: Eliminates real triplication (card.tsx, chip.tsx, list-item.tsx previously each had their own <img> JSX). The Stencil-typing escape hatch is isolated to one call site and clearly comment-flagged with the upstream issue link, so it's removable when typing lands. The "absent" e2e assertion is a useful regression guard. API report (etc/lime-elements.api.md:1240) updated, making the type extension a deliberate public-API change rather than drift.

3. Architecture -- ACCEPTABLE ⚠️

The refactor is sound for the three call sites (verified to be the only Image-typed surfaces in this repo). Main concerns are the helper's directory placement and one missed opportunity to fix a pre-existing type/behavior mismatch.

  • [Medium] Helper placement violates the directory's intent. src/global/shared-types/image.tsx is the only .tsx/JSX-emitting file in shared-types/ (every sibling — color.types.ts, icon.types.ts, file.types.ts, etc. — is a pure type module) and the first file there to import @stencil/core, turning a type-only module into a runtime-bearing one. Two existing conventions fit better: src/components/<x>/<x>.template.tsx exporting a FunctionalComponent (see checkbox/checkbox.template.tsx, select/select.template.tsx) or src/util/ (already referenced from interface.ts:68 via image-resize). A FunctionalComponent would also let consumers write <ImageTemplate image={this.image} /> instead of {renderImg(this.image)}, which is more idiomatic Stencil.
  • [Medium] renderImg silently drops image.loading. Image.loading?: 'lazy' | 'eager' is documented on the public type (image.types.ts:22), but image.tsx:26 hardcodes loading="lazy". Pre-existing in the inline JSX, so not a regression — but a centralising PR was the natural place to fix the type/behavior mismatch. Either honour image.loading ?? 'lazy' or remove loading from the interface.
  • [Low] Spread-cast pattern won't scale gracefully — a fifth attribute (crossorigin, decoding, fetchpriority) needing the same workaround would duplicate. A buildOptionalAttrs(image) helper or single accumulating object would be more extensible. Not blocking.
  • [Low] Inconsistent "is this image renderable?" guards: card.tsx:241 uses !this.image?.src, chip.tsx:298 uses !isEmpty(this.image) (lodash), list-item.tsx:260 uses !this.image. Pre-existing; the helper could absorb the guard (return null when !image?.src) and normalize the contract.
  • [Low] Public-vs-internal export depends on interface.ts enumerating files rather than a structural barrier. src/util/ or *.template.tsx would make the boundary structural.

Positives: Correctly identifies all Image-typed surfaces in the repo (other <img> uses in profile-picture.tsx, file-viewer.tsx, email-viewer/* are on non-Image-typed inputs and rightly excluded). Wrapping decisions stay at the call site (card's image-wrapper is a card-specific layout concern). DRYs up triplication without taking over component-level conditional-render decisions.

4. Security -- SAFE ✅

This PR is itself a security improvement: it adds the plumbing for downstream consumers to suppress Referer leakage for <img> elements pointing at third-party services. No new vulnerabilities introduced.

  • [Low] Runtime values of image.referrerpolicy are not validated (the ReferrerPolicy constraint is TS-only). Browsers ignore unknown tokens per spec — no XSS surface — but consumers passing untrusted strings should know an invalid value silently equals "no leak protection."
  • [Low] Only list-item has e2e assertions for both branches; card and chip have none. A future change to renderImg could regress those silently. Defense-in-depth.

Spread-cast attribute-injection check: referrerPolicyAttr is a fresh object literal with one hard-coded key (image.tsx:18-20). JSX spread of a fresh object literal enumerates only own properties, so attacker-controlled image.referrerpolicy cannot inject sibling attributes (onerror, src, etc.) even under hypothetical prototype pollution. The as Record<string, string> cast is type-only. Default-behavior verification: When referrerpolicy is omitted, the helper produces {} and spreads nothing, so the <img> is emitted with exactly the same attributes as before this PR. No change to default leakage potential — matching the PR's stated intent.

Positives: Centralizing forwarding gives one place for future security-relevant attributes (crossorigin, decoding) to land. Conservative emission avoids any change to existing rendered output. Truthy guard correctly handles ''/null/undefined. Unblocks lime-crm-components #4122 to suppress Referer leakage to Google's favicon endpoint — a concrete information-disclosure improvement.

5. Observability -- SAFE ✅

Pure UI plumbing: synchronous JSX-emitting helper with no failure modes, no I/O, no async behavior. Nothing meaningful to log, instrument, or correlate; no observability regression introduced. lime-elements has no project-wide logger/telemetry module the helper could be bypassing by living in shared-types/.

  • [Low] None of the three call sites wire onError/onLoad on the rendered <img> (card.tsx:240-246, chip.tsx:291-300, list-item.tsx:259-265), so a 404, CSP block, or no-referrer rejection still fails silently. Pre-existing — verified the inline JSX had the same shape — and renderImg is now the single right place to add broken-image telemetry if Lime ever wants it.
  • [Low] renderImg doesn't defensively handle image == null. All current call sites guard, but a future caller forgetting the guard would get a TypeError at render time with no helper-level message. A one-line if (!image?.src) return null; would make misuse self-diagnosing.

Positives: New e2e tests at list-item.e2e.tsx:108-131 give a clean regression signal in both directions. Inline comment on the spread-cast at image.tsx:14-17 documents why and links the upstream Stencil issue.

6. Performance -- SAFE ✅

Near-zero-cost refactor. Bundle, tree-shaking, and JSX equivalence all check out. Per-render cost is one extra small object allocation + spread per <img> rendered, dwarfed by the vdom allocation Stencil already does for the same element.

  • [Low] renderImg allocates a {} (or { referrerpolicy }) per call (image.tsx:18-27). Each limel-list-item is its own Stencil component, so a list of N items produces N calls only when those items actually re-render — not a hot inner loop within a single parent render. Not measurable; don't optimize.
  • [Low] The empty-object branch churns a fresh {} each call in the common case. A module-level const EMPTY = {} would reuse the literal, but this is textbook micro-optimization.

Bundle/tree-shaking checks (all clean): import { h } from '@stencil/core' is already in every Stencil component bundle, so zero new runtime dependency. The Image type is type-only and compiles away — no runtime import of image.types.ts is introduced. renderImg is a single named export with no side effects, so tree-shakers strip it for non-importers. JSX output is structurally equivalent to the previous inline JSX (card's <div class="image-wrapper"> is retained at card.tsx:245).

Positives: Centralisation gives one place to tune loading/referrerpolicy in the future. Helper is invoked after the early-return for missing image, so absent images cost nothing extra.


Overall Verdicts

Dimension Verdict
Backward Compatibility SAFE
Code Quality GOOD
Architecture ACCEPTABLE
Security SAFE
Observability SAFE
Performance SAFE
Merge Readiness READY TO MERGE

Top Recommendations

  1. [Medium from Architecture] Move renderImg out of src/global/shared-types/. The directory previously held only *.types.ts files; introducing JSX + a @stencil/core runtime import there is the first crossing of that line. Two existing conventions fit: src/util/render-image.tsx (matches image-resize re-exported via interface.ts:68) or a *.template.tsx colocated near a consumer (matches checkbox/checkbox.template.tsx). The *.template.tsx form would also let it become a FunctionalComponent and be invoked as <ImageTemplate image={this.image} />, which is more idiomatic Stencil.
  2. [Medium from Architecture] Decide what Image.loading means. The field is documented on the public type (image.types.ts:22) but image.tsx:26 hardcodes loading="lazy" and ignores it (the inline JSX did the same — so this PR doesn't regress, but it's the natural place to fix). Either loading={image.loading ?? 'lazy'} in the helper, or remove loading from the public Image interface.
  3. [Low from Code Quality] Tighten the cast at image.tsx:27: replace as Record<string, string> with Record<'referrerpolicy', ReferrerPolicy> (or a small typed local) so the ReferrerPolicy union typing isn't widened to arbitrary strings.
  4. [Low from Backward Compatibility] Add an @internal TSDoc tag on renderImg (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 on interface.ts not enumerating the file.
  5. [Low from Code Quality] Mention in the JSDoc on image.types.ts:31 that referrerpolicy: '' is treated the same as omitting the attribute, since the helper's truthy check skips emission for '' (a technically-valid ReferrerPolicy literal). Alternatively, change the helper guard to image.referrerpolicy !== undefined for strict pass-through.
  6. [Low from Observability] Optionally add if (!image?.src) return null; at the top of renderImg so a future caller forgetting the call-site guard gets a self-diagnosing no-op instead of a TypeError. All current callers already guard, so this is purely defensive.
  7. [Low from Security] Consider mirroring the attribute-presence/absence assertions to card and chip e2e tests for defense-in-depth, so a future change to renderImg can't silently regress the no-emission contract on those two consumers.

🤖 Generated by /6-agent-review.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 553ff14 and 7d49196.

⛔ Files ignored due to path filters (1)
  • etc/lime-elements.api.md is excluded by !etc/lime-elements.api.md
📒 Files selected for processing (6)
  • src/components/card/card.tsx
  • src/components/chip/chip.tsx
  • src/components/list-item/list-item.e2e.tsx
  • src/components/list-item/list-item.tsx
  • src/global/shared-types/image.types.ts
  • src/util/image.template.tsx

Comment thread src/util/image.template.tsx Outdated
Comment thread src/util/image.template.tsx Outdated
@Kiarokh

Kiarokh commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

we have a bunch of places we use the <img tag in the Building Blocks too. I think updating them would be a really nice next step. That'll also reduce the concerns (in the favicons PR) that initiated this PR 👍

* 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we always default to 'no-referrer'?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

limel-file-viewer can also display images. perhaps it could use this feature too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@civing civing force-pushed the feat/image-referrerpolicy branch from 7d49196 to 7eb29c3 Compare May 11, 2026 09:43

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/util/image.template.tsx (2)

26-27: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Truthy 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), check image.referrerpolicy !== undefined instead.

🤖 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 win

Hardcoded loading="lazy" ignores the Image.loading field.

The Image type exposes a loading field, but ImageTemplate always renders loading="lazy". This makes the public contract misleading for consumers who set image.loading = 'eager'.

Either honor the field with loading={image.loading ?? 'lazy'} or remove loading from the Image interface 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

📥 Commits

Reviewing files that changed from the base of the PR and between 7d49196 and 7eb29c3.

⛔ Files ignored due to path filters (1)
  • etc/lime-elements.api.md is excluded by !etc/lime-elements.api.md
📒 Files selected for processing (6)
  • src/components/card/card.tsx
  • src/components/chip/chip.tsx
  • src/components/list-item/list-item.e2e.tsx
  • src/components/list-item/list-item.tsx
  • src/global/shared-types/image.types.ts
  • src/util/image.template.tsx

Comment on lines +114 to 131
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');
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

@civing civing force-pushed the feat/image-referrerpolicy branch from 7eb29c3 to eeabe18 Compare May 11, 2026 10:05
@civing

civing commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. Per-finding writeup of what landed in this push:

Addressed

  • Architecture 1 (helper placement) — moved to src/util/image.template.tsx, converted to ImageTemplate FunctionalComponent, invoked as <ImageTemplate image={...} />.
  • Architecture 2 (image.loading dropped) — helper now honors image.loading ?? 'lazy'. The field was on the public Image type but silently ignored across all three render sites before this PR.
  • Backward Compatibility 1 / @internal tag — added on ImageTemplate.
  • Code Quality (cast tightening) — variable typed as { referrerpolicy?: Image['referrerpolicy'] }; spread-site cast kept as the minimum needed to satisfy Stencil's missing JSX typing.
  • Commit types (from your line comment) — commit 2 → chore, commit 3 → refactor; helper rename renderImgImageTemplate reflected in messages.

Extra in this push

  • 4th commit: refactor(file-viewer) switches limel-file-viewer's image branch to ImageTemplate. Same shape applies cleanly. profile-picture was considered but skipped — it carries style, onError, and custom logic that doesn't fit the helper's contract.

Deferred (open to doing if you want)

  • Card/chip e2e parity for the attribute-set/absent branches (list-item already has both; the helper is now the single rendering site so coverage is transitive).
  • Defensive if (!image?.src) return null; in the helper.
  • JSDoc note that referrerpolicy: '' is treated as omitted.

Skipped (with rationale)

  • Runtime ReferrerPolicy value validation — browsers ignore unknown tokens per spec; silent fallback is reasonable.
  • onError/onLoad telemetry — pre-existing absence across all sites; the helper is the right place to add later if you ever want it.
  • Allocation micro-optimization — per "don't optimize" guidance.
  • buildOptionalAttrs factoring — single attribute today; revisit if a second needs the same workaround.

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>
@Kiarokh Kiarokh force-pushed the feat/image-referrerpolicy branch from 807914c to 0e5545b Compare June 22, 2026 11:39
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>
@Kiarokh Kiarokh self-assigned this Jun 22, 2026
@Kiarokh Kiarokh force-pushed the feat/image-referrerpolicy branch from 0e5545b to 9c1eb92 Compare June 22, 2026 12:47
civing and others added 3 commits June 23, 2026 09:54
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>
Kiarokh and others added 3 commits June 23, 2026 10:07
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>
@Kiarokh Kiarokh force-pushed the feat/image-referrerpolicy branch from 424c50c to 85b6a99 Compare June 23, 2026 08:07
@Kiarokh

Kiarokh commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-generated review from 6 parallel agents (re-review, updated after the fixes pushed since). Treat as input, not a verdict — agents can be wrong or miss context.

Consolidated PR Review — updated

This 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 [High]. Both [Medium] findings from the prior review are resolved; what remains is optional [Low] nits or a tracked follow-up.

Resolved since the previous review

  • 🟡 → ✅ Unused class prop on ImageTemplate (Architecture) — removed; the helper now exposes only image + style + onError.
  • 🟡 → ✅ file-viewer's unconditional no-referrer (Backward Compatibility) — kept as a deliberate, privacy-positive default and documented in the PR body (including why Lime CRM's SharePoint paths are unaffected). An escape-hatch prop can be added later if a consumer needs it.
  • 🔵 → ✅ style prop type widened to { [key: string]: string | undefined } to match Stencil's <img style> type.
  • 🔵 → ✅ Test coverage — added a limel-profile-picture e2e (no-referrer + object-fit style var + image-error state) that exercises the migration's style/onError forwarding, which previously had no direct test.

Dimensions

1. Backward Compatibility — SAFE ✅

What works well: the Image change is purely additive and matches the api report; ImageTemplate is confirmed @internal and unexported; the profile-picture migration is output-equivalent, and file-viewer's array→<Fragment> switch is DOM-equivalent.

Issues: None.

Minor nits (1)
  • 🔵 file-viewer's renderImage now returns a <Fragment> while sibling renderPdf/renderText/etc. still return arrays — cosmetic inconsistency, output unchanged.

2. Code Quality — GOOD ✅

What works well: the typed referrerPolicy (no cast) is correct for 4.43.5; onError matches Stencil's DOMAttributes.onError; the migration is leftover-free; the new tests cover the headline behavior on two surfaces.

Issues: None.

Minor nits (1)
  • 🔵 profile-picture relies on the helper's loading ?? 'lazy' default (it dropped its explicit loading="lazy"); now guarded by the new profile-picture e2e plus the list-item default-loading test.

3. Architecture — GOOD ✅

What works well: the abstraction is now complete across all five consumers, with a principled split — image data on Image, host presentation (style/onError) on the internal ImageTemplateProps; src/util/ placement is justified for a shared helper.

Issues: None.

Minor nits (2)
  • 🔵 the style passthrough could be documented as "CSS custom properties only" to keep it a variable-injection seam rather than a generic style hatch.
  • 🔵 extensibility is "add-a-prop"; a typed Pick<JSXBase.ImgHTMLAttributes> passthrough would scale better only if the forwarded-attribute list grows — future-only.

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 sanitizeUrl still running before the Image is built; the only style ever passed is a 'cover'|'contain' literal.

Issues: None.

Minor nits (2)
  • 🔵 the style passthrough is a theoretical CSS-exfiltration seam; not real today (internal helper, literal-only callers) — a one-line "trusted values only" comment would document the boundary.
  • 🔵 limel-card / limel-chip / limel-list-item render image.src without URL sanitization (pre-existing; not a script-execution vector for <img>, but inconsistent with limel-file-viewer). Tracked for a separate PR in Sanitize image src URLs in the shared ImageTemplate (defense-in-depth) #4143.

5. Observability — GOOD ✅

What works well: profile-picture's error observability is preserved through the onError passthrough (drives has-image-error / popover); no console.* added; src/page-URL never logged; the helper stays out of public types.

Issues: None.

Minor nits (1)

6. Performance — SAFE ✅

What works well: performance-neutral vs main — the functional component is inlined (no wrapper element, no re-mount that would re-fetch images), the N-items hot path passes the stable this.image prop, and lazy loading is preserved everywhere.

Issues: None.

Minor nits (1)
  • 🔵 profile-picture/file-viewer build a transient image literal per render — immaterial (each renders a single image, not on an N-items loop).

Top Recommendations

None outstanding. The two [Medium] findings are resolved (see Resolved since the previous review), and the remaining [Low] items are optional or tracked in #4143.

@Kiarokh Kiarokh enabled auto-merge (rebase) June 23, 2026 08:30
@Kiarokh Kiarokh merged commit 146b0cd into main Jun 23, 2026
17 checks passed
@Kiarokh Kiarokh deleted the feat/image-referrerpolicy branch June 23, 2026 08:30
@lime-opensource

Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 39.35.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants