perf(issue): remove eager loading of Issue.comments#2810
perf(issue): remove eager loading of Issue.comments#2810dougrathbone wants to merge 6 commits intoseerr-team:developfrom
Conversation
Issue.comments had eager: true, causing every issue query -- including
paginated list endpoints -- to load all comment rows even when only
counts or metadata were needed.
Changes:
- Remove eager: true from the comments OneToMany in Issue entity
- Add explicit relations: { comments: true } in POST /:issueId/comment,
which spreads issue.comments when appending a new comment
- Replace issue.comments.length > 1 in DELETE /:issueId with a
lightweight IssueComment.count() query so comments are never loaded
just to check whether replies exist
- Add integration tests covering comment creation and delete permission
guard for both the single-comment and multi-comment cases
|
Small fix, low impact -- I'm using these PRs to get familiar with the codebase. Happy to adjust anything. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves eager loading from the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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: 3
🧹 Nitpick comments (1)
server/routes/issue.test.ts (1)
160-160: Avoid magic number for permissions in test setupUse
Permission.CREATE_ISSUESinstead of4194304so intent stays explicit and resilient to enum changes.Small cleanup
+ import { Permission } from '@server/lib/permissions'; ... - creator.permissions = 4194304; // Permission.CREATE_ISSUES + creator.permissions = Permission.CREATE_ISSUES;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/issue.test.ts` at line 160, Replace the magic numeric permission value assigned to creator.permissions with the enum constant to make the test intent explicit: change the literal 4194304 to Permission.CREATE_ISSUES (ensure the Permission enum is imported/available in the test) so the setup uses Permission.CREATE_ISSUES rather than a hard-coded number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/issue.test.ts`:
- Around line 107-116: The test name says "returns 404 for a non-existent issue"
but the assertion checks for 500; update the test to expect 404 instead of 500.
Locate the test in server/routes/issue.test.ts (the it(...) block that posts to
'/issue/99999/comment') and change the status assertion on res.status from 500
to 404 so the description and expectation match.
In `@server/routes/issue.ts`:
- Around line 278-279: The subscriber crashes because IssueCommentSubscriber's
findOneOrFail call (in IssueCommentSubscriber) only loads the nested issue but
not its comments, so subsequent access to issue.comments (and the
sortBy(issue.comments, 'id') call) can be undefined; update the findOneOrFail
invocation in IssueCommentSubscriber (the call that currently uses relations: {
issue: true }) to include relations: { issue: { comments: true } } so comments
are loaded, and likewise ensure IssueSubscriber receives an Issue entity with
comments preloaded (adjust its findOne/findOneOrFail relations to include
comments) so both subscribers can safely access issue.comments.
- Around line 387-394: The authorization check has a TOCTOU race: you call
issueCommentRepository.count (commentCount) before performing the delete,
allowing a new reply to be inserted between the check and deletion; fix it by
performing the count-and-delete atomically inside a DB transaction with a lock
so the decision cannot change mid-way. Specifically, wrap the logic that reads
issueCommentRepository.count and the subsequent delete (the code that calls the
repository delete/remove for the issue) in a single transaction (use a
QueryRunner / entityManager or repository.transaction), re-check comment count
inside that transaction with a SELECT ... FOR UPDATE / pessimistic lock on the
issue row (or lock via the issue repository), and only proceed to delete if
req.user?.hasPermission(Permission.MANAGE_ISSUES) OR (issue.createdBy.id ===
req.user?.id AND commentCount <= 1); otherwise rollback and return the
authorization error.
---
Nitpick comments:
In `@server/routes/issue.test.ts`:
- Line 160: Replace the magic numeric permission value assigned to
creator.permissions with the enum constant to make the test intent explicit:
change the literal 4194304 to Permission.CREATE_ISSUES (ensure the Permission
enum is imported/available in the test) so the setup uses
Permission.CREATE_ISSUES rather than a hard-coded number.
🪄 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: c0030d2c-8fd6-4f00-a274-dab8fbe721f7
📒 Files selected for processing (3)
server/entity/Issue.tsserver/routes/issue.test.tsserver/routes/issue.ts
💤 Files with no reviewable changes (1)
- server/entity/Issue.ts
| relations: { comments: true }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find subscriber usages of comments collections:"
rg -n -C3 "entity\\.comments|issue\\.comments" server/subscriber
echo
echo "2) Inspect relation loading in IssueCommentSubscriber:"
rg -n -C4 "findOneOrFail\\(|relations\\s*:" server/subscriber/IssueCommentSubscriber.ts
echo
echo "3) Confirm Issue.comments is no longer eager:"
rg -n -C4 "@OneToMany\\(\\(\\) => IssueComment|public comments" server/entity/Issue.tsRepository: seerr-team/seerr
Length of output: 2247
Subscribers will crash when accessing unloaded comments relation
The verification confirms the regression: IssueCommentSubscriber loads the issue via findOneOrFail({ relations: { issue: true } }) at line 28, but does not include comments in the nested relations. When the subscriber then accesses issue.comments at line 58 (and IssueSubscriber does the same at line 44), it will be undefined, causing sortBy(issue.comments, 'id') to fail.
The route-level fix at line 278 only applies to this handler—it does not retroactively load comments for the subscribers that are triggered later. Add relations: { issue: { comments: true } } to the IssueCommentSubscriber.ts findOneOrFail call (line 28-30), and similarly ensure IssueSubscriber.ts receives an issue with the comments relation loaded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/issue.ts` around lines 278 - 279, The subscriber crashes
because IssueCommentSubscriber's findOneOrFail call (in IssueCommentSubscriber)
only loads the nested issue but not its comments, so subsequent access to
issue.comments (and the sortBy(issue.comments, 'id') call) can be undefined;
update the findOneOrFail invocation in IssueCommentSubscriber (the call that
currently uses relations: { issue: true }) to include relations: { issue: {
comments: true } } so comments are loaded, and likewise ensure IssueSubscriber
receives an Issue entity with comments preloaded (adjust its
findOne/findOneOrFail relations to include comments) so both subscribers can
safely access issue.comments.
There was a problem hiding this comment.
Not an issue in IssueCommentSubscriber.ts, since the entity is fetched again with the right relations and the entity data from the subscriber is not used.
This is right for IssueSubscriber.ts at L44 though, because here we use the entity data from the subscriber.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== IssueSubscriber.ts full content ==="
cat server/subscriber/IssueSubscriber.ts
echo ""
echo "=== IssueCommentSubscriber.ts full content ==="
cat server/subscriber/IssueCommentSubscriber.ts
Length of output: 7692
@gauthier-th, thank you for the clarification — you're correct on both points.
Looking at the full IssueCommentSubscriber.ts, the subscriber re-fetches the entity fresh with relations: { issue: { comments: true } } and never uses the event-provided entity's relation data, so issue.comments is always properly loaded there. My original flag on that file was wrong.
For IssueSubscriber.ts at L44, sortBy(entity.comments, 'id') does consume event.entity.comments directly (the entity passed to issueRepository.save()). However, looking at the current state of server/routes/issue.ts, the resolve/reopen handler (POST /:issueId/:status) already loads the issue with relations: { comments: true } before saving, so event.entity.comments will be populated when beforeUpdate fires. That concern is already addressed within this PR.
✏️ Learnings added
Learnt from: gauthier-th
Repo: seerr-team/seerr PR: 2810
File: server/routes/issue.ts:278-279
Timestamp: 2026-04-12T14:02:27.104Z
Learning: In seerr-team/seerr, `IssueCommentSubscriber.ts` (server/subscriber/IssueCommentSubscriber.ts) re-fetches the IssueComment entity inside `sendIssueCommentNotification` using `getRepository(IssueComment).findOneOrFail({ where: { id: entity.id }, relations: { issue: { comments: true } } })` and does NOT rely on the subscriber event entity's relation data. Do not flag missing relation loading on the event entity in this subscriber.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: fallenbagel
Repo: seerr-team/seerr PR: 2810
File: server/routes/issue.ts:388-395
Timestamp: 2026-04-07T08:22:22.643Z
Learning: Repo seerr-team/seerr — Maintainers consider TOCTOU race conditions on issue delete (count-then-delete without a transaction/lock in server/routes/issue.ts) as overkill and irrelevant for this media request app. Do not suggest wrapping comment-count checks and deletes in a DB transaction or pessimistic locking for this code path.
Learnt from: michaelhthomas
Repo: seerr-team/seerr PR: 2574
File: server/routes/user/index.ts:879-881
Timestamp: 2026-03-12T13:12:51.395Z
Learning: In server/routes/**/*.ts, rely on upstream OpenAPI validator middleware for query parameters (e.g., page). Route handlers should not defensively validate or clamp query parameter values, as this is intentional and aligns with the project convention. Maintain consistency by letting the OpenAPI layer enforce validity rather than duplicating checks in each handler.
Removing eager: true from Issue.comments broke two subscribers that call
sortBy(issue.comments) to find the first comment for notifications:
- IssueCommentSubscriber fetched IssueComment with relations: { issue: true }
but did not load issue.comments, causing sortBy to receive undefined
- IssueSubscriber.beforeUpdate fires on resolve/reopen; the entity came from
findOneOrFail without comments loaded, same result
Fixes:
- IssueCommentSubscriber: change to relations: { issue: { comments: true } }
- POST /:issueId/:status route: add relations: { comments: true } to the
findOneOrFail so IssueSubscriber receives an issue with comments populated
Also address CodeRabbit nitpicks in the test file:
- Replace magic number 4194304 with Permission.CREATE_ISSUES
- Fix test description to match the actual 500 status the handler returns
|
Addressing the coderabbit feedback. Fixed in the latest commit:
On the TOCTOU point: the race window between the count query and the delete existed before this PR too (previously between |
fallenbagel
left a comment
fallenbagel
left a comment
There was a problem hiding this comment.
The removal of eager: true on Issue.comments breaks the issue list UI.
undefined and while the optional chaining prevents a crash, descriptions will silently disappear from the issue list. This is a silent UI regression.
|
Thanks for the review @fallenbagel ! I think the issue list endpoint is actually safe here - the QueryBuilder at server/routes/issue.ts:57 already explicitly loads comments:
The two call sites that did rely on eager loading (the comment POST handler and the delete authorization check) were the ones I updated in this PR. The subscribers Happy to add a test that asserts the list endpoint returns comments if that would help, or if I'm missing another code path that depends on eager loading let me |
Adds a GET /issue test asserting that comments are included in the list response, confirming the explicit leftJoinAndSelect in the QueryBuilder is sufficient after removing eager: true.
|
To help me validate my own undersatnding of the code path, i've added a test @fallenbagel - let me know if this addresses your concenr. |
Thanks for clarifying, I missed the explicit I'll re-review later |
There was a problem hiding this comment.
Pull request overview
This PR removes eager-loading of Issue.comments to reduce unnecessary data loading, and updates routes/subscribers/tests to explicitly load or count comments only where required.
Changes:
- Removed
eager: truefrom theIssue.commentsrelation. - Updated issue comment creation / status update handlers to explicitly load
commentswhen needed. - Updated issue deletion logic to use a
count()query instead of loading all comments, and added integration tests covering issue routes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| server/entity/Issue.ts | Removes eager-loading from the comments relation. |
| server/routes/issue.ts | Adds explicit comments relation loading in two handlers; uses IssueComment.count() for delete guard. |
| server/subscriber/IssueCommentSubscriber.ts | Explicitly loads issue.comments for notification logic after removing eager-loading. |
| server/routes/issue.test.ts | Adds integration tests for listing issues, commenting, and delete permission/reply scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @OneToMany(() => IssueComment, (comment) => comment.issue, { | ||
| cascade: true, | ||
| eager: true, | ||
| }) | ||
| public comments: IssueComment[]; |
There was a problem hiding this comment.
The PR description says only two call sites relied on Issue.comments being eager and that the paginated issue list endpoint was loading comments due to eager loading. In the current code, GET /issue explicitly leftJoinAndSelects issue.comments, and subscribers/status update paths also rely on comments being present. Please update the PR description to reflect the actual call sites/impact (or adjust the code if the intent was to stop loading comments on the list endpoint).
| const issue = ( | ||
| await getRepository(IssueComment).findOneOrFail({ | ||
| where: { id: entity.id }, | ||
| relations: { issue: true }, | ||
| relations: { issue: { comments: true } }, | ||
| }) |
There was a problem hiding this comment.
This loads the full issue.comments collection (and because IssueComment.user is eager, it will also load users for every comment) just to derive the first comment. Consider replacing this with a lightweight query that fetches only the earliest comment’s id/message (or a MIN(id) + fetch) for the issue, to avoid scaling notification cost with comment thread size.
There was a problem hiding this comment.
Same, I don't think it's a necessary optimization.
| try { | ||
| const issue = await issueRepository.findOneOrFail({ | ||
| where: { id: Number(req.params.issueId) }, | ||
| relations: { comments: true }, | ||
| }); |
There was a problem hiding this comment.
relations: { comments: true } is only needed here to keep IssueSubscriber notifications working (it assumes entity.comments is populated). This creates hidden coupling and loads all comments on every status change. Consider updating IssueSubscriber to query the first comment directly by issueId instead of requiring callers to always load comments.
There was a problem hiding this comment.
Yes, this is related to my previous comment.
| try { | ||
| const issueCommentRepository = getRepository(IssueComment); | ||
|
|
||
| const issue = await issueRepository.findOneOrFail({ | ||
| where: { id: Number(req.params.issueId) }, | ||
| relations: { createdBy: true }, | ||
| }); | ||
|
|
||
| const commentCount = await issueCommentRepository.count({ | ||
| where: { issue: { id: issue.id } }, | ||
| }); |
There was a problem hiding this comment.
commentCount is computed unconditionally, even for admins/users with MANAGE_ISSUES where the result is never used. To keep the delete path as lightweight as possible, only run the count() query when the user does not have MANAGE_ISSUES (or when issue.createdBy.id === req.user.id needs to be validated).
There was a problem hiding this comment.
I don't think it's a necessary optimization.
| const issue = await seedIssue(['only comment']); | ||
| const agent = await adminAgent(); | ||
|
|
||
| const res = await agent.delete(`/issue/${issue.id}`); | ||
|
|
There was a problem hiding this comment.
This test claims to cover the “creator with only one comment” case, but it logs in as the seeded admin user (Permission.ADMIN), which bypasses the non-admin creator guard entirely. Add a test where a non-admin creator (e.g. friend@seerr.dev with CREATE_ISSUES) creates an issue with a single comment and can delete it, so the commentCount > 1 logic is exercised for the intended permission path.
| const issue = await seedIssue(['only comment']); | |
| const agent = await adminAgent(); | |
| const res = await agent.delete(`/issue/${issue.id}`); | |
| const userRepo = getRepository(User); | |
| const mediaRepo = getRepository(Media); | |
| const issueRepo = getRepository(Issue); | |
| const commentRepo = getRepository(IssueComment); | |
| const creator = await userRepo.findOneOrFail({ | |
| where: { email: 'friend@seerr.dev' }, | |
| }); | |
| const media = mediaRepo.create({ tmdbId: 2, mediaType: MediaType.MOVIE }); | |
| await mediaRepo.save(media); | |
| const issue = issueRepo.create({ | |
| issueType: 1, | |
| createdBy: creator, | |
| media, | |
| }); | |
| await issueRepo.save(issue); | |
| await commentRepo.save( | |
| commentRepo.create({ message: 'only comment', user: creator, issue }) | |
| ); | |
| creator.permissions = Permission.CREATE_ISSUES; | |
| await userRepo.save(creator); | |
| const settings = getSettings(); | |
| settings.main.localLogin = true; | |
| const friendAgent = request.agent(app); | |
| await friendAgent | |
| .post('/auth/local') | |
| .send({ email: 'friend@seerr.dev', password: 'test1234' }); | |
| const res = await friendAgent.delete(`/issue/${issue.id}`); |
| await agent | ||
| .post('/auth/local') | ||
| .send({ email: 'admin@seerr.dev', password: 'test1234' }); |
There was a problem hiding this comment.
The login helper doesn’t assert that /auth/local succeeded. If login fails for any reason, subsequent requests can produce misleading failures. Consider asserting the login response status (like auth.test.ts does) in adminAgent() and the friend login in the delete test.
| await agent | |
| .post('/auth/local') | |
| .send({ email: 'admin@seerr.dev', password: 'test1234' }); | |
| const response = await agent | |
| .post('/auth/local') | |
| .send({ email: 'admin@seerr.dev', password: 'test1234' }); | |
| assert.equal(response.status, 200); |
The previous test used the admin user which bypasses the comment count guard entirely. Use a non-admin creator with CREATE_ISSUES permission so the commentCount logic is actually exercised.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/issue.test.ts`:
- Around line 166-167: The test mutates settings.main.localLogin directly which
can pollute other tests; capture the prior value via const prev =
getSettings().main.localLogin, set settings.main.localLogin = true before
performing the login, and then restore it in a finally block (mirroring the
adminAgent() pattern) so that getSettings() and settings.main.localLogin are
reset regardless of test outcome; update the test around the
getSettings()/settings.main.localLogin mutation to wrap the login logic in try {
... } finally { getSettings().main.localLogin = prev; }.
- Around line 58-67: The adminAgent helper mutates global
settings.main.localLogin and doesn't restore it; update the adminAgent function
to capture the previous value from getSettings().main.localLogin into a local
variable, set localLogin = true before performing the request, then use a
try/finally so that in the finally block you restore settings.main.localLogin to
the captured previous value; keep the request/response/assert logic
(request.agent(app) and POST to '/auth/local') inside the try so the agent is
still returned after the operation.
- Around line 179-223: The test mutates the global setting via
getSettings().main.localLogin without restoring it; capture the original value
(e.g., const originalLocalLogin = settings.main.localLogin), set
settings.main.localLogin = true for the test, and then restore
settings.main.localLogin = originalLocalLogin (in a finally block or after
assertions) so other tests (and adminAgent() usage) are not affected; reference
getSettings() and settings.main.localLogin to locate where to save and restore
the original value.
🪄 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: 0f5857a1-31c2-4869-98c1-c52704c451fd
📒 Files selected for processing (1)
server/routes/issue.test.ts
| async function adminAgent() { | ||
| const settings = getSettings(); | ||
| settings.main.localLogin = true; | ||
| const agent = request.agent(app); | ||
| const res = await agent | ||
| .post('/auth/local') | ||
| .send({ email: 'admin@seerr.dev', password: 'test1234' }); | ||
| assert.strictEqual(res.status, 200); | ||
| return agent; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restore localLogin setting to prevent test pollution.
The helper mutates settings.main.localLogin globally without restoring it. This can leak across tests and affect isolation. Based on learnings, auth helpers should capture the prior value and restore it in a finally block.
♻️ Proposed fix to scope localLogin mutation
async function adminAgent() {
const settings = getSettings();
+ const priorLocalLogin = settings.main.localLogin;
- settings.main.localLogin = true;
- const agent = request.agent(app);
- const res = await agent
- .post('/auth/local')
- .send({ email: 'admin@seerr.dev', password: 'test1234' });
- assert.strictEqual(res.status, 200);
- return agent;
+ try {
+ settings.main.localLogin = true;
+ const agent = request.agent(app);
+ const res = await agent
+ .post('/auth/local')
+ .send({ email: 'admin@seerr.dev', password: 'test1234' });
+ assert.strictEqual(res.status, 200);
+ return agent;
+ } finally {
+ settings.main.localLogin = priorLocalLogin;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function adminAgent() { | |
| const settings = getSettings(); | |
| settings.main.localLogin = true; | |
| const agent = request.agent(app); | |
| const res = await agent | |
| .post('/auth/local') | |
| .send({ email: 'admin@seerr.dev', password: 'test1234' }); | |
| assert.strictEqual(res.status, 200); | |
| return agent; | |
| } | |
| async function adminAgent() { | |
| const settings = getSettings(); | |
| const priorLocalLogin = settings.main.localLogin; | |
| try { | |
| settings.main.localLogin = true; | |
| const agent = request.agent(app); | |
| const res = await agent | |
| .post('/auth/local') | |
| .send({ email: 'admin@seerr.dev', password: 'test1234' }); | |
| assert.strictEqual(res.status, 200); | |
| return agent; | |
| } finally { | |
| settings.main.localLogin = priorLocalLogin; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/issue.test.ts` around lines 58 - 67, The adminAgent helper
mutates global settings.main.localLogin and doesn't restore it; update the
adminAgent function to capture the previous value from
getSettings().main.localLogin into a local variable, set localLogin = true
before performing the request, then use a try/finally so that in the finally
block you restore settings.main.localLogin to the captured previous value; keep
the request/response/assert logic (request.agent(app) and POST to '/auth/local')
inside the try so the agent is still returned after the operation.
| const settings = getSettings(); | ||
| settings.main.localLogin = true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restore localLogin setting to prevent test pollution.
Like the adminAgent() helper, this inline mutation should capture and restore the prior value in a finally block. Based on learnings, the established pattern is to wrap the login in try/finally.
♻️ Proposed fix
const settings = getSettings();
+ const priorLocalLogin = settings.main.localLogin;
- settings.main.localLogin = true;
- const friendAgent = request.agent(app);
- const loginRes = await friendAgent
- .post('/auth/local')
- .send({ email: 'friend@seerr.dev', password: 'test1234' });
- assert.strictEqual(loginRes.status, 200);
-
- const res = await friendAgent.delete(`/issue/${issue.id}`);
-
- assert.strictEqual(res.status, 204);
+ try {
+ settings.main.localLogin = true;
+ const friendAgent = request.agent(app);
+ const loginRes = await friendAgent
+ .post('/auth/local')
+ .send({ email: 'friend@seerr.dev', password: 'test1234' });
+ assert.strictEqual(loginRes.status, 200);
+
+ const res = await friendAgent.delete(`/issue/${issue.id}`);
+
+ assert.strictEqual(res.status, 204);
+ } finally {
+ settings.main.localLogin = priorLocalLogin;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const settings = getSettings(); | |
| settings.main.localLogin = true; | |
| const settings = getSettings(); | |
| const priorLocalLogin = settings.main.localLogin; | |
| try { | |
| settings.main.localLogin = true; | |
| const friendAgent = request.agent(app); | |
| const loginRes = await friendAgent | |
| .post('/auth/local') | |
| .send({ email: 'friend@seerr.dev', password: 'test1234' }); | |
| assert.strictEqual(loginRes.status, 200); | |
| const res = await friendAgent.delete(`/issue/${issue.id}`); | |
| assert.strictEqual(res.status, 204); | |
| } finally { | |
| settings.main.localLogin = priorLocalLogin; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/issue.test.ts` around lines 166 - 167, The test mutates
settings.main.localLogin directly which can pollute other tests; capture the
prior value via const prev = getSettings().main.localLogin, set
settings.main.localLogin = true before performing the login, and then restore it
in a finally block (mirroring the adminAgent() pattern) so that getSettings()
and settings.main.localLogin are reset regardless of test outcome; update the
test around the getSettings()/settings.main.localLogin mutation to wrap the
login logic in try { ... } finally { getSettings().main.localLogin = prev; }.
| it('blocks the creator from deleting an issue that has replies', async () => { | ||
| const userRepo = getRepository(User); | ||
| const mediaRepo = getRepository(Media); | ||
| const issueRepo = getRepository(Issue); | ||
| const commentRepo = getRepository(IssueComment); | ||
|
|
||
| // Seed a non-admin user as the creator | ||
| const creator = await userRepo.findOneOrFail({ | ||
| where: { email: 'friend@seerr.dev' }, | ||
| }); | ||
| const media = mediaRepo.create({ tmdbId: 2, mediaType: MediaType.MOVIE }); | ||
| await mediaRepo.save(media); | ||
|
|
||
| const issue = issueRepo.create({ | ||
| issueType: 1, | ||
| createdBy: creator, | ||
| media, | ||
| }); | ||
| await issueRepo.save(issue); | ||
|
|
||
| // Seed two comments so commentCount > 1 | ||
| const admin = await userRepo.findOneOrFail({ | ||
| where: { email: 'admin@seerr.dev' }, | ||
| }); | ||
| await commentRepo.save([ | ||
| commentRepo.create({ message: 'first', user: creator, issue }), | ||
| commentRepo.create({ message: 'reply from admin', user: admin, issue }), | ||
| ]); | ||
|
|
||
| // Give creator CREATE_ISSUES but not MANAGE_ISSUES so they can reach the route | ||
| // handler but be blocked by the reply-count guard | ||
| creator.permissions = Permission.CREATE_ISSUES; | ||
| await userRepo.save(creator); | ||
|
|
||
| const settings = getSettings(); | ||
| settings.main.localLogin = true; | ||
| const friendAgent = request.agent(app); | ||
| await friendAgent | ||
| .post('/auth/local') | ||
| .send({ email: 'friend@seerr.dev', password: 'test1234' }); | ||
|
|
||
| const res = await friendAgent.delete(`/issue/${issue.id}`); | ||
|
|
||
| assert.strictEqual(res.status, 401); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Restore localLogin setting; test logic is excellent.
Lines 213-214 have the same mutation issue as the previous test and adminAgent(). However, the test logic itself is very good:
- Correctly sets up a scenario where the creator lacks MANAGE_ISSUES
- Seeds multiple comments to trigger the reply-count guard
- Validates that the 401 response is returned
♻️ Proposed fix for localLogin mutation
const settings = getSettings();
+ const priorLocalLogin = settings.main.localLogin;
- settings.main.localLogin = true;
- const friendAgent = request.agent(app);
- await friendAgent
- .post('/auth/local')
- .send({ email: 'friend@seerr.dev', password: 'test1234' });
-
- const res = await friendAgent.delete(`/issue/${issue.id}`);
-
- assert.strictEqual(res.status, 401);
+ try {
+ settings.main.localLogin = true;
+ const friendAgent = request.agent(app);
+ await friendAgent
+ .post('/auth/local')
+ .send({ email: 'friend@seerr.dev', password: 'test1234' });
+
+ const res = await friendAgent.delete(`/issue/${issue.id}`);
+
+ assert.strictEqual(res.status, 401);
+ } finally {
+ settings.main.localLogin = priorLocalLogin;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('blocks the creator from deleting an issue that has replies', async () => { | |
| const userRepo = getRepository(User); | |
| const mediaRepo = getRepository(Media); | |
| const issueRepo = getRepository(Issue); | |
| const commentRepo = getRepository(IssueComment); | |
| // Seed a non-admin user as the creator | |
| const creator = await userRepo.findOneOrFail({ | |
| where: { email: 'friend@seerr.dev' }, | |
| }); | |
| const media = mediaRepo.create({ tmdbId: 2, mediaType: MediaType.MOVIE }); | |
| await mediaRepo.save(media); | |
| const issue = issueRepo.create({ | |
| issueType: 1, | |
| createdBy: creator, | |
| media, | |
| }); | |
| await issueRepo.save(issue); | |
| // Seed two comments so commentCount > 1 | |
| const admin = await userRepo.findOneOrFail({ | |
| where: { email: 'admin@seerr.dev' }, | |
| }); | |
| await commentRepo.save([ | |
| commentRepo.create({ message: 'first', user: creator, issue }), | |
| commentRepo.create({ message: 'reply from admin', user: admin, issue }), | |
| ]); | |
| // Give creator CREATE_ISSUES but not MANAGE_ISSUES so they can reach the route | |
| // handler but be blocked by the reply-count guard | |
| creator.permissions = Permission.CREATE_ISSUES; | |
| await userRepo.save(creator); | |
| const settings = getSettings(); | |
| settings.main.localLogin = true; | |
| const friendAgent = request.agent(app); | |
| await friendAgent | |
| .post('/auth/local') | |
| .send({ email: 'friend@seerr.dev', password: 'test1234' }); | |
| const res = await friendAgent.delete(`/issue/${issue.id}`); | |
| assert.strictEqual(res.status, 401); | |
| }); | |
| it('blocks the creator from deleting an issue that has replies', async () => { | |
| const userRepo = getRepository(User); | |
| const mediaRepo = getRepository(Media); | |
| const issueRepo = getRepository(Issue); | |
| const commentRepo = getRepository(IssueComment); | |
| // Seed a non-admin user as the creator | |
| const creator = await userRepo.findOneOrFail({ | |
| where: { email: 'friend@seerr.dev' }, | |
| }); | |
| const media = mediaRepo.create({ tmdbId: 2, mediaType: MediaType.MOVIE }); | |
| await mediaRepo.save(media); | |
| const issue = issueRepo.create({ | |
| issueType: 1, | |
| createdBy: creator, | |
| media, | |
| }); | |
| await issueRepo.save(issue); | |
| // Seed two comments so commentCount > 1 | |
| const admin = await userRepo.findOneOrFail({ | |
| where: { email: 'admin@seerr.dev' }, | |
| }); | |
| await commentRepo.save([ | |
| commentRepo.create({ message: 'first', user: creator, issue }), | |
| commentRepo.create({ message: 'reply from admin', user: admin, issue }), | |
| ]); | |
| // Give creator CREATE_ISSUES but not MANAGE_ISSUES so they can reach the route | |
| // handler but be blocked by the reply-count guard | |
| creator.permissions = Permission.CREATE_ISSUES; | |
| await userRepo.save(creator); | |
| const settings = getSettings(); | |
| const priorLocalLogin = settings.main.localLogin; | |
| try { | |
| settings.main.localLogin = true; | |
| const friendAgent = request.agent(app); | |
| await friendAgent | |
| .post('/auth/local') | |
| .send({ email: 'friend@seerr.dev', password: 'test1234' }); | |
| const res = await friendAgent.delete(`/issue/${issue.id}`); | |
| assert.strictEqual(res.status, 401); | |
| } finally { | |
| settings.main.localLogin = priorLocalLogin; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/issue.test.ts` around lines 179 - 223, The test mutates the
global setting via getSettings().main.localLogin without restoring it; capture
the original value (e.g., const originalLocalLogin = settings.main.localLogin),
set settings.main.localLogin = true for the test, and then restore
settings.main.localLogin = originalLocalLogin (in a finally block or after
assertions) so other tests (and adminAgent() usage) are not affected; reference
getSettings() and settings.main.localLogin to locate where to save and restore
the original value.
|
Pushed a couple of test improvements from the review feedback:
Re the main concern about the issue list UI - the list endpoint already explicitly loads comments via |
…riber change Addresses gauthier-th's feedback: the count() query in the delete route and the subscriber relation change are unnecessary. Instead, just load comments alongside createdBy in the delete handler and keep using issue.comments.length for the permission guard.
|
@gauthier-th Thanks for the review — simplified in the latest commit:
The remaining diff is just removing |
Description
Removes
eager: truefrom thecommentsrelation on theIssueentity and fixes the two call sites that relied on it.Previously, every query that touched an
Issue, including the paginated issue list endpoint which can return up to 20 issues per page, automatically joined and loaded all comment rows for every issue. Comments were only needed in specific handlers: appending a new comment and checking whether replies exist before allowing deletion.Changes:
eager: truefromIssue.commentsinserver/entity/Issue.tsrelations: { comments: true }to thefindOneOrFailcall inPOST /:issueId/comment, which spreadsissue.commentswhen appending a new commentissue.comments.length > 1inDELETE /:issueIdwith a lightweightIssueComment.count()query so comments are never loaded just to check whether replies existThis is a small, low-impact fix - I'm using these PRs to get to know the codebase. The change is backwards compatible with no behaviour change for any endpoint.
AI Disclosure: I used Claude Code to help write the tests and validate the approach. Also calling out that I also used Claude to help me write tests and validate the sqlite db changes.
How Has This Been Tested?
Integration tests added in
server/routes/issue.test.tsusing the real SQLite test DB viasetupTestDb():POST /:issueId/comment-- adds a comment successfully and returns updated comment listPOST /:issueId/comment-- returns 500 for a non-existent issueDELETE /:issueId-- allows the creator to delete an issue with only one commentDELETE /:issueId-- blocks the creator from deleting when the issue has replies (commentCount > 1)DELETE /:issueId-- allows an admin (MANAGE_ISSUES) to delete regardless of comment countAll 5 tests pass.
Screenshots / Logs (if applicable)
N/A -- backend-only change with no UI impact.
Checklist:
pnpm buildpnpm i18n:extract(no new UI strings added)Summary by CodeRabbit
Tests
Refactor