Skip to content

[Bugfix:TAGrading] Fix pdf file view visibility#12779

Closed
williamschen23 wants to merge 1 commit into
mainfrom
fix-file-view
Closed

[Bugfix:TAGrading] Fix pdf file view visibility#12779
williamschen23 wants to merge 1 commit into
mainfrom
fix-file-view

Conversation

@williamschen23

@williamschen23 williamschen23 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Why is this Change Important & Necessary?

Bug from #12766
Bug: pdf file view is always available, which messes up other code:
image
const activeView = $('#file-view').is(':visible'); let lastLoadedFile = activeView ? $('#grading_file_name').text().trim() : localStorage.getItem('ta-grading-files-full-view-last-opened') ?? 'upload.pdf';
if file view is available, it will get the file name, which doesnt exist because file view has nothing

What is the New Behavior?

there was no need for display flex, which brought up the file view.
NOTE: this is still a little buggy, as the pdf scroll location is at the bottom somehow?

What steps should a reviewer take to reproduce or test the bug or new feature?

  1. Make gradeable rubric assignable by page number
  2. open file and rubric side by side
  3. open ta grading
  4. notice file view is always available
  5. click rubric element
  6. error

Make sure original functionality form #12766 works, Im pretty sure it does

Automated Testing & Documentation

Other information

@github-project-automation github-project-automation Bot moved this to Seeking Reviewer in Submitty Development Apr 13, 2026
@williamschen23 williamschen23 changed the title fix file view [Bugfix:TAGrading] Fix pdf file view visibility Apr 13, 2026
@codecov

codecov Bot commented Apr 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 21.66%. Comparing base (258c709) to head (3445d30).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #12779      +/-   ##
============================================
- Coverage     21.73%   21.66%   -0.07%     
- Complexity     9660     9754      +94     
============================================
  Files           268      268              
  Lines         36273    36457     +184     
  Branches        487      487              
============================================
+ Hits           7884     7900      +16     
- Misses        27906    28074     +168     
  Partials        483      483              
Flag Coverage Δ
autograder 21.32% <ø> (ø)
js 2.04% <ø> (ø)
migrator 100.00% <ø> (ø)
php 20.69% <ø> (-0.08%) ⬇️
python_submitty_utils 80.08% <ø> (ø)
submitty_daemon_jobs 91.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GarvitKhandelwal31

GarvitKhandelwal31 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

Hey @williamschen23 , I could reproduce the same error in my branch from PR #12766. When I tried again on your branch there was no error. So the PR removes the bug

Also regarding the bottom scroll, I wanted to say that this scroll is not due to your file changes, it was there in my PR as well. I tried to find the cause of it.
image

Bottom scroll is caused by PDF content overflow in #viewer at this narrow width. I think this is expected narrow-width PDF behaviour.

An issue I observed (which is maybe out of scope of this PR) is that when in mobile view seeing the PDF is very difficult due to this PDF header overlap. No doubt by this PR and the #12766 we were solving the issue #12763 and now the PDF no longer remains blocked, one can see the PDF if he scrolls enough but it looks very messy with the PDF header at narrow widht.

Screen.Recording.2026-04-13.155039.mp4

I have attached a screen recording of that.
What can we do regarding this-

  1. Maybe remove the PDF header at narrow widths
  2. Or like introduce a functionality that auto collapses the header when the person scrolls up the header.

@williamschen23

Copy link
Copy Markdown
Contributor Author
image file view browser should not be open 2)
Screen.Recording.2026-04-13.at.9.10.44.AM.mov

@GarvitKhandelwal31

GarvitKhandelwal31 commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

image file view browser should not be open 2)
Screen.Recording.2026-04-13.at.9.10.44.AM.mov

Yes I could reproduce the error, in my earlier try I forgot to change the rubric setting of the gradeable to yes in the option where they asked are student uploads a single PDF with a fixed format. If we check in no to this , no error would come even in my PR.

@williamschen23

Copy link
Copy Markdown
Contributor Author

closed bc PR got reverted

bmcutler added a commit that referenced this pull request Jun 1, 2026
### Why is this Change Important & Necessary?
Solves #12763 
#12766 changes were reverted by the PR #12784 due to a bug identified in
the PR #12779
In PR #12779 in mobile view seeing the PDF is very difficult due to this
PDF header overlap.
Screen recording of that-


https://github.com/user-attachments/assets/f8c0b147-e8b9-494c-b763-ab772d9e2cd0

### What is the New Behavior?
This PR includes all the changes from #12766 while excluding the
unnecessary portion identified in #12779. Also it fixes the poor PDF
readability in mobile view.


https://github.com/user-attachments/assets/78a5cce8-3a93-45c4-8f7a-57428873a8e2


### What steps should a reviewer take to reproduce or test the bug or
new feature?

1) Open TA Grading for a gradeable with PDF submissions.
2) Open a PDF in the Submissions and Results Browser.
3) Verify the top file header remains usable while scrolling the PDF
content.
4) Resize browser to narrow/mobile width.
5) Confirm horizontal overflow is not locking the view and PDF remains
scrollable/usable.
6) Use page navigation/jump behavior and confirm the expected page
scroll target is reached.

### Automated Testing & Documentation
No new automated tests were added in this PR (UI/layout and scroll
behavior adjustment).


### Other information
Breaking change: No.
Migrations required: No.
Security concerns: None identified.

---------

Co-authored-by: Barb Cutler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants