add thread reply list pagination #343
Conversation
Southclaws
commented
Jan 2, 2025
•
Southclaws
commented
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Warning Rate limit exceeded@Southclaws has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 47 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (29)
📝 WalkthroughWalkthroughThis pull request introduces comprehensive pagination support for threads and replies across the entire application stack. The changes span multiple layers, including backend repositories, services, HTTP transports, and frontend components. The primary focus is on enhancing the thread and reply retrieval mechanism by adding pagination parameters, modifying data structures, and updating API interfaces to support paginated results. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTPTransport
participant ThreadService
participant ThreadRepository
participant Database
Client->>HTTPTransport: Request thread with pagination params
HTTPTransport->>ThreadService: Get thread (with page params)
ThreadService->>ThreadRepository: Retrieve thread
ThreadRepository->>Database: Query thread with pagination
Database-->>ThreadRepository: Return paginated results
ThreadRepository-->>ThreadService: Return thread with paginated replies
ThreadService-->>HTTPTransport: Return thread data
HTTPTransport-->>Client: Respond with paginated thread
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Southclaws
force-pushed
the
thread-reply-pagination
branch
from
7d6a2a6 to
cadb52e
Compare
![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
🧹 Nitpick comments (13)
app/transports/http/openapi/server_gen.go (4)
2030-2038: Ensure consistent usage of pagination fields.
The newPaginatedReplyListlooks good for encapsulating pagination data. However, consider unifying this withPaginatedResultif there's overlap or if you want a single structure to manage pagination across different response types.
3798-3803: Parameter struct addition appears sensible.
ThreadGetParamsclarifies what parametersThreadGetcan accept. If you plan to add more pagination attributes (like limit, offset, etc.), you could extend this struct for further flexibility.
6434-6435: Check for nil parameters before creating the request.
Whenparams == nil, you safely skip pagination, but it might be worth adding a quick defensive check or a comment to clarify that scenario.
11423-11444: Consider specifying default pagination parameters.
This logic appendspageif present. If none is provided, you might default to page 1 and page size N. Adding a clear default can improve user experience.web/src/app/(dashboard)/t/[slug]/page.tsx (1)
1-1: Introduce error checking for query parametersYour current
QuerySchematransformation is straightforward for the page query. Consider verifying that the parsed integer is valid (e.g., not negative or zero). This might prevent potential edge cases when invalid query parameters are passed.Example approach:
const QuerySchema = z.object({ page: z .string() .transform((v) => { const parsed = parseInt(v, 10); + if (isNaN(parsed) || parsed < 1) { + return 1; + } return parsed; }) .optional(), });Also applies to: 12-12, 15-20, 22-22, 26-28, 33-33
app/services/thread/get.go (1)
25-25: Ensure robust handling of pagination defaults
Consider validating or providing default values forpageParamswhen the caller omits certain fields (e.g., page size, page number). This helps avoid edge cases or unexpected behavior if defaults aren't specified in the calling logic.web/src/screens/thread/ThreadScreen/useThreadScreen.ts (1)
42-45: Directly converting page number to string
This is a convenient approach for the SWR/HTTP client. Consider defensive checks if negative or zero values are passed. Also, ensurefallbackDataproperly reflects any default page data wheninitialPageis unset.Also applies to: 47-51
app/resources/post/thread/thread.go (1)
34-34: Switch topagination.Resultfor replies
This is a clean design choice that encapsulates pagination info along with the reply list. Ensure downstream code handles this new type correctly (e.g., accessingReplies.Repliesvs.Replies).web/src/api/openapi-server/threads.ts (1)
117-126: Thread retrieval function passing optional parameters.
This ensures that the providedparamsare used to accurately fetch paginated data. Double-check that default values or undefined parameters do not inadvertently break existing behavior.tests/thread/reaction/reaction_test.go (2)
78-78: Validating Reaction Deletion
Again, passingnilfor pagination inThreadGetWithResponseis consistent with the pattern. Ensure test coverage includes valid queries with non-nil pagination when needed.
99-100: Shifting fromRepliestoReplies.Replies
This implies a new paginated data structure. The indexing logic to retrieve[0]is clear but watch for corner cases when no replies exist.web/src/api/openapi-schema/index.ts (2)
296-296: Add docs or usage comments for PaginatedReplyList.Consider adding inline JSDoc or references to guide new contributors on how to consume this new paginated data shape throughout the UI, especially for newcomers unfamiliar with the new approach.
297-297: Explain PaginatedReplyListAllOf usage.Similar to the above, a short description or comment clarifying what is included in
PaginatedReplyListAllOfmight help maintain clarity in the future, particularly if aggregates or other fields appear here later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/openapi.yamlis excluded by!**/*.yaml
📒 Files selected for processing (29)
app/resources/datagraph/hydrate/hydrator.go(2 hunks)app/resources/post/thread/db.go(5 hunks)app/resources/post/thread/repo.go(2 hunks)app/resources/post/thread/thread.go(2 hunks)app/resources/seed/post.go(5 hunks)app/services/semdex/semdexer/pinecone_semdexer/index.go(1 hunks)app/services/thread/delete.go(2 hunks)app/services/thread/get.go(2 hunks)app/services/thread/service.go(2 hunks)app/services/thread/thread_semdex/indexer.go(1 hunks)app/services/thread/update.go(2 hunks)app/transports/http/bindings/threads.go(1 hunks)app/transports/http/bindings/utils.go(1 hunks)app/transports/http/openapi/server_gen.go(14 hunks)tests/collection/items_test.go(3 hunks)tests/like/like_test.go(2 hunks)tests/thread/reaction/reaction_test.go(5 hunks)tests/thread/thread_test.go(4 hunks)web/src/api/openapi-client/threads.ts(4 hunks)web/src/api/openapi-schema/index.ts(2 hunks)web/src/api/openapi-schema/paginatedReplyList.ts(1 hunks)web/src/api/openapi-schema/paginatedReplyListAllOf.ts(1 hunks)web/src/api/openapi-schema/threadAllOf.ts(1 hunks)web/src/api/openapi-schema/threadGetParams.ts(1 hunks)web/src/api/openapi-server/threads.ts(2 hunks)web/src/app/(dashboard)/t/[slug]/page.tsx(2 hunks)web/src/components/thread/ReplyList/ReplyList.tsx(1 hunks)web/src/screens/thread/ThreadScreen/ThreadScreen.tsx(3 hunks)web/src/screens/thread/ThreadScreen/useThreadScreen.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- web/src/api/openapi-schema/threadGetParams.ts
- web/src/api/openapi-schema/paginatedReplyList.ts
- web/src/api/openapi-schema/paginatedReplyListAllOf.ts
🔇 Additional comments (70)
app/resources/post/thread/db.go (10)
22-22: Add pagination import
Introducing thepaginationpackage import aligns with the new pagination functionality, ensuring a consistent approach across the codebase.
25-25: Include reply handling import
Addingreplyimport is appropriate for transforming database rows into domain-level reply entities. This helps keep the code clean and maintainable.
289-301: ValidaterepliesCountQueryfor performance and correctness
The query correctly counts the total replies and whether the requesting account has replied. However, consider verifying:
- Index coverage on
root_post_idanddeleted_atto ensure efficient lookups on large datasets.- Potential counting edge cases (e.g., repeated or nested replies).
330-336: Initial retrieval of reply stats
The updated function signature includespageParams pagination.Parameters, reflecting the newly added pagination requirement. FetchingreplyStatshere is consistent with your overall approach to gather aggregated metrics before assembling the final response.
338-338: Fetch likes from the database
Reusing the same pattern to retrieve likes maintains a consistent code style. Just ensure the query results align with the new pagination requirements if you plan to present likes in a paginated manner in the future.
408-410: Convert post edges to replies
Usingdt.MapErrto mapr.Edges.Postsinto reply models is a clean and idiomatic approach. Verify correct handling of edge cases, such as when the array is empty or contains invalid data.
413-415: Leverage reply stats map
StoringreplyStatsin a map helps decouple data retrieval from business logic. This step looks good and avoids repeated calculations.
416-417: Create paginated results
pagination.NewPageResultproperly wraps the replies into a paginated structure. Double-check any off-by-one boundary scenarios and confirm correct total count usage.
418-418: Mapper for final post
Wrapping the entity withFromModelwhile also supplyinglikes,collections, andreplyStatsis a clean design. Good job consolidating the logic.
424-425: Assign the paginated reply results
Settingp.Repliesto the paginated result ensures a clear delineation between the main post information and the paginated replies. This aligns with the PR's goal of enhancing pagination capabilities.app/transports/http/openapi/server_gen.go (10)
2785-2788: Verify downstream usage of the new PaginatedReplyList.
ReplacingReplyListwithPaginatedReplyListmight break any existing logic relying onThread.Replies. Please confirm that all references are updated consistently throughout the codebase.
4666-4666: Confirm that all calls to ThreadGet pass the new params argument.
Since the method now accepts*ThreadGetParams, ensure that existing callers are updated, otherwise build or runtime errors might occur.
11398-11398: Method signature update is appropriate.
ChangingNewThreadGetRequestto accept*ThreadGetParamsmatches the revised method signature.
12018-12018: Updated interface method signature aligns with the new parameter struct.
Ensure all interface implementations also accommodate*ThreadGetParams.
15861-15862: Handling the updated ThreadGet call is done properly.
ForwardingparamstoThreadGetmaintains consistency.
19754-19754: Revised ThreadGet handler signature is consistent with the new pagination approach.
This ensures the server can receive pagination parameters correctly.
21859-21867: Validate page parameters for invalid or negative values.
You correctly bind the query parameter toparams.Page, but consider adding checks for out-of-range inputs to avoid potential runtime issues.
26709-26709: Adding the Params field to ThreadGetRequestObject is consistent.
This consolidates pagination parameters with other request details, which should simplify handling in the route.
30116-30120: Correct pass-through of the new params field.
StoringparamsinThreadGetRequestObjectaligns with the updated route signatures.
30230-30605: Auto-generated Swagger specification.
Typically, auto-generated documentation code is not manually reviewed because any changes risk breaking the generation process.web/src/api/openapi-schema/threadAllOf.ts (1)
10-13: Ensure consistency withPaginatedReplyListdefinitionYou've updated the
repliesproperty to usePaginatedReplyList. Verify thatPaginatedReplyListis correctly defined (e.g., it must be imported and declared properly in"./paginatedReplyList") and that other references to replies in the codebase match this structure.app/services/thread/thread_semdex/indexer.go (1)
16-16: Confirm default vs. custom pagination usageCalling
i.threadQuerier.Getwith an emptypagination.Parameters{}suggests a default pagination. Verify whether you need a default page size, offset, or limit.app/services/thread/get.go (2)
12-12: Import looks fine.
No issues with adding the pagination package import. This aligns well with the new pagination approach across the codebase.
29-29: Confirm correct usage ofpageParamsin the repository layer
You're correctly passingpageParamsto the repository. Verify that any existing calls tos.thread_repo.Getoutside this method are updated accordingly and that the repository properly handles invalid pagination parameters.app/services/thread/delete.go (2)
13-13: New pagination import is consistent with the overall design
This import confirms that pagination is being applied consistently across the codebase. No issues here.
32-32: Using empty pagination parameters
Passingpagination.Parameters{}ensures the repository receives a consistent signature. Verify that the repository gracefully handles empty pagination values (e.g., defaulting to the first page).web/src/screens/thread/ThreadScreen/useThreadScreen.ts (2)
12-12: New optionalinitialPageparameter
Great addition to allow initial pagination state. Ensure there's no confusion wheninitialPageis undefined or zero.
24-24: Function signature accommodates optional page
The updated signature aligns well with the newinitialPage. Double-check that callers ofuseThreadScreenprovideinitialPageappropriately if needed.app/resources/post/thread/thread.go (1)
14-14: New pagination import
No issues with pulling in the pagination package. This dependency is consistent with the approach used elsewhere in the code.app/resources/datagraph/hydrate/hydrator.go (2)
17-17: New import for pagination parameters is consistent with the PR objective.
This aligns perfectly with the introduction of pagination logic across the application and is appropriately placed.
78-78: Ensure correct pagination usage.
Providingpagination.Parameters{}with no fields set may default to no pagination or rely on downstream defaults. Verify that this behavior meets the intended pagination requirements.app/services/thread/update.go (2)
16-16: Imported the pagination package.
This import addition validates the shift toward structured pagination parameters.
43-43: Parameterizing the thread retrieval with pagination.
Passingpagination.Parameters{}to theGetmethod ensures consistent usage of pagination throughout the system. Confirm that the default parameters produce the expected behavior (e.g., a full dataset or the first page).app/resources/post/thread/repo.go (2)
14-14: Pagination import is appropriately added.
This import signals a coherent shift to incorporate pagination in the thread repository.
57-57: Updated repository interface to include pagination parameters.
This change modifies theGetmethod signature, which could break compatibility with existing calls. Ensure all call sites are updated accordingly to prevent runtime errors.web/src/api/openapi-server/threads.ts (2)
13-13: Introduction of ThreadGetParams type.
This is a solid approach to strongly type pagination (and other query) parameters.
98-112: Constructing query parameters for thread retrieval.
The new logic appends query parameters if they exist, enabling flexible thread fetching with pagination or filtering. Validate that all optional parameters are properly handled in the calling code.web/src/screens/thread/ThreadScreen/ThreadScreen.tsx (3)
12-12: ImportingPaginationControlsfor reply pagination
This is a solid decision to introduce a dedicated pagination component, clearly separating the pagination-related UI logic into a reusable component.
27-27: Switching toVStack
UsingVStackto stack elements vertically helps with cleaner layout control and is consistent with the system’s styled components.
116-136: Implementing Pagination Controls for Replies
The approach of displaying the pagination controls both above and below the reply list is user-friendly. It ensures quick navigation for threads with numerous replies. The logic conditiondata.thread.replies.total_pages > 1looks straightforward and well-implemented. Be mindful to handle potential off-by-one errors if the backend returns edge cases (e.g., an empty page).app/transports/http/bindings/threads.go (1)
214-216: Passing Pagination Params to Service Layer
IncludingppviadeserialisePageParamsand updatingi.thread_svc.Get(ctx, postID, pp)is a good abstraction that keeps pagination logic centralized. Verify that potential invalid page inputs (e.g., negative values) are handled gracefully indeserialisePageParams.tests/thread/reaction/reaction_test.go (5)
57-57: Usingnilfor Pagination Params
The addition of the nil parameter inThreadGetWithResponsecalls suggests an optional pagination usage. This approach appears consistent if default pagination logic is handled server-side.
96-96: Fetching Thread after Reply Reaction
Ensuring correct retrieval after a reaction or reply is good. No issues seen here.
121-121: Consistent ThreadGet Calls
Using the updated method with a nil parameter is consistent.
124-125: Accessing Nested Replies Array
Same note as above aboutReplies.Replies. Looks fine as long as the test environment ensures at least one reply.
144-144: Verifying React Uniqueness
This test ensures react spam is consolidated properly. The pagination parameter is omitted here, which is presumably correct for the scenario.web/src/api/openapi-client/threads.ts (4)
146-150: EnhancingthreadGetwith optionalThreadGetParams
Extending the method signature to accept pagination or additional query parameters is a good approach for flexible data fetching.
154-155: Generating SWR Key with pagination parameters
Includingparamsin the key ensures cached data is invalidated correctly when pagination changes.
175-175: Hook signature updated to accept pagination
This extension is helpful for hooking into thread data with different page contexts.
188-189: Conditional SWR Key creation
Making the SWR key dependent onthreadMarkandparamsensures correct caching and revalidation.tests/thread/thread_test.go (4)
76-76: Confirmed correct usage of paginated replies field
The transition fromRepliestoReplies.Repliescorrectly matches the new paginated replies structure.
96-96: Check for potential query parameters
InvokingThreadGetWithResponsewithnilmight indicate an optional pagination or other query parameters. Confirm there's no unintended default behavior when passingnilin place of an explicit parameter object.
99-100: Accurate retrieval of reply IDs
Mapping reply IDs confirms the function's correctness in enumerating paginated replies. This approach is consistent with the new structure.
105-105: Consistent handling of session
Providingsession2alongside anilparameter ensures this test case covers the expected authentication differences.tests/like/like_test.go (2)
69-79: Ensuring like status withnilparameter
Usingnilas the third parameter inThreadGetWithResponseconsistently sets up the retrieval context. Verify that this approach does not affect the session logic or produce unexpected results.
174-195: Accurate reply-likes retrieval in paginated structure
The switch toReplies.Replies[0]is valid for the paginated replies structure. The updated assertion logic accurately checks the user-specific like status on replies.app/transports/http/bindings/utils.go (2)
121-121: Switch toserialiseThreadRepliesPaginatedList
Replacing direct reply mapping with a dedicated serialization function improves clarity and modularity for paginated data handling.
129-139: NewserialiseThreadRepliesPaginatedListfunction
This helper efficiently wraps pagination data (CurrentPage,NextPage, etc.) together with the serialized replies. The approach is straightforward and maintains code consistency.tests/collection/items_test.go (3)
169-169: Use ofnilfor additional parameters
Passingnilas the third argument toThreadGetWithResponseindicates an optional or default parameter set. Confirm that this usage aligns with the function’s updated signature.
[approve]
183-183: Session-based differences in collection status
This logic effectively ensures that different sessions (owners vs. random users) see the correct collection statuses.
197-197: Guest session behavior
Anilsession parallels guest user access, confirming that the resulting collection data is displayed without personal session data.app/resources/seed/post.go (5)
17-17: Use consistent alias or naming approach for imports.While this import is correct and descriptive, confirm that consistent naming or aliasing is used across files to ensure consistent readability for future contributors.
69-147: Good integration of pagination.Result.Switching from a basic slice to this paginated structure provides strong alignment with the new pagination features. Ensure that existing logic handling plain slices is updated accordingly (e.g., ensuring no leftover references to the old
Repliesslice).
179-195: Maintain uniform approach for new paginated replies.This block mirrors the approach used in
Post_01_Welcome. Confirm that all references or call sites handle replies fromItemsto avoid breaking changes.
211-243: Ensure large text test coverage.This thread is specifically designed for stress testing with large/hacky strings. Now that it is wrapped in
pagination.Result, verify that boundary pagination scenarios (e.g., extremely large or zero-length data sets) remain well-handled.
288-288: Iterate over replies safely.Looping over
t.Replies.Itemsis correct for the new paginated structure. If additional pagination logic is introduced, confirm you filter or limit records as intended.web/src/api/openapi-schema/index.ts (1)
408-408: Adopt consistent naming for threadGetParams.This export matches the naming convention well. Verify that related types (e.g., “threadGetPayload”, “threadGetRequest”) also follow consistent naming in openapi-schema.
app/services/semdex/semdexer/pinecone_semdexer/index.go (1)
103-106: Short-circuit logic is beneficial but re-check concurrency.Short-circuiting when no vector IDs exist avoids unnecessary processing. However, ensure that any parallel or batched job that expects a result set does not hang waiting for subsequent steps when
nil, nil, nilis returned.app/services/thread/service.go (2)
18-18: Import is aligned with new pagination logic.This addition is consistent with the rest of the codebase’s approach to pagination. Continue verifying that all older references to reply slices point to the new pagination functionality.
59-59: Method signature updated to accommodate pagination.Good job adding the
pageParamsargument. Ensure that all callers supply valid pagination parameters or a default for backward compatibility.
| @@ -21,7 +21,7 @@ export function ReplyList({ thread }: Props) { | |||
| flexDir="column" | |||
| width="full" | |||
| > | |||
| {thread.replies.map((reply, i) => { | |||
| {thread.replies.replies.map((reply, i) => { | |||
There was a problem hiding this comment.
Fix inconsistent indexing in ReplyList
You're mapping over thread.replies.replies but using thread.replies[i - 1] instead of thread.replies.replies[i - 1]. This can lead to errors or undefined references. Consider updating both places to point to the same nested structure:
{thread.replies.replies.map((reply, i) => {
- const previous = thread.replies[i - 1];
+ const previous = thread.replies.replies[i - 1];
...
})}Committable suggestion skipped: line range outside the PR's diff.
Southclaws
force-pushed
the
thread-reply-pagination
branch
from
cadb52e to
4f38d1d
Compare
Southclaws
force-pushed
the
thread-reply-pagination
branch
from
4f38d1d to
2c87659
Compare
