-
Notifications
You must be signed in to change notification settings - Fork 0
155 game of life llm feedback #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Set default seed value to 42 for consistency - Enhance prompt instructions for clarity - Improve content extraction logic from text - Ensure generation number is displayed and incremented
|
Automatic code review is enabled for this repository. Reviewing this PR now. |
|
I'm getting to work. See my steps and track my cost here.
I always see new activity while actively working. When paused, I'll only respond when tagged unless this box is checked. |
|
🚀 PR Preview deployed! Preview URL: https://pollinations.github.io/hive/pr-156/ |
1 similar comment
|
🚀 PR Preview deployed! Preview URL: https://pollinations.github.io/hive/pr-156/ |
Testing .mentatTesting ai-chatTesting static HTML app: ai-chat Testing graphics-editorTesting static HTML app: graphics-editor Testing llm-feedbackTesting static HTML app: llm-feedback Testing millionaire-gameTesting static HTML app: millionaire-game Testing placeholder-generatorTesting Node.js app: placeholder-generator Installing dependenciesℹ️ No tests found BuildingTesting pollinations-image-showTesting Node.js app: pollinations-image-show Installing dependenciesℹ️ No tests found BuildingTesting prompt-guessing-gameTesting Node.js app: prompt-guessing-game Installing dependenciesℹ️ No tests found BuildingTesting svg-feedbackTesting static HTML app: svg-feedback ❌ HTML tests failed for svg-feedback Testing tarot-readerTesting Node.js app: tarot-reader Installing dependenciesℹ️ No tests found Building |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the Game of Life integration changes. There's one important bug fix needed in the text extraction logic - the order of extraction should be reversed to properly handle code blocks before tags. I've also suggested two improvements for better code organization and maintainability: using constants for default values and implementing a more structured approach for model-specific message handling. The changes look good overall and can be merged after addressing these comments, particularly the text extraction fix.
| const match = text.match(/```([\s\S]*?)```/); | ||
| return match ? match[1] : text; | ||
|
|
||
| // Then check for content before </think> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of extraction should be reversed. Currently, we check for </think> first and then code blocks, but if there's a code block before </think>, we'll miss it. We should check for code blocks first, then fall back to </think> extraction.
Here's how the function should be structured:
function extractDisplayContent(text) {
// First try to extract content from code blocks
const codeMatch = text.match(/```([\s\S]*?)```/);
if (codeMatch)
return codeMatch[1];
// Then check for content before </think>
const thinkMatch = text.split('</think>');
if (thinkMatch.length > 1)
text = thinkMatch[1].trim();
// If neither pattern matches, return the original text
return text;
}| elements.start.textContent = 'Generating...'; | ||
|
|
||
| let seed = elements.seed.value && elements.seed.value !== '-1' ? | ||
| parseInt(elements.seed.value) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default seed handling could be improved. Instead of hardcoding 42, consider defining it as a constant at the top of the script for better maintainability. Also, the condition could be simplified.
Add this at the top of the script with other constants:
const DEFAULT_SEED = 42;Then simplify the seed assignment to:
let seed = elements.seed.value ? parseInt(elements.seed.value) : DEFAULT_SEED;| const messages = [ | ||
| { | ||
| const model = getSelectedModel(); | ||
| const messages = model === 'deepseek-r1' ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The model-specific message structure could be better organized using a configuration object. This would make it easier to add support for more models in the future.
Consider refactoring to use a model configuration object:
const modelConfig = {
'deepseek-r1': {
formatMessage: (instructions, state) => [{
role: "user",
content: instructions + "\n\n# Current State\n\n" + state
}]
},
'default': {
formatMessage: (instructions, state) => [{
role: "system",
content: instructions
}, {
role: "user",
content: state
}]
}
};
// Then use it like this:
const messages = (modelConfig[model] || modelConfig.default)
.formatMessage(formatInstructions, currentState);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look well-structured and focused on adding Conway's Game of Life functionality to the llm-feedback app. I have a few observations:
-
The Game of Life preset is well-designed:
- Clear rules explanation in the prompt
- Good initial state visualization
- Appropriate temperature setting (0.1) for deterministic behavior
-
Good improvements to text handling:
- Added support for content after tags
- Maintained backwards compatibility with code block extraction
- Fallback to original text if no patterns match
-
Nice optimization of the seed handling:
- Changed from random to fixed default (42)
- Still maintains ability to set custom seeds
There is a test failure in the svg-feedback app, but this is unrelated to your changes as it's a missing styles.css file in that app. This should be addressed separately.
Overall, this looks like a good addition to the llm-feedback app. The changes are focused and well-implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed the changes and agree with devloai[bot]'s assessment. The three issues identified (text extraction order, default seed constant, and model configuration structure) should be addressed before merging.
The rest of the changes look good, particularly:
- The Game of Life preset is well-designed with clear rules and visualization
- The UI changes are clean and maintain consistency with other presets
- The temperature setting of 0.1 is appropriate for the deterministic nature of Game of Life
Once you've addressed the code organization and bug fix suggestions from devloai[bot], this will be ready to merge.
- Introduce `.state-block` class for styling state displays - Add iteration labels above each state in the history - Ensure each state is wrapped in code blocks for clarity - Improve UI by grouping iteration labels with state blocks
|
🚀 PR Preview deployed! Preview URL: https://pollinations.github.io/hive/pr-156/ |
1 similar comment
|
🚀 PR Preview deployed! Preview URL: https://pollinations.github.io/hive/pr-156/ |
Testing .mentatTesting ai-chatTesting static HTML app: ai-chat Testing graphics-editorTesting static HTML app: graphics-editor Testing llm-feedbackTesting static HTML app: llm-feedback Testing millionaire-gameTesting static HTML app: millionaire-game Testing placeholder-generatorTesting Node.js app: placeholder-generator Installing dependenciesℹ️ No tests found BuildingTesting pollinations-image-showTesting Node.js app: pollinations-image-show Installing dependenciesℹ️ No tests found BuildingTesting prompt-guessing-gameTesting Node.js app: prompt-guessing-game Installing dependenciesℹ️ No tests found BuildingTesting svg-feedbackTesting static HTML app: svg-feedback ❌ HTML tests failed for svg-feedback Testing tarot-readerTesting Node.js app: tarot-reader Installing dependenciesℹ️ No tests found Building |
Testing .mentatTesting ai-chatTesting static HTML app: ai-chat Testing graphics-editorTesting static HTML app: graphics-editor Testing llm-feedbackTesting static HTML app: llm-feedback Testing millionaire-gameTesting static HTML app: millionaire-game Testing placeholder-generatorTesting Node.js app: placeholder-generator Installing dependenciesℹ️ No tests found BuildingTesting pollinations-image-showTesting Node.js app: pollinations-image-show Installing dependenciesℹ️ No tests found BuildingTesting prompt-guessing-gameTesting Node.js app: prompt-guessing-game Installing dependenciesℹ️ No tests found BuildingTesting svg-feedbackTesting static HTML app: svg-feedback ❌ HTML tests failed for svg-feedback Testing tarot-readerTesting Node.js app: tarot-reader Installing dependenciesℹ️ No tests found Building |
|
🚀 PR Preview deployed! Preview URL: https://pollinations.github.io/hive/pr-156/ |
1 similar comment
|
🚀 PR Preview deployed! Preview URL: https://pollinations.github.io/hive/pr-156/ |
No description provided.