fix: print label uses wrong tenant on mobile#1480
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Summary by CodeRabbit
WalkthroughLayout mount now calls ChangesCollection Initialization and Auth Tenant
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Security Recommendations
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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}
|
Thanks for such a thorough investigate! I think adding a voided call to the collection load function somewhere in layout probably makes most sense as it will fix other bugs, I also believe (tho im far from certain) that using the selectedCollection.value instead of prefs has some advantage. I think that I switched base api because using the selectedCollection.value caused an infinite loop. In the case we do add a call to load the collection in the layout then switching will be unecesary? |
LabelMaker built the label URL from useCollections().selectedId, which is only populated when Collection/Selector.vue mounts. On mobile the sidebar (and the Selector inside it) lives in a Sheet that is conditionally rendered, so until the user opens the hamburger menu, selectedId stays null and the ?tenant= query param is omitted from the label URL. The backend then falls back to the user's default group, returning the wrong tenant's item — and because the URL is byte-identical between visits, the browser serves a cached image without firing a request. Switch to useViewPreferences().value.collectionId, matching the same pattern that 24e7995 applied to base-api.ts for attachments.
lhns
force-pushed
the
fix/labelmaker-mobile-tenant
branch
from
9bd309d to
6e6d1a2
Compare
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 `@frontend/lib/api/base/base-api.ts`:
- Around line 80-84: BaseAPI.authURL() can miss tenant because ImageDialog
generates URLs once on open before useCollections().selectedId is populated; fix
by making URL generation reactive or waiting for collections: in ImageDialog.vue
modify the dialog open handler (the callback that currently builds URLs) to
either await useCollections().load() or watch useCollections().selectedId and
regenerate the attachment URLs when selectedId becomes non-null, so the tenant
query param is included reliably; keep BaseAPI.authURL() as the single source of
truth but ensure ImageDialog uses the latest useCollections().selectedId value
(or re-computes the URLs from BaseAPI.authURL()) instead of snapshotting a null
selectedId.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b047b0b-536d-419e-9517-ec0ce74a9a41
📒 Files selected for processing (2)
frontend/layouts/default.vuefrontend/lib/api/base/base-api.ts
| const { selectedId } = useCollections(); | ||
| const params: Record<string, string> = { access_token: this.attachmentToken }; | ||
|
|
||
| const prefs = useViewPreferences(); | ||
| if (prefs.value.collectionId) { | ||
| params.tenant = prefs.value.collectionId; | ||
| if (selectedId.value) { | ||
| params.tenant = selectedId.value; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify useCollections().load() error handling and selectedId initialization
# Check load() implementation for error cases
ast-grep --pattern $'export const useCollections = () => {
$$$
const load = async () => {
$$$
}
$$$
}'
# Find all authURL call sites to verify they handle null tenant gracefully
rg -n 'authURL\(' frontend/ --type=ts --type=vue -C3
# Check if any code validates selectedId before calling authURL
rg -n 'selectedId.*authURL|authURL.*selectedId' frontend/ --type=ts --type=vue -C5Repository: sysadminsmedia/homebox
Length of output: 8254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## base-api.ts authURL implementation (lines ~50-130)"
nl -ba frontend/lib/api/base/base-api.ts | sed -n '50,140p'
echo
echo "## authURL call sites (Vue/TS/TSX/JS) - show a few context lines"
rg -n "authURL\(" frontend --glob '*.{vue,ts,tsx,js,jsx}' -S -C2
echo
echo "## Any guarding/validation around selectedId before authURL"
rg -n "selectedId(\.value)?[^\n]*\n[^\n]*authURL\(" frontend --glob '*.{vue,ts,tsx,js,jsx}' -S || true
rg -n "if\s*\(.*selectedId.*\)" frontend --glob '*.{vue,ts,tsx,js,jsx}' -S | head -n 80
echo
echo "## Where tenant is set/used in base-api.ts"
rg -n "tenant" frontend/lib/api/base/base-api.ts -S || trueRepository: sysadminsmedia/homebox
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/lib/api/base/base-api.ts"
echo "## base-api.ts (${FILE}) authURL implementation (lines ~60-140)"
awk 'NR>=60 && NR<=140 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "## authURL call sites (frontend) with context"
rg -n "authURL\(" frontend --glob '*.{vue,ts,tsx,js,jsx}' -S -C2
echo
echo "## Any obvious guard patterns around selectedId before authURL"
rg -n "selectedId(\.value)?[^\n]*" frontend --glob '*.{vue,ts,tsx,js,jsx}' -S -C0 | head -n 120
echo
echo "## Search for base-api tenant usage patterns"
rg -n "access_token|tenant" "$FILE" -SRepository: sysadminsmedia/homebox
Length of output: 8706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Find where useCollections().load() is called"
rg -n "useCollections\(\)\.load\(" frontend --glob '*.{ts,vue}' -S
echo
echo "## Find where useCollections().load (without immediate call) is referenced"
rg -n "useCollections\(\).*load" frontend --glob '*.{ts,vue}' -S
echo
echo "## List all uses of useCollections in frontend (for context)"
rg -n "useCollections\(" frontend --glob '*.{ts,vue}' -SRepository: sysadminsmedia/homebox
Length of output: 1167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# show the default layout call around the reported line
echo "## frontend/layouts/default.vue around useCollections().load()"
awk 'NR>=470 && NR<=540 {printf "%d\t%s\n", NR, $0}' frontend/layouts/default.vue
echo
echo "## frontend/pages/item/[id]/index.vue around api.authURL() usage"
awk 'NR>=120 && NR<=190 {printf "%d\t%s\n", NR, $0}' frontend/pages/item/[id]/index.vue
echo
echo "## frontend/pages/location/[id]/index/index.vue around api.authURL() usage"
awk 'NR>=80 && NR<=140 {printf "%d\t%s\n", NR, $0}' frontend/pages/location/[id]/index/index.vue
echo
echo "## frontend/components/Item/ImageDialog.vue around api.authURL() usage"
awk 'NR>=1 && NR<=120 {printf "%d\t%s\n", NR, $0}' frontend/components/Item/ImageDialog.vueRepository: sysadminsmedia/homebox
Length of output: 10373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/composables/use-collections.ts"
echo "## frontend/composables/use-collections.ts (lines 1-140)"
awk 'NR>=1 && NR<=140 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "## use-collections.ts (lines 140-240 if any)"
awk 'NR>=140 && NR<=240 {printf "%d\t%s\n", NR, $0}' "$FILE" || trueRepository: sysadminsmedia/homebox
Length of output: 4558
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="frontend/lib/api/base/base-api.ts"
echo "## base-api.ts imports (lines 1-60)"
awk 'NR>=1 && NR<=60 {printf "%d\t%s\n", NR, $0}' "$FILE"Repository: sysadminsmedia/homebox
Length of output: 1917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Find where BaseAPI is constructed (attachmentToken usage)"
rg -n "new BaseAPI\(" frontend --glob '*.{ts,js,vue}' -S
echo
echo "## Find constructor param name usage 'attachmentToken' in api code"
rg -n "attachmentToken" frontend --glob '*.{ts,js,vue}' -S
echo
echo "## If there is a useUserApi() that sets attachmentToken, show it"
rg -n "function useUserApi|useUserApi\(" frontend --glob '*.{ts,js}' -SRepository: sysadminsmedia/homebox
Length of output: 127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Search for useUserApi implementation"
rg -n "export (const|function) useUserApi|useUserApi\s*=" frontend --glob '*.{ts,js}' -S
echo
echo "## Search for attachmentToken assignments/usages"
rg -n "\battachmentToken\b" frontend --glob '*.{ts,js,vue}' -S
echo
echo "## Search for .authURL() method usage only (to see how api is obtained)"
rg -n "\.authURL\(" frontend --glob '*.{ts,vue,js}' -S
echo
echo "## Search for BaseAPI authURL wrappers"
rg -n "authURL\s*:\s*|authURL\(" frontend/lib/api --glob '*.{ts}' -SRepository: sysadminsmedia/homebox
Length of output: 3812
Ensure tenant is reliably included (and doesn’t snapshot as null) when building attachment authURLs
BaseAPI.authURL() only adds tenant when useCollections().selectedId.value is truthy (frontend/lib/api/base/base-api.ts:78-88). selectedId starts as null and is only populated by useCollections().load() (frontend/composables/use-collections.ts:11-20, load()), which runs in frontend/layouts/default.vue via onMounted() and is skipped on / (default.vue:506-508, use-collections.ts:17-20). If collections load fails, selectedId remains null (error is caught/logged; finally only flips refreshing).
In frontend/components/Item/ImageDialog.vue, URLs are generated once inside the dialog open callback (ImageDialog.vue:32-47) with no guard/waiting for selectedId, so opening the dialog before collections finish loading (or after a failure) will omit tenant and can cause the backend fallback collection behavior.
Security: treat the tenant query param as untrusted—server-side authorization must be derived from access_token (and validate/ignore tenant) to prevent cross-tenant attachment access.
🤖 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 `@frontend/lib/api/base/base-api.ts` around lines 80 - 84, BaseAPI.authURL()
can miss tenant because ImageDialog generates URLs once on open before
useCollections().selectedId is populated; fix by making URL generation reactive
or waiting for collections: in ImageDialog.vue modify the dialog open handler
(the callback that currently builds URLs) to either await
useCollections().load() or watch useCollections().selectedId and regenerate the
attachment URLs when selectedId becomes non-null, so the tenant query param is
included reliably; keep BaseAPI.authURL() as the single source of truth but
ensure ImageDialog uses the latest useCollections().selectedId value (or
re-computes the URLs from BaseAPI.authURL()) instead of snapshotting a null
selectedId.
6e6d1a2 to
6dfb7ba
Compare
|
Hey! I'm not sure if I understood it correctly but I tried to play around with that approach. In the end I wasn't quite sure about its correctness, so I left it like this for now. |
2 participants
What type of PR is this?
What this PR does / why we need it:
On mobile, opening Print Label on an item from a non-default collection rendered the label for a different item - the item that happened to share the same
assetIdin the default collection. Switching the mobile browser to "desktop mode" worked around it; on desktop it always worked.Root cause
LabelMaker.vue#getLabelUrlread the active tenant fromuseCollections().selectedIdand appended it as a?tenant=query param.selectedIdis a module-level ref that is only populated whenCollection/Selector.vuerunsuseCollections().load()in itsonMountedhook.The Selector lives inside the layout's
<Sidebar>. On desktop the sidebar is a regular always-mounted<div>. On mobile (components/ui/sidebar/Sidebar.vuelines 29–43) it's rendered inside a<Sheet>(Reka UIDialogPortal/DialogContent), which is conditionally rendered - its slot is not mounted until the user opens the hamburger menu. Until that happens,selectedId.valueisnull, so:?tenant=….mwTenantmiddleware falls back touser.DefaultGroupID.HandleGetAssetLabelcallsQueryByAssetID(auth, defaultGroupID, assetID). Because asset ids are tenant-scoped sequential numbers (e.g.000-001), the lookup happily returns a different item with the same id from the default collection.This also explains the "no network request on mobile" symptom: with the tenant param missing, the URL on mobile is byte-identical to the cached URL from a previous visit to the default collection's matching asset id, so the browser serves it from cache without firing a request. On desktop the URL includes a unique tenant uuid, so it never collides with the cache.
Fix
frontend/components/global/LabelMaker.vue- read the active tenant fromuseViewPreferences().value.collectionIdinstead ofuseCollections().selectedId. Preferences are persisted viauseLocalStorageand updated byuseCollections.set()/load()whenever the user switches collection, so they don't depend on the Selector ever being mounted.This is the same fix @tonyaellie applied to attachment URLs in
lib/api/base/base-api.tsin24e79952("fix: use prefs instead of use collection for attachments").LabelMaker.vuewas missed by that earlier pass.Which issue(s) this PR fixes:
none
Special notes:
I audited every consumer of
useCollections(). Two other files have the same root cause but a different user-visible symptom - both early-return whenselectedCollection.valueis null:frontend/pages/collection/index.vue(members page) -loadMembers()(line 90),isActionDisabled(line 87),handleLeaveCollection(line 123).frontend/pages/collection/index/settings.vue-loadSettings()(line 34),save()(line 94).If a user deep-links / bookmarks
/collection/membersor/collection/settingson mobile and lands on it without ever opening the sidebar, they'll see an empty members list or a "Select a collection" empty state. Opening and closing the sidebar mounts the Selector once, populatesselectedId, and the page works. The underlying API calls would actually succeed becausebase-api.tsalready reads from prefs.Two ways to fix these, both reasonable - happy to do either as a follow-up:
selectedCollection.valuechecks/reads in those two files to useprefs.value.collectionId(and re-derive the collection fromcollections.valuewhen the display name is needed).void useCollections().load()once inlayouts/default.vue'sonMounted.load()already self-guards against duplicate / concurrent calls (seeuse-collections.ts:22-25), so this is safe and would populateselectedIdfor every authenticated page, fixing the latent collection-page issue and any future component that readsselectedIddirectly.A couple of other call sites build
/qrcode?data=…URLs without a tenant param (components/global/PageQRCode.vue,pages/reports/label-generator.vue), but the/qrcodeendpoint just renders the supplieddatastring into a QR image, so the rendered output doesn't depend on the active tenant - no visible bug there.Testing
000-001present in two collections, open the non-default collection's000-001item on a mobile viewport (or hard-reload mobile after clearing cache so the previous bad cache entry is gone)./api/v1/labelmaker/asset/000-001?print=false&tenant=<non-default-uuid>and the rendered image shows the correct item's name and parent location.assetId(LabelMaker is invoked withtype="item"/id=item.id) and a location detail page (type="location") to confirm the other branches ofgetLabelUrl()still work.Disclaimer: I have written this MR with the help of Claude Code