Conversation
AntoLC
force-pushed
the
fix/tree-pagination
branch
from
adc082c to
4cf9ea9
Compare
|
Size Change: -1 B (0%) Total Size: 4.25 MB 📦 View Changed
|
Member
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR fixes tree pagination in the frontend by adding lazy-loading “load more” nodes using an IntersectionObserver, updating the tree pagination payload shape, and improving type-safety for tree node handling via a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts`:
- Around line 29-37: The fixture always returns 20 results and a non-null next
despite count=25, so update the mocked response logic in the test (the response
object and the code that builds results using pageId and Array.from) to compute
the real last page: determine pageSize (20), compute totalPages =
Math.ceil(count / pageSize), and when parseInt(pageId) === totalPages set
results length to count % pageSize (or pageSize if evenly divisible) and set
next to null so the test exercises the hasMore = false branch.
- Around line 117-131: After creating titleChild with addChild, wait for the
parent/child relation to be persisted before adding the grandchild and before
navigating away: either extend addChild to wait for the tree mutation to settle
or add explicit waits after addChild (e.g., waitForSelector/assert that the
parent node contains a child link with text 'doc-tree-pagination-child' and that
the child node contains 'doc-tree-pagination-child-2') so that addChild (used
from utils-sub-pages.ts and utils-common.ts) only proceeds when the DOM/tree
relation is visible and stable before calling page.goto(pageParentUrl).
In
`@src/frontend/apps/impress/src/features/docs/doc-tree/components/DocSubPageItem.tsx`:
- Around line 52-85: DocSubPageLoadMore's IntersectionObserver callback closes
over the stale loading state and can fire duplicate handleLoadChildren calls;
replace the captured boolean with a mutable ref (e.g., isLoadingRef) that you
update whenever setLoading is called, and read isLoadingRef.current inside the
observer callback before calling
treeContext.treeData.handleLoadChildren(props.node.data.parentKey as string);
keep the existing loaderRef and cleanup logic, remove the exhaustive-deps
suppression, and ensure the observer creation effect depends on values that can
change (or use an empty deps array but rely on stable refs for loading and
treeContext/treeData) so the callback always checks the current loading flag.
In `@src/frontend/apps/impress/src/pages/docs/`[id]/index.tsx:
- Around line 59-63: The pagination object returned by onLoadChildren
incorrectly hard-codes pagination.currentPage to 1; change it to use the loaded
page parameter (the page variable passed into onLoadChildren) so currentPage:
page (ensure it's the correct numeric type if page can be a string) when
constructing the response using doc, next, and count so downstream pagination
logic sees the correct page.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d58db3f-28fb-4ee9-97ce-3d56e5fde443
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.tssrc/frontend/apps/impress/src/features/docs/doc-tree/components/DocSubPageItem.tsxsrc/frontend/apps/impress/src/features/docs/doc-tree/components/DocTree.tsxsrc/frontend/apps/impress/src/features/docs/doc-tree/utils.tssrc/frontend/apps/impress/src/pages/docs/[id]/index.tsx
Comment on lines
+117
to
+131
| const titleChild = await addChild({ | ||
| page, | ||
| browserName, | ||
| docParent: title, | ||
| docName: 'doc-tree-pagination-child', | ||
| }); | ||
|
|
||
| await addChild({ | ||
| page, | ||
| browserName, | ||
| docParent: titleChild, | ||
| docName: 'doc-tree-pagination-child-2', | ||
| }); | ||
|
|
||
| await page.goto(pageParentUrl); |
There was a problem hiding this comment.
Wait for the tree relation to settle before continuing.
addChild only waits for the title edit in src/frontend/apps/e2e/__tests__/app-impress/utils-sub-pages.ts:63-100 and src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts:237-248; it does not wait for the parent/child mutation to finish. Line 124 can therefore try to add a grandchild before titleChild is actually attached, and Line 131 can navigate back before that nested link is persisted, which will make this test flaky in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts` around lines
117 - 131, After creating titleChild with addChild, wait for the parent/child
relation to be persisted before adding the grandchild and before navigating
away: either extend addChild to wait for the tree mutation to settle or add
explicit waits after addChild (e.g., waitForSelector/assert that the parent node
contains a child link with text 'doc-tree-pagination-child' and that the child
node contains 'doc-tree-pagination-child-2') so that addChild (used from
utils-sub-pages.ts and utils-common.ts) only proceeds when the DOM/tree relation
is visible and stable before calling page.goto(pageParentUrl).
src/frontend/apps/impress/src/features/docs/doc-tree/components/DocSubPageItem.tsx
Show resolved
Hide resolved
When a sub-sub-document had more than 20 children, the pagination was not working. This commit fixes the issue by ensuring that the pagination logic is correctly applied to all levels of the document tree.
AntoLC
force-pushed
the
fix/tree-pagination
branch
from
4cf9ea9 to
6691167
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/frontend/apps/impress/src/features/docs/doc-tree/components/DocSubPageItem.tsx (1)
62-85: 🧹 Nitpick | 🔵 TrivialConsider adding dependencies to the effect array.
The effect relies on
props.node.data.parentKeyandtreeContext, which could theoretically change. While the current empty dependency array withinFlightReffixes the stale closure issue for the loading flag, the effect won't re-run if the parent key or tree context changes.🔧 Suggested fix
observer.observe(el); return () => observer.disconnect(); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [props.node.data.parentKey, treeContext]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/impress/src/features/docs/doc-tree/components/DocSubPageItem.tsx` around lines 62 - 85, The effect currently uses loaderRef, inFlightRef, props.node.data.parentKey and treeContext but has an empty deps array causing a stale subscription; update the useEffect to include parentKey (derived once as const parentKey = props.node.data.parentKey) and treeContext (or treeContext?.treeData) in its dependency array so the observer re-attaches when these change, while keeping loaderRef and inFlightRef out of deps since they are refs; ensure you recreate the IntersectionObserver inside the effect and disconnect it in the cleanup to avoid leaks, and keep the inFlightRef usage inside the observer callback to preserve the loading flag behavior.src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts (1)
29-37: 🧹 Nitpick | 🔵 TrivialMock never returns a terminal page (
next: null).With
count: 40and 20 results per page, page 2 should be the last page withnext: null. Currently, every page returns anextURL, so the test never exercises thehasMore = falsebranch that removes the load-more indicator.🔧 Suggested fix
const response = { count: 40, - next: `http://localhost:8071/api/v1.0/documents/anything/children/?page=${parseInt(pageId) + 1}`, + next: + parseInt(pageId) < 2 + ? `http://localhost:8071/api/v1.0/documents/anything/children/?page=${parseInt(pageId) + 1}` + : null, previous: parseInt(pageId) > 1 ? `http://localhost:8071/api/v1.0/documents/anything/children/?page=${parseInt(pageId) - 1}` : null, results: Array.from({ length: 20 }, (_, i) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts` around lines 29 - 37, The mock response in doc-tree.spec.ts always sets response.next to a URL so the test never covers the terminal page; update the response generation (the response object that uses count, pageId and results) to set next: null when the current page is the last one (e.g., when parseInt(pageId) * resultsPerPage >= count or when pageId === Math.ceil(count / resultsPerPage)); keep the existing next URL otherwise so page 2 (with count: 40 and 20 results per page) returns next: null and exercises the hasMore=false branch that removes the load-more indicator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts`:
- Around line 144-151: The test should assert the spinner disappears on the
final page: update the mock that supplies pagination so the last page (page 2)
returns next: null (so hasMore becomes false), then add an assertion using the
existing docTree locator for '.c__spinner' to verify it's hidden (e.g., await
expect(docTree.locator('.c__spinner')).toBeHidden()) after navigating to the
terminal page; reference docTree, locator('.c__spinner'), and the pagination
mock that currently returns next for page 2.
---
Duplicate comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts`:
- Around line 29-37: The mock response in doc-tree.spec.ts always sets
response.next to a URL so the test never covers the terminal page; update the
response generation (the response object that uses count, pageId and results) to
set next: null when the current page is the last one (e.g., when
parseInt(pageId) * resultsPerPage >= count or when pageId === Math.ceil(count /
resultsPerPage)); keep the existing next URL otherwise so page 2 (with count: 40
and 20 results per page) returns next: null and exercises the hasMore=false
branch that removes the load-more indicator.
In
`@src/frontend/apps/impress/src/features/docs/doc-tree/components/DocSubPageItem.tsx`:
- Around line 62-85: The effect currently uses loaderRef, inFlightRef,
props.node.data.parentKey and treeContext but has an empty deps array causing a
stale subscription; update the useEffect to include parentKey (derived once as
const parentKey = props.node.data.parentKey) and treeContext (or
treeContext?.treeData) in its dependency array so the observer re-attaches when
these change, while keeping loaderRef and inFlightRef out of deps since they are
refs; ensure you recreate the IntersectionObserver inside the effect and
disconnect it in the cleanup to avoid leaks, and keep the inFlightRef usage
inside the observer callback to preserve the loading flag behavior.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20619124-98f7-48fe-9f00-c188b2cf2969
📒 Files selected for processing (6)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.tssrc/frontend/apps/impress/src/features/docs/doc-tree/components/DocSubPageItem.tsxsrc/frontend/apps/impress/src/features/docs/doc-tree/components/DocTree.tsxsrc/frontend/apps/impress/src/features/docs/doc-tree/utils.tssrc/frontend/apps/impress/src/pages/docs/[id]/index.tsx
Comment on lines
+144
to
+151
| await expect(docTree.getByText('doc-child-1-19')).toBeVisible(); | ||
| await expect(docTree.locator('.c__spinner')).toBeVisible(); | ||
| await docTree.getByText('doc-child-1-19').hover(); | ||
| await expect( | ||
| docTree.getByText('doc-child-2-1', { | ||
| exact: true, | ||
| }), | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding an assertion that the spinner disappears on the final page.
After loading page 2 (the terminal page), the test could verify that the load-more spinner is no longer visible, confirming that the hasMore = false logic works correctly. This would require fixing the mock to return next: null for the last page.
// After fixing the mock to return next: null on page 2:
await expect(docTree.locator('.c__spinner')).toBeHidden();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts` around lines
144 - 151, The test should assert the spinner disappears on the final page:
update the mock that supplies pagination so the last page (page 2) returns next:
null (so hasMore becomes false), then add an assertion using the existing
docTree locator for '.c__spinner' to verify it's hidden (e.g., await
expect(docTree.locator('.c__spinner')).toBeHidden()) after navigating to the
terminal page; reference docTree, locator('.c__spinner'), and the pagination
mock that currently returns next for page 2.
lunika
approved these changes
Mar 30, 2026
lunika
added a commit
that referenced
this pull request
Apr 2, 2026
Added - 🔧(backend) settings CONVERSION_UPLOAD_ENABLED to control usage of docspec - 🥚(frontend) add easter egg on doc emoji creation #2155 Changed - ♿(frontend) use aria-haspopup menu on DropButton triggers #2126 - ♿️(frontend) add contextual browser tab titles for docs routes #2120 - ♿️(frontend) fix empty heading before section titles in HTML export #2125 Fixed - ⚡️(frontend) add jitter to WS reconnection #2162 - 🐛(frontend) fix tree pagination #2145 - 🐛(nginx) add page reconciliation on nginx #2154
Merged
AntoLC
pushed a commit
that referenced
this pull request
Apr 3, 2026
Added - 🔧(backend) settings CONVERSION_UPLOAD_ENABLED to control usage of docspec - 🥚(frontend) add easter egg on doc emoji creation #2155 Changed - ♿(frontend) use aria-haspopup menu on DropButton triggers #2126 - ♿️(frontend) add contextual browser tab titles for docs routes #2120 - ♿️(frontend) fix empty heading before section titles in HTML export #2125 Fixed - ⚡️(frontend) add jitter to WS reconnection #2162 - 🐛(frontend) fix tree pagination #2145 - 🐛(nginx) add page reconciliation on nginx #2154
AntoLC
pushed a commit
that referenced
this pull request
Apr 3, 2026
Added - 🔧(backend) settings CONVERSION_UPLOAD_ENABLED to control usage of docspec - 🥚(frontend) add easter egg on doc emoji creation #2155 Changed - ♿(frontend) use aria-haspopup menu on DropButton triggers #2126 - ♿️(frontend) add contextual browser tab titles for docs routes #2120 - ♿️(frontend) fix empty heading before section titles in HTML export #2125 Fixed - ⚡️(frontend) add jitter to WS reconnection #2162 - 🐛(frontend) fix tree pagination #2145 - 🐛(nginx) add page reconciliation on nginx #2154
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
2 participants
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.
Purpose
When a sub-sub-document had more than 20 children, the pagination was not working.
This commit fixes the issue by ensuring that the pagination logic is correctly applied to all levels of the document tree.
Proposal
Enregistrement.2026-03-27.161207.mp4