fix: subtree expand only first level + depth limit#8525
Conversation
Nriver
commented
Jan 27, 2026
Nriver
commented
- Correctly recurses through multiple levels when expanding (expanded=1)
- Only updates branches that are actually collapsed (isExpanded=0) when expanding
- Only updates branches that are actually expanded (isExpanded=1) when collapsing
- Adds depth limit of 4 levels when expanding to prevent performance issues in deep trees
- Collapsing remains unlimited (full subtree)
dosubot
bot
added
the
size:M
Summary of ChangesHello @Nriver, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the subtree expansion and collapse functionality within the API. The changes focus on improving the correctness and performance of these operations by introducing a depth limit for expansion, ensuring that only branches requiring a state change are updated, and refining the recursive SQL query for more precise traversal. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the subtree expansion logic to correctly handle recursion, apply depth limits, and only update branches that need to change state. The changes are a significant improvement over the previous implementation. My review focuses on a potential logic bug in the recursive SQL query that could lead to incomplete updates, some code that could be simplified for better readability, and an inconsistency in the function's return types.
| const branchIds = sql.getColumn<string>( | ||
| ` | ||
| WITH RECURSIVE | ||
| tree(branchId, noteId) AS ( | ||
| SELECT branchId, noteId FROM branches WHERE branchId = ? | ||
| UNION | ||
| SELECT branches.branchId, branches.noteId FROM branches | ||
| JOIN tree ON branches.parentNoteId = tree.noteId | ||
| WHERE branches.isDeleted = 0 | ||
| AND branches.isExpanded = 1 | ||
| tree(branchId, noteId, depth) AS ( | ||
| -- Anchor: starting node (depth 0) | ||
| SELECT branchId, noteId, 0 | ||
| FROM branches | ||
| WHERE branchId = ? | ||
|
|
||
| UNION ALL | ||
|
|
||
| -- Recursive part | ||
| SELECT | ||
| b.branchId, | ||
| b.noteId, | ||
| t.depth + 1 | ||
| FROM branches b | ||
| JOIN tree t ON b.parentNoteId = t.noteId | ||
| WHERE b.isDeleted = 0 | ||
| AND b.isExpanded = ? | ||
| AND t.depth < ? | ||
| ) | ||
| SELECT branchId FROM tree`, | ||
| [branchId] | ||
| SELECT branchId | ||
| FROM tree | ||
| WHERE depth <= ? | ||
| AND branchId != 'none_root' | ||
| `, | ||
| [branchId, currentExpandedWeWantToChange, maxDepth, maxDepth] | ||
| ); |
There was a problem hiding this comment.
The current recursive query logic seems incorrect for finding all branches to update. By filtering on b.isExpanded within the recursive step, you stop traversing parts of the tree. For example, when expanding, if you encounter an already expanded branch, you won't look for its collapsed children. To correctly update all nodes in the subtree that are in the opposite state, you should traverse the whole subtree first (up to maxDepth) and then filter the results. This ensures all branches that need to change state are identified, regardless of their parents' state.
const branchIds = sql.getColumn<string>(
`
WITH RECURSIVE
tree(branchId, noteId, depth) AS (
-- Anchor: starting node (depth 0)
SELECT branchId, noteId, 0
FROM branches
WHERE branchId = ?
UNION ALL
-- Recursive part
SELECT
b.branchId,
b.noteId,
t.depth + 1
FROM branches b
JOIN tree t ON b.parentNoteId = t.noteId
WHERE b.isDeleted = 0
AND t.depth < ?
)
SELECT t.branchId
FROM tree t JOIN branches b ON t.branchId = b.branchId
WHERE t.depth <= ?
AND t.branchId != 'none_root'
AND b.isExpanded = ?
`,
[branchId, maxDepth, maxDepth, currentExpandedWeWantToChange]
);| if (![0, 1].includes(expanded)) { | ||
| // You can change to throw new Error() if preferred | ||
| return { branchIds: [], updatedCount: 0, error: "expanded must be 0 or 1" }; | ||
| } |
There was a problem hiding this comment.
This function now has multiple return paths with inconsistent object structures. This early return provides an object with an error property, while another early return provides maxDepthApplied, and the main success path (outside this diff) returns a different structure. This makes the function's return value unpredictable for consumers. It would be best to standardize the return object across all paths, for example by having a common shape like { branchIds, updatedCount, error, ... }.
| const filteredBranchIds = branchIds.filter((id) => id !== "none_root"); | ||
|
|
||
| if (filteredBranchIds.length === 0) { | ||
| return { | ||
| branchIds: [], | ||
| updatedCount: 0, | ||
| maxDepthApplied: maxDepth | ||
| }; | ||
| } | ||
|
|
||
| sql.executeMany(/*sql*/`UPDATE branches SET isExpanded = ${expanded} WHERE branchId IN (???)`, branchIds); |
There was a problem hiding this comment.
The filteredBranchIds variable seems redundant since the SQL query is already expected to filter out 'none_root'. Using this variable for the length check but then using the original branchIds for the sql.executeMany call is confusing and could be a source of bugs if the filter wasn't redundant. For clarity and safety, it would be better to remove the redundant client-side filter and consistently use branchIds.
| const filteredBranchIds = branchIds.filter((id) => id !== "none_root"); | |
| if (filteredBranchIds.length === 0) { | |
| return { | |
| branchIds: [], | |
| updatedCount: 0, | |
| maxDepthApplied: maxDepth | |
| }; | |
| } | |
| sql.executeMany(/*sql*/`UPDATE branches SET isExpanded = ${expanded} WHERE branchId IN (???)`, branchIds); | |
| if (branchIds.length === 0) { | |
| return { | |
| branchIds: [], | |
| updatedCount: 0, | |
| maxDepthApplied: maxDepth | |
| }; | |
| } | |
| sql.executeMany(/*sql*/`UPDATE branches SET isExpanded = ${expanded} WHERE branchId IN (???)`, branchIds); |
1 participant
