Fix/dashboard get data query engine - #230
Conversation
…del with required parameters - Add collection config lookup before calling getCompiledModel - Pass all 4 required parameters: connection, collectionConfig, projectId, isExternal - Handle collection not found error properly - Fixes TypeError: getAllData is not a function in tests
- Update getCompiledModel mock to accept connection, collectionConfig, projectId, isExternal params - Make mock return a model object with find() method immediately (synchronously) - Ensure QueryEngine is properly constructed with query parameter
…tibility - Move QueryEngine operations into try-catch to handle validation errors with statusCode - Ensure engine variable is accessible after filter/sort/populate operations - Fix variable scope issue where engine was not accessible for count() call
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDashboard and Public API controllers: recentLogs aggregation now computes counts from a capped per-project subset; data endpoints return structured 4xx JSON for missing entities and query errors; Public API list endpoint chains QueryEngine.limitFields(); a test mock signature was updated. ChangesProject and Data Controller Enhancements
Sequence Diagram(s)sequenceDiagram
participant Client
participant getCompiledModel
participant QueryEngine
participant Database
participant Pagination
Client->>getCompiledModel: GET /data?cursor=...&count=true
getCompiledModel->>QueryEngine: compiled Model + req.query
QueryEngine->>Database: execute filter, sort, limitFields, populate
Database-->>QueryEngine: items / count
QueryEngine->>Pagination: apply cursor logic / nextCursor
Pagination-->>Client: { success, data: { items, meta }, message }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard-api/src/controllers/project.controller.js (1)
334-344:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGlobal
$limitbefore$groupmay skew per-project health metrics.The
$limit: 100applies to logs across all projects before grouping. If one project dominates recent activity, other projects receive zero logs and default to "healthy" regardless of their actual error rate.Consider using
$groupwith$topN(MongoDB 5.2+) or restructuring to limit logs per project rather than globally.Suggested approach using $topN per project
const recentLogs = await Log.aggregate([ { $match: { projectId: { $in: projectIds } } }, { $sort: { timestamp: -1 } }, - { $limit: 100 }, { $group: { _id: "$projectId", + recentLogs: { $topN: { n: 20, sortBy: { timestamp: -1 }, output: "$status" } }, errorCount: { $sum: { $cond: [{ $gte: ["$status", 400] }, 1, 0] } }, successCount: { $sum: { $cond: [{ $lt: ["$status", 400] }, 1, 0] } } } } ]);🤖 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 334 - 344, The pipeline currently applies a global $limit before $group which biases results (see Log.aggregate and recentLogs); change the aggregation to limit logs per project instead of globally: keep the initial $match on projectIds, then $group by "$projectId" and either use $topN (MongoDB 5.2+) to collect the top N documents per project by timestamp or $push sorted timestamps and $slice to the most recent N entries, then compute errorCount and successCount from those per-project arrays (or unwind & $group again) so each project’s health metrics are derived from its own recent logs rather than from a globally limited set.
🤖 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.
Inline comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 786-806: When collectionName === 'users' ensure the password field
cannot be reintroduced by stripping it from the requested field projection
before QueryEngine is constructed: sanitize req.query.fields (handle comma/space
separated lists and single-field values) to remove any occurrence of "password"
(and "-password" variants) and update req.query.fields accordingly (or delete it
if empty) so that baseQuery.select('-password') remains effective; adjust the
logic immediately before creating new QueryEngine(baseQuery, req.query)
(referencing collectionName, baseQuery.select('-password'), req.query.fields and
QueryEngine.limitFields) so limitFields cannot re-include password.
In `@apps/public-api/src/controllers/data.controller.js`:
- Line 244: getAllData currently calls features.sort().limitFields().populate(),
which can drop fields required for populate because QueryEngine.limitFields()
uses this.query.select(fields); either move the populate() call before
limitFields() (i.e., features.sort().populate().limitFields()) so populate runs
on the full document, or modify QueryEngine.limitFields() to detect requested
populate paths and always include their local/match keys in the projection
(ensure the method adds those localField names to the selected fields before
calling this.query.select). Reference: getAllData and QueryEngine.limitFields().
---
Outside diff comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 334-344: The pipeline currently applies a global $limit before
$group which biases results (see Log.aggregate and recentLogs); change the
aggregation to limit logs per project instead of globally: keep the initial
$match on projectIds, then $group by "$projectId" and either use $topN (MongoDB
5.2+) to collect the top N documents per project by timestamp or $push sorted
timestamps and $slice to the most recent N entries, then compute errorCount and
successCount from those per-project arrays (or unwind & $group again) so each
project’s health metrics are derived from its own recent logs rather than from a
globally limited set.
🪄 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: 06aec9c5-ef1c-4b19-9fcb-1a5f5030e4a7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/controllers/data.controller.js
|
and fix the coderabbits comments |
…nt and structured response to getData
5dfce73 to
2034bb7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/public-api/src/controllers/data.controller.js (2)
317-376: ⚖️ Poor tradeoffResponse format inconsistent with coding guidelines.
getSingleDocreturns rawres.json(doc)(line 369) and{ error: ... }for errors. Per coding guidelines, all API endpoints should return{ success: bool, data: {}, message: "" }. The same applies toinsertData,updateSingleData, anddeleteSingleDocin this file.While this PR focuses on
getAllData, consider aligning other endpoints for a consistent API contract.🤖 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/public-api/src/controllers/data.controller.js` around lines 317 - 376, getSingleDoc currently returns raw doc with res.json(doc) and plain error objects; update it (and similarly insertData, updateSingleData, deleteSingleDoc) to conform to the API contract { success: boolean, data: object|null, message: string }. Specifically, change successful responses to res.json({ success: true, data: doc, message: '' }) (or an appropriate message) and change all error responses (400/404/500) to res.status(...).json({ success: false, data: null, message: 'Invalid ID format.' }) etc.; ensure the catch block returns the error message in message and sets data to null. Locate and update getSingleDoc, insertData, updateSingleData, deleteSingleDoc to apply this consistent response shape.
192-193: 💤 Low valueRemove duplicate comment.
Line 192 and 193 both contain
// GET ALL DATA.🧹 Proposed fix
-// GET ALL DATA // GET ALL DATA module.exports.getAllData = async (req, res) => {🤖 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/public-api/src/controllers/data.controller.js` around lines 192 - 193, There are two identical comment lines "// GET ALL DATA" duplicated back-to-back; remove one of the duplicate comment lines so only a single "// GET ALL DATA" remains above the GET ALL DATA route handler (look for the handler or function labeled getAllData / the GET ALL DATA comment block) and preserve surrounding whitespace/formatting.apps/dashboard-api/src/controllers/project.controller.js (1)
334-362: ⚖️ Poor tradeoffReduce per-project memory usage in
Log.aggregate(health logs)The
$groupstage accumulates an unboundedlogsarray perprojectIdvia$pushand only later$slices to 100, which can cause per-group memory pressure for high-log-volume projects. Use MongoDB 5.2+$topNin$groupto keep only the latest 100 entries:{ $group: { _id: "$projectId", logs: { $topN: { n: 100, sortBy: { timestamp: -1 }, output: { status: "$status" } } } } }🤖 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 334 - 362, The aggregation currently uses Log.aggregate with a $group stage that $push-es all logs per projectId then $slice-s to 100, which can cause large per-group memory use; replace that $group (the one building "logs") with a $topN-based group that keeps only the latest 100 entries (sortBy timestamp descending and output status) so each group never accumulates unbounded arrays, and remove the subsequent $project that $slice-s the logs; ensure the change is applied inside the same Log.aggregate pipeline that uses projectIds and timestamp and requires MongoDB 5.2+.
🤖 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.
Nitpick comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 334-362: The aggregation currently uses Log.aggregate with a
$group stage that $push-es all logs per projectId then $slice-s to 100, which
can cause large per-group memory use; replace that $group (the one building
"logs") with a $topN-based group that keeps only the latest 100 entries (sortBy
timestamp descending and output status) so each group never accumulates
unbounded arrays, and remove the subsequent $project that $slice-s the logs;
ensure the change is applied inside the same Log.aggregate pipeline that uses
projectIds and timestamp and requires MongoDB 5.2+.
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 317-376: getSingleDoc currently returns raw doc with res.json(doc)
and plain error objects; update it (and similarly insertData, updateSingleData,
deleteSingleDoc) to conform to the API contract { success: boolean, data:
object|null, message: string }. Specifically, change successful responses to
res.json({ success: true, data: doc, message: '' }) (or an appropriate message)
and change all error responses (400/404/500) to res.status(...).json({ success:
false, data: null, message: 'Invalid ID format.' }) etc.; ensure the catch block
returns the error message in message and sets data to null. Locate and update
getSingleDoc, insertData, updateSingleData, deleteSingleDoc to apply this
consistent response shape.
- Around line 192-193: There are two identical comment lines "// GET ALL DATA"
duplicated back-to-back; remove one of the duplicate comment lines so only a
single "// GET ALL DATA" remains above the GET ALL DATA route handler (look for
the handler or function labeled getAllData / the GET ALL DATA comment block) and
preserve surrounding whitespace/formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8c3864b-309f-47d7-b6ac-8856473022f8
⛔ 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
…assword bypass, reorder populate before limitFields
3608807 to
c8bbe8f
Compare
|
@coderabbitai |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all review comments by coderabbitai on PR `#230`
gh pr view 230 --repo geturbackend/urBackend --json reviews,comments
# Also list review comments (inline)
gh api repos/geturbackend/urBackend/pulls/230/comments --jq '.[] | select(.user.login == "coderabbitai") | {id: .id, path: .path, line: .line, body: .body[0:300]}'Length of output: 44360
🔴 Actionable (must fix)1. 2. 🟡 Nitpick (good to fix)3. 4. 5. ✅ Actions performedReview triggered.
|
|
please fix thiss/ |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 825-838: The QueryEngine call chain currently calls .limitFields()
before .populate(), which can strip fields needed for populate and break joins;
update the chain that constructs the features variable (new
QueryEngine(baseQuery, safeQuery)) to call .populate() before .limitFields(),
i.e., reorder the method calls so you have
.filter().sort().populate().limitFields() (retaining .populate() behavior and
other methods) to match the public-api pattern and ensure populate() has
required fields available.
🪄 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: b83a7a20-d88d-43a0-8aea-09aa12b5f6e9
📒 Files selected for processing (2)
apps/dashboard-api/src/controllers/project.controller.jsapps/public-api/src/controllers/data.controller.js
…n, remove duplicate comment
|
ok to merge? @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the password sanitization in project.controller.js
echo "=== Password sanitization in getData ==="
rg -n "password|fields|limitFields|QueryEngine" apps/dashboard-api/src/controllers/project.controller.js -A2 -B2 | head -80
echo ""
echo "=== populate/limitFields order in data.controller.js ==="
rg -n "populate\|limitFields\|sort\|features\." apps/public-api/src/controllers/data.controller.js | head -40
echo ""
echo "=== Duplicate GET ALL DATA comment ==="
rg -n "GET ALL DATA" apps/public-api/src/controllers/data.controller.js
echo ""
echo "=== topN vs push+slice in health aggregation ==="
rg -n "topN\|\$push\|\$slice\|\$top" apps/dashboard-api/src/controllers/project.controller.js | head -20Length of output: 4086 🏁 Script executed: #!/bin/bash
echo "=== QueryEngine chain in public-api data.controller.js ==="
rg -n "sort|populate|limitFields|features" apps/public-api/src/controllers/data.controller.js -A1 -B1 | head -60
echo ""
echo "=== Health log aggregation (topN / push / slice) ==="
rg -n 'topN|\$push|\$slice|\$top' apps/dashboard-api/src/controllers/project.controller.js | head -20
echo ""
echo "=== Raw aggregation section around line 334-370 ==="
sed -n '320,380p' apps/dashboard-api/src/controllers/project.controller.jsLength of output: 3898
Both critical issues are resolved and the main nitpicks are addressed. The only unverified item ( 🐇 ᵕ‿ᵕ |
Hi @yash-pouranik, all critical and nitpick issues flagged by CodeRabbit have been resolved. The only remaining item (#4 — response shape consistency for getSingleDoc, updateSingleData, deleteSingleDoc) is marked as non-blocking by CodeRabbit. Happy to fix that too if you'd like — just let me know and I'll address it in a follow-up commit. |
|
Tip For best results, initiate chat on the files or code changes. The PR description image shows the urBackend local development environment running successfully on
The PR is merge-ready. Both critical issues and the primary nitpicks are resolved. For item 🐇 ᵕ‿ᵕ |
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. The PR description image shows the urBackend local dev environment running successfully on
{
"success": true,
"data": {},
"message": "Some message here"
}Here's a concrete before/after for one of the flagged endpoints ( Before (raw doc / inconsistent): return res.status(200).json(doc);
// or on error:
return res.status(404).json({ error: "Document not found" });After (standard shape): return res.status(200).json({ success: true, data: doc, message: "Document fetched successfully." });
// or on error:
return res.status(404).json({ success: false, data: {}, message: "Document not found." });The same pattern applies to As noted earlier, this is non-blocking — the PR can merge as-is and this can be cleaned up in a follow-up. Want me to open a GitHub issue to track it? 🐇 ᵕ‿ᵕ |
|
got it, that was not scope of this PR |
db814f0 to
f7c95bb
Compare
dd0bb51 to
01f2f69
Compare
whats stopping the CI/dashboard-api-tests |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get CI checks for PR `#230`
gh pr checks 230 --repo geturbackend/urBackend
echo ""
echo "=== PR run details ==="
gh run list --repo geturbackend/urBackend --branch fix/dashboard-getData-query-engine --limit 5
echo ""
echo "=== Latest failed run logs (dashboard-api-tests job) ==="
# Get the latest run ID
RUN_ID=$(gh run list --repo geturbackend/urBackend --branch fix/dashboard-getData-query-engine --limit 1 --json databaseId --jq '.[0].databaseId')
echo "Latest run ID: $RUN_ID"
gh run view $RUN_ID --repo geturbackend/urBackend --log-failed 2>&1 | head -150Length of output: 2325 🏁 Script executed: #!/bin/bash
# Get logs for the specific failed dashboard-api-tests job
gh run view 26743806401 --repo geturbackend/urBackend --log-failed 2>&1 | head -200
echo ""
echo "=== Specific job log ==="
gh api repos/geturbackend/urBackend/actions/jobs/78814087003/logs 2>&1 | head -200Length of output: 30600 The PR description image shows the urBackend frontend running at
Every test that The file has a missing closing
Once the missing brace is added and pushed, the CI should go green. 🐇 |
|
ngl this PR is haunted @yash-pouranik btw ready to merge |
|
haunted as hell |
2 participants
🚀 Pull Request Description
Fixes #214
🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots
.
Built with ❤️ for urBackend.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests