Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
67cb4b3
wip: commit progress on UrlSync class/hook
Parkreiner Apr 26, 2024
e53d127
refactor: consolidate emoji-testing logic
Parkreiner Apr 26, 2024
f26184f
docs: update comments for clarity
Parkreiner Apr 26, 2024
b54324b
refactor: rename helpers to renderHelpers
Parkreiner Apr 26, 2024
7daf597
wip: finish initial implementation of UrlSync
Parkreiner Apr 26, 2024
86b5acc
chore: finish tests for UrlSync class
Parkreiner Apr 26, 2024
5d14d5a
chore: add mock DiscoveryApi helper
Parkreiner Apr 26, 2024
afd0203
chore: finish tests for useUrlSync
Parkreiner Apr 26, 2024
1e16c82
refactor: consolidate mock URL logic for useUrlSync
Parkreiner Apr 26, 2024
d786e34
fix: update test helper to use API list
Parkreiner Apr 26, 2024
063ecf2
fix: remove unneeded imports
Parkreiner Apr 26, 2024
1d1ab3c
fix: get tests for all current code passing
Parkreiner Apr 26, 2024
58566c8
fix: remove typo
Parkreiner Apr 26, 2024
cd5fef6
fix: update useUrlSync to expose underlying api
Parkreiner Apr 26, 2024
abfd949
refactor: increase data hiding for hook
Parkreiner Apr 26, 2024
26ae96d
fix: make useUrlSync tests less dependent on implementation details
Parkreiner Apr 26, 2024
5aabe86
refactor: remove reliance on baseUrl argument for fetch calls
Parkreiner Apr 26, 2024
425d50f
refactor: split Backstage error type into separate file
Parkreiner Apr 26, 2024
04f0f3e
refactor: clean up imports for api file
Parkreiner Apr 26, 2024
111df43
refactor: split main query options into separate file
Parkreiner Apr 26, 2024
1575f8e
consolidate how mock endpoints are defined
Parkreiner Apr 26, 2024
dc6e75c
fix: remove base URL from auth calls
Parkreiner Apr 26, 2024
fd8b3cb
refactor: consolidate almost all auth logic into CoderAuthProvider
Parkreiner Apr 26, 2024
5af006c
move api file into api directory
Parkreiner Apr 26, 2024
644e632
fix: revert prop that was changed for debugging
Parkreiner Apr 26, 2024
a667b48
fix: revert prop definition
Parkreiner Apr 26, 2024
cca343d
refactor: extract token-checking logic into middleware for server
Parkreiner Apr 26, 2024
aac5a0c
refactor: move shared auth key to queryOptions file
Parkreiner Apr 26, 2024
32fb344
docs: add reminder about arrow functions
Parkreiner Apr 26, 2024
e370197
fix: remove configApi from embedded class properties
Parkreiner Apr 30, 2024
9a359dc
fix: update query logic to remove any whitespace
Parkreiner Apr 30, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions plugins/backstage-plugin-coder/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"@material-ui/icons": "^4.9.1",
"@material-ui/lab": "4.0.0-alpha.61",
"@tanstack/react-query": "4.36.1",
"use-sync-external-store": "^1.2.1",
"valibot": "^0.28.1"
},
"peerDependencies": {
Expand Down
90 changes: 90 additions & 0 deletions plugins/backstage-plugin-coder/src/api/UrlSync.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
import { type UrlSyncSnapshot, UrlSync } from './UrlSync';
import { type DiscoveryApi } from '@backstage/core-plugin-api';
import {
getMockConfigApi,
getMockDiscoveryApi,
mockBackstageAssetsEndpoint,
mockBackstageProxyEndpoint,
mockBackstageUrlRoot,
} from '../testHelpers/mockBackstageData';

// Tests have to assume that DiscoveryApi and ConfigApi will always be in sync,
// and can be trusted as being equivalent-ish ways of getting at the same source
// of truth. If they're ever not, that's a bug with Backstage itself
describe(`${UrlSync.name}`, () => {
it('Has cached URLs ready to go when instantiated', () => {
const urlSync = new UrlSync({
apis: {
configApi: getMockConfigApi(),
discoveryApi: getMockDiscoveryApi(),
},
});

const cachedUrls = urlSync.getCachedUrls();
expect(cachedUrls).toEqual<UrlSyncSnapshot>({
baseUrl: mockBackstageUrlRoot,
apiRoute: mockBackstageProxyEndpoint,
assetsRoute: mockBackstageAssetsEndpoint,
});
});

it('Will update cached URLs if getApiEndpoint starts returning new values (for any reason)', async () => {
let baseUrl = mockBackstageUrlRoot;
const mockDiscoveryApi: DiscoveryApi = {
getBaseUrl: async () => baseUrl,
};

const urlSync = new UrlSync({
apis: {
configApi: getMockConfigApi(),
discoveryApi: mockDiscoveryApi,
},
});

const initialSnapshot = urlSync.getCachedUrls();
baseUrl = 'blah';

await urlSync.getApiEndpoint();
const newSnapshot = urlSync.getCachedUrls();
expect(initialSnapshot).not.toEqual(newSnapshot);

expect(newSnapshot).toEqual<UrlSyncSnapshot>({
baseUrl: 'blah',
apiRoute: 'blah/coder/api/v2',
assetsRoute: 'blah/coder',
});
});

it('Lets external systems subscribe and unsubscribe to cached URL changes', async () => {
let baseUrl = mockBackstageUrlRoot;
const mockDiscoveryApi: DiscoveryApi = {
getBaseUrl: async () => baseUrl,
};

const urlSync = new UrlSync({
apis: {
configApi: getMockConfigApi(),
discoveryApi: mockDiscoveryApi,
},
});

const onChange = jest.fn();
urlSync.subscribe(onChange);

baseUrl = 'blah';
await urlSync.getApiEndpoint();

expect(onChange).toHaveBeenCalledWith({
baseUrl: 'blah',
apiRoute: 'blah/coder/api/v2',
assetsRoute: 'blah/coder',
} satisfies UrlSyncSnapshot);

urlSync.unsubscribe(onChange);
onChange.mockClear();
baseUrl = mockBackstageUrlRoot;

await urlSync.getApiEndpoint();
expect(onChange).not.toHaveBeenCalled();
});
});
157 changes: 157 additions & 0 deletions plugins/backstage-plugin-coder/src/api/UrlSync.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
/**
* @file This is basically a fancier version of Backstage's built-in
* DiscoveryApi that is designed to work much better with React. Its hook
* counterpart is useUrlSync.
*
* The class helps with synchronizing URLs between Backstage classes and React
* UI components. It will:
* 1. Make sure URLs are cached so that they can be accessed directly and
* synchronously from the UI
* 2. Make sure that there are mechanisms for binding value changes to React
* state, so that if the URLs change over time, React components can
* re-render correctly
*
* As of April 2024, there are two main built-in ways of getting URLs from
* Backstage config values:
* 1. ConfigApi (offers synchronous methods, but does not have direct access to
* the proxy config - you have to stitch together the full path yourself)
* 2. DiscoveryApi (has access to proxy config, but all methods are async)
*
* Both of these work fine inside event handlers and effects, but are never safe
* to put directly inside render logic. They're not pure functions, so they
* can't be used as derived values, and they don't go through React state, so
* they're completely disconnected from React's render cycles.
*/
import {
type DiscoveryApi,
type ConfigApi,
createApiRef,
} from '@backstage/core-plugin-api';
import {
type Subscribable,
type SubscriptionCallback,
CODER_API_REF_ID_PREFIX,
} from '../typesConstants';
import { StateSnapshotManager } from '../utils/StateSnapshotManager';

// This is the value we tell people to use inside app-config.yaml
export const CODER_PROXY_PREFIX = '/coder';

const BASE_URL_KEY_FOR_CONFIG_API = 'backend.baseUrl';
const PROXY_URL_KEY_FOR_DISCOVERY_API = 'proxy';

type UrlPrefixes = Readonly<{
proxyPrefix: string;
apiRoutePrefix: string;
assetsRoutePrefix: string;
}>;

export const defaultUrlPrefixes = {
proxyPrefix: `/api/proxy`,
apiRoutePrefix: '/api/v2',
assetsRoutePrefix: '', // Deliberately left as empty string
} as const satisfies UrlPrefixes;

export type UrlSyncSnapshot = Readonly<{
baseUrl: string;
apiRoute: string;
assetsRoute: string;
}>;

type Subscriber = SubscriptionCallback<UrlSyncSnapshot>;

type ConstructorInputs = Readonly<{
urlPrefixes?: Partial<UrlPrefixes>;
apis: Readonly<{
discoveryApi: DiscoveryApi;
configApi: ConfigApi;
}>;
}>;

const proxyRouteReplacer = /\/api\/proxy.*?$/;

type UrlSyncApi = Subscribable<UrlSyncSnapshot> &
Readonly<{
getApiEndpoint: () => Promise<string>;
getAssetsEndpoint: () => Promise<string>;
getCachedUrls: () => UrlSyncSnapshot;
}>;

export class UrlSync implements UrlSyncApi {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sync as in synchronous, right, because you can access the URLs synchronously? Not worth blocking over the name, but it does seem at odds with how there are async methods on the class, which look like the main usages of the class except for isEmojiUrl which uses assetsRoute.

Copy link
Member Author

@Parkreiner Parkreiner Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just about to log off, so I won't be looking at the rest of the comments until tomorrow morning, but really quick: it's actually "sync" as in "synchronization"

The main goal of the class is synchronizing React with the Backstage APIs (and also working with useSyncExternalStore, which also uses "sync" in the "synchronizing" sense)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably rename things to remove that ambiguity

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhhhhhh I probably had the ...Sync methods in Node's fs on my brain. No really good alternatives come to my mind, so I am not opposed to just leaving it.

private readonly discoveryApi: DiscoveryApi;
private readonly urlCache: StateSnapshotManager<UrlSyncSnapshot>;
private urlPrefixes: UrlPrefixes;

constructor(setup: ConstructorInputs) {
const { apis, urlPrefixes = {} } = setup;
const { discoveryApi, configApi } = apis;

this.discoveryApi = discoveryApi;
this.urlPrefixes = { ...defaultUrlPrefixes, ...urlPrefixes };

const proxyRoot = this.getProxyRootFromConfigApi(configApi);
this.urlCache = new StateSnapshotManager<UrlSyncSnapshot>({
initialSnapshot: this.prepareNewSnapshot(proxyRoot),
});
}

// ConfigApi is literally only used because it offers a synchronous way to
// get an initial URL to use from inside the constructor. Should not be used
// beyond initial constructor call, so it's not being embedded in the class
private getProxyRootFromConfigApi(configApi: ConfigApi): string {
const baseUrl = configApi.getString(BASE_URL_KEY_FOR_CONFIG_API);
return `${baseUrl}${this.urlPrefixes.proxyPrefix}`;
}

private prepareNewSnapshot(newProxyUrl: string): UrlSyncSnapshot {
const { assetsRoutePrefix, apiRoutePrefix } = this.urlPrefixes;

return {
baseUrl: newProxyUrl.replace(proxyRouteReplacer, ''),
assetsRoute: `${newProxyUrl}${CODER_PROXY_PREFIX}${assetsRoutePrefix}`,
apiRoute: `${newProxyUrl}${CODER_PROXY_PREFIX}${apiRoutePrefix}`,
};
}

/* ***************************************************************************
* All public functions should be defined as arrow functions to ensure they
* can be passed around React without risk of losing their `this` context
****************************************************************************/

getApiEndpoint = async (): Promise<string> => {
const proxyRoot = await this.discoveryApi.getBaseUrl(
PROXY_URL_KEY_FOR_DISCOVERY_API,
);
Comment on lines +122 to +124
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tangential to this PR, but I still think we should consider making queries directly to the Coder API and remove the proxy altogether.

Backstage docs do say this makes senses under circumstances that appear to apply to us, and the reasons for doing it via a proxy do not seem relevant to us: https://backstage.io/docs/plugins/call-existing-api#issuing-requests-directly

But, the reason I bring it up now is that it would eliminate the need for both configApi and discoveryApi, I think, at least as they are used here, so it would affect this new class. We only need the URL on the Coder config object, if I recall correctly.

Copy link
Member Author

@Parkreiner Parkreiner Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that you mention it, as long as we know the single endpoint, and since we're in control of how it's structured, I don't think there would be any issues with ditching the proxy. It would also remove the need for the user to update their app-config.yaml file

I'm going to keep the code the same as-is for now, just because there's two other PRs depending on it, and because even if this isn't the final iteration, the API code does get consolidated a lot more. The current code should make it easier to remove the proxy and extra APIs down the line

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!


const newSnapshot = this.prepareNewSnapshot(proxyRoot);
this.urlCache.updateSnapshot(newSnapshot);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have to call these methods to update the snapshot, you would always need to call these methods, right? Which means you should already have the endpoint you need, and have no need for state.

If we had something like discoveryApi.subscribe(() => urlCache.update()), then it would make more sense to me, because then we would not need the methods at all, the snapshot would always be up to date, and we could just access it synchronously off useUrlSync().state (no idea if discoveryApi has that, just thinking out loud).

I see the only place we access the synchronous state is isEmojiUrl which uses assetsRoute, but IMO we could just remove that function and remove the border radius. The Coder dashboard does not use any rounding, which I think makes sense; even some of the non-emoji icons we use for templates are meant to be square or have tips that get cut off.

tl;dr maybe we do not need any snapshot in this class if we get rid of the one current usage? Although, then it becomes a very thin wrapper around configApi and discoveryApi, and maybe it will make sense to call them directly.

Copy link
Member Author

@Parkreiner Parkreiner Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, a subscription would be nice, and I think that would ultimately be the best design – I'm just not completely sure what the best way to implement it with the current APIs would be. DiscoveryApi doesn't have a subscription method

As far as the rounding, though, that was basically an aesthetic decision. If I look at my Coder dashboard right now:
Screenshot 2024-04-30 at 11 03 45 AM

I think the images look a little sloppy, because there's no "aesthetic consistency" between them. They're not using the same colors, and by default, don't have the same shape, so the only thing giving them visual rhythm is the fact that they're placed consistently in the same spot. Enforcing rounding increases their similarities, and makes them look like they're supposed to be similar. I actually think removing the rounding would make make the UI look less polished

I've actually been meaning to open a PR for Coder core to update the CSS to get the emoji rounding issues fixed, too

Copy link
Member

@code-asher code-asher Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to leave being consistent up to the folks uploading the icons, as frustrating as that is. If you check out the templates page, some of the non-emoji icons are non-circle shapes which I think look weird or wrong when they are forcefully rounded.

I think this is why we have the padding to emojis, to avoid cutting them off, but regular images can have the same problem. How do we tell the difference between an emoji or emoji-like image and one that can be rounded?

Copy link
Member

@code-asher code-asher Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some would look better rounded though, maybe it makes sense to add a "Round this icon?" option to the template settings, otherwise folks have to manually round their icons which seems annoying. Although, it seems like this mostly only happens when the image is kinda bad to begin with, and includes a background color. And sometimes they contain extra space making them overly small which I guess there is not much we can do about except ask the user if they want to cut off some of the image or something.

Also sometimes it seems we display a square person icon (on the dashboard, not sure what would happen in Backstage), which I think happens when the image does not load? Seems like we should put something else there.

Copy link
Member Author

@Parkreiner Parkreiner May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point – I could go either way, honestly

I feel like user avatars (and other images that are basically for UI purposes) kind of have the expectation that they can be morphed or changed into any number of shapes, and that if the image the user supplies doesn't look good with the styling the vendor/app uses, it's the user's responsibility to fix that. I can't remember the last time I've seen a modern site not use some amount of rounding with their "UI-ish" images. But I don't think workspaces/templates quite have that expectation

With the emoji, we at least know that they will always come from the same source (I don't think we give users the ability to define custom emoji), so they're all working with the same uniform constraints and can be offset the same consistent way

Realistically, though, most icons are going to have their main content in the center, so they naturally lend themselves to being rounded. I think I'd rather choose making 95% of icons look better, with 5% looking worse, over 100% looking "okay-ish". I'm not sure if this is worth exposing another property to the end user, though

Copy link
Member Author

@Parkreiner Parkreiner May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the plugin does have logic to handle there not being any images, too – those use the same rounding
Screenshot 2024-05-01 at 9 21 23 AM

If we get rid of the rounding for the images, I'm not sure what to do with the fallbacks. I guess we could keep those rounded, but jumping from non-rounded to rounded might be weird. And even if we don't completely remove the rounding, and just reduce it, I think the UI still starts to look a little funny
Screenshot 2024-05-01 at 9 24 14 AM

Copy link
Member

@code-asher code-asher May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like user avatars (and other images that are basically for UI purposes) kind of have the expectation that they can be morphed or changed into any number of shapes [...] But I don't think workspaces/templates quite have that expectation

I think that expectation can make sense for images, but not as much for icons. We use mostly emojis and logos for icons, but I wonder if that is the case generally for other users.

Tangentially, the current devcontainers icon feels odd to me because it has some built-in padding making it look too small in comparison. I tried a different icon without the padding so it matches the other icons, what do you think?

With the emoji, we at least know that they will always come from the same source (I don't think we give users the ability to define custom emoji), so they're all working with the same uniform constraints and can be offset the same consistent way

A custom emoji could be added like any other icon but yeah no way to modify the default set of emojis pretty sure, unless maybe you build from source with your own emojis.

From playing around on the dashboard, we would need 5px padding to prevent emojis from getting cropped, and that makes them look unusually small compared to the other icons. Is it just me though?

Realistically, though, most icons are going to have their main content in the center, so they naturally lend themselves to being rounded. I think I'd rather choose making 95% of icons look better, with 5% looking worse, over 100% looking "okay-ish".

With the bundled icons and the ones we have added at least, we might make more look worse than better (going off the templates page and https://dev.coder.com/icons), but that might not hold true for icons added by other users.

I think it is not so much about being centered (which I think all ours are), but about being able to fit in the bounds of a circle. For example the Vault triangle turns into a slice of pizza, JFrog on the templates page becomes IFrog with a mangled g, Kotlin turns into Pac-Man 😂 and Fedora loses its little tail.

And the plugin does have logic to handle there not being any images, too – those use the same rounding

Ah I was thinking about the case where there is an image, but it is a 404.

And even if we don't completely remove the rounding, and just reduce it, I think the UI still starts to look a little funny

I actually quite like the decreased rounding on those fallbacks!! But I also like Lisp so who knows if I can be trusted. The letters alone without a background, maybe extra bold, could look cool too. Good topic for FE variety maybe?

I probably sound really passionate about this (maybe I am lol) but I mostly feel motivated to remove it because it means the entire snapshot and subscription code in this class goes away. I think it felt like a lot to me for all that code only to support rounding! 😛

To add one more argument to the small book I have written here, we display these icons in Backstage, the dashboard, and the Gateway plugin; right now Backstage is the odd one out.

return newSnapshot.apiRoute;
};

getAssetsEndpoint = async (): Promise<string> => {
const proxyRoot = await this.discoveryApi.getBaseUrl(
PROXY_URL_KEY_FOR_DISCOVERY_API,
);

const newSnapshot = this.prepareNewSnapshot(proxyRoot);
this.urlCache.updateSnapshot(newSnapshot);
return newSnapshot.assetsRoute;
};

getCachedUrls = (): UrlSyncSnapshot => {
return this.urlCache.getSnapshot();
};

unsubscribe = (callback: Subscriber): void => {
this.urlCache.unsubscribe(callback);
};

subscribe = (callback: Subscriber): (() => void) => {
this.urlCache.subscribe(callback);
return () => this.unsubscribe(callback);
};
}

export const urlSyncApiRef = createApiRef<UrlSync>({
id: `${CODER_API_REF_ID_PREFIX}.url-sync`,
});
Loading