feat(component): allow providing a comment when declining a request#1633
Open
ornelasf1 wants to merge 11 commits into
Open
feat(component): allow providing a comment when declining a request#1633ornelasf1 wants to merge 11 commits into
ornelasf1 wants to merge 11 commits into
Conversation
1 task
|
It should also be able to work in 'auto-approve' mode, because currently, we can only delete the ongoing request without providing a reason. |
Author
Are you sure you want to be deleting the request instead of declining? You can't leave a reason if it's deleted from the system. With this, the reason is optional. Simply decline without adding a message |
|
Hey it's been a few months i was wondering if this could get looked at by the reviewers? not sure if there's anything else it's waiting on |
Member
We are always focused on fixing bugs and merging fixes before adding new features. |
|
Great !! Can't wait 😅😆 |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
github-actions
Bot
removed
the
merge conflict
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
Member
|
@ornelasf1 sorry for the late review. Can you please rebase this on develop to fix the merge conflict? |
|
This PR is stale because it has been open 30 days with no activity. Please address the feedback or provide an update to keep it open. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Member
|
Avoid commenting here, PRs are not meant for that, read the |
Author
Hi, I've been busy, but I'll get this done soon |
…o provide a reason for it
ran format and i18n:extract
ornelasf1
force-pushed
the
feat/decline-w-comment
branch
from
35a86bd to
a4f32da
Compare
github-actions
Bot
removed
the
merge conflict
Contributor
📝 WalkthroughWalkthroughAdds optional decline reason storage, API support, notification rendering, and a modal-based decline flow in the frontend. ChangesDecline Request with Reason
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
server/lib/notifications/agents/webpush.ts (1)
128-139: 💤 Low valueRedundant null check on
payload.request.The condition
payload.request && payload.request?.declineReasonis redundant. Since the second operand uses optional chaining, it will safely evaluate toundefinedifpayload.requestis nullish. The email agent (line 172) uses simplyif (!payload.request.declineReason).Suggested simplification
case Notification.MEDIA_DECLINED: - if (payload.request && payload.request?.declineReason) { + if (payload.request?.declineReason) { message = intl.formatMessage(messages.declinedWithReason, {🤖 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/notifications/agents/webpush.ts` around lines 128 - 139, The condition checking `payload.request && payload.request?.declineReason` contains a redundant null check. Since optional chaining (the `?.` operator) already safely handles nullish values by returning undefined, the explicit `payload.request &&` check is unnecessary. Simplify the if condition to just `if (payload.request?.declineReason)` to remove the redundancy while maintaining the same behavior.
🤖 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 `@seerr-api.yml`:
- Around line 1274-1275: The declineReason field in the MediaRequest schema
definition needs to be marked as nullable to match runtime behavior where this
field can be null when no decline reason is stored. Update the declineReason
property definition to allow null values in addition to string values, ensuring
the OpenAPI schema accurately represents the actual response contract and
prevents mismatches for generated clients.
In `@server/entity/MediaRequest.ts`:
- Around line 556-558: The TypeScript type for the declineReason property does
not include null even though the database column is marked as nullable: true,
creating a type safety mismatch. Update the type annotation of the declineReason
field from string to string | null to accurately reflect that the database can
store null values for this column and maintain type safety with strict null
checks enabled.
In `@server/lib/notifications/agents/discord.ts`:
- Around line 145-155: The field name 'Decline Reason' in the fields.push() call
is hardcoded as a string instead of using the localized message like other
notification agents. Replace the hardcoded string 'Decline Reason' with
intl.formatMessage(globalMessages.declineReason) to ensure the Discord
notification agent uses the same internationalization approach as the Email,
Gotify, Ntfy, Pushbullet, Pushover, and Slack agents.
In `@server/lib/notifications/agents/ntfy.ts`:
- Around line 70-76: The declineReason value is being inserted directly into the
Markdown message without escaping user-provided content, creating a consistency
issue and potential Markdown formatting vulnerability. In the code block
handling Notification.MEDIA_DECLINED, wrap payload.request.declineReason with
this.escapeMarkdown() before concatenating it into the message string, following
the same pattern used for escaping payload.request.requestedBy.displayName
elsewhere in the file.
In `@server/migration/postgres/1781897226978-AddRequestDeclinedMessage.ts`:
- Around line 6-172: The migration in the up() method is recreating the entire
database schema instead of incrementally adding just the new declineReason
column to the media_request table. Replace the full table creation logic with an
ALTER TABLE statement that adds only the declineReason text column to the
existing media_request table. Similarly, update the down() method to use ALTER
TABLE to drop only the declineReason column instead of dropping all tables. This
ensures the migration is safe and non-destructive for environments with existing
data.
In `@server/routes/request.ts`:
- Around line 690-692: The declineReason field persists on the request object
even when the status changes from decline to approve or pending, causing
outdated decline reasons to be displayed on the frontend. Add logic to clear the
declineReason field (set it to null or undefined) whenever the request status is
updated to approve or pending status values, ensuring the field only contains a
value when the current status is decline. This should be implemented in the same
section where you handle status changes and set declineReason.
In `@src/components/RequestButton/index.tsx`:
- Line 166: The issue is that setShowDeclineCommentModal and related modal state
setters only flip a boolean flag without preserving which specific decline
action or request target was selected, causing the wrong request set to be
submitted when the modal confirms. To fix this, store the decline target context
(such as the specific request ID or action type being declined) in state
alongside the modal visibility flag at all affected locations (lines 166, 194,
236, 264, and the block at 396-400). Then use this stored context when the modal
confirms the action, rather than recomputing from current state which may have
changed or be ambiguous.
In `@src/components/RequestModal/DeclineRequestModal/index.tsx`:
- Around line 128-132: The decline button in DeclineRequestModal has both
type="submit" and an onClick handler that manually calls handleSubmit(), causing
the form to submit twice. Remove the onClick={() => handleSubmit()} prop from
the button element since Formik's Form component automatically handles
submission when type="submit" is present. Keep only the type, buttonType, and
disabled props on the button.
- Around line 85-95: The current error handling in the results.forEach loop only
catches rejected promises but does not handle HTTP error responses from
handleSubmit. Since fetch resolves even on 4xx/5xx status codes, failed HTTP
responses appear as fulfilled promises and bypass the error check. Modify the
forEach loop that iterates through results to also check if a fulfilled result
has a Response object with a non-ok status (res.ok === false), and throw an
appropriate error in that case so that HTTP failures are properly treated as
failures before onComplete is called.
---
Nitpick comments:
In `@server/lib/notifications/agents/webpush.ts`:
- Around line 128-139: The condition checking `payload.request &&
payload.request?.declineReason` contains a redundant null check. Since optional
chaining (the `?.` operator) already safely handles nullish values by returning
undefined, the explicit `payload.request &&` check is unnecessary. Simplify the
if condition to just `if (payload.request?.declineReason)` to remove the
redundancy while maintaining the same 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be9ce7ba-9f62-435e-9bdc-3f2a42fd2d17
📒 Files selected for processing (22)
seerr-api.ymlserver/entity/MediaRequest.tsserver/i18n/globalMessages.tsserver/i18n/locale/en.jsonserver/lib/notifications/agents/discord.tsserver/lib/notifications/agents/email.tsserver/lib/notifications/agents/gotify.tsserver/lib/notifications/agents/ntfy.tsserver/lib/notifications/agents/pushbullet.tsserver/lib/notifications/agents/pushover.tsserver/lib/notifications/agents/slack.tsserver/lib/notifications/agents/telegram.tsserver/lib/notifications/agents/webpush.tsserver/migration/postgres/1781897226978-AddRequestDeclinedMessage.tsserver/migration/sqlite/1781897118247-AddRequestDeclinedMessage.tsserver/routes/request.tssrc/components/RequestButton/index.tsxsrc/components/RequestCard/index.tsxsrc/components/RequestList/RequestItem/index.tsxsrc/components/RequestModal/DeclineRequestModal/index.tsxsrc/i18n/globalMessages.tssrc/i18n/locale/en.json
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/components/RequestModal/DeclineRequestModal/index.tsx (1)
50-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-throw request failures so batch decline can’t report false success.
Line 50-58 catches
axios.posterrors but resolves instead of rejecting; then Line 92-94 never detects failures and Line 98-100 may still callonComplete()after partial/failed declines.Suggested fix
const handleSubmit = async ( requestId: number, values: { reason: string } ) => { try { await axios.post(`/api/v1/request/${requestId}/decline`, { declineReason: values.reason, }); - } catch { - if (onError) { - onError(); - } + } catch (error) { + onError?.(); + throw error; } };Also applies to: 88-95
🤖 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 `@src/components/RequestModal/DeclineRequestModal/index.tsx` around lines 50 - 58, The catch block in the axios.post call for declining a request (lines 50-58) handles the error by calling the onError callback but does not re-throw the error, causing the Promise to resolve successfully even when the request fails. This prevents the batch decline logic from detecting failures, allowing false success reports. After calling onError() in the catch block, re-throw the error so that the calling code can properly detect and handle request failures.src/components/RequestButton/index.tsx (1)
166-166:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPersist decline targets in state; current wiring can decline the wrong requests.
Line 166/194/236/264 only toggles modal visibility. At Line 399, modal requests are always derived from non-4K state (
activeRequest/activeRequests), so 4K decline actions can operate on the wrong set. Also, Line 396 prevents rendering when only 4K requests are pending.Suggested fix
+ const [declineTargets, setDeclineTargets] = useState<MediaRequest[]>([]); @@ action: () => { + setDeclineTargets([activeRequest]); setShowDeclineCommentModal(true); }, @@ action: () => { + setDeclineTargets(activeRequests); setShowDeclineCommentModal(true); }, @@ action: () => { + setDeclineTargets([active4kRequest]); setShowDeclineCommentModal(true); }, @@ action: () => { + setDeclineTargets(active4kRequests); setShowDeclineCommentModal(true); }, @@ - {(activeRequest || (activeRequests && activeRequests.length > 0)) && ( + {declineTargets.length > 0 && ( <DeclineRequestModal show={showDeclineCommentModal} - requests={activeRequest ? [activeRequest] : activeRequests} + requests={declineTargets} type={mediaType} - onCancel={() => setShowDeclineCommentModal(false)} + onCancel={() => { + setShowDeclineCommentModal(false); + setDeclineTargets([]); + }} onComplete={() => { onUpdate(); mutate('/api/v1/request/count'); setShowDeclineCommentModal(false); + setDeclineTargets([]); }} /> )}Also applies to: 194-194, 236-236, 264-264, 396-400
🤖 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 `@src/components/RequestButton/index.tsx` at line 166, The decline modal visibility toggles at lines 166, 194, 236, and 264 do not persist which specific request is being declined. The modal confirmation handler at line 399 always uses the generic activeRequest or activeRequests state to determine what to decline, which causes the wrong request to be declined when a 4K request decline action opens the modal. Add a new state variable to track the specific decline target (either a single request or list of requests), populate it alongside setShowDeclineCommentModal(true) at each of the four toggle locations, and then use this persisted decline target in the modal confirmation handler instead of dereferencing activeRequest or activeRequests.
🤖 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.
Duplicate comments:
In `@src/components/RequestButton/index.tsx`:
- Line 166: The decline modal visibility toggles at lines 166, 194, 236, and 264
do not persist which specific request is being declined. The modal confirmation
handler at line 399 always uses the generic activeRequest or activeRequests
state to determine what to decline, which causes the wrong request to be
declined when a 4K request decline action opens the modal. Add a new state
variable to track the specific decline target (either a single request or list
of requests), populate it alongside setShowDeclineCommentModal(true) at each of
the four toggle locations, and then use this persisted decline target in the
modal confirmation handler instead of dereferencing activeRequest or
activeRequests.
In `@src/components/RequestModal/DeclineRequestModal/index.tsx`:
- Around line 50-58: The catch block in the axios.post call for declining a
request (lines 50-58) handles the error by calling the onError callback but does
not re-throw the error, causing the Promise to resolve successfully even when
the request fails. This prevents the batch decline logic from detecting
failures, allowing false success reports. After calling onError() in the catch
block, re-throw the error so that the calling code can properly detect and
handle request failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cee2e09-2fc2-4454-a0cd-b73d7c3a050f
📒 Files selected for processing (6)
server/lib/notifications/agents/discord.tsserver/routes/request.tssrc/components/RequestButton/index.tsxsrc/components/RequestCard/index.tsxsrc/components/RequestList/RequestItem/index.tsxsrc/components/RequestModal/DeclineRequestModal/index.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- server/lib/notifications/agents/discord.ts
- server/routes/request.ts
- src/components/RequestList/RequestItem/index.tsx
- src/components/RequestCard/index.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/migration/sqlite/1781992375456-AddRequestDeclinedMessage.ts (1)
7-20: ⚡ Quick winRemove redundant
user_push_subscriptionrebuilds.The
user_push_subscriptiontable is rebuilt twice inup()(lines 7-20 and 47-60) with identical schema each time. Since this migration only addsdeclineReasontomedia_request, these operations are unnecessary and appear to be TypeORM migration generator artifacts. They double migration time and add DDL risk without effect.Same issue exists in
down()at lines 64-77 and 104-117.♻️ Suggested cleanup for up() method
public async up(queryRunner: QueryRunner): Promise<void> { - await queryRunner.query(`DROP INDEX "IDX_03f7958328e311761b0de675fb"`); - await queryRunner.query( - `CREATE TABLE "temporary_user_push_subscription" ("id" integer PRIMARY KEY AUTOINCREMENT NOT NULL, "endpoint" varchar NOT NULL, "p256dh" varchar NOT NULL, "auth" varchar NOT NULL, "userId" integer, "userAgent" varchar, "createdAt" datetime DEFAULT (CURRENT_TIMESTAMP), CONSTRAINT "UQ_6427d07d9a171a3a1ab87480005" UNIQUE ("endpoint", "userId"), CONSTRAINT "UQ_f90ab5a4ed54905a4bb51a7148b" UNIQUE ("auth"), CONSTRAINT "FK_03f7958328e311761b0de675fbe" FOREIGN KEY ("userId") REFERENCES "user" ("id") ON DELETE CASCADE ON UPDATE NO ACTION)` - ); - await queryRunner.query( - `INSERT INTO "temporary_user_push_subscription"("id", "endpoint", "p256dh", "auth", "userId", "userAgent", "createdAt") SELECT "id", "endpoint", "p256dh", "auth", "userId", "userAgent", "createdAt" FROM "user_push_subscription"` - ); - await queryRunner.query(`DROP TABLE "user_push_subscription"`); - await queryRunner.query( - `ALTER TABLE "temporary_user_push_subscription" RENAME TO "user_push_subscription"` - ); - await queryRunner.query( - `CREATE INDEX "IDX_03f7958328e311761b0de675fb" ON "user_push_subscription" ("userId") ` - ); await queryRunner.query(`DROP INDEX "IDX_4c696e8ed36ae34fe18abe59d2"`); // ... media_request rebuild with declineReason ... await queryRunner.query( `CREATE INDEX "IDX_4c696e8ed36ae34fe18abe59d2" ON "media_request" ("status") ` ); - await queryRunner.query(`DROP INDEX "IDX_03f7958328e311761b0de675fb"`); - // ... second identical user_push_subscription rebuild ... }Also applies to: 47-60
🤖 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/migration/sqlite/1781992375456-AddRequestDeclinedMessage.ts` around lines 7 - 20, The migration file contains duplicate and identical rebuilds of the user_push_subscription table in both the up() and down() methods. In the up() method, remove the second identical user_push_subscription rebuild sequence (lines 47-60) that duplicates the first one (lines 7-20), and in the down() method, remove the second identical sequence (lines 104-117) that duplicates the first one (lines 64-77). Since this migration only adds declineReason to media_request, the duplicate user_push_subscription table operations are unnecessary and should be eliminated to reduce migration time and DDL risk.
🤖 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.
Nitpick comments:
In `@server/migration/sqlite/1781992375456-AddRequestDeclinedMessage.ts`:
- Around line 7-20: The migration file contains duplicate and identical rebuilds
of the user_push_subscription table in both the up() and down() methods. In the
up() method, remove the second identical user_push_subscription rebuild sequence
(lines 47-60) that duplicates the first one (lines 7-20), and in the down()
method, remove the second identical sequence (lines 104-117) that duplicates the
first one (lines 64-77). Since this migration only adds declineReason to
media_request, the duplicate user_push_subscription table operations are
unnecessary and should be eliminated to reduce migration time and DDL risk.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e996bef-1bf1-4ea2-a60f-ea912098ebb2
📒 Files selected for processing (4)
server/entity/MediaRequest.tsserver/lib/notifications/agents/ntfy.tsserver/migration/postgres/1781992427522-AddRequestDeclinedMessage.tsserver/migration/sqlite/1781992375456-AddRequestDeclinedMessage.ts
✅ Files skipped from review due to trivial changes (2)
- server/migration/postgres/1781992427522-AddRequestDeclinedMessage.ts
- server/entity/MediaRequest.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/lib/notifications/agents/ntfy.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/components/RequestButton/index.tsx`:
- Around line 401-414: The close/reset logic in RequestButton’s
DeclineRequestModal flow is being triggered even when some decline requests
fail, so the modal can disappear on partial failure. Update the completion
handling around onComplete so it only calls onUpdate,
mutate('/api/v1/request/count'), setShowDeclineCommentModal(false), and
setDeclineTargets([]) when every request in declineTargets succeeds; otherwise
keep the modal open and surface an error state for retry.
🪄 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: d937d037-a986-44fe-97fb-3fd70b292788
📒 Files selected for processing (1)
src/components/RequestButton/index.tsx
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
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
6 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.
Description
A user is able to decline a request with a message. The request will be declined and the requester will be notified with the reason for it. The message is optional so if the field is left blank, then no message will be added
AI Disclosure: None.
How Has This Been Tested?
I ran the app locally. I signed in with an admin user and a regular on separate browsers. Created requests and declined with and without reasons. Verified the request was updated with message and that the notifications were delivered with messages.
Screenshots / Logs (if applicable)
Modal for declining with message
Comment appearing in list of requests
Email with decline reason.
Discord webhook with decline reason.
Pushbullet with decline reason
And other notification providers.
Checklist:
Summary by CodeRabbit
declineReasonfor declined requests.