Skip to content

Honor configured favicon path in Console (app_favicon_path) - #10566

Merged
KaveeshaPiumini merged 1 commit into
wso2:masterfrom
KaveeshaPiumini:fix-7569-is-master
Jul 23, 2026
Merged

Honor configured favicon path in Console (app_favicon_path)#10566
KaveeshaPiumini merged 1 commit into
wso2:masterfrom
KaveeshaPiumini:fix-7569-is-master

Conversation

@KaveeshaPiumini

Copy link
Copy Markdown
Contributor

Root cause

The Console's favicon <link> was a build-time default hardcoded in apps/console/src/home.jsp and never read the runtime console.ui.app_favicon_path configuration, so a configured custom favicon was ignored.

Fix

  • Surface appFaviconPath from the runtime app config into the config reducer (admin.core.v1).
  • Apply the configured favicon via react-helmet in apps/console/src/app.tsx. Absolute/data URLs are used as-is; relative/default paths are resolved under the theme asset prefix (clientOrigin/appBase from AppUtils).
  • Remove the hardcoded favicon <link> from home.jsp so react-helmet is the single source of truth.

Related Issue

wso2/product-is#28078

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

The Console favicon is now sourced from runtime UI configuration, resolved against theme assets when needed, and rendered dynamically through react-helmet. The static JSP favicon link was removed, and a patch changeset was added.

Runtime favicon configuration

Layer / File(s) Summary
Favicon configuration contract
features/admin.core.v1/models/config.ts, features/admin.core.v1/configs/app.ts
UIConfigInterface and Config.getUIConfig() now expose the optional runtime appFaviconPath.
Dynamic Console favicon rendering
apps/console/src/app.tsx, apps/console/src/home.jsp, .changeset/tall-favicons-honor.md
The Console resolves and injects the configured favicon through Helmet, removes the static HTML favicon link, and records patch releases for the affected packages.

Suggested reviewers: dilshansenarath, pavindulakshan

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: honoring the configured Console favicon path.
Description check ✅ Passed The description covers the purpose, fix, and related issue, but omits several template sections like related PRs and checklists.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Changeset Required ✅ Passed A new .changeset/tall-favicons-honor.md is included in the PR diff and lists the affected packages with patch updates.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • 🛠️ create changeset

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.

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/console/src/app.tsx (1)

194-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Preserve the optional favicon type.

appFaviconPath is optional in UIConfigInterface, so this selector can return undefined. Type it as string | undefined, and annotate the useMemo callback accordingly to match the runtime guard and the explicit-annotation guideline.

Proposed fix
-    const appFaviconPath: string = useSelector((state: AppState) => state?.config?.ui?.appFaviconPath);
+    const appFaviconPath: string | undefined = useSelector(
+        (state: AppState): string | undefined => state?.config?.ui?.appFaviconPath
+    );

-    const faviconHref: string | undefined = useMemo(() => {
+    const faviconHref: string | undefined = useMemo<string | undefined>(
+        (): string | undefined => {
🤖 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 `@apps/console/src/app.tsx` around lines 194 - 226, Update the appFaviconPath
selector in the App component to use the optional type string | undefined, and
annotate the faviconHref useMemo callback with the corresponding string |
undefined return type. Preserve the existing runtime guard and favicon
resolution logic.

Sources: Coding guidelines, MCP tools

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

Outside diff comments:
In `@apps/console/src/app.tsx`:
- Around line 194-226: Update the appFaviconPath selector in the App component
to use the optional type string | undefined, and annotate the faviconHref
useMemo callback with the corresponding string | undefined return type. Preserve
the existing runtime guard and favicon resolution logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: e4a58e02-0f38-40c7-84bb-62540e6dfe59

📥 Commits

Reviewing files that changed from the base of the PR and between bb63c87 and a4dc83f.

📒 Files selected for processing (5)
  • .changeset/tall-favicons-honor.md
  • apps/console/src/app.tsx
  • apps/console/src/home.jsp
  • features/admin.core.v1/configs/app.ts
  • features/admin.core.v1/models/config.ts
💤 Files with no reviewable changes (1)
  • apps/console/src/home.jsp

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.91%. Comparing base (44ee922) to head (a4dc83f).
⚠️ Report is 15 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #10566    +/-   ##
========================================
  Coverage   72.91%   72.91%            
========================================
  Files         470      470            
  Lines       71383    71389     +6     
  Branches      240      448   +208     
========================================
+ Hits        52047    52053     +6     
+ Misses      19225    19043   -182     
- Partials      111      293   +182     

see 182 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KaveeshaPiumini
KaveeshaPiumini merged commit e3f4506 into wso2:master Jul 23, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants