feat(requests): add more sorting options#3184
Open
0xSysR3ll wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds six sort fields ( ChangesExtended Request Sorting
Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestRoute as GET /request
participant parseRequestSort
participant DB as TypeORM Query
participant sortRequestsByTmdbField
participant TheMovieDb
Client->>RequestRoute: GET /request?sort=popularity&sortDirection=asc
RequestRoute->>parseRequestSort: sort="popularity", sortDirection="asc"
parseRequestSort-->>RequestRoute: { field: "popularity", direction: "asc" }
RequestRoute->>DB: query.getMany() (all matching records, no DB order)
DB-->>RequestRoute: MediaRequest[]
RequestRoute->>sortRequestsByTmdbField: requests[], "popularity", "asc"
sortRequestsByTmdbField->>TheMovieDb: fetch popularity per unique tmdbId
TheMovieDb-->>sortRequestsByTmdbField: cached attributes
sortRequestsByTmdbField-->>RequestRoute: sorted MediaRequest[]
RequestRoute->>RequestRoute: slice(skip, skip+pageSize)
RequestRoute-->>Client: { results[], pageInfo }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
0xSysR3ll
force-pushed
the
0xsysr3ll/feat/add-requests-sorting-options
branch
from
3b966f0 to
ef200eb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
server/routes/request.ts (1)
174-183: ⚖️ Poor tradeoffConsider adding pagination limits or response-time monitoring for TMDB sorts.
The TMDB sort path loads all matching requests into memory before sorting and slicing. For large request lists (thousands of records), this could cause:
- Memory pressure from loading all records
- Slow response times from concurrent TMDB API calls (even with deduplication)
- Re-fetching TMDB data on every page navigation since the cache is per-request
This is an acceptable tradeoff given TMDB fields can't be sorted at the DB level, but consider adding observability (e.g., logging when request count exceeds a threshold) to monitor real-world impact.
🤖 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 `@server/routes/request.ts` around lines 174 - 183, Add observability to the TMDB sort path in the isTmdbRequestSort block to monitor real-world performance impact. After loading all records with query.getMany() and before calling sortRequestsByTmdbField, add logging that triggers when the request count exceeds a reasonable threshold (e.g., 1000 records). This will help identify cases where the in-memory sorting of large datasets is causing performance issues without blocking the functionality.server/lib/requestSort.ts (1)
57-64: ⚡ Quick winConsider narrowing the type signature for semantic accuracy.
The function signature accepts all
RequestSortFieldvalues, but according to the route context (server/routes/request.ts:186), this function is only called for non-TMDB fields (addedandmodified). Narrowing the parameter type to'added' | 'modified'would make the contract clearer and catch incorrect usage at compile time.♻️ Suggested type narrowing
-export function getRequestSortColumn(field: RequestSortField): string { +export function getRequestSortColumn(field: 'added' | 'modified'): string { switch (field) { + case 'added': + return 'request.createdAt'; case 'modified': return 'request.updatedAt'; - default: - return 'request.createdAt'; } }🤖 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 `@server/lib/requestSort.ts` around lines 57 - 64, The getRequestSortColumn function currently accepts the broad RequestSortField type parameter, but it is only called with 'added' and 'modified' values according to the actual usage context. Narrow the parameter type from RequestSortField to a union type that only includes 'added' | 'modified' to make the function contract clearer and prevent invalid calls from being compiled. Update the switch statement's default case accordingly since those are now the only expected values.
🤖 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 `@next-env.d.ts`:
- Line 3: The import path in next-env.d.ts uses an incorrect and unstable path
by including `/dev` in the route. Remove `/dev` from the import statement so
that it reads `import "./.next/types/routes.d.ts";` instead of `import
"./.next/dev/types/routes.d.ts";`. This uses the official stable proxy file path
that Next.js maintains for proper compatibility across environments. Note that
since next-env.d.ts is auto-generated, Next.js may revert this change
automatically on the next run.
In `@server/lib/requestSort.ts`:
- Around line 103-180: The sortRequestsByTmdbField function has complex sorting
logic including media deduplication, concurrent TMDB fetching, cache handling,
and field-specific comparisons, but currently lacks unit test coverage. Create
comprehensive unit tests that verify: deduplication by type-tmdbId key (ensuring
TMDB data is fetched only once per unique media), missing cache entry handling
(items without TMDB data sort consistently by ID), each of the four sort fields
(title, popularity, voteAverage, releaseDate) sort correctly in both ascending
and descending directions, tie-breaking logic (equal values ordered
deterministically by request ID), and error resilience (TMDB fetch failures
handled gracefully). Mock the TheMovieDb class and fetchMediaSortCache function
to simulate various success and failure scenarios for testing.
- Around line 145-155: The sortRequestsByTmdbField function in
server/lib/requestSort.ts has complex logic for TMDB data fetching,
deduplication, error handling, and sorting but lacks any unit test coverage in
server/lib/requestSort.test.ts. Add comprehensive test cases covering: the
scenario where items without cached TMDB data are placed at the end,
deduplication by type-tmdbId key, sorting by each field (popularity,
voteAverage, releaseDate, title) in both ascending and descending directions,
tie-breaking behavior using request.id, and error handling when TMDB API calls
fail.
---
Nitpick comments:
In `@server/lib/requestSort.ts`:
- Around line 57-64: The getRequestSortColumn function currently accepts the
broad RequestSortField type parameter, but it is only called with 'added' and
'modified' values according to the actual usage context. Narrow the parameter
type from RequestSortField to a union type that only includes 'added' |
'modified' to make the function contract clearer and prevent invalid calls from
being compiled. Update the switch statement's default case accordingly since
those are now the only expected values.
In `@server/routes/request.ts`:
- Around line 174-183: Add observability to the TMDB sort path in the
isTmdbRequestSort block to monitor real-world performance impact. After loading
all records with query.getMany() and before calling sortRequestsByTmdbField, add
logging that triggers when the request count exceeds a reasonable threshold
(e.g., 1000 records). This will help identify cases where the in-memory sorting
of large datasets is causing performance issues without blocking the
functionality.
🪄 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: CHILL
Plan: Pro
Run ID: 47ecd7a8-02b6-41f9-8342-66a5d61cc793
📒 Files selected for processing (8)
next-env.d.tsseerr-api.ymlserver/lib/requestSort.test.tsserver/lib/requestSort.tsserver/routes/request.test.tsserver/routes/request.tssrc/components/RequestList/index.tsxsrc/i18n/locale/en.json
0xSysR3ll
force-pushed
the
0xsysr3ll/feat/add-requests-sorting-options
branch
from
ef200eb to
7376266
Compare
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.
Description
This PR adds more sorting options to the requests page:
The sorting logic has been added to a dedicated module
server/lib/requestSort.tsinstead of the actualrequest.tsto keep the route handler focused. This also keeps sorting unit tests separate from request route tests.How Has This Been Tested?
Used the new filters.
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
popularity,releaseDate,voteAverage(TMDB rating), andtitle, alongside existingaddedandmodified.sort/sortDirectionto the backend.added/modifiedwith explicit sort directions.