Conversation
HarshP4585
added
bug
WalkthroughThis change removes a console.log in user controller, adds projects and frameworks fields to mock risk creation, and updates utilities to replace projectrisks with projects_risks across update, delete, and reporting queries, including joining risks with projects_risks and adjusting foreign key cleanup to target risks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as AutoDriver
participant API as Driver.insertMockData
participant DB as Database
rect rgb(237, 246, 255)
note over API: Create Risk with associations (new fields)
Dev->>API: insertMockData()
API->>DB: INSERT risk { projects: [project.id], frameworks: [2,3] }
DB-->>API: risk created (id)
API-->>Dev: done
end
rect rgb(245, 240, 255)
note over API,DB: Reporting query uses join
participant Report as Reporting.getProjectRisksReportQuery
Report->>DB: SELECT risks JOIN projects_risks ON risk.id = pr.risk_id WHERE pr.project_id = ?
DB-->>Report: rows with project_id, owner fields, ...
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 (2)
Servers/utils/reporting.utils.ts (1)
30-35: Ambiguous WHERE/ORDER BY columns; qualify to avoid runtime SQL errors.Unqualified project_id/created_at/id can be ambiguous after the JOIN and may fail.
Apply this diff:
- FROM "${tenant}".risks risk - JOIN "${tenant}".projects_risks pr ON risk.id = pr.risk_id + FROM "${tenant}".risks risk + JOIN "${tenant}".projects_risks pr ON risk.id = pr.risk_id @@ - WHERE project_id = :project_id - ORDER BY created_at DESC, id ASC + WHERE pr.project_id = :project_id + ORDER BY risk.created_at DESC, risk.id ASCServers/utils/project.utils.ts (1)
302-346: Re-enable project last_updated updates for risk changesAdd "risks" to the byTable union and map it to the projects_risks join table so project.last_updated is updated when risks change — no callers still pass "projectrisks" (verified).
export const updateProjectUpdatedByIdQuery = async ( id: number, // this is not the project id, byTable: | "controls" | "answers" - // | "projectrisks" + | "risks" | "vendors" | "subclauses" | "annexcategories" | "annexcontrols_iso27001" | "subclauses_iso27001", @@ const queryMap = { controls: `SELECT pf.project_id as id FROM "${tenant}".controls_eu c JOIN "${tenant}".projects_frameworks pf ON pf.id = c.projects_frameworks_id WHERE c.id = :id;`, answers: `SELECT pf.project_id as id FROM "${tenant}".assessments a JOIN "${tenant}".answers_eu ans ON ans.assessment_id = a.id JOIN "${tenant}".projects_frameworks pf ON pf.id = a.projects_frameworks_id WHERE ans.id = :id;`, - // projectrisks: `SELECT p.id FROM - // "${tenant}".projects p JOIN "${tenant}".projectrisks pr ON p.id = pr.project_id - // WHERE pr.id = :id;`, + risks: `SELECT pr.project_id as id FROM "${tenant}".projects_risks pr WHERE pr.risk_id = :id;`, vendors: `SELECT project_id as id FROM "${tenant}".vendors_projects WHERE vendor_id = :id;`, subclauses: `SELECT pf.project_id as id FROM "${tenant}".subclauses_iso sc JOIN "${tenant}".projects_frameworks pf ON pf.id = sc.projects_frameworks_id WHERE sc.id = :id;`, annexcategories: `SELECT pf.project_id as id FROM "${tenant}".annexcategories_iso a JOIN "${tenant}".projects_frameworks pf ON pf.id = a.projects_frameworks_id WHERE a.id = :id;`, subclauses_iso27001: `SELECT pf.project_id as id FROM "${tenant}".subclauses_iso27001 sc JOIN "${tenant}".projects_frameworks pf ON pf.id = sc.projects_frameworks_id WHERE sc.id = :id;`, annexcontrols_iso27001: `SELECT pf.project_id as id FROM "${tenant}".annexcontrols_iso27001 a JOIN "${tenant}".projects_frameworks pf ON pf.id = a.projects_frameworks_id WHERE a.id = :id;`, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Servers/controllers/user.ctrl.ts(0 hunks)Servers/infrastructure.layer/driver/autoDriver.driver.ts(1 hunks)Servers/utils/project.utils.ts(5 hunks)Servers/utils/reporting.utils.ts(1 hunks)Servers/utils/user.utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- Servers/controllers/user.ctrl.ts
🧰 Additional context used
🧬 Code graph analysis (1)
Servers/infrastructure.layer/driver/autoDriver.driver.ts (1)
Servers/database/migrations/20250827144546-convert-iso-42001-to-organizational.js (1)
project(23-25)
🔇 Additional comments (4)
Servers/utils/reporting.utils.ts (1)
26-28: Including project_id in the result is a good addition.Selecting pr.project_id AS project_id aligns the report structure with the new projects_risks join.
Servers/utils/project.utils.ts (2)
483-485: Correctly skipping recursion for join tables.Excluding projects_frameworks and projects_risks from recursive child discovery avoids unnecessary traversal. Good.
608-609: Risk counts query aligns with new relation.Joining risks with projects_risks and filtering by pr.project_id is correct.
Servers/utils/user.utils.ts (1)
333-336: Correct target — switching FK cleanup from projectrisks → risks is right.Verified: Servers/database/migrations/20250411224109-allow-null-values-for-project-owner.js drops NOT NULL on projectrisks.risk_owner and risk_approval, Servers/scripts/createNewTenant.ts creates those columns as nullable, and no ALTER TABLE ...risks ...NOT NULL occurrences were found. Ensure those migrations have been applied to all tenants before running the FK nullification.
| projects: [project.id!], | ||
| frameworks: [2, 3], // ISO frameworks | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid hardcoded framework IDs; align risk-framework association with the project.
Using literal [2, 3] may create inconsistent associations (risk linked to a project that doesn’t have those frameworks). Derive frameworks from the project’s current frameworks for correctness.
Apply this diff:
- projects: [project.id!],
- frameworks: [2, 3], // ISO frameworks
+ projects: [project.id!],
+ frameworks: projectFrameworkIds.map((x) => x.framework_id),Add this helper just above the createRiskQuery call:
// Derive frameworks from the created project
const projectFrameworkIds = (await sequelize.query(
`SELECT framework_id FROM "${tenant}".projects_frameworks WHERE project_id = :project_id`,
{
replacements: { project_id: project.id },
transaction,
}
)) as [{ framework_id: number }[], number];If the intent was to attach this risk to ISO frameworks, consider associating it with the organizational project instead (and then derive frameworks from that project). Confirm intended behavior.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Finding autoDriver.driver.ts..."
file=$(rg -n --hidden --glob '!node_modules' -g '**/autoDriver.driver.ts' -l || true)
if [ -z "$file" ]; then
echo "NOT FOUND"
else
echo "FOUND: $file"
echo "---- snippet lines 1-260 ----"
sed -n '1,260p' "$file" || true
fi
echo "---- matches for frameworks array literals ----"
rg -nP -C3 --hidden --glob '!node_modules' "frameworks\s*:\s*\[[^\]]+\]" || true
echo "---- matches for projects_frameworks ----"
rg -nP -C3 --hidden --glob '!node_modules' "projects_frameworks" || true
echo "---- matches for sequelize.query ----"
rg -nP -C3 --hidden --glob '!node_modules' "sequelize\.query\(" || true
echo "---- matches for createRiskQuery ----"
rg -nP -C3 --hidden --glob '!node_modules' "createRiskQuery" || true
echo "---- matches for tenant variable usage ----"
rg -nP -C3 --hidden --glob '!node_modules' "\$\{tenant\}" || true
echo "---- matches for transaction variable usage ----"
rg -nP -C3 --hidden --glob '!node_modules' "\btransaction\b" || trueLength of output: 1591
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate file(s) named autoDriver.driver.ts"
mapfile -t files < <(find . -type f -iname 'autoDriver.driver.ts' -print)
if [ "${#files[@]}" -eq 0 ]; then
echo "NOT FOUND"
else
for f in "${files[@]}"; do
echo "FOUND: $f"
echo "---- snippet lines 1-260 ----"
sed -n '1,260p' "$f" || true
done
fi
echo
echo "2) Search for frameworks array literals (pattern: frameworks: [ ... ])"
rg --no-ignore -nP -C3 "frameworks\s*:\s*\[[^\]]+\]" || true
echo
echo "3) Search for projects_frameworks usages"
rg --no-ignore -nP -C3 "projects_frameworks" || true
echo
echo "4) Search for sequelize.query calls"
rg --no-ignore -nP -C3 "sequelize\.query\(" || true
echo
echo "5) Search for createRiskQuery"
rg --no-ignore -nP -C3 "createRiskQuery" || true
echo
echo "6) Search for \${tenant} template usage"
rg --no-ignore -nP -C3 "\\$\\{tenant\\}" || true
echo
echo "7) Search for transaction variable references"
rg --no-ignore -nP -C3 "\\btransaction\\b" || trueLength of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate autoDriver.driver.ts files"
files=( $(find . -type f -iname 'autoDriver.driver.ts' -print 2>/dev/null) )
if [ "${#files[@]}" -eq 0 ]; then
echo "NOT FOUND"
else
for f in "${files[@]}"; do
echo "FOUND: $f"
echo "---- snippet lines 1-260 ----"
sed -n '1,260p' "$f" || true
echo "---- snippet lines 150-200 ----"
sed -n '150,200p' "$f" || true
done
fi
echo
echo "2) Search for exact frameworks: [2, 3] occurrences"
if command -v rg >/dev/null 2>&1; then
rg -nP -C3 --hidden -g '!node_modules' "frameworks\s*:\s*\[\s*2\s*,\s*3\s*\]" || true
else
grep -RniP --exclude-dir=node_modules "frameworks\s*:\s*\[\s*2\s*,\s*3\s*\]" . || true
fi
echo
echo "3) Search for any frameworks: [ ... ] array literals"
if command -v rg >/dev/null 2>&1; then
rg -nP -C3 --hidden -g '!node_modules' "frameworks\s*:\s*\[[^\]]+\]" || true
else
grep -RniP --exclude-dir=node_modules "frameworks\s*:\s*\[[^\]]+\]" . || true
fi
echo
echo "4) Search for createRiskQuery usage"
if command -v rg >/dev/null 2>&1; then
rg -nP -C3 --hidden -g '!node_modules' "createRiskQuery" || true
else
grep -Rni --exclude-dir=node_modules "createRiskQuery" . || true
fi
echo
echo "5) Search for projects_frameworks table usage"
if command -v rg >/dev/null 2>&1; then
rg -nP -C3 --hidden -g '!node_modules' "projects_frameworks" || true
else
grep -Rni --exclude-dir=node_modules "projects_frameworks" . || true
fi
echo
echo "6) Search for sequelize.query usages"
if command -v rg >/dev/null 2>&1; then
rg -nP -C3 --hidden -g '!node_modules' "sequelize\.query\(" || true
else
grep -Rni --exclude-dir=node_modules "sequelize\.query(" . || true
fi
echo
echo "7) Search for project.id! occurrences"
if command -v rg >/dev/null 2>&1; then
rg -nP -C3 --hidden -g '!node_modules' "project\.id!" || true
else
grep -Rni --exclude-dir=node_modules "project\.id!" . || true
fi
echo
echo "8) Search for tenant template usage (\${tenant})"
if command -v rg >/dev/null 2>&1; then
rg -nP -C3 --hidden -g '!node_modules' "\$\{tenant\}" || true
else
grep -Rni --exclude-dir=node_modules "\${tenant}" . || true
fi
echo
echo "9) Search for transaction variable references"
if command -v rg >/dev/null 2>&1; then
rg -nP -C3 --hidden -g '!node_modules' "\\btransaction\\b" || true
else
grep -Rni --exclude-dir=node_modules "\\btransaction\\b" . || true
fiLength of output: 12852
Avoid hardcoded framework IDs — derive frameworks from the project
Hardcoding frameworks: [2, 3] assigns ISO frameworks to project.id (the created project uses different frameworks), causing inconsistent risk→project→framework associations.
Location: Servers/infrastructure.layer/driver/autoDriver.driver.ts — createRiskQuery call (around lines 171–173).
Apply this diff:
- projects: [project.id!],
- frameworks: [2, 3], // ISO frameworks
+ projects: [project.id!],
+ frameworks: projectFrameworkIds.map((x) => x.framework_id),Add this helper just above the createRiskQuery call:
// Derive frameworks from the created project
const projectFrameworkIds = (await sequelize.query(
`SELECT framework_id FROM "${tenant}".projects_frameworks WHERE project_id = :project_id`,
{
replacements: { project_id: project.id },
transaction,
}
)) as [{ framework_id: number }[], number];If the intent was to attach this risk to ISO frameworks, associate it with the organizational project (projectOrg) instead and derive frameworks from that project.
🤖 Prompt for AI Agents
In Servers/infrastructure.layer/driver/autoDriver.driver.ts around lines
171–173, the code hardcodes frameworks as [2, 3] when calling createRiskQuery
which mismatches the actual frameworks linked to the created project; replace
the hardcoded list by querying projects_frameworks for the created project just
above the createRiskQuery call (use sequelize.query with replacements {
project_id: project.id } and the current transaction) to derive
projectFrameworkIds and pass those IDs to createRiskQuery; if the intended
target is the organizational project (ISO frameworks), perform the same query
against projectOrg instead and use those framework IDs.
| }, | ||
| { files: { foreignKey: "project_id", model: FileModel } }, | ||
| { projectrisks: { foreignKey: "project_id", model: RiskModel } }, | ||
| { projects_risks: { foreignKey: "project_id", model: RiskModel } }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Cascade deletion updated to projects_risks is right.
Deleting join rows by project fixes the prior project delete bug while preserving risks.
Confirm foreign key on projects_risks (project_id) is indexed for delete performance:
🏁 Script executed:
#!/bin/bash
# Grep migrations for projects_risks FK/index on project_id
rg -nP -C2 'CREATE INDEX.*projects_risks.*project_id|projects_risks.*FOREIGN KEY.*project_id'Length of output: 1364
Add an index on projects_risks.project_id
FK exists (projects_risks_project_id_fkey in Servers/database/migrations/20250918172152-migrate-project-risks.js and Servers/scripts/createNewTenant.ts) but no standalone index was found; the PK is (risk_id, project_id) so the btree PK index won’t help lookups by project_id alone — add an index to avoid full-table scans on project deletes.
Suggested change (migration + tenant script):
CREATE INDEX projects_risks_project_id_idx ON "${tenantHash}".projects_risks(project_id);
2 participants
Describe your changes
Write your issue number after "Fixes "
Enter the corresponding issue number after "Fixes #"
Please ensure all items are checked off before requesting a review: