web: many improvements for 2.0 - #2671
Conversation
…e empty accounts checkbox
…e empty accounts checkbox
… operators to use true/false instead of 0/1
…t, throws an error with a stack installation location
… building from debug menu
…d tasks and launch.json
…multiplatform. Changed the hover documentation for some buttons.
…button not appear on the home page, Made the empty account checkbox on the same row as the close sidebar arrow, and made clicking the dollar amount on the sidebar open to same page as clicking the respective title to that dollar amount.
…ling the main content
…e handler invisible when the sidebar is closed.
…kbox being nonfunctional)
…ing over elemeents in top left controls
|
Super nit below. Looks good. Poking around.... note aspect ratio of graphs x-axis is stretched when you hide the accounts. Current behavior does not increase the width of the graph. |
|
I added a video #2651 (comment) demonstrating the behavior that #2651 is trying to address. This pr does not fix the issue. |
|
I lightly reviewed the code and the ui. It looks good and exposes more of hledger's capabilities to a novice hledger user (such as myself), which I think is a good goal. This is a solid incremental improvement in the functionality of hledger-web. My $.02 is to be a little more ambitious and add a home page with links to at least the journal, balance sheet, and income statement. The benefit would be to help a new user see the capabilities more easily, and it would also be generally useful with little added complexity. |
|
Thanks @AidenTHu for this PR, and @acinader for reviewing. Some code warnings need to be fixed to pass CI tests. You can see those in the CI log, or locally when you build hledger-web, or HLS will show them. The PR seems large, so I'm guessing we might want to split it up. But first, I look forward to also reviewing and giving feedback, when I can. |
simonmichael
added
A-WISH
|
Also, I notice a commit with description "[Merge branch 'codex/hledger-web-sidebar-state' of ../hledgerOther]" (2c076c2), which suggests some AI use ? |
simonmichael
changed the title
simonmichael
added
needs-review
that commit traces back to me. 2c076c2 is a merge of my commit e2b1c4a from my old PR #2644; codex/hledger-web-sidebar-state was my branch there, and that PR was AI-assisted. It was superseded by the clean-room rewrite in #2651, now merged (7b386e5 + bc624be). |
Hi, I just got back from vacation. The CI log just says that the code needs to be rebased. I'll do that. Do you have a copy of the old CI log so I can fix the warnings? As for splitting up the PR, that may take more work than I have time for since school starts in September. Can you accept as is based on my testing and @acinader's testing and then log bugs against what needs to change? |
|
I did some more testing and found some issues:
I opened AidenTHu#1 with some tests that exercise the bugs. I'm concerned that fixing 5 will cause some now-passing tests I added to start failing, but that's just a hunch. Happy to re-test as things get fixed. Also, I will switch to using your branch for my daily accounting work so I can provide feedback from actual use. AI usage: testing and test-writing done with Claude Code; findings verified by hand. |
Loading any register?...#N URL directly (bookmark, new tab, middle-click) throws during startup. Register rows have numeric IDs, and querySelector('simonmichael#3') is an invalid selector (hledger.js:111, outside try/catch). Results in a half-initialized page, so things like the date picker, sidebar handlers, and AJAX nav don't work.
Adding additional posting rows is broken
Account/description autocomplete is gone — typeahead.js was removed, and accountsCompleter is never defined, so the guard never passes.
I wasn't able to recreate #1, but I removed the # selector that was causing the problems
There was a problem hiding this comment.
While I had poked around a bit before, this code review is my attempt at a full, thorough, code review.
I am reviweing this as your contributing colleague. I am am not associated with the plaintext accounting org or the hledger project other than having contributed a few commits, and recently helping identify a few security issues (see the "Security and Quality" tab for two vulnerablitites that I helped Identify and fix over the last few days that are directly related to the code this pull request impacts).
I also have an open pull request (#2702) that has merge conflicts with this pr. Ideally you and I can come to a common view on how to resolve so the collaborators who will have to approve any change don't have to develop a point of view to resolve our conflicts.
I've posted some thoughts on the issues that this pr raises in #200 and also #2703.
One of the contributors left a comment for you on this pull request that the scope of this pr should be reduced. My suggestion is to focus on the key ui changes you introduce here, which are valuable and I think will help a new user succefully understand how to use hledger and hledger-web. I would be glad to help you by reviwing the changes to get them set for a collaborator to review for merge.
On a hunch, I set up a load test on our two pr (which I think of as my jsscrub and your ajax pr). As it currently stands, the ajax navigation doesn't properly release dom object on page reloads, so the memory usage of the browser continues to grow as you use hledger-web. An end user who uses hledger-web through a typical work day would end up with something like the results below:
Navigation stress test: 2,000 link clicks per build
Same journal (~250 transactions), same Playwright harness driving headless Chrome through the 13 register/journal pages in a cycle; memory sampled every 100 clicks after a forced GC; server process sampled with ps.
| metric (2,000 clicks) | full page loads (#2702 JS scrub) | AJAX navigation (#2671) |
|---|---|---|
| navigation latency p50 / p90 / p99 | 100 / 133 / 151 ms | 74 / 97 / 128 ms |
| full page loads counted | 2,001 | 1 |
| JS heap, start → end | 2.0 → 1.5 MB (flat) | 2.7 → 245 MB |
| retained DOM nodes | ~1,500 (flat) | 1.48 million (~1.47M detached) |
| live event listeners | 19 (flat) | 24,241 |
<script> tags in <head> |
2 (flat) | 3,850 |
| server RSS | ~54 MB, flat | ~51 MB, flat |
| server CPU per navigation | ~12.9 ms | ~8.1 ms |
The AJAX approach is ~25% faster per navigation and does ~40% less server CPU (it skips the static-asset requests; the server still renders the full HTML page either way). But the current implementation leaks linearly with no plateau — roughly 120 KB of heap, 740 detached DOM nodes, 12 listeners, and 2 <head> script tags per click. Each AJAX navigation appends the fetched page's scripts to <head> without removing the previous ones, and re-running page init registers handlers whose closures keep the previous page's DOM alive. The leak is an implementation issue rather than something inherent to AJAX navigation.
A detailed review of this pull request was really helpful for me to get better acquainted with the codebase. I use hledger and hledger-web to maintain an org's books, complicated partnership interestes, and public security investments. The bullet proof security of the web interface is very important to me, so that's the angle I am looking at changes through. The priority of the collaborators may be different, so my feedback is really just from my perspective.
Finally, I'd be super appreciative if you looked at and gave me your feedback on:
#2703, #2702, and the discusson on #200.
The most useful thing you could do for me, is build the head on #2702 and do some user testing, compare it to your branch and main and give feedback.
The two security issues I mentioned:
GHSA-vq7r-8w52-jv84
GHSA-538p-cvc4-4qjm
I hope you find the review helpful. It was a useful process for me.
Best,
Arthur
| }); | ||
|
|
||
| const utf8textdecoder = new TextDecoder(); | ||
| if (typeof utf8textdecoder === 'undefined') { |
There was a problem hiding this comment.
#2702 elimintates this encoding procedure, so we're in conflict https://github.com/plaintextaccounting/hledger/pull/2697/changes#diff-2ba4bdc6a3c1d2745594618f53b1c0563f7a6f6b964d98afe22846165a23df3eR49-R66
| vertical-align:bottom; | ||
| } | ||
|
|
||
| #sidebar-menu .main-menu tr.hide { |
There was a problem hiding this comment.
I submit that we should seek to eliminate !important from our styling and not accept any new use of it without elaborating on alternatives considered.
(ps. this references the line below. sorry)
| @@ -0,0 +1,24 @@ | |||
| { | |||
There was a problem hiding this comment.
I think you meant to delete these from the repository, but they are still here. If you now have .vscode in your .gitignore you will need to turn that off, and then make a commit to delete all of this from the repo. Once that's done, you can add vscode back into your ignore.
There was a problem hiding this comment.
It was accidentally removed from the gitignore for a time. For users who use Visual Studio Code, it is useful to keep. For those who don't, it's easily ignorable and poses no harm.
There was a problem hiding this comment.
I disagree.
hledger is pushing twenty years old. It has 178 contributors who have opened almost 1,000 pull requests and 60 releases.
Developer tooling has changed alot in that time, and the various contributors probably use a pretty good mix of environments. If even a small percent checked in their own developer tooling, there'd be alot of it in the repo. There's no documentation or test coverage, so really no guarantee it'll even work for any one other than you.
| map (Just acct,) name ++ map (Nothing,) comma | ||
|
|
||
| -- | Generate javascript/html for a register balance line chart based on | ||
| --- | Generate javascript/html for a register balance line chart based on |
| -- the provided "AccountTransactionsReportItem"s. | ||
| registerChartHtml :: Text -> String -> [(CommoditySymbol, [AccountTransactionsReportItem])] -> HtmlUrl AppRoute | ||
| registerChartHtml q title percommoditytxnreports = $(hamletFile "templates/chart.hamlet") | ||
| registerChartHtml :: Text -> String -> [(CommoditySymbol, [AccountTransactionsReportItem])] -> Widget |
There was a problem hiding this comment.
I see that it is formatting for the chart, can you help me understand why you decided to change it? I think we want to think about the chart. It presents a number of problems: jquery, its old, ugly, and presents a large surface area :).
My thinking is that we should just touch it as little as possible and instead weigh some modern alternatives. My medium term hope is that once we have cleaned up the status quo, we will be able to add some charts and graphs that would take advantage of hledger's current investment reporting as well as the 2.0 lot reporting potential. It's worth thinking about and getting right.
There was a problem hiding this comment.
I tried removing jQuery completely from the code base, but the charts required me to put it back in. I even tried upgrading to the latest version of chart.js, but couldn't get it to work and so backed out. I made the minimum number of changes to get the existing charts working. Fixing the charts properly as you say should be a different issue and PR. I for one would like a checkbox to allow the user to hide the charts if they don't want to see it, but again, that's a different issue and PR.
| document.addEventListener('DOMContentLoaded', function() { | ||
| hledgerInitGlobal(); | ||
| hledgerInitPage(); | ||
| hledgerInitAjaxNavigation(); |
There was a problem hiding this comment.
The use of of ajax is my biggest concern in this pr. I don't think ajax buys you enough to justify using it, you can get everything you want without it, and it makes the potential attack surface area enormous and unprotectable.
If performance were the issue you were trying to address, we can probably get better bang for our buck by doing some performance testing and monitoring to find slow stuff.
My first pass at an hledger-web change (#2644) introduced ajax too, but I believe that that was a mistake and I closed it. I replaced it with (#2651) which can pass the strictest Content-Security-Policy when stamped with a nonce and I'm convinced it gives 100% of the peformance and usability of an ajax solution without having to write a tone of code to do things that the browser and server are super well suited to do.
hledger-web should be bullet proof.
I didn't appreciate this when I opened #2644. Complicated, bespoke js is hard to write well. The good news is that making hledger-web bullet proof is an achieveable, valuable goal and I think we're well on our way.
I'm convinced that with some time and effort you could implement the useful ui changes that this pr introduces in a way that is in accord with #2703.
There was a problem hiding this comment.
I fixed memory leaks related to re-registering event handlers in the Ajax implementation. Further testing is appreciated to see if I caught them all.
Advantages of Ajax implementation:
- Navigating to new links in the sidebar doesn't move the position in the sidebar.
- 25% faster according to acinader's test
- It's actually tested and working now, which is the biggest one for me because school starts next week and I'll have a lot less time to devote to this project.
3 participants
I did not use AI in this PR.
feat: resizable sidebar
feat: sticky header
fix: #200
fix: mobile site no longer requires horizontal scrolling
fix: pass 100% lighthouse report for accessibility
dev: drop IE support
dev: Upgrade Bootstrap to latest version
dev: Replace jQuery with modern javascript except for flot chart dependency
doc: bump version to hledger-web 2.0