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

Initial implementation of History View #3851

Merged
merged 17 commits into from
Feb 10, 2025
Merged

Conversation

ayoy
Copy link
Collaborator

@ayoy ayoy commented Feb 7, 2025

Task/Issue URL: https://app.asana.com/0/0/1209328755192086/f

Description:
This change adds History View behind an internal-only opt-in feature flag.
It supports displaying, filtering and searching history, and opening history items.

Steps to test this PR:

  1. Run the app from Xcode (don't enable historyView feature flag yet)
  2. Press cmd+y and verify that History menu is opened.
  3. Navigate to duck://history and verify that it shows a blank page.
  4. Go to Main Menu -> Debug -> Feature Flag Overrides -> historyView to enable the flag
  5. Press cmd+y and verify that it opens History tab.
  6. Visit some pages, go back to history tab and reload it. Verify that it shows history data.
  7. Verify that you can open history links from history view.
  8. Populate fake history via Main Menu -> Debug -> History submenu.
  9. Verify that you can filter history by day (left-hand side panel) and search using the search field (please mind that keyboard navigation is not ready yet so cmd+f won't work for search).

Definition of Done:

  • Does this PR satisfy our Definition of Done?
    • Unit tests are coming soon in a separate PR, but this feature is not being released yet anyway.

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

Copy link
Contributor

github-actions bot commented Feb 7, 2025

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 3a110af

@ayoy ayoy requested a review from samsymons February 7, 2025 16:27
Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

Overall this is really solid, nice work!

I've verified that Command+Y only opens the History menu, and that it opens the full History view when the flag is enabled. One note though: the Command+Y shortcut to open the History view didn't work when enabling the flag and immediately trying to use it. After a relaunch of the app it worked fine though.

Beyond that, one issue I noticed is that the History view's search field doesn't respect dark mode:

Screenshot 2025-02-09 at 7 06 36 PM

The other issue I noticed is that I've ended up somehow with rows that have no URL value, making them unable to be opened in a tab view. See screenshot above, where there is a NY Times entry with no URL.

Finally, I also don't seem to have history displaying correctly after a refresh. After visiting Hacker News, I have entries in my History menu as expected, but refreshing the History view doesn't show them. This is also true after completely closing the tab and reopening it. (See above screenshot which shows no HNews entry but there's a tab open, and I confirmed that I reloaded the page)

@ayoy
Copy link
Collaborator Author

ayoy commented Feb 10, 2025

@samsymons thanks for the review, and thanks for reporting your findings 🙌

  1. cmd+y only works after loading the menu and history menu is populated dynamically. If the flag is toggled, the shortcut would only work after entering the menu or after app restart. I don't expect this to be an issue in production.
  2. As I was mentioning, this is a WIP feature and the UI is highly in progress, so it's expected that it doesn't support dark mode fully.
  3. Opening tabs is not final and will be worked on on the FE side. We'll definitely figure out how to open a tab that doesn't have a title.
  4. History view not respecting cmd+r was an issue a few commits back, but it should be gone now. If you can confirm that you've tested the latest commit from the branch then I agree it needs more work. (that said, your screenshot doesn't show a HNews tab).

@samsymons
Copy link
Collaborator

@ayoy Ah, wrong screenshot - here it is:

Screenshot 2025-02-09 at 7 13 21 PM

Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

LGTM now after some further testing, DB history state is working fine on both DMG and App Store targets and refreshing works as expected. Really not sure what happened here, I tested this previously through multiple relaunches of the app and browsing a bunch of tabs, and the history wasn't refreshing at all. Sorry for the trouble!

@ayoy ayoy changed the base branch from main to release/1.126.0 February 10, 2025 06:52
@ayoy ayoy merged commit 2cdf242 into release/1.126.0 Feb 10, 2025
24 checks passed
@ayoy ayoy deleted the dominik/history-view branch February 10, 2025 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants