feat: display workspace logos in sidebar navigation #41377
Conversation
- Modified Workspace.java to return null for logoUrl when no logo exists - Updated workspace reducer to sync logo changes to search results - Enhanced WorkspaceMenuItem to display uploaded logos with fallback to default icon - Added proper error handling and styling for workspace logo display
WalkthroughAdds workspace-logo aware rendering on the client (new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Frontend
participant Backend
participant AssetService
User->>Frontend: Open workspace menu
Frontend->>Backend: Fetch workspaces (includes logoAssetId)
Backend->>Backend: getLogoUrl() — returns URL or null
alt logo URL returned
Backend-->>Frontend: workspace with logoUrl
Frontend->>AssetService: Request image at logoUrl
alt image loads
AssetService-->>Frontend: image bytes
Frontend->>Frontend: Render WorkspaceItemRow (logo + text)
else image fails
AssetService-->>Frontend: load error
Frontend->>Frontend: set imageError state
Frontend->>Frontend: Render WorkspaceItemRow (fallback text)
end
else logoUrl null
Backend-->>Frontend: workspace without logoUrl
Frontend->>Frontend: Render default ListItem
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Workspace.java (1)
74-78: Consider usingisBlank()for more robust string validation.The current check uses
isEmpty(), which only catches empty strings. IflogoAssetIdcontains only whitespace, it would still construct an invalid URL like"/api/v1/assets/ ". Consider usingisBlank()to handle null, empty, and whitespace-only strings.Apply this diff:
- if (logoAssetId == null || logoAssetId.isEmpty()) { + if (logoAssetId == null || logoAssetId.isBlank()) { return null; }app/client/src/ce/pages/Applications/index.tsx (1)
478-483: Avoid manual text truncation; prefer CSS or existing utilities.The manual string slicing with a hardcoded length of 22 is less flexible than CSS-based truncation. The file already imports
truncateTextUsingEllipsis(line 45) which could be used for consistent styling. Additionally, this creates an inconsistency: the logo path manually truncates text while the non-logo path (ListItem) handles ellipsizing internally.Consider applying
truncateTextUsingEllipsisto the Text component via styled-components instead of manually slicing the string:- const hasLogo = workspace?.logoUrl && !imageError; - const displayText = isFetchingWorkspaces - ? workspace?.name - : workspace?.name?.length > 22 - ? workspace.name.slice(0, 22).concat(" ...") - : workspace?.name; + const hasLogo = workspace?.logoUrl && !imageError;Then style the Text component to handle overflow:
const WorkspaceNameText = styled(Text)` max-width: 200px; ${truncateTextUsingEllipsis} `;And use it in the render:
- <Text type={TextType.H5} weight={FontWeight.NORMAL}> - {displayText} + <WorkspaceNameText type={TextType.H5} weight={FontWeight.NORMAL}> + {workspace?.name} - </Text> + </WorkspaceNameText>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/ce/pages/Applications/index.tsx(4 hunks)app/client/src/ce/reducers/uiReducers/workspaceReducer.ts(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Workspace.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-16T19:43:14.764Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38171
File: app/client/src/git/components/GitContextProvider/index.tsx:60-70
Timestamp: 2024-12-16T19:43:14.764Z
Learning: Prefer not to throw errors when dispatching actions in the `setImportWorkspaceId` function in `app/client/src/git/components/GitContextProvider/index.tsx`.
Applied to files:
app/client/src/ce/reducers/uiReducers/workspaceReducer.tsapp/client/src/ce/pages/Applications/index.tsx
⏰ 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). (6)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/src/ce/reducers/uiReducers/workspaceReducer.ts (1)
111-124: LGTM! Search entity synchronization looks correct.The logic properly mirrors the main workspace list update pattern, ensuring search results stay in sync when workspace properties (like logoUrl) are updated.
app/client/src/ce/pages/Applications/index.tsx (1)
460-507: Image error handling and conditional rendering look good.The error state management and fallback logic are well-implemented. The component gracefully handles image loading failures by falling back to the ListItem component.
Fix prettier lint errors
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/client/src/ce/pages/Applications/index.tsx (2)
452-457: Add proper type for the workspace prop.The
workspaceprop is typed asany, which bypasses type safety. Use theWorkspacetype already imported at line 83.Apply this diff:
export function WorkspaceMenuItem({ isFetchingWorkspaces, selected, - workspace, // TODO: Fix this the next time the file is edited - // eslint-disable-next-line @typescript-eslint/no-explicit-any -}: any) { + workspace, +}: { + isFetchingWorkspaces: boolean; + selected: boolean; + workspace: Workspace; +}) {
460-474: Reset imageError state when workspace changes.The
imageErrorstate persists across workspace prop changes. If a user navigates from a workspace with a broken logo to another workspace with a valid logo, the error state from the previous workspace will incorrectly hide the new logo.Apply this diff:
export function WorkspaceMenuItem({ isFetchingWorkspaces, selected, workspace, }: { isFetchingWorkspaces: boolean; selected: boolean; workspace: Workspace; }) { const history = useHistory(); const location = useLocation(); const [imageError, setImageError] = React.useState(false); + + React.useEffect(() => { + setImageError(false); + }, [workspace?.id, workspace?.logoUrl]); const handleWorkspaceClick = () => {
🧹 Nitpick comments (1)
app/client/src/ce/pages/Applications/index.tsx (1)
479-483: Consider extracting text truncation logic.The 22-character truncation logic is duplicated between the custom rendering path (lines 479-483) and the ListItem fallback (line 514). Extracting this into a helper function or constant would make future adjustments easier.
Example:
const WORKSPACE_NAME_MAX_LENGTH = 22; const getTruncatedWorkspaceName = (name: string, isFetching: boolean) => { if (isFetching || name.length <= WORKSPACE_NAME_MAX_LENGTH) { return name; } return name.slice(0, WORKSPACE_NAME_MAX_LENGTH) + " ..."; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/ce/pages/Applications/index.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-16T19:43:14.764Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38171
File: app/client/src/git/components/GitContextProvider/index.tsx:60-70
Timestamp: 2024-12-16T19:43:14.764Z
Learning: Prefer not to throw errors when dispatching actions in the `setImportWorkspaceId` function in `app/client/src/git/components/GitContextProvider/index.tsx`.
Applied to files:
app/client/src/ce/pages/Applications/index.tsx
⏰ 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). (7)
- GitHub Check: server-spotless / spotless-check
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
🔇 Additional comments (3)
app/client/src/ce/pages/Applications/index.tsx (3)
63-63: LGTM: FontWeight import added for text styling.The import is properly used in the new workspace logo rendering path.
399-450: Styled components look solid.The new components properly handle selected/hover states and maintain visual consistency with the existing design system. The fixed logo dimensions (20x20px) match the icon size from the fallback ListItem.
486-507: Clean conditional rendering with proper fallback.The rendering logic correctly prioritizes the logo display when available and gracefully falls back to the standard ListItem when loading or when the logo fails to load.
Failed server tests
|
Failed server tests
|
1 similar comment
Failed server tests
|
|
|
||
| const hasLogo = workspace?.logoUrl && !imageError; | ||
| const displayText = isFetchingWorkspaces | ||
| ? workspace?.name |
There was a problem hiding this comment.
If the workspaces are still being fetched, where would you obtain the workspace name? Wouldn't this be undefined in this case?
There was a problem hiding this comment.
@rahulbarwal It should not be undefined. Based on everything I can see, the name comes from the getFetchedWorkspaces. Initially, the workspaces array is empty and isFetchingWorkspaces is true. Nothing would show up. While refreshing, it may contain older data - but then when done fetching, it will refresh over it. In the initial state, it all should be safe
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/19527529275. |
|
Deploy-Preview-URL: https://ce-41377.dp.appsmith.com |
|
@salevine Can we have the same size for the icons? 16x16? Else it doesn't look good: Secondly, I'm seeing a tooltip on hover of the workspace name only when there is an image and even though the name is not long enough to show the tooltip: Also, with larger icons, the icon is not even visible in the list, any reason we find this change useful? |
|
@ankitakinger - I made a change to make the new icons 16x16 . I had them as 20x20. Not a problem. If the user picks something that doesn't look good to them - then that is up to them to pick something that looks better. Pretty common experience across all sorts of platforms. Overall, yes this is a useful feature. Several customers have asked for it. It was partially implemented before. I went ahead and removed the tooltip that I had added. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/ce/pages/Applications/index.tsx (1)
452-505: Logo branch logic is sound; consider tightening truncation and boolean handlingThe new logo-aware path in
WorkspaceMenuItembehaves correctly overall:
imageErrorensures a graceful fallback to the defaultListItemwhen the logo fails to load.hasLogogates the custom row so skeleton/loader states still use the oldListItempath.- Guarding on
!workspace.idavoids rendering invalid entries.A couple of small cleanups would improve maintainability:
displayTextincludes anisFetchingWorkspacescheck, but this branch is unreachable in the logo path because you only use it when!isFetchingWorkspaces. You can simplify it to just the truncation logic.- The constant
22is now duplicated indisplayTextandListItem’sellipsizeprop. Consider extracting a sharedconst WORKSPACE_NAME_MAX_CHARS = 22;to avoid drift if the spec changes later.- For clarity, you may want
const hasLogo = Boolean(workspace?.logoUrl) && !imageError;rather than relying on the string/false truthiness.Functionally this is fine as-is, these are just maintainability tweaks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/ce/pages/Applications/index.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-16T19:43:14.764Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38171
File: app/client/src/git/components/GitContextProvider/index.tsx:60-70
Timestamp: 2024-12-16T19:43:14.764Z
Learning: Prefer not to throw errors when dispatching actions in the `setImportWorkspaceId` function in `app/client/src/git/components/GitContextProvider/index.tsx`.
Applied to files:
app/client/src/ce/pages/Applications/index.tsx
⏰ 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). (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (1)
app/client/src/ce/pages/Applications/index.tsx (1)
58-64: FontWeight import and usage look consistent
FontWeightis correctly imported and used in the new<Text weight={FontWeight.NORMAL}>instance below; no functional concerns here.
| const WorkspaceItemRow = styled.a<{ disabled?: boolean; selected?: boolean }>` | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| text-decoration: none; | ||
| padding: 0px var(--ads-spaces-6); | ||
| background-color: ${(props) => | ||
| props.selected ? "var(--ads-v2-color-bg-muted)" : "transparent"}; | ||
| .${Classes.TEXT} { | ||
| color: var(--ads-v2-color-fg); | ||
| } | ||
| .${Classes.ICON} { | ||
| svg { | ||
| path { | ||
| fill: var(--ads-v2-color-fg); | ||
| } | ||
| } | ||
| } | ||
| height: 38px; | ||
| ${(props) => | ||
| !props.disabled | ||
| ? ` | ||
| &:hover { | ||
| text-decoration: none; | ||
| cursor: pointer; | ||
| background-color: var(--ads-v2-color-bg-subtle); | ||
| border-radius: var(--ads-v2-border-radius); | ||
| }` | ||
| : ` | ||
| &:hover { | ||
| text-decoration: none; | ||
| cursor: default; | ||
| } | ||
| `} | ||
| `; | ||
|
|
||
| const WorkspaceIconContainer = styled.span` | ||
| display: flex; | ||
| align-items: center; | ||
| .${Classes.ICON} { | ||
| margin-right: var(--ads-spaces-5); | ||
| } | ||
| `; | ||
|
|
||
| const WorkspaceLogoImage = styled.img` | ||
| width: 16px; | ||
| height: 16px; | ||
| object-fit: contain; | ||
| margin-right: var(--ads-spaces-5); | ||
| `; |
There was a problem hiding this comment.
Anchor-based WorkspaceItemRow hurts keyboard accessibility; consider button/div instead
WorkspaceItemRow is a styled.a with an onClick handler but no href. This makes it non-focusable by keyboard by default and semantically misleading (it behaves like a button, not a link). That’s a regression in accessibility compared to the existing ListItem-based implementation.
I’d strongly recommend:
- Switching to
styled.buttonorstyled.divwithrole="button"+tabIndex={0}and anonKeyDownhandler forEnter/Space, or - Re-using
ListItemand customizing its icon slot instead of re-creating the row.
Also, note that:
- The
disabledprop is styled but never passed fromWorkspaceMenuItem, so the disabled styling path is currently unused. Either wire it up (e.g., whenisFetchingWorkspaces) or remove it until needed.
🤖 Prompt for AI Agents
In app/client/src/ce/pages/Applications/index.tsx around lines 399 to 450, the
WorkspaceItemRow is implemented as styled.a without an href which breaks
keyboard accessibility and is semantically incorrect for a click-action; change
it to a focusable, actionable element (preferably styled.button, or styled.div
with role="button", tabIndex={0} and an onKeyDown handler that handles
Enter/Space) so it is keyboard-focusable and operable, update event handling
accordingly (use disabled attribute when using button or prevent key activation
when disabled), and either wire the disabled prop through from WorkspaceMenuItem
(e.g., pass disabled when isFetchingWorkspaces) so the disabled styles are
meaningful or remove the unused disabled styling.
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/19727149661. |
|
Deploy-Preview-URL: https://ce-41377.dp.appsmith.com |
|
I have 3 more points:
|
3 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
41376Automation
/ok-to-test tags="@tag.Workspace"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/19708101799
Commit: f4777c5
Cypress dashboard.
Tags:
@tag.WorkspaceSpec:
Wed, 26 Nov 2025 15:23:05 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.