Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: use org name in header #143

Merged
merged 12 commits into from
Oct 31, 2024
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ node_modules
.pnp.loader.mjs
static/dist
.env
.wrangler/
0x4007 marked this conversation as resolved.
Show resolved Hide resolved

cypress/screenshots
cypress/videos
29 changes: 28 additions & 1 deletion src/home/fetch-github/fetch-and-display-previews.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { GitHubIssue } from "../github-types";
import { taskManager } from "../home";
import { applyAvatarsToIssues, renderGitHubIssues } from "../rendering/render-github-issues";
import { renderOrgHeaderLabel } from "../rendering/render-org-header";
import { closeModal } from "../rendering/render-preview-modal";
import { Sorting } from "../sorting/generate-sorting-buttons";
import { sortIssuesController } from "../sorting/sort-issues-controller";
Expand Down Expand Up @@ -45,6 +46,31 @@ function getProposalsOnlyFilter(getProposals: boolean) {
};
}

function filterIssuesByOrganization(issues: GitHubIssue[]): GitHubIssue[] {
// get organization name from first thing after / in URL
const pathSegments = window.location.pathname.split("/").filter(Boolean);
const urlOrgName = pathSegments.length > 0 ? pathSegments[0] : null;

// if there is no organization name in the URL, return all issues
if (!urlOrgName) return issues;

// filter issues by matching the URL organization name with the issue's organization name
const filteredIssues = issues.filter((issue) => {
const [issueOrgName] = issue.repository_url.split("/").slice(-2);
return issueOrgName === urlOrgName;
});

// if no issues match the organization, redirect to home
if (filteredIssues.length === 0) {
console.log(`No issues found for organization "${urlOrgName}". Redirecting to the home page.`);
window.location.href = "/";
}

renderOrgHeaderLabel(urlOrgName);

return filteredIssues;
}

