Identify collections by ID when tagging apps - #723
Draft
johnworth wants to merge 3 commits into
Draft
Conversation
The community tag an app carries was minted here: the collection's display_name was posted as an AVU value, and apps stored it verbatim. That made the browser responsible for a storage format it has no business knowing, and tied every tag to a name that can change -- renaming a collection dropped its apps out of their own listing with nothing raised anywhere. Collections are now identified by ID on both the read and write paths, and the service composes the stored value. constants.METADATA.COMMUNITY_ATTR goes with it: the attribute name was only ever needed to build the AVU by hand. fullName/fullCollectionName are renamed to collectionId rather than left carrying an ID under a name that says otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The selectedCategory query param is JSON minted by whichever bundle the user bookmarked. URLs from pre-cutover bundles carry the old fullCollectionName key instead of collectionId, so the collection apps query fired with an undefined identifier; the backend answered 200 with zero apps and the listing claimed the collection was empty. Fall back to the legacy key (terrain's lookup resolves the colon-path value) and only enable the query once an identifier is actually present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On the legacy Grouper backend, app tags were AVUs keyed by community name, so terrain rejected a rename of a tagged community with ERR_EXISTS until the client resubmitted with retag-apps=true; the confirmation dialog existed to drive that resubmit. App tags now reference collections by ID, terrain accepts and ignores retag-apps, and the rename PATCH can no longer fail with that ERR_EXISTS, so the dialog was unreachable dead code and its copy promised a re-tagging that no longer happens. Drop the retagApps plumbing, the dialog, and the locale copy; rename failures now all surface through the existing generic error path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Part of the Grouper removal. Draft until the service side lands — see the ordering note at the bottom.
Why
The community tag an app carries was minted here.
collection.display_namewas posted as an AVU value andappsstored it verbatim, which made the browser responsible for a storage format it has no business knowing, and tied every tag to a name that can change. Renaming a collection dropped its apps out of their own listing with nothing raised anywhere — that mechanism produced the 17 orphaned tag values (48 apps) currently in production.Worse, it was about to get quieter: once terrain serves collections from the new groups service,
display_namebecomes an ordinary short name rather than a colon path, so this code would have started writing a different wrong value with no error anywhere.What changed
Collections are identified by their ID on both the read and write paths, and the service composes the stored value:
addAppToCollection/removeAppFromCollectionsend{ community_ids: [...] }andDELETE /communities/:idinstead of hand-built AVUs.getCollectionAppsaddresses the collection by ID.constants.METADATA.COMMUNITY_ATTRis deleted. The attribute name was only ever needed to build the AVU by hand, and that is exactly the coupling this removes.fullName/fullCollectionNameare renamed tocollectionId, rather than left carrying an ID under a name that says otherwise.Testing
eslint,prettier, and the jest suite pass (the pre-commit hook runs all three). Three pre-existing failures insrc/__tests__/dataSearch.jsare unrelated and fail identically onmain— verified by running that file on both.Not yet exercised in a browser. The service side is verified end to end against a local cluster, but no browser has driven tagging an app into a collection and seeing it listed. That check is worth doing before this leaves draft, since it is what would catch a mismatch between what this sends and what terrain's schema accepts.
Ordering
The services accept both request shapes during the transition, so a stale bundle keeps working; that is what makes it safe for this to deploy on its own schedule rather than atomically with them. But the services have to land first: