[pull] main from tldraw:main#609
Merged
Merged
Conversation
#9331) Follow-up to #9328, addressing @frolic's review: *"feels like this is a smell and that we should be able get the real result type from the query itself."* ### The smell `fileStates$` and `workspaceMemberships$` had **hand-written** type annotations that mirrored the shape of their Zero queries (`packages/dotcom-shared/src/queries.ts`). That mirror is a second source of truth, and it had already drifted from reality: it declared `.one()` joins as always-present, which is exactly what hid the production crash fixed in #9328 (a `group_file` row whose `file` join resolved to `undefined`). ### The change Derive both signal types straight from the query definitions: ```ts private readonly fileStates$: Signal<QueryResultType<typeof queries.fileStates>> private readonly workspaceMemberships$: Signal<QueryResultType<typeof queries.workspaceMemberships>> ``` Now the shape — and the nullability of every `.one()` join — stays in sync with the queries automatically. This whole class of bug becomes a compile error instead of a runtime crash. ### What it caught Making the types honest surfaced **9 latent nullability errors across 3 files** — and none were about `file`. They were all the `group` join (also a `.one()`), which the hand-written type also falsely claimed was always present. So the original crash wasn't a one-off; the hand-written mirror was concealing the same bug for a second relationship. Fixes per site: - Drop group-less rows from the two workspace lists (workspace switcher, "move to" menu) via a type-guard filter — same pattern as the orphan-`file` fix, so downstream `.map` accesses narrow to non-null. - Optional-chain the display reads (workspace names, invite secret). - Remove three type imports (`TlaFileState`, `TlaGroup`, `TlaGroupUser`) that existed only to prop up the hand-written shapes. ### Testing - `yarn typecheck` passes (the derivation resolves; the `getWorkspaceFilesSorted` type-guard from #9328 still composes with the derived element type). - The `getWorkspaceFilesSorted` regression test from #9328 still passes. - No behavior change for the common case; the only runtime difference is that a membership with an unsynced `group` is omitted from the workspace lists rather than crashing.
In order to stop re-implementing `target="_blank"` / `rel="noopener noreferrer"` on every external link and keep them consistent, this PR replaces raw `<a>` anchors with the shared `ExternalLink` component across four dotcom dialogs: legal acceptance, manage cookies, sign-in, and the a11y debug table. `ExternalLink` already encapsulates the target/rel boilerplate and is the established pattern for external links in the app. Closes #9198 (part of the design-system consistency tracking issue #9189). `IFrameProtector` keeps its raw anchor as a documented exception: it sits outside the `tla` subsystem and `ExternalLink` (a react-router `Link`) would require router context that this low-level iframe guard cannot assume. ### Change type - [x] `improvement` ### Test plan A `dotcom-preview-please` preview is requested for this PR. On the preview: 1. Open the sign-in dialog and click **Terms of Use** and **Privacy Policy** — each opens the static page in a new tab. 2. Open **Manage cookies** and click the **cookie policy** links — each opens `/cookies.html` in a new tab. 3. Trigger the legal acceptance dialog and click the terms / privacy / analytics links. 4. Confirm the links still render inline within their surrounding sentences (they are embedded in i18n rich-text chunks). - [ ] Unit tests - [ ] End to end tests ### Code changes | Section | LOC change | | ------- | ---------- | | Apps | +14 / -34 |
In order to start replacing raw `<button>` elements with the shared `TlaButton` (part of the design-system consistency effort), this PR converts the reload/refresh action buttons on the shared `ErrorPage` from unstyled raw `<button>`s to `TlaButton variant="primary"`. These were the cleanest conversions in the audit for #9190: they have no existing styling, and `TlaButton` depends only on its bundled CSS and global tokens (no React providers), so it is safe in `ErrorPage`'s intentionally provider-independent context (offline, sync failure, Clerk timeout). Part of #9190. The remaining raw buttons in #9190 turned out to carry bespoke CSS (cookie consent, sidebar, workspace-settings inline buttons, etc.) that needs reconciling with `TlaButton` variants — that is really #9193's consolidation work — or are intentionally raw (the mobile sidebar overlay, the file-name width-setters). Those are out of scope here. ### Change type - [x] `improvement` ### Test plan A `dotcom-preview-please` preview is requested. The affected buttons only appear on error screens, so they take some effort to reach. In each case the reload/refresh button should now render as a styled primary button instead of a browser-default `<button>`: 1. **`StoreErrorScreen`** — shown on certain sync errors (e.g. a client-too-old close reason renders the "Refresh" button). 2. **`RefreshErrorBoundary` (`ErrorPage`)** — shown when a React error boundary catches an error; the refresh CTA uses this. 3. **Clerk-unavailable fallback (`TlaRootProviders`)** — shown if Clerk fails to load within the timeout; the refresh button uses this. Confirm the button is styled (primary fill, correct hover) and still reloads the page on click. - [ ] Unit tests - [ ] End to end tests ### Code changes | Section | LOC change | | ------- | ---------- | | Apps | +15 / -4 |
…on (#9217) In order to stop hand-maintaining an inline copy of the external-link icon, this PR replaces `IFrameProtector`'s inline `<svg>` with the shared `<TlaIcon icon="external" inline />`. The sprite already has that icon, and `TlaIcon` has no provider dependencies, so it is safe inside that low-dependency iframe guard. It also documents why `MaybeForceUserRefresh` keeps a custom `position: fixed` overlay rather than `TldrawUiDialog`: it renders at the app-provider root to dim the whole app when the client is outdated, a context where the Radix portal and UI-context hooks (`useTranslation` / `useDirection`) are not available. ### Scope note This PR originally also tokenized `.disabledIcon` (`#ccc`) and added a `--tla-color-drop-cursor` token for `.dropCursorLine`, but both have since been removed from `main` independently — the `.disabledIcon` block is gone, and `.dropCursorLine` was deleted in #9254 — so only the icon swap and the doc comment remain. The remaining colour and CSS items from this effort were resolved by #9326 (SDK token reuse) and #9261 (dead-code removal). Part of the design-system consistency effort #9189 / #9199. ### Change type - [x] `improvement` ### Test plan A `dotcom-preview-please` preview is requested: https://pr-9217-preview-deploy.tldraw.com/ On the preview, embed a non-allowed tldraw room in an iframe so `IFrameProtector` shows its warning, and confirm the "Visit this page on tldraw.com" link still shows the external-link icon (now rendered via `TlaIcon`). - [ ] Unit tests - [ ] End to end tests ### Code changes | Section | LOC change | | ------- | ---------- | | Apps | +5 / -14 |
…9211) In order to route spacing through the shared scale instead of hardcoded pixels, this PR replaces single-value `padding` / `margin` / `gap` declarations that exactly match a `--tl-space-*` rung with the corresponding token, across the dotcom client CSS modules (52 declarations in 10 files). Part of #9197 (design-system consistency, #9189). Because each token resolves to the same pixel value it replaced (e.g. `8px` = `var(--tl-space-3)`), there is **no visual change** — this is a pure refactor. Deliberately left for follow-up: - **Off-scale values** (`6px`, `10px`, `24px`) — no matching rung; they need the scale decision discussed in #9197. - **Multi-value shorthands** (e.g. the bespoke auth/dialog card paddings) — left as-is. - **`button.module.css` / `cta-button.module.css`** — skipped to avoid conflicts with the in-flight button PR #9205. ### Change type - [x] `improvement` ### Test plan No visual change is expected — every substituted token equals its previous pixel value. A reviewer can confirm there is no layout difference versus production in the affected areas (sidebar, dialogs, menus, invite/auth dialogs, admin panel). - [ ] Unit tests - [ ] End to end tests ### Code changes | Section | LOC change | | ------- | ---------- | | Apps | +52 / -52 |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )