Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: unifi controller integration #2236

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

pitschi
Copy link

@pitschi pitschi commented Feb 3, 2025


Homarr

Thank you for your contribution. Please ensure that your pull request meets the following pull request:

  • Builds without warnings or errors (pnpm buid, autofix with pnpm format:fix)
  • Pull request targets dev branch
  • Commits follow the conventional commits guideline
  • No shorthand variable names are used (eg. x, y, i or any abbrevation)

image

@pitschi pitschi requested a review from a team as a code owner February 3, 2025 18:00
Copy link

deepsource-io bot commented Feb 3, 2025

Here's the code health analysis summary for commits d9720ac..aab769f. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScript✅ SuccessView Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@pitschi pitschi changed the title Feat unifi controller integration feat: unifi controller integration Feb 3, 2025
@manuel-rw
Copy link
Member

Thanks a lot for this contribution, @pitschi .
I'm looking forward to this integration because I use UniFi myself! Great work 🚀

@manuel-rw manuel-rw added enhancement New feature or request integration New integration labels Feb 3, 2025
@pitschi
Copy link
Author

pitschi commented Feb 4, 2025

Thanks a lot for this contribution, @pitschi . I'm looking forward to this integration because I use UniFi myself! Great work 🚀
omg... Manuel... I thank YOU so much for taking the time to comment. I will take care of those points and resubmit a new pr.

@pitschi pitschi closed this Feb 4, 2025
@Meierschlumpf
Copy link
Member

@pitschi you can simply continue in this pr if you want to address the comments

@pitschi pitschi reopened this Feb 4, 2025
@pitschi
Copy link
Author

pitschi commented Feb 4, 2025

ups... ok, thanks. have to get into the flow again.

@manuel-rw
Copy link
Member

ups... ok, thanks. have to get into the flow again.

No worries. Here's how it works:

  1. You open a pull request
  2. You set it to "ready for review" as soon as you finished implementation and completed all TODO tasks in the first comment
  3. We review your PR and provide feedback
  4. You implement feedback
  5. Start over from point 3 unless all feedback has been addressed (in which case we approve)
  6. We merge your PR
  7. You must wait for the next weekly release (Friday evening CEST) and then you can pull the new version with your change included!

@manuel-rw
Copy link
Member

Since there is no further activity on this PR, I will take over this pull request. Thank you for the initial contribution @pitschi

@manuel-rw manuel-rw assigned manuel-rw and unassigned pitschi Mar 8, 2025
@manuel-rw manuel-rw force-pushed the feat-unifi-controller-integration branch 4 times, most recently from a738c45 to 8cd2d71 Compare March 8, 2025 21:20
@manuel-rw manuel-rw force-pushed the feat-unifi-controller-integration branch from 8cd2d71 to b221a88 Compare March 8, 2025 22:13
@manuel-rw manuel-rw marked this pull request as draft March 8, 2025 22:13
@manuel-rw manuel-rw force-pushed the feat-unifi-controller-integration branch 5 times, most recently from af4fd14 to b9970e6 Compare March 9, 2025 11:27
@manuel-rw manuel-rw force-pushed the feat-unifi-controller-integration branch 2 times, most recently from 31ee449 to 4ff6d7d Compare March 9, 2025 11:32
@manuel-rw manuel-rw force-pushed the feat-unifi-controller-integration branch from 4ff6d7d to aab769f Compare March 9, 2025 11:36
@manuel-rw manuel-rw marked this pull request as ready for review March 9, 2025 11:42
@manuel-rw manuel-rw requested a review from a team March 9, 2025 11:43
id: integration.id,
name: integration.name,
kind: integration.kind,
updatedAt: timestamp,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes more sense to put this one level out?

Comment on lines +4 to +7
wwwStatus: "enabled" | "disabled";
wwwLatency: number;
wwwPing: number;
wwwUptime: number;
Copy link
Member

Choose a reason for hiding this comment

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

What about using objects instead? Like

const a = {
	www: {
		status: "enabled",
		latency: 5,
		ping: 3,
		uptime: 99999
	}
};

Comment on lines +3 to +21
export const throwByUnifiControllerResponseStatusCode = (statusCode: number) => {
switch (statusCode) {
case 400:
throw new IntegrationTestConnectionError("badRequest");
case 401:
throw new IntegrationTestConnectionError("unauthorized");
case 403:
throw new IntegrationTestConnectionError("forbidden");
case 404:
throw new IntegrationTestConnectionError("notFound");
case 429:
throw new IntegrationTestConnectionError("tooManyRequests");
case 500:
throw new IntegrationTestConnectionError("internalServerError");
case 503:
throw new IntegrationTestConnectionError("serviceUnavailable");
default:
throw new IntegrationTestConnectionError("commonError");
}
Copy link
Member

Choose a reason for hiding this comment

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

Use throwErrorByStatusCode instead and add the missing codes

const responseHeaders = loginResponse.headers;
const newHeaders: Record<string, string> = {};
const loginToken = UnifiControllerIntegration.extractLoginTokenFromCookies(responseHeaders);
newHeaders.Cookie = `${loginToken};`;
Copy link
Member

Choose a reason for hiding this comment

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

What about storing this login token with session-store? Then it will also be persisted for nextjs etc. I would suggest to do it similar to what I did with pi-hole v6 (always fetch new session for test connection)

const result = unifiSummaryResponseSchema.safeParse(await statsResponse.json());

if (!result.success) {
throw new Error("Error parsing response from unifi controller.", result.error);
Copy link
Member

Choose a reason for hiding this comment

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

throw new ParseError()

@@ -51,6 +53,8 @@ export const widgetImports = {
downloads,
"mediaRequests-requestList": mediaRequestsList,
"mediaRequests-requestStats": mediaRequestsStats,
networkControllerSummary,
networkControllerNetworkStatus: networkControllerStatus,
Copy link
Member

Choose a reason for hiding this comment

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

Make them the same name

}

return prevData.map((item) =>
item.integration.id === data.integration.id ? { ...item, summary: data.summary } : item,
Copy link
Member

Choose a reason for hiding this comment

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

You may also need to update the updatedAt when you want it existing. If not needed, just remove it

Comment on lines +28 to +30
xl2: rem(24),
xl3: rem(28),
xl4: rem(36),
Copy link
Member

Choose a reason for hiding this comment

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

What about 2xl instead of xl2?

Copy link
Member

Choose a reason for hiding this comment

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

The two variants are very similar. Why not just one generic component for example with properties

interface NetworkStatusContentProps {
	icon: TablerIcon;
	title: string;
	stats: StatRowProps[]
}

Then the design will always be consistent and we don't need to change (now 2) multiple components

<Box h="100%" p="sm">
<Center h={"100%"}>
<List spacing={"xs"} center>
<List.Item icon={<StatusIcon status={data[0]?.wanStatus} />}>WAN</List.Item>
Copy link
Member

Choose a reason for hiding this comment

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

Can't we just make it no longer an array if we only use [0]?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request integration New integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants