Repository navigation
feat: declutter review - #273
Open
TheBlackSide243 wants to merge 2 commits into
Open
TheBlackSide243 wants to merge 2 commits into
TheBlackSide243 wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Reviewer's GuideIntroduces a full Declutter review workflow: backend candidate calculation and keep-period persistence exposed through authenticated APIs, plus a localized frontend page with filtering, keep/donate/sell actions, navigation, and automated coverage. Sequence diagram for the declutter review workflowsequenceDiagram
actor User
participant Page as DeclutterPage
participant Hook as useDeclutter
participant API as GET_declutter
participant Service as DeclutterService
participant Keep as DeclutterKeep
User->>Page: Select months
Page->>Hook: useDeclutter(months)
Hook->>API: api.get /declutter?months=months
API->>Service: active_items(current_user)
API->>Service: kept_until(current_user)
API->>Service: find_candidates(items, today, months, kept_until)
Service-->>API: sorted candidates
API-->>Hook: DeclutterResponse
Hook-->>Page: candidates and idle_value
User->>Page: Click Keep
Page->>API: api.post /declutter/item_id/keep
API->>Service: keep(item, today, months)
Service->>Keep: upsert kept_until
Keep-->>Service: persisted keep date
Service-->>API: kept_until
API-->>Page: KeepResponse
Entity relationship diagram for declutter keep persistenceerDiagram
CLOTHING_ITEMS {
UUID id PK
UUID user_id
date last_worn_at
decimal purchase_price
boolean is_archived
}
DECLUTTER_KEEPS {
UUID item_id PK, FK
date kept_until
}
CLOTHING_ITEMS ||--o| DECLUTTER_KEEPS : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
javascript
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="frontend/lib/hooks/use-declutter.ts" line_range="27-31" />
<code_context>
+ const { status } = useSession();
+ useSetTokenIfAvailable();
+
+ return useQuery({
+ queryKey: queryKeys.declutter(months),
+ queryFn: () => api.get<DeclutterReview>(`/declutter?months=${months}`),
+ enabled: status === 'authenticated',
+ });
+}
+
</code_context>
<issue_to_address>
**Declutter results remain stale**
When another item mutation changes declutter eligibility or item existence while the cached review is less than one minute old, `useDeclutter` caches the review without being invalidated by other item mutations, so after an item is worn, edited, archived, or deleted, the page continues showing the old candidate list and idle value while that query remains fresh. A user can then act on an item that no longer qualifies, or receive an error trying to keep or archive an item that was deleted.
Invalidate `queryKeys.declutterAll` whenever a mutation changes item data used by the review, including wear, archive, edit, and delete operations.
</issue_to_address>
### Comment 2
<location path="backend/app/api/declutter.py" line_range="80" />
<code_context>
+ db: DbSession,
+ current_user: Annotated[User, Depends(get_current_user)],
+) -> KeepResponse:
+ item = await ItemService(db).get_by_id(item_id, current_user.id)
+ if not item:
+ raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Item not found")
</code_context>
<issue_to_address>
**Archived items retain stale keeps**
When a keep request targets an owned item that is archived or not ready, `ItemService.get_by_id` checks ownership but not whether the item is active, so `keep_item` writes a keep row for archived or processing items. Restoring an archived item later makes it active again, but `find_candidates` still suppresses it until that keep expires, even though it was never eligible for the keep action.
Reject keep requests unless the item is ready and not archived, matching the eligibility filter used by the review endpoint.
</issue_to_address>
### Comment 3
<location path="backend/app/services/declutter_service.py" line_range="95-98" />
<code_context>
+
+ async def keep(self, item: ClothingItem, today: date, months: int) -> date:
+ until = today + timedelta(days=months * DAYS_PER_MONTH)
+ statement = insert(DeclutterKeep).values(item_id=item.id, kept_until=until)
+ await self.db.execute(
+ statement.on_conflict_do_update(
+ index_elements=[DeclutterKeep.item_id], set_={"kept_until": until}
+ )
+ )
</code_context>
<issue_to_address>
**Older keep request overwrites newer choice**
When two keep requests for the same item with different month values execute out of order, `keep` unconditionally upserts `kept_until`, so when an earlier request with a different `months` value reaches this statement after a newer request, it overwrites the newer choice and the item stays hidden for the wrong period.
Add ordering/version protection so a delayed earlier request cannot overwrite a newer keep decision.
</issue_to_address>
### Comment 4
<location path="backend/tests/test_declutter.py" line_range="117" />
<code_context>
+ @pytest.mark.asyncio
+ async def test_months_out_of_range(self, client: AsyncClient, auth_headers):
+ response = await client.get("/api/v1/declutter?months=0", headers=auth_headers)
+ assert response.status_code == 422
</code_context>
<issue_to_address>
**Ownership check is untested**
When the endpoint accepts an existing item owned by another user, when the endpoint incorrectly allows keeping another user's item, `test_cannot_keep_someone_elses_item` still passes: line 110 sends a random UUID that does not exist, and line 112 gets 404 for the missing item rather than verifying ownership is enforced.
Create an item for a second user and assert the authenticated user cannot keep it.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and keeping an item writes a date that can hide it from future reviews for up to 36 months; reverting the code would not remove those saved choices, though they expire and can be cleared. Donate and sell use the existing archive flow after an explicit user action.
Blocking findings: frontend/lib/hooks/use-declutter.ts:31, backend/app/api/declutter.py:80, backend/app/services/declutter_service.py:98, backend/tests/test_declutter.py:117
Comment on lines
+27
to
+31
| return useQuery({ | ||
| queryKey: queryKeys.declutter(months), | ||
| queryFn: () => api.get<DeclutterReview>(`/declutter?months=${months}`), | ||
| enabled: status === 'authenticated', | ||
| }); |
There was a problem hiding this comment.
🟡 Medium · Declutter results remain stale
When another item mutation changes declutter eligibility or item existence while the cached review is less than one minute old, useDeclutter caches the review without being invalidated by other item mutations, so after an item is worn, edited, archived, or deleted, the page continues showing the old candidate list and idle value while that query remains fresh. A user can then act on an item that no longer qualifies, or receive an error trying to keep or archive an item that was deleted.
Invalidate queryKeys.declutterAll whenever a mutation changes item data used by the review, including wear, archive, edit, and delete operations.
Prompt for AI agents
In `frontend/lib/hooks/use-declutter.ts` at lines 27-31:
**Declutter results remain stale**
When another item mutation changes declutter eligibility or item existence while the cached review is less than one minute old, `useDeclutter` caches the review without being invalidated by other item mutations, so after an item is worn, edited, archived, or deleted, the page continues showing the old candidate list and idle value while that query remains fresh. A user can then act on an item that no longer qualifies, or receive an error trying to keep or archive an item that was deleted.
Invalidate `queryKeys.declutterAll` whenever a mutation changes item data used by the review, including wear, archive, edit, and delete operations.| db: DbSession, | ||
| current_user: Annotated[User, Depends(get_current_user)], | ||
| ) -> KeepResponse: | ||
| item = await ItemService(db).get_by_id(item_id, current_user.id) |
There was a problem hiding this comment.
🟡 Medium · Archived items retain stale keeps
When a keep request targets an owned item that is archived or not ready, ItemService.get_by_id checks ownership but not whether the item is active, so keep_item writes a keep row for archived or processing items. Restoring an archived item later makes it active again, but find_candidates still suppresses it until that keep expires, even though it was never eligible for the keep action.
Reject keep requests unless the item is ready and not archived, matching the eligibility filter used by the review endpoint.
Prompt for AI agents
In `backend/app/api/declutter.py` at line 80:
**Archived items retain stale keeps**
When a keep request targets an owned item that is archived or not ready, `ItemService.get_by_id` checks ownership but not whether the item is active, so `keep_item` writes a keep row for archived or processing items. Restoring an archived item later makes it active again, but `find_candidates` still suppresses it until that keep expires, even though it was never eligible for the keep action.
Reject keep requests unless the item is ready and not archived, matching the eligibility filter used by the review endpoint.
Comment on lines
+95
to
+98
| statement = insert(DeclutterKeep).values(item_id=item.id, kept_until=until) | ||
| await self.db.execute( | ||
| statement.on_conflict_do_update( | ||
| index_elements=[DeclutterKeep.item_id], set_={"kept_until": until} |
There was a problem hiding this comment.
🟡 Medium · Older keep request overwrites newer choice
When two keep requests for the same item with different month values execute out of order, keep unconditionally upserts kept_until, so when an earlier request with a different months value reaches this statement after a newer request, it overwrites the newer choice and the item stays hidden for the wrong period.
Add ordering/version protection so a delayed earlier request cannot overwrite a newer keep decision.
Prompt for AI agents
In `backend/app/services/declutter_service.py` at lines 95-98:
**Older keep request overwrites newer choice**
When two keep requests for the same item with different month values execute out of order, `keep` unconditionally upserts `kept_until`, so when an earlier request with a different `months` value reaches this statement after a newer request, it overwrites the newer choice and the item stays hidden for the wrong period.
Add ordering/version protection so a delayed earlier request cannot overwrite a newer keep decision.| @pytest.mark.asyncio | ||
| async def test_months_out_of_range(self, client: AsyncClient, auth_headers): | ||
| response = await client.get("/api/v1/declutter?months=0", headers=auth_headers) | ||
| assert response.status_code == 422 |
There was a problem hiding this comment.
🟠 High · Ownership check is untested
When the endpoint accepts an existing item owned by another user, when the endpoint incorrectly allows keeping another user's item, test_cannot_keep_someone_elses_item still passes: line 110 sends a random UUID that does not exist, and line 112 gets 404 for the missing item rather than verifying ownership is enforced.
Create an item for a second user and assert the authenticated user cannot keep it.
Prompt for AI agents
In `backend/tests/test_declutter.py` at line 117:
**Ownership check is untested**
When the endpoint accepts an existing item owned by another user, when the endpoint incorrectly allows keeping another user's item, `test_cannot_keep_someone_elses_item` still passes: line 110 sends a random UUID that does not exist, and line 112 gets 404 for the missing item rather than verifying ownership is enforced.
Create an item for a second user and assert the authenticated user cannot keep it.- Only ready, unarchived items can be kept, like the review itself. - The review is fetched again whenever the page opens, so items worn, edited, archived or deleted elsewhere do not linger in it. - The ownership test now uses another user's real item. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds a Declutter page listing the clothes that have not been worn for a chosen period (3, 6, 12
or 24 months): never-worn items first, then the longest idle, with the total purchase value
sitting idle. Each item can be kept, which hides it from the review for the same period, or
opened to be edited or deleted.
GET /api/v1/declutter?months=andPOST /api/v1/declutter/{item_id}/keep.declutter_keepstable rather than aclothing_itemscolumn, soitem writes are untouched (migration
c4d9e2a7b1f3)./dashboard/declutter, nav entry, translations for all 8 locales.This is one of a set of small, independent PRs (each branches from
mainand can be reviewed ordropped on its own); they only overlap in the nav/router registration lines.
Note on migrations
This PR and the shopping list one each add a migration on top of
a7d3c9e1f5b2. They areindependent, so whichever is merged second needs its first migration's
down_revisionbumped tothe other's head; happy to rebase it.
Type of Change
Checklist
Testing
ruff check,ruff format --check, migration heads check and the full pytest suite on top of currentmain(2033 passed).npm run lint,i18n:check,vocab:check,tsc --noEmitand vitest (693 passed).Test Environment
🤖 Generated with Claude Code
Summary by Sourcery
Add a localized declutter workflow for reviewing and acting on clothing that has remained unworn for a selected period.
New Features:
Enhancements:
Tests: