feat: zustand refactor and playwright tests for MIP flow#248
feat: zustand refactor and playwright tests for MIP flow#248pinkman7009 wants to merge 8 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors state management across AI-creation Mitra flow components from direct store method calls to selector-based access patterns, centralizing state reads and writes. Adds comprehensive end-to-end test suite for Micro Improvement Plan flow including test data constants, route definitions, and Playwright page object automation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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)
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 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/pages/ai-creation/pages/improvement-plan/index.jsx (1)
10-17:⚠️ Potential issue | 🟠 Major
itemsmissing fromuseEffectdeps —mediastate goes stale when store updates.
itemsis read from the Zustand selector (reactive), but theuseEffectthat syncs it into localmediastate only depends on[projectId]. Ifstate.mediachanges in the store without aprojectIdchange, the component will render with stalemedia.The local
mediastate appears redundant sinceitemsis already reactive. Consider removing it:Proposed simplification
const ImprovementPlan = () => { - const [media, setMedia] = useState([]); const { projectId } = useParams(); - const items = useAICreationSessionStore(state => state.media); - - useEffect(() => { - const mediaItems = items || []; - setMedia(mediaItems); - }, [projectId]); + const media = useAICreationSessionStore(state => state.media) || [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/improvement-plan/index.jsx` around lines 10 - 17, The local media state is kept in sync by a useEffect that only depends on projectId causing stale media when items (from useAICreationSessionStore) updates; either add items to the dependency array of the useEffect so it runs when items changes, or remove the redundant local state entirely and use items directly in the component; update references to media/setMedia accordingly and ensure the useEffect, useState, items, setMedia, useAICreationSessionStore and projectId symbols are adjusted to reflect the chosen approach.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx (1)
152-209:⚠️ Potential issue | 🔴 CriticalData corruption: sequential store writes in
onWebSocketMessageoverwrite each other with stale references.Lines 163-174 (block 1) correctly handle the case where
message.msgis present, including sources and file_url. However, lines 176-180 (block 2) and 182-185 (block 3) run independently and use the originallastMessagecaptured on line 160 — not the updated version written by block 1.When a bot message carries both
msgandsources/file_url, block 2 overwrites block 1's result (dropping the appended msg text), and block 3 overwrites block 2's result (dropping sources). The final store state only retainsfile_url.Blocks 2 and 3 should only execute when block 1 did not run:
Proposed fix
if (message?.msg) { let updatedLastMessage = { ...lastMessage, msg: lastMessage.msg + message.msg }; if (Array.isArray(message?.extra_content?.sources) && message?.extra_content?.sources.length) { updatedLastMessage["sources"] = message?.extra_content?.sources; } if(message?.extra_content?.file_url) { updatedLastMessage["file_url"] = message?.extra_content?.file_url; } setCommonFlowChatHistory([...prevChatHistory.slice(0, lastIndex), updatedLastMessage]); - } - - if (Array.isArray(message?.extra_content?.sources) && message?.extra_content?.sources.length) { - let updatedLastMessage = { ...lastMessage }; - updatedLastMessage["sources"] = message?.extra_content?.sources; - setCommonFlowChatHistory([...prevChatHistory.slice(0, lastIndex), updatedLastMessage]); - } - - if(message?.extra_content?.file_url) { - let updatedLastMessage = { ...lastMessage, file_url: message?.extra_content?.file_url }; - setCommonFlowChatHistory([...prevChatHistory.slice(0, lastIndex), updatedLastMessage]); + } else { + let updatedLastMessage = { ...lastMessage }; + if (Array.isArray(message?.extra_content?.sources) && message?.extra_content?.sources.length) { + updatedLastMessage["sources"] = message?.extra_content?.sources; + } + if (message?.extra_content?.file_url) { + updatedLastMessage["file_url"] = message?.extra_content?.file_url; + } + if (updatedLastMessage !== lastMessage) { + setCommonFlowChatHistory([...prevChatHistory.slice(0, lastIndex), updatedLastMessage]); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx` around lines 152 - 209, In onWebSocketMessage, avoid sequential overwrites by building a single updatedLastMessage from lastMessage and applying msg, extra_content.sources and extra_content.file_url together, then call setCommonFlowChatHistory once; specifically, in the branch where lastIndex >= 0 && lastMessage?.source === 'bot', replace the three separate update blocks that each create updatedLastMessage (the block that appends message.msg, the block that sets sources, and the block that sets file_url) with a single flow that: create let updatedLastMessage = { ...lastMessage }; if (message?.msg) updatedLastMessage.msg = lastMessage.msg + message.msg; if (Array.isArray(message?.extra_content?.sources) && message?.extra_content?.sources.length) updatedLastMessage.sources = message.extra_content.sources; if (message?.extra_content?.file_url) updatedLastMessage.file_url = message.extra_content.file_url; then call setCommonFlowChatHistory([...prevChatHistory.slice(0, lastIndex), updatedLastMessage]).src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/ActionItems.jsx (1)
340-351:⚠️ Potential issue | 🟡 MinorFix ternary operator precedence bug in bot message construction.
The
+operator has higher precedence than?:, sot("actionItems.takeActionItems") + "\n" + hasClickedOnAddmorealways evaluates to a truthy string, making the ternary always pick the first branch. The intended conditional logic is never reached. Wrap the ternary in parentheses to fix:message: (hasClickedOnAddmore ? t("actionItems.takeActionItems") + "\n" + t("actionItems.craftYourOwnActionPlan") : (hasClickedOnAddmore ? t("actionItems.finalizeActionList") + "\n" + t("actionItems.addEachStep") : t("actionItems.finalizeActionList") + "\n" + t("actionItems.editReorderDeleteActions") + "\n" + JSON.stringify(getActionListStore())))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/ActionItems.jsx` around lines 340 - 351, The message string in the botMessage object (symbols: botMessage, hasClickedOnAddmore, getActionListStore, t, BOT) suffers from ternary/concatenation precedence so the conditional always picks the first branch; fix by parenthesizing the ternary expressions and rearranging the concatenation so the intended branches are evaluated (e.g., wrap each conditional in (...) before concatenating newline and translation strings, and ensure the nested conditions use explicit parentheses so the correct t(...) values or JSON.stringify(getActionListStore()) are appended).src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/TitleGeneration.jsx (1)
182-208:⚠️ Potential issue | 🟡 MinorSession marked
COMPLETEDbefore project creation — partial state on failure.
updateChatSessionsetssession_status: "COMPLETED"(line 169) beforecreateProjectis called (line 198). IfcreateProjectfails, the session is stuck inCOMPLETEDstate with no associated project. The catch block redirects to login (line 238), which doesn't roll back the session status.Consider reordering: create the project first, then mark the session as completed. Or add rollback/retry logic in the catch block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/TitleGeneration.jsx` around lines 182 - 208, The code calls updateChatSession (which sets session_status: "COMPLETED") before calling createProject, so if createProject fails the session remains completed; change the flow to call createProject first and only call updateChatSession to mark COMPLETED after createProject returns successfully (or, alternatively, add rollback logic in the catch that calls updateChatSession with the previous status to revert), and ensure the catch block handles createProject failures by retrying or resetting session state using the same session identifier.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/WeeksSelection.jsx (1)
137-164:⚠️ Potential issue | 🟠 MajorStale closure:
onWebSocketMessagecaptures a stalehandleContinueClick.
onWebSocketMessageis memoized withuseCallback([handleScrollIntoView])(line 163), but it callshandleContinueClick(line 160) which is a regular function recreated every render. The memoized callback will always invoke thehandleContinueClickcaptured at the timehandleScrollIntoViewlast changed.
handleContinueClickreadsdurationIntroMessagefrom the component scope (line 222). Since the intro message is fetched asynchronously (lines 61-84), it will likely beundefined/nullwhenonWebSocketMessageis first memoized. The fallback|| t("weeksSelection.howManyWeeks")on line 222 masks this partially, but the intended API-fetched message will be lost for thesaveUserChatsInDBcall.Fix by either: (a) wrapping
handleContinueClickinuseCallbackand adding it toonWebSocketMessage's deps, or (b) using a ref forhandleContinueClick.🛡️ Proposed fix using a ref
+ const handleContinueClickRef = useRef(handleContinueClick); + useEffect(() => { + handleContinueClickRef.current = handleContinueClick; + }); const onWebSocketMessage = useCallback( (event) => { const data = JSON.parse(event.data); const message = data?.text; if (message?.msg && message?.source === "bot") { // ... unchanged ... } else if ( message?.source === "bot" && message?.extra_content?.should_move_forward === "yes" ) { const numOfWeeks = message?.extra_content?.query?.match(/\d+/)?.[0] ?? 1; - handleContinueClick(Number(numOfWeeks)); + handleContinueClickRef.current(Number(numOfWeeks)); } }, [handleScrollIntoView] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/WeeksSelection.jsx` around lines 137 - 164, onWebSocketMessage is memoized with useCallback([handleScrollIntoView]) but calls handleContinueClick which is recreated every render, causing a stale closure and losing the fetched durationIntroMessage used by saveUserChatsInDB; make handleContinueClick stable by wrapping it in useCallback and include it in the dependency array of onWebSocketMessage (also ensure the new useCallback for handleContinueClick includes durationIntroMessage and any other state it reads), so onWebSocketMessage always invokes the latest handleContinueClick and preserves the API-fetched intro message.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx (1)
150-150:⚠️ Potential issue | 🟠 MajorChange line 150 to read chunks from the API response, not the array:
setChunksStore(fetched_objectiveList?.chunks)instead ofobjective_list?.chunks
objective_listis an array (destructured with default= []on line 127), and arrays have no.chunksproperty. The current code at line 150 storesundefined, which then crashes at line 286 whenJSON.parse(getChunks())attempts to parseundefined. Thechunksproperty comes from the API response object itself (fetched_objectiveList), not from theobjective_listarray within it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx` at line 150, The code sets chunks from the wrong variable: replace the use of objective_list?.chunks with the API response object's chunks so setChunksStore gets the actual data; update the call to setChunksStore to use fetched_objectiveList?.chunks (since objective_list is an array and has no .chunks), ensuring getChunks()/JSON.parse later receives the proper value from fetched_objectiveList.
🧹 Nitpick comments (8)
tests/e2e/flows/mip-flow/happy-path.spec.ts (1)
64-107: Excessive hardcodedwait()calls make tests slow and flaky.Multiple
wait(3000),wait(5000), andwait(15000)calls throughout the flow total ~40+ seconds of fixed delays. These are fragile — they may be insufficient on slow CI or wasteful on fast machines. Where possible, replace with condition-based waits (e.g.,waitForBotResponse,waitForElement, orwaitForURL).For example, at line 76, instead of
wait(3000)after each challenge message, usewaitForBotResponsewith an incremented expected count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/flows/mip-flow/happy-path.spec.ts` around lines 64 - 107, The test uses many fixed wait(...) delays making it slow and flaky; replace hardcoded sleeps with condition-based waits: after sending initial or define-challenge messages use mipFlowPage.waitForBotResponse(expectedMessageCount) or mipFlowPage.waitForDefineChallengeLoad()/waitForWeeksSelectionLoad()/waitForObjectivesLoad()/waitForFinalActionPage() as appropriate instead of wait(3000)/wait(5000)/wait(15000); when looping through MIP_DEFINE_CHALLENGE_MESSAGES call sendDefineChallengeMessage then await mipFlowPage.waitForBotResponse(i + 1) (or another element-based checker) to wait for the specific reply, and remove the extra waits after selectObjectiveByIndex, clickObjectiveNext, clickActionItemNext and sendWeeksSelection replacing them with waits for the next page/state via the existing waitFor* helpers or a new waitForElement/waitForURL helper.tests/pages/mip-flow.page.ts (1)
81-83: Hardcoded route duplicatesSITE_ROUTES.MITRA_CHAT.The path
"/mohini/mitra-chat"is already defined intests/constant/site_routes.ts. Consider importing and using the constant for single-source-of-truth maintenance.Proposed fix
+import { SITE_ROUTES } from "../constant/site_routes" ... async navigateToMitraChat(): Promise<void> { - await this.navigate("/mohini/mitra-chat") + await this.navigate(SITE_ROUTES.MITRA_CHAT) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/pages/mip-flow.page.ts` around lines 81 - 83, Replace the hardcoded route string in navigateToMitraChat with the canonical constant: import SITE_ROUTES (or the specific MITRA_CHAT export) from tests/constant/site_routes.ts and change await this.navigate("/mohini/mitra-chat") inside navigateToMitraChat() to await this.navigate(SITE_ROUTES.MITRA_CHAT); ensure the import is added at the top of the file and remove the literal string to keep a single source of truth.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx (3)
464-465:getPrevObjective()andgetPrevObjectiveSource()are imperative reads during render — they won't react to store changes.If the previous-objective data in the store is updated after this component mounts (e.g., by another component or async flow), these calls will return stale data on subsequent renders because they read via
getState()rather than a reactive selector. IfprevObjectiveShownis toggled after the store is updated, the values read here will be current at that instant, but this is fragile.If this is intentional (values are guaranteed stable once set), no change needed. Otherwise, consider reading these via selectors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx` around lines 464 - 465, The code is calling getPrevObjective() and getPrevObjectiveSource() imperatively during render which can return stale data because they read store state directly; change the component to read previous-objective data via reactive selectors instead: replace direct calls to getPrevObjective() and getPrevObjectiveSource() inside the render branch that uses prevObjectiveShown with selector-based reads (e.g., usePrevObjectiveSelector / useSelector(getPrevObjective) or equivalent hook provided by your store) and fall back to objectiveList / objectiveSource when prevObjectiveShown is false so the component updates when store data changes.
87-111: Side effects insideuseStateinitializer are unreliable.Lines 103-104 call
setSelectedIndicesandsetSelectedObjectives(state setters for other state variables) inside theuseStateinitializer forvisibleCount. React does not guarantee that state setters called during another state's lazy initializer will behave correctly—this can lead to dropped updates or render-time warnings depending on React's batching.Move this initialization logic into a
useEffector compute all three values (visibleCount,selectedIndices,selectedObjectives) together before callinguseState.♻️ Proposed refactor: compute initial values together
+ // Compute initial selections from store + const initialSelections = React.useMemo(() => { + const defaultValueToShow = 3; + const storedObjectiveList = getObjective() || []; + const storedSelectedObjectives = getSelectedObjective(); + const selectedObjectivesArray = Array.isArray(storedSelectedObjectives) + ? storedSelectedObjectives + : (storedSelectedObjectives ? [storedSelectedObjectives] : []); + const indices = selectedObjectivesArray + .map(obj => storedObjectiveList.findIndex(o => o?.text === obj || o === obj)) + .filter(idx => idx !== -1); + const maxSelectedIndex = Math.max(...indices, -1); + const visibleCount = maxSelectedIndex !== -1 && maxSelectedIndex > defaultValueToShow - 1 + ? maxSelectedIndex + 1 + : defaultValueToShow; + return { indices, objectives: indices.map(idx => storedObjectiveList[idx]), visibleCount }; + }, []); - const [selectedIndices, setSelectedIndices] = useState([]); - const [selectedObjectives, setSelectedObjectives] = useState([]); + const [selectedIndices, setSelectedIndices] = useState(initialSelections.indices); + const [selectedObjectives, setSelectedObjectives] = useState(initialSelections.objectives); - const [visibleCount, setVisibleCount] = useState(() => { - const defaultValueToShow = 3; - const objectiveList = getObjective() || []; - const storedSelectedObjectives = getSelectedObjective(); - ... - setSelectedIndices(indices); - setSelectedObjectives(indices.map(idx => objectiveList[idx])); - ... - }); + const [visibleCount, setVisibleCount] = useState(initialSelections.visibleCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx` around lines 87 - 111, The useState lazy initializer for visibleCount performs side effects by calling setSelectedIndices and setSelectedObjectives, which is unreliable; instead compute the initial visibleCount, selectedIndices, and selectedObjectives together (using getObjective and getSelectedObjective) and initialize state without calling setters, or move the logic into a useEffect that runs on mount: extract the current objectiveList and storedSelectedObjectives, normalize storedSelectedObjectives into selectedObjectivesArray, compute indices and visibleCount (using Math.max as currently done), then call setSelectedIndices(indices), setSelectedObjectives(indices.map(...)) and setVisibleCount(computedValue) inside that effect (or initialize all three states from the precomputed values before any setters are used). Ensure you update the code references: the visibleCount useState initializer, setSelectedIndices, setSelectedObjectives, getObjective, and getSelectedObjective.
44-62: Mixing reactive selectors and imperativegetState()getters in the same component can confuse maintainability.Lines 45-49 use reactive selectors (
useAICreationSessionStore(state => state.X)) that trigger re-renders, while lines 51-62 destructure getters/setters fromgetState()that never trigger re-renders. This dual pattern is valid in zustand but can be confusing for future maintainers—some store values will re-render the component when changed (e.g.,objectiveChatHistory,selectedObjective) while others won't (e.g., anything read viagetObjective(),getSession()).Consider documenting or standardizing which values are read reactively vs. imperatively, to avoid subtle bugs if a future developer expects
getObjective()to cause re-renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx` around lines 44 - 62, The component mixes reactive selectors (objectiveChatHistory, selectedObjective, profileId, preferredLanguage, isOwnObjective via useAICreationSessionStore(state => state.X)) with imperative getters/setters from useAICreationSessionStore.getState() (getObjective, getSelectedObjective, getIsPrevObjectiveShown, getObjectiveSource, getPrevObjective, getPrevObjectiveSource, getChunks, getSystemError, getUserProblemStatement, getSession and their corresponding setters), which is confusing; either convert the frequently-read getters (e.g., getObjective, getSession, getSelectedObjective) to reactive selectors so the component re-renders when they change, or keep them imperative but add a clear comment above the destructure explaining why getState() is used and which values deliberately do not trigger re-renders, and update usages of setSelectedObjective/setSelectedObjectiveSource/setChunks/etc. to match the chosen pattern for consistency.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/TitleGeneration.jsx (1)
158-162: Emptyifblock — invert the condition for clarity.The success case
if (validate_response?.result) {}has an empty body and all logic is in theelse. Invert the condition to eliminate the empty block.♻️ Proposed refactor
- if (validate_response?.result) { - } else { + if (!validate_response?.result) { setLocalErrorText(validate_response?.error_message); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/TitleGeneration.jsx` around lines 158 - 162, The current if(validate_response?.result) { } else { setLocalErrorText(validate_response?.error_message); return; } contains an empty success branch—invert the condition to handle the failure case first: check if (!validate_response?.result) then call setLocalErrorText(validate_response?.error_message) and return; remove the empty if branch so normal success flow continues after the check; reference validate_response and setLocalErrorText to locate the change.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/WeeksSelection.jsx (2)
44-54: Inconsistent session access: reactive selector on line 47 vs. imperativegetSession()on line 224.
sessionis read reactively via a selector (line 47) and used inonWebSocketOpen(line 128). ButhandleContinueClick(line 224) callsgetSession()instead of using the reactivesession. This is functionally fine (both return the same value) but inconsistent—consider using one pattern for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/WeeksSelection.jsx` around lines 44 - 54, The code mixes reactive selector usage (const session = useAICreationSessionStore(state => state.session) used in onWebSocketOpen) with an imperative call getSession() inside handleContinueClick; make access consistent by replacing the getSession() call in handleContinueClick with the reactive session variable (session) and remove the imperative call usage, ensuring handleContinueClick uses the same session reference as onWebSocketOpen and no longer depends on useAICreationSessionStore.getState().getSession().
218-234:handleContinueClickdoes not guard against concurrent/double invocations.If the WebSocket sends multiple
should_move_forwardmessages (e.g., due to a retry or duplicate),handleContinueClickwill callsaveUserChatsInDBandsetCurrentPageValue(4)multiple times. Consider adding a guard (e.g., a ref flag) to prevent duplicate invocations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/WeeksSelection.jsx` around lines 218 - 234, handleContinueClick can run multiple times concurrently; add a guard using a ref (e.g., isSubmittingRef) to return early if already running, set isSubmittingRef.current = true before the async work and reset it in a finally block to avoid duplicate saveUserChatsInDB calls and repeated setCurrentPageValue(4). Use the existing function names (handleContinueClick, saveUserChatsInDB, setSelectedWeekStore, setCurrentPageValue) and wrap the awaits in try/finally so the ref is cleared on error; also consider disabling the Continue button while isSubmittingRef.current is true.
🤖 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/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx`:
- Line 286: The direct call const chunks = JSON.parse(getChunks()) can throw if
getChunks() returns null/undefined or an already-parsed object; update
SelectObjective.jsx to defensively obtain and parse the value from getChunks:
call getChunks(), check its type (if it's an object/array use it as-is, if it's
a string attempt JSON.parse inside a try-catch), default to an empty array on
failure and log or handle the parse error; refer to the getChunks function and
the chunks variable to implement the guarded parse and error handling.
In `@tests/e2e/flows/mip-flow/happy-path.spec.ts`:
- Around line 27-37: The beforeEach currently calls
mipFlowPage.navigateToMitraChat() before clearing storage which can let the app
initialize with stale state; change the setup so page.context().clearCookies()
and the page.evaluate that calls localStorage.clear()/sessionStorage.clear() run
before constructing/navigating via new MIPFlowPage(page) and before calling
navigateToMitraChat(), ensuring storage is cleared prior to any app load or
initialization.
In `@tests/pages/mip-flow.page.ts`:
- Around line 304-307: getGeneratedTitle currently calls
titleTextarea.inputValue(), which will throw if the matched locator is a
contenteditable element; update getGeneratedTitle to check whether the resolved
element is contenteditable (e.g., use titleTextarea.evaluate(node =>
node.isContentEditable) or querySelector to inspect contentEditable) and, if
true, return its textContent/innerText via evaluate, otherwise call
inputValue(); reference the locator/titleTextarea and the getGeneratedTitle
method when making the change so both branches safely return a string.
- Around line 328-330: The method isCreateMIPButtonEnabled currently checks
visibility instead of the disabled attribute; update it to mirror
isObjectiveNextEnabled and isActionItemNextEnabled by reading the
createMIPButton element's "disabled" attribute and returning its negation (i.e.,
return true when disabled is not set/false). Locate the isCreateMIPButtonEnabled
function and replace the call to isElementVisible(this.createMIPButton) with a
check that gets this.createMIPButton.getAttribute('disabled') (or uses
element.isEnabled()/property retrieval helper if available) to determine enabled
state, ensuring consistency with the other *NextEnabled helpers and referencing
the createMIPButton selector.
---
Outside diff comments:
In `@src/pages/ai-creation/pages/improvement-plan/index.jsx`:
- Around line 10-17: The local media state is kept in sync by a useEffect that
only depends on projectId causing stale media when items (from
useAICreationSessionStore) updates; either add items to the dependency array of
the useEffect so it runs when items changes, or remove the redundant local state
entirely and use items directly in the component; update references to
media/setMedia accordingly and ensure the useEffect, useState, items, setMedia,
useAICreationSessionStore and projectId symbols are adjusted to reflect the
chosen approach.
In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/ActionItems.jsx`:
- Around line 340-351: The message string in the botMessage object (symbols:
botMessage, hasClickedOnAddmore, getActionListStore, t, BOT) suffers from
ternary/concatenation precedence so the conditional always picks the first
branch; fix by parenthesizing the ternary expressions and rearranging the
concatenation so the intended branches are evaluated (e.g., wrap each
conditional in (...) before concatenating newline and translation strings, and
ensure the nested conditions use explicit parentheses so the correct t(...)
values or JSON.stringify(getActionListStore()) are appended).
In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx`:
- Around line 152-209: In onWebSocketMessage, avoid sequential overwrites by
building a single updatedLastMessage from lastMessage and applying msg,
extra_content.sources and extra_content.file_url together, then call
setCommonFlowChatHistory once; specifically, in the branch where lastIndex >= 0
&& lastMessage?.source === 'bot', replace the three separate update blocks that
each create updatedLastMessage (the block that appends message.msg, the block
that sets sources, and the block that sets file_url) with a single flow that:
create let updatedLastMessage = { ...lastMessage }; if (message?.msg)
updatedLastMessage.msg = lastMessage.msg + message.msg; if
(Array.isArray(message?.extra_content?.sources) &&
message?.extra_content?.sources.length) updatedLastMessage.sources =
message.extra_content.sources; if (message?.extra_content?.file_url)
updatedLastMessage.file_url = message.extra_content.file_url; then call
setCommonFlowChatHistory([...prevChatHistory.slice(0, lastIndex),
updatedLastMessage]).
In
`@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx`:
- Line 150: The code sets chunks from the wrong variable: replace the use of
objective_list?.chunks with the API response object's chunks so setChunksStore
gets the actual data; update the call to setChunksStore to use
fetched_objectiveList?.chunks (since objective_list is an array and has no
.chunks), ensuring getChunks()/JSON.parse later receives the proper value from
fetched_objectiveList.
In
`@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/TitleGeneration.jsx`:
- Around line 182-208: The code calls updateChatSession (which sets
session_status: "COMPLETED") before calling createProject, so if createProject
fails the session remains completed; change the flow to call createProject first
and only call updateChatSession to mark COMPLETED after createProject returns
successfully (or, alternatively, add rollback logic in the catch that calls
updateChatSession with the previous status to revert), and ensure the catch
block handles createProject failures by retrying or resetting session state
using the same session identifier.
In
`@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/WeeksSelection.jsx`:
- Around line 137-164: onWebSocketMessage is memoized with
useCallback([handleScrollIntoView]) but calls handleContinueClick which is
recreated every render, causing a stale closure and losing the fetched
durationIntroMessage used by saveUserChatsInDB; make handleContinueClick stable
by wrapping it in useCallback and include it in the dependency array of
onWebSocketMessage (also ensure the new useCallback for handleContinueClick
includes durationIntroMessage and any other state it reads), so
onWebSocketMessage always invokes the latest handleContinueClick and preserves
the API-fetched intro message.
---
Nitpick comments:
In
`@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx`:
- Around line 464-465: The code is calling getPrevObjective() and
getPrevObjectiveSource() imperatively during render which can return stale data
because they read store state directly; change the component to read
previous-objective data via reactive selectors instead: replace direct calls to
getPrevObjective() and getPrevObjectiveSource() inside the render branch that
uses prevObjectiveShown with selector-based reads (e.g.,
usePrevObjectiveSelector / useSelector(getPrevObjective) or equivalent hook
provided by your store) and fall back to objectiveList / objectiveSource when
prevObjectiveShown is false so the component updates when store data changes.
- Around line 87-111: The useState lazy initializer for visibleCount performs
side effects by calling setSelectedIndices and setSelectedObjectives, which is
unreliable; instead compute the initial visibleCount, selectedIndices, and
selectedObjectives together (using getObjective and getSelectedObjective) and
initialize state without calling setters, or move the logic into a useEffect
that runs on mount: extract the current objectiveList and
storedSelectedObjectives, normalize storedSelectedObjectives into
selectedObjectivesArray, compute indices and visibleCount (using Math.max as
currently done), then call setSelectedIndices(indices),
setSelectedObjectives(indices.map(...)) and setVisibleCount(computedValue)
inside that effect (or initialize all three states from the precomputed values
before any setters are used). Ensure you update the code references: the
visibleCount useState initializer, setSelectedIndices, setSelectedObjectives,
getObjective, and getSelectedObjective.
- Around line 44-62: The component mixes reactive selectors
(objectiveChatHistory, selectedObjective, profileId, preferredLanguage,
isOwnObjective via useAICreationSessionStore(state => state.X)) with imperative
getters/setters from useAICreationSessionStore.getState() (getObjective,
getSelectedObjective, getIsPrevObjectiveShown, getObjectiveSource,
getPrevObjective, getPrevObjectiveSource, getChunks, getSystemError,
getUserProblemStatement, getSession and their corresponding setters), which is
confusing; either convert the frequently-read getters (e.g., getObjective,
getSession, getSelectedObjective) to reactive selectors so the component
re-renders when they change, or keep them imperative but add a clear comment
above the destructure explaining why getState() is used and which values
deliberately do not trigger re-renders, and update usages of
setSelectedObjective/setSelectedObjectiveSource/setChunks/etc. to match the
chosen pattern for consistency.
In
`@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/TitleGeneration.jsx`:
- Around line 158-162: The current if(validate_response?.result) { } else {
setLocalErrorText(validate_response?.error_message); return; } contains an empty
success branch—invert the condition to handle the failure case first: check if
(!validate_response?.result) then call
setLocalErrorText(validate_response?.error_message) and return; remove the empty
if branch so normal success flow continues after the check; reference
validate_response and setLocalErrorText to locate the change.
In
`@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/WeeksSelection.jsx`:
- Around line 44-54: The code mixes reactive selector usage (const session =
useAICreationSessionStore(state => state.session) used in onWebSocketOpen) with
an imperative call getSession() inside handleContinueClick; make access
consistent by replacing the getSession() call in handleContinueClick with the
reactive session variable (session) and remove the imperative call usage,
ensuring handleContinueClick uses the same session reference as onWebSocketOpen
and no longer depends on useAICreationSessionStore.getState().getSession().
- Around line 218-234: handleContinueClick can run multiple times concurrently;
add a guard using a ref (e.g., isSubmittingRef) to return early if already
running, set isSubmittingRef.current = true before the async work and reset it
in a finally block to avoid duplicate saveUserChatsInDB calls and repeated
setCurrentPageValue(4). Use the existing function names (handleContinueClick,
saveUserChatsInDB, setSelectedWeekStore, setCurrentPageValue) and wrap the
awaits in try/finally so the ref is cleared on error; also consider disabling
the Continue button while isSubmittingRef.current is true.
In `@tests/e2e/flows/mip-flow/happy-path.spec.ts`:
- Around line 64-107: The test uses many fixed wait(...) delays making it slow
and flaky; replace hardcoded sleeps with condition-based waits: after sending
initial or define-challenge messages use
mipFlowPage.waitForBotResponse(expectedMessageCount) or
mipFlowPage.waitForDefineChallengeLoad()/waitForWeeksSelectionLoad()/waitForObjectivesLoad()/waitForFinalActionPage()
as appropriate instead of wait(3000)/wait(5000)/wait(15000); when looping
through MIP_DEFINE_CHALLENGE_MESSAGES call sendDefineChallengeMessage then await
mipFlowPage.waitForBotResponse(i + 1) (or another element-based checker) to wait
for the specific reply, and remove the extra waits after selectObjectiveByIndex,
clickObjectiveNext, clickActionItemNext and sendWeeksSelection replacing them
with waits for the next page/state via the existing waitFor* helpers or a new
waitForElement/waitForURL helper.
In `@tests/pages/mip-flow.page.ts`:
- Around line 81-83: Replace the hardcoded route string in navigateToMitraChat
with the canonical constant: import SITE_ROUTES (or the specific MITRA_CHAT
export) from tests/constant/site_routes.ts and change await
this.navigate("/mohini/mitra-chat") inside navigateToMitraChat() to await
this.navigate(SITE_ROUTES.MITRA_CHAT); ensure the import is added at the top of
the file and remove the literal string to keep a single source of truth.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
Summary by CodeRabbit
Release Notes
Refactor
Tests