-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: Revise length of generated GitHub issue link to avoid hitting URI max length #1336
Conversation
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.
Thanks for addressing. This seems like a safe enough solution to move forward with!
On the alternatives, GH API would definitely be a solid and long lasting solution. The in-app view sounds like a larger buildout of the /conflicts
page. And with it just being considered an alternative (but always available) if GH cannot take all the content doesn't really place it as too much of a con for me! At a glance, the latter feels more favorable to me right now.
There may also be very minor changes and removal of some unnecessary white space in the template builder but not enough savings for me to require that be done right now.
Other than that, left an inline comment on the truncated text message.
client/utils/createIssueLink.js
Outdated
// This code assumes that URLs can only become too long with conflict markdown present | ||
if (conflictMarkdown && url.length > MAX_GITHUB_URL_LENGTH) { | ||
// If URL is too long, truncate the conflict markdown section | ||
const truncationMessage = `\n\n[Content truncated due to URL length limits. Please visit [${window.location}](${window.location}) and click "Review Conflicts" to review the full conflict information.]`; |
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.
Since we now have the conflicts page, this message may have to hold additional context or be more generic. If it's done from the /conflicts
, there wouldn't be a "Review Conflicts" button to click.
If you want to avoid passing in additional context, maybe something like 'Content truncated due to URL length limits. Please visit link to review the conflicts.' (and if testSequenceNumber
is available, could also append something along the lines of "for Test sequenceNumber")?
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.
I forgot about that page! Thanks for reminding me. I took on your suggestion for messaging with 002aa6f
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.
Confirmed internally that the changes work as expected. Thanks for sorting this out @stalgiag!
This PR truncates conflict markdown when a "Raise an issue" link has more than 8k chars.
Assumptions:
This PR isn't entirely satisfying as it does result in truncated data in the issue. I tried to mitigate the worst effects of this by including a link back to the originating location. I looked into other options but the tradeoffs didn't seem worth it.
Alternatives
related to #1334