Repository navigation
improvement(favicons): resolve platform tenants no provider can see, retry timeout-derived icons, no miss flash - #9497
Merged
Conversation
…and retry icons chosen after a timeout A tenant on a platform whose domain has no icon at either provider, even at the parent (AWS sign-in portals, Coupa, GitLab Dedicated, Databricks apps, Salesforce, Okta preview, UKG, Jama), and a self-hosted gitlab./jira./testrail. host, now end their candidate walk at the platform's own site, after their own host and parents so a tenant's custom icon still wins. A parent's icon reached only because a more specific lookup timed out is cached for a day instead of thirty, since the host's own icon may exist.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
 [Medium impact] The PR appears safe to merge. SummaryThe PR adds platform icon fallbacks, shorter caching for timeout-derived icons, and a brief client-side memory of failed icons.
Reviews (4) · Last reviewed commit: "chore(favicons): import resolver test mo..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 4/5
- In
resolve.server.ts, reusing a cached hit can lose its provisional status, keeping a fallback icon cached for 30 days instead of retrying after one day. Preserve that status when reading and reusing the hit. - In
hosts.ts, a cached parent miss can stopwalkbefore it tries the appended product candidate, sojira.company.commay miss theatlassian.comfallback. Continue past cached misses when candidates remain.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/favicon/resolve.server.ts">
<violation number="1" location="apps/sim/lib/favicon/resolve.server.ts:118">
P2: Preserve the provisional status when reading and reusing cached hits; otherwise a child lookup can cache a fallback icon for 30 days instead of retrying it after one day.</violation>
</file>
<file name="apps/sim/lib/favicon/hosts.ts">
<violation number="1" location="apps/sim/lib/favicon/hosts.ts:33">
P2: A cached parent miss can stop `walk` before this appended product candidate is tried, so `jira.company.com` may miss despite the `atlassian.com` fallback; continue past cached misses when later candidates remain.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
…d keep misses from flashing GitLab Dedicated instances live on gitlab-dedicated.com (and .systems pre-production); .site is their Pages domain, kept to match gitlab.io. Adds coupacloud.com, databricks.us, databricksapps.us, and testrail.io, each a suffix both providers miss at every level; drops entries the parent walk already resolves and the self-hosted sentry label, whose name collides with other products. A host with no icon is remembered client-side for the route's miss lifetime, so a remounted icon shows its fallback without an empty frame, and the cache key moves to v3 so stale misses for newly mapped platforms clear on deploy.
waleedlatif1
changed the title
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Turn on auto-fix | Re-trigger cubic
… past cached parent misses, skip hosting suffixes A cached hit now records whether it was definite or provisional, so a host reusing a parent's timeout-derived icon inherits the one-day lifetime instead of a month. A cached miss at a parent skips only the candidates that parent's own walk covered, so a product fallback derived from the original host is still tried. The self-hosted product label no longer applies on a Public Suffix List private (hosting) suffix. Adds the Databricks military suffix the repo already recognizes, and the client remembers a failed icon only as long as the route retries an outage.
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Platform fallback: a tenant on a platform whose domains have no icon at either provider, at any parent the walk reaches, now ends its candidate walk at the platform's own site:
awsapps.com)coupahost.com,coupacloud.com)gitlab-dedicated.com,.systems, and its Pages domain.site, matching howgitlab.ioalready resolves)databricksapps.com,databricksapps.us,databricksapps.mil,azuredatabricks.net,databricks.us)oktapreview.com)testrail.io)ultipro.com)jamacloud.com)Domain formats come from vendor docs or the Public Suffix List. Both providers were probed to miss every suffix, and every target resolves. Platforms whose parent already resolves (Salesforce, Databricks workspaces, Atlassian, ServiceNow, Workday) need no entry.
Self-hosted tools: hosts at
<product>.<company domain>(gitlab,jira,confluence,grafana,jenkins,testrail) fall back to the product's icon. This only applies when every other candidate missed, only on an exact first label (somygitlab.anddocs.gitlab.don't match), and never under a Public Suffix List private suffix, which is where hosting platforms register, sogitlab.azurewebsites.netdoesn't match.sentryis left out because the name collides with other products.Ordering and exclusions: the platform always comes after the host's own walk, so a tenant's custom icon still wins. Generic hosting domains (
herokuapp.com,azurewebsites.net,cloudfront.net) are deliberately excluded, and internal hosts still never leave the server.Timeouts: a parent's icon reached only because a more specific lookup timed out is cached for a day instead of thirty, since the host's own icon may exist. The cached value records whether it was definite or provisional, so a host that reuses it inherits the shorter lifetime. In the audit below, provider latency flipped 5 of 72 hosts between their own icon and their parent's across runs. The day fits RFC 8767's 1–3 day bound on serving stale data.
Cached parent misses: a cached miss at a parent skips only the candidates that parent's own walk covered, so a product fallback derived from the original host is still tried.
No flash on remount: a failed icon is remembered client-side, in an LRU capped at 500 entries for the route's five-minute outage window (an
<img>can't tell a miss from an outage), so a remounted icon renders its fallback without an empty frame. Confirmed misses stay in the browser's HTTP cache for the route's hour. Table rows already reserve a fixed icon slot, so there's no layout shift.Cache key: moves to
favicon:v3so stale misses for newly mapped platforms clear on deploy. Only staging holds favicon cache entries today.Type of Change
Testing
lib/favicon/hosts.test.tscovers platform tenants, self-hosted products, lookalikes and hosting suffixes, internal hosts, platforms the walk already reaches, and no duplicate candidates.lib/favicon/resolve.server.test.tsruns the resolver against the shared input-validation and Redis mocks: provisional icons stay provisional through reuse, a cached parent miss still reaches the product fallback, and a definite icon is kept for a month. Both use made-up hosts only and were written red-first. 73 favicon tests pass, and 206 including the message-content suites.check:test-patterns,check:source-conventions,check:client-boundary,check:boundaries,check:file-names,check:comment-hygiene,check:utils,check:test-patterns,check:source-text.Checklist
test-auditauthoring gate)