Feature/apcfss fix - #259
Conversation
…; add AUTH_METHOD and AUTH_ROUTE to env configuration
…n7009/mohini-app-frontend into feature/apcfss-fix
|
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 AI-creation pages to use selector hooks and destructured store getters instead of imperative Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
@coderabbitai 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pages/ai-creation/pages/improvement-plan/index.jsx (1)
12-17:⚠️ Potential issue | 🟠 MajorSync the rendered media when the store value changes.
itemsalready subscribes this component tostate.media, but the mirroredmediastate is only refreshed whenprojectIdchanges. Any latersetMedia(...)leavesFileViewerstale or empty until the route changes.💡 Suggested fix
- useEffect(() => { - const mediaItems = items || []; - setMedia(mediaItems); - }, [projectId]); + useEffect(() => { + setMedia(items || []); + }, [items]);🤖 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 12 - 17, The effect currently sets local media state only when projectId changes, causing FileViewer to go stale; update the useEffect watching items (the value returned from useAICreationSessionStore) so setMedia(mediaItems) runs whenever items changes (or remove the mirrored state and use items directly). Locate the useEffect that references items, setMedia, and projectId and add items to the dependency array (or refactor to avoid duplicating state) so the rendered media stays in sync with the store.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx (1)
75-88:⚠️ Potential issue | 🟠 MajorInvalidate the cached intro when
flowTypechanges.
setCommonFlowIntroMessage(message)now stores a single shared intro value, and this branch only checks whether that value is truthy. If a different common flow is opened before the store is cleared,fetchIntroMessage()andgenerateNewSession()are skipped and the new flow can start with the previous flow's intro/session.🤖 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 75 - 88, The code caches a single shared intro in commonFlowIntroMessage so when flowType changes the branch that skips fetchIntroMessage()/generateNewSession() can reuse the old intro/session; to fix, invalidate or reset that cached value when flowType changes (e.g., call setCommonFlowIntroMessage(null) or store intro keyed by flowType) before the if (commonFlowIntroMessage) check so fetchIntroMessage() and generateNewSession() run for the new flow; update references to setCommonFlowIntroMessage, commonFlowIntroMessage, fetchIntroMessage and generateNewSession accordingly.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx (1)
508-513:⚠️ Potential issue | 🟠 MajorClear the local selection when switching to the previous objective list.
This handler swaps the rendered list but only resets
selectedObjectivein the store.selectedIndicesandselectedObjectivesstill point at the old list, so the Next button can stay enabled andhandleNextClick()can submit a stale selection from the wrong objective set.💡 Suggested fix
handleAdditionalCTAClick={() => { setObjectiveList(getPrevObjective()) setObjectiveSource(getPrevObjectiveSource()) + setSelectedIndices([]) + setSelectedObjectives([]) setPrevObjectiveShown(true) setIsPrevObjectiveShownStore(true) setObjectiveListLoading(true) setSelectedObjectiveStore(null)🤖 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 508 - 513, When switching to the previous objective list you only clear the store selection via setSelectedObjectiveStore(null) but leave local selection state (selectedIndices and selectedObjectives) pointing at the old list; reset those local selections as well so the UI and Next button reflect no selection. In the same handler that calls setObjectiveList(...), setObjectiveSource(...), setPrevObjectiveShown(true), setIsPrevObjectiveShownStore(true), setObjectiveListLoading(true) and setSelectedObjectiveStore(null), also clear local selection by calling the setters for selectedIndices and selectedObjectives (e.g., setSelectedIndices([]) and setSelectedObjectives([])) so handleNextClick() cannot submit stale selections from the previous list.
🧹 Nitpick comments (3)
src/pages/ShikshalokamVoiceChat/dynamic-voice-chat.js (1)
1796-1797: RedundantclearFromStorage()call.
clearFromStorage()is already called at line 1789 (unconditionally in the non-accessTokenpath). Calling it again here at line 1796 is unnecessary.🔧 Proposed fix to remove redundant call
if (rerouteUrl && rerouteUrl !== null && rerouteUrl !== undefined && rerouteUrl !== "") { window.location.href = rerouteUrl } else { - clearFromStorage() navigate(-1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ShikshalokamVoiceChat/dynamic-voice-chat.js` around lines 1796 - 1797, Remove the redundant clearFromStorage() call that directly precedes navigate(-1); clearFromStorage() is already invoked unconditionally earlier in the non-accessToken branch, so keep the earlier call and delete the second invocation (leave navigate(-1) intact) to avoid duplicate clearing; look for the clearFromStorage() call paired with navigate(-1) in dynamic-voice-chat.js and remove only that extra invocation.tests/e2e/flows/mip-flow/happy-path.spec.ts (1)
27-37: Clear persisted storage before the first app boot.Line 32 loads the app before
localStorage/sessionStorageare cleared, and Line 46 loads it again. If startup logic reads persisted flags, the first visit can still influence the test before the clean-state navigation happens.Also applies to: 45-48
🤖 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 27 - 37, Move the storage-clearing steps to run before the app is first loaded: call page.context().clearCookies() and run the page.evaluate that clears localStorage and sessionStorage before instantiating/navigating the app (i.e., before new MIPFlowPage(page) and before calling mipFlowPage.navigateToMitraChat()). Ensure the same change is applied to the other setup around lines 45-48 so the first app boot sees a clean storage state.tests/pages/mip-flow.page.ts (1)
81-83: UseSITE_ROUTEShere instead of duplicating path literals.This page object hardcodes both the entry path and the final-page check even though
tests/constant/site_routes.tsnow owns those routes. Pulling from the shared constant keeps navigation and assertions from drifting apart.♻️ Suggested refactor
import { Page, Locator } from "@playwright/test" import { BasePage } from "./base.page" +import { SITE_ROUTES } from "../constant/site_routes" @@ async navigateToMitraChat(): Promise<void> { - await this.navigate("/mohini/mitra-chat") + await this.navigate(SITE_ROUTES.MITRA_CHAT) } @@ async waitForImprovementPlanPage(timeout: number = 120000): Promise<void> { - await this.page.waitForURL("**/improvement-plan**", { timeout }) + await this.page.waitForURL(`**${SITE_ROUTES.IMPROVEMENT_PLAN}**`, { timeout }) } @@ isOnImprovementPlanPage(): boolean { - return this.getCurrentUrl().includes("/improvement-plan") + return this.getCurrentUrl().includes(SITE_ROUTES.IMPROVEMENT_PLAN) }Also applies to: 338-346
🤖 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, The navigateToMitraChat method hardcodes the route string; replace the literal "/mohini/mitra-chat" with the shared route constant from SITE_ROUTES (import from tests/constant/site_routes.ts) so navigation uses SITE_ROUTES.MITRA_CHAT (or the appropriate exported name). Update any other hardcoded occurrences in this file (notably the similar block around lines 338-346) to reference the same SITE_ROUTES constants to keep navigation and assertions consistent.
🤖 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/CommonFlow.jsx`:
- Around line 158-185: The code reads prevChatHistory/lastMessage once but calls
setCommonFlowChatHistory up to three times, causing later branches to overwrite
earlier updates; instead, inside the if (lastIndex >= 0 && lastMessage?.source
=== 'bot') block create a single updatedLastMessage = { ...lastMessage }, then
if (message?.msg) append to updatedLastMessage.msg, if
(Array.isArray(message?.extra_content?.sources)) set updatedLastMessage.sources,
and if (message?.extra_content?.file_url) set updatedLastMessage.file_url, then
call setCommonFlowChatHistory([...prevChatHistory.slice(0, lastIndex),
updatedLastMessage]) exactly once (use
getCommonFlowChatHistory()/prevChatHistory and lastIndex as currently used).
In
`@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx`:
- Around line 87-111: The lazy initializer for visibleCount currently calls
setSelectedIndices and setSelectedObjectives (render-phase side effects);
instead compute initial values first by reading getObjective() and
getSelectedObjective() to derive initialSelectedIndices and
initialSelectedObjectives (handling legacy single-string selection), then pass
those computed values into separate useState calls for selectedIndices and
selectedObjectives, and initialize visibleCount with a pure computed number
(using defaultValueToShow and max selected index) without invoking any setters;
update references to visibleCount, setVisibleCount, setSelectedIndices,
setSelectedObjectives, getObjective, and getSelectedObjective accordingly.
In `@src/utils/env.ts`:
- Line 48: AUTH_ROUTE currently defaults to the literal string "url" due to a
copy/paste error; update the getEnv call for AUTH_ROUTE to use a sensible
default API path (e.g., an empty string "" or a specific path like
"/auth/profile") instead of "url" so that calls using env.AUTH_ROUTE() (via
getEnv and REACT_APP_AUTH_ROUTE) produce a valid endpoint when passed to
apiClient.get(); locate the AUTH_ROUTE definition and replace the default "url"
with the chosen proper default.
In `@tests/pages/mip-flow.page.ts`:
- Around line 170-172: The waitForDefineChallengeLoad helper currently waits for
the shared chat textarea (this.chatBoxTextarea) which is already visible earlier
and doesn't prove the flow advanced; update waitForDefineChallengeLoad (and the
similar helper used around the other occurrence) to wait for a stage-specific
selector or state instead—for example wait for a specific prompt/container
unique to the "define challenge" step (a CSS selector or test-id for that
prompt) or wait for the bot message count to increase (inspect bot message
elements and assert count changed) rather than waiting on this.chatBoxTextarea
so the helper only returns once the UI has actually progressed to the next step.
- Around line 328-330: The method isCreateMIPButtonEnabled currently checks
visibility via isElementVisible (returning true for visible-but-disabled
buttons); change it to check the element's enabled state instead by calling
Playwright's isEnabled on the createMIPButton Locator (or use your existing
helper isElementEnabled if present) so the method accurately returns whether the
button is interactable (ensure you update the implementation of
isCreateMIPButtonEnabled to call createMIPButton.isEnabled() rather than
isElementVisible(createMIPButton)).
- Around line 74-75: getGeneratedTitle is currently using
titleTextarea.inputValue() but titleTextarea can match a <textarea> or a
[contenteditable='true'] element; change getGeneratedTitle to detect which node
was matched (using the locator titleTextarea) and read its text accordingly: if
it's a form control (textarea/input) use inputValue(), otherwise read text via
textContent/innerText or locator.evaluate(node => node.innerText) for
contenteditable elements so the function returns the actual rendered text in
both cases.
---
Outside diff comments:
In `@src/pages/ai-creation/pages/improvement-plan/index.jsx`:
- Around line 12-17: The effect currently sets local media state only when
projectId changes, causing FileViewer to go stale; update the useEffect watching
items (the value returned from useAICreationSessionStore) so
setMedia(mediaItems) runs whenever items changes (or remove the mirrored state
and use items directly). Locate the useEffect that references items, setMedia,
and projectId and add items to the dependency array (or refactor to avoid
duplicating state) so the rendered media stays in sync with the store.
In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx`:
- Around line 75-88: The code caches a single shared intro in
commonFlowIntroMessage so when flowType changes the branch that skips
fetchIntroMessage()/generateNewSession() can reuse the old intro/session; to
fix, invalidate or reset that cached value when flowType changes (e.g., call
setCommonFlowIntroMessage(null) or store intro keyed by flowType) before the if
(commonFlowIntroMessage) check so fetchIntroMessage() and generateNewSession()
run for the new flow; update references to setCommonFlowIntroMessage,
commonFlowIntroMessage, fetchIntroMessage and generateNewSession accordingly.
In
`@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx`:
- Around line 508-513: When switching to the previous objective list you only
clear the store selection via setSelectedObjectiveStore(null) but leave local
selection state (selectedIndices and selectedObjectives) pointing at the old
list; reset those local selections as well so the UI and Next button reflect no
selection. In the same handler that calls setObjectiveList(...),
setObjectiveSource(...), setPrevObjectiveShown(true),
setIsPrevObjectiveShownStore(true), setObjectiveListLoading(true) and
setSelectedObjectiveStore(null), also clear local selection by calling the
setters for selectedIndices and selectedObjectives (e.g., setSelectedIndices([])
and setSelectedObjectives([])) so handleNextClick() cannot submit stale
selections from the previous list.
---
Nitpick comments:
In `@src/pages/ShikshalokamVoiceChat/dynamic-voice-chat.js`:
- Around line 1796-1797: Remove the redundant clearFromStorage() call that
directly precedes navigate(-1); clearFromStorage() is already invoked
unconditionally earlier in the non-accessToken branch, so keep the earlier call
and delete the second invocation (leave navigate(-1) intact) to avoid duplicate
clearing; look for the clearFromStorage() call paired with navigate(-1) in
dynamic-voice-chat.js and remove only that extra invocation.
In `@tests/e2e/flows/mip-flow/happy-path.spec.ts`:
- Around line 27-37: Move the storage-clearing steps to run before the app is
first loaded: call page.context().clearCookies() and run the page.evaluate that
clears localStorage and sessionStorage before instantiating/navigating the app
(i.e., before new MIPFlowPage(page) and before calling
mipFlowPage.navigateToMitraChat()). Ensure the same change is applied to the
other setup around lines 45-48 so the first app boot sees a clean storage state.
In `@tests/pages/mip-flow.page.ts`:
- Around line 81-83: The navigateToMitraChat method hardcodes the route string;
replace the literal "/mohini/mitra-chat" with the shared route constant from
SITE_ROUTES (import from tests/constant/site_routes.ts) so navigation uses
SITE_ROUTES.MITRA_CHAT (or the appropriate exported name). Update any other
hardcoded occurrences in this file (notably the similar block around lines
338-346) to reference the same SITE_ROUTES constants to keep navigation and
assertions consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 638d76a1-9de3-4049-8c4d-6dbefdc3363f
📒 Files selected for processing (13)
src/pages/ShikshalokamVoiceChat/dynamic-voice-chat.jssrc/pages/ai-creation/pages/improvement-plan/index.jsxsrc/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/ActionItems.jsxsrc/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsxsrc/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsxsrc/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/TitleGeneration.jsxsrc/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/WeeksSelection.jsxsrc/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/components/objectives/FinalObjectiveSection.jsxsrc/utils/env.tstests/constant/sg-commons-chat.tstests/constant/site_routes.tstests/e2e/flows/mip-flow/happy-path.spec.tstests/pages/mip-flow.page.ts
| this.titleTextarea = page.locator("div.secondpage-textbox-container textarea, div.secondpage-textbox-container [contenteditable='true']") | ||
| this.createMIPButton = page.locator("button.fifthpage-select-bttn") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and view the relevant sections
wc -l tests/pages/mip-flow.page.tsRepository: ELEVATE-Project/mohini-app-frontend
Length of output: 111
🏁 Script executed:
# View lines 74-75 (selector definition)
sed -n '70,80p' tests/pages/mip-flow.page.tsRepository: ELEVATE-Project/mohini-app-frontend
Length of output: 470
🏁 Script executed:
# View lines 305-310 (usage context around inputValue)
sed -n '300,310p' tests/pages/mip-flow.page.tsRepository: ELEVATE-Project/mohini-app-frontend
Length of output: 272
🏁 Script executed:
# Search for the getGeneratedTitle method
grep -n "getGeneratedTitle" tests/pages/mip-flow.page.tsRepository: ELEVATE-Project/mohini-app-frontend
Length of output: 129
🏁 Script executed:
# Search for all usages of titleTextarea to see if there are other methods interacting with it
grep -n "titleTextarea" tests/pages/mip-flow.page.tsRepository: ELEVATE-Project/mohini-app-frontend
Length of output: 439
🏁 Script executed:
# Let's also verify Playwright's inputValue() behavior by checking the repository for any documentation or other clues
rg "inputValue" tests/ -A 2 -B 2 --max-count=10Repository: ELEVATE-Project/mohini-app-frontend
Length of output: 611
🌐 Web query:
Playwright inputValue() contenteditable support compatibility
💡 Result:
Playwright's inputValue method (available on both ElementHandle and Locator) does not support contenteditable elements. It is specifically designed for form input elements like , <textarea>, and , returning input.value for those. Using it on non-input elements like [contenteditable] typically returns an empty string or throws an error. For contenteditable elements, use alternatives such as: - locator.textContent or locator.innerText to get the text content. - locator.inputValue is unreliable and not intended for contenteditable (confirmed by multiple sources including Stack Overflow discussions and third-party docs noting it fails on divs/contenteditable). Official Playwright docs show locator.fill supports contenteditable (via input event dispatch), but inputValue documentation and examples are limited to input/textarea/select. GitHub issues discuss fill/recognition problems with contenteditable but confirm inputValue expects form controls. Compatibility: No native support in current versions (up to 2026); use textContent for reading values. Citations: 1: https://runebook.dev/en/docs/playwright/api/class-elementhandle/element-handle-input-value 2: https://stackoverflow.com/questions/62002041/getting-value-of-input-element-in-playwright 3: https://runebook.dev/en/articles/playwright/api/class-locator/locator-input-value 4: https://playwright.dev/docs/api/class-elementhandle 5: http://playwright.dev/docs/api/class-locator 🏁 Script executed: # Check the fillInput helper method to see how it handles different element types grep -n "fillInput" tests/pages/mip-flow.page.ts -A 5 -B 2 Repository: ELEVATE-Project/mohini-app-frontend Length of output: 610 🏁 Script executed: # Also check if there's a shared base class or utility that handles both element types grep -n "fillInput\|fill" tests/pages/mip-flow.page.ts | head -20 Repository: ELEVATE-Project/mohini-app-frontend Length of output: 194 🏁 Script executed: # Find the fillInput method definition - it's likely in a base class or utility grep -rn "fillInput" tests/ --include="*.ts" -B 2 -A 8 | grep -A 8 "fillInput\s*(" Repository: ELEVATE-Project/mohini-app-frontend Length of output: 3132 🏁 Script executed: # Check if there's a base page class or helpers file find tests -name "*.ts" -type f | head -20 Repository: ELEVATE-Project/mohini-app-frontend Length of output: 765 getGeneratedTitle() is incompatible with the [contenteditable] fallback. titleTextarea can match either <textarea> or [contenteditable='true'], but inputValue() only works for form controls. If the contenteditable variant is rendered, this will fail or return an empty string instead of the actual text value. 🛠️ Suggested fix async getGeneratedTitle(): Promise<string> { - return await this.titleTextarea.inputValue() + const editor = this.titleTextarea.first() + const tagName = await editor.evaluate((node) => node.tagName.toLowerCase()) + + return tagName === "textarea" + ? await editor.inputValue() + : ((await editor.textContent()) ?? "").trim() } 🤖 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 74 - 75, getGeneratedTitle is currently using titleTextarea.inputValue() but titleTextarea can match a <textarea> or a [contenteditable='true'] element; change getGeneratedTitle to detect which node was matched (using the locator titleTextarea) and read its text accordingly: if it's a form control (textarea/input) use inputValue(), otherwise read text via textContent/innerText or locator.evaluate(node => node.innerText) for contenteditable elements so the function returns the actual rendered text in both cases.
There was a problem hiding this comment.
@pinkman7009, understood! I'll drop this suggestion. Feel free to let me know if you need anything else.
| async waitForDefineChallengeLoad(): Promise<void> { | ||
| await this.waitForElement(this.chatBoxTextarea, 60000) | ||
| } |
There was a problem hiding this comment.
These wait helpers do not prove the flow advanced.
Both methods wait on the shared chat textarea, which is already visible in earlier stages. They can return immediately while the app is still on the previous step, which is why the spec has to rely on fixed sleeps. Wait on a stage-specific prompt/container or a bot-message count change instead.
Also applies to: 280-282
🤖 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 170 - 172, The
waitForDefineChallengeLoad helper currently waits for the shared chat textarea
(this.chatBoxTextarea) which is already visible earlier and doesn't prove the
flow advanced; update waitForDefineChallengeLoad (and the similar helper used
around the other occurrence) to wait for a stage-specific selector or state
instead—for example wait for a specific prompt/container unique to the "define
challenge" step (a CSS selector or test-id for that prompt) or wait for the bot
message count to increase (inspect bot message elements and assert count
changed) rather than waiting on this.chatBoxTextarea so the helper only returns
once the UI has actually progressed to the next step.
There was a problem hiding this comment.
@pinkman7009, understood! I'll defer to your judgment on this one.
(´• ω •`) ʕ•ᴥ•ʔ
🧠 Learnings used
Learnt from: pinkman7009
Repo: ELEVATE-Project/mohini-app-frontend PR: 225
File: src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/InitialSwitch.jsx:79-89
Timestamp: 2026-01-14T02:51:15.737Z
Learning: In the AI creation flow components (InitialSwitch.jsx, CommonFlow.jsx), the pattern of using `pendingMessageRef` with a 100ms `setTimeout` after WebSocket authentication is intentionally used to ensure the authenticate message is sent before any user messages. This is an accepted implementation pattern for these flows.
…t for profile reading
resolve PR comments
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx (1)
83-88:⚠️ Potential issue | 🟡 MinorAdd
botRouteto effect dependency array to ensure intro message refreshes when flow type changes.When
flowTypechanges,botRoutealso changes (e.g., LFA → LCF maps to differentbot_routes), but ifcommonFlowIntroMessageis already cached, the effect skips fetching the new intro message for the current flow type. AddbotRouteto the dependency array:}, [flowType, botRoute]);Alternatively, clear
commonFlowIntroMessagewhenflowTypechanges to force a fresh fetch for each flow.🤖 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 83 - 88, The effect that checks commonFlowIntroMessage and calls fetchIntroMessage currently only depends on flowType, which misses updates when botRoute changes; update the dependency array for the useEffect that references commonFlowIntroMessage, setIsLoadingIntro, and fetchIntroMessage to include botRoute (i.e., }, [flowType, botRoute]); alternatively, if you prefer forcing a reload on flow change, clear commonFlowIntroMessage when flowType changes so fetchIntroMessage runs for the new botRoute.
🧹 Nitpick comments (3)
tests/pages/mip-flow.page.ts (2)
137-145: Duplicated selector string.The selector
"li.div34"is hardcoded here and also in thechatMessageContainerlocator (line 59). If the selector needs to change, both locations must be updated. Consider extracting a constant or deriving the selector string from the locator.♻️ Suggested refactor
+ private readonly chatMessageSelector = "li.div34" + // Chat messages (both user and bot messages in the chat window) - this.chatMessageContainer = page.locator("li.div34") + this.chatMessageContainer = page.locator(this.chatMessageSelector)Then reference
this.chatMessageSelectorinwaitForBotResponse:async waitForBotResponse(expectedCount: number, timeout: number = 60000): Promise<void> { await this.page.waitForFunction( ({ selector, count }) => { return document.querySelectorAll(selector).length >= count }, - { selector: "li.div34", count: expectedCount }, + { selector: this.chatMessageSelector, count: expectedCount }, { timeout } ) }Note: If using an instance field inside
waitForFunction, you'll need to pass it as a parameter since the function runs in browser context.🤖 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 137 - 145, The selector "li.div34" is duplicated; update waitForBotResponse to use the existing chat message selector instead of hardcoding by referencing the class/field used for the locator (e.g., chatMessageContainer or chatMessageSelector). Move the selector string into a single constant/instance field (e.g., this.chatMessageSelector) and pass that selector as an argument into page.waitForFunction so the browser-context function uses the provided selector parameter rather than a hardcoded literal; ensure waitForBotResponse signature uses the same selector source to keep a single point of truth.
63-71: Unused locator declarations.These class members are defined but never used:
objectiveNextButton(line 63) — methods create new locators insteadactionItemNextButton(line 68) —clickActionItemNextandisActionItemNextEnableduse inline locators with different selectorsweeksSelectionBotMessage(line 71) — not referenced anywhereEither remove these unused declarations or refactor the methods to use them consistently.
🤖 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 63 - 71, Remove or consolidate the unused locator fields: objectiveNextButton, actionItemNextButton, and weeksSelectionBotMessage from the class, or update the methods to use them consistently; specifically either delete the declarations for objectiveNextButton and actionItemNextButton and let clickActionItemNext and isActionItemNextEnabled continue using their inline locators, and remove weeksSelectionBotMessage if it's unused, OR change clickActionItemNext and isActionItemNextEnabled to reference the class fields objectiveNextButton and actionItemNextButton (ensure the selectors match the inline locators currently used) and replace any other inline instantiations so all locator usage is consistent.src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx (1)
256-256: Consider usinggetCommonFlowChatHistory()for consistency.The
onWebSocketMessagecallback usesgetCommonFlowChatHistory()to read fresh state, but herecommonFlowChatHistoryfrom the selector is used. While this works correctly for synchronous event handlers, using the getter would maintain a consistent pattern throughout the component.♻️ Suggested change for consistency
- setCommonFlowChatHistory([...commonFlowChatHistory, newMessage]); + setCommonFlowChatHistory([...getCommonFlowChatHistory(), newMessage]);🤖 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` at line 256, Replace the direct usage of the selector state variable when appending a new message and instead call the getter to obtain the freshest history; specifically, in the code that currently calls setCommonFlowChatHistory([...commonFlowChatHistory, newMessage]) use getCommonFlowChatHistory() to read the current array (e.g. setCommonFlowChatHistory([...getCommonFlowChatHistory(), newMessage])) so it matches the pattern used in onWebSocketMessage and avoids stale-state issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/pages/mip-flow.page.ts`:
- Line 59: The selector for chat messages is incorrect; update the locator
assignment and any related selector objects to match the actual DOM: change the
locator in tests/pages/mip-flow.page.ts where chatMessageContainer is set from
"li.div34" to "div.div35", and likewise update the selector used in the selector
object (previously { selector: "li.div34", ... }) to { selector: "div.div35",
... } so methods like waitForBotResponse and getChatMessageCount correctly
target the individual message elements (refer to chatMessageContainer,
waitForBotResponse, and getChatMessageCount to locate usages).
---
Outside diff comments:
In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx`:
- Around line 83-88: The effect that checks commonFlowIntroMessage and calls
fetchIntroMessage currently only depends on flowType, which misses updates when
botRoute changes; update the dependency array for the useEffect that references
commonFlowIntroMessage, setIsLoadingIntro, and fetchIntroMessage to include
botRoute (i.e., }, [flowType, botRoute]); alternatively, if you prefer forcing a
reload on flow change, clear commonFlowIntroMessage when flowType changes so
fetchIntroMessage runs for the new botRoute.
---
Nitpick comments:
In `@src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsx`:
- Line 256: Replace the direct usage of the selector state variable when
appending a new message and instead call the getter to obtain the freshest
history; specifically, in the code that currently calls
setCommonFlowChatHistory([...commonFlowChatHistory, newMessage]) use
getCommonFlowChatHistory() to read the current array (e.g.
setCommonFlowChatHistory([...getCommonFlowChatHistory(), newMessage])) so it
matches the pattern used in onWebSocketMessage and avoids stale-state issues.
In `@tests/pages/mip-flow.page.ts`:
- Around line 137-145: The selector "li.div34" is duplicated; update
waitForBotResponse to use the existing chat message selector instead of
hardcoding by referencing the class/field used for the locator (e.g.,
chatMessageContainer or chatMessageSelector). Move the selector string into a
single constant/instance field (e.g., this.chatMessageSelector) and pass that
selector as an argument into page.waitForFunction so the browser-context
function uses the provided selector parameter rather than a hardcoded literal;
ensure waitForBotResponse signature uses the same selector source to keep a
single point of truth.
- Around line 63-71: Remove or consolidate the unused locator fields:
objectiveNextButton, actionItemNextButton, and weeksSelectionBotMessage from the
class, or update the methods to use them consistently; specifically either
delete the declarations for objectiveNextButton and actionItemNextButton and let
clickActionItemNext and isActionItemNextEnabled continue using their inline
locators, and remove weeksSelectionBotMessage if it's unused, OR change
clickActionItemNext and isActionItemNextEnabled to reference the class fields
objectiveNextButton and actionItemNextButton (ensure the selectors match the
inline locators currently used) and replace any other inline instantiations so
all locator usage is consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f8d68a2-c2e0-4d7c-8fbf-31acd338dc54
📒 Files selected for processing (4)
src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/CommonFlow.jsxsrc/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsxsrc/utils/env.tstests/pages/mip-flow.page.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/ai-creation/pages/shikshalokam-mitra/mitra-pages/SelectObjective.jsx
| this.loadingIndicator = page.locator(".login-load-spinner") | ||
|
|
||
| // Chat messages (both user and bot messages in the chat window) | ||
| this.chatMessageContainer = page.locator("li.div34") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the actual CSS class used in chat message rendering
rg -n "div34|div35" --type=js --type=jsx --type=ts --type=tsx -g '!node_modules' -g '!dist' -C 2
# Also check CSS/SCSS files for these class definitions
rg -n "\.div34|\.div35" --type=css --type=scss -g '!node_modules'Repository: ELEVATE-Project/mohini-app-frontend
Length of output: 135
🏁 Script executed:
# Search for div34 and div35 references without problematic file type flags
rg -n "div34|div35" -C 2
# Also check the specific file to see the context
wc -l tests/pages/mip-flow.page.ts
head -80 tests/pages/mip-flow.page.ts | tail -30Repository: ELEVATE-Project/mohini-app-frontend
Length of output: 10149
Fix selector mismatch: li.div34 does not match the actual MIP chat component DOM structure.
The test page object uses li.div34 selector (lines 59, 142), but the actual MIP flow component (ChatWindow.jsx) uses <div> elements with these classes:
- Container:
<div className="div34"> - Individual messages:
<div className="div35 label1">
Update selectors to match the actual DOM:
- Line 59: Change
"li.div34"to"div.div35"(to target individual message elements) - Line 142: Change
{ selector: "li.div34", ... }to{ selector: "div.div35", ... }
Without this fix, waitForBotResponse and getChatMessageCount will fail with element not found errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/pages/mip-flow.page.ts` at line 59, The selector for chat messages is
incorrect; update the locator assignment and any related selector objects to
match the actual DOM: change the locator in tests/pages/mip-flow.page.ts where
chatMessageContainer is set from "li.div34" to "div.div35", and likewise update
the selector used in the selector object (previously { selector: "li.div34", ...
}) to { selector: "div.div35", ... } so methods like waitForBotResponse and
getChatMessageCount correctly target the individual message elements (refer to
chatMessageContainer, waitForBotResponse, and getChatMessageCount to locate
usages).
| }); | ||
| let chat_history = getCommonFlowChatHistory(); | ||
| if (Array.isArray(chat_history)) { | ||
| chat_history = chat_history.filter((chat, index) => !(index == chat_history.length - 1 && chat.source === "user")) |
There was a problem hiding this comment.
Don't hardcode "user" here, take it from a constant file
| const lastIndex = prevChatHistory.length - 1; | ||
| const lastMessage = prevChatHistory[lastIndex]; | ||
|
|
||
| if (lastIndex >= 0 && lastMessage?.source === 'bot') { |
There was a problem hiding this comment.
take the "bot" from a constant file
| } else { | ||
| const updatedMessage = { | ||
| msg: message?.msg || '', | ||
| source: 'bot', |
There was a problem hiding this comment.
take the hardcoded value from the constant file
3 participants
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests
Chores