Skip to content

Add lending request archiving - #5937

Open
Bestem0r wants to merge 2 commits into
masterfrom
add-lending-request-archivation
Open

Add lending request archiving#5937
Bestem0r wants to merge 2 commits into
masterfrom
add-lending-request-archivation

Conversation

@Bestem0r

@Bestem0r Bestem0r commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Description

Adds archiving of lending requests to clear up inbox.

Backend PR.

Result

  • Changes look good on both light and dark theme.
  • Changes look good with different viewports (mobile, tablet, etc.).
  • Changes look good with slower Internet connections.
Screencast.From.2026-04-13.22-23-36.mp4

Testing

  • I have thoroughly tested my changes.

@Bestem0r
Bestem0r requested a review from ch0rizo April 13, 2026 20:24
@github-actions github-actions Bot added the review-needed Pull requests that need review label Apr 13, 2026
@ch0rizo

ch0rizo commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

I think aesthetically it would look better if the archive wasn't so visible and prominent. Instead of a button, it would look better as a subtle icon that only appears on hover - maybe at the end of the object's title.

@Bestem0r
Bestem0r force-pushed the add-lending-request-archivation branch from 86d8774 to c8e711f Compare August 20, 2026 12:30
@Bestem0r

Copy link
Copy Markdown
Contributor Author

I think aesthetically it would look better if the archive wasn't so visible and prominent. Instead of a button, it would look better as a subtle icon that only appears on hover - maybe at the end of the object's title.

@ch0rizo What do you think about this? I am slightly against hiding the action through hover as it reduces accessibility.
image

@ch0rizo ch0rizo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the change, archived makes much more sense than "Nyeste/Eldste"!

One structural thing before this lands: the archived filter is applied client-side in RequestInbox after +Page.tsx has sliced and counted the list, while pagination ids are never pruned. That single placement causes four separate symptoms - short pages, a missing empty state, a wrong "Se mer", and the inbox re-animating.

Filtering before the slice (or server-side, since archived is already in the query) collapses all four. PR also needs a rebase for migrate_search infra break

Comment thread lego-webapp/pages/lending/_components/RequestInbox.tsx
<a
href={`/lending/${lendingRequest.lendableObject.id}/request/${lendingRequest.id} ${
isFromAdmin ? '?fromAdmin=true' : ''
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but there stray space here and a trailing newline. Breaks cards in /admin

<div
className={styles.archiveButtonWrapper}
onClick={preventCardNavigation}
>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The button is gated on !isFromAdmin, but onArchive is optional and called as onArchive?.(). Any non-admin caller renders a visible, permanently dead "Arkiver" button. Gate on onArchive && instead

const preventCardNavigation = (event: MouseEvent<HTMLDivElement>) => {
event.preventDefault();
event.stopPropagation();
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead after your implementation.

>
<Flex width="100%">
<Flex width="100%">
<Flex>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seems unneccessary

onChange={(value) =>
onArchivedChange(value === 'true' ? 'true' : 'false')
}
ariaLabel="Sorter utlånsforespørsler"

@ch0rizo ch0rizo Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a filter now😅 (ariaLabel)

}: Props) => {
const listRef = useRef<HTMLDivElement>(null);
const loadMoreRef = useRef<HTMLDivElement>(null);
const requestIds = lendingRequests.map((request) => String(request.id));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requestIds drives the gsap animation but is built pre-filter, so the id set doesn't match the rendered DOM nodes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the refetch inserts a new id mid-list, getAnimationStep sees a non-append change and returns all ids causing every card replays the fromTo(y: -18, opacity: 0) entrance and the whole inbox flashes.

) => {
await dispatch(editLendingRequest({ id: requestId, archived }));
await Promise.all([
dispatch(fetchLendingRequests({ query: activeRequestQuery })),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refetches page 1 without next: true, and buildPaginationReducer unconditionally does paginationNext.next = parse(payload.next…). The cursor rewinds to page 2 for a list the user may have paged well past.

Repro: click "Se mer" twice (12 loaded, cursor at page 4), archive one -> cursor is page 2. The next "Se mer" refetches page 2, every id is already in the Set, nothing new appears, but visibleCount still climbs. The button goes dead.

requestId: TransformedLendingRequest['id'],
archived: boolean,
) => {
await dispatch(editLendingRequest({ id: requestId, archived }));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

promiseMiddleware rejects on FAILURE. The refetches below never run and, since onPress discards the returned promise, it surfaces as an unhandledrejection (Sentry noise). Worth a try/catch.

};
const archivedRequestQuery = {
archived: 'true' as const,
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Aktiv tab requestQuery is already {archived: 'false'}

The other list gets refetched by usePreparedEffect (deps [requestArchived]) on tab switch anyway.

These constants would be better next to defaultLendingQuery I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-needed Pull requests that need review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants