feat: move tags grid to mui#904
Conversation
|
Warning Review limit reached
More reviews will be available in 4 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR centralizes tag editing into a modal, removes the dedicated edit page and legacy TagForm, simplifies multiple tag-related thunk signatures and request handling, updates the tag-list reducer shape, adds tests for saveTag, and introduces new i18n keys. ChangesTag management refactor
Sequence DiagramsequenceDiagram
participant User
participant TagListPage
participant TagsDialog
participant Redux
participant API
User->>TagListPage: Click Add or Edit
TagListPage->>TagsDialog: open(initialData)
TagsDialog->>TagsDialog: validate input
TagsDialog->>Redux: dispatch saveTag(tag)
Redux->>API: POST / PUT tag
API-->>Redux: response
Redux-->>TagListPage: updated tags state
TagListPage->>Redux: dispatch getTags(...) to refresh
Redux->>API: GET tags
API-->>Redux: tags list
Redux-->>TagListPage: new list
TagListPage->>User: render updated list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
priscila-moneo
requested review from
martinquiroga-exo,
santipalenque and
smarcet
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/tags/tag-list-page.js (1)
47-49: Consider theuseEffectdependency array.The effect uses
termfrom props but doesn't include it in the dependency array. While this appears intentional (search is triggered explicitly via Enter key), it means component won't re-fetch iftermprop changes externally. If this is the intended behavior, consider adding a comment for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/tags/tag-list-page.js` around lines 47 - 49, The useEffect that calls getTags(term, currentPage, perPage, order, orderDir) references term but does not include it in the dependency array ([currentPage, perPage, order, orderDir]); decide whether to add term to the dependency array so the effect re-runs when term prop changes, or if omission is intentional (search only on Enter) add a concise explanatory comment above the useEffect referencing useEffect and getTags to state that term is intentionally excluded to avoid automatic re-fetches; update accordingly.src/actions/tag-actions.js (1)
275-311: Missingreturnstatement insaveTagGroupfor the update path.The update branch (lines 276-287) doesn't return the promise from
putRequest, while the create branch similarly doesn't return. This is inconsistent with howsaveTagwas refactored to return promises. While this may not cause immediate issues if callers don't chain on the result, it's a potential source of bugs.Suggested fix
if (entity.id) { - putRequest( + return putRequest( createAction(UPDATE_TAG_GROUP), createAction(TAG_GROUP_UPDATED), `${window.API_BASE_URL}/api/v1/summits/${currentSummit.id}/track-tag-groups/${entity.id}`, normalizedEntity, authErrorHandler, entity )(params)(dispatch).then(() => { dispatch( showSuccessMessage(T.translate("edit_tag_group.tag_group_saved")) ); }); } else { // ... - postRequest( + return postRequest(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/tag-actions.js` around lines 275 - 311, The update branch in saveTagGroup doesn't return the promise from putRequest (and the create branch also doesn't return the postRequest promise), which prevents callers from chaining; fix by adding a return before the putRequest(...) call (and likewise before postRequest(...) for consistency) so saveTagGroup returns the promise from putRequest/postRequest; locate saveTagGroup and modify the branches that call putRequest and postRequest to return those call results.
🤖 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/tags/tag-list-page.js`:
- Around line 167-187: The onEdit prop currently names its parameter (id) but
MuiTable passes the full row object; update the onEdit callback to use a
descriptive parameter name like (row) and forward that to handleEditTag (i.e.,
change the onEdit handler from using (id) to (row) => handleEditTag(row)) so the
parameter matches the actual value passed by MuiTable and mirrors the pattern
used by onDelete and other handlers.
In `@src/pages/tags/tags-popup.js`:
- Around line 24-30: handleSave calls setError with
T.translate("edit_tag.name_required") but that i18n key is missing; add an
"name_required" entry under the edit_tag section in the English translations
(src/i18n/en.json) so T.translate("edit_tag.name_required") returns a
user-friendly string (e.g., "Name is required") and then re-run to confirm
setError shows the localized message when handleSave detects an empty tag.
---
Nitpick comments:
In `@src/actions/tag-actions.js`:
- Around line 275-311: The update branch in saveTagGroup doesn't return the
promise from putRequest (and the create branch also doesn't return the
postRequest promise), which prevents callers from chaining; fix by adding a
return before the putRequest(...) call (and likewise before postRequest(...) for
consistency) so saveTagGroup returns the promise from putRequest/postRequest;
locate saveTagGroup and modify the branches that call putRequest and postRequest
to return those call results.
In `@src/pages/tags/tag-list-page.js`:
- Around line 47-49: The useEffect that calls getTags(term, currentPage,
perPage, order, orderDir) references term but does not include it in the
dependency array ([currentPage, perPage, order, orderDir]); decide whether to
add term to the dependency array so the effect re-runs when term prop changes,
or if omission is intentional (search only on Enter) add a concise explanatory
comment above the useEffect referencing useEffect and getTags to state that term
is intentionally excluded to avoid automatic re-fetches; update accordingly.
🪄 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: 244d5413-3233-4aa7-ad2c-d6c203d0f5be
📒 Files selected for processing (8)
src/actions/tag-actions.jssrc/components/forms/tag-form.jssrc/i18n/en.jsonsrc/layouts/tag-layout.jssrc/pages/tags/edit-tag-page.jssrc/pages/tags/tag-list-page.jssrc/pages/tags/tags-popup.jssrc/reducers/tags/tag-list-reducer.js
💤 Files with no reviewable changes (2)
- src/components/forms/tag-form.js
- src/pages/tags/edit-tag-page.js
| authErrorHandler, | ||
| entity | ||
| )(params)(dispatch).then((payload) => { | ||
| )(params)(dispatch).then((response) => { |
| gap={1} | ||
| sx={{ justifyContent: "flex-end", alignItems: "center" }} | ||
| > | ||
| <TextField |
There was a problem hiding this comment.
we now have a mui search input in uicore, lets use that please
| onEdit={(id) => handleEditTag(id)} | ||
| onDelete={(id) => handleDeleteTag(id)} | ||
| getName={(row) => row.tag} | ||
| deleteConfirmTitle={T.translate("general.are_you_sure")} |
There was a problem hiding this comment.
the prop is deleteDialogTitle, and this value is the default
|
|
||
| const handleSave = () => { | ||
| if (!tag.trim()) { | ||
| setError(T.translate("edit_tag.name_required")); |
There was a problem hiding this comment.
also lets use formik validation please
priscila-moneo
force-pushed
the
feature/move-tags-grid-to-mui
branch
2 times, most recently
from
24cce78 to
12e0f3d
Compare
| entity | ||
| )(params)(dispatch).then(() => { | ||
| dispatch( | ||
| showMessage(success_message, () => { |
There was a problem hiding this comment.
New-tag creation navigates away instead of closing dialog — saveTag post-path calls showMessage(…, () => history.push("/app/tags")) inside a dialog context. This fires a full-page navigation instead of closing the popup. Align the create-tag success path with the edit path (use showSuccessMessage) so the promise resolves cleanly; the list-page .then() already handles close and refresh.
| authErrorHandler, | ||
| entity | ||
| )(params)(dispatch).then(() => { | ||
| dispatch( |
There was a problem hiding this comment.
saveTag post-path never dispatches stopLoading. startLoading is dispatched at the top, but the .then() callback for new tags calls showMessage (a modal) without ever calling dispatch(stopLoading()) — the global spinner runs indefinitely if the modal is dismissed without navigating. Add dispatch(stopLoading()) at the start of this .then() callback.
| <Button onClick={handleClose}> | ||
| {T.translate("general.cancel")} | ||
| </Button> | ||
| <Button type="submit" variant="contained"> |
There was a problem hiding this comment.
Missing save guard — this popup does not implement the project's mandatory Save Guard Pattern. There is no isSaving state, the submit button is not disabled during save, disableEscapeKeyDown is missing, and there is no .catch to keep the dialog open on API failure. Rapid double-clicks will fire duplicate saveTag calls; on error the dialog silently closes and the user loses their input. Add isSaving state, disable submit/close/escape during save, and wrap onSave in .catch(() => { /* keep open */ }).finally(() => setIsSaving(false)).
| /> | ||
|
|
||
| <TagsDialog | ||
| open={dialogOpen} |
There was a problem hiding this comment.
Dialog is always rendered (open={bool}) instead of conditionally mounted. The project's mandatory popup pattern requires {dialogOpen && <TagsDialog />} to guarantee full state teardown on close. The current approach keeps the component mounted; the useEffect guard is fragile if initialData reference identity does not change between edits. Change to conditional render with open hardcoded to true inside the popup.
| }; | ||
|
|
||
| const handleSaveTag = (entity) => { | ||
| saveTag(entity).then(() => { |
There was a problem hiding this comment.
Neither handleSaveTag nor handleDeleteTag handle rejected promises. For delete, the reducer removes the row optimistically via TAG_DELETED before the API confirms success — a failure leaves stale data in the list with no recovery. Add .catch(() => getTags(…)) on delete to re-sync on failure; keep the dialog open on save error.
| useEffect(() => { | ||
| setSearch(""); | ||
| getTags("", 1, DEFAULT_PER_PAGE, "id", 1); | ||
| }, []); |
There was a problem hiding this comment.
Tag list does not reload on summit change. The standard pattern uses useEffect([currentSummit]) to react to summit switches; this component uses a mount-only useEffect([], []), so switching summits will display stale tags from the previous summit. Add currentSummit to mapStateToProps, change the effect dependency to [currentSummit], and guard with if (currentSummit) getTags(…).
| case REQUEST_TAGS: { | ||
| let { order, orderDir, term, page } = payload; | ||
| return { ...state, order, orderDir, term, currentPage: page }; | ||
| const { order, orderDir, term, page, perPage } = payload; |
There was a problem hiding this comment.
List reducer is missing the SET_CURRENT_SUMMIT case. Peer reducers (e.g. tag-group-list-reducer.js) reset to DEFAULT_STATE on both SET_CURRENT_SUMMIT and LOGOUT_USER to prevent stale data when the active summit changes. LOGOUT_USER is handled (line 37) but SET_CURRENT_SUMMIT is absent — switching summits will not flush the tag list. Add: case SET_CURRENT_SUMMIT: case LOGOUT_USER: return DEFAULT_STATE;
|
@priscila-moneo
|
priscila-moneo
force-pushed
the
feature/move-tags-grid-to-mui
branch
from
12e0f3d to
fee30e5
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/tag-actions.js (1)
217-224:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid mutating
tagGroupsentries before request payload build.Line 218 deletes
allowed_tagsfrom the object returned by Line 217. That mutates caller data and can desync UI state.Suggested fix
- const tagGroup = tagGroups.find((tg) => tg.id === tagGroupId); - delete tagGroup.allowed_tags; + const sourceTagGroup = tagGroups.find((tg) => tg.id === tagGroupId); + if (!sourceTagGroup) { + return Promise.reject(new Error(`Tag group ${tagGroupId} not found`)); + } + const tagGroup = { ...sourceTagGroup }; + delete tagGroup.allowed_tags;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/actions/tag-actions.js` around lines 217 - 224, The code mutates the found tagGroup by deleting allowed_tags which can desync UI state; instead create a shallow copy before removing the field and send that as the request payload. Locate the tagGroups.find(...) call that assigns tagGroup and replace mutation with a copied object (e.g., const payload = { ...tagGroup }; delete payload.allowed_tags;) and pass payload to putRequest while leaving the original tagGroup/tagGroups unchanged; ensure createAction(TAG_GROUP_ORDER_UPDATED)(tagGroups) still uses the unmutated tagGroups.
♻️ Duplicate comments (1)
src/actions/tag-actions.js (1)
159-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove loading cleanup to
finallyinsaveTag.Line 150 sets loading, but Lines 167 and 179 only clear it on success. A rejected request can leave the spinner stuck.
Suggested fix
if (entity.id) { return putRequest( createAction(UPDATE_TAG), createAction(TAG_UPDATED), `${window.API_BASE_URL}/api/v1/tags/${entity.id}`, normalizedEntity, authErrorHandler, entity - )(params)(dispatch).then(() => { - dispatch(stopLoading()); + )(params)(dispatch).then(() => { dispatch(showSuccessMessage(T.translate("edit_tag.tag_saved"))); + }).finally(() => { + dispatch(stopLoading()); }); } return postRequest( createAction(UPDATE_TAG), createAction(TAG_ADDED), `${window.API_BASE_URL}/api/v1/tags`, normalizedEntity, authErrorHandler, entity - )(params)(dispatch).then(() => { - dispatch(stopLoading()); + )(params)(dispatch).then(() => { dispatch(showSuccessMessage(T.translate("edit_tag.tag_created"))); + }).finally(() => { + dispatch(stopLoading()); }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/actions/tag-actions.js` around lines 159 - 181, The loading spinner is only cleared on success in saveTag; ensure stopLoading is always called by moving the dispatch(stopLoading()) into a .finally() on both the putRequest and postRequest promise chains (keep the success message dispatch in .then()). Locate the saveTag logic that calls putRequest(...) and postRequest(...), and append .finally(() => { dispatch(stopLoading()); }) to each returned promise (while leaving dispatch(showSuccessMessage(...)) in the existing .then()) so the spinner is removed on rejection as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/actions/tag-actions.js`:
- Around line 217-224: The code mutates the found tagGroup by deleting
allowed_tags which can desync UI state; instead create a shallow copy before
removing the field and send that as the request payload. Locate the
tagGroups.find(...) call that assigns tagGroup and replace mutation with a
copied object (e.g., const payload = { ...tagGroup }; delete
payload.allowed_tags;) and pass payload to putRequest while leaving the original
tagGroup/tagGroups unchanged; ensure
createAction(TAG_GROUP_ORDER_UPDATED)(tagGroups) still uses the unmutated
tagGroups.
---
Duplicate comments:
In `@src/actions/tag-actions.js`:
- Around line 159-181: The loading spinner is only cleared on success in
saveTag; ensure stopLoading is always called by moving the
dispatch(stopLoading()) into a .finally() on both the putRequest and postRequest
promise chains (keep the success message dispatch in .then()). Locate the
saveTag logic that calls putRequest(...) and postRequest(...), and append
.finally(() => { dispatch(stopLoading()); }) to each returned promise (while
leaving dispatch(showSuccessMessage(...)) in the existing .then()) so the
spinner is removed on rejection as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00b3f46d-24e3-4bfa-8fd3-9479c5bd5a1e
📒 Files selected for processing (5)
src/actions/__tests__/tag-actions.test.jssrc/actions/tag-actions.jssrc/components/forms/tag-form.jssrc/i18n/en.jsonsrc/layouts/tag-layout.js
💤 Files with no reviewable changes (1)
- src/components/forms/tag-form.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/layouts/tag-layout.js
Signed-off-by: Priscila Moneo <[email protected]>
priscila-moneo
force-pushed
the
feature/move-tags-grid-to-mui
branch
from
fee30e5 to
4770783
Compare
3 participants
ref: https://app.clickup.com/t/86b9kkxkx
Summary by CodeRabbit
New Features
Refactor
Tests
Localization