feat(quickstarts): add quickstarts store to LR#227
feat(quickstarts): add quickstarts store to LR#227aferd wants to merge 9 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:
No actionable comments were generated in the recent review. 🎉 Summary by CodeRabbit
WalkthroughAdds a federated QuickStarts system: a shared module-federated store, QuickStartProvider component, scoped quickstart hook, markdown quickstart link interception, component migrations to use the store, API-backed progress persistence, documentation, and Cypress component tests. Changes
Sequence DiagramssequenceDiagram
participant User
participant Component
participant Store as useQuickstartsStore
participant API as /api/quickstarts/v1
participant DOM as Browser
User->>Component: click "activate quickstart"
Component->>Store: activateQuickstart(name)
Store->>API: GET /api/quickstarts/v1/{name}
API-->>Store: quickstart payload
Store->>Store: update active & states
Store->>DOM: add drawer body class
Store-->>Component: updated state
Component-->>User: quickstart shown in drawer
User->>Component: update task / complete
Component->>Store: setAllQuickStartStates(updates)
Store->>API: POST /api/quickstarts/v1/progress
API-->>Store: confirmation
Store-->>Component: persisted state
sequenceDiagram
participant Markdown
participant Extension as Quickstart Link Extension
participant LinkStore as useQuickstartLinkStore
participant DOM as Browser DOM
participant History as History API
Markdown->>Extension: process [text](url)
Extension->>Extension: parse & detect internal link
Extension->>LinkStore: addLinkElement(id)
LinkStore->>DOM: poll for element by id
DOM-->>LinkStore: element found
LinkStore->>DOM: attach click handler
Extension-->>Markdown: return anchor markup with id
User->>DOM: click link
DOM->>LinkStore: handler intercepts click
LinkStore->>History: replaceState with quickstartLink flag
LinkStore->>DOM: dispatch popstate event
DOM-->>User: SPA navigation / quickstart activation (no reload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Hyperkid123
left a comment
Hyperkid123
left a comment
There was a problem hiding this comment.
Could you please run the testing agent on this code and generate unit tests? I think some of the code is not doing what we want.
|
/retest |
Hyperkid123
left a comment
Hyperkid123
left a comment
There was a problem hiding this comment.
I think it's a good progress. I'll do a full review once the PR is no longer a draft 🙂
|
/retest |
|
I think there is a small bug. A quickstart can have a "dependency". On the network log you can see that the first quicckstart has a "next quickstart" data and you can also see that the UI correctly makes the next request. However, in the state, the next quickstart is not present: 
The state only has the one that was explicitly requested (the first one) |
| const handleResourceClick = () => { | ||
| if (resource.spec.type?.text === 'Quick start') { | ||
| chrome.quickStarts.activateQuickstart(resource.metadata.name); | ||
| activateQuickstart(resource.metadata.name); |
There was a problem hiding this comment.
When I click this, nothing happens. I assume we need the Chrome chages as well?
There was a problem hiding this comment.
yep, you should be able to run it alongside this PR RedHatInsights/insights-chrome#3346.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/quickstarts-api.md`:
- Around line 243-248: The import uses an internal src path
('learning-resources/src/types/quickstarts') which will break for external
consumers; update usage of AllQuickStartStates, ScopedControllerOptions, and
ScopedQuickStartController to import from a public export (e.g. re-export these
types from the federated module's package entry or from the hook modules) or
update package exports to expose a proper path (e.g. 'learning-resources' or
'learning-resources/types/quickstarts') and change the import accordingly, or
alternatively document that consumers should import these types from the hook
modules that re-export them.
In `@src/components/QuickStartProvider/useQuickstartLinkStore.tsx`:
- Around line 11-45: addLinkElement currently creates a setInterval and attaches
an element click listener but neither the interval nor the listener are
tracked/removed on unmount; modify addLinkElement to record the interval id (and
the bound click handler) into a ref (useRef) or have it return a cleanup
function, call clearInterval(intervalId) when the element is found or when
iterations exceed the limit, and store the click handler so you can call
element.removeEventListener('click', handler) during cleanup (e.g., in the
parent hook's useEffect cleanup). Ensure you import/use useRef, keep storing
elements in store.set(id, element), and always clear intervals and remove
listeners to avoid leaks.
In `@src/stores/quickstartsStore.ts`:
- Around line 268-273: The code calls axios.post('/api/quickstarts/v1/progress')
with parseInt(state.accountId) without validating state.accountId; ensure
accountId is a valid integer before sending: check that typeof state.accountId
=== 'string' (or number), trim and test with /^[0-9]+$/ or Number.isInteger
after Number(...) to avoid NaN, and only call axios.post when the
validatedAccountId is a finite integer (otherwise skip the request or handle the
error/log), updating the block around activeState and state.accountId where
parseInt is used.
🧹 Nitpick comments (12)
src/components/QuickStartProvider/useQuickstartLinkStore.tsx (1)
73-78: Fragile markdown link parsing.The regex
/\[.*\]\(.*\)/gis greedy and the string splitting approach won't handle edge cases like:
- Links with
]in the text:[foo]bar](url)- Links with
(or)in the URL:[text](url?foo=(bar))- Multiple links in the same match
Consider using a more robust regex or a proper markdown parser.
♻️ Suggested more robust regex
- // matches MD links like [link text](https://example.com) - regex: /\[.*\]\(.*\)/g, + // matches MD links - non-greedy to handle multiple links + regex: /\[([^\]]+)\]\(([^)]+)\)/g, replace: (text: string) => { try { - let [linkText, linkURL] = text.split(']('); - linkText = linkText.replace(/^\[/, ''); - linkURL = linkURL.replace(/\)$/, ''); + const match = /\[([^\]]+)\]\(([^)]+)\)/.exec(text); + if (!match) return text; + const [, linkText, linkURL] = match;src/components/HelpPanel/HelpPanelTabs/LearnPanel.tsx (1)
86-92: Consider handling async activation errors.
activateQuickstartis an async function (per the store implementation) but is called withoutawaitor error handling. While fire-and-forget may be intentional here, any API errors will be silently swallowed.♻️ Optional: Add error handling
- const handleResourceClick = () => { + const handleResourceClick = async () => { if (resource.spec.type?.text === 'Quick start') { - activateQuickstart(resource.metadata.name); + try { + await activateQuickstart(resource.metadata.name); + } catch (error) { + console.error('Failed to activate quickstart:', error); + } } else if (resource.spec.link?.href) { window.open(resource.spec.link.href, '_blank'); } };src/types/quickstarts.ts (1)
7-7: Consider usingstringonly for QuickStart keys.The
Record<string | number, QuickStartState>allows numeric keys, but QuickStart names are always strings. The actual state reducer at line 365 explicitly uses{ [key: string]: QuickStartState }, and all QuickStart identifiers throughout the codebase are typed as strings. Usingnumberis unintended and creates type inconsistency.♻️ Suggested change
-export type AllQuickStartStates = Record<string | number, QuickStartState>; +export type AllQuickStartStates = Record<string, QuickStartState>;README.md (1)
31-34: Minor inconsistency in module path format.The example uses
'./quickstarts/useQuickstartsStore'but the table on line 22 showslearning-resources/quickstarts/useQuickstartsStore. Consider aligning these for clarity, or add a note explaining the difference between scope-relative and full paths.cypress/component/hooks/useQuickstartsStore.cy.tsx (1)
153-169: Consider test isolation approach.The
beforeEachmounts a component to clear the store, then individual tests mount again. This works because the store is a singleton, but the double-mount pattern is unusual. Consider extracting store reset logic or using a utility function for cleaner test isolation.Also, async operations (
activateQuickstart,loadProgress) aren't tested here since they require API mocking. Consider adding integration tests with mocked API calls in a follow-up.src/components/QuickStartProvider/QuickStartProvider.tsx (3)
58-65: Effect dependencies omit stable functions.The effect uses
setAccountIdandloadProgressbut they're not in the dependency array. While the comment indicates this is intentional to only react toaccountIdchanges, ESLint's exhaustive-deps rule may flag this. If these functions are stable (memoized or from a stable store), this is fine; otherwise consider adding them to dependencies or wrapping withuseCallbackin the store.💡 Option: Include stable dependencies
useEffect(() => { if (accountId && accountId !== initializedAccountIdRef.current) { initializedAccountIdRef.current = accountId; setAccountId(accountId); loadProgress(); } - }, [accountId]); // Only depend on accountId prop + }, [accountId, setAccountId, loadProgress]);
74-77: Type casts may mask type mismatches.The
asandas unknown ascasts suggest the store's function signatures don't exactly matchQuickStartContainerProps. This works but hides potential incompatibilities. Consider aligning the store's types with PatternFly's expected signatures, or add a comment explaining why the casts are safe.
67-68: Consider memoizinggetAllQuickstarts()call.
getAllQuickstarts()is called on every render. If the quickstarts list is large or the component re-renders frequently, this could impact performance. Consider usinguseMemo:💡 Memoize quickstarts array
+ // Memoize to avoid recalculating on every render + const quickStarts = useMemo(() => getAllQuickstarts(), [getAllQuickstarts]); - // Get quickstarts - this will update when store state changes - const quickStarts = getAllQuickstarts();src/hooks/useScopedQuickStart.ts (1)
63-81: Potential stale closure in sync mode.In sync mode (lines 69-75), when
statesOrUpdateris a function, it reads fromstore.allQuickStartStateswhich is captured at callback creation time. If multiple state updates happen in quick succession, this could lead to stale data. The store'ssetAllQuickStartStatesalready handles functional updates internally, so consider passing the function directly when in sync mode.💡 Pass functional updater directly to store
const setAllQuickStartStates = useCallback( ( statesOrUpdater: | AllQuickStartStates | ((prev: AllQuickStartStates) => AllQuickStartStates) ) => { if (syncWithStore) { - // Store expects the full state object, not a setter function - const newStates = - typeof statesOrUpdater === 'function' - ? statesOrUpdater(store.allQuickStartStates) - : statesOrUpdater; - store.setAllQuickStartStates(newStates); + // Store's setAllQuickStartStates handles both objects and functions + store.setAllQuickStartStates(statesOrUpdater); } else { setLocalAllQuickStartStates(statesOrUpdater); } }, - [syncWithStore, store] + [syncWithStore, store.setAllQuickStartStates] );src/stores/quickstartsStore.ts (3)
128-134: Clarify the function type check.The check
typeof id !== 'function'on line 129 is defensive butidis typed asstring. If this guards against a specific edge case (e.g., incorrect payload from older code), add a comment explaining why. Otherwise, the check can be simplified.💡 Simplify the condition
case 'SET_ACTIVE_QUICKSTART': { const id = payload as EventPayload['SET_ACTIVE_QUICKSTART']; // Handle body class for drawer visibility if (typeof document !== 'undefined') { - if (id !== '' && typeof id !== 'function') { + if (id !== '') { document.body.classList.add('quickstarts-open'); } else { document.body.classList.remove('quickstarts-open'); } }
269-281: API persistence is fire-and-forget; consider implications.The progress API call updates the store regardless of whether the POST succeeds. If the API fails, the local state will differ from the server state. For better resilience, consider:
- Retrying failed requests
- Queuing failed updates for later retry
- Notifying the user of sync failures
For now, the current approach is reasonable for non-critical progress data, but document this trade-off.
311-313: Use Set for deduplication.The current
filterwithindexOfis O(n²). For a small number of quickstarts this is negligible, but usingSetis more idiomatic:💡 Use Set for deduplication
// 2. Extract nextQuickStart references - const nextQuickStartNames = mainQuickstarts - .flatMap((qs) => qs.spec.nextQuickStart || []) - .filter((qsName, index, arr) => arr.indexOf(qsName) === index); // Remove duplicates + const nextQuickStartNames = [ + ...new Set(mainQuickstarts.flatMap((qs) => qs.spec.nextQuickStart || [])) + ];
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 18-41: The example uses scope: 'learningResources' and module:
'./quickstarts/useQuickstartsStore' which doesn’t match the table's
'learning-resources/quickstarts/useQuickstartsStore'; update the example to use
the same module federation notation as the table (e.g., set scope to
'learning-resources' and module to 'quickstarts/useQuickstartsStore' or adjust
the table to 'learningResources/quickstarts/useQuickstartsStore') so consumers
configuring useRemoteHook (and calling useQuickstartsStore/useScopedQuickStart
via hookResult) use a consistent scope/module pair.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/quickstarts-api.md`:
- Around line 113-116: Update the docs for the ScopedControllerOptions interface
to document the syncWithStore boolean option (alongside quickStarts/QuickStart),
describing its default value, what "syncing with the store" means for controller
state and quickStart lifecycle, and when consumers should set it true or false
(e.g., to opt out of automatic store updates or to allow controller changes to
reflect in the global store). Reference the symbol ScopedControllerOptions and
the syncWithStore flag in the prose so readers can find it next to the existing
quickStarts/QuickStart entry and include a short example or note about expected
side effects on quickStart availability when toggled.
- Around line 139-167: The diagram's fenced code block should include a language
tag to satisfy markdownlint; update the opening fence for the ASCII diagram to
use a language identifier (e.g., change "```" to "```text") so the block
containing the useQuickstartsStore / useScopedQuickStart ASCII art is fenced as
```text while leaving the content and closing fence unchanged.
In `@src/stores/quickstartsStore.ts`:
- Around line 318-340: The current fetch of referenced quickstarts uses
Promise.all which rejects on the first failed axios.get and discards all
results; update the block that builds promises from nextQuickStartNames and
awaiting them to use Promise.allSettled, then filter for results with status
"fulfilled", extract their .value.data, and pass those to
parseQuickstartsResponse to populate nextQuickstarts while logging or ignoring
rejected entries; update references in this change include nextQuickStartNames,
axios.get<QuickStartAPIResponse>, responses (the settled results) and
parseQuickstartsResponse to locate and implement the fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/stores/quickstartsStore.ts`:
- Around line 302-350: The activateQuickstart flow currently overwrites the
store and sets setActiveQuickStartID(name) even when the API returns no
quickstarts; update activateQuickstart to bail out (or preserve previous state)
if parseQuickstartsResponse(main response) yields an empty array — do not call
setQuickstarts('default', ...) or setActiveQuickStartID(...) in that case; when
a quickstart is returned, prefer the quickstart's metadata.name (from
mainQuickstarts[0].metadata.name) as the active ID instead of the input name,
and only then fetch/merge referenced nextQuickstarts and update the store with
allQuickstarts (main + referenced).
🧹 Nitpick comments (1)
src/stores/quickstartsStore.ts (1)
360-381: Avoid persisting progress as a side effect of loading.
loadProgresscallssetAllQuickStartStates, which can trigger a write-back for the active quickstart. Consider updating state directly here to keep load idempotent.♻️ Suggested change
- setAllQuickStartStates(states); + store.updateState('SET_ALL_STATES', states);
| const activateQuickstart = async (name: string) => { | ||
| try { | ||
| // 1. Fetch main quickstart | ||
| const { data } = await axios.get<QuickStartAPIResponse>( | ||
| '/api/quickstarts/v1/quickstarts', | ||
| { | ||
| params: { name }, | ||
| } | ||
| ); | ||
| const mainQuickstarts = parseQuickstartsResponse(data); | ||
|
|
||
| // 2. Extract nextQuickStart references | ||
| const nextQuickStartNames = mainQuickstarts | ||
| .flatMap((qs) => qs.spec.nextQuickStart || []) | ||
| .filter((qsName, index, arr) => arr.indexOf(qsName) === index); // Remove duplicates | ||
|
|
||
| // 3. Fetch referenced quickstarts | ||
| let nextQuickstarts: QuickStart[] = []; | ||
| if (nextQuickStartNames.length > 0) { | ||
| const results = await Promise.allSettled( | ||
| nextQuickStartNames.map((nextName) => | ||
| axios.get<QuickStartAPIResponse>( | ||
| '/api/quickstarts/v1/quickstarts', | ||
| { params: { name: nextName } } | ||
| ) | ||
| ) | ||
| ); | ||
| const errors: unknown[] = []; | ||
| const fulfilled: QuickStart[] = []; | ||
| for (const result of results) { | ||
| if (result.status === 'fulfilled') { | ||
| fulfilled.push(...parseQuickstartsResponse(result.value.data)); | ||
| } else { | ||
| errors.push(result.reason); | ||
| } | ||
| } | ||
| if (errors.length) { | ||
| console.warn( | ||
| 'Some referenced quickstarts could not be fetched:', | ||
| errors | ||
| ); | ||
| } | ||
| nextQuickstarts = fulfilled; | ||
| } | ||
|
|
||
| // 4. Populate both main and referenced quickstarts | ||
| const allQuickstarts = [...mainQuickstarts, ...nextQuickstarts]; | ||
| setQuickstarts('default', allQuickstarts); | ||
| setActiveQuickStartID(name); |
There was a problem hiding this comment.
Guard against empty API results before activating.
If the API returns an empty list, this still sets activeQuickStartID and overwrites the store with an empty array, which leaves the UI “active” with no data. Bail out (or keep previous state) unless a real quickstart is returned, and use the actual metadata name when available.
🛠️ Proposed fix
const { data } = await axios.get<QuickStartAPIResponse>(
'/api/quickstarts/v1/quickstarts',
{
params: { name },
}
);
const mainQuickstarts = parseQuickstartsResponse(data);
+ if (mainQuickstarts.length === 0) {
+ console.warn('Quickstart not found:', name);
+ return;
+ }
+ const activeId = mainQuickstarts[0].metadata?.name ?? name;
@@
const allQuickstarts = [...mainQuickstarts, ...nextQuickstarts];
setQuickstarts('default', allQuickstarts);
- setActiveQuickStartID(name);
+ setActiveQuickStartID(activeId);🤖 Prompt for AI Agents
In `@src/stores/quickstartsStore.ts` around lines 302 - 350, The
activateQuickstart flow currently overwrites the store and sets
setActiveQuickStartID(name) even when the API returns no quickstarts; update
activateQuickstart to bail out (or preserve previous state) if
parseQuickstartsResponse(main response) yields an empty array — do not call
setQuickstarts('default', ...) or setActiveQuickStartID(...) in that case; when
a quickstart is returned, prefer the quickstart's metadata.name (from
mainQuickstarts[0].metadata.name) as the active ID instead of the input name,
and only then fetch/merge referenced nextQuickstarts and update the store with
allQuickstarts (main + referenced).
|
/retest |
2 similar comments
|
/retest |
|
/retest |
3 participants
Decoupling: Chrome was never the right owner of QuickStarts - it's a shell/framework, not a content provider. Learning Resources is the natural home since it already owns QuickStart content, the catalog, and the creator.
Before: insights-chrome owns QuickStarts API → all apps depend on chromeAfter: learning-resources owns QuickStarts API → chrome becomes a consumer
Benefits:
Separation of concerns - Chrome handles auth/navigation, Learning Resources handles educational content
Independent releases - QuickStarts API can evolve without Chrome releases
Cleaner dependency graph - Apps import from the content owner, not the shell
depends on RedHatInsights/insights-chrome#3346