fix(org): prevent logo 400s in dropdown#1869
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds client-side detection and throttled recovery for broken organization logos, normalizes and re-signs storage image paths with cache/force-refresh semantics, extends the organization store with a logo refresh action and storage-path field, and updates a Supabase cron to avoid deleting linked org logos. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as DropdownOrganization
participant Store as OrganizationStore
participant Service as StorageService
participant Supabase as Supabase
participant Cron as CleanupCron
UI->>UI: <img> onerror -> compute failingLogoKey
alt key not recently refreshed & not inflight
UI->>Store: refreshOrganizationLogos()
activate Store
Store->>Service: resolveImagePath(path)
activate Service
Service->>Supabase: storage.from('images').createSignedUrl(extractedPath, ttl)
activate Supabase
Supabase-->>Service: signed_url
deactivate Supabase
Service-->>Store: refreshed_signed_url
deactivate Service
Store->>Store: update org.logo & logo_storage_path
Store-->>UI: updated organizations
deactivate Store
end
Note over Cron,Supabase: Cron lists org logo folder and calls getStaleOrgLogoPaths
Cron->>Supabase: remove stale logo objects
Supabase-->>Cron: deletion result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cron-clean-orphan-images.unit.test.ts (1)
5-50: Consider usingit.concurrent()for parallel test execution.These tests are stateless pure function tests with no shared mocks or side effects. Per coding guidelines, they can safely run in parallel.
♻️ Suggested change
- it('preserves the linked org logo file and removes only stale files', () => { + it.concurrent('preserves the linked org logo file and removes only stale files', () => {Apply the same change to the other two test cases.
As per coding guidelines: "Design tests with parallel execution in mind; use
it.concurrent()instead ofit()when possible to run tests in parallel within the same file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cron-clean-orphan-images.unit.test.ts` around lines 5 - 50, Replace the three synchronous tests that call getStaleOrgLogoPaths with parallel tests by changing their it(...) declarations to it.concurrent(...); specifically update the tests containing the descriptions "preserves the linked org logo file and removes only stale files", "treats signed logo URLs as the linked file path", and "deletes all org logo files when no linked logo remains in the org row" so they use it.concurrent, leaving the test bodies and assertions (and the getStaleOrgLogoPaths calls) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cron-clean-orphan-images.unit.test.ts`:
- Around line 5-50: Replace the three synchronous tests that call
getStaleOrgLogoPaths with parallel tests by changing their it(...) declarations
to it.concurrent(...); specifically update the tests containing the descriptions
"preserves the linked org logo file and removes only stale files", "treats
signed logo URLs as the linked file path", and "deletes all org logo files when
no linked logo remains in the org row" so they use it.concurrent, leaving the
test bodies and assertions (and the getStaleOrgLogoPaths calls) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5684e1f-aa05-4331-9941-39cf7486b118
📒 Files selected for processing (6)
src/components/dashboard/DropdownOrganization.vuesrc/services/storage.tssrc/stores/organization.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tstests/cron-clean-orphan-images.unit.test.tstests/storage-signed-image-url.unit.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba3f650266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/dashboard/DropdownOrganization.vue`:
- Line 28: The deduplication Set refreshedBrokenLogoUrls currently stores signed
URLs (const refreshedBrokenLogoUrls) which differ after calling
createSignedImageUrl(..., { forceRefresh: true }) causing retries to bypass the
check; update refreshOrganizationLogos() to deduplicate by a stable key (e.g.,
organization id or the storage path from org.logoPath) instead of the signed
URL, and add a retry limit per organization (e.g., a Map<string, number>
trackRetries keyed by org.id or storage path) so that after N failed attempts
you stop calling createSignedImageUrl and mark the logo as permanently broken;
adjust any template bindings that pass the logo URL to pass org.id or logoPath
where needed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5df5370-2d06-4fa8-bebc-5e2c0240602a
📒 Files selected for processing (2)
src/components/dashboard/DropdownOrganization.vuetests/cron-clean-orphan-images.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/cron-clean-orphan-images.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49aeff0a0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Applied fixes for the two open bot threads: stable logo refresh dedupe keyed by storage path (org id fallback) plus URL-decoding in cron logo cleanup. Added unit coverage for encoded logo filenames and ran Commit: 786c6c3 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 786c6c3bbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/dashboard/DropdownOrganization.vue (1)
137-146:⚠️ Potential issue | 🟠 MajorUse
gidas the fallback refresh key, not the signed URL.When
logo_storage_pathis absent, falling back toorg.logomakes the dedupe key change on every re-sign. A permanently broken logo can then re-enter the refresh loop as each new signed URL fails, which partially reopens the earlier retry-loop issue.🛠️ Proposed fix
function getLogoRefreshKey(org?: Organization | null) { if (!org) return '' const storagePath = org.logo_storage_path?.trim() if (storagePath) return storagePath - const logo = org.logo?.trim() - if (logo) - return logo - return org.gid?.trim() ?? '' + const gid = org.gid?.trim() + if (gid) + return gid + return org.logo?.trim() ?? '' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/DropdownOrganization.vue` around lines 137 - 146, getLogoRefreshKey currently falls back to org.logo (a signed URL) which changes on every re-sign; change the fallback order so that after checking org.logo_storage_path you use org.gid as the stable refresh key before considering org.logo. Update the function getLogoRefreshKey to return storagePath if present, then return org.gid?.trim() if present, then only fall back to org.logo?.trim(), and finally '' if none exist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/triggers/cron_clean_orphan_images.ts`:
- Line 19: The current return statement applies .replace(/^images\//, '') before
stripping leading slashes, so paths like "/images/..." normalize incorrectly;
change the order to remove leading slashes first and then strip the "images/"
prefix (i.e. update the expression
decodePathSegment(pathWithoutQuery).replace(/^images\//, '').replace(/^\/+/, '')
to decodePathSegment(pathWithoutQuery).replace(/^\/+/, '').replace(/^images\//,
'')), ensuring the code that normalizes object keys (the decodePathSegment(...)
chain) strips leading slashes first then removes the images/ prefix.
---
Duplicate comments:
In `@src/components/dashboard/DropdownOrganization.vue`:
- Around line 137-146: getLogoRefreshKey currently falls back to org.logo (a
signed URL) which changes on every re-sign; change the fallback order so that
after checking org.logo_storage_path you use org.gid as the stable refresh key
before considering org.logo. Update the function getLogoRefreshKey to return
storagePath if present, then return org.gid?.trim() if present, then only fall
back to org.logo?.trim(), and finally '' if none exist.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1d72607-ae70-43e8-a96e-0586d3021218
📒 Files selected for processing (3)
src/components/dashboard/DropdownOrganization.vuesupabase/functions/_backend/triggers/cron_clean_orphan_images.tstests/cron-clean-orphan-images.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/cron-clean-orphan-images.unit.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/dashboard/DropdownOrganization.vue (1)
29-29:refreshedBrokenLogoKeysgrows without bound for the component lifetime.The Set accumulates keys for every failed logo but is never cleared until the component unmounts. For users with many organizations or multiple logo errors, this could grow during a long session. Consider clearing it when refresh succeeds or periodically.
♻️ Optional: Clear keys after successful refresh
async function refreshOrganizationLogosIfNeeded(force = false) { if (isRefreshingBrokenLogos.value) return if (!force && Date.now() - lastOrganizationLogoRefreshAt.value < ORGANIZATION_LOGO_REFRESH_INTERVAL_MS) return isRefreshingBrokenLogos.value = true try { await organizationStore.refreshOrganizationLogos() lastOrganizationLogoRefreshAt.value = Date.now() + refreshedBrokenLogoKeys.clear() } catch (error) { console.error('Failed to refresh organization logos', error) } finally { isRefreshingBrokenLogos.value = false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/DropdownOrganization.vue` at line 29, refreshedBrokenLogoKeys is a Set that grows for the entire component lifetime; modify the logo-refresh flow to remove keys from refreshedBrokenLogoKeys when a refresh succeeds (e.g., call refreshedBrokenLogoKeys.delete(key) or refreshedBrokenLogoKeys.clear() inside the success branch of your refresh handler such as the method that retries/fetches organization logos) and/or add a periodic cleanup (setInterval) that prunes the Set and is cleared on component unmount; ensure you also clear any interval/timers during unmount to avoid leaks.src/services/storage.ts (1)
68-71: Cache max-age calculation always returnsSIGNED_URL_CACHE_MAX_AGE_MS.Since
SIGNED_URL_TTL_SECONDSis 7 days (604800 seconds → 604800000 ms) andSIGNED_URL_CACHE_MAX_AGE_MSis 15 minutes (900000 ms), theMath.minalways evaluates to 15 minutes. This is functionally correct (conservative caching), but the calculation could be simplified for clarity.♻️ Simplify by removing redundant Math.min
signedUrlCache.set(cacheKey, { url: data.signedUrl, - expiresAt: Date.now() + Math.min(SIGNED_URL_TTL_SECONDS * 1000, SIGNED_URL_CACHE_MAX_AGE_MS), + expiresAt: Date.now() + SIGNED_URL_CACHE_MAX_AGE_MS, })Or keep it if you anticipate reducing
SIGNED_URL_TTL_SECONDSbelow 15 minutes in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/storage.ts` around lines 68 - 71, The cache max-age computation in the signed URL cache entry is redundant because SIGNED_URL_TTL_SECONDS (in ms) is larger than SIGNED_URL_CACHE_MAX_AGE_MS; update the expiresAt assignment in the signedUrlCache.set call (referencing signedUrlCache, cacheKey, SIGNED_URL_TTL_SECONDS, and SIGNED_URL_CACHE_MAX_AGE_MS) to use the simpler value SIGNED_URL_CACHE_MAX_AGE_MS (or a precomputed ttlMs capped by SIGNED_URL_CACHE_MAX_AGE_MS if you prefer explicitness), removing the unnecessary Math.min call for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/dashboard/DropdownOrganization.vue`:
- Line 29: refreshedBrokenLogoKeys is a Set that grows for the entire component
lifetime; modify the logo-refresh flow to remove keys from
refreshedBrokenLogoKeys when a refresh succeeds (e.g., call
refreshedBrokenLogoKeys.delete(key) or refreshedBrokenLogoKeys.clear() inside
the success branch of your refresh handler such as the method that
retries/fetches organization logos) and/or add a periodic cleanup (setInterval)
that prunes the Set and is cleared on component unmount; ensure you also clear
any interval/timers during unmount to avoid leaks.
In `@src/services/storage.ts`:
- Around line 68-71: The cache max-age computation in the signed URL cache entry
is redundant because SIGNED_URL_TTL_SECONDS (in ms) is larger than
SIGNED_URL_CACHE_MAX_AGE_MS; update the expiresAt assignment in the
signedUrlCache.set call (referencing signedUrlCache, cacheKey,
SIGNED_URL_TTL_SECONDS, and SIGNED_URL_CACHE_MAX_AGE_MS) to use the simpler
value SIGNED_URL_CACHE_MAX_AGE_MS (or a precomputed ttlMs capped by
SIGNED_URL_CACHE_MAX_AGE_MS if you prefer explicitness), removing the
unnecessary Math.min call for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bea3b5b2-4e45-4741-95c8-99796f277ae4
📒 Files selected for processing (6)
src/components/dashboard/DropdownOrganization.vuesrc/services/storage.tssrc/stores/organization.tssupabase/functions/_backend/triggers/cron_clean_orphan_images.tstests/cron-clean-orphan-images.unit.test.tstests/organization-store-delete.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/cron-clean-orphan-images.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bc92d3a4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/dashboard/DropdownOrganization.vue (1)
153-161: Minor consideration: Concurrent error handling could skip key registration.When
isRefreshingBrokenLogos.valueis true, the function returns early without adding the key torefreshedBrokenLogoKeys. If multiple org logos error concurrently during a refresh, only the first error's key gets registered. Since the store'srefreshOrganizationLogos()refreshes all orgs anyway, this typically works out—but if a second org's new signed URL also fails, it could trigger one additional refresh cycle before being deduplicated.This is an edge case with minimal impact given the throttle protections. No change required, just documenting the behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/DropdownOrganization.vue` around lines 153 - 161, The function refreshBrokenOrganizationLogo may return early when isRefreshingBrokenLogos.value is true without adding the organization's refreshKey to refreshedBrokenLogoKeys, allowing a concurrent failing logo to trigger an extra refresh; update refreshBrokenOrganizationLogo so that after computing refreshKey (and validating failedLogo/refreshKey) you add refreshKey to refreshedBrokenLogoKeys before returning when isRefreshingBrokenLogos.value is true (or otherwise ensure refreshedBrokenLogoKeys.add(refreshKey) runs even when you bail due to isRefreshingBrokenLogos), keeping the existing checks refreshedBrokenLogoKeys.has(refreshKey) and the call to refreshOrganizationLogosIfNeeded(true) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/dashboard/DropdownOrganization.vue`:
- Around line 153-161: The function refreshBrokenOrganizationLogo may return
early when isRefreshingBrokenLogos.value is true without adding the
organization's refreshKey to refreshedBrokenLogoKeys, allowing a concurrent
failing logo to trigger an extra refresh; update refreshBrokenOrganizationLogo
so that after computing refreshKey (and validating failedLogo/refreshKey) you
add refreshKey to refreshedBrokenLogoKeys before returning when
isRefreshingBrokenLogos.value is true (or otherwise ensure
refreshedBrokenLogoKeys.add(refreshKey) runs even when you bail due to
isRefreshingBrokenLogos), keeping the existing checks
refreshedBrokenLogoKeys.has(refreshKey) and the call to
refreshOrganizationLogosIfNeeded(true) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 661eff8e-1a1a-4769-b2fb-c4e85ae3fecb
📒 Files selected for processing (1)
src/components/dashboard/DropdownOrganization.vue
|
1 participant
Summary (AI generated)
org/{org_id}/logo/*folder is not treated as an app folder and the current org logo is not deletedMotivation (AI generated)
Organization logos were disappearing after some time and the dropdown started returning
400from Supabase storage signing requests. The immediate client-side issue was signed URL expiry, but the underlying data loss came from the orphan image cleanup job deleting thelogofolder because it was misclassified as an app id.Business Impact (AI generated)
This restores reliability for organization branding in the dashboard, prevents avoidable broken UI in org switching, and stops background cleanup from deleting active customer assets. That reduces support load and avoids trust erosion caused by organizations appearing to lose their logos over time.
Test Plan (AI generated)
bunx vitest run tests/cron-clean-orphan-images.unit.test.ts tests/storage-signed-image-url.unit.test.ts tests/organization-store-delete.unit.test.tsbunx eslint src/services/storage.ts src/stores/organization.ts src/components/dashboard/DropdownOrganization.vue supabase/functions/_backend/triggers/cron_clean_orphan_images.ts tests/cron-clean-orphan-images.unit.test.ts tests/storage-signed-image-url.unit.test.tsbun typecheckGenerated with AI
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests