Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat: make display limit configurable from user settings#60761

Merged
jtibshirani merged 4 commits into
sourcegraph:mainfrom
namit-chandwani:feat/make-display-limit-configurable
May 14, 2024
Merged

feat: make display limit configurable from user settings#60761
jtibshirani merged 4 commits into
sourcegraph:mainfrom
namit-chandwani:feat/make-display-limit-configurable

Conversation

@namit-chandwani

@namit-chandwani namit-chandwani commented Feb 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Linked Issues

Motivation and Context:

  • To provide users the flexibility to modify the display limit as per their requirement
  • However, it's worth noting that increasing the display limit could potentially result in decreased performance

Changes Made:

  • On the frontend:
    • Added a new field named search.displayLimit to the User settings
    • Started using the search.displayLimit value while performing stream search
  • On the backend:
    • No changes

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (altering code without changing its external behaviour)
  • Documentation change
  • Other

Checklist:

  • Development completed
  • Comments added to code (wherever necessary)
  • Documentation updated (if applicable)
  • Tested changes locally

Follow-up tasks (if any):

Test Plan

  • Had setup the codebase locally and tested the entire flow locally end-to-end
  • Screen recording which shows the display limit from the newly added site configuration parameter being used while performing code search:
Trimmed.SR.mov

Additional Comments

  • Please let me know if any further changes are required elsewhere

@cla-bot cla-bot Bot added the cla-signed label Feb 27, 2024
@namit-chandwani namit-chandwani mentioned this pull request Feb 27, 2024
Merged
@namit-chandwani namit-chandwani force-pushed the feat/make-display-limit-configurable branch from dfdba91 to 7998788 Compare February 27, 2024 09:07
@namit-chandwani namit-chandwani mentioned this pull request Feb 27, 2024
Closed
@stefanhengl stefanhengl self-requested a review March 13, 2024 12:44
@stefanhengl

stefanhengl commented Mar 13, 2024

Copy link
Copy Markdown
Member

Thanks for the PR! I really appreciate the thorough description and the video! Nice job! However, I think we should avoid changing the API if we can. So another option would be to keep the query parameter, but instead let the client get the display limit from the config. WDYT?

Thinking about it ... I know this was my suggestion, but I think "settings" might be a better choice than site config, because users can override it instead of having to ask an admin to change it for everyone. We could use "search.displayLimit" as key.

There is a function useSettings() in "settings/settings.tsx" which might come in handy.

This design wouldn't require any backend or API changes.

@stefanhengl

stefanhengl commented Apr 10, 2024

Copy link
Copy Markdown
Member

@namit-chandwani friendly ping 👋 . Are you planning to work on this in the near term? It's absolutely fine if not, but we started tracking this on our board and I just need to know if this is actively being worked on or not.

@namit-chandwani

namit-chandwani commented Apr 10, 2024

Copy link
Copy Markdown
Contributor Author

@stefanhengl Hey, I was travelling back then so didn't get a chance to look into this.

I'll resume my work on this feature today and push the new changes within a few days :)

@namit-chandwani namit-chandwani force-pushed the feat/make-display-limit-configurable branch from d7296d9 to 3420049 Compare April 15, 2024 16:43
stefanhengl
stefanhengl reviewed Apr 18, 2024
View reviewed changes
Comment thread client/shared/src/search/stream.ts Outdated
@namit-chandwani namit-chandwani force-pushed the feat/make-display-limit-configurable branch from 3420049 to b75a0b0 Compare April 25, 2024 22:17
@namit-chandwani namit-chandwani changed the title feat: make display limit configurable from site config feat: make display limit configurable from user settings Apr 25, 2024
@namit-chandwani namit-chandwani force-pushed the feat/make-display-limit-configurable branch from b75a0b0 to 9028ba6 Compare April 25, 2024 22:51
@namit-chandwani namit-chandwani mentioned this pull request Apr 25, 2024
Merged
@namit-chandwani

namit-chandwani commented Apr 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hey @stefanhengl, apologies for the delay.

I've pushed the changes based on the updated requirements now:

  • no change to be made on the backend API
  • displayLimit needs to be configured on the user settings instead of the site admin config

Here's the new screen recording of the testing performed locally after making these changes:

Screen.Recording.2024-04-26.at.4.17.08.AM.mov

Request you to review the changes whenever free

MaedahBatool referenced this pull request in sourcegraph/docs May 7, 2024
@namit-chandwani
docs: move displayLimit field description from admin site config to u…
5ce77bf 
…ser settings docs page (#279)

## Description

- Changes to the documentation based on:
	- Issue: https://github.com/sourcegraph/sourcegraph/issues/36691
	- PR: https://github.com/sourcegraph/sourcegraph/pull/60761
stefanhengl
stefanhengl approved these changes May 14, 2024
View reviewed changes

@stefanhengl stefanhengl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! This turned out really nice! Thanks for your work!

jtibshirani
jtibshirani reviewed May 14, 2024
View reviewed changes
Comment thread client/web-sveltekit/src/routes/search/+page.ts
featureOverrides: [],
chunkMatches: true,
searchMode,
// TODO: populate this from user settings

@jtibshirani jtibshirani May 14, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CC @sourcegraph/code-search I added this TODO. Our external contributor @namit-chandwani has been very patient with us, and I didn't want to hold up this PR to implement it.

@camdencheek camdencheek May 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Will follow up after it's merged

@camdencheek camdencheek May 14, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Closing the loop

@jtibshirani

jtibshirani commented May 14, 2024

Copy link
Copy Markdown
Contributor

Merging on behalf of @stefanhengl since he's out. Thanks again for your contribution !

@jtibshirani jtibshirani merged commit 111503d into sourcegraph:main May 14, 2024
@camdencheek camdencheek mentioned this pull request May 14, 2024
Merged
@namit-chandwani namit-chandwani deleted the feat/make-display-limit-configurable branch May 15, 2024 14:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@jtibshirani jtibshirani jtibshirani left review comments

@camdencheek camdencheek camdencheek left review comments

@stefanhengl stefanhengl stefanhengl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

search: make display limit configurable in site configuration

4 participants

@namit-chandwani @stefanhengl @jtibshirani @camdencheek