Add bookmark/favoriting to Search tab#253
Merged
CodyWMitchell merged 2 commits intoRedHatInsights:masterfrom Mar 16, 2026
Merged
Add bookmark/favoriting to Search tab#253CodyWMitchell merged 2 commits intoRedHatInsights:masterfrom
CodyWMitchell merged 2 commits intoRedHatInsights:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds bookmark and favorite support to HelpPanel search: new FavoriteIcon and styles, a serviceFavorites utility to fetch/toggle favorites, SearchPanel/SearchResults/SearchResultItem extended to load/sync and toggle bookmarks/favorites (with API calls and optimistic updates), a LearnPanel URL fix, and new Cypress component tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant Item as SearchResultItem
participant Chrome as Chrome API
participant Backend as Backend API
participant Panel as SearchPanel
User->>Item: Click bookmark or favorite
Item->>Chrome: requestUserIdentity()
Chrome-->>Item: identity (account_id)
alt bookmark flow
Item->>Backend: POST /favorites (quickstartName, favorite)
else favorite flow
Item->>Backend: POST /api/chrome-service/v1/favorite-pages (pathname, favorite)
end
alt success
Backend-->>Item: updated state
Item->>Panel: call onBookmarkToggle/onFavoriteToggle
Panel->>Panel: refresh favoritePages if needed
Item-->>User: update icon/state
else error
Backend-->>Item: error
Item-->>User: revert icon/state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. 🔧 Stylelint (17.4.0)src/components/common/BookmarkIcon.scssConfigurationError: Could not find "stylelint-config-recommended-scss". Do you need to install the package or use the "configBasedir" option? src/components/common/FavoriteIcon.scssConfigurationError: Could not find "stylelint-config-recommended-scss". Do you need to install the package or use the "configBasedir" option? 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: 1
🧹 Nitpick comments (4)
src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsx (2)
573-592: Bookmark toggle doesn't await the data refresh.
handleBookmarkTogglecallsfetchAllDatabut doesn't await the result before returning. While this is non-blocking, it means theallQuickStartsstate update is fire-and-forget, and any errors only log to console. This is acceptable for a background refresh but could lead to stale recommended content until the next interaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsx` around lines 573 - 592, The handleBookmarkToggle function updates local search results but calls fetchAllData without awaiting it, making the allQuickStarts refresh fire-and-forget and error handling only console.log; change handleBookmarkToggle to be async (or return the fetch promise) and await fetchAllData(chrome.auth.getUser, {}) inside a try/catch, then call setAllQuickStarts with the returned quickStarts and surface errors via the existing logging mechanism (instead of silent console.error) so the state refresh is completed before the caller proceeds; reference handleBookmarkToggle, fetchAllData, setAllQuickStarts, and chrome.auth.getUser when applying the change.
472-499: Missing dependency in useEffect for performSearch.
performSearch(Lines 207-305) referencesfavoritePagesstate (Line 263-265) to computeisFavoritedfor service results. However, theuseEffectat Line 473 that callsperformSearchonly depends ondebouncedSearchText, notfavoritePages.This means if a search is performed before
favoritePagesloads, the initial search results will have staleisFavoritedvalues. The sync effect at Lines 456-470 mitigates this, but it would be cleaner to either:
- Add
favoritePagesto the dependency array (may cause unnecessary re-searches)- Move
isFavoritedcomputation entirely to the sync effect (cleaner separation)Given the existing sync effect handles this case, this is a minor concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsx` around lines 472 - 499, The performSearch useEffect currently omits favoritePages from its dependency list which can yield stale isFavorited flags; fix by moving the isFavorited computation out of performSearch and into the existing sync effect that runs when favoritePages changes (the effect that updates results after favorites load), i.e., keep performSearch returning raw results without touching isFavorited and update setRawSearchResults or map results to include isFavorited using favoritePages inside the sync effect; alternatively, if you prefer the simpler change, add favoritePages to the dependency array of the useEffect that calls performSearch so searches re-run when favorites update (refer to performSearch, favoritePages, and the useEffect that calls executeSearch).src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx (1)
75-98: Inconsistent API call location between bookmark and favorite handlers.
handleBookmarkClickmakes the API call directly within the component (Line 90), whilehandleFavoriteClickonly updates local state and delegates the API call to the parent viaonFavoriteToggle. This works but creates an architectural inconsistency.Consider either:
- Moving bookmark API call to parent (consistent with favorite)
- Moving favorite API call into this component (consistent with bookmark)
The current approach works correctly but may cause confusion during maintenance.
Also applies to: 100-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx` around lines 75 - 98, The bookmark handler is making the API call inside handleBookmarkClick while handleFavoriteClick delegates to the parent via onFavoriteToggle; to make this consistent, remove the axios.post from handleBookmarkClick and delegate persistence to the parent by calling onBookmarkToggle(result.resourceName!, newState) (same pattern as onFavoriteToggle), keep optimistic UI with setIsBookmarked(newState), and ensure onBookmarkToggle returns a promise so the parent can perform the axios.post and surface failures back to the component so you can revert via setIsBookmarked(result.isBookmarked ?? false) on error; update any related types/props to reflect onBookmarkToggle’s promise-based signature.src/utils/serviceFavorites.ts (1)
39-49: Consider adding error handling consistency.
fetchFavoritePagescatches errors and returns a fallback, buttoggleFavoritePagelets errors propagate. While the caller (SearchPanel) does handle the error with rollback logic, this inconsistency could lead to confusion. Consider either documenting that callers must handle errors, or adding consistent error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/serviceFavorites.ts` around lines 39 - 49, toggleFavoritePage is inconsistent with fetchFavoritePages because it allows network errors to propagate; wrap the axios.post call in toggleFavoritePage with a try/catch that mirrors fetchFavoritePages' behavior: on error catch and log the error (use the same logging mechanism used elsewhere) and return a safe fallback (e.g., call fetchFavoritePages() or return the same empty-array fallback used by fetchFavoritePages) so callers get a consistent FavoritePage[] result; reference toggleFavoritePage, fetchFavoritePages and the caller SearchPanel in your change to ensure rollback logic still works.
🤖 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/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx`:
- Line 225: The OutlinedBookmarkedIcon uses an invalid token class; replace the
className value "pf-v6-t-color-100" with the utility class "pf-v6-u-color-200"
on the OutlinedBookmarkedIcon element (search for the OutlinedBookmarkedIcon JSX
in SearchResultItem.tsx) so it matches the PatternFly v6 utility naming and the
rest of the component's styling.
---
Nitpick comments:
In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsx`:
- Around line 573-592: The handleBookmarkToggle function updates local search
results but calls fetchAllData without awaiting it, making the allQuickStarts
refresh fire-and-forget and error handling only console.log; change
handleBookmarkToggle to be async (or return the fetch promise) and await
fetchAllData(chrome.auth.getUser, {}) inside a try/catch, then call
setAllQuickStarts with the returned quickStarts and surface errors via the
existing logging mechanism (instead of silent console.error) so the state
refresh is completed before the caller proceeds; reference handleBookmarkToggle,
fetchAllData, setAllQuickStarts, and chrome.auth.getUser when applying the
change.
- Around line 472-499: The performSearch useEffect currently omits favoritePages
from its dependency list which can yield stale isFavorited flags; fix by moving
the isFavorited computation out of performSearch and into the existing sync
effect that runs when favoritePages changes (the effect that updates results
after favorites load), i.e., keep performSearch returning raw results without
touching isFavorited and update setRawSearchResults or map results to include
isFavorited using favoritePages inside the sync effect; alternatively, if you
prefer the simpler change, add favoritePages to the dependency array of the
useEffect that calls performSearch so searches re-run when favorites update
(refer to performSearch, favoritePages, and the useEffect that calls
executeSearch).
In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx`:
- Around line 75-98: The bookmark handler is making the API call inside
handleBookmarkClick while handleFavoriteClick delegates to the parent via
onFavoriteToggle; to make this consistent, remove the axios.post from
handleBookmarkClick and delegate persistence to the parent by calling
onBookmarkToggle(result.resourceName!, newState) (same pattern as
onFavoriteToggle), keep optimistic UI with setIsBookmarked(newState), and ensure
onBookmarkToggle returns a promise so the parent can perform the axios.post and
surface failures back to the component so you can revert via
setIsBookmarked(result.isBookmarked ?? false) on error; update any related
types/props to reflect onBookmarkToggle’s promise-based signature.
In `@src/utils/serviceFavorites.ts`:
- Around line 39-49: toggleFavoritePage is inconsistent with fetchFavoritePages
because it allows network errors to propagate; wrap the axios.post call in
toggleFavoritePage with a try/catch that mirrors fetchFavoritePages' behavior:
on error catch and log the error (use the same logging mechanism used elsewhere)
and return a safe fallback (e.g., call fetchFavoritePages() or return the same
empty-array fallback used by fetchFavoritePages) so callers get a consistent
FavoritePage[] result; reference toggleFavoritePage, fetchFavoritePages and the
caller SearchPanel in your change to ensure rollback logic still works.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c30c399-0cc1-4eb4-bf0a-b1a7b190313b
📒 Files selected for processing (8)
cypress/component/SearchResultItem.cy.tsxsrc/components/HelpPanel/HelpPanelTabs/LearnPanel.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResults.tsxsrc/components/common/BookmarkIcon.tsxsrc/components/common/FavoriteIcon.tsxsrc/utils/serviceFavorites.ts
src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx (2)
229-249: Internationalize aria-labels.The aria-labels are hardcoded in English. Since the component uses
useIntlfor other text (breadcrumbs), these accessibility labels should also be internationalized for consistency and accessibility compliance.♻️ Suggested approach
Add message definitions in your messages file:
bookmarkResource: { id: 'searchResultItem.bookmarkResource', defaultMessage: 'Bookmark learning resource', }, unbookmarkResource: { id: 'searchResultItem.unbookmarkResource', defaultMessage: 'Unbookmark learning resource', }, favoriteService: { id: 'searchResultItem.favoriteService', defaultMessage: 'Favorite {title}', }, unfavoriteService: { id: 'searchResultItem.unfavoriteService', defaultMessage: 'Unfavorite {title}', },Then use them in the component:
aria-label={ isBookmarked - ? 'Unbookmark learning resource' - : 'Bookmark learning resource' + ? intl.formatMessage(messages.unbookmarkResource) + : intl.formatMessage(messages.bookmarkResource) }aria-label={ isFavorited - ? `Unfavorite ${result.title}` - : `Favorite ${result.title}` + ? intl.formatMessage(messages.unfavoriteService, { title: result.title }) + : intl.formatMessage(messages.favoriteService, { title: result.title }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx` around lines 229 - 249, The aria-label strings for the bookmark and favorite buttons are hardcoded in SearchResultItem; add i18n message definitions (e.g., bookmarkResource, unbookmarkResource, favoriteService, unfavoriteService) and use the component's useIntl to replace the hardcoded labels: for the bookmark button use intl.formatMessage for bookmarkResource/unbookmarkResource based on isBookmarked, and for the favorite button use intl.formatMessage for favoriteService/unfavoriteService with the {title: result.title} variable based on isFavorited (leave isFavoriteable and the handlers like handleFavoriteClick unchanged).
101-109: Note: Different API handling pattern for favorites vs bookmarks.
handleFavoriteClickdelegates API persistence to the parent viaonFavoriteToggle, whilehandleBookmarkClickmakes the API call directly. This asymmetry is acceptable if intentional (different APIs), but consider adding error state reversion if the parent callback fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx` around lines 101 - 109, handleFavoriteClick updates local state and calls the parent via onFavoriteToggle but doesn’t handle failures, so revert the optimistic UI if the parent callback fails; modify handleFavoriteClick to capture the previous isFavorited, call setIsFavorited optimistically, then call onFavoriteToggle(result.url, newState) and if it returns a Promise await it (or catch errors) and on failure reset setIsFavorited(previousState) and optionally surface an error; reference handleFavoriteClick, onFavoriteToggle, and handleBookmarkClick (as the direct-API counterpart) when making this change.
🤖 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/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx`:
- Around line 82-98: The bookmark handler assumes
user.identity.internal.account_id exists; first retrieve the user via
chrome.auth.getUser() and validate that account_id is defined (from
user.identity.internal?.account_id) before calling axios.post—if account_id is
missing, either omit the account query parameter or abort and surface an error
instead of sending ?account=undefined; keep updating local state via
setIsBookmarked and still call onBookmarkToggle, but build the POST URL
conditionally (or throw) so the server never receives "undefined". Also update
the catch block to log the actual error (e.g., console.error or a logger) along
with context (include result.resourceName and attempted favorite state) before
resetting state, so failures are visible for debugging; reference
chrome.auth.getUser, account_id, setIsBookmarked, axios.post, and
onBookmarkToggle when making these changes.
---
Nitpick comments:
In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx`:
- Around line 229-249: The aria-label strings for the bookmark and favorite
buttons are hardcoded in SearchResultItem; add i18n message definitions (e.g.,
bookmarkResource, unbookmarkResource, favoriteService, unfavoriteService) and
use the component's useIntl to replace the hardcoded labels: for the bookmark
button use intl.formatMessage for bookmarkResource/unbookmarkResource based on
isBookmarked, and for the favorite button use intl.formatMessage for
favoriteService/unfavoriteService with the {title: result.title} variable based
on isFavorited (leave isFavoriteable and the handlers like handleFavoriteClick
unchanged).
- Around line 101-109: handleFavoriteClick updates local state and calls the
parent via onFavoriteToggle but doesn’t handle failures, so revert the
optimistic UI if the parent callback fails; modify handleFavoriteClick to
capture the previous isFavorited, call setIsFavorited optimistically, then call
onFavoriteToggle(result.url, newState) and if it returns a Promise await it (or
catch errors) and on failure reset setIsFavorited(previousState) and optionally
surface an error; reference handleFavoriteClick, onFavoriteToggle, and
handleBookmarkClick (as the direct-API counterpart) when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abe277d1-7f25-4cda-ba15-8880643e8dbb
📒 Files selected for processing (1)
src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx
Comment on lines
+82
to
+98
| try { | ||
| const user = await chrome.auth.getUser(); | ||
| if (!user) { | ||
| throw new Error('User not logged in'); | ||
| } | ||
| const account = user.identity.internal?.account_id; | ||
|
|
||
| const newState = !isBookmarked; | ||
| setIsBookmarked(newState); | ||
| await axios.post(`${API_BASE}${FAVORITES}?account=${account}`, { | ||
| quickstartName: result.resourceName, | ||
| favorite: newState, | ||
| }); | ||
| onBookmarkToggle?.(result.resourceName!, newState); | ||
| } catch (error) { | ||
| setIsBookmarked(result.isBookmarked ?? false); | ||
| } |
There was a problem hiding this comment.
Handle undefined account_id for external users.
user.identity.internal may be undefined for external users, causing account_id to be undefined. This would result in the API receiving ?account=undefined, which may cause unexpected behavior.
Additionally, the catch block silently swallows errors without logging, making debugging difficult.
🛠️ Suggested fix
const user = await chrome.auth.getUser();
if (!user) {
throw new Error('User not logged in');
}
const account = user.identity.internal?.account_id;
+ if (!account) {
+ console.warn('No account_id available for bookmarking');
+ return;
+ }
const newState = !isBookmarked;
setIsBookmarked(newState);
await axios.post(`${API_BASE}${FAVORITES}?account=${account}`, {
quickstartName: result.resourceName,
favorite: newState,
});
onBookmarkToggle?.(result.resourceName!, newState);
} catch (error) {
+ console.error('Failed to toggle bookmark:', error);
setIsBookmarked(result.isBookmarked ?? false);
}📝 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.
Suggested change
| try { | |
| const user = await chrome.auth.getUser(); | |
| if (!user) { | |
| throw new Error('User not logged in'); | |
| } | |
| const account = user.identity.internal?.account_id; | |
| const newState = !isBookmarked; | |
| setIsBookmarked(newState); | |
| await axios.post(`${API_BASE}${FAVORITES}?account=${account}`, { | |
| quickstartName: result.resourceName, | |
| favorite: newState, | |
| }); | |
| onBookmarkToggle?.(result.resourceName!, newState); | |
| } catch (error) { | |
| setIsBookmarked(result.isBookmarked ?? false); | |
| } | |
| try { | |
| const user = await chrome.auth.getUser(); | |
| if (!user) { | |
| throw new Error('User not logged in'); | |
| } | |
| const account = user.identity.internal?.account_id; | |
| if (!account) { | |
| console.warn('No account_id available for bookmarking'); | |
| return; | |
| } | |
| const newState = !isBookmarked; | |
| setIsBookmarked(newState); | |
| await axios.post(`${API_BASE}${FAVORITES}?account=${account}`, { | |
| quickstartName: result.resourceName, | |
| favorite: newState, | |
| }); | |
| onBookmarkToggle?.(result.resourceName!, newState); | |
| } catch (error) { | |
| console.error('Failed to toggle bookmark:', error); | |
| setIsBookmarked(result.isBookmarked ?? false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx`
around lines 82 - 98, The bookmark handler assumes
user.identity.internal.account_id exists; first retrieve the user via
chrome.auth.getUser() and validate that account_id is defined (from
user.identity.internal?.account_id) before calling axios.post—if account_id is
missing, either omit the account query parameter or abort and surface an error
instead of sending ?account=undefined; keep updating local state via
setIsBookmarked and still call onBookmarkToggle, but build the POST URL
conditionally (or throw) so the server never receives "undefined". Also update
the catch block to log the actual error (e.g., console.error or a logger) along
with context (include result.resourceName and attempted favorite state) before
resetting state, so failures are visible for debugging; reference
chrome.auth.getUser, account_id, setIsBookmarked, axios.post, and
onBookmarkToggle when making these changes.
CodyWMitchell
force-pushed
the
add-search-bookmarks
branch
from
60b5630 to
4fc7320
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx (1)
82-98:⚠️ Potential issue | 🟡 MinorGuard
account_idbefore building the bookmark URL.
accountcan still beundefinedhere, which produces?account=undefined, and the catch still drops the actual failure context. Please either omit the query param or bail out before posting, and log the failed resource/action.In the Red Hat Insights chrome `auth.getUser()` response, is `user.identity.internal.account_id` guaranteed for all users, or can `identity.internal` be undefined for external users?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx` around lines 82 - 98, Guard the account_id before constructing the bookmark URL: check chrome.auth.getUser()'s user.identity.internal?.account_id and if account is undefined either omit the `?account=` query param when calling axios.post or bail out early (do not call axios.post) and restore state via setIsBookmarked(result.isBookmarked ?? false); when you bail out or catch an error, include contextual logging that mentions the resource (result.resourceName) and action (bookmark toggle) so failures are visible; ensure onBookmarkToggle(result.resourceName!, newState) is only called after a successful post.
🤖 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/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsx`:
- Around line 594-619: Race condition: overlapping toggleFavoritePage calls can
apply out-of-order results—add a per-service token/nonce to ignore stale
responses. In handleFavoriteToggle create a unique token (timestamp or
incrementing id) and store it in a ref map keyed by pathname (e.g.,
favoriteToggleTokensRef.current[pathname] = token); before calling
toggleFavoritePage capture that token, and after the await only apply
setFavoritePages if favoriteToggleTokensRef.current[pathname] === token; keep
the optimistic setRawSearchResults update and in the catch revert only when the
token still matches to avoid undoing a newer change. Ensure to reference the
functions/variables: handleFavoriteToggle, toggleFavoritePage,
setRawSearchResults, setFavoritePages, and the new favoriteToggleTokensRef.
- Around line 456-470: The effect that syncs isFavorited only runs on
favoritePages changes and therefore can miss applying favorites to newly-arrived
rawSearchResults; update the useEffect dependencies to include rawSearchResults
(or rawSearchResults.length) so the effect also runs when new results arrive,
leaving the existing early-return and setRawSearchResults logic intact to avoid
unnecessary updates (symbols to change: useEffect, rawSearchResults,
setRawSearchResults, favoritePages).
In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx`:
- Around line 229-249: The aria-label strings added in SearchResultItem for the
bookmark and favorite buttons bypass react-intl; move these labels into the
messages file and replace the literal strings with intl.formatMessage calls (use
the existing messages object and the component's intl or useIntl hook).
Specifically update the aria-label on the bookmark Button and the favorite
Button (where handleFavoriteClick, isBookmarked, isFavorited, and FavoriteIcon
are used) to call intl.formatMessage for both the static labels ("Bookmark
learning resource" / "Unbookmark learning resource") and the interpolated ones
that include result.title (use a message with a {title} placeholder). Ensure you
import the new message keys into SearchResultItem and use
intl.formatMessage(...) where the current template literals are.
In `@src/utils/serviceFavorites.ts`:
- Around line 25-36: The fetchFavoritePages function swallows all errors and
returns [] on failure, making network/auth errors indistinguishable from a user
with no favorites; update the catch block in fetchFavoritePages to log the error
(console.error or processLogger) and rethrow the error (or return
Promise.reject(error)) instead of returning [], so callers can handle failures
via their .catch paths while preserving the existing successful-path behavior
that returns [] only when the response contains no favoritePages.
---
Duplicate comments:
In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx`:
- Around line 82-98: Guard the account_id before constructing the bookmark URL:
check chrome.auth.getUser()'s user.identity.internal?.account_id and if account
is undefined either omit the `?account=` query param when calling axios.post or
bail out early (do not call axios.post) and restore state via
setIsBookmarked(result.isBookmarked ?? false); when you bail out or catch an
error, include contextual logging that mentions the resource
(result.resourceName) and action (bookmark toggle) so failures are visible;
ensure onBookmarkToggle(result.resourceName!, newState) is only called after a
successful post.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd7f70fa-0413-41c5-a1a1-26969562bcf6
📒 Files selected for processing (8)
cypress/component/SearchResultItem.cy.tsxsrc/components/HelpPanel/HelpPanelTabs/LearnPanel.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResults.tsxsrc/components/common/BookmarkIcon.tsxsrc/components/common/FavoriteIcon.tsxsrc/utils/serviceFavorites.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/common/FavoriteIcon.tsx
- src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResults.tsx
- src/components/HelpPanel/HelpPanelTabs/LearnPanel.tsx
- cypress/component/SearchResultItem.cy.tsx
src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsx
Outdated
Show resolved
Hide resolved
src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx
Outdated
Show resolved
Hide resolved
CodyWMitchell
force-pushed
the
add-search-bookmarks
branch
from
ddd5a99 to
225e092
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx (1)
82-98:⚠️ Potential issue | 🟡 MinorHandle undefined
account_idto prevent malformed API requests.The
accountvariable (line 87) can beundefinedfor external users sinceuser.identity.internalmay not exist. This would result in the API receiving?account=undefined, potentially causing unexpected behavior. Additionally, the catch block silently swallows errors without logging, making debugging difficult.🛠️ Suggested fix
try { const user = await chrome.auth.getUser(); if (!user) { throw new Error('User not logged in'); } const account = user.identity.internal?.account_id; + if (!account) { + console.warn('No account_id available for bookmarking'); + return; + } const newState = !isBookmarked; setIsBookmarked(newState); await axios.post(`${API_BASE}${FAVORITES}?account=${account}`, { quickstartName: result.resourceName, favorite: newState, }); onBookmarkToggle?.(result.resourceName!, newState); } catch (error) { + console.error('Failed to toggle bookmark:', error); setIsBookmarked(result.isBookmarked ?? false); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx` around lines 82 - 98, The account value from chrome.auth.getUser() may be undefined; before calling axios.post in the bookmark toggle logic (inside the try block around chrome.auth.getUser, account, setIsBookmarked, axios.post, onBookmarkToggle), validate that account is present and only include the account query parameter or perform the API call when account is defined (otherwise abort the request and revert state), and in the catch block log the caught error (e.g., using console.error or the existing logger) while still restoring isBookmarked via setIsBookmarked(result.isBookmarked ?? false) so failures are visible and no malformed `?account=undefined` requests are sent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsx`:
- Around line 82-98: The account value from chrome.auth.getUser() may be
undefined; before calling axios.post in the bookmark toggle logic (inside the
try block around chrome.auth.getUser, account, setIsBookmarked, axios.post,
onBookmarkToggle), validate that account is present and only include the account
query parameter or perform the API call when account is defined (otherwise abort
the request and revert state), and in the catch block log the caught error
(e.g., using console.error or the existing logger) while still restoring
isBookmarked via setIsBookmarked(result.isBookmarked ?? false) so failures are
visible and no malformed `?account=undefined` requests are sent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9ed77d4-da52-4f81-9c33-8ca3e693dd11
📒 Files selected for processing (9)
cypress/component/SearchResultItem.cy.tsxsrc/Messages.tssrc/components/HelpPanel/HelpPanelTabs/LearnPanel.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchPanel.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResultItem.tsxsrc/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResults.tsxsrc/components/common/BookmarkIcon.tsxsrc/components/common/FavoriteIcon.tsxsrc/utils/serviceFavorites.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/common/BookmarkIcon.tsx
- src/components/HelpPanel/HelpPanelTabs/SearchPanel/SearchResults.tsx
- src/utils/serviceFavorites.ts
- src/components/common/FavoriteIcon.tsx
- src/components/HelpPanel/HelpPanelTabs/LearnPanel.tsx
apinkert
reviewed
Mar 12, 2026
| > | ||
| <StarIcon | ||
| color={ | ||
| !isFavorited ? 'var(--pf-t--global--icon--color--disabled)' : undefined |
Collaborator
There was a problem hiding this comment.
We should keep styles in their own .scss files, similar to the Bookmark icon
apinkert
reviewed
Mar 12, 2026
| <Icon className={className}> | ||
| <OutlinedBookmarkIcon className="lr-c-bookmark__icon" /> | ||
| <Icon isInline className={className}> | ||
| <OutlinedBookmarkIcon color="var(--pf-t--global--icon--color--disabled)" /> |
Collaborator
There was a problem hiding this comment.
same here, this should be added to the .scss file
apinkert
approved these changes
Mar 12, 2026
Collaborator
apinkert
left a comment
apinkert
left a comment
There was a problem hiding this comment.
LGTM outside of that css comment. We're still missing some services in our search but that falls out of scope of this PR
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/common/BookmarkIcon.scss (1)
8-10: Consider adding a hover state for the outlined icon.The filled bookmark icon has a hover state, but the outlined variant does not. For consistent interactive feedback, you may want to add a hover style.
♻️ Optional: Add hover state for outlined icon
.lr-c-bookmark__icon--outlined { color: var(--pf-t--global--icon--color--disabled); + &:hover { + color: var(--pf-t--global--icon--color--subtle); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/common/BookmarkIcon.scss` around lines 8 - 10, The outlined bookmark icon class (.lr-c-bookmark__icon--outlined) lacks a hover state; add a hover rule targeting .lr-c-bookmark__icon--outlined:hover (and optionally focus .lr-c-bookmark__icon--outlined:focus) to apply the same or matching hover color/transform used by the filled variant so interactive feedback is consistent—ensure you use the same CSS variables or values as the filled rule for color and any transition already defined on .lr-c-bookmark__icon to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/common/BookmarkIcon.scss`:
- Around line 8-10: The outlined bookmark icon class
(.lr-c-bookmark__icon--outlined) lacks a hover state; add a hover rule targeting
.lr-c-bookmark__icon--outlined:hover (and optionally focus
.lr-c-bookmark__icon--outlined:focus) to apply the same or matching hover
color/transform used by the filled variant so interactive feedback is
consistent—ensure you use the same CSS variables or values as the filled rule
for color and any transition already defined on .lr-c-bookmark__icon to keep
behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91482f09-4463-4d03-afaa-1095a9aaf75b
📒 Files selected for processing (4)
src/components/common/BookmarkIcon.scsssrc/components/common/BookmarkIcon.tsxsrc/components/common/FavoriteIcon.scsssrc/components/common/FavoriteIcon.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/common/FavoriteIcon.tsx
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.
RHCLOUD-44476
Adds bookmarking learning resources and favoriting services to the Search tab
