Skip to content

Recurring review comments: 13 proposed changes #6319

Open
Open
Recurring review comments: 13 proposed changes#6319

Description

@rtibblesbot
rtibblesbot
opened on Oct 18, 2026

❌ This issue is not open for contribution. Visit Contributing guidelines to learn about the contributing process and how to find suitable issues.

Reviewers keep making these points on PRs to this repo, each on 3+ PRs by 2+ authors in the last 90 days. Each proposes an AGENTS.md line or a lint rule that would have prevented them.

Acceptance criteria

Each change is made, or declined in a comment here.

  • Tests assert outcomes through translator references and roles, not literals, internals or presence
  • Prefer lodash and existing utilities over hand-rolled helpers
  • Accessible names, live regions and focus follow ARIA/WCAG patterns
  • Document parse/build state shapes and non-obvious helpers with JSDoc
  • Rebuilt XML must be valid against the XSD and round-trip losslessly
  • Reuse shared XML helpers in QTIEditor code and tests
  • Batch backfill commands and use cached sizes
  • Keep the PR in scope
  • One source of truth for shared logic and rules
  • Responsive layout must reflow at 320px and truncate long content
  • Reuse existing translated strings and shared string files
  • Fetch failures must surface an error state, not an empty state
  • Dependency bumps must compile clean with the repo toolchain

Proposed changes

Tests assert outcomes through translator references and roles, not literals, internals or presence (5 PRs, 3 authors)

AGENTS.md Testing: "Assert the resulting state, such as document HTML, the role-scoped text (getByRole('status')), or saved DB rows equal to the input, not call chains, fetch strategy or toBeInTheDocument() presence. Reference strings through their translator (tr.$tr(...)/str$()), never literals. Never query KDS/keen-ui internal classes; use within(getByTestId(...))."

Prefer lodash and existing utilities over hand-rolled helpers (3 PRs, 2 authors)

AGENTS.md Code: "Before writing a helper, use lodash (lodash/shuffle, lodash/flatMap), existing composables (shared/composables/useFetch), or a maintained library (@internationalized/number for locale numbers)."

Accessible names, live regions and focus follow ARIA/WCAG patterns (6 PRs, 3 authors)

AGENTS.md a11y: "Live regions (role="status"/aria-live) stay mounted empty and only their text changes, with one writer per region. Key sibling v-ifs. Never unmount the focused control. Use KDS widgets (KDropdownMenu, KTabsPanel, KCard :to) for their ARIA wiring. Accessible names start with the visible text. Focus stays visible on every state." Lint: vuejs-accessibility plugin.

Document parse/build state shapes and non-obvious helpers with JSDoc (4 PRs, 2 authors)

Add to AGENTS.md: "Every QTIEditor interaction's parse.js declares a @typedef for its state and JSDoc on parse and build. The JSDoc says which direction it converts (XML → state or state → XML) and what it hydrates or drops. Every utils/ module and every non-obvious algorithm or prop gets a doc comment. When you change behaviour, update any doc comment that describes it."

Rebuilt XML must be valid against the XSD and round-trip losslessly (5 PRs, 2 authors)

Add to AGENTS.md: "Every interaction's build output must pass validate_qti_item for blank and minimal states, including empty ids and empty passages. Any item the build can't reproduce faithfully must be marked read-only through isSupportedBody or an allowlist. Add a test that runs parse → build on blank and edge-case input through the XSD."

Reuse shared XML helpers in QTIEditor code and tests (6 PRs, 2 authors)

Add to AGENTS.md: "Parse and build XML with serialization/xml.js (parseXML, buildXmlNode, getContentHTML) and QTIDeclaration.fromXML. Keep all XML logic in the interaction's parse.js. On the backend, reuse the secure parser in utils/assessment/qti/validation.py; never reach for innerHTML or regex."

Batch backfill commands and use cached sizes (3 PRs, 3 authors)

Add to AGENTS.md: "Management-command backfills process rows in batches, following the existing backfill commands. Endpoints don't recompute per-channel aggregates on every request; reuse the cached_data values. Never fetch unpaginated channel lists into IndexedDB."

Keep the PR in scope (3 PRs, 2 authors)

Add to AGENTS.md: "Unrelated fixes go in their own PR against unstable. Don't change established behaviour, such as stored-value formatting, unless the issue asks you to."

One source of truth for shared logic and rules (5 PRs, 3 authors)

Rule: When a rule (a fold, a predicate, a parse/validation check) is needed in two places, put it in one exported helper next to the code it mirrors (e.g. parse.js, hints.js, shared/utils/validation.js) and call it from both; never copy it into a component computed or a second module.

Responsive layout must reflow at 320px and truncate long content (3 PRs, 3 authors)

Rule: Check every new layout at 320px and 360px (WCAG 1.4.10): no fixed widths that overflow, controls never pushed off-screen, long user text truncated with KTextTruncator, and numeric/size cells white-space: nowrap.

Reuse existing translated strings and shared string files (5 PRs, 4 authors)

Rule: Before adding a string, reuse an existing translated key or the feature's shared strings file (e.g. shared/strings/organizationStrings.js); copy spec wording verbatim; and every {placeholder} needs its meaning stated in context.

Fetch failures must surface an error state, not an empty state (4 PRs, 3 authors)

Rule: A failed load must show a load-failure message (and route the error through errors/handleAxiosError), never the empty-list message; one failed action must not hide already-loaded rows; backend list endpoints log and skip a bad row instead of failing the whole response.

Dependency bumps must compile clean with the repo toolchain (4 PRs, 2 authors)

Rule: When bumping a dependency, recompile requirements.txt and requirements-dev.txt together with the repo's usual compile tool so uv pip sync resolves and extras are kept; check the new version's Node/Python engines against the pinned runtime; note any KDS bump and its reason in the PR body.

Activity

  1. rtibbles
    changed the title [-]Recurring review comment: Tests assert outcomes through translator references and roles, not literals, internals or presence[/-] [+]Recurring review comments: 3 proposed changes[/+] on Oct 18, 2026
  2. rtibblesbot
    changed the title [-]Recurring review comments: 3 proposed changes[/-] [+]Recurring review comments: 8 proposed changes[/+] on Oct 25, 2026

  3. rtibblesbot commented on Oct 25, 2026

    @rtibblesbot
    rtibblesbot
    on Oct 25, 2026
    ContributorAuthor

    This week's pass added 5 proposed changes:

    • Document parse/build state shapes and non-obvious helpers with JSDoc
    • Rebuilt XML must be valid against the XSD and round-trip losslessly
    • Reuse shared XML helpers in QTIEditor code and tests
    • Batch backfill commands and use cached sizes
    • Keep the PR in scope

    Written by rtibblesbot, an LLM-based coding agent.

  4. rtibblesbot
    changed the title [-]Recurring review comments: 8 proposed changes[/-] [+]Recurring review comments: 13 proposed changes[/+] on Nov 1, 2026

  5. rtibblesbot commented on Nov 1, 2026

    @rtibblesbot
    rtibblesbot
    on Nov 1, 2026
    ContributorAuthor

    This week's pass added 5 proposed changes:

    • One source of truth for shared logic and rules
    • Responsive layout must reflow at 320px and truncate long content
    • Reuse existing translated strings and shared string files
    • Fetch failures must surface an error state, not an empty state
    • Dependency bumps must compile clean with the repo toolchain

    Written by rtibblesbot, an LLM-based coding agent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions