feat: discard only current active action #602
Conversation
RohitR311
added
Type: Enhancement
WalkthroughThe code update adds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant RightSidePanel
participant GlobalInfoStore
participant BrowserStepsContext
RightSidePanel->>GlobalInfoStore: get/set currentTextActionId, currentListActionId, currentScreenshotActionId
RightSidePanel->>GlobalInfoStore: generate new actionId on capture start
RightSidePanel->>BrowserStepsContext: addTextStep(label, data, selectorObj, actionId)
BrowserStepsContext->>BrowserStepsContext: Create TextStep with actionId
BrowserStepsContext-->>RightSidePanel: Step added
RightSidePanel->>BrowserStepsContext: addListStep(listSelector, fields, listId, actionId, ...)
BrowserStepsContext->>BrowserStepsContext: Assign actionId to list and fields
BrowserStepsContext-->>RightSidePanel: ListStep added
RightSidePanel->>BrowserStepsContext: addScreenshotStep(fullPage, actionId)
BrowserStepsContext->>BrowserStepsContext: Create ScreenshotStep with actionId
BrowserStepsContext-->>RightSidePanel: ScreenshotStep added
RightSidePanel->>BrowserStepsContext: deleteStepsByActionId(actionId)
BrowserStepsContext->>BrowserStepsContext: Remove all steps with matching actionId
BrowserStepsContext-->>RightSidePanel: Steps deleted
RightSidePanel->>GlobalInfoStore: clear actionId on confirm or discard
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
RohitR311
removed
the
Status: Work In Progess
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/components/recorder/RightSidePanel.tsx (2)
523-528:⚠️ Potential issueForgot to clear
currentScreenshotActionIdafter the captureOnce the screenshot is taken we call
finishAction('screenshot'), but the global store still retains the oldcurrentScreenshotActionId. This stale value will be:• Exposed to any component reading the store
• Passed toaddScreenshotStepon the next capture before it is overwritten, risking mis-associationQuick fix:
addScreenshotStep(fullPage, currentScreenshotActionId); stopGetScreenshot(); +setCurrentScreenshotActionId(''); // clear scoped action id resetInterpretationLog(); finishAction('screenshot'); onFinishCapture();Please make the same adjustment in the inline discard handler a few lines below (
onClickthat stops screenshot capture).
785-789: 🛠️ Refactor suggestionClear screenshot action ID when the user discards a screenshot capture
For consistency with text and list discards, add:
stopGetScreenshot(); setActiveAction('none'); + setCurrentScreenshotActionId('');Without this, the stale ID issue described above also occurs on a discard flow.
♻️ Duplicate comments (1)
src/components/recorder/RightSidePanel.tsx (1)
486-512: Same duplication pattern as above
discardGetListre-implements the same clean-up loops; applying the helper proposed in the previous comment removes ~25 lines here as well and prevents the two functions from drifting apart.
🧹 Nitpick comments (1)
src/components/recorder/RightSidePanel.tsx (1)
443-482: Large block of near-duplicate clean-up logic – extract a helper
discardGetTextmanually:
- Finds steps for the current action
- Deletes them through
deleteStepsByActionId- Mirrors that deletion into three separate local state maps (
textLabels,errors,confirmedTextSteps)
discardGetList(↓) repeats the same idea with slightly different state maps, and a future screenshot discard will likely do it again.Duplicated, state-specific loops are easy to forget to update and are prone to bugs (e.g. an extra local map is added later and not cleared here).
Suggestion:
+const clearMaps = <T extends Record<number | string, any>>( + ids: number[], + ...maps: React.Dispatch<React.SetStateAction<T>>[] +) => { + maps.forEach(setMap => + setMap(prev => { + const copy = { ...prev }; + ids.forEach(id => delete copy[id]); + return copy; + }) + ); +};const discardGetText = useCallback(() => { stopGetText(); if (currentTextActionId) { const stepsToDelete = browserSteps .filter(s => s.actionId === currentTextActionId) .map(s => s.id); deleteStepsByActionId(currentTextActionId); clearMaps(stepsToDelete, setTextLabels, setErrors, setConfirmedTextSteps); } setCurrentTextActionId(''); setIsCaptureTextConfirmed(false); notify('error', t('right_panel.errors.capture_text_discarded')); }, [currentTextActionId, browserSteps, ...]);Benefits: DRY, safer, and easier to extend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/recorder/RightSidePanel.tsx(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/recorder/RightSidePanel.tsx (2)
src/context/globalInfo.tsx (1)
useGlobalInfoStore(123-123)src/context/browserSteps.tsx (1)
useBrowserSteps(230-236)
| const handleStartGetText = () => { | ||
| setIsCaptureTextConfirmed(false); | ||
| const newActionId = `text-${Date.now()}`; | ||
| setCurrentTextActionId(newActionId); | ||
| startGetText(); | ||
| } | ||
|
|
||
| const handleStartGetList = () => { | ||
| setIsCaptureListConfirmed(false); | ||
| const newActionId = `list-${Date.now()}`; | ||
| setCurrentListActionId(newActionId); | ||
| startGetList(); | ||
| } | ||
|
|
||
| const handleStartGetScreenshot = () => { | ||
| const newActionId = `screenshot-${Date.now()}`; | ||
| setCurrentScreenshotActionId(newActionId); | ||
| startGetScreenshot(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Generate truly-unique actionIds
Date.now() is generally “unique enough”, but two quick successive clicks can still produce the same timestamp (e.g. on fast hardware or in unit tests with mocked timers). A collision would cause two different capture sessions to share the same actionId, defeating the whole purpose of scoping.
Consider using crypto.randomUUID() (native in modern browsers / Node ≥ 14.17) or a tiny uuid helper to guarantee uniqueness:
-const newActionId = `text-${Date.now()}`;
+const newActionId = `text-${crypto.randomUUID()}`; // likewise for list- and screenshot-flowsThis small change fully eliminates the collision risk without adding any noticeable overhead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleStartGetText = () => { | |
| setIsCaptureTextConfirmed(false); | |
| const newActionId = `text-${Date.now()}`; | |
| setCurrentTextActionId(newActionId); | |
| startGetText(); | |
| } | |
| const handleStartGetList = () => { | |
| setIsCaptureListConfirmed(false); | |
| const newActionId = `list-${Date.now()}`; | |
| setCurrentListActionId(newActionId); | |
| startGetList(); | |
| } | |
| const handleStartGetScreenshot = () => { | |
| const newActionId = `screenshot-${Date.now()}`; | |
| setCurrentScreenshotActionId(newActionId); | |
| startGetScreenshot(); | |
| const handleStartGetText = () => { | |
| setIsCaptureTextConfirmed(false); | |
| - const newActionId = `text-${Date.now()}`; | |
| + const newActionId = `text-${crypto.randomUUID()}`; // likewise for list- and screenshot-flows | |
| setCurrentTextActionId(newActionId); | |
| startGetText(); | |
| } |
🤖 Prompt for AI Agents
In src/components/recorder/RightSidePanel.tsx around lines 140 to 157, the
current use of Date.now() to generate action IDs can lead to collisions if two
actions start in the same millisecond. Replace Date.now() with
crypto.randomUUID() to generate truly unique action IDs for text, list, and
screenshot actions, ensuring no collisions even on rapid successive clicks.
What this PR does?
Functionality to discard only the current active actions on clicking discard.
Closes: #586
Summary by CodeRabbit
New Features
Improvements