Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork6.3k
Add pull request files line selections#36014
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 ourterms of service andprivacy 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
| {{else}} | ||
| {{$inlineDiff := $.section.GetComputedInlineDiffFor $linectx.Locale}} | ||
| <tdclass="lines-num lines-num-old"data-line-num="{{if $line.LeftIdx}}{{$line.LeftIdx}}{{end}}"><spanrel="{{if $line.LeftIdx}}diff-{{$.FileNameHash}}L{{$line.LeftIdx}}{{end}}"></span></td> | ||
| <tdclass="lines-num lines-num-old"data-line-num="{{if $line.LeftIdx}}{{$line.LeftIdx}}{{end}}"><spanrel="{{if $line.LeftIdx}}diff-{{$.FileNameHash}}L{{$line.LeftIdx}}{{end}}"{{if $line.LeftIdx}}id="diff-{{$.FileNameHash}}L{{$line.LeftIdx}}"{{end}}></span></td> |
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.
Why duplicaterel= andid=?
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.
| {{else}} | ||
| {{$inlineDiff := $.section.GetComputedInlineDiffFor $linectx.Locale}} | ||
| <tdclass="lines-num lines-num-old"data-line-num="{{if $line.LeftIdx}}{{$line.LeftIdx}}{{end}}"><spanrel="{{if $line.LeftIdx}}diff-{{$.FileNameHash}}L{{$line.LeftIdx}}{{end}}"></span></td> | ||
| <tdclass="lines-num lines-num-old"data-line-num="{{if $line.LeftIdx}}{{$line.LeftIdx}}{{end}}"><spanrel="{{if $line.LeftIdx}}diff-{{$.FileNameHash}}L{{$line.LeftIdx}}{{end}}"{{if $line.LeftIdx}}id="diff-{{$.FileNameHash}}L{{$line.LeftIdx}}"{{end}}></span></td> |
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.
Why not have a clear defination likediff-{hash}-{L|R}xxx?
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.
| functionchangeHash(hash:string){ | ||
| if(window.history.pushState){ | ||
| window.history.pushState(null,null,hash); | ||
| }else{ | ||
| window.location.hash=hash; | ||
| } | ||
| } |
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.
Why this?
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.
If it stays, it should be moved toweb_src/js/utils/dom.ts as a general DOM utility.
But I think it's pointless to test for existancewindow.history.pushState, every browser released in the last 15 years should have that function, so we can just assume it's there, and in which case this function makes no sense and it should just usewindow.history.pushState directly.
Oh and please pass'' as second argument to it, typescript wants it that way.
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.
Oh and likely this should usewindow.history.replaceState (see other comment below).
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.
| } | ||
| exportfunctionparseDiffHashRange(hashValue:string):DiffSelectionRange|null{ | ||
| if(!hashValue.startsWith('diff-'))returnnull; |
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.
Not the first time see the same logicstartsWith('diff-' ....
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.
| } | ||
| } | ||
| if(!applyDiffLineSelection(container,range,{updateHash:false}))returnfalse; |
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.
updateHash option is an over-design.
You can simply:
if (!applyDiffLineSelection(container, range)) return false;updateHash(.....);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.
| // Scroll to the first selected line (scroll to the tr element, not the span) | ||
| // The span is an inline element inside td, we need to scroll to the tr for better visibility | ||
| awaitsleep(10); |
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.
It can't be right
silverwindNov 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Maybe it needs asleep(0) orrequestAnimationFrame? Both should flush the JS event loop of pending tasks with the latter being faster.
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.
| return{anchor, fragment, side, line}; | ||
| } | ||
| functionapplyDiffLineSelection(container:HTMLElement,range:DiffSelectionRange,options?:{updateHash?:boolean}):boolean{ |
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.
All the complex logic needs tests if it is testable in frontend unit test.
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.
Basic DOM interactions should be testable, thought I'm not sure if it's really worth it for this function.
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.
If you can fully understand the logic and confirm its right, then not worth.
silverwindNov 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Replicating the DOM structure in a unit test will be hard. Ideally we should have e2e tests for this kind of stuff, but we don't (yet). If possible, extract testable functions here which ideally have no DOM interaction.
wxiaoguangNov 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Purely logic code:
Details
wxiaoguangNov 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
And DOM operations without layout & backend:
Details
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.
Yeah such non-DOM logic is easily testable and should be tested.
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.
unit test added at4f65ae3
| // Expand the file using the setFileFolding utility | ||
| setFileFolding(container,foldBtn,false); | ||
| // Wait a bit for the expansion animation | ||
| awaitsleep(100); |
silverwindNov 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Can usehttps://developer.mozilla.org/en-US/docs/Web/API/Element/transitionend_event to detect when a transition has finished but it may be better to make setFileFolding return a Promise that resolves when the transition has ended.
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.
| awaitsleep(10); | ||
| consttargetTr=targetSpan.closest('tr'); | ||
| if(targetTr){ | ||
| targetTr.scrollIntoView({behavior:'smooth',block:'center'}); |
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 personally hate smooth scrolling, maybe scroll instantly?
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.
| window.history.pushState(null,null,window.location.pathname+window.location.search); | ||
| }else{ | ||
| window.location.hash=''; | ||
| } |
silverwindNov 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Maybe you actually want to usewindow.history.replaceState? I hate sites that push useless history entries because it pollutes the "back" stack, making the user hit the back button more than is necessary.
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.
| returnmax; | ||
| } | ||
| functionwaitForTransitionEnd(element:Element):Promise<void>{ |
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.
It is 100% garbage function .....
| // avoid reentrance when we are changing the hash to scroll and trigger ":target" selection | ||
| constattrAutoScrollRunning='data-auto-scroll-running'; | ||
| if(document.body.hasAttribute(attrAutoScrollRunning))return; | ||
| if(document.body.hasAttribute(diffAutoScrollAttr))return; |
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.
Why it is calleddiffAutoScrollAttr? See the comment, theattrAutoScrollRunning was designed for "issue comment scrolling"
| lettargetSpan=document.querySelector<HTMLElement>(`#${CSS.escape(targetId)}`); | ||
| if(!targetSpan){ | ||
| // Flush pending DOM mutations (htmx, folding animations, etc.) before giving up | ||
| awaitwaitNextAnimationFrame(); |
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.
Why it needs to wait for the animation?
wxiaoguang commentedDec 14, 2025
The logic is quite complex, I would suggest to carefully review every line and make sure every line is clear. Or you can also help to answer my questions. Thank you.@lafriks@Zettat123 |
| if(e.defaultPrevented)return; | ||
| handleDiffLineNumberClick(cell,e); | ||
| }); | ||
| window.addEventListener('hashchange',()=>{ |
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.
Why duplicate "hashchange" listeners? Won't it conflict withwindow.addEventListener('hashchange', onLocationHashChange);?
| window.location.hash=''; | ||
| window.location.hash=currentHash; |
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.
Do you think readers can understand why it needs these two lines?
When you copy code, why not also copy the comments? Or, why not introduce a common function for such requirement?
| if(targetElement){ | ||
| // Try again to highlight and scroll now that the element is loaded | ||
| constsuccess=awaithighlightDiffSelectionFromHash(); | ||
| if(success)return; |
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.
Is this loop still right?
If the selected line are in the expanded area (blob excerpt), now you just keep "loading more files", but the selected range can never be really loaded.
Uh oh!
There was an error while loading.Please reload this page.
This PR allows select multiple lines on the pull request files view page.