feat: move tags grid to mui#904
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR refactors tag management by eliminating a dedicated edit page and replacing it with a modal dialog approach. Redux action thunks are simplified, class components are converted to functional components with hooks, legacy UI libraries are replaced with Material-UI, and the reducer state structure is updated. Changes
Sequence DiagramsequenceDiagram
participant User
participant TagListPage
participant TagsDialog
participant Redux as Redux<br/>(Actions)
participant API
User->>TagListPage: Click Add or Edit Tag
TagListPage->>TagsDialog: Open Modal (with initialData)
TagsDialog->>TagsDialog: Initialize local state from props
User->>TagsDialog: Enter tag name & Click Save
TagsDialog->>TagsDialog: Validate (non-empty)
alt Validation Fails
TagsDialog->>TagsDialog: Show error message
else Validation Passes
TagsDialog->>Redux: Dispatch saveTag(tag)
Redux->>API: PUT/POST request
API-->>Redux: Response
Redux-->>TagListPage: State updated
TagsDialog->>TagListPage: onClose (reset dialog)
TagListPage->>Redux: Dispatch getTags (refresh)
Redux->>API: GET tags request
API-->>Redux: Tags list
Redux-->>TagListPage: Updated tags state
TagListPage->>User: Display updated list
end
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. Review rate limit: 0/1 reviews remaining, refill in 54 minutes and 20 seconds.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
from
fe32552 to
24cce78
Compare
priscila-moneo
force-pushed
the
feature/move-tags-grid-to-mui
branch
from
24cce78 to
12e0f3d
Compare
2 participants
ref: https://app.clickup.com/t/86b9kkxkx
Summary by CodeRabbit
New Features
Refactor