Repository navigation
Conversation
…tration Adds three of the four new packages and every registration point they need. Two of those points fail silently if missed, which is why they are called out here. models/all is three separate edits: the dependency, the import, and the builder array entry. Without the builder entry the WorkspaceGif class simply does not exist at runtime and nothing raises an error. rush.json is edited textually rather than by parsing. It is JSONC and every comment in it would be stripped by a parse-and-rewrite. The model carries the DomainIndexConfiguration and the IgnoreActions mixin. Without the first, a table holding tens of rows inherits full indexing; without the second, a WorkspaceGif offers Open, OpenInNewTab, Print and NewRelatedIssue, none of which it can honour. WorkspaceGif stores width and height so the grid can reserve the right box before an image loads, which is what stops the grid reflowing as GIFs arrive, and sha256 as a dedup key so the same upload twice is one row and one blob. Logic lives in plugins/gif as plain functions rather than in Svelte, because this repo has no component-mounting harness and anything left in a component is untestable. 22 tests cover keyword derivation, the search predicate, upload validation, both guards of the result union, pagination, Enter-picks-first and the four library view states. Two deliberate test choices. validateUpload returns a reason, never a message, so the tests bind to the reason and a copy edit cannot turn them red. And libraryViewState separates 'empty' from 'no-results', because telling a user their library is empty after a search that found nothing says their library was wiped. All 14 locales are present. T-51 asserts the count from __dirname, and catches a deleted locale that the standard makeLocalesTest does not, since that one compares only en and ru. rush check clean. build and validate green over 56 operations. Refs Platform-Collective#1 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 7b937fb) Signed-off-by: Shannon Kerr <shannon@exaviz.com>
The visible half. Clicking the existing composer emoji button now opens a two-tab picker, Emoji and GIFs, exactly as requested. The shell hosts the real EmojiPopup through Component with its existing embedded prop, which was already in the tree and used by nothing. No emoji code is copied and EmojiPopup itself is unmodified, so the twelve non-composer call sites are untouched and still open the plain popup. Both tabs stay mounted and toggle with CSS. EmojiPopup's onMount walks the whole emoji catalogue and its category strip reads element geometry, so remounting on each tab switch would redo that work and reset scroll position. The tab bar sits outside the fixed 29rem emoji body, so the popup grows rather than the emoji grid losing a row, and both tabs are a fixed 25.5rem wide so nothing resizes on switch. Tabs are text labels, because EmojiPopup renders its own strip of category icons directly below and a second icon row would read as more categories. Opens on Emoji every time: persisting the last tab would mean the emoji button sometimes opens a GIF grid. The grid packs shortest-column-first rather than round-robin, so no column dangles below the others, and reserves each cell from the stored width and height before the image arrives. That reservation is what stops the grid reflowing as GIFs load, which is the single thing that makes a picker feel broken. Cells lazy-load through IntersectionObserver because a GIF is served unresized, fade in as they decode, and carry their own background so a transparent GIF does not read as an artifact. Empty and no-results are different screens. Telling someone their library is empty after a search that found nothing says their uploads are gone. Upload validation lives in one helper that the component calls, rather than inline the way the custom-emoji popup does it, so there is no second place a limit can drift. Each rejection names its own limit. sha256 dedups before upload. The settings table deletes the blob with the row so the datalake does not accumulate orphans. Registration: four points each in dev/prod and desktop, four rush.json entries, three models/all edits, and the workspace dependencies. New test, worth more than this feature: platform registration parity. The desktop app bundles its own front, so that list is maintained twice by hand, and a plugin registered on web but missed on desktop means the Mac app silently lacks the feature. It asserts web is a subset of desktop, since desktop legitimately has more (desktopDownloadsId). Gates: rush check clean, validate green over 20 operations, svelte-check green and verified capable of failing on a deliberate error rather than trusted blind, format idempotent, 235 tests passing across seven suites. Refs Platform-Collective#1 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 0ade3b4) Signed-off-by: Shannon Kerr <shannon@exaviz.com>
The picker change modifies a code path every existing emoji user hits many times a day, and until now CI covered none of it. The plan review assumed this had to be manual; it does not. Both ref-action arrays are plain exported closures, so the routing is testable by mocking showPopup to capture the callback and driving it with each result shape. Four cases, in order of who they protect: - a gif result reaches insertGif and never insertEmoji - a unicode emoji still reaches insertEmoji unchanged - a custom emoji's image argument survives, which is what makes it render as an image rather than literal :shortcode: text - a dismissal inserts nothing The third is the subtle one. The type guards can be perfect and the image field can still be dropped at the call site, so it is asserted on the call rather than on the guard. Negative-mutation verified both ways: removing the gif branch fails the first test only, and removing the null guard fails the dismissal test only. Neither mutation disturbs the other cases. jest.config.js now maps .svelte to the stub packages/presentation already maintains and desktop already borrows, rather than adding a third copy. That is what lets a plain-TS module importing a .svelte icon be tested at all, and it unlocks the same approach for future tests here. 239 tests green across seven suites. validate green, svelte-check exit 0, format idempotent. Refs Platform-Collective#1 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 8732b3b) Signed-off-by: Shannon Kerr <shannon@exaviz.com>
Removing a gif from the workspace library deleted its blob as well. Every message already containing that gif stores the blob id, not a copy, and nothing rewrites sent messages, so the removal broke every one of them permanently, for everyone, with no recovery. The settings category is role AccountRole.User, so any member could do it. Upstream's equivalent emoji settings table offers no delete at all, so this destructive path was ours, not inherited. Removing the row now only takes the gif out of the picker. The cost is an orphan blob per removal, which is deliberate: an accumulating datalake is housekeeping, a broken message history is not recoverable. A server-side sweep of blobs no live message references is the right fix and does not belong behind this button. (cherry picked from commit b552a6a) Signed-off-by: Shannon Kerr <shannon@exaviz.com>
…red raw Huly's string loader reads `string.<Id>` from a plugin's lang file. All 14 gif-assets locales wrapped their entries in a "gif" object instead, so no label resolved and the picker showed raw ids: gif:string:Gifs, gif:string:SearchGifs, gif:string:NoGifsYet and the rest. Renamed the root key to "string" in all 14 files. No text changed. Verified the ids line up: 26 declared IntlStrings in plugins/gif/src/plugin.ts and 26 keys in en.json, with no extras on either side, so nothing else was going to render raw. Reported by Shannon against the deployed build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 88b530b) Signed-off-by: Shannon Kerr <shannon@exaviz.com>
The picker had no provider behind it: the search box only filtered GIFs someone had uploaded, so a fresh workspace showed an empty grid. This adds GIPHY as a third tab, which is the source people actually reach for. Shape is dictated by the GIPHY API terms, not by preference: - Calls go direct from the browser. Their terms say "All requests to GIPHY should be made directly from the client side" and "Do not proxy requests to GIPHY, either API calls or media URL loads", so the server never sees a GIPHY url and the api key is deliberately shipped to the client. A beta key is rate limited rather than billed, so exposure costs throughput. - A chosen GIF is hot-linked by url. Their terms forbid caching copies, so nothing is uploaded to workspace storage. insertGif already accepted `src` alongside `file-id` and gifExt.ts already renders both, so the node needed no change. - "Powered By GIPHY" is displayed in the tab, which they require. Rate limit drove two decisions: a beta key allows 100 calls per hour for the WHOLE workspace, so search is debounced 450ms and the tab fetches nothing until it is actually opened. Config: GIPHY_API_KEY flows env -> front config.json -> client metadata. It is optional everywhere; with no key the tab is absent and the picker still works against the workspace library. Also fixes the tab underline, which was hardcoded to 50% and would have been the wrong width the moment a third tab existed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 5e8cf48) Signed-off-by: Shannon Kerr <shannon@exaviz.com>
`rush validate` failed on packages/presentation and, by cascade, on text-editor-resources and view-resources. CI runs validate before svelte-check, so this branch would not have passed either gate. Cause: markupViewerCoverage.test.ts, gifRegistration.test.ts and gifKit.test.ts read component source with fs/path/__dirname. Every test in this repo that imports fs is one of ours; no upstream test does it. Two layers, and the first alone was not enough: - @types/node was absent from both packages (^22.18.1, the version the rest of the repo pins) - installing it changed nothing, because the `ui` rig profile pins types: ['jest'], so tsc never loads node's declarations no matter what is in node_modules Both packages now declare types: ["node", "jest"], which is exactly what plugins/gif-assets already did for the same reason. That package was the only one of the three that got it right. Verified on a clean tree: validate 112 operations rc=0, svelte-check 64 operations rc=0. Previously 104 with one failure, then three svelte-check failures. Runtime is unaffected; this is a type-check and test-tooling fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit aa96549) Signed-off-by: Shannon Kerr <shannon@exaviz.com>
Review of the GIPHY tab found four defects. All four are rate-limit defects, which matters because a beta key allows 100 calls per hour for the WHOLE workspace. 1. Opening the tab sent trending TWICE. `loadedOnce = true` was itself a dependency of the second reactive statement, so it re-ran with the same empty query and scheduled a duplicate 450ms later. 2. Tab switching re-sent the current query. `active` is a dependency and the picker keeps this component mounted, toggling CSS, so every GIPHY -> Emoji -> GIPHY round trip cost a call. Four people doing that 25 times an hour is 200 calls with nobody searching. 3. Leaving the tab did not cancel a pending search. The `if` had no else, so clearTimeout never ran and the timer fired against a hidden tab. 4. A 429 did not stop anything. `run` cleared `rateLimited` before every fetch, so the next keystroke, tab return or Load More spent another call against an hour that was already exhausted. Now one reactive statement gated on `query !== lastRequested`, an else branch that clears the timer, `rateLimited` cleared only on success, and Load More disabled while limited. Also fixes a race found in the same review: an aborted request's `finally` cleared `loading` while its replacement was still in flight, and a resolved page-2 response could append onto a newer result set. Requests now carry a generation counter and non-current ones drop their result. Verified after: validate rc=0, svelte-check rc=0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 7bc983d) Signed-off-by: Shannon Kerr <shannon@exaviz.com>
…oader - Replace legacy .eslintrc.js with eslint.config.mjs (same as emoji packages) - Align devDependencies with the emoji packages - Add plugins/gif-assets/src/lang.ts and the ./lang export used by addStringsLoader - Add jest.config.js / .npmignore to models/gif - text-editor-resources: add node types for tests; update gif registration test - Apply rush fast-format Signed-off-by: Shannon Kerr <shannon@exaviz.com>
Contributor
|
Thanks for adding the picker. I confirmed these issues:
|
- type the empty column initialiser in packColumns (TS7011) - hash with a software SHA-256 when crypto.subtle is unavailable (insecure HTTP origins) and run the whole upload inside one handler - limit removal of library entries to the uploader and admins, with a confirmation; the blob is kept - search and page the library on the server instead of the newest 500 - derive the row id from the checksum so the primary key dedups concurrent uploads Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
- look up duplicates by checksum as well as derived id, so rows created before ids were derived are still caught - normalise search separators instead of passing "_" to LIKE, where the server and the live-query matcher disagree - hold the empty-library state until the library count has loaded - clear the file input so the same file can be retried after an error Co-Authored-By: Claude Sonnet 5.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.
Summary
Ports the rest of the GIF feature onto current
develop, on top of thegifmarkup node from #25. With this, the emoji button in the message composer and the text editor opens a tabbed picker (Emoji, GIPHY when configured, and the workspace GIF library) instead of the emoji popup.What changed
plugins/gif(plugin ids, types,isGifResultguard),plugins/gif-assets(icons and 14 locales, using the new./langloader),plugins/gif-resources(picker, library grid, upload, settings table, GIPHY tab) andmodels/gif(WorkspaceGifclass in its owngifdomain).text-editor-resourcesdefaultRefActionsandcommunication-resourcesdefaultMessageInputActionsopenGifPickerPopup. A runtime guard sends GIF results toinsertGif(from feat: add a gif node to markup, with serialization and viewer support #25) and everything else toinsertEmojiunchanged.GIPHY_API_KEYis configured. The value goes env →server/frontconfig.json →gifPlugin.metadata.GiphyApiKey, and no key is committed. Requests go straight from the browser and media is hot-linked, as GIPHY's API terms require (no proxying, no caching). That means the key is a client-side key by design. "Powered By GIPHY" is shown. Search is debounced, nothing is fetched until the tab is opened, and a 429 stops further calls. The free beta key allows 100 calls/hour per key.dev/prodanddesktopplatform.ts use the current lang-loader pattern (addStringsLoader(gifId, GifLang.loadLang)), plusaddLocationfor gif-resources. The model is added tomodels/all.eslint.config.mjsand Jest 30 like the emoji packages, andtext-editor-resourcesgets node types for its source-reading tests.What it does not change
common/scripts/version.txtis untouched. The newgifdomain needs no data migration, so I didn't bump the model version.packages/uiorpackages/presentation.Checks run locally
rush build;rush validatefor gif-resources, model-gif, gif-assets, text-editor-resources and dependents: rc=0svelte-checkfor gif-resources, text-editor-resources, communication-resources and presentation: rc=0rush fast-format: applied (last commit)I haven't run the UI test suites locally.
Notes for reviewers
Builds on #25.
cc @ArtyomSavchenko @haiodo for review (same as #2). I can't request reviewers from a fork.