fix: sort Recently Used Apps by settled transactions#2012
fix: sort Recently Used Apps by settled transactions#2012keshav0479 wants to merge 1 commit intogetAlby:masterfrom
Conversation
|
Hi, Thanks for the PR. But I still think it's important to know when there was any activity at all, as well as if there was monetary activity. There are also problems with the current solution that simply requesting an invoice (but it not being paid) would count as monetary activity. I think this needs more planning before working on an implementation. |
|
Fair point,it is useful to know when an app was last online/connected, even if it didn't spend anything. To fix the sorting without losing that data, how about we split it into two fields? |
|
I wonder if it is possible already to find these apps by doing a DB query that joins apps on the transaction list? then there's no DB changes needed. (as long as the query is efficient enough) |
|
Oh that's a great point, i hadn't considered using the existing transactions table. |
|
@keshav0479 only settled payments count. I think you can look at the transaction state as the settled constant (in constants.go) |
|
Got it, that makes perfect sense regarding the |
|
@keshav0479 sounds good to me, thanks! |
keshav0479
force-pushed
the
fix/recently-used-apps
branch
from
dc8d2cd to
25c5517
Compare
keshav0479
changed the title
api/api.go
Outdated
|
|
||
| if orderBy == "" { | ||
| orderBy = "last_used_at" | ||
| var totalCount int64 |
There was a problem hiding this comment.
This is incorrect - counting must be done after filtering.
Also, I would undo your changes and make a new sorting parameter for last_economic_activity or something like that.
There was a problem hiding this comment.
or last_settled_transaction maybe
There was a problem hiding this comment.
Updated! Changed the log field to app_id and added last_settled_transaction as a new sorting option, keeping the original logic intact
keshav0479
force-pushed
the
fix/recently-used-apps
branch
from
25c5517 to
1e5cbd8
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new ListApps ordering option "last_settled_transaction" (sorts by most recent settled transaction timestamp), updates frontend hook and widget to request that ordering and stop client-side resorting, and corrects a structured log field name in an event handler. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as Frontend (Browser)
participant API as API Server
participant DB as Database
User->>Frontend: Open "Latest Used Apps"
Frontend->>API: GET /api/apps?order_by=last_settled_transaction
API->>DB: Query apps LEFT JOIN transactions (state=SETTLED), SELECT MAX(transactions.created_at) AS last_transaction_at, GROUP BY apps.id, ORDER BY last_transaction_at DESC, apps.last_used_at
DB-->>API: Return ordered apps list
API-->>Frontend: Respond with ordered apps
Frontend-->>User: Render apps in received order
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/api.go (1)
526-531: Count query may return incorrect results with GROUP BY.When
orderBy == "last_settled_transaction", the query has aGROUP BY apps.idclause. Calling.Model(&db.App{}).Count(&totalCount)on this grouped query may not return the correct total count of apps—it could count the number of groups or behave unexpectedly depending on the database driver.Consider computing the count before applying the grouping/aggregation for the
last_settled_transactioncase, or use a separate unmodified query for counting.🔧 Suggested approach
+ // Count before applying aggregation for last_settled_transaction + countQuery := api.db + if filters.Name != "" { + if api.db.Dialector.Name() == "postgres" { + countQuery = countQuery.Where("name ILIKE ?", filters.Name+"%") + } else { + countQuery = countQuery.Where("name LIKE ?", filters.Name+"%") + } + } + // ... apply other filters to countQuery ... + var totalCount int64 - result := query.Model(&db.App{}).Count(&totalCount) + result := countQuery.Model(&db.App{}).Count(&totalCount)Alternatively, you could clone the query before adding the SELECT/JOIN/GROUP BY clauses and use the clone for counting.
🤖 Fix all issues with AI agents
In `@api/api.go`:
- Around line 514-521: The orderBy string already contains DESC and causes a
duplicated "DESC" in query.Order(orderBy + " DESC"), and NULL
last_transaction_at values need to be ordered to the end; when handling the
last_settled_transaction branch (the block that calls query.Select(...
MAX(transactions.created_at) as last_transaction_at) / Joins(...) /
Group("apps.id")), set orderBy to a value that includes NULL-safe ordering (for
example using COALESCE(last_transaction_at, '1970-01-01') DESC,
apps.last_used_at DESC) and then stop appending " DESC" later — change the final
call to use query.Order(orderBy) (or ensure orderBy contains no direction if you
intend to append " DESC"), so you neither produce duplicate DESC nor leave NULL
last_transaction_at entries unordered.
|
@keshav0479 LGTM, nice work 🙌 But can we use |
keshav0479
force-pushed
the
fix/recently-used-apps
branch
from
6ebe358 to
3cacb18
Compare
keshav0479
force-pushed
the
fix/recently-used-apps
branch
from
03fd58d to
47f0597
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/api.go (1)
528-543:⚠️ Potential issue | 🔴 CriticalReject unknown
orderByvalues before building the SQL clause.
orderBystill comes from request input, and Line 543 concatenates it directly intoOrder(...). The new frontend literal union does not protect this endpoint from crafted calls, so please whitelist the supported values and build the full order clause server-side.As per coding guidelines, `{api,http}/**/*.go`: Validate all user input at system boundaries; trust internal service calls.🔒 Safer pattern
+ var orderClause string + if orderBy == "" { orderBy = "last_used_at" } - if orderBy == "last_used_at" { - // when ordering by last used at, apps with last_used_at is NULL should be ordered last - orderBy = "last_used_at IS NULL, " + orderBy - } - - if orderBy == "last_settled_transaction" { + + switch orderBy { + case "last_used_at": + orderClause = "last_used_at IS NULL, last_used_at DESC" + case "created_at": + orderClause = "created_at DESC" + case "last_settled_transaction": query = query.Select("apps.*, MAX(transactions.created_at) as last_transaction_at"). Joins("LEFT JOIN transactions ON transactions.app_id = apps.id AND transactions.state = ?", constants.TRANSACTION_STATE_SETTLED). Group("apps.id") - orderBy = "last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at" + orderClause = "last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at DESC" + default: + return nil, fmt.Errorf("invalid orderBy: %s", orderBy) } - query = query.Order(orderBy + " DESC") + query = query.Order(orderClause)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/api.go` around lines 528 - 543, Reject/whitelist untrusted orderBy values before using them in the SQL ORDER clause: validate the incoming orderBy variable against an allowed set (e.g., "", "last_used_at", "last_settled_transaction", any other supported keys) and map each allowed key to a fully formed, safe SQL ordering expression (e.g., "last_used_at IS NULL, last_used_at DESC" and for "last_settled_transaction" ensure you build the JOIN/Group and use "last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at"); if the value is not in the whitelist, fall back to the default ordering, and only then call query.Order(...). Reference the orderBy variable, the query builder block that sets MAX(transactions.created_at) with constants.TRANSACTION_STATE_SETTLED, and ensure no raw request string is concatenated into Order().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/api.go`:
- Around line 537-540: The current SELECT and GROUP uses
MAX(transactions.created_at) to compute last_transaction_at but should use the
settlement timestamp; change the derived column to MAX(transactions.settled_at)
(the DB column for db.Transaction.SettledAt) in the query.Select and keep the
same JOIN filter using constants.TRANSACTION_STATE_SETTLED, and keep the orderBy
logic ("last_transaction_at IS NULL, last_transaction_at DESC,
apps.last_used_at") so apps are ranked by the actual settled time instead of
request creation time.
In `@frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx`:
- Around line 16-18: The widget is still keyed and filtered by lastUsedAt
despite requesting last_settled_transaction from the API; update
LatestUsedAppsWidget to use the API's lastSettledTransaction field instead:
change the useApps consumer to read appsData?.apps[*].lastSettledTransaction
(not lastUsedAt), update usedApps = apps?.filter(x => x.lastSettledTransaction),
and render the relative-time label from lastSettledTransaction; also make the
same replacement for the other occurrences in this component (around the 35-45
region) so filtering, sorting, and display consistently use
lastSettledTransaction.
---
Outside diff comments:
In `@api/api.go`:
- Around line 528-543: Reject/whitelist untrusted orderBy values before using
them in the SQL ORDER clause: validate the incoming orderBy variable against an
allowed set (e.g., "", "last_used_at", "last_settled_transaction", any other
supported keys) and map each allowed key to a fully formed, safe SQL ordering
expression (e.g., "last_used_at IS NULL, last_used_at DESC" and for
"last_settled_transaction" ensure you build the JOIN/Group and use
"last_transaction_at IS NULL, last_transaction_at DESC, apps.last_used_at"); if
the value is not in the whitelist, fall back to the default ordering, and only
then call query.Order(...). Reference the orderBy variable, the query builder
block that sets MAX(transactions.created_at) with
constants.TRANSACTION_STATE_SETTLED, and ensure no raw request string is
concatenated into Order().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09c37dda-6350-48ce-a943-9fb6df1dba1c
📒 Files selected for processing (4)
api/api.gofrontend/src/components/home/widgets/LatestUsedAppsWidget.tsxfrontend/src/hooks/useApps.tsnip47/event_handler.go
✅ Files skipped from review due to trivial changes (1)
- nip47/event_handler.go
| const { data: appsData } = useApps(3, undefined, undefined, "last_settled_transaction"); | ||
| const apps = appsData?.apps; | ||
| const usedApps = apps?.filter((x) => x.lastUsedAt); |
There was a problem hiding this comment.
Don't keep keying this widget off lastUsedAt.
This now asks the API for last_settled_transaction, but it still filters rows and renders the relative-time label from lastUsedAt, which the PR explicitly keeps as connection activity. A recent balance check can therefore still make an app look newer than its last paid action, and the post-limit filter can leave the widget under-filled. Please plumb a dedicated lastSettledTransaction field through the API and use that here instead.
Also applies to: 35-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx` around lines
16 - 18, The widget is still keyed and filtered by lastUsedAt despite requesting
last_settled_transaction from the API; update LatestUsedAppsWidget to use the
API's lastSettledTransaction field instead: change the useApps consumer to read
appsData?.apps[*].lastSettledTransaction (not lastUsedAt), update usedApps =
apps?.filter(x => x.lastSettledTransaction), and render the relative-time label
from lastSettledTransaction; also make the same replacement for the other
occurrences in this component (around the 35-45 region) so filtering, sorting,
and display consistently use lastSettledTransaction.
There was a problem hiding this comment.
The lastUsedAt filter just checks if the app has connected before, the sorting itself comes from the backend. Can add a dedicated field in a follow-up if needed!
There was a problem hiding this comment.
@keshav0479, that's a fair point — using lastUsedAt as a gate to filter out apps that have never connected at all is reasonable, and the backend-driven sorting by settled transactions is the most impactful fix here.
The one remaining UX concern is the displayed time label (dayjs(app.lastUsedAt).fromNow()): a user seeing "2 minutes ago" next to an app that last did a balance check (but hasn't moved value in days) could still be misled. That said, since the ordering itself is now correct, this is a cosmetic/UX issue — a follow-up to plumb lastSettledTransaction through to the frontend for the label sounds like a pragmatic approach. Would you like to open a follow-up issue to track that?
There was a problem hiding this comment.
I think this should be fixed in this PR. If we filter by last economic activity, then we should also show when the last economic activity was, not the last time it was just accessed via NWC.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
keshav0479
force-pushed
the
fix/recently-used-apps
branch
from
47f0597 to
2c9de78
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx (1)
16-18:⚠️ Potential issue | 🟠 MajorUse settled-transaction timestamp consistently in this widget.
Line 18 filters on
lastUsedAt, and Line 45 renders relative time fromlastUsedAt. That keeps this widget tied to connection activity and can hide apps that were correctly ranked by settled transactions.Suggested fix
- const usedApps = apps?.filter((x) => x.lastUsedAt); + const usedApps = apps?.filter((x) => x.lastSettledTransaction); ... - {app.lastUsedAt ? dayjs(app.lastUsedAt).fromNow() : "never"} + {app.lastSettledTransaction + ? dayjs(app.lastSettledTransaction).fromNow() + : "never"}Also applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx` around lines 16 - 18, The widget is filtering and rendering by connection activity (`lastUsedAt`) but the query sorts by `last_settled_transaction`; update the filtering and display to use the settled-transaction timestamp instead. Specifically, change the filter that defines usedApps (currently using apps?.filter((x) => x.lastUsedAt)) to check the settled timestamp property (e.g., x.lastSettledTransactionAt or the exact field returned for "last_settled_transaction"), and update any rendering logic that shows relative time (currently reading x.lastUsedAt) to read that settled timestamp property with optional chaining. Ensure you reference the same property name returned by useApps so filtering and rendering are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/home/widgets/LatestUsedAppsWidget.tsx`:
- Around line 16-18: The widget is filtering and rendering by connection
activity (`lastUsedAt`) but the query sorts by `last_settled_transaction`;
update the filtering and display to use the settled-transaction timestamp
instead. Specifically, change the filter that defines usedApps (currently using
apps?.filter((x) => x.lastUsedAt)) to check the settled timestamp property
(e.g., x.lastSettledTransactionAt or the exact field returned for
"last_settled_transaction"), and update any rendering logic that shows relative
time (currently reading x.lastUsedAt) to read that settled timestamp property
with optional chaining. Ensure you reference the same property name returned by
useApps so filtering and rendering are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a1d308d-ac3c-4bfa-9e58-2d543f9bb743
📒 Files selected for processing (4)
api/api.gofrontend/src/components/home/widgets/LatestUsedAppsWidget.tsxfrontend/src/hooks/useApps.tsnip47/event_handler.go
✅ Files skipped from review due to trivial changes (1)
- nip47/event_handler.go
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/hooks/useApps.ts
- api/api.go
|
Updated! Now using |
3 participants
Description
Fixes #1998
Sorts "Recently Used Apps" by settled transactions instead of
LastUsedAt, so apps that actively move value appear at the top.Changes
LEFT JOINtransactionswherestate = 'SETTLED'and sort byMAX(transactions.created_at).Notes
LastUsedAtstill tracks connection activity (useful for debugging).Summary by CodeRabbit