-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat: add favorite applications #41462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release
Are you sure you want to change the base?
Conversation
Fixed FavitoristesSagas to put in the generic directory
…-push hook blocking The favorite actions are now in src/actions/applicationActions and are imported directly, so we don't need to re-export them from ee/actions/applicationActions.ts
WalkthroughAdds per-user application favorites: UI (card icon, virtual Favorites workspace), Redux actions/reducers/selectors, sagas with optimistic toggle and fetch, API helpers, and backend controller/service/repo support (toggling, retrieval, and cleanup), plus wiring to load favorites on workspace fetch. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Card as Card Component
participant Redux as Redux Store
participant Saga as Favorites Saga
participant API as ApplicationApi
participant Server as Backend Service
User->>Card: Click favorite icon
Card->>Redux: dispatch(TOGGLE_FAVORITE_APPLICATION_INIT)
Redux->>Saga: action handled by saga
Saga->>Redux: dispatch(TOGGLE_FAVORITE_APPLICATION_SUCCESS) [optimistic update]
Redux->>Card: UI updates (isFavorited toggled)
Saga->>API: PUT /users/applications/{appId}/favorite
API->>Server: HTTP PUT
Server->>Server: Toggle favorite in DB
Server-->>API: Response
alt Success
Saga->>Redux: confirm/reconcile state
else Failure
Saga->>Redux: dispatch(TOGGLE_FAVORITE_APPLICATION_SUCCESS) [rollback]
Saga->>Redux: dispatch(TOGGLE_FAVORITE_APPLICATION_ERROR)
Saga->>User: show error toast
end
sequenceDiagram
participant App as App Init
participant WorkspaceSaga as Workspace Sagas
participant Redux as Redux Store
participant FavoritesSaga as Favorites Saga
participant API as ApplicationApi
participant Server as Backend Service
participant DB as Database
App->>WorkspaceSaga: fetchAllWorkspacesSaga
WorkspaceSaga->>Redux: dispatch(FETCH_ALL_WORKSPACES_SUCCESS)
WorkspaceSaga->>Redux: dispatch(FETCH_FAVORITE_APPLICATIONS_INIT)
Redux->>FavoritesSaga: action received
FavoritesSaga->>API: GET /users/favoriteApplications
API->>Server: HTTP GET
Server->>DB: Read favoriteApplicationIds, fetch Applications
Server->>Server: Filter by permissions, cleanup missing entries
Server-->>API: List<Application>
FavoritesSaga->>Redux: dispatch(FETCH_FAVORITE_APPLICATIONS_SUCCESS) [apps]
Redux->>App: Favorites workspace populated with applications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/20201950013. |
|
Deploy-Preview-URL: https://ce-41462.dp.appsmith.com |
|
|
Regarding number 3 on the route, I disagree and am keeping it this way. I had purposefully made it not show edit but will make that change. I have corrected the 50 limit. |
|
@salevine, Still facing the Home page bug. On refreshing the page of a particular workspace, it shows Apps from the favorites instead of the Apps belonging to the particular workspace. Screen.Recording.2025-12-15.at.8.40.40.AM.mov |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/20221639173. |
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/20239685411. |
Failed server tests
|
…ss yarn check types
… handling on loading
Failed server tests
|
1 similar comment
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/20247455203. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/20598271075. |
|
Deploy-Preview-URL: https://ce-41462.dp.appsmith.com |
| // Inject virtual Favorites workspace at the top if user has favorites | ||
| if (hasFavorites && !isFetchingWorkspaces) { | ||
| const favoritesWorkspace = { | ||
| id: "__favorites__", | ||
| name: "Favorites", | ||
| isVirtual: true, | ||
| userPermissions: [], | ||
| }; | ||
|
|
||
| workspaces = [favoritesWorkspace, ...workspaces]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@salevine This callout seems legit; can you check?
|
|
||
| if (!workspace.id) return null; | ||
|
|
||
| const isFavoritesWorkspace = workspace.id === "__favorites__"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a new constant like FAVORITES_KEY = "__favorites__" in src/ce/constants/workspaceConstants.ts and use FAVORITES_KEY here. Let's avoid magic strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok . Did this. The magic strings are gone and new constant is used FAVORITES_KEY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashit-rath - I am not sure what callout you are talking about. That was just a comment .
| const favoritesWorkspace = { | ||
| id: "__favorites__", | ||
| name: "Favorites", | ||
| isVirtual: true, | ||
| userPermissions: [], | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is also a constant; put the whole object in the workspaceConstants file as
DEFAULT_FAVORITES_WORKSPACE = {
id: "__favorites__",
name: "Favorites",
isVirtual: true,
userPermissions: [],
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added export const DEFAULT_FAVORITES_WORKSPACE = {
id: FAVORITES_KEY,
name: "Favorites",
isVirtual: true,
userPermissions: [] as string[],
};
and replaced inline with the constants
| [ReduxActionTypes.FETCH_FAVORITE_APPLICATIONS_INIT]: ( | ||
| draftState: SelectedWorkspaceReduxState, | ||
| ) => { | ||
| draftState.loadingStates.isFetchingApplications = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we updating isFetchingApplications while fetching favorite applications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
favorites is being treated like a regular workspace - so we reused this without creating a new loading state. If you really wanted, we could create a different state. Seemed simpler to keep one
| if (workspaceId === "__favorites__") { | ||
| yield put({ | ||
| type: ReduxActionTypes.SET_CURRENT_WORKSPACE, | ||
| payload: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use DEFAULT_FAVORITES_WORKSPACE here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with changes above
|
|
||
| if (isValidResponse) { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const rawApplications = (response as any).data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using any and remove all the eslint-disables. We should use any in the most extreme cases where we just cannot escape
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. done
| } else { | ||
| // Non-successful API response – notify reducers so loading state is cleared. | ||
| yield put(fetchFavoriteApplicationsError()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else block is unreachable and can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The else block handles the case when validateResponse returns false, which happens when the response has AXIOS_CONNECTION_ABORTED_CODE (connection aborted). It ensures loading state is cleared in that scenario.
If you really don't like this block, we could let the catch block handle errors - but this is cleaner.
| const allApplications: ApplicationPayload[] = yield select( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (state: any) => state.ui.applications.applicationList, | ||
| ); | ||
|
|
||
| // Transform applications to include defaultBasePageId (needed for Launch button) | ||
| // This matches the transformation done in ApplicationSagas.tsx | ||
| const applications = rawApplications.map( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (application: any) => { | ||
| const defaultPage = findDefaultPage(application.pages); | ||
|
|
||
| // Find the corresponding application from the main list (if loaded) | ||
| const existing = allApplications?.find( | ||
| (app) => app.id === application.id, | ||
| ); | ||
|
|
||
| return { | ||
| ...application, | ||
| // Prefer userPermissions from the main application list so edit | ||
| // permissions match the regular workspace cards. | ||
| userPermissions: | ||
| existing?.userPermissions ?? application.userPermissions, | ||
| defaultPageId: defaultPage?.id, | ||
| defaultBasePageId: defaultPage?.baseId, | ||
| }; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing this? why can't we get the userPermissions from the server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.. switched to trust the server logic.
| @@ -0,0 +1,30 @@ | |||
| import { createSelector } from "reselect"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not create a new selector file and have these in the application selector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. done
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
|
/build-deploy-preview skip-tests=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/client/src/ce/selectors/applicationSelectors.tsx`:
- Around line 250-258: The selector getFavoriteApplications should guard against
undefined inputs and use a Set for faster lookups: ensure the inputs from
getApplications and getFavoriteApplicationIds are defaulted to empty
arrays/values (e.g., treat allApps as [] and favoriteIds as [] if undefined),
create a Set from favoriteIds (e.g., favoriteIdSet) and use
favoriteIdSet.has(app.id) in the filter, then sort the filtered results by name
as before; update references to allApps and favoriteIds in
getFavoriteApplications to use these defaults and the Set-based membership
check.
Failed server tests
|
|
/build-deploy-preview skip-tests=true |
1 similar comment
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/21300017121. |
|
Deploy-Preview-URL: https://ce-41462.dp.appsmith.com |
Addressing the backend comments with fixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/client/src/pages/Applications/ApplicationCard.tsx`:
- Line 110: In ApplicationCard, the component uses
useSelector(getApplicationList) which returns a search-filtered list and can
omit the current app, breaking the permission fallback; change the selector to
use useSelector(getApplications) (the unfiltered selector) so the
allApplications lookup used by the permission fallback logic in ApplicationCard
(the code that falls back to finding permissions when direct props are missing)
will always find the app. Ensure you update the const allApplications
declaration to call getApplications instead of getApplicationList.
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java`:
- Line 577: Replace the null-check/ternary for favoriteId with the same
empty-string-safe check used elsewhere: use
StringUtils.isEmpty(application.getBaseId()) to decide whether to fall back to
application.getId(). Update the assignment for favoriteId in
ApplicationPageServiceCEImpl (the line that declares String favoriteId) to
mirror the pattern used at other sites (lines that use
StringUtils.isEmpty(baseId)) so empty strings are treated like nulls.
♻️ Duplicate comments (2)
app/client/src/pages/Applications/ApplicationCard.tsx (1)
217-223: Workaround for missinguserPermissions— root cause still unaddressed.This fallback pattern addresses a symptom but not the underlying issue. A past reviewer also noted this concern. Consider investigating why Favorites views receive applications without
userPermissionspopulated in the first place.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserDataServiceCEImpl.java (1)
430-479: Non-atomic toggle can lose updates under concurrency.
The fetch‑modify‑save flow can drop favorites when two toggles race; prefer atomic $addToSet/$pull updates with a guarded limit check.
| const theme = useContext(ThemeContext); | ||
| const isSavingName = useSelector(getIsSavingAppName); | ||
| const isErroredSavingName = useSelector(getIsErroredSavingAppName); | ||
| const allApplications = useSelector(getApplicationList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: getApplicationList filters by search keyword, breaking fallback lookup.
getApplicationList applies fuzzy search filtering. When a search is active, the filtered list may not contain the application being rendered, causing the permission fallback at lines 219-223 to fail silently.
Use getApplications (the unfiltered selector) instead.
🐛 Proposed fix
-import {
- getApplicationList,
- getIsSavingAppName,
+import {
+ getIsSavingAppName,
getIsErroredSavingAppName,
} from "ee/selectors/applicationSelectors";
+import { getApplications } from "ee/selectors/applicationSelectors";- const allApplications = useSelector(getApplicationList);
+ const allApplications = useSelector(getApplications);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const allApplications = useSelector(getApplicationList); | |
| const allApplications = useSelector(getApplications); |
🤖 Prompt for AI Agents
In `@app/client/src/pages/Applications/ApplicationCard.tsx` at line 110, In
ApplicationCard, the component uses useSelector(getApplicationList) which
returns a search-filtered list and can omit the current app, breaking the
permission fallback; change the selector to use useSelector(getApplications)
(the unfiltered selector) so the allApplications lookup used by the permission
fallback logic in ApplicationCard (the code that falls back to finding
permissions when direct props are missing) will always find the app. Ensure you
update the const allApplications declaration to call getApplications instead of
getApplicationList.
| Mono<AclPermission> actionPermissionMono = | ||
| actionPermission.getDeletePermission().cache(); | ||
| Mono<AclPermission> pagePermissionMono = pagePermission.getDeletePermission(); | ||
| String favoriteId = application.getBaseId() != null ? application.getBaseId() : application.getId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for getBaseId() usage patterns
echo "=== getBaseId() usage patterns ==="
rg -n 'getBaseId\(\)' --type=java -B2 -A2 | head -80Repository: appsmithorg/appsmith
Length of output: 11577
🏁 Script executed:
#!/bin/bash
# Check for StringUtils usage in this file and similar patterns
echo "=== StringUtils.hasLength usage in project ==="
rg -n 'StringUtils\.hasLength' --type=java | head -30Repository: appsmithorg/appsmith
Length of output: 4939
🏁 Script executed:
#!/bin/bash
# Check for similar null-check patterns with IDs in ApplicationPageServiceCEImpl.java
echo "=== ApplicationPageServiceCEImpl patterns ==="
rg -n '!= null.*:.*getId\(\)' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.javaRepository: appsmithorg/appsmith
Length of output: 222
🏁 Script executed:
#!/bin/bash
# Check BaseId class/interface definition to understand its nature
fd -e java | xargs rg -l 'getBaseId\(\)' | head -5
echo "=== Checking Application class for baseId field ==="
rg -n 'class Application|baseId' app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Application.java | head -20Repository: appsmithorg/appsmith
Length of output: 685
Use StringUtils.isEmpty() for consistency with similar checks in this file.
Line 211 and lines 738-740 already use StringUtils.isEmpty(baseId) for the same getBaseId() fallback pattern. Line 577 should align with that approach.
Suggested change
- String favoriteId = application.getBaseId() != null ? application.getBaseId() : application.getId();
+ String favoriteId = StringUtils.isEmpty(application.getBaseId()) ? application.getId() : application.getBaseId();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String favoriteId = application.getBaseId() != null ? application.getBaseId() : application.getId(); | |
| String favoriteId = StringUtils.isEmpty(application.getBaseId()) ? application.getId() : application.getBaseId(); |
🤖 Prompt for AI Agents
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java`
at line 577, Replace the null-check/ternary for favoriteId with the same
empty-string-safe check used elsewhere: use
StringUtils.isEmpty(application.getBaseId()) to decide whether to fall back to
application.getId(). Update the assignment for favoriteId in
ApplicationPageServiceCEImpl (the line that declares String favoriteId) to
mirror the pattern used at other sites (lines that use
StringUtils.isEmpty(baseId)) so empty strings are treated like nulls.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/21364208936. |
Fixed. Added UserDataService to the ApplicationPageServiceImpl constructor and passed it to the super call. The compilation error should be resolved.
|
Deploy-Preview-URL: https://ce-41462.dp.appsmith.com |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/21364208936. |
|
Deploy-Preview-URL: https://ce-41462.dp.appsmith.com |




Description
Feature Request 8479: Allow a user to favorite applications.
In bigger factories there can be many workspaces and applications. Users might have access to a lot of applications, but generally only use a handful. This feature allows a user to favorite one or more applications (up to 50). Any favorited applications will show up in a virtual Favorites workspace in alphabetical order
Fixes #
8479Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 6f9f36d yet
Mon, 26 Jan 2026 17:05:30 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.