Refactor TabManager naming for normal active tabs, normal inactive tabs, and private tabs for consistency

[data-sync-user]

This ticket is with regards to variable naming within the TabManager protocol and implementing classes.

Definitions:

Normal tabs are tabs that where isPrivate == false.

Private tabs are tabs where isPrivate == true.

Active tabs are tabs that have a last execution time within the past 14 days.

Inactive tabs are tabs that have not been executed within 14 days.

Within the app, we consistently group tabs into one of three buckets:

1. Normal active tabs: Tabs that are not private and not stale
2. Normal inactive tabs: Tabs that are not private and are stale
3. Private tabs: Tabs that are private
   1. We do not differentiate anywhere between private “active” and private “inactive” tabs

Background:

Right now, the TabManager protocol defines the following tab arrays:

var tabs: [Tab] // All the user's tabs of every type

// These arrays are all filtered subarrays of the above `tabs`:
var normalTabs: [Tab]
var normalActiveTabs: [Tab]
var inactiveTabs: [Tab] // Proposed to rename: normalInactiveTabs
var privateTabs: [Tab]## Proposal:

[~accountid:712020:7f2701ab-94b5-436e-aaf7-39be0561ee74] proposes we rename inactiveTabs to normalInactiveTabs for symmetry with normalActiveTabs. Furthermore, inactiveTabs never returns inactive private tabs, only inactive normal tabs. So the naming should reflect that for clarity.

[~accountid:712020:15e34b1c-ddcc-48c6-b4c4-f1d6191fd1a8] has noted that if this naming changes, we should be consistent across the entire TabManager as well (functions, variables, documentation, etc.). This will involve a bit more refactoring besides just changing the array name here. As well, this could introduce mental overhead for anyone already really familiar with this code.

┆Issue is synchronized with this Jira Task

[data-sync-user]

➤ ih-codes commented:

Update: We had a good discussion in slack about this: https://mozilla.slack.com/archives/C05C9RET70F/p1726772685850449 ( https://mozilla.slack.com/archives/C05C9RET70F/p1726772685850449|smart-link )

I’ll summarize the main points here.

Matt Reagan proposed a refactor idea that actually addresses the root cause of this naming issue, which I think is the correct approach but will need further investigation:

{quote}I feel like in some ways those arrays are slightly problematic, e.g. some of them are performing a filter every time they're referenced which isn't always obvious, and they create multiple paths for accessing the same underlying tabs data, so it potentially complicates any future logic we wanted to apply to all of the getters.

Just thinking aloud but I wonder if it wouldn't be better to get rid of those public properties entirely and have a single public function like getTabs(…) which lets the caller specify the criteria they want (inactive/active, private/normal, maybe other things, getTabs(.inactive, .private)). I think there might be potential advantages to having a single callsite/entry point for every access to the current tabs. Though I also realize the current array properties we use are convenient and (if renamed accurately) readable.{quote}

ih-codes :

{quote}You have definitely touched on the bigger issue!! I was very confused when I first dug into the inactive tabs bugs because I didn't realize all the tabs were inside the top level tabs array and the subarrays are all just filters. I actually really like your proposal as well, because that makes it very explicit the tabs are coming from the same place. I think the filtered helpers are great and convenient in simpler classes, but for a larger and more complicated class they pose additional risks like you've described.{quote}

Further discussion recorded here for later reference:

Question: Do you foresee any difficulties with calling a common method vs. using the public getters for each subarray? (in terms of Redux state)

Sophie Amin :

{quote}I don't foresee any issues off the top of my head, but I have the same question about the tabs array being blocked for redux state my gut feeling says that may be an issue but probably better to hear from Orla on that{quote}

Question: Do you think we'd need to block public access to the main tabs array as well? I'm also uncertain of any implications to the redux state that holds onto the tabs.

Matt Reagan :

{quote}I have a suspicion that if it causes problems for us to privatize the underlying tab storage (replacing it with something like a public getTabs() func) that might be a code smell that we're currently doing something slightly risky. I don't think any Redux states etc. should need direct access to the underlying data, they should probably be asking essentially for an immutable snapshot at the time that the tabs are requested?It's possible I'm totally misunderstanding though, definitely something we'll want to think through (and I agree Orla's input would be good here ).{quote}