fix(editor): restore profile URL resolution#584
Conversation
|
this code is totally the wrong approach. we should be using the domain store and the normal https to hm url resolution workflow. We used to have this working, so we have suffered a regression. This PR attempts to write new code instead of fixing the broken code path |
aa11b91 to
bbf4874
Compare
|
You’re right — my first pass was the wrong layer. I backed out the ad-hoc public What changed now:
Validation is updated in the PR body. |
|
I think the domain store should keep track of domains who are gateways. So the is_gateway sort of flag must be externalized from the web side of things in the /hm/api/config endpoint, and then saved in the domain store. Then if the cached domain store knows of this domain as a gateway then it can rapidly convert all the /hm/* URLs to hm:// |
bbf4874 to
1b8935e
Compare
|
Thanks — I traced that path and updated the PR to carry the existing What changed in the new push:
I also updated the PR body with the before-fix reproduction, verification, and self-review notes. Frontend/editor/desktop gates pass; Go tests are still blocked locally because this node doesn’t have |
1b8935e to
9cdc00a
Compare
ericvicenti
left a comment
There was a problem hiding this comment.
can you please combine the getSiteProfileMetadataId callers into one place (ideally the loader will pass this information down into the component).
Then once the two callers are combined, get rid of that function entirely. It's simpler to avoid a new confusingly-named function in simple cases like that
| ): UnpackedHypermediaId { | ||
| if (viewTerm === 'profile') { | ||
| return hmId(accountUid || siteId.uid, { | ||
| path: [':profile'], |
There was a problem hiding this comment.
you should be using viewTerm instead of path for this. See how its used in entity-id-url.ts
When there is a : prefix, its not a path. its a viewTerm
There was a problem hiding this comment.
Fixed in the latest push. I switched the OPTIONS/profile metadata path to use extractViewTermFromUrl() from entity-id-url.ts, so :profile is handled as a viewTerm instead of being parsed as a document path. I also removed the duplicate getSiteProfileMetadataId helper and compute the page metadata id once in the loader.
| } | ||
| const metadataId = getSiteProfileMetadataId( | ||
| siteDocument.id, | ||
| (siteDocument as ExtendedSitePayload).viewTerm, |
There was a problem hiding this comment.
you should not need to use ts as keyword.
There was a problem hiding this comment.
Fixed — removed this cast. inspectTab now goes through a small validator before it is passed into loader data, and the meta path now consumes metadataId from the loader instead of casting the payload shape here.
9cdc00a to
a95f7ca
Compare
|
Updated for this review pass and force-pushed Changes:
Re-ran:
I also replied on the two inline threads and updated the PR body with the review-pass notes. |
| import type {UnpackedHypermediaId} from '@seed-hypermedia/client/hm-types' | ||
| import {extractViewTermFromUrl, hmId, viewTermToRouteKey} from '@shm/shared' | ||
|
|
||
| export type RequestResourceIds = { |
There was a problem hiding this comment.
what is the purpose of having both resourceId and metadataId here?
I'm trying to understand if this complexity is justified. Either it should be simplified or the comments+naming conventions should make it obvious why this exists
There was a problem hiding this comment.
Good call — I kept the split because the two IDs are genuinely different for profile URLs, but renamed/commented it so the reason is explicit.
For profile URLs:
loadResourceIdis the daemon-loadable account root (hm://<account>), because:profileis a view term, not a document path.publicMetadataIdis what HTML meta / OPTIONS should advertise (hm://<account>/:profile).
For normal document URLs they are identical. I force-pushed 4d19a9f00 with those names and comments, and reran the focused web test, web typecheck, and git diff --check.
a95f7ca to
4d19a9f
Compare
Summary
https://.../hm/...URLs instead of special-casing publichyper.media/hm/api/configisGatewayvalue through the backend domain store, daemonDomainInfo, generated clients, and shared domain resolver/hm/*as an embedded canonical HM id when the cached domain info says the domain is a gateway; non-gateway custom domains keep/hm/...as normal site pathsresolveHypermediaUrl(..., {domainResolver})workflow and are not short-circuited by temporary linkify marks/:profile/<accountUid>URLs so metadata resolves through the profile view-term path consistently instead of treating:profileas a document pathFixes #582.
Fixes #587.
Before-fix reproduction
origin/mainwith a focusedresolveHypermediaUrlregression test:https://hyper.media/hm/z6Mktarget/:profilehm://z6Mkgateway/hm/z6Mktarget/:profilehm://z6Mktarget/:profilehttps://seedteamtalks.hyper.media/:profile/z6MkwRWdU6HY11Qzi9SQZrwSx3rxFw94ipYyiN7Dtbrcg2yU:hm://z6MkuBbsB1HbSNXLvJCRCrPhimY6g7tzhr4qvcYKPuSZzhnoaccountUidseparately for rendering.Fix summary
/hm/api/configalready emitsisGateway; this PR now decodes, stores, and returns it from the backend domain store.DomainInfonow includesis_gateway, and frontend domain info mapping exposesisGateway.createDomainResolver()returns{registeredAccountUid, isGateway}soresolveHypermediaUrl()can distinguish gateway domains from ordinary custom domains.resolveHypermediaUrl()preserves compatibility with old resolvers returningstring | null, but only canonicalizes/hm/*as embedded HM URLs whenisGateway === true.extractViewTermFromUrl()/viewTerm handling fromentity-id-url.ts, so:profileis interpreted as a view term rather than a document path.as ExtendedSitePayload[...]cast fromroutes/$.tsxby validating inspect tabs before passing them through.After-fix verification
corepack pnpm --filter @seed-hypermedia/client exec vitest --run src/hm-resolver.test.tscorepack pnpm --filter @seed-hypermedia/client run typecheckcorepack pnpm --filter @shm/shared run typecheckcorepack pnpm --filter @shm/web exec vitest --run app/hypermedia-id.test.tscorepack pnpm --filter @shm/web run typecheckcorepack pnpm --filter @shm/editor run typecheckcorepack pnpm --filter @shm/desktop run typecheckcorepack pnpm --filter @shm/editor test -- src/tiptap-extension-link/helpers/pasteHandler.test.tscorepack pnpm --filter @shm/desktop run test:unit(43 files / 414 tests passed)git diff --checkBlocked locally:
go test ./backend/blob ./backend/api/daemon/v1alphacould not run because this environment does not havegoinstalled (go: command not found). I still added a backend regression test for the domain store preservingisGatewayfrom/hm/api/config.Self-review notes
/hm/*canonicalization depends on cached domain metadata./hm/...paths are covered by a regression test and remain relative to that domain account.string | null.getSiteProfileMetadataIdhelper, avoid treating:profileas path, remove the TypeScriptascast from the reviewed route code, and clarify why profile URLs need both a daemon load id (loadResourceId) and a public metadata id (publicMetadataId).