Skip to content
This repository has been archived by the owner on Mar 27, 2019. It is now read-only.

Add pagination to secrets list + fix case sensitivity #110

Merged
merged 6 commits into from
Apr 30, 2017

Conversation

djenriquez
Copy link
Owner

@djenriquez djenriquez commented Apr 29, 2017

Borrowing from the Token management's display, adding pagination to secrets hopefully to solve #109.

Improved filtering by removing case-sensitivity, #106 (comment), and allowing the last known matches to stay displayed rather than clearing the list.

@djenriquez djenriquez requested a review from msessa April 29, 2017 07:11
Copy link
Collaborator

@msessa msessa left a comment

Choose a reason for hiding this comment

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

Pagination works really well. Tested with 1000+ secrets and the UI performance was excellent

return item.includes(v);
})
})
let filtered = v ? _.filter(this.state.fullSecretList, (item) => { return item.includes(v.toLowerCase()); }) : this.state.fullSecretList;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For truly case-insensitive search you need to lowercase both search text and search predicate. In this case it should be:
let filtered = v ? _.filter(this.state.fullSecretList, (item) => { return item.toLowerCase().includes(v.toLowerCase()); }) : this.state.fullSecretList;

Copy link
Owner Author

Choose a reason for hiding this comment

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

Interesting, I had thought Vault was case-insensitive, so I figured i'd save on some compute cycles, but apparently thats not the case. Great catch, fix inc.

@djenriquez
Copy link
Owner Author

djenriquez commented Apr 30, 2017

Changes are in, but the Docker build seems to be failing on

Step 5 : RUN npm install --silent && npm run build-web && npm prune --silent --production
 ---> Running in 630acfeb3091
/app/node_modules/electron/install.js:47
  throw err
  ^
Error: ENOENT: no such file or directory, lstat '/app/node_modules/electron/dist/resources'

I think NPM is just acting up right now, definitely seen similar behavior before. I actually can't even build master. I'll try again tomorrow morning before merging 👍

@msessa
Copy link
Collaborator

msessa commented Apr 30, 2017

That's odd. TravisCI changed something in their build image perhaps?

Anyway, good job on those changes! LGTM merge at will :)

@djenriquez djenriquez merged commit 59385e8 into master Apr 30, 2017
@djenriquez djenriquez deleted the feature/secrets-pagination branch April 30, 2017 18:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants