Fix/query engine limitfields mock - #231
Conversation
…nt and structured response to getData
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds QueryEngine field-limiting and cursor pagination to public and dashboard data endpoints; rewrites dashboard getData response shape; refactors external DB config verification, upload path handling, analytics logs mapping; and applies dispersed comment/format cleanups across project controller. ChangesQueryEngine Integration & Data Query Features
Data Validation, Upload, & Analytics Refactors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
apps/dashboard-api/src/controllers/project.controller.js (2)
766-773:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
getDataerrors to the controller contract.This endpoint now returns
{ success, data, message }on success, but its 404 branches still return raw{ error }, and the catch block forwardserr.messagedirectly. That also turnsQueryEnginevalidation failures into generic 500s here instead of a controlled 4xx response.🛠️ Suggested direction
-module.exports.getData = async (req, res) => { +module.exports.getData = async (req, res, next) => { try { const { projectId, collectionName } = req.params; const project = await Project.findOne({ _id: projectId, owner: req.user._id }); - if (!project) return res.status(404).json({ error: "Project not found." }); + if (!project) return next(new AppError(404, "Project not found.")); const collectionConfig = project.collections.find(c => c.name === collectionName); if (!collectionConfig) { - return res.status(404).json({ - error: "Collection not found", - collection: collectionName - }); + return next(new AppError(404, "Collection not found.")); } // ... } catch (err) { - res.status(500).json({ error: err.message }); + if (err?.statusCode === 400 || err?.name === "QueryFilterError") { + return next(new AppError(400, err.message || "Invalid query filter.")); + } + return next(new AppError(500, "Failed to fetch data.")); } };As per coding guidelines,
**/src/controllers/**/*.{js,ts}: All API endpoints return:{ success: bool, data: {}, message: "" }. Use AppError class for errors — never raw throw, never expose MongoDB errors to client.Also applies to: 852-853
🤖 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 `@apps/dashboard-api/src/controllers/project.controller.js` around lines 766 - 773, Normalize all error responses in the getData controller to the standard { success: boolean, data: {}, message: "" } shape: replace the raw 404 returns (when project is missing and when collectionConfig is missing) to return res.status(404).json({ success:false, data:{}, message: "Project not found." }) and res.status(404).json({ success:false, data:{}, message: `Collection ${collectionName} not found.` }) respectively, and update the catch block to wrap errors using the AppError class (map QueryEngine validation errors to a 4xx AppError) before sending a response so you never forward raw err.message or MongoDB internals to the client; locate these changes around the getData controller and the collectionName/project/collectionConfig checks and the existing catch that currently forwards err.message.
337-344:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMove the log cap after per-project aggregation.
$limit: 100is applied before$group, so health is computed from the latest 100 logs across all of the owner's projects, not the latest logs for each project. A busy project can crowd out the rest, and those projects will default tohealthyeven if their recent requests were failing.🤖 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 `@apps/dashboard-api/src/controllers/project.controller.js` around lines 337 - 344, The $limit stage is currently placed before the $group, so errorCount/successCount (from the aggregation that references _id: "$projectId", errorCount, successCount) are computed from the latest 100 logs across all projects instead of per-project; remove the pre-group { $limit: 100 } and either move a limit after grouping (if you intend to cap number of projects) or, to cap logs per project, change the $group to collect per-project logs with $push and $slice (e.g. push a log entry array then $slice to 100) and compute errorCount/successCount from that sliced array so each project’s health uses its most recent up to 100 logs.
🤖 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 `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 766-773: Normalize all error responses in the getData controller
to the standard { success: boolean, data: {}, message: "" } shape: replace the
raw 404 returns (when project is missing and when collectionConfig is missing)
to return res.status(404).json({ success:false, data:{}, message: "Project not
found." }) and res.status(404).json({ success:false, data:{}, message:
`Collection ${collectionName} not found.` }) respectively, and update the catch
block to wrap errors using the AppError class (map QueryEngine validation errors
to a 4xx AppError) before sending a response so you never forward raw
err.message or MongoDB internals to the client; locate these changes around the
getData controller and the collectionName/project/collectionConfig checks and
the existing catch that currently forwards err.message.
- Around line 337-344: The $limit stage is currently placed before the $group,
so errorCount/successCount (from the aggregation that references _id:
"$projectId", errorCount, successCount) are computed from the latest 100 logs
across all projects instead of per-project; remove the pre-group { $limit: 100 }
and either move a limit after grouping (if you intend to cap number of projects)
or, to cap logs per project, change the $group to collect per-project logs with
$push and $slice (e.g. push a log entry array then $slice to 100) and compute
errorCount/successCount from that sliced array so each project’s health uses its
most recent up to 100 logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c08301b7-87a2-486f-82ea-3977c0f0ac29
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
apps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/__tests__/data.controller.read.test.jsapps/public-api/src/controllers/data.controller.js
|
please fix this last |
These issues are already addressed in PR #230 (fix/dashboard-getData-query-engine) which contains the full getData refactor and aggregation fix. This PR (#231) only adds limitFields() to the QueryEngine mock to fix CI. |
|
ok got it |
Sorry about that! The comments were accidentally removed during the rebase conflict resolution process @yash-pouranik |
ready to merge? or do i need to change em? |
|
U havent revovered the comments? and jsdoc as well? |
|
@Mansi0905 |
i have recovered them already |
|
cool |
2 participants
fix: add limitFields() to QueryEngine mock so CI passes
resulting in populate() never being called and a 500 response
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests