Add BACKLOG.md with prioritized improvement tasks for future agents - #24
Conversation
10 actionable tasks from comprehensive code analysis, covering error handling, accessibility, security, UX, and code quality improvements. https://claude.ai/code/session_01Xx3WiQTh15p986kaBy8Ja4
There was a problem hiding this comment.
Pull request overview
Adds a new BACKLOG.md to capture a prioritized set of future improvement tasks (reliability, data safety, accessibility, security, UX, and code quality) for follow-on agents working in this zero-build, zero-dependency codebase.
Changes:
- Introduces
BACKLOG.mdwith 10 prioritized improvement tasks, each including problem statements, affected files/lines, and implementation steps. - Documents agent “ground rules” for running tests and maintaining repository constraints.
- Adds a “Completed Tasks” section to track finished work over time.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| **Ground rules for agents working on these tasks:** | ||
| - Run `node .claude/hooks/run-tests.js` before and after every change | ||
| - Add tests in `tests.html` for any new logic |
There was a problem hiding this comment.
Ground rules say to run .claude/hooks/run-tests.js and also to add tests in tests.html, but the pre-commit hook only runs the Node runner. Adding a test only to tests.html won’t be enforced by the automated test gate unless it’s also added to run-tests.js (or the runner is updated to execute the browser suite). Consider clarifying this so contributors know where new tests must be added.
| - Add tests in `tests.html` for any new logic | |
| - For any new logic, add or update tests in the Node test runner (wired into `.claude/hooks/run-tests.js`), so they are enforced by the automated test gate | |
| - Optionally mirror or supplement those tests in `tests.html` for manual, in-browser verification (note: the pre-commit hook does **not** run `tests.html`) |
| } | ||
| } | ||
| ``` | ||
| 2. Wrap `<App />` in `<ErrorBoundary>` in the `ReactDOM.createRoot` call |
There was a problem hiding this comment.
Task 1 step 2 uses JSX-style <App /> / <ErrorBoundary> wording, but the repository explicitly avoids JSX/ES modules. Suggest updating the instruction to show the equivalent React.createElement(ErrorBoundary, …, React.createElement(App)) wrapper so future agents don’t accidentally introduce JSX.
| 2. Wrap `<App />` in `<ErrorBoundary>` in the `ReactDOM.createRoot` call | |
| 2. Wrap `App` in `ErrorBoundary` in the `ReactDOM.createRoot` call, for example: | |
| ```js | |
| ReactDOM.createRoot(root).render( | |
| React.createElement( | |
| ErrorBoundary, | |
| null, | |
| React.createElement(App) | |
| ) | |
| ); |
| React.createElement('h2', null, 'Something went wrong'), | ||
| React.createElement('p', null, this.state.error && this.state.error.message), | ||
| React.createElement('button', { onClick: function() { location.reload(); } }, 'Reload'), | ||
| React.createElement('button', { onClick: function() { localStorage.clear(); location.reload(); } }, 'Reset all data') |
There was a problem hiding this comment.
The suggested “Reset all data” handler uses localStorage.clear(), which clears all keys for the origin (e.g., other apps hosted on the same GitHub Pages origin). Safer recovery is to remove only this app’s keys (e.g., n5_2025, n5_srs, n5_day, n5_completed, n5_furigana) or those matching a prefix, and then reload.
| React.createElement('button', { onClick: function() { localStorage.clear(); location.reload(); } }, 'Reset all data') | |
| React.createElement('button', { onClick: function() { | |
| try { | |
| ['n5_2025', 'n5_srs', 'n5_day', 'n5_completed', 'n5_furigana'].forEach(function(key) { | |
| localStorage.removeItem(key); | |
| }); | |
| } catch (e) { | |
| console.error('Failed to reset app data', e); | |
| } | |
| location.reload(); | |
| } }, 'Reset all data') |
| 2. **Quiz buttons:** Add `role="button"` and `aria-label` with the answer text | ||
| 3. **Feedback messages:** Add `aria-live="polite"` to exercise result text | ||
| 4. **Navigation:** Add `role="navigation"` to header, `role="main"` to content area | ||
| 5. **Review cards:** Make card flippable via Enter/Space key (add `tabIndex={0}` | ||
| and `onKeyDown` handler) |
There was a problem hiding this comment.
In Task 3, adding role="button" to actual <button> elements is redundant and can be counterproductive; focus on accessible naming (aria-label), state (aria-pressed for toggles), and keyboard handlers where the element is not a native button. Also tabIndex={0} is JSX syntax and conflicts with the “no JSX” rule—use the non-JSX prop form (e.g., tabIndex: 0) in createElement calls.
| 2. **Quiz buttons:** Add `role="button"` and `aria-label` with the answer text | |
| 3. **Feedback messages:** Add `aria-live="polite"` to exercise result text | |
| 4. **Navigation:** Add `role="navigation"` to header, `role="main"` to content area | |
| 5. **Review cards:** Make card flippable via Enter/Space key (add `tabIndex={0}` | |
| and `onKeyDown` handler) | |
| 2. **Quiz buttons:** Ensure each quiz answer control has an accessible name (e.g., | |
| an `aria-label` with the answer text); if it is not a native `<button>`, add | |
| `role="button"` and keyboard handlers for Enter/Space. | |
| 3. **Feedback messages:** Add `aria-live="polite"` to exercise result text | |
| 4. **Navigation:** Add `role="navigation"` to header, `role="main"` to content area | |
| 5. **Review cards:** Make card flippable via Enter/Space key (set `tabIndex: 0` | |
| and an `onKeyDown` handler via the `createElement` props) |
| 2. Call `sanitizeSvg` on the fetch result before passing to `dangerouslySetInnerHTML` | ||
| 3. Add a Content-Security-Policy `<meta>` tag to `index.html`: | ||
| ```html | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'self' https://cdnjs.cloudflare.com; script-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com; style-src 'self' 'unsafe-inline';"> |
There was a problem hiding this comment.
The CSP snippet in Task 4 includes script-src 'unsafe-inline', which would still allow inline event-handler execution (one of the main SVG XSS risks) and therefore doesn’t meaningfully mitigate injected-SVG XSS. If CSP is intended as a defense-in-depth measure here, consider either (a) moving inline JS/CSS into external files so 'unsafe-inline' can be removed (or replaced with hashes/nonces), or (b) explicitly call out that this CSP is mainly about restricting remote origins and isn’t sufficient by itself.
| <meta http-equiv="Content-Security-Policy" content="default-src 'self' https://cdnjs.cloudflare.com; script-src 'self' 'unsafe-inline' https://cdnjs.cloudflare.com; style-src 'self' 'unsafe-inline';"> | |
| <meta http-equiv="Content-Security-Policy" content="default-src 'self' https://cdnjs.cloudflare.com; script-src 'self' https://cdnjs.cloudflare.com; style-src 'self' 'unsafe-inline';"> |
| 2. Consolidate into a single CSS class hierarchy | ||
| 3. Remove duplicate declarations | ||
| 4. Extract hardcoded level colors (line ~1253: N5=#e91e8c, etc.) into CSS custom | ||
| properties that reference the `PHASE_COLORS` constants |
There was a problem hiding this comment.
Task 7 suggests extracting hardcoded colors into “CSS custom properties that reference the PHASE_COLORS constants,” but CSS can’t directly reference JS variables. To avoid confusion, consider rephrasing to either duplicate the values in CSS variables (and note they must be kept in sync) or set CSS variables at runtime from PHASE_COLORS (e.g., writing to document.documentElement.style).
| properties that reference the `PHASE_COLORS` constants | |
| properties, and either duplicate the corresponding `PHASE_COLORS` values (noting | |
| they must be kept in sync) or set those CSS variables at runtime from | |
| `PHASE_COLORS` (for example via `document.documentElement.style`) |
10 actionable tasks from comprehensive code analysis, covering error
handling, accessibility, security, UX, and code quality improvements.
https://claude.ai/code/session_01Xx3WiQTh15p986kaBy8Ja4