feat: add make lint-eslint-suppressions target and remove ESLint suppressions#97
Open
RudoiDmytro wants to merge 17 commits into
Open
feat: add make lint-eslint-suppressions target and remove ESLint suppressions#97RudoiDmytro wants to merge 17 commits into
RudoiDmytro wants to merge 17 commits into
Conversation
Add Bats coverage that pins the lint-eslint-suppressions exit-code contract: a positive override fixture containing all four directive variants fails with a non-zero exit, a clean override fixture passes with the success message, and a simulated grep scan error (status 2) propagates a non-zero exit instead of being masked as success. No Makefile change was required — the Story 1.1 recipe already implements the contract; this story adds the missing scan-error regression test plus explicit override-based pass/fail coverage.
This repository uses ESLint v9 flat config, so the planning artifacts' .eslintrc.js does not exist and the default scan silently missed the tooling allow-list in eslint.config.mjs:173. Point ESLINT_SUPPRESSION_SCAN_PATHS at eslint.config.mjs and update the Bats policy-placement and default-scope tests accordingly. Reword a Bats comment that contained raw directive tokens so tests/ no longer self-reports a false-positive suppression. Capture the before-cleanup inventory (3 directives: 1 tooling, 1 source, 1 test) in eslint-suppressions-baseline.md and add the Story 2.1 implementation artifact.
The only tooling inventory entry is the eslint-comments/no-use allow list at eslint.config.mjs:173. Dropping it now would make ESLint flag the two real eslint-disable directives still present in src and tests (the rule is set to error), breaking make lint-eslint. Leave the allow list in place for the MVP baseline and record the deferral rationale; it will be dropped in/after Story 2.4 once the source and test suppressions are removed. No tooling code change.
Switch the parse-failure diagnostic in http-error-response-parser from console.debug to console.warn (allowed by no-console) and delete the eslint-disable-next-line directive. The parser's return value and the HttpError thrown by assertOk are unchanged; only the log channel moves from debug to the allowed warn. Update the unit and integration spies (console.debug -> console.warn) and record the source cleanup in the baseline; make lint-eslint-suppressions now reports 2.
… test (story 2.4)
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a standalone Makefile target to inventory ESLint suppressions with Bats validation, removes in-scope suppressions via source and test fixes, tightens ESLint rule config, introduces a dependency-free reactive-var + deferred auth actions, and reorganizes many spec artifacts and docs. ChangesESLint Suppression Inventory & Cleanup
Spec Artifacts Folder Reorganization
Sequence Diagram(s)(Skipped — changes are documentation, Makefile script behavior, and multiple orthogonal refactors; a sequence diagram would require inventing runtime interactions not present in summaries.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
BLOCKER/MAJOR/MINOR notes:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 58 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: the reported issues are documentation-quality problems (severity 3–4/10) rather than runtime, security, or data-handling defects.
- The most significant issue is a factual contradiction in
specs/eslint-suppressions/implementation-artifacts/2-2-clean-up-tooling-suppressions-from-the-recorded-inventory.md, which could mislead future maintainers about whether the allowlist was deferred or removed. - Two broken internal references in
specs/eslint-suppressions/implementation-artifacts/3-2-publish-suppression-baseline-and-enforcement-decision.md(to.ralph/@fix_plan.mdand.ralph/@AGENT.md) reduce artifact reliability but are unlikely to affect product behavior. - Pay close attention to
specs/eslint-suppressions/implementation-artifacts/2-2-clean-up-tooling-suppressions-from-the-recorded-inventory.mdandspecs/eslint-suppressions/implementation-artifacts/3-2-publish-suppression-baseline-and-enforcement-decision.md- resolve the contradiction and broken references to avoid future confusion.
Architecture diagram
sequenceDiagram
participant Dev as Developer
participant Make as Makefile
participant Grep as grep
participant ESLint as ESLint
participant Parser as HttpErrorResponseParser
participant Console as Console
Note over Dev,Console: Suppression scan (new standalone target)
Dev->>Make: make lint-eslint-suppressions
Make->>Grep: grep -rnE pattern on src tests scripts eslint.config.mjs
alt Matches found (grep exit 0)
Grep-->>Make: file:line:matched text
Make-->>Dev: print matches + remediation message
Make-->>Dev: exit 1 (fail)
else No matches (grep exit 1)
Grep-->>Make: (no output)
Make-->>Dev: "No ESLint suppression directives found"
Make-->>Dev: exit 0 (pass)
else Grep error (grep exit >=2)
Grep-->>Make: error message on stderr
Make-->>Dev: propagate error, exit >=2
end
Note over Dev,ESLint: ESLint enforcement (tightened config)
Dev->>ESLint: make lint-eslint (or pre-commit)
ESLint->>ESLint: Rule 'eslint-comments/no-use' → error (no allow list)
alt ESLint disable comment present
ESLint-->>Dev: lint error, blocks commit
else No directive found
ESLint-->>Dev: pass
end
Note over Parser,Console: Logging channel change in parser
Parser->>Parser: parse() throws while cloning response body
Parser->>Console: console.warn("Failed to parse HTTP error response", { message, stack })
Note right of Console: Now allowed by no-console (allow: warn, error)
Console-->>Parser: (logged, suppression removed)
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/services/https-client/http-error-response-parser.test.ts (1)
84-104:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard console spy restoration to prevent cross-test leakage.
warnSpy.mockRestore()is not protected; if an assertion fails before the final line,console.warnstays mocked and can cascade failures into later tests. Wrap these two tests intry/finally(or move toafterEach(jest.restoreAllMocks)).Minimal patch
- it('returns a readable parsed payload when cloning the response fails', async () => { - const parser = new HttpErrorResponseParser(); - const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - - await expect( - parser.parse({ - headers: { get: () => 'application/json' }, - clone: () => { - throw new Error('clone failed'); - }, - } as unknown as Response) - ).resolves.toEqual({ message: 'clone failed', body: undefined }); - - expect(warnSpy).toHaveBeenCalledWith( - 'Failed to parse HTTP error response', - expect.objectContaining({ - message: 'clone failed', - }) - ); - warnSpy.mockRestore(); - }); + it('returns a readable parsed payload when cloning the response fails', async () => { + const parser = new HttpErrorResponseParser(); + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + try { + await expect( + parser.parse({ + headers: { get: () => 'application/json' }, + clone: () => { + throw new Error('clone failed'); + }, + } as unknown as Response) + ).resolves.toEqual({ message: 'clone failed', body: undefined }); + + expect(warnSpy).toHaveBeenCalledWith( + 'Failed to parse HTTP error response', + expect.objectContaining({ + message: 'clone failed', + }) + ); + } finally { + warnSpy.mockRestore(); + } + });Also applies to: 106-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/services/https-client/http-error-response-parser.test.ts` around lines 84 - 104, The console.warn spy in the HttpErrorResponseParser unit tests (where warnSpy is created) isn't guaranteed to be restored if an assertion fails; wrap the test body that creates warnSpy in a try/finally and call warnSpy.mockRestore() in the finally block (for both tests that create warnSpy), or alternatively add a global cleanup like afterEach(() => jest.restoreAllMocks()) in the test file; locate uses of HttpErrorResponseParser and warnSpy in the failing tests and apply the try/finally restore or the afterEach cleanup to prevent cross-test mock leakage.
🧹 Nitpick comments (1)
Makefile (1)
357-357: 💤 Low valueMinor: Unnecessary quotes on numeric exit code.
Line 357 quotes
$$grep_statusin the exit command. While harmless here, it's non-idiomatic for numeric exit codes.✨ Style improvement
- exit "$$grep_status"; \ + exit $$grep_status; \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 357, The exit command is using a quoted numeric variable ("$$grep_status") which is non-idiomatic; update the exit invocation that references $$grep_status so it passes the variable unquoted (use exit $$grep_status) to return the numeric status directly, ensuring no quotes surround the $$grep_status variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 440-451: The CLAUDE.md addition introducing BMAD "Phases" and
commands (`/bmalph`, `/bmad-help`, `/bmalph-status`, `/create-brief`,
`/create-prd`, `/create-ux`, `/create-architecture`, `/create-epics-stories`,
`/sprint-planning`, `/bmalph-implement`) must be removed from this
ESLint-suppression PR and placed in a separate PR or commit scoped to BMAD docs;
revert the CLAUDE.md changes related to BMAD in this branch, then create a new
branch/PR that adds the BMAD docs only. In the new BMAD docs PR, confirm that
`_bmad/COMMANDS.md` exists and that each referenced command name matches the
actual BMAD-METHOD implementations/command registry before publishing the
cross-reference.
In `@Makefile`:
- Around line 335-338: The Makefile variable ESLINT_SUPPRESSION_GREP_ARGS uses
the GNU-only flag --binary-files=without-match which fails on BusyBox/Alpine;
update the variable definition to use the POSIX-compatible short flag -I instead
(i.e., replace --binary-files=without-match with -I) so grep invocation in
ESLINT_SUPPRESSION_GREP_ARGS works on both GNU grep and BusyBox grep without
changing the rest of the excludes.
In
`@specs/eslint-suppressions/implementation-artifacts/2-2-clean-up-tooling-suppressions-from-the-recorded-inventory.md`:
- Around line 44-53: Update the Story 2.2 notes to indicate that the
eslint-comments/no-use allow-list referenced in the baseline was later removed
from eslint.config.mjs and is therefore a historical interim decision; add a
clear “superseded by Story 2.5 / final cleanup” note and brief rationale so the
artifact no longer contradicts current config, and apply the same edit to the
other corresponding section covering the same artifact (the lines noted in the
review).
In
`@specs/eslint-suppressions/implementation-artifacts/3-2-publish-suppression-baseline-and-enforcement-decision.md`:
- Line 94: The Bats test count is inconsistent between Story 3.1 ("Full Bats
suite 25/25 green (24 prior + 1 new)") and this story ("10/10 green"); verify
the actual total and new/prior split by running the Bats suite or checking the
test manifest, then update the incorrect story text (the "25/25..." string in
Story 3.1 or the "10/10 green" string in the 3-2 publish suppression baseline
story) so both stories report the same accurate test counts and wording.
---
Outside diff comments:
In `@tests/unit/services/https-client/http-error-response-parser.test.ts`:
- Around line 84-104: The console.warn spy in the HttpErrorResponseParser unit
tests (where warnSpy is created) isn't guaranteed to be restored if an assertion
fails; wrap the test body that creates warnSpy in a try/finally and call
warnSpy.mockRestore() in the finally block (for both tests that create warnSpy),
or alternatively add a global cleanup like afterEach(() =>
jest.restoreAllMocks()) in the test file; locate uses of HttpErrorResponseParser
and warnSpy in the failing tests and apply the try/finally restore or the
afterEach cleanup to prevent cross-test mock leakage.
---
Nitpick comments:
In `@Makefile`:
- Line 357: The exit command is using a quoted numeric variable
("$$grep_status") which is non-idiomatic; update the exit invocation that
references $$grep_status so it passes the variable unquoted (use exit
$$grep_status) to return the numeric status directly, ensuring no quotes
surround the $$grep_status variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be950d4b-2909-430d-80bd-e755cb46d9c7
⛔ Files ignored due to path filters (1)
tests/bats/make-target-coverage.tsvis excluded by!**/*.tsv
📒 Files selected for processing (57)
CLAUDE.mdMakefileeslint.config.mjsspecs/README.mdspecs/eslint-suppressions/implementation-artifacts/.gitkeepspecs/eslint-suppressions/implementation-artifacts/1-1-add-standalone-suppression-inventory-target.mdspecs/eslint-suppressions/implementation-artifacts/1-2-report-required-directive-variants-once-with-file-and-line-output.mdspecs/eslint-suppressions/implementation-artifacts/1-3-preserve-pass-fail-and-scan-error-exit-semantics.mdspecs/eslint-suppressions/implementation-artifacts/2-1-capture-current-suppression-inventory.mdspecs/eslint-suppressions/implementation-artifacts/2-2-clean-up-tooling-suppressions-from-the-recorded-inventory.mdspecs/eslint-suppressions/implementation-artifacts/2-3-clean-up-source-suppressions-from-the-recorded-inventory.mdspecs/eslint-suppressions/implementation-artifacts/2-4-clean-up-test-suppressions-from-the-recorded-inventory.mdspecs/eslint-suppressions/implementation-artifacts/2-5-record-after-cleanup-inventory-and-baseline-decision.mdspecs/eslint-suppressions/implementation-artifacts/3-1-document-standalone-workflow-placement.mdspecs/eslint-suppressions/implementation-artifacts/3-2-publish-suppression-baseline-and-enforcement-decision.mdspecs/eslint-suppressions/implementation-artifacts/eslint-suppressions-baseline.mdspecs/eslint-suppressions/planning-artifacts/architecture-eslint-suppressions-2026-04-14.mdspecs/eslint-suppressions/planning-artifacts/epics-eslint-suppressions-2026-04-14.mdspecs/eslint-suppressions/planning-artifacts/prd-eslint-suppressions-2026-04-14.mdspecs/makefile-playwright-targets/implementation-artifacts/.gitkeepspecs/makefile-playwright-targets/planning-artifacts/architecture-makefile-playwright-targets-2026-04-16.mdspecs/makefile-playwright-targets/planning-artifacts/epics-makefile-playwright-targets-2026-04-16.mdspecs/makefile-playwright-targets/planning-artifacts/prd-makefile-playwright-targets-2026-04-16.mdspecs/rust-code-analysis/implementation-artifacts/1-1-commit-repository-policy-configuration.mdspecs/rust-code-analysis/implementation-artifacts/1-2-make-lint-metrics-target-full-enforcement-reporting.mdspecs/rust-code-analysis/implementation-artifacts/2-1-github-actions-workflow-automated-analysis.mdspecs/rust-code-analysis/implementation-artifacts/2-2-baseline-compliance-verification-required-check-registration.mdspecs/rust-code-analysis/implementation-artifacts/3-1-contributor-documentation-code-quality-gate.mdspecs/rust-code-analysis/planning-artifacts/architecture-rust-code-analysis-2026-03-11.mdspecs/rust-code-analysis/planning-artifacts/epics-rust-code-analysis-2026-03-11.mdspecs/rust-code-analysis/planning-artifacts/prd-rust-code-analysis-2026-03-11.mdspecs/start-ci-chromium/implementation-artifacts/1-1-move-mockoon-into-the-development-compose-topology.mdspecs/start-ci-chromium/implementation-artifacts/1-2-start-frontend-and-mockoon-together.mdspecs/start-ci-chromium/implementation-artifacts/1-3-add-mockoon-readiness-and-failure-output.mdspecs/start-ci-chromium/implementation-artifacts/1-4-preserve-production-test-startup-after-the-mockoon-move.mdspecs/start-ci-chromium/implementation-artifacts/2-1-add-ci-environment-setup-phase.mdspecs/start-ci-chromium/implementation-artifacts/2-2-add-parallel-lint-phase.mdspecs/start-ci-chromium/implementation-artifacts/2-3-add-environment-assuming-unit-test-phase.mdspecs/start-ci-chromium/implementation-artifacts/2-4-add-top-level-make-ci-orchestration.mdspecs/start-ci-chromium/implementation-artifacts/4-1-introduce-shared-lighthouse-setup-target.mdspecs/start-ci-chromium/implementation-artifacts/4-2-update-desktop-and-mobile-lighthouse-targets-to-use-shared-setup.mdspecs/start-ci-chromium/implementation-artifacts/4-3-remove-obsolete-lighthouse-build-command-wiring.mdspecs/start-ci-chromium/implementation-artifacts/5-1-document-complete-local-startup-behavior.mdspecs/start-ci-chromium/implementation-artifacts/5-2-document-make-ci-usage-and-gnu-make-prerequisite.mdspecs/start-ci-chromium/implementation-artifacts/5-3-make-changed-targets-discoverable-through-make-help.mdspecs/start-ci-chromium/implementation-artifacts/5-4-document-make-targets-as-contracts.mdspecs/start-ci-chromium/implementation-artifacts/5-5-document-the-make-start-ci-migration-path.mdspecs/start-ci-chromium/planning-artifacts/architecture-ci-chromium-2026-04-14.mdspecs/start-ci-chromium/planning-artifacts/epics-start-ci-chromium-2026-04-14.mdspecs/start-ci-chromium/planning-artifacts/prd-start-ci-chromium-2026-04-10.mdsrc/services/https-client/http-error-response-parser.tstests/bats/eslint_suppressions.batstests/integration/services/http-error-response-parser.integration.test.tstests/integration/services/https-client-response-processing.integration.test.tstests/unit/modules/user/features/auth/components/form-section.test.tsxtests/unit/scripts/lint-metrics.test.tstests/unit/services/https-client/http-error-response-parser.test.ts
…ow-ups The pre-commit hook stages the whole working tree, so this bundles three related follow-ups made together: - Makefile: the suppression scan used GNU-only grep flags (--binary-files=without-match, --exclude-dir) that BusyBox grep in the Alpine dev container rejects, failing the eslint_suppressions Bats suite. Replace recursion + directory exclusion with find (-type d -name ... -prune) plus a portable grep -HnEI; exit semantics are preserved. Verified: Bats 26/26 green in the Alpine container. - specs/eslint-suppressions: correct review-flagged artifacts — Bats count in story 3.2 (file 10/10, full suite 26/26), drop the untracked `.ralph/@fix_plan.md` and `.ralph/@AGENT.md` references across stories 1-3/2-5/3-1/3-2 in favor of the in-repo architecture artifact, and mark the story 2.2 allow-list deferral as superseded by Story 2.5. - .claude/skills: surface the global ~/.claude/skills suite in the enforced skill-decision path — inline a task -> skill table for all 61 global skills in SKILL-DECISION-GUIDE.md and update both Mandatory Skill Check sections.
RudoiDmytro
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
f0e9d62
RudoiDmytro
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
435847e
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
RudoiDmytro
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
e6beaeb
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/modules/user/features/auth/stores/auth-var.ts (1)
5-5: ⚡ Quick winUse
@auth/alias for cross-directory import within the auth feature.The import from
../types/reactive-varcrosses directories within the auth feature tree. Per coding guidelines, use the@auth/*alias for cross-directory imports to maintain consistency.📦 Recommended fix
-import type { ReactiveVar } from '../types/reactive-var'; +import type { ReactiveVar } from '`@auth/types/reactive-var`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/user/features/auth/stores/auth-var.ts` at line 5, Replace the relative cross-directory import in auth-var.ts with the auth feature alias: change the import of ReactiveVar from "../types/reactive-var" to use the `@auth` path (e.g., import type { ReactiveVar } from '`@auth/types/reactive-var`') so the module uses the standardized alias for cross-directory imports within the auth feature.Source: Coding guidelines
src/modules/user/features/auth/stores/reactive-var.ts (1)
1-1: ⚡ Quick winUse
@auth/alias for cross-directory import within the auth feature.The import from
../types/reactive-varcrosses directories (stores/→types/) within the auth feature tree. Per coding guidelines, use the@auth/*alias for these imports to maintain consistency and readability.📦 Recommended fix
-import type { ReactiveVar, ReactiveVarListener } from '../types/reactive-var'; +import type { ReactiveVar, ReactiveVarListener } from '`@auth/types/reactive-var`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/user/features/auth/stores/reactive-var.ts` at line 1, Replace the cross-directory relative import in reactive-var.ts with the auth feature alias: update the import of the types ReactiveVar and ReactiveVarListener (symbols: ReactiveVar, ReactiveVarListener) to use the `@auth/` path (e.g. `@auth/types/reactive-var`) so the file imports via the feature alias instead of ../types/reactive-var.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 355-359: The current Makefile snippet captures output of find into
the variable scan_files (using scan_files=$$(find $$scan_paths $$prune_expr
-type f -print)) but ignores find's exit status, allowing permission/read errors
to be masked as "No ESLint suppression directives found"; change the logic to
capture both output and find's exit code (e.g., run find and then check $? or
use a conditional assignment like if ! scan_files=$$(find ...); then propagate
the non-zero exit code with exit $$? ), and only treat an empty scan_files as a
successful "no results" case when find itself succeeded; reference the
variables/command: scan_files, scan_paths, prune_expr, and the find invocation
to locate where to add the exit-status check and propagate failures.
In `@src/modules/user/features/auth/stores/index.ts`:
- Line 1: Update the two relative imports in this auth feature barrel to use the
auth/* alias instead of relative paths: replace the import of AuthError (and the
other import on line 3) from '../types/auth-error' (and its sibling) with the
corresponding aliased paths like 'auth/types/auth-error' (and the other as
'auth/...') so cross-directory auth feature imports use the auth/* alias; keep
the imported symbol names (e.g., AuthError) unchanged.
- Around line 37-48: The catch in DeferredAuthActions.resolveSafely swallows
errors; update the catch to log the caught error (include message/stack) before
clearing DeferredAuthActions.instance and calling
onFailure(DeferredAuthActions.loadFailure). Use the project's logger if
available (or console.error as fallback), and include identifying context like
"DeferredAuthActions.load failed" and the error object so network/chunk/DI
failures are visible in monitoring.
In `@src/modules/user/features/auth/stores/reactive-var-state.ts`:
- Line 1: The import in reactive-var-state.ts uses a relative path; change the
import of the ReactiveVarListener type to use the auth/* alias (e.g. import
ReactiveVarListener from 'auth/types/reactive-var') so cross-directory imports
within the auth feature follow the coding guideline; update the import statement
that references ReactiveVarListener accordingly.
- Around line 26-27: Wrap each listener invocation in a try-catch so one failing
subscriber doesn't stop others: change the two notification loops (the calls to
notified.forEach((listener) => listener(value)) and
this.always.forEach((listener) => listener())) to call each listener inside a
try { listener(...) } catch (err) { console.error('ReactiveVar listener error',
{ listener, err, value }); } (or use the module's logger if available) so
exceptions are logged and subsequent listeners still run.
---
Nitpick comments:
In `@src/modules/user/features/auth/stores/auth-var.ts`:
- Line 5: Replace the relative cross-directory import in auth-var.ts with the
auth feature alias: change the import of ReactiveVar from
"../types/reactive-var" to use the `@auth` path (e.g., import type { ReactiveVar }
from '`@auth/types/reactive-var`') so the module uses the standardized alias for
cross-directory imports within the auth feature.
In `@src/modules/user/features/auth/stores/reactive-var.ts`:
- Line 1: Replace the cross-directory relative import in reactive-var.ts with
the auth feature alias: update the import of the types ReactiveVar and
ReactiveVarListener (symbols: ReactiveVar, ReactiveVarListener) to use the
`@auth/` path (e.g. `@auth/types/reactive-var`) so the file imports via the feature
alias instead of ../types/reactive-var.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9f140dd-e6c6-457e-874a-2ed65ca98822
📒 Files selected for processing (20)
.claude/skills/AI-AGENT-GUIDE.md.claude/skills/SKILL-DECISION-GUIDE.mdMakefilelighthouse/lighthouserc.mobile.jsspecs/eslint-suppressions/implementation-artifacts/1-3-preserve-pass-fail-and-scan-error-exit-semantics.mdspecs/eslint-suppressions/implementation-artifacts/2-2-clean-up-tooling-suppressions-from-the-recorded-inventory.mdspecs/eslint-suppressions/implementation-artifacts/2-5-record-after-cleanup-inventory-and-baseline-decision.mdspecs/eslint-suppressions/implementation-artifacts/3-1-document-standalone-workflow-placement.mdspecs/eslint-suppressions/implementation-artifacts/3-2-publish-suppression-baseline-and-enforcement-decision.mdsrc/modules/user/features/auth/components/protected-route/index.tsxsrc/modules/user/features/auth/stores/auth-var.tssrc/modules/user/features/auth/stores/index.tssrc/modules/user/features/auth/stores/reactive-var-state.tssrc/modules/user/features/auth/stores/reactive-var.tssrc/modules/user/features/auth/types/reactive-var.tstests/integration/modules/user/features/auth/stores/deferred-auth-actions.integration.test.tstests/integration/modules/user/features/auth/stores/reactive-var.integration.test.tstests/unit/lighthouse/lighthouserc.mobile.test.tstests/unit/modules/user/features/auth/stores/deferred-auth-actions.test.tstests/unit/modules/user/features/auth/stores/reactive-var.test.ts
💤 Files with no reviewable changes (2)
- specs/eslint-suppressions/implementation-artifacts/2-5-record-after-cleanup-inventory-and-baseline-decision.md
- specs/eslint-suppressions/implementation-artifacts/1-3-preserve-pass-fail-and-scan-error-exit-semantics.md
✅ Files skipped from review due to trivial changes (5)
- src/modules/user/features/auth/components/protected-route/index.tsx
- tests/unit/lighthouse/lighthouserc.mobile.test.ts
- specs/eslint-suppressions/implementation-artifacts/3-1-document-standalone-workflow-placement.md
- specs/eslint-suppressions/implementation-artifacts/2-2-clean-up-tooling-suppressions-from-the-recorded-inventory.md
- specs/eslint-suppressions/implementation-artifacts/3-2-publish-suppression-baseline-and-enforcement-decision.md
…akefile find status)
There was a problem hiding this comment.
1 issue found across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Kravalg
requested changes
Jun 12, 2026
Kravalg
left a comment
Kravalg
left a comment
Contributor
There was a problem hiding this comment.
We should add separated CI check for this new make command
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
2 participants
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.
Summary
make lint-eslint-suppressionstarget that scanssrc tests scripts eslint.config.mjsfor the four ESLint suppression directive variants, prints grep-stylefile:line:text, and exits non-zero while any exist.make lintor CI — and documents that placement plus future-wiring guidance.lintplacement invariant, and publishes a zero baseline + enforcement decision underspecs/eslint-suppressions/.Changes
Makefile target
lint-eslint-suppressionstarget plusESLINT_SUPPRESSION_PATTERN,ESLINT_SUPPRESSION_SCAN_PATHS(src tests scripts eslint.config.mjs), andESLINT_SUPPRESSION_GREP_ARGSvariables..git,node_modules,dist,coverage,test-results,playwright-report,storybook-static,out,specs,docs; scan paths are overridable from the make invocation.lintif a future baseline decision changes that.Suppression removals
src/services/https-client/http-error-response-parser.ts: dropped// eslint-disable-next-line no-consoleby switchingconsole.debug→console.warn(allowed byno-console); public contract, message, and payload unchanged.tests/unit/modules/user/features/auth/components/form-section.test.tsx: dropped/* eslint-disable testing-library/prefer-screen-queries */by migrating all queries from the destructuredrender()result to the globalscreen.*API; all 10 assertions equivalent and green.Config
eslint.config.mjs: removed theeslint-comments/no-useallowlist, tightening the rule to'eslint-comments/no-use': 'error'so any future directive is flagged at lint time.Tests
tests/bats/eslint_suppressions.bats: covers Makefile policy placement, standalone/future-wiring docs, baseline artifact, default-scope success/report behavior, per-variant reporting, vendor/build/report exclusions, scan-path overrides, fixture-driven pass/fail exit codes, and non-match grep-error handling.console.warnfor the parser.tests/bats/make-target-coverage.tsv.Docs / specs
specs/eslint-suppressions/implementation-artifacts/eslint-suppressions-baseline.md: before/after inventory (3 → 2 → 1 → 0), tooling/source/test cleanup notes, the zero baseline decision, and the finalized standalone enforcement decision.specs/eslint-suppressions/; addedspecs/README.md; minorCLAUDE.mdcleanup.Test Plan
Verified locally on this branch (all green):
make lint— passes (lint-eslint lint-tsc lint-md lint-deps lint-metrics; onlywarn-level*ByTestIdnotices, no errors)make test— 112 + 5 suites, 1305 + 118 tests passedmake test-integration— 29 suites / 365 tests passedmake lint-eslint-suppressions— reports zero suppressions, exits 0Related
Closes #96 (created for this work)
Implements the spec originally tracked in #53.
Summary by cubic
Adds a standalone
make lint-eslint-suppressionstarget with a zero‑suppression baseline, and defers@apollo/client+ DI off the auth critical path via a lightweight reactive var; mobile Lighthouse min score is now 0.85. The suppression check stays standalone (not inmake lintor CI).New Features
lint-eslint-suppressionsscanssrc tests scripts eslint.config.mjsfor the four directive variants, printsfile:line:text, exits non‑zero on matches or scan errors, supports path overrides, ignores vendor/build dirs, and uses portablefind+grep(BusyBox‑friendly).ReactiveVarreplacesmakeVarso auth state and hooks no longer bundle@apollo/client; the DI graph loads on first auth action, with safe notification guards and same‑ref write skips. Tests cover exit semantics, deferred auth flows, and throwing‑listener paths.Refactors
console.warn, migrated tests toscreen.*, and set'eslint-comments/no-use': 'error'(allow list removed). Updated@auth/*imports;ProtectedRoutenow uses@auth/stores/use-auth-token. Neutralized the auth‑load failure message and hardened reactive‑var listener tests.Written for commit a9e7de5. Summary will update on new commits.