From a1b51d9b60a991177ae9bbf56d09d6d5275a7891 Mon Sep 17 00:00:00 2001 From: vocksel Date: Wed, 18 Dec 2024 10:43:09 -0800 Subject: [PATCH] Fix crash from remembering stories that no longer exists (#314) # Problem When Flipbook attempts to reopen the last story but it does not exist in the DM (either removed from the project or swapping to a whole new experience) Flipbook will crash # Solution This boils down to an issue in `getInstanceFromFullName` where the loop does not exit in cases where the lookahead fails # Checklist - [ ] Ran `lune run test` locally before merging --- src/Common/getInstanceFromFullName.luau | 26 ++-- src/Common/getInstanceFromFullName.spec.luau | 8 ++ src/Permissions/tryGetService.luau | 21 +++ .../createTreeNodesFromPartials.spec.luau | 135 +++++++++++------- 4 files changed, 120 insertions(+), 70 deletions(-) create mode 100644 src/Permissions/tryGetService.luau diff --git a/src/Common/getInstanceFromFullName.luau b/src/Common/getInstanceFromFullName.luau index e66aa8e6..e52e9cbb 100644 --- a/src/Common/getInstanceFromFullName.luau +++ b/src/Common/getInstanceFromFullName.luau @@ -8,19 +8,9 @@ local Sift = require("@pkg/Sift") -local PATH_SEPERATOR = "." - -local function maybeGetService(serviceName: string): Instance? - local success, current: any = pcall(function() - return game:GetService(serviceName) - end) +local tryGetService = require("@root/Permissions/tryGetService") - if success and current and current:IsA("Instance") then - return current - else - return nil - end -end +local PATH_SEPERATOR = "." local function getInstanceFromFullName(fullName: string): Instance? local parts = fullName:split(PATH_SEPERATOR) @@ -30,7 +20,7 @@ local function getInstanceFromFullName(fullName: string): Instance? -- This function only works for instances in the DataModel. As such, the -- first part of the path will always be a service, so if we can't find -- one we exit out and return nil - local current = maybeGetService(serviceName) + local current = tryGetService(serviceName) if current then while #parts > 0 do @@ -52,8 +42,14 @@ local function getInstanceFromFullName(fullName: string): Instance? parts = Sift.List.shift(parts, #name:split(PATH_SEPERATOR)) break else - -- Reduce from the right until we find the next instance - tempParts = Sift.List.pop(tempParts) + if #tempParts == 1 then + -- This fixes a crash when searching for paths that + -- no longer exist + return nil + else + -- Reduce from the right until we find the next instance + tempParts = Sift.List.pop(tempParts) + end end end end diff --git a/src/Common/getInstanceFromFullName.spec.luau b/src/Common/getInstanceFromFullName.spec.luau index d8f9d153..d110cbda 100644 --- a/src/Common/getInstanceFromFullName.spec.luau +++ b/src/Common/getInstanceFromFullName.spec.luau @@ -68,3 +68,11 @@ end) test("returns nil if the first part of the path is not a service", function() expect(getInstanceFromFullName("Part")).toBeUndefined() end) + +test("returns nil if the path does not exist", function() + expect(getInstanceFromFullName("Foo.story")).toBeUndefined() + expect(getInstanceFromFullName("Path.To.Foo.story")).toBeUndefined() + expect(getInstanceFromFullName("ReplicatedStorage.Foo.Bar.Baz")).toBeUndefined() + expect(getInstanceFromFullName("ReplicatedStorage.Sample.story")).toBeUndefined() + expect(getInstanceFromFullName("ReplicatedStorage.Sample.spec")).toBeUndefined() +end) diff --git a/src/Permissions/tryGetService.luau b/src/Permissions/tryGetService.luau new file mode 100644 index 00000000..565fa95e --- /dev/null +++ b/src/Permissions/tryGetService.luau @@ -0,0 +1,21 @@ +local function tryGetService(serviceName: string): Instance? + local service + + pcall(function() + service = game:GetService(serviceName) + end) + + if service then + return service + end + + -- Some services cannot be retrieved by GetService but still exist in the DM + -- and can be retrieved by name. + pcall(function() + service = game:FindFirstChild(serviceName) + end) + + return service +end + +return tryGetService diff --git a/src/TreeView/createTreeNodesFromPartials.spec.luau b/src/TreeView/createTreeNodesFromPartials.spec.luau index 613e334b..7291a352 100644 --- a/src/TreeView/createTreeNodesFromPartials.spec.luau +++ b/src/TreeView/createTreeNodesFromPartials.spec.luau @@ -20,26 +20,34 @@ test("top-level nodes with no children", function() }, } - expect(createTreeNodesFromPartials(partials)).toEqual({ - { - label = "Node 1", - icon = "none", - isExpanded = false, - children = {}, - }, - { - label = "Node 2", - icon = "none", - isExpanded = false, - children = {}, - }, - { - label = "Node 3", - icon = "none", - isExpanded = false, - children = {}, - }, - }) + local node1 = { + id = expect.anything(), + label = "Node 1", + icon = "none", + isExpanded = false, + children = {}, + } + local node2 = { + id = expect.anything(), + label = "Node 2", + icon = "none", + isExpanded = false, + children = {}, + } + local node3 = { + id = expect.anything(), + label = "Node 3", + icon = "none", + isExpanded = false, + children = {}, + } + + local nodes = createTreeNodesFromPartials(partials) + + expect(nodes.byInstance).toEqual({}) + expect(nodes.expandedByDefault).toEqual({}) + expect(nodes.leaves).toEqual(expect.arrayContaining({ node1, node2, node3 })) + expect(nodes.roots).toEqual(expect.arrayContaining({ node1, node2, node3 })) end) test("nodes with children", function() @@ -65,41 +73,58 @@ test("nodes with children", function() }, } - expect(createTreeNodesFromPartials(partials)).toEqual({ - { - label = "Node 1", - icon = "none", - isExpanded = false, - children = { - { - label = "Child A 1", - parent = expect.anything(), - icon = "none", - isExpanded = false, - children = { - { - label = "Child B 1", - parent = expect.anything(), - icon = "none", - isExpanded = false, - children = {}, - }, - }, - }, - { - label = "Child A 2", - parent = expect.anything(), - icon = "none", - isExpanded = false, - children = {}, - }, - }, + local childB1 = { + id = expect.anything(), + label = "Child B 1", + parent = expect.anything(), + icon = "none", + isExpanded = false, + children = {}, + } + + local childA1 = { + id = expect.anything(), + label = "Child A 1", + parent = expect.anything(), + icon = "none", + isExpanded = false, + children = { + childB1, }, - { - label = "Node 2", - icon = "none", - isExpanded = false, - children = {}, + } + + local childA2 = { + id = expect.anything(), + label = "Child A 2", + parent = expect.anything(), + icon = "none", + isExpanded = false, + children = {}, + } + + local node1 = { + id = expect.anything(), + label = "Node 1", + icon = "none", + isExpanded = false, + children = { + childA1, + childA2, }, - }) + } + + local node2 = { + id = expect.anything(), + label = "Node 2", + icon = "none", + isExpanded = false, + children = {}, + } + + local nodes = createTreeNodesFromPartials(partials) + + expect(nodes.byInstance).toEqual({}) + expect(nodes.expandedByDefault).toEqual({}) + expect(nodes.leaves).toEqual(expect.arrayContaining({ childA2, childB1, node2 })) + expect(nodes.roots).toEqual(expect.arrayContaining({ node1, node2 })) end)