// checks the cache's integrity, sorts issues, checks Directory/Proposals toggle, renders them and applies avatars
export async function displayGitHubIssues({
sorting,
Expand All @@ -58,7 +84,8 @@ export async function displayGitHubIssues({
await checkCacheIntegrityAndSyncTasks();
const cachedTasks = taskManager.getTasks();
const sortedIssues = sortIssuesController(cachedTasks, sorting, options);
const sortedAndFiltered = sortedIssues.filter(getProposalsOnlyFilter(isProposalOnlyViewer));
let sortedAndFiltered = sortedIssues.filter(getProposalsOnlyFilter(isProposalOnlyViewer));
sortedAndFiltered = filterIssuesByOrganization(sortedAndFiltered);
renderGitHubIssues(sortedAndFiltered, skipAnimation);
applyAvatarsToIssues();
}
30 changes: 30 additions & 0 deletions src/home/rendering/render-org-header.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { organizationImageCache } from "../fetch-github/fetch-issues-full";

export function renderOrgHeaderLabel(orgName: string): void {
const brandingDiv = document.getElementById("branding");
if (!brandingDiv) return;

// Fetch the organization logo from the cache
const logoBlob = organizationImageCache.get(orgName);

if (logoBlob) {
// Convert Blob to a URL
const logoUrl = URL.createObjectURL(logoBlob);

const img = document.createElement("img");
img.src = logoUrl;
img.alt = `${orgName} Logo`;
img.id = "logo";
img.style.width = "24.469px"; // same size as default SVG
img.style.height = "28px";
img.style.objectFit = "contain";
zugdev marked this conversation as resolved.
Show resolved Hide resolved

// Replace the existing SVG with the new image
const svgLogo = brandingDiv.querySelector("svg#logo");
if (svgLogo) brandingDiv.replaceChild(img, svgLogo);
}

// Update the organization name inside the span with class 'full'
const orgNameSpan = brandingDiv.querySelector("span.full");
if (orgNameSpan) orgNameSpan.textContent = `${orgName} | `;
}
92 changes: 52 additions & 40 deletions static/progressive-web-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,75 +14,87 @@ const urlsToCache = [
// Install event (caches all necessary files)
self.addEventListener("install", (event) => {
Copy link
Member

Choose a reason for hiding this comment

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

Async this instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like this? self.addEventListener("install", async (event) => { not sure why though

event.waitUntil(
caches
.open(cacheName)
.then((cache) => {
return cache.addAll(urlsToCache);
})
.catch((error) => console.error("[Service Worker] Cache failed:", error))
(async () => {
try {
const cache = await caches.open(cacheName);
await cache.addAll(urlsToCache);
console.log("[Service Worker] Files cached successfully");
} catch (error) {
console.error("[Service Worker] Cache failed:", error);
}
self.skipWaiting(); // Activate the new worker immediately
})()
);
self.skipWaiting(); // activate the new worker immediately
});

// Activate event (deletes old caches when updated)
self.addEventListener("activate", (event) => {
event.waitUntil(
caches
.keys()
.then((cacheNames) => {
return Promise.all(
(async () => {
try {
const cacheNames = await caches.keys();
await Promise.all(
cacheNames.map((name) => {
if (name !== cacheName) {
return caches.delete(name);
}
})
);
})
.catch((error) => console.error("[Service Worker] Error during activation:", error))
console.log("[Service Worker] Old caches removed");
} catch (error) {
console.error("[Service Worker] Error during activation:", error);
}
self.clients.claim(); // Take control of all pages immediately
})()
);
self.clients.claim(); // Take control of all pages immediately
});

// Fetch event: Cache first approach but update cache anyways
// Fetch event: Cache first approach but update cache anyway
self.addEventListener("fetch", (event) => {
const url = new URL(event.request.url);

// Ignore non-HTTP(S) requests (like 'chrome-extension://')
if (url.protocol !== "http:" && url.protocol !== "https:") {
return;
}
if (url.protocol !== "http:" && url.protocol !== "https:") return;

// If the request has query parameters, bypass the cache
if (url.search) {
if (url.search) {
event.respondWith(fetch(event.request));
return;
}

event.respondWith(
caches.match(event.request).then((cachedResponse) => {
const fetchPromise = fetch(event.request)
.then((networkResponse) => {
// Clone the network response to avoid using the body twice
const responseClone = networkResponse.clone();
(async () => {
try {
const cachedResponse = await caches.match(event.request);

// If the network response is valid, update the cache
if (networkResponse.ok) {
caches.open(cacheName).then((cache) =>
cache.put(event.request, responseClone)
const fetchPromise = fetch(event.request)
.then(async (networkResponse) => {
if (networkResponse.ok) {
const responseClone = networkResponse.clone();
const cache = await caches.open(cacheName);
await cache.put(event.request, responseClone);
}
return networkResponse;
})
.catch((error) => {
console.error("[Service Worker] Network request failed:", error);
return (
cachedResponse ||
new Response("Offline content unavailable", {
status: 503,
statusText: "Service Unavailable",
})
);
}
return networkResponse;
})
.catch((error) => {
console.error("[Service Worker] Network request failed:", error);
return cachedResponse || new Response("Offline content unavailable", {
status: 503,
statusText: "Service Unavailable",
});
});

// Serve from cache first, but update the cache in the background
return cachedResponse || fetchPromise;
})
return cachedResponse || (await fetchPromise);
} catch (error) {
console.error("[Service Worker] Error handling fetch:", error);
return new Response("An error occurred", {
status: 500,
statusText: "Internal Server Error",
});
}
})()
Copy link
Member

Choose a reason for hiding this comment

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

It's strange that you do self invoking functions to do async. Just wrap the entire callback inside an async function like we discussed here and then you can await them like normal.

Also change catch to try catch. And don't use thenable, use async await.

Copy link
Contributor Author

@zugdev zugdev Oct 31, 2024

Choose a reason for hiding this comment

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

The self invoking functions are needed because event's waitUntil() method receives a promise as a parameter - not a function that returns a promise, (see reference). I can only imagine that your idea was to remore the use of waitUntil entirely, for instance:

// Install event (caches all necessary files)
self.addEventListener("install", async (event) => {
  try {
    const cache = await caches.open(cacheName);
    await cache.addAll(urlsToCache);
    console.log("[Service Worker] Files cached successfully");
  } catch (error) {
    console.error("[Service Worker] Cache failed:", error);
  }
  self.skipWaiting(); // Activate the new worker immediately
});

instead of:

// Install event (caches all necessary files)
self.addEventListener("install", async (event) => {
  event.waitUntil(
    (async () => {
      try {
        const cache = await caches.open(cacheName);
        await cache.addAll(urlsToCache);
        console.log("[Service Worker] Files cached successfully");
      } catch (error) {
        console.error("[Service Worker] Cache failed:", error);
      }
      self.skipWaiting(); // Activate the new worker immediately
    })()
  );
});

The difference between these approaches is that with waitUntil() adding assets to cache becomes an asynchronous task, which would allow the task to be completed even after returning inside the event handler. During install this is not a problem, however in a fetch event I want the service worker to return whatever is in cache, but fire an asynchronous task to update the cache with a network response. If we remove waitUntil() and just await the network response, we would have to wait for network response to return whatever is in cache, seriously compromising speed in very slow networks.

Here is a QA using waitUntil() to return from cache and asynchronously update cache with network response, in a simulated 10kbps network speed:

10kbps-network.webm

Here is a QA awaiting the network response, in a simulated 10kbps network speed:

10kbps-network-await.webm

Here are some additional references to clarify:

What does event.waitUntil do in service worker and why is it needed?

JavaScript how to use event.waitUntil() to wait for an async function call

Should I use event.waitUntil() or await fetch() or just fetch()?

Copy link
Member

Choose a reason for hiding this comment

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

Okay seems like you did your research so let's do what you think is right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was good feedback, it's very important that we understand each others code practices, as well as document weird patterns. Thanks.

);
});
Loading
Loading