-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[7641] Multiple share fixes (parents, access to attachments with credentials, exclude note from share) #7759
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @x1arch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the sharing mechanism to properly handle parent-child relationships in shared notes by introducing a new URL path structure. It also adds a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly implements a new URL structure for shared notes to handle multiple parent paths and adds a #shareExclude attribute. However, there's a critical issue with how relative paths to assets and API endpoints are constructed throughout the code. With the new nested URL structure, many of these paths will be broken. I've provided suggestions to fix these paths. Additionally, I've pointed out a minor code simplification in the new route handler.
|
I faced with issue, browser do not sent Authorization header for pictures and attachments. I added cookie based authorization and query based authorization (for browsers with disabled cookies), could be configured by label Added two attributes for modify share template, now it could be possible to hide left panel ( |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…s for hide parts of share template
|
@x1arch , would it be possible for you to split the functionalities in different PRs? |
Not sure about that, could you provide real config example which could broke anything? I've add one more section in path can't imagine rewrite rule which could be broken by that. By default it will use the same behavior like now, just take random parent and show it. I tested it with caddy and nginx it works as expected without any modifications.
Of course I could, but that will mean, the path changed PR never will be merged, right?
They could update rewrite rules, isn't it? PS in theory we could add parent id to cookie and allow switch it to path by the label, as it made with access. I don't like this option because it is implicit behavior, but you're the boss. |
I'll investigate.
Not necessarily, I'm not against the idea but I think we need to refine it first. Nevertheless, splitting it helps things get merged faster.
They sure could, but it's best to avoid breaking changes. We have to balance the benefit of the change versus the impact.
What about using query parameters to specify the path? That shouldn't break anything, right? |
I don't like it, when we use path, we do not need modify template all works as is (I mean the left menu, bottom navigation), with query we need to modify every link. And it's looking ugly, like not from here, it's ok, if user don't see it (as with attachments) but for address line, no sir. |
share/{share_parent_id}/{share_note}Cloned collection, after sharing, on shared page have generated wrong links for left menu, when you have more than 2 clones. #7641#shareExcludefor exclude note from share totally, implemented for remove scripts from share.It fix my problem, will be grateful if someone test it more precisely.
I tested attachments, images all is works like a charm. Old routes
/{note_id}redirects by default to/{parent_id}/{note_id}.