feat: increment opened count when opening link from local server (#255)#381
Conversation
📝 WalkthroughWalkthroughThe PR introduces link open tracking by replacing a static link with a button that opens the URL in a new tab and posts to a new API endpoint to increment an open counter. Both client-side and server-side components are added to support this feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Button as Browser Button
participant Handler as handleOpenLink Handler
participant URLTab as New URL Tab
participant APIServer as Server API
participant ViewModel as ViewModel/DB
User->>Button: Click "Open Link" button
Button->>Handler: Call handleOpenLink(id, url)
Handler->>URLTab: window.open(url, '_blank')
Handler->>APIServer: POST /api/links/increment-count?id={id}
APIServer->>ViewModel: incrementOpenedCount(id)
ViewModel-->>APIServer: Success/Error
APIServer-->>Handler: 200 OK / 400 / 500 response
Handler->>Handler: Log response (error if needed)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (1)
app/src/main/assets/index.html (1)
696-709:handleOpenLinklogic is reasonable; minor UX note on the refresh delay.The function correctly opens the link first (so the user isn't blocked) and then fires the increment in the background. The error is silently caught, which is appropriate for a non-critical tracking call.
The
setTimeout(loadLinks, 1000)on Line 705 is a fire-and-forget heuristic. Consider either:
- Awaiting the fetch response and calling
loadLinks()immediately after (the server should have committed by the time it responds), or- Skipping the reload entirely and optimistically updating
link.openedCountin the localallLinksarray + re-rendering.Both would give a snappier and more reliable UX than the 1-second arbitrary delay.
♻️ Suggested: refresh immediately after successful POST
async function handleOpenLink(id, url) { // Open link in new tab window.open(url, '_blank'); // Increment count in background try { - await fetch(`/api/links/increment-count?id=${id}`, { method: 'POST' }); - // Optional: refresh links to see updated count - setTimeout(loadLinks, 1000); + const response = await fetch(`/api/links/increment-count?id=${id}`, { method: 'POST' }); + if (response.ok) { + loadLinks(); + } } catch (error) { console.error('Error incrementing count:', error); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/assets/index.html` around lines 696 - 709, The current handleOpenLink function uses setTimeout(loadLinks, 1000) to refresh counts; replace this heuristic by either awaiting the POST and calling loadLinks() immediately after a successful response or perform an optimistic local update of the allLinks array and re-render instead of the timeout. Specifically, in handleOpenLink: after the fetch(`/api/links/increment-count?id=${id}`, { method: 'POST' }) call, either await the response and invoke loadLinks() on success, or locate the allLinks state/variable and increment the matching link.openedCount (by id) then call the same render/update function to reflect the change instantly; remove the setTimeout call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/assets/index.html`:
- Around line 680-687: The onclick interpolation allows single-quote breakout
because escapeHtml does not escape single quotes; remove the inline onclick on
the button and instead add data attributes (e.g., data-link-id and data-link-url
using escapeHtml for the URL) and then attach a delegated click handler in
DOMContentLoaded that locates the clicked element (or closest button) and calls
handleOpenLink(parseInt(btn.dataset.linkId,10), btn.dataset.linkUrl);
alternatively, if you must keep inline JS, ensure escapeHtml is extended to
escape single quotes before interpolating into the single-quoted onclick, but
preferred is the data-* + addEventListener approach to eliminate inline JS
entirely.
In
`@app/src/main/java/com/yogeshpaliyal/deepr/server/LocalServerRepositoryImpl.kt`:
- Around line 331-348: The POST handler currently calls
accountViewModel.incrementOpenedCount(id) which internally does
viewModelScope.launch(Dispatchers.IO) (fire-and-forget), so the route responds
200 before DB work completes; change the approach so the route awaits
completion: add or call a suspend function that performs the DB update (e.g.,
create incrementOpenedCountSuspend or expose a suspend DAO method used by
incrementOpenedCount) and invoke it with call.application.coroutineScope or
using runCatching { withContext(Dispatchers.IO) { /* suspend update */ } } to
await completion before responding; additionally check that the link exists (use
the DAO's find/get method) and return 404 if not found, or map the update result
to 200 vs 404 accordingly instead of unconditionally returning OK.
---
Nitpick comments:
In `@app/src/main/assets/index.html`:
- Around line 696-709: The current handleOpenLink function uses
setTimeout(loadLinks, 1000) to refresh counts; replace this heuristic by either
awaiting the POST and calling loadLinks() immediately after a successful
response or perform an optimistic local update of the allLinks array and
re-render instead of the timeout. Specifically, in handleOpenLink: after the
fetch(`/api/links/increment-count?id=${id}`, { method: 'POST' }) call, either
await the response and invoke loadLinks() on success, or locate the allLinks
state/variable and increment the matching link.openedCount (by id) then call the
same render/update function to reflect the change instantly; remove the
setTimeout call.
| <button | ||
| onclick="handleOpenLink(${link.id}, '${escapeHtml(link.link)}')" | ||
| class="w-full bg-gradient-to-r from-primary-500 to-secondary-500 hover:from-primary-600 hover:to-secondary-600 text-white py-2 px-4 rounded-lg text-sm font-medium transition-all duration-200 transform hover:scale-105 flex items-center justify-center gap-2"> | ||
| <svg class="w-4 h-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M10 6H6a2 2 0 00-2 2v10a2 2 0 002 2h10a2 2 0 002-2v-4M14 4h6m0 0v6m0-6L10 14"></path> | ||
| </svg> | ||
| Open Link | ||
| </a> | ||
| </button> |
There was a problem hiding this comment.
Stored XSS via single-quote breakout in onclick attribute.
The escapeHtml() function (Line 890) encodes <, >, &, and " but does not escape single quotes ('). Since the URL is interpolated inside single-quoted string delimiters in the onclick attribute:
onclick="handleOpenLink(${link.id}, '${escapeHtml(link.link)}')"A malicious URL containing a single quote (e.g., https://x.com/');alert(document.cookie);//) will break out of the string literal and execute arbitrary JavaScript when the card is rendered. This is a stored XSS — any user who can add a link via the API can inject script that runs for every viewer.
🔒 Proposed fix: use a data attribute and attach the handler programmatically, or escape single quotes
Option 1 (minimal fix): Escape single quotes in the interpolation context:
- <button
- onclick="handleOpenLink(${link.id}, '${escapeHtml(link.link)}')"
+ <button
+ onclick="handleOpenLink(${link.id}, '${escapeHtml(link.link).replace(/'/g, ''')}')"Option 2 (preferred): Use data-* attributes and bind handlers via addEventListener to avoid inline JS entirely. Store the id and url in data-link-id and data-link-url attributes (which are HTML-escaped by escapeHtml), then in DOMContentLoaded use event delegation:
document.getElementById('linksContainer').addEventListener('click', function(e) {
const btn = e.target.closest('[data-link-id]');
if (btn) {
handleOpenLink(parseInt(btn.dataset.linkId, 10), btn.dataset.linkUrl);
}
});This avoids any inline JS injection surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/assets/index.html` around lines 680 - 687, The onclick
interpolation allows single-quote breakout because escapeHtml does not escape
single quotes; remove the inline onclick on the button and instead add data
attributes (e.g., data-link-id and data-link-url using escapeHtml for the URL)
and then attach a delegated click handler in DOMContentLoaded that locates the
clicked element (or closest button) and calls
handleOpenLink(parseInt(btn.dataset.linkId,10), btn.dataset.linkUrl);
alternatively, if you must keep inline JS, ensure escapeHtml is extended to
escape single quotes before interpolating into the single-quoted onclick, but
preferred is the data-* + addEventListener approach to eliminate inline JS
entirely.
|
|
||
| post("/api/links/increment-count") { | ||
| try { | ||
| val id = call.request.queryParameters["id"]?.toLongOrNull() | ||
| if (id != null) { | ||
| accountViewModel.incrementOpenedCount(id) | ||
| call.respond(HttpStatusCode.OK, SuccessResponse("Count incremented")) | ||
| } else { | ||
| call.respond(HttpStatusCode.BadRequest, ErrorResponse("Invalid link ID")) | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.e("LocalServer", "Error incrementing count", e) | ||
| call.respond( | ||
| HttpStatusCode.InternalServerError, | ||
| ErrorResponse("Error incrementing count: ${e.message}"), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find AccountViewModel file
fd -t f "AccountViewModel" --type kotlinRepository: yogeshpaliyal/Deepr
Length of output: 236
🏁 Script executed:
# Also try broader search in case naming differs
fd -t f -i "viewmodel" | grep -i accountRepository: yogeshpaliyal/Deepr
Length of output: 134
🏁 Script executed:
# Search for incrementOpenedCount in the entire codebase
rg -n "incrementOpenedCount" --type kotlinRepository: yogeshpaliyal/Deepr
Length of output: 966
🏁 Script executed:
# Check the AccountViewModel.incrementOpenedCount implementation
cat -n app/src/main/java/com/yogeshpaliyal/deepr/viewmodel/AccountViewModel.kt | sed -n '488,495p'Repository: yogeshpaliyal/Deepr
Length of output: 363
🏁 Script executed:
# Check the LinkRepositoryImpl.incrementOpenedCount implementation
cat -n app/src/main/java/com/yogeshpaliyal/deepr/data/LinkRepositoryImpl.kt | sed -n '219,235p'Repository: yogeshpaliyal/Deepr
Length of output: 674
🏁 Script executed:
# Also check the LinkRepository interface to understand the contract
cat -n app/src/main/java/com/yogeshpaliyal/deepr/data/LinkRepository.kt | sed -n '107,110p'Repository: yogeshpaliyal/Deepr
Length of output: 182
🏁 Script executed:
# Check how deeprQueries is used - likely SQLDelight generated code
rg -n "deeprQueries.incrementOpenedCount" --type-add 'kotlin:*.kt' -A 3 -B 3Repository: yogeshpaliyal/Deepr
Length of output: 737
🏁 Script executed:
# Check if there's any logging or observable that might show race condition
rg -n "insertDeeprOpenLog" --type-add 'kotlin:*.kt' -A 2 -B 2Repository: yogeshpaliyal/Deepr
Length of output: 2068
Critical race condition: endpoint returns 200 OK before database operation completes.
The issue is more severe than initial threading concern. accountViewModel.incrementOpenedCount(id) uses viewModelScope.launch(Dispatchers.IO) — a fire-and-forget coroutine that returns immediately without waiting for completion. The endpoint returns 200 OK to the client before the database increment and log operations have even started, creating a race condition and violating HTTP semantics.
The endpoint should either await the database operation (wrap in a suspend function and use a proper coroutine job) or use a blocking call if this is a local-only server.
Secondary issue: No validation that the given id corresponds to an existing link. Consider returning 404 if the ID doesn't exist, or ensure the implementation handles non-existent IDs gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/com/yogeshpaliyal/deepr/server/LocalServerRepositoryImpl.kt`
around lines 331 - 348, The POST handler currently calls
accountViewModel.incrementOpenedCount(id) which internally does
viewModelScope.launch(Dispatchers.IO) (fire-and-forget), so the route responds
200 before DB work completes; change the approach so the route awaits
completion: add or call a suspend function that performs the DB update (e.g.,
create incrementOpenedCountSuspend or expose a suspend DAO method used by
incrementOpenedCount) and invoke it with call.application.coroutineScope or
using runCatching { withContext(Dispatchers.IO) { /* suspend update */ } } to
await completion before responding; additionally check that the link exists (use
the DAO's find/get method) and return 404 if not found, or map the update result
to 200 vs 404 accordingly instead of unconditionally returning OK.
2 participants
This PR adds a new endpoint to the local server to increment the opened count of a link when it is opened through the web interface, ensuring consistency with the mobile app behavior.
Summary by CodeRabbit