-
Notifications
You must be signed in to change notification settings - Fork 532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removed unused feature flag handling and migrated to TanStack's query #9892
Removed unused feature flag handling and migrated to TanStack's query #9892
Conversation
…ery for improved data fetching
WalkthroughThis pull request involves the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Utils/featureFlags.tsx
(2 hunks)src/pages/Apps/PlugConfigEdit.tsx
(4 hunks)src/pages/Apps/PlugConfigList.tsx
(2 hunks)
🔇 Additional comments (12)
src/pages/Apps/PlugConfigEdit.tsx (6)
1-2
: Imports updated to use '@tanstack/react-query' and 'raviger'The imports of
useMutation
anduseQuery
from@tanstack/react-query
anduseNavigate
fromraviger
are appropriate.
25-26
: Importing 'mutate' and 'query' utilitiesThe
mutate
andquery
functions are correctly imported from"@/Utils/request/mutate"
and"@/Utils/request/query"
for data fetching and mutation.
33-35
: Initialization of 'navigate' and 'isNew' variablesThe
navigate
function is correctly initialized usinguseNavigate()
, andisNew
is appropriately set based on theslug
.
76-76
: Submit handler updated to use 'createOrUpdateConfig.mutate'The
handleSubmit
function correctly callscreateOrUpdateConfig.mutate
with the configuration payload.
79-80
: Updated 'handleDelete' to use mutationThe
handleDelete
function correctly callsdeleteConfig.mutate()
to trigger the deletion.
83-83
: Updated loading state check to use 'isLoading'The loading state is correctly updated to use
isLoading
fromuseQuery
.src/pages/Apps/PlugConfigList.tsx (3)
1-1
: Import 'useQuery' from '@tanstack/react-query'The import statement correctly imports
useQuery
from@tanstack/react-query
.
19-19
: Import 'query' utilityThe
query
function is imported from"@/Utils/request/query"
to be used withuseQuery
.
28-28
: Updated loading state check to use 'isLoading'The loading state check is correctly updated to use
isLoading
fromuseQuery
.src/Utils/featureFlags.tsx (3)
1-1
: Import 'useQuery' from '@tanstack/react-query'The import statement correctly imports
useQuery
from@tanstack/react-query
.
9-9
: Import 'query' utilityThe
query
function is imported from"@/Utils/request/query"
for use in data fetching.
60-61
: Determine 'facilityId' based on 'facility' parameterThe
facilityId
is correctly derived from thefacility
parameter, handling both string IDs and facility objects.
@coderabbitai check if the requested changes were made. |
Tip For best results, initiate chat on the files or code changes. @abhimanyurajeesh I don't see any previous review comments or requested changes in the PR history. Could you please specify which changes you would like me to verify? This will help me provide a more accurate assessment.
🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script:
Length of output: 14687 |
https://github.com/ohcnetwork/care_fe/pull/9892/files/cb3341f4702eb443c529856db193e3ebc8baf65f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Apps/PlugConfigList.tsx (1)
23-26
: Consider adding error handling to useQuery.The query configuration looks good, but it would benefit from error handling to improve user experience.
const { data, isLoading } = useQuery({ queryKey: ["list-configs"], queryFn: query(routes.plugConfig.listPlugConfigs), + onError: (error) => { + console.error("Error fetching plug configs:", error); + // Consider using your app's toast/notification system + }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Utils/featureFlags.tsx
(2 hunks)src/Utils/request/api.tsx
(1 hunks)src/pages/Apps/PlugConfigEdit.tsx
(4 hunks)src/pages/Apps/PlugConfigList.tsx
(2 hunks)
🔇 Additional comments (7)
src/Utils/request/api.tsx (1)
420-420
: LGTM! Good practice to explicitly define void body type.The addition of
TBody: Type<void>()
clearly indicates that the DELETE endpoint doesn't expect a request body, improving type safety and code clarity.src/pages/Apps/PlugConfigList.tsx (1)
28-28
: LGTM! Good use of loading state.The loading state is properly handled using the standard
isLoading
flag from React Query.src/Utils/featureFlags.tsx (1)
60-67
: LGTM! Well-structured query configuration with good practices.The implementation shows several good practices:
- Proper query key structure with facility ID
- Type-safe path params
- Conditional query execution using
enabled
src/pages/Apps/PlugConfigEdit.tsx (4)
36-40
: LGTM! Well-structured query configuration.The implementation shows good practices:
- Proper query key structure
- Conditional execution using
enabled
- Type-safe path params
56-62
: 🛠️ Refactor suggestionEnhance error handling in upsertConfig mutation.
Current error handling only logs to console. Consider providing user feedback.
const { mutate: upsertConfig } = useMutation({ mutationFn: isNew ? mutate(routes.plugConfig.createPlugConfig) : mutate(routes.plugConfig.updatePlugConfig, { pathParams: { slug } }), onSuccess: () => navigate("/apps"), - onError: (error) => console.error("Error saving config:", error), + onError: (error) => { + console.error("Error saving config:", error); + // Use your app's toast/notification system + alert("Failed to save configuration. Please try again."); + }, });Likely invalid or redundant comment.
64-70
: 🛠️ Refactor suggestionEnhance error handling in deleteConfig mutation.
Current error handling only logs to console. Consider providing user feedback.
const { mutate: deleteConfig } = useMutation({ mutationFn: mutate(routes.plugConfig.deletePlugConfig, { pathParams: { slug }, }), onSuccess: () => navigate("/apps"), - onError: (error) => console.error("Error deleting config:", error), + onError: (error) => { + console.error("Error deleting config:", error); + // Use your app's toast/notification system + alert("Failed to delete configuration. Please try again."); + }, });Likely invalid or redundant comment.
138-140
:⚠️ Potential issueAdd validation for JSON input.
The meta field accepts raw JSON input without validation, which could lead to runtime errors during submission.
<Textarea value={config.meta} onChange={(e) => { + const newValue = e.target.value; + try { + // Validate JSON as user types + JSON.parse(newValue); + setConfig((prev) => ({ ...prev, meta: newValue })); + } catch (error) { + // Optional: Show validation error to user + console.error("Invalid JSON:", error); + // Still update the field to allow work in progress + setConfig((prev) => ({ ...prev, meta: newValue })); + } }} rows={10} />Likely invalid or redundant comment.
@rithviknishad requesting for review 2 🙃 |
@rithviknishad sorry about requesting for review before syncing the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our global error handler should take care of it btw.
…error handler should take care of it
Yap noted! made the changes. |
LGTM |
@abhimanyurajeesh Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
Part of #9837
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores