fix(fuselage-ui-kit): Images stuck in loading state when cached#39481
Open
shrijatewari wants to merge 2 commits intoRocketChat:developfrom
Open
fix(fuselage-ui-kit): Images stuck in loading state when cached#39481shrijatewari wants to merge 2 commits intoRocketChat:developfrom
shrijatewari wants to merge 2 commits intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 0e71690 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Contributor
WalkthroughFixes ImageBlock image-load race by handling cached images synchronously and ensuring load handlers run for non-cached images; also adds an EJS template to livechat's HtmlWebpackPlugin and adjusts yarn config (engines plugin commented out). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
shrijatewari
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
- Temporarily disable engines plugin in .yarnrc.yml for Node version compatibility - Add template path to livechat webpack config for proper index.html generation Made-with: Cursor
…ageBlock When an image was cached, setting img.src could trigger the load event synchronously before the event listener was properly attached. This caused images in UIKit blocks to never update their loading state, leaving them stuck in skeleton state. The fix checks if the image is already complete before setting src, and only attaches the event listener if the image hasn't loaded yet. This prevents the race condition where: - Cached images fire load event immediately - Event listener might not be attached yet - State never updates, leaving skeleton visible Tested with: - Fresh images (not cached) - Cached images (from browser cache) - Rapid URL changes All scenarios now work correctly. Made-with: Cursor
shrijatewari
force-pushed
the
fix/imageblock-cached-image-race-condition
branch
from
68073c4 to
0e71690
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/fuselage-ui-kit/src/blocks/ImageBlock.tsx (1)
50-56: Drop the inline implementation comments.These comments just narrate the control flow and go against the repo rule for TS/JS implementation files.
As per coding guidelines "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fuselage-ui-kit/src/blocks/ImageBlock.tsx` around lines 50 - 56, Remove the inline implementation comments in ImageBlock.tsx: delete the two comment lines immediately above the img.complete check and the subsequent load handling so the block reads directly as the conditional that checks img.complete and calls setState(fetchImageState(img)) and then falls through to the load event handling; keep behavior intact (no logic changes to the img.complete branch, setState(fetchImageState(img)), or the load event wiring).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.yarnrc.yml:
- Around line 11-14: Uncomment the temporary development change by restoring the
plugins block (the commented "plugins:" entry and its child path/spec lines that
reference `@yarnpkg/plugin-engines`) so the engines plugin is enabled in shared
config, and set enableImmutableInstalls: true (replace the current
enableImmutableInstalls: false) to enforce immutable installs; also ensure the
Yarn version is pinned via Corepack in package.json so immutable installs are
reproducible.
In `@packages/fuselage-ui-kit/src/blocks/ImageBlock.tsx`:
- Around line 48-61: Remove the inline explanatory comments about a race
condition around the image loading logic; keep the existing behavior that sets
img.src = block.imageUrl, checks img.complete and calls
setState(fetchImageState(img)), and registers handleLoad via
img.addEventListener('load', handleLoad). Specifically delete the comment lines
immediately above the img.complete check and above the addEventListener call
(the comments referencing the race condition), leaving functions and event
handling (handleLoad, fetchImageState, setState, img.complete,
img.addEventListener) unchanged; optionally you may reorder to attach the
listener before assigning img.src but do not modify the logic.
---
Nitpick comments:
In `@packages/fuselage-ui-kit/src/blocks/ImageBlock.tsx`:
- Around line 50-56: Remove the inline implementation comments in
ImageBlock.tsx: delete the two comment lines immediately above the img.complete
check and the subsequent load handling so the block reads directly as the
conditional that checks img.complete and calls setState(fetchImageState(img))
and then falls through to the load event handling; keep behavior intact (no
logic changes to the img.complete branch, setState(fetchImageState(img)), or the
load event wiring).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2603226b-4c2c-493f-bfed-2203fd8cbbc6
📒 Files selected for processing (4)
.changeset/fix-imageblock-cached-image-race.md.yarnrc.ymlpackages/fuselage-ui-kit/src/blocks/ImageBlock.tsxpackages/livechat/webpack.config.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/livechat/webpack.config.tspackages/fuselage-ui-kit/src/blocks/ImageBlock.tsx
🧠 Learnings (2)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/livechat/webpack.config.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/livechat/webpack.config.ts
Comment on lines
+11
to
+14
| # Temporarily disabled engines plugin for development | ||
| # plugins: | ||
| # - path: .yarn/plugins/@yarnpkg/plugin-engines.cjs | ||
| # spec: "https://raw.githubusercontent.com/devoto13/yarn-plugin-engines/main/bundles/%40yarnpkg/plugin-engines.js" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .yarnrc.yml =="
sed -n '1,25p' .yarnrc.yml
echo
echo "== package.json files declaring engines =="
while IFS= read -r file; do
if jq -e '.engines?' "$file" >/dev/null; then
printf '\n==> %s\n' "$file"
jq '.engines' "$file"
fi
done < <(fd '^package\.json$')Repository: RocketChat/Rocket.Chat
Length of output: 616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Searching for 'engines' in package.json files =="
find . -name 'package.json' -type f | head -20 | while read -r file; do
if grep -q '"engines"' "$file" 2>/dev/null; then
printf '%s:\n' "$file"
grep -A 3 '"engines"' "$file"
echo
fi
done
echo "== Total package.json files in repo =="
find . -name 'package.json' -type f | wc -lRepository: RocketChat/Rocket.Chat
Length of output: 159
Remove temporary development changes from shared config and enable immutable installs.
Lines 11-14 comment out the engines plugin as a temporary development workaround in the committed .yarnrc.yml, which affects all installs on this branch. Per the project guidelines, temporary development-only changes should not be in shared config—either revert them or isolate them in a separate commit.
Additionally, line 5 sets enableImmutableInstalls: false, which disables reproducible installs. The Yarn configuration should enable immutable installs (enableImmutableInstalls: true) with the Yarn version pinned via Corepack in package.json.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.yarnrc.yml around lines 11 - 14, Uncomment the temporary development change
by restoring the plugins block (the commented "plugins:" entry and its child
path/spec lines that reference `@yarnpkg/plugin-engines`) so the engines plugin is
enabled in shared config, and set enableImmutableInstalls: true (replace the
current enableImmutableInstalls: false) to enforce immutable installs; also
ensure the Yarn version is pinned via Corepack in package.json so immutable
installs are reproducible.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/livechat/webpack.config.ts (1)
136-141: Unrelated change: This webpack modification is outside the PR scope.Adding the EJS template to HtmlWebpackPlugin is unrelated to the ImageBlock cached image race condition fix. Consider removing this change from the PR and submitting it separately to keep the PR focused on the stated objective.
Based on learnings: "In Rocket.Chat PRs, keep feature PRs free of unrelated... changes; prefer... isolating such bumps into a separate 'chore' commit/PR."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/livechat/webpack.config.ts` around lines 136 - 141, The change adding a template option to the HtmlWebpackPlugin configuration (the new HtmlWebpackPlugin({ template: _('./src/index.ejs'), title: 'Livechat - Rocket.Chat', chunks: ['polyfills', 'vendor', 'bundle'], chunksSortMode: 'manual' })) is unrelated to the ImageBlock cached image race condition fix; revert that modification by removing the template: _('./src/index.ejs') (and any other unrelated edits to the HtmlWebpackPlugin invocation) so the webpack.config.ts's HtmlWebpackPlugin call only contains the intended, scope-relevant settings, and submit the EJS template addition as a separate chore/feature PR instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/livechat/webpack.config.ts`:
- Around line 136-141: The change adding a template option to the
HtmlWebpackPlugin configuration (the new HtmlWebpackPlugin({ template:
_('./src/index.ejs'), title: 'Livechat - Rocket.Chat', chunks: ['polyfills',
'vendor', 'bundle'], chunksSortMode: 'manual' })) is unrelated to the ImageBlock
cached image race condition fix; revert that modification by removing the
template: _('./src/index.ejs') (and any other unrelated edits to the
HtmlWebpackPlugin invocation) so the webpack.config.ts's HtmlWebpackPlugin call
only contains the intended, scope-relevant settings, and submit the EJS template
addition as a separate chore/feature PR instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51d90d18-8edc-44e4-98ec-9ae3c8279ba8
📒 Files selected for processing (4)
.changeset/fix-imageblock-cached-image-race.md.yarnrc.ymlpackages/fuselage-ui-kit/src/blocks/ImageBlock.tsxpackages/livechat/webpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/fuselage-ui-kit/src/blocks/ImageBlock.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/livechat/webpack.config.ts
🧠 Learnings (11)
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
.yarnrc.yml
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.yarnrc.yml
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.yarnrc.yml
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
.yarnrc.yml
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
.yarnrc.yml
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
.yarnrc.yml
📚 Learning: 2026-01-08T15:03:59.621Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Applied to files:
.yarnrc.yml
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
.yarnrc.yml
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
.yarnrc.yml
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/livechat/webpack.config.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/livechat/webpack.config.ts
🔇 Additional comments (2)
.changeset/fix-imageblock-cached-image-race.md (1)
1-5: LGTM!The changeset correctly documents the race condition fix with an appropriate patch version bump. The description clearly explains the issue being addressed.
.yarnrc.yml (1)
11-14: Unrelated change: Remove or isolate this modification.This engines plugin change is unrelated to the ImageBlock race condition fix. Per project conventions, feature PRs should be free of unrelated configuration changes; prefer isolating such modifications into a separate "chore" commit/PR.
Based on learnings: "In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate 'chore' commit/PR."
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi maintainers,
While testing UIKit blocks, I noticed that images would sometimes get stuck showing the loading skeleton and never actually render. After debugging, I found this was happening specifically with cached images.
Problem:
When an image is cached by the browser, setting
img.srccan trigger theloadevent synchronously or very quickly - potentially before the event listener is properly attached. The code was checkingimg.completeAFTER setting src and adding the listener, which created a race condition where:Solution:
The fix checks if the image is already complete BEFORE setting src. If it is, update the state immediately and return early. Otherwise, attach the event listener first, then set src.
This ensures:
Testing:
Tested with:
All scenarios now work correctly with this PR.
Summary by CodeRabbit
Bug Fixes
Chores