-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
chore: Add error logs for consolidated API #35779
Conversation
WalkthroughThe recent changes enhance error logging within the Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (17 hunks)
Additional context used
Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)
Learnt from: sumitsum PR: appsmithorg/appsmith#29875 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0 Timestamp: 2023-12-27T21:27:19.887Z Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Additional comments not posted (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (11)
157-157
: Well done on adding error logging!The error logging for fetching user profile data is clear and descriptive.
166-166
: Great job on the error logging!The error logging for fetching feature flags is concise and informative.
177-177
: Nice addition of error logging!The error logging for fetching tenant configuration is well-placed and clear.
236-236
: Error logging looks good!The error logging for fetching application pages is appropriately detailed.
247-247
: Effective error logging!The error logging for fetching the current theme is clear and precise.
258-258
: Error logging is well-implemented!The error logging for fetching themes is informative and well-placed.
268-268
: Good work on the error logging!The error logging for fetching custom JS libraries is clear and descriptive.
278-278
: Error logging is clear!The error logging for fetching the current page is well-structured and informative.
294-294
: Excellent error logging!The error logging for fetching actions in view mode is precise and helpful.
305-305
: Great error logging!The error logging for fetching action collections in view mode is clear and effective.
371-371
: Well-placed error logging!The error logging for fetching plugins is informative and well-placed.
Description
PR to add error logs in the consolidated-api as whenever DB operation fails we are not logging these anywhere and ends up in the response only.
/test Sanity
Communication
Should the DevRel and Marketing teams inform users about this change?
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10466559600
Commit: c04482e
Cypress dashboard.
Tags:
@tag.Sanity
Spec:
Tue, 20 Aug 2024 07:27:38 UTC
Summary by CodeRabbit