feat(server): add DefaultShareExpiration config option #4082
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to ae98410 in 1 minute and 25 seconds. Click for details.
- Reviewed
67lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. conf/configuration.go:75
- Draft comment:
Add a doc comment for DefaultShareExpiration (its units and purpose) to improve clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is asking for documentation which is generally good. However, the rules state "Do NOT make purely informative comments" and "Do NOT comment unless there is clearly a code change required." Documentation requests could be considered informative rather than requiring actual code changes. Additionally, the field's purpose seems fairly self-evident from its name and context among other sharing-related fields. The field's purpose may not be completely obvious to all developers. Having clear documentation could help prevent misuse or confusion about the time unit expectations. While documentation is valuable, this comment doesn't point out a clear problem that needs fixing. The field name is descriptive, its type (time.Duration) is clear, and its default value provides context about expected usage. The comment should be deleted as it requests purely informative documentation without identifying a clear code issue that needs to be fixed.
2. core/share.go:98
- Draft comment:
Nice refactor—using conf.Server.DefaultShareExpiration replaces a hardcoded value. Consider adding tests for custom expiration configs. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment correctly identifies the change, suggesting tests is not strictly necessary here. The change itself is simple - just using a config value instead of a hardcoded one. Testing config values is typically done at a higher level, and the actual logic hasn't changed. The comment is more of a nice-to-have suggestion rather than identifying a clear issue that needs to be fixed. The suggestion for tests could be valuable for ensuring the config integration works correctly. Configuration changes can have subtle bugs. However, per the rules, we should not make comments unless there is clearly a code change required. Testing suggestions, while valuable, are not critical issues that need to be fixed. The comment should be removed as it's more of a suggestion than identifying a clear issue that needs to be fixed.
3. ui/src/index.css:3
- Draft comment:
CSS cleanup: font-family declaration formatting streamlined. Ensure consistency with project style. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_ju13mKxZOArrVU1r
You can customize 
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5bd2cdb in 30 seconds. Click for details.
- Reviewed
25lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. ui/src/index.css:3
- Draft comment:
Reformatted 'font-family' for body improves readability; ensure consistent style across the project. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. ui/src/index.css:11
- Draft comment:
Multi-line formatting for 'code' font-family is clear and consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_ztEzWDXiuoFoqxLZ
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 9c56632 in 39 seconds. Click for details.
- Reviewed
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. core/share.go:8
- Draft comment:
Reordered import for 'conf' is fine if consistent with our grouping conventions. Ensure that internal packages are grouped together (e.g., after third‑party packages) as per project standards. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. core/share.go:96
- Draft comment:
When setting s.ExpiresAt using conf.Server.DefaultShareExpiration, ensure that the config value is properly validated/non‐zero to avoid unexpected expiration behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_YhyvYqOUr8SZj8y9
You can customize
LynxCub
approved these changes
May 20, 2025
|
Thanks! We also need a PR to add this option to the docs |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#4071