fix(FAGERSTRÖM): correct option ordering, add smoker gate, bump edition - #18
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Fagerstrom Nicotine Dependence form updates scoring keys for several radio questions, revises the cigarette amount labels, and changes score computation and validation to match the new negative-value scheme. ChangesFagerstrom Nicotine Dependence Form Scoring Revision
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
What's holding this up? |
|
@gdevenyi we use a hacky way of reordering the question options by making the scale negative instead (1 -> -1, 2 -> -2, ...). If this is ok then the pr is good to go. |
|
Why is this needed? Can you explain the underlying issue? |
|
Anne requests that the ordering of the questions options to be changed from lowest to highest score value to highest to lowest score value as seen in issue https://github.com/DouglasNeuroInformatics/CPP/issues/90. The design of ODC form does not allow for this ordering to be possible as it always sorts the options by numerical value, so the current makeshift solution is to make the values negative. A possible solution for this in the future is maybe a |
|
another workaround that could be done is inversing the scale values like done in this form here however this could be confusing comparing it the official form and its scoring method. |
|
I think its a bit silly to auto-sort these things when the display is based on this. @joshunrau can we look into adding the sort control feature and defaulting it to off? |
|
This is not a design in ODC sorting. In the ECMAScript spec, non-negative integer object keys are automatically sorted when iterating over them. An alternative here could be to use something like "1.0".
1. Integer indices, ascending numeric order 2. String keys, in insertion order 3. Symbol keys are NOT included Exampleconst obj = {};
obj["b"] = 1;
obj["2"] = 2;
obj["a"] = 3;
obj["0"] = 4;
obj["1"] = 5;
Object.keys(obj); // ["0", "1", "2", "b", "a"]Integer indices sort numerically first ( Important nuances
The practical implication: if you're using an object as a map with mixed numeric-string keys, your iteration order may surprise you if you added string keys before numeric ones. |
There was a problem hiding this comment.
Pull request overview
Updates the Fagerström Nicotine Dependence (FTND) instrument definition to address option ordering issues by changing option value encodings and adjusting the computed total score logic accordingly.
Changes:
- Updated several radio option value keys (including introducing negative values) to correct ordering behavior.
- Adjusted total score computation to account for negative-coded answers.
- Updated validation schema bounds to match the new value ranges and tweaked one English question label.
Comments suppressed due to low confidence (5)
lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts:101
- The English label has an extra "a" ("How many cigarettes a do you smoke…"), which reads as a typo in the user-facing question text.
lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts:115 - The French option text "31 ou plus 3" appears to have an extraneous trailing "3" in the user-facing label.
lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts:150 - The total score calculation currently special-cases
cigaretteAmountand multiplies the remaining answers by -1. Since the schema now constrains the other answers to be <= 0, computing the score as the sum of absolute values is simpler and avoids depending on a specific field name.
lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts:150 - This computed measure is named
auditCScore, which appears to be a copy/paste artifact from the AUDIT-C instrument and is misleading in the Fagerström instrument. Consider renaming it to something FTND-specific (or adding a new correctly-named measure while keepingauditCScoreas a backwards-compatible alias if consumers rely on it).
lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts:62 - This French option label has a trailing space, which can show up in UI rendering and makes the string inconsistent with the other options.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts (1)
142-149: ⚡ Quick winMeasure name
auditCScoreis misleading.This is the Fagerström Nicotine Dependence test, not AUDIT-C. Consider renaming to
ftndScoreortotalScorefor clarity.Proposed fix
measures: { - auditCScore: { + ftndScore: { kind: 'computed', label: { en: 'Total Score:', fr: 'Score total:' }, value: (data) => sum(Object.values(data).map((v) => Math.abs(v ?? 0))) } },🤖 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 `@lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts` around lines 142 - 149, Rename the misleading computed field auditCScore to a clear name like ftndScore (or totalScore) in the form definition: change the property key auditCScore to ftndScore (and keep its kind, label, and value implementation sum(Object.values(data).map((v) => Math.abs(v ?? 0)))); then update all references/usages, imports, exports and any tests or type interfaces that expect auditCScore to use ftndScore instead so callers read the new property name consistently (ensure no runtime references remain to auditCScore).
🤖 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 `@lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts`:
- Line 115: The French label for option key '3' in the
FAGERSTRÖM_NICOTINE_DEPENDENCE options map contains a stray "3" ("31 ou plus
3"); locate the options object in
lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts and change the value for key
'3' from "31 ou plus 3" to "31 ou plus" so the label reads correctly.
- Line 101: Fix the typo in the FAGERSTRÖM_NICOTINE_DEPENDENCE question text:
replace the string "4. How many cigarettes a do you smoke per day?" (the en
value for that question) with "4. How many cigarettes do you smoke per day?" so
the English prompt reads correctly.
---
Nitpick comments:
In `@lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts`:
- Around line 142-149: Rename the misleading computed field auditCScore to a
clear name like ftndScore (or totalScore) in the form definition: change the
property key auditCScore to ftndScore (and keep its kind, label, and value
implementation sum(Object.values(data).map((v) => Math.abs(v ?? 0)))); then
update all references/usages, imports, exports and any tests or type interfaces
that expect auditCScore to use ftndScore instead so callers read the new
property name consistently (ensure no runtime references remain to auditCScore).
🪄 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: f3f15aea-a056-4264-8e77-264f165eecec
📒 Files selected for processing (1)
lib/forms/FAGERSTRÖM_NICOTINE_DEPENDENCE/index.ts
|
will also close issue https://github.com/DouglasNeuroInformatics/CPP/issues/115 |
gdevenyi
left a comment
gdevenyi
left a comment
There was a problem hiding this comment.
Comprehensive review — typos, auditCScore rename, and earlier CodeRabbit/Copilot findings are already addressed, so focusing on what's still open.
Blocking (address before merge)
- Edition bump (line 34, unchanged context) — schema-shape change breaks compat with edition-1 records: new required
smokesfield, all 6 scoring fields now.optional()with negative value ranges, measure renamedauditCScore→nicotineDependenceScore. Any record captured against edition 1 (positive 0–3 values, nosmokes) will fail validation or mis-score under the new schema. Bumpinternal.editionto 2 (CLAUDE.md §1.1,ScalarInstrumentInternal). - Validation brittleness (line 205) — magic
=== 6, root-level error. UsesuperRefine+ per-fieldpath. - Helper typing (line 20) —
render'sdataparam should bePartialData<TData>, not{ smokes?: unknown }. - Non-smoker score ambiguity (line 181) — return
undefined, not0, to distinguish N/A from zero dependence.
Non-blocking
smokesgate deviates from canonical FTND — needs explicit sign-off and a note indescription.Math.absscoring works by coincidence — document the invariant or dropabs.- Deprecated
details.estimatedDuration/instructions(lines 45–49, unchanged context) — migrate toclientDetails.estimatedDuration/clientDetails.instructions. PR is already rewriting this region. - Negative-key hack — track for cleanup once sort-control lands, or use
joshunrau's"1.0"string-key idea. - Missing
referenceUrl; verify license string (line 50, unchanged context) — FTND has a canonical citation (Heatherton et al. 1991, Br J Addict 86:1116–26). Addingdetails.referenceUrlis in scope for a scoring-correctness PR. Also confirmPUBLIC-DOMAINis an accepted SPDX value in the ODC approved-license list — closest SPDX strings arePublicDomain(deprecated) or aLicenseRef-.
Process
- Title "Apply fagerstrom form suggestions" is vague — suggest
fix(FAGERSTRÖM): correct option ordering, add smoker gate, bump edition. - 7 commits with merge commits + "fix typo" noise — squash-merge.
Lint passes. Details in inline comments (blocking items 2–4 and non-blocking 5, 6, 8 are anchored below; items 1, 7, 9 sit on unchanged lines so are listed here only).
| function createDependentField<const T>(field: T) { | ||
| return { | ||
| kind: 'dynamic' as const, | ||
| deps: ['smokes'] as const, | ||
| render: (data: { smokes?: unknown }) => { | ||
| if (data.smokes === true) { | ||
| return field; | ||
| } | ||
| return null; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛑 Helper typing defeats the type system.
render: (data: { smokes?: unknown }) => ... should be PartialData<TData>. As written:
- no inference of the full form shape
- typos in dependency keys won't be caught
field: Tis unconstrained — should beT extends StaticField<...>
Import the runtime-core dynamic-field / PartialData types and let TData flow through defineInstrument.
| .refine(({ smokes, ...data }) => { | ||
| if (!smokes) { | ||
| return true; | ||
| } | ||
| return Object.values(data).length === 6 && Object.values(data).every((arg) => typeof arg === 'number'); | ||
| }, 'Error: Please fill out all the questions / Erreur: Veuillez répondre à toutes les questions') |
There was a problem hiding this comment.
🛑 .refine is brittle and emits a single root-level error.
Object.values(data).length === 6 // magic numberIf a 7th scoring field is added later, this passes silently for incomplete submissions (length never equals 6). Derive the expected keys from the schema, or enumerate them explicitly.
Also, .refine produces one path-less error — the user sees a generic banner instead of per-field "required" markers. Use .superRefine with ctx.addIssue({ path: [fieldName], ... }) so each missing field highlights individually.
| value: (data) => { | ||
| return sum(Object.values(data)); | ||
| if (!data.smokes) { | ||
| return 0; | ||
| } | ||
| return sum( | ||
| Object.values(data) | ||
| .filter((v): v is number => typeof v === 'number') | ||
| .map((v) => Math.abs(v)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛑 Non-smoker returns 0 — conflates "N/A" with "zero dependence".
A real FTND total of 0 means "low-dependence smoker." Returning 0 for non-smokers is ambiguous. InstrumentMeasureValue permits undefined — return that so downstream consumers can distinguish:
if (!data.smokes) return undefined;(Also a nit on this block: Math.abs at line 186 is coincidentally correct — it works only because every negated field scores abs(key) AND cigaretteAmount happens to already score 0..3 directly. If a future field has positive keys but inverse scoring, abs silently breaks. Either drop abs and store keys at their true score values, or document the invariant inline.)
| smokes: { | ||
| kind: 'boolean', | ||
| label: { | ||
| en: 'Do you smoke?', | ||
| fr: 'Fumez-vous?' | ||
| }, | ||
| options: { | ||
| en: { | ||
| true: 'Yes', | ||
| false: 'No' | ||
| }, | ||
| fr: { | ||
| true: 'Oui', | ||
| false: 'Non' | ||
| } | ||
| }, | ||
| variant: 'radio' | ||
| }, |
There was a problem hiding this comment.
smokes gate deviates from canonical FTND.
Standard FTND is 6 items with no gate. Per CLAUDE.md guideline #2 (faithful source implementation), adding a 7th gating question is a protocol change. If accepted (CPP #115), mention it in details.description so clinicians know this isn't vanilla FTND, and confirm explicit maintainer sign-off.
| options: { | ||
| en: { | ||
| 3: 'Within 5 minutes', | ||
| 2: '6-30 minutes', | ||
| 1: '31-60 minutes', | ||
| 0: 'More than 60 minutes' | ||
| '-3': 'Within 5 minutes', | ||
| '-2': '6-30 minutes', | ||
| '-1': '31-60 minutes', | ||
| '0': 'More than 60 minutes' | ||
| }, | ||
| fr: { | ||
| 3: 'Dans les 5 minutes', | ||
| 2: '6 à 30 minutes', | ||
| 1: '31 à 60 minutes ', | ||
| 0: 'Plus de 60 minutes' | ||
| '-3': 'Dans les 5 minutes', | ||
| '-2': '6 à 30 minutes', | ||
| '-1': '31 à 60 minutes', | ||
| '0': 'Plus de 60 minutes' | ||
| } | ||
| }, |
There was a problem hiding this comment.
ℹ️ Negative-key hack pollutes the schema.
(Already discussed at length above — flagging on the code for traceability.) The negation forces odd validation bounds (min(-3).max(0) at line 194) and disableAutoPrefix: true everywhere (otherwise users see "-3" prefixes). Two cleaner paths:
joshunrau's"1.0"string-key idea — keeps scores readable, avoids negative bounds- wait for the platform-level sort-control feature
gdevenyiproposed and revert to clean integer keys
Acceptable as a stop-gap, but please track for cleanup.
1dd0c20 to
27f5899
Compare
gdevenyi
changed the title
|
Heads up @david-roper — #38 also edits The substantive changes don't overlap (this PR: option ordering, scoring, validation; #38: No preference on ordering from my side; #38 can rebase onto this and take edition 3 if that's easier. Flagging so it isn't missed. |
|
Following up on my note above — we've settled the order. #38 keeps |
|
@CMonnin I'll merge this and you can do a followup PR |
Change the scale and total score calculation to fix order of options within multiple questions.
Questions appear as negative values, get converted to numbers and multiplied by -1
https://github.com/DouglasNeuroInformatics/CPP/issues/90
form can be viewed here
Summary by CodeRabbit
Release Notes