Add forked from feature UI #3371

supersonicbyte · 2024-09-09T12:16:11Z

Adds the forked from feature on the UI. Waiting for @daveverwer to supply the icon for this feature so it can be added.
Current state is this:

Sherlouk · 2024-09-09T12:55:03Z

Sources/App/Views/PackageController/GetRoute.Model+ext.swift

+        if let forkedFromURL {
+            let repoURLNode: Node<HTML.BodyContext> = .a(
+                .href(forkedFromURL),
+                .text("repository")


Complete outsider to conversations, so if this is the agreed solution then just ignore this.

It would feel a bit nicer to me if we were able to display the original author name here perhaps? "Forked from Apple" as an example.

You took my words 😂

In discussions we talked about having it as "Forked from owner/repo.", which I think would be fine.

If we wanted to really push the boat out and be really extra, I could see this:

If the original repo isn't on SPI, it says "Forked from owner/repo." and links to GitHub.

If the original repo is on SPI and the package name (not repo name) is the same, it says "Forked from Original Owner Name" using the owner name that we already have. Thanks James for this suggestion.

If, in addition to that, the package name has changed, it says "Forked from Original Package Name by Original Owner Name"

That'd be the absolute belt and braces implementation!

Let's go for the absolute belt and braces, I'll update the PR to add those.

Thanks for making these changes! This is really close now. Just a couple of tweaks:

In the case where we have a renamed package, the link looks good but let’s split the link into two so there’s a link to the package first, then the word “by” which isn’t a link (this will help visually distinguish the package name from the owner name), then a link to the owner’s page. Like this:

I think it looks slightly odd where the package is not renamed. In some instances it's fine, but examples like XibLoc look odd and it’s not immediately obvious what “happn” is. Let’s slightly adjust this (and make it simpler) by including both the package name and the owner name in both cases, even if the package has not been renamed.

So. This:

Becomes this:

As mentioned above, this should also separate the one link into two. The first being a link to the package and the second being a link to the author. The good news is that we’ve also just reduced the number of possible code paths from three to two.

Finally, I think there’s a crossed wire with the links to GitHub. When I said “Forked from owner/repository” I meant to replace the “repository” link with a link that says the owner and the repository variables, not the words.

So, for example. This:

Becomes this:

daveverwer · 2024-09-09T14:59:00Z

Icon added:

and dark mode:

supersonicbyte · 2024-09-10T13:10:09Z

Sources/App/Controllers/API/API+PackageController+ForkedFromResult.swift

+                  let ownerName = model?.repository.ownerName,
+                  let owner = model?.repository.owner,
+                  let packageName = model?.version.packageName else {
+                return nil


@daveverwer @finestructure not entirely sure about this one. I saw that in some cases if the packageName is null we then use the repo name instead of it. Should we go with the same approach here?

Yep, falling back to repositori name is the approach we usually use. I'll review this properly shortly.

daveverwer · 2024-09-12T12:00:06Z

FrontEnd/styles/package.scss

@@ -69,12 +69,17 @@
                display: block;
                font-size: 11px;
            }
-
+            


Nit: Let's not add whitespace like this. The easiest way to fix this is to set these two options in Xcode's Editing settings.

daveverwer · 2024-09-12T12:05:01Z

Sources/App/Core/Query+Support/Joined3+Package.swift

@@ -40,4 +40,12 @@ extension Joined3 where M == Package, R1 == Repository, R2 == Version {
            .filter(Repository.self, \.$owner, .custom("ilike"), owner)
            .filter(Repository.self, \.$name, .custom("ilike"), repository)
    }
+
+    static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> {


I'll take @finestructure's view on this as he's the keeper of the Joined classes, but I wonder if this might need a more descriptive name. It makes sense to query with the packageID, but the fact that this is filtering to the default branch version too is a little hidden from whoever is calling this function.

Alternatively, we could move the filter to outside this query and let the calling code add that filter.

Don't make any changes based on what I say to this, though as I know @finestructure will have a view on it and we should go with his recommendation.

I agree with Dave, we shouldn't be filtering inside the method. If you look above, there's a version related overload and I'd do the same here. Keep the parameter, that way it's obvious at the call site that versions are filtered:

Suggested change

static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> {

static func query(on database: Database,

packageId: Package.Id,

version: Version.Kind) -> JoinedQueryBuilder<Self> {

query(on: database, version: version)

.filter(Package.self, \Package.$id == packageId)

}

Call site:

let model = try await Joined3<Package, Repository, Version> .query(on: database, packageId: packageId, version: .defaultBranch) .first()

finestructure · 2024-09-12T14:23:55Z

Sources/App/Controllers/API/API+PackageController+ForkedFromResult.swift

+import Vapor
+
+extension API.PackageController {
+     enum ForkedFromResult {


ForkedFromResult and ForkedFromInfo seem very similar. They have the same cases, almost the same associated values - could they be the same type? Is there a reason they're not? If so, perhaps the name could/should carry that concept.

They are very similar. The reason I have two types is because I thought that it might be a good thing to separate the actual result and then to initialize the model (that's gonna be used in the view layer). Same thing if you look at PackageResult and API.PackageController.GetRoute.Model. I'm not really sure. Should I just merge it into one type?

PackageResult (a db model reference type) and API.PackageController.GetRoute.Model (a view model value type) are great examples of why and when to add these types.

We could have avoided transforming data into the various types that make up GetRoute.Model but we wanted to have a plain value type view model that we can use to drive a page. That's a clearer separation but it also allows us to very easily assemble data for snapshot testing etc.

(It was also really helpful early on when Dave designed the pages and just specified in a struct what data he needed and I populated it from the db results independently. It made for a great interface.)

You'll see that some types don't get translated that way. For example we pass App.Reference and others 1:1 to GetRoute.Model - and that's because they're plain value types already. They're easy to construct for the tests and equally simple to pass around.

ForkedFromXXX seems to be the same: it's a value type and it really seems to be packaging all the same data. So it can just be the same type - we wouldn't need to transform it unless there's some other reason for it.

One such reason might be some computation that's happening. I can't think of a great example right now but we'd want to avoid having GetRoute.Model do a lot other than just package up data for a page.

Does that make sense?

I can take a closer look myself to see if the types can be collapsed if you prefer, just let me know.

The only difference FrokedFromInfo has from ForkedFromResult is this

var url: String { switch self { case .fromSPI(_, let originalOwner, _, let originalRepo, _): return SiteURL.package(.value(originalOwner), .value(originalRepo), nil).relativeURL() case .fromGitHub(let url): return url } } var ownerURL: String? { switch self { case .fromSPI(_, let owner, _, _, _): return SiteURL.author(.value(owner)).relativeURL() case .fromGitHub: return nil } }

it has some computed variables which come in handy in the view itself - so I think they are totally mergeable. But where should the resulting ForkedFrom be declared? And would be it be okay to make the fetchForkedFromResult method inside API+PackageController+GetRoute return the new ForkedFrom type and then pass it down to the Model through the initializer.

Could you please take a closer look when you find the time and propose the change, since I am not entirely sure what's the correct approach.

I've opened a PR on top of your branch with the changes here: #3388

That should sort it all out!

Thanks a lot! The comments on the PR were really helpful! I've merged it into the branch.

finestructure · 2024-09-12T14:47:11Z

Sources/App/Core/Query+Support/Joined3+Package.swift

@@ -40,4 +40,12 @@ extension Joined3 where M == Package, R1 == Repository, R2 == Version {
            .filter(Repository.self, \.$owner, .custom("ilike"), owner)
            .filter(Repository.self, \.$name, .custom("ilike"), repository)
    }
+
+    static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> {


I agree with Dave, we shouldn't be filtering inside the method. If you look above, there's a version related overload and I'd do the same here. Keep the parameter, that way it's obvious at the call site that versions are filtered:

Suggested change

static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> {

static func query(on database: Database,

packageId: Package.Id,

version: Version.Kind) -> JoinedQueryBuilder<Self> {

query(on: database, version: version)

.filter(Package.self, \Package.$id == packageId)

}

Call site:

let model = try await Joined3<Package, Repository, Version> .query(on: database, packageId: packageId, version: .defaultBranch) .first()

finestructure · 2024-09-12T14:50:17Z

Sources/App/Models/Repository.swift

+    static func find(on database: Database, for packageId: Package.Id) async throws -> Repository? {
+        return try await Repository.query(on: database)
+            .filter(\.$package.$id == packageId)
+            .first()
+    }
+


Seems to be unused?

Suggested change

static func find(on database: Database, for packageId: Package.Id) async throws -> Repository? {

return try await Repository.query(on: database)

.filter(\.$package.$id == packageId)

.first()

}

Merge ForkedFromResult and ForkedFromInfo

finestructure · 2024-09-15T12:32:40Z

There's one other thing to consider: Fork is referencing a Package.id that's resolved during ingestion. How are we dealing with package deletion or renaming (which we handle as a delete + add)? I think I know the answer but I've only thought about it briefly. It'd be good to think through what the possible errors are when that happens and if and how they are eventually corrected.

supersonicbyte · 2024-09-17T10:37:11Z

There's one other thing to consider: Fork is referencing a Package.id that's resolved during ingestion. How are we dealing with package deletion or renaming (which we handle as a delete + add)? I think I know the answer but I've only thought about it briefly. It'd be good to think through what the possible errors are when that happens and if and how they are eventually corrected.

I've been investigating this a little bit this morning and this is what I found. Package deletion and renaming happens in the Reconcile command, more precisely:

func reconcileLists(db: Database, source: [URL], target: [URL]) async throws {
    let (toAdd, toDelete) = diff(source: source, target: target)
    let insert = toAdd.map { Package(url: $0, processingStage: .reconciliation) }
    try await insert.create(on: db)
    for url in toDelete {
        try await Package.query(on: db)
            .filter(by: url)
            .delete()
    }
}

and since Package.Id is an UUID the renamed Package will have no connection the previous version of itself (the one we deleted). Any Packages that have been forked from it will have a ForkedFrom that is referencing a deleted Package.Id. This will leave us in a state that is not correct, but the Ingestion command will eventually fix this incorrect state while updating the metadata (correct me if I'm wrong). So, what we want to solve is the incorrect intermediate state that a ForkedFrom might have when a package rename/delete happens and the Ingest has not fixed it yet.

What we could is before the deletion of a Package to check if any other Package has a ForkedFrom that references it and nil it out.

As for the rename (delete + add), I am not sure how we can be certain that a package is being renamed in the Reconcile command since we operate only on Package urls, so I would wait for the Ingest to add back the ForkedFrom to the forked packages.

daveverwer · 2024-09-17T11:26:23Z

What we could is before the deletion of a Package to check if any other Package has a ForkedFrom that references it and nil it out.

I think this might be spreading this feature a little too far and wide and there may be a couple of things we can do to cope with this closer to the source of the feature.

We should ensure that any packages that refer to package ids that don't exist don't crash during their page load and treat those packages as if they are not forks (displaying no fork information at all).
When ingestion re-runs, any packages that had old and now invalid package ids get updated to point to the new one. I don't believe this will require a change, but we should check it works.

That way, we never crash and any fork information should be updated within a few hours of the rename.

supersonicbyte · 2024-09-17T11:35:57Z

What we could is before the deletion of a Package to check if any other Package has a ForkedFrom that references it and nil it out.

I think this might be spreading this feature a little too far and wide and there may be a couple of things we can do to cope with this closer to the source of the feature.

We should ensure that any packages that refer to package ids that don't exist don't crash during their page load and treat those packages as if they are not forks (displaying no fork information at all).

When ingestion re-runs, any packages that had old and now invalid package ids get updated to point to the new one. I don't believe this will require a change, but we should check it works.

That way, we never crash and any fork information should be updated within a few hours of the rename.

I agree with this. Since we have this:

extension API.PackageController.GetRoute.Model.ForkedFromInfo {
    static func query(on database: Database, packageId: Package.Id) async -> Self? {
        let model = try? await Joined3<Package, Repository, Version>
            .query(on: database, packageId: packageId, version: .defaultBranch)
            .first()

        guard let repoName = model?.repository.name,
              let ownerName = model?.repository.ownerName,
              let owner = model?.repository.owner else {
            return nil
        }

        return .fromSPI(originalOwner: owner,
                        originalOwnerName: ownerName,
                        originalRepo: repoName,
                        originalPackageName: model?.version.packageName ?? repoName)
    }
}

if the Package.Id from the ForkedFrom cannot be found we will return nil and won't show anything on the UI - so no crashes will occur. I will confirm this manually also. And as you said, the correct info will be updated in few hours in the Ingest process. I agree that it could be a overkill to do the deletion of ForkedFrom in the Reconcille, since that would mean that we would have to do a full table query every time we delete/rename a package that has been forked.

finestructure · 2024-09-17T11:51:37Z

Thanks for the additional analysis, @supersonicbyte . That matches my expectations as well.

Given that package removals / renames are rare (as are forks, at least at the moment), a window of a few hours where a fork isn't properly indicated doesn't strike me as a big problem.

What we could also do, is extend the Fork info with fallback information of the Github url, like so:

enum Fork: Codable, Equatable {
    case parentId(Package.Id, url: String)
    case parentURL(String)
}

Now even if the id is outdated, we could still switch to the parentURL code path via the url parameter and render the link as external until it's properly updated.

The fact that the url itself is also outdated isn't a big problem, because Github maintains redirects.

I mention this for sake of completeness and for consideration. I'm leaning more towards just keeping it simple and letting it self-correct within a few hours myself.

finestructure · 2024-09-17T11:54:59Z

One final note: we could also at least still indicate that it's a fork even if we can't link to it temporarily. That wouldn't require any schema changes and still communicate the most important piece of info, that it's a derived package.

supersonicbyte · 2024-09-17T11:55:08Z

Thanks for the additional analysis, @supersonicbyte . That matches my expectations as well.

Given that package removals / renames are rare (as are forks, at least at the moment), a window of a few hours where a fork isn't properly indicated doesn't strike me as a big problem.

What we could also do, is extend the Fork info with fallback information of the Github url, like so:
enum Fork: Codable, Equatable {
    case parentId(Package.Id, url: String)
    case parentURL(String)
}
Now even if the id is outdated, we could still switch to the parentURL code path via the url parameter and render the link as external until it's properly updated.

The fact that the url itself is also outdated isn't a big problem, because Github maintains redirects.

I mention this for sake of completeness and for consideration. I'm leaning more towards just keeping it simple and letting it self-correct within a few hours myself.

That's actually a clever idea! Fallback to GitHub url and they handle the redirect. I think it should be simple to add. I can implement that, but you call the shots.

supersonicbyte · 2024-09-17T12:00:32Z

One final note: we could also at least still indicate that it's a fork even if we can't link to it temporarily. That wouldn't require any schema changes and still communicate the most important piece of info, that it's a derived package.

We could do that by adding an unavailable case to the ForkedFromInfo and return it instead of nil in the query method of ForkedFromInfo.

daveverwer · 2024-09-17T12:03:55Z

We could do that by adding an unavailable case to the ForkedFromInfo and return it instead of nil in the query method of ForkedFromInfo.

If we're considering changing the enum, let's change it to also include the URL as a string so we can do the former suggestion from @finestructure rather than doing this.

finestructure · 2024-09-17T12:11:42Z

There are different enums though. Fork requires a schema change, ForkedFromInfo is just a view model change.

Bumps the npm-dependencies group with 2 updates: [postcss](https://github.com/postcss/postcss) and [stylelint-scss](https://github.com/stylelint-scss/stylelint-scss). Updates `postcss` from 8.4.45 to 8.4.47 - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.4.45...8.4.47) Updates `stylelint-scss` from 6.5.1 to 6.6.0 - [Release notes](https://github.com/stylelint-scss/stylelint-scss/releases) - [Changelog](https://github.com/stylelint-scss/stylelint-scss/blob/master/CHANGELOG.md) - [Commits](stylelint-scss/stylelint-scss@v6.5.1...v6.6.0) --- updated-dependencies: - dependency-name: postcss dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-dependencies - dependency-name: stylelint-scss dependency-type: direct:development update-type: version-update:semver-minor dependency-group: npm-dependencies ... Signed-off-by: dependabot[bot] <[email protected]>

daveverwer

This is really a great addition to the site @supersonicbyte. Thank you so much for all of your work on it and we're delighted to get it merged and live for everyone to use. 👍

Thank you

cla-bot bot added the cla-signed label Sep 9, 2024

supersonicbyte requested review from finestructure and daveverwer and removed request for finestructure September 9, 2024 12:16

Sherlouk reviewed Sep 9, 2024

View reviewed changes

daveverwer mentioned this pull request Sep 9, 2024

Image change caused by an Illustrator update #3373

Merged

supersonicbyte commented Sep 10, 2024

View reviewed changes

supersonicbyte force-pushed the add-forked-from-ui branch from 36717e8 to 168f8ff Compare September 12, 2024 10:33

supersonicbyte and others added 8 commits September 12, 2024 11:48

Add forked from list item on package view

a8a2166

Fix up tests

287172f

Added Icon and style for forked metadata item.

489b423

Improve ForkedFrom to reflect more info in UI

2e2cac4

Add icon to css

3cebb09

Fix up tests

c60f68f

Add more tests

5f360f2

Fix lint error

e286cae

daveverwer force-pushed the add-forked-from-ui branch from 168f8ff to e286cae Compare September 12, 2024 10:48

daveverwer reviewed Sep 12, 2024

View reviewed changes

finestructure requested changes Sep 12, 2024

View reviewed changes

supersonicbyte and others added 2 commits September 13, 2024 10:43

Resolve PR comments

358e8cb

Merge branch 'main' into add-forked-from-ui

99d167e

supersonicbyte requested a review from finestructure September 13, 2024 08:46

supersonicbyte and others added 4 commits September 13, 2024 11:06

Use relative URLs instead of absolute

e63f8c4

Fix tests

53e70ed

Merge ForkedFromResult and ForkedFromInfo

a6c7883

Merge pull request #3388 from SwiftPackageIndex/enum-rework

0ec21b3

Merge ForkedFromResult and ForkedFromInfo

supersonicbyte mentioned this pull request Sep 18, 2024

Update Fork to contain fallback URL #3394

Merged

dependabot bot and others added 12 commits September 18, 2024 15:35

Remove SwiftUICharts (package removed)

a5c5bb2

Add run 12 results

e2d3a69

Update Fork to containt fallback url

ee4032d

Fix tests

3a5172a

Resolve PR comments

0b40fce

Resort to fallback URL when package can't be found

57a0b21

Fix typo

6e064b0

Fix url in test

5e26cd4

Merge branch 'main' into add-forked-from-ui

6a627d1

Merge branch 'main' into add-forked-from-ui

ce07502

Merge branch 'main' into add-forked-from-ui

c9fa38c

daveverwer approved these changes Sep 24, 2024

View reviewed changes

daveverwer enabled auto-merge September 24, 2024 16:47

finestructure approved these changes Sep 24, 2024

View reviewed changes

daveverwer merged commit d3ce88b into main Sep 24, 2024
6 checks passed

daveverwer deleted the add-forked-from-ui branch September 24, 2024 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add forked from feature UI #3371

Add forked from feature UI #3371

supersonicbyte commented Sep 9, 2024

Sherlouk Sep 9, 2024

daveverwer Sep 9, 2024

supersonicbyte Sep 9, 2024

daveverwer Sep 12, 2024

daveverwer commented Sep 9, 2024

supersonicbyte Sep 10, 2024

daveverwer Sep 10, 2024 •

edited

Loading

daveverwer Sep 12, 2024

daveverwer Sep 12, 2024

finestructure Sep 12, 2024

finestructure Sep 12, 2024

supersonicbyte Sep 13, 2024

finestructure Sep 14, 2024

supersonicbyte Sep 14, 2024

finestructure Sep 15, 2024

supersonicbyte Sep 15, 2024

finestructure Sep 12, 2024

finestructure Sep 12, 2024

finestructure commented Sep 15, 2024

supersonicbyte commented Sep 17, 2024

daveverwer commented Sep 17, 2024 •

edited

Loading

supersonicbyte commented Sep 17, 2024 •

edited

Loading

finestructure commented Sep 17, 2024 •

edited

Loading

finestructure commented Sep 17, 2024

supersonicbyte commented Sep 17, 2024

supersonicbyte commented Sep 17, 2024

daveverwer commented Sep 17, 2024

finestructure commented Sep 17, 2024

daveverwer left a comment

-    static func query(on database: Database, packageId: Package.Id) -> JoinedQueryBuilder<Self> {
+    static func query(on database: Database,
+                      packageId: Package.Id,
+                      version: Version.Kind) -> JoinedQueryBuilder<Self> {
+        query(on: database, version: version)
+            .filter(Package.self, \Package.$id == packageId)
+    }

Add forked from feature UI #3371

Add forked from feature UI #3371

Conversation

supersonicbyte commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveverwer commented Sep 9, 2024

Choose a reason for hiding this comment

daveverwer Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finestructure commented Sep 15, 2024

supersonicbyte commented Sep 17, 2024

daveverwer commented Sep 17, 2024 • edited Loading

supersonicbyte commented Sep 17, 2024 • edited Loading

finestructure commented Sep 17, 2024 • edited Loading

finestructure commented Sep 17, 2024

supersonicbyte commented Sep 17, 2024

supersonicbyte commented Sep 17, 2024

daveverwer commented Sep 17, 2024

finestructure commented Sep 17, 2024

daveverwer left a comment

Choose a reason for hiding this comment

daveverwer Sep 10, 2024 •

edited

Loading

daveverwer commented Sep 17, 2024 •

edited

Loading

supersonicbyte commented Sep 17, 2024 •

edited

Loading

finestructure commented Sep 17, 2024 •

edited

Loading