fix: preserve deep taxonomy tree children#86
Open
BhagyaAmarasinghe wants to merge 1 commit into
Open
Conversation
WalkthroughThis PR refactors the taxonomy tree-building algorithm in 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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 `@internal/repository/taxonomy_repository.go`:
- Around line 1056-1062: The code in buildTaxonomyTree silently drops any root
beyond the first by only setting root when root == nil and continuing, which
masks unexpected multi-root rows; instead keep the existing behavior of using
the first root but add a defensive log when another root is encountered: when
copyNode.ParentID == nil and root != nil, emit a warning (e.g.,
logger.Warn/Warnf) indicating multiple roots for the run and include identifying
info (run id / node id) rather than silently continuing; leave the initial set
root = ©Node path unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc5037a7-dfca-44e8-9440-602602a6b1ed
📒 Files selected for processing (2)
internal/repository/taxonomy_repository.gointernal/repository/taxonomy_repository_test.go
Comment on lines
+1056
to
1062
| if copyNode.ParentID == nil { | ||
| if root == nil { | ||
| root = ©Node | ||
| } | ||
|
|
||
| continue | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for database constraints or application logic that enforces single-root invariant.
# Search for unique constraints or CHECK constraints on parent_id in taxonomy_nodes table
rg -n -A5 -B2 'CREATE TABLE.*taxonomy_nodes' --type=sql
# Search for validation logic that prevents multiple roots
rg -n -C3 'ParentID.*nil.*root' --type=go
rg -n -C3 'multiple.*root' --type=go
# Check if StoreResultAndActivate validates single-root
ast-grep --pattern $'func $_StoreResultAndActivate($$$) {
$$$
}'Repository: formbricks/hub
Length of output: 670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) taxonomy_nodes schema (migration) ==="
nl -ba migrations/010_taxonomy_schema.sql | sed -n '90,170p'
echo
echo "=== 2) buildTaxonomyTree implementation ==="
rg -n "func buildTaxonomyTree" internal -S --glob='*.go'
BUILD_FILE="$(rg -n "func buildTaxonomyTree" internal -S --glob='*.go' --files-with-matches | head -n1 || true)"
if [ -n "$BUILD_FILE" ]; then
echo "--- file: $BUILD_FILE ---"
nl -ba "$BUILD_FILE" | sed -n '1000,1120p'
fi
echo
echo "=== 3) buildTaxonomyTree call sites ==="
rg -n "buildTaxonomyTree\\(" internal -S --glob='*.go'
echo
echo "=== 4) StoreResultAndActivate definition ==="
rg -n "StoreResultAndActivate" internal -S --glob='*.go'
STORE_FILE="$(rg -n "StoreResultAndActivate" internal -S --glob='*.go' --files-with-matches | head -n1 || true)"
if [ -n "$STORE_FILE" ]; then
echo "--- file: $STORE_FILE ---"
# best-effort window around the first match
MATCH_LINE="$(rg -n "StoreResultAndActivate" "$STORE_FILE" -S --glob='*.go' | head -n1 | cut -d: -f1)"
START=$((MATCH_LINE-80)); END=$((MATCH_LINE+120))
nl -ba "$STORE_FILE" | sed -n "${START},${END}p"
fi
echo
echo "=== 5) Any single-root / multiple-root / validation logic ==="
rg -n -S "(multiple.*root|single.*root|rootCount|parent_id.*IS NULL|ParentID.*nil|ParentID.*== *nil)" internal --glob='*.go' || trueRepository: formbricks/hub
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
number_lines() { awk '{printf "%6d\t%s\n", NR, $0}'; }
echo "=== 1) taxonomy_nodes schema (migration) ==="
MIG="migrations/010_taxonomy_schema.sql"
if [ -f "$MIG" ]; then
sed -n '90,170p' "$MIG" | number_lines
else
echo "Missing: $MIG"
fi
echo
echo "=== 2) buildTaxonomyTree implementation ==="
BUILD_FILE="$(rg -n "func buildTaxonomyTree" internal -S --glob='*.go' --files-with-matches | head -n1 || true)"
if [ -n "$BUILD_FILE" ]; then
echo "--- file: $BUILD_FILE ---"
# show around where it likely lives; adjust window if needed
MATCH_LINE="$(rg -n "func buildTaxonomyTree" "$BUILD_FILE" -S --glob='*.go' | head -n1 | cut -d: -f1)"
START=$((MATCH_LINE-40)); END=$((MATCH_LINE+160))
sed -n "${START},${END}p" "$BUILD_FILE" | number_lines
else
echo "buildTaxonomyTree not found under internal/"
fi
echo
echo "=== 3) buildTaxonomyTree call sites ==="
rg -n "buildTaxonomyTree\\(" internal -S --glob='*.go' || true
echo
echo "=== 4) StoreResultAndActivate definition ==="
STORE_FILE="$(rg -n "StoreResultAndActivate" internal -S --glob='*.go' --files-with-matches | head -n1 || true)"
if [ -n "$STORE_FILE" ]; then
echo "--- file: $STORE_FILE ---"
MATCH_LINE="$(rg -n "StoreResultAndActivate" "$STORE_FILE" -S --glob='*.go' | head -n1 | cut -d: -f1)"
START=$((MATCH_LINE-80)); END=$((MATCH_LINE+160))
sed -n "${START},${END}p" "$STORE_FILE" | number_lines
else
echo "StoreResultAndActivate not found under internal/"
fi
echo
echo "=== 5) Any single-root / multiple-root / validation logic ==="
rg -n -S "(multiple.*root|single.*root|rootCount|parent_id.*IS NULL|ParentID.*nil|ParentID.*== *nil)" internal --glob='*.go' || trueRepository: formbricks/hub
Length of output: 15809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
number_lines() { awk '{printf "%6d\t%s\n", NR, $0}'; }
echo "=== buildTaxonomyTree call site context (taxonomy_repository.go around line 600-700) ==="
F="internal/repository/taxonomy_repository.go"
if [ -f "$F" ]; then
sed -n '600,720p' "$F" | number_lines
fi
echo
echo "=== buildTaxonomyTree tests (taxonomy_repository_test.go around line 1-140) ==="
T="internal/repository/taxonomy_repository_test.go"
if [ -f "$T" ]; then
sed -n '1,140p' "$T" | number_lines
fiRepository: formbricks/hub
Length of output: 7196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
F="internal/repository/taxonomy_repository.go"
rg -n "func \\(r \\*TaxonomyRepository\\) listVisibleNodes" "$F" --glob='*.go'
LINE="$(rg -n "func \\(r \\*TaxonomyRepository\\) listVisibleNodes" "$F" --glob='*.go' | head -n1 | cut -d: -f1)"
START=$((LINE-40)); END=$((LINE+120))
sed -n "${START},${END}p" "$F" | head -n 200Repository: formbricks/hub
Length of output: 4486
Drop the silent multiple-root data-loss concern—DB enforces single root per run
The migration creates idx_taxonomy_nodes_one_root_per_run as a UNIQUE INDEX on taxonomy_nodes (run_id) with WHERE parent_id IS NULL, so for any valid run there can’t be more than one root node (ParentID == nil). The buildTaxonomyTree logic that only keeps the first root therefore shouldn’t be reachable in normal operation.
Optional: add a defensive sanity check/log if multiple roots ever appear due to legacy/corrupt data.
🤖 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 `@internal/repository/taxonomy_repository.go` around lines 1056 - 1062, The
code in buildTaxonomyTree silently drops any root beyond the first by only
setting root when root == nil and continuing, which masks unexpected multi-root
rows; instead keep the existing behavior of using the first root but add a
defensive log when another root is encountered: when copyNode.ParentID == nil
and root != nil, emit a warning (e.g., logger.Warn/Warnf) indicating multiple
roots for the run and include identifying info (run id / node id) rather than
silently continuing; leave the initial set root = ©Node path unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
The Formbricks taxonomy explorer depends on Hub returning the full tree. The previous reconstruction appended value copies before descendants were attached, so only shallow levels were visible in the UI.
Validation
go test ./internal/repository