Skip to content

Prefill host when creating new password #383

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 2, 2025
Merged

Prefill host when creating new password #383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a47c59
feat: prefill host when creating new password
ayykamp May 28, 2025
14191e0
fix: suggested changes
ayykamp May 29, 2025
e05d278
refactor: move getCurrentUrl to helpers/ui.js
ayykamp Jun 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/helpers/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ function parseAuthUrl(url) {
return null;
}


/**
* Prepare list of logins based on provided files
*
Expand Down
18 changes: 18 additions & 0 deletions src/helpers/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const m = require("mithril");
const dialog = require("../popup/modalDialog");
const notify = require("../popup/notifications");
const helpers = require("../helpers/base");
const BrowserpassURL = require("@browserpass/url");

const containsNumbersRegEx = RegExp(/[0-9]/);
const containsSymbolsRegEx = RegExp(/[\p{P}\p{S}]/, "u");
Expand All @@ -13,6 +14,7 @@ module.exports = {
handleError,
highlight,
withLogin,
getCurrentUrl
};

//----------------------------------- Function definitions ----------------------------------//
Expand Down Expand Up @@ -152,3 +154,19 @@ async function withLogin(action, params = {}) {
handleError(e);
}
}

/**
* Returns current url
* @param object settings Settings object to use
* @returns object Instance of BrowserpassURL
*/
function getCurrentUrl(settings) {
let url;
const authUrl = helpers.parseAuthUrl(window?.location?.href ?? null);
if (settings.authRequested && authUrl) {
url = new BrowserpassURL(authUrl);
} else {
url = new BrowserpassURL(settings.origin);
}
return url
}
5 changes: 5 additions & 0 deletions src/popup/addEditInterface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,11 @@ function AddEditInterface(settingsModel) {
} else {
// view instance should be a Login
loginObj = new Login(settings);

const url = helpersUI.getCurrentUrl(settings);

// prefill the host of the current tab
loginObj.login = url.isValid ? url.hostname : "";
}
Copy link
Member

@max-baz max-baz May 28, 2025

Choose a reason for hiding this comment

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

cc @patgmiller if you have any feedback on this PR, and also what do you think about folder autocomplete, with this PR merged the autocomplete would basically never show up, people wouldn't know that this feature even exists - I'm currently thinking if we merge this PR we need to change auto complete so that it shows folder suggestions only based on what's to the left side of the cursor, not based on the entire input of the field - so that even when the value is prefilled, user can move cursor to the left and have folders auto suggested.

Copy link
Member

@max-baz max-baz May 29, 2025

Choose a reason for hiding this comment

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

I took a stab at it myself, in #386 - that + this PR together play very nicely in my mind. Feedback welcome, from anyone!

Copy link
Contributor

@patgmiller patgmiller May 30, 2025

Choose a reason for hiding this comment

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

@maximbaz I see what you mean. Personally I love automation, especially when it blends with flexibility for me to adjust it. I'll give them a test run tomorrow and then I should have some thing more concrete to feedback.

Regardless, if you already feel good about it don't let me stop progress. I am sure we can find a way to make them work together.

Copy link
Contributor

@patgmiller patgmiller May 30, 2025

Choose a reason for hiding this comment

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

@maximbaz I checked them both out. I initially experienced what I thought was a bug with this PR, but I think it was just a side affect of the auto pre-filling of the host and my own perception. At first it looked like I couldn't edit the path after it was pre-filled.

I love the end result of both PRs together and in fact the changes with maxim's lhs folder autocomplete fixes any functional issues I felt were present with using the host autocompletion.

However I do have two question to pose.

  1. For the addEditInterface, should we we auto focus the path with the cursor positioned at the start of the path? This could be done afterwards in a separate PR and I think if we did do it, that functionality might need to be configurable (on/off).
  2. Should the host autocomplete be also an optional configuration?

Essentially I am wondering if any auto populating values should be configured as apposed to the optional autocompletion. Does that make sense? I don't personally have any issues with it currently as it is, but I can see some people not wanting forced input changes if they don't match their personal workflow preferences.

Copy link
Member

@max-baz max-baz May 30, 2025

Choose a reason for hiding this comment

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

Thank you, very helpful! 🙏

Here are some arguments against adding new options:

  • We are very minimalistic and conservative in this project when it comes to adding configuration options - I still like this approach, as it continues to force us to think more on how to provide a great UX out of the box.
  • Our selling point is that we help fighting phishing attacks by prefilling popup with the current domain - in this sense, prefilling hostname also during entry creation feels very complementary to this idea.
  • In the Organizing password store we define a convention that this project expects, saying that hostname must be somewhere in the path. If we know it must be there, then prefilling it guarantees to save time typing it, while still letting users edit the path to their preferences.

Having said that, I'm still open to consider adding e.g. file path template as an option, in light of #379 - but I suggest we take that separately. Auto-focus is an interesting idea, but if we do it I don't think we should do it configurable.

Copy link
Contributor

@patgmiller patgmiller May 30, 2025

Choose a reason for hiding this comment

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

Sounds good to me 👍🏻


// set the storePath and get tree dirs
Expand Down
12 changes: 3 additions & 9 deletions src/popup/searchinterface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module.exports = SearchInterface;
const BrowserpassURL = require("@browserpass/url");
const dialog = require("./modalDialog");
const helpers = require("../helpers/base");
const helpersUI = require("../helpers/ui");
const m = require("mithril");

/**
Expand Down Expand Up @@ -33,15 +34,8 @@ function SearchInterface(popup) {
function view(ctl, params) {
var self = this;

let url = "";
const authUrl = helpers.parseAuthUrl(window?.location?.href ?? null);
if (this.popup.settings.authRequested && authUrl) {
url = new BrowserpassURL(authUrl);
} else {
url = new BrowserpassURL(this.popup.settings.origin);
}

const host = url.hostname;
const url = helpersUI.getCurrentUrl(this.popup.settings);
const host = url.isValid ? url.hostname : "";

return m(
"form.part.search",
Expand Down