Fix #311: resolve log path against filteredRuns to support local time mode#312
Open
cj36457 wants to merge 1 commit into
Open
Fix #311: resolve log path against filteredRuns to support local time mode#312cj36457 wants to merge 1 commit into
cj36457 wants to merge 1 commit into
Conversation
…t local time mode When the convert-timezone toggle is on, chart labels show local-time run_start strings but the lookup in open_log_file compared against the raw `runs` global which still holds UTC run_start. The substring match failed, path resolved to empty, and the user saw a 'log file cannot be found' alert even though the log exists on disk. Match against filteredRuns first (whose run_start has been converted/stripped to mirror chart labels), then fall back to the raw runs array for runs that may have been filtered out. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
Ah you already started an implementation. I will look it over and add some comments if I have any. If you make any updates be sure to ping me so I can trigger the tests. Since right now you are a first time contributor I have to manually start them. The next PR will start them automatically 👍 |
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
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.
Closes #311.
Problem
When the "convert to local timezone" toggle is on, clicking a run on any chart shows:
…even though
pathis non-empty in the DB andlog.htmlexists on disk.Root cause
In
js/log.jsopen_log_file, the lookup compared the clicked label against the rawrunsglobal:But
runs[i].run_startholds the original UTC string from the DB, while chart labels (and the clicked value) come fromfilteredRuns, whoserun_starthas been rewritten byconvert_timezone/remove_timezonesinfilter.jsto match the displayed local time. The substring compare never matches →output = undefined→path = ""→ alert.Fix
Look up against
filteredRunsfirst (whoserun_startmirrors what the chart actually shows), then fall back to the rawrunsarray so filtered-out runs still resolve. The downstreamupdate_log_path_with_idpath keeps using rawruns/suites/tests— they share the original UTC timestamps internally so suite/test id lookup remains consistent.Evidence
Added
tests/javascript/log.test.jswith 4 cases. Reverting justlog.jswhile keeping the new test reproduces the issue's exact alert text:With the fix applied:
Full JS suite (
npm run test:js) green — no regressions.