fix(uikit-playground): fix project and screen state update after deletion#39241
Open
AbishekRaj2007 wants to merge 2 commits intoRocketChat:developfrom
Open
fix(uikit-playground): fix project and screen state update after deletion#39241AbishekRaj2007 wants to merge 2 commits intoRocketChat:developfrom
AbishekRaj2007 wants to merge 2 commits intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
WalkthroughDeleteScreen reducer was changed to remove associated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/uikit-playground/src/Context/reducer.ts`:
- Around line 216-222: The reducer currently resets state.activeScreen to the
first screen whenever a screen is removed; change it so you only reassign
activeScreen if the deleted screen was the active one or if the activeScreen no
longer exists. In the reducer (around state.projects[activeProject].screens
handling), detect which screen is being deleted and if that id !==
state.activeScreen, leave state.activeScreen unchanged; if it was the active
screen (or the project becomes empty), then set state.activeScreen to the first
remaining screen or '' and handle deleting the empty project and resetting
state.activeProject as already implemented.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/uikit-playground/src/Context/reducer.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/uikit-playground/src/Context/reducer.ts
🧠 Learnings (2)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/uikit-playground/src/Context/reducer.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/uikit-playground/src/Context/reducer.ts
Comment on lines
216
to
222
| if (state.projects[activeProject].screens.length > 0) { | ||
| state.activeScreen = state.projects[activeProject].screens[0]; | ||
| } else { | ||
| delete state.projects[activeProject]; | ||
| state.activeProject = ''; | ||
| state.activeScreen = ''; | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve activeScreen when deleting a non-active screen.
At Line 216, the reducer always reassigns activeScreen to the first remaining screen. That also runs when deleting a non-active screen, causing unexpected selection jumps.
Suggested fix
- if (state.projects[activeProject].screens.length > 0) {
- state.activeScreen = state.projects[activeProject].screens[0];
+ const remainingScreens = state.projects[activeProject].screens;
+ if (remainingScreens.length > 0) {
+ if (
+ state.activeScreen === action.payload ||
+ !remainingScreens.includes(state.activeScreen)
+ ) {
+ state.activeScreen = remainingScreens[0];
+ }
} else {
delete state.projects[activeProject];
state.activeProject = '';
state.activeScreen = '';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (state.projects[activeProject].screens.length > 0) { | |
| state.activeScreen = state.projects[activeProject].screens[0]; | |
| } else { | |
| delete state.projects[activeProject]; | |
| state.activeProject = ''; | |
| state.activeScreen = ''; | |
| } | |
| const remainingScreens = state.projects[activeProject].screens; | |
| if (remainingScreens.length > 0) { | |
| if ( | |
| state.activeScreen === action.payload || | |
| !remainingScreens.includes(state.activeScreen) | |
| ) { | |
| state.activeScreen = remainingScreens[0]; | |
| } | |
| } else { | |
| delete state.projects[activeProject]; | |
| state.activeProject = ''; | |
| state.activeScreen = ''; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/uikit-playground/src/Context/reducer.ts` around lines 216 - 222, The
reducer currently resets state.activeScreen to the first screen whenever a
screen is removed; change it so you only reassign activeScreen if the deleted
screen was the active one or if the activeScreen no longer exists. In the
reducer (around state.projects[activeProject].screens handling), detect which
screen is being deleted and if that id !== state.activeScreen, leave
state.activeScreen unchanged; if it was the active screen (or the project
becomes empty), then set state.activeScreen to the first remaining screen or ''
and handle deleting the empty project and resetting state.activeProject as
already implemented.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/uikit-playground/src/Context/reducer.ts">
<violation number="1" location="apps/uikit-playground/src/Context/reducer.ts:217">
P1: `activeScreen` is unconditionally reset to the first remaining screen on every deletion, even when the deleted screen is not the active one. This causes the user's current selection to jump unexpectedly. The reassignment should only happen when the deleted screen was active or when `activeScreen` is no longer in the remaining screens list.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ].flowNodes.filter((node) => node.id !== action.payload); | ||
|
|
||
| if (state.projects[activeProject].screens.length > 0) { | ||
| state.activeScreen = state.projects[activeProject].screens[0]; |
Contributor
There was a problem hiding this comment.
P1: activeScreen is unconditionally reset to the first remaining screen on every deletion, even when the deleted screen is not the active one. This causes the user's current selection to jump unexpectedly. The reassignment should only happen when the deleted screen was active or when activeScreen is no longer in the remaining screens list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/uikit-playground/src/Context/reducer.ts, line 217:
<comment>`activeScreen` is unconditionally reset to the first remaining screen on every deletion, even when the deleted screen is not the active one. This causes the user's current selection to jump unexpectedly. The reassignment should only happen when the deleted screen was active or when `activeScreen` is no longer in the remaining screens list.</comment>
<file context>
@@ -223,6 +213,14 @@ const reducer = (state: initialStateType, action: IAction) => {
].flowNodes.filter((node) => node.id !== action.payload);
+ if (state.projects[activeProject].screens.length > 0) {
+ state.activeScreen = state.projects[activeProject].screens[0];
+ } else {
+ delete state.projects[activeProject];
</file context>
Contributor
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)
apps/uikit-playground/src/Context/reducer.ts (1)
305-310:⚠️ Potential issue | 🔴 CriticalAvoid dereferencing a project after deleting it in
DeleteScreen.If the deleted screen is the last one, Line 307 deletes
state.projects[activeProject], but Line 313 and Line 318 still read from that deleted project. This can throw at runtime.Suggested fix (reorder cleanup, then branch on emptiness)
case ActionTypes.DeleteScreen: { - delete state.screens[action.payload]; - state.projects[activeProject].screens = [...state.projects[activeProject].screens.filter((id) => id !== action.payload)]; - if (state.projects[activeProject].screens.length > 0) { - state.activeScreen = state.projects[activeProject].screens[0]; - } else if (state.projects[activeProject].screens.length === 0) { - if (Object.keys(state.projects).length > 0) { - delete state.projects[activeProject]; - state.activeProject = ''; - state.activeScreen = ''; - } - } - - state.projects[activeProject].flowEdges = state.projects[activeProject].flowEdges.filter( - (edge) => edge.source !== action.payload && edge.target !== action.payload, - ); - state.projects[activeProject].flowNodes = state.projects[activeProject].flowNodes.filter((node) => node.id !== action.payload); + const project = state.projects[activeProject]; + delete state.screens[action.payload]; + project.screens = project.screens.filter((id) => id !== action.payload); + project.flowEdges = project.flowEdges.filter((edge) => edge.source !== action.payload && edge.target !== action.payload); + project.flowNodes = project.flowNodes.filter((node) => node.id !== action.payload); + + if (project.screens.length === 0) { + delete state.projects[activeProject]; + const fallbackProjectId = Object.keys(state.projects)[0] ?? ''; + state.activeProject = fallbackProjectId; + state.activeScreen = fallbackProjectId ? state.projects[fallbackProjectId].screens[0] ?? '' : ''; + return { ...state }; + }Also applies to: 313-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/uikit-playground/src/Context/reducer.ts` around lines 305 - 310, In DeleteScreen (reducer) avoid reading state.projects[activeProject] after you delete it: first check whether the project will become empty by inspecting state.projects[activeProject].screens length, then perform any reassignment of state.activeProject/state.activeScreen and only afterwards delete state.projects[activeProject]; or alternatively capture a const project = state.projects[activeProject] and use project to read screens before deleting. Update all branches that reference state.projects[activeProject] (the DeleteScreen logic that adjusts activeProject/activeScreen) so they either read from the cached project or reorder operations to set activeProject/activeScreen before calling delete on state.projects.
♻️ Duplicate comments (1)
apps/uikit-playground/src/Context/reducer.ts (1)
303-305:⚠️ Potential issue | 🟠 MajorPreserve
activeScreenwhen deleting a non-active screen.Line 304 always resets
activeScreento the first remaining screen, which causes unexpected selection jumps for non-active deletions.Suggested fix
- if (state.projects[activeProject].screens.length > 0) { - state.activeScreen = state.projects[activeProject].screens[0]; + const remainingScreens = state.projects[activeProject].screens; + if (remainingScreens.length > 0) { + if ( + state.activeScreen === action.payload || + !remainingScreens.includes(state.activeScreen) + ) { + state.activeScreen = remainingScreens[0]; + } } else if (state.projects[activeProject].screens.length === 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/uikit-playground/src/Context/reducer.ts` around lines 303 - 305, The reducer currently always sets state.activeScreen = state.projects[activeProject].screens[0] after a deletion, which resets selection for non-active deletions; change the logic in the delete-screen branch so you only reassign state.activeScreen when the removed screen was the active one or the current activeScreen is no longer present (e.g., check if deletedScreenId === state.activeScreen or !state.projects[activeProject].screens.includes(state.activeScreen)), otherwise preserve state.activeScreen; use the existing symbols state.activeScreen, state.projects[activeProject].screens and activeProject to locate and implement this conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/uikit-playground/src/Context/reducer.ts`:
- Line 212: Remove the unresolved merge markers ("=======") left in
apps/uikit-playground/src/Context/reducer.ts (they appear around the reducer
implementation) — open the reducer file, delete the conflict marker lines at the
locations shown (around line 212 and 321), and reconcile any surrounding
duplicate/conflicting blocks so the reducer function (the reducer export/handle
switch) contains a single valid TypeScript implementation and the file compiles.
---
Outside diff comments:
In `@apps/uikit-playground/src/Context/reducer.ts`:
- Around line 305-310: In DeleteScreen (reducer) avoid reading
state.projects[activeProject] after you delete it: first check whether the
project will become empty by inspecting state.projects[activeProject].screens
length, then perform any reassignment of state.activeProject/state.activeScreen
and only afterwards delete state.projects[activeProject]; or alternatively
capture a const project = state.projects[activeProject] and use project to read
screens before deleting. Update all branches that reference
state.projects[activeProject] (the DeleteScreen logic that adjusts
activeProject/activeScreen) so they either read from the cached project or
reorder operations to set activeProject/activeScreen before calling delete on
state.projects.
---
Duplicate comments:
In `@apps/uikit-playground/src/Context/reducer.ts`:
- Around line 303-305: The reducer currently always sets state.activeScreen =
state.projects[activeProject].screens[0] after a deletion, which resets
selection for non-active deletions; change the logic in the delete-screen branch
so you only reassign state.activeScreen when the removed screen was the active
one or the current activeScreen is no longer present (e.g., check if
deletedScreenId === state.activeScreen or
!state.projects[activeProject].screens.includes(state.activeScreen)), otherwise
preserve state.activeScreen; use the existing symbols state.activeScreen,
state.projects[activeProject].screens and activeProject to locate and implement
this conditional.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/uikit-playground/src/Context/reducer.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/uikit-playground/src/Context/reducer.ts
🧠 Learnings (2)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/uikit-playground/src/Context/reducer.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/uikit-playground/src/Context/reducer.ts
🧬 Code graph analysis (1)
apps/uikit-playground/src/Context/reducer.ts (2)
apps/uikit-playground/src/utils/filterEdges.ts (1)
filterEdges(3-5)packages/apps-engine/tests/test-data/utilities.ts (1)
getDate(193-195)
🪛 Biome (2.4.4)
apps/uikit-playground/src/Context/reducer.ts
[error] 212-212: Expected a statement but instead found '======='.
(parse)
[error] 321-321: Expected a statement but instead found '======='.
(parse)
| (edge) => | ||
| edge.source !== action.payload && edge.target !== action.payload | ||
| ); | ||
| ======= |
Contributor
There was a problem hiding this comment.
Remove unresolved merge-conflict markers before merge.
======= at Line 212 and Line 321 makes the reducer invalid TypeScript and blocks build/runtime parsing.
Suggested cleanup
-=======
...
-=======Also applies to: 321-321
🧰 Tools
🪛 Biome (2.4.4)
[error] 212-212: Expected a statement but instead found '======='.
(parse)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/uikit-playground/src/Context/reducer.ts` at line 212, Remove the
unresolved merge markers ("=======") left in
apps/uikit-playground/src/Context/reducer.ts (they appear around the reducer
implementation) — open the reducer file, delete the conflict marker lines at the
locations shown (around line 212 and 321), and reconcile any surrounding
duplicate/conflicting blocks so the reducer function (the reducer export/handle
switch) contains a single valid TypeScript implementation and the file compiles.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
This PR addresses a state management bug in the UIKit Playground where deleting a screen or project failed to correctly update
activeScreenandactiveProjectin the application state.Root Cause:
The reducer was recalculating active state (determining which screen/project should be selected after deletion) before removing the target screen from the list. This meant the recalculation operated on stale data — the deleted item was still present in state when the new active indices were being resolved, leading to the UI pointing to an incorrect or non-existent screen/project.
Changes Made:
activeScreenandactiveProjectreflect a valid selection after any deletion operation.No new dependencies were introduced. The changes are isolated to the reducer logic handling deletion actions.
Issue(s)
Closes #39240
Steps to Test or Reproduce
To reproduce the bug (before this fix):
activeScreendoes not update correctly — the UI may still reference the deleted screen or show an incorrect selection.To verify the fix:
activeScreenandactiveProjectremain unchanged and correct.Further Comments
This fix is confined to the reducer and does not affect component rendering logic or the public API surface of the Playground. The reordering of operations is the minimal-footprint correction required — no architectural changes were needed.
If the project in question has downstream persistence (e.g., local storage or session sync), reviewers should verify that the deletion cascade to empty projects does not introduce any unintended side effects in those layers.
Summary by CodeRabbit